Merge remote-tracking branch 'remotes/bretts/bug261-resized-filenames'

mediagoblin/media_types/image/processing.py

@@ -19,10 +19,40 @@ import os
 
 from mediagoblin import mg_globals as mgg
 from mediagoblin.processing import BadMediaFail, \
-    create_pub_filepath, THUMB_SIZE, MEDIUM_SIZE
+    create_pub_filepath, THUMB_SIZE, MEDIUM_SIZE, FilenameBuilder
 from mediagoblin.tools.exif import exif_fix_image_orientation, \
     extract_exif, clean_exif, get_gps_data, get_useful
 
+def resize_image(entry, filename, new_path, exif_tags, workdir, new_size,
+                 size_limits=(0, 0)):
+    """Store a resized version of an image and return its pathname.
+
+    Arguments:
+    entry -- the entry for the image to resize
+    filename -- the filename of the original image being resized
+    new_path -- public file path for the new resized image
+    exif_tags -- EXIF data for the original image
+    workdir -- directory path for storing converted image files
+    new_size -- 2-tuple size for the resized image
+    size_limits (optional) -- image is only resized if it exceeds this size
+
+    """
+    try:
+        resized = Image.open(filename)
+    except IOError:
+        raise BadMediaFail()
+    resized = exif_fix_image_orientation(resized, exif_tags)  # Fix orientation
+
+    if ((resized.size[0] > size_limits[0]) or
+        (resized.size[1] > size_limits[1])):
+        resized.thumbnail(new_size, Image.ANTIALIAS)
+
+    # Copy the new file to the conversion subdir, then remotely.
+    tmp_resized_filename = os.path.join(workdir, new_path[-1])
+    with file(tmp_resized_filename, 'w') as resized_file:
+        resized.save(resized_file)
+    mgg.public_store.copy_local_to_storage(tmp_resized_filename, new_path)
+
 def process_image(entry):
     """
     Code to process an image
@@ -37,67 +67,33 @@ def process_image(entry):
     queued_filename = workbench.localized_file(
         mgg.queue_store, queued_filepath,
         'source')
-
+    name_builder = FilenameBuilder(queued_filename)
-    filename_bits = os.path.splitext(queued_filename)
-    basename = os.path.split(filename_bits[0])[1]
-    extension = filename_bits[1].lower()
 
     # EXIF extraction
     exif_tags = extract_exif(queued_filename)
     gps_data = get_gps_data(exif_tags)
 
-    try:
+    # Always create a small thumbnail
-        thumb = Image.open(queued_filename)
+    thumb_filepath = create_pub_filepath(
-    except IOError:
+        entry, name_builder.fill('{basename}.thumbnail{ext}'))
-        raise BadMediaFail()
+    resize_image(entry, queued_filename, thumb_filepath,
-
+                 exif_tags, conversions_subdir, THUMB_SIZE)
-    thumb = exif_fix_image_orientation(thumb, exif_tags)
-
-    thumb.thumbnail(THUMB_SIZE, Image.ANTIALIAS)
-
-    # Copy the thumb to the conversion subdir, then remotely.
-    thumb_filename = 'thumbnail' + extension
-    thumb_filepath = create_pub_filepath(entry, thumb_filename)
-
-    tmp_thumb_filename = os.path.join(
-        conversions_subdir, thumb_filename)
-
-    with file(tmp_thumb_filename, 'w') as thumb_file:
-        thumb.save(thumb_file)
-
-    mgg.public_store.copy_local_to_storage(
-        tmp_thumb_filename, thumb_filepath)
 
     # If the size of the original file exceeds the specified size of a `medium`
-    # file, a `medium.jpg` files is created and later associated with the media
+    # file, a `.medium.jpg` files is created and later associated with the media
     # entry.
-    medium = Image.open(queued_filename)
+    medium_filepath = create_pub_filepath(
-
+        entry, name_builder.fill('{basename}.medium{ext}'))
-    # Fix orientation
+    resize_image(entry, queued_filename, medium_filepath,
-    medium = exif_fix_image_orientation(medium, exif_tags)
+                 exif_tags, conversions_subdir, MEDIUM_SIZE, MEDIUM_SIZE)
-
-    if medium.size[0] > MEDIUM_SIZE[0] or medium.size[1] > MEDIUM_SIZE[1]:
-        medium.thumbnail(MEDIUM_SIZE, Image.ANTIALIAS)
-
-    medium_filename = 'medium' + extension
-    medium_filepath = create_pub_filepath(entry, medium_filename)
-
-    tmp_medium_filename = os.path.join(
-        conversions_subdir, medium_filename)
-
-    with file(tmp_medium_filename, 'w') as medium_file:
-        medium.save(medium_file)
-
-    mgg.public_store.copy_local_to_storage(
-        tmp_medium_filename, medium_filepath)
 
     # we have to re-read because unlike PIL, not everything reads
     # things in string representation :)
     queued_file = file(queued_filename, 'rb')
 
     with queued_file:
-        #create_pub_filepath(entry, queued_filepath[-1])
+        original_filepath = create_pub_filepath(
-        original_filepath = create_pub_filepath(entry, basename + extension) 
+            entry, name_builder.fill('{basename}{ext}') )
 
         with mgg.public_store.get_file(original_filepath, 'wb') \
             as original_file:

mediagoblin/media_types/video/processing.py

@@ -20,7 +20,7 @@ import os
 
 from mediagoblin import mg_globals as mgg
 from mediagoblin.processing import mark_entry_failed, \
-    THUMB_SIZE, MEDIUM_SIZE, create_pub_filepath
+    THUMB_SIZE, MEDIUM_SIZE, create_pub_filepath, FilenameBuilder
 from . import transcoders
 
 logging.basicConfig()
@@ -49,17 +49,13 @@ def process_video(entry):
     queued_filename = workbench.localized_file(
         mgg.queue_store, queued_filepath,
         'source')
+    name_builder = FilenameBuilder(queued_filename)
 
     medium_filepath = create_pub_filepath(
-        entry,
+        entry, name_builder.fill('{basename}-640p.webm'))
-        '{original}-640p.webm'.format(
-            original=os.path.splitext(
-                queued_filepath[-1])[0]  # Select the 
-            ))
 
     thumbnail_filepath = create_pub_filepath(
-        entry, 'thumbnail.jpg')
+        entry, name_builder.fill('{basename}.thumbnail.jpg'))
-
 
     # Create a temporary file for the video destination
     tmp_dst = tempfile.NamedTemporaryFile()

mediagoblin/processing/__init__.py

@@ -15,6 +15,7 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 import logging
+import os
 
 from mediagoblin.db.util import atomic_update
 from mediagoblin import mg_globals as mgg
@@ -33,6 +34,37 @@ def create_pub_filepath(entry, filename):
              unicode(entry._id),
              filename])
 
+class FilenameBuilder(object):
+    """Easily slice and dice filenames.
+
+    Initialize this class with an original file path, then use the fill()
+    method to create new filenames based on the original.
+
+    """
+    MAX_FILENAME_LENGTH = 255  # VFAT's maximum filename length
+
+    def __init__(self, path):
+        """Initialize a builder from an original file path."""
+        self.dirpath, self.basename = os.path.split(path)
+        self.basename, self.ext = os.path.splitext(self.basename)
+        self.ext = self.ext.lower()
+
+    def fill(self, fmtstr):
+        """Build a new filename based on the original.
+
+        The fmtstr argument can include the following:
+        {basename} -- the original basename, with the extension removed
+        {ext} -- the original extension, always lowercase
+
+        If necessary, {basename} will be truncated so the filename does not
+        exceed this class' MAX_FILENAME_LENGTH in length.
+
+        """
+        basename_len = (self.MAX_FILENAME_LENGTH -
+                        len(fmtstr.format(basename='', ext=self.ext)))
+        return fmtstr.format(basename=self.basename[:basename_len],
+                             ext=self.ext)
+
 
 def mark_entry_failed(entry_id, exc):
     """

