fix(server): fix filter bug #1276
Conversation
WalkthroughThe changes involve the Changes
Possibly related PRs
Suggested reviewers
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
Documentation and Community
|
✅ Deploy Preview for reearth-web canceled.
|
![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 (1)
server/internal/infrastructure/mongo/asset.go (1)
73-74: Consider adding a migration planIf this is a breaking change in filtering behavior, existing assets without the
coresupportfield might need to be updated.Consider:
- Adding a database migration to set a default value for existing records
- Documenting the change in behavior for other developers
- Implementing a gradual rollout strategy if this affects many assets
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
server/internal/infrastructure/mongo/asset.go(1 hunks)
🔇 Additional comments (1)
server/internal/infrastructure/mongo/asset.go (1)
73-74:
Verify the impact of this filtering change
The modification from an $or condition to a direct equality check significantly changes the filtering behavior:
- Previously: matched if
coresupport=trueOR if the field was missing - Now: matches only if
coresupport=true
This could potentially exclude assets that previously matched the filter if they don't have the coresupport field set.
Let's verify the impact:
Please clarify:
- Is this change intentional? The PR description mentions fixing a filter bug but doesn't specify the expected behavior.
- Are there any assets in the database without the
coresupportfield that should still be included in the results? - Has this change been tested with existing data to ensure it doesn't break any functionality?
✅ Verification successful
Change in coresupport filtering is consistent with project implementation
The change in filtering behavior is intentional and aligns with the project repository implementation. The test data shows that:
- All test cases explicitly set the
coresupportfield (either true or false) - The project repository also uses a direct
coresupport: truefilter in its implementation - The CHANGELOG indicates this was an intentional feature addition (PR feat(server): add coreSupport flag to project #424)
The removal of the $or condition with $exists check is correct as the codebase consistently treats this as a required boolean field, not an optional one.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for assets that might be affected by this change
# Description: Look for any tests or code that relies on the previous behavior
# Check for test files that might need updates
rg -l "coresupport" --type go --glob "*_test.go"
# Look for any direct MongoDB queries in tests that might be affected
rg -A 5 '\$or.*coresupport' --type go
# Check for any documentation or comments about coresupport behavior
rg -i "coresupport" --type md
Length of output: 355
Script:
#!/bin/bash
# Let's check the test file and surrounding context for coresupport usage
rg -B 5 -A 5 "coresupport" server/internal/infrastructure/mongo/project_test.go
# Check the implementation of FindByWorkspace in project.go for comparison
ast-grep --pattern 'func (r *ProjectRepo) FindByWorkspace($$$) {
$$$
}'
# Look for any other occurrences of coresupport in the codebase
rg -B 2 -A 2 "coresupport" --type go
Length of output: 5198
| "team": id.String(), | ||
| "coresupport": true, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding test coverage for the filter behavior
The filter logic is critical for asset queries, but I don't see any tests being updated along with this change.
Consider adding test cases to verify:
- Assets with
coresupport=trueare included - Assets with
coresupport=falseare excluded - Assets without the
coresupportfield are excluded (new behavior)
Here's a suggested test structure:
func TestAsset_FindByWorkspace_CoreSupport(t *testing.T) {
tests := []struct {
name string
assets []mongodoc.Asset
expected int
}{
{
name: "should return only assets with coresupport=true",
assets: []mongodoc.Asset{
{CoreSupport: true},
{CoreSupport: false},
{}, // missing coresupport field
},
expected: 1,
},
}
// ... implement test logic
}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
server/e2e/gql_asset_test.go (3)
4-15: Remove debug-related importsThe
fmtpackage is only used for debug printing in the test. Consider removing both the import and the associated debug print statements for cleaner test code.
55-68: Enhance error assertions with descriptive messagesWhile error checking is present, consider adding descriptive messages to make test failures more informative:
-assert.Nil(t, err) +assert.NoError(t, err, "failed to build asset") -assert.Nil(t, err) +assert.NoError(t, err, "failed to save asset")
76-78: Remove debug print statementsRemove the debug print loop as it's not providing any test value:
-for _, a := range as { - fmt.Printf("===== %v\n", a) -}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
server/e2e/gql_asset_test.go(2 hunks)
🔇 Additional comments (2)
server/e2e/gql_asset_test.go (2)
Line range hint 82-183: GraphQL operations look good!
The GraphQL operations are well-structured and properly include the coreSupport field in both mutations and queries.
Line range hint 81-183: Verify test coverage for the filter bug fix
While the test verifies that returned assets have coreSupport=true, let's ensure the test coverage is comprehensive:
✅ Verification successful
Test coverage for coreSupport filtering is comprehensive
The test file demonstrates thorough coverage of the coreSupport filtering functionality:
- Creates assets with both true and false coreSupport values
- Verifies the creation response contains correct coreSupport values
- Tests the filtering by asserting that only assets with coreSupport=true are returned
- Includes both file types (png, csv) in the test scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage for asset filtering logic
# Look for other test files that might need similar updates
fd -e go -e test.go . | xargs rg -l "coreSupport"
# Check for any TODOs or FIXMEs related to asset filtering
rg -i "todo|fixme|hack" -g "**/*asset*.go"
# Look for similar filter implementations that might need the same fix
ast-grep --pattern 'FindByWorkspace($$$) {
$$$
}'
Length of output: 939
Script:
#!/bin/bash
# Let's check the actual implementation of asset filtering logic and its tests
# Check asset filtering implementation
ast-grep --pattern 'func ($$$) Filter($$$) {
$$$
}'
# Look for test cases related to asset filtering
rg -A 5 -B 5 "coreSupport.*filter|filter.*coreSupport" server/pkg/asset/
# Check GraphQL resolver implementation for assets
rg -A 10 "type AssetResolver" server/internal/adapter/gql/
Length of output: 1089
Script:
#!/bin/bash
# Let's examine the test file more closely to understand the coverage
# Check the full test file content to understand test scenarios
cat server/e2e/gql_asset_test.go
# Look for asset query implementation
ast-grep --pattern 'func ($$$) Assets($$$) {
$$$
}'
# Check for asset filtering in repository layer
rg -A 10 "type AssetRepository" server/pkg/asset/
Length of output: 4919
| f := int64(20) | ||
| as, _, err := r.Asset.FindByWorkspace(ctx, wID, repo.AssetFilter{ | ||
| Pagination: usecasex.CursorPagination{ | ||
| First: &f, | ||
| }.Wrap(), | ||
| }) | ||
| assert.Nil(t, err) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage for the filter bug fix
Given that this PR fixes a filter bug related to coreSupport, consider:
- Adding test cases that explicitly verify the fixed filtering behavior
- Testing edge cases with different coreSupport values
- Making the pagination limit configurable for better test flexibility
-f := int64(20)
+const defaultPageSize = int64(20)
+testCases := []struct {
+ name string
+ filter repo.AssetFilter
+ expectedCoreSupport bool
+ expectedCount int
+}{
+ {
+ name: "only core support assets",
+ filter: repo.AssetFilter{
+ Pagination: usecasex.CursorPagination{
+ First: &defaultPageSize,
+ }.Wrap(),
+ },
+ expectedCoreSupport: true,
+ expectedCount: 3, // adjust based on your test data
+ },
+ // Add more test cases
+}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
server/e2e/gql_asset_test.go (2)
80-80: Consider using a named constant for pagination limitThe magic number
20should be defined as a constant for better maintainability.-f := int64(20) +const defaultPageSize = int64(20) +f := defaultPageSize
Line range hint
89-93: Consider enhancing GraphQL response validationWhile the test verifies that returned assets have
coreSupport=true, it could be more comprehensive:
- Verify the total count matches expected filtered results
- Verify other asset properties remain intact
- Add negative test cases to ensure assets with
coreSupport=falseare not returned
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
server/e2e/gql_asset_test.go(2 hunks)
🔇 Additional comments (4)
server/e2e/gql_asset_test.go (4)
4-4: LGTM: Import changes align with new functionality
The added imports support the new test functionality for direct DB operations and pagination.
Also applies to: 11-14
26-26: LGTM: Enhanced test setup with repository access
The change from StartServer to StartServerAndRepos appropriately provides repository access needed for direct DB operations.
54-88: 🛠️ Refactor suggestion
Enhance test coverage for filter bug fix
The test could be more explicit about verifying the filter bug fix. Consider:
- Adding test cases that explicitly verify different coreSupport scenarios
- Testing edge cases (null, undefined, false values)
- Adding descriptive comments about what each test case verifies
The previous review comment about enhancing test coverage is still relevant. Consider implementing the suggested test structure to make the test cases more explicit and maintainable.
81-85: Verify the filter implementation
Let's verify the actual filter implementation that was changed to fix the bug.
✅ Verification successful
Let me generate additional verification scripts to find the implementation of FindByWorkspace and understand the filter implementation.
Let me search for the actual implementation of the Asset repository to understand how the filter is applied.
Let me try one more search to find the asset domain model and filter implementation with corrected paths.
Based on the code analysis, I can now provide a final response about the filter implementation.
Filter implementation is working as expected
The FindByWorkspace implementation in the mongo Asset repository correctly applies the coreSupport filter:
var filter any = bson.M{
"team": id.String(),
"coresupport": true,
}The test code in server/e2e/gql_asset_test.go is only using pagination filter, which is correctly passed to the repository layer. The coreSupport field is properly defined in the Asset model (server/pkg/asset/asset.go) and is being filtered at the database level.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find the implementation of FindByWorkspace method to verify the filter fix
# Search for the FindByWorkspace implementation
ast-grep --pattern 'func (*_) FindByWorkspace(ctx context.Context, $_, $_) ($_, $_, $_) {
$$$
}'
# Search for any filter conditions related to coreSupport
rg -A 5 'coreSupport'
Length of output: 117189
Script:
#!/bin/bash
# Search for FindByWorkspace implementation in repository files
ast-grep --pattern 'func (*_) FindByWorkspace(ctx context.Context, $_, $filter) ($_, $_, $_) {
$$$
}'
# Search for AssetFilter struct definition and usage
rg -A 10 "type AssetFilter struct"
# Search for any filter-related code in asset repository implementation
rg -A 5 "FindByWorkspace.*Filter"
Length of output: 5812
Script:
#!/bin/bash
# Search for the Asset repository implementation in mongo package
rg -A 20 "type Asset struct" server/internal/infrastructure/mongo/
# Search for the actual filter implementation in the mongo Asset repository
ast-grep --pattern 'func (*Asset) FindByWorkspace($ctx context.Context, $id accountdomain.WorkspaceID, $filter repo.AssetFilter) ($_, $_, $_) {
$$$
}'
# Look for any Asset model definition to understand the fields
rg -A 10 "type Asset struct" server/internal/domain/
Length of output: 1834
Script:
#!/bin/bash
# Search for Asset model definition in all possible domain locations
rg -A 10 "type Asset struct" server/pkg/
rg -A 10 "type Asset struct" server/internal/
# Search for the mongo implementation of FindByWorkspace with correct path
rg -A 20 "func \(r \*Asset\) FindByWorkspace" server/internal/infrastructure/mongo/asset.go
# Search for any filter-related code in the asset package
rg -A 5 "Filter" server/internal/infrastructure/mongo/asset.go
Length of output: 6537
| f := int64(20) | ||
| as, _, err := r.Asset.FindByWorkspace(ctx, wID, repo.AssetFilter{ | ||
| Pagination: usecasex.CursorPagination{ | ||
| First: &f, | ||
| }.Wrap(), | ||
| }) | ||
| assert.Nil(t, err) | ||
| assert.Equal(t, len(as), 3) |
There was a problem hiding this comment.
Critical: Test assertion may be incorrect
The assertion assert.Equal(t, len(as), 3) expects all assets to be returned, but based on the PR objectives and AI summary, the filter should only return assets where coreSupport is true. This test might be masking the filter bug instead of verifying the fix.
Consider updating the assertion to verify only assets with coreSupport=true are returned:
-assert.Equal(t, len(as), 3)
+expectedCount := 1 // Only a2 has coreSupport=true
+assert.Equal(t, expectedCount, len(as))
+for _, a := range as {
+ assert.True(t, a.CoreSupport())
+}Committable suggestion skipped: line range outside the PR's diff.
Overview
The query for cases where coresupport does not exist in the database is incorrect.
What I've done
What I haven't done
How I tested
It worked correctly after processing the data locally.
Which point I want you to review particularly
The test case was skipped because data without coresupport could not be created.
Memo
Summary by CodeRabbit
New Features
Bug Fixes
Tests