New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GET with a 302 redirect response causing params to be glued on incorrectly #157

Closed
cactus opened this Issue Sep 8, 2011 · 10 comments

Comments

Projects
None yet
2 participants
@cactus

cactus commented Sep 8, 2011

I am calling a service like this:

import requests
resp = requests.get('http://example.com/some/url', params={'id': 1})

The service responds with a series of 302 redirects.
I am seeing this exchange.

client> GET /some/url?id=1 HTTP/1.1
client> Host: example.com

server< HTTP/1.1 302 Moved Temporarily
server< Location: http://example.com/some/url?id=1&extra_data=2

## this is the bad request right here. Note the [('id', 1)] stuck on the end
client> GET /some/url?id=1&extra_data=2&[('id', 1)] HTTP/1.1.
client> Host: example.com

# the server of course doesn't like it
server< HTTP/1.0 400 Bad request.

Any ideas why this may be happening?

@cactus

This comment has been minimized.

cactus commented Sep 8, 2011

Adding a print statement on line 79 of models.py like this:

print 'params: %r, %r, %r' % (params, self.params, self._enc_params)

Shows me the following output:

params: {'id': 1}, [('id', 1)], 'id=1'
params: [('id', 1)], [('id', 1)], [('id', 1)]

So it looks like the branch in _encode_params is not doing the right thing for the second request (after redirect).

@cactus

This comment has been minimized.

cactus commented Sep 8, 2011

This seems to fix it, but I don't know if it has other unintended side effects.

diff --git a/requests/models.py b/requests/models.py
index 6180e96..b3e32d2 100644
--- a/requests/models.py
+++ b/requests/models.py
@@ -278,7 +278,9 @@ class Request(object):
                                    v.encode('utf-8') if isinstance(v, unicode) 
             return result, urllib.urlencode(result, doseq=True)
         else:
-            return data, data
+            if data == None:
+                return data, data
+            return data, urllib.urlencode(data, doseq=True)


     def _build_url(self):
@cactus

This comment has been minimized.

cactus commented Sep 8, 2011

previous print output now looks like this:

params: {'id': 1}, [('id', 1)], 'id=1'
params: [('id', 1)], [('id', 1)], 'id=1'
@kennethreitz

This comment has been minimized.

Member

kennethreitz commented Sep 8, 2011

Excellent! Thanks for both the bug and the apparent solution.

Can you send a pull request?

@cactus

This comment has been minimized.

cactus commented Sep 8, 2011

Hmm. Note that the patch provide does not guard against the addition of duplicate results in the request.

I am seeing this now (note the additional copy of id=1 in the qs):

client> GET /some/url?id=1&extra_data=2&id=1 HTTP/1.1.
client> Host: example.com

Is there a mechanism to not supply params again for redirect requests, but to simply follow the redirect?

@cactus

This comment has been minimized.

cactus commented Sep 8, 2011

Also, by simply appending (even if the location redirect provides it) identical elements, could there arise a situation where a form array post type query string could accidentally get more elements added to it?

Such as: http://example.com/some/url?arr[]=val1&arr[]=val2

@cactus

This comment has been minimized.

cactus commented Sep 8, 2011

In response to your question, I sure can send a pull request. I just wanted to make sure the solution was tenable (and correct) before doing so.

@cactus

This comment has been minimized.

cactus commented Sep 8, 2011

pull request #158

@kennethreitz

This comment has been minimized.

Member

kennethreitz commented Nov 13, 2011

Fixed!

@cactus

This comment has been minimized.

cactus commented Nov 14, 2011

great! thanks. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment