r46977 - Merge addCookie-str-8067: Make twisted.web.http.Request.addCookie take bytes and unicode arguments

Author: hawkowl
Date: Thu Mar 10 18:25:29 2016
New Revision: 46977

Added:
   trunk/twisted/web/topfiles/8067.feature
Modified:
   trunk/twisted/web/http.py
   trunk/twisted/web/test/test_http.py

Log:
Merge addCookie-str-8067: Make twisted.web.http.Request.addCookie take bytes and unicode arguments

Author: hawkowl
Reviewers: SamyCookie, adiroiban
Fixes: #8067

Modified: trunk/twisted/web/http.py
==============================================================================
--- trunk/twisted/web/http.py	(original)
+++ trunk/twisted/web/http.py	Thu Mar 10 18:25:29 2016
@@ -547,6 +547,9 @@
                 i.e., ?foo=bar&foo=baz&quux=spam results in
                 {'foo': ['bar', 'baz'], 'quux': ['spam']}.
 
+    @ivar cookies: The cookies that will be sent in the response.
+    @type cookies: L{list} of L{bytes}
+
     @type requestHeaders: L{http_headers.Headers}
     @ivar requestHeaders: All received HTTP request headers.
 
@@ -870,7 +873,7 @@
 
         if not self.startedWriting:
             # write headers
-            self.write('')
+            self.write(b'')
 
         if self.chunked:
             # write last chunk and closing CRLF
@@ -938,7 +941,7 @@
                     l.extend([name, b": ", value, b"\r\n"])
 
             for cookie in self.cookies:
-                l.append(networkString('Set-Cookie: %s\r\n' % (cookie,)))
+                l.append(b'Set-Cookie: ' + cookie + b'\r\n')
 
             l.append(b"\r\n")
 
@@ -971,50 +974,75 @@
         L{twisted.web.server.Session} class for details.
 
         @param k: cookie name
-        @type k: L{str}
+        @type k: L{bytes} or L{unicode}
 
         @param v: cookie value
-        @type v: L{str}
+        @type v: L{bytes} or L{unicode}
 
         @param expires: cookie expire attribute value in
-        "Wdy, DD Mon YYYY HH:MM:SS GMT" format
-        @type expires: L{str}
+            "Wdy, DD Mon YYYY HH:MM:SS GMT" format
+        @type expires: L{bytes} or L{unicode}
 
         @param domain: cookie domain
-        @type domain: L{str}
+        @type domain: L{bytes} or L{unicode}
 
         @param path: cookie path
-        @type path: L{str}
+        @type path: L{bytes} or L{unicode}
 
         @param max_age: cookie expiration in seconds from reception
-        @type max_age: L{str}
+        @type max_age: L{bytes} or L{unicode}
 
         @param comment: cookie comment
-        @type comment: L{str}
+        @type comment: L{bytes} or L{unicode}
 
         @param secure: direct browser to send the cookie on encrypted
-        connections only
+            connections only
         @type secure: L{bool}
 
         @param httpOnly: direct browser not to expose cookies through channels
-        other than HTTP (and HTTPS) requests
+            other than HTTP (and HTTPS) requests
         @type httpOnly: L{bool}
+
+        @raises: L{DeprecationWarning} if an argument is not L{bytes} or
+            L{unicode}.
         """
-        cookie = '%s=%s' % (k, v)
+        def _ensureBytes(val):
+            """
+            Ensure that C{val} is bytes, encoding using UTF-8 if needed.
+            """
+            if val is None:
+                # It's None, so we don't want to touch it
+                return val
+
+            if isinstance(val, bytes):
+                return val
+            elif isinstance(val, unicode):
+                return val.encode('utf8')
+
+            # Not bytes or unicode, relying on string conversion legacy
+            # str() it, and warn, it's the best we can do
+            warnings.warn(
+                "Passing non-bytes or non-unicode cookie arguments is "
+                "deprecated since Twisted 16.1.",
+                category=DeprecationWarning, stacklevel=3)
+
+            return str(val).encode('utf8')
+
+        cookie = _ensureBytes(k) + b"=" + _ensureBytes(v)
         if expires is not None:
-            cookie = cookie + "; Expires=%s" % (expires, )
+            cookie = cookie + b"; Expires=" + _ensureBytes(expires)
         if domain is not None:
-            cookie = cookie + "; Domain=%s" % (domain, )
+            cookie = cookie + b"; Domain=" + _ensureBytes(domain)
         if path is not None:
-            cookie = cookie + "; Path=%s" % (path, )
+            cookie = cookie + b"; Path=" + _ensureBytes(path)
         if max_age is not None:
-            cookie = cookie + "; Max-Age=%s" % (max_age, )
+            cookie = cookie + b"; Max-Age=" + _ensureBytes(max_age)
         if comment is not None:
-            cookie = cookie + "; Comment=%s" % (comment, )
+            cookie = cookie + b"; Comment=" + _ensureBytes(comment)
         if secure:
-            cookie = cookie + "; Secure"
+            cookie = cookie + b"; Secure"
         if httpOnly:
-            cookie = cookie + "; HttpOnly"
+            cookie = cookie + b"; HttpOnly"
         self.cookies.append(cookie)
 
     def setResponseCode(self, code, message=None):

Modified: trunk/twisted/web/test/test_http.py
==============================================================================
--- trunk/twisted/web/test/test_http.py	(original)
+++ trunk/twisted/web/test/test_http.py	Thu Mar 10 18:25:29 2016
@@ -1526,29 +1526,99 @@
         self.assertEqual(req.responseHeaders.getRawHeaders(b"test"), [b"lemur"])
 
 
-    def test_addCookieWithMinimumArguments(self):
+    def _checkCookie(self, expectedCookieValue, *args, **kwargs):
         """
