Ticket #2983: 2983.2.diff

tests/regressiontests/file_uploads/tests.py

@@ -12 +12 @@
 from django.utils.hashcompat import sha_constructor
 from django.http.multipartparser import MultiPartParser
 
-from models import FileModel, temp_storage, UPLOAD_TO
+from models import FileModel, FileModelDeleteReplaced, temp_storage, UPLOAD_TO
 import uploadhandler
 
 UNICODE_FILENAME = u'test-0123456789_中文_Orléans.jpg'
@@ -302 +302 @@
             'CONTENT_TYPE':     'multipart/form-data; boundary=_foo',
             'CONTENT_LENGTH':   '1'
         }, StringIO('x'), [], 'utf-8')
+
+class DeleteReplacedTests(TestCase):
+    def setUp(self):
+        if not os.path.isdir(temp_storage.location):
+            os.makedirs(temp_storage.location)
+        if os.path.isdir(UPLOAD_TO):
+            os.chmod(UPLOAD_TO, 0700)
+            shutil.rmtree(UPLOAD_TO)
+        self.file_a = SimpleUploadedFile('alpha.txt', 'A')
+        self.file_b = SimpleUploadedFile('beta.txt', 'B')
+        self.file_g = SimpleUploadedFile('gamma.txt', 'G')
+
+    def tearDown(self):
+        os.chmod(temp_storage.location, 0700)
+        shutil.rmtree(temp_storage.location)
+
+    def test_instance_track_replaced(self):
+        """
+        Setting a new file into a instance's ``FileField`` attribute keeps
+        track of the old file in the new file's ``_replaced`` list. 
+        """
+        obj = FileModel()
+        obj.testfile = self.file_a
+        fieldfile_a = obj.testfile
+        self.assertEqual(obj.testfile._replaced, [])
+        # After a save, nothing changes.
+        obj.save()
+        self.assertEqual(obj.testfile._replaced, [])
+        # Set to B, B replaces A
+        obj.testfile = self.file_b
+        fieldfile_b = obj.testfile
+        self.assertEqual(obj.testfile._replaced, [fieldfile_a])
+        # Set to G, G replaces B (which in turn replaces A)
+        obj.testfile = self.file_g
+        fieldfile_g = obj.testfile
+        self.assertEqual(obj.testfile._replaced, [fieldfile_b])
+        self.assertEqual(obj.testfile._replaced[0]._replaced, [fieldfile_a])
+
+    def test_default_on_delete(self):
+        """
+        Deleting a FieldFile which has a default FileField doesn't delete
+        replaced files.
+        """
+        obj = FileModel.objects.create(testfile=self.file_a)
+        obj.testfile = self.file_b
+        obj.testfile.delete()
+        self.assertEqual(os.listdir(UPLOAD_TO), ['alpha.txt'])
+
+    def test_delete_replaced_on_delete(self):
+        """
+        Deleting a FieldFile which its FileField set ``delete_replaced=True``
+        deletes replaced files (without any "safe" tests).
+        """
+        obj = FileModelDeleteReplaced.objects.create(testfile=self.file_a)
+        obj.testfile = self.file_b
+        obj.testfile.delete()
+        self.assertEqual(os.listdir(UPLOAD_TO), [])
+        # Even if another instance has a reference to the file it will still be
+        # deleted.
+        obj = FileModelDeleteReplaced.objects.create(testfile=self.file_a)
+        obj2 = FileModelDeleteReplaced.objects.create(testfile=obj.testfile)
+        obj.testfile = self.file_b
+        obj.testfile.delete()
+        self.assertEqual(os.listdir(UPLOAD_TO), [])
+
+    def test_default_on_safe_delete(self):
+        """
+        Calling ``safe_delete`` on a FieldFile which has a default FileField
+        doesn't delete replaced files.
+        """
+        obj = FileModel.objects.create(testfile=self.file_a)
+        obj.testfile = self.file_b
+        obj.testfile.safe_delete()
+        self.assertEqual(os.listdir(UPLOAD_TO), ['alpha.txt'])
+        # If another instance has a reference to the file it won't be deleted.
+        obj = FileModel.objects.create(testfile=self.file_b)
+        obj.testfile = self.file_g
+        obj2 = FileModel.objects.create(testfile=obj.testfile)
+        obj.testfile.safe_delete()
+        files = os.listdir(UPLOAD_TO)
+        files.sort()
+        self.assertEqual(files, ['alpha.txt', 'beta.txt', 'gamma.txt'])
+
+    def test_delete_replaced_on_safe_delete(self):
+        """
+        Deleting a FieldFile which its FileField set ``delete_replaced=True``
+        "safely" deletes replaced files.
+        """
+        obj = FileModelDeleteReplaced.objects.create(testfile=self.file_a)
+        obj.testfile = self.file_b
+        obj.testfile.safe_delete()
+        self.assertEqual(os.listdir(UPLOAD_TO), [])
+        # If another instance has a reference to the file it won't be deleted.
+        obj = FileModelDeleteReplaced.objects.create(testfile=self.file_b)
+        obj.testfile = self.file_g
+        obj2 = FileModelDeleteReplaced.objects.create(testfile=obj.testfile)
+        obj.testfile.safe_delete()
+        self.assertEqual(os.listdir(UPLOAD_TO), ['gamma.txt'])
+
+    def test_default_on_save(self):
+        """
+        Saving a FieldFile which has a default FileField doesn't delete
+        replaced files.
+        """
+        obj = FileModel.objects.create(testfile=self.file_a)
+        obj.testfile.save(name=self.file_b.name, content=self.file_b,
+                          save=False)
+        files = os.listdir(UPLOAD_TO)
+        files.sort()
+        self.assertEqual(files, ['alpha.txt', 'beta.txt'])
+
+    def test_delete_replaced_on_save(self):
+        """
+        Saving a FieldFile which its FileField set ``delete_replaced=True``
+        deletes replaced files.
+        
+        The ``safe_delete_replaced`` attribute defaults to ``True``, which
+        causes related files to be "safely" deleted. Setting to ``False``
+        causes related files to be deleted without any "safe" tests.
+        """
+        obj = FileModelDeleteReplaced.objects.create(testfile=self.file_a)
+        obj.testfile.save(name=self.file_b.name, content=self.file_b,
+                          save=False)
+        self.assertEqual(os.listdir(UPLOAD_TO), ['beta.txt'])
+        # If another instance has a reference to the file it won't be deleted.
+        obj = FileModelDeleteReplaced.objects.create(testfile=self.file_a)
+        obj2 = FileModelDeleteReplaced.objects.create(testfile=obj.testfile)
+        obj.testfile.save(name=self.file_g.name, content=self.file_g,
+                          save=False)
+        files = os.listdir(UPLOAD_TO)
+        files.sort()
+        self.assertEqual(files, ['alpha.txt', 'beta.txt', 'gamma.txt'])
+
+    def test_delete_replaced(self):
+        """
+        Calling the ``delete_replaced`` method of a ``FieldFile`` recursively
+        deletes replaced files (regardless of the `FileField``'s
+        ``delete_replaced`` attribute).
+        
+        The ``safe_delete`` argument defaults to ``True``, which causes related
+        files to be "safely" deleted. Setting to ``False`` causes related files
+        to be deleted without any "safe" tests.
+        """
+        obj = FileModel.objects.create(testfile=self.file_a)
+        obj.testfile = self.file_b
+        obj.save()
+        obj.testfile = self.file_g
+        obj.save()
+        files = os.listdir(UPLOAD_TO)
+        files.sort()
+        self.assertEqual(files, ['alpha.txt', 'beta.txt', 'gamma.txt'])
+        obj.testfile.delete_replaced()
+        self.assertEqual(os.listdir(UPLOAD_TO), ['gamma.txt'])
+
+    def test_replace_avoids_loop(self):
+        """
+        Avoid an infinite loop when A replaces B which replaced A
+        """
+        obj = FileModel.objects.create(testfile=self.file_a)
+        fieldfile_a = obj.testfile
+        obj.testfile = self.file_b
+        obj.save()
+        obj.testfile = fieldfile_a
+        obj.testfile.delete_replaced()
+        self.assertEqual(os.listdir(UPLOAD_TO), ['alpha.txt'])
+
+    def test_instance_delete(self):
+        """
+        Deleting an instance deletes replaced files. For backwards
+        compatibility, this is regardless of the `FileField``'s
+        ``delete_replaced`` attribute. Files are only deleted if no other
+        instance of the same model type references that file.
+        """
+        obj = FileModel.objects.create(testfile=self.file_a)
+        obj.delete()
+        self.assertEqual(os.listdir(UPLOAD_TO), [])
+
+        obj = FileModel.objects.create(testfile=self.file_a)
+        obj2 = FileModel.objects.create(testfile=obj.testfile)
+        self.assertEqual(os.listdir(UPLOAD_TO), ['alpha.txt'])
+
+    def test_default_instance_replace_file(self):
+        """
+        Saving a FieldFile which has a default FileField doesn't delete
+        replaced files when the instance is saved.
+        """
+        obj = FileModel.objects.create(testfile=self.file_a)
+        obj.testfile = self.file_b
+        obj.save()
+        files = os.listdir(UPLOAD_TO)
+        files.sort()
+        self.assertEqual(files, ['alpha.txt', 'beta.txt'])
+
+    def test_delete_replaced_instance_replace_file(self):
+        """
+        If the model's FileField sets ``delete_replaced=True``, replacing an
+        instance's file with another file will cause the old file to be deleted
+        when the instance is saved.
+        
+        Files are only deleted if no other instance of the same model type
+        references that file.
+        """
+        obj = FileModelDeleteReplaced.objects.create(testfile=self.file_a)
+        obj.testfile = self.file_b
+        obj.save()
+        self.assertEqual(os.listdir(UPLOAD_TO), ['beta.txt'])
+
+        obj2 = FileModelDeleteReplaced.objects.create(testfile=obj.testfile)
+        obj.testfile = self.file_g
+        obj.save()
+        files = os.listdir(UPLOAD_TO)
+        files.sort()
+        self.assertEqual(files, ['beta.txt', 'gamma.txt'])

