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

protoc-gen-swagger: add example similar to AIP-133 #1514

Merged

Conversation

jonathaningram
Copy link
Contributor

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 :)

@jonathaningram jonathaningram force-pushed the post-query-params branch 2 times, most recently from 9b3a0d5 to 256d1bf Compare July 10, 2020 03:37
Copy link
Collaborator

@johanbrandhorst johanbrandhorst left a 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!

@jonathaningram
Copy link
Contributor Author

jonathaningram commented Jul 10, 2020

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.

@johanbrandhorst
Copy link
Collaborator

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.

@jonathaningram
Copy link
Contributor Author

@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.

@johanbrandhorst
Copy link
Collaborator

@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/.

This example illustrates that the `book_id` field is not converted into
a query string parameter in the generated swagger.json file.
@jonathaningram
Copy link
Contributor Author

@johanbrandhorst this is ready for review now.

@johanbrandhorst johanbrandhorst merged commit cbf52d3 into grpc-ecosystem:master Jul 21, 2020
@johanbrandhorst
Copy link
Collaborator

Thanks for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants