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

implement response check #3892

Merged
merged 8 commits into from
Sep 9, 2019
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
switch to raise_for_status coroutine
  • Loading branch information
samuelcolvin committed Jul 6, 2019
commit 821bc4db8048c9beb28e48cb65cdeffa7b93a6e0
14 changes: 6 additions & 8 deletions aiohttp/client.py
Original file line number Diff line number Diff line change
@@ -170,7 +170,7 @@ class ClientSession:
'_connector_owner', '_default_auth',
'_version', '_json_serialize',
'_requote_redirect_url',
'_timeout', '_raise_for_status', '_response_check', '_auto_decompress',
'_timeout', '_raise_for_status', '_auto_decompress',
'_trust_env', '_default_headers', '_skip_auto_headers',
'_request_class', '_response_class',
'_ws_response_class', '_trace_configs')
@@ -188,8 +188,7 @@ def __init__(self, *, connector: Optional[BaseConnector]=None,
version: HttpVersion=http.HttpVersion11,
cookie_jar: Optional[AbstractCookieJar]=None,
connector_owner: bool=True,
raise_for_status: bool=False,
response_check: Optional[Callable[[ClientResponse, bool], None]] = None, # noqa
raise_for_status: Union[bool, Callable[[ClientResponse], None]]=False, # noqa
samuelcolvin marked this conversation as resolved.
Show resolved Hide resolved
read_timeout: Union[float, object]=sentinel,
conn_timeout: Optional[float]=None,
timeout: Union[object, ClientTimeout]=sentinel,
@@ -257,7 +256,6 @@ def __init__(self, *, connector: Optional[BaseConnector]=None,
"conflict, please setup "
"timeout.connect")
self._raise_for_status = raise_for_status
self._response_check = response_check
self._auto_decompress = auto_decompress
self._trust_env = trust_env
self._requote_redirect_url = requote_redirect_url
@@ -326,7 +324,7 @@ async def _request(
compress: Optional[str]=None,
chunked: Optional[bool]=None,
expect100: bool=False,
raise_for_status: Optional[bool]=None,
raise_for_status: Union[None, bool, Callable[[ClientResponse], None]]=None, # noqa
read_until_eof: bool=True,
proxy: Optional[StrOrURL]=None,
proxy_auth: Optional[BasicAuth]=None,
@@ -574,10 +572,10 @@ async def _request(
if raise_for_status is None:
raise_for_status = self._raise_for_status

if self._response_check is not None:
await self._response_check(resp, raise_for_status)
elif raise_for_status:
if raise_for_status is True:
Copy link
Member

Choose a reason for hiding this comment

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

There is a subtle regression here.
Before a user was allowed to pass 1 / 0 or even an object with __bool__() dunder method implemented.
We discourage this by providing bool type hint but still support.
Now the check works by explicit is True comparison.
I rather suggest reorder checks:

if raise_for_status is None:
    pass
elif callable(raise_for_status):
    await raise_for_status(resp)
elif raise_for_status:  # a shortcut for `bool(raise_for_status)`
    resp.raise_for_status()

What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

resp.raise_for_status()
elif raise_for_status is not False:
await raise_for_status(resp) # type: ignore
samuelcolvin marked this conversation as resolved.
Show resolved Hide resolved

# register connection
if handle is not None:
35 changes: 8 additions & 27 deletions tests/test_client_functional.py
Original file line number Diff line number Diff line change
@@ -2167,46 +2167,27 @@ async def handler(request):
assert False, "never executed" # pragma: no cover


async def test_custom_response_check(aiohttp_client) -> None:
async def test_raise_for_status_coro(aiohttp_client) -> None:
samuelcolvin marked this conversation as resolved.
Show resolved Hide resolved

async def handle(request):
return web.Response(text='ok')

app = web.Application()
app.router.add_route('GET', '/', handle)

response_check_called = False
raise_for_status_called = 0

async def custom_response_check(response, raise_for_status):
nonlocal response_check_called
response_check_called = True
assert raise_for_status is False
async def custom_r4s(response):
nonlocal raise_for_status_called
raise_for_status_called += 1
assert response.status == 200
assert response.request_info.method == 'GET'

client = await aiohttp_client(app, response_check=custom_response_check)
client = await aiohttp_client(app, raise_for_status=custom_r4s)
await client.get('/')
assert response_check_called


async def test_response_check_raise_for_status(aiohttp_client) -> None:

async def handle(request):
return web.Response(text='ok')

app = web.Application()
app.router.add_route('GET', '/', handle)

response_check_called = False

async def custom_response_check(response, raise_for_status):
nonlocal response_check_called
response_check_called = True
assert raise_for_status is True

client = await aiohttp_client(app, response_check=custom_response_check)
assert raise_for_status_called == 1
await client.get('/', raise_for_status=True)
assert response_check_called
assert raise_for_status_called == 1 # custom_r4s not called again
samuelcolvin marked this conversation as resolved.
Show resolved Hide resolved


async def test_invalid_idna() -> None: