Additional documentation for func Methods #699
Conversation
There was a problem hiding this comment.
Hi @henrikac I left a few comments. Would you mind taking look at them? Looking forward to hearing from you.
| @@ -322,6 +322,10 @@ func (m methodMatcher) Match(r *http.Request, match *RouteMatch) bool { | |||
| // Methods adds a matcher for HTTP methods. | |||
| // It accepts a sequence of one or more methods to be matched, e.g.: | |||
| // "GET", "POST", "PUT". | |||
| // | |||
| // NOTE: Chaining multiple Methods will result in the last added | |||
| // Methods will override the allowed methods specified by the | |||
There was a problem hiding this comment.
I don't think this is not the actual behaviour, have you tried this? when I write something like the below:
r := mux.NewRouter()
r.HandleFunc("/health", healthHandle).Methods("POST").Methods("GET")When I curl /health endpoint, it returns HTTP ERROR 405
There was a problem hiding this comment.
@amustaque97 this is weird but I am getting 405 for both the POST and GET request 🤔 but
// ...
r.HandleFunc("/health", healthHandler).Methods("GET", "POST").Methods("GET")
// ...will return GET 200 🤔
There was a problem hiding this comment.
@henrikac this is expected behaviour. Consider it as series of filter. When you filter the requests based on POST and then try to do it with GET, the result will be 0 and hence 405 error
There was a problem hiding this comment.
@henrikac this is expected behaviour. Consider it as series of filter. When you filter the requests based on POST and then try to do it with GET, the result will be 0 and hence 405 error
Yeah, I figured. In my comment below for the README I mentioned that I think it looks like it behaves like it performs an intersection operation :)
There was a problem hiding this comment.
@henrikac so is this PR required? Could you modify or decline PR based on changes/feedback
| @@ -107,6 +107,8 @@ r.PathPrefix("/products/") | |||
|
|
|||
| ```go | |||
| r.Methods("GET", "POST") | |||
| // This is equivalent to .Methods("GET") | |||
There was a problem hiding this comment.
This looks incorrect to me :(
Because when we write multiple Methods calls like: Methods("GET", "POST").Methods("PATCH")
Apparently, In one of the iterations here matchErr is set ErrMethodMismatch and it will return false. I hope you understand what I'm trying to say here.
There was a problem hiding this comment.
Okay, .Methods(methods ...string) seems to act like a set and by chaining them makes them do some kind of intersection operation.
.Methods("GET", "POST").Methods("PATCH") // will return 405 for all requests
.Methods("GET", "POST").Methods("GET", "PATCH") // will return 405 for all requests but GET requests
Reading through #694 it sounds like that the behaviour of chaining multiple
func Methods(methods ...string)is not clear. This PR is not a fix to this issue but it attempts to help make this behaviour more clear to other/new users.Summary of Changes
func Methods(methods ...string)that explains the behavior of chaining multipleMethods.