-
-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
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