mediagoblin/tests/test_processing.py

@@ -0,0 +1,20 @@
+#!/usr/bin/env python
+
+from nose.tools import assert_equal, assert_true, assert_false
+
+from mediagoblin import processing
+
+class TestProcessing(object):
+    def run_fill(self, input, format, output=None):
+        builder = processing.FilenameBuilder(input)
+        result = builder.fill(format)
+        if output is None:
+            return result
+        assert_equal(output, result)
+        
+    def test_easy_filename_fill(self):
+        self.run_fill('/home/user/foo.TXT', '{basename}bar{ext}', 'foobar.txt')
+
+    def test_long_filename_fill(self):
+        self.run_fill('{0}.png'.format('A' * 300), 'image-{basename}{ext}',
+                      'image-{0}.png'.format('A' * 245))

mediagoblin/tests/test_submission.py

@@ -15,30 +15,34 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 import urlparse
-import pkg_resources
+import os
 import re
+import time
 
 from nose.tools import assert_equal, assert_true, assert_false
+from pkg_resources import resource_filename
 
 from mediagoblin.tests.tools import setup_fresh_app, get_test_app, \
     fixture_add_user
 from mediagoblin import mg_globals
+from mediagoblin.processing import create_pub_filepath
 from mediagoblin.tools import template, common
 
