Undocumented breaking change in routing in axum 0.6

[FSMaxB]

This is the reproducer for my comment. Finally found the time.

Admittedly, this slowdown includes some other changes in how the router is built because just switching from 0.5 to 0.6 broke the merging of routes somehow, showing conflicts that weren't there before. If it helps I can also try to reduce the code to get a reproducer for that other issue, just don't know when I'll find the time to do that.

Bug Report

Version

axum v0.6.1
axum-core v0.3.0

Platform

Linux hostname 6.1.1-arch1-1 #1 SMP PREEMPT_DYNAMIC Wed, 21 Dec 2022 22:27:55 +0000 x86_64 GNU/Linux

Description

Starting a server with the following router works with axum 0.5 but fails with thread 'main' panicked at 'Invalid route "/path/:parameter": insertion failed due to conflict with previously registered route: /path/:parameter/*__private__axum_nest_tail_param' on axum 0.6.

pub fn router() -> Router {
    Router::new()
        .route("/path/:parameter", get(fake_handler))
        .nest("/path/:parameter", Router::new())
}

async fn fake_handler() -> impl IntoResponse {}

If this is an intentional breaking change, it would be helpful to document this in the Changelog

[jplatte]

I think this should actually work because .nest("/foo/bar", something) should be equivalent to .route("/foo/bar/*rest", something) in terms of routing (the only big difference being how the inner service sees the request path), and /foo/bar + /foo/bar/*nest should be non-overlapping since the latter requires the / after bar to be present. This smells like a matchit bug.

[FSMaxB]

Doesn't look like it's matchit, this works:

pub fn main() -> anyhow::Result<()> {
	let mut router = Router::new();

	router.insert("/home/:param", "a")?;
	router.insert("/home/:param/*nest", "b")?;

	Ok(())
}

(or maybe I'm testing it incorrectly)

[jplatte]

Aha, so apparently we (intentionally) register three matchit routes for .nest("/foo", ...):

1. /foo/*__private__axum_nest_tail_param
2. /foo (this one conflicts)
3. /foo/ (apparently not otherwise matched by the first route)

So .nest("/foo", foo) is not equivalent to .nest("/foo/", foo), which I previously thought.

[jplatte]

… which means:

• There seems to be a bug in matchit's error reporting
• We probably need a changelog entry along the lines of ".nest("/foo", ...) now matches /foo in addition to /foo/ and /foo/bar, this means you can get conflicts where none were before. Add a trailing slash if you want the old behavior"

[j-tai]

Add a trailing slash if you want the old behavior

This doesn't work if the conflict is at the root. Example

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

[davidpdrsn]

.nest("/foo", _) used to also match /foo because matchit 0.5 accepted /foo for /foo/*. matchit 0.6 doesn't do that but I figured we should keep the behavior for nesting, so therefore we specifically add a route for /foo. Nesting at /foo/ is indeed the intended way to opt out of this.

But yeah we should add that to the changelog.

@j-tai You probably wanna use Router::fallback instead of nest("/", _)

[j-tai]

You probably wanna use Router::fallback instead of nest("/", _)

Thanks. I actually ended up using Router::merge which seems to do what I want.