[protoc-gen-openapi] Support Fully Qualified Schema Names and Annotations for File/Document and Method/Operation #324

jeffsawatzky · 2022-03-28T15:57:23Z

This does a couple things:

Refactor the code a little to provide the actual message and field instances to more places instead of just the descriptor
Removed the formatMessageRef function as it was basically a duplicate of formatMessageName, and with the above refactor it wasn't needed anymore
Added support for adding the message package name to the schema name to support fully qualified schema names. This is done with the new option fq_schema_naming
Added support for options on the File (which are used on the OpenAPI Document) and Method (for the OpenAPI Operation)
Added some tests for the new features
Added functionality to easily generate new test fixtures and update existing test fixtures if needed.

Fixes #308
Fixes #309
Fixes #322

google-cla · 2022-03-28T15:57:28Z

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

For more information, open the CLA check for this pull request.

jeffsawatzky · 2022-03-28T16:05:43Z

@googlebot I signed it!

…oogle#308

jeffsawatzky · 2022-03-28T21:01:19Z

@timburks Could you have a look at the PR when you have time. Thanks!

jeffsawatzky · 2022-03-29T15:02:22Z

@timburks I'm in the process of bringing the functionality of protoc-gen-openapi close to the grpc-gateway-openapiv2 one in terms of customization, but all may changes are based on each other.

Do you want everything in one huge PR? Or would you prefer smaller ones? If smaller ones, then this PR is what most of the newer ones will be based off of so I would really appreciate it if the could get reviewed, and if you approve, merged.

Thanks!

timburks · 2022-03-29T17:43:01Z

@jeffsawatzky Thanks for this work and your explanations. I'll review (and expect to approve) this today; from your note at the top, it all SGTM.

Would you mind spelling out what you mean by "bringing the functionality of protoc-gen-openapi close to the grpc-gateway-openapiv2 one in terms of customization"? What specific things are you hoping to add? Just wanting to sync on top-level goals.

Thanks!

/cc @morphar

jeffsawatzky · 2022-03-29T18:20:46Z

@timburks being able to embed OpenAPI elements into the proto as options so you can define extra things like security on a method within the proto file. I've added examples of this to the tests in this PR.

Also, envoy and grpc-gateway return errors using the google.rpc.Status message type and currently this plugin doesn't support the google.protobuf.Any message properly which is used by google.rpc.Status message (see #327 which adds support for this)

timburks

@jeffsawatzky thanks, this looks great. I really like the options annotations and how well they fit into the gnostic data structures.

I'll let this sit overnight (California time) in case @morphar has time to look and comment and if no objections come up, I think we can merge this tomorrow morning (PDT).

timburks · 2022-03-30T03:19:37Z

openapiv3/annotations.proto

