Merge branch 'remote-videos'

* remote-videos:
  Support remote file storages
  Set an explicit ordering on Videos
  Move w.utils.ffmpeg_installed to w.ffmpeg.installed
  Fix long filename trimming logic

CHANGELOG.rst

 CHANGELOG
 =========
 
+IN DEVELOPMENT
+--------------
+
+- Support remote storage engines, like AWS S3.
+
 0.3.2
 -----
 

tests/app/settings.py

@@ -71,6 +71,8 @@ TEMPLATES = [
     },
 ]
 
+DEFAULT_FILE_STORAGE = 'tests.storage.RemoteStorage'
+
 STATIC_ROOT = os.path.join(os.path.dirname(__file__), 'static')
 STATIC_URL = '/static/'
 

tests/storage.py

+import errno
+import os
+import warnings
+from datetime import datetime
+
+from django.conf import settings
+from django.core.files import File, locks
+from django.core.files.move import file_move_safe
+from django.core.files.storage import FileSystemStorage
+from django.utils import timezone
+from django.utils._os import safe_join
+from django.utils.deconstruct import deconstructible
+from django.utils.deprecation import RemovedInDjango20Warning
+from django.utils.encoding import filepath_to_uri, force_text
+from django.utils.six.moves.urllib.parse import urljoin
+
+
+@deconstructible
+class RemoteStorage(FileSystemStorage):
+    """
+    A "remote" storage that does not support the ``path`` method.
+
+    This is just FileSystemStorage with ``path`` renamed ``_path``.
+    """
+
+    def path(self, filename):
+        raise NotImplementedError("This backend doesn't support absolute paths.")
+
+    def _open(self, name, mode='rb'):
+        return File(open(self._path(name), mode))
+
+    def _save(self, name, content):
+        full_path = self._path(name)
+
+        # Create any intermediate directories that do not exist.
+        # Note that there is a race between os.path.exists and os.makedirs:
+        # if os.makedirs fails with EEXIST, the directory was created
+        # concurrently, and we can continue normally. Refs #16082.
+        directory = os.path.dirname(full_path)
+        if not os.path.exists(directory):
+            try:
+                if self.directory_permissions_mode is not None:
+                    # os.makedirs applies the global umask, so we reset it,
+                    # for consistency with file_permissions_mode behavior.
+                    old_umask = os.umask(0)
+                    try:
+                        os.makedirs(directory, self.directory_permissions_mode)
+                    finally:
+                        os.umask(old_umask)
+                else:
+                    os.makedirs(directory)
+            except OSError as e:
+                if e.errno != errno.EEXIST:
+                    raise
+        if not os.path.isdir(directory):
+            raise IOError("%s exists and is not a directory." % directory)
+
+        # There's a potential race condition between get_available_name and
+        # saving the file; it's possible that two threads might return the
+        # same name, at which point all sorts of fun happens. So we need to
+        # try to create the file, but if it already exists we have to go back
+        # to get_available_name() and try again.
+
+        while True:
+            try:
+                # This file has a file path that we can move.
+                if hasattr(content, 'temporary_file_path'):
+                    file_move_safe(content.temporary_file_path(), full_path)
+
+                # This is a normal uploadedfile that we can stream.
+                else:
+                    # This fun binary flag incantation makes os.open throw an
+                    # OSError if the file already exists before we open it.
+                    flags = (os.O_WRONLY | os.O_CREAT | os.O_EXCL |
+                             getattr(os, 'O_BINARY', 0))
+                    # The current umask value is masked out by os.open!
+                    fd = os.open(full_path, flags, 0o666)
+                    _file = None
+                    try:
+                        locks.lock(fd, locks.LOCK_EX)
+                        for chunk in content.chunks():
+                            if _file is None:
+                                mode = 'wb' if isinstance(chunk, bytes) else 'wt'
+                                _file = os.fdopen(fd, mode)
+                            _file.write(chunk)
+                    finally:
+                        locks.unlock(fd)
+                        if _file is not None:
+                            _file.close()
+                        else:
+                            os.close(fd)
+            except OSError as e:
+                if e.errno == errno.EEXIST:
+                    # Ooops, the file exists. We need a new file name.
+                    name = self.get_available_name(name)
+                    full_path = self._path(name)
+                else:
+                    raise
+            else:
+                # OK, the file save worked. Break out of the loop.
+                break
+
+        if self.file_permissions_mode is not None:
+            os.chmod(full_path, self.file_permissions_mode)
+
+        # Store filenames with forward slashes, even on Windows.
+        return force_text(name.replace('\\', '/'))
+
+    def delete(self, name):
+        assert name, "The name argument is not allowed to be empty."
+        name = self._path(name)
+        # If the file exists, delete it from the filesystem.
+        # If os.remove() fails with ENOENT, the file may have been removed
+        # concurrently, and it's safe to continue normally.
+        try:
+            os.remove(name)
+        except OSError as e:
+            if e.errno != errno.ENOENT:
+                raise
+
+    def exists(self, name):
+        return os.path.exists(self._path(name))
+
+    def listdir(self, path):
+        path = self._path(path)
+        directories, files = [], []
+        for entry in os.listdir(path):
+            if os.path.isdir(os.path.join(path, entry)):
+                directories.append(entry)
+            else:
+                files.append(entry)
+        return directories, files
+
+    def _path(self, name):
+        return safe_join(self.location, name)
+
+    def size(self, name):
+        return os.path.getsize(self._path(name))
+
+    def url(self, name):
+        if self.base_url is None:
+            raise ValueError("This file is not accessible via a URL.")
+        url = filepath_to_uri(name)
+        if url is not None:
+            url = url.lstrip('/')
+        return urljoin(self.base_url, url)
+
+    def accessed_time(self, name):
+        warnings.warn(
+            'FileSystemStorage.accessed_time() is deprecated in favor of '
+            'get_accessed_time().',
+            RemovedInDjango20Warning,
+            stacklevel=2,
+        )
+        return datetime.fromtimestamp(os.path.getatime(self.path(name)))
+
+    def created_time(self, name):
+        warnings.warn(
+            'FileSystemStorage.created_time() is deprecated in favor of '
+            'get_created_time().',
+            RemovedInDjango20Warning,
+            stacklevel=2,
+        )
+        return datetime.fromtimestamp(os.path.getctime(self.path(name)))
+
+    def modified_time(self, name):
+        warnings.warn(
+            'FileSystemStorage.modified_time() is deprecated in favor of '
+            'get_modified_time().',
+            RemovedInDjango20Warning,
+            stacklevel=2,
+        )
+        return datetime.fromtimestamp(os.path.getmtime(self.path(name)))
+
+    def _datetime_from_timestamp(self, ts):
+        """
+        If timezone support is enabled, make an aware datetime object in UTC;
+        otherwise make a naive one in the local timezone.
+        """
+        if settings.USE_TZ:
+            # Safe to use .replace() because UTC doesn't have DST
+            return datetime.utcfromtimestamp(ts).replace(tzinfo=timezone.utc)
+        else:
+            return datetime.fromtimestamp(ts)
+
+    def get_accessed_time(self, name):
+        return self._datetime_from_timestamp(os.path.getatime(self.path(name)))
+
+    def get_created_time(self, name):
+        return self._datetime_from_timestamp(os.path.getctime(self.path(name)))
+
+    def get_modified_time(self, name):
+        return self._datetime_from_timestamp(os.path.getmtime(self.path(name)))

