fix(server): sort duplicate key error #55
Conversation
WalkthroughThe changes in this pull request focus on enhancing the pagination functionality in the Changes
Possibly related PRs
Poem
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 using PR comments)
Other keywords and placeholders
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
🧹 Outside diff range and nitpick comments (2)
mongox/pagination.go (2)
210-214: Consider making collation settings configurableThe current implementation hardcodes collation settings. While these settings are reasonable defaults, consider making them configurable to support different locales and comparison strengths based on collection requirements.
-func aggregateOptionsFromPagination(_ usecasex.Pagination, _ *usecasex.Sort) *options.AggregateOptions { +func aggregateOptionsFromPagination(_ usecasex.Pagination, _ *usecasex.Sort, opts ...func(*options.Collation)) *options.AggregateOptions { collation := options.Collation{ Locale: "en", Strength: 2, } + for _, opt := range opts { + opt(&collation) + } return options.Aggregate().SetAllowDiskUse(true).SetCollation(&collation) }
Line range hint
1-324: Overall implementation assessmentThe changes effectively address the sort duplicate key error through proper handling of compound sorts and the introduction of collation settings. The implementation is solid, with a few suggested improvements:
- Add validation for sort keys to prevent runtime errors
- Make collation settings configurable for better flexibility
- Fix potential nil pointer dereference in sortFilter
The core functionality appears correct and the changes align well with the PR objectives.
Consider adding integration tests to verify the sorting behavior with various combinations of sort keys and pagination parameters.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
mongox/pagination.go(4 hunks)
🔇 Additional comments (2)
mongox/pagination.go (2)
11-11: LGTM: Proper use of primitive.D for sort specification
The change correctly uses primitive.D for sort specifications and properly handles both single and compound sort keys while maintaining consistency with idKey.
Also applies to: 44-49
52-52: Verify sort key validation
While the sort implementation is correct, consider adding validation for sort keys to prevent runtime errors when invalid fields are specified.
| var sortOptions bson.D | ||
| if s != nil && s.Key != "" && s.Key != idKey { | ||
| sortOptions = append(sortOptions, bson.E{Key: s.Key, Value: sortDirection(p, s)}) | ||
| } | ||
| return append(sortOptions, bson.E{Key: idKey, Value: sortDirection(p, s)}) |
There was a problem hiding this comment.
Fix potential nil pointer dereference
The condition s != nil && s.Key != "" is correct, but the subsequent code assumes s is not nil when calling sortDirection(p, s). While sortDirection handles nil s, it would be more explicit to handle this in the sort options construction.
func sortFilter(p usecasex.Pagination, s *usecasex.Sort) bson.D {
var sortOptions bson.D
if s != nil && s.Key != "" && s.Key != idKey {
- sortOptions = append(sortOptions, bson.E{Key: s.Key, Value: sortDirection(p, s)})
+ direction := sortDirection(p, s)
+ sortOptions = append(sortOptions, bson.E{Key: s.Key, Value: direction})
+ return append(sortOptions, bson.E{Key: idKey, Value: direction})
}
- return append(sortOptions, bson.E{Key: idKey, Value: sortDirection(p, s)})
+ return bson.D{{Key: idKey, Value: sortDirection(p, nil)}}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var sortOptions bson.D | |
| if s != nil && s.Key != "" && s.Key != idKey { | |
| sortOptions = append(sortOptions, bson.E{Key: s.Key, Value: sortDirection(p, s)}) | |
| } | |
| return append(sortOptions, bson.E{Key: idKey, Value: sortDirection(p, s)}) | |
| func sortFilter(p usecasex.Pagination, s *usecasex.Sort) bson.D { | |
| var sortOptions bson.D | |
| if s != nil && s.Key != "" && s.Key != idKey { | |
| direction := sortDirection(p, s) | |
| sortOptions = append(sortOptions, bson.E{Key: s.Key, Value: direction}) | |
| return append(sortOptions, bson.E{Key: idKey, Value: direction}) | |
| } | |
| return bson.D{{Key: idKey, Value: sortDirection(p, nil)}} | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
mongox/pagination_test.go (2)
424-500: LGTM! Well-structured test cases for sorting logic.The test cases effectively verify the sorting behavior and specifically address the duplicate key error mentioned in the PR objectives. The implementation properly handles different sort keys and orders, with special handling for the "id" field.
Consider adding a test case that verifies the behavior when sorting with empty/null values in the sort fields to ensure robust handling of edge cases.
250-319: Consider consolidating test cases and reducing setup duplication.While the test cases provide good coverage, consider:
- Moving these test cases to
TestClientCollection_DetailedPaginationsince they test similar scenarios- Creating a helper function for test data setup as it's duplicated across multiple test functions
Example helper function:
func setupTestData(t *testing.T, c *Collection) { seeds := []struct { id string updatedAt int64 }{ {"a", 1000}, {"b", 2000}, {"c", 3000}, {"d", 4000}, {"e", 5000}, } _, err := c.Client().InsertMany(context.Background(), lo.Map(seeds, func(s struct { id string updatedAt int64 }, i int) any { return bson.M{"id": s.id, "updatedAt": s.updatedAt} })) assert.NoError(t, err) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
mongox/pagination_test.go(5 hunks)
🔇 Additional comments (2)
mongox/pagination_test.go (2)
346-397: LGTM! Improved test case structure and readability.
The formatting changes enhance code readability while maintaining comprehensive test coverage for pagination scenarios.
493-497: Verification: Sort duplicate key error fix is properly tested.
The test cases effectively verify that:
- When sorting by "id", only the id field is used in sort options
- When sorting by other fields, "id" is properly added as a secondary sort key to prevent duplicate key issues
This implementation aligns perfectly with the PR objective of fixing the sort duplicate key error.
✅ Verification successful
Sort implementation is consistent and properly handles duplicate key cases
The codebase shows two consistent sort implementations:
- In
Findoperations: Special handling for "id" field to prevent duplicate sort keys - In
Aggregateoperations: UsingsortFilterfunction that always appends "id" as secondary sort key
The test cases in TestPaginate_SortLogic comprehensively verify both ascending and descending sort scenarios for both "id" and other fields, confirming the fix works as intended.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that the sort implementation is consistent across the codebase
# Look for other sort implementations that might need similar fixes
# Search for sort implementations
echo "Searching for sort implementations..."
rg -l "SetSort|sort.*primitive\.D" --type go
# Search for potential test cases involving sort
echo "Searching for sort-related test cases..."
rg -l "test.*sort|sort.*test" --type go
Length of output: 336
Script:
#!/bin/bash
# Let's examine the sort implementations in detail
# Check pagination.go sort implementation
echo "=== Examining pagination.go sort implementation ==="
rg "sort" -B 3 -A 3 mongox/pagination.go
# Check user.go sort implementation
echo "=== Examining account/accountinfrastructure/accountmongo/user.go sort implementation ==="
rg "sort" -B 3 -A 3 account/accountinfrastructure/accountmongo/user.go
# Look for any sort-related test cases with more specific patterns
echo "=== Examining sort-related test cases ==="
rg "TestPaginate.*Sort|test.*sort.*func|sort.*assert" -B 2 -A 2 --type go
Length of output: 2675
Script:
#!/bin/bash
# Let's examine the TestPaginate_SortLogic test function in detail
echo "=== Examining TestPaginate_SortLogic test and its context ==="
rg "TestPaginate_SortLogic" -B 10 -A 50 mongox/pagination_test.go
# Let's also check the implementation of sortFilter function which seems to handle sort logic
echo "=== Examining sortFilter implementation and its usages ==="
rg "sortFilter" -B 5 -A 10 mongox/pagination.go
Length of output: 2665
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
mongox/pagination_test.go (3)
424-427: Remove unused helper functionThe
toJSONStringfunction is not used anywhere in the test file. Consider removing it to maintain code cleanliness.
250-319: Consider consolidating overlapping test functionsThis test function has significant overlap with
TestClientCollection_DetailedPagination. Both test similar pagination scenarios withupdatedAtsorting. Consider consolidating these tests to:
- Reduce code duplication
- Maintain a single source of truth for pagination test cases
- Make the test suite more maintainable
429-499: Add test cases for edge scenariosWhile the test cases cover the basic sorting scenarios well, consider adding the following edge cases to make the test suite more robust:
- Documents with duplicate values in sort fields
- Documents with missing sort fields
These scenarios would help verify the sorting behavior in edge cases and prevent potential regressions.
Example test cases to add:
cases := []struct { name string sortKey string sortOrder int expectedOrder []usecasex.Cursor }{ + { + name: "Sort with duplicate updatedAt values", + sortKey: "updatedAt", + sortOrder: 1, + expectedOrder: []usecasex.Cursor{"a", "b", "c"}, + }, + { + name: "Sort with missing updatedAt field", + sortKey: "updatedAt", + sortOrder: 1, + expectedOrder: []usecasex.Cursor{"missing", "a", "b", "c"}, + }, }And update the test data accordingly:
seeds := []struct { id string updatedAt int64 }{ {"a", 1000}, {"b", 2000}, {"c", 3000}, + {"d", 2000}, // duplicate updatedAt + {"missing", nil}, // missing updatedAt }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
mongox/pagination_test.go(5 hunks)
🔇 Additional comments (1)
mongox/pagination_test.go (1)
Line range hint 346-406: LGTM! Well-structured test cases
The test cases are comprehensive and well-organized, covering all pagination scenarios with both ascending and descending orders.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
mongox/pagination_test.go (1)
249-318: Consider consolidating duplicate test scenarios.While the test implementation is good, there's some overlap with
TestClientCollection_DetailedPagination. Consider consolidating these tests to reduce duplication and improve maintainability.You could:
- Move common test scenarios to a shared test data structure
- Create a helper function for common assertion logic
- Keep only unique test cases in each function
Example refactoring:
type paginationTestCase struct { name string sort *usecasex.Sort pagination *usecasex.CursorPagination expected []string } func commonPaginationTests() []paginationTestCase { return []paginationTestCase{ // Common test cases here } } func (c *ClientCollection) runPaginationTest(t *testing.T, tc paginationTestCase) { con := &consumer{} _, err := c.Paginate(context.Background(), bson.M{}, tc.sort, tc.pagination.Wrap(), con) assert.NoError(t, err) assert.Equal(t, tc.expected, con.Cursors) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
Makefile(1 hunks)mongox/pagination_test.go(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- Makefile
🔇 Additional comments (2)
mongox/pagination_test.go (2)
423-493: LGTM! Well-structured test cases for sorting logic.
The new test function comprehensively covers sorting scenarios for both "id" and "updatedAt" fields in ascending and descending orders. This directly addresses the PR's objective of fixing the sort duplicate key error.
Line range hint 345-405: LGTM! Enhanced test structure with clear naming.
The test cases are well-organized with descriptive names that clearly indicate the test scenario. The structure improvements make the tests more maintainable and easier to understand.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
account/accountdomain/workspace/member.go(3 hunks)
🔇 Additional comments (2)
account/accountdomain/workspace/member.go (2)
226-228: LGTM: Proper mutex implementation in Leave method
The mutex locking pattern is correctly implemented with proper use of defer for unlocking.
Line range hint 84-89: Verify thread safety of sorting operations
The sorting operations in UserIDs() and IntegrationIDs() could be affected by concurrent map modifications. This might be related to the reported sort duplicate key error.
Also applies to: 94-99
✅ Verification successful
Thread safety is properly implemented for sorting operations
The sorting operations are safe from concurrent access issues because:
- The
Membersstruct has a mutex (sync.Mutex) for synchronization - Before accessing the maps, defensive copies are made using
maps.Clone() - The sorting is performed on these cloned copies, not the original data
The reported sort duplicate key error is likely unrelated to these sorting operations as they operate on safely copied data.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all sorting operations in the codebase that might be affected by concurrent access
rg -n "sort\." --type go
# Search for any reported issues or tests related to sorting and concurrency
rg -n "duplicate.*key|concurrent.*sort" --type go
Length of output: 514
Script:
#!/bin/bash
# Let's examine the context of these sorting operations and their data sources
rg -B 5 -A 5 "sort\.SliceStable" account/accountdomain/workspace/member.go
# Check if there are any mutex/sync operations around these methods
rg -B 10 -A 2 "func.*UserIDs|func.*IntegrationIDs" account/accountdomain/workspace/member.go
# Look for any concurrent access patterns or mutex usage in the file
rg "sync\.|mutex|Lock|Unlock" account/accountdomain/workspace/member.go
Length of output: 1558
| @@ -2,6 +2,7 @@ package workspace | |||
|
|
|||
| import ( | |||
| "sort" | |||
| "sync" | |||
There was a problem hiding this comment.
Ensure consistent mutex usage across all map operations
While adding mutex protection is good, the current implementation only protects the Leave method. Other methods that access or modify the users and integrations maps should also be protected to ensure thread safety.
The following methods need mutex protection:
JoinAddIntegrationDeleteIntegrationUpdateUserRoleUpdateIntegrationRole- Any method that modifies the maps
Example pattern to follow:
func (m *Members) MethodName() {
m.mu.Lock()
defer m.mu.Unlock()
// map operations
}Also applies to: 33-33
| @@ -29,6 +30,7 @@ | |||
| users map[UserID]Member | |||
| integrations map[IntegrationID]Member | |||
| fixed bool | |||
| mu sync.Mutex | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using sync.RWMutex for better performance
Since the struct has both read and write operations, using sync.RWMutex instead of sync.Mutex would allow multiple concurrent reads while maintaining exclusive writes.
Apply this change:
- mu sync.Mutex
+ mu sync.RWMutexThen use RLock()/RUnlock() for read-only methods:
func (m *Members) Users() map[UserID]Member {
m.mu.RLock()
defer m.mu.RUnlock()
return maps.Clone(m.users)
}This pattern should be applied to all read-only methods:
Users()UserIDs()Integrations()IntegrationIDs()HasUser()HasIntegration()
A duplicate error occurs when the sort key is "id".
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Chores