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.