Automatic trailing slash redirect is broken when nesting with /

[tasn]

Bug Report

Version

├── axum v0.4.4
│   ├── axum-core v0.1.1

Platform

Linux p15s 5.16.1-arch1-1 #1 SMP PREEMPT Sun, 16 Jan 2022 11:39:23 +0000 x86_64 GNU/Linux

Description

When using a nested router and in the nest just using / the slash redirect (308) works incorrectly.
Code:

pub fn app_router() -> Router {
    Router::new()
        .route("/", get(list_applications))
}

Router::new()
    .nest("/app", app_router())

Expected:

curl -v localhost:8080/app/

This command should return the correct data.

Instead: it returns a 308 redirect to localhost:8080/app (no trailing slash).

[davidpdrsn]

Seems to me like everything is working fine.

I tested:

use axum::{routing::get, Router};
use std::net::SocketAddr;

#[tokio::main]
async fn main() {
    pub fn app_router() -> Router {
        Router::new().route("/", get(|| async { "hi" }))
    }

    let app = Router::new().nest("/app", app_router());

    let addr = SocketAddr::from(([127, 0, 0, 1], 3000));
    axum::Server::bind(&addr)
        .serve(app.into_make_service())
        .await
        .unwrap();
}

• curl -v localhost:3000/app returns 200 OK
• curl -v localhost:3000/app/ returns 308 Permanent Redirect with Location: /app

Note that .nest("/app", Router::new().route("/", foo)) doesn't create a route at /app/ but at /app. This is intentional. You can't create a route for /app/ using nest and would instead have to declare a route directly for that using route.

[jplatte]

Note that .nest("/app", Router::new().route("/", foo)) doesn't create a route at /app/ but at /app. This is intentional. You can't create a route for /app/ using nest and would instead have to declare a route directly for that using route.

I think this is what's confusing. I would have expected the opposite without thinking much about it. I can definitely see the merit of this though (I think routes with a trailing slash are somewhat uncommon in REST APIs as opposed to file servers where you route them to some index file).

[davidpdrsn]

Yeah I can see that. Thinking if there is a way to make it work, or if its worth it 🤔

I think routes with a trailing slash are somewhat uncommon in REST APIs as opposed to file servers where you route them to some index file

Yep thats why I went with the current behavior.

[davidpdrsn]

We could make nest("/", _) and nest("", _) not mean the same thing (they currently do) and thus have nest("/", _) add a trailing slash. Would of course be a breaking change.

nest("", _) also looks kinda weird to me but maybe thats just something to get used to 🤔

Still don't feel like trailing slashes are very common though.

[tasn]

Our API uses a trailing slash, so we need compatibility. I think Django by default advocates for those too.
Axum supports both officially, so I expected that to work.

I gotta say though, strictly in terms of what makes sense to me (nvm my issue), "/app" + "/" = "/app/" and "/app" + "" = "/app". I think the current behavior is very confusing in that regard.

[davidpdrsn]

Well you can just not use nest to build your routes. So it's not impossible to get what you want.

Changing it a breaking change so we can't immediately fix it regardless.

I'm not familiar with Django but Rails doesn't use trailing slashes and I don't remember encountering other APIs that did.

I'm still unsure about that the right thing to do is. Would like to hear about what others think.

[tasn]

Yeah, I think I'll just manually construct the API (so no prefix) for now.

Anyhow, me wade a mistake when we chose to use a trailing slash. Alas, we can't fix it, at least not for now.

[davidpdrsn]

I still think its worth considering what the most useful and consistent behavior for axum should be. Requires thinking through all the different ways routes can be nested to see if something like #714 (comment) leads to inconsistencies elsewhere.

[tasn]

I have an idea that is maybe good maybe bad, so I'll let you be the judge. :)

I wonder whether it makes sense to have trailing slash a framework-wide configuration (either crate config or runtime), and then just default to either. Default will be none (like none).

Added benefit: you can then error (or warn?) when routes forget/accidentally add a trailing slash.

