Handle JSON decode error when parsing error response in client #35
Conversation
|
Hi, sorry for the delay on this. Thanks for the PR! The core maintainer for this project isn't available right now. I'll review and approve/ask for changes if you're ok with making additional changes. Once he finds some time, he can merge this in and make a new release. |
|
Thank you @ofpiyush for stepping in. If approved, I will be happy to merge. |
There was a problem hiding this comment.
@chadawagner Sorry for the delay again, I finally found some time to review. Thanks for the PR again :)
There's a minor bug in the code, could you make the changes to fix the issue?
twirp/client.py
Outdated
| raise exceptions.TwirpServerException.from_json(resp.json()) | ||
| except: |
There was a problem hiding this comment.
This will always catch the exception. Including exceptions.TwirpServerException
As a result, we will always only get either Unavailable or Unknown
There was a problem hiding this comment.
Oops, I was catching the specific error in my local patch, not sure how I missed adding it to the PR. Fixed it, thanks!
There was a problem hiding this comment.
Thanks it seems to work now!
twirp/client.py
Outdated
| else: | ||
| code = errors.Errors.Unknown | ||
| raise exceptions.TwirpServerException( | ||
| code=code, message=resp.text |
There was a problem hiding this comment.
Would it be worth adding the original status code and headers to the meta dict here? And is it ok to assume that resp.text can be sent as message?
There was a problem hiding this comment.
For context: the problem I ran into was that an intermediate load balancer was returning a 503 response with an HTML body, but all we were seeing was the JSONDecodeError. We were able to diagnose the actual source of the error only after surfacing the status, headers, and body of the actual response.
There was a problem hiding this comment.
Maybe we should send standard text message and meta.status_code and meta.body
https://twitchtv.github.io/twirp/docs/errors.html#http-errors-from-intermediary-proxies
Check this out!
There was a problem hiding this comment.
More generally, we should try to follow the go package as closely as possible.
https://github.com/twitchtv/twirp/blob/main/protoc-gen-twirp/generator.go#L635-L664
There was a problem hiding this comment.
Totally agree, I can add to this PR or would that warrant a new one
There was a problem hiding this comment.
You can add to this one 🙏
There was a problem hiding this comment.
@ofpiyush done!
|
Thank you @chadawagner and @ofpiyush! |
No description provided.