diff --git a/src/documents/consumer.py b/src/documents/consumer.py index 94efd5f69a..bb60d21085 100644 --- a/src/documents/consumer.py +++ b/src/documents/consumer.py @@ -762,7 +762,7 @@ class ConsumerPlugin( # Don't save with the lock active. Saving will cause the file # renaming logic to acquire the lock as well. # This triggers things like file renaming - document.save() + document.save(write_file_paths=True) if document.root_document_id: document_updated.send( diff --git a/src/documents/models.py b/src/documents/models.py index f5a9f6ddda..45dbff9fc1 100644 --- a/src/documents/models.py +++ b/src/documents/models.py @@ -1,6 +1,7 @@ import datetime import uuid from pathlib import Path +from typing import Any from typing import Final import pathvalidate @@ -157,6 +158,12 @@ class StoragePath(MatchingModel): class Document(SoftDeleteModel, ModelWithOwner): # type: ignore[django-manager-missing] MAX_STORED_FILENAME_LENGTH: Final[int] = 1024 + # Written only by the file mover, the consumer's file placement and the + # archiver. A full save() of a stale instance must never write them, or + # the DB can point at a path the file has already been moved away from. + FILE_PATH_FIELDS: Final[frozenset[str]] = frozenset( + {"filename", "archive_filename"}, + ) # Per-component limit of common filesystems (ext4, XFS, btrfs, ZFS, APFS) MAX_PUBLIC_FILENAME_BYTES: Final[int] = 255 @@ -368,6 +375,31 @@ class Document(SoftDeleteModel, ModelWithOwner): # type: ignore[django-manager- res += f" {self.title}" return res + def save(self, *, write_file_paths: bool = False, **kwargs: Any) -> None: + """ + Save the document without writing filename / archive_filename on an + existing row, unless write_file_paths=True or update_fields names them. + + This mirrors how Django narrows a save on a deferred instance: only the + loaded, concrete, non-generated, non-primary-key fields are written. + """ + if ( + not write_file_paths + and not self._state.adding + and kwargs.get("update_fields") is None + and not kwargs.get("force_insert", False) + ): + deferred = self.get_deferred_fields() + kwargs["update_fields"] = [ + field.attname + for field in self._meta.concrete_fields + if not field.primary_key + and not field.generated + and field.name not in self.FILE_PATH_FIELDS + and field.attname not in deferred + ] + super().save(**kwargs) + def get_effective_barcodes(self) -> list["DocumentBarcode"]: """ Returns the stored barcodes for the document, like diff --git a/src/documents/tests/test_bulk_edit.py b/src/documents/tests/test_bulk_edit.py index b341f57444..fd7b68255a 100644 --- a/src/documents/tests/test_bulk_edit.py +++ b/src/documents/tests/test_bulk_edit.py @@ -739,7 +739,7 @@ class TestPDFActions(DirectoriesMixin, TestCase): mime_type="application/pdf", ) self.doc1.archive_filename = sample1_archive - self.doc1.save() + self.doc1.save(write_file_paths=True) self.doc2 = Document.objects.create( checksum="B", title="B", @@ -749,7 +749,7 @@ class TestPDFActions(DirectoriesMixin, TestCase): created=date(2023, 1, 2), ) self.doc2.archive_filename = sample2_archive - self.doc2.save() + self.doc2.save(write_file_paths=True) self.doc3 = Document.objects.create( checksum="C", title="C", @@ -774,7 +774,7 @@ class TestPDFActions(DirectoriesMixin, TestCase): created=date(2023, 1, 3), ) self.img_doc.archive_filename = img_doc_archive - self.img_doc.save() + self.img_doc.save(write_file_paths=True) @staticmethod def fake_decrypt( diff --git a/src/documents/tests/test_file_handling.py b/src/documents/tests/test_file_handling.py index fd6d2c85c9..3746c3cdbb 100644 --- a/src/documents/tests/test_file_handling.py +++ b/src/documents/tests/test_file_handling.py @@ -92,7 +92,7 @@ class TestFileHandling(DirectoriesMixin, FileSystemAssertsMixin, TestCase): # Ensure that filename is properly generated self.assertEqual(document.filename, Path("none/none.pdf")) - document.save() + document.save(write_file_paths=True) # test that creating dirs for the source_path creates the correct directory create_source_path_directory(document.source_path) @@ -190,7 +190,7 @@ class TestFileHandling(DirectoriesMixin, FileSystemAssertsMixin, TestCase): # Set a correspondent and save the document document.correspondent = Correspondent.objects.get_or_create(name="test")[0] - document.save() + document.save(write_file_paths=True) # Check proper handling of files self.assertIsFile( @@ -232,7 +232,7 @@ class TestFileHandling(DirectoriesMixin, FileSystemAssertsMixin, TestCase): disable_auditlog(), ): m.side_effect = DatabaseError() - document.save() + document.save(write_file_paths=True) # Check proper handling of files self.assertIsFile(document.source_path) @@ -336,7 +336,7 @@ class TestFileHandling(DirectoriesMixin, FileSystemAssertsMixin, TestCase): # Ensure that filename is properly generated document.filename = generate_filename(document) - document.save() + document.save(write_file_paths=True) self.assertEqual(document.filename, "none/none.pdf") create_source_path_directory(document.source_path) @@ -362,7 +362,7 @@ class TestFileHandling(DirectoriesMixin, FileSystemAssertsMixin, TestCase): # Ensure that filename is properly generated document.filename = generate_filename(document) - document.save() + document.save(write_file_paths=True) self.assertEqual(document.filename, "none/none.pdf") create_source_path_directory(document.source_path) @@ -385,7 +385,7 @@ class TestFileHandling(DirectoriesMixin, FileSystemAssertsMixin, TestCase): document.save() document.filename = generate_filename(document) - document.save() + document.save(write_file_paths=True) create_source_path_directory(document.source_path) Path(document.source_path).touch() document.delete() @@ -421,7 +421,7 @@ class TestFileHandling(DirectoriesMixin, FileSystemAssertsMixin, TestCase): # Set a correspondent and save the document document.correspondent = Correspondent.objects.get_or_create(name="test")[0] - document.save() + document.save(write_file_paths=True) # Check proper handling of files self.assertIsDir(settings.ORIGINALS_DIR / "test") @@ -476,7 +476,7 @@ class TestFileHandling(DirectoriesMixin, FileSystemAssertsMixin, TestCase): def test_filename_relative(self) -> None: doc = Document.objects.create(title="doc1", mime_type="application/pdf") doc.filename = generate_filename(doc) - doc.save() + doc.save(write_file_paths=True) self.assertEqual( doc.source_path, @@ -532,7 +532,7 @@ class TestFileHandling(DirectoriesMixin, FileSystemAssertsMixin, TestCase): # Ensure that filename is properly generated document.filename = generate_filename(document) - document.save() + document.save(write_file_paths=True) self.assertEqual(document.filename, "none/none/none.pdf") create_source_path_directory(document.source_path) Path(document.source_path).touch() @@ -620,13 +620,13 @@ class TestFileHandling(DirectoriesMixin, FileSystemAssertsMixin, TestCase): Path(document.source_path).touch() Path(document2.source_path).touch() document.filename = "0000001.pdf" - document.save() + document.save(write_file_paths=True) self.assertIsFile(document.source_path) self.assertEqual(document.filename, "qwe.pdf") document2.filename = "0000002.pdf" - document2.save() + document2.save(write_file_paths=True) self.assertIsFile(document.source_path) self.assertEqual(document2.filename, "qwe_01.pdf") diff --git a/src/documents/tests/test_management.py b/src/documents/tests/test_management.py index 0fe5d7eb5b..cac9b03665 100644 --- a/src/documents/tests/test_management.py +++ b/src/documents/tests/test_management.py @@ -236,7 +236,7 @@ class TestRenamer(DirectoriesMixin, FileSystemAssertsMixin, TestCase): doc = Document.objects.create(title="test", mime_type="image/jpeg") doc.filename = generate_filename(doc) doc.archive_filename = generate_filename(doc, archive_filename=True) - doc.save() + doc.save(write_file_paths=True) Path(doc.source_path).touch() Path(doc.archive_path).touch() diff --git a/src/documents/tests/test_models.py b/src/documents/tests/test_models.py index 44440be29c..73dea4d811 100644 --- a/src/documents/tests/test_models.py +++ b/src/documents/tests/test_models.py @@ -1,4 +1,7 @@ import pytest +from django.db import DatabaseError +from django.db import models +from django.db import transaction from documents.models import Correspondent from documents.models import Document @@ -22,3 +25,101 @@ class TestDocument: assert Correspondent.objects.count() == 0 assert Document.objects.count() == 1 assert Document.objects.first().correspondent is None + + +@pytest.mark.django_db +@pytest.mark.usefixtures("paperless_dirs") +class TestDocumentFilePathWrites: + def test_full_save_does_not_write_file_paths(self) -> None: + """ + GIVEN: + - A document whose filename columns were changed in the DB after + an instance was loaded + WHEN: + - The stale instance is saved without update_fields + THEN: + - The DB filename columns keep the newer values + - Other fields from the stale instance are written + """ + doc = DocumentFactory(filename="old.pdf", archive_filename="old.pdf") + stale = Document.objects.get(pk=doc.pk) + Document.objects.filter(pk=doc.pk).update( + filename="new.pdf", + archive_filename="new-archive.pdf", + ) + + stale.title = "changed" + stale.save() + + doc.refresh_from_db() + assert doc.filename == "new.pdf" + assert doc.archive_filename == "new-archive.pdf" + assert doc.title == "changed" + + @pytest.mark.parametrize( + "save_kwargs", + [ + pytest.param({"write_file_paths": True}, id="write-file-paths"), + pytest.param( + {"update_fields": ["filename", "archive_filename"]}, + id="explicit-update-fields", + ), + ], + ) + def test_opt_in_writes_file_paths(self, save_kwargs: dict[str, object]) -> None: + """ + GIVEN: + - An existing document + WHEN: + - Its filename columns are changed in memory and saved with an + explicit opt in + THEN: + - The DB filename columns are updated + """ + doc = DocumentFactory(filename="old.pdf", archive_filename="old.pdf") + + doc.filename = "new.pdf" + doc.archive_filename = "new.pdf" + doc.save(**save_kwargs) + + doc.refresh_from_db() + assert doc.filename == "new.pdf" + assert doc.archive_filename == "new.pdf" + + def test_creation_writes_file_paths(self) -> None: + """ + GIVEN: + - A new, unsaved document with filename columns set + WHEN: + - It is saved + THEN: + - The filename columns are written + """ + doc = DocumentFactory.build(filename="a.pdf", archive_filename="b.pdf") + + doc.save() + + doc.refresh_from_db() + assert doc.filename == "a.pdf" + assert doc.archive_filename == "b.pdf" + + def test_full_save_of_deleted_row_raises(self) -> None: + """ + GIVEN: + - An instance whose row was hard-deleted elsewhere + WHEN: + - The instance is saved without update_fields + THEN: + - A DatabaseError is raised instead of re-inserting the row + """ + doc = DocumentFactory() + stale = Document.objects.get(pk=doc.pk) + # A plain QuerySet bypasses the soft-delete manager, so the row is + # really gone (as after emptying the trash). + models.QuerySet(model=Document).filter(pk=doc.pk).delete() + + # The savepoint keeps the failed UPDATE from poisoning the test transaction. + with pytest.raises(DatabaseError), transaction.atomic(): + stale.save() + + assert not Document.global_objects.filter(pk=doc.pk).exists() diff --git a/src/documents/tests/test_sanity_check.py b/src/documents/tests/test_sanity_check.py index e04367f7b3..c02da3ce65 100644 --- a/src/documents/tests/test_sanity_check.py +++ b/src/documents/tests/test_sanity_check.py @@ -132,7 +132,7 @@ class TestCheckSanityOriginal: class TestCheckSanityArchive: def test_checksum_without_filename(self, sample_doc: Document) -> None: sample_doc.archive_filename = None - sample_doc.save() + sample_doc.save(write_file_paths=True) messages = check_sanity() assert messages.has_error assert any( @@ -185,7 +185,7 @@ class TestCheckSanityArchive: Path(sample_doc.archive_path).unlink() sample_doc.archive_checksum = None sample_doc.archive_filename = None - sample_doc.save() + sample_doc.save(write_file_paths=True) messages = check_sanity() assert not messages.has_error diff --git a/src/documents/tests/test_share_link_bundles.py b/src/documents/tests/test_share_link_bundles.py index 6bf064c274..eca4642ca6 100644 --- a/src/documents/tests/test_share_link_bundles.py +++ b/src/documents/tests/test_share_link_bundles.py @@ -366,7 +366,7 @@ class ShareLinkBundleBuildTaskTests(DirectoriesMixin, APITestCase): def _write_document_file(self, *, archive: bool, content: bytes) -> Path: if archive: self.document.archive_filename = f"{self.document.pk:07}.pdf" - self.document.save() + self.document.save(write_file_paths=True) path = self.document.archive_path assert path is not None else: