Fixed #9893 -- Allowed using a field's max_length in the Storage.

django/core/files/storage.py

@@ -1,8 +1,11 @@
-import os
-import errno
 from datetime import datetime
+import errno
+from inspect import getargspec
+import os
+import warnings
 
 from django.conf import settings
+from django.core.exceptions import SuspiciousFileOperation
 from django.core.files import locks, File
 from django.core.files.move import file_move_safe
 from django.utils.crypto import get_random_string
@@ -13,6 +16,7 @@
 from django.utils.text import get_valid_filename
 from django.utils._os import safe_join, abspathu
 from django.utils.deconstruct import deconstructible
+from django.utils.deprecation import RemovedInDjango20Warning
 
 
 __all__ = ('Storage', 'FileSystemStorage', 'DefaultStorage', 'default_storage')
@@ -33,7 +37,7 @@ def open(self, name, mode='rb'):
         """
         return self._open(name, mode)
 
-    def save(self, name, content):
+    def save(self, name, content, max_length=None):
         """
         Saves new content to the file specified by name. The content should be
         a proper File object or any python file-like object, ready to be read
@@ -46,7 +50,18 @@ def save(self, name, content):
         if not hasattr(content, 'chunks'):
             content = File(content)
 
-        name = self.get_available_name(name)
+        args, varargs, varkw, defaults = getargspec(self.get_available_name)
+        if 'max_length' in args:
+            name = self.get_available_name(name, max_length=max_length)
+        else:
+            warnings.warn(
+                'Backwards compatibility for storage backends without '
+                'support for the `max_length` argument in '
+                'Storage.get_available_name() will be removed in Django 2.0.',
+                RemovedInDjango20Warning, stacklevel=2
+            )
+            name = self.get_available_name(name)
+
         name = self._save(name, content)
 
         # Store filenames with forward slashes, even on Windows
@@ -61,7 +76,7 @@ def get_valid_name(self, name):
         """
         return get_valid_filename(name)
 
-    def get_available_name(self, name):
+    def get_available_name(self, name, max_length=None):
         """
         Returns a filename that's free on the target storage system, and
         available for new content to be written to.
@@ -71,10 +86,25 @@ def get_available_name(self, name):
         # If the filename already exists, add an underscore and a random 7
         # character alphanumeric string (before the file extension, if one
         # exists) to the filename until the generated filename doesn't exist.
-        while self.exists(name):
+        # Truncate original name if required, so the new filename does not
+        # exceed the max_length.
+        while self.exists(name) or (max_length and len(name) > max_length):
             # file_ext includes the dot.
             name = os.path.join(dir_name, "%s_%s%s" % (file_root, get_random_string(7), file_ext))
-
+            if max_length is None:
+                continue
+            # Truncate file_root if max_length exceeded.
+            truncation = len(name) - max_length
+            if truncation > 0:
+                file_root = file_root[:-truncation]
+                # Entire file_root was truncated in attempt to find an available filename.
+                if not file_root:
+                    raise SuspiciousFileOperation(
+                        'Storage can not find an available filename for "%s". '
+                        'Please make sure that the corresponding file field '
+                        'allows sufficient "max_length".' % name
+                    )
+                name = os.path.join(dir_name, "%s_%s%s" % (file_root, get_random_string(7), file_ext))
         return name
 
     def path(self, name):

django/db/models/fields/files.py

@@ -1,5 +1,7 @@
 import datetime
+from inspect import getargspec
 import os
+import warnings
 
 from django import forms
 from django.db.models.fields import Field
@@ -11,6 +13,7 @@
 from django.utils.encoding import force_str, force_text
 from django.utils import six
 from django.utils.translation import ugettext_lazy as _
+from django.utils.deprecation import RemovedInDjango20Warning
 
 
 class FieldFile(File):
@@ -85,7 +88,19 @@ def open(self, mode='rb'):
 
     def save(self, name, content, save=True):
         name = self.field.generate_filename(self.instance, name)
-        self.name = self.storage.save(name, content)
+
+        args, varargs, varkw, defaults = getargspec(self.storage.save)
+        if 'max_length' in args:
+            self.name = self.storage.save(name, content, max_length=self.field.max_length)
+        else:
+            warnings.warn(
+                'Backwards compatibility for storage backends without '
+                'support for the `max_length` argument in '
+                'Storage.save() will be removed in Django 2.0.',
+                RemovedInDjango20Warning, stacklevel=2
+            )
+            self.name = self.storage.save(name, content)
+
         setattr(self.instance, self.field.name, self.name)
 
         # Update the filesize cache

docs/howto/custom-file-storage.txt

@@ -50,7 +50,7 @@ typically have to be overridden:
 * :meth:`Storage.size`
 * :meth:`Storage.url`
 
-Note however that not all these methods are required and may be deliberately 
+Note however that not all these methods are required and may be deliberately
 omitted. As it happens, it is possible to leave each method unimplemented and
 still have a working Storage.
 
@@ -87,7 +87,6 @@ instead).
 
 .. method:: get_valid_name(name)
 
-
 Returns a filename suitable for use with the underlying storage system. The
 ``name`` argument passed to this method is the original filename sent to the
 server, after having any path information removed. Override this to customize
@@ -96,21 +95,28 @@ how non-standard characters are converted to safe filenames.
 The code provided on ``Storage`` retains only alpha-numeric characters, periods
 and underscores from the original filename, removing everything else.
 
-.. method:: get_available_name(name)
+.. method:: get_available_name(name, max_length=None)
 
 Returns a filename that is available in the storage mechanism, possibly taking
 the provided filename into account. The ``name`` argument passed to this method
 will have already cleaned to a filename valid for the storage system, according
 to the ``get_valid_name()`` method described above.
 
-.. versionchanged:: 1.7
+The length of the filename will not exceed ``max_length``, if provided. If a
+free unique filename cannot be found, a :exc:`SuspiciousFileOperation
+<django.core.exceptions.SuspiciousOperation>` exception is raised.
+
+If a file with ``name`` already exists, an underscore plus a random 7 character
+alphanumeric string is appended to the filename before the extension.
 
-    If a file with ``name`` already exists, an underscore plus a random 7
-    character alphanumeric string is appended to the filename before the
-    extension.
+.. versionchanged:: 1.7
 
     Previously, an underscore followed by a number (e.g. ``"_1"``, ``"_2"``,
     etc.) was appended to the filename until an available name in the destination
     directory was found. A malicious user could exploit this deterministic
     algorithm to create a denial-of-service attack. This change was also made
     in Django 1.6.6, 1.5.9, and 1.4.14.
+
+.. versionchanged:: 1.8
+
+    The ``max_length`` argument was added.

docs/internals/deprecation.txt

@@ -143,6 +143,10 @@ details on these changes.
 * Support for the ``=`` comparison operator in the ``if`` template tag will be
   removed.
 
+* The backwards compatibility shims to allow ``Storage.get_available_name()``
+  and ``Storage.save()`` to be defined without a ``max_length`` argument will
+  be removed.
+
 .. _deprecation-removed-in-1.9:
 
 1.9

docs/ref/files/storage.txt

@@ -114,12 +114,17 @@ The Storage Class
         in the storage system, or ``False`` if the name is available for a new
         file.
 
-    .. method:: get_available_name(name)
+    .. method:: get_available_name(name, max_length=None)
 
         Returns a filename based on the ``name`` parameter that's free and
         available for new content to be written to on the target storage
         system.
 
+        The length of the filename will not exceed ``max_length``, if provided.
+        If a free unique filename cannot be found, a
+        :exc:`SuspiciousFileOperation
+        <django.core.exceptions.SuspiciousOperation>` exception will be raised.
+
         If a file with ``name`` already exists, an underscore plus a random
         7 character alphanumeric string is appended to the filename before
         the extension.
