Repository: scrapy/scrapy
Pull request #1398 (2015-07-31 20:28:44)

PY3 port http cookies handling
/cc @kmike 

kmike: Hey @dangra,

As I understand it, in Python 3 stdlib header names and values are unicode (see examples in [docs](https://docs.python.org/3/library/urllib.request.html#urllib.request.Request)). But our WrappedRequest and WrappedResponse objects work with scrapy.http.Headers class which returns values as bytes; I wonder if these values are used by stdlib's CookieJar and if it is a problem. 

Patch:
diff --git a/scrapy/downloadermiddlewares/cookies.py b/scrapy/downloadermiddlewares/cookies.py
index 270d621cd..321c0171b 100644
--- a/scrapy/downloadermiddlewares/cookies.py
+++ b/scrapy/downloadermiddlewares/cookies.py
@@ -6,6 +6,7 @@ from collections import defaultdict
 from scrapy.exceptions import NotConfigured
 from scrapy.http import Response
 from scrapy.http.cookies import CookieJar
+from scrapy.utils.python import to_native_str
 
 logger = logging.getLogger(__name__)
 
@@ -52,18 +53,20 @@ class CookiesMiddleware(object):
 
     def _debug_cookie(self, request, spider):
         if self.debug:
-            cl = request.headers.getlist('Cookie')
+            cl = [to_native_str(c, errors='replace')
+                  for c in request.headers.getlist('Cookie')]
             if cl:
-                msg = "Sending cookies to: %s" % request + os.linesep
-                msg += os.linesep.join("Cookie: %s" % c for c in cl)
+                cookies = "\n".join("Cookie: {}\n".format(c) for c in cl)
+                msg = "Sending cookies to: {}\n{}".format(request, cookies)
                 logger.debug(msg, extra={'spider': spider})
 
     def _debug_set_cookie(self, response, spider):
         if self.debug:
-            cl = response.headers.getlist('Set-Cookie')
+            cl = [to_native_str(c, errors='replace')
+                  for c in response.headers.getlist('Set-Cookie')]
             if cl:
-                msg = "Received cookies from: %s" % response + os.linesep
-                msg += os.linesep.join("Set-Cookie: %s" % c for c in cl)
+                cookies = "\n".join("Set-Cookie: {}\n".format(c) for c in cl)
+                msg = "Received cookies from: {}\n{}".format(response, cookies)
                 logger.debug(msg, extra={'spider': spider})
 
     def _format_cookie(self, cookie):
diff --git a/scrapy/http/cookies.py b/scrapy/http/cookies.py
index b1eb767cc..e92c3fe73 100644
--- a/scrapy/http/cookies.py
+++ b/scrapy/http/cookies.py
@@ -1,6 +1,9 @@
 import time
-from cookielib import CookieJar as _CookieJar, DefaultCookiePolicy, IPV4_RE
+from six.moves.http_cookiejar import (
+    CookieJar as _CookieJar, DefaultCookiePolicy, IPV4_RE
+)
 from scrapy.utils.httpobj import urlparse_cached
+from scrapy.utils.python import to_native_str
 
 
 class CookieJar(object):
@@ -97,6 +100,7 @@ def potential_domain_matches(domain):
         pass
     return matches + ['.' + d for d in matches]
 
+
 class _DummyLock(object):
     def acquire(self):
         pass
@@ -133,6 +137,11 @@ class WrappedRequest(object):
         """
         return self.request.meta.get('is_unverifiable', False)
 
+    # python3 uses request.unverifiable
+    @property
+    def unverifiable(self):
+        return self.is_unverifiable()
+
     def get_origin_req_host(self):
         return urlparse_cached(self.request).hostname
 
@@ -140,14 +149,18 @@ class WrappedRequest(object):
         return name in self.request.headers
 
     def get_header(self, name, default=None):
-        return self.request.headers.get(name, default)
+        return to_native_str(self.request.headers.get(name, default),
+                             errors='replace')
 
     def header_items(self):
-        return self.request.headers.items()
+        return [
+            (to_native_str(k, errors='replace'),
+             [to_native_str(x, errors='replace') for x in v])
+            for k, v in self.request.headers.items()
+        ]
 
     def add_unredirected_header(self, name, value):
         self.request.headers.appendlist(name, value)
-        #print 'add_unredirected_header', self.request.headers
 
 
 class WrappedResponse(object):
@@ -158,5 +171,9 @@ class WrappedResponse(object):
     def info(self):
         return self
 
-    def getheaders(self, name):
-        return self.response.headers.getlist(name)
+    # python3 cookiejars calls get_all
+    def get_all(self, name, default=None):
+        return [to_native_str(v, errors='replace')
+                for v in self.response.headers.getlist(name)]
+    # python2 cookiejars calls getheaders
+    getheaders = get_all
diff --git a/scrapy/mail.py b/scrapy/mail.py
index 2b4c57980..ad8ecbe13 100644
--- a/scrapy/mail.py
+++ b/scrapy/mail.py
@@ -20,7 +20,6 @@ else:
     from email import encoders as Encoders
 
 from twisted.internet import defer, reactor, ssl
-from twisted.mail.smtp import ESMTPSenderFactory
 
 logger = logging.getLogger(__name__)
 
@@ -102,6 +101,8 @@ class MailSender(object):
                       'mailattachs': nattachs, 'mailerr': errstr})
 
     def _sendmail(self, to_addrs, msg):
+        # Import twisted.mail here because it is not available in python3
+        from twisted.mail.smtp import ESMTPSenderFactory
         msg = StringIO(msg)
         d = defer.Deferred()
         factory = ESMTPSenderFactory(self.smtpuser, self.smtppass, self.mailfrom, \


Test patch:
diff --git a/tests/py3-ignores.txt b/tests/py3-ignores.txt
index 0d4d397a3..469d2c5e1 100644
--- a/tests/py3-ignores.txt
+++ b/tests/py3-ignores.txt
@@ -11,7 +11,6 @@ tests/test_crawl.py
 tests/test_crawler.py
 tests/test_downloader_handlers.py
 tests/test_downloadermiddleware_ajaxcrawlable.py
-tests/test_downloadermiddleware_cookies.py
 tests/test_downloadermiddleware_defaultheaders.py
 tests/test_downloadermiddleware_downloadtimeout.py
 tests/test_downloadermiddleware_httpauth.py
@@ -24,7 +23,6 @@ tests/test_downloadermiddleware_retry.py
 tests/test_downloadermiddleware_stats.py
 tests/test_downloadermiddleware_useragent.py
 tests/test_engine.py
-tests/test_http_cookies.py
 tests/test_logformatter.py
 tests/test_mail.py
 tests/test_pipeline_files.py
@@ -51,7 +49,6 @@ scrapy/xlib/tx/endpoints.py
 scrapy/xlib/tx/client.py
 scrapy/xlib/tx/_newclient.py
 scrapy/xlib/tx/__init__.py
-scrapy/http/cookies.py
 scrapy/core/downloader/handlers/s3.py
 scrapy/core/downloader/handlers/http11.py
 scrapy/core/downloader/handlers/http.py
diff --git a/tests/test_downloadermiddleware_cookies.py b/tests/test_downloadermiddleware_cookies.py
index 996b8c388..26d9794b6 100644
--- a/tests/test_downloadermiddleware_cookies.py
+++ b/tests/test_downloadermiddleware_cookies.py
@@ -1,15 +1,19 @@
-from unittest import TestCase
 import re
+import logging
+from unittest import TestCase
+from testfixtures import LogCapture
 
 from scrapy.http import Response, Request
 from scrapy.spiders import Spider
+from scrapy.utils.test import get_crawler
+from scrapy.exceptions import NotConfigured
 from scrapy.downloadermiddlewares.cookies import CookiesMiddleware
 
 
 class CookiesMiddlewareTest(TestCase):
 
     def assertCookieValEqual(self, first, second, msg=None):
-        cookievaleq = lambda cv: re.split(';\s*', cv)
+        cookievaleq = lambda cv: re.split(';\s*', cv.decode('latin1'))
         return self.assertEqual(
             sorted(cookievaleq(first)),
             sorted(cookievaleq(second)), msg)
@@ -22,19 +26,91 @@ class CookiesMiddlewareTest(TestCase):
         del self.mw
 
     def test_basic(self):
-        headers = {'Set-Cookie': 'C1=value1; path=/'}
         req = Request('http://scrapytest.org/')
         assert self.mw.process_request(req, self.spider) is None
         assert 'Cookie' not in req.headers
 
+        headers = {'Set-Cookie': 'C1=value1; path=/'}
         res = Response('http://scrapytest.org/', headers=headers)
         assert self.mw.process_response(req, res, self.spider) is res
 
-        #assert res.cookies
+        req2 = Request('http://scrapytest.org/sub1/')
+        assert self.mw.process_request(req2, self.spider) is None
+        self.assertEquals(req2.headers.get('Cookie'), b"C1=value1")
+
+    def test_setting_false_cookies_enabled(self):
+        self.assertRaises(
+            NotConfigured,
+            CookiesMiddleware.from_crawler,
+            get_crawler(settings_dict={'COOKIES_ENABLED': False})
+        )
+
+    def test_setting_default_cookies_enabled(self):
+        self.assertIsInstance(
+            CookiesMiddleware.from_crawler(get_crawler()),
+            CookiesMiddleware
+        )
+
+    def test_setting_true_cookies_enabled(self):
+        self.assertIsInstance(
+            CookiesMiddleware.from_crawler(
+                get_crawler(settings_dict={'COOKIES_ENABLED': True})
+            ),
+            CookiesMiddleware
+        )
+
+    def test_setting_enabled_cookies_debug(self):
+        crawler = get_crawler(settings_dict={'COOKIES_DEBUG': True})
+        mw = CookiesMiddleware.from_crawler(crawler)
+        with LogCapture('scrapy.downloadermiddlewares.cookies',
+                        propagate=False,
+                        level=logging.DEBUG) as l:
+            req = Request('http://scrapytest.org/')
+            res = Response('http://scrapytest.org/',
+                           headers={'Set-Cookie': 'C1=value1; path=/'})
+            mw.process_response(req, res, crawler.spider)
+            req2 = Request('http://scrapytest.org/sub1/')
+            mw.process_request(req2, crawler.spider)
+
+            l.check(
+                ('scrapy.downloadermiddlewares.cookies',
+                 'DEBUG',
+                 'Received cookies from: <200 http://scrapytest.org/>\n'
+                 'Set-Cookie: C1=value1; path=/\n'),
+                ('scrapy.downloadermiddlewares.cookies',
+                 'DEBUG',
+                 'Sending cookies to: <GET http://scrapytest.org/sub1/>\n'
+                 'Cookie: C1=value1\n'),
+            )
+
+    def test_setting_disabled_cookies_debug(self):
+        crawler = get_crawler(settings_dict={'COOKIES_DEBUG': False})
+        mw = CookiesMiddleware.from_crawler(crawler)
+        with LogCapture('scrapy.downloadermiddlewares.cookies',
+                        propagate=False,
+                        level=logging.DEBUG) as l:
+            req = Request('http://scrapytest.org/')
+            res = Response('http://scrapytest.org/',
+                           headers={'Set-Cookie': 'C1=value1; path=/'})
+            mw.process_response(req, res, crawler.spider)
+            req2 = Request('http://scrapytest.org/sub1/')
+            mw.process_request(req2, crawler.spider)
+
+            l.check()
+
+    def test_do_not_break_on_non_utf8_header(self):
+        req = Request('http://scrapytest.org/')
+        assert self.mw.process_request(req, self.spider) is None
+        assert 'Cookie' not in req.headers
+
+        headers = {'Set-Cookie': b'C1=in\xa3valid; path=/',
+                   'Other': b'ignore\xa3me'}
+        res = Response('http://scrapytest.org/', headers=headers)
+        assert self.mw.process_response(req, res, self.spider) is res
 
         req2 = Request('http://scrapytest.org/sub1/')
         assert self.mw.process_request(req2, self.spider) is None
-        self.assertEquals(req2.headers.get('Cookie'), "C1=value1")
+        self.assertIn('Cookie', req2.headers)
 
     def test_dont_merge_cookies(self):
         # merge some cookies into jar
@@ -55,12 +131,12 @@ class CookiesMiddlewareTest(TestCase):
         # check that cookies are merged back
         req = Request('http://scrapytest.org/mergeme')
         assert self.mw.process_request(req, self.spider) is None
-        self.assertEquals(req.headers.get('Cookie'), 'C1=value1')
+        self.assertEquals(req.headers.get('Cookie'), b'C1=value1')
 
         # check that cookies are merged when dont_merge_cookies is passed as 0
         req = Request('http://scrapytest.org/mergeme', meta={'dont_merge_cookies': 0})
         assert self.mw.process_request(req, self.spider) is None
-        self.assertEquals(req.headers.get('Cookie'), 'C1=value1')
+        self.assertEquals(req.headers.get('Cookie'), b'C1=value1')
 
     def test_complex_cookies(self):
         # merge some cookies into jar
@@ -76,12 +152,12 @@ class CookiesMiddlewareTest(TestCase):
         # embed C1 and C3 for scrapytest.org/foo
         req = Request('http://scrapytest.org/foo')
         self.mw.process_request(req, self.spider)
-        assert req.headers.get('Cookie') in ('C1=value1; C3=value3', 'C3=value3; C1=value1')
+        assert req.headers.get('Cookie') in (b'C1=value1; C3=value3', b'C3=value3; C1=value1')
 
         # embed C2 for scrapytest.org/bar
         req = Request('http://scrapytest.org/bar')
         self.mw.process_request(req, self.spider)
-        self.assertEquals(req.headers.get('Cookie'), 'C2=value2')
+        self.assertEquals(req.headers.get('Cookie'), b'C2=value2')
 
         # embed nothing for scrapytest.org/baz
         req = Request('http://scrapytest.org/baz')
@@ -91,7 +167,7 @@ class CookiesMiddlewareTest(TestCase):
     def test_merge_request_cookies(self):
         req = Request('http://scrapytest.org/', cookies={'galleta': 'salada'})
         assert self.mw.process_request(req, self.spider) is None
-        self.assertEquals(req.headers.get('Cookie'), 'galleta=salada')
+        self.assertEquals(req.headers.get('Cookie'), b'galleta=salada')
 
         headers = {'Set-Cookie': 'C1=value1; path=/'}
         res = Response('http://scrapytest.org/', headers=headers)
@@ -100,12 +176,12 @@ class CookiesMiddlewareTest(TestCase):
         req2 = Request('http://scrapytest.org/sub1/')
         assert self.mw.process_request(req2, self.spider) is None
 
-        self.assertCookieValEqual(req2.headers.get('Cookie'), "C1=value1; galleta=salada")
+        self.assertCookieValEqual(req2.headers.get('Cookie'), b"C1=value1; galleta=salada")
 
     def test_cookiejar_key(self):
         req = Request('http://scrapytest.org/', cookies={'galleta': 'salada'}, meta={'cookiejar': "store1"})
         assert self.mw.process_request(req, self.spider) is None
-        self.assertEquals(req.headers.get('Cookie'), 'galleta=salada')
+        self.assertEquals(req.headers.get('Cookie'), b'galleta=salada')
 
         headers = {'Set-Cookie': 'C1=value1; path=/'}
         res = Response('http://scrapytest.org/', headers=headers, request=req)
@@ -113,11 +189,11 @@ class CookiesMiddlewareTest(TestCase):
 
         req2 = Request('http://scrapytest.org/', meta=res.meta)
         assert self.mw.process_request(req2, self.spider) is None
-        self.assertCookieValEqual(req2.headers.get('Cookie'),'C1=value1; galleta=salada')
+        self.assertCookieValEqual(req2.headers.get('Cookie'), b'C1=value1; galleta=salada')
 
         req3 = Request('http://scrapytest.org/', cookies={'galleta': 'dulce'}, meta={'cookiejar': "store2"})
         assert self.mw.process_request(req3, self.spider) is None
-        self.assertEquals(req3.headers.get('Cookie'), 'galleta=dulce')
+        self.assertEquals(req3.headers.get('Cookie'), b'galleta=dulce')
 
         headers = {'Set-Cookie': 'C2=value2; path=/'}
         res2 = Response('http://scrapytest.org/', headers=headers, request=req3)
@@ -125,7 +201,7 @@ class CookiesMiddlewareTest(TestCase):
 
         req4 = Request('http://scrapytest.org/', meta=res2.meta)
         assert self.mw.process_request(req4, self.spider) is None
-        self.assertCookieValEqual(req4.headers.get('Cookie'), 'C2=value2; galleta=dulce')
+        self.assertCookieValEqual(req4.headers.get('Cookie'), b'C2=value2; galleta=dulce')
 
         #cookies from hosts with port
         req5_1 = Request('http://scrapytest.org:1104/')
@@ -137,11 +213,11 @@ class CookiesMiddlewareTest(TestCase):
 
         req5_2 = Request('http://scrapytest.org:1104/some-redirected-path')
         assert self.mw.process_request(req5_2, self.spider) is None
-        self.assertEquals(req5_2.headers.get('Cookie'), 'C1=value1')
+        self.assertEquals(req5_2.headers.get('Cookie'), b'C1=value1')
 
         req5_3 = Request('http://scrapytest.org/some-redirected-path')
         assert self.mw.process_request(req5_3, self.spider) is None
-        self.assertEquals(req5_3.headers.get('Cookie'), 'C1=value1')
+        self.assertEquals(req5_3.headers.get('Cookie'), b'C1=value1')
 
         #skip cookie retrieval for not http request
         req6 = Request('file:///scrapy/sometempfile')
@@ -152,5 +228,4 @@ class CookiesMiddlewareTest(TestCase):
         request = Request("http://example-host/", cookies={'currencyCookie': 'USD'})
         assert self.mw.process_request(request, self.spider) is None
         self.assertIn('Cookie', request.headers)
-        self.assertIn('currencyCookie', request.headers['Cookie'])
-
+        self.assertEqual(b'currencyCookie=USD', request.headers['Cookie'])
diff --git a/tests/test_http_cookies.py b/tests/test_http_cookies.py
index 3d6993491..d529f609b 100644
--- a/tests/test_http_cookies.py
+++ b/tests/test_http_cookies.py
@@ -8,8 +8,8 @@ from scrapy.http.cookies import WrappedRequest, WrappedResponse
 class WrappedRequestTest(TestCase):
 
     def setUp(self):
-        self.request = Request("http://www.example.com/page.html", \
-            headers={"Content-Type": "text/html"})
+        self.request = Request("http://www.example.com/page.html",
+                               headers={"Content-Type": "text/html"})
         self.wrapped = WrappedRequest(self.request)
 
     def test_get_full_url(self):
@@ -23,10 +23,12 @@ class WrappedRequestTest(TestCase):
 
     def test_is_unverifiable(self):
         self.assertFalse(self.wrapped.is_unverifiable())
+        self.assertFalse(self.wrapped.unverifiable)
 
     def test_is_unverifiable2(self):
         self.request.meta['is_unverifiable'] = True
         self.assertTrue(self.wrapped.is_unverifiable())
+        self.assertTrue(self.wrapped.unverifiable)
 
     def test_get_origin_req_host(self):
         self.assertEqual(self.wrapped.get_origin_req_host(), 'www.example.com')
@@ -40,17 +42,19 @@ class WrappedRequestTest(TestCase):
         self.assertEqual(self.wrapped.get_header('xxxxx', 'def'), 'def')
 
     def test_header_items(self):
-        self.assertEqual(self.wrapped.header_items(), [('Content-Type', ['text/html'])])
+        self.assertEqual(self.wrapped.header_items(),
+                         [('Content-Type', ['text/html'])])
 
     def test_add_unredirected_header(self):
         self.wrapped.add_unredirected_header('hello', 'world')
-        self.assertEqual(self.request.headers['hello'], 'world')
+        self.assertEqual(self.request.headers['hello'], b'world')
+
 
 class WrappedResponseTest(TestCase):
 
     def setUp(self):
-        self.response = Response("http://www.example.com/page.html", 
-            headers={"Content-TYpe": "text/html"})
+        self.response = Response("http://www.example.com/page.html",
+                                 headers={"Content-TYpe": "text/html"})
         self.wrapped = WrappedResponse(self.response)
 
     def test_info(self):
@@ -58,3 +62,7 @@ class WrappedResponseTest(TestCase):
 
     def test_getheaders(self):
         self.assertEqual(self.wrapped.getheaders('content-type'), ['text/html'])
+
+    def test_get_all(self):
+        # get_all result must be native string
+        self.assertEqual(self.wrapped.get_all('content-type'), ['text/html'])