-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
protoc-gen-swagger: add example similar to AIP-133 #1514
protoc-gen-swagger: add example similar to AIP-133 #1514
Conversation
9b3a0d5
to
256d1bf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, thanks for your interest in the project! If I understand correctly this is meant to exhibit a bug, but I'm not sure I understand where it is. Regardless, it'd be great to have the fix included too in the same PR. Thanks!
@johanbrandhorst Thanks. Hopefully my inline comment reply has helped clear up what the actual issue is. Apologies for not being clearer originally. Generally speaking, the main issue is that for requests with a body, non-body fields need to be added as query parameters. I think this is the same conclusion that @wotzhs came to in their comment at #559 (comment). If that does not sound like the correct behaviour please let me know. Happy to also push forward a fix in this PR. |
Perfect, thanks for the clarification. I'd love to see this fixed in the swagger generator. Note that we're very close to tagging a stable v2, and this issue is reported on v1, can I ask that you test v2 as well? I don't think the logic has changed much in this area so I don't expect it to be working, but I'd rather see this fixed on v2 first and backported than the other way around if there are differences that make it hard to do it for both. |
@johanbrandhorst sure, can do. If the issue exists in v2 I'll open a PR against that and go from there. Note: heading into the weekend here so you'll likely not hear from me until next week. |
Have a good weekend 😄. If you want more synchronous communication to discuss the changes, we have a support channel on the Gophers Slack (#grpc-gateway), join via https://invite.slack.golangbridge.org/. |
256d1bf
to
f140220
Compare
This example illustrates that the `book_id` field is not converted into a query string parameter in the generated swagger.json file.
f140220
to
24c2cf0
Compare
@johanbrandhorst this is ready for review now. |
Thanks for your contribution! |
This example illustrates that the
book_id
field is not converted into a query string parameter in the generated swagger.json file.References to other Issues or PRs
This PR contains an example required for a fix for #559.
Have you read the Contributing Guidelines?
Yes.
Brief description of what is fixed or changed
I added an example to
examples/internal/proto/examplepb/a_bit_of_everything.proto
that is very similar to the example in https://google.aip.dev/133#user-specified-ids that allows users to provide their own ID. The user-specified ID needs to become a query string parameter per aip-dev/google.aip.dev#542 (comment).The gateway generator is fine, but the Swagger generator does not generate query string parameters when there is a body on the binding.
Other comments
I started to fix this locally and I have a rough idea of what needs doing, but I wanted to start the PR with a broken example first and was hoping that I could have some support if I have any questions along the way!
BTW if you are happy with including a broken example feel free to merge this PR. I'm happy to provide a subsequent PR that fixes the issue. Not sure if that's a better way to tackle the issue rather than a massive PR?
Thanks for reading :)