Performance: avoid unnecessary filename operations on bulk custom field updates ...

diff --git a/src/documents/signals/handlers.py b/src/documents/signals/handlers.py
index 6cafbc1f8a92eab080dd9fb56873b3d0c5b8a11e..5f2c8b4b288b6aae7e344ef492f9b94ff0fcd7e2 100644 (file)
--- a/src/documents/signals/handlers.py
+++ b/src/documents/signals/handlers.py
@@ -28,6 +28,7 @@ from documents import matching
 from documents.caching import clear_document_caches
 from documents.file_handling import create_source_path_directory
 from documents.file_handling import delete_empty_directories
+from documents.file_handling import generate_filename
 from documents.file_handling import generate_unique_filename
 from documents.models import CustomField
 from documents.models import CustomFieldInstance
@@ -42,6 +43,7 @@ from documents.models import WorkflowAction
 from documents.models import WorkflowRun
 from documents.models import WorkflowTrigger
 from documents.permissions import get_objects_for_user_owner_aware
+from documents.templating.utils import convert_format_str_to_template_format
 from documents.workflows.actions import build_workflow_action_context
 from documents.workflows.actions import execute_email_action
 from documents.workflows.actions import execute_webhook_action
@@ -389,6 +391,19 @@ class CannotMoveFilesException(Exception):
     pass
 
 
+def _filename_template_uses_custom_fields(doc: Document) -> bool:
+    template = None
+    if doc.storage_path is not None:
+        template = doc.storage_path.path
+    elif settings.FILENAME_FORMAT is not None:
+        template = convert_format_str_to_template_format(settings.FILENAME_FORMAT)
+
+    if not template:
+        return False
+
+    return "custom_fields" in template
+
+
 # should be disabled in /src/documents/management/commands/document_importer.py handle
 @receiver(models.signals.post_save, sender=CustomFieldInstance, weak=False)
 @receiver(models.signals.m2m_changed, sender=Document.tags.through, weak=False)
@@ -399,6 +414,8 @@ def update_filename_and_move_files(
     **kwargs,
 ):
     if isinstance(instance, CustomFieldInstance):
+        if not _filename_template_uses_custom_fields(instance.document):
+            return
         instance = instance.document
 
     def validate_move(instance, old_path: Path, new_path: Path):
@@ -436,21 +453,47 @@ def update_filename_and_move_files(
             old_filename = instance.filename
             old_source_path = instance.source_path
 
+            candidate_filename = generate_filename(instance)
+            candidate_source_path = (
+                settings.ORIGINALS_DIR / candidate_filename
+            ).resolve()
+            if candidate_filename == Path(old_filename):
+                new_filename = Path(old_filename)
+            elif (
+                candidate_source_path.exists()
+                and candidate_source_path != old_source_path
+            ):
+                # Only fall back to unique search when there is an actual conflict
+                new_filename = generate_unique_filename(instance)
+            else:
+                new_filename = candidate_filename
+
             # Need to convert to string to be able to save it to the db
-            instance.filename = str(generate_unique_filename(instance))
+            instance.filename = str(new_filename)
             move_original = old_filename != instance.filename
 
             old_archive_filename = instance.archive_filename
             old_archive_path = instance.archive_path
 
             if instance.has_archive_version:
-                # Need to convert to string to be able to save it to the db
-                instance.archive_filename = str(
-                    generate_unique_filename(
+                archive_candidate = generate_filename(instance, archive_filename=True)
+                archive_candidate_path = (
+                    settings.ARCHIVE_DIR / archive_candidate
+                ).resolve()
+                if archive_candidate == Path(old_archive_filename):
+                    new_archive_filename = Path(old_archive_filename)
+                elif (
+                    archive_candidate_path.exists()
+                    and archive_candidate_path != old_archive_path
+                ):
+                    new_archive_filename = generate_unique_filename(
                         instance,
                         archive_filename=True,
-                    ),
-                )
+                    )
+                else:
+                    new_archive_filename = archive_candidate
+
+                instance.archive_filename = str(new_archive_filename)
 
                 move_archive = old_archive_filename != instance.archive_filename
             else:

diff --git a/src/documents/tests/test_file_handling.py b/src/documents/tests/test_file_handling.py
index a1fd3d67441eed132200e9149653976f8ace566a..befc7050fddb7a269b527cca02a9c185119c1a97 100644 (file)
--- a/src/documents/tests/test_file_handling.py
+++ b/src/documents/tests/test_file_handling.py
@@ -16,6 +16,7 @@ from django.utils import timezone
 from documents.file_handling import create_source_path_directory
 from documents.file_handling import delete_empty_directories
 from documents.file_handling import generate_filename
+from documents.file_handling import generate_unique_filename
 from documents.models import Correspondent
 from documents.models import CustomField
 from documents.models import CustomFieldInstance
@@ -1632,6 +1633,73 @@ class TestFilenameGeneration(DirectoriesMixin, TestCase):
             )
 
 
