Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider changing the behavior of / route when nesting #2659

Open
jplatte opened this issue Mar 17, 2024 · 5 comments
Open

Consider changing the behavior of / route when nesting #2659

jplatte opened this issue Mar 17, 2024 · 5 comments
Labels
A-axum C-musings Category: musings about a better world E-hard Call for participation: Experience needed to fix: Hard / a lot
Milestone

Comments

@jplatte
Copy link
Member

jplatte commented Mar 17, 2024

Currently, given a router

let inner = Router::new().route("/", method_router);

and an outer router

  • Router::new().nest("/foo", inner) => only the route /foo exists
  • Router::new().nest("/foo/", inner) => only the route /foo/ exists

It seems like people (me included) generally prefer the first form, but oftentimes expect different behavior (see #2267). We should (re)consider what behavior(s) are reasonable for the upcoming release. Also worth checking: What happens with nest_service?

@jplatte jplatte added C-musings Category: musings about a better world E-hard Call for participation: Experience needed to fix: Hard / a lot A-axum labels Mar 17, 2024
@jplatte jplatte added this to the 0.8 milestone Mar 17, 2024
@jplatte
Copy link
Member Author

jplatte commented Mar 17, 2024

cc @yanns @mladedav @ibraheemdev do any of you have ideas / opinions on this?

@jplatte jplatte mentioned this issue Mar 17, 2024
1 task
@mladedav
Copy link
Contributor

I've also sometimes guessed wrong how the nesting with root handlers works. One option would be to simply concatenate, but there should be rules to disallow things like this:

Router::new().nest("/foo", Router::new().route("bar", todo)); // matches `/foobar` ??

What I imagine could work is that axum would require:

  • paths in nest
    • must start with a slash
    • must not end with a slash
  • paths in route
    • must be empty or start with a slash

For example:

Router::new().nest("", Router::new().route("/foo", todo)); // error - nest must start with a slash
Router::new().nest("/", Router::new().route("/foo", todo)); // error - nest must not ends with a slash
Router::new().nest("/foo", Router::new().route("", todo)); // matches `/foo`
Router::new().nest("/foo", Router::new().route("/", todo)); //matches `/foo/`
Router::new().nest("/foo", Router::new().route("/bar", todo)); //matches `/foo/bar`
Router::new().nest("/foo", Router::new().route("/bar/", todo)); //matches `/foo/bar/`
Router::new().nest("/foo/", Router::new().route("/bar", todo)); // error - nest ends with a slash
Router::new().nest("/foo", Router::new().route("bar", todo)); // error - route doesn't start with a slash

Currently, axum only requires all paths to start with a slash as far as I know.

This would allow setting routes with or without trailing slashes and would even prevent users from building the same router multiple ways (nest with or without a trailing slash is identical except for handling the root as far as I know).

The thing I dislike about this the most is that

let router = Router::new().route("/", root).route("", empty);

would behave differently based on whether it would be nested or not. On top level the two routes are equivalent (so we should maybe even reject starting the server due to conflicting routes) but in a nested router they are different.

This is because RFC 3386 mentions that example.com and example.com/ are the same, but later states that http://example.com/data should be considered the same as http://example.com/data/ only if the server hints that they are the same for example by redirects between them.

One way out of this would be to either let the user provide the "" handler only when they call nest but this could be elsewhere than the rest of the inner router definition.

Another would be to make the empty route even more special in a way that it returns different type, e.g. NestableRouter which can be used only for Nest and not for starting the app. I think this would be preferred if possible.

@yanns
Copy link
Contributor

yanns commented Mar 18, 2024

cc @yanns @mladedav @ibraheemdev do any of you have ideas / opinions on this?

I have never used nest myself, having the feeling (maybe wrong) that it can be dangerous with lots of subtleties.
So I would not comment on this.

@timhughes
Copy link

This was discussed back in 2022 #714 #1118 but I cannot find where the decisions were made

You can use NormalizePath

use axum::{
    extract::Request, routing::get, routing::get_service,
    Router, ServiceExt,
};
use tokio::net::TcpListener;
use tower_http::{
    normalize_path::NormalizePathLayer,
    services::{ServeDir, ServeFile},
};
use tower_layer::Layer;

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

    let serve_dir =
        get_service(ServeDir::new("static").not_found_service(ServeFile::new("static/index.html")));

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

    let app = NormalizePathLayer::trim_trailing_slash().layer(app);
    let app = ServiceExt::<Request>::into_make_service(app);

    let listener = TcpListener::bind("127.0.0.1:3000").await.unwrap();
    axum::serve(listener, app)
        .await
        .unwrap();
}

@timhughes
Copy link

there is also some strange behaviours around /path vs /path/ when you have a .fallback()

I have made a gist with the details https://gist.github.com/timhughes/745a07746f96c64586682d78829b840b

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-axum C-musings Category: musings about a better world E-hard Call for participation: Experience needed to fix: Hard / a lot
Projects
None yet
Development

No branches or pull requests

4 participants