Throw error if multiple matched endpoint formals are found

NAMESPACE

@@ -23,11 +23,11 @@ export(include_rmd)
 export(options_plumber)
 export(parser_csv)
 export(parser_feather)
+export(parser_form)
 export(parser_json)
 export(parser_multi)
 export(parser_none)
 export(parser_octet)
-export(parser_query)
 export(parser_rds)
 export(parser_read_file)
 export(parser_text)

NEWS.md

@@ -89,7 +89,7 @@ both UIs integration are available from https://github.com/meztez/rapidoc/ and h
 
 * Added yaml support, serializer and parser. (@meztez, #556)
 
-* Added parsers: `parser_csv()`, `parser_json()`, `parser_multi()`, `parser_octet()`, `parser_query()`, `parser_rds()`, `parser_text()`, `parser_tsv()`, `parser_yaml()`, `parser_none()`, and pseudo `"all"` (#584)
+* Added parsers: `parser_csv()`, `parser_json()`, `parser_multi()`, `parser_octet()`, `parser_form()`, `parser_rds()`, `parser_text()`, `parser_tsv()`, `parser_yaml()`, `parser_none()`, and pseudo `"all"` (#584)
 
 * Added `serializer_csv()` (@pachamaltese, #520)
 
@@ -147,6 +147,12 @@ both UIs integration are available from https://github.com/meztez/rapidoc/ and h
 
 * Date response header is now supplied by httpuv and not plumber. Fixes non standard date response header issues when using different locales. (@shrektan, #319, #380)
 
+* 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)
+
 
 plumber 0.4.6
 --------------------------------------------------------------------------------

R/parse-body.R

@@ -51,14 +51,14 @@ looks_like_json <- local({
 })
 parser_picker <- function(content_type, first_byte, filename = NULL, parsers = NULL) {
 
-  # parse as a query string
+  # parse as json or a form
   if (length(content_type) == 0) {
     # fast default to json when first byte is 7b (ascii {)
     if (looks_like_json(first_byte)) {
       return(parsers$alias$json)
     }
 
-    return(parsers$alias$query)
+    return(parsers$alias$form)
   }
 
   # remove trailing content type information
@@ -87,9 +87,9 @@ parser_picker <- function(content_type, first_byte, filename = NULL, parsers = N
     return(parsers$regex[[which(fpm)[1]]])
   }
 
-  # query string
+  # parse as a form submission
   if (is.null(filename)) {
-    return(parsers$alias$query)
+    return(parsers$alias$form)
   }
 
   # octet
@@ -234,7 +234,7 @@ registered_parsers <- function() {
 # ' make_parser(list(json = list(simplifyVector = FALSE), rds = list()))
 # '
 # ' # default plumber parsers
-# ' make_parser(c("json", "query", "text", "octet", "multi"))
+# ' make_parser(c("json", "form", "text", "octet", "multi"))
 make_parser <- function(aliases) {
   if (inherits(aliases, "plumber_parsed_parsers")) {
     return(aliases)
@@ -316,7 +316,7 @@ make_parser <- function(aliases) {
 #' non-default behavior.
 #'
 #' Parsers are optional. When unspecified, only the [parser_json()],
-#' [parser_octet()], [parser_query()] and [parser_text()] are available.
+#' [parser_octet()], [parser_form()] and [parser_text()] are available.
 #' You can use `@parser parser` tag to activate parsers per endpoint.
 #' Multiple parsers can be activated for the same endpoint using multiple `@parser parser` tags.
 #'
@@ -326,7 +326,7 @@ make_parser <- function(aliases) {
 #' See [registered_parsers()] for a list of registered parsers.
 #'
 #' @param ... parameters supplied to the appropriate internal function
-#' @describeIn parsers Query string parser
+#' @describeIn parsers Form query string parser
 #' @examples
 #' \dontrun{
 #' # Overwrite `text/json` parsing behavior to not allow JSON vectors to be simplified
@@ -338,7 +338,7 @@ make_parser <- function(aliases) {
 #' pr$handle("GET", "/upload", function(rds) {rds}, parsers = c("multi", "rds"))
 #' }
 #' @export
-parser_query <- function() {
+parser_form <- function() {
   parser_text(parseQS)
 }
 
@@ -502,7 +502,7 @@ register_parsers_onLoad <- function() {
   register_parser("json",    parser_json,    fixed = c("application/json", "text/json"))
   register_parser("multi",   parser_multi,   fixed = "multipart/form-data")
   register_parser("octet",   parser_octet,   fixed = "application/octet-stream")
-  register_parser("query",   parser_query,   fixed = "application/x-www-form-urlencoded")
+  register_parser("form",    parser_form,   fixed = "application/x-www-form-urlencoded")
   register_parser("rds",     parser_rds,     fixed = "application/rds")
   register_parser("feather", parser_feather, fixed = "application/feather")
   register_parser("text",    parser_text,    fixed = "text/plain", regex = "^text/")

R/parse-query.R

@@ -3,6 +3,7 @@ queryStringFilter <- function(req){
   if (is.null(handled) || handled != TRUE) {
     qs <- req$QUERY_STRING
     args <- parseQS(qs)
+    req$argsQuery <- args
     req$args <- c(req$args, args)
     req$.internal$queryStringHandled <- TRUE
   }
@@ -17,8 +18,8 @@ parseQS <- function(qs){
   }
 
   # Looked into using webutils::parse_query()
-  # Currently not pursuing `parse_query` as it does not handle Encoding issues handled below
-  # (Combining keys are also not handled by `parse_query`)
+  # Currently not pursuing `webutils::parse_query` as it does not handle Encoding issues handled below
+  # (Combining keys are also not handled by `webutils::parse_query`)
 
   qs <- stri_replace_first_regex(qs, "^[?]", "")
   qs <- chartr("+", " ", qs)

R/plumber-step.R

@@ -101,16 +101,16 @@ PlumberStep <- R6Class(
 )
 
 # @param positional list with names where they were provided.
-getRelevantArgs <- function(args, plumberExpression){
-  if (length(args) == 0){
+getRelevantArgs <- function(args, plumberExpression) {
+  if (length(args) == 0) {
     unnamedArgs <- NULL
-  } else if (is.null(names(args))){
+  } else if (is.null(names(args))) {
     unnamedArgs <- 1:length(args)
   } else {
     unnamedArgs <- which(names(args) == "")
   }
 
-  if (length(unnamedArgs) > 0 ){
+  if (length(unnamedArgs) > 0 ) {
     stop("Can't call a Plumber function with unnammed arguments. Missing names for argument(s) #",
          paste0(unnamedArgs, collapse=", "),
          ". Names of argument list was: \"",
@@ -120,11 +120,46 @@ getRelevantArgs <- function(args, plumberExpression){
   # Extract the names of the arguments this function supports.
   fargs <- names(formals(eval(plumberExpression)))
 
-  if (!"..." %in% fargs){
+  if (length(fargs) == 0) {
+    # no matches
+    return(list())
+  }
+
+  # If only req and res are found in function definition...
+  # Only call using the first matches of req and res.
+  #   This allows for post body content to have `req` and `res` named arguments and not duplicated values cause issues.
+  if (all(fargs %in% c("req", "res"))) {
+    ret <- list()
+    # using `$` will retrieve the 1st occurance of req,res
+    # args$req <- req is used within `plumber$route()`
+    if ("req" %in% fargs) {
+      ret$req <- args$req
+    }
+    if ("res" %in% fargs) {
+      ret$res <- args$res
+    }
+    return(ret)
+  }
+
+  if (!"..." %in% fargs) {
     # Use the named arguments that match, drop the rest.
     args <- args[names(args) %in% fargs]
   }
 
+  # for all args, check if they are duplicated
+  arg_names <- names(args)
+  matched_arg_names <- arg_names[arg_names %in% fargs]
+  duplicated_matched_arg_names <- duplicated(matched_arg_names, fromLast = TRUE)
+
+  if (any(duplicated_matched_arg_names)) {
+    stop(
+      "Can't call a Plumber function with duplicated matching formal arguments: ",
+      paste0(unique(matched_arg_names[duplicated_matched_arg_names]), collapse = ", "),
+      "\nPlumber recommends that the route's function signature be `function(req, res)`",
+      "\nand to access arguments via `req$args`, `req$argsPath`, `req$argsPostBody`, or `req$argsQuery`."
+    )
+  }
+
   args
 }
 

R/plumber.R

@@ -150,7 +150,7 @@ plumber <- R6Class(
       private$maxSize <- getOption('plumber.maxRequestSize', 0) #0  Unlimited
       self$setSerializer(serializer_json())
       # Default parsers to maintain legacy features
-      self$set_parsers(c("json", "query", "text", "octet", "multi"))
+      self$set_parsers(c("json", "form", "text", "octet", "multi"))
       self$setErrorHandler(defaultErrorHandler())
       self$set404Handler(default404Handler)
       self$set_ui()
@@ -646,17 +646,20 @@ plumber <- R6Class(
         return(NULL)
       }
 
-      # Get args out of the query string, + req/res
-      args <- list()
-      if (!is.null(req$args)) {
-        args <- req$args
+      # These situations should NOT happen as req,res are set in self$call()
+      # For testing purposes, these checks are added
+      if (is.null(req$args)) {
+        req$args <- list(req = req, res = res)
+      } else {
+        if (is.null(req$args$req)) {
+          req$args$req <- req
+        }
+        if (is.null(req$args$res)) {
+          req$args$res <- res
+        }
       }
-      args$res <- res
-      args$req <- req
 
-      req$args <- args
       path <- req$PATH_INFO
-
       makeHandleStep <- function(name) {
         function(...) {
           resetForward()
@@ -673,11 +676,21 @@ plumber <- R6Class(
             } else {
               private$default_parsers
             }
+          req$argsPath <- h$getPathParams(path)
+          req$argsPostBody <- postbody_parser(req, parsers)
+
           req$args <- c(
-            h$getPathParams(path),
+            # req, res
+            # query string params and any other `req$args`
+            ## Query string params have been added to `req$args`.
+            ## At this point, can not include both `req,res` and `req$argsQuery`. So using `req$args`
             req$args,
-            postbody_parser(req, parsers)
+            # path params
+            req$argsPath,
+            # post body params
+            req$argsPostBody
           )
+
           return(do.call(h$exec, req$args))
         }
       }
@@ -776,11 +789,11 @@ plumber <- R6Class(
     #' @details required for httpuv interface
     call = function(req) {
       # Set the arguments to an empty list
-      req$args <- list()
       req$pr <- self
       req$.internal <- new.env()
 
       res <- PlumberResponse$new(private$default_serializer)
+      req$args <- list(req = req, res = res)
 
       # maybe return a promise object
       self$serve(req, res)
@@ -828,7 +841,7 @@ plumber <- R6Class(
       private$default_serializer <- serializer
     },
     #' @description Sets the default parsers of the router.
-    #' @details Initialized to `c("json", "query", "text", "octet", "multi")`
+    #' @details Initialized to `c("json", "form", "text", "octet", "multi")`
     #' @template pr_set_parsers__parsers
     set_parsers = function(parsers) {
       private$default_parsers <- make_parser(parsers)

inst/plumber/17-arguments/plumber.R

@@ -0,0 +1,111 @@
+#* Works with query string arguments
+#* @serializer print
+#* @post /bad-practice/<a>/<b>
+function(a, b) {
+  list(a = a, b = b)
+}
+
+
+
+#* Can have conflicting path, query, and post body arguments
+#* @serializer print
+#* @post /good-practice/<a>/<b>
+function(req, res) {
+  list(
+    all = req$args,
+    query = req$argsQuery,
+    path = req$argsPath,
+    postBody = req$argsPostBody
+  )
+}
+
+
+# Test this api...
+
+### In an R session...
+### Run plumber API
+# plumb_api("plumber", "17-arguments") %>% print() %>% pr_run(port = 1234)
+
+
+### In a terminal...
+### Curl API
+
+## Fails (conflicting variable `a`)
+# curl --data '' '127.0.0.1:1234/bad-practice/1/2?a=3'
+# curl --data 'a=3' '127.0.0.1:1234/bad-practice/1/2'
+# curl --data 'a=4' '127.0.0.1:1234/bad-practice/1/2?a=3'
+
+## Works (but missing variable `d`)
+# curl --data '' '127.0.0.1:1234/bad-practice/1/2?d=3'
+#> $a
+#> [1] "1"
+#>
+#> $b
+#> [1] "2"
+
+## Works (but missing variable `d`)
+# curl --data 'd=3' '127.0.0.1:1234/bad-practice/1/2'
+#> $a
+#> [1] "1"
+#>
+#> $b
+#> [1] "2"
+
+
+
+## Safe endpoint setup
+# curl --data 'a=5&b=6' '127.0.0.1:1234/good-practice/3/4?a=1&b=2&d=10'
+#> $all
+#> $all$req
+#> <environment>
+#>
+#> $all$res
+#> <PlumberResponse>
+#>
+#> $all$a
+#> [1] "1"
+#>
+#> $all$b
+#> [1] "2"
+#>
+#> $all$d
+#> [1] "10"
+#>
+#> $all$a
+#> [1] "3"
+#>
+#> $all$b
+#> [1] "4"
+#>
+#> $all$a
+#> [1] "5"
+#>
+#> $all$b
+#> [1] "6"
+#>
+#>
+#> $query
+#> $query$a
+#> [1] "1"
+#>
+#> $query$b
+#> [1] "2"
+#>
+#> $query$d
+#> [1] "10"
+#>
+#>
+#> $path
+#> $path$a
+#> [1] "3"
+#>
+#> $path$b
+#> [1] "4"
+#>
+#>
+#> $postBody
+#> $postBody$a
+#> [1] "5"
+#>
+#> $postBody$b
+#> [1] "6"

man-roxygen/pr_set_parsers__parsers.R

@@ -21,5 +21,5 @@
 #' parsers = list(json = list(simplifyVector = FALSE), rds = list())
 #'
 #' # default plumber parsers
-#' parsers = c("json", "query", "text", "octet", "multi")
+#' parsers = c("json", "form", "text", "octet", "multi")
 #' ```