From 2e1beef6b45db237c9c0381bcc3eaa64d95ac59f Mon Sep 17 00:00:00 2001 From: shamoon <4887959+shamoon@users.noreply.github.com> Date: Tue, 25 Aug 2026 15:49:34 -0700 Subject: [PATCH] Fix: annotate effective content in the filters rather than relying on callers --- src/documents/filters.py | 24 ++-- .../tests/test_api_document_versions.py | 116 ++++++++++++++---- src/documents/tests/test_api_search.py | 23 ++++ src/documents/versioning.py | 15 +++ src/documents/views.py | 10 +- 5 files changed, 147 insertions(+), 41 deletions(-) diff --git a/src/documents/filters.py b/src/documents/filters.py index 40ae8978b..a1c346e87 100644 --- a/src/documents/filters.py +++ b/src/documents/filters.py @@ -12,7 +12,6 @@ from typing import TYPE_CHECKING from typing import Any from django.contrib.contenttypes.models import ContentType -from django.core.exceptions import FieldError from django.db.models import Case from django.db.models import CharField from django.db.models import Count @@ -51,6 +50,7 @@ from documents.models import ShareLinkBundle from documents.models import StoragePath from documents.models import Tag from documents.permissions import permitted_object_ids +from documents.versioning import ensure_effective_content if TYPE_CHECKING: from collections.abc import Callable @@ -180,14 +180,9 @@ class TitleContentFilter(Filter): logger.warning( "Deprecated document filter parameter 'title_content' used; use `text` instead.", ) - try: - return qs.filter( - Q(title__icontains=value) | Q(effective_content__icontains=value), - ) - except FieldError: - return qs.filter( - Q(title__icontains=value) | Q(content__icontains=value), - ) + return ensure_effective_content(qs).filter( + Q(title__icontains=value) | Q(effective_content__icontains=value), + ) else: return qs @@ -198,14 +193,9 @@ class EffectiveContentFilter(Filter): value = value.strip() if isinstance(value, str) else value if not value: return qs - try: - return qs.filter( - **{f"effective_content__{self.lookup_expr}": value}, - ) - except FieldError: - return qs.filter( - **{f"content__{self.lookup_expr}": value}, - ) + return ensure_effective_content(qs).filter( + **{f"effective_content__{self.lookup_expr}": value}, + ) @extend_schema_field(serializers.BooleanField) diff --git a/src/documents/tests/test_api_document_versions.py b/src/documents/tests/test_api_document_versions.py index 41822af71..398f41a81 100644 --- a/src/documents/tests/test_api_document_versions.py +++ b/src/documents/tests/test_api_document_versions.py @@ -2,15 +2,14 @@ from __future__ import annotations import datetime from typing import TYPE_CHECKING -from unittest import TestCase from unittest import mock from auditlog.models import LogEntry # type: ignore[import-untyped] from django.contrib.auth.models import Permission 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.test import TestCase from django.utils import timezone from rest_framework import status from rest_framework.test import APITestCase @@ -21,6 +20,8 @@ from documents.filters import TitleContentFilter from documents.models import Document from documents.tests.utils import DirectoriesMixin from documents.tests.utils import read_streaming_response +from documents.versioning import annotate_effective_content +from documents.views import DocumentSelectionMixin if TYPE_CHECKING: from pathlib import Path @@ -890,31 +891,102 @@ class TestDocumentVersioningApi(DirectoriesMixin, APITestCase): class TestVersionAwareFilters(TestCase): - def test_title_content_filter_falls_back_to_content(self) -> None: - queryset = mock.Mock() - fallback_queryset = mock.Mock() - queryset.filter.side_effect = [FieldError("missing field"), fallback_queryset] + """ + The filters annotate effective_content themselves rather than relying on + the caller's queryset carrying it, so they stay version-aware on a plain + Document queryset (e.g. the bulk-edit "select all matching" path). + """ - result = TitleContentFilter().filter(queryset, " latest ") + def setUp(self) -> None: + super().setUp() + self.root = Document.objects.create( + title="root", + checksum="root", + mime_type="application/pdf", + content="superseded-content", + ) + Document.objects.create( + title="version", + checksum="version", + mime_type="application/pdf", + root_document=self.root, + version_index=1, + content="latest-content", + ) + self.unversioned = Document.objects.create( + title="unversioned", + checksum="unversioned", + mime_type="application/pdf", + content="latest-content", + ) - self.assertIs(result, fallback_queryset) - self.assertEqual(queryset.filter.call_count, 2) - - def test_effective_content_filter_falls_back_to_content_lookup(self) -> None: - queryset = mock.Mock() - fallback_queryset = mock.Mock() - queryset.filter.side_effect = [FieldError("missing field"), fallback_queryset] - - result = EffectiveContentFilter(lookup_expr="icontains").filter( - queryset, + def test_title_content_filter_matches_latest_version_content(self) -> None: + result = TitleContentFilter().filter( + Document.objects.filter(root_document__isnull=True), " latest ", ) - self.assertIs(result, fallback_queryset) - first_kwargs = queryset.filter.call_args_list[0].kwargs - second_kwargs = queryset.filter.call_args_list[1].kwargs - self.assertEqual(first_kwargs, {"effective_content__icontains": "latest"}) - self.assertEqual(second_kwargs, {"content__icontains": "latest"}) + self.assertCountEqual( + [doc.id for doc in result], + [self.root.id, self.unversioned.id], + ) + + def test_effective_content_filter_matches_latest_version_content(self) -> None: + result = EffectiveContentFilter(lookup_expr="icontains").filter( + Document.objects.filter(root_document__isnull=True), + " latest ", + ) + + self.assertCountEqual( + [doc.id for doc in result], + [self.root.id, self.unversioned.id], + ) + + def test_effective_content_filter_ignores_superseded_content(self) -> None: + result = EffectiveContentFilter(lookup_expr="icontains").filter( + Document.objects.filter(root_document__isnull=True), + "superseded", + ) + + self.assertEqual(list(result), []) + + def test_filters_reuse_an_existing_annotation(self) -> None: + """ + Annotating twice under the same alias is an error, so an already + annotated queryset (the search path) has to be left alone. + """ + annotated = annotate_effective_content( + Document.objects.filter(root_document__isnull=True), + ) + + result = EffectiveContentFilter(lookup_expr="icontains").filter( + annotated, + "latest", + ) + + self.assertCountEqual( + [doc.id for doc in result], + [self.root.id, self.unversioned.id], + ) + + def test_bulk_selection_does_not_match_superseded_content(self) -> None: + """ + Bulk edit's "select all matching" builds its own queryset, so before + the filters annotated for themselves it matched the root document's + superseded content -- selecting documents the list view, filtered by + the same term, does not show. + """ + user = User.objects.create_superuser(username="bulk_selection") + + selected = DocumentSelectionMixin()._resolve_document_ids( + user=user, + validated_data={ + "all": True, + "filters": {"content__icontains": "superseded"}, + }, + ) + + self.assertEqual(selected, []) def test_effective_content_filter_returns_input_for_empty_values(self) -> None: queryset = mock.Mock() diff --git a/src/documents/tests/test_api_search.py b/src/documents/tests/test_api_search.py index e597904dd..6ff17a89e 100644 --- a/src/documents/tests/test_api_search.py +++ b/src/documents/tests/test_api_search.py @@ -1917,6 +1917,29 @@ class TestDocumentSearchApi(DirectoriesMixin, APITestCase): self.assertEqual(len(response.data["documents"]), 1) self.assertEqual(response.data["documents"][0]["id"], title_match.id) + def test_global_search_returns_latest_version_content(self) -> None: + root = Document.objects.create( + title="bank statement", + content="superseded content", + checksum="GSV1", + pk=23, + ) + Document.objects.create( + title="bank statement v2", + content="latest content", + checksum="GSV2", + pk=24, + root_document=root, + version_index=1, + ) + + self.client.force_authenticate(self.user) + + response = self.client.get("/api/search/?query=bank&db_only=true") + self.assertEqual(response.status_code, status.HTTP_200_OK) + returned = {doc["id"]: doc["content"] for doc in response.data["documents"]} + self.assertEqual(returned.get(root.id), "latest content") + def test_global_search_filters_owned_mail_objects(self) -> None: user1 = User.objects.create_user("mail-search-user") user2 = User.objects.create_user("other-mail-search-user") diff --git a/src/documents/versioning.py b/src/documents/versioning.py index bba2495c8..853bfed98 100644 --- a/src/documents/versioning.py +++ b/src/documents/versioning.py @@ -43,6 +43,21 @@ def annotate_effective_content(documents: QuerySet[Document]) -> QuerySet[Docume ) +def ensure_effective_content(documents: QuerySet[Document]) -> QuerySet[Document]: + """ + Annotates effective_content unless the queryset already carries it. + + Lets a filter depend on effective_content without having to assume its + caller annotated one -- annotating twice under the same alias is an error, + and silently matching on the root document's own content instead is worse, + because the same filter then selects different documents depending on which + queryset it was handed. + """ + if "effective_content" in documents.query.annotations: + return documents + return annotate_effective_content(documents) + + def sort_versions_newest_first(documents: list[Document]) -> list[Document]: """ Same sorting as versions_newest_first() diff --git a/src/documents/views.py b/src/documents/views.py index 0d3f9de99..5abd49f58 100644 --- a/src/documents/views.py +++ b/src/documents/views.py @@ -230,6 +230,7 @@ from documents.tasks import train_classifier from documents.tasks import update_document_parent_tags from documents.utils import get_boolean from documents.versioning import VersionResolutionError +from documents.versioning import annotate_effective_content from documents.versioning import get_latest_version_for_root from documents.versioning import get_request_version_param from documents.versioning import get_root_document @@ -3616,8 +3617,13 @@ class GlobalSearchView(PassUserMixin): OBJECT_LIMIT = 3 docs = [] if request.user.has_perm("documents.view_document"): - all_docs = Document.objects.filter( - id__in=permitted_document_ids(request.user), + # Never more than OBJECT_LIMIT rows come back here, so annotating + # is cheap -- and without it these results show the root + # document's superseded content. + all_docs = annotate_effective_content( + Document.objects.filter( + id__in=permitted_document_ids(request.user), + ), ) if db_only: docs = all_docs.filter(title__icontains=query)[:OBJECT_LIMIT]