django/db/models/fields/files.py

@@ -22 +22 @@
         self.field = field
         self.storage = field.storage
         self._committed = True
+        self._replaced = []
+        self.delete_replaced_files = self.field.delete_replaced
 
     def __eq__(self, other):
         # Older code may be expecting FileField values to be simple strings.
@@ -86 +88 @@
     # to further manipulate the underlying file, as well as update the
     # associated model instance.
 
-    def save(self, name, content, save=True):
+    def save(self, name, content, save=True, safe_delete_replaced=True):
+        if self.delete_replaced_files:
+            self.delete_replaced(safe_delete=safe_delete_replaced)
+            if self._committed:
+                # Delete this file as well (since we're saving a new one).
+                if safe_delete_replaced:
+                    self.safe_delete(save=False)
+                else:
+                    self.delete(save=False)
+
         name = self.field.generate_filename(self.instance, name)
         self.name = self.storage.save(name, content)
-        setattr(self.instance, self.field.name, self.name)
+        setattr(self.instance, self.field.name, self)
 
         # Update the filesize cache
         self._size = len(content)
@@ -100 +111 @@
             self.instance.save()
     save.alters_data = True
 
-    def delete(self, save=True):
+    def delete(self, save=True, _delete_replaced_files=None):
+        """
+        Deletes the file from the backend.
+        
+        If ``save`` is ``True`` (default), the file's instance will be saved
+        after the file is deleted.
+        """
+        if ((_delete_replaced_files is not None and _delete_replaced_files) or
+            (_delete_replaced_files is None and self.delete_replaced_files)):
+            self.delete_replaced(safe_delete=False)
+
         # Only close the file if it's already open, which we know by the
         # presence of self._file
         if hasattr(self, '_file'):
