From 73c23f320ee142b429ba899af23d3bc7abce8325 Mon Sep 17 00:00:00 2001 From: stumpylog <797416+stumpylog@users.noreply.github.com> Date: Wed, 9 Sep 2026 09:49:44 -0700 Subject: [PATCH] CLean up the new test with the docstrings, handle the fields in one place --- ...ument_list_effective_content_annotation.py | 256 ++++++++++++++++-- src/documents/views.py | 20 +- 2 files changed, 243 insertions(+), 33 deletions(-) diff --git a/src/documents/tests/test_document_list_effective_content_annotation.py b/src/documents/tests/test_document_list_effective_content_annotation.py index d54ffbaff..785c72145 100644 --- a/src/documents/tests/test_document_list_effective_content_annotation.py +++ b/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) == [] diff --git a/src/documents/views.py b/src/documents/views.py index bfdbcfe9b..71138520c 100644 --- a/src/documents/views.py +++ b/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(