Merge pull request #12559 from jkozera/svg-thumbnail-generation

[TNL-3960] Fix for thumbnail generation code crashing with SVG files

common/lib/xmodule/xmodule/contentstore/content.py

@@ -44,13 +44,17 @@ class StaticContent(object):
         return self.location.category == 'thumbnail'
 
     @staticmethod
-    def generate_thumbnail_name(original_name, dimensions=None):
+    def generate_thumbnail_name(original_name, dimensions=None, extension=None):
         """
         - original_name: Name of the asset (typically its location.name)
         - dimensions: `None` or a tuple of (width, height) in pixels
+        - extension: `None` or desired filename extension of the thumbnail
         """
+        if extension is None:
+            extension = XASSET_THUMBNAIL_TAIL_NAME
+
         name_root, ext = os.path.splitext(original_name)
-        if not ext == XASSET_THUMBNAIL_TAIL_NAME:
+        if not ext == extension:
             name_root = name_root + ext.replace(u'.', u'-')
 
         if dimensions:
@@ -59,7 +63,7 @@ class StaticContent(object):
 
         return u"{name_root}{extension}".format(
             name_root=name_root,
-            extension=XASSET_THUMBNAIL_TAIL_NAME,
+            extension=extension,
         )
 
     @staticmethod
@@ -330,9 +334,10 @@ class ContentStore(object):
         pixels. It defaults to None.
         """
         thumbnail_content = None
+        is_svg = content.content_type == 'image/svg+xml'
         # use a naming convention to associate originals with the thumbnail
         thumbnail_name = StaticContent.generate_thumbnail_name(
-            content.location.name, dimensions=dimensions
+            content.location.name, dimensions=dimensions, extension='.svg' if is_svg else None
         )
         thumbnail_file_location = StaticContent.compute_location(
             content.location.course_key, thumbnail_name, is_thumbnail=True
@@ -340,28 +345,42 @@ class ContentStore(object):
 
         # if we're uploading an image, then let's generate a thumbnail so that we can
         # serve it up when needed without having to rescale on the fly
-        if content.content_type is not None and content.content_type.split('/')[0] == 'image':
-            try:
+        try:
+            if is_svg:
+                # for svg simply store the provided svg file, since vector graphics should be good enough
+                # for downscaling client-side
+                if tempfile_path is None:
+                    thumbnail_file = StringIO.StringIO(content.data)
+                else:
+                    with open(tempfile_path) as f:
+                        thumbnail_file = StringIO.StringIO(f.read())
+                thumbnail_content = StaticContent(thumbnail_file_location, thumbnail_name,
+                                                  'image/svg+xml', thumbnail_file)
+                self.save(thumbnail_content)
+            elif content.content_type is not None and content.content_type.split('/')[0] == 'image':
                 # use PIL to do the thumbnail generation (http://www.pythonware.com/products/pil/)
                 # My understanding is that PIL will maintain aspect ratios while restricting
                 # the max-height/width to be whatever you pass in as 'size'
                 # @todo: move the thumbnail size to a configuration setting?!?
                 if tempfile_path is None:
-                    im = Image.open(StringIO.StringIO(content.data))
+                    source = StringIO.StringIO(content.data)
                 else:
-                    im = Image.open(tempfile_path)
+                    source = tempfile_path
 
-                # I've seen some exceptions from the PIL library when trying to save palletted
-                # PNG files to JPEG. Per the google-universe, they suggest converting to RGB first.
-                im = im.convert('RGB')
+                # We use the context manager here to avoid leaking the inner file descriptor
+                # of the Image object -- this way it gets closed after we're done with using it.
+                thumbnail_file = StringIO.StringIO()
+                with Image.open(source) as image:
+                    # I've seen some exceptions from the PIL library when trying to save palletted
+                    # PNG files to JPEG. Per the google-universe, they suggest converting to RGB first.
+                    thumbnail_image = image.convert('RGB')
 
-                if not dimensions:
-                    dimensions = (128, 128)
+                    if not dimensions:
+                        dimensions = (128, 128)
 
-                im.thumbnail(dimensions, Image.ANTIALIAS)
-                thumbnail_file = StringIO.StringIO()
-                im.save(thumbnail_file, 'JPEG')
-                thumbnail_file.seek(0)
+                    thumbnail_image.thumbnail(dimensions, Image.ANTIALIAS)
+                    thumbnail_image.save(thumbnail_file, 'JPEG')
+                    thumbnail_file.seek(0)
 
                 # store this thumbnail as any other piece of content
                 thumbnail_content = StaticContent(thumbnail_file_location, thumbnail_name,
@@ -369,9 +388,11 @@ class ContentStore(object):
 
                 self.save(thumbnail_content)
 
-            except Exception, e:
-                # log and continue as thumbnails are generally considered as optional
-                logging.exception(u"Failed to generate thumbnail for {0}. Exception: {1}".format(content.location, str(e)))
+        except Exception, exc:  # pylint: disable=broad-except
+            # log and continue as thumbnails are generally considered as optional
+            logging.exception(
+                u"Failed to generate thumbnail for {0}. Exception: {1}".format(content.location, str(exc))
+            )
 
         return thumbnail_content, thumbnail_file_location
 

common/lib/xmodule/xmodule/tests/test_content.py

@@ -3,6 +3,7 @@
 import os
 import unittest
 import ddt
+from mock import Mock, patch
 from path import Path as path
 
 from xmodule.contentstore.content import StaticContent, StaticContentStream
@@ -58,6 +59,7 @@ class Content(object):
     def __init__(self, location, content_type):
         self.location = location
         self.content_type = content_type
+        self.data = None
 
 
 class FakeGridFsItem(object):
@@ -84,6 +86,17 @@ class FakeGridFsItem(object):
         return chunk
 
 
+class MockImage(Mock):
+    """
+    This class pretends to be PIL.Image for purposes of thumbnails testing.
+    """
+    def __enter__(self):
+        return self
+
+    def __exit__(self, *args):
+        self.close()
+
+
 @ddt.ddt
 class ContentTest(unittest.TestCase):
     def test_thumbnail_none(self):
@@ -103,11 +116,43 @@ class ContentTest(unittest.TestCase):
     )
     @ddt.unpack
     def test_generate_thumbnail_image(self, original_filename, thumbnail_filename):
-        contentStore = ContentStore()
+        content_store = ContentStore()
         content = Content(AssetLocation(u'mitX', u'800', u'ignore_run', u'asset', original_filename), None)
-        (thumbnail_content, thumbnail_file_location) = contentStore.generate_thumbnail(content)
+        (thumbnail_content, thumbnail_file_location) = content_store.generate_thumbnail(content)
         self.assertIsNone(thumbnail_content)
-        self.assertEqual(AssetLocation(u'mitX', u'800', u'ignore_run', u'thumbnail', thumbnail_filename), thumbnail_file_location)
+        self.assertEqual(
+            AssetLocation(u'mitX', u'800', u'ignore_run', u'thumbnail', thumbnail_filename),
+            thumbnail_file_location
+        )
+
+    @patch('xmodule.contentstore.content.Image')
+    def test_image_is_closed_when_generating_thumbnail(self, image_class_mock):
+        # We used to keep the Image's file descriptor open when generating a thumbnail.
+        # It should be closed after being used.
+        mock_image = MockImage()
+        image_class_mock.open.return_value = mock_image
+
+        content_store = ContentStore()
+        content = Content(AssetLocation(u'mitX', u'800', u'ignore_run', u'asset', "monsters.jpg"), "image/jpeg")
+        content.data = 'mock data'
+        content_store.generate_thumbnail(content)
+        self.assertTrue(image_class_mock.open.called, "Image.open not called")
+        self.assertTrue(mock_image.close.called, "mock_image.close not called")
+
+    def test_store_svg_as_thumbnail(self):
+        # We had a bug that caused generate_thumbnail to attempt to pass SVG to PIL to generate a thumbnail.
+        # SVG files should be stored in original form for thumbnail purposes.
+        content_store = ContentStore()
+        content_store.save = Mock()
+        thumbnail_filename = u'test.svg'
+        content = Content(AssetLocation(u'mitX', u'800', u'ignore_run', u'asset', u'test.svg'), 'image/svg+xml')
+        content.data = 'mock svg file'
+        (thumbnail_content, thumbnail_file_location) = content_store.generate_thumbnail(content)
+        self.assertEqual(thumbnail_content.data.read(), b'mock svg file')
+        self.assertEqual(
+            AssetLocation(u'mitX', u'800', u'ignore_run', u'thumbnail', thumbnail_filename),
+            thumbnail_file_location
+        )
 
     def test_compute_location(self):
         # We had a bug that __ got converted into a single _. Make sure that substitution of INVALID_CHARS (like space)
@@ -115,7 +160,10 @@ class ContentTest(unittest.TestCase):
         asset_location = StaticContent.compute_location(
             SlashSeparatedCourseKey('mitX', '400', 'ignore'), 'subs__1eo_jXvZnE .srt.sjson'
         )
-        self.assertEqual(AssetLocation(u'mitX', u'400', u'ignore', u'asset', u'subs__1eo_jXvZnE_.srt.sjson', None), asset_location)
+        self.assertEqual(
+            AssetLocation(u'mitX', u'400', u'ignore', u'asset', u'subs__1eo_jXvZnE_.srt.sjson', None),
+            asset_location
+        )
 
     def test_get_location_from_path(self):
         asset_location = StaticContent.get_location_from_path(u'/c4x/a/b/asset/images_course_image.jpg')