Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
jchrom opened this issue Apr 28, 2022 · 12 comments
Closed

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

jchrom opened this issue Apr 28, 2022 · 12 comments

Comments

@jchrom
Copy link
Contributor

jchrom commented Apr 28, 2022

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
Copy link
Member

hadley commented Apr 28, 2022

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
Copy link
Contributor Author

jchrom commented Apr 28, 2022

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
Copy link
Member

hadley commented Apr 28, 2022

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
Copy link
Contributor Author

jchrom commented Apr 28, 2022

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
Copy link
Contributor Author

jchrom commented Apr 28, 2022

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
Copy link
Member

hadley commented Apr 29, 2022

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
Copy link
Contributor Author

jchrom commented Apr 29, 2022

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
Copy link
Member

hadley commented Apr 29, 2022

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
Copy link
Contributor Author

jchrom commented Apr 29, 2022

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
Copy link
Member

hadley commented Apr 29, 2022

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
Copy link
Contributor Author

jchrom commented Apr 29, 2022

Alright! Shall I prepare a PR?

@hadley
Copy link
Member

hadley commented Apr 29, 2022

@jchrom yes please 😄

jchrom added a commit to jchrom/httr2 that referenced this issue Apr 30, 2022
jchrom added a commit to jchrom/httr2 that referenced this issue May 3, 2022
jchrom added a commit to jchrom/httr2 that referenced this issue May 3, 2022
@hadley hadley closed this as completed in c18eff6 May 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants