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

Add mounted router / route to mounted location if no definition exists #465

Closed
antoine-sachet opened this issue Jul 30, 2019 · 9 comments · Fixed by #746
Closed

Add mounted router / route to mounted location if no definition exists #465

antoine-sachet opened this issue Jul 30, 2019 · 9 comments · Fixed by #746
Labels
difficulty: advanced Best for maintainers to address effort: medium < 3 days of work Fixed in PR A PR has implemented a fix for this Issue priority: low Will be fixed eventually theme: trailing slash type: enhancement Adds a new, backwards-compatible feature
Milestone

Comments

@antoine-sachet
Copy link
Contributor

antoine-sachet commented Jul 30, 2019

A trailing slash breaks the endpoint path but is required when accessing a mounted endpoint. This can make the API inconsistent.

Consider:

# plumber.R
#* @get /root
function() {
  rnorm(1)
}
# sub_api.R
#* @get /
function() {
  rnorm(1)
}
# entrypoint.R
library("plumber")
root <- plumber::plumber$new("plumber.R")
sub_api <- plumber$new("sub_api.R")
root$mount("/sub", sub_api)
root

Running the API:

  • url/root works
  • url/root/ gives a 404
  • url/sub gives a 404
  • url/sub/ works

Is there a way to make /sub be equivalent to /sub/ and /root/ to /root? Or is it a bad idea?

@schloerke
Copy link
Collaborator

Are trailing slashes required when visiting regular endpoints?

@antoine-sachet
Copy link
Contributor Author

antoine-sachet commented Jul 31, 2019

No, you're right, it's mostly url/sub not working which is very annoying as it creates an inconsistent experience for the API user because of an implementation detail.

It would be nice to have /root/ work as well (to make life easier for the API user), but this is completely secondary and, if it makes life harder for the programmer, it is not worth it.

@schloerke schloerke changed the title Should both /endpoint and /endpoint/ work? Trailing slashes should route to the same place Jul 31, 2019
@schloerke
Copy link
Collaborator

I'm in the camp that trailing slashes matter and should be treated as separate routes as they are separate routes.

I also understand the ease of use and common pattern of defining the route once and having it work in both places. I don't think I can enable it by default at this point, but adding a method (or init flag) would work well and tell of route mismatches.

@schloerke schloerke added this to the future - v0.5.1 milestone Jul 31, 2019
@schloerke schloerke added difficulty: advanced Best for maintainers to address effort: medium < 3 days of work priority: low Will be fixed eventually type: enhancement Adds a new, backwards-compatible feature duplicate Already catalogued labels Jul 31, 2019
@schloerke
Copy link
Collaborator

Closing in favor of #100

@antoine-sachet
Copy link
Contributor Author

antoine-sachet commented Aug 19, 2019

I agree with you, the routes are different and should be treated differently.

However, my point is different from issue #100 and I disagree with the renaming of this thread to "Trailing slashes should route to the same place".

My core issue is that when you mount an endpoint on /endpoint, then /endpoint does not actually work. You have to call the mounted endpoint with a trailing slash (/endpoint/) and this is very inconsistent, since trailing slashes are not otherwise required.

I maintain a relatively complex API whose main components are broken down in several plumber files and mounted (to e.g. /report, /review, /user)

People who integrate with our API ask me why they must put a trailing slash to /report/ but mustn't put one for all the endpoints below /report/ (like /report/<id> or /report/<id>/reset). Same thing for /review/, /user/, etc.

As far as the end-user is concerned, it seems like some random endpoints take a trailing slash while most others do not. Feels like a leaky abstraction?

@schloerke
Copy link
Collaborator

Are you wanting the example above to work like the lists below?

  • url/root works
  • url/root/ gives a 404
  • url/sub gives a 404 works
  • url/sub/ works gives a 404

And if sub_api had an extra route...

  • url/sub/extra works

The way it currently implemented, you are allowed to have a custom route at url/sub with a mounted router starting at url/sub/.

So, would it be good to add the / mounted route to the base location? So it would be

  • url/root works
  • url/root/ gives a 404
  • url/sub works ; (defined as url/sub/ if url/sub is not already defined)
  • url/sub/ works

@schloerke schloerke reopened this Aug 19, 2019
@antoine-sachet
Copy link
Contributor Author

antoine-sachet commented Aug 20, 2019

Exactly! Sorry I wasn't clear in my initial post, I tried to make it generic but just confounded the issues.

@antoine-sachet antoine-sachet changed the title Trailing slashes should route to the same place Mounted routes should not require a trailing slash Aug 20, 2019
@schloerke schloerke changed the title Mounted routes should not require a trailing slash Add mounted router / route to mounted location if no definition exists Aug 20, 2019
@schloerke schloerke removed the duplicate Already catalogued label Aug 20, 2019
@schloerke
Copy link
Collaborator

Notes: Should be done at the same time as trailing slashes

@blairj09
Copy link
Collaborator

blairj09 commented Dec 29, 2020

This could be accomplished if there was a way to opt-in to redirects on mounted routers:

options(plumber.mount.redirect = TRUE)

or

pr_mount(..., redirect = TRUE)

Example redirect:

pr_get("/foo", function(res) {
      res$status <- 307
      res$setHeader(name = "Location",
                    value = "/foo/")
      res
    })

@schloerke schloerke modified the milestones: should address, v1.1.0 Dec 30, 2020
@schloerke schloerke added the Fixed in PR A PR has implemented a fix for this Issue label Jan 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
difficulty: advanced Best for maintainers to address effort: medium < 3 days of work Fixed in PR A PR has implemented a fix for this Issue priority: low Will be fixed eventually theme: trailing slash type: enhancement Adds a new, backwards-compatible feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants