From 4a3d4f3dd2a7b32b26dc262a143440e75767f94d Mon Sep 17 00:00:00 2001
From: Matthew <mtso@tuta.io>
Date: Tue, 28 Nov 2017 11:51:17 -0800
Subject: [PATCH] [bugfix] Fix method subrouter handler matching (#300) (#317)

* Test method-based subrouters for multiple matching paths

* Pass TestMethodsSubrouter

* Change http.Method* constants to string literals
- Make compatible with Go v1.5

* Make TestMethodsSubrouter stateless and concurrent

* Remove t.Run and break up tests for concurrency

* Use backticks to remove quote escaping

* Remove global method handlers and HTTP method constants
---
 mux_test.go | 312 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 route.go    |   2 +
 2 files changed, 314 insertions(+)

diff --git a/mux_test.go b/mux_test.go
index 6c7e30d1..c39cb86a 100644
--- a/mux_test.go
+++ b/mux_test.go
@@ -1967,6 +1967,318 @@ func TestErrMatchNotFound(t *testing.T) {
 	}
 }
 
+// methodsSubrouterTest models the data necessary for testing handler
+// matching for subrouters created after HTTP methods matcher registration.
+type methodsSubrouterTest struct {
+	title    string
+	wantCode int
+	router   *Router
+	// method is the input into the request and expected response
+	method string
+	// input request path
+	path string
+	// redirectTo is the expected location path for strict-slash matches
+	redirectTo string
+}
+
+// methodHandler writes the method string in response.
+func methodHandler(method string) http.HandlerFunc {
+	return func(w http.ResponseWriter, r *http.Request) {
+		w.Write([]byte(method))
+	}
+}
+
+// TestMethodsSubrouterCatchall matches handlers for subrouters where a
+// catchall handler is set for a mis-matching method.
+func TestMethodsSubrouterCatchall(t *testing.T) {
+	t.Parallel()
+
+	router := NewRouter()
+	router.Methods("PATCH").Subrouter().PathPrefix("/").HandlerFunc(methodHandler("PUT"))
+	router.Methods("GET").Subrouter().HandleFunc("/foo", methodHandler("GET"))
+	router.Methods("POST").Subrouter().HandleFunc("/foo", methodHandler("POST"))
+	router.Methods("DELETE").Subrouter().HandleFunc("/foo", methodHandler("DELETE"))
+
+	tests := []methodsSubrouterTest{
+		{
+			title:    "match GET handler",
+			router:   router,
+			path:     "http://localhost/foo",
+			method:   "GET",
+			wantCode: http.StatusOK,
+		},
+		{
+			title:    "match POST handler",
+			router:   router,
+			method:   "POST",
+			path:     "http://localhost/foo",
+			wantCode: http.StatusOK,
+		},
+		{
+			title:    "match DELETE handler",
+			router:   router,
+			method:   "DELETE",
+			path:     "http://localhost/foo",
+			wantCode: http.StatusOK,
+		},
+		{
+			title:    "disallow PUT method",
+			router:   router,
+			method:   "PUT",
+			path:     "http://localhost/foo",
+			wantCode: http.StatusMethodNotAllowed,
+		},
+	}
+
+	for _, test := range tests {
+		testMethodsSubrouter(t, test)
+	}
+}
+
+// TestMethodsSubrouterStrictSlash matches handlers on subrouters with
+// strict-slash matchers.
+func TestMethodsSubrouterStrictSlash(t *testing.T) {
+	t.Parallel()
+
+	router := NewRouter()
+	sub := router.PathPrefix("/").Subrouter()
+	sub.StrictSlash(true).Path("/foo").Methods("GET").Subrouter().HandleFunc("", methodHandler("GET"))
+	sub.StrictSlash(true).Path("/foo/").Methods("PUT").Subrouter().HandleFunc("/", methodHandler("PUT"))
+	sub.StrictSlash(true).Path("/foo/").Methods("POST").Subrouter().HandleFunc("/", methodHandler("POST"))
+
+	tests := []methodsSubrouterTest{
+		{
+			title:    "match POST handler",
+			router:   router,
+			method:   "POST",
+			path:     "http://localhost/foo/",
+			wantCode: http.StatusOK,
+		},
+		{
+			title:    "match GET handler",
+			router:   router,
+			method:   "GET",
+			path:     "http://localhost/foo",
+			wantCode: http.StatusOK,
+		},
+		{
+			title:      "match POST handler, redirect strict-slash",
+			router:     router,
+			method:     "POST",
+			path:       "http://localhost/foo",
+			redirectTo: "http://localhost/foo/",
+			wantCode:   http.StatusMovedPermanently,
+		},
+		{
+			title:      "match GET handler, redirect strict-slash",
+			router:     router,
+			method:     "GET",
+			path:       "http://localhost/foo/",
+			redirectTo: "http://localhost/foo",
+			wantCode:   http.StatusMovedPermanently,
+		},
+		{
+			title:    "disallow DELETE method",
+			router:   router,
+			method:   "DELETE",
+			path:     "http://localhost/foo",
+			wantCode: http.StatusMethodNotAllowed,
+		},
+	}
+
+	for _, test := range tests {
+		testMethodsSubrouter(t, test)
+	}
+}
+
+// TestMethodsSubrouterPathPrefix matches handlers on subrouters created
+// on a router with a path prefix matcher and method matcher.
+func TestMethodsSubrouterPathPrefix(t *testing.T) {
+	t.Parallel()
+
+	router := NewRouter()
+	router.PathPrefix("/1").Methods("POST").Subrouter().HandleFunc("/2", methodHandler("POST"))
+	router.PathPrefix("/1").Methods("DELETE").Subrouter().HandleFunc("/2", methodHandler("DELETE"))
+	router.PathPrefix("/1").Methods("PUT").Subrouter().HandleFunc("/2", methodHandler("PUT"))
+	router.PathPrefix("/1").Methods("POST").Subrouter().HandleFunc("/2", methodHandler("POST2"))
+
+	tests := []methodsSubrouterTest{
+		{
+			title:    "match first POST handler",
+			router:   router,
+			method:   "POST",
+			path:     "http://localhost/1/2",
+			wantCode: http.StatusOK,
+		},
+		{
+			title:    "match DELETE handler",
+			router:   router,
+			method:   "DELETE",
+			path:     "http://localhost/1/2",
+			wantCode: http.StatusOK,
+		},
+		{
+			title:    "match PUT handler",
+			router:   router,
+			method:   "PUT",
+			path:     "http://localhost/1/2",
+			wantCode: http.StatusOK,
+		},
+		{
+			title:    "disallow PATCH method",
+			router:   router,
+			method:   "PATCH",
+			path:     "http://localhost/1/2",
+			wantCode: http.StatusMethodNotAllowed,
+		},
+	}
+
+	for _, test := range tests {
+		testMethodsSubrouter(t, test)
+	}
+}
+
+// TestMethodsSubrouterSubrouter matches handlers on subrouters produced
+// from method matchers registered on a root subrouter.
+func TestMethodsSubrouterSubrouter(t *testing.T) {
+	t.Parallel()
+
+	router := NewRouter()
+	sub := router.PathPrefix("/1").Subrouter()
+	sub.Methods("POST").Subrouter().HandleFunc("/2", methodHandler("POST"))
+	sub.Methods("GET").Subrouter().HandleFunc("/2", methodHandler("GET"))
+	sub.Methods("PATCH").Subrouter().HandleFunc("/2", methodHandler("PATCH"))
+	sub.HandleFunc("/2", methodHandler("PUT")).Subrouter().Methods("PUT")
+	sub.HandleFunc("/2", methodHandler("POST2")).Subrouter().Methods("POST")
+
+	tests := []methodsSubrouterTest{
+		{
+			title:    "match first POST handler",
+			router:   router,
+			method:   "POST",
+			path:     "http://localhost/1/2",
+			wantCode: http.StatusOK,
+		},
+		{
+			title:    "match GET handler",
+			router:   router,
+			method:   "GET",
+			path:     "http://localhost/1/2",
+			wantCode: http.StatusOK,
+		},
+		{
+			title:    "match PATCH handler",
+			router:   router,
+			method:   "PATCH",
+			path:     "http://localhost/1/2",
+			wantCode: http.StatusOK,
+		},
+		{
+			title:    "match PUT handler",
+			router:   router,
+			method:   "PUT",
+			path:     "http://localhost/1/2",
+			wantCode: http.StatusOK,
+		},
+		{
+			title:    "disallow DELETE method",
+			router:   router,
+			method:   "DELETE",
+			path:     "http://localhost/1/2",
+			wantCode: http.StatusMethodNotAllowed,
+		},
+	}
+
+	for _, test := range tests {
+		testMethodsSubrouter(t, test)
+	}
+}
+
+// TestMethodsSubrouterPathVariable matches handlers on matching paths
+// with path variables in them.
+func TestMethodsSubrouterPathVariable(t *testing.T) {
+	t.Parallel()
+
+	router := NewRouter()
+	router.Methods("GET").Subrouter().HandleFunc("/foo", methodHandler("GET"))
+	router.Methods("POST").Subrouter().HandleFunc("/{any}", methodHandler("POST"))
+	router.Methods("DELETE").Subrouter().HandleFunc("/1/{any}", methodHandler("DELETE"))
+	router.Methods("PUT").Subrouter().HandleFunc("/1/{any}", methodHandler("PUT"))
+
+	tests := []methodsSubrouterTest{
+		{
+			title:    "match GET handler",
+			router:   router,
+			method:   "GET",
+			path:     "http://localhost/foo",
+			wantCode: http.StatusOK,
+		},
+		{
+			title:    "match POST handler",
+			router:   router,
+			method:   "POST",
+			path:     "http://localhost/foo",
+			wantCode: http.StatusOK,
+		},
+		{
+			title:    "match DELETE handler",
+			router:   router,
+			method:   "DELETE",
+			path:     "http://localhost/1/foo",
+			wantCode: http.StatusOK,
+		},
+		{
+			title:    "match PUT handler",
+			router:   router,
+			method:   "PUT",
+			path:     "http://localhost/1/foo",
+			wantCode: http.StatusOK,
+		},
+		{
+			title:    "disallow PATCH method",
+			router:   router,
+			method:   "PATCH",
+			path:     "http://localhost/1/foo",
+			wantCode: http.StatusMethodNotAllowed,
+		},
+	}
+
+	for _, test := range tests {
+		testMethodsSubrouter(t, test)
+	}
+}
+
+// testMethodsSubrouter runs an individual methodsSubrouterTest.
+func testMethodsSubrouter(t *testing.T, test methodsSubrouterTest) {
+	// Execute request
+	req, _ := http.NewRequest(test.method, test.path, nil)
+	resp := NewRecorder()
+	test.router.ServeHTTP(resp, req)
+
+	switch test.wantCode {
+	case http.StatusMethodNotAllowed:
+		if resp.Code != http.StatusMethodNotAllowed {
+			t.Errorf(`(%s) Expected "405 Method Not Allowed", but got %d code`, test.title, resp.Code)
+		} else if matchedMethod := resp.Body.String(); matchedMethod != "" {
+			t.Errorf(`(%s) Expected "405 Method Not Allowed", but %q handler was called`, test.title, matchedMethod)
+		}
+
+	case http.StatusMovedPermanently:
+		if gotLocation := resp.HeaderMap.Get("Location"); gotLocation != test.redirectTo {
+			t.Errorf("(%s) Expected %q route-match to redirect to %q, but got %q", test.title, test.method, test.redirectTo, gotLocation)
+		}
+
+	case http.StatusOK:
+		if matchedMethod := resp.Body.String(); matchedMethod != test.method {
+			t.Errorf("(%s) Expected %q handler to be called, but %q handler was called", test.title, test.method, matchedMethod)
+		}
+
+	default:
+		expectedCodes := []int{http.StatusMethodNotAllowed, http.StatusMovedPermanently, http.StatusOK}
+		t.Errorf("(%s) Expected wantCode to be one of: %v, but got %d", test.title, expectedCodes, test.wantCode)
+	}
+}
+
 // 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 69aeae79..2b3de8bf 100644
--- a/route.go
+++ b/route.go
@@ -75,6 +75,8 @@ func (r *Route) Match(req *http.Request, match *RouteMatch) bool {
 	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
 	}
 
 	// Yay, we have a match. Let's collect some info about it.