diff options
| -rw-r--r-- | django/core/files/base.py | 16 | ||||
| -rw-r--r-- | django/core/files/images.py | 4 | ||||
| -rw-r--r-- | django/core/files/uploadedfile.py | 23 | ||||
| -rw-r--r-- | django/db/models/fields/files.py | 119 | ||||
| -rw-r--r-- | tests/modeltests/model_forms/models.py | 7 | ||||
| -rw-r--r-- | 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) """} |