Fix #271: Returning 405 instead of 404 when request has method mismatch with route #288

maknahar · 2017-08-18T10:27:48Z

This is my attempt to handle the Issue #271 . All existing tests are passing and I have added an additional test for current feature.

This is my first major contribution to a big open source project. Please feel free to suggest any change.

… mismatch

kisielk · 2017-08-18T18:01:26Z

mux.go

@@ -545,3 +555,12 @@ func matchMapWithRegex(toCheck map[string]*regexp.Regexp, toMatch map[string][]s
 	}
 	return true
 }
+
+// MethodNotAllowed replies to the request with an HTTP status code 405.
+func MethodNotAllowed(w http.ResponseWriter, r *http.Request) {


I don't think this needs to be exported

kisielk · 2017-08-18T18:01:36Z

mux.go

+
+// MethodNotAllowedHandler returns a simple request handler
+// that replies to each request with a status code 405.
+func MethodNotAllowedHandler() http.Handler { return http.HandlerFunc(MethodNotAllowed) }


This probably doesn't need to be exported either

kisielk · 2017-08-18T18:03:41Z

mux.go

+
+	// MethodMismatch flag is set to true if a route is matched but there
+	// is a mismatch in the request method and route method
+	MethodMismatch bool


I wonder if it would be better to have a MatchError error field instead. Then there could be a ErrMethodMismatch value used there in the case of methodMatcher, and then it could be extended with other error types in the future.

kisielk · 2017-08-18T18:04:15Z

Overall approach looks good to me, I left some comments about possible improvements. What do you think?

maknahar · 2017-08-19T12:12:44Z

All changes suggested by you make perfect sense @kisielk . I will make the change ASAP.

maknahar · 2017-08-19T13:31:53Z

@kisielk I have made the changes. Please let me know if anymore improvement is required.

kisielk · 2017-08-21T15:55:16Z

@elithrar can you give this a second look ?

elithrar

See suggested changes & commentary.

elithrar · 2017-08-21T18:24:21Z

mux.go

@@ -13,6 +13,10 @@ import (
 	"strings"
 )

+var (
+	ErrMethodMismatch = errors.New("Method is Not Allowed")


Since we don't use the text for this, we should either:

Lowercase this as convention - e.g. method is not allowed

Use an enum to reflect the type of error it is.

String errors are inflexible and fragile.

Will Change it to lower case.

elithrar · 2017-08-21T18:25:11Z

mux_test.go

@@ -1871,3 +1871,29 @@ func newRequest(method, url string) *http.Request {
 	}
 	return req
 }
+
+func TestNoMatchMethodErrors(t *testing.T) {
+	func1 := func(w http.ResponseWriter, r *http.Request) {}


Call it handler

elithrar · 2017-08-21T18:26:21Z

old_test.go

-			} else {
-				t.Errorf("Should not match.")
-			}
+			t.Errorf("Match Expectation: %v\nGot: %v\nRequest: %v %v", shouldMatch, matched, request.Method, url)


Follow the format used elsewhere in this file - e.g. Expected: %v Got: %v

elithrar · 2017-08-21T18:29:13Z

mux.go

+	// MatchErr is set to appropriate matching error
+	// It is set to ErrMethodMismatch if there is a mismatch in
+	// the request method and route method
+	MatchErr error


This doesn't feel right: if you provide an error here you can't are extremely limited in how you can customize 405 responses.

Should we not offer a MethodNotAllowedHandler field on the Router akin to NotFoundHandler that a package user can customize? See https://godoc.org/github.com/gorilla/mux#Router

The purpose of this is not to customize the response, but a way for the match to signal to the router the reason why it failed to match anything. The other option is to have a flag on RouteMatch specifically for the method not matching, but that feels less extensible to me.

I agree that the router should ultimately have a way to customize the handler, but that's a separate bit of logic than this.

I agree that router should have MethodNotAllowedHandler. We can default to local methodNotAllowedHandler if it is not set, same as we do with http.NotFoundHandler. Should we make those change in a current fix #271 is the question.

@elithrar @kisielk Can we look into merging this PR if no other change is required? Or do you guys want to add methodNotAllowedHandler in this as well?

@maknahar If you can add that as well, we can merge - that way it's feature-complete :)

maknahar · 2017-08-29T12:30:44Z

@elithrar I have added custom MethodNotAllowedHandler in Router. Please have a look.

elithrar

Minor documentation change, but otherwise LGTM. @kisielk for final approval/merge.

elithrar · 2017-08-29T12:45:55Z

mux.go

@@ -39,6 +43,10 @@ func NewRouter() *Router {
 type Router struct {
 	// Configurable Handler to be used when no route matches.
 	NotFoundHandler http.Handler
+
+	// Configurable Handler to be used request has method mismatch with route


Change to "Configurable Handler to be used when the request method does not match the route."

kisielk · 2017-08-30T05:00:18Z

LGTM after the requested change.

maknahar · 2017-08-30T05:38:02Z

@kisielk I have done the requested change.

kisielk · 2017-08-30T05:39:27Z

Thanks a lot!

dmitshur · 2017-08-30T16:48:05Z

mux_test.go

+		t.Errorf("Expecting code %v", 405)
+	}
+
+	//Add matching route now


Minor style nit. Unlike all others, this comment is missing a space between // and the text.

silverweed · 2017-10-02T09:57:20Z

This commit breaks the possibility to have a "catch-all" route, as it will always be called instead of any specific route with a different method.

Consider the following code:

package main

import (
	"fmt"
	"net/http"

	"github.com/gorilla/mux"
)

func main() {
	router := mux.NewRouter()
	get := router.Methods("GET").Subrouter()
	post := router.Methods("POST").Subrouter()

        // Specific POST route without parameters
	post.HandleFunc("/something", func(rw http.ResponseWriter, req *http.Request) {
		fmt.Fprintln(rw, "post")
	})
        // Catch-all route that should match any GET request, and only GET requests 
	get.HandleFunc("/{anything}", func(rw http.ResponseWriter, req *http.Request) {
		fmt.Fprintln(rw, "get")
	})

	http.Handle("/", router)
	http.ListenAndServe(":8888", nil)
}

The expected behaviour (and the actual behaviour before this commit) is:

curl localhost:8888/something -X POST  # -> post
curl localhost:8888/foobar # -> get

The actual behaviour as of this commit is that both requests are answered with GET.
It appears to only happen for routes without parameters.

This is especially problematic with routes that have the same path but different methods, as they'll all be answered with the method of the catch-all route.

The problem disappears when lines 61-64 are commented out or removed from route.go.

silverweed · 2017-10-02T10:11:36Z

Even worse:

post.HandleFunc("/some/{thing}", func(rw http.ResponseWriter, req *http.Request) {
	fmt.Fprintln(rw, "post")
})
 // Same route but different method
get.HandleFunc("/some/{thing}", func(rw http.ResponseWriter, req *http.Request) {
	fmt.Fprintln(rw, "get")
})

curl localhost:8888/some/thing -X POST # -> get

silverweed · 2017-10-02T10:28:47Z

I opened an issue for this.

maknahar added 3 commits August 18, 2017 15:46

Fix gorilla#271: Returning 405 instead of 404 when request has method…

64267ac

… mismatch

Removing debug logs

7d42c2d

Fixing a Go Vet Warning

7faf53a

kisielk reviewed Aug 18, 2017

View reviewed changes

maknahar force-pushed the master branch from 9a3abd6 to 010c361 Compare August 20, 2017 10:26

Addressed Review Comments and a Possible Bug

235709b

maknahar force-pushed the master branch from 010c361 to 235709b Compare August 20, 2017 14:54

kisielk requested a review from elithrar August 21, 2017 15:55

elithrar suggested changes Aug 21, 2017

View reviewed changes

maknahar added 2 commits August 22, 2017 14:17

Made some changes based on review comments

a86d565

Added custom MethodNotAllowedHandler in Router

9ce1e45

maknahar force-pushed the master branch from b768ac3 to 9ce1e45 Compare August 29, 2017 12:28

elithrar suggested changes Aug 29, 2017

View reviewed changes

Channged documentational comment for MethodNotAllowedHandler

c8a8cd8

kisielk merged commit a659b61 into gorilla:master Aug 30, 2017

dmitshur reviewed Aug 30, 2017

View reviewed changes

silverweed mentioned this pull request Oct 2, 2017

Subrouters match the wrong method when more than one matches #300

Closed

Fix #271: Returning 405 instead of 404 when request has method mismatch with route #288

Fix #271: Returning 405 instead of 404 when request has method mismatch with route #288

Uh oh!

Conversation

maknahar commented Aug 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kisielk commented Aug 18, 2017

Uh oh!

maknahar commented Aug 19, 2017

Uh oh!

maknahar commented Aug 19, 2017

Uh oh!

kisielk commented Aug 21, 2017

Uh oh!

elithrar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maknahar Aug 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maknahar commented Aug 29, 2017

Uh oh!

elithrar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kisielk commented Aug 30, 2017

Uh oh!

maknahar commented Aug 30, 2017

Uh oh!

kisielk commented Aug 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silverweed commented Oct 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

silverweed commented Oct 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

silverweed commented Oct 2, 2017

Uh oh!

Uh oh!

maknahar Aug 22, 2017 •

edited

Loading

silverweed commented Oct 2, 2017 •

edited

Loading

silverweed commented Oct 2, 2017 •

edited

Loading