update file renaming logic

src/documents/signals/handlers.py

@@ -208,21 +208,16 @@ def update_filename_and_move_files(sender, instance, **kwargs):
     with FileLock(settings.MEDIA_LOCK):
         old_filename = instance.filename
         new_filename = generate_unique_filename(instance)
-
+        move_original = old_filename != new_filename
-        if new_filename == instance.filename:
-            # Don't do anything if its the same.
-            return
-
         old_source_path = instance.source_path
         new_source_path = os.path.join(settings.ORIGINALS_DIR, new_filename)
 
-        if not validate_move(instance, old_source_path, new_source_path):
+        if move_original and not validate_move(instance, old_source_path, new_source_path):
             return
 
-        # archive files are optional, archive checksum tells us if we have one,
-        # since this is None for documents without archived files.
-        if instance.has_archive_version:
         old_archive_filename = instance.archive_filename
+
+        if instance.has_archive_version:
             new_archive_filename = generate_unique_filename(
                 instance, archive_filename=True
             )
@@ -230,25 +225,26 @@ def update_filename_and_move_files(sender, instance, **kwargs):
             new_archive_path = os.path.join(settings.ARCHIVE_DIR,
                                             new_archive_filename)
 
-            if not validate_move(instance, old_archive_path, new_archive_path):
+            move_archive = old_archive_filename != new_archive_filename
+            if move_archive and not validate_move(instance, old_archive_path, new_archive_path):
+                return
+        else:
+            move_archive = False
+
+        if not move_original and not move_archive:
+            # Don't do anything if filenames did not change.
             return
 
-            create_source_path_directory(new_archive_path)
-        else:
-            old_archive_filename = None
-            new_archive_filename = None
-            old_archive_path = None
-            new_archive_path = None
-
-        create_source_path_directory(new_source_path)
-
         try:
-            os.rename(old_source_path, new_source_path)
+            if move_original:
                 instance.filename = new_filename
+                create_source_path_directory(new_source_path)
+                os.rename(old_source_path, new_source_path)
 
-            if instance.has_archive_version:
+            if move_archive:
-                os.rename(old_archive_path, new_archive_path)
                 instance.archive_filename = new_archive_filename
+                create_source_path_directory(new_archive_path)
+                os.rename(old_archive_path, new_archive_path)
 
             # Don't save() here to prevent infinite recursion.
             Document.objects.filter(pk=instance.pk).update(
@@ -256,16 +252,24 @@ def update_filename_and_move_files(sender, instance, **kwargs):
                 archive_filename=instance.archive_filename,
             )
 
-        except OSError as e:
+        except Exception as e:
+            # This happens when either:
+            #  - moving the files failed due to file system errors
+            #  - saving to the database failed due to database errors
+            # In both cases, we need to revert to the original state.
+
+            # restore old values on the instance
             instance.filename = old_filename
             instance.archive_filename = old_archive_filename
-            # this happens when we can't move a file. If that's the case for
+
-            # the archive file, we try our best to revert the changes.
+            # Try to move files to their original location.
-            # no need to save the instance, the update() has not happened yet.
             try:
+                if move_original and os.path.isfile(new_source_path):
                     os.rename(new_source_path, old_source_path)
-                if instance.has_archive_version:
+
+                if move_archive and os.path.isfile(new_archive_path):
                     os.rename(new_archive_path, old_archive_path)
+
             except Exception as e:
                 # This is fine, since:
                 # A: if we managed to move source from A to B, we will also
@@ -276,17 +280,6 @@ def update_filename_and_move_files(sender, instance, **kwargs):
                 # B: if moving the orignal file failed, nothing has changed
                 #  anyway.
                 pass
-        except DatabaseError as e:
-            # this happens after moving files, so move them back into place.
-            # since moving them once succeeded, it's very likely going to
-            # succeed again.
-            os.rename(new_source_path, old_source_path)
-            if instance.has_archive_version:
-                os.rename(new_archive_path, old_archive_path)
-            instance.filename = old_filename
-            instance.archive_filename = old_archive_filename
-            # again, no need to save the instance, since the actual update()
-            # operation failed.
 
         # finally, remove any empty sub folders. This will do nothing if
         # something has failed above.

src/documents/tests/test_file_handling.py

@@ -446,6 +446,18 @@ class TestFileHandling(DirectoriesMixin, TestCase):
         self.assertEqual(document2.filename, "qwe.pdf")
 
 
+    @override_settings(PAPERLESS_FILENAME_FORMAT="{title}")
+    @mock.patch("documents.signals.handlers.Document.objects.filter")
+    def test_no_update_without_change(self, m):
+        doc = Document.objects.create(title="document", filename="document.pdf", archive_filename="document.pdf", checksum="A", archive_checksum="B", mime_type="application/pdf")
+        Path(doc.source_path).touch()
+        Path(doc.archive_path).touch()
+
+        doc.save()
+
+        m.assert_not_called()
+
+
 
 class TestFileHandlingWithArchive(DirectoriesMixin, TestCase):
 
@@ -558,6 +570,7 @@ class TestFileHandlingWithArchive(DirectoriesMixin, TestCase):
         Path(archive).touch()
         doc = Document.objects.create(mime_type="application/pdf", title="my_doc", filename="0000001.pdf", checksum="A", archive_checksum="B", archive_filename="0000001.pdf")
 
+        m.assert_called()
         self.assertTrue(os.path.isfile(original))
         self.assertTrue(os.path.isfile(archive))
         self.assertTrue(os.path.isfile(doc.source_path))
@@ -595,6 +608,7 @@ class TestFileHandlingWithArchive(DirectoriesMixin, TestCase):
         Path(archive).touch()
         doc = Document.objects.create(mime_type="application/pdf", title="my_doc", filename="0000001.pdf", archive_filename="0000001.pdf", checksum="A", archive_checksum="B")
 
+        m.assert_called()
         self.assertTrue(os.path.isfile(original))
         self.assertTrue(os.path.isfile(archive))
         self.assertTrue(os.path.isfile(doc.source_path))

src/documents/tests/test_migration_archive_files.py

@@ -260,6 +260,7 @@ class TestMigrateArchiveFilesErrors(DirectoriesMixin, TestMigrations):
         self.assertIsNone(doc1.archive_filename)
         self.assertIsNone(doc2.archive_filename)
 
+
 @override_settings(PAPERLESS_FILENAME_FORMAT="")
 class TestMigrateArchiveFilesBackwards(DirectoriesMixin, TestMigrations):