@@ -133,6 +138,10 @@ The Storage Class
             attack. This change was also made in Django 1.6.6, 1.5.9, and
             1.4.14.
 
+        .. versionchanged:: 1.8
+
+            The ``max_length`` argument was added.
+
     .. method:: get_valid_name(name)
 
         Returns a filename based on the ``name`` parameter that's suitable
@@ -165,17 +174,24 @@ The Storage Class
         standard ``open()``. For storage systems that aren't accessible from
         the local filesystem, this will raise ``NotImplementedError`` instead.
 
-    .. method:: save(name, content)
+    .. method:: save(name, content, max_length=None)
 
         Saves a new file using the storage system, preferably with the name
         specified. If there already exists a file with this name ``name``, the
         storage system may modify the filename as necessary to get a unique
         name. The actual name of the stored file will be returned.
 
+        The ``max_length`` argument is passed along to
+        :meth:`get_available_name`.
+
         The ``content`` argument must be an instance of
         :class:`django.core.files.File` or of a subclass of
         :class:`~django.core.files.File`.
 
+        .. versionchanged:: 1.8
+
+            The ``max_length`` argument was added.
+
     .. method:: size(name)
 
         Returns the total size, in bytes, of the file referenced by ``name``.

docs/releases/1.8.txt

@@ -317,7 +317,15 @@ Email
 File Storage
 ^^^^^^^^^^^^
 
-* ...
+* :meth:`Storage.get_available_name()
+  <django.core.files.storage.Storage.get_available_name>` and
+  :meth:`Storage.save() <django.core.files.storage.Storage.save>`
+  now take a ``max_length`` argument to implement storage-level maximum
+  filename length constraints. Filenames exceeding this argument will get
+  truncated. This prevents a database error when appending a unique suffix to a
+  long filename that already exists on the storage. See the :ref:`deprecation
+  note <storage-max-length-update>` about adding this argument to your custom
+  storage classes.
 
 File Uploads
 ^^^^^^^^^^^^
@@ -1432,6 +1440,17 @@ loader that inherits ``BaseLoader``, you must inherit ``Loader`` instead.
 Private API ``django.test.utils.TestTemplateLoader`` is deprecated in favor of
 ``django.template.loaders.locmem.Loader``.
 
+.. _storage-max-length-update:
+
+Support for the ``max_length`` argument on custom ``Storage`` classes
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+``Storage`` subclasses should add ``max_length=None`` as a parameter to
+:meth:`~django.core.files.storage.Storage.get_available_name` and/or
+:meth:`~django.core.files.storage.Storage.save` if they override either method.
+Support for storages that do not accept this argument will be removed in
+Django 2.0.
+
 ``qn`` replaced by ``compiler``
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 

tests/file_storage/models.py

@@ -13,6 +13,19 @@
 from django.core.files.storage import FileSystemStorage
 
 
+class OldStyleFSStorage(FileSystemStorage):
+    """
+    Storage backend without support for the ``max_length`` argument in
+    ``get_available_name()`` and ``save()``; for backward-compatibility and
+    deprecation testing.
+    """
+    def get_available_name(self, name):
+        return name
+
+    def save(self, name, content):
+        return super(OldStyleFSStorage, self).save(name, content)
+
+
 temp_storage_location = tempfile.mkdtemp(dir=os.environ['DJANGO_TEST_TEMP_DIR'])
 temp_storage = FileSystemStorage(location=temp_storage_location)
 
@@ -31,3 +44,9 @@ def random_upload_to(self, filename):
     random = models.FileField(storage=temp_storage, upload_to=random_upload_to)
     default = models.FileField(storage=temp_storage, upload_to='tests', default='tests/default.txt')
     empty = models.FileField(storage=temp_storage)
+    limited_length = models.FileField(storage=temp_storage, upload_to='tests', max_length=20)
+    extended_length = models.FileField(storage=temp_storage, upload_to='tests', max_length=300)
+    old_style = models.FileField(
+        storage=OldStyleFSStorage(location=temp_storage_location),
+        upload_to='tests',
+    )