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,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):
+        elif self.name and os.path.exists(self.name):
-            self.file = open(self.file.name, mode or self.file.mode)
+            self.file = open(self.name, mode or self.mode)
-            self.closed = False
         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

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):

django/core/files/uploadedfile.py

@@ -74,16 +74,13 @@ class TemporaryUploadedFile(UploadedFile):
 
     def close(self):
         try:
-            try:
+            return self.file.close()
-                return self.file.close()
+        except OSError, e:
-            except OSError, e:
+            if e.errno != 2:
-                if e.errno != 2:
+                # Means the file was moved or deleted before the tempfile
-                    # Means the file was moved or deleted before the tempfile
+                # could unlink it.  Still sets self.file.close_called and
-                    # could unlink it.  Still sets self.file.close_called and
+                # calls self.file.file.close() before the exception
-                    # calls self.file.file.close() before the exception
+                raise
-                    raise
-        finally:
-            self.closed = True
 
 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):
+    def open(self, mode=None):
-        self.closed = False
         self.file.seek(0)
 
+    def close(self):
+        pass
+
     def chunks(self, chunk_size=None):
         self.file.seek(0)
         yield self.read()

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
+            attr = self.field.attr_class(instance, self.field, file)
-            instance.__dict__[self.field.name] = 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):
+class ImageFileDescriptor(FileDescriptor):
-    def save(self, name, content, save=True):
+    """
-        # Repopulate the image dimension cache.
+    Just like the FileDescriptor, but for ImageFields. The only difference is
-        self._dimensions_cache = get_image_dimensions(content)
+    assigning the width/height to the width_field/height_field, if appropriate.
-
+    """
-        # Update width/height fields, if needed
+    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)
+            setattr(value.instance, self.field.height_field, height)
-
-        super(ImageFieldFile, self).save(name, content, save)
 
+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

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):

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
+# It won't have an opened file.
-# the internals of FieldFile, but there's no other way of telling if the
+>>> p3.mugshot.closed
-# file's been opened or not.
+True
->>> p3.mugshot._file is not None
-False
 
 # After asking for the size, the file should still be closed.
 >>> _ = p3.mugshot.size
->>> p3.mugshot._file is not None
+>>> p3.mugshot.closed
-False
+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)
 """}