Commit d89ba464 authored by Gary Wilson Jr's avatar Gary Wilson Jr
Browse files

Changes to `ImageFileDescriptor` and `ImageField` to fix a few cases of... 

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
parent 7638651c

django/db/models/fields/files.py

+88 −45
Original line number Diff line number Diff line
@@ -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)

tests/modeltests/model_forms/models.py

+12 −9
Original line number Diff line number Diff line
@@ -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

tests/regressiontests/file_storage/models.py

+0 −93
Original line number Diff line number Diff line 
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)

"""}

tests/regressiontests/model_fields/4x8.png

0 → 100644
+87 B
Loading image diff...

tests/regressiontests/model_fields/8x4.png

0 → 100644
+87 B
Loading image diff...