Subrouters match the wrong method when more than one matches

[silverweed]

Introduced by #288.
Given the following code:

package main

import (
	"fmt"
	"net/http"

	"github.com/gorilla/mux"
)

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

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

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

a POST request to /some/thing will always match the get subrouter:

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

The same happens when there is a subrouter with a "catch-all" route, like

get.HandleFunc("/{anything}", getAnything)

This route will match any valid POST route without parameters.

These behaviours happen no matter the calling order of the HandleFuncs.

Commenting these lines seems to solve the issue, but of course breaks #288

[leofachini]

I'm facing the same problem as you @silverweed, but with another route setup. (Would you think these might be related? If not, I will open a new issue.)

With the following code:

`
func main() {
r := mux.NewRouter()
sub := r.PathPrefix("/").Subrouter()

sub.StrictSlash(true).Path("/test").Methods("GET").HandlerFunc(getFunc)
sub.StrictSlash(true).Path("/test").Methods("POST").HandlerFunc(postFunc)

log.Fatal(http.ListenAndServe(":3000", r))

}

func getFunc(w http.ResponseWriter, r *http.Request) {

fmt.Printf("%+v\n", r.Method)

json.NewEncoder(w).Encode(r.Method)

}

func postFunc(w http.ResponseWriter, r *http.Request) {

fmt.Printf("%+v\n", r.Method)

json.NewEncoder(w).Encode(r.Method)

}
`

A GET to localhost:3000/test will result in:
GET ✔️

A GET to localhost:3000/test/ will result in:
GET ✔️

A POST to localhost:3000/test will result in:
POST ✔️

A POST to localhost:3000/test/ will result in:
GET ❌ (unexpected behavior)

[stevenh]

This has hit us too, the current 1.5.0 release is totally broken for any users that have Method based routing.

This means anyone updating using ^1.x... semantic versioning will end up with broken apps.

Given this there really needs to be 1.5.1 release urgently to address this.

[elithrar]

Pinging @kisielk, because I’m away, but if it’s critical I suggest updating your dependency manifest to pin against the ref that resolves the bug (which is what makes pinning useful!).

…

On Thu, Oct 19, 2017 at 1:11 PM Steven Hartland ***@***.***> wrote: This has hit us too, the current 1.5.0 release is totally broken for any users that have Method based routing. It appears to be fixed in master, but no release has been performed yet which means anyone updating using ^1.x... semantic versioning will end up with broken apps. Given this there really need to be 1.5.1 release urgently to address this. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#300 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABIcC2JR7kIHRG4XhRWGPLxfvtYxgfvks5sty5zgaJpZM4PqdDa> .

[stevenh]

Yes thats exactly what we’ve done but not before it caused an outage for our customers.
So just trying to help protect others from such an outage.

[nadiamoe]

Maybe I can look into this if any of the affected users add some test cases. I just tweaked this code in #311, so it shouldn't be difficult for me to fix this.

[elithrar]

Plan: 1. Add more test cases to the library for this - especially around Subrouters / PathPrefix cases, to see whether we can reproduce internally w/o user code 2. If identifies issues, fix. There are already a good number of tests for no. 1 so these reports are worrying, but definitely want to fix. I'll earmark some time tomorrow to tackle no. 1.

…

On Fri, Nov 3, 2017 at 11:23 AM Roberto Santalla ***@***.***> wrote: Maybe I can look into this if any of the affected users add some test cases. I just tweaked this code in #311 <#311>, so it shouldn't be difficult for me to fix this. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#300 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABIcN-TO4CmdI-Wj4fjkitvpRhKWsI4ks5sy1ovgaJpZM4PqdDa> .

[nadiamoe]

Maybe I can take a look at the user examples to see if they can be ported to tests. I'll give it a try tomorrow.

[mtso]

Hi @roobre, are you still working on this? If not, I'd like to take a stab at it.

I found something odd here: https://github.com/gorilla/mux/blob/master/route.go#L84
Using OP's example, it's assigning a handler only if match.Handler is nil, but it’s already assigned to GET when executing the POST match. The fix may lie before that, though, because match.Handler should be nil for an invalid method match.

[nadiamoe]

I'm quite on hold since reviewers are on vacation :(

I planned to get on this when my PR (#294) gets resolved, but if you want to look at it, be my guest :3