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,8 @@ 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.  Users should define their route to only use `req`, `res`, and/or `...` to avoid multiple matchings. (#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,47 @@ 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",
+      "\nthe route's function signature be `function(req, res)`",
+      "\nand 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)