CLean up the new test with the docstrings, handle the fields in one place

src/documents/tests/test_document_list_effective_content_annotation.py

@@ -50,15 +50,68 @@ class TestNeedsEffectiveContentAnnotation:
         params: dict[str, str],
         expected: bool,  # noqa: FBT001
     ) -> None:
-        # GIVEN a view bound to a request carrying the given query params
+        """
+        GIVEN:
+            - A view bound to a request carrying the given query params
+        WHEN:
+            - Checking whether the effective_content annotation is needed
+        THEN:
+            - It is needed only for requests that actually filter on it
+        """
         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
 
 
+class TestNeedsEffectiveContentPrefetch:
+    """
+    DocumentViewSet._needs_effective_content_prefetch() decides whether the
+    single-version content prefetch is worth attaching. It has to read the
+    `fields` param exactly the way get_serializer() does, or a request whose
+    response includes content ends up without the prefetch and pays
+    get_effective_content()'s per-instance fallback instead.
+    """
+
+    @pytest.mark.parametrize(
+        ("params", "expected"),
+        [
+            pytest.param({}, True, id="no-fields-param-keeps-every-field"),
+            pytest.param({"fields": ""}, True, id="blank-fields-keeps-every-field"),
+            pytest.param(
+                {"fields": "id,content"},
+                True,
+                id="content-among-requested-fields",
+            ),
+            pytest.param({"fields": "content"}, True, id="content-only"),
+            pytest.param({"fields": "id"}, False, id="content-not-requested"),
+            pytest.param(
+                {"fields": "id,title"},
+                False,
+                id="several-fields-without-content",
+            ),
+        ],
+    )
+    def test_detects_whether_content_can_reach_the_response(
+        self,
+        params: dict[str, str],
+        expected: bool,  # noqa: FBT001
+    ) -> None:
+        """
+        GIVEN:
+            - A view bound to a request carrying the given query params
+        WHEN:
+            - Checking whether the content prefetch is needed
+        THEN:
+            - It is needed exactly when get_serializer() would emit content,
+              which treats a blank `fields` the same as an absent one
+        """
+        view = DocumentViewSet()
+        view.request = SimpleNamespace(query_params=params)
+
+        assert view._needs_effective_content_prefetch() is expected
+
+
 @pytest.mark.django_db
 class TestDocumentListEffectiveContentAnnotation:
     """
@@ -75,7 +128,15 @@ class TestDocumentListEffectiveContentAnnotation:
         self,
         admin_client: APIClient,
     ) -> None:
-        # GIVEN a root document whose latest version has different content
+        """
+        GIVEN:
+            - A root document whose latest version has different content
+        WHEN:
+            - Listing documents with no search/content-filter param
+        THEN:
+            - The response still reflects the latest version's content
+            - The database never evaluates effective_content per row
+        """
         root = DocumentFactory(content="old-root-content")
         DocumentFactory(
             root_document=root,
@@ -83,24 +144,123 @@ class TestDocumentListEffectiveContentAnnotation:
             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
         )
 
+    @pytest.mark.parametrize(
+        "fields_param",
+        [
+            pytest.param("", id="blank-fields"),
+            pytest.param("id,content", id="content-requested"),
+        ],
+    )
+    def test_content_resolves_without_a_query_per_document(
+        self,
+        admin_client: APIClient,
+        fields_param: str,
+    ) -> None:
+        """
+        GIVEN:
+            - One versioned root document, then two more
+        WHEN:
+            - Listing documents with a `fields` param that keeps content
+        THEN:
+            - Every root's content resolves to its latest version's
+            - The query count does not grow with the number of documents,
+              i.e. a blank `fields` does not skip the prefetch and fall back
+              to loading each root's deferred version content
+        """
+        first = DocumentFactory(content="first-root-content")
+        DocumentFactory(
+            root_document=first,
+            version_index=1,
+            content="first-version-content",
+        )
+
+        with CaptureQueriesContext(connection) as one_document:
+            response = admin_client.get(f"/api/documents/?fields={fields_param}")
+
+        assert response.status_code == status.HTTP_200_OK
+        assert [r["content"] for r in response.data["results"]] == [
+            "first-version-content",
+        ]
+
+        for index in range(2):
+            root = DocumentFactory(content=f"root-content-{index}")
+            DocumentFactory(
+                root_document=root,
+                version_index=1,
+                content=f"version-content-{index}",
+            )
+        with CaptureQueriesContext(connection) as three_documents:
+            response = admin_client.get(f"/api/documents/?fields={fields_param}")
+
+        assert response.status_code == status.HTTP_200_OK
+        assert sorted(r["content"] for r in response.data["results"]) == [
+            "first-version-content",
+            "version-content-0",
+            "version-content-1",
+        ]
+        assert len(three_documents.captured_queries) == len(
+            one_document.captured_queries,
+        )
+
+    def test_list_without_content_field_skips_prefetch_and_omits_content(
+        self,
+        admin_client: APIClient,
+    ) -> None:
+        """
+        GIVEN:
+            - A versioned root document
+        WHEN:
+            - Listing documents without asking for content
+        THEN:
+            - Content is neither serialized nor resolved
+            - Nothing pays for the prefetch or the per-instance fallback
+        """
+        root = DocumentFactory(content="root-content")
+        DocumentFactory(
+            root_document=root,
+            version_index=1,
+            content="version-content",
+        )
+
+        with CaptureQueriesContext(connection) as ctx:
+            response = admin_client.get("/api/documents/?fields=id")
+
+        assert response.status_code == status.HTTP_200_OK
+        assert response.data["results"] == [{"id": root.id}]
+        assert _get_effective_content_fallback_queries(ctx) == []
+        # Only the list query itself reads a content column: no extra query
+        # for the skipped prefetch, none for a per-instance fallback
+        content_queries = [
+            query
+            for query in ctx.captured_queries
+            if '"documents_document"."content"' in query["sql"]
+        ]
+        assert len(content_queries) == 1
+
     def test_latest_version_content_prefetch_carries_only_the_newest_version(
         self,
     ) -> None:
-        # GIVEN a root document with two versions
+        """
+        GIVEN:
+            - A root document with two versions
+        WHEN:
+            - Fetching the root through latest_version_content_prefetch()
+        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)
+        """
         root = DocumentFactory(content="root-content")
         DocumentFactory(
             root_document=root,
@@ -113,7 +273,6 @@ class TestDocumentListEffectiveContentAnnotation:
             content="newest-version-content",
         )
 
-        # WHEN fetching the root through latest_version_content_prefetch()
         fetched_root = (
             Document.objects.filter(pk=root.pk)
             .prefetch_related(
@@ -122,9 +281,6 @@ class TestDocumentListEffectiveContentAnnotation:
             .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"]
 
@@ -139,22 +295,58 @@ class TestHasPrefetchedEffectiveContent:
     """
 
     def test_false_with_no_annotation_or_prefetch(self) -> None:
