Fix #1955: Do not use original request in redirect #1963
Conversation
The original request was never being properly overriden in resolve_redirects. As such being having a POST request respond with a 303 would generate a GET request. If the GET request encountered another redirect to something like a 307, then it would use the original request and generate another POST request. There are two parts to this fix: - The fix itself - The test infrastructure to ensure it does not regress because HTTPBin is insufficient
|
The code change seems correct to me. Re the test infrastructure, though, may I suggest you write a mock adapter object rather than a mock session object? That may be a bit more work, but it would be a more "natural" test (in the sense that more of the normal logic is exercised), and it would be more easily generalizable to cases like the bad gzipped response body in #1944. |
|
We could use Betamax but we don't have a real URL to test this against. |
|
I was imagining an adapter that wasn't backed by any actual network activity, just an internal list of pseudo-URLs and how it should react to them. |
|
Betamax only needs the network once. After the responses are recorded, it would respond to an internal list of URLs and it would know how to react to them. |
|
LGTM. I'll let you come up with a testing plan. |
|
@kennethreitz since you're around, care to weigh in on this one? |
|
Wow, this is an old, old, old bug. I totally forgot about it. |
Fix #1955: Do not use original request in redirect
The original request was never being properly overriden in resolve_redirects.
As such being having a POST request respond with a 303 would generate a GET
request. If the GET request encountered another redirect to something like a
307, then it would use the original request and generate another POST request.
There are two parts to this fix:
insufficient