Fix bug 1054472: Use ETags to determine if we can send 304s.

Changes the fetch_snippets view to add ETags to outgoing responses that
are a hash of their content. It then uses the incoming ETags from
clients to determine what snippet content the client currently has 
cached, and whether new snippet content needs to be delivered or if a
304 can be sent to instruct the client to use their cached snippets.

This should allow us to increase the frequency of updates by lowering
the amount of data we need to send.

snippets/base/middleware.py

@@ -1,20 +1,16 @@
 from django.core.urlresolvers import resolve
 
-from snippets.base.views import fetch_json_snippets, fetch_snippets
 
-
-class FetchSnippetsMiddleware(object):
+class SkipMiddleware(object):
     """
-    If the incoming request is for the fetch_snippets view, execute the view
-    and return it before other middleware can run.
+    If the incoming request is for a view that has the skip_middleware
+    kwarg, execute the view and return the response before other
+    middleware can run.
 
-    fetch_snippets is a very very basic view that doesn't need any of the
-    middleware that the rest of the site needs, such as the session or csrf
-    middlewares. To avoid unintended issues (such as headers we don't want
-    being added to the response) this middleware detects requests to that view
-    and executes the view early, bypassing the rest of the middleware.
+    Allows views like the FetchSnippets view to bypass unnecessary
+    middleware.
     """
     def process_request(self, request):
         result = resolve(request.path)
-        if result.func in (fetch_snippets, fetch_json_snippets):
+        if result.kwargs.pop('skip_middleware', False):
             return result.func(request, *result.args, **result.kwargs)

snippets/base/templates/base/fetch_snippets.html

@@ -5,4 +5,3 @@
   {% endfor %}
   {% include 'base/includes/snippet_js.html' %}
 </div>
-<!-- Content generated at {{ current_time }} -->

snippets/base/tests/__init__.py

@@ -64,3 +64,27 @@ class JSONSnippetFactory(BaseSnippetFactory):
 class ClientMatchRuleFactory(factory.DjangoModelFactory):
     FACTORY_FOR = models.ClientMatchRule
     description = factory.Sequence(lambda n: 'Client Match Rule {0}'.format(n))
+
+
+class CONTAINS(object):
+    """
+    Helper object that is equal to any object that contains a specific
+    value.
+
+    If exclusive=True is passed to the constructor, sets will be used
+    for comparison, meaning that an iterable is equal to this object
+    only if it contains the same values given in the constructor,
+    ignoring the order of values.
+    """
+    def __init__(self, *values, **kwargs):
+        self.values = values
+        self.exclusive = kwargs.get('exclusive', False)
+
+    def __eq__(self, other):
+        if self.exclusive:
+            return set(v for v in other) == set(self.values)
+        else:
+            return all(v in other for v in self.values)
+
+    def __ne__(self, other):
+        return not self.__eq__(other)

snippets/base/tests/test_middleware.py

@@ -1,57 +1,27 @@
-from mock import Mock, patch
+from django.test.client import RequestFactory
+
 from nose.tools import eq_
 
-from snippets.base.middleware import FetchSnippetsMiddleware
+from snippets.base.middleware import SkipMiddleware
 from snippets.base.tests import TestCase
 
 
-class FetchSnippetsMiddlewareTests(TestCase):
-    def setUp(self):
-        self.middleware = FetchSnippetsMiddleware()
-
-    @patch('snippets.base.middleware.resolve')
-    @patch('snippets.base.middleware.fetch_snippets')
-    def test_resolve_fetch_snippets_match(self, fetch_snippets, resolve):
-        """
-        If resolve returns a match to the fetch_snippets view, return the
-        result of the view.
-        """
-        request = Mock()
-        result = resolve.return_value
-        result.func = fetch_snippets
-        result.args = (1, 'asdf')
-        result.kwargs = {'blah': 5}
-
-        eq_(self.middleware.process_request(request),
-            fetch_snippets.return_value)
-        fetch_snippets.assert_called_with(request, 1, 'asdf', blah=5)
+class SkipMiddlewareTests(TestCase):
+    urls = 'snippets.base.tests.urls'
 
