From 2b0903b2c4b6f5534e88d25254c6dcf02cc9d485 Mon Sep 17 00:00:00 2001
From: Jacob Kaplan-Moss <jacob@jacobian.org>
Date: Mon, 11 May 2009 09:57:19 +0000
Subject: [PATCH] Fixed #10404: ImageField height_field and width_field options
no longer 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
---
django/core/files/base.py | 16 +--
django/core/files/images.py | 4 +
django/core/files/uploadedfile.py | 23 ++--
django/db/models/fields/files.py | 119 ++++++++++++++++---
tests/modeltests/model_forms/models.py | 7 +-
tests/regressiontests/file_storage/models.py | 18 +--
6 files changed, 139 insertions(+), 48 deletions(-)
diff --git a/django/core/files/base.py b/django/core/files/base.py
index f9e1c9a0d7..f9e3f1039a 100644
--- a/django/core/files/base.py
+++ b/django/core/files/base.py
@@ -16,7 +16,6 @@ class File(FileProxyMixin):
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 @@ class File(FileProxyMixin):
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 @@ class File(FileProxyMixin):
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 @@ class ContentFile(File):
return True
def open(self, mode=None):
- if self.closed:
- self.closed = False
self.seek(0)
+
+ def close(self):
+ pass
diff --git a/django/core/files/images.py b/django/core/files/images.py
index 0e5b778af8..24c1151e8d 100644
--- a/django/core/files/images.py
+++ b/django/core/files/images.py
@@ -21,7 +21,11 @@ class ImageFile(File):
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):
diff --git a/django/core/files/uploadedfile.py b/django/core/files/uploadedfile.py
index df16fd7f6b..5178f0bacc 100644
--- a/django/core/files/uploadedfile.py
+++ b/django/core/files/uploadedfile.py
@@ -74,16 +74,13 @@ class TemporaryUploadedFile(UploadedFile):
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 @@ class InMemoryUploadedFile(UploadedFile):
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()
diff --git a/django/db/models/fields/files.py b/django/db/models/fields/files.py
index f110aeaf8a..61a903e36c 100644
--- a/django/db/models/fields/files.py
+++ b/django/db/models/fields/files.py
@@ -78,7 +78,7 @@ class FieldFile(File):
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 @@ class FieldFile(File):
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 @@ class FieldFile(File):
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 @@ class FileField(Field):
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 @@ class FileField(Field):
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 @@ class ImageFieldFile(ImageFile, FieldFile):
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
diff --git a/tests/modeltests/model_forms/models.py b/tests/modeltests/model_forms/models.py
index c5795b7e56..95fda273f0 100644
--- a/tests/modeltests/model_forms/models.py
+++ b/tests/modeltests/model_forms/models.py
@@ -112,10 +112,13 @@ try:
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):
diff --git a/tests/regressiontests/file_storage/models.py b/tests/regressiontests/file_storage/models.py
index c3dafcc287..42d16b6382 100644
--- a/tests/regressiontests/file_storage/models.py
+++ b/tests/regressiontests/file_storage/models.py
@@ -73,18 +73,20 @@ True
# 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)
"""}