[bug] Using route.Methods(...) more than once

[edgy-sphere]

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 mux@v1.8.0

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 field routeConf.matchers from type []matcher to type map[string]matcher, where key string (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 return ErrMethodMismatch 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.

[amustaque97]

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!

[edgy-sphere]

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.

[jrussellsmyth]

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.

[edgy-sphere]

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.

[iamdlfl]

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,

Why would you want it to silently fail? By giving an error, it tells you that something is wrong. If it just ignored or replaced one of the calls, it would be much harder to track down where the issue is.

[edgy-sphere]

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,

Why would you want it to silently fail? By giving an error, it tells you that something is wrong. If it just ignored or replaced one of the calls, it would be much harder to track down where the issue is.

I do not agree. The server app did not throw any error. When I sent a request to the server with either PUT or PATCH, I got a response with status code 405 Method Not Allowed.

If Route.Methods(...) would either replace by the last method provided or only keep the first method provided, you would get response with status code 200 OK for one method and a response with status code 405 Method Not Allowed for the other. I do not see how this would be any more silent or more difficult to debug.

I think my suggestion (only keep first or only keep last) would make the issue clearer. As previously said, during debugging I saw routeConf.matchers as roughly:

[0]: matcher(*routeRegexp) {template: "/test", ...}
[1]: matcher(methodMatcher) ["PUT"]
[2]: matcher(methodMatcher) ["PATCH"]

It is not obvious that this means that any request has to match PUT and PATCH simultanously, i.e. that the expected matchers should rather look like this:

[0]: matcher(*routeRegexp) {template: "/test", ...}
[1]: matcher(methodMatcher) ["PUT","PATCH"]

For this reason debugging this was not very straightforward for me.

Just for illustrative purposes: If instead, for example, the Route.Methods(...) function would check if a methodMatcher has been set already, i.e. if the function body would look like

func (r *Route) Methods(methods ...string) *Route {
    ...

    if !r.hasMethodMatcher() {
        return r.addMatcher(methodMatcher(methods))
    } else {
        return r
    }
}

it would be way clearer why only one of the methods provided is working.

[edgy-sphere]

...

Just for illustrative purposes: If instead, for example, the Route.Methods(...) function would check if a methodMatcher has been set already, i.e. if the function body would look like

func (r *Route) Methods(methods ...string) *Route {
    ...

    if !r.hasMethodMatcher() {
        return r.addMatcher(methodMatcher(methods))
    } else {
        return r
    }
}

it would be way clearer why only one of the methods provided is working.

Or to make it even clearer to the user, an error could be thrown, e.g.:

func (r *Route) Methods(methods ...string) *Route {
    ...

    if r.hasMethodMatcher() {
        panic("You just tried to brick this route")
    }

    return r.addMatcher(methodMatcher(methods))
}