From dde8a3ea4f669ec0f0ceea44ffec767026ea0272 Mon Sep 17 00:00:00 2001 From: Ravikiran001 Date: Mon, 2 Sep 2019 21:54:04 +0530 Subject: [PATCH 1/3] Fixes path order dependent response when a nil handler is defined for a route(#515) --- mux_test.go | 42 ++++++++++++++++++++++++++++++++++++++++++ route.go | 6 ++++-- 2 files changed, 46 insertions(+), 2 deletions(-) diff --git a/mux_test.go b/mux_test.go index edcee57..f2f2d52 100644 --- a/mux_test.go +++ b/mux_test.go @@ -2803,6 +2803,48 @@ func TestSubrouterNotFound(t *testing.T) { } } +// testOptionsMiddleWare 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) + }) +} + +// TestRouterOrder Should Pass whichever order route is defined +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) + } + + reversedPathRouter := NewRouter() + reversedPathRouter.Path("/a/{a}").Handler(http.HandlerFunc(handler)).Methods(http.MethodGet) + reversedPathRouter.Path("/a/b").Handler(nil).Methods(http.MethodOptions) + reversedPathRouter.Use(MiddlewareFunc(testOptionsMiddleWare)) + + w = NewRecorder() + + reversedPathRouter.ServeHTTP(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("Expected status code 200 (got %d)", w.Code) + } +} + // mapToPairs converts a string map to a slice of string pairs func mapToPairs(m map[string]string) []string { var i int diff --git a/route.go b/route.go index 7343d78..606e770 100644 --- a/route.go +++ b/route.go @@ -74,11 +74,13 @@ func (r *Route) Match(req *http.Request, match *RouteMatch) bool { return false } - if match.MatchErr == ErrMethodMismatch && r.handler != nil { + if match.MatchErr == ErrMethodMismatch { // We found a route which matches request method, clear MatchErr match.MatchErr = nil // Then override the mis-matched handler - match.Handler = r.handler + if r.handler != nil { + match.Handler = r.handler + } } // Yay, we have a match. Let's collect some info about it. From 53dfa85261cc3ea1166ae8426690a4803c92e1e0 Mon Sep 17 00:00:00 2001 From: RaviKiran Kilingar Date: Tue, 26 Jan 2021 19:07:08 +0530 Subject: [PATCH 2/3] Changes the tests into a subtest structure(#515) --- mux_test.go | 155 +++++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 135 insertions(+), 20 deletions(-) diff --git a/mux_test.go b/mux_test.go index f6bbdc6..8ceac20 100644 --- a/mux_test.go +++ b/mux_test.go @@ -2841,32 +2841,147 @@ func testOptionsMiddleWare(inner http.Handler) http.Handler { // TestRouterOrder Should Pass whichever order route is defined 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) + type requestCase struct { + request *http.Request + expCode int + } - if w.Code != http.StatusOK { - t.Fatalf("Expected status code 200 (got %d)", w.Code) + tests := []struct { + name string + routes []*Route + customMiddleware MiddlewareFunc + requests []requestCase + }{ + { + name: "Routes added with same method and intersecting path regex", + routes: []*Route{ + new(Route).Path("/a/b").Handler(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNotFound) + })).Methods(http.MethodGet), + new(Route).Path("/a/{a}").Handler(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + })).Methods(http.MethodGet), + }, + requests: []requestCase{ + { + request: newRequest(http.MethodGet, "/a/b"), + expCode: http.StatusNotFound, + }, + { + request: newRequest(http.MethodGet, "/a/a"), + expCode: http.StatusOK, + }, + }, + }, + { + name: "Routes added with same method and intersecting path regex, path with pathVariable first", + routes: []*Route{ + new(Route).Path("/a/{a}").Handler(http.HandlerFunc( + func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + })).Methods(http.MethodGet), + new(Route).Path("/a/b").Handler(http.HandlerFunc( + func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNotFound) + })).Methods(http.MethodGet), + }, + requests: []requestCase{ + { + request: newRequest(http.MethodGet, "/a/b"), + expCode: http.StatusOK, + }, + { + request: newRequest(http.MethodGet, "/a/a"), + expCode: http.StatusOK, + }, + }, + }, + { + name: "Routes added same path - different methods, no path variables", + routes: []*Route{ + new(Route).Path("/a/b").Handler(http.HandlerFunc( + func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + })).Methods(http.MethodGet), + new(Route).Path("/a/b").Handler(http.HandlerFunc( + func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNotFound) + })).Methods(http.MethodOptions), + }, + requests: []requestCase{ + { + request: newRequest(http.MethodGet, "/a/b"), + expCode: http.StatusOK, + }, + { + request: newRequest(http.MethodOptions, "/a/b"), + expCode: http.StatusNotFound, + }, + }, + }, + { + name: "Routes added same path - different methods, with path variables and middleware", + routes: []*Route{ + new(Route).Path("/a/{a}").Handler(http.HandlerFunc( + func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNotFound) + })).Methods(http.MethodGet), + new(Route).Path("/a/b").Handler(nil).Methods(http.MethodOptions), + }, + customMiddleware: testOptionsMiddleWare, + requests: []requestCase{ + { + request: newRequest(http.MethodGet, "/a/b"), + expCode: http.StatusNotFound, + }, + { + request: newRequest(http.MethodOptions, "/a/b"), + expCode: http.StatusOK, + }, + }, + }, + { + name: "Routes added same path - different methods, with path variables and middleware order reversed", + routes: []*Route{ + new(Route).Path("/a/b").Handler(nil).Methods(http.MethodOptions), + new(Route).Path("/a/{a}").Handler(http.HandlerFunc( + func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNotFound) + })).Methods(http.MethodGet), + }, + customMiddleware: testOptionsMiddleWare, + requests: []requestCase{ + { + request: newRequest(http.MethodGet, "/a/b"), + expCode: http.StatusNotFound, + }, + { + request: newRequest(http.MethodOptions, "/a/b"), + expCode: http.StatusOK, + }, + }, + }, } - reversedPathRouter := NewRouter() - reversedPathRouter.Path("/a/{a}").Handler(http.HandlerFunc(handler)).Methods(http.MethodGet) - reversedPathRouter.Path("/a/b").Handler(nil).Methods(http.MethodOptions) - reversedPathRouter.Use(MiddlewareFunc(testOptionsMiddleWare)) + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + router := NewRouter() - w = NewRecorder() + if test.customMiddleware != nil { + router.Use(test.customMiddleware) + } + + router.routes = test.routes + w := NewRecorder() - reversedPathRouter.ServeHTTP(w, req) + for _, requestCase := range test.requests { + router.ServeHTTP(w, requestCase.request) - if w.Code != http.StatusOK { - t.Fatalf("Expected status code 200 (got %d)", w.Code) + if w.Code != requestCase.expCode { + t.Fatalf("Expected status code %d (got %d)", requestCase.expCode, w.Code) + } + } + }) } } From 16bead4007c61f31e8d431fcedea13966b13d991 Mon Sep 17 00:00:00 2001 From: RaviKiran Kilingar Date: Tue, 26 Jan 2021 21:40:50 +0530 Subject: [PATCH 3/3] Removes line as use case is covered by #422 --- route.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/route.go b/route.go index d5afa46..330cf0a 100644 --- a/route.go +++ b/route.go @@ -74,13 +74,14 @@ func (r *Route) Match(req *http.Request, match *RouteMatch) bool { return false } + // If a route matches, but the HTTP method does not, we do one of two (2) things: + // 1. Reset the match error if we find a matching method later. + // 2. Else, we override the matched handler in the event we have a possible fallback handler for that route. + // + // This prevents propagation of ErrMethodMismatch once a suitable match is found for a Method-Path combination if match.MatchErr == ErrMethodMismatch { // We found a route which matches request method, clear MatchErr match.MatchErr = nil - // Then override the mis-matched handler - if r.handler != nil { - match.Handler = r.handler - } } // Yay, we have a match. Let's collect some info about it.