From d89ba464dde14abc3aaf6d1e02202721f5fd2b3f Mon Sep 17 00:00:00 2001 From: Gary Wilson Jr Date: Thu, 28 May 2009 05:46:09 +0000 Subject: [PATCH] Changes to `ImageFileDescriptor` and `ImageField` to fix a few cases of setting image dimension fields. * Moved dimension field update logic out of `ImageFileDescriptor.__set__` and into its own method on `ImageField`. * New `ImageField.update_dimension_fields` method is attached to model instance's `post_init` signal so that: * Dimension fields are set when defined before the ImageField. * Dimension fields are set when the field is assigned in the model constructor (fixes #11196), but only if the dimension fields don't already have values, so we avoid updating the dimensions every time an object is loaded from the database (fixes #11084). * Clear dimension fields when the ImageField is set to None, which also causes dimension fields to be cleared when `ImageFieldFile.delete()` is used. * Added many more tests for ImageField that test edge cases we weren't testing before, and moved the ImageField tests out of `file_storage` and into their own module within `model_fields`. git-svn-id: http://code.djangoproject.com/svn/django/trunk@10858 bcc190cf-cafb-0310-a4f2-bffc1f526a37 --- django/db/models/fields/files.py | 133 ++++-- tests/modeltests/model_forms/models.py | 21 +- tests/regressiontests/file_storage/models.py | 93 ---- tests/regressiontests/model_fields/4x8.png | Bin 0 -> 87 bytes tests/regressiontests/model_fields/8x4.png | Bin 0 -> 87 bytes .../model_fields/imagefield.py | 403 ++++++++++++++++++ tests/regressiontests/model_fields/models.py | 107 ++++- tests/regressiontests/model_fields/tests.py | 16 +- 8 files changed, 621 insertions(+), 152 deletions(-) create mode 100644 tests/regressiontests/model_fields/4x8.png create mode 100644 tests/regressiontests/model_fields/8x4.png create mode 100644 tests/regressiontests/model_fields/imagefield.py diff --git a/django/db/models/fields/files.py b/django/db/models/fields/files.py index 61a903e36c1..aab4f3789f1 100644 --- a/django/db/models/fields/files.py +++ b/django/db/models/fields/files.py @@ -142,13 +142,13 @@ class FileDescriptor(object): """ The descriptor for the file attribute on the model instance. Returns a FieldFile when accessed so you can do stuff like:: - + >>> instance.file.size - + Assigns a file object on assignment so you can do:: - + >>> instance.file = File(...) - + """ def __init__(self, field): self.field = field @@ -156,9 +156,9 @@ class FileDescriptor(object): def __get__(self, instance=None, owner=None): if instance is None: raise AttributeError( - "The '%s' attribute can only be accessed from %s instances." + "The '%s' attribute can only be accessed from %s instances." % (self.field.name, owner.__name__)) - + # This is slightly complicated, so worth an explanation. # instance.file`needs to ultimately return some instance of `File`, # probably a subclass. Additionally, this returned object needs to have @@ -168,8 +168,8 @@ class FileDescriptor(object): # peek below you can see that we're not. So depending on the current # value of the field we have to dynamically construct some sort of # "thing" to return. - - # The instance dict contains whatever was originally assigned + + # The instance dict contains whatever was originally assigned # in __set__. file = instance.__dict__[self.field.name] @@ -186,14 +186,14 @@ class FileDescriptor(object): # Other types of files may be assigned as well, but they need to have # the FieldFile interface added to the. Thus, we wrap any other type of - # File inside a FieldFile (well, the field's attr_class, which is + # File inside a FieldFile (well, the field's attr_class, which is # usually FieldFile). elif isinstance(file, File) and not isinstance(file, FieldFile): file_copy = self.field.attr_class(instance, self.field, file.name) file_copy.file = file file_copy._committed = False instance.__dict__[self.field.name] = file_copy - + # Finally, because of the (some would say boneheaded) way pickle works, # the underlying FieldFile might not actually itself have an associated # file. So we need to reset the details of the FieldFile in those cases. @@ -201,7 +201,7 @@ class FileDescriptor(object): file.instance = instance file.field = self.field file.storage = self.field.storage - + # That was fun, wasn't it? return instance.__dict__[self.field.name] @@ -212,7 +212,7 @@ class FileField(Field): # The class to wrap instance attributes in. Accessing the file object off # the instance will always return an instance of attr_class. attr_class = FieldFile - + # The descriptor to use for accessing the attribute off of the class. descriptor_class = FileDescriptor @@ -300,40 +300,20 @@ class ImageFileDescriptor(FileDescriptor): assigning the width/height to the width_field/height_field, if appropriate. """ def __set__(self, instance, value): + previous_file = instance.__dict__.get(self.field.name) super(ImageFileDescriptor, self).__set__(instance, value) - - # The rest of this method deals with width/height fields, so we can - # bail early if neither is used. - if not self.field.width_field and not self.field.height_field: - return - - # We need to call the descriptor's __get__ to coerce this assigned - # value into an instance of the right type (an ImageFieldFile, in this - # case). - value = self.__get__(instance) - - if not value: - return - - # Get the image dimensions, making sure to leave the file in the same - # state (opened or closed) that we got it in. However, we *don't* rewind - # the file pointer if the file is already open. This is in keeping with - # most Python standard library file operations that leave it up to the - # user code to reset file pointers after operations that move it. - from django.core.files.images import get_image_dimensions - close = value.closed - value.open() - try: - width, height = get_image_dimensions(value) - finally: - if close: - value.close() - - # Update the width and height fields - if self.field.width_field: - setattr(value.instance, self.field.width_field, width) - if self.field.height_field: - setattr(value.instance, self.field.height_field, height) + + # To prevent recalculating image dimensions when we are instantiating + # an object from the database (bug #11084), only update dimensions if + # the field had a value before this assignment. Since the default + # value for FileField subclasses is an instance of field.attr_class, + # previous_file will only be None when we are called from + # Model.__init__(). The ImageField.update_dimension_fields method + # hooked up to the post_init signal handles the Model.__init__() cases. + # Assignment happening outside of Model.__init__() will trigger the + # update right here. + if previous_file is not None: + self.field.update_dimension_fields(instance, force=True) class ImageFieldFile(ImageFile, FieldFile): def delete(self, save=True): @@ -350,6 +330,69 @@ class ImageField(FileField): self.width_field, self.height_field = width_field, height_field FileField.__init__(self, verbose_name, name, **kwargs) + def contribute_to_class(self, cls, name): + super(ImageField, self).contribute_to_class(cls, name) + # Attach update_dimension_fields so that dimension fields declared + # after their corresponding image field don't stay cleared by + # Model.__init__, see bug #11196. + signals.post_init.connect(self.update_dimension_fields, sender=cls) + + def update_dimension_fields(self, instance, force=False, *args, **kwargs): + """ + Updates field's width and height fields, if defined. + + This method is hooked up to model's post_init signal to update + dimensions after instantiating a model instance. However, dimensions + won't be updated if the dimensions fields are already populated. This + avoids unnecessary recalculation when loading an object from the + database. + + Dimensions can be forced to update with force=True, which is how + ImageFileDescriptor.__set__ calls this method. + """ + # Nothing to update if the field doesn't have have dimension fields. + has_dimension_fields = self.width_field or self.height_field + if not has_dimension_fields: + return + + # getattr will call the ImageFileDescriptor's __get__ method, which + # coerces the assigned value into an instance of self.attr_class + # (ImageFieldFile in this case). + file = getattr(instance, self.attname) + + # Nothing to update if we have no file and not being forced to update. + if not file and not force: + return + + dimension_fields_filled = not( + (self.width_field and not getattr(instance, self.width_field)) + or (self.height_field and not getattr(instance, self.height_field)) + ) + # When both dimension fields have values, we are most likely loading + # data from the database or updating an image field that already had + # an image stored. In the first case, we don't want to update the + # dimension fields because we are already getting their values from the + # database. In the second case, we do want to update the dimensions + # fields and will skip this return because force will be True since we + # were called from ImageFileDescriptor.__set__. + if dimension_fields_filled and not force: + return + + # file should be an instance of ImageFieldFile or should be None. + if file: + width = file.width + height = file.height + else: + # No file, so clear dimensions fields. + width = None + height = None + + # Update the width and height fields. + if self.width_field: + setattr(instance, self.width_field, width) + if self.height_field: + setattr(instance, self.height_field, height) + def formfield(self, **kwargs): defaults = {'form_class': forms.ImageField} defaults.update(kwargs) diff --git a/tests/modeltests/model_forms/models.py b/tests/modeltests/model_forms/models.py index 95fda273f01..0fd24c18ad2 100644 --- a/tests/modeltests/model_forms/models.py +++ b/tests/modeltests/model_forms/models.py @@ -1175,8 +1175,9 @@ True >>> instance.height 16 -# Delete the current file since this is not done by Django. ->>> instance.image.delete() +# Delete the current file since this is not done by Django, but don't save +# because the dimension fields are not null=True. +>>> instance.image.delete(save=False) >>> f = ImageFileForm(data={'description': u'An image'}, files={'image': SimpleUploadedFile('test.png', image_data)}) >>> f.is_valid() @@ -1207,9 +1208,9 @@ True >>> instance.width 16 -# Delete the current image since this is not done by Django. - ->>> instance.image.delete() +# Delete the current file since this is not done by Django, but don't save +# because the dimension fields are not null=True. +>>> instance.image.delete(save=False) # Override the file by uploading a new one. @@ -1224,8 +1225,9 @@ True >>> instance.width 48 -# Delete the current file since this is not done by Django. ->>> instance.image.delete() +# Delete the current file since this is not done by Django, but don't save +# because the dimension fields are not null=True. +>>> instance.image.delete(save=False) >>> instance.delete() >>> f = ImageFileForm(data={'description': u'Changed it'}, files={'image': SimpleUploadedFile('test2.png', image_data2)}) @@ -1239,8 +1241,9 @@ True >>> instance.width 48 -# Delete the current file since this is not done by Django. ->>> instance.image.delete() +# Delete the current file since this is not done by Django, but don't save +# because the dimension fields are not null=True. +>>> instance.image.delete(save=False) >>> instance.delete() # Test the non-required ImageField diff --git a/tests/regressiontests/file_storage/models.py b/tests/regressiontests/file_storage/models.py index 32af5d7588e..e69de29bb2d 100644 --- a/tests/regressiontests/file_storage/models.py +++ b/tests/regressiontests/file_storage/models.py @@ -1,93 +0,0 @@ -import os -import tempfile -import shutil -from django.db import models -from django.core.files.storage import FileSystemStorage -from django.core.files.base import ContentFile - -# Test for correct behavior of width_field/height_field. -# Of course, we can't run this without PIL. - -try: - # Checking for the existence of Image is enough for CPython, but - # for PyPy, you need to check for the underlying modules - from PIL import Image, _imaging -except ImportError: - Image = None - -# If we have PIL, do these tests -if Image: - temp_storage_dir = tempfile.mkdtemp() - temp_storage = FileSystemStorage(temp_storage_dir) - - class Person(models.Model): - name = models.CharField(max_length=50) - mugshot = models.ImageField(storage=temp_storage, upload_to='tests', - height_field='mug_height', - width_field='mug_width') - mug_height = models.PositiveSmallIntegerField() - mug_width = models.PositiveSmallIntegerField() - - __test__ = {'API_TESTS': """ ->>> from django.core.files import File ->>> image_data = open(os.path.join(os.path.dirname(__file__), "test.png"), 'rb').read() ->>> p = Person(name="Joe") ->>> p.mugshot.save("mug", ContentFile(image_data)) ->>> p.mugshot.width -16 ->>> p.mugshot.height -16 ->>> p.mug_height -16 ->>> p.mug_width -16 - -# Bug #9786: Ensure '==' and '!=' work correctly. ->>> image_data = open(os.path.join(os.path.dirname(__file__), "test1.png"), 'rb').read() ->>> p1 = Person(name="Bob") ->>> p1.mugshot.save("mug", ContentFile(image_data)) ->>> p2 = Person.objects.get(name="Joe") ->>> p.mugshot == p2.mugshot -True ->>> p.mugshot != p2.mugshot -False ->>> p.mugshot != p1.mugshot -True - -Bug #9508: Similarly to the previous test, make sure hash() works as expected -(equal items must hash to the same value). ->>> hash(p.mugshot) == hash(p2.mugshot) -True - -# Bug #8175: correctly delete files that have been removed off the file system. ->>> import os ->>> p2 = Person(name="Fred") ->>> p2.mugshot.save("shot", ContentFile(image_data)) ->>> os.remove(p2.mugshot.path) ->>> p2.delete() - -# Bug #8534: FileField.size should not leave the file open. ->>> p3 = Person(name="Joan") ->>> p3.mugshot.save("shot", ContentFile(image_data)) - -# Get a "clean" model instance ->>> p3 = Person.objects.get(name="Joan") - -# It won't have an opened file. ->>> p3.mugshot.closed -True - -# After asking for the size, the file should still be closed. ->>> _ = p3.mugshot.size ->>> p3.mugshot.closed -True - -# Make sure that wrapping the file in a file still works ->>> p3.mugshot.file.open() ->>> p = Person.objects.create(name="Bob The Builder", mugshot=File(p3.mugshot.file)) ->>> p.save() ->>> p3.mugshot.file.close() - -# Delete all test files ->>> shutil.rmtree(temp_storage_dir) -"""} diff --git a/tests/regressiontests/model_fields/4x8.png b/tests/regressiontests/model_fields/4x8.png new file mode 100644 index 0000000000000000000000000000000000000000..ffce444d487d9c39a6c060b200e5bf7a1b18c9c0 GIT binary patch literal 87 zcmeAS@N?(olHy`uVBq!ia0vp^EI`b`!2~1&15bhk7>k44ofy`glX(f`2zt6WhHzX@ i{&T*8!H37>F#|)EBO|}3Wal!VB!j1`pUXO@geCyuc@ypc literal 0 HcmV?d00001 diff --git a/tests/regressiontests/model_fields/8x4.png b/tests/regressiontests/model_fields/8x4.png new file mode 100644 index 0000000000000000000000000000000000000000..60e6d69ee1da4a154ab87c29e347e76cdb76169c GIT binary patch literal 87 zcmeAS@N?(olHy`uVBq!ia0vp^96-#%!2~32*1ud1q!^2X+?^QKos)S9