Skip to content

Commit

Permalink
[BUG] Inconsistent HTTP status code on query mismatch (#712)
Browse files Browse the repository at this point in the history
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: #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 <cdaley@redhat.com>
  • Loading branch information
soheilrt and coreydaley authored Aug 17, 2023
1 parent 24c3e7f commit 395ad81
Show file tree
Hide file tree
Showing 2 changed files with 57 additions and 0 deletions.
47 changes: 47 additions & 0 deletions mux_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {}

Expand Down
10 changes: 10 additions & 0 deletions route.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
}

Expand Down

0 comments on commit 395ad81

Please sign in to comment.