making `body` an attribute of the request object; maybe fixes #445

This occurred to me as a potential workaround for issues with pre-request hooks
being unable to alter `body` within their scope. Particularly those found in
oauth-requests, where the body of a request cannot be recalculated. Here the
issue seems to be that `body` is assigned as a local variable. So if we simply
make `body` an attribute of the object we can manually override its value in
the scope of the hook. Here is a simple example of what this might look like:
`request.data, request._enc_data = request._encode_params(my_params)` this sets
us up for `request.body = request._enc_data` and we can now alter body as we
should like.


This is an interesting alternative. I believe requests should do the calculation of these things based on the manipulation of some of the request parameters, but this patch is easier to merge without conflicts in other features, like stream uploads.

I think there is one issue with:

``` python
request.data, request._enc_data = request._encode_params(my_params)
request.body = request._enc_data
```

Remember that the `request.body` is not only `data`, but also `files` merged in. The problem with this, would be that I will have to replicate some of the encoding in the hook.

The perfect fit would be that this pull request added a `_encode_body` static method similar to the one I proposed in #445, but expecting external parameters. Beware that `Content-Type` should be editable too, because recalculating multipart requests, gets new Content-Types.

Cheers,
Miguel


The benefit here is that you can calculate `request.body` however you like, up to and including `files`. Additionally the `Content-Type` header is easily overridden as is in the scope of hook, e.g.: `request.headers['Content-Type'] = 'application/x-www-form-urlencoded'`.


You are right Max, I forgot `Content-Type` can be edited through headers.

I agree with you, this gives all the choices to the hook's author. I think adding a body encoding method to requests, would be helpful, to avoid replicating functionality that I don't think should be done by the hook.

Good work!


I agree completely with you Miguel: in the long run it would be nice to see the hook logic handle this internally so that pre-request hooks can be less hacky, e.g. by not having to call `request._encode_params`.

Anyway looking forward to more feedback on this idea. :cake:


Yes. Just to make sure we've understood each other. The hook should call something like `request._encode_body(data, files)`. `request._encode_params`, doesn't take `files` into account. That's why I was proposing this method.


Right, it doesn't, however in the scope of the hook you could use whatever method you like to generate the body. So if you wanted you could create a `request._encode_body` method and use it to generate the body as you liked. The primary thrust of this patch is to liberate the body variable by making it an attribute of the request without disrupting the existing infrastructure too much.


Ok, then we are on the same track :)


I've been thinking a lot lately about `Request` keeping a reference to the original value of every parameter as well as the calculated version.

`Request._url` = value of `url` param
`Request._params` = value of `params` param
`Request.url` = url + parms


Thoughts?


I would prefer a naming like Werkzeug or other WSGI frameworks use, `args` for GET parameters, `form` for POST parameters, `url` for the whole url and `base_url` for the url without the query string.

So a generig `args` or `params` plus `url` and `base_url` could be fine, I do not see a reason why both should be hidden, they can easily be public.


Requests already has a well thought-out naming system for those things. No need to change it for this.


Keeping references seems like a good idea to me. 


Do you think making `body` an attribute for the time being is unobtrusive enough to merge?


I'd prefer `_data` and `data`


Sure. So are suggesting we rename `body` to `data`? (The problem here seems to be related to the fact that `body` is set in the scope of the method and then cannot be reset in the scope of the hook.)


I'd be happy to fix up this commit to conform with your ideas for this Kenneth, but I guess I'm still unclear how keeping original values around is going to address the fact that `body` isn't currently an attribute of `Request` and thus is unmodifiable in the scope of a pre-request hook, which seems to be at the heart of the bug referenced.


@maxcountryman right now, original/modified values are all mixed together. Its unclear. `body` needs to be called `content`. 


And could we make it an attribute of the request object? I can take a closer look at this if you'd like.


Correct. 


Not sure if this is what you had in mind or not... :)


How about

`Request.original.url` = value of `url` param
`Request.original.params` = value of `params` param

instead of

`Request._url` = value of `url` param
`Request._params` = value of `params` param


`original` could be a dictionary.


`content_type` as previously discussed, can already be overridden.

Can I make a suggestion that the naming convention changes be moved to another commit? I don't think they necessarily relate to #445.


> `original` could be a dictionary.

Sure, this is what I had in mind.


@maxcountryman 

I'm talking about passing the original value of `content_type` to hooks not the ability to modify it. Passing content and not passing content type feels wrong.


@piotr-dobrogost even so I think it's beyond the scope of this particular bug, no? Easy enough to add but, it just seems like this patch is starting to grow past the original issue... Maybe we should break the two objectives off into separate commits?


This patch is also needed by Rauth: https://github.com/litl/rauth/issues/6


We don't want to add something that will need to be changed later. 

Please be patient :)


This shouldn't be an issue anymore.