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

[bug] Giving methodNotAllowed even when route is defined #515

Closed
rkilingr opened this issue Aug 27, 2019 · 3 comments · May be fixed by #517
Closed

[bug] Giving methodNotAllowed even when route is defined #515

rkilingr opened this issue Aug 27, 2019 · 3 comments · May be fixed by #517
Assignees

Comments

@rkilingr
Copy link

Describe the bug

When there is a route pattern with a different Method after the same pattern ( "OPTIONS /a/b
after GET /a/b") for which there is a middle-ware response, it gives 405. I guess it's related to #509

Versions

Go version: go1.11.5 linux/amd64
package version: 9536e4053d763b54d935f1ce731a315cfb42b979

Steps to Reproduce

Define a route /a/b with method GET
Define a route /a/{a} with method OPTIONS
Define middleware which gives 200 for defined options routes
It gives StatusMethodMismatch(405) instead of 200
If the steps 1 and 2 are reversed it gives 200

Expected behavior

The expected behaviour is to give 200

Code Snippets

mux_test.go



// testMiddleWare returns 200 on an OPTIONS request
func testOptionsMiddleWare(inner http.Handler) http.Handler {
	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		if r.Method == http.MethodOptions {
			w.WriteHeader(http.StatusOK)
			return
		}
		inner.ServeHTTP(w, r)
	})
}

// Passes if /a/{a} is defined first
func TestRouterOrder(t *testing.T) {
	handler := func(w http.ResponseWriter, r *http.Request) {}
	router := NewRouter()
	router.Path("/a/b").Handler(http.HandlerFunc(handler)).Methods(http.MethodGet)
	router.Path("/a/{a}").Handler(nil).Methods(http.MethodOptions)
	router.Use(MiddlewareFunc(testOptionsMiddleWare))

	w := NewRecorder()
	req := newRequest(http.MethodOptions, "/a/b")

	router.ServeHTTP(w, req)

	if w.Code != http.StatusOK {
		t.Fatalf("Expected status code 200 (got %d)", w.Code)
	}
}

Possible Solution

route.go Line https://github.com/gorilla/mux/blob/master/route.go#L77

	diff --git a/route.go b/route.go
index 7343d78..1972c77 100644
--- a/route.go
+++ b/route.go
@@ -74,7 +74,7 @@ func (r *Route) Match(req *http.Request, match *RouteMatch) bool {
                return false
        }
 
-       if match.MatchErr == ErrMethodMismatch && r.handler != nil {
+       if match.MatchErr == ErrMethodMismatch && match.Handler == nil {
                // We found a route which matches request method, clear MatchErr
                match.MatchErr = nil
                // Then override the mis-matched handler
@rkilingr rkilingr added the bug label Aug 27, 2019
@rkilingr rkilingr changed the title [bug] Giving methodNotAllowed even when route is defines [bug] Giving methodNotAllowed even when route is defined Aug 27, 2019
@elithrar elithrar self-assigned this Aug 27, 2019
@elithrar
Copy link
Contributor

Are you willing to submit a PR for this, including tests?

We clearly need more test coverage here, since we continue to trade route matching bugs.

@rkilingr
Copy link
Author

rkilingr commented Sep 2, 2019

I have submitted a PR including tests for the particular case, you can review it and say whether it's fine.

@stale
Copy link

stale bot commented Nov 16, 2019

This issue has been automatically marked as stale because it hasn't seen a recent update. It'll be automatically closed in a few days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants