From ffec059c121cabda64610cf2f4a3a67cff89af75 Mon Sep 17 00:00:00 2001 From: Karl Hobley Date: Tue, 7 Oct 2014 16:36:01 +0100 Subject: [PATCH] Refactored image validation into a custom field This fixes issues with Django closing the image file before running validators. This may give a slight improvement in upload speed as the file doesn't keep having to be reopened. This unifies our image validation with Django's image validation so error messages could be shared (not implemented yet though). The code is more maintainable (the validators no longer need to be called manually from the multiple image uploader for example) --- wagtail/wagtailimages/fields.py | 82 +++++++++++++++++++ wagtail/wagtailimages/forms.py | 22 ++--- .../wagtailimages/migrations/0001_initial.py | 3 +- wagtail/wagtailimages/models.py | 3 +- .../wagtailimages/tests/test_admin_views.py | 2 +- wagtail/wagtailimages/utils/validators.py | 63 -------------- wagtail/wagtailimages/views/chooser.py | 2 +- wagtail/wagtailimages/views/images.py | 2 +- wagtail/wagtailimages/views/multiple.py | 73 +++++++++++------ 9 files changed, 145 insertions(+), 107 deletions(-) create mode 100644 wagtail/wagtailimages/fields.py delete mode 100644 wagtail/wagtailimages/utils/validators.py diff --git a/wagtail/wagtailimages/fields.py b/wagtail/wagtailimages/fields.py new file mode 100644 index 000000000..ef768e837 --- /dev/null +++ b/wagtail/wagtailimages/fields.py @@ -0,0 +1,82 @@ +import os + +from PIL import Image + +from django.forms.fields import ImageField +from django.core.exceptions import ValidationError +from django.utils.translation import ugettext_lazy as _ +from django.template.defaultfilters import filesizeformat +from django.conf import settings + + +def get_max_image_filesize(): + return getattr(settings, 'WAGTAILIMAGES_MAX_UPLOAD_SIZE', 10 * 1024 * 1024) + + +class WagtailImageField(ImageField): + default_error_messages = { + 'invalid_image': _( + "Not a supported image type. Please use a gif, jpeg or png file " + "with the correct file extension (*.gif, *.jpg or *.png)." + ), + 'file_too_large': _( + "This file is too big (%s). Image files must not exceed %s." + ), + } + + def check_image_file_format(self, f): + # Check file extension + extension = os.path.splitext(f.name)[1].lower()[1:] + + if extension == 'jpg': + extension = 'jpeg' + + if extension not in ['gif', 'jpeg', 'png']: + raise ValidationError(self.error_messages['invalid_image'], code='invalid_image') + + if hasattr(f, 'image'): + # Django 1.8 annotates the file object with the PIL image + image = f.image + elif not f.closed: + # Open image file + file_position = f.tell() + f.seek(0) + + try: + image = Image.open(f) + except IOError: + # Uploaded file is not even an image file (or corrupted) + raise ValidationError(self.error_messages['invalid_image'], code='invalid_image') + + f.seek(file_position) + else: + # Couldn't get the PIL image, skip checking the internal file format + return + + # Check that the internal format matches the extension + # It is possible to upload PSD files if their extension is set to jpg, png or gif. This should catch them out + if image.format.upper() != extension.upper(): + raise ValidationError(self.error_messages['invalid_image'], code='invalid_image') + + def check_image_file_size(self, f): + # Get max size + max_size = get_max_image_filesize() + + # Upload size checking can be disabled by setting max upload size to None + if max_size is None: + return + + # Check the filesize + if f.size > max_size: + raise ValidationError(self.error_messages['file_too_large'] % ( + filesizeformat(f.size), + filesizeformat(max_size), + ), code='file_too_large') + + def to_python(self, data): + f = super(WagtailImageField, self).to_python(data) + + self.check_image_file_format(f) + self.check_image_file_size(f) + + return f diff --git a/wagtail/wagtailimages/forms.py b/wagtail/wagtailimages/forms.py index 46bdd997d..4131db94f 100644 --- a/wagtail/wagtailimages/forms.py +++ b/wagtail/wagtailimages/forms.py @@ -4,11 +4,23 @@ from django.utils.translation import ugettext as _ from wagtail.wagtailimages.models import get_image_model from wagtail.wagtailimages.formats import get_image_formats +from wagtail.wagtailimages.fields import WagtailImageField + + +# Callback to allow us to override the default form field for the image file field +def formfield_for_dbfield(db_field, **kwargs): + # Check if this is the file field + if db_field.name == 'file': + return WagtailImageField(**kwargs) + + # For all other fields, just call its formfield() method. + return db_field.formfield(**kwargs) def get_image_form(): return modelform_factory( get_image_model(), + formfield_callback=formfield_for_dbfield, # set the 'file' widget to a FileInput rather than the default ClearableFileInput # so that when editing, we don't get the 'currently: ...' banner which is # a bit pointless here @@ -21,16 +33,6 @@ def get_image_form(): }) -def get_image_form_for_multi(): - # exclude the file widget - return modelform_factory(get_image_model(), exclude=('file',), widgets={ - 'focal_point_x': forms.HiddenInput(attrs={'class': 'focal_point_x'}), - 'focal_point_y': forms.HiddenInput(attrs={'class': 'focal_point_y'}), - 'focal_point_width': forms.HiddenInput(attrs={'class': 'focal_point_width'}), - 'focal_point_height': forms.HiddenInput(attrs={'class': 'focal_point_height'}), - }) - - class ImageInsertionForm(forms.Form): """ Form for selecting parameters of the image (e.g. format) prior to insertion diff --git a/wagtail/wagtailimages/migrations/0001_initial.py b/wagtail/wagtailimages/migrations/0001_initial.py index 64bb96257..745c03476 100644 --- a/wagtail/wagtailimages/migrations/0001_initial.py +++ b/wagtail/wagtailimages/migrations/0001_initial.py @@ -2,7 +2,6 @@ from __future__ import unicode_literals from django.db import models, migrations -import wagtail.wagtailimages.utils.validators import wagtail.wagtailimages.models import taggit.managers from django.conf import settings @@ -32,7 +31,7 @@ class Migration(migrations.Migration): fields=[ ('id', models.AutoField(primary_key=True, serialize=False, auto_created=True, verbose_name='ID')), ('title', models.CharField(verbose_name='Title', max_length=255)), - ('file', models.ImageField(width_field='width', upload_to=wagtail.wagtailimages.models.get_upload_to, verbose_name='File', height_field='height', validators=[wagtail.wagtailimages.utils.validators.validate_image_format])), + ('file', models.ImageField(width_field='width', upload_to=wagtail.wagtailimages.models.get_upload_to, verbose_name='File', height_field='height')), ('width', models.IntegerField(editable=False)), ('height', models.IntegerField(editable=False)), ('created_at', models.DateTimeField(auto_now_add=True)), diff --git a/wagtail/wagtailimages/models.py b/wagtail/wagtailimages/models.py index 36195120d..b3905755d 100644 --- a/wagtail/wagtailimages/models.py +++ b/wagtail/wagtailimages/models.py @@ -23,7 +23,6 @@ from unidecode import unidecode from wagtail.wagtailadmin.taggable import TagSearchable from wagtail.wagtailimages.backends import get_image_backend from wagtail.wagtailsearch import index -from wagtail.wagtailimages.utils.validators import validate_image_format, validate_image_filesize from wagtail.wagtailimages.utils.focal_point import FocalPoint from wagtail.wagtailimages.utils.feature_detection import FeatureDetector, opencv_available from wagtail.wagtailadmin.utils import get_object_usage @@ -46,7 +45,7 @@ def get_upload_to(instance, filename): @python_2_unicode_compatible class AbstractImage(models.Model, TagSearchable): title = models.CharField(max_length=255, verbose_name=_('Title') ) - file = models.ImageField(verbose_name=_('File'), upload_to=get_upload_to, width_field='width', height_field='height', validators=[validate_image_format, validate_image_filesize]) + file = models.ImageField(verbose_name=_('File'), upload_to=get_upload_to, width_field='width', height_field='height') width = models.IntegerField(editable=False) height = models.IntegerField(editable=False) created_at = models.DateTimeField(auto_now_add=True) diff --git a/wagtail/wagtailimages/tests/test_admin_views.py b/wagtail/wagtailimages/tests/test_admin_views.py index 5f7584624..b45276bb5 100644 --- a/wagtail/wagtailimages/tests/test_admin_views.py +++ b/wagtail/wagtailimages/tests/test_admin_views.py @@ -296,7 +296,7 @@ class TestMultipleImageUploader(TestCase, WagtailTestUtils): self.assertIn('success', response_json) self.assertIn('error_message', response_json) self.assertFalse(response_json['success']) - self.assertEqual(response_json['error_message'], 'Not a valid image. Please use a gif, jpeg or png file with the correct file extension (*.gif, *.jpg or *.png).') + self.assertEqual(response_json['error_message'], 'Not a supported image type. Please use a gif, jpeg or png file with the correct file extension (*.gif, *.jpg or *.png).') def test_edit_get(self): """ diff --git a/wagtail/wagtailimages/utils/validators.py b/wagtail/wagtailimages/utils/validators.py deleted file mode 100644 index 9b8bcc8ab..000000000 --- a/wagtail/wagtailimages/utils/validators.py +++ /dev/null @@ -1,63 +0,0 @@ -import os - -from PIL import Image - -from django.core.exceptions import ValidationError -from django.utils.translation import ugettext_lazy as _ -from django.template.defaultfilters import filesizeformat -from django.conf import settings - - -def validate_image_format(f): - # Check file extension - extension = os.path.splitext(f.name)[1].lower()[1:] - - if extension == 'jpg': - extension = 'jpeg' - - if extension not in ['gif', 'jpeg', 'png']: - raise ValidationError(_("Not a supported image type. Please use a gif, jpeg or png file with the correct file extension (*.gif, *.jpg or *.png).")) - - if not f.closed: - # Open image file - file_position = f.tell() - f.seek(0) - - try: - image = Image.open(f) - except IOError: - # Uploaded file is not even an image file (or corrupted) - raise ValidationError(_("Not a valid image. Please use a gif, jpeg or png file with the correct file extension (*.gif, *.jpg or *.png).")) - - f.seek(file_position) - - # Check that the internal format matches the extension - # It is possible to upload PSD files if their extension is set to jpg, png or gif. This should catch them out - if image.format.upper() != extension.upper(): - raise ValidationError(_("Not a valid %s image. Please use a gif, jpeg or png file with the correct file extension (*.gif, *.jpg or *.png).") % (extension.upper())) - - -def get_max_image_filesize(): - return getattr(settings, 'WAGTAILIMAGES_MAX_UPLOAD_SIZE', 10 * 1024 * 1024) - - -def validate_image_filesize(f): - # Get max size - max_size = get_max_image_filesize() - - # Upload size checking can be disabled by setting max upload size to None - if max_size is None: - return - - # Get the filesize - old_position = f.tell() - f.seek(0, 2) - file_size = f.tell() - f.seek(old_position) - - # Check the filesize - if file_size > max_size: - raise ValidationError(_("This file is too big (%s). Image files must not exceed %s.") % ( - filesizeformat(file_size), - filesizeformat(max_size), - )) diff --git a/wagtail/wagtailimages/views/chooser.py b/wagtail/wagtailimages/views/chooser.py index 68871db8a..2ce319e7e 100644 --- a/wagtail/wagtailimages/views/chooser.py +++ b/wagtail/wagtailimages/views/chooser.py @@ -11,7 +11,7 @@ from wagtail.wagtailsearch.backends import get_search_backends from wagtail.wagtailimages.models import get_image_model from wagtail.wagtailimages.forms import get_image_form, ImageInsertionForm from wagtail.wagtailimages.formats import get_image_format -from wagtail.wagtailimages.utils.validators import get_max_image_filesize +from wagtail.wagtailimages.fields import get_max_image_filesize def get_image_json(image): diff --git a/wagtail/wagtailimages/views/images.py b/wagtail/wagtailimages/views/images.py index 9c191d0d1..d75373c2b 100644 --- a/wagtail/wagtailimages/views/images.py +++ b/wagtail/wagtailimages/views/images.py @@ -17,7 +17,7 @@ from wagtail.wagtailsearch.backends import get_search_backends from wagtail.wagtailimages.models import get_image_model, Filter from wagtail.wagtailimages.forms import get_image_form, URLGeneratorForm from wagtail.wagtailimages.utils.crypto import generate_signature -from wagtail.wagtailimages.utils.validators import get_max_image_filesize +from wagtail.wagtailimages.fields import get_max_image_filesize @permission_required('wagtailimages.add_image') diff --git a/wagtail/wagtailimages/views/multiple.py b/wagtail/wagtailimages/views/multiple.py index 9cb8e8c64..1d1119306 100644 --- a/wagtail/wagtailimages/views/multiple.py +++ b/wagtail/wagtailimages/views/multiple.py @@ -9,24 +9,39 @@ from django.http import HttpResponse, HttpResponseBadRequest from django.template import RequestContext from django.template.loader import render_to_string from django.utils.translation import ugettext as _ +from django.utils.encoding import force_text from wagtail.wagtailsearch.backends import get_search_backends from wagtail.wagtailimages.models import get_image_model -from wagtail.wagtailimages.forms import get_image_form_for_multi -from wagtail.wagtailimages.utils.validators import validate_image_format, validate_image_filesize, get_max_image_filesize +from wagtail.wagtailimages.forms import get_image_form +from wagtail.wagtailimages.fields import get_max_image_filesize def json_response(document): return HttpResponse(json.dumps(document), content_type='application/json') +def get_image_edit_form(): + Image = get_image_model() + ImageForm = get_image_form() + + # Make a new form with the file field excluded + class ImageEditForm(ImageForm): + class Meta(ImageForm.Meta): + model = Image + exclude = ('file', ) + + return ImageEditForm + + @permission_required('wagtailimages.add_image') @vary_on_headers('X-Requested-With') def add(request): Image = get_image_model() - ImageForm = get_image_form_for_multi() + ImageForm = get_image_form() + # Create a new image form class which doesn't contain the file field if request.method == 'POST': if not request.is_ajax(): return HttpResponseBadRequest("Cannot POST to this view without AJAX") @@ -34,33 +49,37 @@ def add(request): if not request.FILES: return HttpResponseBadRequest("Must upload a file") - # Check that the uploaded file is valid - try: - validate_image_format(request.FILES['files[]']) - validate_image_filesize(request.FILES['files[]']) - except ValidationError as e: + # Build a form for validation + form = ImageForm({ + 'title': request.FILES['files[]'].name, + }, { + 'file': request.FILES['files[]'], + }) + + if form.is_valid(): + # Save it + image = form.save(commit=False) + image.uploaded_by_user = request.user + image.save() + + # Success! Send back an edit form for this image to the user + return json_response({ + 'success': True, + 'image_id': int(image.id), + 'form': render_to_string('wagtailimages/multiple/edit_form.html', { + 'image': image, + 'form': get_image_edit_form()(instance=image, prefix='image-%d' % image.id), + }, context_instance=RequestContext(request)), + }) + else: + # Validation error return json_response({ 'success': False, - 'error_message': '\n'.join(e.messages), + + # https://github.com/django/django/blob/stable/1.6.x/django/forms/util.py#L45 + 'error_message': '\n'.join(['\n'.join([force_text(i) for i in v]) for k, v in form.errors.items()]), }) - # Save it - image = Image(uploaded_by_user=request.user, title=request.FILES['files[]'].name, file=request.FILES['files[]']) - image.save() - - # Success! Send back an edit form for this image to the user - form = ImageForm(instance=image, prefix='image-%d' % image.id) - - return json_response({ - 'success': True, - 'image_id': int(image.id), - 'form': render_to_string('wagtailimages/multiple/edit_form.html', { - 'image': image, - 'form': form, - }, context_instance=RequestContext(request)), - }) - - return render(request, 'wagtailimages/multiple/add.html', { 'max_filesize': get_max_image_filesize(), }) @@ -70,7 +89,7 @@ def add(request): @permission_required('wagtailadmin.access_admin') # more specific permission tests are applied within the view def edit(request, image_id, callback=None): Image = get_image_model() - ImageForm = get_image_form_for_multi() + ImageForm = get_image_edit_form() image = get_object_or_404(Image, id=image_id)