fix: address review feedback on effective_content annotation skip

- _needs_effective_content_annotation() now checks for a non-blank, stripped param value rather than mere key presence, matching how SearchFilter/TitleContentFilter/EffectiveContentFilter themselves no-op on a blank value. An empty ?search= or a saved view with a cleared text filter no longer re-triggers the annotation. - The "versions" prefetch on DocumentViewSet no longer carries content for every historical version of every document -- that's unused bloat for version-heavy documents. Added latest_version_content_prefetch() (versioning.py), a separate, windowed prefetch scoped to just the newest version's content per root, and taught Document.get_effective_content() to check it first. - DocumentSerializer.to_representation() no longer unconditionally calls get_effective_content(). Added has_prefetched_effective_content() (versioning.py) as a cheap upfront check: only resolve version-aware content when an SQL annotation or a versions prefetch is already on the instance. TrashView and GlobalSearchView build their own querysets independently of DocumentViewSet and never display document content at all (checked both frontend components), so they now keep showing the document's own, unresolved content with zero extra queries -- the same behavior as before effective_content resolution existed, just generalized past the narrow hasattr() check it replaced.
2026-08-27 21:23:20 +00:00 · 2026-08-25 14:18:50 -07:00
parent 3221c3a3e8
commit 8e65ba0efa
5 changed files with 246 additions and 9 deletions
@@ -373,6 +373,7 @@ class Document(SoftDeleteModel, ModelWithOwner):  # type: ignore[django-manager-
        If the queryset already annotated ``effective_content``, that value is used.
        """
        # Here to avoid circular import
+        from documents.versioning import LATEST_VERSION_CONTENT_PREFETCH_ATTR
        from documents.versioning import sort_versions_newest_first
        from documents.versioning import versions_newest_first

@@ -382,6 +383,19 @@ class Document(SoftDeleteModel, ModelWithOwner):  # type: ignore[django-manager-
        if self.root_document_id is not None or self.pk is None:
            return self.content

+        latest_version_prefetch = getattr(
+            self,
+            LATEST_VERSION_CONTENT_PREFETCH_ATTR,
+            None,
+        )
+        if latest_version_prefetch is not None:
+            # Empty list means prefetch ran and found no versions — use own content.
+            return (
+                latest_version_prefetch[0].content
+                if latest_version_prefetch
+                else self.content
+            )
+
        prefetched_cache = getattr(self, "_prefetched_objects_cache", None)
        prefetched_versions = (
            prefetched_cache.get("versions")
@@ -88,6 +88,7 @@ from documents.templating.utils import convert_format_str_to_template_format
 from documents.templating.workflows import validate_workflow_template
 from documents.validators import uri_validator
 from documents.validators import url_validator
+from documents.versioning import has_prefetched_effective_content
 from documents.versioning import sort_versions_newest_first

 if TYPE_CHECKING:
@@ -1146,12 +1147,13 @@ class DocumentSerializer(

    def to_representation(self, instance):
        doc = super().to_representation(instance)
-        if "content" in self.fields:
-            # get_effective_content() already falls back through the SQL
-            # annotation (when the queryset attached one), the prefetched
-            # "versions" cache, and finally a direct query -- so this stays
-            # correct whether or not DocumentViewSet.get_queryset() decided
-            # the annotation was needed for this request.
+        if "content" in self.fields and has_prefetched_effective_content(instance):
+            # Only resolve version-aware content when it's cheap: an SQL
+            # annotation or a versions prefetch is already on the instance.
+            # A caller that set up neither (e.g. TrashView, GlobalSearchView,
+            # which build their own querysets) gets the document's own,
+            # unresolved content instead of paying for an extra per-instance
+            # query -- same as before effective_content resolution existed.
            doc["content"] = instance.get_effective_content() or ""
        if self.truncate_content and "content" in self.fields:
            doc["content"] = doc.get("content")[0:550]
@@ -8,7 +8,11 @@ from django.db import connection
 from django.test.utils import CaptureQueriesContext
 from rest_framework import status

+from documents.models import Document
 from documents.tests.factories import DocumentFactory
+from documents.versioning import LATEST_VERSION_CONTENT_PREFETCH_ATTR
+from documents.versioning import has_prefetched_effective_content
+from documents.versioning import latest_version_content_prefetch
 from documents.views import DocumentViewSet

 if TYPE_CHECKING:
@@ -30,6 +34,9 @@ class TestNeedsEffectiveContentAnnotation:
            ({}, False),
            ({"ordering": "-added"}, False),
            ({"tags__id__in": "1,2"}, False),
+            ({"search": ""}, False),
+            ({"search": "   "}, False),
+            ({"content__icontains": ""}, False),
            ({"search": "foo"}, True),
            ({"title_content": "foo"}, True),
            ({"content__istartswith": "foo"}, True),
@@ -89,3 +96,144 @@ class TestDocumentListEffectiveContentAnnotation:
        assert not any(
            "effective_content" in query["sql"] for query in ctx.captured_queries
        )
+
+    def test_latest_version_content_prefetch_carries_only_the_newest_version(
+        self,
+    ) -> None:
+        # GIVEN a root document with two versions
+        root = DocumentFactory(content="root-content")
+        DocumentFactory(
+            root_document=root,
+            version_index=1,
+            content="older-version-content",
+        )
+        DocumentFactory(
+            root_document=root,
+            version_index=2,
+            content="newest-version-content",
+        )
+
+        # WHEN fetching the root through latest_version_content_prefetch()
+        fetched_root = (
+            Document.objects.filter(pk=root.pk)
+            .prefetch_related(
+                latest_version_content_prefetch(),
+            )
+            .get()
+        )
+
+        # THEN the prefetch carries only the single newest version, not
+        # every historical version's content (the whole point of not
+        # reusing the metadata-only "versions" prefetch for this)
+        latest = getattr(fetched_root, LATEST_VERSION_CONTENT_PREFETCH_ATTR)
+        assert [v.content for v in latest] == ["newest-version-content"]
+
+
+class TestHasPrefetchedEffectiveContent:
+    """
+    DocumentSerializer.to_representation() only calls get_effective_content()
+    when has_prefetched_effective_content() says it's cheap -- otherwise a
+    caller that never set up an annotation or prefetch (TrashView,
+    GlobalSearchView, which build their own querysets and don't display
+    content at all) would pay for a per-instance query nobody asked for.
+    """
+
+    def test_false_with_no_annotation_or_prefetch(self) -> None:
+        document = Document()
+        assert has_prefetched_effective_content(document) is False
+
+    def test_true_with_effective_content_annotation(self) -> None:
+        document = Document()
+        document.effective_content = "resolved"
+        assert has_prefetched_effective_content(document) is True
+
+    def test_true_with_lean_prefetch_attr_even_when_empty(self) -> None:
+        document = Document()
+        setattr(document, LATEST_VERSION_CONTENT_PREFETCH_ATTR, [])
+        assert has_prefetched_effective_content(document) is True
+
+    def test_true_with_metadata_versions_prefetch_cache(self) -> None:
+        document = Document()
+        document._prefetched_objects_cache = {"versions": []}
+        assert has_prefetched_effective_content(document) is True
+
+
+def _get_effective_content_fallback_queries(
+    ctx: CaptureQueriesContext,
+) -> list[dict[str, str]]:
+    """
+    Document.get_effective_content()'s per-instance fallback (no annotation,
+    no prefetch) is a `.values_list("content", flat=True).first()` query --
+    a SELECT of just the content column. Distinct from get_versions()'s own,
+    unrelated per-instance metadata query (id/checksum/added/etc, no
+    content) run to build the "versions" response field, which isn't part
+    of what this test file covers.
+    """
+    return [
+        q
+        for q in ctx.captured_queries
+        if q["sql"].startswith('SELECT "documents_document"."content" FROM')
+    ]
+
+
+@pytest.mark.django_db
+class TestTrashAndGlobalSearchDoNotResolveEffectiveContent:
+    """
+    TrashView and GlobalSearchView serialize Document instances with
+    DocumentSerializer too, but build their querysets independently of
+    DocumentViewSet.get_queryset() -- and neither actually displays
+    document content. They should keep showing the document's own,
+    unresolved content with no extra query, exactly as before
+    effective_content resolution existed.
+    """
+
+    def test_trash_list_shows_unresolved_content_with_no_extra_query(
+        self,
+        admin_client: APIClient,
+    ) -> None:
+        # GIVEN a trashed root document whose own content differs from what
+        # a (also trashed, since deletion cascades) version would have had
+        root = DocumentFactory(content="own-content")
+        DocumentFactory(
+            root_document=root,
+            version_index=1,
+            content="version-content",
+        )
+        root.delete()
+
+        # WHEN listing trash
+        with CaptureQueriesContext(connection) as ctx:
+            response = admin_client.get("/api/trash/")
+
+        # THEN the response shows the document's own content...
+        assert response.status_code == status.HTTP_200_OK
+        [result] = [r for r in response.data["results"] if r["id"] == root.id]
+        assert result["content"] == "own-content"
+        # ...without ever querying for versions to resolve it
+        assert _get_effective_content_fallback_queries(ctx) == []
+
+    def test_global_search_db_only_shows_unresolved_content_with_no_extra_query(
+        self,
+        admin_client: APIClient,
+    ) -> None:
+        # GIVEN a root document, findable by title, whose own content
+        # differs from its latest version's
+        root = DocumentFactory(title="findme", content="own-content")
+        DocumentFactory(
+            root_document=root,
+            version_index=1,
+            content="version-content",
+        )
+
+        # WHEN using the global search endpoint's db_only mode
+        with CaptureQueriesContext(connection) as ctx:
+            response = admin_client.get(
+                "/api/search/?query=findme&db_only=true",
+            )
+
+        # THEN the response shows the document's own content...
+        assert response.status_code == status.HTTP_200_OK
+        [result] = [d for d in response.data["documents"] if d["id"] == root.id]
+        assert result["content"] == "own-content"
+        # ...without ever querying for versions to resolve it
+        assert _get_effective_content_fallback_queries(ctx) == []
@@ -7,9 +7,12 @@ from typing import Any

 from django.db.models import F
 from django.db.models import OuterRef
+from django.db.models import Prefetch
 from django.db.models import QuerySet
 from django.db.models import Subquery
+from django.db.models import Window
 from django.db.models.functions import Coalesce
+from django.db.models.functions import RowNumber

 from documents.models import Document

@@ -43,6 +46,68 @@ def annotate_effective_content(documents: QuerySet[Document]) -> QuerySet[Docume
    )


+LATEST_VERSION_CONTENT_PREFETCH_ATTR = "_latest_version_content_prefetch"
+
+
+def latest_version_content_prefetch() -> Prefetch:
+    """
+    A Prefetch for Document.versions scoped to just the newest version's
+    content, for get_effective_content()'s fallback when no SQL annotation
+    is present.
+
+    Deliberately not merged into a metadata-only "versions" prefetch (the one
+    used for the serialized versions list): that one fetches every historical
+    version of every document, and pulling full OCR content for versions
+    nobody will read wastes DB transfer/memory at scale. This one is windowed
+    down to a single row per root, then bounded by Prefetch's own IN-list to
+    whatever page/result set it's attached to -- one cheap bulk query total,
+    not one per document and not one per version.
+    """
+    return Prefetch(
+        "versions",
+        queryset=(
+            Document.objects.filter(
+                root_document_id__isnull=False,
+                deleted_at__isnull=True,
+            )
+            .annotate(
+                rn=Window(
+                    RowNumber(),
+                    partition_by=F("root_document_id"),
+                    order_by=[
+                        F("version_index").desc(nulls_last=True),
+                        F("id").desc(),
+                    ],
+                ),
+            )
+            .filter(rn=1)
+            .only("id", "root_document_id", "content")
+        ),
+        to_attr=LATEST_VERSION_CONTENT_PREFETCH_ATTR,
+    )
+
+
+def has_prefetched_effective_content(document: Document) -> bool:
+    """
+    True if document.get_effective_content() can answer without an extra
+    per-instance query -- an SQL ``effective_content`` annotation, the lean
+    latest_version_content_prefetch(), or the metadata-only "versions"
+    prefetch is already present on the instance.
+
+    Callers that haven't set any of those up (e.g. views that build their
+    own querysets independently of DocumentViewSet.get_queryset(), like
+    TrashView or GlobalSearchView) intentionally don't pay for version-aware
+    content resolution -- see DocumentSerializer.to_representation(), which
+    uses this to decide whether to call get_effective_content() at all.
+    """
+    if hasattr(document, "effective_content"):
+        return True
+    if getattr(document, LATEST_VERSION_CONTENT_PREFETCH_ATTR, None) is not None:
+        return True
+    prefetched_cache = getattr(document, "_prefetched_objects_cache", None)
+    return isinstance(prefetched_cache, dict) and "versions" in prefetched_cache
+
+
 def sort_versions_newest_first(documents: list[Document]) -> list[Document]:
    """
    Same sorting as versions_newest_first()