-GOOD_JPG = pkg_resources.resource_filename(
+def resource(filename):
-  'mediagoblin.tests', 'test_submission/good.jpg')
+    return resource_filename('mediagoblin.tests', 'test_submission/' + filename)
-GOOD_PNG = pkg_resources.resource_filename(
+
-  'mediagoblin.tests', 'test_submission/good.png')
+GOOD_JPG = resource('good.jpg')
-EVIL_FILE = pkg_resources.resource_filename(
+GOOD_PNG = resource('good.png')
-  'mediagoblin.tests', 'test_submission/evil')
+EVIL_FILE = resource('evil')
-EVIL_JPG = pkg_resources.resource_filename(
+EVIL_JPG = resource('evil.jpg')
-  'mediagoblin.tests', 'test_submission/evil.jpg')
+EVIL_PNG = resource('evil.png')
-EVIL_PNG = pkg_resources.resource_filename(
+BIG_BLUE = resource('bigblue.png')
-  'mediagoblin.tests', 'test_submission/evil.png')
 
 GOOD_TAG_STRING = 'yin,yang'
 BAD_TAG_STRING = 'rage,' + 'f' * 26 + 'u' * 26
 
+FORM_CONTEXT = ['mediagoblin/submit/start.html', 'submit_form']
+REQUEST_CONTEXT = ['mediagoblin/user_pages/user.html', 'request']
 
 class TestSubmission:
     def setUp(self):
@@ -61,234 +65,168 @@ class TestSubmission:
     def logout(self):
         self.test_app.get('/auth/logout/')
 
+    def do_post(self, data, *context_keys, **kwargs):
+        url = kwargs.pop('url', '/submit/')
+        do_follow = kwargs.pop('do_follow', False)
+        template.clear_test_template_context()
+        response = self.test_app.post(url, data, **kwargs)
+        if do_follow:
+            response.follow()
+        context_data = template.TEMPLATE_TEST_CONTEXT
+        for key in context_keys:
+            context_data = context_data[key]
+        return response, context_data
+        
+    def upload_data(self, filename):
+        return {'upload_files': [('file', filename)]}
+
+    def check_comments(self, request, media, count):
+        comments = request.db.MediaComment.find({'media_entry': media._id})
+        assert_equal(count, len(list(comments)))
+
     def test_missing_fields(self):
         # Test blank form
         # ---------------
-        template.clear_test_template_context()
+        response, form = self.do_post({}, *FORM_CONTEXT)
-        response = self.test_app.post(
+        assert_equal(form.file.errors, [u'You must provide a file.'])
-            '/submit/', {})
-        context = template.TEMPLATE_TEST_CONTEXT['mediagoblin/submit/start.html']
-        form = context['submit_form']
-        assert form.file.errors == [u'You must provide a file.']
 
         # Test blank file
         # ---------------
-        template.clear_test_template_context()
+        response, form = self.do_post({'title': 'test title'}, *FORM_CONTEXT)
-        response = self.test_app.post(
+        assert_equal(form.file.errors, [u'You must provide a file.'])
-            '/submit/', {
-                'title': 'test title'})
-        context = template.TEMPLATE_TEST_CONTEXT['mediagoblin/submit/start.html']
-        form = context['submit_form']
-        assert form.file.errors == [u'You must provide a file.']
 
