feat: add pagination to change requests list with envelope response#5733
Conversation
rajohnson90
requested review from
Santi-3rd,
fpigeonjr,
johndeange,
josbell and
weimiao67
as code owners
josbell
left a comment
josbell
left a comment
There was a problem hiding this comment.
Review Decision: REQUEST CHANGES
After thorough investigation, I've identified two blocking issues that must be addressed before approval. While the overall approach is solid and the N+1 query fix is excellent work, these issues will cause problems at scale.
🚨 BLOCKING ISSUE #1: Critical Performance Problem
Location: backend/ops_api/ops/utils/change_requests_helpers.py:141-159
Problem: The function loads ALL in-review change requests into memory before filtering and pagination:
stmt = select(cr_poly).where(cr_poly.status == ChangeRequestStatus.IN_REVIEW).order_by(ChangeRequest.id)
results = current_app.db_session.execute(stmt).scalars().all() # ← Loads everything
# ... filter in Python ...
return filtered_results[offset : offset + limit], total_countImpact:
- With 1,000 in-review change requests, every pagination request (page 1, page 2, etc.) loads all 1,000 records into memory
- Polymorphic loading eagerly loads related
Agreementobjects - Python-side filtering and pagination defeats the entire purpose of pagination
- This creates a memory bomb at scale
Evidence: The old version had .limit(limit).offset(offset) on the SQL query - this was removed in favor of Python list slicing at line 159.
Recommendation: Move pagination to SQL level after permission filtering. Two approaches:
Option A (Preferred): Push permission filtering into SQL:
# Build SQL filters for reviewable CRs
bli_filter = and_(
cr_poly.type == 'budget_line_item_change_request',
BudgetLineItemChangeRequest.managing_division_id.in_(reviewable_division_ids)
)
agr_filter = and_(
cr_poly.type == 'agreement_change_request',
AgreementChangeRequest.agreement_id.in_(reviewable_agreement_ids)
)
stmt = (
select(cr_poly)
.where(cr_poly.status == ChangeRequestStatus.IN_REVIEW)
.where(or_(bli_filter, agr_filter))
.order_by(ChangeRequest.id)
)
# Count and paginate at SQL level
count_stmt = select(func.count()).select_from(stmt.subquery())
total_count = current_app.db_session.execute(count_stmt).scalar()
results = current_app.db_session.execute(stmt.limit(limit).offset(offset)).scalars().all()
return results, total_countOption B (Acceptable for MVP): Add safeguards and document the limitation:
if len(results) > 1000: # Configurable threshold
current_app.logger.warning(f"Large change request result set: {len(results)} items")
# Proceed with Python filtering🚨 BLOCKING ISSUE #2: API Contract Violation
Location: backend/ops_api/ops/services/change_requests.py:79-80
Problem: When query params are missing, the response violates the OpenAPI schema:
page, total_count = find_in_review_requests_by_user(
int(reviewer_user_id),
data.get("limit"), # ← Returns None if missing
data.get("offset"), # ← Returns None if missing
)
return page, {"count": total_count, "limit": data.get("limit"), "offset": data.get("offset")}Impact:
- OpenAPI schema declares
limitandoffsetas required integers (lines 27-31 in openapi.yml) - Response can contain
{"data": [...], "count": 10, "limit": null, "offset": null} - This violates the API contract and may break clients that rely on schema validation
Note: While the function signature has defaults (limit: int = 10, offset: int = 0), those only protect the pagination logic - the metadata dictionary still contains None values.
Recommendation: Add explicit defaults in the service layer:
page, total_count = find_in_review_requests_by_user(
int(reviewer_user_id),
data.get("limit", 10), # ← Explicit default
data.get("offset", 0), # ← Explicit default
)
return page, {
"count": total_count,
"limit": data.get("limit", 10),
"offset": data.get("offset", 0)
}⚠️ Non-Blocking: API Breaking Change
Location: backend/openapi.yml and backend/ops_api/ops/resources/change_requests.py
Observation: The response format changes from a plain array to an envelope object:
- Before:
[{...}, {...}] - After:
{"data": [...], "count": 10, "limit": 10, "offset": 0}
This is a breaking change for any API consumers. Existing code expecting response[0] will break (should be response.data[0]).
Impact Assessment:
- ✅ Frontend is updated in the same PR, so internal consumers are safe
⚠️ External consumers (if any) will break⚠️ Requires atomic deployment (backend + frontend together)
Recommendation: If this is internal-only API, acceptable with atomic deployment. Otherwise, consider API versioning or a transition period.
📝 Minor Observations
Count Edge Case (Standard Behavior)
If offset >= total_count, the response returns empty data: [] but count still shows the total. This is correct per REST pagination conventions (e.g., GitHub API, Stripe API) - count should remain stable across pages.
Database Index
The query filters by change_request.status - verify that an index exists on this column to avoid table scans at scale.
✅ What's Good
- Excellent N+1 Fix: The new
get_reviewable_agreement_ids_for_userfunction eliminates N+1 queries with a single upfront JOIN - textbook SQL optimization - Clean Envelope Pattern: The
{data, count, limit, offset}structure follows REST pagination best practices - Comprehensive Test Coverage: Tests updated correctly, including new E2E test for pagination
- Deterministic Ordering: Adding
.order_by(ChangeRequest.id)ensures consistent pagination - Well-Documented OpenAPI Schema: Clear descriptions for each envelope field
Summary
Please address the two blocking issues:
- Critical: Fix the load-all-then-paginate pattern (performance/memory issue)
- High: Add explicit defaults to prevent
nullin required integer fields (API contract violation)
Once these are resolved, I'll be happy to re-review. The core architecture is sound - these are implementation details that need refinement for production readiness.
|
I've moved the filter to be in the SQL query. I provided default values for the limit and offset in the service. I verified there is already an existing index on status so no need to worry on that. |
jonnalley
left a comment
jonnalley
left a comment
There was a problem hiding this comment.
reviewed and looks pretty decent to me. I also had a couple different sets of sub-agents review under separate contexts and using different methodologies and only found nit-pick and pre-existing issues. LGTM, thank you!
|
🎉 This PR is included in version 1.405.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
3 participants
What changed
The
/change-requests/GET endpoint now returns a paginated envelope{data, count, limit, offset}instead of a plain array.countreflects the total number of change requests after permission filtering but before the pagination slice, so a division director always sees their true total even when results span multiple pages.backend/ops_api/ops/utils/change_requests_helpers.pyfind_in_review_requests_by_usernow returns a(page, total_count)tuple. Pagination is applied in Python after permission filtering (not at SQL level) so no reviewable CRs are silently dropped. N+1get_division_directors_for_agreementcalls replaced by a single SQL join viaget_reviewable_agreement_ids_for_user.backend/ops_api/ops/services/change_requests.pyget_listunpacks the tuple and forwardscount/limit/offsetin metadata.backend/ops_api/ops/resources/change_requests.py{data, count, limit, offset}envelope.limitandoffsetare now read from query params and forwarded through the service.backend/openapi.yml/change-requests/200 response updated fromarrayto the paginated envelope schema.frontend/src/api/opsAPI.jsgetChangeRequestsListquery now accepts and forwardslimitandoffsetquery params.frontend/src/components/ChangeRequests/ChangeRequestsList/ChangeRequestsList.jsxcurrentPagestate andPaginationNav(page size 10, server-side). ReadschangeRequestsResponse.datafrom envelope; derivestotalPagesfromcount.frontend/src/hooks/useChangeRequests.hooks.jsuseChangeRequestTotalpasseslimit: 10, offset: 0to share the RTK Query cache withChangeRequestsList, and readscountfrom the envelope instead ofdata.length.Tests updated:
ChangeRequestList.test.jsx,AgreementsList.test.jsx,useChangeRequests.hooks.test.js,test_change_requests_helpers.py,test_change_requests.py(workflow).frontend/cypress/e2e/changeRequestsPagination.cy.jsIssue
#5725
How to test
Backend unit/integration tests:
cd backend/ops_api pipenv run pytest tests/ops/utils/test_change_requests_helpers.py tests/ops/workflows/test_change_requests.py -x -qFrontend unit tests:
cd frontend npx vitest run src/components/ChangeRequests/ChangeRequestsList/ChangeRequestList.test.jsx src/hooks/useChangeRequests.hooks.test.js src/pages/agreements/list/AgreementsList.test.jsxCypress E2E test:
Manual verification:
Budget Team Membervia FakeAuth.Division Director(Dave Director, division 4)./agreements?filter=change-requests("For Review" tab).A11y impact
A11Y-SUPPRESSIONmetadata (owner, expires, rationale)Screenshots
Page 1
Page 2
Definition of Done Checklist
Links
N/A