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.

Sign up for GitHub

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

Jump to bottom

implement response check #3892

Merged
merged 8 commits into from
Sep 9, 2019
Merged

implement response check #3892

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
9e2046c
implement response check, #3864
samuelcolvin Jul 6, 2019
821bc4d
switch to raise_for_status coroutine
samuelcolvin Jul 6, 2019
2fec78d
fix linting
samuelcolvin Jul 6, 2019
3f5bce2
Merge branch 'master' into response_check
samuelcolvin Aug 19, 2019
bd207a1
revert name corrections
samuelcolvin Aug 19, 2019
351f7fa
fix types, add request tests
samuelcolvin Aug 19, 2019
0e21a76
change and docs
samuelcolvin Aug 19, 2019
c8eded8
allow non callable truey raise_for_status
samuelcolvin Sep 6, 2019
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
10 changes: 7 additions & 3 deletions aiohttp/client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
from types import SimpleNamespace, TracebackType
from typing import ( # noqa
Any,
Callable,
Coroutine,
Generator,
Generic,
Expand Down Expand Up @@ -187,7 +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,
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,
Expand Down Expand Up @@ -323,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,
Expand Down Expand Up @@ -570,8 +571,11 @@ async def _request(
# check response status
if raise_for_status is None:
raise_for_status = self._raise_for_status
if 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:
Expand Down
35 changes: 29 additions & 6 deletions tests/test_client_functional.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2090,11 +2090,11 @@ async def handler_redirect(request):

async def test_raise_for_status(aiohttp_client) -> None:

async def handler_redirect(request):
async def handle(request):
raise web.HTTPBadRequest()

app = web.Application()
app.router.add_route('GET', '/', handler_redirect)
app.router.add_route('GET', '/', handle)
client = await aiohttp_client(app, raise_for_status=True)

with pytest.raises(aiohttp.ClientResponseError):
Expand All @@ -2103,11 +2103,11 @@ async def handler_redirect(request):

async def test_raise_for_status_per_request(aiohttp_client) -> None:

async def handler_redirect(request):
async def handle(request):
samuelcolvin marked this conversation as resolved.
Show resolved Hide resolved
raise web.HTTPBadRequest()

app = web.Application()
app.router.add_route('GET', '/', handler_redirect)
app.router.add_route('GET', '/', handle)
client = await aiohttp_client(app)

with pytest.raises(aiohttp.ClientResponseError):
Expand All @@ -2116,11 +2116,11 @@ async def handler_redirect(request):

async def test_raise_for_status_disable_per_request(aiohttp_client) -> None:

async def handler_redirect(request):
async def handle(request):
raise web.HTTPBadRequest()

app = web.Application()
app.router.add_route('GET', '/', handler_redirect)
app.router.add_route('GET', '/', handle)
client = await aiohttp_client(app, raise_for_status=True)

resp = await client.get('/', raise_for_status=False)
Expand Down Expand Up @@ -2167,6 +2167,29 @@ async def handler(request):
assert False, "never executed" # pragma: no cover


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)

raise_for_status_called = 0

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, raise_for_status=custom_r4s)
await client.get('/')
assert raise_for_status_called == 1
await client.get('/', raise_for_status=True)
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:
session = aiohttp.ClientSession()
try:
Expand Down