Unexpected behaviour in `route_with_tsr` in v0.6.0-RC1

[gnieto]

Bug Report

Version

v0.6.0-rc.1

Crates

axum v0.6.0-rc1 axum-extra v0.4.0-rc.1

Description

Release notes for the RC 0.6.0 mentions that trailing slashes support was removed from axum and suggests using route_with_tsr from axum-extra. My understanding from the release notes is that the suggested function should have the same behaviour than using route in version 0.5.

For example, the following code in v0.5 works and redirects the trailing slashes as expected:

use axum::{response::Html, routing::get, routing::post, Router};
use std::net::SocketAddr;
use axum_extra::routing::RouterExt;

#[tokio::main]
async fn main() {
    // build our application with a route
    let app = Router::new()
        .route("/a", get(handler))
        .route("/a", post(handler2));

    // run it
    let addr = SocketAddr::from(([127, 0, 0, 1], 3001));
    println!("listening on {}", addr);
    axum::Server::bind(&addr)
        .serve(app.into_make_service())
        .await
        .unwrap();
}

async fn handler() -> Html<&'static str> {
    Html("<h1>Hello, World!</h1>")
}

async fn handler2() -> Html<&'static str> {
    Html("<h1>Hello, World 2!</h1>")
}

While trying v0.6.0-rc1, I tried replacing route by route_with_trs in order to maintain the behaviour, but the applications panics once is ran:

use axum::{response::Html, routing::get, routing::post, Router};
use std::net::SocketAddr;
use axum_extra::routing::RouterExt;

#[tokio::main]
async fn main() {
    // build our application with a route
    // Panics on v0.6.0-rc1
    let app = Router::new()
        .route_with_tsr("/a", get(handler))
        .route_with_tsr("/a", post(handler2));

    // run it
    let addr = SocketAddr::from(([127, 0, 0, 1], 3001));
    println!("listening on {}", addr);
    axum::Server::bind(&addr)
        .serve(app.into_make_service())
        .await
        .unwrap();
}

async fn handler() -> Html<&'static str> {
    Html("<h1>Hello, World!</h1>")
}

async fn handler2() -> Html<&'static str> {
    Html("<h1>Hello, World 2!</h1>")
}

When I run the application, I receive this panic:

hread 'main' panicked at 'Invalid route "/a/": insertion failed due to conflict with previously registered route: /a/', /home/gnieto/git/axum/axum/src/routing/mod.rs:215:14
stack backtrace:
   0: rust_begin_unwind
             at /rustc/a8314ef7d0ec7b75c336af2c9857bfaf43002bfc/library/std/src/panicking.rs:584:5
   1: core::panicking::panic_fmt
             at /rustc/a8314ef7d0ec7b75c336af2c9857bfaf43002bfc/library/core/src/panicking.rs:142:14
   2: axum::routing::Router<S,B>::set_node
             at /home/gnieto/git/axum/axum/src/routing/mod.rs:226:13
   3: axum::routing::Router<S,B>::route_service
             at /home/gnieto/git/axum/axum/src/routing/mod.rs:215:9
   4: <axum::routing::Router<S,B> as axum_extra::routing::RouterExt<S,B>>::route_with_tsr
             at /home/gnieto/git/axum/axum-extra/src/routing/mod.rs:277:13
   5: axum_test::main::{{closure}}
             at ./src/main.rs:11:15
   6: <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll
             at /rustc/a8314ef7d0ec7b75c336af2c9857bfaf43002bfc/library/core/src/future/mod.rs:91:19
   7: tokio::park::thread::CachedParkThread::block_on::{{closure}}
             at /home/gnieto/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.21.0/src/park/thread.rs:267:54
   8: tokio::coop::with_budget::{{closure}}
             at /home/gnieto/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.21.0/src/coop.rs:102:9
   9: std::thread::local::LocalKey<T>::try_with
             at /rustc/a8314ef7d0ec7b75c336af2c9857bfaf43002bfc/library/std/src/thread/local.rs:445:16
  10: std::thread::local::LocalKey<T>::with
             at /rustc/a8314ef7d0ec7b75c336af2c9857bfaf43002bfc/library/std/src/thread/local.rs:421:9
  11: tokio::coop::with_budget
             at /home/gnieto/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.21.0/src/coop.rs:95:5
  12: tokio::coop::budget
             at /home/gnieto/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.21.0/src/coop.rs:72:5
  13: tokio::park::thread::CachedParkThread::block_on
             at /home/gnieto/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.21.0/src/park/thread.rs:267:31
  14: tokio::runtime::enter::Enter::block_on
             at /home/gnieto/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.21.0/src/runtime/enter.rs:152:13
  15: tokio::runtime::scheduler::multi_thread::MultiThread::block_on
             at /home/gnieto/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.21.0/src/runtime/scheduler/multi_thread/mod.rs:79:9
  16: tokio::runtime::Runtime::block_on
             at /home/gnieto/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.21.0/src/runtime/mod.rs:492:44
  17: axum_test::main
             at ./src/main.rs:27:5
  18: core::ops::function::FnOnce::call_once
             at /rustc/a8314ef7d0ec7b75c336af2c9857bfaf43002bfc/library/core/src/ops/function.rs:248:5

If the route is injected in a single function call like this, the application does not panic:

    let app = Router::new()
        .route_with_tsr("/a", get(handler).post(handler2));

After looking at the code, route_with_tsr is unconditionally registering a service with the redirect request with the "trailing slash" unconditionally, which provokes the mentioned panic. The second version, only calls route_with_tsr once for this route, so the service is registered once and the panic is not triggered.

[davidpdrsn]

I've looked into this a bit and don't think its easy to solve unfortunately.

.route_with_tsr("/a", get(handler)) does two things:

1. It adds a regular router for /a that calls your handler
2. It also adds a router for /a/ that accepts any HTTP method and redirects to /a

So when you do an additional .route_with_tsr("/a", post(handler)) that will conflict on 2. because it tries to add a catch-all route for /a/ which already exists so it panics.

axum-extra cannot inspect the actual route before adding it, because its a separate crate and that data is private to the Router, and we don't wanna expose that in general.

I can see two options:

1. Do nothing and just document this limitation.
2. Add Router::try_route and Router::try_route_service which wont panic on overlapping routes but instead return a Result. Then RouterExt::route_with_tsr can just ignore any errors it might get.

I have considered adding panic-free router methods before but require some thinking around what the error type should be and how much we want to expose there. Doing that is a backwards compatible change so we should just document this limitation until we have a proper fix.

[jplatte]

How about adding a method to MethodRouter that gets you a MethodFilter of which methods the router has handlers / services for? Then we could redirect only those. That would mean no redirect if it's going to end up with a method-not-allowed error anyways, which seems like an advantage to me.

[davidpdrsn]

I still think that would require getting the MethodRouter at a given route, from Router. So before adding /a/ we could check if there already is something at that route and then inspect which methods it has handlers for.

[jplatte]

No, we can call that method on the MethodRouter given to route_with_tsr.

[ibraheemdev]

I think this can be solved by relying on matchit's tsr errors like axum used to, but only for routes that want that behavior. So the handler trait would get a method should_tsr (default returns false) that if true, means axum checks for a matchit tsr error on failure and redirects.

[epilys]

Would it be possible to pass {any, get, post, .. } as a parameter? Then route_with_tsr could wrap the alternative handler with it. Here's how I did it:

    #[inline]
    fn tsr_redirect_route(path: &'_ str) -> (Cow<'_, str>, fn(Uri) -> Response) {
        fn redirect_handler(uri: Uri) -> Response {
            let new_uri = map_path(uri, |path| {
                path.strip_suffix('/')
                    .map(Cow::Borrowed)
                    .unwrap_or_else(|| Cow::Owned(format!("{path}/")))
            });

            if let Some(new_uri) = new_uri {
                Redirect::permanent(&new_uri.to_string()).into_response()
            } else {
                StatusCode::BAD_REQUEST.into_response()
            }
        }

        if let Some(path_without_trailing_slash) = path.strip_suffix('/') {
            (Cow::Borrowed(path_without_trailing_slash), redirect_handler)
        } else {
            (Cow::Owned(format!("{path}/")), redirect_handler)
        }
    }

    #[inline]
    async fn tsr_handler_into_async(u: Uri, h: fn(Uri) -> Response) -> Response {
        h(u)
    }

and then in a Router method

        fn typed_get<H, T, P>(mut self, handler: H) -> Self
        where
            H: axum::handler::Handler<T, S, B>,
            T: SecondElementIs<P> + 'static,
            P: TypedPath,
        {
            let (tsr_path, tsr_handler) = tsr_redirect_route(P::PATH);
            self = self.route(
                tsr_path.as_ref(),
                axum::routing::get(move |url| tsr_handler_into_async(url, tsr_handler)),
            );
            self = self.route(P::PATH, axum::routing::get(handler));
            self
        }

I couldn't quickly find if I could make tsr_redirect_route generic over any, get, post, .. so I gave it up because I already knew the methods I would use, since it's for typed paths. Could this be done with generics?

[davidpdrsn]

How is that related to this bug?

But to answer your question you probably want MethodRouter::or. For example .route("/", on(MethodFilter::GET | MethodFilter::POST, handler))

[epilys]

Because route_with_tsr uses any, so doing

.route_with_tsr("/test", get(handler)) // [1]
.route_with_tsr("/test", post(handler))

Doesn't work because [1] sets an any handler for the alternative route. I got the same problem as the OP. Did I misunderstand the cause?

[davidpdrsn]

You can do .route_with_tsr("/test", get(handler).post(handler))