+    def check_url(self, response, path):
+        assert_equal(urlparse.urlsplit(response.location)[2], path)
 
-    def test_normal_uploads(self):
+    def check_normal_upload(self, title, filename):
-        # Test JPG
+        response, context = self.do_post({'title': title}, do_follow=True,
-        # --------
+                                         **self.upload_data(filename))
-        template.clear_test_template_context()
+        self.check_url(response, '/u/{0}/'.format(self.test_user.username))
-        response = self.test_app.post(
+        assert_true(context.has_key('mediagoblin/user_pages/user.html'))
-            '/submit/', {
-                'title': 'Normal upload 1'
-                }, upload_files=[(
-                    'file', GOOD_JPG)])
-
-        # User should be redirected
-        response.follow()
-        assert_equal(
-            urlparse.urlsplit(response.location)[2],
-            '/u/chris/')
-        assert template.TEMPLATE_TEST_CONTEXT.has_key(
-            'mediagoblin/user_pages/user.html')
-
         # Make sure the media view is at least reachable, logged in...
-        self.test_app.get('/u/chris/m/normal-upload-1/')
+        url = '/u/{0}/m/{1}/'.format(self.test_user.username,
+                                     title.lower().replace(' ', '-'))
+        self.test_app.get(url)
         # ... and logged out too.
         self.logout()
-        self.test_app.get('/u/chris/m/normal-upload-1/')
+        self.test_app.get(url)
-        # Log back in for the remaining tests.
-        self.login()
 
-        # Test PNG
+    def test_normal_jpg(self):
-        # --------
+        self.check_normal_upload('Normal upload 1', GOOD_JPG)
-        template.clear_test_template_context()
-        response = self.test_app.post(
-            '/submit/', {
-                'title': 'Normal upload 2'
-                }, upload_files=[(
-                    'file', GOOD_PNG)])
 
-        response.follow()
+    def test_normal_png(self):
-        assert_equal(
+        self.check_normal_upload('Normal upload 2', GOOD_PNG)
-            urlparse.urlsplit(response.location)[2],
+
-            '/u/chris/')
+    def check_media(self, request, find_data, count=None):
-        assert template.TEMPLATE_TEST_CONTEXT.has_key(
+        media = request.db.MediaEntry.find(find_data)
-            'mediagoblin/user_pages/user.html')
+        if count is not None:
+            assert_equal(media.count(), count)
+            if count == 0:
+                return
+        return media[0]
 
     def test_tags(self):
         # Good tag string
         # --------
-        template.clear_test_template_context()
+        response, request = self.do_post({'title': 'Balanced Goblin',
-        response = self.test_app.post(
+                                          'tags': GOOD_TAG_STRING},
-            '/submit/', {
+                                         *REQUEST_CONTEXT, do_follow=True,
-                'title': 'Balanced Goblin',
+                                         **self.upload_data(GOOD_JPG))
-                'tags': GOOD_TAG_STRING
+        media = self.check_media(request, {'title': 'Balanced Goblin'}, 1)
-                }, upload_files=[(
-                    'file', GOOD_JPG)])
-
-        # New media entry with correct tags should be created
-        response.follow()
-        context = template.TEMPLATE_TEST_CONTEXT['mediagoblin/user_pages/user.html']
-        request = context['request']
-        media = request.db.MediaEntry.find({'title': 'Balanced Goblin'})[0]
         assert_equal(media.tags,
                      [{'name': u'yin', 'slug': u'yin'},
-                                            {'name': u'yang', 'slug': u'yang'}])
+                      {'name': u'yang', 'slug': u'yang'}])
 
         # Test tags that are too long
         # ---------------
