diff --git a/src/documents/serialisers.py b/src/documents/serialisers.py index c9dc2845b..9affa2a1a 100644 --- a/src/documents/serialisers.py +++ b/src/documents/serialisers.py @@ -1146,8 +1146,13 @@ class DocumentSerializer( def to_representation(self, instance): doc = super().to_representation(instance) - if "content" in self.fields and hasattr(instance, "effective_content"): - doc["content"] = getattr(instance, "effective_content") or "" + 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. + doc["content"] = instance.get_effective_content() or "" if self.truncate_content and "content" in self.fields: doc["content"] = doc.get("content")[0:550] return doc diff --git a/src/documents/tests/test_document_list_effective_content_annotation.py b/src/documents/tests/test_document_list_effective_content_annotation.py new file mode 100644 index 000000000..015de0d6f --- /dev/null +++ b/src/documents/tests/test_document_list_effective_content_annotation.py @@ -0,0 +1,91 @@ +from __future__ import annotations + +from types import SimpleNamespace +from typing import TYPE_CHECKING + +import pytest +from django.db import connection +from django.test.utils import CaptureQueriesContext +from rest_framework import status + +from documents.tests.factories import DocumentFactory +from documents.views import DocumentViewSet + +if TYPE_CHECKING: + from rest_framework.test import APIClient + + +class TestNeedsEffectiveContentAnnotation: + """ + DocumentViewSet._needs_effective_content_annotation() decides whether + the effective_content correlated subquery is worth attaching to the + queryset at all -- see TestDocumentListEffectiveContentAnnotation below + for why. This only checks that decision's own logic (a plain query-param + membership test), not that Django/DRF's filtering machinery works. + """ + + @pytest.mark.parametrize( + ("params", "expected"), + [ + ({}, False), + ({"ordering": "-added"}, False), + ({"tags__id__in": "1,2"}, False), + ({"search": "foo"}, True), + ({"title_content": "foo"}, True), + ({"content__istartswith": "foo"}, True), + ({"content__iendswith": "foo"}, True), + ({"content__icontains": "foo"}, True), + ({"content__iexact": "foo"}, True), + ], + ) + def test_detects_content_filter_params( + self, + params: dict[str, str], + expected: bool, # noqa: FBT001 + ) -> None: + # GIVEN a view bound to a request carrying the given query params + view = DocumentViewSet() + view.request = SimpleNamespace(query_params=params) + + # WHEN checking whether the effective_content annotation is needed + # THEN it's needed only for requests that actually filter on it + assert view._needs_effective_content_annotation() is expected + + +@pytest.mark.django_db +class TestDocumentListEffectiveContentAnnotation: + """ + DocumentViewSet.get_queryset() only attaches the effective_content + correlated subquery when a request actually filters on it. Attaching it + unconditionally re-executes it once per candidate row before the page's + LIMIT is applied -- fine on SQLite/Postgres, but pathological on + MariaDB's default cardinality estimation for the root_document_id + self-join once candidate counts get large (see the root_document_id / + effective_content perf investigation). + """ + + def test_list_without_content_filter_skips_annotation_but_returns_latest_content( + self, + admin_client: APIClient, + ) -> None: + # GIVEN a root document whose latest version has different content + root = DocumentFactory(content="old-root-content") + DocumentFactory( + root_document=root, + version_index=1, + content="new-version-content", + ) + + # WHEN listing documents with no search/content-filter param + with CaptureQueriesContext(connection) as ctx: + response = admin_client.get("/api/documents/?fields=id,content") + + # THEN the response still reflects the latest version's content... + assert response.status_code == status.HTTP_200_OK + assert response.data["results"] == [ + {"id": root.id, "content": "new-version-content"}, + ] + # ...without the database ever evaluating effective_content per row + assert not any( + "effective_content" in query["sql"] for query in ctx.captured_queries + ) diff --git a/src/documents/views.py b/src/documents/views.py index 0d3f9de99..3d40546cd 100644 --- a/src/documents/views.py +++ b/src/documents/views.py @@ -1085,12 +1085,33 @@ class DocumentViewSet( ], } + # Query params whose filtering needs effective_content evaluated in SQL + # against every candidate row -- see _needs_effective_content_annotation(). + _CONTENT_FILTER_PARAMS = ( + "search", # DRF SearchFilter's search_fields includes effective_content + "title_content", + "content__istartswith", + "content__iendswith", + "content__icontains", + "content__iexact", + ) + + def _needs_effective_content_annotation(self) -> bool: + # effective_content is a per-row correlated subquery resolving each + # document's latest version. Cheap when evaluated only for the page + # that survives filtering/sorting/pagination (the common case, via + # the "versions" prefetch + Document.get_effective_content()'s + # fallback), but if anything filters *on* it, the database has to + # evaluate it for every candidate row before the LIMIT is reached -- + # pathological on MariaDB specifically for the root_document_id + # 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. + params = self.request.query_params + return any(param in params for param in self._CONTENT_FILTER_PARAMS) + def get_queryset(self): - latest_version_content = Subquery( - versions_newest_first( - Document.objects.filter(root_document=OuterRef("pk")), - ).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. @@ -1110,10 +1131,9 @@ class DocumentViewSet( # ObjectFilter.filter(). A blanket .distinct() here forces the # database to fully sort and dedupe every visible document before # it can apply LIMIT, which is disastrous at scale. - return ( + queryset = ( Document.objects.filter(root_document__isnull=True) .order_by("-created", "-id") - .annotate(effective_content=Coalesce(latest_version_content, F("content"))) .annotate(num_notes=Coalesce(note_count, 0)) .select_related("correspondent", "storage_path", "document_type", "owner") .prefetch_related( @@ -1126,6 +1146,7 @@ class DocumentViewSet( "version_label", "root_document_id", "version_index", + "content", ), ), "tags", @@ -1136,6 +1157,16 @@ class DocumentViewSet( "notes", ) ) + if self._needs_effective_content_annotation(): + latest_version_content = Subquery( + versions_newest_first( + Document.objects.filter(root_document=OuterRef("pk")), + ).values("content")[:1], + ) + queryset = queryset.annotate( + effective_content=Coalesce(latest_version_content, F("content")), + ) + return queryset def get_serializer(self, *args, **kwargs): fields_param = self.request.query_params.get("fields", None)