Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

Fixed #10404: ImageField height_field and width_field options no long…

…er depend on putting the image field after the height/width fields as they did after r9766.

This bug actually exposed a related handful of inconsistancies in the underlying file handling and wraping, so a few related changes are in here as well:

    * Dimensions are also now calculated the moment the image is assigned to the field instead of upon save.
    * The base `File` object now when possible delegates its closed attribute down to the os-level file it wrapps.
    * In-memory files' `close()` now is a no-op. Without this certain APIs that should be able to handle in-memory files were failing.
    * Accessing `FieldFile.closed` used to open the file. That's silly, and it doesn't any more.
    * Some over-eager error handling was squishing some errors that would normally be raised. One unit test was incorrectly depending on this behavior, so the test was removed.

Thanks to Armin Ronacher for much of this work.

git-svn-id: http://code.djangoproject.com/svn/django/trunk@10737 bcc190cf-cafb-0310-a4f2-bffc1f526a37
  • Loading branch information...
commit 2b0903b2c4b6f5534e88d25254c6dcf02cc9d485 1 parent 4f9fd44
Jacob Kaplan-Moss jacobian authored
16 django/core/files/base.py
View
@@ -16,7 +16,6 @@ def __init__(self, file, name=None):
name = getattr(file, 'name', None)
self.name = name
self.mode = getattr(file, 'mode', None)
- self.closed = False
def __str__(self):
return smart_str(self.name or '')
@@ -48,6 +47,10 @@ def _set_size(self, size):
size = property(_get_size, _set_size)
+ def _get_closed(self):
+ return not self.file or self.file.closed
+ closed = property(_get_closed)
+
def chunks(self, chunk_size=None):
"""
Read the file and yield chucks of ``chunk_size`` bytes (defaults to
@@ -101,15 +104,13 @@ def __iter__(self):
def open(self, mode=None):
if not self.closed:
self.seek(0)
- elif os.path.exists(self.file.name):
- self.file = open(self.file.name, mode or self.file.mode)
- self.closed = False
+ elif self.name and os.path.exists(self.name):
+ self.file = open(self.name, mode or self.mode)
else:
raise ValueError("The file cannot be reopened.")
def close(self):
self.file.close()
- self.closed = True
class ContentFile(File):
"""
@@ -127,6 +128,7 @@ def __nonzero__(self):
return True
def open(self, mode=None):
- if self.closed:
- self.closed = False
self.seek(0)
+
+ def close(self):
+ pass
4 django/core/files/images.py
View
@@ -21,7 +21,11 @@ def _get_height(self):
def _get_image_dimensions(self):
if not hasattr(self, '_dimensions_cache'):
+ close = self.closed
+ self.open()
self._dimensions_cache = get_image_dimensions(self)
+ if close:
+ self.close()
return self._dimensions_cache
def get_image_dimensions(file_or_path):
23 django/core/files/uploadedfile.py
View
@@ -74,16 +74,13 @@ def temporary_file_path(self):
def close(self):
try:
- try:
- return self.file.close()
- except OSError, e:
- if e.errno != 2:
- # Means the file was moved or deleted before the tempfile
- # could unlink it. Still sets self.file.close_called and
- # calls self.file.file.close() before the exception
- raise
- finally:
- self.closed = True
+ return self.file.close()
+ except OSError, e:
+ if e.errno != 2:
+ # Means the file was moved or deleted before the tempfile
+ # could unlink it. Still sets self.file.close_called and
+ # calls self.file.file.close() before the exception
+ raise
class InMemoryUploadedFile(UploadedFile):
"""
@@ -93,10 +90,12 @@ def __init__(self, file, field_name, name, content_type, size, charset):
super(InMemoryUploadedFile, self).__init__(file, name, content_type, size, charset)
self.field_name = field_name
- def open(self):
- self.closed = False
+ def open(self, mode=None):
self.file.seek(0)
+ def close(self):
+ pass
+
def chunks(self, chunk_size=None):
self.file.seek(0)
yield self.read()
119 django/db/models/fields/files.py
View
@@ -78,7 +78,7 @@ def _get_size(self):
def open(self, mode='rb'):
self._require_file()
- return super(FieldFile, self).open(mode)
+ self.file.open(mode)
# open() doesn't alter the file's contents, but it does reset the pointer
open.alters_data = True
@@ -121,11 +121,15 @@ def delete(self, save=True):
self.instance.save()
delete.alters_data = True
+ def _get_closed(self):
+ file = getattr(self, '_file', None)
+ return file is None or file.closed
+ closed = property(_get_closed)
+
def close(self):
file = getattr(self, '_file', None)
if file is not None:
file.close()
- self.closed = True
def __getstate__(self):
# FieldFile needs access to its associated model field and an instance
@@ -135,35 +139,82 @@ def __getstate__(self):
return {'name': self.name, 'closed': False, '_committed': True, '_file': None}
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." % (self.field.name, owner.__name__))
+ raise AttributeError(
+ "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
+ # the FieldFile API so that users can easily do things like
+ # instance.file.path and have that delegated to the file storage engine.
+ # Easy enough if we're strict about assignment in __set__, but if you
+ # 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
+ # in __set__.
file = instance.__dict__[self.field.name]
+
+ # If this value is a string (instance.file = "path/to/file") or None
+ # then we simply wrap it with the appropriate attribute class according
+ # to the file field. [This is FieldFile for FileFields and
+ # ImageFieldFile for ImageFields; it's also conceivable that user
+ # subclasses might also want to subclass the attribute class]. This
+ # object understands how to convert a path to a file, and also how to
+ # handle None.
if isinstance(file, basestring) or file is None:
- # Create a new instance of FieldFile, based on a given file name
- instance.__dict__[self.field.name] = self.field.attr_class(instance, self.field, file)
+ attr = self.field.attr_class(instance, self.field, file)
+ instance.__dict__[self.field.name] = attr
+
+ # 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
+ # usually FieldFile).
elif isinstance(file, File) and not isinstance(file, FieldFile):
- # Other types of files may be assigned as well, but they need to
- # have the FieldFile interface added to them
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'):
- # The FieldFile was pickled, so some attributes need to be reset.
file.instance = instance
file.field = self.field
file.storage = self.field.storage
+
+ # That was fun, wasn't it?
return instance.__dict__[self.field.name]
def __set__(self, instance, value):
instance.__dict__[self.field.name] = value
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
def __init__(self, verbose_name=None, name=None, upload_to='', storage=None, **kwargs):
for arg in ('primary_key', 'unique'):
@@ -203,7 +254,7 @@ def pre_save(self, model_instance, add):
def contribute_to_class(self, cls, name):
super(FileField, self).contribute_to_class(cls, name)
- setattr(cls, self.name, FileDescriptor(self))
+ setattr(cls, self.name, self.descriptor_class(self))
signals.post_delete.connect(self.delete_file, sender=cls)
def delete_file(self, instance, sender, **kwargs):
@@ -243,19 +294,48 @@ def formfield(self, **kwargs):
defaults.update(kwargs)
return super(FileField, self).formfield(**defaults)
-class ImageFieldFile(ImageFile, FieldFile):
- def save(self, name, content, save=True):
- # Repopulate the image dimension cache.
- self._dimensions_cache = get_image_dimensions(content)
-
- # Update width/height fields, if needed
+class ImageFileDescriptor(FileDescriptor):
+ """
+ Just like the FileDescriptor, but for ImageFields. The only difference is
+ assigning the width/height to the width_field/height_field, if appropriate.
+ """
+ def __set__(self, instance, value):
+ 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(self.instance, self.field.width_field, self.width)
+ setattr(value.instance, self.field.width_field, width)
if self.field.height_field:
- setattr(self.instance, self.field.height_field, self.height)
-
- super(ImageFieldFile, self).save(name, content, save)
+ setattr(value.instance, self.field.height_field, height)
+class ImageFieldFile(ImageFile, FieldFile):
def delete(self, save=True):
# Clear the image dimensions cache
if hasattr(self, '_dimensions_cache'):
@@ -264,6 +344,7 @@ def delete(self, save=True):
class ImageField(FileField):
attr_class = ImageFieldFile
+ descriptor_class = ImageFileDescriptor
def __init__(self, verbose_name=None, name=None, width_field=None, height_field=None, **kwargs):
self.width_field, self.height_field = width_field, height_field
7 tests/modeltests/model_forms/models.py
View
@@ -112,10 +112,13 @@ def custom_upload_path(self, filename):
return '%s/%s' % (path, filename)
description = models.CharField(max_length=20)
- image = models.ImageField(storage=temp_storage, upload_to=custom_upload_path,
- width_field='width', height_field='height')
+
+ # Deliberately put the image field *after* the width/height fields to
+ # trigger the bug in #10404 with width/height not getting assigned.
width = models.IntegerField(editable=False)
height = models.IntegerField(editable=False)
+ image = models.ImageField(storage=temp_storage, upload_to=custom_upload_path,
+ width_field='width', height_field='height')
path = models.CharField(max_length=16, blank=True, default='')
def __unicode__(self):
18 tests/regressiontests/file_storage/models.py
View
@@ -73,18 +73,20 @@ class Person(models.Model):
# Get a "clean" model instance
>>> p3 = Person.objects.get(name="Joan")
-# It won't have an opened file. This is a bit brittle since it depends on the
-# the internals of FieldFile, but there's no other way of telling if the
-# file's been opened or not.
->>> p3.mugshot._file is not None
-False
+# 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._file is not None
-False
+>>> p3.mugshot.closed
+True
->>> p = Person.objects.create(name="Bob", mugshot=File(p3.mugshot.file))
+# 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()
+# Delete all test files
>>> shutil.rmtree(temp_storage_dir)
"""}
Please sign in to comment.
Something went wrong with that request. Please try again.