From 459c18e54cabc64b3bb994f83e06089437e49e61 Mon Sep 17 00:00:00 2001 From: Barret Schloerke Date: Thu, 31 Dec 2020 12:35:26 -0500 Subject: [PATCH 01/13] Sort mounts by decreasing path order More specific mount paths will be found by using this order Co-Authored-By: Bruno Tremblay --- R/plumber.R | 2 ++ 1 file changed, 2 insertions(+) diff --git a/R/plumber.R b/R/plumber.R index e30cefedf..edafe1cd8 100644 --- a/R/plumber.R +++ b/R/plumber.R @@ -242,7 +242,9 @@ Plumber <- R6Class( path <- paste0(path, "/") } + # order the mounts so that the more specific mount paths are ahead of the less specific mount paths private$mnts[[path]] <- router + private$mnts <- private$mnts[order(names(private$mnts), decreasing = TRUE)] }, #' @description Unmount a Plumber router #' @param path a character string. Where to unmount router. From fe524e6530cc78fe6ed117fab0850d443884fdc0 Mon Sep 17 00:00:00 2001 From: Barret Schloerke Date: Thu, 31 Dec 2020 12:40:17 -0500 Subject: [PATCH 02/13] Use pre sorted `private$mnts` for routing --- R/plumber.R | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/R/plumber.R b/R/plumber.R index edafe1cd8..51bf3922b 100644 --- a/R/plumber.R +++ b/R/plumber.R @@ -706,8 +706,18 @@ 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) { + # This router can not serve this endpoint; See if any mounted routers will. + # `private$mnts` is reverse alpha-sorted to have more specific mount paths be found first. + # `self$mounts` is alpha-sorted to have less specific mount paths be found first. + # Ex: + # * `/aaa/bbb/foo` is requested. + # * Mounts `/aaa` and `/aaa/bbb` exist. + # * We want to use mount `/aaa/bbb` as it is more specific + # TODO + # * If `/aaa/bbb` mount does not support `/aaa/bbb/foo`, then try mount `/aaa`. + # * Current behavior is to return a 404 + mnts <- private$mnts + mountSteps <- lapply(names(mnts), function(mountPath) { # (make step function) function(...) { resetForward() @@ -718,7 +728,7 @@ Plumber <- R6Class( # 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)) + return(mnts[[mountPath]]$route(req, res)) } else { return(forward()) } From b745d1443eec36e78fd86a76e7569d1dd2cca742 Mon Sep 17 00:00:00 2001 From: Barret Schloerke Date: Thu, 31 Dec 2020 12:42:54 -0500 Subject: [PATCH 03/13] Use regular sorted mounts for pr$mounts --- R/plumber.R | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/R/plumber.R b/R/plumber.R index 51bf3922b..adf7ef04a 100644 --- a/R/plumber.R +++ b/R/plumber.R @@ -1041,9 +1041,10 @@ Plumber <- R6Class( filters = function(){ private$filts }, - #' @field mounts Plumber router mounts read-only + #' @field mounts Plumber router mounts sorted by mount location. Read-only. mounts = function(){ - private$mnts + mnts <- private$mnts + mnts[sort(names(mnts), decreasing = FALSE)] }, #' @field environment Plumber router environment read-only environment = function() { From e34cbc06f56133a4a4c48d819c501135d68f9a92 Mon Sep 17 00:00:00 2001 From: Barret Schloerke Date: Thu, 31 Dec 2020 12:43:21 -0500 Subject: [PATCH 04/13] pull out mounts to a variable to avoid sorting within the active binding or use `private$mnts` where possible --- R/default-handlers.R | 7 ++++--- R/plumber.R | 28 +++++++++++++++------------- man/Plumber.Rd | 10 +++++----- 3 files changed, 24 insertions(+), 21 deletions(-) diff --git a/R/default-handlers.R b/R/default-handlers.R index 8784c36c3..0334a985d 100644 --- a/R/default-handlers.R +++ b/R/default-handlers.R @@ -54,9 +54,10 @@ allowed_verbs <- function(pr, path_to_find) { } # look at all possible mounts - for (i in seq_along(pr$mounts)) { - mount <- pr$mounts[[i]] - mount_path <- sub("/$", "", names(pr$mounts)[i]) # trim trailing `/` + mnts <- pr$mounts + for (i in seq_along(mnts)) { + mount <- mnts[[i]] + mount_path <- sub("/$", "", names(mnts)[i]) # trim trailing `/` # if the front of the urls don't match, move on to next mount if (!identical( diff --git a/R/plumber.R b/R/plumber.R index adf7ef04a..f1f301580 100644 --- a/R/plumber.R +++ b/R/plumber.R @@ -412,7 +412,7 @@ Plumber <- R6Class( cat(crayon::silver("# Plumber router with ", endCount, " endpoint", ifelse(endCount == 1, "", "s"),", ", as.character(length(private$filts)), " filter", ifelse(length(private$filts) == 1, "", "s"),", and ", - as.character(length(self$mounts)), " sub-router", ifelse(length(self$mounts) == 1, "", "s"),".\n", sep="")) + as.character(length(private$mnts)), " sub-router", ifelse(length(private$mnts) == 1, "", "s"),".\n", sep="")) if(topLevel){ cat(prefix, crayon::silver("# Use `pr_run()` on this object to start the API.\n"), sep="") @@ -947,7 +947,7 @@ Plumber <- R6Class( private$api_spec_handler <- api_fun }, #' @description Retrieve openAPI file - getApiSpec = function() { #FIXME: test + getApiSpec = function() { routerSpec <- private$routerSpecificationInternal(self) @@ -1033,11 +1033,11 @@ Plumber <- R6Class( self$getApiSpec() } ), active = list( - #' @field endpoints Plumber router endpoints read-only + #' @field endpoints Plumber router endpoints. Read-only endpoints = function(){ private$ends }, - #' @field filters Plumber router filters read-only + #' @field filters Plumber router filters. Read-only filters = function(){ private$filts }, @@ -1046,11 +1046,11 @@ Plumber <- R6Class( mnts <- private$mnts mnts[sort(names(mnts), decreasing = FALSE)] }, - #' @field environment Plumber router environment read-only + #' @field environment Plumber router environment. Read-only environment = function() { private$envir }, - #' @field routes Plumber router routes read-only + #' @field routes Plumber router routes. Read-only routes = function(){ paths <- list() @@ -1104,14 +1104,15 @@ Plumber <- R6Class( }) # Sub-routers - if (length(self$mounts) > 0){ - for(i in 1:length(self$mounts)){ + mnts <- self$mounts + if (length(mnts) > 0){ + for(i in 1:length(mnts)){ # Trim leading slash - path <- sub("^/", "", names(self$mounts)[i]) + path <- sub("^/", "", names(mnts)[i]) levels <- strsplit(path, "/", fixed=TRUE)[[1]] - m <- self$mounts[[i]] + m <- mnts[[i]] paths <- addPath(paths, levels, m) } } @@ -1224,10 +1225,11 @@ Plumber <- R6Class( } # recursively gather mounted enpoint entries - if (length(router$mounts) > 0) { - for (mountPath in names(router$mounts)) { + router_mnts <- router$mounts + if (length(router_mnts) > 0) { + for (mountPath in sort(names(router_mnts))) { mountEndpoints <- private$routerSpecificationInternal( - router$mounts[[mountPath]], + router_mnts[[mountPath]], join_paths(parentPath, mountPath) ) endpointList <- utils::modifyList(endpointList, mountEndpoints) diff --git a/man/Plumber.Rd b/man/Plumber.Rd index 11736bc4e..fc3d44d99 100644 --- a/man/Plumber.Rd +++ b/man/Plumber.Rd @@ -126,15 +126,15 @@ pr$setErrorHandler(function(req, res, err) { \section{Active bindings}{ \if{html}{\out{
}} \describe{ -\item{\code{endpoints}}{Plumber router endpoints read-only} +\item{\code{endpoints}}{Plumber router endpoints. Read-only} -\item{\code{filters}}{Plumber router filters read-only} +\item{\code{filters}}{Plumber router filters. Read-only} -\item{\code{mounts}}{Plumber router mounts read-only} +\item{\code{mounts}}{Plumber router mounts sorted by mount location. Read-only.} -\item{\code{environment}}{Plumber router environment read-only} +\item{\code{environment}}{Plumber router environment. Read-only} -\item{\code{routes}}{Plumber router routes read-only} +\item{\code{routes}}{Plumber router routes. Read-only} } \if{html}{\out{
}} } From 300a145773f2dfe47a188de0e53e1a2c9181b83f Mon Sep 17 00:00:00 2001 From: Barret Schloerke Date: Thu, 31 Dec 2020 13:05:25 -0500 Subject: [PATCH 05/13] Add docs explaining the mount exec order --- R/plumber.R | 6 ++++++ R/pr.R | 7 +++++++ man/Plumber.Rd | 6 ++++++ man/pr_mount.Rd | 8 ++++++++ 4 files changed, 27 insertions(+) diff --git a/R/plumber.R b/R/plumber.R index f1f301580..f9b16ed4a 100644 --- a/R/plumber.R +++ b/R/plumber.R @@ -220,6 +220,12 @@ Plumber <- R6Class( #' using the `mount()` method. This allows you to compartmentalize your API #' by paths which is a great technique for decomposing large APIs into smaller files. #' + #' Mounts with a more specific path name will be used over mounts with a + #' matching, but less specific path name. For example, let's say we have two + #' mounts: `/aaa` and `/aaa/bbb`. If a request came in for `/aaa/bbb/ccc`, + #' mount `/aaa/bbb` will be used to process the request. If a request came in + #' for `/aaa/ccc`, mount `/aaa` will be used to process the request. + #' #' See also: [pr_mount()] #' @param path a character string. Where to mount router. #' @param router a Plumber router. Router to be mounted. diff --git a/R/pr.R b/R/pr.R index ec2c4219e..71363171d 100644 --- a/R/pr.R +++ b/R/pr.R @@ -209,6 +209,12 @@ pr_head <- function(pr, #' files. This function mutates the Plumber router ([pr()]) in place and #' returns the updated router. #' +#' Mounts with a more specific path name will be used over mounts with a +#' matching, but less specific path name. For example, let's say we have two +#' mounts: `/aaa` and `/aaa/bbb`. If a request came in for `/aaa/bbb/ccc`, +#' mount `/aaa/bbb` will be used to process the request. If a request came in +#' for `/aaa/ccc`, mount `/aaa` will be used to process the request. +#' #' @param pr The host Plumber router. #' @param path A character string. Where to mount router. #' @param router A Plumber router. Router to be mounted. @@ -222,6 +228,7 @@ pr_head <- function(pr, #' #' pr() %>% #' pr_get("/goodbye", function() "Goodbye") %>% +#' # By mounting pr1, we can make a GET request to `/hi/hello` #' pr_mount("/hi", pr1) %>% #' pr_run() #' } diff --git a/man/Plumber.Rd b/man/Plumber.Rd index fc3d44d99..f126a5fdf 100644 --- a/man/Plumber.Rd +++ b/man/Plumber.Rd @@ -256,6 +256,12 @@ Plumber routers can be “nested” by mounting one into another using the \code{mount()} method. This allows you to compartmentalize your API by paths which is a great technique for decomposing large APIs into smaller files. +Mounts with a more specific path name will be used over mounts with a +matching, but less specific path name. For example, let's say we have two +mounts: \verb{/aaa} and \verb{/aaa/bbb}. If a request came in for \verb{/aaa/bbb/ccc}, +mount \verb{/aaa/bbb} will be used to process the request. If a request came in +for \verb{/aaa/ccc}, mount \verb{/aaa} will be used to process the request. + See also: \code{\link[=pr_mount]{pr_mount()}} \subsection{Usage}{ \if{html}{\out{
}}\preformatted{Plumber$mount(path, router)}\if{html}{\out{
}} diff --git a/man/pr_mount.Rd b/man/pr_mount.Rd index 7f08d1c13..cacb7ab7a 100644 --- a/man/pr_mount.Rd +++ b/man/pr_mount.Rd @@ -23,6 +23,13 @@ by paths which is a great technique for decomposing large APIs into smaller files. This function mutates the Plumber router (\code{\link[=pr]{pr()}}) in place and returns the updated router. } +\details{ +Mounts with a more specific path name will be used over mounts with a +matching, but less specific path name. For example, let's say we have two +mounts: \verb{/aaa} and \verb{/aaa/bbb}. If a request came in for \verb{/aaa/bbb/ccc}, +mount \verb{/aaa/bbb} will be used to process the request. If a request came in +for \verb{/aaa/ccc}, mount \verb{/aaa} will be used to process the request. +} \examples{ \dontrun{ pr1 <- pr() \%>\% @@ -30,6 +37,7 @@ pr1 <- pr() \%>\% pr() \%>\% pr_get("/goodbye", function() "Goodbye") \%>\% + # By mounting pr1, we can make a GET request to `/hi/hello` pr_mount("/hi", pr1) \%>\% pr_run() } From 1b03190eba03064772ffc70f5dbc63dc6f139c00 Mon Sep 17 00:00:00 2001 From: Barret Schloerke Date: Thu, 31 Dec 2020 13:05:30 -0500 Subject: [PATCH 06/13] Update NEWS.md --- NEWS.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/NEWS.md b/NEWS.md index c7a64c04e..25cefd9c0 100644 --- a/NEWS.md +++ b/NEWS.md @@ -7,6 +7,8 @@ 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) +* When processing a route, the mount location with the larger path match will be used. Before, the first mount added that could possibly process the requested path was used. For more details, see `?pr_mount`. (#748) + ### New features * Guess OpenApi response content type from serializer. (@meztez #684) From 3c56426f46ff88efc2c7bd07720129d383d2c98a Mon Sep 17 00:00:00 2001 From: Barret Schloerke Date: Thu, 31 Dec 2020 14:10:45 -0500 Subject: [PATCH 07/13] Use `Map` to loop over the mounts and mount paths --- R/plumber.R | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/R/plumber.R b/R/plumber.R index f9b16ed4a..dbaa657d0 100644 --- a/R/plumber.R +++ b/R/plumber.R @@ -1111,16 +1111,15 @@ Plumber <- R6Class( # Sub-routers mnts <- self$mounts - if (length(mnts) > 0){ - for(i in 1:length(mnts)){ + if (length(mnts) > 0) { + Map(mnts, names(mnts), f = function(mnt, mnt_name) { # Trim leading slash - path <- sub("^/", "", names(mnts)[i]) + path <- sub("^/", "", mnt_name) levels <- strsplit(path, "/", fixed=TRUE)[[1]] - m <- mnts[[i]] - paths <- addPath(paths, levels, m) - } + paths <<- addPath(paths, levels, mnt) + }) } lexisort <- function(paths) { From 7d3b8b4b76ae5adf75c27e0e5d48fbbab3173c84 Mon Sep 17 00:00:00 2001 From: Barret Schloerke Date: Thu, 31 Dec 2020 14:11:16 -0500 Subject: [PATCH 08/13] If a node is a Endpoint or Plumber router, then merge --- R/plumber.R | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/R/plumber.R b/R/plumber.R index dbaa657d0..26d70b763 100644 --- a/R/plumber.R +++ b/R/plumber.R @@ -1076,7 +1076,8 @@ Plumber <- R6Class( # Check for existing endpoints at current children node that share the same name matching_name_nodes <- node[names(node) == children[1]] - existing_endpoints <- vapply(matching_name_nodes, inherits, logical(1), "PlumberEndpoint") + # if there is an endpoint or router... + existing_endpoints <- vapply(matching_name_nodes, inherits, logical(1), c("PlumberEndpoint", "Plumber")) # This is for situation where an endpoint is on `/A` and you # also have route with an endpoint on `A/B`. Resulting nested list From 5ad400ffe8f0879f16f9ff23d412550ef7e54e6a Mon Sep 17 00:00:00 2001 From: Barret Schloerke Date: Thu, 31 Dec 2020 14:11:54 -0500 Subject: [PATCH 09/13] Add test for a mounted path that conflicts with another mounted path --- tests/testthat/test-routing.R | 37 +++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/tests/testthat/test-routing.R b/tests/testthat/test-routing.R index 224d08451..a7294ac1b 100644 --- a/tests/testthat/test-routing.R +++ b/tests/testthat/test-routing.R @@ -33,3 +33,40 @@ test_that("Routing to errors and 404s works", { expect_equal(er, errRes) expect_equal(errors, 1) }) + + +test_that("mounts with more specific paths are used", { + + root <- pr() %>% + pr_mount("/aaa", + pr() %>% + pr_get("/bbb/hello", function() "/aaa - /bbb/hello") %>% + pr_get("/bbb/test", function() "/aaa - /bbb/test") + ) %>% + pr_mount("/aaa/bbb", + pr() %>% + pr_get("/hello", function() "/aaa/bbb - /hello") %>% + pr_set_404(function(...) { "404" }) + ) + + + # make sure it can print... which calls root$routes + expect_silent({ + capture.output(print(root)) + }) + + res <- PlumberResponse$new() + # route with more specific mount is used + expect_equal( + root$route(make_req("GET", "/aaa/bbb/hello"), res), + "/aaa/bbb - /hello" + ) + + # currently "bad" behavior. TODO - make this return "/aaa - /bbb/test" + expect_equal( + root$route(make_req("GET", "/aaa/bbb/test"), res), + "404" + ) + + +}) From 99d51833f5be2796ade6c34b6a0cf64f39d829ca Mon Sep 17 00:00:00 2001 From: Barret Schloerke Date: Thu, 31 Dec 2020 14:21:55 -0500 Subject: [PATCH 10/13] Earlier R version don't like to check `is.na(NULL)`. Check `is.null(x)` first --- R/openapi-spec.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/R/openapi-spec.R b/R/openapi-spec.R index 7dce8466f..dfa9a1cd5 100644 --- a/R/openapi-spec.R +++ b/R/openapi-spec.R @@ -210,7 +210,7 @@ isNa <- function(x) { #' Check na or null #' @noRd isNaOrNull <- function(x) { - any(isNa(x)) || is.null(x) + is.null(x) || any(isNa(x)) } #' Remove na or null From 838b18a0a8f8c65135644247328c68a84633e133 Mon Sep 17 00:00:00 2001 From: Barret Schloerke Date: Thu, 31 Dec 2020 14:22:14 -0500 Subject: [PATCH 11/13] Validate that there is no error produced while printing --- tests/testthat/test-routing.R | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/tests/testthat/test-routing.R b/tests/testthat/test-routing.R index a7294ac1b..a3a4d4620 100644 --- a/tests/testthat/test-routing.R +++ b/tests/testthat/test-routing.R @@ -50,10 +50,8 @@ test_that("mounts with more specific paths are used", { ) - # make sure it can print... which calls root$routes - expect_silent({ - capture.output(print(root)) - }) + # make sure it can print without error... which calls root$routes + expect_error(capture.output(print(root)), NA) res <- PlumberResponse$new() # route with more specific mount is used From 6ca073a6fcbaa4a9f0d8883df348a9dcd47fe32f Mon Sep 17 00:00:00 2001 From: Barret Schloerke Date: Thu, 31 Dec 2020 14:30:10 -0500 Subject: [PATCH 12/13] Remove some R 3.4 warnings about sort(NULL) --- R/default-handlers.R | 5 ++++- R/plumber.R | 1 + 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/R/default-handlers.R b/R/default-handlers.R index 0334a985d..02d31bb64 100644 --- a/R/default-handlers.R +++ b/R/default-handlers.R @@ -42,7 +42,7 @@ defaultErrorHandler <- function(){ #' @noRd allowed_verbs <- function(pr, path_to_find) { - verbs_allowed <- c() + verbs_allowed <- NULL # look at all possible endpoints for (endpoint_group in pr$endpoints) { @@ -78,6 +78,9 @@ allowed_verbs <- function(pr, path_to_find) { } # return verbs + if (is.null(verbs_allowed)) { + return(verbs_allowed) + } sort(unique(verbs_allowed)) } diff --git a/R/plumber.R b/R/plumber.R index 26d70b763..1637d0247 100644 --- a/R/plumber.R +++ b/R/plumber.R @@ -1050,6 +1050,7 @@ Plumber <- R6Class( #' @field mounts Plumber router mounts sorted by mount location. Read-only. mounts = function(){ mnts <- private$mnts + if (length(mnts) == 0) return(mnts) mnts[sort(names(mnts), decreasing = FALSE)] }, #' @field environment Plumber router environment. Read-only From 6bc2cbfcb56c00fc9d35823cf727ff232485680a Mon Sep 17 00:00:00 2001 From: Barret Schloerke Date: Thu, 31 Dec 2020 14:38:17 -0500 Subject: [PATCH 13/13] Try to remove last R 3.4 warning when package loads --- R/openapi-types.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/R/openapi-types.R b/R/openapi-types.R index 45a558e9f..a966078fe 100644 --- a/R/openapi-types.R +++ b/R/openapi-types.R @@ -92,7 +92,7 @@ plumberToApiType <- function(type, inPath = FALSE) { return(vapply(type, plumberToApiType, character(1), inPath, USE.NAMES = FALSE)) } # default type is "string" type - if (is.na(type)) { + if (isNaOrNull(type)) { return(defaultApiType) }