Add swagger support #68

achew22 · 2015-12-02T05:54:33Z

Yuki,

I started to create a grpc-gateway swagger client which generates a .swagger.json file for every proto that you load in. This is what I have right now. It is broken into two commits. The first clones protoc-gen-grpc-gateway into protoc-gen-swagger and the 2nd modifies it to emit a really terrible swagger definition.

Right now it has a number of shortcomings.

It doesn't take parameters in requests
It generates both a post and a get (and no other verbs)
It's super hacky
I have no idea what I'm doing
Doesn't load any comment information into the swagger spec which would make it MUCH easier to understand the output

I was wondering if you could provide some general feedback. This is the first thing of any size I've ever done in go so sorry if it isn't idiomatic. I'm seeking comments on places that you think could be improved but please do the delta from the 2nd commit on or it will be very painful to review. "This looks like it's on the right track" or "try again with this strategy" is ultimately what I would like to get from you.

Thanks so much for creating this project! It is making my life so much less painful.

yugui · 2015-12-02T08:55:20Z

Amazing!

I'm afraid, but it takes a bit long to take a look at this PR since it is large.
Maybe I have some time to do that this weekend.

yugui · 2015-12-06T08:28:22Z

@achew22
Thank you. I really like the overall direction, e.g.

Map proto message to predefined message schema
Construct Swagger definition in process and marshal it in JSON afterward.

There are some comments.

You don't have to copy entire the protoc plugin. Code generator part of protoc-gen-grpc-gateway is pluggable by intention. So you can reuse the existing message/option parser. Feel free to propose some extension of the parser if is functionality is not sufficient for your purpose.
I'm not sure, but sometimes it might be better to define parameters of paths with parameterObject rather than referenceObject because it is allowed to place some fields in request paths.

achew22 · 2015-12-14T08:11:31Z

I have been on vacation for the past week. I intend to start taking a look at this in my spare time over this week.

yugui · 2015-12-14T08:12:17Z

👍

achew22 · 2015-12-20T07:37:06Z

@yugui, I have done a bit of cleanup on this PR. Just for reference, once you're happy with it I would like to squash it into a single commit, so don't just hit the merge button 😄.

I think your suggestion about using a parameter object is a great one. I looked at the docs and agreed with you when I was writing but I couldn't find a compelling way to do union types in Golang. Do you know what the best way to do a union type is? That seems to me like the only way to do it unfortunately.

I have updated the branch with changes to make it reuse the existing protoc-gen-grpc-gateway instead of my clone. Turns out almost all the stuff I wanted was there.

yugui · 2015-12-24T09:51:26Z

protoc-gen-swagger/genswagger/generator.go

+import (
+	"errors"
+	"fmt"
+	// Don't use the formatter


Could you just remove this if you don't need "go/format"?

yugui · 2015-12-24T10:04:36Z

https://travis-ci.org/gengo/grpc-gateway/builds/97933023

The test is broken because master branch was broken.
Maybe you just need to rebase onto the current master to fix the test.

achew22 · 2015-12-27T23:35:20Z

The thing I'm stuck on right now is that you can import arbitrary protos into your api. They don't necessarily have to be included in the file that you're currently compiling. My plan thus far had been to use "#/definitions/{{message.FQMN()}}" for all the definitions. This is great, except when it comes to iterating over all the messages, I get stuck because they may not be in the file.

Right now I'm expecting that I will have to create a registry of messages, like you did, then recurse through all the fields creating a set of messages that have been seen. I was planning on using a hashmap from FQMN to the actual message object then in the stage where I generate the definitions I can just emit each entry as a definition. Looking at the go code it solves this by importing 1 layer deep and then using the protoc-go library's output to solve the problem of transitive dependence, but you might have a cool trick up your sleeve so I thought I would ask.

achew22 · 2015-12-28T02:41:44Z

Now I remember why this was hard! Field [https://godoc.org/github.com/gengo/grpc-gateway/protoc-gen-grpc-gateway/descriptor#Field] has a message attached, but it isn't the message I would expect. It is the message of it's parent. This is, no doubt, useful but not in the context that I need it. Lemme go dig in to see if I can figure out how to add a FieldMessage field to the Field struct. Meta-programming is hard!

achew22 · 2015-12-28T03:36:21Z

After a bit of playing, I just plumbed the registry through to the generator and now I'm able to resolve these. Thanks for writing that!

achew22 · 2015-12-28T07:52:55Z

Sorry, I accidentally closed this by pushing your master to github. It's all fixed now.

I think this is getting a lot closer. I have created a non-streaming example file which is the a bit of everything with the word streaming removed https://raw.githubusercontent.com/achew22/grpc-gateway/master/examples/examplepb/streamless_everything.swagger.json

If you want to see how their API viewer sees it, check out http://petstore.swagger.io/#/ and type that URL in at the top. If you want to see what linter checks it is failing you can go to https://editor.swagger.io and do the same thing.

Things that still need to be done:

EmptyProto is still an interesting edge case. Right now it is not being included in output and I haven't figured out why.
Even if empty proto is fixed in the swagger codegen to emit an empty dictionary, their editor will still barf on it Using empty schema object does not work anymore OAI/OpenAPI-Specification#452
Repeated fields need to be marked as arrays
Theoretical support for enums -- I couldn't get enums to be loaded in the params object. What I have should handle them properly.
Parameters should not all be typed as "String". They have type information and we should represent that.
More tests

yugui · 2015-12-28T12:33:49Z

I see. I'm looking forward to seeing it.

My intent is to turn protoc-gen-swagger into a swagger JSON generating protoc plugin.

Previously I had forked the protoc-gen-grpc-gateway folder because I wasn't sure how many changes I was going to have to make to it. Since there weren't very many changes required. I have patched it to add the fields I required and moved gen-swagger to use gen-grpc.

This also adds an error condition that I hadn't previously considered. If there is a streaming API swagger doesn't currently support it. I handle that now with an error.

Also does a major refactor. This changes the way that Messages are discovered and breaks the generation step down from a monoloithic task into a much easier to test set. Also parses paths and adjusts them so that they match what swagger expects.

Swagger uses the operationId as the "nickname" for the method generated for your service.

The tag is used to define the class name in code generation out of Swagger.

yugui · 2016-01-19T23:29:36Z

@achew22
I want to hear your opinion about https://github.com/gengo/grpc-gateway/pull/68/files#r50193073.
Besides the comment and other small coding-level issues, I like the overall direction.

achew22 · 2016-01-25T04:26:08Z

PTAL

yugui · 2016-01-26T07:53:36Z

protoc-gen-swagger/genswagger/generator.go

+		output := fmt.Sprintf("%s.swagger.json", base)
+		files = append(files, &plugin.CodeGeneratorResponse_File{
+			Name:    proto.String(output),
+			Content: proto.String(string(formatted.Bytes())),


Use bytes.Buffer.String() to avoid unnecessary copy.

yugui · 2016-01-26T07:59:19Z

The design looks good to me.
I'll add some coding-level comments.

yugui · 2016-01-26T08:08:25Z

protoc-gen-swagger/genswagger/template.go

+
+	// We now have rendered the entire swagger object. Write the bytes out to a
+	// string so it can be written to disk.
+	w := bytes.NewBuffer(nil)


You don't have to allocate the buffer in heap.

var w bytes.Buffer enc := json.NewEncoder(&w)

Saving memory 👍

achew22 · 2016-01-28T03:46:30Z

Thank you so much for the review. This is my first bout with Golang and I'm happy to have someone reviewing it. I think I have addressed all your concerns and would appreciate if you could take another look at the code.

yugui · 2016-01-28T05:11:57Z

LGTM.

Thank you for your excellent work!

Add swagger support

…ttransactioninfobyid_api add transactioninfobyid api

yugui added the enhancement label Dec 2, 2015

achew22 force-pushed the master branch from ad72c14 to 31abec9 Compare December 20, 2015 07:22

achew22 force-pushed the master branch from 31abec9 to 2123e0d Compare December 20, 2015 07:38

yugui reviewed Dec 24, 2015
View reviewed changes

achew22 closed this Dec 28, 2015

achew22 force-pushed the master branch from 2123e0d to b56786c Compare December 28, 2015 07:28

achew22 reopened this Dec 28, 2015

achew22 added 8 commits January 8, 2016 23:55

Branch off protoc-gen-swagger.

ab2e590

My intent is to turn protoc-gen-swagger into a swagger JSON generating protoc plugin.

Generate definitions for messages

d9cef9e

Remove unnecessary python script

6315abf

Add tests for the template.go file.

a514de5

This also adds an error condition that I hadn't previously considered. If there is a streaming API swagger doesn't currently support it. I handle that now with an error.

Create streamless example service.

5ffe854

Also does a major refactor. This changes the way that Messages are discovered and breaks the generation step down from a monoloithic task into a much easier to test set. Also parses paths and adjusts them so that they match what swagger expects.

Recompile http.proto from third_party

07df08f

Move types into their own file.

fcaae47

achew22 force-pushed the master branch from 524e235 to b575198 Compare January 9, 2016 06:57

achew22 added 3 commits January 13, 2016 23:56

Add operationId to Swagger output.

6e9bc39

Swagger uses the operationId as the "nickname" for the method generated for your service.

Add a tag to each operation.

a929ef6

The tag is used to define the class name in code generation out of Swagger.

Update local copy of protobuf and regenerate

449d3ff

achew22 changed the title ~~Add non-functional swagger support~~ Add swagger support Jan 21, 2016

yugui reviewed Jan 26, 2016
View reviewed changes

Daniel Low and others added 3 commits January 27, 2016 20:42

Add request in error handlers

337605e

Generate simpler names for protos in Swagger schema.

4ce9e97

Respond to comments

2429634

yugui added a commit that referenced this pull request Jan 28, 2016

Merge pull request #68 from achew22/master

a7e1d20

Add swagger support

yugui merged commit a7e1d20 into grpc-ecosystem:master Jan 28, 2016

achew22 mentioned this pull request Jan 28, 2016

JavaScript Proxy #61

Closed

ithinker1991 pushed a commit to tronprotocol/grpc-gateway that referenced this pull request Jun 22, 2018

Merge pull request grpc-ecosystem#68 from tronprotocol/feature/add_ge…

d7bf54e

…ttransactioninfobyid_api add transactioninfobyid api

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add swagger support #68

Add swagger support #68

achew22 commented Dec 2, 2015

yugui commented Dec 2, 2015

yugui commented Dec 6, 2015

achew22 commented Dec 14, 2015

yugui commented Dec 14, 2015

achew22 commented Dec 20, 2015

yugui Dec 24, 2015

yugui commented Dec 24, 2015

achew22 commented Dec 27, 2015

achew22 commented Dec 28, 2015

achew22 commented Dec 28, 2015

achew22 commented Dec 28, 2015

yugui commented Dec 28, 2015

yugui commented Jan 19, 2016

achew22 commented Jan 25, 2016

yugui Jan 26, 2016

achew22 Jan 28, 2016

yugui commented Jan 26, 2016

yugui Jan 26, 2016

achew22 Jan 28, 2016

achew22 commented Jan 28, 2016

yugui commented Jan 28, 2016

Add swagger support #68

Add swagger support #68

Conversation

achew22 commented Dec 2, 2015

yugui commented Dec 2, 2015

yugui commented Dec 6, 2015

achew22 commented Dec 14, 2015

yugui commented Dec 14, 2015

achew22 commented Dec 20, 2015

yugui Dec 24, 2015

Choose a reason for hiding this comment

yugui commented Dec 24, 2015

achew22 commented Dec 27, 2015

achew22 commented Dec 28, 2015

achew22 commented Dec 28, 2015

achew22 commented Dec 28, 2015

yugui commented Dec 28, 2015

yugui commented Jan 19, 2016

achew22 commented Jan 25, 2016

yugui Jan 26, 2016

Choose a reason for hiding this comment

achew22 Jan 28, 2016

Choose a reason for hiding this comment

yugui commented Jan 26, 2016

yugui Jan 26, 2016

Choose a reason for hiding this comment

achew22 Jan 28, 2016

Choose a reason for hiding this comment

achew22 commented Jan 28, 2016

yugui commented Jan 28, 2016