+class TestCustomFieldFilenameUpdates(
+    DirectoriesMixin,
+    FileSystemAssertsMixin,
+    TestCase,
+):
+    def setUp(self):
+        self.cf = CustomField.objects.create(
+            name="flavor",
+            data_type=CustomField.FieldDataType.STRING,
+        )
+        self.doc = Document.objects.create(
+            title="document",
+            mime_type="application/pdf",
+            checksum="abc123",
+        )
+        self.cfi = CustomFieldInstance.objects.create(
+            field=self.cf,
+            document=self.doc,
+            value_text="initial",
+        )
+        return super().setUp()
+
+    @override_settings(FILENAME_FORMAT=None)
+    def test_custom_field_not_in_template_skips_filename_work(self):
+        storage_path = StoragePath.objects.create(path="{{created}}/{{ title }}")
+        self.doc.storage_path = storage_path
+        self.doc.save()
+        initial_filename = generate_filename(self.doc)
+        Document.objects.filter(pk=self.doc.pk).update(filename=str(initial_filename))
+        self.doc.refresh_from_db()
+        Path(self.doc.source_path).parent.mkdir(parents=True, exist_ok=True)
+        Path(self.doc.source_path).touch()
+
+        with mock.patch("documents.signals.handlers.generate_unique_filename") as m:
+            m.side_effect = generate_unique_filename
+            self.cfi.value_text = "updated"
+            self.cfi.save()
+
+        self.doc.refresh_from_db()
+        self.assertEqual(Path(self.doc.filename), initial_filename)
+        self.assertEqual(m.call_count, 0)
+
+    @override_settings(FILENAME_FORMAT=None)
+    def test_custom_field_in_template_triggers_filename_update(self):
+        storage_path = StoragePath.objects.create(
+            path="{{ custom_fields|get_cf_value('flavor') }}/{{ title }}",
+        )
+        self.doc.storage_path = storage_path
+        self.doc.save()
+        initial_filename = generate_filename(self.doc)
+        Document.objects.filter(pk=self.doc.pk).update(filename=str(initial_filename))
+        self.doc.refresh_from_db()
+        Path(self.doc.source_path).parent.mkdir(parents=True, exist_ok=True)
+        Path(self.doc.source_path).touch()
+
+        with mock.patch("documents.signals.handlers.generate_unique_filename") as m:
+            m.side_effect = generate_unique_filename
+            self.cfi.value_text = "updated"
+            self.cfi.save()
+
+        self.doc.refresh_from_db()
+        expected_filename = Path("updated/document.pdf")
+        self.assertEqual(Path(self.doc.filename), expected_filename)
+        self.assertTrue(Path(self.doc.source_path).is_file())
+        self.assertLessEqual(m.call_count, 1)
+
+
 class TestPathDateLocalization:
     """
     Groups all tests related to the `localize_date` function.