From 1d94d75c579f1b721d93cd0ff50798f173c47a15 Mon Sep 17 00:00:00 2001 From: David Pedersen Date: Thu, 18 Nov 2021 22:19:06 +0100 Subject: [PATCH] Panic instead of silently discarding fallbacks (#529) This introduces two new possible panics when constructing routers: - If merging two routers that each have a fallback. Previously that left side fallback would be silently discarded. - If nesting a router that has a fallback. Previously it would be silently discarded. Overall this should make things more explicit and users shouldn't have to worry "why isn't my fallback" working. Fixes #488 --- axum/CHANGELOG.md | 5 ++++ axum/src/docs/routing/fallback.md | 42 ------------------------------ axum/src/docs/routing/merge.md | 5 ++++ axum/src/docs/routing/nest.md | 2 ++ axum/src/routing/mod.rs | 11 +++++--- axum/src/routing/tests/fallback.rs | 22 ---------------- axum/src/routing/tests/mod.rs | 18 +++++++++++++ 7 files changed, 38 insertions(+), 67 deletions(-) diff --git a/axum/CHANGELOG.md b/axum/CHANGELOG.md index 7ae31e5f..4b97aebb 100644 --- a/axum/CHANGELOG.md +++ b/axum/CHANGELOG.md @@ -28,10 +28,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **breaking:** The `Handler` trait is now defined as `Handler`. That is the type parameters have been swapped and `B` defaults to `axum::body::Body` ([#527]) +- **breaking:** `Router::merge` will panic if both routers have fallbacks. + Previously the left side fallback would be silently discarded ([#529]) +- **breaking:** `Router::nest` will panic if the nested router has a fallback. + Previously it would be silently discarded ([#529]) - Update WebSockets to use tokio-tungstenite 0.16 ([#525]) [#525]: https://github.com/tokio-rs/axum/pull/525 [#527]: https://github.com/tokio-rs/axum/pull/527 +[#529]: https://github.com/tokio-rs/axum/pull/529 [#534]: https://github.com/tokio-rs/axum/pull/534 # 0.3.3 (13. November, 2021) diff --git a/axum/src/docs/routing/fallback.md b/axum/src/docs/routing/fallback.md index 6648db16..78cac4e1 100644 --- a/axum/src/docs/routing/fallback.md +++ b/axum/src/docs/routing/fallback.md @@ -26,45 +26,3 @@ async fn fallback(uri: Uri) -> impl IntoResponse { Fallbacks only apply to routes that aren't matched by anything in the router. If a handler is matched by a request but returns 404 the fallback is not called. - -## When used with `Router::merge` - -If a router with a fallback is merged with another router that also has -a fallback the fallback of the second router takes precedence: - -```rust -use axum::{ - Router, - routing::get, - handler::Handler, - response::IntoResponse, - http::{StatusCode, Uri}, -}; - -let one = Router::new() - .route("/one", get(|| async {})) - .fallback(fallback_one.into_service()); - -let two = Router::new() - .route("/two", get(|| async {})) - .fallback(fallback_two.into_service()); - -let app = one.merge(two); - -async fn fallback_one() -> impl IntoResponse {} -async fn fallback_two() -> impl IntoResponse {} - -// the fallback for `app` is `fallback_two` -# async { -# hyper::Server::bind(&"".parse().unwrap()).serve(app.into_make_service()).await.unwrap(); -# }; -``` - -If only one of the routers have a fallback that will be used in the -merged router. - -## When used with `Router::nest` - -If a router with a fallback is nested inside another router the fallback -of the nested router will be discarded and not used. This is such that -the outer router's fallback takes precedence. diff --git a/axum/src/docs/routing/merge.md b/axum/src/docs/routing/merge.md index 78263029..a8038d38 100644 --- a/axum/src/docs/routing/merge.md +++ b/axum/src/docs/routing/merge.md @@ -36,3 +36,8 @@ let app = Router::new() # hyper::Server::bind(&"".parse().unwrap()).serve(app.into_make_service()).await.unwrap(); # }; ``` + +## Panics + +- If two routers that each have a [fallback](Router::fallback) are merged. This + is because `Router` only allows a single fallback. diff --git a/axum/src/docs/routing/nest.md b/axum/src/docs/routing/nest.md index ed7a727c..947ac573 100644 --- a/axum/src/docs/routing/nest.md +++ b/axum/src/docs/routing/nest.md @@ -121,5 +121,7 @@ let app = Router::new() for more details. - If the route contains a wildcard (`*`). - If `path` is empty. +- If the nested router has a [fallback](Router::fallback). This is because + `Router` only allows a single fallback. [`OriginalUri`]: crate::extract::OriginalUri diff --git a/axum/src/routing/mod.rs b/axum/src/routing/mod.rs index 261208a0..7448b25b 100644 --- a/axum/src/routing/mod.rs +++ b/axum/src/routing/mod.rs @@ -189,14 +189,17 @@ where let Router { mut routes, node, - // discard the fallback of the nested router - fallback: _, + 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 = if &*nested_path == "/" { @@ -253,7 +256,9 @@ where (Fallback::Default(_), pick @ Fallback::Default(_)) => pick, (Fallback::Default(_), pick @ Fallback::Custom(_)) => pick, (pick @ Fallback::Custom(_), Fallback::Default(_)) => pick, - (Fallback::Custom(_), pick @ Fallback::Custom(_)) => pick, + (Fallback::Custom(_), Fallback::Custom(_)) => { + panic!("Cannot merge two `Router`s that both have a fallback") + } }; self.nested_at_root = self.nested_at_root || nested_at_root; diff --git a/axum/src/routing/tests/fallback.rs b/axum/src/routing/tests/fallback.rs index 8107c13f..49874077 100644 --- a/axum/src/routing/tests/fallback.rs +++ b/axum/src/routing/tests/fallback.rs @@ -49,25 +49,3 @@ async fn or() { assert_eq!(res.status(), StatusCode::OK); assert_eq!(res.text().await, "fallback"); } - -#[tokio::test] -async fn fallback_on_or() { - let one = Router::new() - .route("/one", get(|| async {})) - .fallback((|| async { "fallback one" }).into_service()); - - let two = Router::new() - .route("/two", get(|| async {})) - .fallback((|| async { "fallback two" }).into_service()); - - let app = one.merge(two); - - let client = TestClient::new(app); - - assert_eq!(client.get("/one").send().await.status(), StatusCode::OK); - assert_eq!(client.get("/two").send().await.status(), StatusCode::OK); - - let res = client.get("/does-not-exist").send().await; - assert_eq!(res.status(), StatusCode::OK); - assert_eq!(res.text().await, "fallback two"); -} diff --git a/axum/src/routing/tests/mod.rs b/axum/src/routing/tests/mod.rs index f74f2ca7..66892899 100644 --- a/axum/src/routing/tests/mod.rs +++ b/axum/src/routing/tests/mod.rs @@ -566,3 +566,21 @@ async fn different_methods_added_in_different_routes_deeply_nested() { 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() { + async fn fallback() {} + let one = Router::new().fallback(fallback.into_service()); + let two = Router::new().fallback(fallback.into_service()); + 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); +}