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

ClientResponse.raise_for_status() raises an AssertionError for empty reason #3532

Closed
rhwlo opened this issue Jan 13, 2019 · 2 comments · Fixed by #3533
Closed

ClientResponse.raise_for_status() raises an AssertionError for empty reason #3532

rhwlo opened this issue Jan 13, 2019 · 2 comments · Fixed by #3533

Comments

@rhwlo
Copy link
Contributor

@rhwlo rhwlo commented Jan 13, 2019

Long story short

ClientResponse.raise_for_status() raises an AssertionError for a >= 400 status code if its reason is an empty string.

Expected behaviour

I expect ClientResponse.raise_for_status() to raise a ClientResponseError if it’s invoked on a ClientResponse with a status >= 400, and to either:

  • raise a ClientError (or some subclass of ClientError) if it’s called on a ClientResponse with a reason = None, regardless of the HTTP status code, if this can only happen when raise_for_status() has been called before start() has been called; or
  • convert a ClientResponse.reason of None into "" if None is a valid value for ClientResponse.reason

Actual behaviour

  • ClientResponse.raise_for_status() raises an AssertionError if it’s invoked on a ClientResponse when a status >= 400 and bool(reason) is False, which includes reason == ""
  • ClientResponse.raise_for_status() raises a ClientResponseError if it’s invoked on a ClientResponse with a status >= 400 if the reason is not falsey
  • ClientResponse.raise_for_status() raises no error if it’s invoked on a ClientResponse with a status < 400 and a reason = None

Steps to reproduce

A colleague and I uncovered this using aioresponses, which currently doesn’t set a reason on the ClientResponse at all – see pnuckowski/aioresponses#114.

You can reproduce this with the example client and server in this gist: https://gist.github.com/rhwlo/5ff3cb6bae8deb6abb4a1e527ad9583e

Your environment

I’m using the latest aiohttp from the repository, but this problem shows up only in v3.5.0 and newer.

@aio-libs-bot
Copy link
Collaborator

@aio-libs-bot aio-libs-bot commented Jan 13, 2019

GitMate.io thinks the contributor most likely able to help you is @asvetlov.

Possibly related issues are #1177 (Static handler raises AssertionError), #156 (ClientResponse.content.read() raises ConnectionResetError), #1670 (ClientResponse ergonomics), #3248 (history tuple on ClientResponseError empty with raise_for_status() or ClientSession(raise_for_status=True)), and #1633 (Testing with pytest-aiohttp test_client raises an AssertionError.).

@lock
Copy link

@lock lock bot commented Jan 14, 2020

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.

@lock lock bot added the outdated label Jan 14, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Jan 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants