feat(mongox): fix pagination #47
Conversation
WalkthroughThe updates to the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Collection
participant Database
Client->>Collection: Request Paginate
Collection->>Collection: Generate Filters
Collection->>Collection: Determine Sort Order
Collection->>Database: Execute Query with Filters and Sort
Database-->>Collection: Return Paginated Results
Collection-->>Client: Send Paginated Data
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- mongox/pagination.go (4 hunks)
- mongox/pagination_test.go (1 hunks)
Additional comments not posted (7)
mongox/pagination.go (6)
20-24: Ensure error handling is comprehensive.The error handling for the
pageFiltermethod call looks good. Ensure that all potential errors are covered and that the context is appropriately used for error reporting.
30-37: Clarify sort key and order logic.The logic for determining the
sortKeyandsortOrderis clear and handles theRevertedflag well. Ensure that this logic aligns with the expected behavior of the pagination feature.
71-71: Ensure cursor swap logic is correct.The logic for swapping
startCursorandendCursorwhen reversing items is correct. Ensure that this behavior is intended and tested.
237-299: ReviewpageFilterlogic for correctness.The
pageFiltermethod has been updated to return abson.Mtype, improving type safety. The logic for constructing the filter based on the cursor and sort order is sound. Ensure that the filter logic aligns with the expected pagination behavior.
301-307: Review new helper functiongetCursorDocument.The
getCursorDocumentfunction encapsulates the logic for retrieving a document based on the cursor. This improves modularity and readability. Ensure that error handling covers all potential failure points.
39-41: Verify cursor handling logic.The logic for reversing the sort order when
p.Cursor.Lastis present is correct. Verify that this logic is consistent with the rest of the pagination implementation.Verification successful
Cursor handling logic is consistent.
The logic for reversing the sort order when
p.Cursor.Lastis present is consistently applied across the pagination implementation inmongox/pagination.go. This includes reversing the sort order, reversing items, and determining pagination limits. The handling ofp.Cursor.Lastis coherent with the intended pagination behavior.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify cursor handling logic consistency in the codebase. # Test: Search for references to `p.Cursor.Last`. Expect: Consistent handling of cursor logic. rg --type go 'p\.Cursor\.Last'Length of output: 516
Script:
#!/bin/bash # Extract more context around each occurrence of `p.Cursor.Last` to verify the logic. rg --type go 'p\.Cursor\.Last' -A 3 -B 3Length of output: 1956
mongox/pagination_test.go (1)
341-433: Comprehensive test coverage for pagination.The
TestClientCollection_DetailedPaginationfunction provides extensive test coverage for various cursor-based pagination scenarios. The use of seeded data and multiple test cases ensures robustness. Ensure that the test cases cover all edge cases and expected behaviors.
| findOpts := options.Find(). | ||
| SetSort(bson.D{{Key: sortKey, Value: sortOrder}, {Key: idKey, Value: sortOrder}}). | ||
| SetLimit(limit(*p)) | ||
|
|
||
| if p.Offset != nil { | ||
| findOpts.SetSkip(p.Offset.Offset) | ||
| } |
There was a problem hiding this comment.
Optimize find options configuration.
The findOpts configuration correctly sets sorting and limit options. Consider consolidating the logic for setting options to enhance readability and maintainability.
- findOpts := options.Find().
- SetSort(bson.D{{Key: sortKey, Value: sortOrder}, {Key: idKey, Value: sortOrder}}).
- SetLimit(limit(*p))
+ findOpts := options.Find().
+ SetSort(bson.D{{Key: sortKey, Value: sortOrder}, {Key: idKey, Value: sortOrder}}).
+ SetLimit(limit(*p))
if p.Offset != nil {
findOpts.SetSkip(p.Offset.Offset)
}Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- mongox/pagination.go (6 hunks)
Additional comments not posted (3)
mongox/pagination.go (3)
20-24: Approve the use ofpageFilter.The use of
c.pageFilterimproves type safety and clarity in filter handling.
43-49: Optimize find options configuration.The
findOptsconfiguration correctly sets sorting and limit options. Consider consolidating the logic for setting options to enhance readability and maintainability.
273-279: Approve the addition ofgetCursorDocument.The function encapsulates cursor retrieval logic, enhancing modularity and readability.
| func (c *Collection) pageFilter(ctx context.Context, p usecasex.Pagination, s *usecasex.Sort) (bson.M, error) { | ||
| if p.Cursor == nil { | ||
| return nil, nil | ||
| } | ||
|
|
||
| var filter bson.M | ||
| sortKey := idKey | ||
| sortOrder := 1 | ||
|
|
||
| if s != nil && s.Key != "" { | ||
| sortKey = s.Key | ||
| if s.Reverted { | ||
| sortOrder = -1 | ||
| } | ||
| } | ||
|
|
||
| var cursor *usecasex.Cursor | ||
| var op string | ||
| var cur *usecasex.Cursor | ||
|
|
||
| if p.Cursor.First != nil { | ||
| if p.Cursor.After != nil { | ||
| cursor = p.Cursor.After | ||
| op = "$gt" | ||
| cur = p.Cursor.After | ||
| } else if p.Cursor.Last != nil { | ||
| } else if p.Cursor.Before != nil { | ||
| cursor = p.Cursor.Before | ||
| op = "$lt" | ||
| cur = p.Cursor.Before | ||
| } else { | ||
| return nil, errors.New("neither first nor last are set") | ||
| } | ||
| if cur == nil { | ||
| return nil, nil | ||
| } | ||
|
|
||
| var sortKey *string | ||
| if s != nil { | ||
| sortKey = &s.Key | ||
| } | ||
| var paginationFilter bson.M | ||
| if sortKey == nil || *sortKey == "" { | ||
| paginationFilter = bson.M{idKey: bson.M{op: *cur}} | ||
| } else { | ||
| var cursorDoc bson.M | ||
| if err := c.collection.FindOne(ctx, bson.M{idKey: *cur}).Decode(&cursorDoc); err != nil { | ||
| return nil, fmt.Errorf("failed to find cursor element") | ||
| } | ||
|
|
||
| if cursorDoc[*sortKey] == nil { | ||
| return nil, fmt.Errorf("invalied sort key") | ||
| if cursor != nil { | ||
| cursorDoc, err := c.getCursorDocument(ctx, *cursor) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| paginationFilter = bson.M{ | ||
| filter = bson.M{ | ||
| "$or": []bson.M{ | ||
| {*sortKey: bson.M{op: cursorDoc[*sortKey]}}, | ||
| {sortKey: bson.M{op: cursorDoc[sortKey]}}, | ||
| { | ||
| *sortKey: cursorDoc[*sortKey], | ||
| idKey: bson.M{op: *cur}, | ||
| sortKey: cursorDoc[sortKey], | ||
| idKey: bson.M{op: cursorDoc[idKey]}, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| if sortOrder == -1 { | ||
| if op == "$gt" { | ||
| op = "$lt" | ||
| } else { | ||
| op = "$gt" | ||
| } | ||
| filter = bson.M{ | ||
| "$or": []bson.M{ | ||
| {sortKey: bson.M{op: cursorDoc[sortKey]}}, | ||
| { | ||
| sortKey: cursorDoc[sortKey], | ||
| idKey: bson.M{op: cursorDoc[idKey]}, | ||
| }, | ||
| }, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return paginationFilter, nil | ||
| return filter, nil |
There was a problem hiding this comment.
Optimize duplicated filter logic for reversed sort order.
The logic for creating the filter when the sort order is reversed is duplicated. Consider refactoring to reduce redundancy.
if cursor != nil {
cursorDoc, err := c.getCursorDocument(ctx, *cursor)
if err != nil {
return nil, err
}
op1, op2 := op, "$lt"
if sortOrder == -1 {
op1, op2 = "$lt", "$gt"
}
filter = bson.M{
"$or": []bson.M{
{sortKey: bson.M{op1: cursorDoc[sortKey]}},
{
sortKey: cursorDoc[sortKey],
idKey: bson.M{op2: cursorDoc[idKey]},
},
},
}
}
Summary by CodeRabbit
New Features
Bug Fixes
Tests