Merge pull request #1896 from diox/cachebust-icons-by-hash

Cachebust icons according to their hash (bug 985290)

apps/addons/models.py

@@ -258,6 +258,7 @@ class Addon(amo.models.OnChangeMixin, amo.models.ModelBase):
         db_column='higheststatus')
     icon_type = models.CharField(max_length=25, blank=True,
                                  db_column='icontype')
+    icon_hash = models.CharField(max_length=8, blank=True, null=True)
     homepage = TranslatedField()
     support_email = TranslatedField(db_column='supportemail')
     support_url = TranslatedField(db_column='supporturl')
@@ -1003,9 +1004,16 @@ def get_icon_url(self, size, use_default=True):
         else:
             # [1] is the whole ID, [2] is the directory
             split_id = re.match(r'((\d*?)\d{1,3})$', str(self.id))
+            # If we don't have the icon_hash, and we are dealing with a Webapp,
+            # it's fine, set to a dummy string ("never"), when the icon is
+            # eventually changed, icon_hash will be updated. For regular Addons
+            # we rely on the modified instead like we always have.
+            if self.type == amo.ADDON_WEBAPP:
+                suffix = getattr(self, 'icon_hash', None) or 'never'
+            else:
+                suffix = int(time.mktime(self.modified.timetuple()))
             return settings.ADDON_ICON_URL % (
-                split_id.group(2) or 0, self.id, size,
-                int(time.mktime(self.modified.timetuple())))
+                split_id.group(2) or 0, self.id, size, suffix)
 
     @write
     def update_status(self):

apps/addons/tests/test_models.py

@@ -600,15 +600,26 @@ def test_icon_url(self):
         a = Addon.objects.get(pk=3615)
         expected = (settings.ADDON_ICON_URL % (3, 3615, 32, 0)).rstrip('/0')
         assert a.icon_url.startswith(expected)
+
         a = Addon.objects.get(pk=6704)
         a.icon_type = None
         assert a.icon_url.endswith('/icons/default-theme.png'), (
             'No match for %s' % a.icon_url)
+
         a = Addon.objects.get(pk=3615)
         a.icon_type = None
-
         assert a.icon_url.endswith('icons/default-32.png')
 
+        a.icon_type = 'image/png'
+        assert a.icon_url.endswith('?modified=%s' %
+                                   int(time.mktime(a.modified.timetuple())))
+
+        a.type = amo.ADDON_WEBAPP  # a is now a Webapp, with no icon_hash.
+        assert a.icon_url.endswith('?modified=never')
+
+        a.icon_hash = 'fakehash'  # a is now a Webapp, with an icon_hash.
+        assert a.icon_url.endswith('?modified=fakehash')
+
     def test_icon_url_default(self):
         a = Addon.objects.get(pk=3615)
         a.update(icon_type='')

apps/amo/decorators.py

@@ -172,8 +172,11 @@ def write(f):
 
 def set_modified_on(f):
     """
-    Will update the modified timestamp on the provided objects
-    when the wrapped function exits sucessfully (returns True).
+    Will update the modified timestamp on the provided objects when the wrapped
+    function exits sucessfully (returns a truthy value). If the function
+    returns a dict, it will also use that dict as additional keyword arguments
+    to update on the provided objects.
+
     Looks up objects defined in the set_modified_on kwarg.
     """
     from amo.tasks import set_modified_on_object
@@ -183,11 +186,12 @@ def wrapper(*args, **kw):
         objs = kw.pop('set_modified_on', None)
         result = f(*args, **kw)
         if objs and result:
+            extra_kwargs = result if isinstance(result, dict) else {}
             for obj in objs:
                 task_log.info('Delaying setting modified on object: %s, %s' %
                               (obj.__class__.__name__, obj.pk))
                 set_modified_on_object.apply_async(
-                    args=[obj], kwargs=None,
+                    args=[obj], kwargs=extra_kwargs,
                     eta=datetime.datetime.now() +
                         datetime.timedelta(seconds=settings.NFS_LAG_DELAY))
         return result

apps/amo/tasks.py

@@ -79,7 +79,7 @@ def set_modified_on_object(obj, **kw):
     try:
         log.info('Setting modified on object: %s, %s' %
                  (obj.__class__.__name__, obj.pk))
-        obj.update(modified=datetime.datetime.now())
+        obj.update(modified=datetime.datetime.now(), **kw)
     except Exception, e:
         log.error('Failed to set modified on: %s, %s - %s' %
                   (obj.__class__.__name__, obj.pk, e))

migrations/766-add-icon-hash.sql

@@ -0,0 +1 @@
+ALTER TABLE addons ADD COLUMN `icon_hash` char(8) NULL;

mkt/developers/tasks.py

@@ -1,5 +1,6 @@
 # -*- coding: utf-8 -*-
 import base64
+import hashlib
 import json
 import logging
 import os
@@ -125,26 +126,30 @@ def run_validator(file_path, url=None):
                     url=url)
 
 
+def _hash_file(fd):
+    return hashlib.md5(fd.read()).hexdigest()[:8]
+
+
 @task
 @set_modified_on
-def resize_icon(src, dst, size, locally=False, **kw):
+def resize_icon(src, dst, sizes, locally=False, **kw):
     """Resizes addon icons."""
     log.info('[1@None] Resizing icon: %s' % dst)
     try:
-        if isinstance(size, list):
-            for s in size:
-                resize_image(src, '%s-%s.png' % (dst, s), (s, s),
-                             remove_src=False, locally=locally)
-            if locally:
-                os.remove(src)
-            else:
-                storage.delete(src)
+        for s in sizes:
+            resize_image(src, '%s-%s.png' % (dst, s), (s, s),
+                         remove_src=False, locally=locally)
+        if locally:
+            with open(src) as fd:
+                icon_hash = _hash_file(fd)
+            os.remove(src)
         else:
-            resize_image(src, dst, (size, size), remove_src=True,
-                         locally=locally)
+            with storage.open(src) as fd:
+                icon_hash = _hash_file(fd)
+            storage.delete(src)
 
         log.info('Icon resizing completed for: %s' % dst)
-        return True
+        return {'icon_hash': icon_hash}
     except Exception, e:
         log.error("Error saving addon icon: %s; %s" % (e, dst))
 
@@ -251,7 +256,7 @@ def save_icon(webapp, content):
                 set_modified_on=[webapp])
 
     # Need to set the icon type so .get_icon_url() works
-    # normally submit step 4 does it through AddonFormMedia,
+    # normally submit step 4 does it through AppFormMedia,
     # but we want to beat them to the punch.
     # resize_icon outputs pngs, so we know it's 'image/png'
     webapp.icon_type = 'image/png'

mkt/developers/tests/test_tasks.py

@@ -35,26 +35,26 @@
 def test_resize_icon_shrink():
     """ Image should be shrunk so that the longest side is 32px. """
 
-    resize_size = 32
-    final_size = (32, 12)
+    resize_size = [32]
+    final_size = [(32, 12)]
 
     _uploader(resize_size, final_size)
 
 
 def test_resize_icon_enlarge():
     """ Image stays the same, since the new size is bigger than both sides. """
 
-    resize_size = 1000
-    final_size = (339, 128)
+    resize_size = [1000]
+    final_size = [(339, 128)]
 
     _uploader(resize_size, final_size)
 
 
 def test_resize_icon_same():
     """ Image stays the same, since the new size is the same. """
 
-    resize_size = 339
-    final_size = (339, 128)
+    resize_size = [339]
+    final_size = [(339, 128)]
 
     _uploader(resize_size, final_size)
 
@@ -72,48 +72,35 @@ def _uploader(resize_size, final_size):
     img = get_image_path('mozilla.png')
     original_size = (339, 128)
 
