diff --git a/DESCRIPTION b/DESCRIPTION index 624aa3b4..01000870 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -1,6 +1,6 @@ Package: orderly2 Title: Orderly Next Generation -Version: 1.99.27 +Version: 1.99.28 Authors@R: c(person("Rich", "FitzJohn", role = c("aut", "cre"), email = "rich.fitzjohn@gmail.com"), person("Robert", "Ashton", role = "aut"), @@ -28,7 +28,7 @@ Imports: Suggests: DBI, RSQLite, - httr, + httr2, jsonvalidate (>= 1.4.0), knitr, mockery, diff --git a/R/location_http.R b/R/location_http.R index 4d153396..66205a03 100644 --- a/R/location_http.R +++ b/R/location_http.R @@ -11,7 +11,7 @@ orderly_location_http <- R6::R6Class( }, list = function() { - dat <- private$client$get("/metadata/list", parse_json = TRUE)$data + dat <- private$client$request("/metadata/list")$data data_frame( packet = vcapply(dat, "[[", "packet"), time = num_to_time(vnapply(dat, "[[", "time")), @@ -21,8 +21,8 @@ orderly_location_http <- R6::R6Class( metadata = function(packet_ids) { ret <- vcapply(packet_ids, function(id) { tryCatch( - trimws(private$client$get(sprintf("/metadata/%s/text", id), - parse_json = FALSE)), + trimws(private$client$request(sprintf("/metadata/%s/text", id), + parse_json = FALSE)), outpack_http_client_error = function(e) { if (e$code == 404) { e$message <- sprintf("Some packet ids not found: '%s'", id) @@ -43,7 +43,7 @@ orderly_location_http <- R6::R6Class( ## progress in the client, but there's not much point until ## then. tryCatch( - private$client$get(sprintf("/file/%s", hash), download = dest), + private$client$request(sprintf("/file/%s", hash), download = dest), outpack_http_client_error = function(e) { if (e$code == 404) { unlink(dest) @@ -56,32 +56,36 @@ orderly_location_http <- R6::R6Class( ## TODO: we could get the schemas here from outpack_server too list_unknown_packets = function(ids) { - body <- to_json(list(ids = ids, unpacked = scalar(TRUE)), NULL) - content <- httr::content_type("application/json") - res <- private$client$post("/packets/missing", body, content) + res <- private$client$request("/packets/missing", function(r) { + httr2::req_body_json(r, list(ids = ids, unpacked = scalar(TRUE))) + }) list_to_character(res$data) }, list_unknown_files = function(hashes) { - body <- to_json(list(hashes = hashes), NULL) - res <- private$client$post("/files/missing", body, - httr::content_type("application/json")) + res <- private$client$request( + "/files/missing", + function(r) httr2::req_body_json(r, list(hashes = hashes))) list_to_character(res$data) }, push_file = function(src, hash) { - body <- httr::upload_file(src, "application/octet-stream") - res <- private$client$post(sprintf("/file/%s", hash), body) + res <- private$client$request( + sprintf("/file/%s", hash), + function(r) httr2::req_body_file(r, src, "application/octet-stream")) + invisible(NULL) }, push_metadata = function(packet_id, hash, path) { meta <- read_string(path) - res <- private$client$post(sprintf("/packet/%s", hash), meta, - httr::content_type("text/plain")) + res <- private$client$request( + sprintf("/packet/%s", hash), + function(r) httr2::req_body_raw(r, meta, "text/plain")) invisible(NULL) } - )) + ) +) orderly_location_packit <- function(url, token) { diff --git a/R/outpack_http_client.R b/R/outpack_http_client.R index f21ea84d..94a33c55 100644 --- a/R/outpack_http_client.R +++ b/R/outpack_http_client.R @@ -21,36 +21,37 @@ outpack_http_client <- R6::R6Class( } }, - get = function(path, ...) { + request = function(path, customize = identity, ...) { self$authorise() - http_client_request(httr::GET, paste0(self$url, path), ..., - self$auth$header) - }, - - post = function(path, body, ...) { - self$authorise() - http_client_request(httr::POST, paste0(self$url, path), body = body, ..., - self$auth$header) + http_client_request( + self$url, + function(r) { + r <- httr2::req_url_path_append(r, path) + r <- httr2::req_headers(r, !!!self$auth$header) + customize(r) + }, ...) } )) - -http_client_request <- function(verb, url, ..., parse_json = TRUE, - download = NULL) { - if (is.null(download)) { - response <- verb(url, ...) - } else { - response <- verb(url, ..., - http_client_download_options(download)) +http_client_request <- function(url, customize = identity, download = NULL, + parse_json = TRUE) { + req <- httr2::request(url) + if (!is.null(download)) { + req <- httr2::req_headers(req, Accept = "application/octet-stream") } - http_client_handle_error(response) + req <- customize(req) + resp <- tryCatch( + httr2::req_perform(req, path = download), + httr2_http = function(cnd) { + http_client_handle_error(cnd$resp) + }) + if (is.null(download)) { - txt <- httr::content(response, "text", encoding = "UTF-8") if (parse_json) { - from_json(txt) + httr2::resp_body_json(resp, simplifyVector = FALSE) } else { - txt + httr2::resp_body_string(resp) } } else { download @@ -63,17 +64,19 @@ http_client_handle_error <- function(response) { ## that an expired timeout produces a certain error code we watch ## for that and then reauthenticate; that requires that a callback ## is passed through here too. - code <- httr::status_code(response) - if (httr::http_error(code)) { - if (httr::http_type(response) == "application/json") { - txt <- httr::content(response, "text", encoding = "UTF-8") - res <- from_json(txt) + if (httr2::resp_is_error(response)) { + if (httr2::resp_content_type(response) == "application/json") { + res <- httr2::resp_body_json(response) ## I am seeing Packit returning an element 'error' not a list of ## errors errors <- if ("error" %in% names(res)) list(res$error) else res$errors - stop(http_client_error(errors[[1]]$detail, code, errors)) + stop(http_client_error(errors[[1]]$detail, + httr2::resp_status(response), + errors)) } else { - stop(http_client_error(httr::http_status(code)$message, code, NULL)) + stop(http_client_error(httr2::resp_status_desc(response), + httr2::resp_status(response), + NULL)) } } response @@ -87,12 +90,6 @@ http_client_error <- function(msg, code, errors) { } -http_client_download_options <- function(dest) { - c(httr::write_disk(dest), - httr::accept("application/octet-stream")) -} - - ## Logging in with packit is quite slow and we'll want to cache this; ## but we won't be holding a persistant handle to the root. So for ## now at least we'll keep a pool of generated bearer token headers, @@ -108,11 +105,12 @@ http_client_login <- function(name, auth) { key <- rlang::hash(auth) if (is.null(auth_cache[[key]])) { cli::cli_alert_info("Logging in to {name}") - res <- http_client_request(httr::POST, auth$url, - body = auth$data, encode = "json") + + res <- http_client_request(auth$url, + function(r) httr2::req_body_json(r, auth$data)) + cli::cli_alert_success("Logged in successfully") - auth_cache[[key]] <- httr::add_headers( - "Authorization" = paste("Bearer", res$token)) + auth_cache[[key]] <- list("Authorization" = paste("Bearer", res$token)) } auth_cache[[key]] } diff --git a/R/util.R b/R/util.R index 1da7e3bd..ee25909b 100644 --- a/R/util.R +++ b/R/util.R @@ -332,11 +332,6 @@ to_json <- function(x, schema = NULL, auto_unbox = FALSE, ...) { } -from_json <- function(x, ...) { - jsonlite::fromJSON(x, simplifyVector = FALSE, ...) -} - - as_json <- function(str) { assert_scalar_character(str) structure(str, class = "json") diff --git a/tests/testthat/helper-outpack-http.R b/tests/testthat/helper-outpack-http.R index ed01819f..46438b93 100644 --- a/tests/testthat/helper-outpack-http.R +++ b/tests/testthat/helper-outpack-http.R @@ -5,9 +5,8 @@ mock_headers <- function(...) { mock_response <- function(content, status = 200L, wrap = TRUE, download = NULL) { headers <- mock_headers() - if (!is.null(download)) { + if (is.raw(content)) { headers <- mock_headers("content-type" = "application/octet-stream") - writeBin(content, download) } else if (inherits(content, "json")) { headers <- mock_headers("content-type" = "application/json") if (wrap) { @@ -16,16 +15,20 @@ mock_response <- function(content, status = 200L, wrap = TRUE, } class(content) <- NULL content <- c(writeBin(content, raw()), as.raw(0L)) - } else if (inherits(content, "character")) { + } else if (is.character(content)) { headers <- mock_headers("content-type" = "text/plain") content <- c(writeBin(content, raw()), as.raw(0L)) } else { stop("Unhandled mock response type") } - structure(list(status_code = status, - headers = headers, - content = content), - class = "response") + + if (!is.null(download)) { + writeBin(content, download) + } + + httr2::response(status_code = status, + headers = headers, + body = content) } @@ -38,3 +41,9 @@ json_string <- function(s) { clear_auth_cache <- function() { rm(list = ls(auth_cache), envir = auth_cache) } + +local_mock_response <- function(..., env = rlang::caller_env()) { + mock <- mockery::mock(mock_response(...)) + httr2::local_mocked_responses(function(req) mock(req), env = env) + mock +} diff --git a/tests/testthat/helper-outpack-server.R b/tests/testthat/helper-outpack-server.R index dcbb35c9..03df09f0 100644 --- a/tests/testthat/helper-outpack-server.R +++ b/tests/testthat/helper-outpack-server.R @@ -10,7 +10,10 @@ outpack_server <- function(path, timeout = 10) { t_end <- Sys.time() + timeout success <- FALSE while (!success && px$is_alive() && Sys.time() < t_end) { - result <- tryCatch(httr::GET("http://localhost:8000"), error = identity) + r <- httr2::request("http://localhost:8000") + result <- tryCatch( + httr2::req_perform(r), + error = identity) success <- !inherits(result, "error") } if (!success) { diff --git a/tests/testthat/test-outpack-http-client.R b/tests/testthat/test-outpack-http-client.R index d2e5e4de..d793a14f 100644 --- a/tests/testthat/test-outpack-http-client.R +++ b/tests/testthat/test-outpack-http-client.R @@ -1,24 +1,49 @@ test_that("client sends well formed requests", { skip_if_not_installed("mockery") - verb <- mockery::mock(mock_response(json_string("[1,2,3]"))) - res <- http_client_request(verb, "http://example.com/path") + mock <- local_mock_response(json_string("[1,2,3]")) + + res <- http_client_request("http://example.com/path") expect_mapequal( res, list(status = "success", errors = NULL, data = list(1, 2, 3))) - mockery::expect_called(verb, 1) - expect_equal(mockery::mock_args(verb)[[1]], - list("http://example.com/path")) + + mockery::expect_called(mock, 1) + args <- mockery::mock_args(mock)[[1]] + expect_equal(args[[1]]$url, "http://example.com/path") +}) + + +test_that("client can add headers", { + skip_if_not_installed("mockery") + + mock <- local_mock_response(json_string("[1,2,3]")) + + res <- http_client_request( + "http://example.com/path", + function(r) httr2::req_headers(r, Authorization = "Bearer yogi")) + + expect_mapequal( + res, + list(status = "success", errors = NULL, data = list(1, 2, 3))) + + mockery::expect_called(mock, 1) + args <- mockery::mock_args(mock)[[1]] + expect_equal(args[[1]]$url, "http://example.com/path") + expect_equal(args[[1]]$headers, + list(Authorization = "Bearer yogi"), + ignore_attr = TRUE) }) test_that("client can return json verbatim as text", { skip_if_not_installed("mockery") + ## A little whitespace here to ensure that this has not gone through ## any json processor - verb <- mockery::mock(mock_response(json_string("[1,2, 3]"), wrap = FALSE)) + local_mock_response(json_string("[1,2, 3]"), wrap = FALSE) - res <- http_client_request(verb, "http://example.com/path", + res <- http_client_request("http://example.com/path", parse_json = FALSE) expect_equal(res, "[1,2, 3]") }) @@ -27,24 +52,21 @@ test_that("client can return json verbatim as text", { test_that("client can download files", { skip_if_not_installed("mockery") content <- charToRaw("result") - dest <- temp_file() - verb <- mockery::mock(mock_response(content, download = dest)) - mock_download_options <- mockery::mock(list(TRUE)) - mockery::stub(http_client_request, "http_client_download_options", - mock_download_options) + dest <- temp_file() - res <- http_client_request(verb, "http://example.com/path", - download = dest) + mock <- local_mock_response(content, download = dest) + res <- http_client_request("http://example.com/path", download = dest) expect_identical(res, dest) - mockery::expect_called(verb, 1) - args <- mockery::mock_args(verb)[[1]] - expect_equal(args, list("http://example.com/path", - list(TRUE))) - mockery::expect_called(mock_download_options, 1) - expect_equal(mockery::mock_args(mock_download_options)[[1]], list(dest)) + mockery::expect_called(mock, 1) + args <- mockery::mock_args(mock)[[1]] + + expect_equal(args[[1]]$url, "http://example.com/path") + expect_equal(args[[1]]$headers, + list(Accept = "application/octet-stream"), + ignore_attr = TRUE) }) @@ -53,9 +75,12 @@ test_that("handle errors", { '{"status":"failure",', '"errors":[{"error":"NOT_FOUND","detail":"Resource not found"}],', '"data":null}') - r <- mock_response(json_string(str), status = 404L, wrap = FALSE) - err <- expect_error(http_client_handle_error(r), + + local_mock_response(json_string(str), status = 404L, wrap = FALSE) + + err <- expect_error(http_client_request("http://example.com/path"), "Resource not found") + expect_s3_class(err, "outpack_http_client_error") expect_equal(err$code, 404) expect_equal(err$errors, list(list(error = "NOT_FOUND", @@ -68,9 +93,11 @@ test_that("handle errors from packit", { '{"status":"failure",', '"error":{"error":"NOT_FOUND","detail":"Resource not found"},', '"data":null}') - r <- mock_response(json_string(str), status = 404L, wrap = FALSE) - err <- expect_error(http_client_handle_error(r), - "Resource not found") + + local_mock_response(json_string(str), status = 404L, wrap = FALSE) + + cl <- outpack_http_client$new("http://example.com", NULL) + err <- expect_error(cl$request("path"), "Resource not found") expect_s3_class(err, "outpack_http_client_error") expect_equal(err$code, 404) expect_equal(err$errors, list(list(error = "NOT_FOUND", @@ -79,47 +106,44 @@ test_that("handle errors from packit", { test_that("handle plain text errors", { - r <- mock_response("foobar", status = 503L, wrap = FALSE) - err <- expect_error(http_client_handle_error(r), - "Server error: \\(503\\) Service Unavailable") - expect_s3_class(err, "outpack_http_client_error") - expect_equal(err$code, 503) -}) + local_mock_response("Error", status = 503L, wrap = FALSE) + cl <- outpack_http_client$new("http://example.com", NULL) + err <- expect_error(cl$request("path"), "Service Unavailable") -test_that("can construct sensible download options", { - path <- temp_file() - res <- http_client_download_options(path) - expect_s3_class(res, "request") - expect_equal(res$headers, c(Accept = "application/octet-stream")) - expect_equal(res$output, httr::write_disk(path)$output) + expect_s3_class(err, "outpack_http_client_error") + expect_equal(err$code, 503) + expect_null(err$errors) }) test_that("can use the client to make requests", { skip_if_not_installed("mockery") + + mock <- local_mock_response(json_string("[1,2,3]")) + cl <- outpack_http_client$new("http://example.com", NULL) - mock_get <- mockery::mock(mock_response(json_string("[1,2,3]"))) - mockery::stub(cl$get, "httr::GET", mock_get) - res <- cl$get("/path") - expect_mapequal( - res, - list(status = "success", errors = NULL, data = list(1, 2, 3))) - mockery::expect_called(mock_get, 1) - expect_equal(mockery::mock_args(mock_get)[[1]], - list("http://example.com/path", NULL)) + res <- cl$request("/path") + expect_mapequal(res, + list(status = "success", errors = NULL, data = list(1, 2, 3))) + + args <- mockery::mock_args(mock)[[1]] + expect_equal(args[[1]]$url, "http://example.com/path") }) test_that("can add auth details to the client", { auth <- list(url = "http://example.com/api/login", data = list(token = "mytoken")) + cl <- outpack_http_client$new("http://example.com", auth) - h <- httr::add_headers("Authorization" = paste("Bearer", "yogi")) + + h <- list("Authorization" = "Bearer yogi") mock_login <- mockery::mock(h, cycle = TRUE) mockery::stub(cl$authorise, "http_client_login", mock_login) cl$authorise() + mockery::expect_called(mock_login, 1) expect_equal( mockery::mock_args(mock_login)[[1]], @@ -132,15 +156,18 @@ test_that("can add auth details to the client", { mockery::expect_called(mock_login, 1) ## Actually perform an api call now: - mock_get <- mockery::mock(mock_response(json_string("[1,2,3]"))) - mockery::stub(cl$get, "httr::GET", mock_get) - res <- cl$get("/path") + mock_get <- local_mock_response(json_string("[1,2,3]")) + res <- cl$request("/path") expect_mapequal( res, list(status = "success", errors = NULL, data = list(1, 2, 3))) + mockery::expect_called(mock_get, 1) - expect_equal(mockery::mock_args(mock_get)[[1]], - list("http://example.com/path", h)) + args <- mockery::mock_args(mock_get)[[1]] + expect_equal(args[[1]]$url, "http://example.com/path") + expect_equal(args[[1]]$headers, + list(Authorization = "Bearer yogi"), + ignore_attr = TRUE) }) @@ -148,15 +175,13 @@ test_that("can send authentication request", { clear_auth_cache() withr::defer(clear_auth_cache()) + mock_post <- local_mock_response( + to_json(list(token = jsonlite::unbox("mytoken"))), + wrap = FALSE) + auth <- list( url = "https://example.com/login", data = list(token = "98e02a382db6a3a18e9d2e02c698478b")) - mock_post <- mockery::mock( - mock_response( - to_json(list(token = jsonlite::unbox("mytoken"))), - wrap = FALSE)) - - mockery::stub(http_client_login, "httr::POST", mock_post) res <- evaluate_promise( http_client_login("foo", auth)) @@ -164,14 +189,13 @@ test_that("can send authentication request", { expect_length(res$messages, 2) expect_match(res$messages[[1]], "Logging in to foo") expect_match(res$messages[[2]], "Logged in successfully") - expect_equal(res$result, - httr::add_headers("Authorization" = paste("Bearer", "mytoken"))) + expect_equal(res$result, list("Authorization" = paste("Bearer", "mytoken"))) mockery::expect_called(mock_post, 1) - expect_equal(mockery::mock_args(mock_post)[[1]], - list(auth$url, - body = list(token = auth$data$token), - encode = "json")) + args <- mockery::mock_args(mock_post)[[1]] + expect_equal(args[[1]]$url, auth$url) + expect_equal(args[[1]]$body$data, list(token = auth$data$token)) + expect_equal(args[[1]]$body$type, "json") ## And a second time, does not call mock_post again: expect_silent( @@ -179,3 +203,41 @@ test_that("can send authentication request", { expect_equal(res2, res$result) mockery::expect_called(mock_post, 1) }) + + +test_that("can send authenticated request", { + clear_auth_cache() + withr::defer(clear_auth_cache()) + + mock <- mockery::mock( + mock_response(to_json(list(token = jsonlite::unbox("mytoken"))), + wrap = FALSE), + mock_response(json_string("[1,2,3]"))) + httr2::local_mocked_responses(function(req) mock(req)) + + auth <- list( + url = "https://example.com/api/login", + data = list(token = "98e02a382db6a3a18e9d2e02c698478b")) + + cl <- outpack_http_client$new("http://example.com", auth) + res <- evaluate_promise(cl$request("data")) + + expect_length(res$messages, 2) + expect_match(res$messages[[1]], "Logging in to http://example.com") + expect_match(res$messages[[2]], "Logged in successfully") + expect_mapequal( + res$result, + list(status = "success", errors = NULL, data = list(1, 2, 3))) + + mockery::expect_called(mock, 2) + post_args <- mockery::mock_args(mock)[[1]] + expect_equal(post_args[[1]]$url, auth$url) + expect_equal(post_args[[1]]$body$data, list(token = auth$data$token)) + expect_equal(post_args[[1]]$body$type, "json") + + get_args <- mockery::mock_args(mock)[[2]] + expect_equal(get_args[[1]]$url, "http://example.com/data") + expect_equal(get_args[[1]]$headers, + list(Authorization = paste("Bearer mytoken")), + ignore_attr = TRUE) +}) diff --git a/tests/testthat/test-zzz-location-http.R b/tests/testthat/test-zzz-location-http.R index 7bb998b7..6e3a342b 100644 --- a/tests/testthat/test-zzz-location-http.R +++ b/tests/testthat/test-zzz-location-http.R @@ -122,9 +122,10 @@ describe("http location integration tests", { ## Trigger the error directly: cl <- outpack_http_client$new(url, NULL) - err <- expect_error(cl$post(sprintf("/packet/%s", hash), meta, - httr::content_type("text/plain")), - "Expected hash '.+' but found '.+'") + err <- expect_error( + cl$request(sprintf("/packet/%s", hash), + function(r) httr2::req_body_raw(r, meta, "text/plain")), + "Expected hash '.+' but found '.+'") }) it("throws sensible error if file hash does not match expected", {