protoc-gen-openapiv2: support overriding path parameter names

[oyvindwe]

References to other Issues or PRs

Make the automatic generated path parameters introduced in PR #2461 configurable.

Have you read the Contributing Guidelines?

Yes.

Brief description of what is fixed or changed

Added a new proto option to fields: path_param_name. Alternative parameter name when used as path parameter. If set, this will be used as the complete parameter name when this field is used as a path parameter. Use this to avoid having auto generated path parameter names for overlapping paths.

This is useful for large APIs that follows the Google API guidelines and have a lot of services/HTTP bindings with overlapping paths, e.g. the following hypothetical paths using in HTTP bindings for 2 different services in the same API:

• /v1/{name=shelves/*/books/*}
• /v1/{parent=shelves/*}
• /v1/{name=floors/*/rooms/*}
• /v1/{parent=floors/*}

This will currently generate the following paths:

• /v1/{name}
• /v1/{parent}
• /v1/{name_1}
• /v1/{parent_1}

Additionally, the operations are mixed and matched between the paths. /v1/{name} could be the path for PUT /v1/{name=shelves/*/books/*} and PATCH for /v1/{name=floors/*/rooms/*}, which may be more or less confusing, depending on the tool used to render the OpenAPI specification file.

With this change, it's possible to override the path parameter name in the fields that are used in that path. Consider the following messages used in the operations with the above bindings and paths:

message Book {
  // The resource name for a book, e.g. `shelves/1/book/2`
  string name = 1 [(grpc.gateway.protoc_gen_openapiv2.options.openapiv2_field) = {path_param_name: "bookName"}];
}
message ListRoomsRequest {
  // The resource name for a shelf, e.g. `shelves/1`
  string parent = 1 [(grpc.gateway.protoc_gen_openapiv2.options.openapiv2_field) = {path_param_name: "shelfName"}];
}
message Room {
  // The resource name for a room, e.g. `floors/first/room/closet`
  string name = 1 [(grpc.gateway.protoc_gen_openapiv2.options.openapiv2_field) = {path_param_name: "roomName"}];
}
message ListRoomsRequest {
  // The resource name for a floor, e.g. `floors/first`
  string parent = 1 [(grpc.gateway.protoc_gen_openapiv2.options.openapiv2_field) = {path_param_name: "floorName"}];
}

This will generate the following paths:

• /v1/{bookName}
• /v1/{shelfName}
• /v1/{roomName}
• /v1/{floorName}

Also, operations for the same resource will not be mixed across the paths, as they now are unique.

If a duplicate path is encountered when applying path_param_name, the existing numbering logic is still used.

Other comments

Note - the path parameter is expected to be in camelCase if JSON format is used.

[codecov-commenter]

Codecov Report

Merging #2562 (b7732f9) into master (1d70eab) will decrease coverage by 0.04%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master    #2562      +/-   ##
==========================================
- Coverage   60.99%   60.95%   -0.05%     
==========================================
  Files          38       38              
  Lines        4933     4945      +12     
==========================================
+ Hits         3009     3014       +5     
- Misses       1594     1599       +5     
- Partials      330      332       +2     

Impacted Files	Coverage Δ
protoc-gen-openapiv2/internal/genopenapi/types.go	74.35% <ø> (ø)
...otoc-gen-openapiv2/internal/genopenapi/template.go	63.04% <50.00%> (-0.16%)	⬇️

---

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 1d70eab...b7732f9. Read the comment docs.

[oyvindwe]

Note - get and put switched order in the generated swagger.json, as I didn't add the path_param_name override to grpc.gateway.examples.internal.proto.sub2.IdMessage (the request message in used by the get operation), and then introduced a new path only for some of the operations. This makes it a bit more difficult to compare this file.

[johanbrandhorst]

I appreciate the initiative in opening this PR, but I think, so far, we've only added fields here for valid fields of the equivalent OpenAPIv2 spec object, which in this case is the Schema object: https://github.com/OAI/OpenAPI-Specification/blob/3.0.0/versions/2.0.md#schemaObject. Adding this field breaks this pattern. Is there some alternative way we can inject this information?

[oyvindwe]

That's a fair point, and the same thought struck me as well when implementing this. How about introducing separate options for configuring the behaviour of the generator to keep it separated from the OpenAPI objects? It looks like that requires a separate assigned message id in annotations.proto though

[yinzara]

So now that I've eaten my own dog food regarding the path parameters, the automatic name generation kinda sucks (just as outlined above) especially when you start to have a lot of API endpoints that all have a similar structure. I've also discovered a bug (though I don't have reproduction steps) where you can get duplicate endpoints generated for the same actual API endpoint. (I'll figure out a simple repro so I can file it at some point).

I really love the idea of being able to name the path parameters but i also understand @johanbrandhorst point that the openapiv2 proto is really meant for openapi2 specific attributes.

I'd really love it if we could have some way of doing this. I'm not sure what the best way is though. Adding and additional extension just for this one concept is a pretty big ask as the field number would have to be assigned by the protobuf-global-extension-registry.

But adding a field to the JSONSchema message also seems wrong.

Maybe there could be a special syntax of the google.api.http attribute that specifies the name as it would be in the URL like:

option(google.api.http) = {
   get: "/v1/{name=foo/*:fooName}"
}

[johanbrandhorst]

I don't want to go down the path of having the openapiv2 generator mess with the http.proto annotations either (they are separate today). Is this operation something that can be done as a post-processing step of the swagger file? Maybe the best option here is to provide some tooling for users to perform post-processing of names... though that would require keeping a mapping somewhere.

[oyvindwe]

Good point about post processing, but it won't work well, as the operations get mixed between paths. It's not just a matter of replacing {name} and {name_1}. Both the path and the parameter name must be replaced as well.

IMO, JSONSchema is specific to protoc-gen-openapiv2 (just look at all the reserved fields), and could contain fields that doesn't correspond to the OpenAPI spec. We could jump field numbers to 1000 for the fields related to transformation.

Note that extension 1042 is reserved for "grpc-gateway protoc-gen-swagger", so it's this project that decides how it's used: https://chromium.googlesource.com/chromium/src/+/refs/heads/main/third_party/protobuf/docs/options.md

[oyvindwe]

I think that my change to make the specification valid makes this solution impossible with any version of grpc-gateway beyond 2.7.1

I agree - the post processing depends on an invalid OpenAPI file, which is kind of the wrong starting point. The context needed to do the post processing is lost in the valid file. It could be possible to rewrite the post processing to use the pattern field of path parameters though.

If going down this route, we could as well include the same logic in the generator, and enable it with a CLI argument. I'm still not entirely happy about it, as there is no way to provide parameter descriptions and examples.

[oyvindwe]

I just realised that supporting paths transforms like {name=shelves/*} -> shelves/{shelf_id} requires a stemming dictionary to find the singular (to avoid results like shelves/{shelve_id}). This makes me even more uncertain of this approach.

[johanbrandhorst]

I'm slightly lost, are we suggesting that neither post processing nor annotations are going to work?

[oyvindwe]

@johanbrandhorst apologies for late follow up from my side.

Regarding post processing as suggested by @dgparker : This no longer works due to @yinzara 's path fixes. Also, as discussed in issue #720 this approach has several shortcomings; the generated path parameters lacks descriptions, type information, and examples values. Also, as pointed out above, creating singular from plural isn't straight forward in English. I suggest we continue the discussion on how to generate path parameters from a path template in that issue (e.g {name=shelves/*/book/*} -> shelves/{shelf_id}/books/{book_id}).

This PR suggests an annotation to override the generated path parameter (name, name_1, name_2, etc.). This path parameter would use the same description, type, and example value as currently used. The only issue raised with this PR is that it will introduce a field to JSONSchema that isn't a part of the OpenAPI v2 specification. Technically, this PR works fine.

[johanbrandhorst]

Thanks for the update. I see we need a rebase/merge of this PR, but I think unless there are strong objects from others that adding the annotation might be okay. This is a very real request from users and I'm keen to allow this to be configured.

[ivucica]

I think this could work. I'm not too worried about new fields being added to the OpenAPIv2 spec definition of JSONSchema since that version is pretty much deprecated already. I would welcome @ivucica's thoughts on this as well.

Putting this in FieldConfiguration, as proposed later in this thread, LGTM. I have not looked at the code. Please let me know if you’d like me to do that as well.

[johanbrandhorst]

@oyvindwe, to be clear, I think we have consensus on an approach - when/if you find the time, please rebase and move the field configuration into a FieldConfiguration message 👍🏻.

[oyvindwe]

Rebased and moved field configuration to separate message!

[oyvindwe]

@johanbrandhorst any documentation that needs to be updated as well?

[johanbrandhorst]

@johanbrandhorst any documentation that needs to be updated as well?

It certainly would be nice to add a little blurb to the openAPI mapping doc: https://github.com/grpc-ecosystem/grpc-gateway/blob/master/docs/docs/mapping/customizing_openapi_output.md!

[oyvindwe]

It certainly would be nice to add a little blurb to the openAPI mapping doc: https://github.com/grpc-ecosystem/grpc-gateway/blob/master/docs/docs/mapping/customizing_openapi_output.md!

Added!

[yinzara]

Super excited to use this feature with my team! It will really help our developers who prefer the Swagger UI to our gRPC UI.

[johanbrandhorst]

One small comment, this looks generally great!

[johanbrandhorst]

Should we add a FieldConfiguration struct here too to encapsulate potential future additions?

[oyvindwe]

Done! Also reran the code generation, as expected this did not generate any more changes.

[johanbrandhorst]

Looks like there are some failing tests 😬

[oyvindwe]

Fixed, but I'm not sure if this is the right way to fix it (see comment below).

[oyvindwe]

The tests failed when I made this a pointer. Is it OK to have like this?

[johanbrandhorst]

Thinking a bit more about it, this is a struct that's supposed to define the shape of the OpenAPI Schema object, and this field doesn't exist in that object. Is there some other way we can smuggle the path parameter mapping from the annotations into the schema rendering? This feels like a hack.

[oyvindwe]

Good point. I made a separate method to get the JSONSchema_FieldConfiguration. It means an additional call to getFieldOpenAPIOption(), but I got rid of redundant calls to set pathParamName, so I think this is cleaner.

[johanbrandhorst]

LGTM

[johanbrandhorst]

Kinda regret not waiting to push out v2.9.0 now, but there are no rules against making more releases so I think this should make it into v2.10.0!

[oyvindwe]

Kinda regret not waiting to push out v2.9.0 now, but there are no rules against making more releases so I think this should make it into v2.10.0!

Ah, I saw 2.9.0 being released and assumed this was in. It with be great with 2.10.0 that also includes PRs #2599 and #2600 when they are accepted. (We depend on the fix in #2600, but we're not hit by #2599.)

[oyvindwe]

Ah, I saw 2.9.0 being released and assumed this was in. It with be great with 2.10.0 that also includes PRs #2599 and #2600 when they are accepted. (We depend on the fix in #2600, but we're not hit by #2599.)

You're releasing faster than I brew cups of coffee! :-D I guess above mentioned fixes are good candidates for 2.10.1?

[johanbrandhorst]

You're releasing faster than I brew cups of coffee! :-D I guess above mentioned fixes are good candidates for 2.10.1?

Indeed