r-lib / httr2

Make HTTP requests and process their responses. A modern reimagining of httr.
https://httr2.r-lib.org
Other
237 stars 57 forks source link

Make `req_template()` append the path, instead of replace #133

Closed jchrom closed 2 years ago

jchrom commented 2 years ago

It is somewhat inconvenient that when modifying an existing request, the path gets overwritten wholly. Using req_url_path_append() instead of req_url_path() would be more convenient. For example, the MediaWiki REST API has a path component which is common for all endpoints, but it gets overwritten if I use req_template().

Could we squeeze this in for 0.2, if it makes sense?

hadley commented 2 years ago

Sorry I missed this and just submitted. But could you explain a bit more how you're using req_template? Why don't you include the common path component in the template?

jchrom commented 2 years ago

Well... I'll live :)

The problem: When copying from the spec, the common bit is not there, so either the user of my API client has to do extra work pasting it themselves, or the common bit needs to be "injected" into template in between the method and the path. This is doable of course, but it would be neater if the path just got appended rather than replaced.

Here is an example:

library(httr2)

# I have a high-level function: 
nice_request <- function(hostname, template, ..., .env = parent.frame()) {

  # This is where MediaWikis expose the core REST v1 API:
  server <- "/w/rest.php/v1"

  request(hostname) %>%
    req_url_path(server) %>% 
    req_template(template, ..., .env = .env)
}

# I copy the template from the spec (which does not have the common bit):
tmp <- "POST /greeting/{endpoint}"

# But...
nice_request("https://my-wiki.org", tmp, endpoint = "hi")
#> <httr2_request>
#> POST https://my-wiki.org/greeting/hi
#> Body: empty

Created on 2022-04-28 by the reprex package (v2.0.1)

hadley commented 2 years ago

Why not do this?

nice_request <- function(hostname, template, ..., .env = parent.frame()) {
  # This is where MediaWikis expose the core REST v1 API:
  server <- "/w/rest.php/v1"

  request(hostname) %>%
    req_template(paste(server, template), ..., .env = .env)
}

?

jchrom commented 2 years ago

The "POST" in the beginning ruins it:

library(httr2)

nice_request <- function(hostname, template, ..., .env = parent.frame()) {
  # This is where MediaWikis expose the core REST v1 API:
  server <- "/w/rest.php/v1"

  request(hostname) %>%
    req_template(paste(server, template), ..., .env = .env)
}

# I copy the template from the spec:
tmp <- "POST /greeting/{endpoint}"

# But...
nice_request("https://my-wiki.org", tmp, endpoint = "hi")
#> Error in `req_template()`:
#> ! Can't parse template `template`
#> ℹ Should have form like 'GET /a/b/c' or 'a/b/c/'

Created on 2022-04-28 by the reprex package (v2.0.1)

jchrom commented 2 years ago

I am currently doing something like the below, and it works, so it's not a huge issue. It would just make more sense if it was a native behavior (or perhaps a new .append = FALSE argument).

library(httr2)

nice_request <- function(hostname, template, ..., .env = parent.frame()) {

  template <- sub("(GET |POST )?/?(.*)", "\\1/w/rest.php/v1/\\2", template)

  request(hostname) %>%
    req_template(template, ..., .env = .env)
}

# I copy the template from the spec:
tmp <- "POST /greeting/{endpoint}"

# But...
nice_request("https://my-wiki.org", tmp, endpoint = "hi")
#> <httr2_request>
#> POST https://my-wiki.org/w/rest.php/v1/greeting/hi
#> Body: empty

Created on 2022-04-28 by the reprex package (v2.0.1)

hadley commented 2 years ago

Ooooh I see what you mean. It feels confusing for req_template() to append to the path, but maybe it could take a prefix argument that was inserted between the method and the template path?

httr2 is on CRAN, but is failing on a CRAN machine so I'll have to do a patch release so I could still get this in in the next couple of days.

jchrom commented 2 years ago

Hmmm. To me it's more confusing that req_template() accepts a request as it's first argument, but instead of adding to it, it replaces. I guess my expectation would be: Don't mess with my req. If req already has a path, keep it.

Funny how this "replace vs modify" keeps popping up in places.

hadley commented 2 years ago

Using a templating feels very "set"-like to me, because you're presumably giving it a complete path. You're unlikely to want to call req_template() multiple times in a row (or at least I'd find that very hard to reason about).

I think the difference between paths and the other places we modify requests is that paths have to append to the end; we can't modify specific components.

jchrom commented 2 years ago

I see your point about templating. I think I'd have the same intuition if req_template() was creating the request from scratch.

Looking at this Open API spec. It has a common path component in its "servers" element, and this has to prepend to individual paths. If I wanted to use templating, I couldn't just create a starting request object with server path included. Instead, I would have to ensure the shared component is added to every template (e.g. via a prefix argument), increasing the complexity of req_template() call.

Even though the logic makes sense, it feels like adding hoops to jump through (compared to just adding to existing path).

hadley commented 2 years ago

Ok, you've convinced me. Let's make req_template() append to the end of the path; we can just document that you probably don't want to call it multiple times on the same request.

jchrom commented 2 years ago

Alright! Shall I prepare a PR?

hadley commented 2 years ago

@jchrom yes please 😄