runtime.ServeMux: wrong handler chosen

[movsb]

🐛 Bug Report

Adding custom routes to the mux | gRPC-Gateway

The gRPC-Gateway allows you to add custom routes to the serve mux, for example, if you want to support a use case that isn’t supported by the gRPC-Gateway, like file uploads.

So, I have a ListFiles method in gRPC service proto which list files: /v3/files.
And I registered another GetFile method manually using HandlePath: /v3/files/{path=**}.

The final routing tree may be like the following:

func main() {
	mux := runtime.NewServeMux()
	mux.HandlePath(`GET`, `/v3/files`, func(w http.ResponseWriter, r *http.Request, pathParams map[string]string) {
		fmt.Fprintln(w, 1, pathParams)
	})
	mux.HandlePath(`GET`, `/v3/files/{path=**}`, func(w http.ResponseWriter, r *http.Request, pathParams map[string]string) {
		fmt.Fprintln(w, 2, pathParams)
	})
	http.ListenAndServe(`:8080`, mux)
}

Result:

$ curl localhost:8080/v3/files
2 map[path:]
$ curl localhost:8080/v3/files/a.txt
2 map[path:a.txt]

I'm expecting that the first request is routing the first handler, which prints 1.

Swapping the handling order fixes this problem, but I just think that the handling order shouldn't matter:

1. grpc-gateway/runtime/mux.go

   Line 354 in 031e845

   s.handlers[meth] = append([]handler{{pat: pat, h: h}}, s.handlers[meth]...)

2. grpc-gateway/runtime/mux.go

   Line 412 in 031e845

   for _, h := range s.handlers[r.Method] {

The handlers added later will be put into the front of the handlers slice, and will be used firstly.

[johanbrandhorst]

Thanks for the bug report! I'm not sure what we do for ambiguous paths like this, sounds like it's last-match-wins at the moment. Since most web servers will treat /v3/files and /v3/files/ identically, I think this behavior is correct, in that we're simply applying last-match-wins logic, which is consistent with the rest of the gateway when handling ambiguous paths. As you can see, it even identifies the path as an empty string. I think you'll need to be explicit in differentiating between these two paths.

[movsb]

@johanbrandhorst Thanks.

According the Google Reference:

Package google.api  |  Cloud Endpoints with gRPC  |  Google Cloud

The syntax * matches a single URL path segment. The syntax ** matches zero or more URL path segments, which must be the last part of the URL path except the Verb.

No matter what ** actually matches, I think that the / before it shouldn't be matched and be skipped.

Hence, /v3/files/{path=**} can match the following:

• /v3/files/
• /v3/files/a.txt
• /v3/files/dir/a.txt

But not: /v3/files.

For the func (p Pattern) MatchAndEscape method at

grpc-gateway/runtime/pattern.go

Line 154 in 031e845

func (p Pattern) MatchAndEscape(components []string, verb string, unescapingMode UnescapingMode) (map[string]string, error) {

, components should be at least the same length of Pattern.pool before being tested for a match.

[johanbrandhorst]

I buy it, but I'm worried that introducing this change will break existing users. Could you create a PR with a fix and see if it breaks any tests?