-    src = tempfile.NamedTemporaryFile(mode='r+w+b', suffix=".png",
-                                      delete=False)
-
-    # resize_icon removes the original
-    shutil.copyfile(img, src.name)
-
-    with storage.open(src.name) as fp:
-        src_image = Image.open(fp)
-        src_image.load()
-    eq_(src_image.size, original_size)
-
-    if isinstance(final_size, list):
-        for rsize, fsize in zip(resize_size, final_size):
-            dest_name = os.path.join(settings.ADDON_ICONS_PATH, '1234')
-
-            tasks.resize_icon(src.name, dest_name, resize_size, locally=True)
-            with storage.open("%s-%s.png" % (dest_name, rsize)) as fp:
-                dest_image = Image.open(fp)
-                dest_image.load()
-
-            # Assert that the width is always identical.
-            eq_(dest_image.size[0], fsize[0])
-            # Assert that the height can be a wee bit fuzzy.
-            assert -1 <= dest_image.size[1] - fsize[1] <= 1, (
-                "Got width %d, expected %d" %
-                    (fsize[1], dest_image.size[1]))
-
-            if os.path.exists(dest_image.filename):
-                os.remove(dest_image.filename)
-            assert not os.path.exists(dest_image.filename)
-    else:
-        dest = tempfile.NamedTemporaryFile(mode='r+w+b', suffix=".png")
-        tasks.resize_icon(src.name, dest.name, resize_size, locally=True)
-        with storage.open(dest.name) as fp:
+
+    for rsize, fsize in zip(resize_size, final_size):
+        dest_name = os.path.join(settings.ADDON_ICONS_PATH, '1234')
+        src = tempfile.NamedTemporaryFile(mode='r+w+b', suffix='.png',
+                                          delete=False)
+        # resize_icon removes the original, copy it to a tempfile and use that.
+        shutil.copyfile(img, src.name)
+        # Sanity check.
+        with storage.open(src.name) as fp:
+            src_image = Image.open(fp)
+            src_image.load()
+        eq_(src_image.size, original_size)
+
+        val = tasks.resize_icon(src.name, dest_name, resize_size, locally=True)
+        eq_(val, {'icon_hash': 'bb362450'})
+        with storage.open('%s-%s.png' % (dest_name, rsize)) as fp:
             dest_image = Image.open(fp)
             dest_image.load()
 
         # Assert that the width is always identical.
-        eq_(dest_image.size[0], final_size[0])
+        eq_(dest_image.size[0], fsize[0])
         # Assert that the height can be a wee bit fuzzy.
-        assert -1 <= dest_image.size[1] - final_size[1] <= 1, (
-            "Got width %d, expected %d" % (final_size[1], dest_image.size[1]))
+        assert -1 <= dest_image.size[1] - fsize[1] <= 1, (
+            'Got width %d, expected %d' %
+                (fsize[1], dest_image.size[1]))
+
+        if os.path.exists(dest_image.filename):
+            os.remove(dest_image.filename)
+        assert not os.path.exists(dest_image.filename)
 
     assert not os.path.exists(src.name)
 

mkt/search/serializers.py

@@ -119,9 +119,9 @@ def create_fake_app(self, data):
         # It doesn't mean they'll get exposed in the serializer output, that
         # depends on what the fields/exclude attributes in Meta.
         for field_name in ('created', 'modified', 'default_locale',
-                           'is_escalated', 'is_offline', 'manifest_url',
-                           'premium_type', 'regions', 'reviewed', 'status',
-                           'weekly_downloads'):
+                           'icon_hash', 'is_escalated', 'is_offline',
+                           'manifest_url', 'premium_type', 'regions',
+                           'reviewed', 'status', 'weekly_downloads'):
             setattr(obj, field_name, data.get(field_name))
 
         # Attach translations for all translated attributes.
@@ -217,8 +217,10 @@ def data(self):
     def to_native(self, obj):
         # fake_app is a fake instance because we need to access a couple
         # properties and methods on Webapp. It should never hit the database.
