search

requests / requests-oauthlib

OAuthlib support for Python-Requests!
https://requests-oauthlib.readthedocs.org/
ISC License
1.71k stars 422 forks source link

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

Open ab opened 3 years ago

ab commented 3 years ago

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().

coveralls commented 3 years ago

Coverage Status

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

coveralls commented 3 years ago

Coverage Status

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

ab commented 3 years ago

@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 commented 3 years ago

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

ab commented 3 years ago

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 commented 3 years ago

+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.

deathiop commented 2 months ago

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