Parameter collisions don't err (but should?)

[mmuurr]

Issue created as a result of this Posit Community thread.

In spinning-up an API, I realized I had some lingering questions after reading the docs. One situation I was trying to understand is exactly how Plumber would handle scenarios where:

1. The API designer has used <x> as part of a dynamic path,
2. The API caller may (on their own merit) include x as a query string.
3. The API caller may (on their own merit) include x as part of a parsed request body.

In Routing & Input, there's this paragraph:

If multiple parameters are matched to the endpoint formals, an error will be thrown. Due to the nature of how multiple values can be matched to the same argument, it is recommended that POST endpoints have a function definition that only accepts the formals req, res, and .... If the endpoint arguments are to be processed like a list, they are available at req$argsBody, with all arguments at req$args. req$args is a combination of list(req = req, res = res), req$argsPath, req$argsBody, and req$argsQuery.

Here's a (very) simple endpoint definition:

#* @get /foo/<x>
#* @parser json
#* @serializer text
function(req, x) {
  str(x)  ## what's x?
  print(req)  ## inspect the request
  x
}

The request will be a GET to /foo/bar?x=baz with body {"x":"qux"}.

No error (the actual issue)

The response is not an error, but rather "baz" is returned, suggesting the query string takes precedence. I assume this is not necessarily the intended behavior but rather simply a byproduct of the order in which req$args is stitched together from the argsQuery, argsPath and argsBody (which, BTW, is not the order mentioned in that doc paragraph) ... here's req$args:

 $args
 $args$x
 [1] "baz"

 $args$x
 [1] "bar"

 $args$x
 [1] "qux"

A related issue/challenge?

In thinking about a safe way to deal with this, a potential related issue that comes up. I think(?) the most logical approach one would take to handle that scenario safely is something like:

1. Add @preempt queryString to the annotations (to prevent "baz" from trumping "bar", above)
2. Change to @parser text to force explicit handling of body data.

The problem with those two steps is that @preempt queryString then also preempts the body parser. This is, of course, due to the ordered defaultPlumberFilters, but figuring this out requires a decent amount of reverse engineering & {plumber} source spelunking ... more than most R programmers are comfortable with, I think(?).

There doesn't seem to be an easy way to deal with this dynamic path collision issue, save for: (i) creating a new router and explicitly excluding queryString from the filters (i.e. not using defaultPlumberFilters) then (ii) adding an explicit query string parsing step (i.e. webutils::parse_query()) to any endpoint expecting a query string.

I believe this makes dynamic path parameters very challenging to use for any sort of public-facing API, as they're fragile and it takes sophistication beyond what's described in the docs for how to safely manage parameter name collision. Maybe I'm missing some additional docs on how to best do this without these additional steps (i, ii) mentioned above?

[mmuurr]

I've also just noted from the Posit Community thread that the documented error may only be thrown if the request method is a POST? That's useful to help the API auther from making some bad naming choices, but it doesn't help when the API caller decides to send query strings and payloads on other requests where the API author didn't expect them.

[mmuurr]

Ah, it has just occurred to me that perhaps the easiest way to handle dynamic paths safely is to:

1. Change the handler signature to just function(req) (i.e. remove x from the formals).
2. Ignore req$args and instead deal with req$argsPath, req$argsQuery, and req$argsPath explicitly.

Minimally it might be nice to add this a possible suggestion in the documentation re: params?

[meztez]

Add this part to the doc https://github.com/rstudio/plumber/blob/6d310b30e1d7c7340963c224d6bab1638a99bd01/NEWS.md?plain=1#L150-L154

[meztez]

@mmuurr Does the doc update makes it clearer what is happening?

https://www.rplumber.io/articles/routing-and-input.html#named-parameters-collision-note