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

plumber/NEWS.md

Lines 150 to 154 in 6d310b3

	* An error will be thrown if multiple arguments are matched to an Plumber Endpoint route definition.
	While it is not required, it is safer to define routes to only use `req` and `res` when there is a possiblity to have multiple arguments match a single parameter name.
	Use `req$argsPath`, `req$argsQuery`, and `req$argsPostBody` to access path, query, and postBody parameters respectively.
	See `system.file("plumber/17-arguments/plumber.R", package = "plumber")` to view an example with expected output and `plumb_api("plumber", "17-arguments")` to retrieve the api.
	(#637)

[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

[thomasp85]

It still seems problematic that query parameters gets prioritised over path arguments (especially as it is contrary to the docs)

However, since these are populated by a filter they cannot be treated separately. The solution would be to have all arguments added by filters hove lowest priority but I don't think that's a better solution as you may want to override an argument with a filter.

[schloerke]

I'd be up for changing when/how query args are parsed. They are such a pain having them done differently than the body or path arguments.

I'd be up for always parsing query arguments to make our lives consistent/easier.

OR, we move the body and parser logic out to a filter. But that doesn't seem to make our lives easier.