From 68a890e79f660484d05482902663b6168f0bd71e Mon Sep 17 00:00:00 2001 From: Jacob Kaplan-Moss Date: Fri, 8 May 2009 15:08:09 +0000 Subject: [PATCH] Fixed #7712, #9404, #10249, #10300: a light refactor and cleanup of file storage and the `File` object. Thanks to Armin Ronacher and Alex Gaynor. git-svn-id: http://code.djangoproject.com/svn/django/trunk@10717 bcc190cf-cafb-0310-a4f2-bffc1f526a37 --- django/core/files/base.py | 76 +++++------------- django/core/files/temp.py | 9 +-- django/core/files/uploadedfile.py | 81 +++++++------------- django/core/files/utils.py | 29 +++++++ django/db/models/fields/files.py | 40 ++++++---- tests/modeltests/files/models.py | 2 +- tests/regressiontests/file_storage/models.py | 9 ++- 7 files changed, 107 insertions(+), 139 deletions(-) create mode 100644 django/core/files/utils.py diff --git a/django/core/files/base.py b/django/core/files/base.py index 1bf9330fed..9013acf2cc 100644 --- a/django/core/files/base.py +++ b/django/core/files/base.py @@ -1,20 +1,23 @@ import os -from django.utils.encoding import smart_str, smart_unicode - try: from cStringIO import StringIO except ImportError: from StringIO import StringIO -class File(object): +from django.utils.encoding import smart_str, smart_unicode +from django.core.files.utils import FileProxyMixin + +class File(FileProxyMixin): DEFAULT_CHUNK_SIZE = 64 * 2**10 - def __init__(self, file): + def __init__(self, file, name=None): self.file = file - self._name = file.name - self._mode = file.mode - self._closed = False + if name is 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 '') @@ -26,25 +29,11 @@ class File(object): return "<%s: %s>" % (self.__class__.__name__, self or "None") def __nonzero__(self): - return not not self.name + return bool(self.name) def __len__(self): return self.size - def _get_name(self): - if not hasattr(self, '_name'): - raise ValueError("This operation requires the file to have a name.") - return self._name - name = property(_get_name) - - def _get_mode(self): - return self._mode - mode = property(_get_mode) - - def _get_closed(self): - return self._closed - closed = property(_get_closed) - def _get_size(self): if not hasattr(self, '_size'): if hasattr(self.file, 'size'): @@ -66,7 +55,7 @@ class File(object): ``UploadedFile.DEFAULT_CHUNK_SIZE``). """ if not chunk_size: - chunk_size = self.__class__.DEFAULT_CHUNK_SIZE + chunk_size = self.DEFAULT_CHUNK_SIZE if hasattr(self, 'seek'): self.seek(0) @@ -89,12 +78,6 @@ class File(object): chunk_size = self.DEFAULT_CHUNK_SIZE return self.size > chunk_size - def xreadlines(self): - return iter(self) - - def readlines(self): - return list(self.xreadlines()) - def __iter__(self): # Iterate over this file-like object by newlines buffer_ = None @@ -121,43 +104,22 @@ class File(object): self.seek(0) elif os.path.exists(self.file.name): self.file = open(self.file.name, mode or self.file.mode) + self.closed = False else: raise ValueError("The file cannot be reopened.") - def seek(self, position): - self.file.seek(position) - - def tell(self): - return self.file.tell() - - def read(self, num_bytes=None): - if num_bytes is None: - return self.file.read() - return self.file.read(num_bytes) - - def write(self, content): - if not self.mode.startswith('w'): - raise IOError("File was not opened with write access.") - self.file.write(content) - - def flush(self): - if not self.mode.startswith('w'): - raise IOError("File was not opened with write access.") - self.file.flush() - def close(self): self.file.close() - self._closed = True + self.closed = True class ContentFile(File): """ A File-like object that takes just raw content, rather than an actual file. """ def __init__(self, content): - self.file = StringIO(content or '') - self.size = len(content or '') - self.file.seek(0) - self._closed = False + content = content or '' + super(ContentFile, self).__init__(StringIO(content)) + self.size = len(content) def __str__(self): return 'Raw content' @@ -166,6 +128,6 @@ class ContentFile(File): return True def open(self, mode=None): - if self._closed: - self._closed = False + if self.closed: + self.closed = False self.seek(0) diff --git a/django/core/files/temp.py b/django/core/files/temp.py index 02a4f02320..b607291294 100644 --- a/django/core/files/temp.py +++ b/django/core/files/temp.py @@ -11,11 +11,12 @@ NamedTemporaryFile uses the O_TEMPORARY flag, and thus cannot be reopened [1]. import os import tempfile +from django.core.files.utils import FileProxyMixin __all__ = ('NamedTemporaryFile', 'gettempdir',) if os.name == 'nt': - class TemporaryFile(object): + class TemporaryFile(FileProxyMixin): """ Temporary file object constructor that works in Windows and supports reopening of the temporary file in windows. @@ -48,12 +49,6 @@ if os.name == 'nt': def __del__(self): self.close() - # Proxy to the file object. - def __getattr__(self, name): - return getattr(self.file, name) - def __iter__(self): - return iter(self.file) - NamedTemporaryFile = TemporaryFile else: NamedTemporaryFile = tempfile.NamedTemporaryFile diff --git a/django/core/files/uploadedfile.py b/django/core/files/uploadedfile.py index 9fc4bcbbe5..df16fd7f6b 100644 --- a/django/core/files/uploadedfile.py +++ b/django/core/files/uploadedfile.py @@ -26,14 +26,15 @@ class UploadedFile(File): """ DEFAULT_CHUNK_SIZE = 64 * 2**10 - def __init__(self, name=None, content_type=None, size=None, charset=None): - self.name = name + def __init__(self, file=None, name=None, content_type=None, size=None, charset=None): + super(UploadedFile, self).__init__(file, name) self.size = size self.content_type = content_type self.charset = charset def __repr__(self): - return "<%s: %s (%s)>" % (self.__class__.__name__, smart_str(self.name), self.content_type) + return "<%s: %s (%s)>" % ( + self.__class__.__name__, smart_str(self.name), self.content_type) def _get_name(self): return self._name @@ -53,95 +54,66 @@ class UploadedFile(File): name = property(_get_name, _set_name) - # Abstract methods; subclasses *must* define read() and probably should - # define open/close. - def read(self, num_bytes=None): - raise NotImplementedError() - - def open(self): - pass - - def close(self): - pass - class TemporaryUploadedFile(UploadedFile): """ A file uploaded to a temporary location (i.e. stream-to-disk). """ def __init__(self, name, content_type, size, charset): - super(TemporaryUploadedFile, self).__init__(name, content_type, size, charset) if settings.FILE_UPLOAD_TEMP_DIR: - self._file = tempfile.NamedTemporaryFile(suffix='.upload', dir=settings.FILE_UPLOAD_TEMP_DIR) + file = tempfile.NamedTemporaryFile(suffix='.upload', + dir=settings.FILE_UPLOAD_TEMP_DIR) else: - self._file = tempfile.NamedTemporaryFile(suffix='.upload') + file = tempfile.NamedTemporaryFile(suffix='.upload') + super(TemporaryUploadedFile, self).__init__(file, name, content_type, size, charset) def temporary_file_path(self): """ Returns the full path of this file. """ - return self._file.name + return self.file.name - # Most methods on this object get proxied to NamedTemporaryFile. - # We can't directly subclass because NamedTemporaryFile is actually a - # factory function - def read(self, *args): return self._file.read(*args) - def seek(self, *args): return self._file.seek(*args) - def write(self, s): return self._file.write(s) - def tell(self, *args): return self._file.tell(*args) - def __iter__(self): return iter(self._file) - def readlines(self, size=None): return self._file.readlines(size) - def xreadlines(self): return self._file.xreadlines() def close(self): 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 - return - else: - raise e + 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 class InMemoryUploadedFile(UploadedFile): """ A file uploaded into memory (i.e. stream-to-memory). """ def __init__(self, file, field_name, name, content_type, size, charset): - super(InMemoryUploadedFile, self).__init__(name, content_type, size, charset) - self._file = file + super(InMemoryUploadedFile, self).__init__(file, name, content_type, size, charset) self.field_name = field_name - self._file.seek(0) def open(self): - self._file.seek(0) + self.closed = False + self.file.seek(0) def chunks(self, chunk_size=None): - self._file.seek(0) + self.file.seek(0) yield self.read() def multiple_chunks(self, chunk_size=None): # Since it's in memory, we'll never have multiple chunks. return False - # proxy methods to StringIO - def read(self, *args): return self._file.read(*args) - def seek(self, *args): return self._file.seek(*args) - def tell(self, *args): return self._file.tell(*args) - def close(self): return self._file.close() class SimpleUploadedFile(InMemoryUploadedFile): """ A simple representation of a file, which just has content, size, and a name. """ def __init__(self, name, content, content_type='text/plain'): - self._file = StringIO(content or '') - self.name = name - self.field_name = None - self.size = len(content or '') - self.content_type = content_type - self.charset = None - self._file.seek(0) + content = content or '' + super(SimpleUploadedFile, self).__init__(StringIO(content), None, name, + content_type, len(content), None) def from_dict(cls, file_dict): """ @@ -154,5 +126,4 @@ class SimpleUploadedFile(InMemoryUploadedFile): return cls(file_dict['filename'], file_dict['content'], file_dict.get('content-type', 'text/plain')) - from_dict = classmethod(from_dict) diff --git a/django/core/files/utils.py b/django/core/files/utils.py new file mode 100644 index 0000000000..8cc212fe1f --- /dev/null +++ b/django/core/files/utils.py @@ -0,0 +1,29 @@ +class FileProxyMixin(object): + """ + A mixin class used to forward file methods to an underlaying file + object. The internal file object has to be called "file":: + + class FileProxy(FileProxyMixin): + def __init__(self, file): + self.file = file + """ + + encoding = property(lambda self: self.file.encoding) + fileno = property(lambda self: self.file.fileno) + flush = property(lambda self: self.file.flush) + isatty = property(lambda self: self.file.isatty) + newlines = property(lambda self: self.file.newlines) + read = property(lambda self: self.file.read) + readinto = property(lambda self: self.file.readinto) + readline = property(lambda self: self.file.readline) + readlines = property(lambda self: self.file.readlines) + seek = property(lambda self: self.file.seek) + softspace = property(lambda self: self.file.softspace) + tell = property(lambda self: self.file.tell) + truncate = property(lambda self: self.file.truncate) + write = property(lambda self: self.file.write) + writelines = property(lambda self: self.file.writelines) + xreadlines = property(lambda self: self.file.xreadlines) + + def __iter__(self): + return iter(self.file) diff --git a/django/db/models/fields/files.py b/django/db/models/fields/files.py index 43853369d1..f110aeaf8a 100644 --- a/django/db/models/fields/files.py +++ b/django/db/models/fields/files.py @@ -17,11 +17,10 @@ from django.db.models.loading import cache class FieldFile(File): def __init__(self, instance, field, name): + super(FieldFile, self).__init__(None, name) self.instance = instance self.field = field self.storage = field.storage - self._name = name or u'' - self._closed = False self._committed = True def __eq__(self, other): @@ -48,10 +47,17 @@ class FieldFile(File): def _get_file(self): self._require_file() - if not hasattr(self, '_file'): + if not hasattr(self, '_file') or self._file is None: self._file = self.storage.open(self.name, 'rb') return self._file - file = property(_get_file) + + def _set_file(self, file): + self._file = file + + def _del_file(self): + del self._file + + file = property(_get_file, _set_file, _del_file) def _get_path(self): self._require_file() @@ -65,6 +71,8 @@ class FieldFile(File): def _get_size(self): self._require_file() + if not self._committed: + return len(self.file) return self.storage.size(self.name) size = property(_get_size) @@ -80,7 +88,7 @@ class FieldFile(File): def save(self, name, content, save=True): name = self.field.generate_filename(self.instance, name) - self._name = self.storage.save(name, content) + self.name = self.storage.save(name, content) setattr(self.instance, self.field.name, self.name) # Update the filesize cache @@ -97,11 +105,11 @@ class FieldFile(File): # presence of self._file if hasattr(self, '_file'): self.close() - del self._file - + del self.file + self.storage.delete(self.name) - self._name = None + self.name = None setattr(self.instance, self.field.name, self.name) # Delete the filesize cache @@ -113,12 +121,18 @@ class FieldFile(File): self.instance.save() delete.alters_data = True + 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 # it's attached to in order to work properly, but the only necessary # data to be pickled is the file's name itself. Everything else will # be restored later, by FileDescriptor below. - return {'_name': self.name, '_closed': False, '_committed': True} + return {'name': self.name, 'closed': False, '_committed': True, '_file': None} class FileDescriptor(object): def __init__(self, field): @@ -134,12 +148,8 @@ class FileDescriptor(object): 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 = copy.copy(file) - file_copy.__class__ = type(file.__class__.__name__, - (file.__class__, self.field.attr_class), {}) - file_copy.instance = instance - file_copy.field = self.field - file_copy.storage = self.field.storage + 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 elif isinstance(file, FieldFile) and not hasattr(file, 'field'): diff --git a/tests/modeltests/files/models.py b/tests/modeltests/files/models.py index 30bbdf2841..beea97e808 100644 --- a/tests/modeltests/files/models.py +++ b/tests/modeltests/files/models.py @@ -6,6 +6,7 @@ and where files should be stored. """ import shutil +import random import tempfile from django.db import models from django.core.files.base import ContentFile @@ -26,7 +27,6 @@ class Storage(models.Model): def random_upload_to(self, filename): # This returns a different result each time, # to make sure it only gets called once. - import random return '%s/%s' % (random.randint(100, 999), filename) normal = models.FileField(storage=temp_storage, upload_to='tests') diff --git a/tests/regressiontests/file_storage/models.py b/tests/regressiontests/file_storage/models.py index 94acc8e534..c3dafcc287 100644 --- a/tests/regressiontests/file_storage/models.py +++ b/tests/regressiontests/file_storage/models.py @@ -29,7 +29,7 @@ if Image: 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)) @@ -76,14 +76,15 @@ True # 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. ->>> hasattr(p3.mugshot, '_file') +>>> p3.mugshot._file is not None False # After asking for the size, the file should still be closed. >>> _ = p3.mugshot.size ->>> hasattr(p3.mugshot, '_file') +>>> p3.mugshot._file is not None False +>>> p = Person.objects.create(name="Bob", mugshot=File(p3.mugshot.file)) + >>> shutil.rmtree(temp_storage_dir) """} -