diff --git a/axum-extra/CHANGELOG.md b/axum-extra/CHANGELOG.md index b1dd34c359..932c55f5a7 100644 --- a/axum-extra/CHANGELOG.md +++ b/axum-extra/CHANGELOG.md @@ -9,12 +9,16 @@ and this project adheres to [Semantic Versioning]. - **added:** Add `RouterExt::route_with_tsr` for adding routes with an additional "trailing slash redirect" route ([#1119]) +- **breaking:** `Resource::nest` and `Resource::nest_collection` has been + removed. You can instead convert the `Resource` into a `Router` and + add additional routes as necessary ([#1086]) - **changed:** For methods that accept some `S: Service`, the bounds have been relaxed so the response type must implement `IntoResponse` rather than being a literal `Response` - **added:** Support chaining handlers with `HandlerCallWithExtractors::or` ([#1170]) - **change:** axum-extra's MSRV is now 1.60 ([#1239]) +[#1086]: https://github.com/tokio-rs/axum/pull/1086 [#1119]: https://github.com/tokio-rs/axum/pull/1119 [#1170]: https://github.com/tokio-rs/axum/pull/1170 [#1239]: https://github.com/tokio-rs/axum/pull/1239 diff --git a/axum-extra/src/routing/resource.rs b/axum-extra/src/routing/resource.rs index 18e1bde8d5..910410fb01 100644 --- a/axum-extra/src/routing/resource.rs +++ b/axum-extra/src/routing/resource.rs @@ -31,18 +31,7 @@ use tower_service::Service; /// // `PUT or PATCH /users/:users_id` /// .update(|Path(user_id): Path| async {}) /// // `DELETE /users/:users_id` -/// .destroy(|Path(user_id): Path| async {}) -/// // Nest another router at the "member level" -/// // This defines a route for `GET /users/:users_id/tweets` -/// .nest(Router::new().route( -/// "/tweets", -/// get(|Path(user_id): Path| async {}), -/// )) -/// // Nest another router at the "collection level" -/// // This defines a route for `GET /users/featured` -/// .nest_collection( -/// Router::new().route("/featured", get(|| async {})), -/// ); +/// .destroy(|Path(user_id): Path| async {}); /// /// let app = Router::new().merge(users); /// # let _: Router = app; @@ -136,34 +125,6 @@ where self.route(&path, delete(handler)) } - /// Nest another route at the "member level". - /// - /// The routes will be nested at `/{resource_name}/:{resource_name}_id`. - pub fn nest(mut self, svc: T) -> Self - where - T: Service, Error = Infallible> + Clone + Send + 'static, - T::Response: IntoResponse, - T::Future: Send + 'static, - { - let path = self.show_update_destroy_path(); - self.router = self.router.nest(&path, svc); - self - } - - /// Nest another route at the "collection level". - /// - /// The routes will be nested at `/{resource_name}`. - pub fn nest_collection(mut self, svc: T) -> Self - where - T: Service, Error = Infallible> + Clone + Send + 'static, - T::Response: IntoResponse, - T::Future: Send + 'static, - { - let path = self.index_create_path(); - self.router = self.router.nest(&path, svc); - self - } - fn index_create_path(&self) -> String { format!("/{}", self.name) } @@ -214,14 +175,7 @@ mod tests { .show(|Path(id): Path| async move { format!("users#show id={}", id) }) .edit(|Path(id): Path| async move { format!("users#edit id={}", id) }) .update(|Path(id): Path| async move { format!("users#update id={}", id) }) - .destroy(|Path(id): Path| async move { format!("users#destroy id={}", id) }) - .nest(Router::new().route( - "/tweets", - get(|Path(id): Path| async move { format!("users#tweets id={}", id) }), - )) - .nest_collection( - Router::new().route("/featured", get(|| async move { "users#featured" })), - ); + .destroy(|Path(id): Path| async move { format!("users#destroy id={}", id) }); let mut app = Router::new().merge(users); @@ -264,16 +218,6 @@ mod tests { call_route(&mut app, Method::DELETE, "/users/1").await, "users#destroy id=1" ); - - assert_eq!( - call_route(&mut app, Method::GET, "/users/1/tweets").await, - "users#tweets id=1" - ); - - assert_eq!( - call_route(&mut app, Method::GET, "/users/featured").await, - "users#featured" - ); } async fn call_route(app: &mut Router, method: Method, uri: &str) -> String { diff --git a/axum/CHANGELOG.md b/axum/CHANGELOG.md index 9034c900a0..de706cfe8d 100644 --- a/axum/CHANGELOG.md +++ b/axum/CHANGELOG.md @@ -12,6 +12,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 Use `axum::middleware::from_extractor` instead ([#1077]) - **breaking:** Allow `Error: Into` for `Route::{layer, route_layer}` ([#924]) - **breaking:** `MethodRouter` now panics on overlapping routes ([#1102]) +- **breaking:** Nested `Router`s will no longer delegate to the outer `Router`'s + fallback. Instead you must explicitly set a fallback on the inner `Router` ([#1086]) +- **breaking:** The request `/foo/` no longer matches `/foo/*rest`. If you want + to match `/foo/` you have to add a route specifically for that ([#1086]) +- **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` ([#1086]) +- **fixed:** Routes like `/foo` and `/*rest` are no longer considered + overlapping. `/foo` will take priority ([#1086]) - **breaking:** Remove trailing slash redirects. Previously if you added a route for `/foo`, axum would redirect calls to `/foo/` to `/foo` (or vice versa for `/foo/`). That is no longer supported and such requests will now be sent to @@ -36,6 +45,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 [#1171]: https://github.com/tokio-rs/axum/pull/1171 [#1077]: https://github.com/tokio-rs/axum/pull/1077 +[#1086]: https://github.com/tokio-rs/axum/pull/1086 [#1088]: https://github.com/tokio-rs/axum/pull/1088 [#1102]: https://github.com/tokio-rs/axum/pull/1102 [#1119]: https://github.com/tokio-rs/axum/pull/1119 diff --git a/axum/Cargo.toml b/axum/Cargo.toml index 6f27063557..d832179cd6 100644 --- a/axum/Cargo.toml +++ b/axum/Cargo.toml @@ -38,7 +38,7 @@ 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" +matchit = "0.6" memchr = "2.4.1" mime = "0.3.16" percent-encoding = "2.1" diff --git a/axum/src/docs/routing/nest.md b/axum/src/docs/routing/nest.md index a63b14dfbc..c1d14907ef 100644 --- a/axum/src/docs/routing/nest.md +++ b/axum/src/docs/routing/nest.md @@ -1,4 +1,4 @@ -Nest a group of routes (or a [`Service`]) at some path. +Nest a [`Service`] at some path. This allows you to break your application into smaller pieces and compose them together. @@ -64,36 +64,6 @@ let app = Router::new().nest("/:version/api", users_api); # }; ``` -# Nesting services - -`nest` also accepts any [`Service`]. This can for example be used with -[`tower_http::services::ServeDir`] to serve static files from a directory: - -```rust -use axum::{ - Router, - routing::get_service, - http::StatusCode, - error_handling::HandleErrorLayer, -}; -use std::{io, convert::Infallible}; -use tower_http::services::ServeDir; - -// Serves files inside the `public` directory at `GET /public/*` -let serve_dir_service = get_service(ServeDir::new("public")) - .handle_error(|error: io::Error| async move { - ( - StatusCode::INTERNAL_SERVER_ERROR, - format!("Unhandled internal error: {}", error), - ) - }); - -let app = Router::new().nest("/public", serve_dir_service); -# async { -# axum::Server::bind(&"".parse().unwrap()).serve(app.into_make_service()).await.unwrap(); -# }; -``` - # Differences to wildcard routes Nested routes are similar to wildcard routes. The difference is that @@ -103,18 +73,73 @@ the prefix stripped: ```rust use axum::{routing::get, http::Uri, Router}; +let nested_router = Router::new() + .route("/", get(|uri: Uri| async { + // `uri` will _not_ contain `/bar` + })); + let app = Router::new() .route("/foo/*rest", get(|uri: Uri| async { // `uri` will contain `/foo` })) - .nest("/bar", get(|uri: Uri| async { - // `uri` will _not_ contain `/bar` - })); + .nest("/bar", nested_router); # async { # axum::Server::bind(&"".parse().unwrap()).serve(app.into_make_service()).await.unwrap(); # }; ``` +# Fallbacks + +When nesting a router, if a request matches the prefix but the nested router doesn't have a matching +route, the outer fallback will _not_ be called: + +```rust +use axum::{routing::get, http::StatusCode, handler::Handler, Router}; + +async fn fallback() -> (StatusCode, &'static str) { + (StatusCode::NOT_FOUND, "Not Found") +} + +let api_routes = Router::new().nest("/users", get(|| async {})); + +let app = Router::new() + .nest("/api", api_routes) + .fallback(fallback.into_service()); +# let _: Router = app; +``` + +Here requests like `GET /api/not-found` will go into `api_routes` and then to +the fallback of `api_routes` which will return an empty `404 Not Found` +response. The outer fallback declared on `app` will _not_ be called. + +Think of nested services as swallowing requests that matches the prefix and +not falling back to outer router even if they don't have a matching route. + +You can still add separate fallbacks to nested routers: + +```rust +use axum::{routing::get, http::StatusCode, handler::Handler, Json, Router}; +use serde_json::{json, Value}; + +async fn fallback() -> (StatusCode, &'static str) { + (StatusCode::NOT_FOUND, "Not Found") +} + +async fn api_fallback() -> (StatusCode, Json) { + (StatusCode::NOT_FOUND, Json(json!({ "error": "Not Found" }))) +} + +let api_routes = Router::new() + .nest("/users", get(|| async {})) + // add dedicated fallback for requests starting with `/api` + .fallback(api_fallback.into_service()); + +let app = Router::new() + .nest("/api", api_routes) + .fallback(fallback.into_service()); +# let _: Router = app; +``` + # Panics - If the route overlaps with another route. See [`Router::route`] @@ -125,3 +150,4 @@ for more details. `Router` only allows a single fallback. [`OriginalUri`]: crate::extract::OriginalUri +[fallbacks]: Router::fallback diff --git a/axum/src/docs/routing/route.md b/axum/src/docs/routing/route.md index e0a753a005..cd9d703e1b 100644 --- a/axum/src/docs/routing/route.md +++ b/axum/src/docs/routing/route.md @@ -51,6 +51,8 @@ Examples: - `/:id/:repo/*tree` Wildcard captures can also be extracted using [`Path`](crate::extract::Path). +Note that the leading slash is not included, i.e. for the route `/foo/*rest` and +the path `/foo/bar/baz` the value of `rest` will be `bar/baz`. # Accepting multiple methods @@ -184,22 +186,6 @@ let app = Router::new() The static route `/foo` and the dynamic route `/:key` are not considered to overlap and `/foo` will take precedence. -Take care when using [`Router::nest`] as it behaves like a wildcard route. -Therefore this setup panics: - -```rust,should_panic -use axum::{routing::get, Router}; - -let app = Router::new() - // this is similar to `/api/*` - .nest("/api", get(|| async {})) - // which overlaps with this route - .route("/api/users", get(|| async {})); -# async { -# axum::Server::bind(&"".parse().unwrap()).serve(app.into_make_service()).await.unwrap(); -# }; -``` - Also panics if `path` is empty. ## Nesting diff --git a/axum/src/extract/matched_path.rs b/axum/src/extract/matched_path.rs index 8965bd30bf..d0cb01afd6 100644 --- a/axum/src/extract/matched_path.rs +++ b/axum/src/extract/matched_path.rs @@ -85,8 +85,9 @@ where mod tests { use super::*; use crate::{extract::Extension, handler::Handler, routing::get, test_helpers::*, Router}; - use http::Request; + use http::{Request, StatusCode}; use std::task::{Context, Poll}; + use tower::layer::layer_fn; use tower_service::Service; #[derive(Clone)] @@ -147,25 +148,37 @@ mod tests { .nest("/api", api) .nest( "/public", - Router::new().route("/assets/*path", get(handler)), + Router::new() + .route("/assets/*path", get(handler)) + // have to set the middleware here since otherwise the + // matched path is just `/public/*` since we're nesting + // this router + .layer(layer_fn(SetMatchedPathExtension)), ) .nest("/foo", handler.into_service()) - .layer(tower::layer::layer_fn(SetMatchedPathExtension)); + .layer(layer_fn(SetMatchedPathExtension)); let client = TestClient::new(app); - let res = client.get("/foo").send().await; - assert_eq!(res.text().await, "/:key"); - let res = client.get("/api/users/123").send().await; assert_eq!(res.text().await, "/api/users/:id"); + // the router nested at `/public` doesn't handle `/` + let res = client.get("/public").send().await; + assert_eq!(res.status(), StatusCode::NOT_FOUND); + let res = client.get("/public/assets/css/style.css").send().await; assert_eq!( res.text().await, "extractor = /public/assets/*path, middleware = /public/assets/*path" ); + let res = client.get("/foo").send().await; + assert_eq!(res.text().await, "extractor = /foo, middleware = /foo"); + + let res = client.get("/foo/").send().await; + assert_eq!(res.text().await, "extractor = /foo/, middleware = /foo/"); + let res = client.get("/foo/bar/baz").send().await; assert_eq!( res.text().await, @@ -176,4 +189,20 @@ mod tests { ), ); } + + #[tokio::test] + async fn nested_opaque_routers_append_to_matched_path() { + let app = Router::new().nest( + "/:a", + Router::new().route( + "/:b", + get(|path: MatchedPath| async move { path.as_str().to_owned() }), + ), + ); + + let client = TestClient::new(app); + + let res = client.get("/foo/bar").send().await; + assert_eq!(res.text().await, "/:a/:b"); + } } diff --git a/axum/src/extract/path/mod.rs b/axum/src/extract/path/mod.rs index f559ae1fb0..20a23ce70b 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 eda58c732c..01db8e55ab 100644 --- a/axum/src/routing/mod.rs +++ b/axum/src/routing/mod.rs @@ -5,21 +5,19 @@ use crate::{ body::{Body, HttpBody}, extract::connect_info::IntoMakeServiceWithConnectInfo, response::Response, - routing::strip_prefix::StripPrefix, util::try_downcast, }; use axum_core::response::IntoResponse; use http::Request; use matchit::MatchError; use std::{ - borrow::Cow, collections::HashMap, convert::Infallible, fmt, sync::Arc, task::{Context, Poll}, }; -use tower::{layer::layer_fn, util::MapResponseLayer, ServiceBuilder}; +use tower::{util::MapResponseLayer, ServiceBuilder}; use tower_layer::Layer; use tower_service::Service; @@ -65,7 +63,6 @@ pub struct Router { routes: HashMap>, node: Arc, fallback: Fallback, - nested_at_root: bool, } impl Clone for Router { @@ -74,7 +71,6 @@ impl Clone for Router { routes: self.routes.clone(), node: Arc::clone(&self.node), fallback: self.fallback.clone(), - nested_at_root: self.nested_at_root, } } } @@ -94,7 +90,6 @@ 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() } } @@ -115,7 +110,6 @@ where routes: Default::default(), node: Default::default(), fallback: Fallback::Default(Route::new(NotFound)), - nested_at_root: false, } } @@ -177,7 +171,7 @@ where let mut node = Arc::try_unwrap(Arc::clone(&self.node)).unwrap_or_else(|node| (*node).clone()); if let Err(err) = node.insert(path, id) { - self.panic_on_matchit_error(err); + panic!("Invalid route {path:?}: {err}"); } self.node = Arc::new(node); } @@ -200,63 +194,22 @@ where let prefix = path; - if path == "/" { - self.nested_at_root = true; - } - - 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; - - if let Fallback::Custom(_) = fallback { - panic!("Cannot nest `Router`s that has a fallback"); - } + let path = if path.ends_with('/') { + format!("{path}*{NEST_TAIL_PARAM}") + } else { + format!("{path}/*{NEST_TAIL_PARAM}") + }; - 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)) - } - }; - } + let svc = strip_prefix::StripPrefix::new(svc, prefix); + self = self.route(&path, svc.clone()); - 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()); + if !prefix.ends_with('/') { + // same goes for `/foo/`, that should also match + self = self.route(&format!("{prefix}/"), svc); } self @@ -271,7 +224,6 @@ where routes, node, fallback, - nested_at_root, } = other.into(); for (id, route) in routes { @@ -294,8 +246,6 @@ where } }; - self.nested_at_root = self.nested_at_root || nested_at_root; - self } @@ -334,7 +284,6 @@ where routes, node: self.node, fallback, - nested_at_root: self.nested_at_root, } } @@ -371,7 +320,6 @@ where routes, node: self.node, fallback: self.fallback, - nested_at_root: self.nested_at_root, } } @@ -474,17 +422,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/merge.rs b/axum/src/routing/tests/merge.rs index 58af01511b..afb0bcb94f 100644 --- a/axum/src/routing/tests/merge.rs +++ b/axum/src/routing/tests/merge.rs @@ -240,7 +240,7 @@ async fn all_the_uris( #[tokio::test] async fn nesting_and_seeing_the_right_uri() { - let one = Router::new().nest("/foo", Router::new().route("/bar", get(all_the_uris))); + let one = Router::new().nest("/foo/", Router::new().route("/bar", get(all_the_uris))); let two = Router::new().route("/foo", get(all_the_uris)); let client = TestClient::new(one.merge(two)); @@ -271,7 +271,7 @@ async fn nesting_and_seeing_the_right_uri() { #[tokio::test] async fn nesting_and_seeing_the_right_uri_at_more_levels_of_nesting() { let one = Router::new().nest( - "/foo", + "/foo/", Router::new().nest("/bar", Router::new().route("/baz", get(all_the_uris))), ); let two = Router::new().route("/foo", get(all_the_uris)); diff --git a/axum/src/routing/tests/mod.rs b/axum/src/routing/tests/mod.rs index cd8608cf7c..b7f53cb163 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}, @@ -359,44 +358,23 @@ async fn with_and_without_trailing_slash() { // 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(); + let client = TestClient::new(app); + + let res = client.get("/x").send().await; assert_eq!(res.status(), StatusCode::NOT_FOUND); - // 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/foo/bar").send().await; + assert_eq!(res.status(), StatusCode::OK); + assert_eq!(res.text().await, "foo/bar"); } #[tokio::test] @@ -500,34 +478,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() @@ -545,29 +495,6 @@ async fn different_methods_added_in_different_routes() { assert_eq!(body, "POST"); } -#[tokio::test] -async fn different_methods_added_in_different_routes_deeply_nested() { - let app = Router::new() - .route("/foo/bar/baz", get(|| async { "GET" })) - .nest( - "/foo", - Router::new().nest( - "/bar", - Router::new().route("/baz", post(|| async { "POST" })), - ), - ); - - let client = TestClient::new(app); - - let res = client.get("/foo/bar/baz").send().await; - let body = res.text().await; - assert_eq!(body, "GET"); - - let res = client.post("/foo/bar/baz").send().await; - let body = res.text().await; - assert_eq!(body, "POST"); -} - #[tokio::test] #[should_panic(expected = "Cannot merge two `Router`s that both have a fallback")] async fn merging_routers_with_fallbacks_panics() { @@ -577,15 +504,6 @@ async fn merging_routers_with_fallbacks_panics() { TestClient::new(one.merge(two)); } -#[tokio::test] -#[should_panic(expected = "Cannot nest `Router`s that has a fallback")] -async fn nesting_router_with_fallbacks_panics() { - async fn fallback() {} - let one = Router::new().fallback(fallback.into_service()); - let app = Router::new().nest("/", one); - TestClient::new(app); -} - #[tokio::test] async fn merging_routers_with_same_paths_but_different_methods() { let one = Router::new().route("/", get(|| async { "GET" })); diff --git a/axum/src/routing/tests/nest.rs b/axum/src/routing/tests/nest.rs index d4cb466c56..cabadaf545 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; @@ -17,7 +14,6 @@ async fn nesting_apps() { "/users/:id", get( |params: extract::Path>| async move { - dbg!(¶ms); format!( "{}: users#show ({})", params.get("version").unwrap(), @@ -239,38 +235,12 @@ async fn nested_multiple_routes() { assert_eq!(client.get("/api/teams").send().await.text().await, "teams"); } -#[tokio::test] -async fn nested_with_other_route_also_matching_with_route_first() { - let app = Router::new().route("/api", get(|| async { "api" })).nest( - "/api", - Router::new() - .route("/users", get(|| async { "users" })) - .route("/teams", get(|| async { "teams" })), - ); - - let client = TestClient::new(app); - - assert_eq!(client.get("/api").send().await.text().await, "api"); - assert_eq!(client.get("/api/users").send().await.text().await, "users"); - assert_eq!(client.get("/api/teams").send().await.text().await, "teams"); -} - -#[tokio::test] -async fn nested_with_other_route_also_matching_with_route_last() { - let app = Router::new() - .nest( - "/api", - Router::new() - .route("/users", get(|| async { "users" })) - .route("/teams", get(|| async { "teams" })), - ) - .route("/api", get(|| async { "api" })); - - let client = TestClient::new(app); - - assert_eq!(client.get("/api").send().await.text().await, "api"); - assert_eq!(client.get("/api/users").send().await.text().await, "users"); - assert_eq!(client.get("/api/teams").send().await.text().await, "teams"); +#[test] +#[should_panic = "Invalid route \"/\": insertion failed due to conflict with previously registered route: /*__private__axum_nest_tail_param"] +fn nested_at_root_with_other_routes() { + let _: Router = Router::new() + .nest("/", Router::new().route("/users", get(|| async {}))) + .route("/", get(|| async {})); } #[tokio::test] @@ -366,64 +336,97 @@ 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("/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); +} + +#[tokio::test] +async fn doesnt_call_outer_fallback() { + let app = Router::new() + .nest("/foo", Router::new().route("/", get(|| async {}))) + .fallback((|| async { (StatusCode::NOT_FOUND, "outer fallback") }).into_service()); + + let client = TestClient::new(app); + + let res = client.get("/foo").send().await; + assert_eq!(res.status(), StatusCode::OK); + + let res = client.get("/foo/not-found").send().await; + assert_eq!(res.status(), StatusCode::NOT_FOUND); + // the default fallback returns an empty body + assert_eq!(res.text().await, ""); +} + +#[tokio::test] +async fn nesting_with_root_inner_router() { + let app = Router::new().nest( + "/foo", + Router::new().route("/", get(|| async { "inner route" })), + ); + + let client = TestClient::new(app); + + // `/foo/` does match the `/foo` prefix and the remaining path is technically + // empty, which is the same as `/` which matches `.route("/", _)` + let res = client.get("/foo").send().await; + assert_eq!(res.status(), StatusCode::OK); + + // `/foo/` does match the `/foo` prefix and the remaining path is `/` + // which matches `.route("/", _)` + let res = client.get("/foo/").send().await; + assert_eq!(res.status(), StatusCode::OK); +} + +#[tokio::test] +async fn fallback_on_inner() { + let app = Router::new() + .nest( + "/foo", + Router::new() + .route("/", get(|| async {})) + .fallback((|| async { (StatusCode::NOT_FOUND, "inner fallback") }).into_service()), + ) + .fallback((|| async { (StatusCode::NOT_FOUND, "outer fallback") }).into_service()); + + let client = TestClient::new(app); + + let res = client.get("/foo/not-found").send().await; + assert_eq!(res.status(), StatusCode::NOT_FOUND); + assert_eq!(res.text().await, "inner fallback"); +} + 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 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); - } } }; - - ( - $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,23 +436,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/"); - -// TODO(david): we'll revisit this in https://github.com/tokio-rs/axum/pull/1086 -//// 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/");