From a185f03c6620b195e145cd3df43c0ab4c5d497f3 Mon Sep 17 00:00:00 2001
From: Barret Schloerke <schloerke@gmail.com>
Date: Mon, 31 Aug 2020 17:44:56 -0400
Subject: [PATCH 01/17] PlumberEndpoint $exec() now takes req and res instead
 of `...`

---
 R/parse-query.R                   |  1 -
 R/plumber-step.R                  | 83 +++++++++++++++++--------------
 R/plumber.R                       | 34 +++----------
 man/PlumberStep.Rd                |  4 +-
 tests/testthat/files/serializer.R |  2 +-
 tests/testthat/test-endpoint.R    | 47 +++++++++--------
 tests/testthat/test-plumber.R     | 27 ++++++----
 tests/testthat/test-serializer.R  |  7 ++-
 8 files changed, 98 insertions(+), 107 deletions(-)

diff --git a/R/parse-query.R b/R/parse-query.R
index db83c71d6..9544fcd37 100644
--- a/R/parse-query.R
+++ b/R/parse-query.R
@@ -4,7 +4,6 @@ queryStringFilter <- function(req){
     qs <- req$QUERY_STRING
     args <- parseQS(qs)
     req$argsQuery <- args
-    req$args <- c(req$args, args)
     req$.internal$queryStringHandled <- TRUE
   }
   forward()
diff --git a/R/plumber-step.R b/R/plumber-step.R
index 711f150bc..bea6ff85b 100644
--- a/R/plumber-step.R
+++ b/R/plumber-step.R
@@ -56,21 +56,34 @@ 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(
+        # make sure req, res have priority
+        list(req = req, res = res),
+        # include any other args that users have provided
+        req$args,
+        # path is more important than query
+        req$argsPath,
+        # query is more important than body
+        req$argsQuery,
+        # body is added last
+        req$argsBody
+      )
+
+      # str(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 +115,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 +123,11 @@ getRelevantArgs <- function(args, plumberExpression) {
     return(list())
   }
 
+  # fast return
+  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,25 +144,33 @@ getRelevantArgs <- function(args, plumberExpression) {
     return(ret)
   }
 
