301/302 redirects for non-POST with body #3757

nateprewitt · 2016-12-08T18:23:36Z

This patch is an attempt to address #2590 by relaxing the way Requests handles redirects.

Taking a look at how we’re handling stripping bodies, I modified the conditional to avoid an overly complicated check. The only requests we want to strip bodies off of are ones that have been modified from the redirect. That means the method will have already been changed to GET in rebuild_method and we can perform a uniform stripping for only GET requests. Personally, I'd like to have rebuild_method return a value, either a boolean or the new method, and use that as the parameter for the check. Then move the code inside the conditional into it's own function. This will help a bit with the overall size of resolve_redirects.

I also removed the 302 catch-all which was put in place in #1704 when RFC 2616 was still the standard. The previous mandates for 302 aren’t included in RFC 7231 6.4.3 and don’t appear to be reproducible in Chrome or Firefox with any tools supporting PUT, PATCH, or DELETE. (Note: if there’s a repro for this that I’m missing please let me know and I’ll gladly revert this. I’m just going off of the information available.)

The "test all methods" tests may be a bit overboard, I added them primarily for my own assurance to make sure I wasn't missing edge cases. I can remove those if they're deemed unnecessary.

nateprewitt · 2016-12-08T18:55:34Z

tests/test_requests.py

+    def test_http_301_for_redirectable_methods(self, httpbin, method, body, expected):
+        """Tests all methods except OPTIONS for expected redirect behaviour.
+
+        OPTIONS isn't a redirectable method and will always return values for


This was a hasty comment. There isn't anything in the RFC that states this, and it seems servers from major platforms treat the request differently (Facebook treats it as a GET??). I think the problem here is that it's unpredictable how a server may respond. I'll clarify the comment in the next review pass.

I think there is no reason to assume that OPTIONS cannot be redirected, and nothing in the Requests codebase forbids it.

Lukasa

Generally this looks really good. There's some confusion around OPTIONS that needs to be cleaned up, and I'd like some more explanatory comments, but mostly this seems fab.

Lukasa · 2016-12-09T09:34:10Z

tests/test_requests.py

+    def test_http_301_for_redirectable_methods(self, httpbin, method, body, expected):
+        """Tests all methods except OPTIONS for expected redirect behaviour.
+
+        OPTIONS isn't a redirectable method and will always return values for


I think there is no reason to assume that OPTIONS cannot be redirected, and nothing in the Requests codebase forbids it.

Lukasa · 2016-12-09T09:41:53Z

tests/test_requests.py

+        assert r.status_code == 200
+        assert r.history == []
+        assert r.request.method == 'OPTIONS'
+        assert 'Allow' in r.headers


This is really only a test of httpbin's particular behaviour.

Lukasa · 2016-12-09T09:42:35Z

tests/test_requests.py

+        """Tests all methods except OPTIONS for expected redirect behaviour.
+
+        OPTIONS isn't a redirectable method and will always return values for
+        the original URI.


Same note re OPTIONS here.

Lukasa · 2016-12-09T09:44:09Z

tests/test_requests.py

+        """Tests all methods except OPTIONS for expected redirect behaviour.
+
+        OPTIONS isn't a redirectable method and will always return values for
+        the original URI.


Same note re OPTIONS.

Lukasa · 2016-12-09T09:47:37Z

requests/sessions.py

-        # This bizarre behaviour is explained in Issue 1704.
-        if response.status_code == codes.moved and method == 'POST':
+        # If a POST is responded to with a 301 or 302, turn it into a GET.
+        if response.status_code in (codes.found, codes.moved) and method == 'POST':


We may want to flesh this comment out a bit here to point out that there is a lack of consistency over what exactly happens here. Some browsers have historically changed other methods (I can find some citations of Chrome changing DELETE to GET), but ultimately most browsers rarely ever issue methods outside of POST and GET.

Our best option, then, is to mostly emulate what curl does, and curl restricts this behaviour to POST only. That seems like the most consistent position to take. But we should aim to cite our sources. ;)

nateprewitt · 2016-12-09T18:47:29Z

Alright, comments updated.

Personally, I'd like to have rebuild_method return a value, either a boolean or the new method, and use that as the parameter for the check. Then move the code inside the conditional into it's own function. This will help a bit with the overall size of resolve_redirects.

Thoughts on mucking with this? If not, I think we're good to go.

Lukasa

I'm open to changing the signature of rebuild_method.

Lukasa · 2016-12-09T21:28:06Z

