rstudio / plumber

Turn your R code into a web API.
https://www.rplumber.io
Other
1.39k stars 256 forks source link

Improved documentation on specifying param type #755

Open JosiahParry opened 3 years ago

JosiahParry commented 3 years ago

The plumber documentation on specifying input parameters is limited (see https://www.rplumber.io/articles/annotations.html#more-details-on-param).

Take the following example:

#* @param double:dbl
#* @get /test
function(double = 0) {
  class(double)
}

When using the swagger interface with double set to 0.5 the response is

[
  "character"
]

The only way to achieve the type specification is with an override from R

#* @param double:dbl
#* @get /override
function(double = 0) {
  class(as.double(0))
}

Which returns

[
  "numeric"
]

My intention with this example is that, given the documentation, it is not clear how to specify input types. I am using plumber v1.0.0

meztez commented 3 years ago

I agree. When I first started using plumber I was confused why plumber did not do this conversion.

Plumber will do the conversion in the case of dynamic path parameter.

#* @get /test/<double:dbl>
function(double = 0) {
  class(double)
}

which you call using host/test/0.5

But not in the case of query parameters, query parameters will always be sent back to the API as character. The only reason to define the type in this case is for the OpenAPI document generation.

#* @param double:dbl
#* @get /test
function(double = 0) {
  class(double)
}

I think I had this explained to me at some point. I'll take a look if I can find it. Maybe specify it in the documentation.

JosiahParry commented 1 year ago

This has come up again in a (inflamitory, imo 😉 ) blog post.

They make a valid point that the param type annotations are not checked or enforced. Reprex

library(plumber)

#* @param n:int
#* @get /types
function(n) {
  n  * 2L
}

In browser make a request with any valid n and the response in the R client is <simpleError in n * 2: non-numeric argument to binary operator>

Assertions and coersions do not work well here becuase as.integer("asdf") returns NA_integer_ which, if passed into a type check for stopifnot(is.integer(as.integer(n)) would return TRUE.

JosiahParry commented 1 year ago

If you can point me where to most likely more a change here, I'd be happy to submit a PR.

schloerke commented 1 year ago

@JosiahParry Check out vignettes/annotations.Rmd. Thank you!

JosiahParry commented 1 year ago

Yes, the important part here is:

Query parameters currently need to be explicitly converted as they are pushed as is (character) to block expression. Only dynamic route parameters are converted to specified @param type before being pushed to block expression.

The following endpoint will correctly do type conversion:

#* @param n
#* @get /type/<n:int>
function(n) {
  n  * 2L
}

How can we get this same behavior for non-dynamic routes?

schloerke commented 1 year ago

@JosiahParry I believe this PR has it done: https://github.com/rstudio/plumber/pull/905 .

I'll start carving away time to get this through.