-  if (!"..." %in% fargs) {
-    # Use the named arguments that match, drop the rest.
-    args <- args[names(args) %in% fargs]
+  if (is.null(names(args))) {
+    unnamedArgs <- seq_along(args)
+  } else {
+    unnamedArgs <- which(names(args) == "")
   }
 
-  # 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)
-
-  if (any(duplicated_matched_arg_names)) {
+  if (length(unnamedArgs) > 0 ) {
     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`."
+      "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 = ","), "\""
     )
   }
 
+
+  if (!("..." %in% fargs)) {
+    # Use the named arguments that match, drop the rest.
+    args <- args[names(args) %in% fargs]
+  }
+
+  # dedupe matched formals
+  arg_names <- names(args)
+  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
 }
 
diff --git a/R/plumber.R b/R/plumber.R
index 156915a19..1a127e395 100644
--- a/R/plumber.R
+++ b/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(...) {
@@ -612,19 +599,10 @@ Plumber <- R6Class(
           # `req_body_parser()` will also set `req$body` with the untouched body value
           req$argsBody <- req_body_parser(req, parsers)
 
-          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`
-            req$args,
-            # path params
-            req$argsPath,
-            # body params
-            req$argsBody
+          # req$args will be filled inside `h$exec()`
+          return(
+            h$exec(req, res)
           )
-
-          return(do.call(h$exec, req$args))
         }
       }
 
@@ -646,7 +624,7 @@ Plumber <- R6Class(
 
           filterExecStep <- function(...) {
             resetForward()
-            do.call(fi$exec, req$args)
+            fi$exec(req, res)
           }
           postFilterStep <- function(fres, ...) {
             if (hasForwarded()) {
@@ -655,7 +633,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)
@@ -725,7 +703,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)
diff --git a/man/PlumberStep.Rd b/man/PlumberStep.Rd
index 044184911..8b3101316 100644
--- a/man/PlumberStep.Rd
+++ b/man/PlumberStep.Rd
@@ -67,13 +67,13 @@ A new \code{PlumberStep} object
 \subsection{Method \code{exec()}}{
 step execution function
 \subsection{Usage}{
-\if{html}{\out{<div class="r">}}\preformatted{PlumberStep$exec(...)}\if{html}{\out{</div>}}
+\if{html}{\out{<div class="r">}}\preformatted{PlumberStep$exec(req, res)}\if{html}{\out{</div>}}
 }
 
 \subsection{Arguments}{
 \if{html}{\out{<div class="arguments">}}
 \describe{
-\item{\code{...}}{additional arguments for step execution}
+\item{\code{req, res}}{Request and response objects created by a Plumber request}
 }
 \if{html}{\out{</div>}}
 }
diff --git a/tests/testthat/files/serializer.R b/tests/testthat/files/serializer.R
index 4a47cd01d..e8dfebc47 100644
--- a/tests/testthat/files/serializer.R
+++ b/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()
   }
diff --git a/tests/testthat/test-endpoint.R b/tests/testthat/test-endpoint.R
index b22297a5e..f9cb7c09a 100644
--- a/tests/testthat/test-endpoint.R
+++ b/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,7 +19,7 @@ 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", {
@@ -29,48 +29,47 @@ test_that("Unnamed arguments error", {
 
   foo <- parse(text="foo <- function(x, ...){ x + 1 }")
   r <- PlumberEndpoint$new('verb', 'path', foo, new.env(parent = globalenv()))
-  expect_error(r$exec(x=1, 3))
+  expect_error(r$exec(req = list(args = list(1)), res = 20), 3)
 })
 
 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", {
diff --git a/tests/testthat/test-plumber.R b/tests/testthat/test-plumber.R
index 4e3852194..2f16aff7c 100644
--- a/tests/testthat/test-plumber.R
+++ b/tests/testthat/test-plumber.R
@@ -1,14 +1,19 @@
 context("Plumber")
 
+exec_endpoint <- function(pr, ep_pos, subset = 1) {
+  # This is a poor setup of `req` and `res`. But it works for testing purposes
+  pr$endpoints[[subset]][[ep_pos]]$exec(make_req(), PlumberResponse$new())
+}
+
 test_that("Endpoints are properly identified", {
   r <- pr(test_path("files/endpoints.R"))
   expect_equal(length(r$endpoints), 1)
   expect_equal(length(r$endpoints[[1]]), 5)
-  expect_equal(r$endpoints[[1]][[1]]$exec(), 5)
-  expect_equal(r$endpoints[[1]][[2]]$exec(), 5)
-  expect_equal(r$endpoints[[1]][[3]]$exec(), 10)
-  expect_equal(r$endpoints[[1]][[4]]$exec(), 12)
-  expect_equal(r$endpoints[[1]][[5]]$exec(), 14)
+  expect_equal(exec_endpoint(r, 1), 5)
+  expect_equal(exec_endpoint(r, 2), 5)
+  expect_equal(exec_endpoint(r, 3), 10)
+  expect_equal(exec_endpoint(r, 4), 12)
+  expect_equal(exec_endpoint(r, 5), 14)
 })
 
 test_that("Empty file is OK", {
@@ -20,7 +25,7 @@ test_that("The file is sourced in the envir", {
   r <- pr(test_path("files/in-env.R"))
   expect_equal(length(r$endpoints), 1)
   expect_equal(length(r$endpoints[[1]]), 2)
-  expect_equal(r$endpoints[[1]][[1]]$exec(), 15)
+  expect_equal(exec_endpoint(r, 1), 15)
 })
 
 test_that("Verbs translate correctly", {
@@ -129,11 +134,11 @@ test_that("The old roxygen-style comments work", {
   r <- pr(test_path("files/endpoints-old.R"))
   expect_equal(length(r$endpoints), 1)
   expect_equal(length(r$endpoints[[1]]), 5)
-  expect_equal(r$endpoints[[1]][[1]]$exec(), 5)
-  expect_equal(r$endpoints[[1]][[2]]$exec(), 5)
-  expect_equal(r$endpoints[[1]][[3]]$exec(), 10)
-  expect_equal(r$endpoints[[1]][[4]]$exec(), 12)
-  expect_equal(r$endpoints[[1]][[5]]$exec(), 14)
+  expect_equal(exec_endpoint(r, 1), 5)
+  expect_equal(exec_endpoint(r, 2), 5)
+  expect_equal(exec_endpoint(r, 3), 10)
+  expect_equal(exec_endpoint(r, 4), 12)
+  expect_equal(exec_endpoint(r, 5), 14)
 })
 
 test_that("routes can be constructed correctly", {
diff --git a/tests/testthat/test-serializer.R b/tests/testthat/test-serializer.R
index 723105716..3dc7c35e5 100644
--- a/tests/testthat/test-serializer.R
+++ b/tests/testthat/test-serializer.R
@@ -58,12 +58,11 @@ test_that("Overridden serializers apply on filters and endpoints", {
     req <- make_req("GET", "/something")
     res <- PlumberResponse$new(customSer())
     expect_equal(r$serve(req, res)$body, "CUSTOM")
-    res$serializer <- customSer()
 
-    req <- make_req("GET", "/something")
-    req$QUERY_STRING <- "type=json"
+    # have the filter set the serializer
+    req <- make_req("GET", "/something", qs = "type=json")
+    res <- PlumberResponse$new(NULL) # default to something that doesn't exist!
     expect_equal(r$serve(req, res)$body, jsonlite::toJSON(4))
-    res$serializer <- serializer_json()
 
     res <- PlumberResponse$new("json")
     expect_equal(r$serve(make_req("GET", "/another"), res)$body, "CUSTOM3")

From de0ba8e704ead83f513c81296a5aa5e7f8c319de Mon Sep 17 00:00:00 2001
From: Barret Schloerke <schloerke@gmail.com>
Date: Mon, 31 Aug 2020 17:45:06 -0400
Subject: [PATCH 02/17] Update NEWS.md

---
 NEWS.md | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/NEWS.md b/NEWS.md
index 53e190073..ec290c07e 100644
--- a/NEWS.md
+++ b/NEWS.md
@@ -12,12 +12,13 @@ plumber 1.0.0
 
 * 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$argsPath`, `req$argsQuery`, 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)
 
+* Named Plumber endpoint arguments are de-duplicated so that the endpoint function does not fail due to too many formal argument matchings.  To be sure you are getting all argument values, use `req$argsPath`, `req$argsQuery`, and `req$argsBody` to access path, query, and body parameters respectively.
 
 #### OpenAPI
 
@@ -118,6 +119,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)

From 11921b4579988379e9151f43431424be647c537a Mon Sep 17 00:00:00 2001
From: Barret Schloerke <schloerke@gmail.com>
Date: Mon, 31 Aug 2020 17:45:14 -0400
Subject: [PATCH 03/17] Update routing-and-input.Rmd

---
 vignettes/routing-and-input.Rmd | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/vignettes/routing-and-input.Rmd b/vignettes/routing-and-input.Rmd
index 47af93e8d..d9e6c999f 100644
--- a/vignettes/routing-and-input.Rmd
+++ b/vignettes/routing-and-input.Rmd
@@ -236,7 +236,7 @@ Name | Example | Description
 `argsBody` | `list(a=1,b=2)` | The parsed body output. Typically this is the same as `req$body` except when type is `"multipart/*"`.
 `argsPath` | `list(c=3,d=4)` | The values of the path arguments.
 `argsQuery` | `list(e=5,f=6)` | The parsed query string output.
-`args` | `list(req=req,res=res,e=5,f=6,c=3,d=4,a=1,b=2)` | In a route, the combined arguments of `list(req = req, res = res)`, `argsQuery`, `argsPath`, and `argsBody`. In a filter, `req$args` only contains `argsQuery` as the route information will not have been processed yet.
+`args` | `list(req=req,res=res,e=5,f=6,c=3,d=4,a=1,b=2)` | In a route, the combined arguments of `list(req = req, res = res)`, `req$args` (added by filters), `req$argsQuery`, `req$argsPath`, and `req$argsBody`. In a filter, `req$args` is initialized to an empty list, so when processing filters `req$args` will only contain arguments set by previously processed filters as the route information will not have been processed yet.
 `QUERY_STRING` | `"?a=123&b=abc"` | The query-string portion of the HTTP request
 `REMOTE_ADDR` | `"1.2.3.4"` | The IP address of the client making the request
 `REMOTE_PORT` | `"62108"` | The client port from which the request originated

From 3ad9f53bc59e46c02cf47cb18235305763b72e31 Mon Sep 17 00:00:00 2001
From: Barret Schloerke <schloerke@gmail.com>
Date: Mon, 31 Aug 2020 18:30:24 -0400
Subject: [PATCH 04/17] Add information into `req$args` incrementally.

This avoids circular dependencies when setting in filters and reading within the route.

This also matches historical behavior. So it's a welcome change.
---
 NAMESPACE        |  1 +
 NEWS.md          |  4 ++--
 R/parse-body.R   |  4 ++--
 R/parse-query.R  |  2 ++
 R/plumber-step.R | 11 ++---------
 R/plumber.R      | 11 +++++++++++
 6 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/NAMESPACE b/NAMESPACE
index 7be1facec..e9ca55f41 100644
--- a/NAMESPACE
+++ b/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)
diff --git a/NEWS.md b/NEWS.md
index ec290c07e..5714184b3 100644
--- a/NEWS.md
+++ b/NEWS.md
@@ -12,13 +12,13 @@ plumber 1.0.0
 
 * 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 body 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)
 
-* Named Plumber endpoint arguments are de-duplicated so that the endpoint function does not fail due to too many formal argument matchings.  To be sure you are getting all argument values, use `req$argsPath`, `req$argsQuery`, and `req$argsBody` to access path, query, and body parameters respectively.
+* Named Plumber endpoint arguments are de-duplicated so that the endpoint function does not fail due to too many formal argument matchings.  To be sure you are getting all argument values, use `req$argsQuery`, `req$argsPath`, and `req$argsBody` to access path, query, and body parameters respectively.
 
 #### OpenAPI
 
diff --git a/R/parse-body.R b/R/parse-body.R
index f53b646f1..a1aff7476 100644
--- a/R/parse-body.R
+++ b/R/parse-body.R
@@ -201,7 +201,7 @@ register_parser <- function(
     }
 
     create_list <- function(names) {
-      stats::setNames(
+      setNames(
         replicate(
           length(names),
           parser_function),
@@ -269,7 +269,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
     )
diff --git a/R/parse-query.R b/R/parse-query.R
index 9544fcd37..a01cf9a00 100644
--- a/R/parse-query.R
+++ b/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$args <- c(req$args, args)
     req$argsQuery <- args
     req$.internal$queryStringHandled <- TRUE
   }
@@ -154,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)
diff --git a/R/plumber-step.R b/R/plumber-step.R
index bea6ff85b..d1bca6942 100644
--- a/R/plumber-step.R
+++ b/R/plumber-step.R
@@ -61,16 +61,9 @@ PlumberStep <- R6Class(
       hookEnv <- new.env()
 
       args <- c(
-        # make sure req, res have priority
+        # add in `req`, `res` as they have been removed from `req$args`
         list(req = req, res = res),
-        # include any other args that users have provided
-        req$args,
-        # path is more important than query
-        req$argsPath,
-        # query is more important than body
-        req$argsQuery,
-        # body is added last
-        req$argsBody
+        req$args
       )
 
       # str(args)
diff --git a/R/plumber.R b/R/plumber.R
index 1a127e395..fbab3d5a9 100644
--- a/R/plumber.R
+++ b/R/plumber.R
@@ -599,6 +599,17 @@ Plumber <- R6Class(
           # `req_body_parser()` will also set `req$body` with the untouched body value
           req$argsBody <- req_body_parser(req, parsers)
 
+          req$args <- c(
+            # (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 is more important than query
+            req$argsPath,
+            # body is added last
+            req$argsBody
+          )
+
           # req$args will be filled inside `h$exec()`
           return(
             h$exec(req, res)

From 8e5541cd3b8e41d537b9f20671a1c4ce95b6f759 Mon Sep 17 00:00:00 2001
From: Barret Schloerke <schloerke@gmail.com>
Date: Tue, 1 Sep 2020 10:49:00 -0400
Subject: [PATCH 05/17] fix pkgdown

---
 tests/testthat/test-endpoint.R  |  2 +-
 vignettes/quickstart.Rmd        |  9 +++++----
 vignettes/rendering-output.Rmd  | 10 +++++-----
 vignettes/routing-and-input.Rmd | 10 +++++-----
 4 files changed, 16 insertions(+), 15 deletions(-)

diff --git a/tests/testthat/test-endpoint.R b/tests/testthat/test-endpoint.R
index f9cb7c09a..9de6377a1 100644
--- a/tests/testthat/test-endpoint.R
+++ b/tests/testthat/test-endpoint.R
@@ -25,7 +25,7 @@ test_that("Endpoints are exec'able with named arguments.", {
 test_that("Unnamed arguments error", {
   foo <- parse(text="foo <- function(){ 1 }")
   r <- PlumberEndpoint$new('verb', 'path', foo, new.env(parent = globalenv()))
-  expect_error(r$exec(3))
+  expect_error(r$exec(req = list(args = list(3))))
 
   foo <- parse(text="foo <- function(x, ...){ x + 1 }")
   r <- PlumberEndpoint$new('verb', 'path', foo, new.env(parent = globalenv()))
diff --git a/vignettes/quickstart.Rmd b/vignettes/quickstart.Rmd
index 2e73b6997..52b7160d5 100644
--- a/vignettes/quickstart.Rmd
+++ b/vignettes/quickstart.Rmd
@@ -21,13 +21,14 @@ This file defines two Plumber "endpoints." One is hosted at the path `/echo` and
 If you haven't installed `plumber` yet, see the [installation section](./index.html#installation). Once  you have `plumber` installed, you can use the `pr()` function to translate this R file into a Plumber API:
 
 ```r
-pr("plumber.R")
+root <- pr("plumber.R")
+root
 ```
 
 The `pr` object now encapsulates all the logic represented in your `plumber.R` file. The next step is to bring the API to life using the `pr_run()` method:
 
 ```r
-pr_run()
+root %>% pr_run()
 ```
 
 You should see a message about your API running on your computer on port `8000`. The API will continue running in your R session until you press the `Esc` key. If you're running this code locally on your personal machine, you should be able to open [http://localhost:8000/echo](http://localhost:8000/echo) or [http://localhost:8000/plot](http://localhost:8000/plot) in a web browser to test your new API endpoints.
@@ -39,7 +40,7 @@ The `/echo` endpoint should show output resembling the following.
 ```{r, echo=FALSE, results='asis'}
 r <- plumber::plumb("files/apis/01-01-quickstart.R")
 e <- r$endpoints[[1]][[1]]
-code_chunk(json_serialize(e$exec()), "json")
+code_chunk(json_serialize(e$exec(req = list(), res = NULL)), "json")
 ```
 
 The `/plot` endpoint will show you a simple plot of some data from the iris dataset.
@@ -82,7 +83,7 @@ This endpoint would produce something like the following, when visited. It also
 ```{r, echo=FALSE, results='asis'}
 r <- plumber::plumb("files/apis/01-02-html.R")
 e <- r$endpoints[[1]][[1]]
-code_chunk(e$exec(), "html")
+code_chunk(e$exec(req = list(), res = NULL), "html")
 ```
 
 You can even provide your own custom serializers and define how to translate the R object produced by your endpoint into the bits that will produce Plumber's HTTP response.
diff --git a/vignettes/rendering-output.Rmd b/vignettes/rendering-output.Rmd
index 69827c0c8..08bbbe047 100644
--- a/vignettes/rendering-output.Rmd
+++ b/vignettes/rendering-output.Rmd
@@ -97,7 +97,7 @@ Visiting http://localhost:8000/boxed?letter=U or http://localhost:8000/unboxed?l
 ```{r, echo=FALSE, results='asis'}
 r <- plumber::plumb("files/apis/04-03-letters.R")
 e <- r$endpoints[[1]][[1]]
-code_chunk(json_serialize(e$exec(letter="U")), "json")
+code_chunk(json_serialize(e$exec(req = list(args = list(letter="U")), res = NULL)), "json")
 ```
 
 However, http://localhost:8000/boxed?letter=Y will produce:
@@ -105,14 +105,14 @@ However, http://localhost:8000/boxed?letter=Y will produce:
 ```{r, echo=FALSE, results='asis'}
 r <- plumber::plumb("files/apis/04-03-letters.R")
 e <- r$endpoints[[1]][[1]]
-code_chunk(jsonlite::toJSON(e$exec(letter="Y"), auto_unbox = FALSE), "json")
+code_chunk(jsonlite::toJSON(e$exec(req = list(args = list(letter="Y")), res = NULL), auto_unbox = FALSE), "json")
 ```
 
 while http://localhost:8000/unboxed?letter=Y will produce:
 
 ```{r, echo=FALSE, results='asis'}
 e <- r$endpoints[[1]][[2]]
-code_chunk(jsonlite::toJSON(e$exec(letter="Y"), auto_unbox = TRUE))
+code_chunk(jsonlite::toJSON(e$exec(req = list(args = list(letter="Y")), res = NULL), auto_unbox = TRUE))
 ```
 
 The `/boxed` endpoint, as the name implies, produces "boxed" JSON output in which length-1 vectors are still rendered as an array. Conversely, the `/unboxed` endpoint sets `auto_unbox=TRUE` in its call to `jsonlite::toJSON`, causing length-1 R vectors to be rendered as JSON scalars.
@@ -169,7 +169,7 @@ This means that it is possible for you to intentionally `stop()` in an endpoint
 ```{r, echo=FALSE, results='asis'}
 r <- plumber::plumb("files/apis/04-05-error.R")
 e <- r$endpoints[[1]][[2]]
-code_chunk(json_serialize(e$exec(res=list(status=1))), "json")
+code_chunk(json_serialize(e$exec(req = list(), res=list(status=1))), "json")
 ```
 
 A custom error handler can be set using the `setErrorHandler()` method:
@@ -217,7 +217,7 @@ Since this API is using a `PUT` request to test this API, we'll use `curl` on th
 ```{r, echo=FALSE, results='asis'}
 r <- plumber::plumb("files/apis/06-01-capitalize.R")
 e <- r$endpoints[[1]][[2]]
-code_chunk(json_serialize(e$exec(req=list(cookies=list()))), "json")
+code_chunk(json_serialize(e$exec(req=list(cookies=list()), res = NULL)), "json")
 ```
 
 If we send a `PUT` request and specify the `capital` parameter, a cookie will be set on the client which will allow the server to accommodate our preference in future requests. In `curl`, you need to specify a file in which you want to save these cookies using the `-c` option. This is a good reminder that clients handle cookies differently -- some won't support them at all -- so be sure that the clients you intend to support with your API play nicely with cookies if you want to use them.
diff --git a/vignettes/routing-and-input.Rmd b/vignettes/routing-and-input.Rmd
index d9e6c999f..00dbbf56d 100644
--- a/vignettes/routing-and-input.Rmd
+++ b/vignettes/routing-and-input.Rmd
@@ -157,7 +157,7 @@ Visiting http://localhost:8000/types/14 will return:
 ```{r, echo=FALSE, results='asis'}
 r <- plumber::plumb("files/apis/03-02-types.R")
 e <- r$endpoints[[1]][[1]]
-code_chunk(json_serialize(e$exec(id="14")), "json")
+code_chunk(json_serialize(e$exec(req = list(args = list(id = "14")), res = NULL)), "json")
 ```
 
 If you only intend to support a particular data type for a particular parameter in your dynamic route, you can specify the desired type in the route itself.
@@ -265,19 +265,19 @@ Visiting http://localhost:8000/?q=bread&pretty=1 will print:
 ```{r, echo=FALSE, results='asis'}
 r <- plumber::plumb("files/apis/03-03-search.R")
 e <- r$endpoints[[1]][[1]]
-code_chunk(json_serialize(e$exec(q="bread", pretty="1")), "json")
+code_chunk(json_serialize(e$exec(req = list(args = list(q = "bread", pretty = "1")), res = NULL)), "json")
 ```
 
 This is equivalent to calling `search(q="bread", pretty="1")`. If a parameter were not specified in the query string, it would just be omitted from the invocation of the endpoint. For example http://localhost:8000/?q=cereal would be equivalent to `search(q="cereal")`. The function would fall back to the default value of the `pretty` parameter (`0`), since that was defined in the function signature.
 
 ```{r, echo=FALSE, results='asis'}
-code_chunk(json_serialize(e$exec(q="cereal")), "json")
+code_chunk(json_serialize(e$exec(req = list(args = list(q = "cereal")), res = NULL)), "json")
 ```
 
 Including additional query string arguments that do not map to a parameter of the function has no effect. For instance http://localhost:8000/?test=123 will return the same results as calling `search()`.
 
 ```{r, echo=FALSE, results='asis'}
-code_chunk(json_serialize(e$exec()), "json")
+code_chunk(json_serialize(e$exec(req = NULL, res = NULL)), "json")
 ```
 
 (Note that the raw query string is available as `req$QUERY_STRING`.)
@@ -300,7 +300,7 @@ Running `curl --data "id=123&name=Jennifer" "http://localhost:8000/user"` will r
 ```{r, echo=FALSE, results='asis'}
 r <- plumber::plumb("files/apis/03-04-body.R")
 e <- r$endpoints[[1]][[1]]
-code_chunk(json_serialize(e$exec(req=list(bodyRaw = charToRaw("id=123&name=Jennifer"), body=list(id = 123, name = "Jennifer")), id=123, name="Jennifer")), "json")
+code_chunk(json_serialize(e$exec(req=list(bodyRaw = charToRaw("id=123&name=Jennifer"), body=list(id = 123, name = "Jennifer"), args = list(id = 123, name = "Jennifer")), res = NULL)), "json")
 ```
 
 Alternatively, `curl --data '{"id":123, "name": "Jennifer"}' "http://localhost:8000/user"` (formatting the body as JSON) will have the same effect.

From b42b6ea69cec6fa32088e4b36a0a85e9f9741868 Mon Sep 17 00:00:00 2001
From: Barret Schloerke <barret@rstudio.com>
Date: Tue, 1 Sep 2020 16:24:18 -0400
Subject: [PATCH 06/17] Apply suggestions from code review

Co-authored-by: Carson Sievert <cpsievert1@gmail.com>
---
 R/plumber-step.R | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/R/plumber-step.R b/R/plumber-step.R
index d1bca6942..4e81181dd 100644
--- a/R/plumber-step.R
+++ b/R/plumber-step.R
@@ -65,9 +65,6 @@ PlumberStep <- R6Class(
         list(req = req, res = res),
         req$args
       )
-
-      # str(args)
-
       preexecStep <- function(...) {
         private$runHooks("preexec", c(list(data = hookEnv), args))
       }

From 4a204ac6c0354266088092b8e88e8d1b1ce5a2d3 Mon Sep 17 00:00:00 2001
From: Barret Schloerke <schloerke@gmail.com>
Date: Tue, 1 Sep 2020 16:48:40 -0400
Subject: [PATCH 07/17] Add more information to news

---
 NEWS.md | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/NEWS.md b/NEWS.md
index 5714184b3..677d884f7 100644
--- a/NEWS.md
+++ b/NEWS.md
@@ -10,15 +10,20 @@ 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)
+
+
 * 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$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)
-
-* Named Plumber endpoint arguments are de-duplicated so that the endpoint function does not fail due to too many formal argument matchings.  To be sure you are getting all argument values, use `req$argsQuery`, `req$argsPath`, and `req$argsBody` to access path, query, and body parameters respectively.
 
 #### OpenAPI
 

From c9cea23eb43f5e8f3faf8fb6b4b7f6cfa281537e Mon Sep 17 00:00:00 2001
From: Barret Schloerke <schloerke@gmail.com>
Date: Tue, 1 Sep 2020 16:49:07 -0400
Subject: [PATCH 08/17] Allow unnamed args to be passed through in `...`

---
 NEWS.md          |  1 +
 R/plumber-step.R | 19 ++++---------------
 2 files changed, 5 insertions(+), 15 deletions(-)

diff --git a/NEWS.md b/NEWS.md
index 677d884f7..1896455bc 100644
--- a/NEWS.md
+++ b/NEWS.md
@@ -16,6 +16,7 @@ plumber 1.0.0
   * 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 arguments 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.
diff --git a/R/plumber-step.R b/R/plumber-step.R
index 4e81181dd..bcd472b61 100644
--- a/R/plumber-step.R
+++ b/R/plumber-step.R
@@ -114,6 +114,7 @@ getRelevantArgs <- function(args, plumberExpression) {
   }
 
   # fast return
+  # also works with unnamed arguments
   if (identical(fargs, "...")) {
     return(args)
   }
@@ -134,21 +135,9 @@ getRelevantArgs <- function(args, plumberExpression) {
     return(ret)
   }
 
-  if (is.null(names(args))) {
-    unnamedArgs <- seq_along(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 = ","), "\""
-    )
-  }
-
+  # 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.

From a0ec5277d22d476e96b04b2cd9adb4e4f23ce412 Mon Sep 17 00:00:00 2001
From: Barret Schloerke <schloerke@gmail.com>
Date: Tue, 1 Sep 2020 17:06:33 -0400
Subject: [PATCH 09/17] fix unnamed argument test

---
 tests/testthat/test-endpoint.R | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/tests/testthat/test-endpoint.R b/tests/testthat/test-endpoint.R
index 9de6377a1..7215ece3c 100644
--- a/tests/testthat/test-endpoint.R
+++ b/tests/testthat/test-endpoint.R
@@ -22,14 +22,14 @@ test_that("Endpoints are exec'able with named arguments.", {
   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(req = list(args = list(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(req = list(args = list(1)), res = 20), 3)
+  expect_equal(r$exec(req = list(args = list(x = 3, 1)), res = 100), 5)
 })
 
 test_that("Ellipses allow any named args through", {

From 2337293ec4011e078e06464e230e120c7151d6db Mon Sep 17 00:00:00 2001
From: Joe Cheng <joe@rstudio.com>
Date: Thu, 3 Sep 2020 12:39:47 -0700
Subject: [PATCH 10/17] Tweak NEWS wording

---
 NEWS.md | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/NEWS.md b/NEWS.md
index 1896455bc..bab9ce4bf 100644
--- a/NEWS.md
+++ b/NEWS.md
@@ -16,7 +16,7 @@ plumber 1.0.0
   * 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 arguments 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)
+  * 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.

From 1d10397b02fd9165888cb476dc97f7404d03278f Mon Sep 17 00:00:00 2001
From: Joe Cheng <joe@rstudio.com>
Date: Thu, 3 Sep 2020 12:49:10 -0700
Subject: [PATCH 11/17] Give hook env emptyenv as parent

---
 R/plumber-step.R | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/R/plumber-step.R b/R/plumber-step.R
index bcd472b61..329a00084 100644
--- a/R/plumber-step.R
+++ b/R/plumber-step.R
@@ -58,7 +58,7 @@ PlumberStep <- R6Class(
     #' @description step execution function
     #' @param req,res Request and response objects created by a Plumber request
     exec = function(req, res) {
-      hookEnv <- new.env()
+      hookEnv <- new.env(parent = emptyenv())
 
       args <- c(
         # add in `req`, `res` as they have been removed from `req$args`

From e405946dd6cf13546299284782b1a6fbf6a74366 Mon Sep 17 00:00:00 2001
From: Joe Cheng <joe@rstudio.com>
Date: Thu, 3 Sep 2020 12:59:29 -0700
Subject: [PATCH 12/17] Remove outdated comment

---
 R/plumber-step.R | 1 -
 1 file changed, 1 deletion(-)

diff --git a/R/plumber-step.R b/R/plumber-step.R
index 329a00084..9668779da 100644
--- a/R/plumber-step.R
+++ b/R/plumber-step.R
@@ -121,7 +121,6 @@ getRelevantArgs <- function(args, plumberExpression) {
 
   # 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.
   if (all(fargs %in% c("req", "res"))) {
     ret <- list()
     # using `$` will retrieve the 1st occurance of req,res

From d024fc11d99ce67ffed4e16b0003ae8edb8d8780 Mon Sep 17 00:00:00 2001
From: Joe Cheng <joe@rstudio.com>
Date: Thu, 3 Sep 2020 13:03:22 -0700
Subject: [PATCH 13/17] Fix comment

---
 R/plumber.R | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/R/plumber.R b/R/plumber.R
index 95c2f17dd..8b3ce73cf 100644
--- a/R/plumber.R
+++ b/R/plumber.R
@@ -605,7 +605,7 @@ Plumber <- R6Class(
             # will contain all args added in filters
             # `req$argsQuery` is available, but already absorbed into `req$args`
             req$args,
-            # path is more important than query
+            # path is more important than body
             req$argsPath,
             # body is added last
             req$argsBody

From 863c3530e98511a29f95c34019a268ac41c13ac5 Mon Sep 17 00:00:00 2001
From: Joe Cheng <joe@rstudio.com>
Date: Thu, 3 Sep 2020 13:04:25 -0700
Subject: [PATCH 14/17] Remove outdated comment

---
 R/plumber.R | 1 -
 1 file changed, 1 deletion(-)

diff --git a/R/plumber.R b/R/plumber.R
index 8b3ce73cf..43634b159 100644
--- a/R/plumber.R
+++ b/R/plumber.R
@@ -611,7 +611,6 @@ Plumber <- R6Class(
             req$argsBody
           )
 
-          # req$args will be filled inside `h$exec()`
           return(
             h$exec(req, res)
           )

From cf7c32906dd7788d2c2c266c38611aa0579cf9bb Mon Sep 17 00:00:00 2001
From: Barret Schloerke <schloerke@gmail.com>
Date: Thu, 3 Sep 2020 16:48:38 -0400
Subject: [PATCH 15/17] Collect args for each stage in `$exec(req, res)`

---
 R/plumber-step.R | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/R/plumber-step.R b/R/plumber-step.R
index 9668779da..5d70ba442 100644
--- a/R/plumber-step.R
+++ b/R/plumber-step.R
@@ -60,20 +60,24 @@ PlumberStep <- R6Class(
     exec = function(req, res) {
       hookEnv <- new.env(parent = emptyenv())
 
-      args <- c(
-        # add in `req`, `res` as they have been removed from `req$args`
-        list(req = req, res = res),
-        req$args
-      )
+      # use a function as items could possibly be added to `req$args` in each step
+      args_for_formal_matching <- function() {
+        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), args))
+        private$runHooks("preexec", c(list(data = hookEnv), args_for_formal_matching()))
       }
       execStep <- function(...) {
-        relevant_args <- getRelevantArgs(args, plumberExpression=private$func)
+        relevant_args <- getRelevantArgs(args_for_formal_matching(), plumberExpression=private$func)
         do.call(private$func, relevant_args, envir = private$envir)
       }
       postexecStep <- function(value, ...) {
-        private$runHooks("postexec", c(list(data = hookEnv, value = value), args))
+        private$runHooks("postexec", c(list(data = hookEnv, value = value), args_for_formal_matching()))
       }
       runSteps(
         NULL,

From 3cddea2bd601bf9e015d6e4f7724fb9206ed93df Mon Sep 17 00:00:00 2001
From: Barret Schloerke <schloerke@gmail.com>
Date: Thu, 3 Sep 2020 17:15:10 -0400
Subject: [PATCH 16/17] Make sure `PlumberStep` `private$func` is a function.
 Use it as such

---
 NEWS.md                           |  2 ++
 R/hookable.R                      |  5 +++--
 R/openapi-spec.R                  |  9 ++++-----
 R/plumber-step.R                  | 17 +++++++++++++----
 tests/testthat/test-endpoint.R    |  2 +-
 tests/testthat/test-parse-block.R |  2 +-
 6 files changed, 24 insertions(+), 13 deletions(-)

diff --git a/NEWS.md b/NEWS.md
index 6844f187c..4452bbc27 100644
--- a/NEWS.md
+++ b/NEWS.md
@@ -127,6 +127,8 @@ plumber 1.0.0
 
 * `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.
 
+* When creating a `PlumberFilter` or `PlumberEndpoint`, an error will be thrown if `expr` does not evaluate to a function. (#666)
+
 ### 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)
diff --git a/R/hookable.R b/R/hookable.R
index c738e72fc..774de3870 100644
--- a/R/hookable.R
+++ b/R/hookable.R
@@ -6,7 +6,8 @@ Hookable <- R6Class(
     #' @description Register a hook on a router
     #' @param stage a character string.
     #' @param handler a hook function.
-    registerHook = function(stage, handler){
+    registerHook = function(stage, handler) {
+      stopifnot(is.function(handler))
       private$hooks[[stage]] <- c(private$hooks[[stage]], handler)
     },
     #' @description Register hooks on a router
@@ -46,7 +47,7 @@ Hookable <- R6Class(
             stageHookArgs <- list()
             list(
               function(...) {
-                stageHookArgs <<- getRelevantArgs(args, plumberExpression = stageHook)
+                stageHookArgs <<- getRelevantArgs(args, func = stageHook)
               },
               function(...) {
                 do.call(stageHook, stageHookArgs) #TODO: envir=private$envir?
diff --git a/R/openapi-spec.R b/R/openapi-spec.R
index 3814f9eb4..1c0a3c862 100644
--- a/R/openapi-spec.R
+++ b/R/openapi-spec.R
@@ -249,17 +249,16 @@ isJSONserializable <- function(x) {
   )
 }
 
-#' Extract metadata on args of plumberExpression
+#' Extract metadata on args of endpoint_func
 #' @noRd
-getArgsMetadata <- function(plumberExpression){
+getArgsMetadata <- function(endpoint_func) {
   #return same format as getTypedParams or params?
-  if (!is.function(plumberExpression)) plumberExpression <- eval(plumberExpression)
-  args <- formals(plumberExpression)
+  args <- formals(endpoint_func)
   lapply(args[! (names(args) %in% c("req", "res", "..."))], function(arg) {
     required <- identical(arg, formals(function(x){})$x)
     if (is.call(arg) || is.name(arg)) {
       arg <- tryCatch(
-        eval(arg, envir = environment(plumberExpression)),
+        eval(arg, envir = environment(endpoint_func)),
         error = function(cond) {NA})
     }
     # Check that it is possible to transform arg value into
diff --git a/R/plumber-step.R b/R/plumber-step.R
index 5d70ba442..2a1de3621 100644
--- a/R/plumber-step.R
+++ b/R/plumber-step.R
@@ -44,7 +44,7 @@ PlumberStep <- R6Class(
       } else {
         private$func <- expr
       }
-
+      throw_if_func_is_not_a_function(private$func)
       private$envir <- envir
 
       if (!missing(lines)){
@@ -73,7 +73,7 @@ PlumberStep <- R6Class(
         private$runHooks("preexec", c(list(data = hookEnv), args_for_formal_matching()))
       }
       execStep <- function(...) {
-        relevant_args <- getRelevantArgs(args_for_formal_matching(), plumberExpression=private$func)
+        relevant_args <- getRelevantArgs(args_for_formal_matching(), func = private$func)
         do.call(private$func, relevant_args, envir = private$envir)
       }
       postexecStep <- function(value, ...) {
@@ -108,9 +108,9 @@ PlumberStep <- R6Class(
 )
 
 # @param positional list with names where they were provided.
-getRelevantArgs <- function(args, plumberExpression) {
+getRelevantArgs <- function(args, func) {
   # Extract the names of the arguments this function supports.
-  fargs <- names(formals(eval(plumberExpression)))
+  fargs <- names(formals(func))
 
   if (length(fargs) == 0) {
     # no matches
@@ -230,6 +230,7 @@ PlumberEndpoint <- R6Class(
       } else {
         private$func <- expr
       }
+      throw_if_func_is_not_a_function(private$func)
       private$envir <- envir
 
       private$regex <- createPathRegex(path, self$getFuncParams())
@@ -298,6 +299,7 @@ PlumberFilter <- R6Class(
       } else {
         private$func <- expr
       }
+      throw_if_func_is_not_a_function(private$func)
       private$envir <- envir
 
       if (!missing(serializer)){
@@ -309,3 +311,10 @@ PlumberFilter <- R6Class(
     }
   )
 )
+
+
+throw_if_func_is_not_a_function <- function(func) {
+  if(!is.function(func)) {
+    stop("`expr` did not evaluate to a function")
+  }
+}
diff --git a/tests/testthat/test-endpoint.R b/tests/testthat/test-endpoint.R
index 7215ece3c..a52b8a239 100644
--- a/tests/testthat/test-endpoint.R
+++ b/tests/testthat/test-endpoint.R
@@ -12,7 +12,7 @@ test_that("Endpoints execute in their environment", {
 
 test_that("Missing lines are ok", {
   expect_silent({
-    PlumberEndpoint$new('verb', 'path', { 1 }, new.env(parent = globalenv()))
+    PlumberEndpoint$new('verb', 'path', { function() { 1 }}, new.env(parent = globalenv()))
   })
 })
 
diff --git a/tests/testthat/test-parse-block.R b/tests/testthat/test-parse-block.R
index ac11e6165..fd2c008e2 100644
--- a/tests/testthat/test-parse-block.R
+++ b/tests/testthat/test-parse-block.R
@@ -216,7 +216,7 @@ test_that("@parser parameters produce an error or not", {
     evaluateBlock(
       srcref = 3, # which evaluates to line 2
       file = c("#' @get /test", "#' @parser octet list(key = \"val\")"),
-      expr = substitute(identity),
+      expr = as.expression(substitute(identity)),
       envir = new.env(),
       addEndpoint = function(a, b, ...) { stop("should not reach here")},
       addFilter = as.null,

From 6ad8dccc25371a1879c260b85c3fc5c18f243e8c Mon Sep 17 00:00:00 2001
From: Barret Schloerke <schloerke@gmail.com>
Date: Thu, 3 Sep 2020 17:15:28 -0400
Subject: [PATCH 17/17] Remove bad merge news item

---
 NEWS.md | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/NEWS.md b/NEWS.md
index 4452bbc27..b8b419038 100644
--- a/NEWS.md
+++ b/NEWS.md
@@ -19,13 +19,6 @@ plumber 1.0.0
   * 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$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)
-
-
 #### OpenAPI
 
 * API Documentation is now hosted at `/__docs__`. If `swagger` documentation is being used, `/__swagger__` will redirect to `/__docs__`. (#654)