-        template.clear_test_template_context()
+        response, form = self.do_post({'title': 'Balanced Goblin',
-        response = self.test_app.post(
+                                       'tags': BAD_TAG_STRING},
-            '/submit/', {
+                                      *FORM_CONTEXT,
-                'title': 'Balanced Goblin',
+                                      **self.upload_data(GOOD_JPG))
-                'tags': BAD_TAG_STRING
+        assert_equal(form.tags.errors, [
-                }, upload_files=[(
+                u'Tags must be shorter than 50 characters.  ' \
-                    'file', GOOD_JPG)])
+                    'Tags that are too long: ' \
-
+                    'ffffffffffffffffffffffffffuuuuuuuuuuuuuuuuuuuuuuuuuu'])
-        # Too long error should be raised
-        context = template.TEMPLATE_TEST_CONTEXT['mediagoblin/submit/start.html']
-        form = context['submit_form']
-        assert form.tags.errors == [
-            u'Tags must be shorter than 50 characters.  Tags that are too long'\
-             ': ffffffffffffffffffffffffffuuuuuuuuuuuuuuuuuuuuuuuuuu']
 
     def test_delete(self):
-        template.clear_test_template_context()
+        response, request = self.do_post({'title': 'Balanced Goblin'},
-        response = self.test_app.post(
+                                         *REQUEST_CONTEXT, do_follow=True,
-            '/submit/', {
+                                         **self.upload_data(GOOD_JPG))
-                'title': 'Balanced Goblin',
+        media = self.check_media(request, {'title': 'Balanced Goblin'}, 1)
-                }, upload_files=[(
-                    'file', GOOD_JPG)])
-
-        # Post image
-        response.follow()
-
-        request = template.TEMPLATE_TEST_CONTEXT[
-            'mediagoblin/user_pages/user.html']['request']
-
-        media = request.db.MediaEntry.find({'title': 'Balanced Goblin'})[0]
-
-        # Does media entry exist?
-        assert_true(media)
 
         # Add a comment, so we can test for its deletion later.
-        get_comments = lambda: list(
+        self.check_comments(request, media, 0)
-            request.db.MediaComment.find({'media_entry': media._id}))
+        comment_url = request.urlgen(
-        assert_false(get_comments())
+            'mediagoblin.user_pages.media_post_comment',
-        response = self.test_app.post(
+            user=self.test_user.username, media=media._id)
-            request.urlgen('mediagoblin.user_pages.media_post_comment',
+        response = self.do_post({'comment_content': 'i love this test'},
-                           user=self.test_user.username,
+                                url=comment_url, do_follow=True)[0]
-                           media=media._id),
+        self.check_comments(request, media, 1)
-            {'comment_content': 'i love this test'})
-        response.follow()
-        assert_true(get_comments())
 
         # Do not confirm deletion
         # ---------------------------------------------------
-        response = self.test_app.post(
+        delete_url = request.urlgen(
-            request.urlgen('mediagoblin.user_pages.media_confirm_delete',
+            'mediagoblin.user_pages.media_confirm_delete',
-                           # No work: user=media.uploader().username,
+            user=self.test_user.username, media=media._id)
-                           user=self.test_user.username,
+        # Empty data means don't confirm
-                           media=media._id),
+        response = self.do_post({}, do_follow=True, url=delete_url)[0]
-            # no value means no confirm
+        media = self.check_media(request, {'title': 'Balanced Goblin'}, 1)
-            {})
-
-        response.follow()
-
-        request = template.TEMPLATE_TEST_CONTEXT[
-            'mediagoblin/user_pages/user.html']['request']
-
-        media = request.db.MediaEntry.find({'title': 'Balanced Goblin'})[0]
-
-        # Does media entry still exist?
-        assert_true(media)
 
         # Confirm deletion
         # ---------------------------------------------------
-        response = self.test_app.post(
+        response, request = self.do_post({'confirm': 'y'}, *REQUEST_CONTEXT,
-            request.urlgen('mediagoblin.user_pages.media_confirm_delete',
+                                         do_follow=True, url=delete_url)
-                           # No work: user=media.uploader().username,
+        self.check_media(request, {'_id': media._id}, 0)
-                           user=self.test_user.username,
+        self.check_comments(request, media, 0)
-                           media=media._id),
-            {'confirm': 'y'})
 
-        response.follow()
+    def test_evil_file(self):
-
-        request = template.TEMPLATE_TEST_CONTEXT[
-            'mediagoblin/user_pages/user.html']['request']
-
-        # Does media entry still exist?
-        assert_false(
-            request.db.MediaEntry.find(
-                {'_id': media._id}).count())
-
-        # How about the comment?
-        assert_false(get_comments())
-
-    def test_malicious_uploads(self):
         # Test non-suppoerted file with non-supported extension
         # -----------------------------------------------------
