add custom options to allow more control of swagger/openapi output #145

ivucica · 2016-05-07T22:43:50Z

Continuation of #134, pending review of that.

tmc · 2016-05-07T22:58:10Z

I'm personally welcoming of the switch of the naming around swagger to open api spec. I don't have a better label/tag offhand but putting "Swagger: " into this api means a slower transition to OpenAPI Specification.

openapi: ?

ivucica · 2016-05-07T23:10:05Z

While OpenAPI is more accurate, Swagger is a very 'googleable' name -- it's a shame it's being dropped. Still, I've switched to the OpenAPI: prefix. This looks a bit off, given that absolutely everything else in this project refers to 'swagger'; nonetheless, you're right, if it'll live in .protos, it deserves a more long-term name.

I also think using JSON here in the first place is a short-term cop-out fix (i.e. better than nothing), even though the resulting JSON looks just awful everywhere else. Long term, something that's still hierarchical such as:

OpenAPI-Info-Version: 2.0
OpenAPI-ExternalDocs-Name: Find out more at
OpenAPI-ExternalDocs-URL: https://www.example.net/

would be more readable, while not much harder to parse. Perhaps even with Debian-style multilines:

OpenAPI-Description: Hello world.
 This is a text over multiple lines,
 with a blank line, too.
 .
 This is a new paragraph.

That wouldn't look too horrible when seen by tooling of other languages in the generated code, either.

That's a topic for some other PR, however.

tmc · 2016-05-08T00:56:05Z

I agree that it feels off in context. When considering longer term I do think your change makes more sense.

yugui · 2016-05-09T08:52:30Z

I wonder if we can use custom options to annotate patches.
What do you think?

tmc · 2016-05-12T17:57:50Z

@yugui I like that idea -- could you propose how that might look?

ivucica · 2016-05-15T22:37:29Z

@yugui It's interesting. I guess we need to describe the Swagger schema as a .proto, in which case we might be able to do away with (what will then be) the duplicate definitions in types.go.

I probably won't have a chance to work on this any time soon, though.

yugui · 2016-05-20T05:25:42Z

@tmc, @ivucica
In my personal opinion, it is not too bad even if we only have a single string custom option which stores schema patch in JSON because at least we could avoid parsing comments.

I am worrying about parsing comments because it is not very common in protobuf convention and it has less power than custom options.
With custom option, we could deprecate the simple string option and introduce a well-organized messages to describe schema in future.

tmc · 2016-05-21T02:50:47Z

I like this as a middle ground. @ivucica thoughts?

On Thursday, May 19, 2016, Yuki Yugui Sonoda [email protected]
wrote:

@tmc https://github.com/tmc, @ivucica https://github.com/ivucica
In my personal opinion, it is not too bad even if we only have a single
string custom option which stores schema patch in JSON because at least we
could avoid parsing comments.

I am worrying about parsing comments because it is not very common in
protobuf convention and it has less power than custom options.
With custom option, we could deprecate the simple string option and
introduce a well-organized messages to describe schema in future.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#145 (comment)

ivucica · 2016-05-21T20:23:20Z

SGTM, but again, I am not sure when I'll be able to work on this. :)

googlebot · 2016-10-07T17:10:40Z

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

ivucica · 2016-10-09T23:20:57Z

Rebased and pushed. No other changes, i.e. still not based around proto options.

ivucica · 2017-02-10T18:35:57Z

Rebased and pushed once again. From: 4ea3bbf 3525ecb e3b11c1

Still no work on proto options.

ivucica · 2017-03-10T21:22:47Z

@yugui @tmc Could you PTAL and let me know if the direction I'm taking is worth pursuing? I can also switch to just a single option.

For now I've kept the JSON in the comment, too.

I have not tried to properly update all the tests, or squashing the commits, given we might change the direction.

Random thought: There is an interesting side effect if there is an option to put JSON in the comment: generated code would include this metadata in its docstrings. It might not be a bad thing, either?

tmc · 2017-03-10T21:52:59Z

