Permalink
Browse files

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.
  • Loading branch information...
bdarnell committed Dec 2, 2012
1 parent 0b1b515 commit 69ead0286cddf4af7b18d8955aec833532512dc1
Showing with 69 additions and 7 deletions.
  1. +34 −7 maint/test/redbot/red_test.py
  2. +31 −0 tornado/test/web_test.py
  3. +4 −0 tornado/web.py
@@ -46,8 +46,22 @@ def get_handlers(self):
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 @@ def check_url(self, path, method='GET', body=None, headers=None,
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()
View
@@ -616,6 +616,11 @@ def test_get_argument(self):
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 @@ def test_404_xsrf(self):
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')
View
@@ -1768,6 +1768,10 @@ def __init__(self, request):
"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 \

0 comments on commit 69ead02

Please sign in to comment.