Permalink
Browse files

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/branches/soc2009/http-wsgi-improvements@10987 bcc190cf-cafb-0310-a4f2-bffc1f526a37
  • Loading branch information...
1 parent 2b9cefc commit f36391ecf53cf179c73e88fa3bdf0990fba5f230 @ccahoon ccahoon committed Jun 13, 2009
@@ -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)
@@ -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
@@ -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)
-"""}
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Oops, something went wrong.

0 comments on commit f36391e

Please sign in to comment.