-        document = Document()
+        """
+        GIVEN:
+            - A document the ORM never annotated or prefetched for
+        WHEN:
+            - Asking whether its effective content is already resolved
+        THEN:
+            - It is not, so the serializer must leave it alone
+        """
+        document = DocumentFactory.build()
+
         assert has_prefetched_effective_content(document) is False
 
     def test_true_with_effective_content_annotation(self) -> None:
-        document = Document()
+        """
+        GIVEN:
+            - A document carrying the queryset's effective_content annotation
+        WHEN:
+            - Asking whether its effective content is already resolved
+        THEN:
+            - It is, straight off the annotation
+        """
+        document = DocumentFactory.build()
         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()
+        """
+        GIVEN:
+            - A document the lean content prefetch ran for, finding no versions
+        WHEN:
+            - Asking whether its effective content is already resolved
+        THEN:
+            - It is: an empty prefetch is an answer, not a missing one
+        """
+        document = DocumentFactory.build()
         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()
+        """
+        GIVEN:
+            - A document carrying only the metadata "versions" prefetch
+        WHEN:
+            - Asking whether its effective content is already resolved
+        THEN:
+            - It is, via get_effective_content()'s prefetch-cache branch
+        """
+        document = DocumentFactory.build()
         document._prefetched_objects_cache = {"versions": []}
