Pass in req and res directly to PlumberStep$exec(req, res)

NAMESPACE

@@ -102,5 +102,6 @@ importFrom(jsonlite,toJSON)
 importFrom(lifecycle,deprecated)
 importFrom(magrittr,"%>%")
 importFrom(stats,runif)
+importFrom(stats,setNames)
 importFrom(utils,installed.packages)
 importFrom(webutils,parse_multipart)

NEWS.md

@@ -10,14 +10,21 @@ plumber 1.0.0
 
 * Added support for `#' @plumber` tag to gain programmatic access to the `plumber` router via `function(pr) {....}`. See `system.file("plumber/06-sessions/plumber.R", package = "plumber")` and how it adds cookie support from within `plumber.R`. (@meztez and @blairj09, #568)
 
+* Added `plumb_api()` for standardizing where to locate (`inst/plumber`) and how to run (`plumb_api(package, name)`) plumber apis inside an R package. To view the available Plumber APIs, call `available_apis()`. (#631)
+
+* Improved argument handling in Plumber Endpoint route definitions. 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. Improvements include:
+  * The value supplied to `req` and `res` arguments in a route definition are now _always_ Plumber request and response objects. In the past, this was not guaranteed. (#666, #637)
+  * To assist with conflicts in argument names deriving from different locations, `req$argsQuery`, `req$argsPath`, and `req$argsBody` have been added to access query, path, and `req$body` parameters, respectively. For this reason, we suggest defining routes with only `req` and `res` (i.e., `function(req, res) {}`) and accessing argument(s) under these new fields to avoid naming conflicts. (#637)
+  * An error is no longer thrown if multiple arguments are matched to an Plumber Endpoint route definition. Instead, Plumber now retains the first named argument according to the highest priority match (`req$argsQuery` is 1st priority, then `req$argsPath`, then `req$argsBody`). (#666)
+  * Unnamed elements that are added to `req$args` by filters or creating `req$argsBody` will no longer throw an error. They will only be passed through via `...` (#666)
+
+
 * 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$argsBody` to access path, query, and postBody parameters respectively.
+  Use `req$argsQuery`, `req$argsPath`, and `req$argsBody` to access path, query, and body 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)
 
-* Added `plumb_api()` for standardizing where to locate (`inst/plumber`) and how to run (`plumb_api(package, name)`) plumber apis inside an R package. To view the available Plumber APIs, call `available_apis()`. (#631)
-
 
 #### OpenAPI
 
@@ -118,6 +125,8 @@ plumber 1.0.0
 
 * `options(plumber.debug)` is not set anymore when running the plumber application. Instead retrieve the debug value using `$getDebug()` on the Plumber router directly. Ex: `function(req, res) { req$pr$getDebug() }`. (#639)
 
+* `PlumberEndpoint`'s method `$exec()` now has a shape of `$exec(req, res)` (vs `$exec(...)`).  This allows for fine tune control over the arguments being sent to the endpoint function.
+
 ### Deprecations
 
 * Shorthand serializers are now deprecated. `@html`, `@json`, `@png`, `@jpeg`, `@svg` should be replaced with the `@serializer` syntax. Ex: `@serializer html` or `@serializer jpeg` (#630)

R/parse-body.R

@@ -212,7 +212,7 @@ register_parser <- function(
     }
 
     create_list <- function(names) {
-      stats::setNames(
+      setNames(
         replicate(
           length(names),
           parser_function),
@@ -280,7 +280,7 @@ make_parser <- function(aliases) {
       aliases <- setdiff(registered_parsers(), c("all", "none"))
     }
     # turn aliases into a named list with empty values
-    aliases <- stats::setNames(
+    aliases <- setNames(
       replicate(length(aliases), {list()}),
       aliases
     )

R/parse-query.R

@@ -3,8 +3,8 @@ 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$argsQuery <- args
     req$.internal$queryStringHandled <- TRUE
   }
   forward()
@@ -155,6 +155,7 @@ extractPathParams <- function(def, path){
 
 #' combine args that share the same name
 #' @noRd
+#' @importFrom stats setNames
 combine_keys <- function(obj, type) {
 
   keys <- names(obj)

R/plumber-step.R

@@ -56,21 +56,24 @@ PlumberStep <- R6Class(
       }
     },
     #' @description step execution function
-    #' @param ... additional arguments for step execution
-    exec = function(...) {
-      allArgs <- list(...)
-      args <- getRelevantArgs(allArgs, plumberExpression=private$func)
-
+    #' @param req,res Request and response objects created by a Plumber request
+    exec = function(req, res) {
       hookEnv <- new.env()
 
+      args <- c(
+        # add in `req`, `res` as they have been removed from `req$args`
+        list(req = req, res = res),
+        req$args
+      )
       preexecStep <- function(...) {
-        private$runHooks("preexec", c(list(data = hookEnv), allArgs))
+        private$runHooks("preexec", c(list(data = hookEnv), args))
       }
       execStep <- function(...) {
-        do.call(private$func, args, envir = private$envir)
+        relevant_args <- getRelevantArgs(args, plumberExpression=private$func)
+        do.call(private$func, relevant_args, envir = private$envir)
       }
       postexecStep <- function(value, ...) {
-        private$runHooks("postexec", c(list(data = hookEnv, value = value), allArgs))
+        private$runHooks("postexec", c(list(data = hookEnv, value = value), args))
       }
       runSteps(
         NULL,
@@ -102,21 +105,6 @@ PlumberStep <- R6Class(
 
 # @param positional list with names where they were provided.
 getRelevantArgs <- function(args, plumberExpression) {
-  if (length(args) == 0) {
-    unnamedArgs <- NULL
-  } else if (is.null(names(args))) {
-    unnamedArgs <- 1:length(args)
-  } else {
-    unnamedArgs <- which(names(args) == "")
-  }
-
-  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: \"",
-         paste0(names(args), collapse=","), "\"")
-  }
-
   # Extract the names of the arguments this function supports.
   fargs <- names(formals(eval(plumberExpression)))
 
@@ -125,6 +113,12 @@ getRelevantArgs <- function(args, plumberExpression) {
     return(list())
   }
 
+  # fast return
+  # also works with unnamed arguments
+  if (identical(fargs, "...")) {
+    return(args)
+  }
+
   # If only req and res are found in function definition...
   # Only call using the first matches of req and res.
   #   This allows for body content to have `req` and `res` named arguments and not duplicated values which cause issues.
@@ -141,24 +135,20 @@ getRelevantArgs <- function(args, plumberExpression) {
     return(ret)
   }
 
-  if (!"..." %in% fargs) {
+  # The remaining code MUST work with unnamed arguments
+  # If there is no `...`, then the unnamed args will not be in `fargs` and will be removed
+  # If there is `...`, then the unnamed args will not be in `fargs` and will be passed through
+
+  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
+  # dedupe matched formals
   arg_names <- names(args)
-  matched_arg_names <- arg_names[arg_names %in% fargs]
-  duplicated_matched_arg_names <- duplicated(matched_arg_names)
-
-  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$argsBody`, or `req$argsQuery`."
-    )
-  }
+  is_farg <- arg_names %in% fargs
+  # keep only the first matched formal argument (and all other non-`farg` params)
+  args <- args[(is_farg & !duplicated(arg_names)) | (!is_farg)]
 
   args
 }

R/plumber.R

@@ -578,19 +578,6 @@ Plumber <- R6Class(
         return(NULL)
       }
 
-      # 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
-        }
-      }
-
       path <- req$PATH_INFO
       makeHandleStep <- function(name) {
         function(...) {
@@ -614,18 +601,20 @@ Plumber <- R6Class(
           req$argsBody <- req_body_args(req)
 
           req$args <- c(
-            # 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`
+            # (does not contain req or res)
+            # will contain all args added in filters
+            # `req$argsQuery` is available, but already absorbed into `req$args`
             req$args,
-            # path params
+            # path is more important than query
             req$argsPath,
-            # body params
+            # body is added last
             req$argsBody
           )
 
-          return(do.call(h$exec, req$args))
+          # req$args will be filled inside `h$exec()`
+          return(
+            h$exec(req, res)
+          )
         }
       }
 
@@ -647,7 +636,7 @@ Plumber <- R6Class(
 
           filterExecStep <- function(...) {
             resetForward()
-            do.call(fi$exec, req$args)
+            fi$exec(req, res)
           }
           postFilterStep <- function(fres, ...) {
             if (hasForwarded()) {
@@ -656,7 +645,7 @@ Plumber <- R6Class(
             }
             # forward() wasn't called, presumably meaning the request was
             # handled inside of this filter.
-            if (!is.null(fi$serializer)){
+            if (!is.null(fi$serializer)) {
               res$serializer <- fi$serializer
             }
             return(fres)
@@ -726,7 +715,7 @@ Plumber <- R6Class(
       req$.internal <- new.env()
 
       res <- PlumberResponse$new(private$default_serializer)
-      req$args <- list(req = req, res = res)
+      req$args <- list()
 
       # maybe return a promise object
       self$serve(req, res)

tests/testthat/files/serializer.R

@@ -13,7 +13,7 @@ function(req, res){
 }
 
 #* @filter foo2
-function(req, res, type=""){
+function(req, res, type="") {
   if (type == "json"){
     res$serializer <- plumber:::serializer_json()
   }

tests/testthat/test-endpoint.R

@@ -7,7 +7,7 @@ test_that("Endpoints execute in their environment", {
   foo <- parse(text="foo <- function(){ a }")
 
   r <- PlumberEndpoint$new('verb', 'path', foo, env, 1:2)
-  expect_equal(r$exec(), 5)
+  expect_equal(r$exec(req = list(), res = 2), 5)
 })
 
 test_that("Missing lines are ok", {
@@ -19,58 +19,57 @@ test_that("Missing lines are ok", {
 test_that("Endpoints are exec'able with named arguments.", {
   foo <- parse(text="foo <- function(x){ x + 1 }")
   r <- PlumberEndpoint$new('verb', 'path', foo, new.env(parent = globalenv()))
-  expect_equal(r$exec(x=3), 4)
+  expect_equal(r$exec(req = list(args = list(x = 3)), res = 20), 4)
 })
 
-test_that("Unnamed arguments error", {
-  foo <- parse(text="foo <- function(){ 1 }")
+test_that("Unnamed arguments do not throw an error", {
+  foo <- parse(text="foo <- function(){ -1 }")
   r <- PlumberEndpoint$new('verb', 'path', foo, new.env(parent = globalenv()))
-  expect_error(r$exec(3))
+  expect_equal(r$exec(req = list(args = list(3)), res = 100), -1)
 
-  foo <- parse(text="foo <- function(x, ...){ x + 1 }")
+  foo <- parse(text="foo <- function(req, res, x, ...){ x + sum(1, ...) }")
   r <- PlumberEndpoint$new('verb', 'path', foo, new.env(parent = globalenv()))
-  expect_error(r$exec(x=1, 3))
+  expect_equal(r$exec(req = list(args = list(x = 3, 1)), res = 100), 5)
 })
 
 test_that("Ellipses allow any named args through", {
-  foo <- parse(text="function(...){ sum(unlist(list(...))) }")
+  foo <- parse(text="function(req, res, ...){ sum(unlist(list(...))) }")
   r <- PlumberEndpoint$new('verb', 'path', foo, new.env(parent = globalenv()))
-  expect_equal(r$exec(a=1, b=2, c=3), 6)
+  expect_equal(r$exec(req = list(args = list(a=1, b=2, c=3)), res = 20), 6)
 
-  lapply(
-    c(
+  for ( txt in c(
       "", # with no req or res formals
       "req, res, "
-    ),
-    function(txt) {
-      foo <- parse(text=paste0("function(", txt, "...){ list(...) }"))
-      r <- PlumberEndpoint$new('verb', 'path', foo, new.env(parent = globalenv()))
-      expect_equal(r$exec(a="aa", b="ba"), list(a="aa", b="ba"))
-      expect_equal(r$exec(a="aa1", a="aa2", b = "ba"), list(a="aa1", a="aa2", b = "ba"))
-
-      foo <- parse(text=paste0("function(", txt, "a, ...){ list(a, ...) }"))
-      r <- PlumberEndpoint$new('verb', 'path', foo, new.env(parent = globalenv()))
-      expect_error(r$exec(a="aa1", a="aa2", b = "ba"), "duplicated matching formal arguments")
-    }
-  )
+  )) {
+    foo <- parse(text=paste0("function(", txt, "...){ ret <- list(...); ret[!names(ret) %in% c('req', 'res')] }"))
+    r <- PlumberEndpoint$new('verb', 'path', foo, new.env(parent = globalenv()))
+    expect_equal(r$exec(req = list(args = list(a="aa", b="ba")), res = 2), list(a="aa", b="ba"))
+    expect_equal(r$exec(req = list(args = list(a="aa1", a="aa2", b = "ba")), res = 2), list(a="aa1", a="aa2", b = "ba"))
+
+    foo <- parse(text=paste0("function(", txt, "a, ...){ ret <- list(a = a, ...); ret[!names(ret) %in% c('req', 'res')] }"))
+    r <- PlumberEndpoint$new('verb', 'path', foo, new.env(parent = globalenv()))
+    expect_equal(r$exec(req = list(args = list(a="aa1", a="aa2", b = "ba")), res = 2), list(a = "aa1", b = "ba"))
+  }
 })
 
 test_that("If only req and res are defined, duplicated arguments do not throw an error", {
+  full_req <- list(args = list(req = 1, req = 2, res = 3, res = 4))
+  full_res <- list(args = list(res = 1, res = 2, res = 3, res = 4))
   foo <- parse(text="function(req){ req }")
   r <- PlumberEndpoint$new('verb', 'path', foo, new.env(parent = globalenv()))
-  expect_equal(r$exec(req = 1, req = 2, res = 3, res = 4), 1)
+  expect_equal(r$exec(req = full_req, res = full_res), full_req)
 
   foo <- parse(text="function(res){ res }")
   r <- PlumberEndpoint$new('verb', 'path', foo, new.env(parent = globalenv()))
-  expect_equal(r$exec(req = 1, req = 2, res = 3, res = 4), 3)
+  expect_equal(r$exec(req = full_req, res = full_res), full_res)
 
   foo <- parse(text="function(req, res){ list(req, res) }")
   r <- PlumberEndpoint$new('verb', 'path', foo, new.env(parent = globalenv()))
-  expect_equal(r$exec(req = 1, req = 2, res = 3, res = 4), list(1,3))
+  expect_equal(r$exec(req = full_req, res = full_res), list(full_req, full_res))
 
-  foo <- parse(text="function(req, res, ...){ -1 }")
+  foo <- parse(text="function(req, res, ...){ list(req, res, ...) }")
   r <- PlumberEndpoint$new('verb', 'path', foo, new.env(parent = globalenv()))
-  expect_error(r$exec(req = 1, req = 2, res = 3, res = 4), "duplicated matching formal arguments")
+  expect_equal(r$exec(req = full_req, res = full_res), list(full_req, full_res))
 })
 
 test_that("Programmatic endpoints work", {