tests/test_requests.py

+        to them. For that reason they aren't included here.
+        """
+        s = requests.Session()
+        params = {'url': '/%s' % method.lower(), 'status_code': '301'}


Should the URL parameter not be expected?

Lukasa · 2016-12-09T21:29:00Z

tests/test_requests.py

+        s = requests.Session()
+        params = {'url': '/%s' % method.lower(), 'status_code': '301'}
+        req = requests.Request(method, httpbin('redirect-to'), data=body, params=params)
+        r = s.send(req.prepare())


Why are we using the PreparedRequest flow?

So I went with the PreparedRequest flow because I felt it was easier to understand. We could use the API mappings by writing the test as:

params = {'url': '/%s' % method.lower(), 'status_code': '301'} req = getattr(requests, method) req(httpbin('redirect-to'), data=body, params=params)

but that's probably not a paradigm to encourage and makes the test kind of odd for the sake of saving 2 lines.

Is it not easier just to use requests.request?

First method in the api and I don't know if I knew that existed before now. That's definitely simpler, I'll get that updated.

Lukasa · 2016-12-09T21:30:28Z

tests/test_requests.py

+        s = requests.Session()
+        params = {'url': '/%s' % method.lower()}
+        req = requests.Request(method, httpbin('redirect-to'), data=body, params=params)
+        r = s.send(req.prepare())


Same notes as above.

Lukasa · 2016-12-09T21:31:15Z

tests/test_requests.py

+        s = requests.Session()
+        params = {'url': '/%s' % method.lower(), 'status_code': '303'}
+        req = requests.Request(method, httpbin('redirect-to'), data=body, params=params)
+        r = s.send(req.prepare())


Same notes here.

nateprewitt · 2016-12-09T23:00:18Z

requests/sessions.py

-            old_method = prepared_request.method
-            self.rebuild_method(prepared_request, response)
-            new_method = prepared_request.method
+            method_changed = self.rebuild_method(prepared_request, response)


Eh, so I guess I'm indifferent on this now that I'm really looking at it. Pulling the logic from the conditional into it's own function adds code without significantly reducing the resolve_redirects footprint.

The return value for rebuild_method provides an op vs no-op check but isn't particularly useful. I threw it in there as a slight readability improvement, but we can ditch it if you aren't feeling it.

sigmavirus24 · 2016-12-10T13:50:36Z

requests/sessions.py

-            if response.status_code not in (codes.temporary_redirect,
-                    codes.permanent_redirect):
-                # https://github.com/kennethreitz/requests/issues/3490
+            # https://github.com/kennethreitz/requests/issues/2590


We should probably keep old issue references if only for historical context.

I think #3490 still makes sense to have, but #1084 is relating to code that is being removed here. We can keep it in if you think it's still providing value, but I feel it will end up as more of a red herring. Maybe making a note in #2590 with the link would be more useful if it's needed?

sigmavirus24 · 2016-12-10T13:56:28Z

requests/sessions.py

        """
-        method = prepared_request.method
+        original_method = prepared_request.method


Nit: This might be less confusing if we did

method = original_method = prepared_request.method

Agreed, I like that better too.

nateprewitt · 2016-12-12T14:26:32Z

@Lukasa, I think this is ready for a peek whenever you've got a moment :)

Lukasa

Looking pretty good! Got a note about the testing layout, but not too much otherwise.

Lukasa · 2016-12-12T15:16:27Z

tests/test_requests.py

+        assert r.history[0].status_code == 301
+        assert r.history[0].is_redirect
+        assert r.request.body == 'test body'
+        assert r.json()['data'] == 'test body'


This test looks pretty close to a duplicate of the one below. Any reason we wouldn't just move the check on status_code into the test below?

You're right, this one was written before test_http_301_for_redirectable_methods was, following the styling above. We can actually collapse the three tests above into the one below, but I left them to ensure we were always testing for those specific cases. The thought being if test_http_301_for_redirectable_methods is later modified it would be nice for those to still fail, but I understand if you'd like to see them squashed together.

As for the status code, I left that out of the test below because /head doesn't exist in httpbin (because it's non-sensical). The test logic becomes more complex trying to code around the HEAD case and the status code isn't needed confirm we got what we wanted from the test, only that httpbin is functioning properly.

I think there's no reason to keep them separate. =)

Ok, I'll squash them all down then.

nateprewitt · 2016-12-12T15:38:45Z

