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

Fix inconsistent exception type in response.json() method #5856

Merged
merged 28 commits into from Jul 26, 2021

Conversation

steveberdy
Copy link
Contributor

@steveberdy steveberdy commented Jul 3, 2021

Summary

Fixes the inconsistency of errors thrown in the response.json() method in models.py, while preserving backwards compatibility.

What we used to have

Depending on whether or not simplejson was installed in the user's library, and whether the user was running Python 3+ versus Python 2, a different exception would get thrown when there was an issue with decoding response text as JSON. If simplejson was installed, simplejson.JSONDecodeError would get thrown. If not, json.JSONDecodeError would get thrown for Python 3+ users, and ValueError for Python 2 users. Thus, depending on the scenario, users would find themselves having to catch either simplejson.JSONDecodeError, json.JSONDecodeError, or ValueError in the case of the response.json() method failing. This inconsistency is not ideal.

What we have now

There is now one error class in exceptions.py that will represent all errors that can be thrown or caught in the response.json() method: requests.JSONDecodeError. All simplejson functionality was replaced with json functionality, but the new error type aliases both json.JSONDecodeError and simplejson.JSONDecodeError. If simplejson is not installed, its JSONDecodError is replaced with a plain Exception when aliased. If the user is running Python 3+, json.JSONDecodeError is aliased, but if the user is running Python 2, that is replaced with ValueError when aliased, as the JSONDecodeError was not previously a part of the json library. Now, all five error types, json.JSONDecodeError, simplejson.JSONDecodeError, ValueError, and requests.JSONDecodeError and its parent class requests.RequestException, will be caught from the requests.JSONDecodeError that will be raised.

Fixes #5794
@sigmavirus24

requests/compat.py Outdated Show resolved Hide resolved
requests/exceptions.py Outdated Show resolved Hide resolved
requests/models.py Outdated Show resolved Hide resolved
requests/models.py Outdated Show resolved Hide resolved
requests/models.py Outdated Show resolved Hide resolved
requests/models.py Outdated Show resolved Hide resolved
requests/models.py Outdated Show resolved Hide resolved
requests/models.py Outdated Show resolved Hide resolved
requests/models.py Outdated Show resolved Hide resolved
requests/models.py Outdated Show resolved Hide resolved
requests/models.py Outdated Show resolved Hide resolved
requests/models.py Outdated Show resolved Hide resolved
requests/models.py Outdated Show resolved Hide resolved
requests/models.py Outdated Show resolved Hide resolved
requests/models.py Outdated Show resolved Hide resolved
requests/models.py Outdated Show resolved Hide resolved
Reverted some changes
Added changes for the latest version and mention backwards compatibility
Added latest changes to the changelog
@steveberdy
Copy link
Contributor Author

@sigmavirus24 @nateprewitt I made the necessary edits to fix workflow failures. Any feedback on Python 2 compatibility before running the workflows again? Thanks.

@nateprewitt
Copy link
Member

Hi @steveberdy, I'd recommend setting up a testing environment locally with our tox script and something like pyenv to manage Python versions. If you're having difficulty with that, you can test changes yourself on another branch using Github actions in your own repository. That will help expedite your testing cycle and avoid needing to ping this thread every time a change is made.

@steveberdy
Copy link
Contributor Author

@nateprewitt Thank you for the advice. I ran the workflows, and all tests passed: https://github.com/steveberdy/requests/actions/runs/1023889042

@steveberdy
Copy link
Contributor Author

Is this ready to merge?

requests/models.py Outdated Show resolved Hide resolved
HISTORY.md Outdated Show resolved Hide resolved
HISTORY.md Outdated Show resolved Hide resolved
docs/api.rst Outdated Show resolved Hide resolved
Copy link
Member

@nateprewitt nateprewitt left a comment

Choose a reason for hiding this comment

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

Thanks for the updates, @steveberdy. I added a couple of comments for discussion. I'm not convinced they're necessarily the right direction, but it does feel like the simplejson/json problems are potentially being spread more places than needed.

Let me know what you think, thanks!

requests/compat.py Show resolved Hide resolved
requests/exceptions.py Outdated Show resolved Hide resolved
@sigmavirus24 sigmavirus24 self-requested a review July 20, 2021 22:37
Copy link
Member

@nateprewitt nateprewitt left a comment

Choose a reason for hiding this comment

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

Cool, I think this looks like we’re ready now. Thanks for seeing this through, @steveberdy.

For merging, I’m not sure if we want to put this on a 2.27.x branch while we wait for the next release, or if we merge now knowing we won’t do another patch release for 2.26.x. Any thoughts @sigmavirus24?

@sigmavirus24
Copy link
Contributor

Let's just commit to the next release being 2.27.0. Heck we can push that out whenever and if we can get the proxy stuff merged too (once it's in a good state) then I think we can cut 2.27 with just these two "features"

@nateprewitt
Copy link
Member

Alright, sounds like a plan to me. Thanks again, @steveberdy.

@nateprewitt nateprewitt merged commit db575ee into psf:master Jul 26, 2021
@steveberdy steveberdy deleted the json-opt-2 branch July 26, 2021 16:04
@nateprewitt nateprewitt added this to the 2.27.0 milestone Sep 2, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

response.json() raises inconsistent exception type
3 participants