Consider removing support for trailing slash redirects #1118
Comments
|
Makes sense. It might be nice to have a configuration option to turn this behavior on though. I'm not sure if it should be removed entirely. |
|
The middleware would be in tower-http, right? I think it would make sense to get that out before 0.6.0 in any case, just so there's a clear upgrade path. Overall I agree this can be surprising, and it makes sense to remove it. |
Yep makes sense to put in tower-http. |
|
Will API remain to easily 301 redirect a trailing-slashed route to its proper unslashed equivalent? |
|
@kellytk yes. We are adding tower-rs/tower-http#275 to tower-http. |
|
@davidpdrsn but that makes both return the same response, it doesn't mean one will redirect to the other. |
|
I mean that comes out to the same thing in the end. |
|
Not strictly. |
|
Yeah, there's a few differences, to name some of the disadvantages of serving the same thing on both:
|
|
That is what this whole discussion what about. Sounded like you were onboard with removing it. Like I stated in my original post I don't think we should remove trailing slash redirects and then also add a config for bringing it back. Its straight forward to add a little helper for: use axum::{
body::HttpBody,
handler::Handler,
http::Request,
response::{Redirect, Response},
routing::get,
Router,
};
use std::{convert::Infallible, net::SocketAddr};
use tower::Service;
#[tokio::main]
async fn main() {
let app = Router::new().route_with_tsr("/foo", get(|| async { "Hello, World!" }));
let addr = SocketAddr::from(([127, 0, 0, 1], 3000));
axum::Server::bind(&addr)
.serve(app.into_make_service())
.await
.unwrap();
}
trait RouterExt<B> {
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;
}
impl<B> RouterExt<B> for Router<B>
where
B: HttpBody + Send + 'static,
{
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,
{
let path = path.to_owned();
self
.route(&path, service)
.route(
&format!("{}/", path),
(move || async move { Redirect::permanent(&path) }).into_service(),
)
}
}So imo nothing is lost by requiring users to do this. |
|
I'd be fine with adding that to |
|
I was and I'm still onboard with removing the current behavior from
I think this is what we should do, with helpers for either w/ or w/o slash being canonical. It might also work in more situations, i.e. the ones that were causing crashes in axum <0.5.10? |
|
Cool! I'll add it to axum-extra. |
|
I'm curious what names you'll come up with 😄 |
Personally, the fact that removing this makes it hard to do this for external people should be a reason to maybe reconsider it. I don't really see what the harm is in supporting trailing slashes. Actix has a middleware for this purpose but making the same thing in Axum is not possible (see #1120). The proposed solution also just duplicates routing which doesn't seem ideal. |
That issue is open because we intend to fix it for 0.6. Trailing slash redirects isn't removed in a published version yet. Also like discussed above the middleware doesn't have quite the same effect. That was my assumption at first but people pointed out that wasn't the case.
Why not? How is it different from what Axum used to implicitly do? |
Right, I can sympathise and understand that.
Yup, but I came across this in a published version due to it not working in I figured I'd try writing a middleware to fix it since I didn't mind doing it but found that it isn't quite possible which led me to this issue and the one you posted earlier, so I thought I'd just give my opinion.
If you want to do this for every route you have (for my project so far it's small since I only have 30 routes) then you've essentially doubled the amount of routes + handlers, just doesn't quite seem ideal. |
Hm that seems like a bug 🤔
I don't think hidden routes in the way axum used to it is the answer. I think this is a good argument for fixing #1120. |
If you define a route like
.route("/foo", _)and someone calls/foo/axum will issue a redirect response to/foo. Same goes if you define/foo/and someone calls/foo. If both routes are defined (i.e./foo/and/foo) axum will not redirect between them.We've had this feature since 0.3.0 (since moving to
matchit) but I've started to think that it was a mistake to add. I think its a surprising feature and not something most users would expect the default behavior to be. Its almost like adding a hidden route.I think for axum 0.6 we should consider removing this feature and instead treat such requests as "not found" and send them to the fallback.
I don't think we should provide a configuration option to get the old behavior back. If users want both
/fooand/foo/they can add those routes themselves.We might also consider providing a
NormalizePathmiddleware that removes trailing slashes from incoming requests but I'd treat that as separate and a nice to have.Thoughts @jplatte @programatik29 @ibraheemdev?
The text was updated successfully, but these errors were encountered: