Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add parser and swagger support for enum, no gengateway yet #108

Merged
merged 3 commits into from
Feb 16, 2016

Conversation

t-yuki
Copy link
Contributor

@t-yuki t-yuki commented Feb 10, 2016

This change adds enum feature to parser and gen-swagger.
But no gen-grpc-gateway/gengateway yet.

One of problems is enum json encoding.
proto3 spec says enum should be encoded using string name.
However, since golang/protobuf generated codes have no MarshalJSON/UnmarshalJSON methods, it is encoded as a int, even if proto.Unmarshal/MarshalJSONEnum methods exist.
So json.Unmarshal on request will cause error if string name value is provided.

Note that Swagger support in this change uses string name as spec.

Refs: https://developers.google.com/protocol-buffers/docs/proto3#json

@yugui
Copy link
Member

yugui commented Feb 15, 2016

Almost LGTM.

  • Could you re-generate .pb.go files with the latest protoc-gen-go because the test is failing due to the difference?

@t-yuki
Copy link
Contributor Author

t-yuki commented Feb 15, 2016

Oops, I didn't make realclean after update.
It's done.

@t-yuki
Copy link
Contributor Author

t-yuki commented Feb 15, 2016

Currently json Marshal/Unmarshal of enum string for gengateway requires jsonpb.
Actually proto2 files may work with enum string because protoc-gen-go generates if it is not proto3 (why??): https://github.com/golang/protobuf/blob/master/protoc-gen-go/generator/generator.go#L1407

@yugui
Copy link
Member

yugui commented Feb 16, 2016

LGTM

yugui added a commit that referenced this pull request Feb 16, 2016
Add parser and swagger support for enum, no gengateway yet
@yugui yugui merged commit 4ce23d2 into grpc-ecosystem:master Feb 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants