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

Merge fallbacks with the rest of the router #3158

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mladedav
Copy link
Collaborator

@mladedav mladedav commented Jan 7, 2025

Motivation

Closes #3138

Inheritance of fallbacks in nested routers does not work as documented. With this fallbacks of nested routers take precedence over outside routes with different prefix.

Since #1711 there are two PathRouters inside each Router, one for registered routes and one specifically for fallbacks. Both got merged or nested separately and were queried in turn. That means that a wildcard on the top of the first router can completely prevent ever hitting the fallback router.

Solution

The fallback router was completely deleted. Instead routes for / and /{*} are registered. This in turn works better with nesting and merging.

This changes some behavior such as making it impossible to register a top-level wildcard after a fallback.

This PR also adds several tests to show how some of these collision work. Some of those should be considered to be changed.

Copy link
Collaborator

@yanns yanns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not an expert in this code, but I like this idea. The result is simpler, with less edge-cases.

@mladedav mladedav force-pushed the dm/nesting-inheritance branch from 550562f to 13c0602 Compare January 8, 2025 20:50
@mladedav mladedav force-pushed the dm/nesting-inheritance branch from 13c0602 to 8dbe911 Compare January 8, 2025 20:59
@jplatte
Copy link
Member

jplatte commented Jan 8, 2025

Looks like you have it in a state now where no existing tests need changing? So we can probably ship this as not a breaking change? Nice!

@mladedav
Copy link
Collaborator Author

mladedav commented Jan 8, 2025

I didn't have to change any of the current tests, but notably three of the new tests behave differently:

#[test]
fn colliding_fallback_with_wildcard() {
    _ = Router::<()>::new()
        .fallback(|| async { "fallback" })
        .route("/{*wild}", get(|| async { "wildcard" }));
}

This previously did not panic and does now. Fallback would be hit only on /. If the route and fallback are switched, we accept this and it acts as did before.


async fn colliding_fallback_with_fallback() {
    let router = Router::new()
        .fallback(|| async { "fallback1" })
        .fallback(|| async { "fallback2" });

    let client = TestClient::new(router);

    let res = client.get("/").await;
    let body = res.text().await;
    assert_eq!(body, "fallback1");

    let res = client.get("/x").await;
    let body = res.text().await;
    assert_eq!(body, "fallback1");
}

We previously preferred the last fallback, now the first fallback will be used.


#[crate::test]
async fn nesting_router_with_fallback() {
    let nested = Router::new().fallback(|| async { "nested" });
    let router = Router::new().route("/{x}/{y}", get(|| async { "two segments" }));

    let client = TestClient::new(router.nest("/nest", nested));

    let res = client.get("/a/b").await;
    let body = res.text().await;
    assert_eq!(body, "two segments");

    let res = client.get("/nest/b").await;
    let body = res.text().await;
    assert_eq!(body, "nested");
}

This is the intended and basically #3138.


I guess this should be mostly safe, though I can imagine someone (accidentally) depending on any of these. So if you think this is fine we can merge it as is.

As a follow up we can decide if some of the calls should be rejected, e.g. double fallback.

@jplatte
Copy link
Member

jplatte commented Jan 8, 2025

Hmmm... I think I'd rather be conservative and change nothing in 0.8 then.

I'm definitely in favor of shipping this in 0.9 though (and rejecting double fallbacks as well).

@jplatte jplatte added the breaking change A PR that makes a breaking change. label Jan 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change A PR that makes a breaking change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fallback in nested router isn't used when other routes on the root router match
3 participants