Repository: scrapy/scrapy
Pull request #1488 (2015-09-10 20:21:57)

[MRG+1] PY3 redirect downloader mware
Hey fellows!

So, here is my stab at porting the redirect downloader middleware to Python 3.
The important change here is decoding the `Location` header as latin1.

To verify that it's a reasonable assumption I wrote a [small Flask app](https://gist.github.com/eliasdorneles/ffd12f05811fee803fa0) and tested it on Chrome and FF.

You can see the screenshots of the app for Chrome here: http://imgur.com/a/1AfMt
Note that when using UTF-8 in the Location header we get a mojibake but things work normally when using latin1.

Se also this [related SO answer](http://stackoverflow.com/a/4410331/149872).

The other changes are a minor refactoring and adding the encoding argument to HtmlResponse in the tests.

Does this look good?


Patch:
diff --git a/scrapy/downloadermiddlewares/redirect.py b/scrapy/downloadermiddlewares/redirect.py
index 3cf8d2bee..4ed7e4c24 100644
--- a/scrapy/downloadermiddlewares/redirect.py
+++ b/scrapy/downloadermiddlewares/redirect.py
@@ -3,6 +3,7 @@ from six.moves.urllib.parse import urljoin
 
 from scrapy.http import HtmlResponse
 from scrapy.utils.response import get_meta_refresh
+from scrapy.utils.python import to_native_str
 from scrapy.exceptions import IgnoreRequest, NotConfigured
 
 logger = logging.getLogger(__name__)
@@ -55,30 +56,26 @@ class RedirectMiddleware(BaseRedirectMiddleware):
 
     def process_response(self, request, response, spider):
         if (request.meta.get('dont_redirect', False) or
-               response.status in getattr(spider, 'handle_httpstatus_list', []) or
-               response.status in request.meta.get('handle_httpstatus_list', []) or
-               request.meta.get('handle_httpstatus_all', False)):
+                response.status in getattr(spider, 'handle_httpstatus_list', []) or
+                response.status in request.meta.get('handle_httpstatus_list', []) or
+                request.meta.get('handle_httpstatus_all', False)):
             return response
 
-        if request.method == 'HEAD':
-            if response.status in [301, 302, 303, 307] and 'Location' in response.headers:
-                redirected_url = urljoin(request.url, response.headers['location'])
-                redirected = request.replace(url=redirected_url)
-                return self._redirect(redirected, request, spider, response.status)
-            else:
-                return response
-
-        if response.status in [302, 303] and 'Location' in response.headers:
-            redirected_url = urljoin(request.url, response.headers['location'])
-            redirected = self._redirect_request_using_get(request, redirected_url)
-            return self._redirect(redirected, request, spider, response.status)
+        allowed_status = (301, 302, 303, 307)
+        if 'Location' not in response.headers or response.status not in allowed_status:
+            return response
+
+        # HTTP header is ascii or latin1, redirected url will be percent-encoded utf-8
+        location = to_native_str(response.headers['location'].decode('latin1'))
 
-        if response.status in [301, 307] and 'Location' in response.headers:
-            redirected_url = urljoin(request.url, response.headers['location'])
+        redirected_url = urljoin(request.url, location)
+
+        if response.status in (301, 307) or request.method == 'HEAD':
             redirected = request.replace(url=redirected_url)
             return self._redirect(redirected, request, spider, response.status)
 
-        return response
+        redirected = self._redirect_request_using_get(request, redirected_url)
+        return self._redirect(redirected, request, spider, response.status)
 
 
 class MetaRefreshMiddleware(BaseRedirectMiddleware):


Test patch:
diff --git a/tests/py3-ignores.txt b/tests/py3-ignores.txt
index 95e2181e3..e40b4c73c 100644
--- a/tests/py3-ignores.txt
+++ b/tests/py3-ignores.txt
@@ -10,7 +10,6 @@ tests/test_downloadermiddleware_httpcache.py
 tests/test_downloadermiddleware_httpcompression.py
 tests/test_downloadermiddleware_httpproxy.py
 tests/test_downloadermiddleware.py
-tests/test_downloadermiddleware_redirect.py
 tests/test_downloadermiddleware_retry.py
 tests/test_engine.py
 tests/test_mail.py
diff --git a/tests/test_downloadermiddleware_redirect.py b/tests/test_downloadermiddleware_redirect.py
index b3db7c42b..9db073cc5 100644
--- a/tests/test_downloadermiddleware_redirect.py
+++ b/tests/test_downloadermiddleware_redirect.py
@@ -1,3 +1,5 @@
+# -*- coding: utf-8 -*-
+
 import unittest
 
 from scrapy.downloadermiddlewares.redirect import RedirectMiddleware, MetaRefreshMiddleware
@@ -150,6 +152,22 @@ class RedirectMiddlewareTest(unittest.TestCase):
                                                            [404, 301, 302]}))
         _test_passthrough(Request(url, meta={'handle_httpstatus_all': True}))
 
+    def test_latin1_location(self):
+        req = Request('http://scrapytest.org/first')
+        latin1_location = u'/ação'.encode('latin1')  # HTTP historically supports latin1
+        resp = Response('http://scrapytest.org/first', headers={'Location': latin1_location}, status=302)
+        req_result = self.mw.process_response(req, resp, self.spider)
+        perc_encoded_utf8_url = 'http://scrapytest.org/a%C3%A7%C3%A3o'
+        self.assertEquals(perc_encoded_utf8_url, req_result.url)
+
+    def test_location_with_wrong_encoding(self):
+        req = Request('http://scrapytest.org/first')
+        utf8_location = u'/ação'  # header with wrong encoding (utf-8)
+        resp = Response('http://scrapytest.org/first', headers={'Location': utf8_location}, status=302)
+        req_result = self.mw.process_response(req, resp, self.spider)
+        perc_encoded_utf8_url = 'http://scrapytest.org/a%C3%83%C2%A7%C3%83%C2%A3o'
+        self.assertEquals(perc_encoded_utf8_url, req_result.url)
+
 
 class MetaRefreshMiddlewareTest(unittest.TestCase):
 
@@ -159,8 +177,8 @@ class MetaRefreshMiddlewareTest(unittest.TestCase):
         self.mw = MetaRefreshMiddleware.from_crawler(crawler)
 
     def _body(self, interval=5, url='http://example.org/newpage'):
-        return """<html><head><meta http-equiv="refresh" content="{0};url={1}"/></head></html>"""\
-                .format(interval, url)
+        html = u"""<html><head><meta http-equiv="refresh" content="{0};url={1}"/></head></html>"""
+        return html.format(interval, url).encode('utf-8')
 
     def test_priority_adjust(self):
         req = Request('http://a.com')
@@ -178,7 +196,9 @@ class MetaRefreshMiddlewareTest(unittest.TestCase):
     def test_meta_refresh_with_high_interval(self):
         # meta-refresh with high intervals don't trigger redirects
         req = Request(url='http://example.org')
-        rsp = HtmlResponse(url='http://example.org', body=self._body(interval=1000))
+        rsp = HtmlResponse(url='http://example.org',
+                           body=self._body(interval=1000),
+                           encoding='utf-8')
         rsp2 = self.mw.process_response(req, rsp, self.spider)
         assert rsp is rsp2
 
@@ -231,5 +251,6 @@ class MetaRefreshMiddlewareTest(unittest.TestCase):
         self.assertEqual(req3.url, 'http://scrapytest.org/redirected2')
         self.assertEqual(req3.meta['redirect_urls'], ['http://scrapytest.org/first', 'http://scrapytest.org/redirected'])
 
+
 if __name__ == "__main__":
     unittest.main()