mux
mux copied to clipboard
[bug] Using route.Methods(...) more than once
Describe the bug
Using func (r *Route) Methods(methods ...string)
more than once for a single route results in a response with status code 405 Method Not Allowed
for this route.
Background I tried to simplify the development of my web APIs, so I introduced some utility functions, with the relevant part essentially being:
func RegisterRoute( mux *mux.Router, path string, methods []string, handler func(http.ResponseWriter, *http.Request), ) { mux.HandleFunc(path, handler).Methods("OPTIONS").Methods(methods...) }
(OPTIONS
is basically always required; methods
as a slice because PUT
, PATCH
and even POST
may point to the same handler)
Versions go version go1.19 windows/amd64
package version: run
git rev-parse HEAD
inside the repo -- what repo? I used go to get[email protected]
Steps to Reproduce (GitHub repo)
package main import ( "log" "net/http" "github.com/gorilla/mux" ) func main() { mux := mux.NewRouter() mux.HandleFunc("/test", handler).Methods("PUT").Methods("PATCH") server := &http.Server{ Addr: ":9710", Handler: mux, } err := server.ListenAndServe() if err != nil { log.Fatal(err) } } func handler(w http.ResponseWriter, r *http.Request) { w.WriteHeader(200) }
Expected behavior
Response with status code 200 (for the latter example).
Solutions? Either:
- Disallow using
Methods(...)
more than once, e.g. via changing fieldrouteConf.matchers
from type[]matcher
to typemap[string]matcher
, where keystring
(or any other practical type) may be"METHODS"
. - Force method matchers to be unique and to be merged if it already exists, e.g. as per above.
- Change
func (r *Route) Match(req *http.Request, match *RouteMatch)
to not returnErrMethodMismatch
if any method does not match, i.e. if there are multiple method matchers.
It has been not clear to me that using Methods(...)
twice leads to this behaviour (hence this issue), so I would at least appreciate some kind of information at the function description in the source file -- although I do know now.
Also, I neither fully understand your intended design nor Git things like Pull Requests, so I am sorry if my provided solutions may very well be rather lacking.
Correct syntax to call Methods
method is to write something like this:
mux.HandleFunc("/test", handler).Methods("PUT", "GET", "OPTIONS")
I believe there are ample examples mentioned in the README.md
file. For instance under the matching routes section there is an example like
r.Methods("GET", "POST")
Regarding your background utility function, you can write something like this
func RegisterRoute(
mux *mux.Router,
path string,
methods []string,
handler func(http.ResponseWriter, *http.Request),
) {
methods = append(methods, "OPTIONS")
mux.HandleFunc(path, handler).Methods(methods...)
}
Just for understanding, I have tried to write a basic generic minimal code to understand what happens when we call Methods
multiple times.
// You can edit this code!
// Click here and start typing.
package main
import "fmt"
type matcher interface{}
func main() {
str := []matcher{"GET"}
str = append(str, matcher([]string{"PUT"}))
fmt.Println(str)
}
Output of the above code is
[GET [PUT]]
Similarly in mux while matching the HTTP method [PUT]
is not a valid method thus returns 405 not allowed.
I hope I answered your queries and doubts @edgy-sphere Thank you!
Thank you for your elaborate reply. My issue was less one of getting my code to work, because as your code snippet indicates the solution is quite trivial.
But I have to disagree with your last code snippet: this module (gorilla/mux
) provides Route.Method(methods ...string)
which will always add a type methodMatcher = []string
and never just a matcher
of type string
.
For clarity I provide my original code snippet again:
package main import ( "log" "net/http" "github.com/gorilla/mux" ) func main() { mux := mux.NewRouter() mux.HandleFunc("/test", handler).Methods("PUT").Methods("PATCH") server := &http.Server{ Addr: ":9710", Handler: mux, } err := server.ListenAndServe() if err != nil { log.Fatal(err) } } func handler(w http.ResponseWriter, r *http.Request) { w.WriteHeader(200) }
When using Route.Methods(...)
twice, or more, regardless of the potential reasons, I would reasonably expect one of the following effects:
- The second call is ignored,
- the second call replaces the methods from the first call,
- the methods from both calls are merged so that any one method must be present in the request.
This is not how your module handles it. Instead, it has the least logical and least useful effect, which is that it requires a request with the method being both PUT
and PATCH
.
In more detail: on receiving a request, Router.Match(...)
will match each route via Route.Match(...)
. This last function requires that for all of Route.routeConf.matchers
calling matcher.Match(...)
returns true
.
In my provided code snippet these matchers are:
[0]: matcher(*routeRegexp) {template: "/test", ...} [1]: matcher(methodMatcher) ["PUT"] [2]: matcher(methodMatcher) ["PATCH"]
So in either case, when the request method is PUT
or PATCH
, one matcher
will fail and matchErr = ErrMethodMismatch
will be set, and the response will be accordingly created.
This is why I created this issue, because using Route.Methods(...)
more than once entails this behaviour, which I find neither obvious nor reasonable.
This is not how your module handles it. Instead, it has the least logical and least useful effect, which is that it requires a request with the method being both PUT and PATCH.
I would posit this is not really true - each new matcher is ANDed to the previous matchers. you are asking for this one matcher to be ORed, which would be inconsistent. All other matchers work this way. If anything, this method should be documented to explicitly state calling twice on the same route is an error.
This is not how your module handles it. Instead, it has the least logical and least useful effect, which is that it requires a request with the method being both PUT and PATCH.
I would posit this is not really true - each new matcher is ANDed to the previous matchers. you are asking for this one matcher to be ORed, which would be inconsistent. All other matchers work this way. If anything, this method should be documented to explicitly state calling twice on the same route is an error.
I agree. To be clear, that is why I did not ask to change Router.Match(...)
or Route.Match(...)
so that their conditional logic is changed from AND
to OR
. I see the issue rather arising from how Route.Methods(...)
works.
So I would think it more preferable to change the type of Route.routeConf.matchers
, or at least some kind of information about the resulting behaviour.