+option go_package = "./openapiv3;openapi_v3";
+
+extend google.protobuf.FileOptions {
+  Document document = 1042;


So (just digesting), this allows an arbitrary OpenAPI document to be attached to a file, presumably a partial document that gets overlaid/merged onto the generated OpenAPI spec.

@timburks Correct. It will do a proto merge on the result Document and any openapi.v3.document options it finds while compiling the proto files.

timburks · 2022-03-30T03:19:59Z

openapiv3/annotations.proto

+}
+
+extend google.protobuf.MethodOptions {
+  Operation operation = 1042;


Similarly, this merges an Operation into the operation generated for a method. This is a gnostic type (as is Document) - and I notice from your example that you're assigning them with valid structs, which are unintuitive to most OpenAPI users but fit the gnostic protobuf model - and I think that consistency is good. Without looking through your generator code (yet), I guess that makes it easier to merge these with the generated doc since that's working on gnostic protobuf-based structs.

Protocol Buffer options need to be defined using protocol buffers themselves. So you will never get a 1-1 mapping of the options structure with OpenAPI structure as protocol buffers are strictly typed and yaml/json are not. Also, this is similar to how the protoc-gen-openapiv2 generator works from the grpc-gateway project (though they use protocol buffers that are specific to OpenAPI v2/Swagger). I think most developers that will use this will understand this.

Since I need to use protocol buffer messages for the options, I reused the same ones that gnostic was already using to make merging them easier, as you said.

timburks · 2022-03-30T03:30:59Z

cmd/protoc-gen-openapi/generator/openapi-v3.go

+			// Merge any `Document` annotations with the current
+			extDocument := proto.GetExtension(file.Desc.Options(), v3.E_Document)
+			if extDocument != nil {
+				proto.Merge(d, extDocument.(*v3.Document))


Nice, this seems really clever and elegant. I wonder if there are any other places (besides methods and files) where it would make sense to attach a partial document - maybe messages or fields?

Yeah, I can add the same for Messages=>Schemas and Fields=>Parameters. But didn't want to do too much in case you reject the whole idea. I can do this in a separate PR.

I think that would be great, thanks!

timburks · 2022-03-30T03:38:16Z

cmd/protoc-gen-openapi/main.go

+		Title:          flags.String("title", "", "name of the API"),
+		Description:    flags.String("description", "", "description of the API"),
+		Naming:         flags.String("naming", "json", `naming convention. Use "proto" for passing names directly from the proto files`),
+		FQSchemaNaming: flags.Bool("fq_schema_naming", false, `schema naming convention. If "true" prefixes the schema name with the proto message package name`),


Let's spell out what "fq" stands for here in the help string, maybe like this:

If "true", generates fully-qualified schema names by prefixing them with the proto message package name.

I have a couple more PRs coming. I will update the help string there, if that's ok with you.

timburks · 2022-03-30T03:51:35Z

openapiv3/annotations.proto

+// The Go package name.
+option go_package = "./openapiv3;openapi_v3";
+
+extend google.protobuf.FileOptions {


The other files in this directory are automatically-generated - that's not a problem, and I think this is a good place to put this file, especially since it refers to types defined inside the package.

Eventually (not necessarily in this PR) I think it would be good to have some documentation here of how these annotations work. google/api/http.proto is a good example of this - though I'm not thinking of anything nearly as elaborate as that, that file seems to be a good example of where to put proto documentation -- in comments in the protos so that it never gets separated.

morphar · 2022-03-30T04:02:09Z

Sorry, I was away from my computer, I'll have a look at it now :)

morphar · 2022-03-30T04:57:52Z

I have no comments to add whatsoever! It looks really good to me 👍

As far as I can tell, all existing things just works, while adding some nice functionality.
I just did a full run of tests on a local project (gRPC, gRPC-Web, Envoy, OpenAPI, etc.) and nothing breaks there either.

Really nice @jeffsawatzky 👍

ppaanngggg · 2022-03-30T12:52:05Z

so, merge this plz. I really need this feature too. 👍

jeffsawatzky · 2022-03-30T13:21:15Z

@timburks / @morphar once you merge this PR, can you also take a look at my other PR #327 which is based on this? I have some more functionality that I would like to add for my use case, such as adding an option for a default response for error responses that use the google.rpc.Status message (since Envoy and grpc-gateway both use google.rpc.Status as their error response). But it would be helpful if these two PRs could be merged first.

ppaanngggg · 2022-03-31T07:02:01Z

Could you please add a Tag on this commit? @timburks @morphar

morphar · 2022-03-31T07:10:41Z

It's only @timburks who can add tags and versions. I'm just a contributor :)

support fully qualified schema names. fixes google#309

15e2bfb

jeffsawatzky requested a review from a team as a code owner March 28, 2022 15:57

jeffsawatzky changed the title ~~Support Fully Qualified Schema Names~~ [protoc-gen-openapi] Support Fully Qualified Schema Names Mar 28, 2022

add support for file/document and method/operation annotations. fixes g…

73b03aa

…oogle#308

jeffsawatzky changed the title ~~[protoc-gen-openapi] Support Fully Qualified Schema Names~~ [protoc-gen-openapi] Support Fully Qualified Schema Names and Option Annotations for File/Document and Method/Operation Mar 28, 2022

jeffsawatzky changed the title ~~[protoc-gen-openapi] Support Fully Qualified Schema Names and Option Annotations for File/Document and Method/Operation~~ [protoc-gen-openapi] Support Fully Qualified Schema Names and Annotations for File/Document and Method/Operation Mar 28, 2022

jeffsawatzky mentioned this pull request Mar 29, 2022

Feat/fullpackage #325

Closed

fix the package name in my tests.

6297941

jeffsawatzky mentioned this pull request Mar 29, 2022

[protoc-gen-openapi] Support google.protobuf.Any and google.rpc.Status #327

Merged

timburks approved these changes Mar 30, 2022

View reviewed changes

timburks merged commit ec4d35c into google:master Mar 30, 2022

morphar mentioned this pull request Apr 27, 2022

How should I specify application/x-www-form-urlencoded as Content-Type in the post request? #350

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[protoc-gen-openapi] Support Fully Qualified Schema Names and Annotations for File/Document and Method/Operation #324

[protoc-gen-openapi] Support Fully Qualified Schema Names and Annotations for File/Document and Method/Operation #324

jeffsawatzky commented Mar 28, 2022 •

edited

google-cla bot commented Mar 28, 2022

jeffsawatzky commented Mar 28, 2022

jeffsawatzky commented Mar 28, 2022

jeffsawatzky commented Mar 29, 2022

timburks commented Mar 29, 2022

jeffsawatzky commented Mar 29, 2022 •

edited

timburks left a comment

timburks Mar 30, 2022

jeffsawatzky Mar 30, 2022

timburks Mar 30, 2022

jeffsawatzky Mar 30, 2022

timburks Mar 30, 2022

jeffsawatzky Mar 30, 2022

timburks Mar 30, 2022

timburks Mar 30, 2022

jeffsawatzky Mar 30, 2022

timburks Mar 30, 2022

timburks Mar 30, 2022

morphar commented Mar 30, 2022

morphar commented Mar 30, 2022 •

edited

ppaanngggg commented Mar 30, 2022

jeffsawatzky commented Mar 30, 2022

ppaanngggg commented Mar 31, 2022

morphar commented Mar 31, 2022

[protoc-gen-openapi] Support Fully Qualified Schema Names and Annotations for File/Document and Method/Operation #324

[protoc-gen-openapi] Support Fully Qualified Schema Names and Annotations for File/Document and Method/Operation #324

Conversation

jeffsawatzky commented Mar 28, 2022 • edited

google-cla bot commented Mar 28, 2022

jeffsawatzky commented Mar 28, 2022

jeffsawatzky commented Mar 28, 2022

jeffsawatzky commented Mar 29, 2022

timburks commented Mar 29, 2022

jeffsawatzky commented Mar 29, 2022 • edited

timburks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morphar commented Mar 30, 2022

morphar commented Mar 30, 2022 • edited

ppaanngggg commented Mar 30, 2022

jeffsawatzky commented Mar 30, 2022

ppaanngggg commented Mar 31, 2022

morphar commented Mar 31, 2022

jeffsawatzky commented Mar 28, 2022 •

edited

jeffsawatzky commented Mar 29, 2022 •

edited

morphar commented Mar 30, 2022 •

edited