From 914c72be2abb1c6dd860cb9279beaa66409ae1b2 Mon Sep 17 00:00:00 2001 From: Cristiano Date: Sun, 20 Mar 2016 22:51:17 -0300 Subject: [PATCH] Fixed #26058 -- Delegated os.path bits of FileField's filename generation to the Storage. --- django/core/files/storage.py | 17 ++- django/db/models/fields/files.py | 31 +++-- docs/internals/deprecation.txt | 3 + docs/ref/files/storage.txt | 15 +++ docs/releases/1.10.txt | 24 ++++ tests/file_storage/test_generate_filename.py | 117 +++++++++++++++++++ tests/file_storage/tests.py | 2 +- tests/model_fields/test_imagefield.py | 4 +- 8 files changed, 198 insertions(+), 15 deletions(-) create mode 100644 tests/file_storage/test_generate_filename.py diff --git a/django/core/files/storage.py b/django/core/files/storage.py index 371efb92e13..b5348948fec 100644 --- a/django/core/files/storage.py +++ b/django/core/files/storage.py @@ -51,10 +51,7 @@ class Storage(object): content = File(content, name) name = self.get_available_name(name, max_length=max_length) - name = self._save(name, content) - - # Store filenames with forward slashes, even on Windows - return force_text(name.replace('\\', '/')) + return self._save(name, content) # These methods are part of the public API, with default implementations. @@ -96,6 +93,15 @@ class Storage(object): name = os.path.join(dir_name, "%s_%s%s" % (file_root, get_random_string(7), file_ext)) return name + def generate_filename(self, filename): + """ + Validate the filename by calling get_valid_name() and return a filename + to be passed to the save() method. + """ + # `filename` may include a path as returned by FileField.upload_to. + dirname, filename = os.path.split(filename) + return os.path.normpath(os.path.join(dirname, self.get_valid_name(filename))) + def path(self, name): """ Returns a local filesystem path where the file can be retrieved using @@ -367,7 +373,8 @@ class FileSystemStorage(Storage): if self.file_permissions_mode is not None: os.chmod(full_path, self.file_permissions_mode) - return name + # Store filenames with forward slashes, even on Windows. + return force_text(name.replace('\\', '/')) def delete(self, name): assert name, "The name argument is not allowed to be empty." diff --git a/django/db/models/fields/files.py b/django/db/models/fields/files.py index 1d0308b7da9..3ba2c14325d 100644 --- a/django/db/models/fields/files.py +++ b/django/db/models/fields/files.py @@ -1,5 +1,7 @@ import datetime import os +import posixpath +import warnings from django import forms from django.core import checks @@ -9,6 +11,7 @@ from django.core.files.storage import default_storage from django.db.models import signals from django.db.models.fields import Field from django.utils import six +from django.utils.deprecation import RemovedInDjango20Warning from django.utils.encoding import force_str, force_text from django.utils.translation import ugettext_lazy as _ @@ -294,20 +297,34 @@ class FileField(Field): setattr(cls, self.name, self.descriptor_class(self)) def get_directory_name(self): + warnings.warn( + 'FileField now delegates file name and folder processing to the ' + 'storage. get_directory_name() will be removed in Django 2.0.', + RemovedInDjango20Warning, stacklevel=2 + ) return os.path.normpath(force_text(datetime.datetime.now().strftime(force_str(self.upload_to)))) def get_filename(self, filename): + warnings.warn( + 'FileField now delegates file name and folder processing to the ' + 'storage. get_filename() will be removed in Django 2.0.', + RemovedInDjango20Warning, stacklevel=2 + ) return os.path.normpath(self.storage.get_valid_name(os.path.basename(filename))) def generate_filename(self, instance, filename): - # If upload_to is a callable, make sure that the path it returns is - # passed through get_valid_name() of the underlying storage. + """ + Apply (if callable) or prepend (if a string) upload_to to the filename, + then delegate further processing of the name to the storage backend. + Until the storage layer, all file paths are expected to be Unix style + (with forward slashes). + """ if callable(self.upload_to): - directory_name, filename = os.path.split(self.upload_to(instance, filename)) - filename = self.storage.get_valid_name(filename) - return os.path.normpath(os.path.join(directory_name, filename)) - - return os.path.join(self.get_directory_name(), self.get_filename(filename)) + filename = self.upload_to(instance, filename) + else: + dirname = force_text(datetime.datetime.now().strftime(force_str(self.upload_to))) + filename = posixpath.join(dirname, filename) + return self.storage.generate_filename(filename) def save_form_data(self, instance, data): # Important: None means "no change", other false value means "clear" diff --git a/docs/internals/deprecation.txt b/docs/internals/deprecation.txt index 23dd56576be..b3d03dd37a0 100644 --- a/docs/internals/deprecation.txt +++ b/docs/internals/deprecation.txt @@ -165,6 +165,9 @@ details on these changes. * Support for ``Widget._format_value()`` will be removed. +* ``FileField`` methods ``get_directory_name()`` and ``get_filename()`` will be + removed. + .. _deprecation-removed-in-1.10: 1.10 diff --git a/docs/ref/files/storage.txt b/docs/ref/files/storage.txt index be197f7210d..dd0670137cf 100644 --- a/docs/ref/files/storage.txt +++ b/docs/ref/files/storage.txt @@ -164,6 +164,21 @@ The ``Storage`` class Returns a filename based on the ``name`` parameter that's suitable for use on the target storage system. + .. method:: generate_filename(filename) + + .. versionadded:: 1.10 + + Validates the ``filename`` by calling :attr:`get_valid_name()` and + returns a filename to be passed to the :meth:`save` method. + + The ``filename`` argument may include a path as returned by + :attr:`FileField.upload_to `. + In that case, the path won't be passed to :attr:`get_valid_name()` but + will be prepended back to the resulting name. + + The default implementation uses :mod:`os.path` operations. Override + this method if that's not appropriate for your storage. + .. method:: listdir(path) Lists the contents of the specified path, returning a 2-tuple of lists; diff --git a/docs/releases/1.10.txt b/docs/releases/1.10.txt index 8de01e4fc7f..3eac9bb3118 100644 --- a/docs/releases/1.10.txt +++ b/docs/releases/1.10.txt @@ -251,6 +251,10 @@ File Storage timezone-aware ``datetime`` if :setting:`USE_TZ` is ``True`` and a naive ``datetime`` in the local timezone otherwise. +* The new :meth:`Storage.generate_filename() + ` method makes it easier + to implement custom storages that don't use the ``os.path`` calls previously + in :class:`~django.db.models.FileField`. File Uploads ~~~~~~~~~~~~ @@ -789,6 +793,21 @@ Miscellaneous * The ``Model._deferred`` attribute is removed as dynamic model classes when using ``QuerySet.defer()`` and ``only()`` is removed. +* :meth:`Storage.save() ` no longer + replaces ``'\'`` with ``'/'``. This behavior is moved to + :class:`~django.core.files.storage.FileSystemStorage` since this is a storage + specific implementation detail. Any Windows user with a custom storage + implementation that relies on this behavior will need to implement it in the + custom storage's ``save()`` method. + +* Private :class:`~django.db.models.FileField` methods ``get_directory_name()`` + and ``get_filename()`` are no longer called (and are now deprecated) which is + a backwards incompatible change for users overriding those methods on custom + fields. To adapt such code, override ``FileField.generate_filename()`` or + :meth:`Storage.generate_filename() + ` instead. It + might be possible to use :attr:`~django.db.models.FileField.upload_to` also. + .. _deprecated-features-1.10: Features deprecated in 1.10 @@ -998,6 +1017,11 @@ Miscellaneous :meth:`~django.forms.Widget.format_value`. The old name will work through a deprecation period. +* Private ``FileField`` methods ``get_directory_name()`` and ``get_filename()`` + are deprecated in favor of performing this work in + :meth:`Storage.generate_filename() + `). + .. _removed-features-1.10: Features removed in 1.10 diff --git a/tests/file_storage/test_generate_filename.py b/tests/file_storage/test_generate_filename.py new file mode 100644 index 00000000000..44320138509 --- /dev/null +++ b/tests/file_storage/test_generate_filename.py @@ -0,0 +1,117 @@ +import os +import warnings + +from django.core.files.base import ContentFile +from django.core.files.storage import Storage +from django.db.models import FileField +from django.test import SimpleTestCase + + +class AWSS3Storage(Storage): + """ + Simulate an AWS S3 storage which uses Unix-like paths and allows any + characters in file names but where there aren't actual folders but just + keys. + """ + prefix = 'mys3folder/' + + def _save(self, name, content): + """ + This method is important to test that Storage.save() doesn't replace + '\' with '/' (rather FileSystemStorage.save() does). + """ + return name + + def get_valid_name(self, name): + return name + + def get_available_name(self, name, max_length=None): + return name + + def generate_filename(self, filename): + """ + This is the method that's important to override when using S3 so that + os.path() isn't called, which would break S3 keys. + """ + return self.prefix + self.get_valid_name(filename) + + +class GenerateFilenameStorageTests(SimpleTestCase): + + def test_filefield_get_directory_deprecation(self): + with warnings.catch_warnings(record=True) as warns: + warnings.simplefilter('always') + f = FileField(upload_to='some/folder/') + self.assertEqual(f.get_directory_name(), os.path.normpath('some/folder/')) + + self.assertEqual(len(warns), 1) + self.assertEqual( + warns[0].message.args[0], + 'FileField now delegates file name and folder processing to the ' + 'storage. get_directory_name() will be removed in Django 2.0.' + ) + + def test_filefield_get_filename_deprecation(self): + with warnings.catch_warnings(record=True) as warns: + warnings.simplefilter('always') + f = FileField(upload_to='some/folder/') + self.assertEqual(f.get_filename('some/folder/test.txt'), 'test.txt') + + self.assertEqual(len(warns), 1) + self.assertEqual( + warns[0].message.args[0], + 'FileField now delegates file name and folder processing to the ' + 'storage. get_filename() will be removed in Django 2.0.' + ) + + def test_filefield_generate_filename(self): + f = FileField(upload_to='some/folder/') + self.assertEqual( + f.generate_filename(None, 'test with space.txt'), + os.path.normpath('some/folder/test_with_space.txt') + ) + + def test_filefield_generate_filename_with_upload_to(self): + def upload_to(instance, filename): + return 'some/folder/' + filename + + f = FileField(upload_to=upload_to) + self.assertEqual( + f.generate_filename(None, 'test with space.txt'), + os.path.normpath('some/folder/test_with_space.txt') + ) + + def test_filefield_awss3_storage(self): + """ + Simulate a FileField with an S3 storage which uses keys rather than + folders and names. FileField and Storage shouldn't have any os.path() + calls that break the key. + """ + storage = AWSS3Storage() + folder = 'not/a/folder/' + + f = FileField(upload_to=folder, storage=storage) + key = 'my-file-key\\with odd characters' + data = ContentFile('test') + expected_key = AWSS3Storage.prefix + folder + key + + # Simulate call to f.save() + result_key = f.generate_filename(None, key) + self.assertEqual(result_key, expected_key) + + result_key = storage.save(result_key, data) + self.assertEqual(result_key, expected_key) + + # Repeat test with a callable. + def upload_to(instance, filename): + # Return a non-normalized path on purpose. + return folder + filename + + f = FileField(upload_to=upload_to, storage=storage) + + # Simulate call to f.save() + result_key = f.generate_filename(None, key) + self.assertEqual(result_key, expected_key) + + result_key = storage.save(result_key, data) + self.assertEqual(result_key, expected_key) diff --git a/tests/file_storage/tests.py b/tests/file_storage/tests.py index 42699dddb80..04d4f0c8d67 100644 --- a/tests/file_storage/tests.py +++ b/tests/file_storage/tests.py @@ -817,7 +817,7 @@ class FileFieldStorageTests(TestCase): # upload_to can be empty, meaning it does not use subdirectory. obj = Storage() obj.empty.save('django_test.txt', ContentFile('more content')) - self.assertEqual(obj.empty.name, "./django_test.txt") + self.assertEqual(obj.empty.name, "django_test.txt") self.assertEqual(obj.empty.read(), b"more content") obj.empty.close() diff --git a/tests/model_fields/test_imagefield.py b/tests/model_fields/test_imagefield.py index a27476ff8d2..74926a53635 100644 --- a/tests/model_fields/test_imagefield.py +++ b/tests/model_fields/test_imagefield.py @@ -54,10 +54,10 @@ class ImageFieldTestMixin(SerializeMixin): os.mkdir(temp_storage_dir) file_path1 = os.path.join(os.path.dirname(upath(__file__)), "4x8.png") - self.file1 = self.File(open(file_path1, 'rb')) + self.file1 = self.File(open(file_path1, 'rb'), name='4x8.png') file_path2 = os.path.join(os.path.dirname(upath(__file__)), "8x4.png") - self.file2 = self.File(open(file_path2, 'rb')) + self.file2 = self.File(open(file_path2, 'rb'), name='8x4.png') def tearDown(self): """