Memorising of web.Request's read, text, json

[mind1m]

No description provided.

[asvetlov]

+1
I've found very annoying that the next request.json() call return not the same as former.
Also I think it's safe to keep cached values: when user want to do with streaming he never call the methods for reading the whole request body.

But, please add tests to introduce new behavior explicitly: read .json() twice with comparing results and so on.

[asvetlov]

Also .json() accepts loader parameter.
.text() doesnt have any but perhaps we would to add encoding.

The simplest solution to keep only binary data for now (but if we would to be really smart we may make second layer cache for .json() and .text()).

@fafhrd91 what do you like?

[mind1m]

I have removed internal caching for json and text, since these depend on functions' params (loader, charset). Byte cache seems to be enough for me, while two level cache seems redundant.

@asvetlov I have added checks for double calling in simple tests, or it is better to make separate tests?

[fafhrd91]

-0.5

if your code requires to read same request several times, that is clear indication of architectural problems. i'd raise RuntimeError on second read.

[kxepal]

@fafhrd91 the same behavior is true for ClientResponse - you can yield from resp.read() many times and get the same data. Shouldn't it be in sync with Requestsread` behavior?

[mind1m]

@fafhrd91 an example - I have REST with some auth key. In my handlers I do smth with request's json, but also I have a decorator that checks auth key from request's json. I want decorator and handler function to be independent.

However, in this case header will do it's job better.

[kxepal]

@mind1master [bikeshedding] if you have a REST with some auth key, it should be defined in http headers which are already always available without need to read request payload. And you shouldn't be affected by this issue. [/bikeshedding]

[asvetlov]

@kxepal probably you don't get @mind1master reason (while the reason is not fully correct).

Let me introduce a realistic scenario.

I have REST validators for incoming request. They are use trafaret library for whole JSON body.
I may implement those as decorator for web-handler or as direct call like:

def handler(request):
    yield from validate(request, trafaret)
    json = yield from request.json()
    data = process_json(json)

Moreover, I like to have generic pagination for incoming request. That can be done by make_pagination_params(request) function which returns LIMIT and OFFSET in notation good to be passed into aiopg calls for example.

Anyway, I like to have aiohttp client and web API as close as possible.

Client caches incoming stream into response._content attr on .read() call but allows to use response.content if you don't want to cache (e.g. for process downloading huge files etc.)

web.Request has almost the same: .read method (non-caching now but we can change it easy) and .payload property for reading payload without the cache.

Perhaps we need to add web.Request.content to mimic client response one with deprecation warning on web.Request.payload usage.

[asvetlov]

@fafhrd91 did my the last message change your -0.5 opinion?

[fafhrd91]

Yes. Make change

On Monday, January 19, 2015, Andrew Svetlov notifications@github.com
wrote:

@fafhrd91 https://github.com/fafhrd91 did my the last message change
your -0.5 opinion?

—
Reply to this email directly or view it on GitHub
#250 (comment).

[asvetlov]

@mind1master please add web.Request.content property with deprecation warning to Request.payload.

After the commit (with tests for deprecations and docs update) I'll merge the PR.

Thank you, Тоха.

[kxepal]

@asvetlov no, I understand the reasons and +1 for this change. Just @mind1master example was not the best one (;

[mind1m]

@kxepal agreed, I got it after I posted it :)

[asvetlov]

Thanks

[fafhrd91]

add this change to CHANGES

[asvetlov]

Done. @fafhrd91 would you add docs for chunking support for web.Response? I've done it for compression only. Please don't forget ..versionadded: 0.14 directive.

[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.