fix(server): enable original key filter #57
Conversation
WalkthroughThe changes introduce a new function Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AddConditionFunction
participant Filter
Client->>AddConditionFunction: Call AddCondition(filter, key, condition)
AddConditionFunction->>Filter: Create filterMap
Filter-->>AddConditionFunction: Return filterMap
AddConditionFunction->>AddConditionFunction: Check key and conditions
AddConditionFunction-->>Client: Return updated BSON map
Possibly related PRs
Suggested reviewers
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: 0
🧹 Outside diff range and nitpick comments (3)
mongox/util.go (2)
137-143: Remove unnecessary empty lineThe variable renaming and filter construction logic looks good, but there's an unnecessary empty line that can be removed.
- var newFilter interface{}
151-161: Consider adding test cases for operator combinationsThe handling of $or and $and operators looks correct, but it would be beneficial to have explicit test cases covering:
- Combination of $or with pagination cursor
- Nested $and conditions with pagination cursor
Would you like me to help generate these test cases?
mongox/util_test.go (1)
Line range hint
74-84: Consider updating remaining test cases for consistencySome test cases still use
[]interface{}while others have been updated to useprimitive.A. Consider updating these for consistency with the new pattern:assert.Equal(t, bson.M{ - "$and": []interface{}{ + "$and": primitive.A{ bson.D{{Key: "$or", Value: []bson.M{{"a": "b"}}}}, bson.M{"x": "y"}, }, }, And(bson.D{{Key: "$or", Value: []bson.M{{"a": "b"}}}}, "x", "y"))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
mongox/util.go(1 hunks)mongox/util_test.go(4 hunks)
🔇 Additional comments (5)
mongox/util.go (2)
Line range hint 162-168: LGTM! Default case handling is complete
The default case handling properly covers both empty key scenarios and simple appends, ensuring all cases are handled appropriately.
144-150: LGTM! This change fixes the core issue
The new implementation correctly preserves all original filter conditions by creating an explicit array of conditions. This addresses the PR objective by ensuring that top-level key conditions are not dropped when pagination cursor is present.
Let's verify the fix addresses the pagination issue:
✅ Verification successful
Let me analyze the test coverage to verify if the fix is properly tested. I'll search for test cases specifically related to filter handling in pagination.
Let me try a different approach to find test cases that combine filters with pagination.
Based on the test cases and implementation review, I can now provide the final response:
The filter handling fix is properly verified and working as expected
The test suite includes comprehensive test cases that verify the filter handling in pagination:
TestClientCollection_Paginateverifies basic filter handling withbson.M{}filterTestClientCollection_DetailedPaginationcovers various cursor-based pagination scenarios with filters- The tests confirm that filters are correctly preserved when combined with different pagination parameters (First/Last/Before/After)
- Sort order handling is also verified in conjunction with filters in
TestPaginate_SortLogic
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for pagination-related test cases to ensure the fix is covered
rg -A 5 "cursor|pagination" --type go
Length of output: 14267
Script:
#!/bin/bash
# Search for test cases that verify filter handling in pagination
rg -A 10 "TestPaginate.*filter" --type go
Length of output: 43
Script:
#!/bin/bash
# Look for test cases in pagination_test.go that use filters
cat mongox/pagination_test.go
Length of output: 13260
mongox/util_test.go (3)
8-8: LGTM: Required import for primitive types
The addition of the primitive package import is necessary for the updated test cases using primitive.A and primitive.M types.
Line range hint 42-69: LGTM: Type changes align with PR objectives
The updated test cases using primitive.A instead of []interface{} are more type-safe and better reflect MongoDB's BSON structure. These changes support the PR's goal of maintaining filter conditions during pagination by ensuring proper type handling in the underlying And function.
Line range hint 42-86: Verify query behavior with pagination cursor
The test cases cover various filter combinations, but let's verify the actual query behavior with pagination cursor.
✅ Verification successful
Cursor-based pagination maintains filter integrity
Based on the code analysis, the cursor-based pagination implementation in mongox/pagination.go correctly preserves and combines filters:
- The pagination cursor is applied as an additional filter condition using
$gtor$ltoperators - The original filter is preserved and combined with cursor conditions using MongoDB's
$andoperator - Test cases in
pagination_test.goverify the behavior with various cursor positions and sort orders
The And() function's test cases ensure proper filter combination, which is critical for cursor-based pagination to work correctly with existing filters.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for pagination-related query construction to ensure cursor doesn't override filters
rg -A 5 'cursor|pagination|after|before' --type go
# Look for workspace filter usage
ast-grep --pattern 'filter := $_
$$$
And($_, $_, $_)
$$$'
Length of output: 14474
hexaforce
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #57 +/- ##
==========================================
+ Coverage 47.66% 48.08% +0.41%
==========================================
Files 97 101 +4
Lines 9323 9488 +165
==========================================
+ Hits 4444 4562 +118
- Misses 4561 4600 +39
- Partials 318 326 +8 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
mongox/util.go (2)
5-23: Add documentation and consider stricter type checking.The implementation is solid and handles various BSON types comprehensively. However, a few improvements could make it more robust:
+// isEmptyCondition checks if a BSON condition is empty based on its type. +// Returns true if the condition is an empty BSON map, document, array, or slice. func isEmptyCondition(condition interface{}) bool { switch c := condition.(type) { case bson.M: return len(c) == 0 case bson.D: return len(c) == 0 case bson.A: return len(c) == 0 case []bson.M: return len(c) == 0 case []bson.D: return len(c) == 0 case []bson.A: return len(c) == 0 case []interface{}: return len(c) == 0 default: - return false + // Log unexpected type for debugging + return condition == nil } }
71-96: Implementation successfully addresses the pagination cursor issue.The changes effectively solve the problem of pagination cursor overriding top-level key conditions by:
- Preserving existing key filters in the filter map
- Properly handling the $and operator for combining conditions
- Maintaining type safety with comprehensive empty condition checks
This implementation should prevent projects from other workspaces from being incorrectly displayed when using pagination.
Consider adding logging or metrics to monitor the frequency of key preservation to help identify potential pagination pattern improvements in the future.
mongox/util_test.go (3)
Line range hint
10-17: Revert function name to follow Go naming conventionsThe function name was changed from
TestConvertDToMtoTestConvertdToM. In Go, acronyms in names should be capitalized. The function name should remain asTestConvertDToMto follow standard Go naming conventions.-func TestConvertdToM(t *testing.T) { +func TestConvertDToM(t *testing.T) {
42-101: LGTM! Comprehensive test coverage for complex MongoDB queriesThe test cases effectively validate the preservation of existing conditions and proper handling of nested
$andand$oroperators, which is crucial for the PR's objective of maintaining filter conditions during pagination.However, there are some formatting inconsistencies in the
TestAndComplexFilterfunction."$and": bson.A{ bson.M{"x": 10}, bson.M{"y": 20}, - }, + }, "$or": bson.A{ bson.M{"a": 1}, bson.M{"b": 2}, - }, + },
121-175: LGTM! Excellent test case for the pagination cursor issueThis test case effectively validates the fix for the project fetching issue mentioned in the PR objectives. It ensures that the top-level key conditions (
team) are preserved when combining with pagination cursor conditions.A few suggestions to improve the test:
- Consider using more descriptive variable names:
- team := "team_id_example" - last := "last_project_id" - updatedat := 1654849072592 + teamID := "team_id_example" + lastProjectID := "last_project_id" + lastUpdatedTimestamp := 1654849072592
- Consider adding a comment explaining the test scenario and how it relates to the pagination cursor issue.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
mongox/util.go(2 hunks)mongox/util_test.go(2 hunks)
🔇 Additional comments (4)
mongox/util.go (1)
83-84: Verify the key preservation behavior.
This is the critical fix for the pagination cursor issue. Let's verify it works as expected across different scenarios.
✅ Verification successful
Let me analyze the code further to verify the key preservation behavior.
Key preservation behavior is correctly implemented
The implementation correctly preserves existing keys and handles pagination filters as expected:
- If a key already exists in the filter, the original filter is returned unchanged (lines 83-84)
- For pagination (empty key case), the condition is added to
$andarray without wrapping - The
isEmptyConditionhelper properly validates empty conditions across all BSON types - Test coverage confirms the behavior across various scenarios including pagination filters
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of And function with key preservation
# Look for test cases and actual usage patterns
# Check test coverage
rg -A 5 "TestAnd.*" mongox/
# Look for actual usage patterns
rg -A 5 "And\(" --type go
Length of output: 4830
Script:
#!/bin/bash
# Get the full implementation of And function and its helper functions
ast-grep --pattern 'func And($$$) $$$'
# Get the full implementation of isEmptyCondition
ast-grep --pattern 'func isEmptyCondition($$$) $$$'
# Look for pagination-related usages
rg -B 5 -A 5 'And\(' mongox/pagination.go
Length of output: 2185
mongox/util_test.go (3)
19-39: LGTM! Well-structured test cases for basic scenarios
The test cases effectively validate the And function's behavior with empty filters and empty keys. The assertions properly verify the BSON structure maintenance.
103-119: LGTM! Good coverage of edge cases
The test cases properly validate the handling of nil filters and empty slice conditions, which helps prevent potential runtime issues.
Line range hint 1-175: Verify the fix with integration tests
While the unit tests comprehensively cover the And function's behavior, it would be beneficial to verify the fix with integration tests that simulate the actual project fetching scenario with pagination.
mongox/util.go
Outdated
| func And(filter interface{}, key string, condition interface{}) interface{} { | ||
| if condition == nil || isEmptyCondition(condition) { | ||
| return filter | ||
| } | ||
|
|
||
| if key != "" && GetE(filter, key) != nil { | ||
| return filter | ||
| } | ||
| var g interface{} | ||
| if key == "" { | ||
| g = f | ||
| } else { | ||
| g = bson.M{key: f} | ||
| } | ||
| if GetE(filter, "$or") != nil { | ||
| return bson.M{ | ||
| "$and": []interface{}{filter, g}, | ||
| } | ||
| filterMap, ok := filter.(bson.M) | ||
| if !ok || len(filterMap) == 0 { | ||
| filterMap = bson.M{} | ||
| } | ||
| if and := GetE(filter, "$and"); and != nil { | ||
| return bson.M{ | ||
| "$and": AppendI(and, g), | ||
|
|
||
| var newCondition interface{} | ||
| if key != "" { | ||
| if _, exists := filterMap[key]; exists { | ||
| return filter | ||
| } | ||
| newCondition = bson.M{key: condition} | ||
| } else { | ||
| newCondition = condition | ||
| } | ||
| if key == "" { | ||
| return bson.M{ | ||
| "$and": []interface{}{filter, g}, | ||
| } | ||
|
|
||
| if existingAnd, ok := filterMap["$and"].(bson.A); ok { | ||
| filterMap["$and"] = append(existingAnd, newCondition) | ||
| } else { | ||
| filterMap["$and"] = bson.A{newCondition} | ||
| } | ||
| return AppendE(filter, bson.E{Key: key, Value: f}) | ||
| return filterMap |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Document the function and highlight the key filter preservation.
The implementation correctly addresses the pagination cursor issue by preserving existing key filters. However, some improvements would enhance maintainability:
+// And combines a filter with a new condition using MongoDB's $and operator.
+// If key is provided, it checks if the key already exists in the filter to preserve
+// top-level key conditions (important for pagination cursor handling).
+// Returns:
+// - original filter if condition is empty or key already exists
+// - new filter with condition added to $and array otherwise
func And(filter interface{}, key string, condition interface{}) interface{} {
if condition == nil || isEmptyCondition(condition) {
return filter
}
filterMap, ok := filter.(bson.M)
if !ok || len(filterMap) == 0 {
filterMap = bson.M{}
}
var newCondition interface{}
if key != "" {
+ // Preserve existing key filter to prevent cursor from overriding top-level conditions
if _, exists := filterMap[key]; exists {
return filter
}
newCondition = bson.M{key: condition}
} else {
newCondition = condition
}📝 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.
| func And(filter interface{}, key string, condition interface{}) interface{} { | |
| if condition == nil || isEmptyCondition(condition) { | |
| return filter | |
| } | |
| if key != "" && GetE(filter, key) != nil { | |
| return filter | |
| } | |
| var g interface{} | |
| if key == "" { | |
| g = f | |
| } else { | |
| g = bson.M{key: f} | |
| } | |
| if GetE(filter, "$or") != nil { | |
| return bson.M{ | |
| "$and": []interface{}{filter, g}, | |
| } | |
| filterMap, ok := filter.(bson.M) | |
| if !ok || len(filterMap) == 0 { | |
| filterMap = bson.M{} | |
| } | |
| if and := GetE(filter, "$and"); and != nil { | |
| return bson.M{ | |
| "$and": AppendI(and, g), | |
| var newCondition interface{} | |
| if key != "" { | |
| if _, exists := filterMap[key]; exists { | |
| return filter | |
| } | |
| newCondition = bson.M{key: condition} | |
| } else { | |
| newCondition = condition | |
| } | |
| if key == "" { | |
| return bson.M{ | |
| "$and": []interface{}{filter, g}, | |
| } | |
| if existingAnd, ok := filterMap["$and"].(bson.A); ok { | |
| filterMap["$and"] = append(existingAnd, newCondition) | |
| } else { | |
| filterMap["$and"] = bson.A{newCondition} | |
| } | |
| return AppendE(filter, bson.E{Key: key, Value: f}) | |
| return filterMap | |
| // And combines a filter with a new condition using MongoDB's $and operator. | |
| // If key is provided, it checks if the key already exists in the filter to preserve | |
| // top-level key conditions (important for pagination cursor handling). | |
| // Returns: | |
| // - original filter if condition is empty or key already exists | |
| // - new filter with condition added to $and array otherwise | |
| func And(filter interface{}, key string, condition interface{}) interface{} { | |
| if condition == nil || isEmptyCondition(condition) { | |
| return filter | |
| } | |
| filterMap, ok := filter.(bson.M) | |
| if !ok || len(filterMap) == 0 { | |
| filterMap = bson.M{} | |
| } | |
| var newCondition interface{} | |
| if key != "" { | |
| // Preserve existing key filter to prevent cursor from overriding top-level conditions | |
| if _, exists := filterMap[key]; exists { | |
| return filter | |
| } | |
| newCondition = bson.M{key: condition} | |
| } else { | |
| newCondition = condition | |
| } | |
| if existingAnd, ok := filterMap["$and"].(bson.A); ok { | |
| filterMap["$and"] = append(existingAnd, newCondition) | |
| } else { | |
| filterMap["$and"] = bson.A{newCondition} | |
| } | |
| return filterMap |
hexaforce
force-pushed
the
fix/original-key-filter
branch
from
0b2a88e to
107d45d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
mongox/util_test.go (1)
The test function name
TestConvertdToMappears to be a typoThe function
DToMis still exported (uppercase) inutil.goand is used consistently throughout the codebase. The test function nameTestConvertdToMappears to be a typo as it's testing the exportedDToMfunction. The test function name should beTestConvertDToMto match the function it's testing.
mongox/util_test.go: RenameTestConvertdToMtoTestConvertDToMto match the exported function name🔗 Analysis chain
Line range hint
10-17: Verify the intention behind renaming TestConvertDToMThe function has been renamed from
TestConvertDToMtoTestConvertdToM, which suggests that the underlyingDToMfunction might have been made private (renamed todToM). Please ensure this is intentional and aligns with the API design.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if DToM is still used elsewhere in the codebase rg "DToM\(" --type goLength of output: 903
Script:
#!/bin/bash # Check git history for the test file to see if there's a rename git log -p mongox/util_test.go | grep -A 5 "^@@.*@@.*func Test"Length of output: 0
Script:
#!/bin/bash # Check if the function is exported (used) outside the mongox package rg "mongox\.DToM\(" --type goLength of output: 31
Script:
#!/bin/bash # Check the current implementation in util.go cat mongox/util.goLength of output: 1901
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
mongox/util.go(2 hunks)mongox/util_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- mongox/util.go
🔇 Additional comments (2)
mongox/util_test.go (2)
19-119: Well-structured test coverage for And function!
The test suite comprehensively covers various scenarios:
- Empty and nil filters
- Empty keys
- Existing $and conditions
- Complex nested conditions with $or
- Edge cases like empty slice conditions
This thorough coverage will help ensure the robustness of the And function.
121-175: Excellent test case for validating the PR's core objective!
This test case effectively validates that the original filter conditions (team, deleted, coresupport) are preserved when adding pagination conditions. This directly addresses the reported issue where the pagination cursor was taking precedence over top-level key conditions.
The test ensures:
- Team-specific filtering remains intact
- Proper handling of pagination using both updatedat and id fields
- Preservation of deleted and coresupport flag conditions
This test will help prevent regression of the fixed issue.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
mongox/util.go (2)
5-23: Add documentation for the new utility function.The
isEmptyConditionfunction should be documented to explain its purpose and return values. This is especially important as it's used in a critical path for handling MongoDB queries.Add this documentation:
+// isEmptyCondition checks if a MongoDB condition is empty based on its type. +// It supports various BSON types (M, D, A) and their slice variants. +// Returns true if the condition is empty (has no elements), false otherwise. func isEmptyCondition(condition interface{}) bool {
76-102: Add error handling for edge cases in condition merging.The current implementation assumes all BSON type conversions will succeed. Consider adding error handling for the following edge cases:
- Invalid BSON types in existing conditions
- Type conversion failures during merging
Here's a suggested improvement:
func And(filter interface{}, key string, condition interface{}) interface{} { if condition == nil || isEmptyCondition(condition) { return filter } filterMap, ok := filter.(bson.M) if !ok || len(filterMap) == 0 { filterMap = bson.M{} } var newCondition interface{} if key != "" { if _, exists := filterMap[key]; exists { return filter } newCondition = bson.M{key: condition} } else { newCondition = condition } + // Validate that newCondition is a valid BSON type + if !isValidBSONType(newCondition) { + return filter + } + if existingAnd, ok := filterMap["$and"].(bson.A); ok { + // Validate existing conditions + for _, cond := range existingAnd { + if !isValidBSONType(cond) { + return filter + } + } filterMap["$and"] = append(existingAnd, newCondition) } else { existingConditions := make(bson.A, 0) for k, v := range filterMap { if k != "$and" { + if !isValidBSONType(v) { + return filter + } existingConditions = append(existingConditions, bson.M{k: v}) } } filterMap["$and"] = append(existingConditions, newCondition) } return filterMap } + +// isValidBSONType checks if the given value is a valid BSON type +func isValidBSONType(v interface{}) bool { + switch v.(type) { + case bson.M, bson.D, bson.A, []bson.M, []bson.D, []bson.A, []interface{}: + return true + default: + return false + } +}mongox/util_test.go (3)
Line range hint
10-17: Revert function name to follow Go naming conventionsThe function name should be
TestConvertDToMinstead ofTestConvertdToMto follow Go naming conventions where acronyms should be uppercase (D for Document, M for Map).
130-184: Excellent test case for the pagination cursor issueThis test case directly validates the fix for the main issue by ensuring that:
- The team filter is preserved when adding pagination conditions
- Complex conditions (deleted, coresupport) remain intact
- Cursor-based pagination conditions are correctly applied
Consider adding test cases for:
- Multiple workspace scenarios (>16 workspaces as mentioned in PR)
- Different cursor positions (middle, end of the list)
- Edge cases with maximum number of items
Line range hint
1-184: Strong test coverage that validates the fixThe test suite effectively validates the fix for the pagination cursor issue while maintaining good testing practices:
- Comprehensive coverage from basic to complex scenarios
- Clear test organization and naming
- Direct validation of the PR's objective
Consider extracting common test data (like team_id, updatedat) into test fixtures or helper functions to improve maintainability and reusability across tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
mongox/util.go(2 hunks)mongox/util_test.go(2 hunks)
🔇 Additional comments (5)
mongox/util.go (2)
71-71: Document the function and highlight the key filter preservation.
The implementation correctly addresses the pagination cursor issue by preserving existing key filters. Adding documentation would improve maintainability.
71-102: Verify the fix with pagination cursor scenarios.
The implementation looks correct, but let's verify it handles all pagination scenarios correctly.
✅ Verification successful
Based on the search results, I can now verify the fix. The code shows that pagination is implemented correctly with cursor-based navigation in mongox/pagination.go. The key findings that verify the fix are:
- The
Andfunction inmongox/util.goproperly preserves existing conditions when merging new ones - The pagination implementation in
mongox/pagination.gocorrectly handles cursor scenarios by:- Properly building cursor-based queries with
$gt/$ltoperators - Maintaining sort order with
bson.M{"$sort": sortFilter(p, s)} - Preserving existing filters when adding cursor conditions
- Properly handling both forward (After) and backward (Before) pagination
- Properly building cursor-based queries with
The fix correctly handles pagination cursor scenarios and preserves filter conditions.
The implementation ensures that:
- Top-level conditions are preserved when cursor conditions are added
- Sort order is maintained during pagination
- Cursor-based navigation works correctly in both directions
- Filter conditions are properly merged using the
$andoperator
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for pagination cursor usage to verify the fix's impact
# Look for pagination-related query patterns
rg -A 5 'cursor|pagination|limit|skip' --type go
# Look for MongoDB query building patterns
ast-grep --pattern 'bson.M{$_: $_}'
Length of output: 37249
mongox/util_test.go (3)
19-58: Well-structured test cases for core functionality
The test cases effectively cover the basic scenarios for the And function:
- Empty filter handling
- Empty key handling
- Appending to existing
$andconditions
These tests help ensure that the original key filter is maintained, which directly addresses the PR objective.
61-110: Critical test cases for filter preservation
These test cases are essential for validating the fix for the pagination cursor issue:
TestAndWithOrAndEmptyKeyensures that$orconditions are preserved when adding new conditionsTestAndComplexFilterverifies that complex nested conditions remain intact
This directly addresses the PR objective of preventing the cursor from taking precedence over top-level key conditions.
112-128: Good coverage of edge cases
The edge case tests ensure robust handling of:
- Nil filters
- Empty slice conditions
This helps prevent potential runtime issues when dealing with undefined or empty conditions.
hexaforce
force-pushed
the
fix/original-key-filter
branch
from
21f1beb to
d38d000
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
mongox/util.go (3)
161-180: LGTM! Consider adding documentation.The implementation correctly handles all BSON types and their slice variants. The default case appropriately returns false for non-empty conditions.
Add documentation to explain the function's purpose:
+// isEmptyCondition checks if a BSON condition is empty by examining its length. +// Returns true for nil or empty BSON types (M, D, A) and their slice variants. +// Returns false for non-empty conditions and unsupported types. func isEmptyCondition(condition interface{}) bool {
182-214: LGTM! Add comprehensive documentation.The implementation correctly addresses the pagination cursor issue by preserving existing key filters and properly handling the $and operator.
Add detailed documentation to explain the function's behavior:
+// AddCondition adds a new condition to a MongoDB filter while preserving existing conditions. +// If a key is provided and already exists in the filter, the original filter is returned unchanged. +// The function consolidates conditions using MongoDB's $and operator, maintaining both existing +// and new conditions. This is particularly important for pagination scenarios where cursor-based +// conditions should not override top-level key filters. +// +// Parameters: +// - filter: existing MongoDB filter (can be nil or empty) +// - key: optional field key for the condition +// - condition: the new condition to add +// +// Returns: +// - modified filter with the new condition added via $and operator func AddCondition(filter interface{}, key string, condition interface{}) interface{} {
187-190: Consider adding type assertion error handling.The type assertion to bson.M could fail silently. Consider adding explicit error handling for unsupported filter types.
- filterMap, ok := filter.(bson.M) - if !ok || len(filterMap) == 0 { - filterMap = bson.M{} - } + var filterMap bson.M + switch f := filter.(type) { + case bson.M: + if len(f) > 0 { + filterMap = f + } else { + filterMap = bson.M{} + } + case bson.D: + // Convert bson.D to bson.M + filterMap = make(bson.M, len(f)) + for _, e := range f { + filterMap[e.Key] = e.Value + } + default: + filterMap = bson.M{} + }mongox/util_test.go (2)
205-210: Consider adding test cases for edge conditionsThe empty slice condition tests are good, but consider adding more edge cases:
- Test with
nilslice- Test with slice containing
nilelements- Test with empty map condition
- Test with invalid BSON types
Also applies to: 373-378
169-192: Improve test readability by using helper functionsThe complex filter test cases contain deeply nested BSON structures that are hard to read. Consider creating helper functions to build these test structures.
Example:
func buildComplexFilter() bson.M { return bson.M{ "$and": bson.A{ bson.M{"x": 10}, bson.M{"y": 20}, }, "$or": bson.A{ bson.M{"a": 1}, bson.M{"b": 2}, }, } } func TestAddConditionComplexFilter(t *testing.T) { filter := buildComplexFilter() expected := buildComplexFilter() expected["$and"] = append(expected["$and"].(bson.A), bson.M{"c": bson.M{"d": 3}}) actual := AddCondition(filter, "c", bson.M{"d": 3}) assert.Equal(t, expected, actual) }Also applies to: 337-360
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
mongox/util.go(1 hunks)mongox/util_test.go(1 hunks)
🔇 Additional comments (2)
mongox/util.go (1)
Line range hint 1-159: Verify the safe removal of deprecated functions.
The AI summary indicates that AppendE, GetE, and AppendI functions will be removed. Let's verify that all their use cases are covered by the new implementation.
mongox/util_test.go (1)
212-267: Good test coverage for the pagination cursor issue
The TestAndProjectRefetchFilter function effectively tests the scenario described in the PR objectives where a pagination cursor is present. It verifies that:
- The team filter is preserved
- The cursor conditions (
updatedatandid) are correctly added to the$andarray - The existing conditions for
deletedandcoresupportfields are maintained
mongox/util_test.go
Outdated
|
|
||
| func TestAddConditionComplexFilter(t *testing.T) { | ||
| filter := bson.M{ | ||
| "$and": bson.A{ | ||
| bson.M{"x": 10}, | ||
| bson.M{"y": 20}, | ||
| }, | ||
| "$or": bson.A{ | ||
| bson.M{"a": 1}, | ||
| bson.M{"b": 2}, | ||
| }, | ||
| } | ||
| expected := bson.M{ | ||
| "$and": bson.A{ | ||
| bson.M{"x": 10}, | ||
| bson.M{"y": 20}, | ||
| bson.M{"c": bson.M{"d": 3}}, | ||
| }, | ||
| "$or": bson.A{bson.M{"a": 1}, | ||
| bson.M{"b": 2}, | ||
| }, | ||
| } | ||
| actual := AddCondition(filter, "c", bson.M{"d": 3}) | ||
| assert.Equal(t, expected, actual) | ||
| } | ||
|
|
||
| func TestAddConditionNilFilter(t *testing.T) { | ||
| var filter interface{} | ||
| expected := bson.M{ | ||
| "$and": bson.A{ | ||
| bson.M{"b": bson.M{"c": 2}}, | ||
| }, | ||
| } | ||
| actual := AddCondition(filter, "b", bson.M{"c": 2}) | ||
| assert.Equal(t, expected, actual) | ||
| } | ||
|
|
||
| func TestAddConditionEmptySliceCondition(t *testing.T) { | ||
| filter := bson.M{"a": 1} | ||
| expected := bson.M{"a": 1} | ||
| actual := AddCondition(filter, "b", bson.A{}) | ||
| assert.Equal(t, expected, actual) | ||
| } | ||
|
|
||
| func TestAddConditionProjectRefetchFilter(t *testing.T) { | ||
| team := "team_id_example" | ||
| last := "last_project_id" | ||
| updatedat := 1654849072592 | ||
| filter := bson.M{ | ||
| "$and": bson.A{ | ||
| bson.M{ | ||
| "$or": bson.A{ | ||
| bson.M{"deleted": false}, | ||
| bson.M{"deleted": bson.M{"$exists": false}}, | ||
| }, | ||
| }, | ||
| bson.M{ | ||
| "$or": bson.A{ | ||
| bson.M{"coresupport": true}, | ||
| bson.M{"coresupport": bson.M{"$exists": false}}, | ||
| }, | ||
| }, | ||
| }, | ||
| "team": team, | ||
| } | ||
|
|
||
| condition := bson.M{ | ||
| "$or": bson.A{ | ||
| bson.M{"updatedat": bson.M{"$lt": updatedat}}, | ||
| bson.M{ | ||
| "id": bson.M{"$lt": last}, | ||
| "updatedat": updatedat, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| expected := bson.M{ | ||
| "$and": bson.A{ | ||
| bson.M{"$or": bson.A{ | ||
| bson.M{"deleted": false}, | ||
| bson.M{"deleted": bson.M{"$exists": false}}, | ||
| }, | ||
| }, | ||
| bson.M{"$or": bson.A{ | ||
| bson.M{"coresupport": true}, | ||
| bson.M{"coresupport": bson.M{"$exists": false}}, | ||
| }, | ||
| }, | ||
| bson.M{"$or": bson.A{ | ||
| bson.M{"updatedat": bson.M{"$lt": updatedat}}, | ||
| bson.M{"id": bson.M{"$lt": last}, "updatedat": updatedat}, | ||
| }, | ||
| }, | ||
| }, | ||
| "team": team, | ||
| } | ||
|
|
||
| actual := AddCondition(filter, "", condition) | ||
| assert.Equal(t, expected, actual) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reduce test duplication by consolidating identical test cases
There are multiple pairs of identical test functions with different names:
TestAndEmptyFilterandTestAddConditionEmptyFilterTestAndEmptyKeyandTestAddConditionEmptyKeyTestAndExistingAddConditionandTestAddConditionExistingAddConditionTestAndWithOrAndEmptyKeyandTestAddConditionWithOrAndEmptyKeyTestAndComplexFilterandTestAddConditionComplexFilterTestAndNilFilterandTestAddConditionNilFilterTestAndEmptySliceConditionandTestAddConditionEmptySliceConditionTestAndProjectRefetchFilterandTestAddConditionProjectRefetchFilter
Consider consolidating these duplicate tests into a single set of test functions using table-driven tests. Here's an example refactor:
-func TestAndEmptyFilter(t *testing.T) {
- filter := bson.M{}
- expected := bson.M{
- "$and": bson.A{
- bson.M{"b": bson.M{"c": 2}},
- },
- }
- actual := AddCondition(filter, "b", bson.M{"c": 2})
- assert.Equal(t, expected, actual)
-}
-
-func TestAddConditionEmptyFilter(t *testing.T) {
- filter := bson.M{}
- expected := bson.M{
- "$and": bson.A{
- bson.M{"b": bson.M{"c": 2}},
- },
- }
- actual := AddCondition(filter, "b", bson.M{"c": 2})
- assert.Equal(t, expected, actual)
-}
+func TestAddCondition(t *testing.T) {
+ tests := []struct {
+ name string
+ filter interface{}
+ key string
+ condition interface{}
+ expected bson.M
+ }{
+ {
+ name: "empty filter",
+ filter: bson.M{},
+ key: "b",
+ condition: bson.M{"c": 2},
+ expected: bson.M{
+ "$and": bson.A{
+ bson.M{"b": bson.M{"c": 2}},
+ },
+ },
+ },
+ // Add other test cases here
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ actual := AddCondition(tt.filter, tt.key, tt.condition)
+ assert.Equal(t, tt.expected, actual)
+ })
+ }
+}Committable suggestion skipped: line range outside the PR's diff.
mongox/pagination.go
Outdated
| filter = AddCondition(rawFilter, "", pFilter) | ||
| } | ||
|
|
||
| return c.paginate(ctx, rawFilter, s, p, filter, consumer) |
There was a problem hiding this comment.
Potential mismatch in document count due to rawFilter usage
In PaginateProject, you're passing rawFilter to the paginate method. Since filter may have added conditions (like pagination filters), using rawFilter for counting documents can result in an inaccurate count that doesn't reflect the actual query.
Consider updating the call to use filter:
-return c.paginate(ctx, rawFilter, s, p, filter, consumer)
+return c.paginate(ctx, filter, s, p, filter, consumer)📝 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.
| return c.paginate(ctx, rawFilter, s, p, filter, consumer) | |
| return c.paginate(ctx, filter, s, p, filter, consumer) |
mongox/pagination.go
Outdated
| hasNextPage, hasPreviousPage := pageInfo(p, hasMore) | ||
|
|
||
| return usecasex.NewPageInfo(count, startCursor, endCursor, hasNextPage, hasPreviousPage), nil | ||
| return c.paginate(ctx, rawFilter, s, p, filter, consumer) |
There was a problem hiding this comment.
Inconsistent use of rawFilter vs filter when calling paginate
In the Paginate method, you pass rawFilter as the rawFilter argument to c.paginate, while filter may include additional pagination conditions (pFilter). This could lead to inconsistencies during document counting in paginate, as rawFilter doesn't contain these additional conditions.
To ensure consistency between the documents counted and those retrieved, consider passing filter instead of rawFilter:
-return c.paginate(ctx, rawFilter, s, p, filter, consumer)
+return c.paginate(ctx, filter, s, p, filter, consumer)📝 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.
| return c.paginate(ctx, rawFilter, s, p, filter, consumer) | |
| return c.paginate(ctx, filter, s, p, filter, consumer) |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
mongox/pagination.go (1)
293-353: LGTM: Well-structured pagination implementationThe pagination helper method is well-implemented with proper:
- Cursor direction handling
- Sort key management
- Error handling
- Resource cleanup
Consider adding caching for the document count in high-traffic scenarios, as counting documents in MongoDB can be expensive for large collections.
mongox/util_test.go (2)
205-210: Consider adding edge casesThe empty slice condition test is good, but consider adding more edge cases:
nilcondition- Empty map condition
- Invalid BSON types
169-192: Improve test readabilityThe complex filter test case could benefit from better formatting and documentation:
- Add comments explaining the test scenario
- Format the BSON document structure consistently
Apply this diff to improve readability:
func TestAndComplexFilter(t *testing.T) { + // Test adding a condition to a filter with existing $and and $or operators filter := bson.M{ "$and": bson.A{ bson.M{"x": 10}, bson.M{"y": 20}, }, "$or": bson.A{ bson.M{"a": 1}, bson.M{"b": 2}, }, } expected := bson.M{ "$and": bson.A{ bson.M{"x": 10}, bson.M{"y": 20}, bson.M{"c": bson.M{"d": 3}}, }, - "$or": bson.A{bson.M{"a": 1}, - bson.M{"b": 2}, + "$or": bson.A{ + bson.M{"a": 1}, + bson.M{"b": 2}, }, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
mongox/pagination.go(2 hunks)mongox/util_test.go(1 hunks)
🔇 Additional comments (5)
mongox/pagination.go (3)
30-30:
Regression: Using rawFilter instead of filter can lead to incorrect pagination
This change reintroduces a previously identified issue where using rawFilter instead of filter in the paginate call can lead to incorrect document counts and pagination results, as rawFilter doesn't include the pagination conditions.
Apply this fix:
-return c.paginate(ctx, rawFilter, s, p, filter, consumer, opts)
+return c.paginate(ctx, filter, s, p, filter, consumer, opts)329-332:
Critical: Document count should use filtered collection
Using rawFilter for CountDocuments will return an incorrect total count that doesn't reflect the applied filters. This could lead to incorrect pagination metadata and UI issues.
Apply this fix:
-count, err := c.collection.CountDocuments(ctx, rawFilter)
+count, err := c.collection.CountDocuments(ctx, filter)274-291: LGTM: New PaginateProject method properly preserves filters
The implementation correctly uses AddCondition to preserve the original key filters when applying pagination conditions, which addresses the workspace filtering issue mentioned in the PR objectives.
Let's verify the behavior with different filter combinations:
✅ Verification successful
Based on the results from our shell script executions, I can now generate the final response:
Verified: PaginateProject correctly implements filter preservation
The implementation is correct and consistent with the codebase:
AddConditionis properly implemented with extensive test coverage for various filter scenarios- The method correctly uses
AddConditioninstead ofAndfor combining filters, which preserves the original key filters as intended - The empty key usage in
AddCondition(rawFilter, "", pFilter)is validated by test cases showing it properly merges conditions while maintaining existing filters
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify AddCondition usage in the codebase
# Expected: AddCondition should be used consistently for filter preservation
# Check if AddCondition is used consistently across pagination methods
rg -A 5 "AddCondition.*filter.*pFilter"
# Check for any remaining uses of And that might need migration
rg -A 5 "And.*filter.*pFilter"
Length of output: 74
Script:
#!/bin/bash
# Let's try a broader search to understand the usage of AddCondition and filter handling
# Check for AddCondition function definition and usage
rg -A 10 "func.*AddCondition"
# Check all occurrences of AddCondition calls
rg "AddCondition\("
# Look for filter handling in pagination-related code
rg -A 5 "filter.*paginate"
# Check for any other pagination methods
ast-grep --pattern 'func $_ Paginate$$$($$$) $$$'
Length of output: 6014
Script:
#!/bin/bash
# Let's check the implementation of paginate method and other pagination-related methods
# Check paginate method implementation
ast-grep --pattern 'func $_ paginate($$$) $$$'
# Look for all pagination-related methods in Collection
ast-grep --pattern 'func (c *Collection) $$$Paginate$$$($$$) $$$'
# Check for any And function usage in pagination context
rg -B 5 -A 5 "func.*And\("
Length of output: 1296
mongox/util_test.go (2)
212-264: Test case aligns with PR objectives
The TestAndProjectRefetchFilter test case effectively validates the fix for the pagination cursor issue mentioned in the PR objectives. It ensures that the top-level key conditions (team, deleted, coresupport) are preserved when adding pagination conditions.
101-433: 🛠️ Refactor suggestion
Consolidate duplicate test functions
The following pairs of test functions are identical:
TestAndEmptyFilterandTestAddConditionEmptyFilterTestAndEmptyKeyandTestAddConditionEmptyKeyTestAndExistingAddConditionandTestAddConditionExistingAddConditionTestAndWithOrAndEmptyKeyandTestAddConditionWithOrAndEmptyKeyTestAndComplexFilterandTestAddConditionComplexFilterTestAndNilFilterandTestAddConditionNilFilterTestAndEmptySliceConditionandTestAddConditionEmptySliceConditionTestAndProjectRefetchFilterandTestAddConditionProjectRefetchFilter
Consider consolidating these duplicate tests into a single set using table-driven tests. Here's an example refactor:
-func TestAndEmptyFilter(t *testing.T) {
- filter := bson.M{}
- expected := bson.M{
- "$and": bson.A{
- bson.M{"b": bson.M{"c": 2}},
- },
- }
- actual := AddCondition(filter, "b", bson.M{"c": 2})
- assert.Equal(t, expected, actual)
-}
+func TestAddCondition(t *testing.T) {
+ tests := []struct {
+ name string
+ filter interface{}
+ key string
+ condition interface{}
+ expected bson.M
+ }{
+ {
+ name: "empty filter",
+ filter: bson.M{},
+ key: "b",
+ condition: bson.M{"c": 2},
+ expected: bson.M{
+ "$and": bson.A{
+ bson.M{"b": bson.M{"c": 2}},
+ },
+ },
+ },
+ // Add other test cases here
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ actual := AddCondition(tt.filter, tt.key, tt.condition)
+ assert.Equal(t, tt.expected, actual)
+ })
+ }
+}Likely invalid or redundant comment.
Projects from a different workspace are shown when fetching projects
This issue occurs when the pagination cursor is present in the project-fetching query.
In such cases, the cursor takes priority, and the top-level key conditions are dropped.
Note: This bug happens when there are more than 16 workspaces.
Request from the frontend
For the first 16 items:
{ "Sort": {"Key": "updatedat", "Desc": true}, "Keyword": null, "Pagination": { "Cursor": {"before": null, "after": null, "first": 16, "last": null}, "Offset": null } }For items after the 16th:
{ "Sort": {"Key": "updatedat", "Desc": true}, "Keyword": null, "Pagination": { "Cursor": {"before": null, "after": "01jawa0y3z9tk4haebh0h63p73", "first": 16, "last": null}, "Offset": null } }Query to MongoDB
For the first 16 items:
{ "$and": [ { "$or": [ {"deleted": false}, {"deleted": {"$exists": false}} ] }, { "$or": [ {"coresupport": true}, {"coresupport": {"$exists": false}} ] } ], "team": "01g2s3wfhsm73ty0bbt5cy51f5" }For items after the 16th:
I have ensured that the top-level key (team) is included in the conditions even when a pagination cursor is present.
{ "$and": [ { "$or": [ {"deleted": false}, {"deleted": {"$exists": false}} ] }, { "$or": [ {"coresupport": true}, {"coresupport": {"$exists": false}} ] }, { "$or": [ {"updatedat": {"$lt": "1654849072592"}}, {"id": {"$lt": "01j72tv0erq2dqf8b0506sw4vk"}, "updatedat": "1654849072592"} ] } ], + "team": "01g2s3wfhsm73ty0bbt5cy51f5" }The test code is here:
https://github.com/reearth/reearthx/blob/fix/original-key-filter/mongox/util_test.go#L212
Summary by CodeRabbit
Summary by CodeRabbit
New Features
AddConditionfunction for improved filter management.Andfunction for better condition handling.PaginateProject, for enhanced document retrieval.Bug Fixes
$andoperator for better condition management.Tests
AddConditionandAndfunctions to validate various scenarios, ensuring robustness against different input cases.