axum icon indicating copy to clipboard operation
axum copied to clipboard

Consider changing the behavior of / route when nesting

Open jplatte opened this issue 1 year ago • 8 comments

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 avatar Mar 17 '24 20:03 jplatte

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

jplatte avatar Mar 17 '24 20:03 jplatte

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.

mladedav avatar Mar 17 '24 23:03 mladedav

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.

yanns avatar Mar 18 '24 13:03 yanns

This was discussed back in 2022 https://github.com/tokio-rs/axum/issues/714 https://github.com/tokio-rs/axum/issues/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 avatar Mar 23 '24 22:03 timhughes

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

timhughes avatar Mar 24 '24 02:03 timhughes

@mladedav I have (re-)reviewed your proposal above and I quite like it. I think I previously assumed that route("", ...) wasn't even supported, but if it indeed is, let's take advantage of that and have exactly one way to do nested paths.

W.r.t. what to do when both an empty route and a / route are defined, I think it's not really possible to define when a router is the "root" so my preference would be to do nothing. I'm guessing the empty route would never actually be used since hyper probably normalizes the URL. That seems alright to me, people will figure it out fast enough if they ever think to register the empty route.

Also, I've moved this to the 0.9 milestone. I think we've got enough breaking changes in 0.8 that people need to adjust to. Lmk if you disagree.

jplatte avatar Dec 01 '24 10:12 jplatte

I think I previously assumed that route("", ...) wasn't even supported

It isn't in the current version of axum, just in the proposal. Not sure which one you meant here.

W.r.t. what to do when both an empty route and a / route are defined, I think it's not really possible to define when a router is the "root" so my preference would be to do nothing. I'm guessing the empty route would never actually be used since hyper probably normalizes the URL.

That's not just hyper, but the whole of HTTP I think. There is no empty path, just /. But there are /foo and /foo/ and these can be different. So if we let people define handler for the empty path (and the router is not nested), they will never hit that handler. We can also panic on serve in that case.

mladedav avatar Jan 06 '25 17:01 mladedav

Also worth checking: What happens with nest_service?

Well, nest_service routes both paths to / of the nested router. At the same time, nest routes only the exact match to /. I hope there is a way to make both methods consistent...

To reproduce, consider the following code:

use axum::{
    Router,
    body::Body,
    extract::Request,
    http::StatusCode,
    response::{IntoResponse, Response},
    routing::get,
};
use futures::executor::block_on;
use tower_service::Service;

async fn handler() -> Response {
    StatusCode::OK.into_response()
}

fn main() {
    let inner = Router::new()
        .route("/", get(handler))
        .route("/bar", get(handler));
    let mut nest: Router<()> = Router::new().nest("/foo", inner.clone());
    let mut nest_service: Router<()> = Router::new().nest_service("/foo", inner);

    fn test(app: &mut Router, uri: &str) {
        let req = Request::builder()
            .uri(format!("https://example{}", uri))
            .body(Body::empty())
            .unwrap();
        let rsp = block_on(app.call(req)).unwrap();
        println!("  {} -> {}", uri, rsp.status());
    }

    println!("nest():");
    test(&mut nest, "/foo");
    test(&mut nest, "/foo/");
    test(&mut nest, "/foo/bar");
    test(&mut nest, "/foo/baz");
    println!("nest_service():");
    test(&mut nest_service, "/foo");
    test(&mut nest_service, "/foo/");
    test(&mut nest_service, "/foo/bar");
    test(&mut nest_service, "/foo/baz");
}

and its output:

nest():
  /foo -> 200 OK
  /foo/ -> 404 Not Found
  /foo/bar -> 200 OK
  /foo/baz -> 404 Not Found
nest_service():
  /foo -> 200 OK
  /foo/ -> 200 OK
  /foo/bar -> 200 OK
  /foo/baz -> 404 Not Found

xaberus avatar Feb 23 '25 16:02 xaberus

Any progress?

bowenxuuu avatar Sep 29 '25 01:09 bowenxuuu