Opt in to trailing slash 307 redirect. Opt out of 405 logic. (#746)

rstudio · Jan 5, 2021 · e9dac93 · e9dac93
1 parent 1d11c9e
commit e9dac93
Show file tree

Hide file tree

Showing 10 changed files with 220 additions and 88 deletions.
diff --git a/DESCRIPTION b/DESCRIPTION
@@ -2,7 +2,7 @@ Encoding: UTF-8
 Package: plumber
 Type: Package
 Title: An API Generator for R
-Version: 1.0.0.9999
+Version: 1.0.0.90001
 Roxygen: list(markdown = TRUE)
 Authors@R: c(
   person("Barret", "Schloerke", role = c("cre", "aut"), email = "barret@rstudio.com"),
@@ -31,7 +31,8 @@ Imports:
     swagger (>= 3.33.0),
     magrittr,
     mime,
-    lifecycle (>= 0.2.0)
+    lifecycle (>= 0.2.0),
+    ellipsis (>= 0.3.0)
 LazyData: TRUE
 ByteCompile: TRUE
 Suggests:
@@ -67,11 +68,11 @@ Collate:
     'new-rstudio-project.R'
     'openapi-spec.R'
     'openapi-types.R'
+    'options_plumber.R'
     'paths.R'
     'plumb-block.R'
     'plumb-globals.R'
     'plumb.R'
-    'plumber-options.R'
     'plumber-response.R'
     'plumber-static.R'
     'plumber-step.R'

diff --git a/NEWS.md b/NEWS.md
@@ -7,11 +7,18 @@ plumber 1.0.0.9999 Development version
 
 * When plumbing a Plumber file and using a Plumber router modifier (`#* @plumber`), an error will be thrown if the original router is not returned. (#738)
 
+* `options_plumber()` now requires that all options are named. If no option name is provided, an error with be thrown. (#746)
+
 ### New features
 
+* Added option `plumber.trailingSlash`. This option (which is **disabled** by default) allows routes to be redirected to route definitions with a trailing slash. For example, if a `GET` request is submitted to `/test?a=1` with no `/test` route is defined, but a `GET` `/test/` route definition does exist, then the original request will respond with a `307` to reattempt against `GET` `/test/?a=1`. This option will be _enabled_ by default in a future release. This logic executed for before calling the `404` handler. (#746)
+
+* Added option `plumber.methodNotFound`. This option (which is enabled by default) allows for a status of `405` to be returned if an invalid method is used when requesting a valid route. This logic executed for before calling the default `404` handler. (#746)
+
 * Guess OpenApi response content type from serializer. (@meztez #684)
 
 * Passing `edit = TRUE` to `plumb_api()` will open the API source file. (#699)
+
 * Allow for spaces in `@apiTag` and `@tag` when tag is surrended by single or double quotes. (#685)
 
 * OpenAPI Specification can be set using a file path. (@meztez #696)

diff --git a/R/default-handlers.R b/R/default-handlers.R
@@ -1,10 +1,4 @@
-default404Handler <- function(req, res){
-  if (is_405(req$pr, req$PATH_INFO, req$REQUEST_METHOD)) {
-    res$status <- 405L
-    # https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Allow
-    res$setHeader("Allow", paste(req$verbsAllowed, collapse = ", "))
-    return(list(error = "405 - Method Not Allowed"))
-  }
+default404Handler <- function(req, res) {
   res$status <- 404
   res$serializer <- serializer_unboxed_json()
   list(error="404 - Resource Not Found")
@@ -16,12 +10,14 @@ defaultErrorHandler <- function(){
 
     li <- list()
 
+    # always serialize error with unboxed json
+    res$serializer <- serializer_unboxed_json()
+
     if (res$status == 200L){
       # The default is a 200. If that's still set, then we should probably override with a 500.
       # It's possible, however, than a handler set a 40x and then wants to use this function to
       # render an error, though.
       res$status <- 500
-      res$serializer <- serializer_unboxed_json()
       li$error <- "500 - Internal server error"
     } else {
       li$error <- "Internal error"
@@ -87,13 +83,15 @@ is_405 <- function(pr, path_to_find, verb_to_find) {
   # nothing found, not 405
   if (length(verbs_allowed) == 0) return(FALSE)
 
+  # is verb excluded?
   !(verb_to_find %in% verbs_allowed)
 }
 router_has_route <- function(pr, path_to_find, verb_to_find) {
   verbs_allowed <- allowed_verbs(pr, path_to_find)
 
-  # nothing found, not 405
+  # nothing found, not a route
   if (length(verbs_allowed) == 0) return(FALSE)
 
+  # is verb found?
   verb_to_find %in% verbs_allowed
 }
diff --git a/R/plumber-options.R → R/options_plumber.R b/R/plumber-options.R → R/options_plumber.R
@@ -11,6 +11,14 @@
 #' \item{`plumber.docs.callback`}{A function. Called with
 #' a single parameter corresponding to the visual documentation url after Plumber server is ready. This can be used
 #' by RStudio to open the docs when then API is ran from the editor. Defaults to option `NULL`.}
+#' \item{`plumber.trailingSlash`}{Logical value which allows the router to redirect any request
+#' that has a matching route with a trailing slash. For example, if set to `TRUE` and the
+#' GET route `/test/` existed, then a GET request of `/test?a=1` would redirect to
+#' `/test/?a=1`. Defaults to `FALSE`. This option will default to `TRUE` in a future release.}
+#' \item{`plumber.methodNotAllowed`}{Logical value which allows the router to notify that an
+#' unavailable method was requested, but a different request method is allowed. For example,
+#' if set to `TRUE` and the GET route `/test` existed, then a POST request of `/test` would
+#' receive a 405 status and the allowed methods. Defaults to `TRUE`.}
 #' \item{`plumber.apiURL`}{Server urls for OpenAPI Specification respecting
 #' pattern `scheme://host:port/path`. Other `api*` options will be ignored when set.}
 #' \item{`plumber.apiScheme`}{Scheme used to build OpenAPI url and server url for
@@ -34,16 +42,20 @@
 #' by settings this option to `FALSE`. Defaults to `TRUE`}
 #' }
 #'
-#' @param port,docs,docs.callback,apiScheme,apiHost,apiPort,apiPath,apiURL,maxRequestSize,sharedSecret,legacyRedirects See details
+#' @param ... Ignored. Should be empty
+#' @param port,docs,docs.callback,trailingSlash,methodNotAllowed,apiScheme,apiHost,apiPort,apiPath,apiURL,maxRequestSize,sharedSecret,legacyRedirects See details
 #' @return
 #' The complete, prior set of [options()] values.
 #' If a particular parameter is not supplied, it will return the current value.
 #' If no parameters are supplied, all returned values will be the current [options()] values.
 #' @export
 options_plumber <- function(
+  ...,
   port                 = getOption("plumber.port"),
   docs                 = getOption("plumber.docs"),
   docs.callback        = getOption("plumber.docs.callback"),
+  trailingSlash        = getOption("plumber.trailingSlash"),
+  methodNotAllowed     = getOption("plumber.methodNotAllowed"),
   apiURL               = getOption("plumber.apiURL"),
   apiScheme            = getOption("plumber.apiScheme"),
   apiHost              = getOption("plumber.apiHost"),
@@ -53,17 +65,21 @@ options_plumber <- function(
   sharedSecret         = getOption("plumber.sharedSecret"),
   legacyRedirects      = getOption("plumber.legacyRedirects")
 ) {
+  ellipsis::check_dots_empty()
+
   options(
-    plumber.apiScheme = apiScheme,
-    plumber.apiHost = apiHost,
-    plumber.apiPort = apiPort,
-    plumber.apiPath = apiPath,
-    plumber.apiURL = apiURL,
-    plumber.maxRequestSize = maxRequestSize,
-    plumber.port = port,
-    plumber.sharedSecret = sharedSecret,
-    plumber.docs = docs,
-    plumber.docs.callback = docs.callback,
-    plumber.legacyRedirects = legacyRedirects
+    plumber.port                 =   port,
+    plumber.docs                 =   docs,
+    plumber.docs.callback        =   docs.callback,
+    plumber.trailingSlash        =   trailingSlash,
+    plumber.methodNotAllowed     =   methodNotAllowed,
+    plumber.apiURL               =   apiURL,
+    plumber.apiScheme            =   apiScheme,
+    plumber.apiHost              =   apiHost,
+    plumber.apiPort              =   apiPort,
+    plumber.apiPath              =   apiPath,
+    plumber.maxRequestSize       =   maxRequestSize,
+    plumber.sharedSecret         =   sharedSecret,
+    plumber.legacyRedirects      =   legacyRedirects
   )
 }
diff --git a/R/plumber.R b/R/plumber.R
@@ -726,6 +726,48 @@ Plumber <- R6Class(
 
       # No endpoint could handle this request. 404
       notFoundStep <- function(...) {
+
+        if (isTRUE(getOption("plumber.trailingSlash", FALSE))) {
+          # Redirect to the slash route, if it exists
+          path <- req$PATH_INFO
+          # If the path does not end in a slash,
+          if (!grepl("/$", path)) {
+            new_path <- paste0(path, "/")
+            # and a route with a slash exists...
+            if (router_has_route(req$pr, new_path, req$REQUEST_METHOD)) {
+
+              # Temp redirect with same REQUEST_METHOD
+              # Add on the query string manually. They do not auto transfer
+              # The POST body will be reissued by caller
+              new_location <- paste0(new_path, req$QUERY_STRING)
+              res$status <- 307
+              res$setHeader(
+                name = "Location",
+                value = new_location
+              )
+              res$serializer <- serializer_unboxed_json()
+              return(
+                list(message = "307 - Redirecting with trailing slash")
+              )
+            }
+          }
+        }
+
+        # No trailing-slash route exists...
+        # Try allowed verbs
+
+        if (isTRUE(getOption("plumber.methodNotAllowed", TRUE))) {
+          # Notify about allowed verbs
+          if (is_405(req$pr, req$PATH_INFO, req$REQUEST_METHOD)) {
+            res$status <- 405L
+            # https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Allow
+            res$setHeader("Allow", paste(req$verbsAllowed, collapse = ", "))
+            res$serializer <- serializer_unboxed_json()
+            return(list(error = "405 - Method Not Allowed"))
+          }
+        }
+
+        # Notify that there is no route found
         private$notFoundHandler(req = req, res = res)
       }
       steps <- append(steps, list(notFoundStep))

diff --git a/man/options_plumber.Rd b/man/options_plumber.Rd
diff --git a/tests/testthat/helper-options.R b/tests/testthat/helper-options.R
@@ -0,0 +1,8 @@
+with_options <- function(opts, code) {
+  old_opts <- options(opts)
+  on.exit({
+    options(old_opts)
+  }, add = TRUE)
+
+  force(code)
+}
diff --git a/tests/testthat/test-options.R b/tests/testthat/test-options.R
@@ -1,14 +1,12 @@
 context("Options")
 
 test_that("Options set and get", {
-  option_value <- getOption("plumber.port")
-  on.exit({
-    options(plumber.port = option_value)
-  }, add = TRUE)
-  options_plumber(port = FALSE)
-  expect_false(getOption("plumber.port"))
-  options_plumber(port = NULL)
-  expect_null(getOption("plumber.port"))
+  with_options(list(plumber.port = NULL), {
+    options_plumber(port = FALSE)
+    expect_false(getOption("plumber.port"))
+    options_plumber(port = NULL)
+    expect_null(getOption("plumber.port"))
+  })
 })
 
 test_that("all options used are `options_plumber()` parameters", {
@@ -39,7 +37,12 @@ test_that("all options used are `options_plumber()` parameters", {
   deprecated_options <-  c("plumber.swagger.url")
   plumber_options_used <- plumber_options_used[!(plumber_options_used %in% deprecated_options)]
   ### code to match formals
-  options_plumber_formals <- paste0("plumber.", sort(names(formals(options_plumber))))
+  formals_to_match <-
+    sort(setdiff(
+      names(formals(options_plumber)),
+      "..."
+    ))
+  options_plumber_formals <- paste0("plumber.", formals_to_match)
 
   expect_equal(
     plumber_options_used,
@@ -49,16 +52,13 @@ test_that("all options used are `options_plumber()` parameters", {
 
 
 test_that("Legacy swagger redirect can be disabled", {
-  option_value <- getOption("plumber.legacyRedirects")
-  on.exit({
-    options(plumber.legacyRedirects = option_value)
-  }, add = TRUE)
+  with_options(list(), {
+    options_plumber(legacyRedirects = TRUE)
+    redirects <- swagger_redirects()
+    expect_gt(length(redirects), 0)
 
-  options_plumber(legacyRedirects = TRUE)
-  redirects <- swagger_redirects()
-  expect_gt(length(redirects), 0)
-
-  options_plumber(legacyRedirects = FALSE)
-  redirects <- swagger_redirects()
-  expect_equal(length(redirects), 0)
+    options_plumber(legacyRedirects = FALSE)
+    redirects <- swagger_redirects()
+    expect_equal(length(redirects), 0)
+  })
 })