From 737cec6d3add4db21d3681d1873c88e6a18cec25 Mon Sep 17 00:00:00 2001 From: Marcelo Mollaj Date: Tue, 19 Dec 2023 11:21:05 +0200 Subject: [PATCH 1/3] fix(#739): Remove else statement on route mismatch When a path with specific query params does not match we should return a ErrMethodMismatch instead of ErrNotFound --- mux_test.go | 28 ++++++++++++++++++++++++++-- route.go | 10 ---------- 2 files changed, 26 insertions(+), 12 deletions(-) diff --git a/mux_test.go b/mux_test.go index cb3bbe0a..5390ad8b 100644 --- a/mux_test.go +++ b/mux_test.go @@ -2095,8 +2095,8 @@ func TestMultipleDefinitionOfSamePathWithDifferentMethods(t *testing.T) { if matched { t.Error("Should not have matched route for methods") } - if match.MatchErr != ErrNotFound { - t.Error("Should have ErrNotFound error. Found:", match.MatchErr) + if match.MatchErr != ErrMethodMismatch { + t.Error("Should have ErrMethodMismatch error. Found:", match.MatchErr) } }) @@ -2114,6 +2114,30 @@ func TestMultipleDefinitionOfSamePathWithDifferentMethods(t *testing.T) { } }) + t.Run("A mismach method of a valid path on subrouter should return ErrMethodMismatch", func(t *testing.T) { + r := NewRouter() + r.MethodNotAllowedHandler = http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + rw.WriteHeader(http.StatusMethodNotAllowed) + _, _ = rw.Write([]byte("Method not allowed")) + }) + + subrouter := r.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 := NewRecorder() + req, _ := http.NewRequest("POST", "/v1/api", nil) + r.ServeHTTP(rw, req) + + if rw.Code != http.StatusMethodNotAllowed { + t.Fatalf("Should have status code 405 (StatusMethodNotAllowed). Got %v\n", rw.Code) + } + }) + } func TestErrMatchNotFound(t *testing.T) { diff --git a/route.go b/route.go index 8a9e754a..0a68b1b6 100644 --- a/route.go +++ b/route.go @@ -66,16 +66,6 @@ func (r *Route) Match(req *http.Request, match *RouteMatch) bool { matchErr = nil // nolint:ineffassign return false - } else { - // Multiple routes may share the same path but use different HTTP methods. For instance: - // Route 1: POST "/users/{id}". - // Route 2: GET "/users/{id}", parameters: "id": "[0-9]+". - // - // The router must handle these cases correctly. For a GET request to "/users/abc" with "id" as "-2", - // The router should return a "Not Found" error as no route fully matches this request. - if match.MatchErr == ErrMethodMismatch { - match.MatchErr = nil - } } } From 4aaf464433a313f6533b66e0e1135f7cc3265893 Mon Sep 17 00:00:00 2001 From: Marcelo Mollaj Date: Thu, 21 Dec 2023 16:41:28 +0200 Subject: [PATCH 2/3] updated test cases for subrouter query mismatch --- mux_test.go | 42 ++++++++++++++++++++++++++++++++---------- 1 file changed, 32 insertions(+), 10 deletions(-) diff --git a/mux_test.go b/mux_test.go index 5390ad8b..93c4074c 100644 --- a/mux_test.go +++ b/mux_test.go @@ -2825,32 +2825,54 @@ func TestSubrouterCustomMethodNotAllowed(t *testing.T) { router.MethodNotAllowedHandler = customMethodNotAllowedHandler{msg: "custom router handler"} subrouter := router.PathPrefix("/sub").Subrouter() - subrouter.HandleFunc("/test", handler).Methods(http.MethodGet) + subrouter.HandleFunc("/test", handler).Queries("time", "{time:[0-9]+}").Methods(http.MethodGet) subrouter.MethodNotAllowedHandler = customMethodNotAllowedHandler{msg: "custom sub router handler"} + subrouter.NotFoundHandler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNotFound) + fmt.Fprint(w, "page not found") + }) testCases := map[string]struct { - path string - expMsg string + method string + path string + expMsg string + expStatus int }{ "router method not allowed": { - path: "/test", - expMsg: "custom router handler", + method: "POST", + path: "/test", + expMsg: "custom router handler", + expStatus: 405, }, "subrouter method not allowed": { - path: "/sub/test", - expMsg: "custom sub router handler", + method: "POST", + path: "/sub/test?time=2", + expMsg: "custom sub router handler", + expStatus: 405, + }, + "subrouter method not allowed with invalid query": { + method: "POST", + path: "/sub/test?time=-2", + expMsg: "page not found", + expStatus: 404, + }, + "subrouter method allowed with invalid query": { + method: "GET", + path: "/sub/test?time=-2", + expMsg: "page not found", + expStatus: 404, }, } for name, tc := range testCases { t.Run(name, func(tt *testing.T) { w := NewRecorder() - req := newRequest(http.MethodPut, tc.path) + req := newRequest(tc.method, tc.path) router.ServeHTTP(w, req) - if w.Code != http.StatusMethodNotAllowed { - tt.Errorf("Expected status code 405 (got %d)", w.Code) + if w.Code != tc.expStatus { + tt.Errorf("Expected status code %d (got %d)", tc.expStatus, w.Code) } b, err := io.ReadAll(w.Body) From d1f5d5663b698b7d189d0ed905c69ecaff701aeb Mon Sep 17 00:00:00 2001 From: Marcelo Mollaj Date: Thu, 21 Dec 2023 23:18:27 +0200 Subject: [PATCH 3/3] feat: add test cases for query string mismatch --- mux_test.go | 93 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 93 insertions(+) diff --git a/mux_test.go b/mux_test.go index 93c4074c..e82ae8ac 100644 --- a/mux_test.go +++ b/mux_test.go @@ -2887,6 +2887,99 @@ func TestSubrouterCustomMethodNotAllowed(t *testing.T) { } } +func TestQueryMismatchError(t *testing.T) { + handler := func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(200) + fmt.Fprint(w, "OK") + } + + router := NewRouter() + router.HandleFunc("/test", handler).Queries("time", "{time:[0-9]+}").Methods("GET") + router.MethodNotAllowedHandler = customMethodNotAllowedHandler{msg: "custom router handler"} + + subrouter := router.PathPrefix("/sub").Subrouter() + subrouter.HandleFunc("/test", handler).Queries("time", "{time:[0-9]+}").Methods("GET") + subrouter.MethodNotAllowedHandler = customMethodNotAllowedHandler{msg: "custom sub router handler"} + + testCases := map[string]struct { + method string + path string + expMsg string + expStatus int + }{ + "router method not allowed with valid query": { + method: "POST", + path: "/test?time=2", + expMsg: "custom router handler", + expStatus: 405, + }, + "router method not allowed with invalid query": { + method: "POST", + path: "/test?time=-2", + expMsg: "404 page not found\n", + expStatus: 404, + }, + "router method allowed with valid query": { + method: "GET", + path: "/test?time=2", + expMsg: "OK", + expStatus: 200, + }, + "router method allowed with invalid query": { + method: "GET", + path: "/test?time=-2", + expMsg: "404 page not found\n", + expStatus: 404, + }, + "subrouter method not allowed with valid query": { + method: "POST", + path: "/sub/test?time=2", + expMsg: "custom sub router handler", + expStatus: 405, + }, + "subrouter method not allowed with invalid query": { + method: "POST", + path: "/sub/test?time=-2", + expMsg: "404 page not found\n", + expStatus: 404, + }, + "subrouter method allowed with valid query": { + method: "GET", + path: "/sub/test?time=2", + expMsg: "OK", + expStatus: 200, + }, + "subrouter method allowed with invalid query": { + method: "GET", + path: "/sub/test?time=-2", + expMsg: "404 page not found\n", + expStatus: 404, + }, + } + + for name, tc := range testCases { + t.Run(name, func(tt *testing.T) { + w := NewRecorder() + req := newRequest(tc.method, tc.path) + + router.ServeHTTP(w, req) + + if w.Code != tc.expStatus { + tt.Errorf("Expected status code %d (got %d)", tc.expStatus, w.Code) + } + + b, err := io.ReadAll(w.Body) + if err != nil { + tt.Errorf("failed to read body: %v", err) + } + + if string(b) != tc.expMsg { + tt.Errorf("expected msg %q, got %q", tc.expMsg, string(b)) + } + }) + } +} + func TestSubrouterNotFound(t *testing.T) { handler := func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) } router := NewRouter()