[enhancement] ability to return redirect responses which don't have 'location' header

[thehesiod]

based on a recent aiobotocore investigation: aio-libs/aiobotocore#267 it seems that the requests module treats responses which don't have the 'location' header as a regular response and not an exception...however aiohttp ends up throwing a RuntimeError/ClientRedirectError: https://github.com/aio-libs/aiohttp/blob/master/aiohttp/client.py#L278

AWS occasionally stores the location in the body. Since aiohttp raises an exception we no longer have access to the response to generate the redirect.

We are then left with having to disable redirects and handle redirects on our own. This would mean re-implementing max_redirects and redirect resolution on our own...so instead I propose adding a flag which allows aiohttp to behave like requests. If this is approved I can work on a PR. Thanks!

[asvetlov]

Hmm. But it looks like buggy server support.
Location in BODY? How it looks like?

[kxepal]

I guesss the devil in the details. HTTP RFC uses SHOULD word about Location header for redirection status codes, so it's not required one. And the user MAY follow it or may not.

So while it's awkward, it seems to be legit. Dont' we have some follow_redirect flag to control that behaviour?

[asvetlov]

Please show example of such response. I cannot figure out how to process something which is not specified at all

[thehesiod]

Oh I don't need aiohttp to process the response, Just return response if Location header not available. I'll give u an example with requests later today.

[thehesiod]

here's an example in difference of behavior:

import asyncio
import aiohttp.web
import requests

async def redirect_handler(request):
    response = aiohttp.web.Response(status=301, body=b'one', headers={'Location': '/redirect2'})
    return response


async def redirect_handler2(request):
    response = aiohttp.web.Response(status=301, body=b'two')
    return response


async def build_server():
    loop = asyncio.get_event_loop()
    app = aiohttp.web.Application()
    app.router.add_route('GET', '/redirect', redirect_handler)
    app.router.add_route('GET', '/redirect2', redirect_handler2)
    return await loop.create_server(app.make_handler(), 'localhost', 8080)


async def main():
    loop = asyncio.get_event_loop()
    asyncio.ensure_future(build_server())

    await asyncio.sleep(3)

    # this will not raise
    resp = await loop.run_in_executor(None, requests.get, 'http://localhost:8080/redirect')

    assert resp.content == b'two'

    # this will raise
    async with aiohttp.ClientSession(read_timeout=None) as session:
        response = await session.get('http://localhost:8080/redirect')
        body = await response.read()
        assert body == b'two'

    print()

if __name__ == '__main__':
    _loop = asyncio.get_event_loop()
    _loop.run_until_complete(main())

if the redirect response gets returned we can then forward it to botocore to handle the redirect as it does with requests.

[thehesiod]

ok updated example to show it will follow redirects until it reaches one that doesn't have a Location

[asvetlov]

Aah, got it.
Yep, perhaps we should just return this response instead of raising exception.
That's sad because in master we added a special exception type for it :( (#2009)

[thehesiod]

ya I saw...another option is maybe adding the response to that exception?

[asvetlov]

Yes, it's possible. But I don't sure what is average user expectation: get response with 301 status code but payload as for regular 200 or receive an exception with response inside it?
For me second is stricter and more attractive. It still allows to work with buggy servers but explicitly points that the server does something discouraged.

[thehesiod]

looks like apache had the same issue and they reverted throwing the exception: https://issues.apache.org/jira/browse/HTTPCLIENT-939 so it sounds like aiohttp should behave like requests + apache httpclient.

[asvetlov]

Good finding

[asvetlov]

Deal!
@thehesiod please make a PR

[lock]

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.