diff --git a/axum-extra/src/routing/resource.rs b/axum-extra/src/routing/resource.rs index 98105fac..86e83505 100644 --- a/axum-extra/src/routing/resource.rs +++ b/axum-extra/src/routing/resource.rs @@ -146,7 +146,7 @@ where T::Future: Send + 'static, { let path = self.show_update_destroy_path(); - self.router = self.router.nest(&path, svc); + self.router = self.router.nest_service(&path, svc); self } @@ -159,7 +159,7 @@ where T::Future: Send + 'static, { let path = self.index_create_path(); - self.router = self.router.nest(&path, svc); + self.router = self.router.nest_service(&path, svc); self } diff --git a/axum-extra/src/routing/spa.rs b/axum-extra/src/routing/spa.rs index 594b15c2..f8e3d0b0 100644 --- a/axum-extra/src/routing/spa.rs +++ b/axum-extra/src/routing/spa.rs @@ -161,7 +161,7 @@ where .handle_error(spa.handle_error.clone()); Router::new() - .nest(&spa.paths.assets_path, assets_service) + .nest_service(&spa.paths.assets_path, assets_service) .fallback( get_service(ServeFile::new(&spa.paths.index_file)).handle_error(spa.handle_error), ) diff --git a/axum/CHANGELOG.md b/axum/CHANGELOG.md index 3bd2a04d..a3c74aed 100644 --- a/axum/CHANGELOG.md +++ b/axum/CHANGELOG.md @@ -9,6 +9,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **added:** Support resolving host name via `Forwarded` header in `Host` extractor ([#1078]) +- **breaking:** `Router::nest` now only accepts `Router`s. Use + `Router::nest_service` to nest opaque services +- **added:** Add `Router::nest_service` for nesting opaque services. Use this to + nest services like `tower::services::ServeDir` +- **breaking:** The route `/foo/` no longer matches `/foo/*rest`. If you want + to match `/foo/` you have to add a route specifically for that +- **breaking:** Path params for wildcard routes no longer include the prefix + `/`. e.g. `/foo.js` will match `/*filepath` with a value of `foo.js`, _not_ + `/foo.js` +- **fixed:** Routes like `/foo` and `/*rest` are no longer considered + overlapping. `/foo` will take priority [#1078]: https://github.com/tokio-rs/axum/pull/1078 diff --git a/axum/Cargo.toml b/axum/Cargo.toml index c5de4abf..bb66b2f4 100644 --- a/axum/Cargo.toml +++ b/axum/Cargo.toml @@ -33,7 +33,9 @@ http = "0.2.5" http-body = "0.4.4" hyper = { version = "0.14.14", features = ["server", "tcp", "stream"] } itoa = "1.0.1" -matchit = "0.5.0" +# TODO(david): cannot ship until matchit has released a new version but we can +# start making changes +matchit = { git = "https://github.com/ibraheemdev/matchit", branch = "catchall-revamp" } memchr = "2.4.1" mime = "0.3.16" percent-encoding = "2.1" diff --git a/axum/src/extract/matched_path.rs b/axum/src/extract/matched_path.rs index 8965bd30..9a3381e1 100644 --- a/axum/src/extract/matched_path.rs +++ b/axum/src/extract/matched_path.rs @@ -149,12 +149,14 @@ mod tests { "/public", Router::new().route("/assets/*path", get(handler)), ) - .nest("/foo", handler.into_service()) + .nest_service("/foo", handler.into_service()) .layer(tower::layer::layer_fn(SetMatchedPathExtension)); let client = TestClient::new(app); - let res = client.get("/foo").send().await; + // we cannot call `/foo` because `nest_service("/foo", _)` registers routes + // for `/foo/*rest` and `/foo` + let res = client.get("/public").send().await; assert_eq!(res.text().await, "/:key"); let res = client.get("/api/users/123").send().await; diff --git a/axum/src/extract/path/mod.rs b/axum/src/extract/path/mod.rs index f559ae1f..20a23ce7 100644 --- a/axum/src/extract/path/mod.rs +++ b/axum/src/extract/path/mod.rs @@ -499,10 +499,10 @@ mod tests { let client = TestClient::new(app); let res = client.get("/foo/bar/baz").send().await; - assert_eq!(res.text().await, "/bar/baz"); + assert_eq!(res.text().await, "bar/baz"); let res = client.get("/bar/baz/qux").send().await; - assert_eq!(res.text().await, "/baz/qux"); + assert_eq!(res.text().await, "baz/qux"); } #[tokio::test] diff --git a/axum/src/routing/mod.rs b/axum/src/routing/mod.rs index ca178a75..b7955e15 100644 --- a/axum/src/routing/mod.rs +++ b/axum/src/routing/mod.rs @@ -3,7 +3,7 @@ use self::{future::RouteFuture, not_found::NotFound}; use crate::{ body::{boxed, Body, Bytes, HttpBody}, - extract::connect_info::IntoMakeServiceWithConnectInfo, + extract::{connect_info::IntoMakeServiceWithConnectInfo, MatchedPath}, response::{IntoResponse, Redirect, Response}, routing::strip_prefix::StripPrefix, util::try_downcast, @@ -66,7 +66,6 @@ pub struct Router { routes: HashMap>, node: Node, fallback: Fallback, - nested_at_root: bool, } impl Clone for Router { @@ -75,7 +74,6 @@ impl Clone for Router { routes: self.routes.clone(), node: self.node.clone(), fallback: self.fallback.clone(), - nested_at_root: self.nested_at_root, } } } @@ -95,13 +93,11 @@ impl fmt::Debug for Router { .field("routes", &self.routes) .field("node", &self.node) .field("fallback", &self.fallback) - .field("nested_at_root", &self.nested_at_root) .finish() } } pub(crate) const NEST_TAIL_PARAM: &str = "__private__axum_nest_tail_param"; -const NEST_TAIL_PARAM_CAPTURE: &str = "/*__private__axum_nest_tail_param"; impl Router where @@ -116,7 +112,6 @@ where routes: Default::default(), node: Default::default(), fallback: Fallback::Default(Route::new(NotFound)), - nested_at_root: false, } } @@ -163,7 +158,7 @@ where }; if let Err(err) = self.node.insert(path, id) { - self.panic_on_matchit_error(err); + panic!("Invalid route: {}", err); } self.routes.insert(id, service); @@ -171,8 +166,57 @@ where self } + // TODO(david): update docs #[doc = include_str!("../docs/routing/nest.md")] - pub fn nest(mut self, mut path: &str, svc: T) -> Self + pub fn nest(mut self, mut path: &str, router: Router) -> Self { + if path.is_empty() { + // nesting at `""` and `"/"` should mean the same thing + path = "/"; + } + + if path.contains('*') { + panic!("Invalid route: nested routes cannot contain wildcards (*)"); + } + + let prefix = path; + + let Router { + mut routes, + node, + fallback, + } = router; + + if let Fallback::Custom(_) = fallback { + panic!("Cannot nest `Router`s that has a fallback"); + } + + for (id, nested_path) in node.route_id_to_path { + let route = routes.remove(&id).unwrap(); + let full_path: Cow = if &*nested_path == "/" { + path.into() + } else if path == "/" { + (&*nested_path).into() + } else if let Some(path) = path.strip_suffix('/') { + format!("{}{}", path, nested_path).into() + } else { + format!("{}{}", path, nested_path).into() + }; + self = match route { + Endpoint::MethodRouter(method_router) => self.route( + &full_path, + method_router.layer(layer_fn(|s| StripPrefix::new(s, prefix))), + ), + Endpoint::Route(route) => self.route(&full_path, StripPrefix::new(route, prefix)), + }; + } + + debug_assert!(routes.is_empty()); + + self + } + + /// TODO + pub fn nest_service(mut self, mut path: &str, svc: T) -> Self where T: Service, Response = Response, Error = Infallible> + Clone + Send + 'static, T::Future: Send + 'static, @@ -188,64 +232,21 @@ where let prefix = path; - if path == "/" { - self.nested_at_root = true; - } + let path = if path.ends_with('/') { + format!("{}*{}", path, NEST_TAIL_PARAM) + } else { + format!("{}/*{}", path, NEST_TAIL_PARAM) + }; - match try_downcast::, _>(svc) { - // if the user is nesting a `Router` we can implement nesting - // by simplying copying all the routes and adding the prefix in - // front - Ok(router) => { - let Router { - mut routes, - node, - fallback, - // nesting a router that has something nested at root - // doesn't mean something is nested at root in _this_ router - // thus we don't need to propagate that - nested_at_root: _, - } = router; + let svc = strip_prefix::StripPrefix::new(svc, prefix); + self = self.route(&path, svc.clone()); - if let Fallback::Custom(_) = fallback { - panic!("Cannot nest `Router`s that has a fallback"); - } - - for (id, nested_path) in node.route_id_to_path { - let route = routes.remove(&id).unwrap(); - let full_path: Cow = if &*nested_path == "/" { - path.into() - } else if path == "/" { - (&*nested_path).into() - } else if let Some(path) = path.strip_suffix('/') { - format!("{}{}", path, nested_path).into() - } else { - format!("{}{}", path, nested_path).into() - }; - self = match route { - Endpoint::MethodRouter(method_router) => self.route( - &full_path, - method_router.layer(layer_fn(|s| StripPrefix::new(s, prefix))), - ), - Endpoint::Route(route) => { - self.route(&full_path, StripPrefix::new(route, prefix)) - } - }; - } - - debug_assert!(routes.is_empty()); - } - // otherwise we add a wildcard route to the service - Err(svc) => { - let path = if path.ends_with('/') { - format!("{}*{}", path, NEST_TAIL_PARAM) - } else { - format!("{}/*{}", path, NEST_TAIL_PARAM) - }; - - self = self.route(&path, strip_prefix::StripPrefix::new(svc, prefix)); - } - } + // `/*rest` is not matched by `/` so we need to also register a router at the + // prefix itself. Otherwise if you were to nest at `/foo` then `/foo` itself + // wouldn't match, which it should + self = self.route(prefix, svc.clone()); + // same goes for `/foo/`, that should also match + self = self.route(&format!("{}/", prefix), svc); self } @@ -259,7 +260,6 @@ where routes, node, fallback, - nested_at_root, } = other.into(); for (id, route) in routes { @@ -282,8 +282,6 @@ where } }; - self.nested_at_root = self.nested_at_root || nested_at_root; - self } @@ -324,7 +322,6 @@ where routes, node: self.node, fallback, - nested_at_root: self.nested_at_root, } } @@ -363,7 +360,6 @@ where routes, node: self.node, fallback: self.fallback, - nested_at_root: self.nested_at_root, } } @@ -419,24 +415,8 @@ where #[cfg(feature = "matched-path")] if let Some(matched_path) = self.node.route_id_to_path.get(&id) { - use crate::extract::MatchedPath; - - let matched_path = if let Some(previous) = req.extensions_mut().get::() { - // a previous `MatchedPath` might exist if we're inside a nested Router - let previous = if let Some(previous) = - previous.as_str().strip_suffix(NEST_TAIL_PARAM_CAPTURE) - { - previous - } else { - previous.as_str() - }; - - let matched_path = format!("{}{}", previous, matched_path); - matched_path.into() - } else { - Arc::clone(matched_path) - }; - req.extensions_mut().insert(MatchedPath(matched_path)); + req.extensions_mut() + .insert(MatchedPath(Arc::clone(matched_path))); } else { #[cfg(debug_assertions)] panic!("should always have a matched path for a route id"); @@ -455,17 +435,6 @@ where Endpoint::Route(inner) => inner.call(req), } } - - fn panic_on_matchit_error(&self, err: matchit::InsertError) { - if self.nested_at_root { - panic!( - "Invalid route: {}. Note that `nest(\"/\", _)` conflicts with all routes. Use `Router::fallback` instead", - err, - ); - } else { - panic!("Invalid route: {}", err); - } - } } impl Service> for Router diff --git a/axum/src/routing/tests/mod.rs b/axum/src/routing/tests/mod.rs index b253bc52..76164349 100644 --- a/axum/src/routing/tests/mod.rs +++ b/axum/src/routing/tests/mod.rs @@ -8,10 +8,9 @@ use crate::{ test_helpers::*, BoxError, Json, Router, }; -use http::{header::CONTENT_LENGTH, HeaderMap, Method, Request, Response, StatusCode, Uri}; +use http::{header::CONTENT_LENGTH, HeaderMap, Request, Response, StatusCode, Uri}; use hyper::Body; -use serde::Deserialize; -use serde_json::{json, Value}; +use serde_json::json; use std::{ convert::Infallible, future::{ready, Ready}, @@ -387,47 +386,24 @@ async fn without_trailing_slash_post() { assert_eq!(res.headers().get("location").unwrap(), "/foo/"); } -// for https://github.com/tokio-rs/axum/issues/420 #[tokio::test] -async fn wildcard_with_trailing_slash() { - #[derive(Deserialize, serde::Serialize)] - struct Tree { - user: String, - repo: String, - path: String, - } - - let app: Router = Router::new().route( - "/:user/:repo/tree/*path", - get(|Path(tree): Path| async move { Json(tree) }), +async fn wildcard_doesnt_match_just_trailing_slash() { + let app = Router::new().route( + "/x/*path", + get(|Path(path): Path| async move { path }), ); - // low level check that the correct redirect happens - let res = app - .clone() - .oneshot( - Request::builder() - .method(Method::GET) - .uri("/user1/repo1/tree") - .body(Body::empty()) - .unwrap(), - ) - .await - .unwrap(); - assert_eq!(res.status(), StatusCode::PERMANENT_REDIRECT); - assert_eq!(res.headers()["location"], "/user1/repo1/tree/"); - - // check that the params are deserialized correctly let client = TestClient::new(app); - let res = client.get("/user1/repo1/tree/").send().await; - assert_eq!( - res.json::().await, - json!({ - "user": "user1", - "repo": "repo1", - "path": "/", - }) - ); + + let res = client.get("/x").send().await; + assert_eq!(res.status(), StatusCode::NOT_FOUND); + + let res = client.get("/x/").send().await; + assert_eq!(res.status(), StatusCode::NOT_FOUND); + + let res = client.get("/x/foo/bar").send().await; + assert_eq!(res.status(), StatusCode::OK); + assert_eq!(res.text().await, "foo/bar"); } #[tokio::test] @@ -531,34 +507,6 @@ async fn route_layer() { assert_eq!(res.status(), StatusCode::UNAUTHORIZED); } -#[tokio::test] -#[should_panic( - expected = "Invalid route: insertion failed due to conflict with previously registered \ - route: /*__private__axum_nest_tail_param. \ - Note that `nest(\"/\", _)` conflicts with all routes. \ - Use `Router::fallback` instead" -)] -async fn good_error_message_if_using_nest_root() { - let app = Router::new() - .nest("/", get(|| async {})) - .route("/", get(|| async {})); - TestClient::new(app); -} - -#[tokio::test] -#[should_panic( - expected = "Invalid route: insertion failed due to conflict with previously registered \ - route: /*__private__axum_nest_tail_param. \ - Note that `nest(\"/\", _)` conflicts with all routes. \ - Use `Router::fallback` instead" -)] -async fn good_error_message_if_using_nest_root_when_merging() { - let one = Router::new().nest("/", get(|| async {})); - let two = Router::new().route("/", get(|| async {})); - let app = one.merge(two); - TestClient::new(app); -} - #[tokio::test] async fn different_methods_added_in_different_routes() { let app = Router::new() diff --git a/axum/src/routing/tests/nest.rs b/axum/src/routing/tests/nest.rs index 31149c94..83b80a63 100644 --- a/axum/src/routing/tests/nest.rs +++ b/axum/src/routing/tests/nest.rs @@ -1,8 +1,5 @@ use super::*; -use crate::{ - body::boxed, - extract::{Extension, MatchedPath}, -}; +use crate::{body::boxed, extract::Extension}; use std::collections::HashMap; use tower_http::services::ServeDir; @@ -117,7 +114,7 @@ async fn nesting_router_at_empty_path() { #[tokio::test] async fn nesting_handler_at_root() { - let app = Router::new().nest("/", get(|uri: Uri| async move { uri.to_string() })); + let app = Router::new().nest_service("/", get(|uri: Uri| async move { uri.to_string() })); let client = TestClient::new(app); @@ -205,7 +202,7 @@ async fn nested_service_sees_stripped_uri() { #[tokio::test] async fn nest_static_file_server() { - let app = Router::new().nest( + let app = Router::new().nest_service( "/static", get_service(ServeDir::new(".")).handle_error(|error| async move { ( @@ -357,7 +354,7 @@ async fn nest_at_capture() { ) .boxed_clone(); - let app = Router::new().nest("/:a", api_routes); + let app = Router::new().nest_service("/:a", api_routes); let client = TestClient::new(app); @@ -366,64 +363,48 @@ async fn nest_at_capture() { assert_eq!(res.text().await, "a=foo b=bar"); } +#[tokio::test] +async fn nest_with_and_without_trailing() { + let app = Router::new().nest_service("/foo", get(|| async {})); + + let client = TestClient::new(app); + + let res = client.get("/foo").send().await; + assert_eq!(res.status(), StatusCode::OK); + + let res = client.get("/foo/").send().await; + assert_eq!(res.status(), StatusCode::OK); + + let res = client.get("/foo/bar").send().await; + assert_eq!(res.status(), StatusCode::OK); +} + macro_rules! nested_route_test { ( $name:ident, + // the path we nest the inner router at nest = $nested_path:literal, + // the route the inner router accepts route = $route_path:literal, - expected = $expected_path:literal, - opaque_redirect = $opaque_redirect:expr $(,)? + // the route we expect to be able to call + expected = $expected_path:literal $(,)? ) => { #[tokio::test] async fn $name() { - let inner = Router::new().route( - $route_path, - get(|matched: MatchedPath| async move { matched.as_str().to_owned() }), - ); + let inner = Router::new().route($route_path, get(|| async {})); let app = Router::new().nest($nested_path, inner); let client = TestClient::new(app); let res = client.get($expected_path).send().await; let status = res.status(); - let matched_path = res.text().await; assert_eq!(status, StatusCode::OK, "Router"); - let inner = Router::new() - .route( - $route_path, - get(|matched: MatchedPath| async move { matched.as_str().to_owned() }), - ) - .boxed_clone(); - let app = Router::new().nest($nested_path, inner); + let inner = Router::new().route($route_path, get(|| async {})); + let app = Router::new().nest_service($nested_path, inner); let client = TestClient::new(app); - let res = client.get($expected_path).send().await; - if $opaque_redirect { - assert_eq!(res.status(), StatusCode::PERMANENT_REDIRECT, "opaque"); - - let location = res.headers()[http::header::LOCATION].to_str().unwrap(); - let res = client.get(location).send().await; - assert_eq!(res.status(), StatusCode::OK, "opaque with redirect"); - assert_eq!(res.text().await, location); - } else { - assert_eq!(res.status(), StatusCode::OK, "opaque"); - assert_eq!(res.text().await, matched_path); - } + let res = client.get(dbg!($expected_path)).send().await; + assert_eq!(res.status(), StatusCode::OK, "opaque"); } }; - - ( - $name:ident, - nest = $nested_path:literal, - route = $route_path:literal, - expected = $expected_path:literal $(,)? - ) => { - nested_route_test!( - $name, - nest = $nested_path, - route = $route_path, - expected = $expected_path, - opaque_redirect = false, - ); - }; } // test cases taken from https://github.com/tokio-rs/axum/issues/714#issuecomment-1058144460 @@ -433,22 +414,7 @@ nested_route_test!(nest_3, nest = "", route = "/a/", expected = "/a/"); nested_route_test!(nest_4, nest = "/", route = "/", expected = "/"); nested_route_test!(nest_5, nest = "/", route = "/a", expected = "/a"); nested_route_test!(nest_6, nest = "/", route = "/a/", expected = "/a/"); - -// This case is different for opaque services. -// -// The internal route becomes `/a/*__private__axum_nest_tail_param` which, according to matchit -// doesn't match `/a`. However matchit detects that a route for `/a/` exists and so it issues a -// redirect to `/a/`, which ends up calling the inner route as expected. -// -// So while the behavior isn't identical, the outcome is the same -nested_route_test!( - nest_7, - nest = "/a", - route = "/", - expected = "/a", - opaque_redirect = true, -); - +nested_route_test!(nest_7, nest = "/a", route = "/", expected = "/a",); nested_route_test!(nest_8, nest = "/a", route = "/a", expected = "/a/a"); nested_route_test!(nest_9, nest = "/a", route = "/a/", expected = "/a/a/"); nested_route_test!(nest_11, nest = "/a/", route = "/", expected = "/a/");