From 7346f3e59e13c909f1b7a2c317cdc34d7a21716f Mon Sep 17 00:00:00 2001 From: Scott Chamberlain Date: Mon, 4 Mar 2024 15:04:37 -0800 Subject: [PATCH] #33 added error handling for http requests to expose actual cromwell server error messages, bump version --- DESCRIPTION | 6 ++-- NAMESPACE | 4 +++ R/cromwellSubmitBatch.R | 4 +-- R/http.R | 37 +++++++++++++++++++---- tests/testthat/helper-stubs.R | 3 ++ tests/testthat/test-error-handling.R | 45 ++++++++++++++++++++++++++++ 6 files changed, 89 insertions(+), 10 deletions(-) create mode 100644 tests/testthat/helper-stubs.R create mode 100644 tests/testthat/test-error-handling.R diff --git a/DESCRIPTION b/DESCRIPTION index 9268502..f2536d2 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -1,6 +1,6 @@ Package: rcromwell Title: Convenience Tools for Managing WDL Workflows via Cromwell -Version: 3.2.1 +Version: 3.2.1.92 Authors@R: c( person("Amy", "Paguirigan", role = "aut", comment = c(ORCID = "0000-0002-6819-9736")), @@ -27,11 +27,13 @@ Roxygen: list(markdown = TRUE, roclets = c("collate", "namespace", "rd", RoxygenNote: 7.2.3 Encoding: UTF-8 Suggests: + curl, knitr, rmarkdown, roxyglobals, testthat (>= 3.0.0), - vcr (>= 0.6.0) + vcr (>= 0.6.0), + webmockr Remotes: ropensci/vcr Config/testthat/edition: 3 diff --git a/NAMESPACE b/NAMESPACE index 3e9548a..7c87ce1 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -24,6 +24,7 @@ importFrom(dplyr,tibble) importFrom(httr,GET) importFrom(httr,POST) importFrom(httr,content) +importFrom(httr,status_code) importFrom(httr,stop_for_status) importFrom(httr,upload_file) importFrom(jsonlite,fromJSON) @@ -37,6 +38,9 @@ importFrom(purrr,map) importFrom(purrr,map_dfr) importFrom(purrr,pluck) importFrom(purrr,reduce) +importFrom(rlang,abort) +importFrom(rlang,caller_env) +importFrom(rlang,has_name) importFrom(rlang,is_character) importFrom(rlang,is_logical) importFrom(rlang,set_names) diff --git a/R/cromwellSubmitBatch.R b/R/cromwellSubmitBatch.R index 072a7e8..d0993e2 100644 --- a/R/cromwellSubmitBatch.R +++ b/R/cromwellSubmitBatch.R @@ -50,7 +50,7 @@ cromwell_submit_batch <- } if (!is.null(options)) { body <- c(body, - workflowOptions = list(httr::upload_file(options)) + workflow_options = list(httr::upload_file(options)) ) } if (!is.null(labels)) { @@ -67,4 +67,4 @@ cromwell_submit_batch <- token = token ) dplyr::as_tibble(crom_dat) - } +} diff --git a/R/http.R b/R/http.R index bcd3d49..1052558 100644 --- a/R/http.R +++ b/R/http.R @@ -5,20 +5,45 @@ cw_url <- function() { Sys.getenv("CROMWELLURL") } +con_utf8 <- function(response, as = NULL, ...) { + httr::content(response, as = as, encoding = "utf-8", ...) +} + +#' @importFrom rlang has_name +#' @importFrom httr status_code +#' @importFrom rlang abort caller_env +handle_error <- function(response, call = caller_env()) { + status <- httr::status_code(response) + if (status >= 400) { + err <- con_utf8(response, as = "parsed") + if (rlang::has_name(err, "error")) { + mssg <- err$error + } else if (rlang::has_name(err, "message")) { + mssg <- err$message + } else { + httr::stop_for_status(response) + } + abort( + sprintf("(HTTP %s) - %s", as.character(status), mssg), + call = call + ) + } +} + #' @importFrom httr GET POST stop_for_status content upload_file #' @noRd #' @keywords internal #' @author Scott Chamberlain -http_get <- function(url, as = NULL, token = NULL, ...) { +http_get <- function(url, as = NULL, token = NULL, call = caller_env(), ...) { result <- httr::GET(url, try_auth_header(token), ...) - httr::stop_for_status(result) - httr::content(result, as = as) + handle_error(result, call) + con_utf8(result, as = as) } -http_post <- function(url, as = NULL, token = NULL, ...) { +http_post <- function(url, as = NULL, token = NULL, call = caller_env(), ...) { result <- httr::POST(url, try_auth_header(token), ...) - httr::stop_for_status(result) - httr::content(result, as = as) + handle_error(result, call) + con_utf8(result, as = as) } make_url <- function(base_url, ...) { diff --git a/tests/testthat/helper-stubs.R b/tests/testthat/helper-stubs.R new file mode 100644 index 0000000..b44efce --- /dev/null +++ b/tests/testthat/helper-stubs.R @@ -0,0 +1,3 @@ +response_proof_401 <- list( + error = "Unauthorized" +) diff --git a/tests/testthat/test-error-handling.R b/tests/testthat/test-error-handling.R new file mode 100644 index 0000000..d4bb42c --- /dev/null +++ b/tests/testthat/test-error-handling.R @@ -0,0 +1,45 @@ +# try_url <- function(url) { +# tryCatch(httr::GET(url), error = function(e) e) +# } + +# skip_if_offline("proof-api.fredhutch.org") + +test_that("proof api or DIY cromwell server down", { + # This should happen whether proof or DIY if not on + # campus or VPN for Fred Hutch at least + # and happens if someone puts in a bad url + + httr::set_callback("response", \(req, res) curl::nslookup("abcdefg")) + expect_error( + cromwell_submit_batch(wdl = file_hello, params = file_inputs), + "Unable to resolve host" + ) + httr::set_callback("response", NULL) +}) + +test_that("401 unauthorized for proof api", { + # This should only happen with proof api + # regular cromwell server + + webmockr::stub_registry_clear() + webmockr::stub_request("get", make_url(cw_url(), "engine/v1/version")) %>% + webmockr::to_return( + body = jsonlite::toJSON(response_proof_401, auto_unbox = TRUE), + status = 401L, + headers = list("Content-type" = "application/json") + ) + + unloadNamespace("vcr") + webmockr::enable(quiet = TRUE) + + expect_error( + cromwell_version(), + "Unauthorized" + ) + + webmockr::stub_registry_clear() + webmockr::disable(quiet = TRUE) +}) + +# reload vcr +attachNamespace("vcr")