-        template.clear_test_template_context()
+        response, form = self.do_post({'title': 'Malicious Upload 1'},
-        response = self.test_app.post(
+                                      *FORM_CONTEXT,
-            '/submit/', {
+                                      **self.upload_data(EVIL_FILE))
-                'title': 'Malicious Upload 1'
+        assert_equal(len(form.file.errors), 1)
-                }, upload_files=[(
+        assert_true(re.match(
-                    'file', EVIL_FILE)])
+                r'^Could not extract any file extension from ".*?"$',
+                str(form.file.errors[0])))
 
-        context = template.TEMPLATE_TEST_CONTEXT['mediagoblin/submit/start.html']
+    def check_false_image(self, title, filename):
-        form = context['submit_form']
+        # NOTE: These images should ultimately fail, but they
-        assert re.match(r'^Could not extract any file extension from ".*?"$', str(form.file.errors[0]))
-        assert len(form.file.errors) == 1
-
-        # NOTE: The following 2 tests will ultimately fail, but they
         #   *will* pass the initial form submission step.  Instead,
         #   they'll be caught as failures during the processing step.
+        response, context = self.do_post({'title': title}, do_follow=True,
+                                         **self.upload_data(filename))
+        self.check_url(response, '/u/{0}/'.format(self.test_user.username))
+        entry = mg_globals.database.MediaEntry.find_one({'title': title})
+        assert_equal(entry.state, 'failed')
+        assert_equal(entry.fail_error, u'mediagoblin.processing:BadMediaFail')
 
+    def test_evil_jpg(self):
         # Test non-supported file with .jpg extension
         # -------------------------------------------
-        template.clear_test_template_context()
+        self.check_false_image('Malicious Upload 2', EVIL_JPG)
-        response = self.test_app.post(
-           '/submit/', {
-               'title': 'Malicious Upload 2'
-               }, upload_files=[(
-                   'file', EVIL_JPG)])
-        response.follow()
-        assert_equal(
-            urlparse.urlsplit(response.location)[2],
-            '/u/chris/')
-
-        entry = mg_globals.database.MediaEntry.find_one(
-            {'title': 'Malicious Upload 2'})
-        assert_equal(entry.state, 'failed')
-        assert_equal(
-            entry.fail_error,
-            u'mediagoblin.processing:BadMediaFail')
 
+    def test_evil_png(self):
         # Test non-supported file with .png extension
         # -------------------------------------------
-        template.clear_test_template_context()
+        self.check_false_image('Malicious Upload 3', EVIL_PNG)
-        response = self.test_app.post(
-           '/submit/', {
-               'title': 'Malicious Upload 3'
-               }, upload_files=[(
-                   'file', EVIL_PNG)])
-        response.follow()
-        assert_equal(
-            urlparse.urlsplit(response.location)[2],
-            '/u/chris/')
 
-        entry = mg_globals.database.MediaEntry.find_one(
+    def test_processing(self):
-            {'title': 'Malicious Upload 3'})
+        data = {'title': 'Big Blue'}
-        assert_equal(entry.state, 'failed')
+        response, request = self.do_post(data, *REQUEST_CONTEXT, do_follow=True,
-        assert_equal(
+                                         **self.upload_data(BIG_BLUE))
-            entry.fail_error,
+        media = self.check_media(request, data, 1)
-            u'mediagoblin.processing:BadMediaFail')
+        last_size = 1024 ** 3  # Needs to be larger than bigblue.png
+        for key, basename in (('original', 'bigblue.png'),
+                              ('medium', 'bigblue.medium.png'),
+                              ('thumb', 'bigblue.thumbnail.png')):
+            # Does the processed image have a good filename?
+            filename = resource_filename(
+                'mediagoblin.tests',
+                os.path.join('test_user_dev/media/public',
+                             *media['media_files'].get(key, [])))
+            assert_true(filename.endswith('_' + basename))
+            # Is it smaller than the last processed image we looked at?
+            size = os.stat(filename).st_size
+            assert_true(last_size > size)
+            last_size = size