Ok, 301-303 individual tests removed in favor of the big ones.

Lukasa

Cool, I like this. Thanks for the work @nateprewitt!

nateprewitt commented Dec 8, 2016

View reviewed changes

nateprewitt force-pushed the 2590_redirected_bodies branch from 56f8854 to 9f84319 Compare December 9, 2016 04:21

Lukasa requested changes Dec 9, 2016

View reviewed changes

nateprewitt force-pushed the 2590_redirected_bodies branch from 9f84319 to 77cae3f Compare December 9, 2016 18:43

Lukasa requested changes Dec 9, 2016

View reviewed changes

nateprewitt force-pushed the 2590_redirected_bodies branch from 77cae3f to a3b5539 Compare December 9, 2016 22:40

nateprewitt commented Dec 9, 2016

View reviewed changes

nateprewitt changed the title ~~[WIP] 301/302 redirects for non-POST with body~~ 301/302 redirects for non-POST with body Dec 9, 2016

sigmavirus24 reviewed Dec 10, 2016

View reviewed changes

nateprewitt force-pushed the 2590_redirected_bodies branch from a3b5539 to 326bb4a Compare December 10, 2016 19:02

nateprewitt added 3 commits December 10, 2016 12:12

fixing redirects for non-GET/HEAD/POST methods

084fb05

adding return variable to rebuild_method for method change

eff9326

updating 3.0-HISTORY for psf#3757

3bb25a1

nateprewitt force-pushed the 2590_redirected_bodies branch from 326bb4a to 3bb25a1 Compare December 11, 2016 05:02

Lukasa requested changes Dec 12, 2016

View reviewed changes

squash 3xx tests into super tests

354ac7e

Lukasa approved these changes Dec 12, 2016

View reviewed changes

Lukasa merged commit 0b4302b into psf:proposed/3.0.0 Dec 12, 2016

nateprewitt deleted the 2590_redirected_bodies branch December 12, 2016 16:09

nateprewitt mentioned this pull request Dec 12, 2016

PATCH get response 301(moved permanently) is redirected with empty body #2590

Closed

nodakai mentioned this pull request Feb 19, 2017

Suggestion: add a new field method to Response class #3881

Closed

nateprewitt mentioned this pull request Dec 9, 2020

301 on PUT drops body #5686

Closed

github-actions bot locked as resolved and limited conversation to collaborators Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

301/302 redirects for non-POST with body #3757

301/302 redirects for non-POST with body #3757

nateprewitt commented Dec 8, 2016

nateprewitt Dec 8, 2016

Lukasa Dec 9, 2016

Lukasa left a comment

Lukasa Dec 9, 2016

Lukasa Dec 9, 2016

Lukasa Dec 9, 2016

Lukasa Dec 9, 2016

Lukasa Dec 9, 2016

nateprewitt commented Dec 9, 2016

Lukasa left a comment

Lukasa Dec 9, 2016

Lukasa Dec 9, 2016

nateprewitt Dec 9, 2016 •

edited

Loading

Lukasa Dec 9, 2016

nateprewitt Dec 9, 2016

Lukasa Dec 9, 2016

Lukasa Dec 9, 2016

nateprewitt Dec 9, 2016

sigmavirus24 Dec 10, 2016

nateprewitt Dec 10, 2016 •

edited

Loading

sigmavirus24 Dec 10, 2016

nateprewitt Dec 10, 2016

nateprewitt commented Dec 12, 2016 •

edited

Loading

Lukasa left a comment

Lukasa Dec 12, 2016

nateprewitt Dec 12, 2016 •

edited

Loading

Lukasa Dec 12, 2016

nateprewitt Dec 12, 2016

nateprewitt commented Dec 12, 2016

Lukasa left a comment

301/302 redirects for non-POST with body #3757

301/302 redirects for non-POST with body #3757

Conversation

nateprewitt commented Dec 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lukasa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nateprewitt commented Dec 9, 2016

Lukasa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nateprewitt Dec 9, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nateprewitt Dec 10, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nateprewitt commented Dec 12, 2016 • edited Loading

Lukasa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nateprewitt Dec 12, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nateprewitt commented Dec 12, 2016

Lukasa left a comment

Choose a reason for hiding this comment

nateprewitt Dec 9, 2016 •

edited

Loading

nateprewitt Dec 10, 2016 •

edited

Loading

nateprewitt commented Dec 12, 2016 •

edited

Loading

nateprewitt Dec 12, 2016 •

edited

Loading