Fix (beta): avoid DRF update calling save on all fields (#12992)

src/documents/serialisers.py

@@ -48,6 +48,7 @@ from rest_framework import serializers
 from rest_framework.exceptions import PermissionDenied
 from rest_framework.fields import SerializerMethodField
 from rest_framework.filters import OrderingFilter
+from rest_framework.utils import model_meta
 
 if settings.AUDIT_LOG_ENABLED:
     from auditlog.context import set_actor
@@ -121,6 +122,45 @@ class DynamicFieldsModelSerializer(serializers.ModelSerializer[Any]):
                 self.fields.pop(field_name)
 
 
+class DocumentUpdateFieldsModelSerializer(DynamicFieldsModelSerializer):
+    stale_update_excluded_fields = frozenset({"filename", "archive_filename"})
+
+    def _get_update_fields(self, validated_data) -> list[str]:
+        model_fields = {
+            field.name
+            for field in self.Meta.model._meta.concrete_fields
+            if field.name not in self.stale_update_excluded_fields
+        }
+        update_fields = [
+            field_name for field_name in validated_data if field_name in model_fields
+        ]
+        if "modified" in model_fields and "modified" not in update_fields:
+            update_fields.append("modified")
+        return update_fields
+
+    def update(self, instance, validated_data):
+        serializers.raise_errors_on_nested_writes("update", self, validated_data)
+        info = model_meta.get_field_info(instance)
+
+        m2m_fields = []
+        for attr, value in validated_data.items():
+            if attr in info.relations and info.relations[attr].to_many:
+                m2m_fields.append((attr, value))
+            else:
+                setattr(instance, attr, value)
+
+        # File names are managed by post-save file handling.  Saving only the
+        # serializer-updated fields prevents stale in-memory path values from
+        # overwriting a concurrent move.
+        instance.save(update_fields=self._get_update_fields(validated_data))
+
+        for attr, value in m2m_fields:
+            field = getattr(instance, attr)
+            field.set(value)
+
+        return instance
+
+
 class MatchingModelSerializer(serializers.ModelSerializer[Any]):
     document_count = serializers.IntegerField(read_only=True)
 
@@ -989,7 +1029,7 @@ class DocumentVersionInfoSerializer(serializers.Serializer[_DocumentVersionInfo]
 class DocumentSerializer(
     OwnedObjectSerializer,
     NestedUpdateMixin,
-    DynamicFieldsModelSerializer,
+    DocumentUpdateFieldsModelSerializer,
 ):
     correspondent = CorrespondentField(allow_null=True)
     tags = TagsField(many=True)
@@ -1128,10 +1168,9 @@ class DocumentSerializer(
         return super().validate(attrs)
 
     def update(self, instance: Document, validated_data):
-        if "created_date" in validated_data and "created" not in validated_data:
-            instance.created = validated_data.get("created_date")
-            instance.save()
         if "created_date" in validated_data:
+            if "created" not in validated_data:
+                validated_data["created"] = validated_data["created_date"]
             logger.warning(
                 "created_date is deprecated, use created instead",
             )
@@ -1201,11 +1240,13 @@ class DocumentSerializer(
                     for tag in instance.tags.all()
                     if tag not in inbox_tags_not_being_added
                 ]
+
         if settings.AUDIT_LOG_ENABLED:
             with set_actor(self.user):
                 super().update(instance, validated_data)
         else:
             super().update(instance, validated_data)
+
         # hard delete custom field instances that were soft deleted
         CustomFieldInstance.deleted_objects.filter(document=instance).delete()
         return instance

src/documents/tests/test_file_handling.py

@@ -24,6 +24,7 @@ from documents.models import CustomFieldInstance
 from documents.models import Document
 from documents.models import DocumentType
 from documents.models import StoragePath
+from documents.serialisers import DocumentSerializer
 from documents.tasks import empty_trash
 from documents.tests.factories import DocumentFactory
 from documents.tests.utils import DirectoriesMixin
@@ -251,6 +252,46 @@ class TestFileHandling(DirectoriesMixin, FileSystemAssertsMixin, TestCase):
         self.assertIsNotFile(settings.ORIGINALS_DIR / "old" / "document.pdf")
         self.assertIsNotFile(settings.ARCHIVE_DIR / "old" / "document.pdf")
 
+    @override_settings(FILENAME_FORMAT="{title}")
+    def test_serializer_stale_update_does_not_clobber_filename(self) -> None:
+        old_path = settings.ORIGINALS_DIR / "original.pdf"
+        old_path.touch()
+        doc = Document.objects.create(
+            title="original",
+            mime_type="application/pdf",
+            checksum=hashlib.sha256(b"").hexdigest(),
+            filename="original.pdf",
+        )
+
+        first_instance = Document.objects.get(pk=doc.pk)
+        stale_instance = Document.objects.get(pk=doc.pk)
+
+        serializer = DocumentSerializer(
+            first_instance,
+            data={"title": "first"},
+            partial=True,
+        )
+        self.assertTrue(serializer.is_valid(), serializer.errors)
+        serializer.save()
+
+        doc.refresh_from_db()
+        self.assertEqual(doc.filename, "first.pdf")
+        self.assertIsFile(settings.ORIGINALS_DIR / "first.pdf")
+
+        serializer = DocumentSerializer(
+            stale_instance,
+            data={"title": "second"},
+            partial=True,
+        )
+        self.assertTrue(serializer.is_valid(), serializer.errors)
+        serializer.save()
+
+        doc.refresh_from_db()
+        self.assertEqual(doc.filename, "second.pdf")
+        self.assertIsFile(settings.ORIGINALS_DIR / "second.pdf")
+        self.assertIsNotFile(settings.ORIGINALS_DIR / "first.pdf")
+        self.assertIsNotFile(old_path)
+
     @override_settings(FILENAME_FORMAT="{correspondent}/{correspondent}")
     def test_document_delete(self) -> None:
         document = Document()