-    @patch('snippets.base.middleware.resolve')
-    @patch('snippets.base.middleware.fetch_json_snippets')
-    def test_resolve_fetch_json_snippets_match(self, fetch_json_snippets, resolve):
-        """
-        If resolve returns a match to the fetch_json_snippets view,
-        return the result of the view.
-        """
-        request = Mock()
-        result = resolve.return_value
-        result.func = fetch_json_snippets
-        result.args = (1, 'asdf')
-        result.kwargs = {'blah': 5}
+    def setUp(self):
+        self.middleware = SkipMiddleware()
+        self.factory = RequestFactory()
 
-        eq_(self.middleware.process_request(request),
-            fetch_json_snippets.return_value)
-        fetch_json_snippets.assert_called_with(request, 1, 'asdf', blah=5)
+    def test_process_request_no_kwargs(self):
+        """If the skip_middleware kwarg is missing, return None."""
+        request = self.factory.get('test')
+        eq_(self.middleware.process_request(request), None)
 
-    @patch('snippets.base.middleware.resolve')
-    @patch('snippets.base.middleware.fetch_snippets')
-    def test_resolve_no_match(self, fetch_snippets, resolve):
+    def test_process_request_kwargs(self):
         """
-        If resolve doesn't return a match to the fetch_snippets view, return
-        None.
+        If the skip_middleware kwarg is present, execute the view and
+        return the response.
         """
-        request = Mock()
-        result = resolve.return_value
-        result.func = lambda request: 'asdf'
-
-        eq_(self.middleware.process_request(request), None)
+        request = self.factory.get('test_skip')
+        eq_(self.middleware.process_request(request), 'skipped')

snippets/base/tests/test_views.py

@@ -2,23 +2,45 @@
 
 from django.contrib.auth.models import User
 from django.core.urlresolvers import reverse
+from django.http import HttpResponse
+from django.test.client import RequestFactory
 from django.test.utils import override_settings
 
 from funfactory.helpers import urlparams
-from mock import patch
+from mock import ANY, patch
 from nose.tools import eq_, ok_
 
-import snippets.base.models
+from snippets.base import views
 from snippets.base.models import Client
