add method_not_allowed_fallback to router

[Lachstec]

Adds the possibility to specify a default fallback if a route exists, but the method is not allowed. Fixes #2251.

Motivation

There are usecases where it is required to specify a default handler for when a route exists, but the method that was used to access it is not allowed. An example would be returning a JSON response when the method is not allowed, but have a different handler if no route exists at all. This is currently not possible. The issue #2251 suggests that this functionality is desired by users.

Solution

Add a new public function method_not_allowed_fallback(self, handler: H) -> Self to Router that calls a new private function, method_not_allowed_fallback(self, handler: H), on PathRouter. This function loops over the registered endpoints and sets the fallback on an endpoint if it is a method router.

[jplatte]

Hey, thanks for the PR, especially for including good documentation!

Before this can be merged, I think we'd need some unit and/or integration tests though.

In addition to the example in the review comment below, a useful test that I would actually expect to fail with the current implementation strategy after thinking a bit about it would be calling a router like

let app = Router::new()
    .route("/", get(index))
    .with_state(()) // would be something different in the real world, of course
    .method_not_allowed_fallback(mna_fallback);

If you find that I'm right (that this is broken) and don't want to dig into it yourself, I'm happy to write up an explanation on why this would fail.

[Lachstec]

Hey, thanks for the PR, especially for including good documentation!

Before this can be merged, I think we'd need some unit and/or integration tests though.

In addition to the example in the review comment below, a useful test that I would actually expect to fail with the current implementation strategy after thinking a bit about it would be calling a router like

let app = Router::new()
    .route("/", get(index))
    .with_state(()) // would be something different in the real world, of course
    .method_not_allowed_fallback(mna_fallback);

If you find that I'm right (that this is broken) and don't want to dig into it yourself, I'm happy to write up an explanation on why this would fail.

I thought about your mentioned test, but I think I don't understand the codebase deeply enough to get what you mean, so I would be very thankful if you could share an explanation.

[jplatte]

Right, so since you didn't indicate whether you had actually tried whether I'm right, I just looked again and think I was actually wrong. What I thought would happen was that with_state would turn Endpoint::MethodRouter into Endpoint::Route, but looking at the code that fortunately doesn't appear to be true.

Please add the test just to be sure, if it succeeds we're good, if not I'm happy to dig deeper and suggest a solution.

[Lachstec]

I did test it and it did not work at first. On further investigation, I noticed it works when the call to Router::with_state is after method_not_allowed_fallback, which confuses me a bit.

[jplatte]

Aha! Then that needs investigation. I specifically had with_state first because I expected that to cause issues.

It's pretty late here so I'll get back to this tomorrow.

[jplatte]

… and two weeks have passed 🥲

@yanns since you're very active now, curious whether you have time to look into this.

To repeat the important bits from above.. The test added in 6cd8386 (#2903) fails if with_state is called before method_not_allowed_fallback.

I anticipated this problem, but apparently for the wrong reason.

What I thought would happen was that with_state would turn Endpoint::MethodRouter into Endpoint::Route, but looking at the code that fortunately doesn't appear to be true.

Still, something is preventing the method_not_allowed_fallback from applying in the (modified) test case.

[yanns]

I'm not sure I understand what the problem is.
If I try to call with_state before, like:

    let app = Router::new()
        .route("/", get(|| async { "index" }))
        .with_state("state")
        .method_not_allowed_fallback(|State(state): State<&'static str>| async move { state });

then the code does not compile:

error[E0277]: the trait bound `routing::Router<&'static str>: tower::Service<http::Request<axum_core::body::Body>>` is not satisfied
   --> axum/src/routing/tests/fallback.rs:360:34
    |
360 |     let client = TestClient::new(app);
    |                  --------------- ^^^ the trait `tower::Service<http::Request<axum_core::body::Body>>` is not implemented for `routing::Router<&'static str>`
    |                  |
    |                  required by a bound introduced by this call
    |
    = help: the following other types implement trait `tower::Service<Request>`:
              `routing::Router` implements `tower::Service<IncomingStream<'_>>`
              `routing::Router` implements `tower::Service<http::Request<B>>`

Is it the issue and the code should compile?

[jplatte]

Ah, maybe there was simply a misunderstanding! Could you try .with_state(()) and a method-not-allowed fallback that does not involve reading state? I was purely concerned about the fallback not being called when it should be due to a prior .with_state call. The compilation error is perfectly fine.

[yanns]

This is also working:

async fn mna_fallback_with_state() {
    let app = Router::new()
        .route("/", get(|| async { "index" }))
        .with_state(())
        .method_not_allowed_fallback(|| async move { "bla" });

    let client = TestClient::new(app);
    let res = client.post("/").await;
    assert_eq!(res.text().await, "bla");
}

[jplatte]

Great. @Lachstec could you resolve the merge conflicts and add the test above (keep the one you wrote too)?

[Lachstec]

@jplatte Done! Sorry for taking a while, was fiddling around with rebasing because I was a little bit confused, but it should be fine now. If you require any other changes, let me know :)

[jplatte]

Implementation and tests are good now. I took a closer look at the docs now, can you check whether all these suggestions make sense?

[Lachstec]

@jplatte Thank you for the suggestions, I think they are a well fit, so I adapted them.

[jplatte]

Merging. Thanks for your endurance through many rounds of review!