Merge lp:~abentley/charmworld/icon-as-file into lp:~juju-jitsu/charmworld/trunk

[Aaron Bentley]

Store icon.svg

This branch enables using charm/file to retrieve the icon for a charm.

As well as ensuring it's provided, it also ensures the HTTP headers are suitable-- the MIME type is provided, and Cache-Control ensures that icons are cached for 24 hours. It is save to cache them this long because they change rarely, and when they do change, their default URL also changes.

[Curtis Hovey (sinzui)]

Thank you.

review: Approve (code)

=== modified file 'charmworld/models.py'
--- charmworld/models.py	2013-05-03 15:47:38 +0000
+++ charmworld/models.py	2013-05-16 20:49:26 +0000
@@ -4,6 +4,7 @@
 
 import errno
 import logging
+from mimetypes import guess_type
 from os import listdir
 from os.path import dirname
 from os.path import exists
@@ -260,6 +261,7 @@
         'metadata.yaml',
         'readme.*',
         'revision',
+        'icon.svg',
     ]
 
     store_directories = [
@@ -270,11 +272,13 @@
     def make_charm_file(cls, fs, charm_data, file_path, filename):
         charmid = cls.gen_charmid(charm_data)
         fileid = cls.gen_fileid(charm_data, file_path)
+        content_type = guess_type(filename)[0]
         return CharmFile(
             fs,
             fileid,
             charmid,
             filename=filename,
+            contentType=content_type,
         )
 
     @classmethod
@@ -406,6 +410,7 @@
     attributes = [
         'fileid',
         'charmid',
+        'contentType',
         'subdir',
         'filename',
         'length',
@@ -448,8 +453,11 @@
         else:
             return self._id.split('/')[-1]
 
+    # value to use as default to distinguish between None/null and unset.
+    undefined = object()
+
     def __init__(self, fs, fileid, charmid, filename=None,
-                 instance=None, metadata=None):
+                 instance=None, metadata=None, contentType=undefined):
         """Get a file object started.
 
         :param fs: the gridfs client to use.
@@ -466,6 +474,12 @@
         self._id = fileid
         self.charmid = charmid
         self._filename = None
+        if contentType is not self.undefined:
+            self.contentType = contentType
+        elif instance is not None:
+            self.contentType = instance.contentType
+        else:
+            self.contentType = None
 
         if instance:
             self._gridout = instance
@@ -506,6 +520,7 @@
         self.fs.put(
             contents,
             filename=self.filename,
+            contentType=self.contentType,
             **self.metadata
         )
 

=== modified file 'charmworld/tests/test_models.py'
--- charmworld/tests/test_models.py	2013-05-03 19:46:57 +0000
+++ charmworld/tests/test_models.py	2013-05-16 20:49:26 +0000
@@ -223,6 +223,7 @@
             quote_key('config.yaml'),
             quote_key('metadata.yaml'),
             quote_key('README.md'),
+            quote_key('icon.svg'),
             'hooks/config-changed',
             'hooks/database-relation-broken',
             'hooks/database-relation-changed',
@@ -352,6 +353,24 @@
         self.assertEqual(expected, messages)
         self.assertEqual([], files)
 
+    def test_provides_content_type(self):
+        path = self.use_context(temp_dir())
+        with file(join(path, 'icon.svg'), 'w') as f:
+            f.write('foo')
+        with file(join(path, 'readme.txt'), 'w') as f:
+            f.write('foo')
+        CharmFileSet.save_files(
+            self.fs,
+            self.charm_data,
+            path,
+            self.logger)
+        icon_id = self.base_fileid + quote_key('icon.svg')
+        cf = CharmFileSet.get_by_id(self.fs, icon_id)
+        self.assertEqual('image/svg+xml', cf.contentType)
+        readme_id = self.base_fileid + quote_key('readme.txt')
+        cf = CharmFileSet.get_by_id(self.fs, readme_id)
+        self.assertEqual('text/plain', cf.contentType)
+
 
 class TestCharmFile(MongoTestBase):
 

=== modified file 'charmworld/views/api.py'
--- charmworld/views/api.py	2013-05-16 15:44:30 +0000
+++ charmworld/views/api.py	2013-05-16 20:49:26 +0000
@@ -5,7 +5,6 @@
 from inspect import getargspec
 import json
 from os.path import (
-    basename,
     join,
 )
 import re
@@ -311,19 +310,23 @@
 
     def _charm_file(self, charm, trailing):
         path = trailing.split('/', 1)[1]
+        file_id = CharmFileSet.gen_fileid(charm, path)
         fs = getfs(self.request.db)
-        charm_file = CharmFileSet.make_charm_file(fs, charm, path,
-                                                  basename(path))
         try:
-            return Response(
-                charm_file.read(),
-                headerlist=[
-                    ("Access-Control-Allow-Origin", "*"),
-                    ("Access-Control-Allow-Headers", "X-Requested-With"),
-                ],
-                status_code=200)
+            charm_file = CharmFileSet.get_by_id(fs, file_id)
         except NoFile:
             return json_response(404, {'type': 'no_such_file', 'path': path})
+        headerlist = [
+            ("Access-Control-Allow-Origin", "*"),
+            ("Access-Control-Allow-Headers", "X-Requested-With"),
+            ("Cache-Control", "max-age=86400, public"),
+        ]
+        if charm_file.contentType is not None:
+            headerlist.append(('Content-Type', charm_file.contentType))
+        return Response(
+            charm_file.read(),
+            headerlist=headerlist,
+            status_code=200)
 
     @staticmethod
     def _format_category(category):

=== modified file 'charmworld/views/tests/test_api.py'
--- charmworld/views/tests/test_api.py	2013-05-16 14:46:25 +0000
+++ charmworld/views/tests/test_api.py	2013-05-16 20:49:26 +0000
@@ -1015,6 +1015,32 @@
         self.assertEqual({'type': 'no_such_file', 'path': 'subdir/README'},
                          response.json_body)
 
+    def test_file_includes_content_type(self):
+        charm = factory.makeCharm(self.db)[1]
+        factory.make_charm_file(self.db, charm, 'readme.txt')
+        factory.make_charm_file(self.db, charm, 'readme')
+        factory.make_charm_file(self.db, charm, 'readme.svg')
+        charm_id = self.api_class._get_api_id(charm)
+        response = self.get_response(
+            'charm', remainder='/' + charm_id + '/file/readme.txt')
+        self.assertEqual('text/plain', response.content_type)
+        response = self.get_response(
+            'charm', remainder='/' + charm_id + '/file/readme')
+        response = self.get_response(
+            'charm', remainder='/' + charm_id + '/file/readme.svg')
+        self.assertEqual('image/svg+xml', response.content_type)
+
+    def test_file_includes_cache_instructions(self):
+        # The files can be cached a long time because they rarely change, and
+        # when they do, their default URL will also change.
+        charm = factory.makeCharm(self.db)[1]
+        factory.make_charm_file(self.db, charm, 'readme.txt')
+        charm_id = self.api_class._get_api_id(charm)
+        response = self.get_response(
+            'charm', remainder='/' + charm_id + '/file/readme.txt')
+        self.assertIn(("Cache-Control", "max-age=86400, public"),
+                      response.headerlist)
+
     def test_unknown_sub_endpoint(self):
         charm = factory.makeCharm(self.db)[1]
         charm_id = self.api_class._get_api_id(charm)