[davidpdrsn]

Interesting idea but I would like to avoid global configuration/action at a distance type stuff for axum. I'm a little worried that it's a slippery slope that could make things hard to understand for users in the future.

[tasn]

Yeah, so maybe not a global but rather an axium configuration on the Server object or something.

[davidpdrsn]

Maybe not global but router/server wide config falls into the same category imo. At least we've been able to avoid stuff like that before (with the exception of fallback which is similar).

It also makes nest and merge more complicated: If you combine two routers that have different settings, which do you pick or do you panic? Neither option is great, hence why I'd like to avoid it if possible.

[davidpdrsn]

I've done some more testing of the current behavior and these are all the combinations:

N	Nested path	Route path	Final path
1	""	""	Panic since route doesn't allow empty paths
2	""	/	/
3	""	/foo	/foo
4	""	/foo/	/foo/
5	/	""	Panic since route doesn't allow empty paths
6	/	/	/
7	/	/foo	/foo
8	/	/foo/	/foo/
9	/foo	""	Panic since route doesn't allow empty paths
10	/foo	/	/foo
11	/foo	/foo	/foo/foo
12	/foo	/foo/	/foo/foo/
13	/foo/	""	Panic since route doesn't allow empty paths
14	/foo/	/	/foo/
15	/foo/	/foo	/foo//foo
16	/foo/	/foo/	/foo//foo/

All these seem correct to me.

It seems .nest("/app/", _) does what you'd want:

let inner = Router::new().route("/", get(handler));
Router::new().nest("/app/", inner);

That gives a route at /app/, ie with a trailing slash. Maybe that solves this issue?

[tasn]

This is not really a solution though because of 15 and 16.

My code actually looks like:

pub fn app_router() -> Router {
    Router::new()
        .route("/", get(list_applications))
        .route("/:app_id/", get(get_applications))
}

Router::new()
    .nest("/app", app_router())

So the second one will break if I add the slash per your examples (double slash).

Also 14 is odd, it really is weird that it deduplicates the /.

[davidpdrsn]

It seems you can do

let inner = Router::new()
    .route("/", get(|| async {}))
    .route(":app_id/", get(|| async {}));
let app = Router::<Body>::new().nest("/app/", inner);
dbg!(&app);

but I'd honestly consider accepting .route(":app_id/", _) a bug 😅

I think a better solution would be to make this work

let inner = Router::new()
    .route("/", get(|| async {}))
    .route("/:app_id/", get(|| async {}));
let app = Router::<Body>::new().nest("/app/", inner);

Currently it results in /app/ and /app//:app_id/. But if the second one gave you /app/:app_id/ then I guess thats right 🤔

And inner would still make sense on its own. I think its important that each router still make sense on their own if not nested. This is also why 14 works the way it does. I think .route("", _) should be invalid. So if the slash wasn't deduplicated then you'd get /foo// which would then break the other use case you're describing where you have multiples routes including /.

What do you think?

[tasn]

I think that given that Axum already de-duplicates slashes (6-8), it makes sense to be consistent and fix the anyway broken 15 and 16.

[jplatte]

My 2c: I think it's sensible to require routes to start in /. More things will be rejected that way, but it still just be a single easy to understand rule (before: "route must be non-empty", after: "route must begin with /").

I find it a little bit unfortunate that it's not "just" nest("/foo", route("", handler)) for /foo¹ so it would be equivalent to string concatenation, but I can appreciate the need or desire to have the nested router be usable even if not nested.

¹ and consequently nest("/foo", route("/", handler)) would be for /foo/

[davidpdrsn]

Sounds good! I'll fix cases 15 and 16 and add a check for routes not starting with /.

[tasn]

+1 on forcing routes to start with /. Routers should definitely be valid standalone.

[davidpdrsn]

@tasn #824 is merged and should have fixed it! Are you able to give it a try?

[tasn]

I'll try to find time later today, thanks!

[tasn]

Yup, it works. Thanks!