Use curl to modify/construct urls (#741)

Note that `req_url_path_append()` now only actually affects the path, not other components of the URL like the query params.

Fixes #732

Author: hadley

DESCRIPTION

@@ -17,7 +17,7 @@ Depends:
     R (>= 4.1)
 Imports:
     cli (>= 3.0.0),
-    curl (>= 6.2.1),
+    curl (>= 6.4.0),
     glue,
     lifecycle,
     magrittr,

NEWS.md

@@ -1,5 +1,6 @@
 # httr2 (development version)
 
+* URL construction is now powered by `curl::curl_modify_url()`, and hence now (correctly) escapes the `path` component (#732). This means that `req_url_path()` now can only affect the path component of the URL, not the query params or fragment.
 * Redacted headers are no longer serialized to disk. This is important since it makes it harder to accidentally leak secrets to files on disk, but comes at a cost: you can longer perform such requests that have been saved and reloaded (#721).
 * New `req_get_method()` and `req_get_body()` allow you to do some limited prediction of what a request _will_ do when it's performed (#718).
 * Functions that capture interrutps (like `req_perform_parallel()` and friends) are now easier to escape if they're called inside a loop: you can press Ctrl + C twice to guarantee an exit (#1810).

R/resp-url.R

@@ -9,7 +9,8 @@
 #' @export
 #' @examples
 #' resp <- request(example_url()) |>
-#'   req_url_path("/get?hello=world") |>
+#'   req_url_path("/get") |>
+#'   req_url_query(hello = "world") |>
 #'   req_perform()
 #'
 #' resp |> resp_url()

R/url.R

@@ -24,7 +24,7 @@ url_parse <- function(url, base_url = NULL) {
   check_string(url)
   check_string(base_url, allow_null = TRUE)
 
-  curl <- curl::curl_parse_url(url, baseurl = base_url, decode = FALSE)
+  curl <- curl::curl_parse_url(url, baseurl = base_url)
 
   parsed <- list(
     scheme = curl$scheme,
@@ -267,44 +267,21 @@ url_build <- function(url) {
     stop_input_type(url, "a parsed URL")
   }
 
-  if (!is.null(url$query)) {
-    query <- url_query_build(url$query)
+  if (length(url$query) == 0) {
+    query <- ""
   } else {
-    query <- NULL
+    query <- I(url_query_build(url$query))
   }
 
-  if (is.null(url$username) && is.null(url$password)) {
-    user_pass <- NULL
-  } else if (is.null(url$username) && !is.null(url$password)) {
-    cli::cli_abort("Cannot set url {.arg password} without {.arg username}.")
-  } else if (!is.null(url$username) && is.null(url$password)) {
-    user_pass <- paste0(url$username, "@")
-  } else {
-    user_pass <- paste0(url$username, ":", url$password, "@")
-  }
-
-  if (!is.null(user_pass) || !is.null(url$hostname) || !is.null(url$port)) {
-    authority <- paste0(user_pass, url$hostname)
-    if (!is.null(url$port)) {
-      authority <- paste0(authority, ":", url$port)
-    }
-  } else {
-    authority <- NULL
-  }
-
-  if (is.null(url$path) || !startsWith(url$path, "/")) {
-    url$path <- paste0("/", url$path)
-  }
-
-  prefix <- function(prefix, x) if (!is.null(x)) paste0(prefix, x)
-  paste0(
-    url$scheme,
-    if (!is.null(url$scheme)) ":",
-    if (!is.null(url$scheme) || !is.null(authority)) "//",
-    authority,
-    url$path,
-    prefix("?", query),
-    prefix("#", url$fragment)
+  curl::curl_modify_url(
+    scheme = url$scheme,
+    host = url$hostname,
+    user = url$username,
+    password = url$password,
+    port = url$port,
+    path = url$path,
+    query = query,
+    fragment = url$fragment
   )
 }
 

man/resp_url.Rd

@@ -1,10 +1,19 @@
 # Revdeps
 
-## Failed to check (3)
+## Failed to check (1)
 
-|package    |version |error |warning |note |
-|:----------|:-------|:-----|:-------|:----|
-|GeoTox     |?       |      |        |     |
-|insight    |?       |      |        |     |
-|parameters |?       |      |        |     |
+|package      |version |error |warning |note |
+|:------------|:-------|:-----|:-------|:----|
+|arcgisplaces |0.1.2   |1     |        |     |
+
+## New problems (6)
+
+|package       |version |error  |warning |note |
+|:-------------|:-------|:------|:-------|:----|
+|[atrrr](problems.md#atrrr)|0.1.0   |__+1__ |        |     |
+|[httptest2](problems.md#httptest2)|1.1.0   |__+1__ |        |     |
+|[osmapiR](problems.md#osmapir)|0.2.3   |__+2__ |        |     |
+|[planscorer](problems.md#planscorer)|0.0.2   |__+1__ |        |     |
+|[spanishoddata](problems.md#spanishoddata)|0.2.0   |__+1__ |        |     |
+|[tidyllm](problems.md#tidyllm)|0.3.4   |__+1__ |        |     |
 

revdep/README.md

@@ -1,13 +1,34 @@
 ## revdepcheck results
 
-We checked 201 reverse dependencies (200 from CRAN + 1 from Bioconductor), comparing R CMD check results across CRAN and dev versions of this package.
+We checked 221 reverse dependencies, comparing R CMD check results across CRAN and dev versions of this package.
 
- * We saw 0 new problems
- * We failed to check 2 packages
+ * We saw 6 new problems
+ * We failed to check 1 packages
 
 Issues with CRAN packages are summarised below.
 
+### New problems
+(This reports the first line of each new failure)
+
+* atrrr
+  checking tests ... ERROR
+
+* httptest2
+  checking tests ... ERROR
+
+* osmapiR
+  checking examples ... ERROR
+  checking tests ... ERROR
+
+* planscorer
+  checking tests ... ERROR
+
+* spanishoddata
+  checking tests ... ERROR
+
+* tidyllm
+  checking tests ... ERROR
+
 ### Failed to check
 
-* insight    (NA)
-* parameters (NA)
+* arcgisplaces (NA)