tests/test_admin_views.py

@@ -106,7 +106,7 @@ class TestVideoAddView(TestCase, WagtailTestUtils):
         root_collection = Collection.get_first_root_node()
         self.assertEqual(video.collection, root_collection)
 
-    @patch('wagtailvideos.models.ffmpeg_installed')
+    @patch('wagtailvideos.ffmpeg.installed')
     def test_add_no_ffmpeg(self, ffmpeg_installed):
         ffmpeg_installed.return_value = False
 
@@ -163,6 +163,21 @@ class TestVideoAddView(TestCase, WagtailTestUtils):
             )
         )
 
+    def test_add_too_long_filename(self):
+        video_file = create_test_video_file()
+
+        name = 'a_very_long_filename_' + ('x' * 100) + '.mp4'
+        response = self.post({
+            'title': "Test video",
+            'file': SimpleUploadedFile(name, video_file.read(), "video/mp4"),
+        })
+
+        # Should be valid
+        self.assertEqual(response.status_code, 302)
+        video = Video.objects.get()
+
+        self.assertEqual(len(video.file.name), Video._meta.get_field('file').max_length)
+
     def test_add_with_collections(self):
         root_collection = Collection.get_first_root_node()
         evil_plans_collection = root_collection.add_child(name="Evil plans")

