From: Ben Darnell <ben@bendarnell.com>
Date: Sun, 2 Dec 2012 17:33:51 +0000 (-0500)
Subject: Return a Vary: Accept-Encoding header whenever gzip is enabled.
X-Git-Tag: v3.0.0~205
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=69ead0286cddf4af7b18d8955aec833532512dc1;p=thirdparty%2Ftornado.git

Return a Vary: Accept-Encoding header whenever gzip is enabled.

This is one of two problems found with redbot.  The other,
that etags should change when content-encoding is used, is trickier to
fix and seems to be less problematic.

Closes #578.
---

diff --git a/maint/test/redbot/red_test.py b/maint/test/redbot/red_test.py
index cc0024201..b6d9e3910 100644
--- a/maint/test/redbot/red_test.py
+++ b/maint/test/redbot/red_test.py
@@ -46,8 +46,22 @@ class TestMixin(object):
     def get_app_kwargs(self):
         return dict(static_path='.')
 
+    def get_allowed_warnings(self):
+        return [
+            # We can't set a non-heuristic freshness at the framework level,
+            # so just ignore this warning
+            rs.FRESHNESS_HEURISTIC,
+            # For our small test responses the Content-Encoding header
+            # wipes out any gains from compression
+            rs.CONNEG_GZIP_BAD,
+            ]
+
+    def get_allowed_errors(self):
+        return []
+
     def check_url(self, path, method='GET', body=None, headers=None,
-                  expected_status=200, allowed_warnings=None):
+                  expected_status=200, allowed_warnings=None,
+                  allowed_errors=None):
         url = self.get_url(path)
         state = self.run_redbot(url, method, body, headers)
         if not state.res_complete:
@@ -59,20 +73,19 @@ class TestMixin(object):
 
         self.assertEqual(int(state.res_status), expected_status)
 
-        allowed_warnings = tuple(allowed_warnings or ())
-        # We can't set a non-heuristic freshness at the framework level,
-        # so just ignore this error.
-        allowed_warnings += (rs.FRESHNESS_HEURISTIC,)
+        allowed_warnings = (allowed_warnings or []) + self.get_allowed_warnings()
+        allowed_errors = (allowed_errors or []) + self.get_allowed_errors()
 
         errors = []
         warnings = []
         for msg in state.messages:
             if msg.level == 'bad':
                 logger = logging.error
-                errors.append(msg)
+                if not isinstance(msg, tuple(allowed_errors)):
+                    errors.append(msg)
             elif msg.level == 'warning':
                 logger = logging.warning
-                if not isinstance(msg, allowed_warnings):
+                if not isinstance(msg, tuple(allowed_warnings)):
                     warnings.append(msg)
             elif msg.level in ('good', 'info', 'uri'):
                 logger = logging.info
@@ -140,6 +153,20 @@ class DefaultHTTPTest(AsyncHTTPTestCase, LogTrapTestCase, TestMixin):
     def get_app(self):
         return Application(self.get_handlers(), **self.get_app_kwargs())
 
+class GzipHTTPTest(AsyncHTTPTestCase, LogTrapTestCase, TestMixin):
+    def get_app(self):
+        return Application(self.get_handlers(), gzip=True, **self.get_app_kwargs())
+
+    def get_allowed_errors(self):
+        return super(GzipHTTPTest, self).get_allowed_errors() + [
+            # TODO: The Etag is supposed to change when Content-Encoding is
+            # used.  This should be fixed, but it's difficult to do with the
+            # way GZipContentEncoding fits into the pipeline, and in practice
+            # it doesn't seem likely to cause any problems as long as we're
+            # using the correct Vary header.
+            rs.VARY_ETAG_DOESNT_CHANGE,
+            ]
+
 if __name__ == '__main__':
     parse_command_line()
     unittest.main()
diff --git a/tornado/test/web_test.py b/tornado/test/web_test.py
index 6b1fdf960..39ba5ea9f 100644
--- a/tornado/test/web_test.py
+++ b/tornado/test/web_test.py
@@ -616,6 +616,11 @@ js_embed()
         self.assertEqual(response.body, b(""))
         response = self.fetch("/get_argument")
         self.assertEqual(response.body, b("default"))
+
+    def test_no_gzip(self):
+        response = self.fetch('/get_argument')
+        self.assertNotIn('Accept-Encoding', response.headers.get('Vary', ''))
+        self.assertNotIn('gzip', response.headers.get('Content-Encoding', ''))
 wsgi_safe.append(WSGISafeWebTest)
 
 
@@ -979,3 +984,29 @@ class ErrorHandlerXSRFTest(WebTestCase):
         response = self.fetch('/404', method='POST', body='')
         self.assertEqual(response.code, 404)
 wsgi_safe.append(ErrorHandlerXSRFTest)
+
+
+class GzipTestCase(SimpleHandlerTestCase):
+    class Handler(RequestHandler):
+        def get(self):
+            if self.get_argument('vary', None):
+                self.set_header('Vary', self.get_argument('vary'))
+            self.write('hello world')
+
+    def get_app_kwargs(self):
+        return dict(gzip=True)
+
+    def test_gzip(self):
+        response = self.fetch('/')
+        self.assertEqual(response.headers['Content-Encoding'], 'gzip')
+        self.assertEqual(response.headers['Vary'], 'Accept-Encoding')
+
+    def test_gzip_not_requested(self):
+        response = self.fetch('/', use_gzip=False)
+        self.assertNotIn('Content-Encoding', response.headers)
+        self.assertEqual(response.headers['Vary'], 'Accept-Encoding')
+
+    def test_vary_already_present(self):
+        response = self.fetch('/?vary=Accept-Language')
+        self.assertEqual(response.headers['Vary'],
+                         'Accept-Language, Accept-Encoding')
diff --git a/tornado/web.py b/tornado/web.py
index 4d8362a6f..eb235488d 100644
--- a/tornado/web.py
+++ b/tornado/web.py
@@ -1768,6 +1768,10 @@ class GZipContentEncoding(OutputTransform):
             "gzip" in request.headers.get("Accept-Encoding", "")
 
     def transform_first_chunk(self, status_code, headers, chunk, finishing):
+        if 'Vary' in headers:
+            headers['Vary'] += b(', Accept-Encoding')
+        else:
+            headers['Vary'] = b('Accept-Encoding')
         if self._gzipping:
             ctype = _unicode(headers.get("Content-Type", "")).split(";")[0]
             self._gzipping = (ctype in self.CONTENT_TYPES) and \