-        Add a Set-Cookie header with just name and value to the response.
+        Call L{http.Request.setCookie} with C{*args} and C{**kwargs}, and check
+        that the cookie value is equal to C{expectedCookieValue}.
         """
-        req = http.Request(DummyChannel(), False)
-        req.addCookie("foo", "bar")
-        self.assertEqual(req.cookies[0], "foo=bar")
+        channel = DummyChannel()
+        req = http.Request(channel, False)
+        req.addCookie(*args, **kwargs)
+        self.assertEqual(req.cookies[0], expectedCookieValue)
+
+        # Write nothing to make it produce the headers
+        req.write(b"")
+        writtenLines = channel.transport.written.getvalue().split(b"\r\n")
 
+        # There should be one Set-Cookie header
+        setCookieLines = [x for x in writtenLines
+                          if x.startswith(b"Set-Cookie")]
+        self.assertEqual(len(setCookieLines), 1)
+        self.assertEqual(setCookieLines[0],
+                         b"Set-Cookie: " + expectedCookieValue)
 
-    def test_addCookieWithAllArguments(self):
+
+    def test_addCookieWithMinimumArgumentsUnicode(self):
         """
-        Add a Set-Cookie header with name and value and all the supported
-        options to the response.
+        L{http.Request.setCookie} adds a new cookie to be sent with the
+        response, and can be called with just a key and a value. L{unicode}
+        arguments are encoded using UTF-8.
         """
-        req = http.Request(DummyChannel(), False)
-        req.addCookie(
-            "foo", "bar", expires="Fri, 31 Dec 9999 23:59:59 GMT",
-            domain=".example.com", path="/", max_age="31536000",
-            comment="test", secure=True, httpOnly=True)
-        self.assertEqual(req.cookies[0],
-                         "foo=bar; Expires=Fri, 31 Dec 9999 23:59:59 GMT; "
-                         "Domain=.example.com; Path=/; Max-Age=31536000; "
-                         "Comment=test; Secure; HttpOnly")
+        expectedCookieValue = b"foo=bar"
+
+        self._checkCookie(expectedCookieValue, u"foo", u"bar")
+
+
+    def test_addCookieWithAllArgumentsUnicode(self):
+        """
+        L{http.Request.setCookie} adds a new cookie to be sent with the
+        response. L{unicode} arguments are encoded using UTF-8.
+        """
+        expectedCookieValue = (
+            b"foo=bar; Expires=Fri, 31 Dec 9999 23:59:59 GMT; "
+            b"Domain=.example.com; Path=/; Max-Age=31536000; "
+            b"Comment=test; Secure; HttpOnly")
+
+        self._checkCookie(expectedCookieValue,
+            u"foo", u"bar", expires=u"Fri, 31 Dec 9999 23:59:59 GMT",
+            domain=u".example.com", path=u"/", max_age=u"31536000",
+            comment=u"test", secure=True, httpOnly=True)
+
+
+    def test_addCookieWithMinimumArgumentsBytes(self):
+        """
+        L{http.Request.setCookie} adds a new cookie to be sent with the
+        response, and can be called with just a key and a value. L{bytes}
+        arguments are not decoded.
+        """
+        expectedCookieValue = b"foo=bar"
+
+        self._checkCookie(expectedCookieValue, b"foo", b"bar")
+
+
+    def test_addCookieWithAllArgumentsBytes(self):
+        """
+        L{http.Request.setCookie} adds a new cookie to be sent with the
+        response. L{bytes} arguments are not decoded.
+        """
+        expectedCookieValue = (
+            b"foo=bar; Expires=Fri, 31 Dec 9999 23:59:59 GMT; "
+            b"Domain=.example.com; Path=/; Max-Age=31536000; "
+            b"Comment=test; Secure; HttpOnly")
+
+        self._checkCookie(expectedCookieValue,
+            b"foo", b"bar", expires=b"Fri, 31 Dec 9999 23:59:59 GMT",
+            domain=b".example.com", path=b"/", max_age=b"31536000",
+            comment=b"test", secure=True, httpOnly=True)
+
+
+    def test_addCookieNonStringArgument(self):
+        """
+        L{http.Request.setCookie} will raise a L{DeprecationWarning} if
+        non-string (not L{bytes} or L{unicode}) arguments are given, and will
+        call C{str()} on it to preserve past behaviour.
+        """
+        expectedCookieValue = b"foo=10"
+
+        self._checkCookie(expectedCookieValue, b"foo", 10)
+
+        warnings = self.flushWarnings([self._checkCookie])
+        self.assertEqual(1, len(warnings))
+        self.assertEqual(warnings[0]['category'], DeprecationWarning)
+        self.assertEqual(
+            warnings[0]['message'],
+            "Passing non-bytes or non-unicode cookie arguments is "
+            "deprecated since Twisted 16.1.")
 
 
     def test_firstWrite(self):