Repository navigation
Add query count tests and remove N+1 queries in page rendering and search - #501
Open
ShubhJain09 wants to merge 3 commits into
Open
ShubhJain09 wants to merge 3 commits into
ShubhJain09 wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The optimizations preserve existing behavior and include focused regression coverage for correctness and query scaling.
Review effort: Balanced
Findings: None
What changed in this PR
Adds query-count regression coverage and removes N+1 queries from page rendering and search suggestions.
Changes:
- Builds table-of-contents data directly from stored rich text.
- Batch-loads search-result sections and generates URLs with request context.
- Adds query-count and search endpoint tests.
| File | Description |
|---|---|
apps/search/views.py |
Optimizes search serialization queries. |
apps/search/tests/test_views.py |
Tests search output and query scaling. |
apps/search/tests/__init__.py |
Initializes the search test package. |
apps/core/models/content.py |
Avoids rendering page bodies twice. |
apps/core/tests/test_content_page.py |
Tests TOC extraction and query behavior. |
apps/core/tests/test_query_counts.py |
Adds page-rendering query regressions. |
apps/core/tests/test_page_response.py |
Updates expected template render counts. |
apps/core/tests/test_text_block_annotated.py |
Updates annotated-block render expectations. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refs #481
Description
This covers the first item on #481 (query count assertions in tests), plus two fixes for places that were making a query per item.
Tests (
apps/core/tests/test_query_counts.py)Table of contents
create_table_of_contentsrendered the whole page body a second time just to find the headings, so every link in the body was looked up twice. It now reads the headings and their ids from the stored rich text, and makes no queries. A content page with 6 linked text blocks goes from 52 queries to 34.Two existing tests expected each block template to render twice (
test_page_responseandtest_text_block_annotated), which was this double render, so I updated their counts.Search suggestions
search_jsonlooked up each result's parent section with a separate query, and built each URL without the request. It now gets all the sections in one query and passes the request through. With 20 results it goes from 26 queries to 7 (or 46 to 8 counting cache reads). There were no tests for the search views, so I added some for the sections, URLs and empty queries.Left for later
Some per-link queries come from Wagtail itself: the Markdown converter looks up each linked page and its parent separately, and
Locale.get_active()isn't cached. Happy to raise those on the Wagtail repo.Testing
All 151 tests pass locally on PostgreSQL 17 and SQLite, along with the backend lint and Django checks. CI is currently failing on
mainatnpm run lintbecause of 3 YAML files incontent/evals, so that step will fail here too untilmainis fixed.AI usage
None