Fixed #10258: handle duplicate file names better.

Instead of just continually appending "_" to duplicate file names, Django's
default storage now appends `_1`, `_2`, `_3`, etc.

Thanks to ianschenck and Thilo.

git-svn-id: http://code.djangoproject.com/svn/django/trunk@12552 bcc190cf-cafb-0310-a4f2-bffc1f526a37

4 files changed, 29 insertions, 16 deletions

diff --git a/django/core/files/storage.py b/django/core/files/storage.py
index 96c0b54623..d312681c28 100644
--- a/django/core/files/storage.py
+++ b/django/core/files/storage.py
@@ -1,6 +1,7 @@
 import os
 import errno
 import urlparse
+import itertools
 
 from django.conf import settings
 from django.core.exceptions import ImproperlyConfigured, SuspiciousOperation
@@ -65,13 +66,14 @@ class Storage(object):
         """
         dir_name, file_name = os.path.split(name)
         file_root, file_ext = os.path.splitext(file_name)
-        # If the filename already exists, keep adding an underscore (before the
-        # file extension, if one exists) to the filename until the generated
+        # If the filename already exists, add an underscore and a number (before
+        # the file extension, if one exists) to the filename until the generated
         # filename doesn't exist.
+        count = itertools.count(1)
         while self.exists(name):
-            file_root += '_'
             # file_ext includes the dot.
-            name = os.path.join(dir_name, file_root + file_ext)
+            name = os.path.join(dir_name, "%s_%s%s" % (file_root, count.next(), file_ext))
+
         return name
 
     def path(self, name):
diff --git a/docs/howto/custom-file-storage.txt b/docs/howto/custom-file-storage.txt
index cfb3226289..5005feaa80 100644
--- a/docs/howto/custom-file-storage.txt
+++ b/docs/howto/custom-file-storage.txt
@@ -88,5 +88,5 @@ 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.
 
-The code provided on ``Storage`` simply appends underscores to the filename
-until it finds one that's available in the destination directory.
+The code provided on ``Storage`` simply appends ``"_1"``, ``"_2"``, etc. to the
+filename until it finds one that's available in the destination directory.
diff --git a/tests/modeltests/files/models.py b/tests/modeltests/files/models.py
index beea97e808..271fd8de9e 100644
--- a/tests/modeltests/files/models.py
+++ b/tests/modeltests/files/models.py
@@ -5,14 +5,11 @@
 and where files should be stored.
 """
 
-import shutil
 import random
 import tempfile
 from django.db import models
 from django.core.files.base import ContentFile
-from django.core.files.uploadedfile import SimpleUploadedFile
 from django.core.files.storage import FileSystemStorage
-from django.core.cache import cache
 
 temp_storage_location = tempfile.mkdtemp()
 temp_storage = FileSystemStorage(location=temp_storage_location)
@@ -64,6 +61,7 @@ ValueError: The 'normal' attribute has no file associated with it.
 # File objects can be assigned to FileField attributes,  but shouldn't get
 # committed until the model it's attached to is saved.
 
+>>> from django.core.files.uploadedfile import SimpleUploadedFile
 >>> obj1.normal = SimpleUploadedFile('assignment.txt', 'content')
 >>> dirs, files = temp_storage.listdir('tests')
 >>> dirs
@@ -93,16 +91,17 @@ ValueError: The 'normal' attribute has no file associated with it.
 >>> obj2 = Storage()
 >>> obj2.normal.save('django_test.txt', ContentFile('more content'))
 >>> obj2.normal
-<FieldFile: tests/django_test_.txt>
+<FieldFile: tests/django_test_1.txt>
 >>> obj2.normal.size
 12
 
 # Push the objects into the cache to make sure they pickle properly
 
+>>> from django.core.cache import cache
 >>> cache.set('obj1', obj1)
 >>> cache.set('obj2', obj2)
 >>> cache.get('obj2').normal
-<FieldFile: tests/django_test_.txt>
+<FieldFile: tests/django_test_1.txt>
 
 # Deleting an object deletes the file it uses, if there are no other objects
 # still using that file.
@@ -110,7 +109,17 @@ ValueError: The 'normal' attribute has no file associated with it.
 >>> obj2.delete()
 >>> obj2.normal.save('django_test.txt', ContentFile('more content'))
 >>> obj2.normal
-<FieldFile: tests/django_test_.txt>
+<FieldFile: tests/django_test_1.txt>
+
+# Multiple files with the same name get _N appended to them.
+
+>>> objs = [Storage() for i in range(3)] 
+>>> for o in objs:  
+...     o.normal.save('multiple_files.txt', ContentFile('Same Content')) 
+>>> [o.normal for o in objs]
+[<FieldFile: tests/multiple_files.txt>, <FieldFile: tests/multiple_files_1.txt>, <FieldFile: tests/multiple_files_2.txt>]
+>>> for o in objs: 
+...     o.delete() 
 
 # Default values allow an object to access a single file.
 
@@ -139,5 +148,7 @@ ValueError: The 'normal' attribute has no file associated with it.
 >>> obj2.normal.delete()
 >>> obj3.default.delete()
 >>> obj4.random.delete()
+
+>>> import shutil
 >>> shutil.rmtree(temp_storage_location)
 """}
diff --git a/tests/regressiontests/file_storage/tests.py b/tests/regressiontests/file_storage/tests.py
index 81ea48950c..b1e31d1811 100644
--- a/tests/regressiontests/file_storage/tests.py
+++ b/tests/regressiontests/file_storage/tests.py
@@ -126,9 +126,9 @@ class FileSaveRaceConditionTest(TestCase):
         name = self.save_file('conflict')
         self.thread.join()
         self.assert_(self.storage.exists('conflict'))
-        self.assert_(self.storage.exists('conflict_'))
+        self.assert_(self.storage.exists('conflict_1'))
         self.storage.delete('conflict')
-        self.storage.delete('conflict_')
+        self.storage.delete('conflict_1')
 
 class FileStoragePermissions(TestCase):
     def setUp(self):
@@ -167,7 +167,7 @@ class FileStoragePathParsing(TestCase):
 
         self.failIf(os.path.exists(os.path.join(self.storage_dir, 'dotted_.path')))
         self.assert_(os.path.exists(os.path.join(self.storage_dir, 'dotted.path/test')))
-        self.assert_(os.path.exists(os.path.join(self.storage_dir, 'dotted.path/test_')))
+        self.assert_(os.path.exists(os.path.join(self.storage_dir, 'dotted.path/test_1')))
 
     def test_first_character_dot(self):
         """
@@ -183,7 +183,7 @@ class FileStoragePathParsing(TestCase):
         if sys.version_info < (2, 6):
             self.assert_(os.path.exists(os.path.join(self.storage_dir, 'dotted.path/_.test')))
         else:
-            self.assert_(os.path.exists(os.path.join(self.storage_dir, 'dotted.path/.test_')))
+            self.assert_(os.path.exists(os.path.join(self.storage_dir, 'dotted.path/.test_1')))
 
 if Image is not None:
     class DimensionClosingBug(TestCase):