@@ -233,6 +233,7 @@ from documents.versioning import VersionResolutionError
 from documents.versioning import get_latest_version_for_root
 from documents.versioning import get_request_version_param
 from documents.versioning import get_root_document
+from documents.versioning import latest_version_content_prefetch
 from documents.versioning import resolve_requested_version_for_root
 from documents.versioning import versions_newest_first
 from paperless import version
@@ -1107,9 +1108,16 @@ class DocumentViewSet(
        # self-join once real candidate counts get large. Everything on this
        # list is deprecated in favor of the Tantivy-backed search endpoint
        # (see filters.py's TitleContentFilter/EffectiveContentFilter docs),
-        # so keep paying that cost only when one is actually used.
+        # so keep paying that cost only when one is actually used. Checked as
+        # a stripped, non-blank value (not just key presence) to match how
+        # DRF's SearchFilter and TitleContentFilter/EffectiveContentFilter
+        # themselves no-op on a blank value -- otherwise an empty `?search=`
+        # or a saved view with a cleared text filter would still pay for the
+        # annotation despite applying no actual predicate.
        params = self.request.query_params
-        return any(param in params for param in self._CONTENT_FILTER_PARAMS)
+        return any(
+            params.get(param, "").strip() for param in self._CONTENT_FILTER_PARAMS
+        )

    def get_queryset(self):
        # A correlated subquery avoids the LEFT JOIN + Count() this used to
@@ -1146,9 +1154,9 @@ class DocumentViewSet(
                        "version_label",
                        "root_document_id",
                        "version_index",
-                        "content",
                    ),
                ),
+                latest_version_content_prefetch(),
                "tags",
                Prefetch(
                    "custom_fields",