-        fake_app = Webapp(id=obj['id'], icon_type='image/png',
+        fake_app = Webapp(
+            id=obj['id'], icon_type='image/png', type=amo.ADDON_WEBAPP,
             default_locale=obj.get('default_locale', settings.LANGUAGE_CODE),
+            icon_hash=obj.get('icon_hash'),
             modified=datetime.strptime(obj['modified'], '%Y-%m-%dT%H:%M:%S'))
         ESTranslationSerializerField.attach_translations(fake_app, obj, 'name')
         return {

mkt/search/tests/test_api.py

@@ -98,6 +98,7 @@ def setUp(self):
         self.webapp = Webapp.objects.get(pk=337141)
         self.category = Category.objects.create(name='test', slug='test',
                                                 type=amo.ADDON_WEBAPP)
+        self.webapp.icon_hash = 'fakehash'
         self.webapp.save()
         self.refresh('webapp')
 
@@ -204,6 +205,7 @@ def test_dehydrate(self):
             eq_(obj['description'],
                 {'en-US': self.webapp.description.localized_string})
             eq_(obj['icons']['128'], self.webapp.get_icon_url(128))
+            ok_(obj['icons']['128'].endswith('?modified=fakehash'))
             eq_(obj['id'], long(self.webapp.id))
             eq_(obj['manifest_url'], self.webapp.get_manifest_url())
             eq_(obj['payment_account'], None)
@@ -674,6 +676,15 @@ def test_usk_refused_exclude(self):
         res = self.client.get(self.url, {'region': 'de'})
         ok_(not res.json['objects'])
 
+    def test_icon_url_never(self):
+        self.webapp.update(icon_hash=None)
+        self.refresh('webapp')
+        res = self.client.get(self.url)
+        eq_(res.status_code, 200)
+        obj = res.json['objects'][0]
+        eq_(obj['icons']['64'], self.webapp.get_icon_url(64))
+        ok_(obj['icons']['64'].endswith('?modified=never'))
+
 
 class TestApiFeatures(RestOAuth, ESTestCase):
     fixtures = fixture('webapp_337141')
@@ -1017,6 +1028,7 @@ def setUp(self):
         self.app2 = app_factory(name=u'Something Second Something Something',
                                 description=u'Second dèsc' * 25,
                                 icon_type='image/png',
+                                icon_hash='fakehash',
                                 created=self.days_ago(3),
                                 manifest_url='http://rocket.example.com')
         self.app2.addondevicetype_set.create(device_type=amo.DEVICE_GAIA.id)
@@ -1046,6 +1058,7 @@ def test_suggestions(self):
                         'icon': self.app2.get_icon_url(64),
                         'name': unicode(self.app2.name),
                         'slug': self.app2.app_slug})
+        ok_(self.app2.get_icon_url(64).endswith('?modified=fakehash'))
 
     def test_suggestion_default_locale(self):
         self.app2.name.locale = 'es'
@@ -1071,7 +1084,7 @@ def test_suggestions_multiple_results(self):
         eq_(parsed[0], {'manifest_url': self.app1.get_manifest_url(),
                         'icon': self.app1.get_icon_url(64),
                         'name': unicode(self.app1.name),
-                      'slug': self.app1.app_slug})
+                        'slug': self.app1.app_slug})
         eq_(parsed[1], {'manifest_url': self.app2.get_manifest_url(),
                         'icon': self.app2.get_icon_url(64),
                         'name': unicode(self.app2.name),

mkt/webapps/models.py

@@ -1543,12 +1543,8 @@ def _locale_field_mapping(field, analyzer):
                             for f in APP_FEATURES)
                     },
                     'has_public_stats': {'type': 'boolean'},
-                    'icons': {
-                        'type': 'object',
-                        'properties': {
-                            'size': {'type': 'short'},
-                        }
-                    },
+                    'icon_hash': {'type': 'string',
+                                  'index': 'not_analyzed'},
                     'interactive_elements': {
                         'type': 'string',
                         'index': 'not_analyzed'
@@ -1712,7 +1708,7 @@ def extract_document(cls, pk, obj=None):
         d['device'] = getattr(obj, 'device_ids', [])
         d['features'] = features
         d['has_public_stats'] = obj.public_stats
-        d['icons'] = [{'size': icon_size} for icon_size in (16, 48, 64, 128)]
+        d['icon_hash'] = obj.icon_hash
         d['interactive_elements'] = obj.get_interactives_slugs()
         d['is_escalated'] = is_escalated
         d['is_offline'] = getattr(obj, 'is_offline', False)
@@ -1823,12 +1819,13 @@ def extract_document(cls, pk, obj=None):
                 'output': unicode(obj.id),  # We only care about the payload.
                 'weight': d['_boost'],
                 'payload': {
+                    'default_locale': d['default_locale'],
+                    'icon_hash': d['icon_hash'],
                     'id': d['id'],
+                    'manifest_url': d['manifest_url'],
                     'modified': d['modified'],
+                    'name_translations': d['name_translations'],
                     'slug': d['app_slug'],
-                    'manifest_url': d['manifest_url'],
-                    'default_locale': d['default_locale'],
-                    'name_translations': d['name_translations']
                 }
             }