@@ -110 +131 @@
         self.storage.delete(self.name)
 
         self.name = None
-        setattr(self.instance, self.field.name, self.name)
+        setattr(self.instance, self.field.name, self)
 
         # Delete the filesize cache
         if hasattr(self, '_size'):
@@ -121 +142 @@
             self.instance.save()
     delete.alters_data = True
 
+    def safe_delete(self, save=True, queryset=None,
+                    _delete_replaced_files=None):
+        """
+        Deletes the file from the backend if no objects in the queryset
+        reference the file and it's not the default value for future objects.
+
+        Otherwise, the file is simply closed so it doesn't tie up resources.
+        
+        If ``save`` is ``True`` (default), the file's instance will be saved
+        if the file is deleted.
+
+        Under most circumstances, ``queryset`` does not need to be passed - 
+        it will be calculated based on the current instance.
+        """
+        if ((_delete_replaced_files is not None and _delete_replaced_files) or
+            (_delete_replaced_files is None and self.delete_replaced_files)):
+            self.delete_replaced(safe_delete=True)
+
+        if queryset is None:
+            queryset = self.instance._default_manager.all()
+            if self.instance.pk:
+                queryset = queryset.exclude(pk=self.instance.pk)
+        queryset = queryset.filter(**{self.field.name: self.name})
+
+        if self.name != self.field.default and not queryset:
+            self.delete(save=save, _delete_replaced_files=False)
+        else:
+            self.close()
+    safe_delete.alters_data = True
+
+    def delete_replaced(self, safe_delete=True, _seen=None):
+        seen = _seen or []
+        seen.append(self.name)
+        for file in self._replaced:
+            if file._committed and file.name not in seen:
+                file.delete_replaced(safe_delete=safe_delete, _seen=seen)
+                if safe_delete:
+                    file.safe_delete(save=False, _delete_replaced_files=False)
+                else:
+                    file.delete(save=False, _delete_replaced_files=False)
+        self._replaced = []
+    delete_replaced.alters_data = True
+
     def _get_closed(self):
         file = getattr(self, '_file', None)
         return file is None or file.closed