+
         assert has_prefetched_effective_content(document) is True
 
 
@@ -191,8 +383,16 @@ class TestTrashAndGlobalSearchEffectiveContentIsNeverPerInstance:
         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
+        """
+        GIVEN:
+            - A trashed root document whose own content differs from what a
+              version would have had (also trashed, deletion cascades)
+        WHEN:
+            - Listing trash
+        THEN:
+            - The response shows the document's own content
+            - Nothing ever queries for versions to resolve it
+        """
         root = DocumentFactory(content="own-content")
         DocumentFactory(
             root_document=root,
@@ -201,23 +401,29 @@ class TestTrashAndGlobalSearchEffectiveContentIsNeverPerInstance:
         )
         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_latest_version_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
+        """
+        GIVEN:
+            - A root document, findable by title, whose own content differs
+              from its latest version's
+        WHEN:
+            - Using the global search endpoint's db_only mode
+        THEN:
+            - The response shows the latest version's content, resolved by
+              GlobalSearchView's own effective_content annotation
+            - There is no per-instance fallback query
+        """
         root = DocumentFactory(title="findme", content="own-content")
         DocumentFactory(
             root_document=root,
@@ -225,16 +431,12 @@ class TestTrashAndGlobalSearchEffectiveContentIsNeverPerInstance:
             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 latest version's content, resolved by
-        # GlobalSearchView's own effective_content annotation...
         assert response.status_code == status.HTTP_200_OK
         [result] = [d for d in response.data["documents"] if d["id"] == root.id]
         assert result["content"] == "version-content"
-        # ...with no per-instance fallback query
         assert _get_effective_content_fallback_queries(ctx) == []

src/documents/views.py

@@ -1120,12 +1120,22 @@ class DocumentViewSet(
             params.get(param, "").strip() for param in self._content_filter_params()
         )
 
+    def _requested_fields(self) -> list[str] | None:
+        # The sparse-fieldset `fields` param, as DynamicFieldsModelSerializer
+        # wants it: None means "no restriction, serialize everything", which
+        # a blank value means too. get_queryset() and get_serializer() both
+        # branch on this, and they have to read it identically -- a queryset
+        # that skips the content prefetch for a response that still
+        # serializes content reintroduces get_effective_content()'s
+        # per-instance fallback.
+        fields_param = self.request.query_params.get("fields")
+        return fields_param.split(",") if fields_param else None
+
     def _needs_effective_content_prefetch(self) -> bool:
         # The prefetch spares get_effective_content() a per-instance fallback
         # query, but only earns itself when content can reach the response.
-        # Mirror get_serializer() below: no `fields` param keeps every field.
-        fields_param = self.request.query_params.get("fields", None)
-        return fields_param is None or "content" in fields_param.split(",")
+        fields = self._requested_fields()
+        return fields is None or "content" in fields
 
     def get_queryset(self):
         # A correlated subquery avoids the LEFT JOIN + Count() this used to
@@ -1181,11 +1191,9 @@ class DocumentViewSet(
         return queryset
 
     def get_serializer(self, *args, **kwargs):
-        fields_param = self.request.query_params.get("fields", None)
-        fields = fields_param.split(",") if fields_param else None
         truncate_content = self.request.query_params.get("truncate_content", "False")
         kwargs.setdefault("context", self.get_serializer_context())
-        kwargs.setdefault("fields", fields)
+        kwargs.setdefault("fields", self._requested_fields())
         kwargs.setdefault("truncate_content", truncate_content.lower() in ["true", "1"])
         try:
             full_perms = get_boolean(