diff --git a/axum-extra/CHANGELOG.md b/axum-extra/CHANGELOG.md index 4ce07962..b2e9b416 100644 --- a/axum-extra/CHANGELOG.md +++ b/axum-extra/CHANGELOG.md @@ -7,7 +7,10 @@ and this project adheres to [Semantic Versioning]. # Unreleased -- None. +- **added:** Add `RouterExt::route_with_tsr` for adding routes with an + additional "trailing slash redirect" route ([#1119]) + +[#1119]: https://github.com/tokio-rs/axum/pull/1119 # 0.3.5 (27. June, 2022) diff --git a/axum-extra/src/routing/mod.rs b/axum-extra/src/routing/mod.rs index 8e693e2b..6298a36e 100644 --- a/axum-extra/src/routing/mod.rs +++ b/axum-extra/src/routing/mod.rs @@ -1,6 +1,13 @@ //! Additional types for defining routes. -use axum::{handler::Handler, Router}; +use axum::{ + handler::Handler, + http::Request, + response::{Redirect, Response}, + Router, +}; +use std::{convert::Infallible, future::ready}; +use tower_service::Service; mod resource; @@ -126,6 +133,37 @@ pub trait RouterExt<B>: sealed::Sealed { H: Handler<T, B>, T: FirstElementIs<P> + 'static, P: TypedPath; + + /// Add another route to the router with an additional "trailing slash redirect" route. + /// + /// If you add a route _without_ a trailing slash, such as `/foo`, this method will also add a + /// route for `/foo/` that redirects to `/foo`. + /// + /// If you add a route _with_ a trailing slash, such as `/bar/`, this method will also add a + /// route for `/bar` that redirects to `/bar/`. + /// + /// This is similar to what axum 0.5.x did by default, except this explicitly adds another + /// route, so trying to add a `/foo/` route after calling `.route_with_tsr("/foo", /* ... */)` + /// will result in a panic due to route overlap. + /// + /// # Example + /// + /// ``` + /// use axum::{Router, routing::get}; + /// use axum_extra::routing::RouterExt; + /// + /// let app = Router::new() + /// // `/foo/` will redirect to `/foo` + /// .route_with_tsr("/foo", get(|| async {})) + /// // `/bar` will redirect to `/bar/` + /// .route_with_tsr("/bar/", get(|| async {})); + /// # let _: Router = app; + /// ``` + fn route_with_tsr<T>(self, path: &str, service: T) -> Self + where + T: Service<Request<B>, Response = Response, Error = Infallible> + Clone + Send + 'static, + T::Future: Send + 'static, + Self: Sized; } impl<B> RouterExt<B> for Router<B> @@ -211,9 +249,62 @@ where { self.route(P::PATH, axum::routing::trace(handler)) } + + fn route_with_tsr<T>(mut self, path: &str, service: T) -> Self + where + T: Service<Request<B>, Response = Response, Error = Infallible> + Clone + Send + 'static, + T::Future: Send + 'static, + Self: Sized, + { + self = self.route(path, service); + + let redirect = Redirect::permanent(path); + + if let Some(path_without_trailing_slash) = path.strip_suffix('/') { + self.route( + path_without_trailing_slash, + (move || ready(redirect.clone())).into_service(), + ) + } else { + self.route( + &format!("{}/", path), + (move || ready(redirect.clone())).into_service(), + ) + } + } } mod sealed { pub trait Sealed {} impl<B> Sealed for axum::Router<B> {} } + +#[cfg(test)] +mod tests { + use super::*; + use crate::test_helpers::*; + use axum::{http::StatusCode, routing::get}; + + #[tokio::test] + async fn test_tsr() { + let app = Router::new() + .route_with_tsr("/foo", get(|| async {})) + .route_with_tsr("/bar/", get(|| async {})); + + let client = TestClient::new(app); + + let res = client.get("/foo").send().await; + assert_eq!(res.status(), StatusCode::OK); + + let res = client.get("/foo/").send().await; + assert_eq!(res.status(), StatusCode::PERMANENT_REDIRECT); + assert_eq!(res.headers()["location"], "/foo"); + + let res = client.get("/bar/").send().await; + assert_eq!(res.status(), StatusCode::OK); + + let res = client.get("/bar").send().await; + assert_eq!(res.status(), StatusCode::PERMANENT_REDIRECT); + assert_eq!(res.headers()["location"], "/bar/"); + } +} diff --git a/axum/CHANGELOG.md b/axum/CHANGELOG.md index 310dbf21..82413421 100644 --- a/axum/CHANGELOG.md +++ b/axum/CHANGELOG.md @@ -11,9 +11,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:** 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 + the fallback. Consider using `axum_extra::routing::RouterExt::route_with_tsr` + if you want the old behavior ([#1119]) [#1077]: https://github.com/tokio-rs/axum/pull/1077 [#1102]: https://github.com/tokio-rs/axum/pull/1102 +[#1119]: https://github.com/tokio-rs/axum/pull/1119 [#924]: https://github.com/tokio-rs/axum/pull/924 # 0.5.10 (28. June, 2022) diff --git a/axum/src/routing/mod.rs b/axum/src/routing/mod.rs index df9f8340..4cf7abbe 100644 --- a/axum/src/routing/mod.rs +++ b/axum/src/routing/mod.rs @@ -4,12 +4,12 @@ use self::{future::RouteFuture, not_found::NotFound}; use crate::{ body::{boxed, Body, Bytes, HttpBody}, extract::connect_info::IntoMakeServiceWithConnectInfo, - response::{IntoResponse, Redirect, Response}, + response::Response, routing::strip_prefix::StripPrefix, util::try_downcast, BoxError, }; -use http::{Request, Uri}; +use http::Request; use matchit::MatchError; use std::{ borrow::Cow, @@ -499,47 +499,18 @@ where match self.node.at(&path) { Ok(match_) => self.call_route(match_, req), - Err(err) => { - let mut fallback = match &self.fallback { - Fallback::Default(inner) => inner.clone(), - Fallback::Custom(inner) => inner.clone(), - }; - - let new_uri = match err { - MatchError::MissingTrailingSlash => { - replace_path(req.uri(), &format!("{}/", &path)) - } - MatchError::ExtraTrailingSlash => { - replace_path(req.uri(), path.strip_suffix('/').unwrap()) - } - MatchError::NotFound => None, - }; - - if let Some(new_uri) = new_uri { - RouteFuture::from_response( - Redirect::permanent(&new_uri.to_string()).into_response(), - ) - } else { - fallback.call(req) - } - } + Err( + MatchError::NotFound + | MatchError::ExtraTrailingSlash + | MatchError::MissingTrailingSlash, + ) => match &self.fallback { + Fallback::Default(inner) => inner.clone().call(req), + Fallback::Custom(inner) => inner.clone().call(req), + }, } } } -fn replace_path(uri: &Uri, new_path: &str) -> Option<Uri> { - let mut new_path_and_query = new_path.to_owned(); - if let Some(query) = uri.query() { - new_path_and_query.push('?'); - new_path_and_query.push_str(query); - } - - let mut parts = uri.clone().into_parts(); - parts.path_and_query = Some(new_path_and_query.parse().unwrap()); - - Uri::from_parts(parts).ok() -} - /// Wrapper around `matchit::Router` that supports merging two `Router`s. #[derive(Clone, Default)] struct Node { @@ -645,14 +616,3 @@ fn traits() { use crate::test_helpers::*; assert_send::<Router<()>>(); } - -// https://github.com/tokio-rs/axum/issues/1122 -#[test] -fn test_replace_trailing_slash() { - let uri = "api.ipify.org:80".parse::<Uri>().unwrap(); - assert!(uri.scheme().is_none()); - assert_eq!(uri.authority(), Some(&"api.ipify.org:80".parse().unwrap())); - assert!(uri.path_and_query().is_none()); - - replace_path(&uri, "/foo"); -} diff --git a/axum/src/routing/route.rs b/axum/src/routing/route.rs index 53fbad46..6e56d77a 100644 --- a/axum/src/routing/route.rs +++ b/axum/src/routing/route.rs @@ -112,16 +112,6 @@ impl<B, E> RouteFuture<B, E> { } } - pub(super) fn from_response(response: Response) -> Self { - Self { - kind: RouteFutureKind::Response { - response: Some(response), - }, - strip_body: false, - allow_header: None, - } - } - pub(crate) fn strip_body(mut self, strip_body: bool) -> Self { self.strip_body = strip_body; self diff --git a/axum/src/routing/tests/mod.rs b/axum/src/routing/tests/mod.rs index b253bc52..5976f8a0 100644 --- a/axum/src/routing/tests/mod.rs +++ b/axum/src/routing/tests/mod.rs @@ -316,33 +316,26 @@ async fn middleware_applies_to_routes_above() { } #[tokio::test] -async fn with_trailing_slash() { +async fn not_found_for_extra_trailing_slash() { let app = Router::new().route("/foo", get(|| async {})); let client = TestClient::new(app); let res = client.get("/foo/").send().await; - assert_eq!(res.status(), StatusCode::PERMANENT_REDIRECT); - assert_eq!(res.headers().get("location").unwrap(), "/foo"); + assert_eq!(res.status(), StatusCode::NOT_FOUND); - let res = client.get("/foo/?bar=baz").send().await; - assert_eq!(res.status(), StatusCode::PERMANENT_REDIRECT); - assert_eq!(res.headers().get("location").unwrap(), "/foo?bar=baz"); + let res = client.get("/foo").send().await; + assert_eq!(res.status(), StatusCode::OK); } #[tokio::test] -async fn without_trailing_slash() { +async fn not_found_for_missing_trailing_slash() { let app = Router::new().route("/foo/", get(|| async {})); let client = TestClient::new(app); let res = client.get("/foo").send().await; - assert_eq!(res.status(), StatusCode::PERMANENT_REDIRECT); - assert_eq!(res.headers().get("location").unwrap(), "/foo/"); - - let res = client.get("/foo?bar=baz").send().await; - assert_eq!(res.status(), StatusCode::PERMANENT_REDIRECT); - assert_eq!(res.headers().get("location").unwrap(), "/foo/?bar=baz"); + assert_eq!(res.status(), StatusCode::NOT_FOUND); } #[tokio::test] @@ -362,31 +355,6 @@ async fn with_and_without_trailing_slash() { assert_eq!(res.text().await, "without tsr"); } -// for https://github.com/tokio-rs/axum/issues/681 -#[tokio::test] -async fn with_trailing_slash_post() { - let app = Router::new().route("/foo", post(|| async {})); - - let client = TestClient::new(app); - - // `TestClient` automatically follows redirects - let res = client.post("/foo/").send().await; - assert_eq!(res.status(), StatusCode::PERMANENT_REDIRECT); - assert_eq!(res.headers().get("location").unwrap(), "/foo"); -} - -// for https://github.com/tokio-rs/axum/issues/681 -#[tokio::test] -async fn without_trailing_slash_post() { - let app = Router::new().route("/foo/", post(|| async {})); - - let client = TestClient::new(app); - - let res = client.post("/foo").send().await; - assert_eq!(res.status(), StatusCode::PERMANENT_REDIRECT); - assert_eq!(res.headers().get("location").unwrap(), "/foo/"); -} - // for https://github.com/tokio-rs/axum/issues/420 #[tokio::test] async fn wildcard_with_trailing_slash() { @@ -414,8 +382,7 @@ async fn wildcard_with_trailing_slash() { ) .await .unwrap(); - assert_eq!(res.status(), StatusCode::PERMANENT_REDIRECT); - assert_eq!(res.headers()["location"], "/user1/repo1/tree/"); + assert_eq!(res.status(), StatusCode::NOT_FOUND); // check that the params are deserialized correctly let client = TestClient::new(app); diff --git a/axum/src/routing/tests/nest.rs b/axum/src/routing/tests/nest.rs index 31149c94..d4cb466c 100644 --- a/axum/src/routing/tests/nest.rs +++ b/axum/src/routing/tests/nest.rs @@ -434,20 +434,21 @@ 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/"); -// 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, -); +// TODO(david): we'll revisit this in https://github.com/tokio-rs/axum/pull/1086 +//// 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/");