Aiohttp: Parser.parse doesn't work after request body was read.

[ku3o]

When a content of a request is read before parser.parse is called (with location=('json',)), parser returns that JSON fields are missing in the request.

Example code:

async def handler(request):
    data = await request.json()
    args = await parser.parse(handler_args, req=request, locations=('json',))
    return web.Response()

The issue is the following line of code https://github.com/sloria/webargs/blob/dev/webargs/aiohttpparser.py#L92 specifically req.has_body. This attribute has an unfortunate name and it's not constant over a lifetime of a request. It checks if there are more bytes to read. Once the body is read by await request.json() there are no more bytes to read and value of req.has_body is False.

[sloria]

Good catch. Would you like to send a PR?

[ku3o]

Yeah. I will try to create a PR :). But this issue is sort of issue of aiohttp package as well, as has_body should be actually consistent over a lifetime of a request object. Therefore I also created an issue in aiohttp aio-libs/aiohttp#2005 . So not sure where to focus first. Either to fix it here or fix it there :D

[sloria]

Thanks! Good call on reporting that issue on aiohttp; looks like the maintainers are open to changing the behavior or adding a separate method.

Since webargs will need to support the current releases of aiohttp, it will need to be fixed in webargs, so a PR would be greatly appreciated.

[lafrech]

I just read the discussions in aio-libs/aiohttp#2005.

.has_body was a misleading name. From aiohttp 2.3.0 on, It has been deprecated in aio-libs/aiohttp#2169, and replaced with can_read_body and body_exists.

IIUC, we should change has_body into body_exists. This will break compatibility with aiohttp < 2.3.0 but anyway, this was already sort of broken (hence this issue).

This would fix both this and #186.

@ariddell, @ku3o, would you like to try that?

There might be another issue, though. In https://github.com/sloria/webargs/blob/dev/dev-requirements-py3.txt, it says:

# webtest-aiohttp doesn't yet support aiohttp>=2.0.0
aiohttp>=1.0.0,<2.0.0
webtest-aiohttp==1.1.1

So webtest-aiohttp might require an update.

[sloria]

Yeah, we're pinning to aiohttp<2 because of webtest-aiohttp. I haven't yet found a way to make webtest-aiohttp compatible with newer versions. I'm considering just rewriting the aiohttpparser's tests to use aiohttp's built-in testing client.

[sloria]

Actually, I was able to get webtest-aiohttp working with aiohttp 2, so we can finally move forward with getting the aiohttpparser up to date. I think it will be pretty straightforward.

Question is: is dropping support for aiohttp 1 considered a breaking change? aiohttp isn't a hard dependency of webargs, but breakages may occur for users of the aiohttpparser module, so I'm thinking we bump to webargs 2.0.0. Thoughts @lafrech @ku3o ?

[lafrech]

The question could be how long you'd be maintaining webargs 1.x.

A compromise would be bump to 2.0.0 to be on the safe side, but deprecate 1.x right away to urge people to upgrade. This is acceptable as the upgrade would be easy. From your perspective, it is pretty much the same as releasing this as a minor change, except it is more explicit about the breaking change;

[ku3o]

The release of aiohttp==2.0.0 was on 2017-03-20. To me, it seems to be enough time and my guess is that the number of people using 1.x.x is very low right now and it will get lower and lower.

I think bumping version to 2.0.0 is a reasonable step in order to keep Webargs compatible with the latest releases of packages where Webargs is used.

[sloria]

OK, thank you both for your input.

@lafrech I'm not sure if this is the same as what you're suggesting, but I think we'll go with the following path:

• Release webargs 1.10.0, which deprecates aiohttp 1 support (and adds marshmallow 3 support).
• Release webargs 2.0.0, which drops aiohttp 1 support completely.

[lafrech]

Yes, this is what I meant.

What I also tried to say is that moving to 2.0.0 should be straightforward for the clients, so it is fair to abandon 1.x branch right after 1.10.0 and 2.0.0 are released.

There is no point in maintaining both branches in parallel, as there shouldn't be users stuck to 1.x.

Maybe this means adding a deprecation warning on the 1.x branch to tell people to move to 2.x.

[sloria]

@lafrech. I agree. Ok, I'll release those when I'm off the clock.

[sloria]

OK, both 1.10.0 and 2.0.0 are released.

@ku3o Is this issue still unsolved?

[ku3o]

I think it solved @sloria

[sloria]

Great!