requests-oauthlib icon indicating copy to clipboard operation
requests-oauthlib copied to clipboard

Raise HTTPError in case of HTTP 5XX responses.

Open ab opened this issue 3 years ago • 7 comments

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 avatar Apr 14 '21 21:04 ab

Coverage Status

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

coveralls avatar Apr 19 '21 13:04 coveralls

Coverage Status

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

coveralls avatar Apr 19 '21 13:04 coveralls

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

ab avatar Apr 19 '21 18:04 ab

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

sigmavirus24 avatar Apr 19 '21 19:04 sigmavirus24

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

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

ab avatar Apr 19 '21 21:04 ab

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

Rjevski avatar May 03 '21 12:05 Rjevski