protoc-gen-swagger: Add ability to specify custom response objects #663

johanbrandhorst · 2018-05-31T10:05:06Z

Extends the openapiv2 protofile to allow control over response objects.

Fixes #304

johanbrandhorst · 2018-05-31T10:05:31Z

codecov-io · 2018-05-31T14:04:02Z

Codecov Report

Merging #663 into master will decrease coverage by 1.36%.
The diff coverage is 13.15%.

@@            Coverage Diff             @@
##           master     #663      +/-   ##
==========================================
- Coverage   57.78%   56.42%   -1.37%     
==========================================
  Files          30       30              
  Lines        2914     2995      +81     
==========================================
+ Hits         1684     1690       +6     
- Misses       1066     1141      +75     
  Partials      164      164

Impacted Files	Coverage Δ
protoc-gen-swagger/genswagger/template.go	`38.52% <13.15%> (-3.59%)`	⬇️

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 a5b66c1...be7eb36. Read the comment docs.

achew22 · 2018-05-31T15:51:08Z

I like this PR, I need to look at it in a bigger screen then my phone but it looks like a great additional piece of documentation to include

achew22 · 2018-06-01T20:18:54Z

examples/proto/examplepb/a_bit_of_everything.proto

-	    key: "admin";
-	    value: "Grants read and write access to administrative information";
-	  }
+	info: {


Out of curiosity, why did you change everything to be tabs? Can you run this through a modern version of clang-format?

Everything except for the examples added by @ivucica was already tabs, so I just made it consistent as it was driving me mad. I could format this (and the other protofiles) with prototool format 1 if you like, but I haven't touched the others.

(Sorry about this.)

johanbrandhorst · 2018-06-04T08:52:50Z

Gonna see if I can add automatic references for namespaced messages as well, so please don't merge just yet.

johanbrandhorst · 2018-06-04T08:53:19Z

Actually, go ahead and merge if it's good, I'll make another PR for the additional functionality.

johanbrandhorst · 2018-06-04T10:33:58Z

Wasn't too hard it turns out :)

johanbrandhorst · 2018-06-04T16:46:07Z

protoc-gen-swagger/genswagger/template.go

+		} else {
+			ret.schemaCore.Ref += s.GetJsonSchema().GetRef()
+		}
+	} else {


One thing this doesn't do is include external messages that aren't already inlined. I think we could potentially do some proto registry lookups and add those messages if necessary, but it'd be quite complex so I'm not sure we want to?

ivucica

Mostly in favor. I have just a few nitpicks, and a few requests to evaluate code reuse.

Thanks for this!

ivucica · 2018-06-04T21:30:06Z

examples/proto/examplepb/a_bit_of_everything.proto

@@ -155,7 +183,7 @@ message ABitOfEverything {

 	google.protobuf.Timestamp timestamp_value = 27;

-        // repeated enum value. it is comma-separated in query
+		// repeated enum value. it is comma-separated in query


Remove one tab maybe?

Woops! Removed.

ivucica · 2018-06-04T21:36:18Z

protoc-gen-swagger/genswagger/template.go

@@ -1187,3 +1230,70 @@ func extractSwaggerOptionFromFileDescriptor(file *pbdescriptor.FileDescriptorPro
 	}
 	return opts, nil
 }
+
+func swaggerSchemaFromProtoSchema(s *swagger_options.Schema, reg *descriptor.Registry) swaggerSchemaObject {


Should https://github.com/johanbrandhorst/grpc-gateway/blob/97aefd3e41117e62982ef489dfb516a228a39ea3/protoc-gen-swagger/genswagger/template.go#L231-L245 and other places extracting the schema object be combined with this?

Great idea, I'll see if I can make that work.

ivucica · 2018-06-04T21:36:59Z

protoc-gen-swagger/genswagger/template.go

+	}
+}
+
+func protoExternalDocumentationToSwaggerExternalDocumentation(in *swagger_options.ExternalDocumentation) *swaggerExternalDocumentationObject {


Should https://github.com/johanbrandhorst/grpc-gateway/blob/97aefd3e41117e62982ef489dfb516a228a39ea3/protoc-gen-swagger/genswagger/template.go#L233-L241 and other places with ExternalDocs be combined with this?

I think I squashed these.

ivucica · 2018-06-04T21:37:54Z

protoc-gen-swagger/genswagger/template_test.go

@@ -4,12 +4,13 @@ import (
 	"reflect"
 	"testing"

+	"fmt"
+


Actually if this is being moved, maybe move it together with "testing" and "reflect" and others? Seems strange to have it hang on its own

Didn't even realise I had changed this, thanks linters!

ivucica · 2018-06-04T21:39:57Z

protoc-gen-swagger/options/openapiv2.proto

-  // field 3 is reserved for '$ref', although it is unclear how it would be used.
-  reserved 3;
+  // Ref is used to define an external reference to include in the message.
+  // This should be a fully qualified message reference, and that type must be imported


qualified message reference

I'd be slightly confused about this, maybe fully qualified protobuf message reference? Not sure.

ivucica · 2018-06-04T21:40:41Z

protoc-gen-swagger/options/openapiv2.proto

-  // field 3 is reserved for '$ref', although it is unclear how it would be used.
-  reserved 3;
+  // Ref is used to define an external reference to include in the message.
+  // This should be a fully qualified message reference, and that type must be imported


This should be

This could be, I suppose?

I've adjusted it accordingly. I hope together with the example it is obvious enough.

johanbrandhorst · 2018-06-05T13:15:41Z

protoc-gen-swagger/genswagger/template.go

+			}
+			if protoSchema.Description != "" {
+				schema.Description = protoSchema.Description
+			}


I added these because I need them :P

johanbrandhorst · 2018-06-06T08:34:58Z

@ivucica are you happy with the changes? @achew22 could you rerun the failure please, seems like a github TLS timeout error?

achew22 · 2018-06-06T17:19:39Z

Retriggered

achew22 · 2018-06-06T17:36:48Z

I think the error comes from an update to the protobuf generator. Could you update your protoc and go plugin and regenerate with make examples then upload again?

I expect it should add the following

index cb6fb3a..d416004 100644
--- a/examples/clients/abe/examplepb_a_bit_of_everything.go
+++ b/examples/clients/abe/examplepb_a_bit_of_everything.go
@@ -14,6 +14,7 @@ import (
 	"time"
 )
 
+// Intentionaly complicated message type to cover much features of Protobuf.
 type ExamplepbABitOfEverything struct {
 
 	SingleNested ABitOfEverythingNested `json:"single_nested,omitempty"`

ivucica · 2018-06-06T18:58:19Z

No objections on my part as long as the tests are fixed.

johanbrandhorst · 2018-06-06T22:22:18Z

Cool, I think I should have fixed the failing test, lets get this in :D.

achew22 · 2018-06-06T22:45:35Z

Looks like there are a few remaining issues. Can you give it another whack?

johanbrandhorst · 2018-06-06T23:11:46Z

@achew22 I can't seem to get swagger-codegen installed - I hate to be a bother but any chance you could just clone and regenerate the examples? I'll take another stab tomorrow at work but haven't got time now.

ivucica · 2018-06-07T07:56:56Z

Last time I was doing it, I just did what was done for Travis. :-)

--

Sent from Gmail Mobile

johanbrandhorst · 2018-06-07T08:01:41Z

@ivucica thanks I will take a look through the logs. I tried using their docker image and it didn't work.

johanbrandhorst · 2018-06-07T09:12:31Z

Alright I should've gotten them this time. Sure is fiddly 🤔.

johanbrandhorst · 2018-06-07T11:47:29Z

OK so I had some time to look into recursive resolution of manually referenced types - I got it working but I can't help but feel like it's a bit hacky. I'd be happy with any tips on improving it, but basically it required me to keep a separate set of referenced definitions and render them after everything else, recursively.

johanbrandhorst · 2018-06-11T15:01:28Z

Bump - any thoughts on the additions?

ivucica

A few nits to correct and I will approve.

As they're tiny issues, I would be willing to approve even as is.

ivucica · 2018-06-15T15:39:30Z

examples/clients/abe/examplepb_a_bit_of_everything.go

@@ -14,6 +14,7 @@ import (
 	"time"
 )

+// Intentionaly complicated message type to cover much features of Protobuf.


Nit: s/much/many/

Yeh I intentionally didn't change this existing typo but may as well :D

ivucica · 2018-06-15T15:40:42Z

examples/proto/examplepb/a_bit_of_everything.proto

 };


 // Intentionaly complicated message type to cover much features of Protobuf.
 // NEXT ID: 30
 message ABitOfEverything {
 	option (grpc.gateway.protoc_gen_swagger.options.openapiv2_schema) = {
+		json_schema: {
+			title: "A bit of everything"
+			description: "Intentionaly complicated message type to cover much features of Protobuf."


Nit: s/much/many/

We can skip this if it'd take you a lot of time to regenerate examples.

ivucica · 2018-06-15T15:42:30Z

protoc-gen-swagger/genswagger/template.go

@@ -512,7 +515,7 @@ func templateToSwaggerPath(path string) string {
 	return strings.Join(parts, "/")
 }

-func renderServices(services []*descriptor.Service, paths swaggerPathsObject, reg *descriptor.Registry, refs refMap) error {
+func renderServices(services []*descriptor.Service, paths swaggerPathsObject, reg *descriptor.Registry, requestResponseRefs refMap, customRefs refMap) error {


requestResponseRefs, customRefs refMap is also valid

ivucica · 2018-06-15T15:44:15Z

protoc-gen-swagger/genswagger/template.go

-	// Find all the service's messages and enumerations that are defined (recursively) and then
-	// write their request and response types out as definition objects.
+	// Find all the service's messages and enumerations that are defined (recursively)
+	// and write request, response and referenced types out as definition objects.


Maybe: s/referenced types/other custom (but referenced) types/ ?

Extends the openapiv2 protofile to allow control over response objects. Fixes grpc-ecosystem#304

johanbrandhorst · 2018-06-15T18:24:26Z

Made changes as requested

johanbrandhorst · 2018-06-15T18:48:05Z

@tmc @achew22 I think the test failure was a flaky test, could one of you rerun it please?

achew22 · 2018-06-16T06:43:44Z

Rerun. I'll try to check on it in 20 but I'll probably forget. If I don't do something by tomorrow morning please reply here, it means I forgot (sorry in advance)

johanbrandhorst · 2018-06-16T10:56:45Z

@achew22 thanks for retriggering it, seems like it passed ;). I appreciate the time you put into maintaining this project, so no worries about forgetting!

johanbrandhorst · 2018-06-16T10:58:08Z

@ivucica anymore comments?

johanbrandhorst · 2018-06-18T21:33:15Z

Friendly Monday bump @achew22 @ivucica

achew22 · 2018-06-18T23:35:29Z

LGTM. Let's give @ivucica 24 hours from your ping before merging. I will rebase tomorrow around this time. If I don't please reping and I'll merge when I get to a computer.

PS: Sorry this is takling forever, it's a great feature and I am REALLY looking forward to using it. Taking a little extra time is definitely worth it.

johanbrandhorst · 2018-06-20T08:13:26Z

At the risk of becoming a tyrant on my first day, I'm going to commandeer and this and merge as @ivucica has been given ample time to add any more opinions and he conceded that he was happy with the PR even without the minor changes, that have been addressed.

ivucica · 2018-06-20T13:34:04Z

Sorry, travelling. No objection to the PR.

…

On Wed, Jun 20, 2018 at 4:13 AM Johan Brandhorst ***@***.***> wrote: At the risk of becoming a tyrant on my first day, I'm going to commandeer and this and merge as @ivucica <https://github.com/ivucica> has been given ample time to add any more opinions and he conceded that he was happy with the PR even without the minor changes, that have been addressed.

achew22 · 2018-06-20T22:35:33Z

@johanbrandhorst I approve of the action and the attitude. Thanks for merging

Examples: Consistently use tabs in a_bit_of_everything

15cae3f

googlebot added the cla: yes label May 31, 2018

johanbrandhorst mentioned this pull request May 31, 2018

Response codes and descriptions in Swagger docs #304

Closed

johanbrandhorst force-pushed the add-swagger-response-configuration branch from 342942d to d29e934 Compare May 31, 2018 13:50

achew22 reviewed Jun 1, 2018

View reviewed changes

johanbrandhorst force-pushed the add-swagger-response-configuration branch from d29e934 to 97aefd3 Compare June 4, 2018 10:33

johanbrandhorst commented Jun 4, 2018

View reviewed changes

ivucica requested changes Jun 4, 2018

View reviewed changes

johanbrandhorst force-pushed the add-swagger-response-configuration branch 3 times, most recently from 2bd2081 to 002aef4 Compare June 5, 2018 13:14

johanbrandhorst commented Jun 5, 2018

View reviewed changes

ivucica approved these changes Jun 6, 2018

View reviewed changes

achew22 approved these changes Jun 6, 2018

View reviewed changes

johanbrandhorst force-pushed the add-swagger-response-configuration branch from 002aef4 to 7897160 Compare June 6, 2018 22:21

johanbrandhorst force-pushed the add-swagger-response-configuration branch 2 times, most recently from 8029320 to 9909ef7 Compare June 7, 2018 09:08

johanbrandhorst force-pushed the add-swagger-response-configuration branch from 9909ef7 to 760aa75 Compare June 7, 2018 11:45

johanbrandhorst force-pushed the add-swagger-response-configuration branch 2 times, most recently from bb657d1 to be7eb36 Compare June 7, 2018 22:20

ivucica requested changes Jun 15, 2018

View reviewed changes

protoc-gen-swagger: Add ability to specify custom response objects

9ed7b67

Extends the openapiv2 protofile to allow control over response objects. Fixes grpc-ecosystem#304

johanbrandhorst force-pushed the add-swagger-response-configuration branch from be7eb36 to 9ed7b67 Compare June 15, 2018 18:23

johanbrandhorst merged commit 9b9677c into grpc-ecosystem:master Jun 20, 2018

johanbrandhorst deleted the add-swagger-response-configuration branch June 20, 2018 08:13

protoc-gen-swagger: Add ability to specify custom response objects #663

protoc-gen-swagger: Add ability to specify custom response objects #663

Conversation

johanbrandhorst commented May 31, 2018

johanbrandhorst commented May 31, 2018

codecov-io commented May 31, 2018 • edited Loading

Codecov Report

achew22 commented May 31, 2018

Choose a reason for hiding this comment

johanbrandhorst Jun 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johanbrandhorst commented Jun 4, 2018

johanbrandhorst commented Jun 4, 2018

johanbrandhorst commented Jun 4, 2018 • edited Loading

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

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

johanbrandhorst commented Jun 6, 2018

achew22 commented Jun 6, 2018

achew22 commented Jun 6, 2018

ivucica commented Jun 6, 2018

johanbrandhorst commented Jun 6, 2018 • edited Loading

achew22 commented Jun 6, 2018

johanbrandhorst commented Jun 6, 2018 • edited Loading

ivucica commented Jun 7, 2018 via email

johanbrandhorst commented Jun 7, 2018

johanbrandhorst commented Jun 7, 2018

johanbrandhorst commented Jun 7, 2018 • edited Loading

johanbrandhorst commented Jun 11, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johanbrandhorst commented Jun 15, 2018

johanbrandhorst commented Jun 15, 2018

achew22 commented Jun 16, 2018

johanbrandhorst commented Jun 16, 2018

johanbrandhorst commented Jun 16, 2018

johanbrandhorst commented Jun 18, 2018

achew22 commented Jun 18, 2018

johanbrandhorst commented Jun 20, 2018

ivucica commented Jun 20, 2018 via email

achew22 commented Jun 20, 2018

codecov-io commented May 31, 2018 •

edited

Loading

johanbrandhorst Jun 1, 2018 •

edited

Loading

johanbrandhorst commented Jun 4, 2018 •

edited

Loading

johanbrandhorst commented Jun 6, 2018 •

edited

Loading

johanbrandhorst commented Jun 6, 2018 •

edited

Loading

johanbrandhorst commented Jun 7, 2018 •

edited

Loading