Adding nested fallbacks is harder than it should be

[davidpdrsn]

We currently don't support nesting routers that have fallbacks. This panics:

let api_routes = Router::new()
    .route("/v1/foo", get(|| async {}))
    .fallback((|| async { "api fallback" }).into_service());

let app = Router::new()
    .nest("/api", api_routes)
    .fallback((|| async { "general fallback" }).into_service());

This is because while Router::nest takes T: Service it attempts to downcast that into a Router. If that succeeds it takes a different code path that simply moves routes from one router to another while adding the prefix. However that means we cannot have two fallbacks since we only have one Router in the end.

A possible workaround is to use another Router as a fallback:

let api_routes = Router::new()
    .route("/v1/foo", get(|| async {}));

let app = Router::new()
    .nest("/api", api_routes)
    .fallback(
        Router::new()
            .nest("/api", (|| async { "api fallback" }).into_service())
            .fallback((|| async { "general fallback" }).into_service()),
    );

While this works its not very obvious and it would be great if multiple nested fallbacks just worked.

Haven't thought through how to make it work but I believe it requires ibraheemdev/matchit#18

[lilyball]

In the short term, I believe that simply treating the nested router as an opaque service in the case where it has a fallback would work. You cannot currently add a nested router that has a fallback, so that approach can't possibly break any code. This would only need to be done if the nested router actually has a fallback, otherwise the current behavior is fine.

[davidpdrsn]

That's a good idea actually 🤔 might get us most of the way there.

[davidpdrsn]

I've thought about this some more and ran into this:

let api = Router::new()
    .fallback(api_fallback);

let app = Router::new()
    .nest("/api", api)
    .fallback(general_fallback)
    .route_layer(some_layer);

Middleware added with route_layer should not be called for fallbacks. But if nest detects that the router has a fallback and instead automatically nests it as an opaque service then the middleware will be called for api_fallback.

I think thats very surprising so unless we can find a solution for that I don't think we should do it.

---

The solution I have in mind is once ibraheemdev/matchit#18 is fixed then we change fallbacks to be regular wildcard routes. It would require tracking which routes were added with Router::fallback (since they're all just stored together in map currently) such that route_layer can skip those.

Then nesting routers will fallbacks should just work since the nested fallback would become /nested-path/*fallback.

[lilyball]

I don't know what the expected time frame for ibraheemdev/matchit#18 is. If it will be a while, another potential workaround would be to detect the nested router w/fallback, don't merge it, but do convert all of its own routes to be wrapped in StripPrefix (instead of wrapping the whole router with that). Then route_layer itself could detect that the service for the nested route is a Router and call route_layer on that nested Router instead of applying the layer around it (like how it does for Endpoint::MethodRouter).

If I correctly understand the way this works, that would have a similar effect to merging the nested router with ibraheemdev/matchit#18 fixed, and since it's pushing the StripPrefix out to the nested routes that would preserve the full path for route layers.

[davidpdrsn]

This is blocked by #1086

[davidpdrsn]

I just opened #1161 which should fix this. @lilyball Do you wanna try and test it?

[davidpdrsn]

I've decided not to support this its not worth the additional complexity it adds. See #1086 (comment).

[jplatte]

Suggested solution (extended for clarity) so people don't have to click around:

fn router() -> Router {
    Router::new().fallback(...)
}

fn make_some_routes() -> Router {
    router().route("/", ...)
}

fn make_more_routes() -> Router {
    router().route("/", ...)
}

fn my_routes() -> Router {
    router()
        .nest("/a", make_some_routes())
        .nest("/b", make_more_routes())
}

[lilyball]

If this is really the solution we're going with then please make sure any documentation recommending this explicitly recommends using OriginalUri in the fallback, as copying the fallback onto the nested router will potentially break any fallbacks that are just using the request URI.