@ivucica I think the custom option to supply additional openapi to inject is the way to go here.

ivucica · 2017-03-11T01:44:36Z

@tmc Yes, but which variant? I'm currently creating a larger proto descriptor instead of just accepting a piece of JSON. See PR for details :)

tmc · 2017-04-19T13:55:05Z

@ivucica sorry I missed this comment. -1 to overloading comments, +1 to rich options.

(option (grpc_gateway.protoc_gen_swagger.options.swagger_swagger) = {
externalDocs: {}

over

//   "externalDocs": {)

Question -- can we avoid the options.swagger_swagger stutter?

ivucica · 2017-04-19T15:04:46Z

What I meant to ask is, should the proto option accept a JSON string, or as I started, describe the Swagger doc schema in the proto language :)

I'll see what I can do to avoid the "stutter". :)

tmc · 2017-04-19T15:26:34Z

Unless it presents a serious technical challenge I think the richer option is better.

I think it might be worth revisiting the s/swagger/openapi/g issue -- do you have thoughts there?

eddiezane · 2017-10-25T22:13:24Z

Any update on this?

Looks like Swagger has officially been renamed to the OpenAPI Specification.

I'm looking to open a PR to add some additional options that will allow for filtering out private/internal fields as well as marking things as Output only.

Looking to this PR as guidance.

ivucica · 2017-10-25T23:12:17Z

@eddiezane This surprises me, but apparently, technically:

As a result, the last version of the Swagger Specification is v2.0, and the first OpenAPI Specification is version 3.0.

(Taken from the post you linked to)

On multiple occasions, I was trying to figure out what to reply in relation to the rename, and my general observation is that a lot of the v2 spec still uses "Swagger" terminology (see "Swagger Object", for example). Also, proto generator that this PR applies to is called protoc-gen-swagger. So I'll do the rename where reasonable! :)

I can try to tackle this this weekend. If you see updates here by, say, mid-next week, you can take my work and run with it.

(On a separate note, I think OpenAPI v3 generator should be a separate generator. A quick glance at the spec about a week ago made me think it's distinct enough that it deserves a protoc-gen-openapi or so. That's a totally separate issue.)

Depends on PR grpc-ecosystem#134. Reverts 562956f.

…agger: '. Per discussion in grpc-ecosystem#134.

achew22 · 2017-10-28T21:15:20Z

First... someone, please confirm whether it seems like this is on the right track. I won't mind a round of code review :) @tmc? @yugui?

I know I'm not on your list, but I'm happy to give you a review. I think you're on the right track. I would venture to say you're probably 1 or 2 back/forths on review before we can merge this.

I want to move the demonstration from echo_service.proto to a_bit_of_everything.proto.

Do it. I think that's a better place for this.

I should totally email [email protected] and get us four extension IDs for attaching metadata to files, methods, messages and services (patching swagger object, operation object, schema object in definitions object, and tag object, respectively). We should not risk future binary incompatibility in on-disk protos which may merely import annotations.proto.

+1, emailing them is a great idea.

I will squash most / all of the commits.

You're welcome to do that but the default when we merge is to squash rebase so if you wanna ignore that then you're fine.

What can be punted to a future PR:
Process remaining OpenAPI v2 properties that have already been added to the openapiv2.proto.

I would prefer if you omitted them from the openapiv2.proto file if there is no support for them.

Support remaining OpenAPI v2 objects ('security', 'security definitions', etc)

These would be nice but the perfect shouldn't be the enemy of the good (which this clearly is very good).

Document individual fields in openapiv2. (Or remove the TODOs, as the openapiv2.proto option definitions generally won't be visible/autocompletable in any sort of smart code editor. Hence documenting fields might be an overkill, and referencing OpenAPI is enough.)

Yep, that's a good idea. I think giving links to the OpenAPI definitions of these things (or copy/paste into the proto) would be good.

Use existing 'deprecated' option on a method, not (just) OpenAPI field.

That's also a good idea but it can wait.

achew22

Only a couple small nits and a request to get the proto extension number registered. You're SUPER close

achew22 · 2017-10-28T21:20:00Z

protoc-gen-swagger/genswagger/template.go

 )

+var swaggerExtrasRegexp = regexp.MustCompile(`(?s)^(.*[^\s])[\s]*<!-- swagger extras start(.*)swagger extras end -->[\s]*(.*)$`)


swaggerExtrasRegexp... what's that? Could you add a comment here explaining it's purpose? It doesn't seem to get used anywhere else

That's dead code from the original approach in 6ff06d8. Thanks for spotting! Removed.

achew22 · 2017-10-28T21:21:17Z

protoc-gen-swagger/genswagger/template.go

@@ -175,6 +179,25 @@ func renderMessagesAsDefinition(messages messageMap, d swaggerDefinitionsObject,
 		if err := updateSwaggerDataFromComments(&schema, msgComments); err != nil {
 			panic(err)
 		}
+		opts, err := extractSchemaOptionFromMessageDescriptor(msg.DescriptorProto)
+		if opts != nil {
+			if err != nil {


Can you promote error checking to immediately after the call to extractSchemaOptionFromMessageDescription?

achew22 · 2017-10-28T21:30:54Z

protoc-gen-swagger/genswagger/template.go

+	}
+	opts, ok := ext.(*swagger_options.Schema)
+	if !ok {
+		return nil, fmt.Errorf("extension is %T; want an Schema", ext)


Nit: "want a Schema"

achew22 · 2017-10-28T21:31:48Z

protoc-gen-swagger/options/annotations.proto

+import "google/protobuf/descriptor.proto";
+
+extend google.protobuf.FileOptions {
+  // TODO(ivucica): ask [email protected] to assign ID


Let's do this soon so we can merge

achew22 · 2017-10-28T21:33:16Z

protoc-gen-swagger/options/openapiv2.proto

@@ -0,0 +1,223 @@
+syntax = "proto3";


Nice touch on reserving all the numbers for the order they appear in the swagger spec!

achew22 · 2017-10-28T21:35:16Z

protoc-gen-swagger/genswagger/template.go

+		if spb.BasePath != "" {
+			s.BasePath = spb.BasePath
+		}
+		if len(spb.Schemes) > 0 {


What is the correct default here if schemes is empty? Right now I don't think it emit anything but is that the correct behavior? Should it emit "http" only so at least there is a value?

Good question! This is correct as-is and has the same general approach that the rest of the code has: if OpenAPI v2 values are manually defined in the proto, then they overwrite the defaults; otherwise, they don't.

For schemes value, the defaults are already defined as http and https, in line 639 (function applyTemplate).

… out of check for opts != nil.

ivucica

Thanks! PTAL

ivucica · 2017-10-29T13:40:18Z

protoc-gen-swagger/genswagger/template.go

 )

+var swaggerExtrasRegexp = regexp.MustCompile(`(?s)^(.*[^\s])[\s]*<!-- swagger extras start(.*)swagger extras end -->[\s]*(.*)$`)


That's dead code from the original approach in 6ff06d8. Thanks for spotting! Removed.

ivucica · 2017-10-29T13:40:58Z

protoc-gen-swagger/genswagger/template.go

@@ -175,6 +179,25 @@ func renderMessagesAsDefinition(messages messageMap, d swaggerDefinitionsObject,
 		if err := updateSwaggerDataFromComments(&schema, msgComments); err != nil {
 			panic(err)
 		}
+		opts, err := extractSchemaOptionFromMessageDescriptor(msg.DescriptorProto)
+		if opts != nil {
+			if err != nil {


ivucica · 2017-10-29T13:44:46Z

protoc-gen-swagger/genswagger/template.go

+		if spb.BasePath != "" {
+			s.BasePath = spb.BasePath
+		}
+		if len(spb.Schemes) > 0 {


Good question! This is correct as-is and has the same general approach that the rest of the code has: if OpenAPI v2 values are manually defined in the proto, then they overwrite the defaults; otherwise, they don't.

For schemes value, the defaults are already defined as http and https, in line 639 (function applyTemplate).

ivucica · 2017-10-29T13:45:43Z

protoc-gen-swagger/genswagger/template.go

+	}
+	opts, ok := ext.(*swagger_options.Schema)
+	if !ok {
+		return nil, fmt.Errorf("extension is %T; want an Schema", ext)


ivucica · 2017-10-29T13:46:20Z

protoc-gen-swagger/options/openapiv2.proto

@@ -0,0 +1,223 @@
+syntax = "proto3";


achew22 · 2017-10-29T17:34:16Z

LGTM. Let's get the extension numbers reserved and merge it.

Thanks so much for doing this! Awesome work

ivucica · 2017-10-30T00:06:57Z

Email has been sent. Pending reply.

I would prefer if you omitted them from the openapiv2.proto file if there is no support for them.

I'd like to leave them; they should definitely be added. Removing them (while leaving a comment and id reservation -- the right way to do it) would be extra work for not much benefit, IMHO.

Also, I'll deal with the rest of the changes I mentioned soon.

tmc · 2017-10-31T00:38:50Z

Super happy to see this progress!

achew22 · 2017-11-01T04:25:21Z

@ivucica got the email issuing us the extension IDs. When you're ready please add the new IDs and we can merge this as soon as that happens.

ivucica · 2017-11-01T16:22:42Z

New IDs have been assigned. However let's block this on move to a_bit_of_everything.proto. I'll try to do this soon.

…ile from echo service to a bit of everything service.

achew22 · 2017-11-01T20:21:51Z

protoc-gen-swagger/options/annotations.proto

@@ -8,18 +8,18 @@ import "protoc-gen-swagger/options/openapiv2.proto";
 import "google/protobuf/descriptor.proto";

 extend google.protobuf.FileOptions {
-  // TODO(ivucica): ask [email protected] to assign ID
-  Swagger openapiv2_swagger = 123456788;
+  // ID assigned by [email protected] for grpc-gateway project.


Can you add a note on why they are all the same ID and why that is okay?

johanbrandhorst · 2017-11-02T09:18:44Z

examples/examplepb/a_bit_of_everything.proto

@@ -8,10 +8,43 @@ import "google/protobuf/duration.proto";
 import "examples/sub/message.proto";
 import "examples/sub2/message.proto";
 import "google/protobuf/timestamp.proto";
+import "protoc-gen-swagger/options/annotations.proto";
+
+option (grpc_gateway.protoc_gen_swagger.options.openapiv2_swagger) = {


Don't you need to import the relevant proto file to access options?

Ah, that's annotations.proto, I see. I was confused by the package here, importing from protoc-gen-swagger and then accessing a grpc_gateway namespace. Might just be me but feels like a disconnect?

My approach for setting proto package name:

this is the grpc_gateway project, and users may want some more uniqueness -> grpc_gateway

this project contains generators protoc-gen-grpc-gateway and protoc-gen-swagger; this only applies to the latter -> protoc_gen_swagger

these are option protos extending the proto descriptors a bit -> options

I'm not sure why I did not go for a fully global namespace com.github.grpc_ecosystem.grpc_gateway..... Either way there's a variety of approaches taken.

You did get me to look around so I will change this to use grpc.gateway, as that seems to be used for the runtime and example protos.

Sounds good to me :)

I do not have swagger-codegen installed, so that was broken for me while developing.

… the approach elsewhere.

ivucica · 2017-11-02T11:15:45Z

PTAL, I think this might be it for this PR on my side.

I updated my Github settings so the correct email address for this PR should be used if you're squashing the PR.

zxy198717 · 2017-12-02T12:22:04Z

@ivucica Thanks for your works!

Have you been working on Support remaining OpenAPI v2 objects ('security', 'security definitions', etc)？

ivucica · 2017-12-02T13:19:29Z

No, I haven't. I wanted to get this merged in case someone else wants to continue, but otherwise I have not yet needed things such as oauth2 definitions in the proto.

FWIW, I'll take the opportunity to speculate. I don't think I'll be working on the following, but it's an interesting thought, I think:

I can imagine a solution integrating security definitions inside the grpc-gateway-compatible and swagger-annotated .proto file with an oauth2 identity provider.

There may also be a complementary approach to this PR, #145 (which is just overriding Swagger fields) which is: use more definition files to describe the API, rather than just the .proto itself. That is: integrate with and read things such as:

The actual gateway would thus be complemented by an extra lib to support the above and by an oauth2 provider. That is, grpc-gateway's runtime+generator wouldn't be the only thing covered by protoc-gen-swagger, which would use this input to generate a more complete swagger description, too.

achew22 · 2017-12-02T18:51:41Z

I'm also in favor of the techniques described by @ivucica. Since grpc-gateway isn't going to be generating the entirety of your http server for you it is unlikely that we will be able to handle all cases. Oauth, to pick an example, is not going to be included in grpc-gateway, it is too complex and there are too many specific requirements for it to make a generalized form of it. Additionally, I know that people are already doing oauth based security on services by using interceptors that live outside the grpc-gateway codebase. Since it isn't a hard requirement that it be in grpc-gateway I think it should be in another repo. We have this today with @tmc's grpc-websocket-proxy. It is a simple 1 liner function call at handler registration and it makes things easy to handle.

Just to demarcate the lines in the sand that I will be using to determine if functionality should be included in grpc-gateway proper.

Is the functionality a part of the grpc to http spec? Internal
Is the functionality possible to implement as an interceptor? External
Is the functionality possible to implement as an interceptor with some kind of modification to the generated code (e.g. passing request context through the http handlers into grpc calls Add request_context flag to utilize (*http.Request).Context() in handlers #265)? Internalize the functionality necessary to allow a hook to perform the action but externalize the functionality itself.

Does anyone have any opinion on additional tests that should be included in order to prevent goalpost moving? I'm happy to codify these in the README file if people think that's of value.

this is a continuation of work in grpc-ecosystem#145, which left as a TODO to check remaining options fields for the operationObject during swagger generation. This PR causes protoc-gen-swagger to check if opts.Tags exists and overwrite the default value if it does exist.

…rpc-ecosystem#145) Creates additional options that can be applied to RPCs and messages that provide additional information in the OpenAPIv2 spec. These new options allow for the inclusion of Swagger object information (maps to a .proto file): * `info` * `host` * `base_path` * `schemes` * `consumes` * `produces` * `external_docs` Service information: * `tags` * `summary` * `description` * `external_docs` * `operation_id` * `consumes` * `produces` * `schemes` Operation information (maps to a method): * `tags` * `summary` * `description` * `external_docs` * `operation_id` * `consumes` * `produces` * `schemes` Schema information (maps to a message): * `json_schema` * `descriptor` * `read_only` * `external_docs` * `example` Additionally: * Change from 'Swagger: ' to 'OpenAPI: ' prefix. It should also be relatively simple to add any field that is missing to the new OpenAPI proto objects.

ivucica force-pushed the swagger_json branch from 6d157cc to 01efa6a Compare May 7, 2016 22:49

ivucica mentioned this pull request May 7, 2016

Generate Swagger description for service methods using proto comments. #134

Merged

ivucica force-pushed the swagger_json branch from d0713ed to e3b11c1 Compare October 9, 2016 23:19

ivucica force-pushed the swagger_json branch from e3b11c1 to 2bc9ddd Compare February 10, 2017 18:35

ivucica force-pushed the swagger_json branch from bc06210 to acce01c Compare March 7, 2017 16:30

tmc changed the title ~~Patch Swagger objects by specifying raw JSON in .proto~~ add custom options to allow more control of swagger/openapi output Apr 19, 2017

tmc mentioned this pull request Apr 19, 2017

Response codes and descriptions in Swagger docs #304

Closed

ivucica added 3 commits October 27, 2017 21:54

Apply additional Swagger properties using JSON.

6ff06d8

Depends on PR grpc-ecosystem#134. Reverts 562956f.

Swagger: Replaced '' for raw JSON with last paragraph 'Sw…

fcc2dca

…agger: '. Per discussion in grpc-ecosystem#134.

Change from 'Swagger: ' to 'OpenAPI: ' prefix.

20a6d40

achew22 requested changes Oct 28, 2017

View reviewed changes

ivucica added 3 commits October 29, 2017 13:40

WIP: Remove swaggerExtrasRegexp.

a386e97

WIP: Move error checking for extractSchemaOptionFromMessageDescriptor…

290f777

… out of check for opts != nil.

WIP: Fix typo: 'want a Schema'.

97ee19b

ivucica commented Oct 29, 2017

View reviewed changes

Assign IDs for proto options extensions.

ceb4c4e

ivucica force-pushed the swagger_json branch from acab412 to ceb4c4e Compare November 1, 2017 16:21

Move test definitions for options-based extension to Swagger output f…

099fda5

…ile from echo service to a bit of everything service.

achew22 reviewed Nov 1, 2017

View reviewed changes

johanbrandhorst reviewed Nov 2, 2017

View reviewed changes

ivucica added 4 commits November 2, 2017 10:32

Undo commenting out dependency on EXAMPLE_CLIENT_SRCS.

d7b0ba4

I do not have swagger-codegen installed, so that was broken for me while developing.

Change the proto package to start with 'grpc.gateway', as seems to be…

d7a9a69

… the approach elsewhere.

Add a comment about all IDs being ok to be the same.

84c3046

Regenerated Swagger-generated clients.

a543b9e

achew22 approved these changes Nov 3, 2017

View reviewed changes

achew22 merged commit 8cc3a55 into grpc-ecosystem:master Nov 3, 2017

thurt mentioned this pull request Jan 15, 2018

adds opts.Tags to operationObject.Tags #524

Closed

devnull- mentioned this pull request Jan 16, 2018

Add description, summary and tags fields in operationObject (swagger) #526

Merged

		)

		var swaggerExtrasRegexp = regexp.MustCompile(`(?s)^(.[^\s])[\s]<!-- swagger extras start(.)swagger extras end -->[\s](.*)$`)

add custom options to allow more control of swagger/openapi output #145

add custom options to allow more control of swagger/openapi output #145

Conversation

ivucica commented May 7, 2016

tmc commented May 7, 2016 • edited Loading

ivucica commented May 7, 2016

tmc commented May 8, 2016

yugui commented May 9, 2016

tmc commented May 12, 2016

ivucica commented May 15, 2016

yugui commented May 20, 2016

tmc commented May 21, 2016

ivucica commented May 21, 2016 • edited Loading

googlebot commented Oct 7, 2016

ivucica commented Oct 9, 2016

ivucica commented Feb 10, 2017 • edited Loading

ivucica commented Mar 10, 2017 • edited Loading

tmc commented Mar 10, 2017

ivucica commented Mar 11, 2017

tmc commented Apr 19, 2017

ivucica commented Apr 19, 2017

tmc commented Apr 19, 2017

eddiezane commented Oct 25, 2017

ivucica commented Oct 25, 2017

achew22 commented Oct 28, 2017

achew22 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

ivucica 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

achew22 commented Oct 29, 2017

ivucica commented Oct 30, 2017 • edited Loading

tmc commented Oct 31, 2017

achew22 commented Nov 1, 2017

ivucica commented Nov 1, 2017

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

ivucica commented Nov 2, 2017

zxy198717 commented Dec 2, 2017 • edited Loading

ivucica commented Dec 2, 2017

achew22 commented Dec 2, 2017

tmc commented May 7, 2016 •

edited

Loading

ivucica commented May 21, 2016 •

edited

Loading

ivucica commented Feb 10, 2017 •

edited

Loading

ivucica commented Mar 10, 2017 •

edited

Loading

ivucica commented Oct 30, 2017 •

edited

Loading

zxy198717 commented Dec 2, 2017 •

edited

Loading