From 4e52dd57105ad1afe536909628808a98f019d855 Mon Sep 17 00:00:00 2001 From: Trenton H <797416+stumpylog@users.noreply.github.com> Date: Sun, 19 Jul 2026 18:04:16 -0700 Subject: [PATCH 1/7] Fix: cache per-request effective-document resolution for thumb/metadata/preview (#13166) Django's condition() decorator invokes etag_func and last_modified_func separately, and the view itself may resolve again -- each call to resolve_effective_document_by_pk() was redoing the same root/version DB lookups. Memoize the resolution on the request object so a single thumb/metadata/preview request resolves the effective document once instead of up to three times. Related to paperless-ngx/paperless-ngx#13161. Co-authored-by: Claude Sonnet 5 --- src/documents/conditionals.py | 6 +-- .../tests/test_version_conditionals.py | 44 +++++++++++++----- src/documents/versioning.py | 45 ++++++++++++++----- 3 files changed, 69 insertions(+), 26 deletions(-) diff --git a/src/documents/conditionals.py b/src/documents/conditionals.py index bb937cc93..ffb183e4a 100644 --- a/src/documents/conditionals.py +++ b/src/documents/conditionals.py @@ -1,9 +1,9 @@ from datetime import UTC from datetime import datetime -from typing import Any from django.conf import settings from django.core.cache import cache +from rest_framework.request import Request from documents.caching import CACHE_5_MINUTES from documents.caching import CACHE_50_MINUTES @@ -117,7 +117,7 @@ def preview_last_modified(request, pk: int) -> datetime | None: return doc.modified -def thumbnail_etag(request: Any, pk: int) -> str | None: +def thumbnail_etag(request: Request, pk: int) -> str | None: """ Thumbnails are version-dependent, so use the effective document checksum as the ETag to invalidate cache when the latest version changes. @@ -128,7 +128,7 @@ def thumbnail_etag(request: Any, pk: int) -> str | None: return doc.checksum -def thumbnail_last_modified(request: Any, pk: int) -> datetime | None: +def thumbnail_last_modified(request: Request, pk: int) -> datetime | None: """ Returns the filesystem last modified either from cache or from filesystem. Cache should be (slightly?) faster than filesystem diff --git a/src/documents/tests/test_version_conditionals.py b/src/documents/tests/test_version_conditionals.py index d26d7b03d..a2531c7e6 100644 --- a/src/documents/tests/test_version_conditionals.py +++ b/src/documents/tests/test_version_conditionals.py @@ -1,8 +1,12 @@ from datetime import timedelta from types import SimpleNamespace +from typing import TYPE_CHECKING +from typing import cast from unittest import mock +from django.db import connection from django.test import TestCase +from django.test.utils import CaptureQueriesContext from django.utils import timezone from documents.conditionals import metadata_etag @@ -13,6 +17,9 @@ from documents.models import Document from documents.tests.utils import DirectoriesMixin from documents.versioning import resolve_effective_document_by_pk +if TYPE_CHECKING: + from rest_framework.request import Request + class TestConditionals(DirectoriesMixin, TestCase): def test_metadata_etag_uses_latest_version_for_root_request(self) -> None: @@ -29,14 +36,18 @@ class TestConditionals(DirectoriesMixin, TestCase): mime_type="application/pdf", root_document=root, ) - request = SimpleNamespace(query_params={}) + request = cast("Request", SimpleNamespace(query_params={})) self.assertEqual( metadata_etag(request, root.id), f"{latest.checksum}:{latest.modified.isoformat()}", ) - self.assertEqual(preview_etag(request, root.id), latest.archive_checksum) - self.assertEqual(thumbnail_etag(request, root.id), latest.checksum) + # preview_etag/thumbnail_etag resolve the same (pk, request) and should + # reuse metadata_etag's cached resolution instead of re-querying. + with CaptureQueriesContext(connection) as ctx: + self.assertEqual(preview_etag(request, root.id), latest.archive_checksum) + self.assertEqual(thumbnail_etag(request, root.id), latest.checksum) + self.assertEqual(len(ctx.captured_queries), 0) def test_metadata_etag_changes_when_document_modified_changes(self) -> None: doc = Document.objects.create( @@ -44,15 +55,20 @@ class TestConditionals(DirectoriesMixin, TestCase): checksum="same-checksum", mime_type="application/pdf", ) - request = SimpleNamespace(query_params={}) - - original_etag = metadata_etag(request, doc.id) + # Each call simulates a separate incoming HTTP request, so it gets its + # own request object -- the per-request resolution cache must not leak + # a stale result across genuinely different requests. + original_etag = metadata_etag( + cast("Request", SimpleNamespace(query_params={})), + doc.id, + ) new_modified = timezone.now() + timedelta(seconds=5) Document.objects.filter(id=doc.id).update(modified=new_modified) - self.assertNotEqual(metadata_etag(request, doc.id), original_etag) + second_request = cast("Request", SimpleNamespace(query_params={})) + self.assertNotEqual(metadata_etag(second_request, doc.id), original_etag) self.assertEqual( - metadata_etag(request, doc.id), + metadata_etag(second_request, doc.id), f"{doc.checksum}:{new_modified.isoformat()}", ) @@ -76,9 +92,13 @@ class TestConditionals(DirectoriesMixin, TestCase): root_document=other_root, ) - invalid_request = SimpleNamespace(query_params={"version": "not-a-number"}) - unrelated_request = SimpleNamespace( - query_params={"version": str(other_version.id)}, + invalid_request = cast( + "Request", + SimpleNamespace(query_params={"version": "not-a-number"}), + ) + unrelated_request = cast( + "Request", + SimpleNamespace(query_params={"version": str(other_version.id)}), ) self.assertIsNone( @@ -105,7 +125,7 @@ class TestConditionals(DirectoriesMixin, TestCase): latest.thumbnail_path.parent.mkdir(parents=True, exist_ok=True) latest.thumbnail_path.write_bytes(b"thumb") - request = SimpleNamespace(query_params={}) + request = cast("Request", SimpleNamespace(query_params={})) with mock.patch( "documents.conditionals.get_thumbnail_modified_key", return_value="thumb-modified-key", diff --git a/src/documents/versioning.py b/src/documents/versioning.py index 844a5a136..d9e71674a 100644 --- a/src/documents/versioning.py +++ b/src/documents/versioning.py @@ -8,7 +8,7 @@ from typing import Any from documents.models import Document if TYPE_CHECKING: - from django.http import HttpRequest + from rest_framework.request import Request class VersionResolutionError(StrEnum): @@ -26,7 +26,7 @@ def _document_manager(*, include_deleted: bool) -> Any: return Document.global_objects if include_deleted else Document.objects -def get_request_version_param(request: HttpRequest) -> str | None: +def get_request_version_param(request: Request) -> str | None: if hasattr(request, "query_params"): return request.query_params.get("version") return None @@ -57,7 +57,7 @@ def get_latest_version_for_root( def resolve_requested_version_for_root( root_doc: Document, - request: Any, + request: Request, *, include_deleted: bool = False, ) -> VersionResolution: @@ -86,7 +86,7 @@ def resolve_requested_version_for_root( def resolve_effective_document( request_doc: Document, - request: Any, + request: Request, *, include_deleted: bool = False, ) -> VersionResolution: @@ -107,18 +107,41 @@ def resolve_effective_document( return VersionResolution(document=request_doc) +_EFFECTIVE_DOCUMENT_CACHE_ATTR = "_effective_document_resolution_cache" + + def resolve_effective_document_by_pk( pk: int, - request: Any, + request: Request, *, include_deleted: bool = False, ) -> VersionResolution: + # Django's `condition()` decorator (used for ETag/Last-Modified) invokes the + # etag_func and last_modified_func separately, and the view itself may resolve + # again -- all against the same request. Cache per-request so a single thumb/ + # metadata/preview request doesn't redo this resolution multiple times. + cache = getattr(request, _EFFECTIVE_DOCUMENT_CACHE_ATTR, None) + if cache is None: + cache = {} + setattr(request, _EFFECTIVE_DOCUMENT_CACHE_ATTR, cache) + + key = (pk, include_deleted) + if key in cache: + return cache[key] + manager = _document_manager(include_deleted=include_deleted) request_doc = manager.only("id", "root_document_id").filter(pk=pk).first() if request_doc is None: - return VersionResolution(document=None, error=VersionResolutionError.NOT_FOUND) - return resolve_effective_document( - request_doc, - request, - include_deleted=include_deleted, - ) + resolution = VersionResolution( + document=None, + error=VersionResolutionError.NOT_FOUND, + ) + else: + resolution = resolve_effective_document( + request_doc, + request, + include_deleted=include_deleted, + ) + + cache[key] = resolution + return resolution From c9716252f06110c3ddb1c2a346cda1e2e95e7d5a Mon Sep 17 00:00:00 2001 From: Trenton H <797416+stumpylog@users.noreply.github.com> Date: Mon, 20 Jul 2026 06:12:54 -0700 Subject: [PATCH 2/7] Performance: Add DB indexes for common query/sort patterns (#13167) Adds indexes on Document.checksum, Document.page_count, and a composite (owner, created) index on Document, plus composite (field, value_*) indexes on CustomFieldInstance for each typed value column. Benchmarked against a 100k-document / 200k-custom-field-instance SQLite dataset: Document.checksum lookups ~69x faster, page_count sort ~40x, owner+created list queries ~6x. CustomFieldInstance composite indexes show large gains (10-24x) on realistic narrow-selectivity queries matching the workflow scheduler and custom-field search code paths. --- .../migrations/0022_add_perf_indexes.py | 73 +++++++++++++++++++ src/documents/models.py | 12 ++- 2 files changed, 84 insertions(+), 1 deletion(-) create mode 100644 src/documents/migrations/0022_add_perf_indexes.py diff --git a/src/documents/migrations/0022_add_perf_indexes.py b/src/documents/migrations/0022_add_perf_indexes.py new file mode 100644 index 000000000..fe66e8c49 --- /dev/null +++ b/src/documents/migrations/0022_add_perf_indexes.py @@ -0,0 +1,73 @@ +# Generated by Django 5.2.16 on 2026-07-19 20:33 + +import django.core.validators +from django.conf import settings +from django.db import migrations +from django.db import models + + +class Migration(migrations.Migration): + dependencies = [ + ("documents", "0021_widen_workflow_integer_fields"), + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ] + + operations = [ + migrations.AlterField( + model_name="document", + name="checksum", + field=models.CharField( + db_index=True, + editable=False, + help_text="The checksum of the original document.", + max_length=64, + verbose_name="checksum", + ), + ), + migrations.AlterField( + model_name="document", + name="page_count", + field=models.PositiveIntegerField( + db_index=True, + help_text="The number of pages of the document.", + null=True, + validators=[django.core.validators.MinValueValidator(1)], + verbose_name="page count", + ), + ), + migrations.AddIndex( + model_name="customfieldinstance", + index=models.Index( + fields=["field", "value_date"], + name="documents_c_field_i_30b51d_idx", + ), + ), + migrations.AddIndex( + model_name="customfieldinstance", + index=models.Index( + fields=["field", "value_int"], + name="documents_c_field_i_d25ab0_idx", + ), + ), + migrations.AddIndex( + model_name="customfieldinstance", + index=models.Index( + fields=["field", "value_float"], + name="documents_c_field_i_c35174_idx", + ), + ), + migrations.AddIndex( + model_name="customfieldinstance", + index=models.Index( + fields=["field", "value_monetary_amount"], + name="documents_c_field_i_53ce4e_idx", + ), + ), + migrations.AddIndex( + model_name="document", + index=models.Index( + fields=["owner", "created"], + name="documents_d_owner_i_ec6ab3_idx", + ), + ), + ] diff --git a/src/documents/models.py b/src/documents/models.py index a77447512..c9b7109bc 100644 --- a/src/documents/models.py +++ b/src/documents/models.py @@ -217,6 +217,7 @@ class Document(SoftDeleteModel, ModelWithOwner): # type: ignore[django-manager- _("checksum"), max_length=64, editable=False, + db_index=True, help_text=_("The checksum of the original document."), ) @@ -234,7 +235,7 @@ class Document(SoftDeleteModel, ModelWithOwner): # type: ignore[django-manager- blank=False, null=True, unique=False, - db_index=False, + db_index=True, validators=[MinValueValidator(1)], help_text=_( "The number of pages of the document.", @@ -338,6 +339,9 @@ class Document(SoftDeleteModel, ModelWithOwner): # type: ignore[django-manager- ordering = ("-created",) verbose_name = _("document") verbose_name_plural = _("documents") + indexes = [ + models.Index(fields=["owner", "created"]), + ] constraints = [ models.UniqueConstraint( fields=["root_document", "version_index"], @@ -1190,6 +1194,12 @@ class CustomFieldInstance(SoftDeleteModel): ordering = ("created",) verbose_name = _("custom field instance") verbose_name_plural = _("custom field instances") + indexes = [ + models.Index(fields=["field", "value_date"]), + models.Index(fields=["field", "value_int"]), + models.Index(fields=["field", "value_float"]), + models.Index(fields=["field", "value_monetary_amount"]), + ] constraints = [ models.UniqueConstraint( fields=["document", "field"], From 2b784e709b2c12be8261fe10bf4fe10cdb797c55 Mon Sep 17 00:00:00 2001 From: Trenton H <797416+stumpylog@users.noreply.github.com> Date: Mon, 20 Jul 2026 08:22:30 -0700 Subject: [PATCH 3/7] Fix: prevent tag assignment from reverting other pending workflow assignments (#13178) apply_assignment_to_document() mutated tags directly on the shared document instance via add_nested_tags(), whose m2m_changed signal triggers update_filename_and_move_files() -> instance.refresh_from_db(), discarding any not-yet-saved fields (e.g. storage_path) staged by an earlier-ordered action in the same workflow. Apply tag changes to a freshly-fetched instance instead, matching the pattern already used in apply_removal_to_document(). --- src/documents/tests/test_workflows.py | 63 +++++++++++++++++++++++++++ src/documents/workflows/mutations.py | 6 ++- 2 files changed, 68 insertions(+), 1 deletion(-) diff --git a/src/documents/tests/test_workflows.py b/src/documents/tests/test_workflows.py index 329ed0af0..f3dace48f 100644 --- a/src/documents/tests/test_workflows.py +++ b/src/documents/tests/test_workflows.py @@ -2938,6 +2938,69 @@ class TestWorkflows( self.assertFalse(doc.tags.filter(pk=self.t1.pk).exists()) self.assertTrue(doc.tags.filter(pk=self.t2.pk).exists()) + def test_document_updated_workflow_assignment_storage_path_persists_with_tag_assignment( + self, + ) -> None: + """ + GIVEN: + - A document updated workflow filtered on a tag + - One assignment action assigns a storage path, a second (later-ordered) + assignment action adds a tag + WHEN: + - The document is updated and the workflow is triggered + THEN: + - Both the tag and the storage path are persisted + """ + trigger = WorkflowTrigger.objects.create( + type=WorkflowTrigger.WorkflowTriggerType.DOCUMENT_UPDATED, + ) + trigger.filter_has_tags.add(self.t1) + assign_storage_path = WorkflowAction.objects.create( + type=WorkflowAction.WorkflowActionType.ASSIGNMENT, + assign_storage_path=self.sp, + order=0, + ) + assign_tag = WorkflowAction.objects.create( + type=WorkflowAction.WorkflowActionType.ASSIGNMENT, + order=1, + ) + assign_tag.assign_tags.add(self.t2) + assign_tag.save() + + workflow = Workflow.objects.create( + name="Workflow assign storage path then tag", + order=0, + ) + workflow.triggers.add(trigger) + workflow.actions.add(assign_storage_path, assign_tag) + workflow.save() + + doc = Document.objects.create( + title="sample test", + mime_type="application/pdf", + checksum="assign-tag-and-storage-path", + original_filename="sample.pdf", + ) + generated = generate_unique_filename(doc) + destination = (settings.ORIGINALS_DIR / generated).resolve() + create_source_path_directory(destination) + shutil.copy(self.SAMPLE_DIR / "simple.pdf", destination) + Document.objects.filter(pk=doc.pk).update(filename=generated.as_posix()) + doc.refresh_from_db() + doc.tags.set([self.t1]) + + superuser = User.objects.create_superuser("superuser") + self.client.force_authenticate(user=superuser) + self.client.patch( + f"/api/documents/{doc.id}/", + {"title": "user update to trigger workflow"}, + format="json", + ) + + doc.refresh_from_db() + self.assertEqual(doc.storage_path, self.sp) + self.assertTrue(doc.tags.filter(pk=self.t2.pk).exists()) + def test_removal_action_document_updated_removeall(self) -> None: """ GIVEN: diff --git a/src/documents/workflows/mutations.py b/src/documents/workflows/mutations.py index 7dbb317a8..7d33bc545 100644 --- a/src/documents/workflows/mutations.py +++ b/src/documents/workflows/mutations.py @@ -24,7 +24,11 @@ def apply_assignment_to_document( action: WorkflowAction, annotated with 'has_assign_*' boolean fields """ if action.has_assign_tags: - document.add_nested_tags(action.assign_tags.all()) + # Apply to a freshly-fetched instance rather than the shared `document`. + # Document.tags.add() fires an m2m_changed signal that ultimately calls + # instance.refresh_from_db(), which would discard any other unsaved + # assignment fields (e.g. storage_path) already staged on `document`. + Document.objects.get(pk=document.pk).add_nested_tags(action.assign_tags.all()) if action.assign_correspondent: document.correspondent = action.assign_correspondent From ff609c29872bbdb5b780d6bb45506505d50cc084 Mon Sep 17 00:00:00 2001 From: shamoon <4887959+shamoon@users.noreply.github.com> Date: Mon, 20 Jul 2026 08:45:24 -0700 Subject: [PATCH 4/7] Change: update root modified timestamp on version changes (#13170) --- src/documents/consumer.py | 5 +++++ src/documents/tests/test_api_document_versions.py | 9 +++++++++ src/documents/tests/test_consumer.py | 4 ++++ src/documents/views.py | 4 ++++ 4 files changed, 22 insertions(+) diff --git a/src/documents/consumer.py b/src/documents/consumer.py index f050c7416..7c01853c5 100644 --- a/src/documents/consumer.py +++ b/src/documents/consumer.py @@ -621,6 +621,11 @@ class ConsumerPlugin( else: original_document.save() + # Adding a version changes the effective document, so update root modified + Document.objects.filter(pk=root_doc.pk).update( + modified=timezone.now(), + ) + # Create a log entry for the version addition, if enabled if settings.AUDIT_LOG_ENABLED: from auditlog.models import ( # type: ignore[import-untyped] diff --git a/src/documents/tests/test_api_document_versions.py b/src/documents/tests/test_api_document_versions.py index a81b9d545..3ff32998f 100644 --- a/src/documents/tests/test_api_document_versions.py +++ b/src/documents/tests/test_api_document_versions.py @@ -1,5 +1,6 @@ from __future__ import annotations +import datetime from typing import TYPE_CHECKING from unittest import TestCase from unittest import mock @@ -10,6 +11,7 @@ from django.contrib.auth.models import User from django.contrib.contenttypes.models import ContentType from django.core.exceptions import FieldError from django.core.files.uploadedfile import SimpleUploadedFile +from django.utils import timezone from rest_framework import status from rest_framework.test import APITestCase @@ -137,6 +139,8 @@ class TestDocumentVersioningApi(DirectoriesMixin, APITestCase): root_document=root, content="v2-content", ) + original_modified = timezone.now() - datetime.timedelta(days=1) + Document.objects.filter(pk=root.pk).update(modified=original_modified) with mock.patch("documents.search.get_backend"): resp = self.client.delete(f"/api/documents/{root.id}/versions/{v2.id}/") @@ -146,6 +150,7 @@ class TestDocumentVersioningApi(DirectoriesMixin, APITestCase): self.assertEqual(resp.data["current_version_id"], v1.id) root.refresh_from_db() self.assertEqual(root.content, "root-content") + self.assertGreater(root.modified, original_modified) with mock.patch("documents.search.get_backend"): resp = self.client.delete(f"/api/documents/{root.id}/versions/{v1.id}/") @@ -326,6 +331,8 @@ class TestDocumentVersioningApi(DirectoriesMixin, APITestCase): root_document=root, version_label="old", ) + original_modified = timezone.now() - datetime.timedelta(days=1) + Document.objects.filter(pk=root.pk).update(modified=original_modified) resp = self.client.patch( f"/api/documents/{root.id}/versions/{version.id}/", @@ -339,6 +346,8 @@ class TestDocumentVersioningApi(DirectoriesMixin, APITestCase): self.assertEqual(resp.data["version_label"], "Label 1") self.assertEqual(resp.data["id"], version.id) self.assertFalse(resp.data["is_root"]) + root.refresh_from_db() + self.assertGreater(root.modified, original_modified) def test_update_version_label_clears_on_blank(self) -> None: root = Document.objects.create( diff --git a/src/documents/tests/test_consumer.py b/src/documents/tests/test_consumer.py index c8a740a7e..fccc736f6 100644 --- a/src/documents/tests/test_consumer.py +++ b/src/documents/tests/test_consumer.py @@ -767,6 +767,8 @@ class TestConsumer( root_doc.archive_serial_number = 42 root_doc.save() + original_modified = timezone.now() - datetime.timedelta(days=1) + Document.objects.filter(pk=root_doc.pk).update(modified=original_modified) actor = User.objects.create_user( username="actor", email="actor@example.com", @@ -818,6 +820,8 @@ class TestConsumer( self.assertIsNone(version.archive_serial_number) self.assertEqual(version.original_filename, version_file.name) self.assertTrue(bool(version.content)) + root_doc.refresh_from_db() + self.assertGreater(root_doc.modified, original_modified) @override_settings(AUDIT_LOG_ENABLED=True) @mock.patch("documents.consumer.load_classifier") diff --git a/src/documents/views.py b/src/documents/views.py index 1591d0b06..20dbe9247 100644 --- a/src/documents/views.py +++ b/src/documents/views.py @@ -2057,6 +2057,8 @@ class DocumentViewSet( _backend.remove(version_doc.pk) version_doc_id = version_doc.id version_doc.delete() + root_doc.modified = timezone.now() + Document.objects.filter(pk=root_doc.pk).update(modified=root_doc.modified) _backend.add_or_update(root_doc) if settings.AUDIT_LOG_ENABLED: actor = ( @@ -2136,6 +2138,8 @@ class DocumentViewSet( old_label = version_doc.version_label version_doc.version_label = serializer.validated_data["version_label"] version_doc.save(update_fields=["version_label"]) + root_doc.modified = timezone.now() + Document.objects.filter(pk=root_doc.pk).update(modified=root_doc.modified) if settings.AUDIT_LOG_ENABLED and old_label != version_doc.version_label: actor = ( From d8d88724145f25cf0281a33e4880f5cc6b180e27 Mon Sep 17 00:00:00 2001 From: Trenton H <797416+stumpylog@users.noreply.github.com> Date: Mon, 20 Jul 2026 13:59:44 -0700 Subject: [PATCH 5/7] Fix (beta): compute num_notes via a subquery instead of Count()+distinct() (#13182) DocumentViewSet.get_queryset() annotated num_notes via a LEFT JOIN to documents_note plus Count(), which requires the database to aggregate every matching document's note count before it can even sort or paginate the result -- on every list request, not just filtered ones. Switched to the same correlated-subquery pattern already used two lines above for effective_content (Subquery + OuterRef), which Django compiles to portable SQL across sqlite/postgresql/mariadb rather than a join-based aggregate. Benchmarked against a 100k synthetic document corpus (Postgres): the plain document list request dropped from ~2.2-2.4s to ~0.9-1.0s. Note that a separate, larger cost remains in the same queryset's plain .distinct() call, which still forces a full sort over every matching row regardless of this fix -- tracked separately, not addressed here. Co-authored-by: Claude Sonnet 5 --- src/documents/views.py | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/documents/views.py b/src/documents/views.py index 20dbe9247..3c49d6c76 100644 --- a/src/documents/views.py +++ b/src/documents/views.py @@ -1040,12 +1040,23 @@ class DocumentViewSet( .order_by("-id") .values("content")[:1], ) + # A correlated subquery avoids the LEFT JOIN + Count() this used to + # be, which forced a GROUP BY aggregate over every matching document + # before the query could even be sorted or limited. + note_count = Subquery( + Note.objects.filter(document=OuterRef("pk")) + .order_by() + .values("document") + .annotate(count=Count("pk")) + .values("count"), + output_field=IntegerField(), + ) return ( Document.objects.filter(root_document__isnull=True) .distinct() .order_by("-created", "-id") .annotate(effective_content=Coalesce(latest_version_content, F("content"))) - .annotate(num_notes=Count("notes")) + .annotate(num_notes=Coalesce(note_count, 0)) .select_related("correspondent", "storage_path", "document_type", "owner") .prefetch_related( Prefetch( From 8404198ec8a53cde95c9e1a1a419a60779b7e355 Mon Sep 17 00:00:00 2001 From: Trenton H <797416+stumpylog@users.noreply.github.com> Date: Tue, 21 Jul 2026 07:31:58 -0700 Subject: [PATCH 6/7] Fix (beta): short-circuit ObjectOwnedOrGrantedPermissionsFilter for superusers (#13183) Mirrors the existing short-circuit already present in its sibling class, ObjectOwnedPermissionsFilter, which this one lacked. Benchmarked as a superuser against a 100k-document corpus: no measurable difference. django-guardian's own get_objects_for_user() (called via super().filter_queryset()) already returns the queryset unrestricted for superusers before this change, so the guardian permission tables were never actually queried for superusers either way. This change only skips building a redundant 3-way OR of conditions that are all subsets of the already-unrestricted queryset -- worth keeping for clarity/consistency with the sibling filter, not as a performance claim. Co-authored-by: Claude Sonnet 5 --- src/documents/filters.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/documents/filters.py b/src/documents/filters.py index e2e2501ce..fe15c927d 100644 --- a/src/documents/filters.py +++ b/src/documents/filters.py @@ -1024,6 +1024,8 @@ class ObjectOwnedOrGrantedPermissionsFilter(ObjectPermissionsFilter): """ def filter_queryset(self, request, queryset, view): + if request.user.is_superuser: + return queryset objects_with_perms = super().filter_queryset(request, queryset, view) objects_owned = queryset.filter(owner=request.user) objects_unowned = queryset.filter(owner__isnull=True) From 8aab34ea738c64fc6c218840d0bb6639a4811e07 Mon Sep 17 00:00:00 2001 From: shamoon <4887959+shamoon@users.noreply.github.com> Date: Tue, 21 Jul 2026 09:50:28 -0700 Subject: [PATCH 7/7] Security (beta): two small permission check fixes (#13186) --- src/documents/tests/test_api_bulk_edit.py | 92 +++++++++++++++++++ .../tests/test_api_document_versions.py | 20 ++++ src/documents/views.py | 18 +++- 3 files changed, 125 insertions(+), 5 deletions(-) diff --git a/src/documents/tests/test_api_bulk_edit.py b/src/documents/tests/test_api_bulk_edit.py index ce3b24b5c..e4272e78f 100644 --- a/src/documents/tests/test_api_bulk_edit.py +++ b/src/documents/tests/test_api_bulk_edit.py @@ -1419,6 +1419,30 @@ class TestBulkEditAPI(DirectoriesMixin, APITestCase): self.assertEqual(response.status_code, status.HTTP_200_OK) m.assert_called_once() + @mock.patch("documents.views.bulk_edit.merge") + def test_merge_and_delete_requires_change_permission(self, m) -> None: + self.setup_mock(m, "merge") + user = User.objects.create_user(username="no-change") + user.user_permissions.add( + Permission.objects.get(codename="add_document"), + Permission.objects.get(codename="delete_document"), + ) + self.client.force_authenticate(user=user) + + response = self.client.post( + "/api/documents/merge/", + json.dumps( + { + "documents": [self.doc2.id, self.doc3.id], + "delete_originals": True, + }, + ), + content_type="application/json", + ) + + self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) + m.assert_not_called() + @mock.patch("documents.views.bulk_edit.merge") def test_merge_invalid_parameters(self, m) -> None: self.setup_mock(m, "merge") @@ -1668,6 +1692,74 @@ class TestBulkEditAPI(DirectoriesMixin, APITestCase): self.assertEqual(response.status_code, status.HTTP_200_OK) m.assert_called_once() + @mock.patch("documents.views.bulk_edit.edit_pdf") + def test_edit_pdf_update_requires_change_permission(self, m) -> None: + self.setup_mock(m, "edit_pdf") + user = User.objects.create_user(username="no-change") + self.client.force_authenticate(user=user) + + response = self.client.post( + "/api/documents/edit_pdf/", + json.dumps( + { + "documents": [self.doc2.id], + "operations": [{"page": 1}], + "update_document": True, + }, + ), + content_type="application/json", + ) + + self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) + m.assert_not_called() + + @mock.patch("documents.views.bulk_edit.remove_password") + @mock.patch("documents.views.bulk_edit.edit_pdf") + def test_delete_original_requires_delete_permission( + self, + edit_pdf_mock, + remove_password_mock, + ) -> None: + self.setup_mock(edit_pdf_mock, "edit_pdf") + self.setup_mock(remove_password_mock, "remove_password") + user = User.objects.create_user(username="no-delete") + user.user_permissions.add( + Permission.objects.get(codename="add_document"), + Permission.objects.get(codename="change_document"), + ) + self.client.force_authenticate(user=user) + + cases = [ + ( + "/api/documents/edit_pdf/", + { + "documents": [self.doc2.id], + "operations": [{"page": 1}], + "delete_original": True, + }, + edit_pdf_mock, + ), + ( + "/api/documents/remove_password/", + { + "documents": [self.doc2.id], + "password": "secret", + "delete_original": True, + }, + remove_password_mock, + ), + ] + for endpoint, payload, operation_mock in cases: + with self.subTest(endpoint=endpoint): + response = self.client.post( + endpoint, + json.dumps(payload), + content_type="application/json", + ) + + self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) + operation_mock.assert_not_called() + @mock.patch("documents.views.bulk_edit.remove_password") def test_remove_password(self, m) -> None: self.setup_mock(m, "remove_password") diff --git a/src/documents/tests/test_api_document_versions.py b/src/documents/tests/test_api_document_versions.py index 3ff32998f..38aa78706 100644 --- a/src/documents/tests/test_api_document_versions.py +++ b/src/documents/tests/test_api_document_versions.py @@ -669,6 +669,26 @@ class TestDocumentVersioningApi(DirectoriesMixin, APITestCase): self.assertEqual(resp.status_code, status.HTTP_403_FORBIDDEN) + def test_update_version_requires_global_change_permission(self) -> None: + user = User.objects.create_user(username="add-only") + user.user_permissions.add(Permission.objects.get(codename="add_document")) + root = Document.objects.create( + title="root", + checksum="root", + mime_type="application/pdf", + ) + self.client.force_authenticate(user=user) + + with mock.patch("documents.views.consume_file") as consume_mock: + resp = self.client.post( + f"/api/documents/{root.id}/update_version/", + {"document": self._make_pdf_upload()}, + format="multipart", + ) + + self.assertEqual(resp.status_code, status.HTTP_403_FORBIDDEN) + consume_mock.apply_async.assert_not_called() + def test_update_version_returns_404_for_missing_document(self) -> None: resp = self.client.post( "/api/documents/9999/update_version/", diff --git a/src/documents/views.py b/src/documents/views.py index 3c49d6c76..ae83fe23c 100644 --- a/src/documents/views.py +++ b/src/documents/views.py @@ -1947,10 +1947,13 @@ class DocumentViewSet( "root_document", ).get(pk=pk) root_doc = get_root_document(request_doc) - if request.user is not None and not has_perms_owner_aware( - request.user, - "change_document", - root_doc, + if request.user is not None and ( + not request.user.has_perm("documents.change_document") + or not has_perms_owner_aware( + request.user, + "change_document", + root_doc, + ) ): return HttpResponseForbidden("Insufficient permissions") except Document.DoesNotExist: @@ -2756,7 +2759,7 @@ class DocumentOperationPermissionMixin(PassUserMixin, DocumentSelectionMixin): ) or (method == bulk_edit.edit_pdf and parameters.get("update_document")) ): - has_perms = user_is_owner_of_all_documents + has_perms = has_perms and user_is_owner_of_all_documents # check global add permissions for methods that create documents if ( @@ -2781,6 +2784,11 @@ class DocumentOperationPermissionMixin(PassUserMixin, DocumentSelectionMixin): method in [bulk_edit.merge, bulk_edit.split] and parameters.get("delete_originals") ) + or ( + method in [bulk_edit.edit_pdf, bulk_edit.remove_password] + and parameters.get("delete_original") + and not parameters.get("update_document") + ) ) and not user.has_perm("documents.delete_document") ):