Repository navigation
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughOffsetPaginator now determines whether a next page exists by comparing the total count with the offset and limit. Unit tests cover next-page flags, count usage, and lazy result handling. ChangesOffset pagination
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Refactor Merge Risk: ⚪ Minimal · up to The pagination change is ready to merge after normal checks; no actionable regression was established. Security Architecture ReviewSecurity architecture risk: ⚪ Minimal · up to The change removes a redundant page-window count without changing which rows are returned, access controls, or response fields. No introduced security concern was identified. Equivalent next-page behavior depends on consistent counts and page contents. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
OffsetPaginator ran two COUNT queries per page: one for total_count, and a second one over the page window purely to work out whether a next page exists. total_count already answers that, so the second query is redundant. Every list endpoint in plane/api, plane/app and plane/space goes through this paginator, so this is one less query per list request across the board. results is deliberately left as an unevaluated queryset -- on_results callbacks call queryset methods on it (issue_on_results does .values()). Adds unit tests covering the next-page flag at the page boundaries, the query count, and that results is handed back unevaluated.
This branch has not been deployed
1 participant
Description
OffsetPaginator.get_result()runs two COUNT queries for every page:The second one exists only to answer "is there a next page?". It counts the rows in the current page window (
queryset[offset : offset + limit + 1]) and checks whether there are more thanlimitof them.But
total_countalready answers that. If the total row count runs past the end of the current window, there's a next page:So the
page_resultsqueryset and its COUNT can go away entirely. One less query per page.This is worth doing because
OffsetPaginatoris the default paginator inBasePaginator.paginate(), which every list endpoint inplane/api,plane/appandplane/spacegoes through.Why the two are equivalent
page_resultsisqueryset[offset : offset + limit + 1], so its count ismin(limit + 1, total_count - offset). Therefore:Exact, including at the boundaries — an exactly-full final page still correctly reports no next page. Those cases are covered by parametrised tests below.
When
total_count_querysetis passed in, it's a pre-annotation copy of the same filtered queryset (copy.deepcopy(issue_queryset)inissue/base.py,view/base.py,module/issue.pyand others) — same rows, just cheaper to count — so the equivalence holds there too.One thing I deliberately did not do
The tempting version of this fix is to materialise the page and use
len():That breaks
on_results.issue_on_resultsinplane/utils/grouper.pydoeslist(issues.values(*required_fields))on whatever it's handed, soresultshas to stay an unevaluated queryset. This change leaves it completely alone, and there's a test pinning that behaviour so it doesn't regress later.Type of Change
Screenshots and Media (if applicable)
N/A — no user-facing change. Same response payload, one less query per page.
Test Scenarios
Added to the existing
plane/tests/unit/utils/test_paginator.py. No database needed — they use a small fake queryset that recordscount()calls and whether the result set was evaluated.test_next_page_flag_matches_row_count— parametrised over 8 cases including the boundaries: empty result set, exactly one full page, one row past the boundary, exact multiples, and the final partial page.test_page_costs_a_single_count_query— assertscount()is called exactly once per page.test_total_count_queryset_is_counted_instead_of_the_page_queryset— whentotal_count_querysetis supplied, that one is counted and the page queryset is not.test_results_are_handed_back_unevaluated— guards theon_resultscontract described above.The 5 existing GHSA-wwgj-929g-42cm regression tests in the same file still pass.
ruff checkandruff format --checkare clean on both files.References
No existing issue — spotted while reading the paginator. Happy to open one first if that's the preferred route for perf changes.