-from snippets.base.tests import (JSONSnippetFactory, SnippetFactory,
+from snippets.base.tests import (CONTAINS, JSONSnippetFactory, SnippetFactory,
                                  SnippetTemplateFactory, TestCase)
 
-snippets.base.models.CHANNELS = ('release', 'beta', 'aurora', 'nightly')
-snippets.base.models.FIREFOX_STARTPAGE_VERSIONS = ('1', '2', '3', '4')
-
 
+@override_settings(ETAG_CACHE_TIMEOUT=90)
 class FetchSnippetsTests(TestCase):
-    def test_base(self):
+    def setUp(self):
+        self.view = views.FetchSnippets()
+        self.view.get_client_cache_key = lambda *args: 'client_key'
+
+        self.mock_response = HttpResponse('')
+
+        self.factory = RequestFactory()
+        self.mock_request = self.factory.get('/')
+
+        self.view_kwargs = {
+            'startpage_version': '4',
+            'name': 'Firefox',
+            'version': '23.0a1',
+            'appbuildid': '20130510041606',
+            'build_target': 'Darwin_Universal-gcc3',
+            'locale': 'en-US',
+            'channel': 'nightly',
+            'os_version': 'Darwin 10.8.0',
+            'distribution': 'default',
+            'distribution_version': 'default_version'
+        }
+
+    def test_generate_response(self):
         # Matching snippets.
         snippet_1 = SnippetFactory.create(on_nightly=True)
 
@@ -28,14 +50,112 @@ def test_base(self):
         # Snippet that doesn't match.
         SnippetFactory.create(on_nightly=False),
 
-        snippets_ok = [snippet_1]
-        params = ('4', 'Firefox', '23.0a1', '20130510041606',
-                  'Darwin_Universal-gcc3', 'en-US', 'nightly',
-                  'Darwin%2010.8.0', 'default', 'default_version')
-        response = self.client.get('/{0}/'.format('/'.join(params)))
+        client = Client('4', 'Firefox', '23.0a1', '20130510041606', 'Darwin_Universal-gcc3',
+                        'en-US', 'nightly', 'Darwin%2010.8.0', 'default', 'default_version')
+        request = self.factory.get('/')
+
+        with patch.object(views, 'render') as render:
+            render.return_value = HttpResponse('asdf')
+
+            response = self.view.generate_response(request, client)
+            render.assert_called_with(request, ANY, {
+                'snippets': CONTAINS(snippet_1, exclusive=True),
+                'client': client,
+                'locale': 'en-US',
+            })
+
+            eq_(response, render.return_value)
+            asdf_sha256 = 'f0e4c2f76c58916ec258f246851bea091d14d4247a2fc3e18694461b1816e13b'
+            eq_(response['ETag'], asdf_sha256)
+            eq_(response['Vary'], 'If-None-Match')
+
+    def test_get_request_match_cache(self):
+        """
+        If the request has an ETag and it matches the cached ETag,
+        return a 304.
+        """
+        self.view.generate_response = lambda *args: self.mock_response
+        self.mock_request.META['HTTP_IF_NONE_MATCH'] = 'etag'
+
+        with patch('snippets.base.views.cache') as cache:
+            cache.get.return_value = 'etag'
+            response = self.view.get(self.mock_request, **self.view_kwargs)
+
+        ok_(not cache.set.called)
+        eq_(response.status_code, 304)
+
+    def test_get_cache_doesnt_match_response(self):
+        """
+        If the request ETag and cached ETag don't match, and the
+        resulting response's ETag doesn't match the cache, the cache
+        should be updated.
+        """
+        self.view.generate_response = lambda *args: self.mock_response
+        self.mock_response['ETag'] = 'etag'
+
+        with patch('snippets.base.views.cache') as cache:
+            cache.get.return_value = 'other_etag'
+            self.view.get(self.mock_request, **self.view_kwargs)
+
+        cache.set.assert_called_with('client_key', 'etag', 90)
 
-        eq_(set(snippets_ok), set(response.context['snippets']))
-        eq_(response.context['locale'], 'en-US')
+    def test_get_cache_empty(self):
+        """
+        If the cache is empty, it should be updated with the response's
+        ETag.
+        """
+        self.view.generate_response = lambda *args: self.mock_response
+        self.mock_response['ETag'] = 'etag'
+
+        with patch('snippets.base.views.cache') as cache:
+            cache.get.return_value = None
+            self.view.get(self.mock_request, **self.view_kwargs)
+
+        cache.set.assert_called_with('client_key', 'etag', 90)
+
+    def test_get_request_doesnt_match_cache_matches_response(self):
+        """
+        If the request's ETag doesn't match the cache but it matches
+        the response's ETag, return a 304.
+        """
+        self.view.generate_response = lambda *args: self.mock_response
+        self.mock_request.META['HTTP_IF_NONE_MATCH'] = 'etag'
+        self.mock_response['ETag'] = 'etag'
+
+        with patch('snippets.base.views.cache') as cache:
+            cache.get.return_value = 'other_etag'
+            response = self.view.get(self.mock_request, **self.view_kwargs)
+
+        cache.set.assert_called_with('client_key', 'etag', 90)
+        eq_(response.status_code, 304)
+
+    def test_get_request_doesnt_match_cache_or_response(self):
+        """
+        If the request's ETag doesn't match the cache or the response's
+        ETag, send the full response.
+        """
+        self.view.generate_response = lambda *args: self.mock_response
+        self.mock_request.META['HTTP_IF_NONE_MATCH'] = 'etag'
+        self.mock_response['ETag'] = 'other_etag'
+
+        with patch('snippets.base.views.cache') as cache:
+            cache.get.return_value = 'other_etag'
+            response = self.view.get(self.mock_request, **self.view_kwargs)
+
+        ok_(not cache.set.called)
+        eq_(response, self.mock_response)
+
+    def test_get_request_no_etag(self):
+        """If the request has no ETag, send the full response."""
+        self.view.generate_response = lambda *args: self.mock_response
+        self.mock_response['ETag'] = 'etag'
+
+        with patch('snippets.base.views.cache') as cache:
+            cache.get.return_value = 'etag'
+            response = self.view.get(self.mock_request, **self.view_kwargs)
+
+        ok_(not cache.set.called)
+        eq_(response, self.mock_response)
 
     @patch('snippets.base.views.Client', wraps=Client)
     def test_client_construction(self, ClientMock):
@@ -63,15 +183,15 @@ def test_client_construction(self, ClientMock):
     def test_cache_headers(self):
         """
         view_snippets should always have Cache-control set to
-        'public, max-age={settings.SNIPPET_HTTP_MAX_AGE}' and no Vary header,
-        even after middleware is executed.
+        'public, max-age={settings.SNIPPET_HTTP_MAX_AGE}' and only Vary
+        on If-None-Match, even after middleware is executed.
         """
         params = ('4', 'Firefox', '23.0a1', '20130510041606',
                   'Darwin_Universal-gcc3', 'en-US', 'nightly',
                   'Darwin%2010.8.0', 'default', 'default_version')
         response = self.client.get('/{0}/'.format('/'.join(params)))
         eq_(response['Cache-control'], 'public, max-age=75')
-        ok_('Vary' not in response)
+        eq_(response['Vary'], 'If-None-Match')
 
 
 class JSONSnippetsTests(TestCase):

snippets/base/tests/urls.py

@@ -0,0 +1,16 @@
+from django.conf.urls.defaults import patterns, url
+
+
+def test_view(request):
+    return 'test'
+
+
+def test_view_skip_middleware(request):
+    return 'skipped'
+
+
+urlpatterns = patterns('',
+    url(r'^test$', test_view),
+    url(r'^test_skip$', test_view_skip_middleware, {'skip_middleware': True}),
+)
+

snippets/base/urls.py

@@ -8,13 +8,13 @@
     url(r'^(?P<startpage_version>[^/]+)/(?P<name>[^/]+)/(?P<version>[^/]+)/'
         '(?P<appbuildid>[^/]+)/(?P<build_target>[^/]+)/(?P<locale>[^/]+)/'
         '(?P<channel>[^/]+)/(?P<os_version>[^/]+)/(?P<distribution>[^/]+)/'
-        '(?P<distribution_version>[^/]+)/$', views.fetch_snippets,
-        name='base.fetch_snippets'),
+        '(?P<distribution_version>[^/]+)/$', views.FetchSnippets.as_view(),
+        kwargs={'skip_middleware': True}, name='base.fetch_snippets'),
     url(r'^json/(?P<startpage_version>[^/]+)/(?P<name>[^/]+)/(?P<version>[^/]+)/'
         '(?P<appbuildid>[^/]+)/(?P<build_target>[^/]+)/(?P<locale>[^/]+)/'
         '(?P<channel>[^/]+)/(?P<os_version>[^/]+)/(?P<distribution>[^/]+)/'
         '(?P<distribution_version>[^/]+)/$', views.fetch_json_snippets,
-        name='base.fetch_json_snippets'),
+        kwargs={'skip_middleware': True}, name='base.fetch_json_snippets'),
     url(r'^preview/$', views.preview_snippet, name='base.preview'),
     url(r'^show/(?P<snippet_id>\d+)/$', views.show_snippet, name='base.show'),
 )