Fix routing for opaque nested services (#842)

* Fix routing for opaque nested services

* Also test `MatchedPath`

* clean up and more comments

axum/CHANGELOG.md

@@ -70,6 +70,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   requests ([#734])
 - **fixed:** Fix wrong `content-length` for `HEAD` requests to endpoints that returns chunked
   responses ([#755])
+- **fixed:** Fixed several routing bugs related to nested "opaque" tower services (i.e.
+  non-`Router` services) ([#841] and [#842])
 - **changed:** Update to tokio-tungstenite 0.17 ([#791])
 
 [#644]: https://github.com/tokio-rs/axum/pull/644
@@ -89,6 +91,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 [#819]: https://github.com/tokio-rs/axum/pull/819
 [#823]: https://github.com/tokio-rs/axum/pull/823
 [#824]: https://github.com/tokio-rs/axum/pull/824
+[#841]: https://github.com/tokio-rs/axum/pull/841
+[#842]: https://github.com/tokio-rs/axum/pull/842
 
 # 0.4.4 (13. January, 2022)
 

axum/src/routing/mod.rs

@@ -237,8 +237,8 @@ where
             }
             // otherwise we add a wildcard route to the service
             Err(svc) => {
-                let path = if path == "/" {
+                let path = if path.ends_with('/') {
-                    format!("/*{}", NEST_TAIL_PARAM)
+                    format!("{}*{}", path, NEST_TAIL_PARAM)
                 } else {
                     format!("{}/*{}", path, NEST_TAIL_PARAM)
                 };

axum/src/routing/strip_prefix.rs

@@ -42,19 +42,19 @@ where
 }
 
 fn strip_prefix(uri: &Uri, prefix: &str) -> Option<Uri> {
-    let path_and_query: http::uri::PathAndQuery = if let Some(path_and_query) = uri.path_and_query()
+    let path_and_query = uri.path_and_query()?;
-    {
+
     // Check whether the prefix matches the path and if so how long the matching prefix is.
     //
-        // # Examples
+    // For example:
     //
     // prefix = /api
-        // uri    = /api/users
+    // path   = /api/users
     //          ^^^^ this much is matched and the length is 4. Thus if we chop off the first 4
     //          characters we get the remainder
     //
     // prefix = /api/:version
-        // uri    = /api/v0/users
+    // path   = /api/v0/users
     //          ^^^^^^^ this much is matched and the length is 7.
     let mut matching_prefix_length = Some(0);
     for item in zip_longest(segments(path_and_query.path()), segments(prefix)) {
@@ -64,15 +64,35 @@ fn strip_prefix(uri: &Uri, prefix: &str) -> Option<Uri> {
         match item {
             Item::Both(path_segment, prefix_segment) => {
                 if prefix_segment.starts_with(':') || path_segment == prefix_segment {
+                    // the prefix segment is either a param, which matches anything, or
+                    // it actually matches the path segment
                     *matching_prefix_length.as_mut().unwrap() += path_segment.len();
+                } else if prefix_segment.is_empty() {
+                    // the prefix ended in a `/` so we got a match.
+                    //
+                    // For example:
+                    //
+                    // prefix = /foo/
+                    // path   = /foo/bar
+                    //
+                    // The prefix matches and the new path should be `/bar`
+                    break;
                 } else {
+                    // the prefix segment didn't match so there is no match
                     matching_prefix_length = None;
                     break;
                 }
             }
+            // the path had more segments than the prefix but we got a match.
+            //
+            // For example:
+            //
+            // prefix = /foo
+            // path   = /foo/bar
             Item::First(_) => {
                 break;
             }
+            // the prefix had more segments than the path so there is no match
             Item::Second(_) => {
                 matching_prefix_length = None;
                 break;
@@ -80,24 +100,20 @@ fn strip_prefix(uri: &Uri, prefix: &str) -> Option<Uri> {
         }
     }
 
-        let after_prefix = if let Some(idx) = matching_prefix_length {
+    // if the prefix matches it will always do so up until a `/`, it cannot match only
-            uri.path().split_at(idx).1
+    // part of a segment. Therefore this will always be at a char boundary and `split_at` wont
-        } else {
+    // panic
-            return None;
+    let after_prefix = uri.path().split_at(matching_prefix_length?).1;
-        };
 
-        match (after_prefix.starts_with('/'), path_and_query.query()) {
+    let new_path_and_query = match (after_prefix.starts_with('/'), path_and_query.query()) {
         (true, None) => after_prefix.parse().unwrap(),
         (true, Some(query)) => format!("{}?{}", after_prefix, query).parse().unwrap(),
         (false, None) => format!("/{}", after_prefix).parse().unwrap(),
         (false, Some(query)) => format!("/{}?{}", after_prefix, query).parse().unwrap(),
-        }
-    } else {
-        return None;
     };
 
     let mut parts = uri.clone().into_parts();
-    parts.path_and_query = Some(path_and_query);
+    parts.path_and_query = Some(new_path_and_query);
 
     Some(Uri::from_parts(parts).unwrap())
 }
@@ -183,7 +199,7 @@ mod tests {
         single_segment_root_prefix,
         uri = "/a",
         prefix = "/",
-        expected = None,
+        expected = Some("/a"),
     );
 
     test!(
@@ -334,6 +350,13 @@ mod tests {
         expected = Some("/"),
     );
 
+    test!(
+        param_13,
+        uri = "/a/a",
+        prefix = "/a/",
+        expected = Some("/a"),
+    );
+
     #[quickcheck]
     fn does_not_panic(uri_and_prefix: UriAndPrefix) -> bool {
         let UriAndPrefix { uri, prefix } = uri_and_prefix;

axum/src/routing/tests/nest.rs

@@ -1,8 +1,10 @@
-use tower_http::services::ServeDir;
-
 use super::*;
-use crate::{body::boxed, extract::Extension};
+use crate::{
+    body::boxed,
+    extract::{Extension, MatchedPath},
+};
 use std::collections::HashMap;
+use tower_http::services::ServeDir;
 
 #[tokio::test]
 async fn nesting_apps() {
@@ -369,18 +371,58 @@ macro_rules! nested_route_test {
         $name:ident,
         nest = $nested_path:literal,
         route = $route_path:literal,
-        expected = $expected_path:literal $(,)?
+        expected = $expected_path:literal,
+        opaque_redirect = $opaque_redirect:expr $(,)?
     ) => {
         #[tokio::test]
         async fn $name() {
-            let inner = Router::new().route($route_path, get(|| async {}));
+            let inner = Router::new().route(
+                $route_path,
+                get(|matched: MatchedPath| async move { matched.as_str().to_owned() }),
+            );
             let app = Router::new().nest($nested_path, inner);
             let client = TestClient::new(app);
-            assert_eq!(
+            let res = client.get($expected_path).send().await;
-                client.get($expected_path).send().await.status(),
+            let status = res.status();
-                StatusCode::OK
+            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,
+        );
     };
 }
 
@@ -391,7 +433,22 @@ 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/");
-nested_route_test!(nest_7, nest = "/a", route = "/", 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_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/");