From 24712b4d8b6b5647aa65409512b4e3a6d901b052 Mon Sep 17 00:00:00 2001 From: Andrew Plummer Date: Thu, 12 Apr 2018 12:12:51 +0100 Subject: [PATCH] Images: handle all not found errors in get_file_size --- CHANGELOG.txt | 1 + CONTRIBUTORS.rst | 1 + docs/releases/2.2.rst | 2 ++ wagtail/images/models.py | 10 +++++++--- wagtail/images/tests/test_admin_views.py | 18 ++++++++++++++++++ wagtail/images/tests/test_models.py | 10 ++++++++++ wagtail/images/views/images.py | 9 +++++++-- wagtail/tests/dummy_external_storage.py | 11 +++++++++++ 8 files changed, 57 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.txt b/CHANGELOG.txt index b8bd6ca4e1..c6eb6f3606 100644 --- a/CHANGELOG.txt +++ b/CHANGELOG.txt @@ -6,6 +6,7 @@ Changelog * Added another valid AudioBoom oEmbed pattern (Bertrand Bordage) * Added `annotate_score` support to PostgreSQL search backend (Bertrand Bordage) + * Fix: Handle all exceptions from `Image.get_file_size` (Andrew Plummer) 2.1 (22.05.2018) diff --git a/CONTRIBUTORS.rst b/CONTRIBUTORS.rst index 7f29e58af4..e1f6097ebc 100644 --- a/CONTRIBUTORS.rst +++ b/CONTRIBUTORS.rst @@ -300,6 +300,7 @@ Contributors * Pierre Geier * Jérôme Lebleu * Victor Miti +* Andrew Plummer Translators =========== diff --git a/docs/releases/2.2.rst b/docs/releases/2.2.rst index da4937d327..9fdd4d38a4 100644 --- a/docs/releases/2.2.rst +++ b/docs/releases/2.2.rst @@ -19,5 +19,7 @@ Other features Bug fixes ~~~~~~~~~ + * Handle all exceptions from ``Image.get_file_size`` (Andrew Plummer) + Upgrade considerations ====================== diff --git a/wagtail/images/models.py b/wagtail/images/models.py index dabfeeaff0..912011c0d5 100644 --- a/wagtail/images/models.py +++ b/wagtail/images/models.py @@ -98,9 +98,13 @@ class AbstractImage(CollectionMember, index.Indexed, models.Model): if self.file_size is None: try: self.file_size = self.file.size - except OSError: - # File doesn't exist - return + except Exception as e: + # File not found + # + # Have to catch everything, because the exception + # depends on the file subclass, and therefore the + # storage being used. + raise SourceImageIOError(str(e)) self.save(update_fields=['file_size']) diff --git a/wagtail/images/tests/test_admin_views.py b/wagtail/images/tests/test_admin_views.py index 37a8e3f1bd..a87b6a57b4 100644 --- a/wagtail/images/tests/test_admin_views.py +++ b/wagtail/images/tests/test_admin_views.py @@ -373,6 +373,24 @@ class TestImageEditView(TestCase, WagtailTestUtils): self.assertEqual(response.status_code, 200) self.assertTemplateUsed(response, 'wagtailimages/images/edit.html') + def check_get_missing_file_displays_warning(self): + # Need to recreate image to use a custom storage per test. + image = Image.objects.create(title="Test image", file=get_test_image_file()) + image.file.storage.delete(image.file.name) + + response = self.client.get(reverse('wagtailimages:edit', args=(image.pk,))) + self.assertEqual(response.status_code, 200) + self.assertTemplateUsed(response, 'wagtailimages/images/edit.html') + self.assertContains(response, "File not found") + + def test_get_missing_file_displays_warning_with_default_storage(self): + self.check_get_missing_file_displays_warning() + + @override_settings(DEFAULT_FILE_STORAGE='wagtail.tests.dummy_external_storage.DummyExternalStorage') + def test_get_missing_file_displays_warning_with_custom_storage(self): + self.check_get_missing_file_displays_warning() + + def get_content(self, f=None): if f is None: f = self.image.file diff --git a/wagtail/images/tests/test_models.py b/wagtail/images/tests/test_models.py index b0cea61fa0..96fd07bbce 100644 --- a/wagtail/images/tests/test_models.py +++ b/wagtail/images/tests/test_models.py @@ -85,6 +85,16 @@ class TestImage(TestCase): def test_is_stored_locally_with_external_storage(self): self.assertFalse(self.image.is_stored_locally()) + def test_get_file_size(self): + file_size = self.image.get_file_size() + self.assertIsInstance(file_size, int) + self.assertGreater(file_size, 0) + + def test_get_file_size_on_missing_file_raises_sourceimageioerror(self): + self.image.file.delete(save=False) + with self.assertRaises(SourceImageIOError): + self.image.get_file_size() + class TestImageQuerySet(TestCase): def test_search_method(self): diff --git a/wagtail/images/views/images.py b/wagtail/images/views/images.py index 049c501711..5fcc6b5fd6 100644 --- a/wagtail/images/views/images.py +++ b/wagtail/images/views/images.py @@ -14,7 +14,7 @@ from wagtail.core.models import Collection, Site from wagtail.images import get_image_model from wagtail.images.exceptions import InvalidFilterSpecError from wagtail.images.forms import URLGeneratorForm, get_image_form -from wagtail.images.models import Filter +from wagtail.images.models import Filter, SourceImageIOError from wagtail.images.permissions import permission_policy from wagtail.images.views.serve import generate_signature from wagtail.search import index as search_index @@ -138,11 +138,16 @@ def edit(request, image_id): messages.button(reverse('wagtailimages:delete', args=(image.id,)), _('Delete')) ]) + try: + filesize = image.get_file_size() + except SourceImageIOError: + filesize = None + return render(request, "wagtailimages/images/edit.html", { 'image': image, 'form': form, 'url_generator_enabled': url_generator_enabled, - 'filesize': image.get_file_size(), + 'filesize': filesize, 'user_can_delete': permission_policy.user_has_permission_for_instance( request.user, 'delete', image ), diff --git a/wagtail/tests/dummy_external_storage.py b/wagtail/tests/dummy_external_storage.py index 11bbc9365f..bddf217819 100644 --- a/wagtail/tests/dummy_external_storage.py +++ b/wagtail/tests/dummy_external_storage.py @@ -4,6 +4,7 @@ # 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 +# - File.size exceptions raise DummyExternalStorageError from django.core.files.base import File from django.core.files.storage import FileSystemStorage, Storage @@ -56,6 +57,10 @@ class DummyExternalStorage(Storage): return self.wrapped.modified_time(name) +class DummyExternalStorageError(Exception): + pass + + class DummyExternalStorageFile(File): def open(self, mode=None): # Based on: @@ -71,3 +76,9 @@ class DummyExternalStorageFile(File): # self.file = open(self.name, mode or self.mode) else: raise ValueError("The file cannot be reopened.") + + def size(self): + try: + return super().size + except Exception as e: + raise DummyExternalStorageError(str(e))