-
Notifications
You must be signed in to change notification settings - Fork 27
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
fix: detect JSON unmarshal errors in responses #157
Conversation
This commit fixes BaseService.send() so that it will now detect a JSON parsing error when processing a success (2xx) response. If, in fact, a JSON parsing error occurs while trying to process a JSON response body, then a requests.exceptions.JSONDecodeError is raised and propagated back to the caller. Signed-off-by: Phil Adams <phil_adams@us.ibm.com>
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.
Looks good to me 👍
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.
LGTM!
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.
@padamstx Thanks, the change looks ok to me though I have one thought/suggestion around the exception itself. We are testing with our SDK this week, I'll report back when we've done that.
Signed-off-by: Phil Adams <phil_adams@us.ibm.com>
I pushed a commit that implements @ricellis's suggestion to throw an ApiException with the JSONDecodeError as the "caused by". |
Note: I'm going to hold off merging this until @ricellis's team has a chance to test with it. |
@padamstx For error responses, |
Yes, I considered this but decided against it because we're already in the process of building an ApiException to reflect an actual error that was returned by the server (e.g. 400 Bad Request), so I didn't think the resulting JSONDecodeError should take precedence over that. |
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.
+1 all set with testing
## [3.16.3](v3.16.2...v3.16.3) (2023-03-22) ### Bug Fixes * detect JSON unmarshal errors in responses ([#157](#157)) ([30d7c52](30d7c52))
🎉 This PR is included in version 3.16.3 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
This commit fixes BaseService.send() so that it
will now detect a JSON parsing error when processing a success (2xx) response.
If, in fact, a JSON parsing error occurs while
trying to process a JSON response body,
then a requests.exceptions.JSONDecodeError
is raised and propagated back to the caller.