Fixes #407 and #1700 - google.api.http path parameter constraints and multiple HTTP path/method endpoint support

[betmix-matt]

References to other Issues or PRs

Fixes #407
Fixes #1700

Brief description of what is fixed or changed

This PR changes the behavior of the protoc-gen-openapiv2 plugin when multiple gRPC operations match the same HTTP path and method and include a path parameter. Previously, all path parameters were converted from their google.api.http syntax to OpenAPI syntax (i.e. /v1/{bar=foos/*} to /v1/{bar}) unless the parameter was named name or parent. This caused invalid OpenAPI specifications when name or parent path params existed with constraints as OpenAPI does not permit that syntax.

See https://github.com/googleapis/googleapis/blob/master/google/api/http.proto

There is a small "breaking change" in that with previous versions of the plugin, if multiple gRPC service methods had the same HTTP path and method, the last one specified was the result (i.e. last in wins). This PR changes that behavior to now log an error if duplicate mappings exist but maintains the legacy behavior of "last in wins".

With this PR, when a given gRPC service method has an identical OpenAPI HTTP endpoint to another service, the plugin will now rename the path parameter on the 2nd service adding a _1 suffix (or _2 etc until it doesn't match an existing endpoint). This will not change the actual contents of requests being sent to the endpoint as path parameter names are replaced with the values when making a request.

This PR also adds automatic "pattern" generation on path parameters that use the google.api.http path parameter constraints like /v1/{name=foos/*}/bar so that the OpenAPI specification properly matches the proto definition's requirements. It also fixes a bug where the "pattern" from proto query parameters was not being copied to the OpenAPI parameter.

Tests added for all changes.

[johanbrandhorst]

Thanks for your contribution! I will review this in a bit, but note that you'll probably need to sign the Google CLA if you haven't already (see #2307 (comment) for instructions - not sure why the bot isn't replying to the PR directly anymore). It may be that you need to sign it and the bot still doesn't reply, in that case can you please share a screenshot of your registered signing of the CLA from the CLA website? I will get to looking into this soon!

[betmix-matt]

I wanted to note I have used the updated version in my company's repository and it is now able to properly route the RFC 6570 format and enforced that the path parameter on the swagger page matched the given pattern.

[codecov-commenter]

Codecov Report

Merging #2461 (4ee8f1b) into master (c77c77c) will increase coverage by 0.49%.
The diff coverage is 87.14%.

@@            Coverage Diff             @@
##           master    #2461      +/-   ##
==========================================
+ Coverage   59.85%   60.35%   +0.49%     
==========================================
  Files          36       36              
  Lines        4653     4709      +56     
==========================================
+ Hits         2785     2842      +57     
- Misses       1571     1572       +1     
+ Partials      297      295       -2     

Impacted Files	Coverage Δ
protoc-gen-openapiv2/internal/genopenapi/types.go	65.21% <ø> (ø)
...otoc-gen-openapiv2/internal/genopenapi/template.go	64.27% <86.15%> (+1.22%)	⬆️
runtime/mux.go	64.51% <100.00%> (+0.93%)	⬆️
runtime/pattern.go	83.55% <0.00%> (+0.88%)	⬆️

---

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 c77c77c...4ee8f1b. Read the comment docs.

[johanbrandhorst]

This is a really amazing contribution, thank you so much! I would ideally like to preserve the existing behavior here and add a warning log in the case of two matching routes.

I would like to see us remove the assert test function that was added as it's very at odds with the existing conventions of the test code, even if that makes the test verbose.

I'm also a little worried about the mux change. Could you please explain what the change is for?

[johanbrandhorst]

I can see you didn't write this comment or this code, but could you explain exactly what's happening here (this comment is nonsensical 😅)? It looks to me like it's matching each part against the canRegexp above and replacing it with the first capture group? Could you add an example of what this is doing to the input, practically?

[johanbrandhorst]

Could you provide a reference here to the relevant part of the OpenAPI spec?

[johanbrandhorst]

Nit:

Suggested change

	// Based on expressions defined by RFC 6570
	// Based on expressions defined by RFC 6570.

[johanbrandhorst]

As above, could you provide a simple example of this transformation in a comment?

[johanbrandhorst]

I appreciate the intention here, but this pattern is inconsistent with the rest of the code, and I don't think we should introduce it here. If you want to do this, we'll need one function for each type you want to test:

func assertEqualInt(t *testing.T, testName string, a int, b int) {

[johanbrandhorst]

I would sooner just make this explicit than use assertion functions

Suggested change

	assertEqual(t, "Results paths length", len(result.Paths), 4)
	if got, want := len(result.Paths), 4; got != want {
	t.Fatalf("Results paths length differed, got %d want %d", got, want)
	}

[johanbrandhorst]

Does this need to be exported?

[johanbrandhorst]

What, if any, are the routing consequences of this change? Is this a change in behavior?

[betmix-matt]

We should be all set now! All requested changes have been incorporated.

[johanbrandhorst]

This is hands down one of the best contributions I've ever reviewed - very thoughtfully researched, implemented and documented. On behalf of the maintainers and the community - thank you!

[betmix-matt]

Thank you for putting your time into making this project function!

Users of grpc-gateway can now use the Google style API syntax for the gRPC endpoints and still have a RESTful JSON HTTP 1.0 API with a valid OpenAPI specification!

Could I possibly ask for a release? I want to use this immediately in our organization and I can't merge if it isn't a public version :-)

[johanbrandhorst]

I'll make a patch release this evening, as this should probably just be considered a bug fix.

[johanbrandhorst]

v2.7.2 is out!

[dgparker]

This is huge! thank you so much @betmix-matt

[johanbrandhorst]

It appears this may have introduced a regression: #2489. We should be able to add a test case mimicking the behaviour reported to help track down the issue.