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 cc00242012043a1670a0f09b3fdd0b9b6087e6b0..b6d9e3910857ebb5987939a1ce6b62bdbe876acb 100644 (file)
--- 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 6b1fdf9602b7a6df052dc011ecb34e158553a715..39ba5ea9f13d247701ea7091808b2b3b60d62d15 100644 (file)
--- 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 4d8362a6f3bd91990f5451c91691911f1f872c96..eb235488d84f828ed8c6191251b940e60c730c59 100644 (file)
--- 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 \