Fix verb parsing (#1947)

* Start variable/verb fix

* Add tests for parser side

* Update test

* Adjust runtime logic to use the verb in the pattern

* Remove now-unneeded test

Author: jefferai

internal/httprule/parse.go

@@ -45,9 +45,7 @@ func tokenize(path string) (tokens []string, verb string) {
 		field
 		nested
 	)
-	var (
-		st = init
-	)
+	st := init
 	for path != "" {
 		var idx int
 		switch st {
@@ -80,8 +78,30 @@ func tokenize(path string) (tokens []string, verb string) {
 	}
 
 	l := len(tokens)
+	// See
+	// https://github.com/grpc-ecosystem/grpc-gateway/pull/1947#issuecomment-774523693 ;
+	// although normal and backwards-compat logic here is to use the last index
+	// of a colon, if the final segment is a variable followed by a colon, the
+	// part following the colon must be a verb. Hence if the previous token is
+	// an end var marker, we switch the index we're looking for to Index instead
+	// of LastIndex, so that we correctly grab the remaining part of the path as
+	// the verb.
+	var penultimateTokenIsEndVar bool
+	switch l {
+	case 0, 1:
+		// Not enough to be variable so skip this logic and don't result in an
+		// invalid index
+	default:
+		penultimateTokenIsEndVar = tokens[l-2] == "}"
+	}
 	t := tokens[l-1]
-	if idx := strings.LastIndex(t, ":"); idx == 0 {
+	var idx int
+	if penultimateTokenIsEndVar {
+		idx = strings.Index(t, ":")
+	} else {
+		idx = strings.LastIndex(t, ":")
+	}
+	if idx == 0 {
 		tokens, verb = tokens[:l-1], t[1:]
 	} else if idx > 0 {
 		tokens[l-1], verb = t[:idx], t[idx+1:]

internal/httprule/parse_test.go

@@ -13,6 +13,7 @@ func TestTokenize(t *testing.T) {
 	for _, spec := range []struct {
 		src    string
 		tokens []string
+		verb   string
 	}{
 		{
 			src:    "",
@@ -81,22 +82,51 @@ func TestTokenize(t *testing.T) {
 				eof,
 			},
 		},
+		{
+			src: "v1/a/{endpoint}:a",
+			tokens: []string{
+				"v1", "/",
+				"a", "/",
+				"{", "endpoint", "}",
+				eof,
+			},
+			verb: "a",
+		},
+		{
+			src: "v1/a/{endpoint}:b:c",
+			tokens: []string{
+				"v1", "/",
+				"a", "/",
+				"{", "endpoint", "}",
+				eof,
+			},
+			verb: "b:c",
+		},
 	} {
 		tokens, verb := tokenize(spec.src)
 		if got, want := tokens, spec.tokens; !reflect.DeepEqual(got, want) {
 			t.Errorf("tokenize(%q) = %q, _; want %q, _", spec.src, got, want)
 		}
-		if got, want := verb, ""; got != want {
-			t.Errorf("tokenize(%q) = _, %q; want _, %q", spec.src, got, want)
-		}
 
-		src := fmt.Sprintf("%s:%s", spec.src, "LOCK")
-		tokens, verb = tokenize(src)
-		if got, want := tokens, spec.tokens; !reflect.DeepEqual(got, want) {
-			t.Errorf("tokenize(%q) = %q, _; want %q, _", src, got, want)
-		}
-		if got, want := verb, "LOCK"; got != want {
-			t.Errorf("tokenize(%q) = _, %q; want _, %q", src, got, want)
+		switch {
+		case spec.verb != "":
+			if got, want := verb, spec.verb; !reflect.DeepEqual(got, want) {
+				t.Errorf("tokenize(%q) = %q, _; want %q, _", spec.src, got, want)
+			}
+
+		default:
+			if got, want := verb, ""; got != want {
+				t.Errorf("tokenize(%q) = _, %q; want _, %q", spec.src, got, want)
+			}
+
+			src := fmt.Sprintf("%s:%s", spec.src, "LOCK")
+			tokens, verb = tokenize(src)
+			if got, want := tokens, spec.tokens; !reflect.DeepEqual(got, want) {
+				t.Errorf("tokenize(%q) = %q, _; want %q, _", src, got, want)
+			}
+			if got, want := verb, "LOCK"; got != want {
+				t.Errorf("tokenize(%q) = _, %q; want _, %q", src, got, want)
+			}
 		}
 	}
 }
@@ -209,7 +239,8 @@ func TestParseSegments(t *testing.T) {
 				"a", "/", "b", "/", "*", "/", "c",
 				"}", "/",
 				"**",
-				eof},
+				eof,
+			},
 			want: []segment{
 				literal("v1"),
 				variable{

runtime/mux.go

@@ -205,18 +205,6 @@ func (s *ServeMux) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	}
 
 	components := strings.Split(path[1:], "/")
-	l := len(components)
-	var verb string
-	idx := strings.LastIndex(components[l-1], ":")
-	if idx == 0 {
-		_, outboundMarshaler := MarshalerForRequest(s, r)
-		s.routingErrorHandler(ctx, s, outboundMarshaler, w, r, http.StatusNotFound)
-		return
-	}
-	if idx > 0 {
-		c := components[l-1]
-		components[l-1], verb = c[:idx], c[idx+1:]
-	}
 
 	if override := r.Header.Get("X-HTTP-Method-Override"); override != "" && s.isPathLengthFallback(r) {
 		r.Method = strings.ToUpper(override)
@@ -227,7 +215,35 @@ func (s *ServeMux) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 			return
 		}
 	}
+
+	// Verb out here is to memoize for the fallback case below
+	var verb string
+
 	for _, h := range s.handlers[r.Method] {
+		// If the pattern has a verb, explicitly look for a suffix in the last
+		// component that matches a colon plus the verb. This allows us to
+		// handle some cases that otherwise can't be correctly handled by the
+		// former LastIndex case, such as when the verb literal itself contains
+		// a colon. This should work for all cases that have run through the
+		// parser because we know what verb we're looking for, however, there
+		// are still some cases that the parser itself cannot disambiguate. See
+		// the comment there if interested.
+		patVerb := h.pat.Verb()
+		l := len(components)
+		lastComponent := components[l-1]
+		var idx int = -1
+		if patVerb != "" && strings.HasSuffix(lastComponent, ":"+patVerb) {
+			idx = len(lastComponent) - len(patVerb) - 1
+		}
+		if idx == 0 {
+			_, outboundMarshaler := MarshalerForRequest(s, r)
+			s.routingErrorHandler(ctx, s, outboundMarshaler, w, r, http.StatusNotFound)
+			return
+		}
+		if idx > 0 {
+			components[l-1], verb = lastComponent[:idx], lastComponent[idx+1:]
+		}
+
 		pathParams, err := h.pat.Match(components, verb)
 		if err != nil {
 			continue

runtime/mux_test.go

@@ -308,6 +308,28 @@ func TestMuxServeHTTP(t *testing.T) {
 			},
 			respStatus: http.StatusBadRequest,
 		},
+		{
+			patterns: []stubPattern{
+				{
+					method: "POST",
+					ops:    []int{int(utilities.OpLitPush), 0, int(utilities.OpPush), 0, int(utilities.OpConcatN), 1, int(utilities.OpCapture), 1},
+					pool:   []string{"foo", "id"},
+				},
+				{
+					method: "POST",
+					ops:    []int{int(utilities.OpLitPush), 0, int(utilities.OpPush), 0, int(utilities.OpConcatN), 1, int(utilities.OpCapture), 1},
+					pool:   []string{"foo", "id"},
+					verb:   "verb:subverb",
+				},
+			},
+			reqMethod: "POST",
+			reqPath:   "/foo/bar:verb:subverb",
+			headers: map[string]string{
+				"Content-Type": "application/json",
+			},
+			respStatus:  http.StatusOK,
+			respContent: "POST /foo/{id=*}:verb:subverb",
+		},
 	} {
 		t.Run(strconv.Itoa(i), func(t *testing.T) {
 			var opts []runtime.ServeMuxOption
@@ -437,5 +459,4 @@ func TestServeMux_HandlePath(t *testing.T) {
 			}
 		})
 	}
-
 }