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

fix(#739): Remove else statement on route mismatch #746

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
163 changes: 151 additions & 12 deletions mux_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
})

Expand All @@ -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) {
Expand Down Expand Up @@ -2801,32 +2825,147 @@ 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(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 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(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)
Expand Down
10 changes: 0 additions & 10 deletions route.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
}

Expand Down