Update matchit and fix nesting inconsistencies (#1086)

* Break `Router::nest` into `nest` and `nest_service` methods

* fix doc tests

* update docs

* fix

* Only accept `Router` in `Resource::{nest, nest_collection}`

* update changelog

* fix docs

* fix `MatchedPath` with `Router`s nested with `nest_service`

* Apply suggestions from code review

Co-authored-by: Jonas Platte <[email protected]>

* adjust docs for fallbacks

* Always nest services as opaque

* fix old docs reference

* more tests for `MatchedPath` with nested handlers

* minor clean up

* use identifier captures in format strings

* Apply suggestions from code review

Co-authored-by: Jonas Platte <[email protected]>

* fix test

Co-authored-by: Jonas Platte <[email protected]>

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

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<u64>| async {})
 ///     // `DELETE /users/:users_id`
-///     .destroy(|Path(user_id): Path<u64>| 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<u64>| 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<u64>| async {});
 ///
 /// let app = Router::new().merge(users);
 /// # let _: Router<axum::body::Body> = 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<T>(mut self, svc: T) -> Self
-    where
-        T: Service<Request<B>, 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<T>(mut self, svc: T) -> Self
-    where
-        T: Service<Request<B>, 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<u64>| async move { format!("users#show id={}", id) })
             .edit(|Path(id): Path<u64>| async move { format!("users#edit id={}", id) })
             .update(|Path(id): Path<u64>| async move { format!("users#update id={}", id) })
-            .destroy(|Path(id): Path<u64>| async move { format!("users#destroy id={}", id) })
-            .nest(Router::new().route(
-                "/tweets",
-                get(|Path(id): Path<u64>| async move { format!("users#tweets id={}", id) }),
-            ))
-            .nest_collection(
-                Router::new().route("/featured", get(|| async move { "users#featured" })),
-            );
+            .destroy(|Path(id): Path<u64>| 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 {

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<Infallible>` 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

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"

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<Value>) {
+    (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

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

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");
+    }
 }

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]