Fixed #26058 -- Delegated os.path bits of FileField's filename generation to the Storage.

8 files changed, 198 insertions, 15 deletions

diff --git a/django/core/files/storage.py b/django/core/files/storage.py
index 371efb92e1..b5348948fe 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 1d0308b7da..3ba2c14325 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 23dd56576b..b3d03dd37a 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 be197f7210..dd0670137c 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 <django.db.models.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 8de01e4fc7..3eac9bb311 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()
+  <django.core.files.storage.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() <django.core.files.storage.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()
+  <django.core.files.storage.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()
+  <django.core.files.storage.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 0000000000..4432013850
--- /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 42699dddb8..04d4f0c8d6 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 a27476ff8d..74926a5363 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):
         """