From 395ad81d0ebffa0c5a36f3e5a2e720a7d5870d92 Mon Sep 17 00:00:00 2001 From: Soheil Rahmat Date: Thu, 17 Aug 2023 04:34:43 +0300 Subject: [PATCH] [BUG] Inconsistent HTTP status code on query mismatch (#712) The logical behavior of a router should return an HTTP status code of 404 when a request fails to satisfy route validation logic. Previously, MUX was returning a 405 HTTP status code in some rare scenarios, which was not valid in its case. For more info, See: https://github.com/gorilla/mux/issues/704 Fixes #704 **Summary of Changes** 1. Clear the mismatch error of the previous validations on method match. 2. Added related tests > PS: Make sure your PR includes/updates tests! If you need help with this part, just ask! Co-authored-by: Corey Daley --- mux_test.go | 47 +++++++++++++++++++++++++++++++++++++++++++++++ route.go | 10 ++++++++++ 2 files changed, 57 insertions(+) diff --git a/mux_test.go b/mux_test.go index 4345254b..bd97d33b 100644 --- a/mux_test.go +++ b/mux_test.go @@ -2069,6 +2069,53 @@ func TestNoMatchMethodErrorHandler(t *testing.T) { } } +func TestMultipleDefinitionOfSamePathWithDifferentMethods(t *testing.T) { + emptyHandler := func(w http.ResponseWriter, r *http.Request) {} + + r := NewRouter() + r.HandleFunc("/api", emptyHandler).Methods("POST") + r.HandleFunc("/api", emptyHandler).Queries("time", "{time:[0-9]+}").Methods("GET") + + t.Run("Post Method should be matched properly", func(t *testing.T) { + req, _ := http.NewRequest("POST", "http://localhost/api", nil) + match := new(RouteMatch) + matched := r.Match(req, match) + if !matched { + t.Error("Should have matched route for methods") + } + if match.MatchErr != nil { + t.Error("Should not have any matching error. Found:", match.MatchErr) + } + }) + + t.Run("Get Method with invalid query value should not match", func(t *testing.T) { + req, _ := http.NewRequest("GET", "http://localhost/api?time=-4", nil) + match := new(RouteMatch) + matched := r.Match(req, match) + if matched { + t.Error("Should not have matched route for methods") + } + if match.MatchErr != ErrNotFound { + t.Error("Should have ErrNotFound error. Found:", match.MatchErr) + } + }) + + t.Run("A mismach method of a valid path should return ErrMethodMismatch", func(t *testing.T) { + r := NewRouter() + r.HandleFunc("/api2", emptyHandler).Methods("POST") + req, _ := http.NewRequest("GET", "http://localhost/api2", nil) + match := new(RouteMatch) + matched := r.Match(req, match) + if matched { + t.Error("Should not have matched route for methods") + } + if match.MatchErr != ErrMethodMismatch { + t.Error("Should have ErrMethodMismatch error. Found:", match.MatchErr) + } + }) + +} + func TestErrMatchNotFound(t *testing.T) { emptyHandler := func(w http.ResponseWriter, r *http.Request) {} diff --git a/route.go b/route.go index ce1c9bfe..abea99be 100644 --- a/route.go +++ b/route.go @@ -66,6 +66,16 @@ 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 + } } }