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

genswagger: don't emit default response if there's no Error definition #1166

Merged

Conversation

utrack
Copy link
Contributor

@utrack utrack commented Mar 10, 2020

Swagger: do not create default response definitions if we can't find Error/StreamError in Registry, even if DisableDefaultErrors() is false.

Logs are still emitted if default errors are enabled, but error definition was not found.

grep: .grpc.gateway.runtime.Error, .grpc.gateway.runtime.StreamError

Closes #1162

utrack added 2 commits March 10, 2020 13:29
Swagger: do not create default response definitions if we can't find
Error/StreamError in Registry, even if `DisableDefaultErrors()` is false.

Logs are still emitted if default errors are enabled, but error
definition was not found.

grep: `.grpc.gateway.runtime.Error`, `.grpc.gateway.runtime.StreamError`

Closes grpc-ecosystem#1162
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.

Nice work, this will do nicely. Lets double down on the lookup that can fail, we should never render any look ups that fail.

utrack added 2 commits March 11, 2020 14:14
now tests require properly loaded registry to lookup swagger names and
stuff. :)
@utrack utrack requested a review from johanbrandhorst March 11, 2020 11:26
@utrack
Copy link
Contributor Author

utrack commented Mar 11, 2020

@johanbrandhorst the code looks a bit messier now because of more if branches; however, I don't want to do refactoring in the same PR so the changes would be as small as possible. This calls for a separate PR :)

@johanbrandhorst
Copy link
Collaborator

Would appreciate a further refactor in a separate PR if you have the time :).

@johanbrandhorst johanbrandhorst merged commit d1093df into grpc-ecosystem:master Mar 11, 2020
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.

Swagger definition is broken if there's no default error response set
3 participants