From 34942f3d708f78b318793c1622fe2b1d36df8f73 Mon Sep 17 00:00:00 2001
From: David Pedersen <david.pdrsn@gmail.com>
Date: Thu, 11 Nov 2021 19:22:25 +0100
Subject: [PATCH] Improve error message for `nest("/", _)` (#500)

---
 axum/src/routing/mod.rs       | 31 +++++++++++++++++++++++++++++--
 axum/src/routing/tests/mod.rs | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 62 insertions(+), 2 deletions(-)

diff --git a/axum/src/routing/mod.rs b/axum/src/routing/mod.rs
index a4af3269..54e70952 100644
--- a/axum/src/routing/mod.rs
+++ b/axum/src/routing/mod.rs
@@ -66,6 +66,7 @@ pub struct Router<B = Body> {
     routes: HashMap<RouteId, Route<B>>,
     node: Node,
     fallback: Fallback<B>,
+    nested_at_root: bool,
 }
 
 impl<B> Clone for Router<B> {
@@ -74,6 +75,7 @@ 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,
         }
     }
 }
@@ -103,6 +105,7 @@ where
             routes: Default::default(),
             node: Default::default(),
             fallback: Fallback::Default(Route::new(NotFound)),
+            nested_at_root: false,
         }
     }
 
@@ -129,7 +132,7 @@ where
         let id = RouteId::next();
 
         if let Err(err) = self.node.insert(path, id) {
-            panic!("Invalid route: {}", err);
+            self.panic_on_matchit_error(err);
         }
 
         self.routes.insert(id, Route::new(service));
@@ -156,6 +159,10 @@ where
 
         let prefix = path;
 
+        if path == "/" {
+            self.nested_at_root = true;
+        }
+
         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
@@ -166,6 +173,10 @@ where
                     node,
                     // discard the fallback of the nested router
                     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;
 
                 for (id, nested_path) in node.paths {
@@ -201,10 +212,11 @@ where
             routes,
             node,
             fallback,
+            nested_at_root,
         } = other;
 
         if let Err(err) = self.node.merge(node) {
-            panic!("Invalid route: {}", err);
+            self.panic_on_matchit_error(err);
         }
 
         for (id, route) in routes {
@@ -218,6 +230,8 @@ where
             (Fallback::Custom(_), pick @ Fallback::Custom(_)) => pick,
         };
 
+        self.nested_at_root = self.nested_at_root || nested_at_root;
+
         self
     }
 
@@ -253,6 +267,7 @@ where
             routes,
             node: self.node,
             fallback,
+            nested_at_root: self.nested_at_root,
         }
     }
 
@@ -286,6 +301,7 @@ where
             routes,
             node: self.node,
             fallback: self.fallback,
+            nested_at_root: self.nested_at_root,
         }
     }
 
@@ -380,6 +396,17 @@ where
 
         RouterFuture::from_oneshot(route.oneshot(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>
diff --git a/axum/src/routing/tests/mod.rs b/axum/src/routing/tests/mod.rs
index 81b7bcb7..af2ecfdc 100644
--- a/axum/src/routing/tests/mod.rs
+++ b/axum/src/routing/tests/mod.rs
@@ -504,3 +504,36 @@ async fn route_layer() {
     let res = client.post("/foo").send().await;
     assert_eq!(res.status(), StatusCode::UNAUTHORIZED);
 }
+
+#[tokio::test]
+#[should_panic(
+    expected = "Invalid route: insertion failed due to conflict with previously registered route: /foo"
+)]
+async fn conflicting_route() {
+    let app = Router::new()
+        .route("/foo", get(|| async {}))
+        .route("/foo", get(|| async {}));
+    TestClient::new(app);
+}
+
+#[tokio::test]
+#[should_panic(
+    expected = "Invalid route: insertion failed due to conflict with previously registered route: /*axum_nest. 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: /*axum_nest. 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);
+}