MatchErr is set to ErrNotFound if NotFoundHandler is used (#311)

Author: Roberto Santalla

mux.go

@@ -14,6 +14,7 @@ import (
 
 var (
 	ErrMethodMismatch = errors.New("method is not allowed")
+	ErrNotFound       = errors.New("no matching route was found")
 )
 
 // NewRouter returns a new router instance.
@@ -82,16 +83,23 @@ func (r *Router) Match(req *http.Request, match *RouteMatch) bool {
 		}
 	}
 
-	if match.MatchErr == ErrMethodMismatch && r.MethodNotAllowedHandler != nil {
-		match.Handler = r.MethodNotAllowedHandler
-		return true
+	if match.MatchErr == ErrMethodMismatch {
+		if r.MethodNotAllowedHandler != nil {
+			match.Handler = r.MethodNotAllowedHandler
+			return true
+		} else {
+			return false
+		}
 	}
 
 	// Closest match for a router (includes sub-routers)
 	if r.NotFoundHandler != nil {
 		match.Handler = r.NotFoundHandler
+		match.MatchErr = ErrNotFound
 		return true
 	}
+
+	match.MatchErr = ErrNotFound
 	return false
 }
 

mux_test.go

@@ -1877,6 +1877,96 @@ func TestSubrouterHeader(t *testing.T) {
 	}
 }
 
+func TestNoMatchMethodErrorHandler(t *testing.T) {
+	func1 := func(w http.ResponseWriter, r *http.Request) {}
+
+	r := NewRouter()
+	r.HandleFunc("/", func1).Methods("GET", "POST")
+
+	req, _ := http.NewRequest("PUT", "http://localhost/", 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 get ErrMethodMismatch error")
+	}
+
+	resp := NewRecorder()
+	r.ServeHTTP(resp, req)
+	if resp.Code != 405 {
+		t.Errorf("Expecting code %v", 405)
+	}
+
+	// Add matching route
+	r.HandleFunc("/", func1).Methods("PUT")
+
+	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)
+	}
+}
+
+func TestErrMatchNotFound(t *testing.T) {
+	emptyHandler := func(w http.ResponseWriter, r *http.Request) {}
+
+	r := NewRouter()
+	r.HandleFunc("/", emptyHandler)
+	s := r.PathPrefix("/sub/").Subrouter()
+	s.HandleFunc("/", emptyHandler)
+
+	// Regular 404 not found
+	req, _ := http.NewRequest("GET", "/sub/whatever", nil)
+	match := new(RouteMatch)
+	matched := r.Match(req, match)
+
+	if matched {
+		t.Errorf("Subrouter should not have matched that, got %v", match.Route)
+	}
+	// Even without a custom handler, MatchErr is set to ErrNotFound
+	if match.MatchErr != ErrNotFound {
+		t.Errorf("Expected ErrNotFound MatchErr, but was %v", match.MatchErr)
+	}
+
+	// Now lets add a 404 handler to subrouter
+	s.NotFoundHandler = http.NotFoundHandler()
+	req, _ = http.NewRequest("GET", "/sub/whatever", nil)
+
+	// Test the subrouter first
+	match = new(RouteMatch)
+	matched = s.Match(req, match)
+	// Now we should get a match
+	if !matched {
+		t.Errorf("Subrouter should have matched %s", req.RequestURI)
+	}
+	// But MatchErr should be set to ErrNotFound anyway
+	if match.MatchErr != ErrNotFound {
+		t.Errorf("Expected ErrNotFound MatchErr, but was %v", match.MatchErr)
+	}
+
+	// Now test the parent (MatchErr should propagate)
+	match = new(RouteMatch)
+	matched = r.Match(req, match)
+
+	// Now we should get a match
+	if !matched {
+		t.Errorf("Router should have matched %s via subrouter", req.RequestURI)
+	}
+	// But MatchErr should be set to ErrNotFound anyway
+	if match.MatchErr != ErrNotFound {
+		t.Errorf("Expected ErrNotFound MatchErr, but was %v", match.MatchErr)
+	}
+}
+
 // mapToPairs converts a string map to a slice of string pairs
 func mapToPairs(m map[string]string) []string {
 	var i int
@@ -1943,42 +2033,3 @@ func newRequest(method, url string) *http.Request {
 	}
 	return req
 }
-
-func TestNoMatchMethodErrorHandler(t *testing.T) {
-	func1 := func(w http.ResponseWriter, r *http.Request) {}
-
-	r := NewRouter()
-	r.HandleFunc("/", func1).Methods("GET", "POST")
-
-	req, _ := http.NewRequest("PUT", "http://localhost/", 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 get ErrMethodMismatch error")
-	}
-
-	resp := NewRecorder()
-	r.ServeHTTP(resp, req)
-	if resp.Code != 405 {
-		t.Errorf("Expecting code %v", 405)
-	}
-
-	// Add matching route
-	r.HandleFunc("/", func1).Methods("PUT")
-
-	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)
-	}
-}

route.go

@@ -72,7 +72,11 @@ func (r *Route) Match(req *http.Request, match *RouteMatch) bool {
 		return false
 	}
 
-	match.MatchErr = nil
+	if match.MatchErr == ErrMethodMismatch {
+		// We found a route which matches request method, clear MatchErr
+		match.MatchErr = nil
+	}
+
 	// Yay, we have a match. Let's collect some info about it.
 	if match.Route == nil {
 		match.Route = r