Feat/pagination poc#7708
Conversation
Introduces CursorPage/CursorRequest models with opaque Base64 keyset cursors, and implements cursor pagination across MongoDB, JDBC, and dataplane repositories for domains, applications, and users.
New endpoints with sort and totalCount support alongside existing offset-based endpoints to avoid breaking changes.
Replaces offset pagination with cursor prev/next navigation in domains, applications, and users list views. Adds external sorting support.
- Return 400 instead of 500 for malformed cursors - Fix JDBC date parsing crash on null updatedAt (use epoch millis) - Throw on invalid date cursor values instead of silent epoch-zero fallback - Handle null username in MongoDB user cursor encoding - Add COALESCE for NULL usernames in JDBC keyset queries - Derive hasNext from nextCursor to prevent contradictory states - Add totalCount to all JDBC cursor queries for consistency with MongoDB - Encode sort field in JDBC cursors (4-arg encode) - Reject conflicting sort field between cursor and query param - Narrow SCIM FilterCriteria catch blocks - Validate sortParam="-" edge case and null direction
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements cursor-based pagination for security domains, applications, and users across the Management API, data repositories (JDBC and MongoDB), and the UI to enhance performance at scale. Feedback highlights several critical issues in the repository layer, specifically where sort fields are hardcoded, causing the system to ignore user-specified sorting and potentially leading to NumberFormatException when parsing cursor values. Performance improvements are suggested to avoid full table scans caused by COALESCE in SQL clauses and to mitigate the overhead of executing full document counts on every request. Additionally, the manual manipulation of SQL strings to strip ordering was identified as a fragile implementation detail.
| sql.append(" AND (COALESCE(u.username, '') ").append(compareOp).append(" :lastSort") | ||
| .append(" OR (COALESCE(u.username, '') = :lastSort AND u.id ").append(compareOp).append(" :lastId))"); | ||
| } | ||
| sql.append(" ORDER BY COALESCE(u.username, '') ").append(sortDir).append(", u.id ").append(sortDir) |
There was a problem hiding this comment.
The use of COALESCE(u.username, '') in both the WHERE and ORDER BY clauses will likely prevent the database from utilizing an index on the username column. This can lead to full table scans, which significantly impacts performance at scale. Consider ensuring the sort column is non-nullable or using a strategy that maintains index eligibility.
| sql.append(" AND (COALESCE(u.username, '') ").append(compareOp).append(" :lastSort") | ||
| .append(" OR (COALESCE(u.username, '') = :lastSort AND u.id ").append(compareOp).append(" :lastId))"); | ||
| } | ||
| sql.append(" ORDER BY COALESCE(u.username, '') ").append(sortDir).append(", u.id ").append(sortDir) |
There was a problem hiding this comment.
The repository implementation hardcodes username as the sort field, ignoring the sortField specified in the CursorRequest. This will cause incorrect results or failures if the user attempts to sort by a different field (e.g., updatedAt) as supported by the UI.
| sql.append(" AND (a.updated_at ").append(compareOp).append(" :lastSort") | ||
| .append(" OR (a.updated_at = :lastSort AND a.id ").append(compareOp).append(" :lastId))"); | ||
| } | ||
|
|
||
| sql.append(" ORDER BY a.updated_at ").append(sortDir).append(", a.id ").append(sortDir) |
There was a problem hiding this comment.
The repository implementation hardcodes updated_at as the sort field, ignoring the sortField specified in the CursorRequest. This will cause incorrect results or failures if the user attempts to sort by a different field (e.g., name) as supported by the UI.
|
|
||
| if (!cursor.isFirstPage()) { | ||
| try { | ||
| long epochMs = Long.parseLong(cursor.getLastSortValue()); |
There was a problem hiding this comment.
This line assumes the lastSortValue is always a numeric timestamp. If the sort field is changed to name (as supported by the UI), Long.parseLong will throw a NumberFormatException. The logic should dynamically handle the type of the sort value based on the requested sort field.
| sql.append(" AND (LOWER(d.name) ").append(compareOp).append(" LOWER(:lastSort)") | ||
| .append(" OR (LOWER(d.name) = LOWER(:lastSort) AND d.id ").append(compareOp).append(" :lastId))"); | ||
| } | ||
|
|
||
| sql.append(" ORDER BY LOWER(d.name) ").append(sortDir).append(", d.id ").append(sortDir) |
There was a problem hiding this comment.
The repository implementation hardcodes name as the sort field, ignoring the sortField in the CursorRequest.
| return findCursorPage(usersCollection, baseFilter, cursor, FIELD_USERNAME, | ||
| this::convert, u -> u.getUsername() != null ? u.getUsername() : "", User::getId); |
There was a problem hiding this comment.
The repository implementation hardcodes FIELD_USERNAME as the sort field, ignoring the sortField specified in the CursorRequest. This will cause incorrect results if the user attempts to sort by a different field.
| int orderByIdx = selectQuery.toUpperCase().lastIndexOf("ORDER BY"); | ||
| String baseQuery = orderByIdx > 0 ? selectQuery.substring(0, orderByIdx) : selectQuery; |
There was a problem hiding this comment.
Manual string manipulation of SQL queries generated by databaseDialectHelper is fragile. Using lastIndexOf("ORDER BY") to strip ordering can fail if the query contains ORDER BY within subqueries or string literals. It is recommended to use a more robust approach for query modification or ensure the dialect helper provides a base query without ordering.
|
|
||
| Bson sort = new BasicDBObject(sortFieldName, sortDir).append(FIELD_ID, sortDir); | ||
|
|
||
| Single<Long> countOperation = countItems(collection, baseFilter, countOptions()); |
There was a problem hiding this comment.
Executing a countDocuments operation on every page request can be expensive for large collections. Since cursor-based pagination is intended for performance at scale, consider making the total count optional or caching it to avoid the overhead of a full count on every request.
|
1 participant
🆔 Reference related issue.
✏️ A description of the changes proposed in the pull request
📝 Test scenarios
💻 Add screenshots for UI
📚 Any other comments that will help with documentation
👨 👩@mentions of the person or team responsible for reviewing proposed changes
This is a nice example: #1822