Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 2 commits into from
Aug 1, 2018
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1f98a92
Add test for colon in final path segment not meant as verb
jfhamlin Jul 24, 2018
6d5b579
If a pattern has no verb, match with a verb arg appended to last comp…
jfhamlin Jul 24, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions runtime/mux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"strings"

"context"

"github.com/golang/protobuf/proto"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/metadata"
Expand Down
32 changes: 32 additions & 0 deletions runtime/mux_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,38 @@ func TestMuxServeHTTP(t *testing.T) {
respStatus: http.StatusOK,
respContent: "POST /foo:bar",
},
{
patterns: []stubPattern{
{
method: "GET",
ops: []int{int(utilities.OpLitPush), 0, int(utilities.OpPush), 0, int(utilities.OpConcatN), 1, int(utilities.OpCapture), 1},
pool: []string{"foo", "id"},
},
},
reqMethod: "GET",
reqPath: "/foo/bar",
headers: map[string]string{
"Content-Type": "application/json",
},
respStatus: http.StatusOK,
respContent: "GET /foo/{id=*}",
},
{
patterns: []stubPattern{
{
method: "GET",
ops: []int{int(utilities.OpLitPush), 0, int(utilities.OpPush), 0, int(utilities.OpConcatN), 1, int(utilities.OpCapture), 1},
pool: []string{"foo", "id"},
},
},
reqMethod: "GET",
reqPath: "/foo/bar:123",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@jfhamlin jfhamlin Jul 31, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@achew22, does this make sense?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

headers: map[string]string{
"Content-Type": "application/json",
},
respStatus: http.StatusOK,
respContent: "GET /foo/{id=*}",
},
} {
mux := runtime.NewServeMux()
for _, p := range spec.patterns {
Expand Down
11 changes: 10 additions & 1 deletion runtime/pattern.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,16 @@ func MustPattern(p Pattern, err error) Pattern {
// If otherwise, the function returns an error.
func (p Pattern) Match(components []string, verb string) (map[string]string, error) {
if p.verb != verb {
return nil, ErrNotMatch
if p.verb != "" {
return nil, ErrNotMatch
}
if len(components) == 0 {
components = []string{":" + verb}
} else {
components = append([]string{}, components...)
components[len(components)-1] += ":" + verb
}
verb = ""
}

var pos int
Expand Down