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.

Sign up for GitHub

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

Jump to bottom

fix: detect JSON unmarshal errors in responses #157

Merged
merged 2 commits into from
Mar 22, 2023
Merged

fix: detect JSON unmarshal errors in responses #157

merged 2 commits into from
Mar 22, 2023

Conversation

padamstx
Copy link
Member

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.

@padamstx
fix: detect JSON unmarshal errors in responses
19e22e0 
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 <[email protected]>
@padamstx padamstx requested a review from pyrooka March 14, 2023 20:38
@padamstx padamstx self-assigned this Mar 14, 2023
dpopp07
dpopp07 approved these changes Mar 14, 2023
View reviewed changes
Copy link
Member

@dpopp07 dpopp07 left a 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 👍

pyrooka
pyrooka approved these changes Mar 14, 2023
View reviewed changes
Copy link
Member

@pyrooka pyrooka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

ricellis
ricellis reviewed Mar 20, 2023
View reviewed changes
Copy link
Member

@ricellis ricellis left a 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.

ibm_cloud_sdk_core/base_service.py Outdated Show resolved Hide resolved
emlaver
emlaver reviewed Mar 21, 2023
View reviewed changes
ibm_cloud_sdk_core/base_service.py Outdated Show resolved Hide resolved
ibm_cloud_sdk_core/utils.py Outdated Show resolved Hide resolved
@padamstx
Copy link
Member Author

padamstx commented Mar 21, 2023 •
edited
Loading

I pushed a commit that implements @ricellis's suggestion to throw an ApiException with the JSONDecodeError as the "caused by".

@padamstx
Copy link
Member Author

Note: I'm going to hold off merging this until @ricellis's team has a chance to test with it.

@emlaver
Copy link
Contributor

emlaver commented Mar 22, 2023

@padamstx For error responses, ApiException tries to parse the response as JSON and if there's an exception during parsing it'll return response.text: https://github.com/IBM/python-sdk-core/blob/main/ibm_cloud_sdk_core/api_exception.py#L75
I'm wondering if it's worth updating this logic to be similar to what you've done for handling malformed JSON in success responses.

@padamstx
Copy link
Member Author

@padamstx For error responses, ApiException tries to parse the response as JSON and if there's an exception during parsing it'll return response.text: https://github.com/IBM/python-sdk-core/blob/main/ibm_cloud_sdk_core/api_exception.py#L75 I'm wondering if it's worth updating this logic to be similar to what you've done for handling malformed JSON in success 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.

emlaver
emlaver approved these changes Mar 22, 2023
View reviewed changes
Copy link
Contributor

@emlaver emlaver left a 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

@padamstx padamstx merged commit 30d7c52 into main Mar 22, 2023
@padamstx padamstx deleted the json-response-errors branch March 22, 2023 18:36
ibm-devx-sdk pushed a commit that referenced this pull request Mar 22, 2023
@semantic-release-bot
chore(release): 3.16.3 release notes
4e7de78 
## [3.16.3](v3.16.2...v3.16.3) (2023-03-22)

### Bug Fixes

* detect JSON unmarshal errors in responses ([#157](#157)) ([30d7c52](30d7c52))
@ibm-devx-sdk
Copy link

🎉 This PR is included in version 3.16.3 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ricellis ricellis ricellis left review comments

@dpopp07 dpopp07 dpopp07 approved these changes

@emlaver emlaver emlaver approved these changes

@pyrooka pyrooka pyrooka approved these changes

Assignees

@padamstx padamstx

Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@padamstx @emlaver @ibm-devx-sdk @dpopp07 @ricellis @pyrooka