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

Raise HTTPError in case of HTTP 5XX responses. #441

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

ab
Copy link

@ab ab commented Apr 14, 2021

It is very confusing to raise a MissingTokenError when the server has
returned an HTTP server error.

Instead, raise requests.exceptions.HTTPError if the server has returned
an HTTP 5XX server error.

Prior PR conversation in PR #217 indicates that the maintainers do not
want to raise on 4XX errors due to certain providers using those
responses to send data. So we need a custom handler with a slight
variation on the built-in requests.models.Response.raise_for_status().

ab added 2 commits April 14, 2021 17:31
It is misleading to raise a MissingTokenError when the server has
returned an HTTP server error.

Instead, raise requests.exceptions.HTTPError if the server has returned
an HTTP 5XX server error.

Prior PR conversation in PR requests#217 indicates that the maintainers do not
want to raise on 4XX errors due to certain providers using those
responses to send data. So we need a custom handler with a slight
variation on the built-in requests.models.Response.raise_for_status().
@coveralls
Copy link

Coverage Status

Coverage decreased (-1.3%) to 88.911% when pulling bb90460 on ab:raise-5xx into 46f886c on requests:master.

@coveralls
Copy link

coveralls commented Apr 19, 2021

Coverage Status

Coverage increased (+0.1%) to 90.297% when pulling fa78265 on ab:raise-5xx into 46f886c on requests:master.

@ab ab marked this pull request as ready for review April 19, 2021 18:37
@ab
Copy link
Author

ab commented Apr 19, 2021

@jtroussard @Lukasa @sigmavirus24 @singingwolfboy

This could be considered a revised PR for #217.

I'm pretty sure tests for pypy3.5-6.0 are failing on master. The error regarding Python cryptography and OpenSSL 1.0.2 doesn't seem related to my code changes at all.

Tests pass on all other Python versions.

@sigmavirus24
Copy link
Contributor

Please don't randomly ping people who don't work on the project

@ab
Copy link
Author

ab commented Apr 19, 2021

Please don't randomly ping people who don't work on the project

Sorry about that, you had weighed in on the linked PR.

@Rjevski
Copy link

Rjevski commented May 3, 2021

+1 for this - I'm seeing confusing exceptions when the upstream provider returns a generic error response (Nginx's bad gateway error page for example) so I would recommend merging this too.

The tests are failing for an unrelated error and would most likely fail on master too. Depending on how the project wants to fix this we could either upgrade to a newer OpenSSL version which the cryptography module now requires or set the CRYPTOGRAPHY_ALLOW_OPENSSL_102 environment variable to skip the warning and try using the outdated OpenSSL anyway.

Comment on lines +559 to +569
if isinstance(response.reason, bytes):
# We attempt to decode utf-8 first because some servers
# choose to localize their reason strings. If the string
# isn't utf-8, we fall back to iso-8859-1 for all other
# encodings. (See psf/requests PR #3538)
try:
reason = response.reason.decode("utf-8")
except UnicodeDecodeError:
reason = response.reason.decode("iso-8859-1")
else:
reason = response.reason
Copy link

Choose a reason for hiding this comment

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

You seem to only need the reason in the elif 500 <= response.status_code < 600 block below, so maybe worth moving it in there? That way the behavior doesn't change for any other response codes (at the moment you try decode the reason even if ultimately you might not need it, thus introducing potential risk of breakage for no reason.

Copy link
Author

Choose a reason for hiding this comment

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

This code is from requests.Response.raise_for_status(), and I chose to make as few changes as possible since the only desired difference in behavior is to ignore 4XX errors.

Comment on lines +581 to +582
if http_error_msg:
raise HTTPError(http_error_msg, response=response)
Copy link

Choose a reason for hiding this comment

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

You only define http_error_msg in the elif 500 <= response.status_code < 600 block above, so maybe worth moving it up there (and then not explicitly checking for it nor assigning it to a variable and raising directly)?

@deathiop
Copy link

Is this PR still being considered? I came across the same need and would love for this feature to move forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants