From 870ce11bd622161bca02967afb3b027ef5bd10a5 Mon Sep 17 00:00:00 2001 From: Karl Hobley Date: Thu, 13 Aug 2015 16:04:36 +0100 Subject: [PATCH 1/3] Failing tests for #1397 Testing the following with an external image backend: - Uploading through image chooser (fails) - Visiting the edit page of an image with a null file_size (fails) - Editing an image, changing the image file - Adding a new image The latter two do not fail, I thought I'd add them to reduce risk of regressions in the future --- wagtail/tests/dummy_external_storage.py | 72 +++++++++++++++++++ .../wagtailimages/tests/test_admin_views.py | 62 ++++++++++++++++ 2 files changed, 134 insertions(+) create mode 100644 wagtail/tests/dummy_external_storage.py diff --git a/wagtail/tests/dummy_external_storage.py b/wagtail/tests/dummy_external_storage.py new file mode 100644 index 0000000000..e7412324c3 --- /dev/null +++ b/wagtail/tests/dummy_external_storage.py @@ -0,0 +1,72 @@ +# This file contains a file storage backend that imitates behaviours of +# common external storage backends (S3 boto, libcloud, etc). + +# The following behaviours have been added to this backend: +# - Calling .path on the storage or image file raises NotImplementedError +# - File.open() after the file has been closed raises an error + +from django.core.files.storage import Storage, FileSystemStorage +from django.core.files.base import File +from django.utils.deconstruct import deconstructible + + +@deconstructible +class DummyExternalStorage(Storage): + def __init__(self, *args, **kwargs): + self.wrapped = FileSystemStorage(*args, **kwargs) + + def path(self, name): + # Overridden to give it the behaviour of the base Storage class + # This is what an external storage backend would have + raise NotImplementedError("This backend doesn't support absolute paths.") + + def _open(self, name, mode='rb'): + # Overridden to return a DummyExternalStorageFile instead of a normal + # File object + return DummyExternalStorageFile(open(self.wrapped.path(name), mode)) + + + # Wrap all other functions + + def _save(self, name, content): + return self.wrapped._save(name, content) + + def delete(self, name): + self.wrapped.delete(name) + + def exists(self, name): + return self.wrapped.exists(name) + + def listdir(self, path): + return self.wrapped.listdir(path) + + def size(self, name): + return self.wrapped.size(name) + + def url(self, name): + return self.wrapped.url(name) + + def accessed_time(self, name): + return self.wrapped.accessed_time(name) + + def created_time(self, name): + return self.wrapped.created_time(name) + + def modified_time(self, name): + return self.wrapped.modified_time(name) + + +class DummyExternalStorageFile(File): + def open(self, mode=None): + # Based on: https://github.com/django/django/blob/2c39f282b8389f47fee4b24e785a58567c6c3629/django/core/files/base.py#L135-L141 + + # I've commented out two lines of this function which stops it checking + # the filesystem for the file. Making it behave as if it is using an + # external file storage. + + if not self.closed: + self.seek(0) + # elif self.name and os.path.exists(self.name): + # self.file = open(self.name, mode or self.mode) + else: + raise ValueError("The file cannot be reopened.") diff --git a/wagtail/wagtailimages/tests/test_admin_views.py b/wagtail/wagtailimages/tests/test_admin_views.py index 4e215bb892..5205bee228 100644 --- a/wagtail/wagtailimages/tests/test_admin_views.py +++ b/wagtail/wagtailimages/tests/test_admin_views.py @@ -1,6 +1,7 @@ from __future__ import unicode_literals import json +import unittest from django.test import TestCase, override_settings from django.utils.http import urlquote @@ -89,6 +90,19 @@ class TestImageAddView(TestCase, WagtailTestUtils): # Test that the file_size field was set self.assertTrue(image.file_size) + @override_settings(DEFAULT_FILE_STORAGE='wagtail.tests.dummy_external_storage.DummyExternalStorage') + def test_add_with_external_file_storage(self): + response = self.post({ + 'title': "Test image", + 'file': SimpleUploadedFile('test.png', get_test_image_file().file.getvalue()), + }) + + # Should redirect back to index + self.assertRedirects(response, reverse('wagtailimages:index')) + + # Check that the image was created + self.assertTrue(Image.objects.filter(title="Test image").exists()) + def test_add_no_file_selected(self): response = self.post({ 'title': "Test image", @@ -142,6 +156,20 @@ class TestImageEditView(TestCase, WagtailTestUtils): self.assertEqual(response.status_code, 200) self.assertTemplateUsed(response, 'wagtailimages/images/edit.html') + @unittest.expectedFailure + @override_settings(DEFAULT_FILE_STORAGE='wagtail.tests.dummy_external_storage.DummyExternalStorage') + def test_simple_with_external_storage(self): + # The view calls get_file_size on the image that closes the file if + # file_size wasn't prevously populated. + + # The view then attempts to reopen the file when rendering the template + # which caused crashes when certian storage backends were in use. + # See #1397 + + response = self.get() + self.assertEqual(response.status_code, 200) + self.assertTemplateUsed(response, 'wagtailimages/images/edit.html') + def test_edit(self): response = self.post({ 'title': "Edited", @@ -173,6 +201,26 @@ class TestImageEditView(TestCase, WagtailTestUtils): image = Image.objects.get(id=self.image.id) self.assertNotEqual(image.file_size, 100000) + @override_settings(DEFAULT_FILE_STORAGE='wagtail.tests.dummy_external_storage.DummyExternalStorage') + def test_edit_with_new_image_file_and_external_storage(self): + file_content = get_test_image_file().file.getvalue() + + # Change the file size of the image + self.image.file_size = 100000 + self.image.save() + + response = self.post({ + 'title': "Edited", + 'file': SimpleUploadedFile('new.png', file_content), + }) + + # Should redirect back to index + self.assertRedirects(response, reverse('wagtailimages:index')) + + # Check that the image file size changed (assume it changed to the correct value) + image = Image.objects.get(id=self.image.id) + self.assertNotEqual(image.file_size, 100000) + def test_with_missing_image_file(self): self.image.file.delete(False) @@ -304,6 +352,20 @@ class TestImageChooserUploadView(TestCase, WagtailTestUtils): # The form should have an error self.assertFormError(response, 'uploadform', 'file', "This field is required.") + @unittest.expectedFailure + @override_settings(DEFAULT_FILE_STORAGE='wagtail.tests.dummy_external_storage.DummyExternalStorage') + def test_upload_with_external_storage(self): + response = self.client.post(reverse('wagtailimages:chooser_upload'), { + 'title': "Test image", + 'file': SimpleUploadedFile('test.png', get_test_image_file().file.getvalue()), + }) + + # Check response + self.assertEqual(response.status_code, 200) + + # Check that the image was created + self.assertTrue(Image.objects.filter(title="Test image").exists()) + class TestMultipleImageUploader(TestCase, WagtailTestUtils): """ From 20036e5b5f1b2fc5ab1a46623c9035fb052ea890 Mon Sep 17 00:00:00 2001 From: Karl Hobley Date: Thu, 13 Aug 2015 16:58:47 +0100 Subject: [PATCH 2/3] Implemented Image.is_stored_locally helper function --- wagtail/wagtailimages/models.py | 11 +++++++++++ wagtail/wagtailimages/tests/test_models.py | 7 +++++++ wagtail/wagtailimages/views/images.py | 10 ++-------- 3 files changed, 20 insertions(+), 8 deletions(-) diff --git a/wagtail/wagtailimages/models.py b/wagtail/wagtailimages/models.py index 032a6cfff6..a4737175ac 100644 --- a/wagtail/wagtailimages/models.py +++ b/wagtail/wagtailimages/models.py @@ -75,6 +75,17 @@ class AbstractImage(models.Model, TagSearchable): file_size = models.PositiveIntegerField(null=True, editable=False) + 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 get_file_size(self): if self.file_size is None: try: diff --git a/wagtail/wagtailimages/tests/test_models.py b/wagtail/wagtailimages/tests/test_models.py index 63f614bdd1..9bd41dc7bb 100644 --- a/wagtail/wagtailimages/tests/test_models.py +++ b/wagtail/wagtailimages/tests/test_models.py @@ -79,6 +79,13 @@ class TestImage(TestCase): self.assertEqual(self.image.focal_point_width, None) self.assertEqual(self.image.focal_point_height, None) + def test_is_stored_locally(self): + self.assertTrue(self.image.is_stored_locally()) + + @override_settings(DEFAULT_FILE_STORAGE='wagtail.tests.dummy_external_storage.DummyExternalStorage') + def test_is_stored_locally_with_external_storage(self): + self.assertFalse(self.image.is_stored_locally()) + class TestImagePermissions(TestCase): def setUp(self): diff --git a/wagtail/wagtailimages/views/images.py b/wagtail/wagtailimages/views/images.py index 605d80733b..ee9eda62a3 100644 --- a/wagtail/wagtailimages/views/images.py +++ b/wagtail/wagtailimages/views/images.py @@ -123,15 +123,9 @@ def edit(request, image_id): except NoReverseMatch: url_generator_enabled = False - try: - local_path = image.file.path - except NotImplementedError: - # Image is hosted externally (eg, S3) - local_path = None - - if local_path: + if image.is_stored_locally(): # Give error if image file doesn't exist - if not os.path.isfile(local_path): + if not os.path.isfile(image.file.path): messages.error(request, _("The source image file could not be found. Please change the source or delete the image.").format(image.title), buttons=[ messages.button(reverse('wagtailimages:delete', args=(image.id,)), _('Delete')) ]) From 6a9a80a16dac4703e8ac81549f688d7ad17070fe Mon Sep 17 00:00:00 2001 From: Karl Hobley Date: Thu, 13 Aug 2015 16:39:52 +0100 Subject: [PATCH 3/3] Don't reopen files that are from external hosting Fixes #1397 --- wagtail/wagtailimages/models.py | 18 ++++++++++++++---- .../wagtailimages/tests/test_admin_views.py | 3 --- 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/wagtail/wagtailimages/models.py b/wagtail/wagtailimages/models.py index a4737175ac..347bd6bc36 100644 --- a/wagtail/wagtailimages/models.py +++ b/wagtail/wagtailimages/models.py @@ -118,8 +118,18 @@ class AbstractImage(models.Model, TagSearchable): # Open file if it is closed close_file = False try: + image_file = self.file + if self.file.closed: - self.file.open('rb') + # Reopen the file + if self.is_stored_locally(): + self.file.open('rb') + else: + # Some external storage backends don't allow reopening + # the file. Get a fresh file instance. #1397 + storage = self._meta.get_field('file').storage + image_file = storage.open(self.file.name, 'rb') + close_file = True except IOError as e: # re-throw this as a SourceImageIOError so that calling code can distinguish @@ -127,13 +137,13 @@ class AbstractImage(models.Model, TagSearchable): raise SourceImageIOError(text_type(e)) # Seek to beginning - self.file.seek(0) + image_file.seek(0) try: - yield WillowImage.open(self.file) + yield WillowImage.open(image_file) finally: if close_file: - self.file.close() + image_file.close() def get_rect(self): return Rect(0, 0, self.width, self.height) diff --git a/wagtail/wagtailimages/tests/test_admin_views.py b/wagtail/wagtailimages/tests/test_admin_views.py index 5205bee228..593d44c530 100644 --- a/wagtail/wagtailimages/tests/test_admin_views.py +++ b/wagtail/wagtailimages/tests/test_admin_views.py @@ -1,7 +1,6 @@ from __future__ import unicode_literals import json -import unittest from django.test import TestCase, override_settings from django.utils.http import urlquote @@ -156,7 +155,6 @@ class TestImageEditView(TestCase, WagtailTestUtils): self.assertEqual(response.status_code, 200) self.assertTemplateUsed(response, 'wagtailimages/images/edit.html') - @unittest.expectedFailure @override_settings(DEFAULT_FILE_STORAGE='wagtail.tests.dummy_external_storage.DummyExternalStorage') def test_simple_with_external_storage(self): # The view calls get_file_size on the image that closes the file if @@ -352,7 +350,6 @@ class TestImageChooserUploadView(TestCase, WagtailTestUtils): # The form should have an error self.assertFormError(response, 'uploadform', 'file', "This field is required.") - @unittest.expectedFailure @override_settings(DEFAULT_FILE_STORAGE='wagtail.tests.dummy_external_storage.DummyExternalStorage') def test_upload_with_external_storage(self): response = self.client.post(reverse('wagtailimages:chooser_upload'), {