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

@@ -142,23 +142,23 @@ 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
 
     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 @@ def __get__(self, instance=None, owner=None):
         # 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,22 +186,22 @@ def __get__(self, instance=None, owner=None):
 
         # 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.
         elif isinstance(file, FieldFile) and not hasattr(file, 'field'):
             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 @@ def __init__(self, verbose_name=None, name=None, width_field=None, height_field=
         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)

tests/modeltests/model_forms/models.py

@@ -1175,8 +1175,9 @@ def __unicode__(self):
 >>> 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 @@ def __unicode__(self):
 >>> 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 @@ def __unicode__(self):
 >>> 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 @@ def __unicode__(self):
 >>> 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

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)
-"""}