From 6f78779b43bc060cbc9a7d66614028aff499c355 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Wed, 4 Sep 2024 13:16:55 +0100 Subject: [PATCH 1/4] Extend `curl_opts` to allow multiple data elements (#539) Fixes #509 --- R/curl.R | 2 +- tests/testthat/test-curl.R | 7 +++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/R/curl.R b/R/curl.R index 5a7710a0..cc5cef56 100644 --- a/R/curl.R +++ b/R/curl.R @@ -197,7 +197,7 @@ curl_data <- function(x, binary = FALSE, raw = FALSE) { } # Format described at -curl_opts <- "Usage: curl [] [-H
...] [options] [] +curl_opts <- "Usage: curl [] [-H
...] [-d ...] [options] [] --basic (IGNORED) --compressed (IGNORED) --digest (IGNORED) diff --git a/tests/testthat/test-curl.R b/tests/testthat/test-curl.R index b3302991..225983a4 100644 --- a/tests/testthat/test-curl.R +++ b/tests/testthat/test-curl.R @@ -34,6 +34,13 @@ test_that("captures key components of call", { expect_equal(curl_args("curl 'http://example.com' --verbose")$`--verbose`, TRUE) }) +test_that("can accept multiple data arguments", { + expect_equal( + curl_args("curl https://example.com -d x=1 -d y=2")$`--data`, + c("x=1", "y=2") + ) +}) + test_that("can handle line breaks", { expect_equal( curl_args("curl 'http://example.com' \\\n -H 'A: 1' \\\n -H 'B: 2'")$`--header`, From fa27e9e7a58ff81a64bf2e1c8eab1ccaa8b774ef Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Thu, 5 Sep 2024 13:49:03 +0100 Subject: [PATCH 2/4] Default to no progress bar in tests (#541) --- NEWS.md | 1 + R/utils.R | 9 +++++---- tests/testthat/_snaps/utils.md | 6 +++--- tests/testthat/test-utils.R | 6 ++---- 4 files changed, 11 insertions(+), 11 deletions(-) diff --git a/NEWS.md b/NEWS.md index 4b086fa7..dab50a9e 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,6 @@ # httr2 (development version) +* `req_perform()` no longer displays a progress bar when sleeping during tests. You can override this behaviour by setting the option `httr2_progress`. * `req_cache()` now re-caches the response if the body is hasn't been modified but the headers have changed (#442). * `req_cache()` works better when `req_perform()` sets a path (#442). * `req_body_*()` now give informative error if you attempt to change the body type (#451). diff --git a/R/utils.R b/R/utils.R index ac707da4..8626a97a 100644 --- a/R/utils.R +++ b/R/utils.R @@ -43,11 +43,12 @@ modify_list <- function(.x, ..., error_call = caller_env()) { } -sys_sleep <- function(seconds, - task, - fps = 10, - progress = getOption("httr2_progress", TRUE)) { +sys_sleep <- function(seconds, task, fps = 10, progress = NULL) { check_number_decimal(seconds) + check_string(task) + check_number_decimal(fps) + progress <- progress %||% getOption("httr2_progress", !is_testing()) + check_bool(progress, allow_null = TRUE) if (seconds == 0) { return(invisible()) diff --git a/tests/testthat/_snaps/utils.md b/tests/testthat/_snaps/utils.md index c879e3d0..1197670b 100644 --- a/tests/testthat/_snaps/utils.md +++ b/tests/testthat/_snaps/utils.md @@ -6,10 +6,10 @@ Error: ! All components of `...` must be named. -# can suppress progress bar +# progress bar suppressed in tests Code - sys_sleep(0.1, "for test") + sys_sleep(0.1, "in test") Message - > Waiting 1s for test + > Waiting 1s in test diff --git a/tests/testthat/test-utils.R b/tests/testthat/test-utils.R index aeb46a1f..20e3e19c 100644 --- a/tests/testthat/test-utils.R +++ b/tests/testthat/test-utils.R @@ -29,8 +29,6 @@ test_that("respects httr verbose config", { expect_equal(httr2_verbosity(), 1) }) -test_that("can suppress progress bar", { - withr::local_options(httr2_progress = FALSE) - - expect_snapshot(sys_sleep(0.1, "for test")) +test_that("progress bar suppressed in tests", { + expect_snapshot(sys_sleep(0.1, "in test")) }) From 88f093272f77e9599bcb7035bc8094aba3b43683 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Thu, 5 Sep 2024 14:11:17 +0100 Subject: [PATCH 3/4] Add a done callback (#542) So we can ensure that open connections are always closed. Fixes #534. --- NEWS.md | 1 + R/multi-req.R | 4 ++++ R/req-body.R | 30 ++++++------------------------ R/req-perform-stream.R | 6 ++++-- R/req-perform.R | 18 +++++++++++++++--- tests/testthat/helper.R | 3 +++ 6 files changed, 33 insertions(+), 29 deletions(-) create mode 100644 tests/testthat/helper.R diff --git a/NEWS.md b/NEWS.md index dab50a9e..076e94d4 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,6 @@ # httr2 (development version) +* `req_body_file()` no longer leaks a connection if the response doesn't complete succesfully (#534). * `req_perform()` no longer displays a progress bar when sleeping during tests. You can override this behaviour by setting the option `httr2_progress`. * `req_cache()` now re-caches the response if the body is hasn't been modified but the headers have changed (#442). * `req_cache()` works better when `req_perform()` sets a path (#442). diff --git a/R/multi-req.R b/R/multi-req.R index 0558a82a..dcfda0e2 100644 --- a/R/multi-req.R +++ b/R/multi-req.R @@ -147,6 +147,7 @@ pool_run <- function(pool, perfs, on_error = "continue") { # Wrap up all components of request -> response in a single object Performance <- R6Class("Performance", public = list( req = NULL, + req_prep = NULL, path = NULL, handle = NULL, @@ -166,6 +167,7 @@ Performance <- R6Class("Performance", public = list( if (is_response(req)) { self$resp <- req } else { + self$req_prep <- req_prepare(req) self$handle <- req_handle(req) curl::handle_setopt(self$handle, url = req$url) } @@ -190,6 +192,7 @@ Performance <- R6Class("Performance", public = list( succeed = function(res) { self$progress$update() + req_completed(self$req_prep) if (is.null(self$path)) { body <- res$content @@ -220,6 +223,7 @@ Performance <- R6Class("Performance", public = list( fail = function(msg) { self$progress$update() + req_completed(self$req_prep) self$resp <- error_cnd( "httr2_failure", diff --git a/R/req-body.R b/R/req-body.R index 74c344ef..e4840f53 100644 --- a/R/req-body.R +++ b/R/req-body.R @@ -229,35 +229,17 @@ req_body_apply <- function(req) { if (type == "raw-file") { size <- file.info(data)$size - done <- FALSE # Only open connection if needed delayedAssign("con", file(data, "rb")) - # Leaks connection if request doesn't complete - readfunction <- function(nbytes, ...) { - if (done) { - return(raw()) - } - out <- readBin(con, "raw", nbytes) - if (length(out) < nbytes) { - close(con) - done <<- TRUE - con <<- NULL - } - out - } - seekfunction <- function(offset, ...) { - if (done) { - con <<- file(data, "rb") - done <<- FALSE - } - seek(con, where = offset) - } - + req <- req_policies( + req, + done = function() close(con) + ) req <- req_options(req, post = TRUE, - readfunction = readfunction, - seekfunction = seekfunction, + readfunction = function(nbytes, ...) readBin(con, "raw", nbytes), + seekfunction = function(offset, ...) seek(con, where = offset), postfieldsize_large = size ) } else if (type == "raw") { diff --git a/R/req-perform-stream.R b/R/req-perform-stream.R index cdc8ac44..3f84585a 100644 --- a/R/req-perform-stream.R +++ b/R/req-perform-stream.R @@ -39,7 +39,6 @@ req_perform_stream <- function(req, round = c("byte", "line")) { check_request(req) - handle <- req_handle(req) check_function(callback) check_number_decimal(timeout_sec, min = 0) check_number_decimal(buffer_kb, min = 0) @@ -123,7 +122,8 @@ req_perform_connection <- function(req, mode <- arg_match(mode) con_mode <- if (mode == "text") "rf" else "rbf" - handle <- req_handle(req) + req_prep <- req_prepare(req) + handle <- req_handle(req_prep) the$last_request <- req tries <- 0 @@ -147,6 +147,8 @@ req_perform_connection <- function(req, } } + req_completed(req_prep) + if (error_is_error(req, resp)) { # Read full body if there's an error conn <- resp$body diff --git a/R/req-perform.R b/R/req-perform.R index aa18c4df..cab28baa 100644 --- a/R/req-perform.R +++ b/R/req-perform.R @@ -94,7 +94,8 @@ req_perform <- function( return(req) } - handle <- req_handle(req) + req_prep <- req_prepare(req) + handle <- req_handle(req_prep) max_tries <- retry_max_tries(req) deadline <- Sys.time() + retry_max_seconds(req) @@ -122,6 +123,7 @@ req_perform <- function( ) } ) + req_completed(req_prep) if (is_error(resp)) { tries <- tries + 1 @@ -129,7 +131,8 @@ req_perform <- function( } else if (!reauth && resp_is_invalid_oauth_token(req, resp)) { reauth <- TRUE req <- auth_oauth_sign(req, TRUE) - handle <- req_handle(req) + req_prep <- req_prepare(req) + handle <- req_handle(req_prep) delay <- 0 } else if (retry_is_transient(req, resp)) { tries <- tries + 1 @@ -258,6 +261,7 @@ req_dry_run <- function(req, quiet = FALSE, redact_headers = TRUE) { req <- req_options(req, debugfunction = debug, verbose = TRUE) } + req <- req_prepare(req) handle <- req_handle(req) curl::handle_setopt(handle, url = req$url) resp <- curl::curl_echo(handle, progress = FALSE) @@ -269,7 +273,9 @@ req_dry_run <- function(req, quiet = FALSE, redact_headers = TRUE) { )) } -req_handle <- function(req) { +# Must call req_prepare(), then req_handle(), then after the request has been +# performed, req_completed() +req_prepare <- function(req) { req <- req_method_apply(req) req <- req_body_apply(req) @@ -277,6 +283,9 @@ req_handle <- function(req) { req <- req_user_agent(req) } + req +} +req_handle <- function(req) { handle <- curl::new_handle() curl::handle_setheaders(handle, .list = headers_flatten(req$headers)) curl::handle_setopt(handle, .list = req$options) @@ -286,6 +295,9 @@ req_handle <- function(req) { handle } +req_completed <- function(req) { + req_policy_call(req, "done", list(), NULL) +} new_path <- function(x) structure(x, class = "httr2_path") is_path <- function(x) inherits(x, "httr2_path") diff --git a/tests/testthat/helper.R b/tests/testthat/helper.R new file mode 100644 index 00000000..0eef785d --- /dev/null +++ b/tests/testthat/helper.R @@ -0,0 +1,3 @@ +testthat::set_state_inspector(function() { + getAllConnections() +}) From a241b04b079c5a76a04e5ac449c0612c99e0fd00 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Thu, 5 Sep 2024 14:18:03 +0100 Subject: [PATCH 4/4] Implement `req_cookies_set()` (#540) Fixes #369 --- NAMESPACE | 1 + NEWS.md | 1 + R/req-cookies.R | 57 ++++++++++++++++++++++++------- R/url.R | 8 +++-- man/req_cookie_preserve.Rd | 44 +++++++++++++++++++----- tests/testthat/test-req-cookies.R | 18 ++++++++++ 6 files changed, 105 insertions(+), 24 deletions(-) diff --git a/NAMESPACE b/NAMESPACE index e2f4ff2a..112fc391 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -60,6 +60,7 @@ export(req_body_multipart) export(req_body_raw) export(req_cache) export(req_cookie_preserve) +export(req_cookies_set) export(req_dry_run) export(req_error) export(req_headers) diff --git a/NEWS.md b/NEWS.md index 076e94d4..1f14699b 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,6 @@ # httr2 (development version) +* New `req_cookie_set()` allows you to set client side cookies (#369). * `req_body_file()` no longer leaks a connection if the response doesn't complete succesfully (#534). * `req_perform()` no longer displays a progress bar when sleeping during tests. You can override this behaviour by setting the option `httr2_progress`. * `req_cache()` now re-caches the response if the body is hasn't been modified but the headers have changed (#442). diff --git a/R/req-cookies.R b/R/req-cookies.R index 29291f4b..65e13159 100644 --- a/R/req-cookies.R +++ b/R/req-cookies.R @@ -1,37 +1,68 @@ -#' Preserve cookies across requests +#' Set and preserve cookies +#' +#' @description +#' Use `req_cookie_set()` to set client side cookies that are sent to the +#' server. #' #' By default, httr2 uses a clean slate for every request meaning that cookies -#' are not automatically preserved across requests. To preserve cookies, you -#' must set a cookie file which will be read before and updated after each -#' request. +#' are not automatically preserved across requests. To preserve cookies, use +#' `req_cookie_preserve()` along with the path to cookie file that will be +#' read before and updated after each request. #' #' @inheritParams req_perform #' @param path A path to a file where cookies will be read from before and updated after the request. #' @export #' @examples +#' # Use `req_cookies_set()` to set client-side cookies +#' request(example_url()) |> +#' req_cookies_set(a = 1, b = 1) |> +#' req_dry_run() +#' +#' # Use `req_cookie_preserve()` to preserve server-side cookies across requests #' path <- tempfile() -#' httpbin <- request(example_url()) |> -#' req_cookie_preserve(path) #' -#' # Manually set two cookies -#' httpbin |> +#' # Set a server-side cookie +#' request(example_url()) |> +#' req_cookie_preserve(path) |> #' req_template("/cookies/set/:name/:value", name = "chocolate", value = "chip") |> #' req_perform() |> #' resp_body_json() #' -#' httpbin |> +#' # Set another sever-side cookie +#' request(example_url()) |> +#' req_cookie_preserve(path) |> #' req_template("/cookies/set/:name/:value", name = "oatmeal", value = "raisin") |> #' req_perform() |> #' resp_body_json() #' +#' # Add a client side cookie +#' request(example_url()) |> +#' req_url_path("/cookies/set") |> +#' req_cookie_preserve(path) |> +#' req_cookies_set(snicker = "doodle") |> +#' req_perform() |> +#' resp_body_json() +#' #' # The cookie path has a straightforward format #' cat(readChar(path, nchars = 1e4)) req_cookie_preserve <- function(req, path) { check_request(req) check_string(path, allow_empty = FALSE) - req_options(req, - cookiejar = path, - cookiefile = path - ) + req_options(req, cookiejar = path, cookiefile = path) +} + +#' @export +#' @rdname req_cookie_preserve +#' @param ... <[`dynamic-dots`][rlang::dyn-dots]> +#' Name-value pairs that define query parameters. Each value must be +#' an atomic vector, which is automatically escaped. To opt-out of escaping, +#' wrap strings in `I()`. +req_cookies_set <- function(req, ...) { + check_request(req) + req_options(req, cookie = cookies_build(list2(...))) +} + +cookies_build <- function(x, error_call = caller_env()) { + elements_build(x, "Cookies", ";", error_call = error_call) } diff --git a/R/url.R b/R/url.R index b4f3cf0c..8ffab225 100644 --- a/R/url.R +++ b/R/url.R @@ -165,8 +165,12 @@ query_parse <- function(x) { } query_build <- function(x, error_call = caller_env()) { + elements_build(x, "Query", "&", error_call = error_call) +} + +elements_build <- function(x, name, collapse, error_call = caller_env()) { if (!is_list(x) || (!is_named(x) && length(x) > 0)) { - cli::cli_abort("Query must be a named list.", call = error_call) + cli::cli_abort("{name} must be a named list.", call = error_call) } x <- compact(x) @@ -177,7 +181,7 @@ query_build <- function(x, error_call = caller_env()) { values <- map2_chr(x, names(x), format_query_param, error_call = error_call) names <- curl::curl_escape(names(x)) - paste0(names, "=", values, collapse = "&") + paste0(names, "=", values, collapse = collapse) } format_query_param <- function(x, diff --git a/man/req_cookie_preserve.Rd b/man/req_cookie_preserve.Rd index 5bbcb23b..0ef9f237 100644 --- a/man/req_cookie_preserve.Rd +++ b/man/req_cookie_preserve.Rd @@ -2,37 +2,63 @@ % Please edit documentation in R/req-cookies.R \name{req_cookie_preserve} \alias{req_cookie_preserve} -\title{Preserve cookies across requests} +\alias{req_cookies_set} +\title{Set and preserve cookies} \usage{ req_cookie_preserve(req, path) + +req_cookies_set(req, ...) } \arguments{ \item{req}{A httr2 \link{request} object.} \item{path}{A path to a file where cookies will be read from before and updated after the request.} + +\item{...}{<\code{\link[rlang:dyn-dots]{dynamic-dots}}> +Name-value pairs that define query parameters. Each value must be +an atomic vector, which is automatically escaped. To opt-out of escaping, +wrap strings in \code{I()}.} } \description{ +Use \code{req_cookie_set()} to set client side cookies that are sent to the +server. + By default, httr2 uses a clean slate for every request meaning that cookies -are not automatically preserved across requests. To preserve cookies, you -must set a cookie file which will be read before and updated after each -request. +are not automatically preserved across requests. To preserve cookies, use +\code{req_cookie_preserve()} along with the path to cookie file that will be +read before and updated after each request. } \examples{ +# Use `req_cookies_set()` to set client-side cookies +request(example_url()) |> + req_cookies_set(a = 1, b = 1) |> + req_dry_run() + +# Use `req_cookie_preserve()` to preserve server-side cookies across requests path <- tempfile() -httpbin <- request(example_url()) |> - req_cookie_preserve(path) -# Manually set two cookies -httpbin |> +# Set a server-side cookie +request(example_url()) |> + req_cookie_preserve(path) |> req_template("/cookies/set/:name/:value", name = "chocolate", value = "chip") |> req_perform() |> resp_body_json() -httpbin |> +# Set another sever-side cookie +request(example_url()) |> + req_cookie_preserve(path) |> req_template("/cookies/set/:name/:value", name = "oatmeal", value = "raisin") |> req_perform() |> resp_body_json() +# Add a client side cookie +request(example_url()) |> + req_url_path("/cookies/set") |> + req_cookie_preserve(path) |> + req_cookies_set(snicker = "doodle") |> + req_perform() |> + resp_body_json() + # The cookie path has a straightforward format cat(readChar(path, nchars = 1e4)) } diff --git a/tests/testthat/test-req-cookies.R b/tests/testthat/test-req-cookies.R index 039362af..85d3cda4 100644 --- a/tests/testthat/test-req-cookies.R +++ b/tests/testthat/test-req-cookies.R @@ -20,3 +20,21 @@ test_that("can read/write cookies", { expect_mapequal(cookies, list(x = "a", y = "b", z = "c")) }) + +test_that("can set cookies", { + resp <- request(example_url()) %>% + req_cookies_set(a = 1, b = 1) %>% + req_url_path("/cookies") %>% + req_perform() + + expect_equal(resp_body_json(resp), list(cookies = list(a = "1", b = "1"))) +}) + +test_that("cookie values are usually escaped", { + resp <- request(example_url()) %>% + req_cookies_set(a = I("%20"), b = "%") %>% + req_url_path("/cookies") %>% + req_perform() + + expect_equal(resp_body_json(resp), list(cookies = list(a = "%20", b = "%25"))) +})