Raise HTTPError in case of HTTP 5XX responses. #441
Conversation
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().
|
@jtroussard @Lukasa @sigmavirus24 @singingwolfboy This could be considered a revised PR for #217. I'm pretty sure tests for Tests pass on all other Python versions. |
|
Please don't randomly ping people who don't work on the project |
Sorry about that, you had weighed in on the linked PR. |
|
+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 |
| 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| if http_error_msg: | ||
| raise HTTPError(http_error_msg, response=response) |
There was a problem hiding this comment.
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)?
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().