diff --git a/CHANGELOG.md b/CHANGELOG.md index ae6482ac..355cf140 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,9 @@ ## Unreleased +### Changed +- Failed HTTP requests are now retried before raising errors (#235) + ## [2.1.2] - 2020-02-24 ### Fixed diff --git a/R/io.R b/R/io.R index c8ec196e..7a27b951 100644 --- a/R/io.R +++ b/R/io.R @@ -258,7 +258,7 @@ write_civis.character <- function(x, tablename, database = NULL, if_exists = "fa credential_id = credential_id), import_args) job_r <- do.call(start_import_job, args) - put_r <- httr::PUT(job_r[["uploadUri"]], body = httr::upload_file(x)) + put_r <- httr::RETRY("PUT", job_r[["uploadUri"]], body = httr::upload_file(x), terminate_on = setdiff(400:499, 429)) if (put_r$status_code != 200) { msg <- httr::content(put_r) stop(msg) @@ -562,9 +562,11 @@ download_civis.numeric <- function(x, file, } args <- c(list(files_get(x)$fileUrl), - list(httr::write_disk(file, overwrite = overwrite))) + list(httr::write_disk(file, overwrite = overwrite)), + verb = "GET", + terminate_on = setdiff(400:499, 429)) if (progress) args <- c(args, list(httr::progress())) - resp <- do.call(httr::GET, args) + resp <- do.call(httr::RETRY, args) httr::stop_for_status(resp, task = "download file from S3") invisible(file) @@ -902,14 +904,14 @@ download_script_results <- function(script_id, run_id, stop_if_no_output(script_results) url <- script_results[["output"]][[1]][["path"]] - args <- list(url) + args <- list(url, verb = "GET", terminate_on = setdiff(400:499, 429)) if (!is.null(filename)) { args <- c(args, list(httr::write_disk(filename, overwrite = TRUE))) } if (progress) { args <- c(args, list(httr::progress())) } - r <- do.call(httr::GET, args) + r <- do.call(httr::RETRY, args) httr::stop_for_status(r) r } diff --git a/tests/testthat/test_client_base.R b/tests/testthat/test_client_base.R index d6ca32f1..ca10d089 100644 --- a/tests/testthat/test_client_base.R +++ b/tests/testthat/test_client_base.R @@ -35,7 +35,6 @@ body_params <- NULL test_that("call api extracts json content correctly", { response <- with_mock( `civis::api_key` = function(...) "fake_key", - `httr::VERB` = function(...) httr_200, `httr::RETRY` = function(...) httr_200, call_api("GET", path, path_params, query_params, body_params)) expect_equal(response[[1]]$name, "Leanne Graham") @@ -45,7 +44,6 @@ test_that("call api extracts json content correctly", { test_that("print and str methods for civis_api", { response <- with_mock( `civis::api_key` = function(...) "fake_key", - `httr::VERB` = function(...) httr_200, `httr::RETRY` = function(...) httr_200, call_api("GET", path, path_params, query_params, body_params)) @@ -62,7 +60,6 @@ test_that("print and str methods for civis_api", { test_that("call api catches http error", { with_mock( `civis::api_key` = function(...) "fake_key", - `httr::VERB` = function(...) httr_504, `httr::RETRY` = function(...) httr_504, expect_error( call_api("GET", path, path_params, query_params, body_params), diff --git a/tests/testthat/test_io.R b/tests/testthat/test_io.R index 8001a9bf..97cbd7b6 100644 --- a/tests/testthat/test_io.R +++ b/tests/testthat/test_io.R @@ -48,7 +48,7 @@ test_that("read_civis.numeric reads a csv", { } with_mock( `civis::files_get` = function(...) list(fileUrl = "fakeurl.com"), - `httr::GET` = mock_response, + `httr::RETRY` = mock_response, `civis::download_civis` = function(id, fn) write.csv(d, file = fn), expect_equal(d, read_civis(123, using = read.csv, row.names = 1)) ) @@ -101,7 +101,7 @@ test_that("write_civis.character returns meta data if successful", { }, `civis::default_credential` = function(...) 1234, `civis::tables_post_refresh` = function(id) "", - `httr::PUT` = function(...) list(status_code = 200), + `httr::RETRY` = function(...) list(status_code = 200), `civis::imports_post_files_runs` = function(...) list(""), `civis::imports_get_files_runs` = function(...) list(state = "succeeded"), write_civis("mockfile", "mock.table", "mockdb") @@ -116,7 +116,7 @@ test_that("write_civis.character fails if file doesn't exist", { `civis::start_import_job` = function(...) { list(uploadUri = "fake") }, - `httr::PUT` = function(...) list(status_code = 200), + `httr::RETRY` = function(...) list(status_code = 200), `civis::imports_post_files_runs` = function(...) list(""), `civis::imports_get_files_runs` = function(...) list(state = "succeeded"), tryCatch(write_civis("mockfile", "mock.table", "mockdb"), error = function(e) e$message) @@ -133,7 +133,7 @@ test_that("write_civis.data.frame returns meta data if successful", { }, `civis::default_credential` = function(...) 1, `civis::tables_post_refresh` = function(id) "", - `httr::PUT` = function(...) list(status_code = 200), + `httr::RETRY` = function(...) list(status_code = 200), `civis::imports_post_files_runs` = function(...) list(""), `civis::imports_get_files_runs` = function(...) list(state = "succeeded"), `civis::tables_list` = function(...) 1, @@ -148,7 +148,7 @@ test_that("write_civis.character warns under failure", { `civis::start_import_job` = function(...) { list(uploadUri = "fake", id = -999) }, - `httr::PUT` = function(...) list(status_code = 200), + `httr::RETRY` = function(...) list(status_code = 200), `civis::imports_post_files_runs` = function(...) "", `civis::imports_get_files_runs` = function(...) list(state = "failed"), `httr::content` = function(...) "error", @@ -167,7 +167,7 @@ test_that("write_civis.character calls imports endpoints correctly", { `civis::imports_post_files` = ipf, `civis::imports_post_files_runs` = function(...) list(id = 44), `civis::imports_get_files_runs` = function(...) list(state = "succeeded"), - `httr::PUT` = mock(list(status_code = 200)), + `httr::RETRY` = mock(list(status_code = 200)), res <- write_civis(fn, "mock.table", "mockdb", credential_id = 1), expect_equal(get_status(res), "succeeded"), expect_args(ipf, n = 1,