Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

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

Closed
wants to merge 1 commit into from

5 participants

@maxcountryman

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.

@maraujop

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:

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

@maxcountryman

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

@maraujop

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!

@maxcountryman

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:

@maraujop

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.

@maxcountryman

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.

@maraujop

Ok, then we are on the same track :)

@kennethreitz

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

@kennethreitz

Thoughts?

@EnTeQuAk

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.

@kennethreitz

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

@maxcountryman

Keeping references seems like a good idea to me.

@maxcountryman

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

@kennethreitz
Owner

I'd prefer _data and data

@maxcountryman

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

@maxcountryman

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.

@kennethreitz
Owner

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

@maxcountryman

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

@kennethreitz
Owner

Correct.

@maxcountryman maxcountryman 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.

This commit has been updated to rename `body` to `content` and to create an
original value of the content variable, `_content` as well as a modified
variable `content`.

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.content = request._enc_data and we can now alter body as we
should like.
57c9205
@maxcountryman

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

@piotr-dobrogost

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

@kennethreitz
Owner

original could be a dictionary.

@piotr-dobrogost

Don't we want to make content_type available to hooks as well?

@maxcountryman

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.

@piotr-dobrogost

original could be a dictionary.

Sure, this is what I had in mind.

@piotr-dobrogost

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

@maxcountryman

@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?

@maxcountryman

This patch is also needed by Rauth: litl/rauth#6

@kennethreitz

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

Please be patient :)

@kennethreitz
Owner

This shouldn't be an issue anymore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 7, 2012
  1. @maxcountryman

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

    maxcountryman authored
    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.
    
    This commit has been updated to rename `body` to `content` and to create an
    original value of the content variable, `_content` as well as a modified
    variable `content`.
    
    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.content = request._enc_data and we can now alter body as we
    should like.
This page is out of date. Refresh to see the latest.
Showing with 9 additions and 4 deletions.
  1. +9 −4 requests/models.py
View
13 requests/models.py
@@ -92,6 +92,10 @@ def __init__(self,
#: :class:`Request <Request>`.
self.params = None
+ #: The content of a :class:`Request <Request>`.
+ self._content = None # unmodified
+ self.content = None # modified
+
#: True if :class:`Request <Request>` is part of a redirect chain (disables history
#: and HTTPError storage).
self.redirect = redirect
@@ -410,7 +414,8 @@ def send(self, anyway=False, prefetch=False):
))
# Nottin' on you.
- body = None
+ self._content = None
+ self.content = None
content_type = None
# Multi-part file uploads.
@@ -431,14 +436,14 @@ def send(self, anyway=False, prefetch=False):
fp = v
fields.update({k: (fn, fp.read())})
- (body, content_type) = encode_multipart_formdata(fields)
+ (self._content, content_type) = encode_multipart_formdata(fields)
else:
pass
# TODO: Conflict?
else:
if self.data:
- body = self._enc_data
+ self._content = self._enc_data
if isinstance(self.data, str):
content_type = None
else:
@@ -538,7 +543,7 @@ def send(self, anyway=False, prefetch=False):
r = conn.urlopen(
method=self.method,
url=self.path_url,
- body=body,
+ body=self.content or self._content,
headers=self.headers,
redirect=False,
assert_same_host=False,
Something went wrong with that request. Please try again.