Add test case and proposed fix for path component with trailing colon (and string)

[jfhamlin]

If a pattern has no verb, match with the verb argument appended to last component.

As I understand the implementation, parsing out a "verb" from the input path is a convenience for matching. Whether the colon and string are actually a verb depends on the matching HTTP rule. This change gives a verb-less pattern a chance to match a path by assuming the colon+string suffix are a part of the final component.

There is a backwards compatibility issue here: this can change the behavior of existing rule sets. For example:

    option(google.api.http) = {
      get: "/users/{user_id}"
    };
    option(google.api.http) = {
      get: "/users/{user_id}:averb"
    };

Before this change, only the second rule matched a request for /users/123:averb. After this change, both rules match (correctly, by my understanding of https://github.com/googleapis/googleapis/blob/master/google/api/http.proto). Note that the spec describes a deterministic way of breaking the tie: // **NOTE:** All service configuration rules follow "last one wins" order. grpc-ecosystem/grpc-gateway's implementation looks buggy on this score, since it accepts the first matching handler.

Fixes #224

[googlebot]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

---

What to do if you already signed the CLA

Individual signers

• It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

• Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
• The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
• The email used to register you as an authorized contributor must also be attached to your GitHub account.

[codecov-io]

Codecov Report

Merging #708 into master will increase coverage by <.01%.
The diff coverage is 62.5%.

@@            Coverage Diff             @@
##           master     #708      +/-   ##
==========================================
+ Coverage   56.43%   56.43%   +<.01%     
==========================================
  Files          30       30              
  Lines        3046     3053       +7     
==========================================
+ Hits         1719     1723       +4     
- Misses       1158     1159       +1     
- Partials      169      171       +2

Impacted Files	Coverage Δ
runtime/mux.go	44.16% <ø> (ø)	⬆️
runtime/pattern.go	89.79% <62.5%> (-1.64%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 209d7ce...6d5b579. Read the comment docs.

[jfhamlin]

fyi @yugui @tmc

After this change, both rules match (correctly, by my understanding of https://github.com/googleapis/googleapis/blob/master/google/api/http.proto).

I'll provide a little more color on why I think this is so. Here's the relevant section of http.proto:

// The syntax of the path template is as follows:
//
//     Template = "/" Segments [ Verb ] ;
//     Segments = Segment { "/" Segment } ;
//     Segment  = "*" | "**" | LITERAL | Variable ;
//     Variable = "{" FieldPath [ "=" Segments ] "}" ;
//     FieldPath = IDENT { "." IDENT } ;
//     Verb     = ":" LITERAL ;

This is the syntax of templates, not the paths templates are to match. It's actually a template that defines the syntax of the input paths it matches. If a template doesn't include a verb, then any trailing colon and string in an input path must be a part of the final segment.

[googlebot]

CLAs look good, thanks!

[jfhamlin]

bump on this (and cc @johanbrandhorst since i've seen you chime in on recent PRs 😃)

[achew22]

Is the verb in this case "123"? What does it mean to have a verb of 123?

[jfhamlin]

Is the verb in this case "123"?

My reading of the HttpRule spec makes me interpret it like this: this path matches the path template in the stub pattern, which has no verb, so "123" isn't a verb (I try to make a case for this reading in #708 (comment)). Instead, "bar:123" is the captured value of the id variable.

To flesh this out further:

• An HTTP rule path template describes a set of paths, basically by a regular expression
• If a request path is matched by an RPC's HTTP rule (regular expression), then it is handled by that RPC, with fields mapped according to the rule
• It follows that if an HTTP rule path template doesn't end with a verb, then it expects no verb, and the trailing path component will be scanned the same way as any other segment or variable

Here's another way to think about it: an alternative implementation of these patterns in grpc-gateway could have been to build up golang regexp.Regexps directly from the HTTP rules. Then when a request comes in, iterate through the regexps and use the last one that matches the path. Example mappings from rule to regular expression ([^/]* probably isn't precisely correct, it's just an approximation of "The syntax * matches a single path segment"):

get: "/foo/{id}" => "/foo/(?P<id>[^/]*)"
get: "/foo/{id}:myverb" => "/foo/(?P<id>[^/]*):myverb"
get: "/foo/{id}/bar" => "/foo/(?P<id>[^/]*)/bar"

[jfhamlin]

@achew22, does this make sense?

[achew22]

Ah, I see. You are trying to describe the positive case, and not the negative case as I had interpreted it. That seems entirely reasonable. Merging.

Thanks so much for fighting through my confusion and for contributing to the project as a whole!

[kassiansun]

Then, how to gain the old behavior of {id}:verb? Breaking the old behavior without any solution?

[kassiansun]

ping @jfhamlin @achew22

[kassiansun]

Who can tell me what's the meaning of syntax {id}:verb after this patch, if it matches the whole segment to id?

[kassiansun]

Actually it matches to the {id} route, which has no trailing :verb.

And in #224 , I don't think URN is valid inside URL, they are exclusive.

[jfhamlin]

Who can tell me what's the meaning of syntax {id}:verb after this patch, if it matches the whole segment to id?

Hi @kassiansun. It would be useful in these examples to not only have the path pattern but also some example paths and expected variable binding. For example:

pattern: /{id}:verb
path: /foo:verb match binding: id = foo
path: /foo no match

pattern: /{id}
path: /foo:verb match binding: id = foo:verb
path: /foo match binding: id = foo

Does that make sense?

[kassiansun]

No, the real behavior is /{id}:verb matches /foo:verb with id=foo:verb. Have you tested this patch?

[kassiansun]

#224 makes no sense, users should not use : in url as grpc-gateway use : for verb literals. If they really want to use, use url queries, don't mess with path variable.