@@ -136 +200 @@
         # 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, '_file': None}
+        return {'name': self.name, 'closed': False, '_committed': True,
+                '_file': None,
+                'delete_replaced_files': self.delete_replaced_files}
 
 class FileDescriptor(object):
     """
@@ -194 +260 @@
             file_copy._committed = False
             instance.__dict__[self.field.name] = file_copy
 
-        # Finally, because of the (some would say boneheaded) way pickle works,
-        # the underlying FieldFile might not actually itself have an associated
-        # file. So we need to reset the details of the FieldFile in those cases.
+        # Because of the (some would say boneheaded) way pickle works, the
+        # underlying FieldFile might not actually itself have an associated
+        # file. So we need to reset the details of the FieldFile in those
+        # cases.
         elif isinstance(file, FieldFile) and not hasattr(file, 'field'):
             file.instance = instance
             file.field = self.field
             file.storage = self.field.storage
 
+        # Finally, the file set may have been a FieldFile from another
+        # instance, so copy it if the instance doesn't match.
+        elif isinstance(file, FieldFile) and file.instance != instance:
+            file_copy = self.field.attr_class(instance, self.field, file.name)
+            file_copy.file = file
+            file_copy._committed = file._committed
+            instance.__dict__[self.field.name] = file_copy
+
         # That was fun, wasn't it?
         return instance.__dict__[self.field.name]
 
     def __set__(self, instance, value):
+        if self.field.name in instance.__dict__:
+            previous_file = getattr(instance, self.field.name)
+        else:
+            previous_file = None
         instance.__dict__[self.field.name] = value
+        if previous_file:
+            # Rather than just using value, we get the file from the instance,
+            # so that the __get__ logic of the file descriptor is processed.
+            # This ensures we will be dealing with a FileField (or subclass of
+            # FileField) instance.
+            file = getattr(instance, self.field.name)
+            if previous_file is not file:
+                # Remember that the previous file was replaced.
+                file._replaced.append(previous_file)
 
 class FileField(Field):
     # The class to wrap instance attributes in. Accessing the file object off
@@ -216 +304 @@
     # The descriptor to use for accessing the attribute off of the class.
     descriptor_class = FileDescriptor
 
-    def __init__(self, verbose_name=None, name=None, upload_to='', storage=None, **kwargs):
+    def __init__(self, verbose_name=None, name=None, upload_to='',
+                 storage=None, delete_replaced=False, **kwargs):
         for arg in ('primary_key', 'unique'):
             if arg in kwargs:
                 raise TypeError("'%s' is not a valid argument for %s." % (arg, self.__class__))
@@ -225 +314 @@
         self.upload_to = upload_to
         if callable(upload_to):
             self.generate_filename = upload_to
+        self.delete_replaced = delete_replaced
 
         kwargs['max_length'] = kwargs.get('max_length', 100)
         super(FileField, self).__init__(verbose_name, name, **kwargs)
@@ -258 +348 @@
         signals.post_delete.connect(self.delete_file, sender=cls)
 
     def delete_file(self, instance, sender, **kwargs):
+        """
+        Signal receiver which deletes an attached file from the backend when
+        the model is deleted.
+        """
         file = getattr(instance, self.attname)
-        # If no other object of this type references the file,
-        # and it's not the default value for future objects,
-        # delete it from the backend.
-        if file and file.name != self.default and \
-            not sender._default_manager.filter(**{self.name: file.name}):
-                file.delete(save=False)
-        elif file:
-            # Otherwise, just close the file, so it doesn't tie up resources.
-            file.close()
+        if file:
+            file.safe_delete(save=False,
+                             queryset=sender._default_manager.all())
 
     def get_directory_name(self):
         return os.path.normpath(force_unicode(datetime.datetime.now().strftime(smart_str(self.upload_to))))

tests/regressiontests/file_uploads/models.py

@@ -4 +4 @@
 from django.core.files.storage import FileSystemStorage
 
 temp_storage = FileSystemStorage(tempfile.mkdtemp())
-UPLOAD_TO = os.path.join(temp_storage.location, 'test_upload')
+UPLOAD_TO_NAME = 'test_upload'
+UPLOAD_TO = os.path.join(temp_storage.location, UPLOAD_TO_NAME)
 
 class FileModel(models.Model):
-    testfile = models.FileField(storage=temp_storage, upload_to='test_upload')
+    testfile = models.FileField(storage=temp_storage, upload_to=UPLOAD_TO_NAME)
+
+class FileModelDeleteReplaced(models.Model):
+    testfile = models.FileField(storage=temp_storage, upload_to=UPLOAD_TO_NAME,
+                                delete_replaced=True)