Update matchit and fix nesting inconsistencies

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
     }
 

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),
             )

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
 

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"

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;

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]

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<B = Body> {
     routes: HashMap<RouteId, Endpoint<B>>,
     node: Node,
     fallback: Fallback<B>,
-    nested_at_root: bool,
 }
 
 impl<B> Clone for Router<B> {
@@ -75,7 +74,6 @@ impl<B> Clone for Router<B> {
             routes: self.routes.clone(),
             node: self.node.clone(),
             fallback: self.fallback.clone(),
-            nested_at_root: self.nested_at_root,
         }
     }
 }
@@ -95,13 +93,11 @@ impl<B> fmt::Debug for Router<B> {
             .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<B> Router<B>
 where
@@ -116,7 +112,6 @@ where
             routes: Default::default(),
             node: Default::default(),
             fallback: Fallback::Default(Route::new(NotFound)),
-            nested_at_root: false,
         }
     }
 
@@ -163,20 +158,17 @@ 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);
 
         self
     }
 
+    // TODO(david): update docs
     #[doc = include_str!("../docs/routing/nest.md")]
-    pub fn nest<T>(mut self, mut path: &str, svc: T) -> Self
-    where
-        T: Service<Request<B>, Response = Response, Error = Infallible> + Clone + Send + 'static,
-        T::Future: Send + 'static,
-    {
+    pub fn nest(mut self, mut path: &str, router: Router<B>) -> Self {
         if path.is_empty() {
             // nesting at `""` and `"/"` should mean the same thing
             path = "/";
@@ -188,65 +180,74 @@ where
 
         let prefix = path;
 
-        if path == "/" {
-            self.nested_at_root = true;
+        let Router {
+            mut routes,
+            node,
+            fallback,
+        } = router;
+
+        if let Fallback::Custom(_) = fallback {
+            panic!("Cannot nest `Router`s that has a fallback");
         }
 
-        match try_downcast::<Router<B>, _>(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");
-                }
+        for (id, nested_path) in node.route_id_to_path {
+            let route = routes.remove(&id).unwrap();
+            let full_path: Cow<str> = 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)),
+            };
+        }
 
-                for (id, nested_path) in node.route_id_to_path {
-                    let route = routes.remove(&id).unwrap();
-                    let full_path: Cow<str> = 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());
 
-                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 = self.route(&path, strip_prefix::StripPrefix::new(svc, prefix));
-            }
+    /// TODO
+    pub fn nest_service<T>(mut self, mut path: &str, svc: T) -> Self
+    where
+        T: Service<Request<B>, Response = Response, Error = Infallible> + Clone + Send + 'static,
+        T::Future: Send + 'static,
+    {
+        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 path = if path.ends_with('/') {
+            format!("{}*{}", path, NEST_TAIL_PARAM)
+        } else {
+            format!("{}/*{}", path, NEST_TAIL_PARAM)
+        };
+
+        let svc = strip_prefix::StripPrefix::new(svc, prefix);
+        self = self.route(&path, svc.clone());
+
+        // `/*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::<MatchedPath>() {
-                // 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<B> Service<Request<B>> for Router<B>