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

Merging swagger specs fails to use rpc comments (again) #923

Closed
co3k opened this issue Apr 15, 2019 · 5 comments · Fixed by #945
Closed

Merging swagger specs fails to use rpc comments (again) #923

co3k opened this issue Apr 15, 2019 · 5 comments · Fixed by #945

Comments

@co3k
Copy link
Contributor

co3k commented Apr 15, 2019

This is same issue with #664 but the current version of grpc-gateway (it might be v1.6.3 or later) has this issue again.

Steps you follow to reproduce the error:

  1. Create a.proto and b.proto by following Merging swagger specs fails to use rpc comments #664 (comment) instruction
  2. Generate the swagger spec without merging:
protoc -I=. \
        -I=$GOPATH/src/github.com/grpc-ecosystem/grpc-gateway \
        -I=$GOPATH/src/github.com/grpc-ecosystem/grpc-gateway/third_party/googleapis \
        --swagger_out=logtostderr=true:. \
        a.proto b.proto
  1. Generate the swagger spec with merging:
protoc -I=. \
        -I=$GOPATH/src/github.com/grpc-ecosystem/grpc-gateway \
        -I=$GOPATH/src/github.com/grpc-ecosystem/grpc-gateway/third_party/googleapis \
        --swagger_out=logtostderr=true,allow_merge=true:. \
        a.proto b.proto

What did you expect to happen instead:

b.swagger.json has valid summary and description for "ListBars" operation but apidocs.swagger.json doesn't.

What's your theory on why it isn't working:

I've confirmed v1.6.2 generates a valid merged swagger spec. Changes of v1.6.3 might break this feature: v1.6.2...v1.6.3

Oh, I've tried revert 8140d98 and I could get the expected behavior.

@johanbrandhorst
Copy link
Collaborator

Hi @co3k, sorry this has broken again 😞. I'm a little confused, did you establish that it was working without 8140d98 applied?

@co3k
Copy link
Contributor Author

co3k commented Apr 16, 2019

did you establish that it was working without 8140d98 applied?

Yes. I've confirmed "fix" this problem by the following step.

  1. $ git checkout v1.6.3
  2. $ git revert 8140d98
  3. build protoc-gen-swagger
  4. you will confirm "resolved" this problem

Of course we really need the 8140d98 changes but these might have some side effects which we should also fix.

@johanbrandhorst
Copy link
Collaborator

Hm, ideally what we need here is a way to reproduce this with a test case, so we can iterate quickly on a fix and ensure this error doesn't happen again in the future. Would you be interested in trying to fix this? I don't have the time to investigate further.

@prestonvanloon
Copy link
Contributor

Has there been any update on this? It seems that it's still broken.

@johanbrandhorst
Copy link
Collaborator

Not that I know. I think we'll have to revert 8140d98 for now and make another release. I'll get on that.

johanbrandhorst added a commit that referenced this issue Jun 13, 2019
This reverts commit 8140d98. It caused
a regression in the swagger generator allow_merge behaviour.

Fixes #923
johanbrandhorst added a commit that referenced this issue Jun 13, 2019
This reverts commit 8140d98. It caused
a regression in the swagger generator allow_merge behaviour.

Fixes #923
johanbrandhorst added a commit that referenced this issue Jun 13, 2019
This reverts the changes introduced in #833. It caused
a regression in the swagger generator allow_merge behaviour.

Fixes #923
johanbrandhorst added a commit that referenced this issue Jun 13, 2019
This reverts the changes introduced in #833. It caused
a regression in the swagger generator allow_merge behaviour.

Fixes #923
adasari pushed a commit to adasari/grpc-gateway that referenced this issue Apr 9, 2020
This reverts the changes introduced in grpc-ecosystem#833. It caused
a regression in the swagger generator allow_merge behaviour.

Fixes grpc-ecosystem#923
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 a pull request may close this issue.

3 participants