Compare commits

...
Author SHA1 Message Date
shamoon 82cfea86d4 Just merge these 2026-08-31 08:53:20 -07:00
shamoon a13e8a59dd Fix: annotate effective content in the filters rather than relying on callers 2026-08-26 09:54:18 -07:00
5 changed files with 138 additions and 45 deletions
+7 -17
View File
@@ -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 annotate_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 annotate_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 annotate_effective_content(qs).filter(
**{f"effective_content__{self.lookup_expr}": value},
)
@extend_schema_field(serializers.BooleanField)
@@ -2,14 +2,12 @@ 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 as DjangoTestCase
from django.utils import timezone
@@ -22,6 +20,7 @@ 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:
@@ -891,32 +890,104 @@ 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]
class TestVersionAwareFilters(DjangoTestCase):
"""
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),
)
self.assertIs(annotate_effective_content(annotated), annotated)
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()
+23
View File
@@ -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")
+6 -3
View File
@@ -27,10 +27,13 @@ def versions_newest_first(documents: QuerySet[Document]) -> QuerySet[Document]:
def annotate_effective_content(documents: QuerySet[Document]) -> QuerySet[Document]:
"""
Annotates documents with the content of their newest version, falling back
to their own, so get_effective_content() can answer from the row rather
than querying for the versions of each document
Annotates documents with the content of their newest version unless the
queryset already carries the annotation, falling back to their own, so
get_effective_content() can answer from the row rather than querying for
the versions of each document.
"""
if "effective_content" in documents.query.annotations:
return documents
return documents.annotate(
effective_content=Coalesce(
Subquery(
+8 -2
View File
@@ -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
@@ -3613,8 +3614,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]