wagtailvideos/apps.py

@@ -3,12 +3,12 @@ from __future__ import absolute_import, print_function, unicode_literals
 from django.apps import AppConfig
 from django.core.checks import Warning, register
 
-from wagtailvideos.utils import ffmpeg_installed
+from wagtailvideos import ffmpeg
 
 
 def ffmpeg_check(app_configs, **kwargs):
     messages = []
-    if not ffmpeg_installed():
+    if not ffmpeg.installed():
         messages.append(
             Warning(
                 'ffmpeg could not be found on your system. Transcoding will be disabled',

wagtailvideos/ffmpeg.py

+from __future__ import absolute_import, print_function, unicode_literals
+
+import datetime
+import logging
+import os
+import re
+import shutil
+import subprocess
+import tempfile
+
+from django.core.files.base import ContentFile
+
+logger = logging.getLogger(__name__)
+
+try:
+    from shutil import which
+except ImportError:
+    from distutils.spawn import find_executable as which
+
+
+def DEVNULL():
+    return open(os.devnull, 'r+b')
+
+
+def installed(path=None):
+    return which('ffmpeg', path=path) is not None
+
+
+def get_duration(file_path):
+    if not installed():
+        raise RuntimeError('ffmpeg is not installed')
+
+    try:
+        show_format = subprocess.check_output(
+            ['ffprobe', file_path, '-show_format', '-v', 'quiet'],
+            stdin=DEVNULL(), stderr=DEVNULL())
+        show_format = show_format.decode("utf-8")
+        # show_format comes out in key=value pairs seperated by newlines
+        duration = re.findall(r'([duration^=]+)=([^=]+)(?:\n|$)', show_format)[0][1]
+        return datetime.timedelta(seconds=float(duration))
+    except subprocess.CalledProcessError:
+        logger.exception("Getting video duration failed")
+        return None
+
+
+def get_thumbnail(file_path):
+    if not installed():
+        raise RuntimeError('ffmpeg is not installed')
+
+    file_name = os.path.basename(file_path)
+    thumb_name = '{}_thumb{}'.format(os.path.splitext(file_name)[0], '.jpg')
+
+    try:
+        output_dir = tempfile.mkdtemp()
+        output_file = os.path.join(output_dir, thumb_name)
+        try:
+            subprocess.check_call([
+                'ffmpeg',
+                '-v', 'quiet',
+                '-itsoffset', '-4',
+                '-i', file_path,
+                '-vcodec', 'mjpeg',
+                '-vframes', '1',
+                '-an', '-f', 'rawvideo',
+                '-s', '320x240',
+                output_file,
+            ], stdin=DEVNULL(), stdout=DEVNULL())
+        except subprocess.CalledProcessError:
+            return None
+        return ContentFile(open(output_file, 'rb').read(), thumb_name)
+    finally:
+        shutil.rmtree(output_dir, ignore_errors=True)

wagtailvideos/models.py

 from __future__ import absolute_import, print_function, unicode_literals
 
-import datetime
 import logging
 import mimetypes
 import os
 import os.path
-import re
 import shutil
 import subprocess
 import tempfile
 import threading
+from contextlib import contextmanager
 
 from django.conf import settings
+from django.core.exceptions import SuspiciousFileOperation
 from django.core.files.base import ContentFile
+from django.core.files.temp import NamedTemporaryFile
 from django.core.urlresolvers import reverse
 from django.db import models
 from django.db.models.signals import post_save, pre_delete
@@ -28,7 +29,7 @@ from wagtail.wagtailcore.models import CollectionMember
 from wagtail.wagtailsearch import index
 from wagtail.wagtailsearch.queryset import SearchableQuerySetMixin
 
-from wagtailvideos.utils import ffmpeg_installed
+from wagtailvideos import ffmpeg
 
 logger = logging.getLogger(__name__)
 
@@ -101,15 +102,9 @@ class AbstractVideo(CollectionMember, index.Indexed, models.Model):
         index.FilterField('uploaded_by_user'),
     ]
 
-    def is_stored_locally(self):
-        """
-        Returns True if the image is hosted on the local filesystem
-        """
-        try:
-            self.file.path
-            return True
-        except NotImplementedError:
-            return False
+    def __init__(self, *args, **kwargs):
+        super(AbstractVideo, self).__init__(*args, **kwargs)
+        self._initial_file = self.file
 
     def get_file_size(self):
         if self.file_size is None:
@@ -126,12 +121,18 @@ class AbstractVideo(CollectionMember, index.Indexed, models.Model):
     def get_upload_to(self, filename):
         folder_name = 'original_videos'
         filename = self.file.field.storage.get_valid_name(filename)
+        max_length = self._meta.get_field('file').max_length
 
         # Truncate filename so it fits in the 100 character limit
         # https://code.djangoproject.com/ticket/9893
-        while len(os.path.join(folder_name, filename)) >= 95:
-            prefix, dot, extension = filename.rpartition('.')
-            filename = prefix[:-1] + dot + extension
+        file_path = os.path.join(folder_name, filename)
+        too_long = len(file_path) - max_length
+        if too_long > 0:
+            head, ext = os.path.splitext(filename)
+            if too_long > len(head) + 1:
+                raise SuspiciousFileOperation('File name can not be shortened to a safe length')
+            filename = head[:-too_long] + ext
+            file_path = os.path.join(folder_name, filename)
         return os.path.join(folder_name, filename)
 
     def get_usage(self):
@@ -152,57 +153,6 @@ class AbstractVideo(CollectionMember, index.Indexed, models.Model):
     def __str__(self):
         return self.title
 
-    def get_duration(self):
-        if self.duration:
-            return self.duration
-
-        if not ffmpeg_installed():
-            return None
-
-        file_path = self.file.path
-        try:
-            # FIXME prints out extra stuff on travis, pip stderr to dev/null
-            show_format = subprocess.check_output(['ffprobe', file_path, '-show_format', '-v', 'quiet'])
-            show_format = show_format.decode("utf-8")
-            # show_format comes out in key=value pairs seperated by newlines
-            duration = re.findall(r'([duration^=]+)=([^=]+)(?:\n|$)', show_format)[0][1]
-            return datetime.timedelta(seconds=float(duration))
-        except subprocess.CalledProcessError:
-            logger.exception("Getting video duration failed")
-            return None
-
-    def get_thumbnail(self):
-        if self.thumbnail:
-            return self.thumbnail
-
-        if not ffmpeg_installed():
-            return None
-
-        file_path = self.file.path
-        file_name = self.filename(include_ext=False) + '_thumb.jpg'
-
-        try:
-            output_dir = tempfile.mkdtemp()
-            output_file = os.path.join(output_dir, file_name)
-            try:
-                FNULL = open(os.devnull, 'r')
-                subprocess.check_call([
-                    'ffmpeg',
-                    '-v', 'quiet',
-                    '-itsoffset', '-4',
-                    '-i', file_path,
-                    '-vcodec', 'mjpeg',
-                    '-vframes', '1',
-                    '-an', '-f', 'rawvideo',
-                    '-s', '320x240',
-                    output_file,
-                ], stdin=FNULL)
-            except subprocess.CalledProcessError:
-                return None
-            return ContentFile(open(output_file, 'rb').read(), file_name)
-        finally:
-            shutil.rmtree(output_dir, ignore_errors=True)
-
     def save(self, **kwargs):
         super(AbstractVideo, self).save(**kwargs)
 
@@ -273,6 +223,7 @@ class AbstractVideo(CollectionMember, index.Indexed, models.Model):
 
     class Meta:
         abstract = True
+        ordering = ['-created_at']
 
 
 class Video(AbstractVideo):
@@ -339,6 +290,29 @@ class TranscodingThread(threading.Thread):
             shutil.rmtree(output_dir, ignore_errors=True)
 
 
+@contextmanager
+def get_local_file(file):
+    """
+    Get a local version of the file, downloading it from the remote storage if
+    required. The returned value should be used as a context manager to
+    ensure any temporary files are cleaned up afterwards.
+    """
+    try:
+        with open(file.path):
+            yield file.path
+    except NotImplementedError:
+        _, ext = os.path.splitext(file.name)
+        with NamedTemporaryFile(prefix='wagtailvideo-', suffix=ext) as tmp:
+            try:
+                file.open('rb')
+                for chunk in file.chunks():
+                    tmp.write(chunk)
+            finally:
+                file.close()
+            tmp.flush()
+            yield tmp.name
+
+
 # Delete files when model is deleted
 @receiver(pre_delete, sender=Video)
 def video_delete(sender, instance, **kwargs):
@@ -349,10 +323,22 @@ def video_delete(sender, instance, **kwargs):
 # Fields that need the actual video file to create
 @receiver(post_save, sender=Video)
 def video_saved(sender, instance, **kwargs):
+    if not ffmpeg.installed():
+        return
+
     if hasattr(instance, '_from_signal'):
         return
-    instance.thumbnail = instance.get_thumbnail()
-    instance.duration = instance.get_duration()
+
+    has_changed = instance._initial_file is not instance.file
+    filled_out = instance.thumbnail is not None and instance.duration is not None
+    if has_changed or not filled_out:
+        with get_local_file(instance.file) as file_path:
+            if has_changed or instance.thumbnail is None:
+                instance.thumbnail = ffmpeg.get_thumbnail(file_path)
+
+            if has_changed or instance.duration is None:
+                instance.duration = ffmpeg.get_duration(file_path)
+
     instance.file_size = instance.file.size
     instance._from_signal = True
     instance.save()

wagtailvideos/utils.py

-from __future__ import absolute_import, print_function, unicode_literals
-
-try:
-    from shutil import which
-except ImportError:
-    from distutils.spawn import find_executable as which
-
-
-def ffmpeg_installed(path=None):
-    return which('ffmpeg', path=path) is not None

wagtailvideos/views/videos.py

 from __future__ import absolute_import, print_function, unicode_literals
 
-import os
-
 from django.core.urlresolvers import reverse
 from django.http import HttpResponseNotAllowed
 from django.shortcuts import get_object_or_404, redirect, render
@@ -15,10 +13,10 @@ from wagtail.wagtailadmin.utils import (
 from wagtail.wagtailcore.models import Collection
 from wagtail.wagtailsearch.backends import get_search_backends
 
+from wagtailvideos import ffmpeg
 from wagtailvideos.forms import VideoTranscodeAdminForm, get_video_form
 from wagtailvideos.models import Video
 from wagtailvideos.permissions import permission_policy
-from wagtailvideos.utils import ffmpeg_installed
 
 permission_checker = PermissionPolicyChecker(permission_policy)
 
@@ -107,20 +105,19 @@ def edit(request, video_id):
     else:
         form = VideoForm(instance=video)
 
-    if video.is_stored_locally():
+    if not video._meta.get_field('file').storage.exists(video.file.name):
         # Give error if image file doesn't exist
-        if not os.path.isfile(video.file.path):
-            messages.error(request, _(
-                "The source video file could not be found. Please change the source or delete the video."
-            ).format(video.title), buttons=[
-                messages.button(reverse('wagtailvideos:delete', args=(video.id,)), _('Delete'))
-            ])
+        messages.error(request, _(
+            "The source video file could not be found. Please change the source or delete the video."
+        ).format(video.title), buttons=[
+            messages.button(reverse('wagtailvideos:delete', args=(video.id,)), _('Delete'))
+        ])
 
     return render(request, "wagtailvideos/videos/edit.html", {
         'video': video,
         'form': form,
         'filesize': video.get_file_size(),
-        'can_transcode': ffmpeg_installed(),
+        'can_transcode': ffmpeg.installed(),
         'transcodes': video.transcodes.all(),
         'transcode_form': VideoTranscodeAdminForm(video=video),
         'user_can_delete': permission_policy.user_has_permission_for_instance(request.user, 'delete', video)