[BUG] MethodNotAllowedHandler does not work for subrouter with different routes

[BlasterAlex]

Is there an existing issue for this?

• I have searched the existing issues

Current Behavior

Go version: 1.20
Mux version: 1.8.1

Starting with mux v1.8.1 we encountered the following problem, which was not present in previous versions of the library.
If there are several routes in one subrouter, for example /api and /api/{id} MethodNotAllowedHandler does not work and NotFoundHandler is used by default.

An example test to reproduce the bug is shown below. The bug is not reproduced if:

• no subrouter is used, just main mux.Router
• subrouter contains only routes of the same type (only /api for example)
• place HandleFunc for /api/{id} before HandleFunc for /api in the code

After examining the modified code, it was found out that the problem is related to the addition of this else condition:
https://github.com/gorilla/mux/blob/v1.8.1/route.go#L70-L78
Since now the result of the Route.Match function depends on which Route was processed last (the value of match.MatchErr by reference is updated).

Expected Behavior

Result 405 Method Not Allowed

Steps To Reproduce

1. Run test:

func TestMiddlewareMethodMismatch(t *testing.T) {
	router := mux.NewRouter()
	router.MethodNotAllowedHandler = http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
		rw.WriteHeader(http.StatusMethodNotAllowed)
		_, _ = rw.Write([]byte("Method not allowed"))
	})

	subrouter := router.PathPrefix("/v1").Subrouter()
	subrouter.HandleFunc("/api", func(w http.ResponseWriter, e *http.Request) {
		_, _ = w.Write([]byte("Logic"))
	}).Methods("GET")
	subrouter.HandleFunc("/api/{id}", func(w http.ResponseWriter, e *http.Request) {
		_, _ = w.Write([]byte("Logic"))
	}).Methods("GET")

	rw := httptest.NewRecorder()
	req, _ := http.NewRequest("POST", "/v1/api", nil)
	router.ServeHTTP(rw, req)

	if rw.Code == http.StatusNotFound {
		t.Fatal("Not found instead of Method Not Allowed")
	}
}

Anything else?

No response

[akfaew]

I have the same issue, thank you for raising it.

When I GET a path that only has a POST handler, I get the different behaviour. It used to be StatusMethodNotAllowed, now it's 404.

[mateusmarquezini]

Maybe I can help. Can I take a look into this?

[coreydaley]

@mateusmarquezini I have assigned the issue to you, ping me if you need any assistance and thanks for volunteering!

[marcelom97]

I have created a PR for this.

Basically what we need to do is to remove the else statement on the route mismatch.

If we have a route with specific query params and we call it with wrong query params, this is not a route not found error but a route mismatch error.

If I understood something wrong on this please tell me!!

[BlasterAlex]

Hi, all.
I finally took the time to take a closer look at this problem.

I think the solution #746 proposed by @marcelom97 is not quite correct, as it completely cancels the earlier fix #712.

I propose to make the following changes #748. This solution allows correct handling of errors related to invalid query parameters described in #704 and preserves the possibility of receiving ErrMethodMismatch for subrouter as well.

[marcelom97]

Hey @BlasterAlex,
I think my solution is fine if we take in mind that first we want to find a match for the path including the query params, and if we find a matching path with query params then we go and check if the method matches.

[BlasterAlex]

Hi, @marcelom97. The problem is now we are not handling the case when query params are invalid. And in this case we get error 405 when we should have 404.

Why in this case you think we should have this error and not 404?

mux/mux_test.go

Line 2098 in 737cec6

if match.MatchErr != ErrMethodMismatch {

Query params don't match the expected regular expression, so we shouldn't go to the request method check.

[marcelom97]

@BlasterAlex I added some test cases to demonstrate the errors returned when the query params does not match on my PR#746

• If we have an allowed method and no match on query params we return 404.
• if we have not allowed method and no match on query params we return 404 because we first match the route (including the query params) and then we match the method.

[BlasterAlex]

I apologize, but the fact that your cases work doesn't negate the fact that the edited old test doesn't work as expected)

Why was it necessary to change the expected error from ErrNotFound to ErrMethodMismatch?
https://github.com/gorilla/mux/pull/746/files#diff-b39fc442f543ad6df386d0772fdbafafee01095292545626a39251403bec6b5cL2098-R2099

Router.Match method call here should still return ErrNotFound.