diff --git a/R/default-handlers.R b/R/default-handlers.R index 3ea03b21..7660f8ff 100644 --- a/R/default-handlers.R +++ b/R/default-handlers.R @@ -1,8 +1,4 @@ -default404Handler <- function(req, res) { - res$status <- 404 - res$serializer <- serializer_unboxed_json() - list(error="404 - Resource Not Found") -} + defaultErrorHandler <- function(){ function(req, res, err){ @@ -95,3 +91,70 @@ router_has_route <- function(pr, path_to_find, verb_to_find) { # is verb found? verb_to_find %in% verbs_allowed } + + +default307Handler <- function(req, res, location) { + res$status <- 307 + res$setHeader( + name = "Location", + value = location + ) + res$serializer <- serializer_unboxed_json() + + list(message = "307 - Redirecting with trailing slash") +} + +default404Handler <- function(req, res) { + res$status <- 404 + res$serializer <- serializer_unboxed_json() + list(error="404 - Resource Not Found") +} + +default405Handler <- function(req, res) { + 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() + + list(error = "405 - Method Not Allowed") +} + + +# When we want to end route execution and declare the route can not be handled, +# we check for: +# * trailing slash support (`307` redirect) +# * different verb support (`405` method not allowed) +# Then we return a 404 given `handle404(req, res)` (`404` method not found) +lastChanceRouteNotFound <- function(req, res, pr, handle404 = default404Handler) { + + # Try trailing slash route + 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(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) + return(default307Handler(req, res, new_location)) + } + } + } + + # No trailing-slash route exists... + # Try allowed verbs + if (isTRUE(getOption("plumber.methodNotAllowed", TRUE))) { + # Notify about allowed verbs + if (is_405(pr, req$PATH_INFO, req$REQUEST_METHOD)) { + return(default405Handler(req, res)) + } + } + + # Handle 404 logic + handle404(req = req, res = res) +} diff --git a/R/plumber-static.R b/R/plumber-static.R index 8d8073d5..95d2eca2 100644 --- a/R/plumber-static.R +++ b/R/plumber-static.R @@ -62,9 +62,9 @@ PlumberStatic <- R6Class( path <- httpuv::decodeURIComponent(path) abs.path <- resolve_path(direc, path) if (is.null(abs.path)) { - # TODO: Should this be inherited from a parent router? - val <- private$notFoundHandler(req=req, res=res) - return(val) + # If the file doesn't exist, forward on to the router 404 handler + # (Default 404 handler calls `routeNotFound()` which will move on to next mount). + return(forward()) } ext <- tools::file_ext(abs.path) diff --git a/R/plumber-step.R b/R/plumber-step.R index 400b4cd0..57f43776 100644 --- a/R/plumber-step.R +++ b/R/plumber-step.R @@ -11,6 +11,8 @@ forward_class <- "plumber_forward" forward <- function() { exec <- getCurrentExec() exec$forward <- TRUE + # Currently not used. Would prefer this structure in future versions + structure(list(), class = "plumber_forward") } hasForwarded <- function() { getCurrentExec()$forward @@ -20,6 +22,14 @@ resetForward <- function() { exec$forward <- FALSE } +# Handle mounted routes not being found +routeNotFound <- function() { + structure(list(), class = "plumber_route_not_found") +} +isRouteNotFound <- function(x) { + inherits(x, "plumber_route_not_found") +} + #' plumber step R6 class #' @description an object representing a step in the lifecycle of the treatment #' of a request by a plumber router. diff --git a/R/plumber.R b/R/plumber.R index 03cefd73..c4441698 100644 --- a/R/plumber.R +++ b/R/plumber.R @@ -91,7 +91,7 @@ Plumber <- R6Class( # Default parsers to maintain legacy features self$setParsers(c("json", "form", "text", "octet", "multi")) self$setErrorHandler(defaultErrorHandler()) - self$set404Handler(default404Handler) + self$set404Handler(NULL) # Allows for fall through to next router self$setDocs(TRUE) private$docs_info$has_not_been_set <- TRUE # set to know if `$setDocs()` has been called before `$run()` private$docs_callback <- rlang::missing_arg() @@ -567,10 +567,30 @@ Plumber <- R6Class( routeStep <- function(...) { self$route(req, res) } + # No endpoint could handle this request. 307, 405, 404 + routeNotFoundStep <- function(value, ...) { + if (!isRouteNotFound(value)) { + # This router handled the request + # Go to the next step + return(value) + } + + # No endpoint could handle this request. + lastChanceRouteNotFound( + req = req, + res = res, + pr = self, + # `$route()` already had a chance to call `private$notFoundHandler()` + # Using `default404Handler()` as `private$notFoundHandler()` would be missing + handle404 = default404Handler + ) + } + postrouteStep <- function(value, ...) { private$runHooks("postroute", list(data = hookEnv, req = req, res = res, value = value)) } + serializeSteps <- function(value, ...) { if ("PlumberResponse" %in% class(value)) { return(res$toResponse()) @@ -614,6 +634,7 @@ Plumber <- R6Class( list( prerouteStep, routeStep, + routeNotFoundStep, postrouteStep, serializeSteps ) @@ -761,73 +782,89 @@ Plumber <- R6Class( # If we still haven't found a match, check the un-preempt'd endpoints. steps <- append(steps, list(makeHandleStep("__no-preempt__"))) - # We aren't going to serve this endpoint; see if any mounted routers will - mountSteps <- lapply(names(private$mnts), function(mountPath) { - # (make step function) - function(...) { - resetForward() - # TODO: support globbing? - if (nchar(path) >= nchar(mountPath) && substr(path, 0, nchar(mountPath)) == mountPath) { - # This is a prefix match or exact match. Let this router handle. + ## We aren't going to serve this endpoint; see if any mounted routers will - # First trim the prefix off of the PATH_INFO element - req$PATH_INFO <- substr(req$PATH_INFO, nchar(mountPath), nchar(req$PATH_INFO)) - return(private$mnts[[mountPath]]$route(req, res)) - } else { - return(forward()) + # Capture the path info so it can be reset before/after executing the mount + curPathInfo <- req$PATH_INFO + + # Dynamically add mount steps to avoid wasting time on mounts that don't match + mountSteps <- list() + Map( + mountPath = names(private$mnts), + mount = private$mnts, + f = function(mountPath, mount) { + # TODO: support globbing? + mountSupportsPath <- + nchar(path) >= nchar(mountPath) && + substr(path, 0, nchar(mountPath)) == mountPath + if (!mountSupportsPath) { + # This router does not support this path + # Do not add to mountSteps + return() } - } - }) - steps <- append(steps, mountSteps) - # 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") - ) + mountSteps[[length(mountSteps) + 1]] <<- function(...) { + mountExecStep <- function(...) { + resetForward() # Doesn't hurt to reset here + ## First trim the prefix off of the PATH_INFO element + # Reset the path info for each mount + req$PATH_INFO <- curPathInfo + req$PATH_INFO <- substr(req$PATH_INFO, nchar(mountPath), nchar(req$PATH_INFO)) + + # str(list(mountPath = mountPath, curPathInfo = curPathInfo, req_path_info = req$PATH_INFO)) + # Handle mount asynchronously + private$mnts[[mountPath]]$route(req, res) + } + postMountExecStep <- function(mountRes, ...) { + # Don't know what happened in the mount. Reset again. + resetForward() + + # Reset the req path info + # TODO-future; this should really be in a `finally()` after `mountExecStep`. + req$PATH_INFO <- curPathInfo + + # Only move on to the next mount if a `routeNotFound()` was returned + if (isRouteNotFound(mountRes)) { + # This router did not support this path + # `forward()` to the next router + return(forward()) + } + + # Regular response, return it + return(mountRes) } + + runSteps( + NULL, + stop, + list( + mountExecStep, + postMountExecStep + ) + ) } } + ) + steps <- append(steps, mountSteps) - # 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")) - } + routeNotFoundStep <- function(...) { + if (is.function(private$notFoundHandler)) { + # Terminate the route handling using the local 404 method + return( + lastChanceRouteNotFound( + req = req, + res = res, + pr = self, + handle404 = private$notFoundHandler + ) + ) } - # Notify that there is no route found - private$notFoundHandler(req = req, res = res) + # Let the next mount or `$serve()` handle it + return(routeNotFound()) } - steps <- append(steps, list(notFoundStep)) + steps <- append(steps, list(routeNotFoundStep)) errorHandlerStep <- function(error, ...) { private$errorHandler(req, res, error) diff --git a/scripts/route_execution.md b/scripts/route_execution.md new file mode 100644 index 00000000..d7e2bce8 --- /dev/null +++ b/scripts/route_execution.md @@ -0,0 +1,103 @@ +Thoughts for https://github.com/rstudio/plumber/pull/883 + +# Independent questions +- [ ] Should `post**` hooks be run in reverse order? (Like `on.exit(after = TRUE)`) +- [ ] Should we add support for `afterserve` hooks? + * Would call `later::later(function() { private$runHooks("afterserve") })` + + +# TODO for this PR +- [ ] Remove or relocate this file before merging +- [ ] Merge in #882 logic +- [ ] Be comfortable with the breaking changes below + + +# Known breaking changes +* When adding amount, the previous mount will continue to exist +* If a mount can not handle a request, the next mount will be attempted. + * Previously, if the first matching mount could not handle the request, a 404 was returned. + * To restore previous behavior, set `mount$set404Handler(plumber:::default404Handler)` + + +# Design desires +* Remove `forward()` as a global flag. Instead use a sentinal value that can be returned. (E.g. `next()`) +* Registration system for different return status situation. (E.g. 405, 307, 404 handlers) + + +# Adding a mount + +Current `$mount(path=, router=)`: +* If mount path exists, replace it +* If no mount path exists, append mount to end + +Proposed `$mount(path=, router=, ..., after=)` (with fall through): +* Do not unmount existing path locations +* If `after=NULL`, set `after = length(mounts)` +* Append mount at `after` location + +Proposed `$unmount(path=)`: +* Allow for `path=` to be a Plumber router + + +# Route execution + +Updates: + * Use `routeNotFound()` when `forward()` global flag is overloaded by (possibly) multiple mounts + * Move `405`, `307`, `404` logic to `lastChanceRouteNotFound()` instead of end of `$route()` logic + * `$route()`: Call `lastChanceRouteNotFound(handle404 = private$notFoundHandler)` only if no 404 handler has been set + * `$serve()`: Only on root. Call `lastChanceRouteNotFound(handle404 = default404Handler)` as `$route()` had its chance + * Set default 404 handler to `rlang::missing_arg()` by default + * Overwriting this value will allow per-mount control of 404 handling + * If nothing is done, the original `default404Handler()` will be called + + +* `$call(req)` - Hook for {httpuv} + * Set root router at `req$pr` + * Make `res` + * call `serve(req, res) + +* `$serve(res)` + * Run preroute hooks + * Call `$route(req, res)` + * If current value is `routeNotFound()` + * Set value to `lastChanceRouteNotFound(req, res, pr = self, default404Handler)` + * Run postroute hooks + * Serialize content + * Run `preserialize` hooks + * Run serializer code + * Run `postserialize` hooks + * **Proposal: Run `afterserve` hooks?** + * Runs in a later execution. Users have requested to have a "after the route has returned, run this code" hook + +* `$route(req, res)` + * Try to exec route from `__first__` routes + * If found, return result + * Execute each Filter + * Try to exec route from `__no-preempt__` routes + * If found, return result + * Try each mount... + * If mount path matches request path... + * Run `MOUNT$route(req, res)` + * If _value_ is not `routeNotFound()`... + * Return _value_ + * If 404 handler has been set + * (404 Handling can not be `forward()`ed) + * Return `lastChanceRouteNotFound(req, res, pr = self, default404Handler)` + * Return `routeNotFound()` + +* `lastChanceRouteNotFound(req, res, pr, handle404)` + * If in `307` situation + * Return `default307Handle(req, res, location)` + * If in `405` situation + * Return `default405Handle(req, res)` + * Return `handle404(req, res)` + + +## Assertions +* Mount should be able to return own 404 method +* `307` / `405` logic should work within the mount that it is terminating from + * Ex: If in `mnt1`, `mnt2` can not help find more possible routes +* Routes should be able to `forward()` when matched + * Static file mounts use this +* Should be able to mount multiple routers at the same location +* Docs should be the last mount (reverting `after=0` logic for #882) diff --git a/tests/testthat/files/router.R b/tests/testthat/files/router.R index a52a543f..ab19e5df 100644 --- a/tests/testthat/files/router.R +++ b/tests/testthat/files/router.R @@ -61,3 +61,17 @@ function(res){ function(){ "dual path" } + +#* @plumber +function(pr) { + mnt_1 <- + pr() %>% + pr_get("/hello", function() "say hello") + mnt_2 <- + pr() %>% + pr_get("/world", function() "say hello world") + + pr %>% + pr_mount("/say", mnt_1) %>% + pr_mount("/say/hello", mnt_2) +} diff --git a/tests/testthat/test-async.R b/tests/testthat/test-async.R index 02a3c2e8..ff580f40 100644 --- a/tests/testthat/test-async.R +++ b/tests/testthat/test-async.R @@ -55,9 +55,10 @@ expect_route_async <- function(x) { } async_router <- function() { - "files/async.R" %>% - test_path() %>% - pr() + root <- pr(test_path("files/async.R")) + mnt <- pr(test_path("files/async.R")) + # Mount a async router at /mnt + pr_mount(root, "/mount", mnt) } serve_route <- function(pr, route) { @@ -78,6 +79,14 @@ test_that("sync works", { expect_route_sync() }) +test_that("sync works", { + async_router() %>% + serve_route("/mount/sync") %>% + expect_not_promise() %>% + get_result() %>% + expect_route_sync() +}) + test_that("async works", { async_router() %>% serve_route("/async") %>% @@ -86,6 +95,14 @@ test_that("async works", { expect_route_async() }) +test_that("async works", { + async_router() %>% + serve_route("/mount/async") %>% + expect_promise() %>% + get_result() %>% + expect_route_async() +}) + context("Promise - hooks") diff --git a/tests/testthat/test-path-subst.R b/tests/testthat/test-path-subst.R index a0881508..17a2d5e6 100644 --- a/tests/testthat/test-path-subst.R +++ b/tests/testthat/test-path-subst.R @@ -102,15 +102,19 @@ test_that("integration of path parsing works", { expect_equal(r$route(make_req("GET", "/car/ratio/-1.5"), PlumberResponse$new()), -1.5) expect_equal(r$route(make_req("GET", "/car/ratio/-.5"), PlumberResponse$new()), -.5) expect_equal(r$route(make_req("GET", "/car/ratio/.5"), PlumberResponse$new()), .5) - expect_equal(r$route(make_req("GET", "/car/ratio/a"), PlumberResponse$new()), - list(error = "404 - Resource Not Found")) - expect_equal(r$route(make_req("GET", "/car/ratio/"), PlumberResponse$new()), - list(error = "404 - Resource Not Found")) - expect_equal(r$route(make_req("GET", "/car/ratio/."), PlumberResponse$new()), - list(error = "404 - Resource Not Found")) - expect_equal(r$route(make_req("GET", "/car/sold/true"), PlumberResponse$new()), TRUE) - expect_match(r$call(make_req("POST", "/car/sold/true"))$body, - "405 - Method Not Allowed") + + expect_body <- function(method, path, status, body, auto_unbox = TRUE) { + res <- r$call(make_req(method, path)) + expect_equal(res$status, status) + expect_equal( + res$body, + toJSON(body, auto_unbox = auto_unbox) + ) + } + expect_body("GET", "/car/ratio/", 404L, list(error = "404 - Resource Not Found")) + expect_body("GET", "/car/ratio/.", 404L, list(error = "404 - Resource Not Found")) + expect_body("GET", "/car/sold/true", 200L, TRUE, auto_unbox = FALSE) + expect_body("POST", "/car/sold/true", 405L, list(error = "405 - Method Not Allowed")) }) test_that("multiple variations in path works nicely with function args detection", { diff --git a/tests/testthat/test-plumber.R b/tests/testthat/test-plumber.R index f87a71ae..613071fb 100644 --- a/tests/testthat/test-plumber.R +++ b/tests/testthat/test-plumber.R @@ -225,9 +225,10 @@ test_that("mounts work", { pr$mount("/subpath", sub) + # `/` mount should not stop `/nested/path` from being found res <- PlumberResponse$new() pr$route(make_req("GET", "/nested/path"), res) - expect_equal(res$status, 404) + expect_equal(res$status, 200) val <- pr$route(make_req("GET", "/subpath/nested/path", qs="?a=123"), PlumberResponse$new()) expect_equal(val, "123") diff --git a/tests/testthat/test-routing.R b/tests/testthat/test-routing.R index 224d0845..17a8d393 100644 --- a/tests/testthat/test-routing.R +++ b/tests/testthat/test-routing.R @@ -11,7 +11,7 @@ test_that("Routing to errors and 404s works", { r$setErrorHandler(function(req, res, err){ errors <<- errors + 1; errRes }) r$set404Handler(function(req, res){ notFounds <<- notFounds + 1; notFoundRes }) - res <- PlumberResponse$new() + res <- PlumberResponse$new(serializer = serializer_identity()) expect_equal(r$route(make_req("GET", "/"), res), "first") expect_equal(r$route(make_req("GET", "/abc"), res), "abc get") @@ -20,16 +20,24 @@ test_that("Routing to errors and 404s works", { expect_equal(r$route(make_req("GET", "/path1"), res), "dual path") expect_equal(r$route(make_req("GET", "/path2"), res), "dual path") + ## Mounts fall back to parent router when route is not found + # Mount at `/say` with route `/hello` + expect_equal(r$route(make_req("GET", "/say/hello"), res), "say hello") + # Mount at `/say/hello` with route `/world` + expect_equal(r$route(make_req("GET", "/say/hello/world"), res), "say hello world") + expect_equal(errors, 0) expect_equal(notFounds, 0) - nf <- r$route(make_req("GET", "/something-crazy"), res) - expect_equal(res$serializer, serializer_json()) - expect_equal(nf, notFoundRes) + res <- PlumberResponse$new(serializer = serializer_identity()) + nf <- r$serve(make_req("GET", "/something-crazy"), res) + expect_equal(res$serializer, serializer_identity()) + expect_equal(nf$body, notFoundRes) expect_equal(notFounds, 1) - er <- r$route(make_req("GET", "/error"), res) - expect_equal(res$serializer, serializer_json()) - expect_equal(er, errRes) + res <- PlumberResponse$new(serializer = serializer_identity()) + er <- r$serve(make_req("GET", "/error"), res) + expect_equal(res$serializer, serializer_identity()) + expect_equal(er$body, errRes) expect_equal(errors, 1) }) diff --git a/tests/testthat/test-static.R b/tests/testthat/test-static.R index d6ac39dd..f2b725d0 100644 --- a/tests/testthat/test-static.R +++ b/tests/testthat/test-static.R @@ -70,8 +70,7 @@ test_that("static binary file is served", { }) test_that("404s are handled", { - res <- PlumberResponse$new() - pr$route(make_req("GET", "/i-dont-exist"), res) + res <- pr$call(make_req("GET", "/i-dont-exist")) expect_equal(res$status, 404) })