feat(server): Add coreSupport to Asset #1246
Conversation
WalkthroughThis pull request introduces a new test file for end-to-end testing of asset management functionalities in a GraphQL API. It adds a Changes
Possibly related PRs
Suggested labels
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
Documentation and Community
|
✅ Deploy Preview for reearth-web ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (16)
server/internal/adapter/gql/generated.go (1)
59847-59847: Consider adding input validation for the visualizer fieldWhile the input handling is implemented correctly, consider adding validation to ensure the visualizer value meets any business rules or constraints.
Example validation could include:
case "visualizer": ctx := graphql.WithPathContext(ctx, graphql.NewPathWithField("visualizer")) data, err := ec.unmarshalNBoolean2bool(ctx, v) if err != nil { return it, err } + // Add any business rule validation here it.Visualizer = dataAlso applies to: 59861-59867
server/internal/adapter/gql/gqlmodel/convert_asset.go (1)
Line range hint
7-21: Consider adding validation for the visualizer field.While the mapping is correct, consider if any validation is needed for the visualizer field, especially for backwards compatibility with existing assets as mentioned in the PR objectives.
You might want to:
- Add validation logic to handle cases where the visualizer field is not set
- Consider documenting the default behavior for existing assets
- Add logging for cases where the field is missing to help with monitoring during the transition
server/pkg/asset/asset.go (2)
22-22: Consider adding field documentation and default value implications.The new
visualizerfield lacks documentation explaining its purpose and behavior. Additionally, since this affects existing data, we should consider the implications of the defaultfalsevalue for existing records.Add documentation to clarify the field's purpose:
type Asset struct { id ID createdAt time.Time workspace WorkspaceID name string // file name size int64 // file size url string contentType string - visualizer bool + visualizer bool // determines if the asset should be included in visualization
22-22: Consider data migration strategy and deployment implications.Since this change affects existing assets and requires manual data updates:
- Consider creating a data migration script to set the default visualizer value for existing assets
- Plan the deployment to handle the transition period where some assets might not have the visualizer field
- Document the default behavior in cases where the visualizer field is not present
Would you like assistance in:
- Creating a data migration script?
- Documenting the migration strategy?
- Implementing graceful handling of missing visualizer fields?
server/gql/asset.graphql (1)
Line range hint
1-72: Schema changes look well-structuredThe GraphQL schema changes are clean, consistent, and follow best practices. The new visualizer field is properly integrated into both the Asset type and CreateAssetInput.
Consider documenting the visualizer field's purpose and behavior in the schema using GraphQL descriptions (""" """) to improve schema documentation.
server/internal/adapter/gql/resolver_mutation_asset.go (1)
19-22: Consider documenting data migration requirements.Since this change affects asset creation and the PR objectives mention that past data needs direct updates, consider adding a code comment about data migration requirements.
Add a comment before the CreateAssetParam struct:
+ // Note: Existing assets without a visualizer flag need to be updated directly in the database. + // See migration guide in PR #1246 res, err := usecases(ctx).Asset.Create(ctx, interfaces.CreateAssetParam{ WorkspaceID: tid, Visualizer: input.Visualizer, File: gqlmodel.FromFile(&input.File), }, getOperator(ctx))server/internal/usecase/interfaces/asset.go (1)
27-27: Add documentation for the Visualizer fieldConsider adding a comment to document:
- The purpose of the Visualizer flag
- Whether it's required or optional
- Any default values or constraints
Example:
type CreateAssetParam struct { WorkspaceID accountdomain.WorkspaceID + // Visualizer indicates whether the asset should be available for visualization + // Default: true for new assets Visualizer bool File *file.File }server/pkg/asset/builder.go (2)
Line range hint
14-33: Consider adding visualizer validation in Build method.The
Build()method validates other required fields but doesn't enforce any validation rules for the new visualizer field. According to the PR objectives, this field is required for new assets.Consider updating the validation:
func (b *Builder) Build() (*Asset, error) { if b.a.id.IsNil() { return nil, ErrInvalidID } if b.a.workspace.IsNil() { return nil, ErrEmptyWorkspaceID } if b.a.url == "" { return nil, ErrEmptyURL } if b.a.size <= 0 { return nil, ErrEmptySize } + // Set default value for visualizer if not explicitly set + if !b.a.visualizer { + b.a.visualizer = true + } if b.a.createdAt.IsZero() { b.a.createdAt = b.a.CreatedAt() } return b.a, nil }
Line range hint
9-12: Set default visualizer value in New() constructor.According to the PR objectives, new assets should have visualizer set to true by default. Consider setting this in the constructor.
Update the constructor to set the default value:
func New() *Builder { - return &Builder{a: &Asset{}} + return &Builder{a: &Asset{ + visualizer: true, + }} }server/internal/infrastructure/mongo/mongodoc/asset.go (1)
63-63: LGTM: Consider documenting error handling for the Visualizer fieldThe Visualizer field is correctly mapped in the asset builder chain. Since this is part of the Model() method that can return an error, consider documenting whether any validation rules apply to the Visualizer field that could cause a build error.
server/internal/usecase/interactor/asset_test.go (1)
48-48: Add test cases for different visualizer scenariosWhile this test covers the basic case where
Visualizeris true, consider adding test cases for:
Visualizer: false- Omitted
Visualizerfield (to verify default behavior)This will ensure complete coverage of the new visualizer functionality.
Here's a suggested test table structure:
tests := []struct { name string visualizer *bool wantErr bool }{ { name: "with visualizer true", visualizer: ptr(true), wantErr: false, }, { name: "with visualizer false", visualizer: ptr(false), wantErr: false, }, { name: "without visualizer", visualizer: nil, wantErr: false, }, }server/internal/infrastructure/memory/asset.go (1)
Line range hint
89-103: Update TotalSizeByWorkspace to respect visualizer flagThe
TotalSizeByWorkspacemethod should be consistent withFindByWorkspaceand only include assets that are visualized.Apply this change to maintain consistency:
func (r *Asset) TotalSizeByWorkspace(_ context.Context, wid accountdomain.WorkspaceID) (t int64, err error) { if !r.f.CanRead(wid) { return 0, nil } r.data.Range(func(k id.AssetID, v *asset.Asset) bool { - if v.Workspace() == wid { + if v.Workspace() == wid && (!v.HasVisualizer() || v.Visualizer()) { t += v.Size() } return true }) return }server/internal/usecase/interactor/asset.go (2)
Line range hint
38-47: Update FindByWorkspace to filter by visualizer flag.According to the PR objectives, asset retrieval should return only assets where the visualizer flag is true or doesn't exist. However, the
FindByWorkspacemethod doesn't implement this filtering.Update the
AssetFilterto include visualizer filtering:return i.repos.Asset.FindByWorkspace(ctx, tid, repo.AssetFilter{ Sort: sort, Keyword: keyword, Pagination: p, + Visualizer: true, // Include assets where visualizer is true or not set })
Line range hint
48-104: Consider documenting data migration strategy.The PR objectives mention that past data needs manual updates to accommodate the new visualizer flag. Consider adding a comment in the
Createmethod documenting this requirement and any default behavior for existing assets.Add a comment before the builder chain:
+ // Note: Existing assets without a visualizer flag need manual updates. + // New assets default to the specified visualizer value. a, err := asset.New(). NewID(). Workspace(inp.WorkspaceID).server/e2e/gql_asset_test.go (1)
143-166: Enhance flexibility of asset retrieval.The getAssets helper function could be more flexible and robust.
Consider these improvements:
+// GetAssetsOptions contains options for asset retrieval +type GetAssetsOptions struct { + First int + After string + SortField string + SortDir string +} + -func getAssets(e *httpexpect.Expect, teamId string) *httpexpect.Value { +func getAssets(e *httpexpect.Expect, teamId string, opts *GetAssetsOptions) *httpexpect.Value { + if opts == nil { + opts = &GetAssetsOptions{ + First: 20, + SortField: "DATE", + SortDir: "DESC", + } + } + requestBody := GraphQLRequest{ OperationName: "GetAssets", Query: GetAssetsQuery, Variables: map[string]any{ "teamId": teamId, "pagination": map[string]any{ - "first": 20, + "first": opts.First, + "after": opts.After, }, "sort": map[string]string{ - "direction": "DESC", - "field": "DATE", + "direction": opts.SortDir, + "field": opts.SortField, }, }, }server/internal/adapter/gql/resolver_mutation_property.go (1)
103-103: Consider making the visualizer flag configurableWhile setting
Visualizer: truealigns with the PR objectives, hardcoding this value might limit future flexibility. Consider:
- Making it configurable through the input parameters
- Using a configuration value or environment variable for the default setting
Example modification to make it configurable:
- Visualizer: true, + Visualizer: input.Visualizer != nil && *input.Visualizer,This would require updating the
UploadFileToPropertyInputstruct to include an optional visualizer flag:type UploadFileToPropertyInput struct { // ... existing fields ... Visualizer *bool }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (2)
server/e2e/test.csvis excluded by!**/*.csvserver/e2e/test.pngis excluded by!**/*.png
📒 Files selected for processing (16)
server/e2e/gql_asset_test.go(1 hunks)server/gql/asset.graphql(3 hunks)server/internal/adapter/gql/generated.go(12 hunks)server/internal/adapter/gql/gqlmodel/convert_asset.go(1 hunks)server/internal/adapter/gql/gqlmodel/models_gen.go(2 hunks)server/internal/adapter/gql/resolver_mutation_asset.go(1 hunks)server/internal/adapter/gql/resolver_mutation_property.go(1 hunks)server/internal/infrastructure/memory/asset.go(1 hunks)server/internal/infrastructure/mongo/asset.go(2 hunks)server/internal/infrastructure/mongo/mongodoc/asset.go(3 hunks)server/internal/usecase/interactor/asset.go(1 hunks)server/internal/usecase/interactor/asset_test.go(2 hunks)server/internal/usecase/interfaces/asset.go(1 hunks)server/pkg/asset/asset.go(2 hunks)server/pkg/asset/asset_test.go(4 hunks)server/pkg/asset/builder.go(1 hunks)
🔇 Additional comments (31)
server/internal/adapter/gql/generated.go (5)
156-156: LGTM: Schema changes align with requirements
The schema changes correctly implement the visualizer flag feature by:
- Adding a non-nullable
visualizerfield to the Asset type - Including the field in asset creation input
- Providing proper query filtering options
Also applies to: 8633-8633, 8646-8646, 8683-8695
1985-1991: LGTM: Resolver implementation is robust
The resolver implementation includes:
- Proper complexity calculation
- Comprehensive error handling
- Null checks
- Correct field resolution
Also applies to: 14680-14722
14829-14830: LGTM: Field context handling is consistent
The visualizer field context is properly integrated across all required locations with consistent error handling.
Also applies to: 15032-15033, 15695-15696
64336-64340: LGTM: Field resolution handles concurrency properly
The field resolution implementation correctly:
- Follows the concurrent execution pattern
- Handles null values appropriately
8633-8633: Verify handling of existing assets without visualizer flag
The PR objectives mention that past data needs to be updated directly. Please verify that the resolver properly handles existing assets where the visualizer flag might be missing.
Also applies to: 14680-14709
✅ Verification successful
Let me search for database migration files and visualizer field handling in the codebase.
Let me search for the asset creation and handling code to verify how the visualizer flag is handled.
Let me search for the asset creation implementation to verify how the visualizer flag is handled.
Based on the search results, I can see that:
- The Asset model has a
visualizerboolean field in both the database schema and GraphQL API - The CreateAssetParam struct includes the visualizer flag
- The asset creation mutation accepts the visualizer flag as a parameter
- The asset creation implementation properly handles the visualizer flag during asset creation
The code shows that the visualizer flag is a core part of the asset model and is properly handled during creation. There's no need for special handling of missing values since:
- The flag is a required boolean field in the GraphQL API
- The flag is included in the asset creation parameters
- New assets will always have this flag set during creation
The resolver properly handles the visualizer flag for both new and existing assets. No additional validation is needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for null checks or default value handling for the visualizer field
# Check for null handling in asset-related files
rg -A 5 "visualizer.*null|null.*visualizer"
# Check for default value assignments
rg -A 5 "visualizer.*default|default.*visualizer"
# Check for database migration files
fd -e sql -e js -e ts "migration"
Length of output: 35972
Script:
#!/bin/bash
# Search for database migration files and visualizer field handling
# Look for migrations related to visualizer field
rg -A 5 "visualizer.*migration|migration.*visualizer"
# Look for asset schema definitions and updates
rg -A 10 "type.*Asset|asset.*schema"
# Look for asset creation and update handling
ast-grep --pattern 'func $_(asset|Asset)$_($$$) {
$$$
}'
Length of output: 110911
Script:
#!/bin/bash
# Search for asset creation and handling code
# Look for asset creation with visualizer flag
ast-grep --pattern 'func $_(Create|New)Asset($$$) {
$$$
}'
# Look for asset database schema and migrations
rg -A 10 "CREATE TABLE.*asset|ALTER TABLE.*asset"
# Look for asset repository implementation
rg -A 10 "func.*Save.*asset|func.*Create.*asset"
Length of output: 9956
Script:
#!/bin/bash
# Search for asset creation implementation
# Look for the complete asset creation function
rg -A 30 "func.*Create.*ctx.*CreateAssetParam.*operator.*Asset"
# Look for asset model/struct initialization
rg -A 10 "func.*New.*Asset.*\(.*\).*\*?Asset"
# Look for visualizer flag handling in asset
rg -A 5 "visualizer.*=|=.*visualizer"
Length of output: 66336
server/internal/adapter/gql/gqlmodel/convert_asset.go (1)
20-20: LGTM! Verify domain model implementation.
The addition of the Visualizer field mapping looks correct. However, let's verify the implementation of the Visualizer() method in the domain model.
✅ Verification successful
The domain model implementation is verified and correct ✅
The Visualizer() method is properly implemented in the domain model (server/pkg/asset/asset.go) and returns a boolean value from the private visualizer field. The GraphQL model also correctly defines the Visualizer field as a boolean, making the conversion in convert_asset.go valid and type-safe.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the Visualizer() method exists in the asset domain model
# and check its implementation details
# Search for the Visualizer method definition in the asset package
ast-grep --pattern 'func ($_ *Asset) Visualizer() bool {
$$$
}'
# Look for the Asset struct definition to verify the field
ast-grep --pattern 'type Asset struct {
$$$
Visualizer bool
$$$
}'
Length of output: 1275
server/pkg/asset/asset.go (1)
49-51: LGTM! The getter follows the established pattern.
The Visualizer() method follows the consistent getter pattern used throughout the struct, maintaining good code organization.
Let's verify the consistency of getter methods across the codebase:
✅ Verification successful
Getter methods in Asset struct follow a consistent pattern
The verification confirms that all getter methods in the Asset struct follow the same pattern:
- Simple method name matching the field name
- Receiver parameter
(a *Asset) - Direct return of the corresponding field
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify getter method patterns in the asset package
# Expected: All getter methods should follow the same pattern
ast-grep --pattern 'func (a *Asset) $_() $_ {
return a.$_
}'
Length of output: 1044
server/gql/asset.graphql (1)
23-23: Verify default value implementation in UploadFileToProperty
The PR objectives mention that UploadFileToProperty should set visualizer to true by default. Let's verify this implementation.
#!/bin/bash
# Description: Look for UploadFileToProperty implementation
# to verify default visualizer value
# Search for the function implementation
ast-grep --pattern 'func $$$UploadFileToProperty$$$'
# Search for any tests verifying this behavior
rg -l "TestUploadFileToProperty" server/internal/adapter/gql/resolver_mutation_asset.go (1)
20-20: Verify input validation for the visualizer field.
The implementation correctly passes the visualizer flag to the asset creation parameters. However, we should verify that the input validation is properly handled.
✅ Verification successful
Based on the search results, I can now generate the final response:
Visualizer field validation is properly enforced at the schema level
The GraphQL schema in server/gql/asset.graphql shows that the visualizer field is defined as a non-nullable boolean (Boolean!), which ensures basic type validation. The field is properly integrated into the input type and the GraphQL engine will automatically validate:
- Required field presence
- Boolean type validation
- No additional validation is needed as it's a simple boolean flag
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the CreateAssetInput type has proper validation for the visualizer field
# Look for the CreateAssetInput definition and any validation
ast-grep --pattern 'input CreateAssetInput {
$$$
visualizer: $_
$$$
}'
# Check for any validation middleware or decorators
rg -A 5 'CreateAssetInput.*validate'
Length of output: 117
Script:
#!/bin/bash
# Let's try different approaches to find the input definition and validation
# Search for GraphQL schema files
fd -e graphql -e gql
# Search for CreateAssetInput in any file
rg -A 10 "CreateAssetInput"
# Look for validation related code for assets
rg -A 5 "validate.*Asset|Asset.*validate"
# Search for any visualizer related validation
rg -A 5 "visualizer.*validate|validate.*visualizer"
Length of output: 16601
server/internal/usecase/interfaces/asset.go (2)
27-27: Consider adding validation for the Visualizer field
Since this is a new required field that affects asset filtering, consider:
- Adding validation logic in the implementation to ensure the field is properly set
- Documenting any business rules around when this should be true/false
Let's check if any validation is implemented in the asset creation flow:
#!/bin/bash
# Description: Check for validation of the Visualizer field in asset creation
# Look for Create method implementations
ast-grep --pattern 'func ($x *$type) Create(ctx context.Context, param CreateAssetParam, op *usecase.Operator) (*asset.Asset, error) {
$$$
}'
# Look for any validation utilities
rg -g '!generated*' -g '!*_test.go' "func.*[Vv]alidate.*Asset"27-27: Verify backward compatibility
The PR notes mention that past data needs direct updates. Consider:
- Adding a database migration for existing assets
- Documenting the default behavior for assets without this flag
Let's check for any migration files:
server/pkg/asset/builder.go (1)
72-75: LGTM! Method implementation follows builder pattern.
The implementation follows the established builder pattern consistently with other methods in the file.
server/internal/infrastructure/mongo/mongodoc/asset.go (3)
20-20: Verify handling of existing documents without the Visualizer field
The addition of the Visualizer field looks good. However, we should ensure proper handling of existing documents in the database that won't have this field.
Consider documenting the default behavior for existing documents and whether any data migration is needed. According to the PR objectives, past data will need direct updates.
41-41: LGTM: Proper initialization of the Visualizer field
The initialization of the Visualizer field from the domain model is implemented correctly and consistently with other fields.
Line range hint 20-63: Verify implementation of visualizer-based filtering
The PR objectives mention that asset retrieval should return assets where "visualizer flag is set to true or does not exist". Let's verify the implementation of this filtering logic.
✅ Verification successful
Visualizer filtering implementation is correct
The MongoDB query in FindByWorkspace correctly implements the requirement to return assets where "visualizer flag is set to true or does not exist" using the $or operator:
"$or": []bson.M{
{"visualizer": true},
{"visualizer": bson.M{"$exists": false}},
}🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for asset retrieval query implementations
rg -t go "GetAssets|Find.*Asset" --glob "server/**/*.go" -A 5
# Look for MongoDB query filters related to the visualizer field
rg -t go "\$or.*visualizer|find.*visualizer" --glob "server/**/*.go" -A 5
Length of output: 8653
Script:
#!/bin/bash
# Search for MongoDB query filter implementation in the asset repository
rg -t go "filter.*:=.*bson" --glob "server/internal/infrastructure/mongo/asset.go" -A 10
# Search for AssetFilter struct definition
rg -t go "type AssetFilter" --glob "server/**/*.go" -A 5
# Search for visualizer-related MongoDB queries
rg -t go "visualizer.*bson|bson.*visualizer" --glob "server/**/*.go" -A 5
Length of output: 1421
Script:
#!/bin/bash
# Get more context around the visualizer query implementation
rg -t go "visualizer.*bson|bson.*visualizer" --glob "server/internal/infrastructure/mongo/asset.go" -B 10 -A 10
# Look for the complete FindByWorkspace implementation
ast-grep --pattern 'func (r *Asset) FindByWorkspace($$$) {
$$$
}'
Length of output: 1738
server/pkg/asset/asset_test.go (3)
26-26: LGTM: Visualizer field properly integrated into test structure
The addition of the Visualizer boolean field to the test structure aligns with the PR objectives, and the default value of true correctly implements the requirement for new assets.
Also applies to: 39-39, 48-48
75-75: LGTM: Proper assertion for visualizer field
The assertion correctly validates the visualizer field using the getter method, maintaining proper encapsulation.
Line range hint 1-77: Verify handling of existing assets without visualizer flag
The PR notes that past data needs manual updates. Consider adding a test case that verifies the behavior when an asset doesn't have the visualizer flag set (to ensure backward compatibility).
Let's check if there are any migration tests or handling of null/undefined visualizer flags:
server/internal/usecase/interactor/asset_test.go (2)
70-70: LGTM! Thorough assertion of the visualizer field
The test properly validates both the returned asset and the persisted asset, ensuring the visualizer field is correctly set and stored.
Line range hint 1-85: Verify migration testing coverage
The PR objectives mention that past data needs manual updates for the visualizer flag. Consider adding migration tests to verify how the system handles existing assets without the visualizer field.
Let's check if there are any migration tests:
server/internal/infrastructure/memory/asset.go (2)
52-57: Consider adding error handling for invalid visualizer values
The current implementation assumes the visualizer flag is always valid. Consider adding error handling for cases where the flag might be in an invalid state.
Let's check the error handling patterns in the codebase:
#!/bin/bash
# Description: Check error handling patterns for asset-related operations
# Test: Look for error handling in asset operations
rg -A 5 "return .*, err(or)?" server/internal/infrastructure/memory/Consider adding validation and error handling:
func (r *Asset) FindByWorkspace(_ context.Context, wid accountdomain.WorkspaceID, filter repo.AssetFilter) ([]*asset.Asset, *usecasex.PageInfo, error) {
if !r.f.CanRead(wid) {
return nil, usecasex.EmptyPageInfo(), nil
}
+ if err := validateAssetFilter(filter); err != nil {
+ return nil, usecasex.EmptyPageInfo(), fmt.Errorf("invalid filter: %w", err)
+ }
+
result := r.data.FindAll(func(k id.AssetID, v *asset.Asset) bool {
return v.Workspace() == wid && v.Visualizer() && (filter.Keyword == nil || strings.Contains(v.Name(), *filter.Keyword))
})55-55:
Verify handling of existing assets without visualizer flag
The current implementation might hide existing assets that don't have the visualizer flag set. According to the PR objectives, past data needs direct updates.
Let's verify the potential impact:
Consider modifying the condition to handle assets without the visualizer flag as mentioned in the PR objectives:
-return v.Workspace() == wid && v.Visualizer() && (filter.Keyword == nil || strings.Contains(v.Name(), *filter.Keyword))
+return v.Workspace() == wid && (!v.HasVisualizer() || v.Visualizer()) && (filter.Keyword == nil || strings.Contains(v.Name(), *filter.Keyword))server/internal/usecase/interactor/asset.go (1)
93-93: Verify visualizer flag validation.
The visualizer flag is now included in asset creation, but there's no validation to ensure it meets business requirements.
Let's verify the validation in the related files:
✅ Verification successful
Let me gather more information about any existing validation mechanisms in the codebase.
Let me check for any validation patterns or error handling in the asset-related code.
Based on the gathered information, I can now provide the final response:
The visualizer flag implementation is safe and doesn't require additional validation
The visualizer flag is a simple boolean field in CreateAssetParam that's directly passed to the asset builder. Given its binary nature (true/false) and the straightforward setter implementation in the builder, no additional validation is necessary. The existing error handling in the asset creation flow adequately covers the critical aspects like storage constraints and persistence.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check CreateAssetParam definition and any validation
rg -A 5 "type CreateAssetParam struct" server/internal/usecase/interfaces/
# Check Asset builder's Visualizer method implementation
rg -A 5 "func.*Visualizer" server/pkg/asset/
Length of output: 1104
Script:
#!/bin/bash
# Check for any validation related to assets and visualizer flag
rg -A 5 "func.*Validate.*Asset" server/
# Check for any validation in the asset interactor
rg -A 5 "validate" server/internal/usecase/interactor/asset.go
Length of output: 104
Script:
#!/bin/bash
# Look for error handling or validation patterns in asset creation
ast-grep --pattern 'if err := $$$; err != nil {
$$$
}' server/internal/usecase/interactor/asset.go
# Check for any validation or error handling in the builder
rg -B 2 -A 5 "type Builder struct" server/pkg/asset/builder.go
Length of output: 1028
server/e2e/gql_asset_test.go (2)
1-11: LGTM! Clean and well-structured imports.
The imports are appropriate for e2e testing with httpexpect.
1-166: Verify test coverage for visualizer functionality.
Let's ensure we have adequate test coverage for the new visualizer feature.
✅ Verification successful
Test coverage for visualizer functionality is adequate
The visualizer functionality has comprehensive test coverage across multiple layers:
- Unit tests in
asset_test.goverify the visualizer field in the Asset model - Integration tests in
asset_test.gocover the builder pattern with visualizer - E2E tests in
gql_asset_test.govalidate the GraphQL API behavior - Interactor tests in
asset_test.goconfirm the creation of assets with visualizer flag
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage for visualizer-related functionality
# Find all test files that might test visualizer functionality
echo "Searching for related test files..."
rg -l "visualizer.*bool" --type go
# Look for visualizer-related functions that might need testing
echo "Checking for untested visualizer-related functions..."
ast-grep --pattern 'func $_($$) {
$$$
visualizer
$$$
}'
Length of output: 381
Script:
#!/bin/bash
# Let's examine the asset-related code more thoroughly
# Check the asset builder implementation
echo "=== Asset Builder Implementation ==="
cat server/pkg/asset/builder.go
# Check the asset model implementation
echo "=== Asset Model Implementation ==="
cat server/pkg/asset/asset.go
# Look for any asset-related test files
echo "=== Other Asset Tests ==="
fd "asset.*test" --type f
Length of output: 2740
Script:
#!/bin/bash
# Let's check the other test files for visualizer coverage
echo "=== Asset Model Tests ==="
cat server/pkg/asset/asset_test.go
echo "=== Asset Interactor Tests ==="
cat server/internal/usecase/interactor/asset_test.go
echo "=== Asset Infrastructure Tests ==="
cat server/internal/infrastructure/mongo/asset_test.go
Length of output: 6524
server/internal/infrastructure/mongo/asset.go (3)
81-83: Good security practice using regexp.QuoteMeta!
The use of QuoteMeta properly escapes regex metacharacters in the search keyword, preventing potential regex injection attacks.
96-97: LGTM! URL filter implementation is correct.
The case-insensitive regex filter using the bucket pattern is appropriate for URL matching.
74-77: LGTM! Visualizer filter implementation looks correct.
The $or condition properly handles both new assets with the visualizer flag and existing assets without it, ensuring backward compatibility.
Let's verify the MongoDB query structure:
✅ Verification successful
Based on the search results, I can now provide a definitive response about the visualizer filter implementation:
The visualizer filter implementation is correct and consistent with the codebase patterns.
The search results show that:
- The
$orcondition pattern is commonly used across the codebase for similar backward compatibility scenarios (e.g., inproject.go,property.go,layer.go) - The visualizer field is a well-established concept in the codebase, used in multiple contexts (assets, projects, plugins)
- The implementation follows the same pattern used for other optional flags (e.g.,
deletedfield in projects) - The query structure matches MongoDB's recommended approach for handling fields that may or may not exist
The implementation correctly handles both cases:
{"visualizer": true}- for assets explicitly marked as visualizer assets{"visualizer": {"$exists": false}}- for legacy assets without the flag
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the MongoDB query structure in other files
# Look for similar query patterns to ensure consistency
rg -U '\$or.*\[.*visualizer' --type go
Length of output: 40
Script:
#!/bin/bash
# Let's try a broader search to understand the usage of visualizer field in MongoDB queries
rg -U 'visualizer.*bson' --type go -A 2 -B 2
# Also search for any other $or conditions to verify query patterns
rg -U '\$or.*bson\.M' --type go -A 2 -B 2
# Let's also check if visualizer is used in other contexts
rg 'visualizer' --type go -A 2 -B 2
Length of output: 71080
server/internal/adapter/gql/resolver_mutation_property.go (1)
102-105: Verify handling of existing assets
The PR notes that "past data will not be considered in this update". Please ensure that:
- The UI/API handles cases where the visualizer flag is not set
- There's a migration plan or documentation for updating existing assets
server/internal/adapter/gql/gqlmodel/models_gen.go (2)
285-287:
Review the CreateAssetInput field ordering and required status
The CreateAssetInput struct has been modified to include the required Visualizer field. Consider:
- The field ordering places
VisualizerbetweenTeamIDandFile, which is logical - Making the field optional to maintain backward compatibility with existing clients
- Documenting the default value behavior in the GraphQL schema
Let's verify the asset creation mutations and their usage:
✅ Verification successful
Based on the gathered information, I can now provide a final response:
The Visualizer field should be required as implemented
The field ordering and required status are correct for the following reasons:
- The implementation in
resolver_mutation_asset.goshows that theVisualizerfield is directly used in the asset creation process without any default value handling - The field is properly passed through to the usecase layer (
interfaces.CreateAssetParam) where it's used to determine the asset type - The field ordering (TeamID, Visualizer, File) is logical as it groups the metadata fields before the content field
There's no need to make the field optional or add default value behavior as:
- The field appears to be a fundamental property that determines the asset's type
- No default value handling is implemented in any of the resolvers or business logic
- The codebase consistently treats this as a required field throughout the asset creation flow
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check asset creation mutations and their usage
# Look for CreateAsset mutation definition
rg "mutation.*CreateAsset.*{" --type graphql
# Look for CreateAssetInput usage in Go code
rg "CreateAssetInput.*struct.*{" --type go
Length of output: 321
Script:
#!/bin/bash
# Look for CreateAsset mutation definition
rg "mutation.*CreateAsset" --type graphql -A 5
# Look for CreateAssetInput usage in Go code
rg "type CreateAssetInput" --type go -A 5
# Check GraphQL schema for asset creation
fd "schema.graphql" --exec cat {}
# Look for asset creation handler implementation
rg "func.*CreateAsset" --type go -A 10
Length of output: 10093
Script:
#!/bin/bash
# Search for schema.gql or schema.graphqls files
fd "\.(gql|graphqls)$" --exec cat {}
# Look for the default value handling of Visualizer in the resolver
rg "Visualizer.*=.*" --type go -A 2
# Check for any existing asset creation tests that might show the expected behavior
rg "TestCreateAsset" --type go -A 10
Length of output: 3771
226-226:
Consider the impact of adding a required boolean field
The addition of Visualizer bool to the Asset struct is a breaking change. According to the PR objectives, past data will not have this field set. Consider:
- Making the field optional (
*bool) to handle existing assets - Documenting the default behavior for existing assets
- Adding a data migration plan
Let's verify the GraphQL schema and queries that use this field:
hexaforce
force-pushed
the
feat/exclusion-classic-assets
branch
2 times, most recently
from
d07debd to
948e16f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
server/internal/infrastructure/mongo/asset.go (1)
74-77: Consider improving documentation and configurability of the visualizer filter.While the implementation correctly filters assets based on the visualizer flag, consider these improvements:
- Add comments explaining the visualizer flag's purpose and behavior
- Consider making the default visualizer behavior configurable through environment variables
+// Default visualizer filter configuration +const defaultVisualizerEnabled = true var filter any = bson.M{ "team": id.String(), "$or": []bson.M{ + // Return assets where visualizer is explicitly enabled or implicitly enabled (field doesn't exist) + // This maintains backward compatibility with existing assets while supporting the new visualizer feature {"visualizer": true}, {"visualizer": bson.M{"$exists": false}}, }, }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (2)
server/e2e/test.csvis excluded by!**/*.csvserver/e2e/test.pngis excluded by!**/*.png
📒 Files selected for processing (16)
server/e2e/gql_asset_test.go(1 hunks)server/gql/asset.graphql(3 hunks)server/internal/adapter/gql/generated.go(12 hunks)server/internal/adapter/gql/gqlmodel/convert_asset.go(1 hunks)server/internal/adapter/gql/gqlmodel/models_gen.go(2 hunks)server/internal/adapter/gql/resolver_mutation_asset.go(1 hunks)server/internal/adapter/gql/resolver_mutation_property.go(1 hunks)server/internal/infrastructure/memory/asset.go(1 hunks)server/internal/infrastructure/mongo/asset.go(2 hunks)server/internal/infrastructure/mongo/mongodoc/asset.go(3 hunks)server/internal/usecase/interactor/asset.go(1 hunks)server/internal/usecase/interactor/asset_test.go(2 hunks)server/internal/usecase/interfaces/asset.go(1 hunks)server/pkg/asset/asset.go(2 hunks)server/pkg/asset/asset_test.go(4 hunks)server/pkg/asset/builder.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (15)
- server/e2e/gql_asset_test.go
- server/gql/asset.graphql
- server/internal/adapter/gql/generated.go
- server/internal/adapter/gql/gqlmodel/convert_asset.go
- server/internal/adapter/gql/gqlmodel/models_gen.go
- server/internal/adapter/gql/resolver_mutation_asset.go
- server/internal/adapter/gql/resolver_mutation_property.go
- server/internal/infrastructure/memory/asset.go
- server/internal/infrastructure/mongo/mongodoc/asset.go
- server/internal/usecase/interactor/asset.go
- server/internal/usecase/interactor/asset_test.go
- server/internal/usecase/interfaces/asset.go
- server/pkg/asset/asset.go
- server/pkg/asset/asset_test.go
- server/pkg/asset/builder.go
🔇 Additional comments (2)
server/internal/infrastructure/mongo/asset.go (2)
88-90: The previous review comment about refactoring the bucket pattern logic is still applicable.
The suggestions from the previous review about:
- Converting magic strings to constants
- Using environment variables for test configuration
- Simplifying the conditional flow
are still relevant to this code segment.
74-77: Verify index coverage for the visualizer filter.
The new $or condition on the visualizer field might benefit from an index to optimize query performance, especially for large collections.
Let's check the existing indexes and their coverage:
Consider adding an index if query performance is a concern:
var (
- assetIndexes = []string{"team"}
+ assetIndexes = []string{"team", "visualizer"}
assetUniqueIndexes = []string{"id"}
)✅ Verification successful
No index needed for the visualizer field
Based on the codebase analysis, the visualizer field in the asset collection does not require a dedicated index for the following reasons:
- The existing index coverage is sufficient:
assetIndexes = []string{"team"}- The
$orquery onvisualizerfield is used in combination with a simple equality match, which is not a performance-critical operation since:
- The condition is simple (
trueor non-existent) - The field has low cardinality (boolean/null values)
- The query is likely not frequently executed
Adding an index would:
- Increase storage overhead
- Impact write performance
- Provide minimal query performance benefit given the nature of the field and query pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check existing indexes and their coverage for the visualizer field
# Check if visualizer field is covered by any existing indexes
ast-grep --pattern 'assetIndexes = []string{$$$}'
# Check if there are any explain plan hints about index usage
ast-grep --pattern 'client.Find($$$explain$$$)'
Length of output: 101
Script:
#!/bin/bash
# Let's try a broader search for index definitions and mongo queries
# Search for index-related code
rg -i "index|indexes" --type go -A 5 -B 5
# Search for mongo collection setup and configuration
rg "mongo" --type go -A 5 -B 5
# Search specifically for asset collection setup
rg "asset.*collection" --type go -i -A 5 -B 5
Length of output: 150570
hexaforce
force-pushed
the
feat/exclusion-classic-assets
branch
from
acacab6 to
6f0ea74
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (29)
server/internal/usecase/interfaces/published.go (1)
40-40: LGTM! Consider adding unit tests.The mapping between the interface method and struct field is correct. Consider adding unit tests to verify the behavior with both true and false values for the core flag.
Would you like me to help generate unit tests for the
PublishedMetadataFromfunction?server/internal/infrastructure/mongo/mongodoc/asset.go (1)
Line range hint
1-64: Consider data migration strategyAs noted in the PR objectives, existing data won't automatically include the new field. Consider implementing a data migration strategy to set appropriate default values for existing assets.
Options to consider:
- Add a database migration script
- Implement fallback logic in the
Model()method- Add a background job to update existing records
Would you like assistance in implementing any of these migration strategies?
server/pkg/storytelling/story_bulider_test.go (1)
Line range hint
1-1: Fix typo in filename: "bulider" should be "builder"The filename contains a typo:
story_bulider_test.goshould bestory_builder_test.go.server/pkg/storytelling/story_bulider.go (1)
Line range hint
1-1: Fix typo in filename: "bulider" should be "builder"The filename contains a typo:
story_bulider.goshould bestory_builder.go. This should be corrected for better maintainability and consistency.server/internal/usecase/interfaces/project.go (1)
26-26: LGTM! Consider adding documentationThe implementation of the optional
Corefield as*boolis technically sound and maintains backward compatibility.Consider adding a comment to document the purpose of this field:
+ // Core indicates whether the project supports core functionality Core *boolserver/internal/infrastructure/mongo/mongodoc/project.go (1)
126-126: Consider grouping related builder calls together.The
Core()builder call could be grouped with other related boolean flags likeIsArchived()orIsBasicAuthActive()for better code organization and readability.server/e2e/gql_project_export_test.go (1)
Line range hint
1-140: Consider improving test maintainabilityThe test structure is well-organized, but there are a few improvements that could make it more maintainable:
- Consider extracting test URLs into constants
- Add more descriptive error messages in assertions
- Consider adding sub-tests using
t.Run()for better test organizationHere's a suggested improvement for the test structure:
func TestCallExportProject(t *testing.T) { + const ( + testImageURL = "https://test.com/project.jpg" + testAssetURL = "http://localhost:8080/assets/01jbbhhtq2jq7mx39dhyq1cfr2.png" + ) + + t.Run("should successfully export project with external images", func(t *testing.T) { e := StartServer(t, &config.Config{ Origins: []string{"https://example.com"}, AuthSrv: config.AuthSrvConfig{ Disabled: true, }, }, true, baseSeeder) // ... rest of the test + }) }server/pkg/scene/builder/builder.go (2)
95-100: LGTM! Consider adding documentation.The parameter rename from
coreSupporttocoreimproves clarity. Consider adding a documentation comment to explain the purpose of thecoreparameter.+// Build writes the scene JSON to the provided writer. The core parameter determines +// whether core features should be enabled for the scene. func (b *Builder) Build(ctx context.Context, w io.Writer, publishedAt time.Time, core bool, enableGa bool, trackingId string) error {
132-137: Improve error handling using fmt.Errorf.While the parameter rename looks good, the error handling could be improved:
- Use
fmt.Errorfinstead of string concatenation- Make error messages more descriptive
func (b *Builder) BuildResult(ctx context.Context, publishedAt time.Time, core bool, enableGa bool, trackingId string) (*sceneJSON, error) { if b == nil || b.scene == nil { return nil, errors.New("invalid builder state") } sceneData, err := b.buildScene(ctx, publishedAt, core, enableGa, trackingId) if err != nil { - return nil, errors.New("Fail buildScene :" + err.Error()) + return nil, fmt.Errorf("failed to build scene: %w", err) }server/e2e/gql_asset_test.go (2)
55-70: Add documentation for the GraphQL mutation.Consider adding a comment describing the purpose and parameters of this mutation.
+// CreateAssetMutation is a GraphQL mutation for creating assets. +// Parameters: +// - teamId: The ID of the team the asset belongs to +// - file: The file to upload +// - visualizer: Boolean flag indicating if the asset should be visible in the visualizer const CreateAssetMutation = `mutation CreateAsset($teamId: ID!, $file: Upload!, $visualizer: Boolean!) {
103-141: Add documentation for the GraphQL query.Consider adding a comment describing the purpose and parameters of this query.
+// GetAssetsQuery is a GraphQL query for retrieving assets with pagination and sorting. +// Parameters: +// - teamId: The ID of the team to fetch assets from +// - pagination: Optional pagination parameters +// - keyword: Optional search keyword +// - sort: Optional sorting parameters const GetAssetsQuery = `query GetAssets($teamId: ID!, $pagination: Pagination, $keyword: String, $sort: AssetSort) {server/pkg/storytelling/story.go (1)
105-106: Consider adding a setter method for thecorefield.While the getter method is properly implemented, there's no corresponding setter method. This might limit the ability to modify the
corestatus after creation.Consider adding a setter method:
+func (s *Story) SetCore(core bool) { + s.core = core + s.updatedAt = util.Now() +}server/pkg/scene/builder/scene.go (2)
26-28: Add documentation for the new configuration fields.The new fields lack documentation explaining their purpose and usage. Consider adding comments to clarify:
- What the
Coreflag represents and its impact- When
EnableGAshould be true/false- Expected format and purpose of
TrackingIDtype sceneJSON struct { + // Core indicates whether the scene supports core functionality Core bool `json:"core"` + // EnableGA determines if Google Analytics tracking is enabled EnableGA bool `json:"enableGa"` + // TrackingID specifies the Google Analytics tracking identifier TrackingID string `json:"trackingId"` }
31-31: Consider adding parameter validation and documentation.The method could benefit from:
- Documentation explaining the parameters and their requirements
- Input validation for
trackingIdwhenenableGais true+// sceneJSON creates a JSON representation of the scene with the specified configuration. +// Parameters: +// - ctx: Context for the operation +// - publishedAt: Time when the scene was published +// - l: List of layer JSON objects +// - p: List of properties +// - core: Whether core functionality is enabled +// - enableGa: Whether Google Analytics is enabled +// - trackingId: Google Analytics tracking ID (required if enableGa is true) func (b *Builder) sceneJSON(ctx context.Context, publishedAt time.Time, l []*layerJSON, p []*property.Property, core bool, enableGa bool, trackingId string) (*sceneJSON, error) { + if enableGa && trackingId == "" { + return nil, fmt.Errorf("tracking ID is required when Google Analytics is enabled") + } tags, err := b.tags(ctx) if err != nil { return nil, err } // ... rest of the implementation }Also applies to: 48-50
server/internal/usecase/interactor/project_test.go (2)
75-75: Add documentation for the Core fieldThe test cases properly cover both true and false values for the new Core field. However, it would be helpful to add documentation explaining what this field represents and its significance in the project context.
Consider adding a comment above the test cases explaining the purpose and impact of the Core field.
Also applies to: 90-90, 106-106
GraphQL schema needs to be updated for the Core field
The Core field is missing from the Project type in the GraphQL schema, although it's present in the CreateProjectInput. This needs to be addressed:
server/gql/project.graphql: Add the Core field to the Project type definition- The field is properly defined in CreateProjectInput but missing in UpdateProjectInput
- No database migration appears to be present for this new field
🔗 Analysis chain
Line range hint
75-106: Verify related changes for the Core fieldSince this introduces a new Core field to projects, we should verify related changes:
Let me check the GraphQL schema and migrations to verify if the Core field is properly defined.
Let me check for any Core field definitions in the GraphQL schema input types and mutations, as well as look for any recent migration files that might contain Core field additions.
Let me check the content of the GraphQL input types to verify if the Core field is properly defined in the schema.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for related Core field changes in UI, GraphQL schema, and migrations # Check for UI components using the Core field rg -g '*.{tsx,jsx}' -A 3 'core.*project' # Check for GraphQL schema changes rg -g '*.graphql' 'type Project' # Check for database migrations fd -g '*migration*.{go,sql}'Length of output: 465
Script:
#!/bin/bash # Check GraphQL schema for Core field rg -g '*.graphql' -A 10 'type Project' # Check recent migrations for Core field cat server/internal/infrastructure/mongo/migration/migrations.goLength of output: 3062
Script:
#!/bin/bash # Check GraphQL schema for Core field in input types and mutations rg -g '*.graphql' 'input.*Project|mutation.*project' # Check for recent migration files fd -e go -e sql . server/internal/infrastructure/mongo/migration/Length of output: 1860
Script:
#!/bin/bash # Check GraphQL input types for Core field rg -g '*.graphql' -A 10 'input CreateProjectInput|input UpdateProjectInput' # Also check the Project type definition again to ensure we didn't miss the Core field rg -g '*.graphql' -A 15 'type Project implements Node'Length of output: 1954
server/pkg/project/project_test.go (1)
Line range hint
324-346: Test function name doesn't match tested functionalityThe test function name
TestProject_Experimentalshould be renamed toTestProject_Coreto accurately reflect the functionality being tested. This improves code maintainability and makes it easier for developers to understand the purpose of the test.-func TestProject_Experimental(t *testing.T) { +func TestProject_Core(t *testing.T) { tests := []struct { name string p *Project expected bool }{server/internal/usecase/interactor/scene_test.go (2)
Line range hint
1-194: Test structure needs improvementThe test could be improved in several ways:
- The test data is a large inline JSON string which makes it hard to maintain
- The test only covers the "happy path" with
core: true- Missing test cases for
core: falseand when the field is absentConsider refactoring the test to:
- Move the test data to a separate test fixture file
- Add test cases for different
corevalues using table-driven tests- Add assertions specifically for the
corefield behaviorHere's a suggested refactor for the test structure:
func TestImportScene(t *testing.T) { + tests := []struct { + name string + coreVal *bool + wantErr bool + }{ + { + name: "with core true", + coreVal: lo.ToPtr(true), + wantErr: false, + }, + { + name: "with core false", + coreVal: lo.ToPtr(false), + wantErr: false, + }, + { + name: "without core field", + coreVal: nil, + wantErr: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // ... test setup ... + + // Modify test data based on test case + if tt.coreVal != nil { + sceneData["core"] = *tt.coreVal + } + + // ... rest of the test ... + }) + } }
Line range hint
151-194: Improve test assertionsThe test removes several fields before comparison but doesn't document why these fields are excluded. This makes it harder to understand the test's intentions and maintain it.
Consider:
- Adding comments explaining why each field is excluded
- Creating helper functions for field exclusion
- Adding specific assertions for important fields before the JSON comparison
Example:
+ // Helper to exclude dynamic fields that change on each test run + func excludeDynamicFields(m map[string]interface{}) { + // IDs are generated dynamically + delete(m, "rootLayerId") + delete(m, "propertyId") + // Timestamps change on each run + delete(m, "updatedAt") + delete(m, "createdAt") + } // actual temp := gqlmodel.ToScene(result) resultJSON, err := json.Marshal(temp) assert.NoError(t, err) var resultMap map[string]interface{} err = json.Unmarshal(resultJSON, &resultMap) assert.NoError(t, err) - delete(resultMap, "rootLayerId") - delete(resultMap, "propertyId") - delete(resultMap, "updatedAt") - delete(resultMap, "createdAt") + excludeDynamicFields(resultMap) + // Assert important fields before JSON comparison + assert.Equal(t, sceneData["core"], resultMap["core"], "core field should match input")server/pkg/project/builder_test.go (2)
57-58: Consider adding more test scenarios for Core functionalityWhile the basic test case looks good, consider adding more scenarios to thoroughly test the Core functionality:
- Default value when not set
- Setting to false explicitly
- Multiple toggles of the value
Example test cases to add:
func TestBuilder_Experimental(t *testing.T) { var tb = New().NewID() - res := tb.Core(true).MustBuild() - assert.True(t, res.Core()) + t.Run("default value", func(t *testing.T) { + res := tb.MustBuild() + assert.False(t, res.Core()) + }) + + t.Run("set to true", func(t *testing.T) { + res := tb.Core(true).MustBuild() + assert.True(t, res.Core()) + }) + + t.Run("set to false", func(t *testing.T) { + res := tb.Core(false).MustBuild() + assert.False(t, res.Core()) + }) + + t.Run("toggle multiple times", func(t *testing.T) { + res := tb.Core(true).Core(false).Core(true).MustBuild() + assert.True(t, res.Core()) + }) }
Line range hint
266-267: Update Build and MustBuild tests to include Core fieldThe comprehensive test cases in
TestBuilder_BuildandTestBuilder_MustBuildshould be updated to include the Core field in their test scenarios.Add the Core field to the test structs and assertions:
type args struct { name, description string alias, publicTitle string publicDescription string publicImage string id ID isArchived bool + core bool updatedAt time.Time // ... other fields ... }Update the test cases to include Core scenarios:
{ name: "build normal project", args: args{ name: "xxx.aaa", description: "ddd", + core: true, // ... other fields ... }, expected: &Project{ id: pid, description: "ddd", + core: true, // ... other fields ... }, }Also update the builder chain in both test functions:
return New(). ID(tt.args.id). + Core(tt.args.core). // ... other builder calls ... MustBuild()Also applies to: 379-380
server/e2e/gql_project_test.go (1)
164-164: Consider extracting ProjectFragment to reduce duplicationThe ProjectFragment is duplicated in two locations (lines 164 and 427) with identical fields. Consider extracting it into a shared GraphQL fragment file or constant to maintain DRY principles and ensure consistency.
Example approach:
const projectFragment = ` fragment ProjectFragment on Project { id name description imageUrl isArchived isBasicAuthActive basicAuthUsername basicAuthPassword publicTitle publicDescription publicImage alias enableGa trackingId publishmentStatus updatedAt createdAt core starred __typename }` // Then in your tests: query := fmt.Sprintf(` query GetProjects($teamId: ID!, $pagination: Pagination, $keyword: String, $sort: ProjectSort) { projects(/*...*/) { /*...*/ } } %s`, projectFragment)Also applies to: 427-427
server/internal/usecase/interactor/project.go (2)
155-156: Add validation for the Core fieldConsider adding validation for the Core field to ensure it meets any business requirements or constraints.
324-324: Document the purpose and impact of the Core fieldThe Core field is used during scene building, but its purpose and impact on the scene building process are not documented. Consider adding documentation to explain:
- What the Core field represents
- How it affects scene building
- Any constraints or requirements
Also applies to: 440-440
server/e2e/gql_nlslayer_test.go (3)
Line range hint
825-854: Consider adding schema validation and error case handlingThe
addCustomPropertieshelper function could be more robust:
- Add schema structure validation before sending the request
- Add test cases for error scenarios (invalid schema, invalid layer ID)
func addCustomProperties( e *httpexpect.Expect, layerId string, schema map[string]any, ) (GraphQLRequest, *httpexpect.Value) { + // Validate required schema fields + if schema == nil { + panic("schema cannot be nil") + } + requiredFields := []string{"id", "type"} + for _, field := range requiredFields { + if _, ok := schema[field]; !ok { + panic(fmt.Sprintf("schema missing required field: %s", field)) + } + } + requestBody := GraphQLRequest{ OperationName: "AddCustomProperties", Query: `mutation AddCustomProperties($input: AddCustomPropertySchemaInput!) {
Line range hint
856-935: Enhance test coverage with edge casesThe
TestCustomPropertiesfunction would benefit from additional test cases:
- Negative test cases (invalid schema)
- Boundary testing for extrudedHeight
- Validation of positions array length
- Error scenarios (non-existent layer)
func TestCustomProperties(t *testing.T) { + t.Run("invalid schema", func(t *testing.T) { + // Test with missing required fields + invalidSchema := map[string]any{ + "extrudedHeight": 0, + } + // Expect error response + }) + + t.Run("boundary values", func(t *testing.T) { + // Test extreme values for extrudedHeight + // Test empty and large positions arrays + }) + + t.Run("non-existent layer", func(t *testing.T) { + // Test with invalid layer ID + // Expect appropriate error response + })
Line range hint
485-515: Remove or document commented-out codeThe
removeNLSInfoboxfunction is commented out without any explanation. Either:
- Remove it if it's no longer needed
- Document why it's kept for future reference
server/internal/usecase/interactor/scene.go (1)
128-130: Consider extracting visualizer selection logic.The visualizer selection logic is repeated in multiple places. Consider extracting this into a helper method to improve maintainability and reduce duplication.
+func getVisualizer(isCore bool) visualizer.Visualizer { + if isCore { + return visualizer.VisualizerCesiumBeta + } + return visualizer.VisualizerCesium +}Then use it in the code:
-var viz = visualizer.VisualizerCesium -if prj.Core() { - viz = visualizer.VisualizerCesiumBeta -} +viz := getVisualizer(prj.Core())Also applies to: 632-632, 809-811
server/e2e/gql_storytelling_test.go (1)
1051-1051: Test assertion needs to be more specificThe test assertion includes
"core":truein the regex pattern, but it's mixed with many other fields. Consider extracting core-specific assertions for better test clarity.- pub := regexp.MustCompile(fmt.Sprintf(`{"schemaVersion":1,"id":"%s","publishedAt":".*","property":{"tiles":\[{"id":".*"}]},"plugins":{},"layers":null,"widgets":\[],"widgetAlignSystem":null,"tags":\[],"clusters":\[],"story":{"id":"%s","title":"","property":{},"pages":\[{"id":"%s","property":{},"title":"test","blocks":\[{"id":"%s","property":{"default":{"text":"test value"},"panel":{"padding":{"top":2,"bottom":3,"left":0,"right":1}}},"plugins":null,"extensionId":"%s","pluginId":"%s"}],"swipeable":true,"swipeableLayers":\[],"layers":\[]}],"position":"left","bgColor":""},"nlsLayers":null,"layerStyles":null,"core":true,"enableGa":false,"trackingId":""}`, sID, storyID, pageID, blockID, extensionId, pluginId)) + expectedCore := `"core":true` + expectedFormat := fmt.Sprintf(`{"schemaVersion":1,"id":"%s","publishedAt":".*","property":{"tiles":\[{"id":".*"}]},"plugins":{},"layers":null,"widgets":\[],"widgetAlignSystem":null,"tags":\[],"clusters":\[],"story":{"id":"%s","title":"","property":{},"pages":\[{"id":"%s","property":{},"title":"test","blocks":\[{"id":"%s","property":{"default":{"text":"test value"},"panel":{"padding":{"top":2,"bottom":3,"left":0,"right":1}}},"plugins":null,"extensionId":"%s","pluginId":"%s"}],"swipeable":true,"swipeableLayers":\[],"layers":\[]}],"position":"left","bgColor":""},"nlsLayers":null,"layerStyles":null,%s,"enableGa":false,"trackingId":""}`, sID, storyID, pageID, blockID, extensionId, pluginId, expectedCore) + pub := regexp.MustCompile(expectedFormat)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (2)
server/e2e/test.csvis excluded by!**/*.csvserver/e2e/test.pngis excluded by!**/*.png
📒 Files selected for processing (40)
server/e2e/gql_asset_test.go(1 hunks)server/e2e/gql_nlslayer_test.go(1 hunks)server/e2e/gql_project_export_test.go(2 hunks)server/e2e/gql_project_test.go(5 hunks)server/e2e/gql_storytelling_test.go(3 hunks)server/gql/asset.graphql(3 hunks)server/gql/project.graphql(2 hunks)server/internal/adapter/gql/generated.go(26 hunks)server/internal/adapter/gql/gqlmodel/convert_asset.go(1 hunks)server/internal/adapter/gql/gqlmodel/convert_project.go(1 hunks)server/internal/adapter/gql/gqlmodel/convert_project_test.go(2 hunks)server/internal/adapter/gql/gqlmodel/models_gen.go(4 hunks)server/internal/adapter/gql/resolver_mutation_asset.go(1 hunks)server/internal/adapter/gql/resolver_mutation_project.go(1 hunks)server/internal/adapter/gql/resolver_mutation_property.go(1 hunks)server/internal/infrastructure/memory/asset.go(1 hunks)server/internal/infrastructure/mongo/asset.go(2 hunks)server/internal/infrastructure/mongo/mongodoc/asset.go(3 hunks)server/internal/infrastructure/mongo/mongodoc/project.go(3 hunks)server/internal/usecase/interactor/asset.go(1 hunks)server/internal/usecase/interactor/asset_test.go(2 hunks)server/internal/usecase/interactor/project.go(4 hunks)server/internal/usecase/interactor/project_test.go(5 hunks)server/internal/usecase/interactor/scene.go(3 hunks)server/internal/usecase/interactor/scene_test.go(1 hunks)server/internal/usecase/interfaces/asset.go(1 hunks)server/internal/usecase/interfaces/project.go(1 hunks)server/internal/usecase/interfaces/published.go(3 hunks)server/pkg/asset/asset.go(2 hunks)server/pkg/asset/asset_test.go(4 hunks)server/pkg/asset/builder.go(1 hunks)server/pkg/project/builder.go(1 hunks)server/pkg/project/builder_test.go(1 hunks)server/pkg/project/project.go(2 hunks)server/pkg/project/project_test.go(2 hunks)server/pkg/scene/builder/builder.go(4 hunks)server/pkg/scene/builder/scene.go(2 hunks)server/pkg/storytelling/story.go(2 hunks)server/pkg/storytelling/story_bulider.go(1 hunks)server/pkg/storytelling/story_bulider_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- server/gql/asset.graphql
- server/internal/adapter/gql/gqlmodel/convert_asset.go
- server/internal/adapter/gql/gqlmodel/models_gen.go
- server/internal/adapter/gql/resolver_mutation_asset.go
- server/internal/adapter/gql/resolver_mutation_property.go
- server/internal/infrastructure/memory/asset.go
- server/internal/infrastructure/mongo/asset.go
- server/internal/usecase/interactor/asset.go
- server/internal/usecase/interactor/asset_test.go
- server/internal/usecase/interfaces/asset.go
- server/pkg/asset/asset.go
- server/pkg/asset/asset_test.go
- server/pkg/asset/builder.go
🔇 Additional comments (31)
server/internal/adapter/gql/generated.go (4)
14680-14709: LGTM! Resolver implementations follow best practices
The resolver implementations for both Asset and Project types are well-structured with:
- Proper error handling and null checks
- Thread-safe panic recovery
- Correct context propagation
Also applies to: 39826-39840
64336-64340: LGTM! Concurrent field resolution is properly implemented
The core field is correctly integrated into the concurrent field resolution system with:
- Thread-safe invalid field counting
- Proper null handling
- Consistent implementation pattern
Also applies to: 71480-71484
14829-14830: LGTM! Field context handlers are properly implemented
The field context handlers for the core field are correctly implemented with:
- Proper field registration
- Consistent error handling
- Clear error messages
Also applies to: 39857-39866, 40265-40266
Line range hint 59847-59867: Verify consistent handling of the core field
The implementation correctly handles the core field as:
- Required for new assets
- Optional for project updates
Let's verify this is consistently handled across the codebase.
Also applies to: 59942-60004
✅ Verification successful
Based on the shell script results, I can now generate the final response:
Core field handling is consistently implemented across the codebase
The verification confirms that:
CreateAssetInputconsistently requires the core field as a non-optional boolean in both GraphQL schema and Go implementationUpdateProjectInputcorrectly makes all fields optional, including core-related fields- The implementation in resolvers and interactors maintains this contract
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent handling of core field in mutations and queries
# Check for any GraphQL operations that might be missing the core field
rg -A 5 'mutation.*CreateAsset|query.*assets' --type graphql
# Check for any direct database operations that might need updating
rg -A 5 'CreateAsset|UpdateProject' --type go
Length of output: 31856
server/internal/usecase/interfaces/published.go (3)
28-28: LGTM! The struct field follows Go conventions.
The Core field is properly defined with the appropriate JSON tag and omitempty option for backward compatibility.
Line range hint 17-40: Clarify the relationship between visualizer and core flags.
The PR objectives mention adding a visualizer flag to assets, but these changes introduce a core flag in project metadata. Could you please clarify:
- Is there a relationship between the visualizer and core flags?
- Are these changes part of the same feature or a separate concern?
✅ Verification successful
Let me search for more context about the relationship between these flags.
Based on the search results, I can now generate a final response as there's clear evidence about the relationship between the core flag and visualizer functionality.
The core flag is part of the visualizer feature set
The changes are consistent with the PR objectives. The core flag (coreSupport) is directly related to the visualizer functionality as evidenced by:
- It's used alongside the visualizer parameter in project creation (see
CreateProjectDocumentmutation) - The changelog shows it was added as part of visualizer features:
- "Add coreSupport flag to project" (feat(server): add coreSupport flag to project #424)
- "Use experimental core flag" (refactor(web): Beta tab routing #448)
- It's connected to experimental/advanced visualizer features in the builtin manifests (
experimental_core)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for visualizer-related code to understand the relationship
# Expected: Find connections between visualizer and core flags
# Search for visualizer-related code
rg -i "visualizer.*flag|flag.*visualizer"
# Search for core flag usage
rg -i "core.*flag|flag.*core"
Length of output: 1628
Script:
#!/bin/bash
# Search for PR #424 and #448 to understand the relationship between core and visualizer flags
gh pr view 424 || true
gh pr view 448 || true
# Search for any mentions of core support or experimental core in the codebase
rg -i "coresupport|experimental.*core|core.*experimental" -A 3 -B 3
Length of output: 95501
17-17: Verify interface implementations and add documentation.
The renamed method Core() needs to be implemented by all types that implement HasPublicMeta. Additionally, consider adding documentation to explain the purpose of this flag.
✅ Verification successful
Core() method is properly implemented and used across the codebase
The verification shows that the Core() method is implemented by multiple types (Project, Story, and Asset) and is actively used throughout the codebase in various contexts including:
- MongoDB document conversion
- GraphQL model conversion
- Scene and project operations
- Asset filtering
The rename from CoreSupport() to Core() is consistent with the implementations and usages.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all implementations of HasPublicMeta interface to ensure they're updated
# Expected: All implementations should have the Core() method
# Search for struct types that might implement the interface
ast-grep --pattern 'type $_ struct {
$$$
}'
# Search for Core() method implementations
ast-grep --pattern 'func ($_ $_) Core() bool {
$$$
}'
Length of output: 102156
Script:
#!/bin/bash
# Let's try a different approach to find Core() implementations
# Search for Core() method implementations in the codebase
rg -g '*.go' 'func \([^)]+\) Core\(\) bool'
# Also search for any references to Core() method
rg -g '*.go' '\.Core\(\)'
Length of output: 1292
server/internal/infrastructure/mongo/mongodoc/asset.go (2)
63-63: LGTM!
The Core field mapping is correctly implemented using the builder pattern, maintaining consistency with the existing codebase structure.
41-41: Verify the Core() method implementation
The code assumes the existence of a Core() method in the asset package.
Let's verify the method implementation:
✅ Verification successful
Core() method is properly implemented in the asset package
The verification confirms that the Core() method is correctly implemented in server/pkg/asset/asset.go and returns a boolean value, making the usage in the mongodoc package valid.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if Core() method exists in the asset package
ast-grep --pattern 'func \($_\) Core() bool {
$$$
}'
Length of output: 56
Script:
#!/bin/bash
# Let's try a broader search for Core() method without assuming the return type
ast-grep --pattern 'func \($_\) Core() $_'
# Also search for Core() method using ripgrep for better coverage
rg "func.*Core\(\)" -A 2
Length of output: 487
server/pkg/storytelling/story_bulider.go (1)
25-25: Verify the unconditional core flag assignment
The core field is always set to true without any conditional logic. Consider if this is the intended behavior or if it should be configurable through the builder pattern like other fields.
✅ Verification successful
Let me analyze the results and generate additional verification scripts to understand the usage pattern of the core field across the codebase.
Let me try one more verification to understand the Story builder pattern usage:
Based on the comprehensive analysis of the codebase, I can now provide the final response:
Hardcoded core flag is the intended behavior
The core field being set to true unconditionally in the Story builder is correct because:
- The Story builder is used in controlled contexts where core stories are created, as seen in
internal/usecase/interactor/storytelling.go - The field is properly exposed through a getter method
Core()and is used in the GraphQL layer - Other entities (Project, Asset) also use this field with configurable values through their builders, showing a consistent pattern across the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any conditional usage of the core field
rg -A 5 "core\s*[:=]" --type go
Length of output: 5633
Script:
#!/bin/bash
# Search for the Story struct definition and its core field
ast-grep --pattern 'type Story struct {
$$$
core $$$
$$$
}'
# Search for any Story creation or core field usage in tests
rg -B 3 -A 3 "NewStory|Story{|StoryBuilder" --type go
Length of output: 1545
Script:
#!/bin/bash
# Search for Story builder usage and core field initialization
rg -B 3 -A 3 "NewStory|Story\{|StoryBuilder" --type go
# Search for any methods that check or use the core field in Story
ast-grep --pattern 'func ($_) Core() bool {
$$$
}'
Length of output: 21694
server/gql/project.graphql (1)
23-23: Verify the impact of making core a required field
The core field is marked as non-nullable (Boolean!), which means existing projects in the database must have this field populated.
Let's verify the database migration strategy:
✅ Verification successful
Core field is already part of the Project model with default value handling
The verification shows that:
- The
corefield already exists in the Project model (server/pkg/project/project.go) - It has a getter method
Core()implemented - The field is a boolean type at the model level
- No explicit migrations were found, suggesting MongoDB is used which is schema-less
Since MongoDB is being used (which is schema-less) and the field already exists in the model with proper getter methods, making it non-nullable in GraphQL schema is safe as:
- Existing documents will return
falsefor undefined boolean fields in MongoDB - The model already handles the field, so new projects will have it defined
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there's a migration file for this change
fd -e go -e sql migration | rg -i "core|project"
# Check if there are any database-related tests
fd -e go test | rg -i "test.*project.*core"
Length of output: 96
Script:
#!/bin/bash
# Let's check for database schema changes or migrations in a broader scope
fd migration
# Look for any SQL files that might contain schema changes
fd -e sql
# Check for database related code
rg -i "database.*project.*core" -g "!{test,vendor}/**"
# Look for project-related model definitions
rg -i "type\s+Project\s+struct" -A 10
# Check for any database initialization or schema setup
rg -i "createtable.*project" -g "*.{sql,go}"
Length of output: 4910
Script:
#!/bin/bash
# Let's check the mongo migrations since we found mongo infrastructure
rg -i "core" server/internal/infrastructure/mongo/migration/ -A 5
# Check the project model and repository for core field
rg -i "core" server/pkg/project/project.go -A 5
# Look for core field in mongo schema
rg -i "core" server/internal/infrastructure/mongo/project.go -A 5
# Check for any default value handling in project creation
rg -i "NewProject.*core" -g "*.go"
Length of output: 475
server/internal/adapter/gql/gqlmodel/convert_project_test.go (1)
72-72: Verify the field renaming across the codebase
Let's ensure this renaming is consistently applied across related files.
Also applies to: 96-96
✅ Verification successful
Field renaming from CoreSupport to Core is consistently applied
The verification shows that:
- No remaining instances of "CoreSupport" were found in the codebase
- "Core" field is consistently used as a boolean type across:
- Generated models in
models_gen.go - Project builder in
builder.go - Project entity in
project.go
- Generated models in
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining instances of CoreSupport and verify Core field usage
# Check for any remaining instances of CoreSupport
echo "Checking for remaining CoreSupport instances:"
rg "CoreSupport" --type go
# Check Core field usage in related files
echo "Checking Core field usage in related files:"
rg "Core.*bool" --type go server/internal/adapter/gql/gqlmodel/
rg "Core.*bool" --type go server/pkg/project/
Length of output: 852
server/pkg/project/builder.go (2)
125-126: Implementation follows established patterns
The Core method implementation is clean and consistent with other builder methods in the file, properly following the builder pattern with method chaining.
125-126: Verify if this change is within the PR scope
This change introduces a Core method to set a core flag on projects, but the PR objectives specifically mention adding a visualizer flag to assets. Could you clarify if this change is necessary for the visualizer feature or if it should be in a separate PR?
Let's check for any connections between the core and visualizer features:
✅ Verification successful
The Core method change is correctly scoped and necessary
The change is part of the project's core functionality and is correctly scoped. From the evidence:
- The
corefield is a fundamental part of the Project model, as shown inproject.go - The GraphQL schema shows
coreis an optional boolean field inCreateProjectInput - The field is used in project creation through the GraphQL mutation resolver
The change from CoreSupport to Core aligns with the field name in both the GraphQL schema and the Project model, making the codebase more consistent.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for relationships between core and visualizer in the codebase
rg -l "core.*visualizer|visualizer.*core" --type go
# Check if the core field is used in asset-related code
rg "core.*asset|asset.*core" --type go
Length of output: 240
Script:
#!/bin/bash
# Let's check the content of these files to understand the relationship
echo "=== server/e2e/gql_storytelling_test.go ==="
rg -A 5 -B 5 "core.*visualizer|visualizer.*core" server/e2e/gql_storytelling_test.go
echo -e "\n=== server/e2e/gql_project_test.go ==="
rg -A 5 -B 5 "core.*visualizer|visualizer.*core" server/e2e/gql_project_test.go
echo -e "\n=== server/e2e/gql_project_export_test.go ==="
rg -A 5 -B 5 "core.*visualizer|visualizer.*core" server/e2e/gql_project_export_test.go
# Let's also check the PR changes to understand the context better
echo -e "\n=== Changes to core and visualizer related files ==="
fd -e go -e graphql -e gql | xargs rg "core|visualizer" -l
# Check if there are any GraphQL schema changes related to this
echo -e "\n=== GraphQL schema changes ==="
fd "schema.graphql$" | xargs cat
Length of output: 4943
Script:
#!/bin/bash
# Let's check the project.graphql file for the schema definition
echo "=== Project GraphQL Schema ==="
cat server/gql/project.graphql
# Let's also check the project model and its relationship with visualizer
echo -e "\n=== Project Model ==="
cat server/pkg/project/project.go
# Check the actual implementation of project creation
echo -e "\n=== Project Creation Implementation ==="
cat server/internal/adapter/gql/resolver_mutation_project.go
Length of output: 19644
server/internal/infrastructure/mongo/mongodoc/project.go (1)
33-33: Please clarify the purpose of the Core field and document its impact.
The addition of the Core field (renamed from CoreSupport) needs clarification:
- What is the purpose of this field and how does it relate to the visualizer flag mentioned in the PR objectives?
- How will existing data be migrated from
CoreSupporttoCore? - Consider adding documentation to explain the field's purpose.
Let's check for any existing documentation or references:
✅ Verification successful
The Core field is a documented feature and its renaming is justified
Based on the changelog entries and codebase analysis:
- The field was initially added as
coreSupportin PR feat(server): add coreSupport flag to project #424 - It's related to the experimental core features mentioned in PR refactor(web): Beta tab routing #448
- The field exists consistently across the project model in multiple layers (pkg/project/project.go, gqlmodel, mongodoc)
- The renaming from
CoreSupporttoCorealigns with the codebase's evolution towards core feature support
The change is well-documented in the project's history and appears to be part of a broader feature implementation rather than an isolated change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any documentation or usage of Core/CoreSupport
rg -i "coresupport|core.*support|core.*flag" --type md
Length of output: 598
Script:
#!/bin/bash
# Let's check the PR history and related files for more context
rg -l "CoreSupport|coreSupport" --type go
# Also check for any Core-related fields in the project model
ast-grep --pattern 'type Project struct {
$$$
}'
Length of output: 7151
Script:
#!/bin/bash
# Let's check the changelog entries and related commits for context about the Core field
rg -A 5 "core.*flag|coresupport" CHANGELOG.md
# Also check if there are any migrations related to this field
fd migration --type f --exec rg -l "CoreSupport|core.*support|core.*flag" {}
Length of output: 2755
server/pkg/scene/builder/builder.go (1)
169-169: Verify the impact on scene JSON generation.
The parameter rename is consistent, but we should verify that the core parameter is correctly handled in the sceneJSON method and the final JSON output.
Also applies to: 186-186
✅ Verification successful
Parameter rename is correctly handled in scene JSON generation
The verification shows that:
- The
coreparameter is correctly passed through frombuildScenetosceneJSONmethod - The
sceneJSONstruct has a correspondingCorefield with proper JSON serialization tagjson:"core" - The parameter is properly assigned in the struct initialization at line 48:
Core: core
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how the core parameter affects scene JSON generation
# Search for sceneJSON method implementation
ast-grep --pattern 'func (b *Builder) sceneJSON($$$) (*sceneJSON, error) {
$$$
}'
# Search for sceneJSON struct definition
ast-grep --pattern 'type sceneJSON struct {
$$$
}'
Length of output: 3430
server/e2e/gql_asset_test.go (1)
12-53: Improve test structure and resource management.
The test function needs improvements in several areas:
- Test data management
- Resource cleanup
- Test case organization
- Error cases coverage
The previous review comment suggesting comprehensive test coverage improvements is still valid. Please refer to that comment for the detailed implementation suggestion.
server/pkg/storytelling/story.go (1)
29-29: Verify the purpose of the core field.
There appears to be a discrepancy between:
- PR objectives: Mentions adding a "visualizer flag"
- Actual changes: Adds a "core" field
- AI summary: Describes this as a rename from
coreSupport
Please clarify if this is:
- A new field for a different feature
- The visualizer flag with a different name
- A rename as suggested by the AI summary
server/pkg/project/project.go (3)
36-36: LGTM! Field addition follows Go conventions.
The core field is appropriately placed and follows Go conventions for field naming and visibility.
109-110: LGTM! Getter method is consistent with codebase patterns.
The Core() method follows the established getter pattern used throughout the codebase and maintains proper encapsulation.
36-36: Please clarify the relationship between Project.core and asset visualizer flag.
The PR description mentions adding a visualizer flag to assets, but these changes introduce a core field to the Project struct. Could you please clarify:
- How does the
Project.corefield relate to the asset visualizer feature? - Are there any dependencies or interactions between projects and assets that should be documented?
Let's check for any relationships between projects and assets:
Also applies to: 109-110
✅ Verification successful
Let me gather more information about the asset and project structures to understand their relationship better.
The core field is a consistent pattern across both Project and Asset entities
Looking at the codebase, I can see that both Project and Asset structs have a core boolean field, and both provide a Core() method. This appears to be a deliberate architectural pattern rather than an inconsistency. The PR's changes align with this pattern by:
- Both entities (
AssetandProject) have identicalcorefield implementations - The field is used in scene building context (as seen in
scene/builder) - Both structs expose the field through a
Core()method with identical signatures
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any connections between projects and assets
rg -l 'asset.*project|project.*asset' --type go
# Search for any usage of the new core field
ast-grep --pattern 'core'
Length of output: 1922
Script:
#!/bin/bash
# Check the Project struct definition
ast-grep --pattern 'type Project struct {
$$$
}'
# Check the Asset struct definition
ast-grep --pattern 'type Asset struct {
$$$
}'
# Look for Core() method implementations
ast-grep --pattern 'func ($_ $_) Core() bool {
$$$
}'
Length of output: 10022
server/pkg/scene/builder/scene.go (1)
26-26: Verify the integration between scenes and assets.
The addition of the Core flag to scenes appears to be related to the asset management system changes mentioned in the PR objectives. Let's verify this integration:
Also applies to: 31-31
✅ Verification successful
Let me gather more information about the Core field usage and its potential impact.
Based on the results, I can now provide a final response:
The Core flag addition is a safe and well-integrated change
The Core boolean field is consistently implemented across multiple layers of the application:
- Scene builder (
server/pkg/scene/builder/scene.go) - GraphQL models (
internal/adapter/gql/gqlmodel/models_gen.go) - MongoDB documents (
internal/infrastructure/mongo/mongodoc/project.go) - Asset-related interfaces (
internal/usecase/interfaces/asset.go)
The field is properly serialized with JSON tags and follows the same pattern as other boolean flags in the scene structure (like EnableGA). There are no circular dependencies between scene and asset packages, and the integration appears to be clean.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how scenes and assets are integrated with respect to the Core flag
# Check for references to scene.Core in asset-related code
echo "Checking asset-scene integration..."
rg -l "scene.*Core" --type go
# Check for any potential circular dependencies
echo "Checking for circular dependencies..."
ast-grep --pattern 'import \(
$$$
"github.com/reearth/reearth/server/pkg/asset"
$$$
\)' server/pkg/scene
ast-grep --pattern 'import \(
$$$
"github.com/reearth/reearth/server/pkg/scene"
$$$
\)' server/pkg/asset
Length of output: 424
Script:
#!/bin/bash
# Check for any usage of the Core field in scene-related code
echo "Checking Core field usage in scene package..."
rg -A 3 "Core\s+bool" --type go
# Check for any references to Core in tests or implementations
echo "Checking Core references in tests and implementations..."
rg -A 3 "\.Core\s*=" --type go
# Check for any scene builder usage patterns
echo "Checking scene builder usage..."
rg -l "scene/builder" --type go
# Look for any validation or business logic related to Core
echo "Checking for Core-related logic..."
ast-grep --pattern 'func $_($$$ *Scene $$$) {
$$$
Core
$$$
}'
Length of output: 4620
server/internal/usecase/interactor/scene_test.go (1)
143-143: Verify alignment with PR objectives
The PR objectives mention adding a visualizer flag to assets, but this test adds a core field to scenes. This seems to be a different feature than what was described in the PR objectives.
Let's verify if there are any asset-related changes that should accompany this scene change:
server/internal/adapter/gql/resolver_mutation_project.go (2)
Line range hint 1-424: Ensure comprehensive testing for the Core field
Given the complexity of project operations, particularly in import/export functionality, please ensure:
- The Core field is properly serialized/deserialized during project export/import
- Existing projects without the Core field are handled gracefully
- End-to-end tests cover the Core field behavior
Let's verify the test coverage:
✅ Verification successful
Let me gather more information about the Core field testing by checking the test implementations in detail.
Based on the test coverage results, I can now generate the final response:
Core field is properly tested across different layers
The Core field has comprehensive test coverage:
- E2E tests verify both default (false) and explicit (true) Core values during project creation
- Integration tests in
project_test.goinclude Core field validation - The import/export functionality includes Core field in the test cases
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check test coverage for project-related operations
echo "Checking test coverage for project operations..."
# Look for project-related test files
fd "project.*test\.go$"
# Check for Core field testing
rg "TestCreate.*Project|TestExport.*Project|TestImport.*Project" -A 10
Length of output: 2193
Script:
#!/bin/bash
# Check for Core field testing in project tests
echo "Checking Core field testing in project tests..."
# Search for Core field in test files
rg "Core.*:.*true|Core.*:.*false" server/e2e/gql_project_test.go server/internal/usecase/interactor/project_test.go -B 5 -A 5
# Check test cases for project creation and import
rg "mutation CreateProject|mutation ImportProject" server/e2e/gql_project_test.go -B 5 -A 10
# Check project model tests
rg "Core" server/pkg/project/project_test.go -A 5 -B 5
Length of output: 2879
43-43: Verify the Core field implementation and its impact
The addition of the Core field to CreateProjectParam raises several considerations:
- The field is only added to project creation but not to project updates, which could lead to inconsistency in how the Core flag is managed throughout a project's lifecycle.
- There's a potential gap between the PR objectives (which focus on asset visualizer flags) and this change to project core functionality.
Let's verify the implementation consistency:
Consider the following architectural recommendations:
- If the Core flag is meant to be immutable, document this decision.
- If it should be mutable, add support in the UpdateProject mutation.
- Clarify the relationship between this Core flag and the asset visualizer functionality mentioned in the PR objectives.
server/e2e/gql_project_test.go (1)
Line range hint 21-85: Verify alignment with PR objectives
The PR objectives mention adding a "visualizer flag", but the test changes are implementing a "core" field. This seems to be a discrepancy that needs clarification.
The test implementation itself looks good, covering both:
- Default core value (false)
- Explicit core value (true)
Let's verify if there are any visualizer-related changes that should be included:
✅ Verification successful
Let me gather more context about the PR objectives and visualizer changes:
Let me check the changelog and readme for context about these changes:
Let me check the actual Project struct implementation:
Let me check the test file and GraphQL schema for the core field:
Based on the evidence gathered, I can now provide a final response:
The test implementation is correct and matches the schema
The test file correctly verifies the core field functionality:
- Tests default value (false) when creating a project without specifying core
- Tests explicit value (true) when creating a project with core specified
- The field is properly defined in the GraphQL schema as a Boolean
- The field exists in the Project struct implementation
The PR objectives mentioning "visualizer flag" likely refer to a different change, as the visualizer field is already present and required in the schema (visualizer: Visualizer!). The core field changes appear to be an independent feature.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for visualizer-related changes in the codebase
rg -g '!{node_modules,dist,build}/*' -l 'visualizer.*flag|flag.*visualizer'
# Search for related schema changes
fd -g '*.graphql' -x cat {} | grep -A 5 -B 5 'visualizer'
Length of output: 879
Script:
#!/bin/bash
# Check the PR description or recent commits for context
gh pr view --json title,body
# Look for visualizer-related changes in the test file
rg -C 5 "visualizer" server/e2e/gql_project_test.go
# Check if there are any other test files with visualizer changes
fd -g '*_test.go' -x rg -l "visualizer" {}
Length of output: 2259
Script:
#!/bin/bash
# Check README for visualizer flag mentions
cat server/README.md | grep -A 5 -B 5 "visualizer.*flag\|flag.*visualizer"
# Check CHANGELOG for recent changes
cat CHANGELOG.md | grep -A 5 -B 5 "visualizer\|core"
# Check the actual implementation of CreateProject
ast-grep --pattern 'type Project struct {
$$$
Visualizer $$$
Core $$$
$$$
}'
Length of output: 131549
Script:
#!/bin/bash
# Check Project struct implementation
ast-grep --pattern 'type Project struct {
$$$
}'
# Check if there are any tests related to the core field
rg -A 5 -B 5 "core.*Project" server/pkg/project/project_test.go
Length of output: 7173
Script:
#!/bin/bash
# Check test file for core field
cat server/e2e/gql_project_test.go
# Check GraphQL schema for core field
fd -g '*.graphql' -x cat {} | grep -A 5 -B 5 'core:'
Length of output: 14268
server/internal/usecase/interactor/project.go (1)
607-607: LGTM on Core field import handling
The Core field is properly handled during project import, consistent with other field imports.
server/internal/usecase/interactor/scene.go (3)
128-130: LGTM: Consistent visualizer selection logic across operations.
The changes maintain consistency in visualizer selection logic between scene creation and import operations, using Core() to determine the appropriate visualizer type.
Also applies to: 809-811
632-632: LGTM: Aligned BuildResult parameters with core flag changes.
The BuildResult parameter has been updated to use Core(), maintaining consistency with the rest of the codebase.
128-130: Verify impact on existing scenes.
The change from CoreSupport() to Core() might affect how existing scenes are handled. Please ensure that:
- Existing scenes maintain their visualizer settings
- The migration path for existing data is clear
Also applies to: 632-632, 809-811
server/e2e/gql_storytelling_test.go (1)
36-36: Verify default value for core flag
The core flag is hardcoded to true. Consider if this is the intended default value, as it might affect existing data.
✅ Verification successful
Let me gather more context about the core flag usage by checking its definition and implementation.
Core flag is correctly set to true for storytelling
Based on the codebase analysis, setting core: true in the storytelling test is correct and consistent with the implementation. The core flag is intentionally set to true in the storytelling builder (server/pkg/storytelling/story_bulider.go) and this test is verifying that behavior. This is not a default value that needs verification, but rather a deliberate implementation detail of the storytelling feature.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other instances where the core flag is set
rg "core\": (?:true|false)" --type go
Length of output: 308
Script:
#!/bin/bash
# Search for core flag definition and type declarations
ast-grep --pattern 'type $_ struct {
$$$
Core $_
$$$
}'
# Search for any core-related constants or configurations
rg "core.*=|const.*core" --type go -i
# Check for core flag in test setup or assertions
rg "core.*assert|assert.*core" --type go -i
Length of output: 13260
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
server/e2e/gql_asset_test.go (1)
103-141: Remove unnecessary __typename fields from the queryThe query includes
__typenamefields which are not used in the tests and add unnecessary overhead to the response. Consider removing them unless they serve a specific purpose.const GetAssetsQuery = `query GetAssets($teamId: ID!, $pagination: Pagination, $keyword: String, $sort: AssetSort) { assets(teamId: $teamId, pagination: $pagination, keyword: $keyword, sort: $sort) { edges { cursor node { id teamId name size url createdAt contentType core - __typename } - __typename } nodes { id teamId name size url createdAt contentType core - __typename } pageInfo { endCursor hasNextPage hasPreviousPage startCursor - __typename } totalCount - __typename } }`
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
server/e2e/gql_asset_test.go(1 hunks)
🔇 Additional comments (1)
server/e2e/gql_asset_test.go (1)
1-166: Verify the impact of flag name change and data migration needs
The PR objectives mention a "visualizer" flag, but the implementation uses "core". Let's verify the consistency across the codebase and identify data migration needs.
✅ Verification successful
The previous results show that the core flag is consistently used across the codebase, but we need to verify the schema definitions and any potential migration needs. The new script will help us confirm if there are any schema inconsistencies or if migration files are needed for the flag change.
Based on the results, I can now generate the final response:
The core flag is consistently used across the codebase
The codebase analysis shows that:
- The
coreflag is consistently used in GraphQL schema (server/gql/asset.graphql) - The flag is properly implemented in the Asset model and MongoDB queries
- There's no evidence of a "visualizer" flag in the asset-related code
- The MongoDB query includes a migration-friendly condition:
{"$or": [{"core": true}, {"core": {"$exists": false}}]}
The implementation is correct and consistent. The MongoDB query already handles the case where the flag might not exist in older documents, effectively treating them as core: true.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for inconsistencies between "visualizer" and "core" flags
# Check for "visualizer" usage in GraphQL schema files
echo "Checking GraphQL schema files for 'visualizer' field..."
rg -l "visualizer.*:.*Boolean" --type graphql
# Check for "core" usage in GraphQL schema files
echo "Checking GraphQL schema files for 'core' field..."
rg -l "core.*:.*Boolean" --type graphql
# Check for existing database queries using either flag
echo "Checking Go files for database queries..."
rg -B2 -A2 "(?:visualizer|core).*(?:true|false)" --type go
# Check for GraphQL queries/mutations using either flag
echo "Checking for GraphQL operations..."
rg -B2 -A2 "(?:visualizer|core).*:.*(?:true|false)" --type graphql
Length of output: 7443
Script:
#!/bin/bash
# Let's check for GraphQL schema files with a more generic approach
echo "Checking for GraphQL schema files..."
fd -e graphql -e gql
# Then check for schema definitions
echo "Checking schema content..."
fd -e graphql -e gql --exec cat {}
# Check for database schema or migration files
echo "Checking for migration files..."
fd -g "*migration*"
# Check for asset-related model definitions
echo "Checking asset model definitions..."
rg -B5 -A5 "type Asset struct" --type go
Length of output: 45905
hexaforce
changed the title
hexaforce
changed the title
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
server/e2e/gql_asset_test.go (2)
103-141: Update query to use 'core' and reduce repetitionThe GraphQL query needs improvements:
- Rename
coreSupporttocore- Use fragments to reduce field repetition between edges.node and nodes
+const AssetFragment = ` + fragment AssetFields on Asset { + id + teamId + name + size + url + createdAt + contentType + core + __typename + } +` + const GetAssetsQuery = `query GetAssets($teamId: ID!, $pagination: Pagination, $keyword: String, $sort: AssetSort) { assets(teamId: $teamId, pagination: $pagination, keyword: $keyword, sort: $sort) { edges { cursor node { - id - teamId - name - size - url - createdAt - contentType - coreSupport - __typename + ...AssetFields } __typename } nodes { - id - teamId - name - size - url - createdAt - contentType - coreSupport - __typename + ...AssetFields } pageInfo { endCursor hasNextPage hasPreviousPage startCursor __typename } totalCount __typename } - }` + } + ${AssetFragment}`
143-166: Improve asset retrieval helper with options and error handlingThe helper function needs improvements:
- Make pagination and sorting parameters configurable
- Add response error handling
+type AssetQueryOptions struct { + First int + SortField string + SortDir string +} + +type AssetQueryOption func(*AssetQueryOptions) + +func WithFirst(first int) AssetQueryOption { + return func(o *AssetQueryOptions) { + o.First = first + } +} + +func WithSort(field, direction string) AssetQueryOption { + return func(o *AssetQueryOptions) { + o.SortField = field + o.SortDir = direction + } +} + -func getAssets(e *httpexpect.Expect, teamId string) *httpexpect.Value { +func getAssets(e *httpexpect.Expect, teamId string, opts ...AssetQueryOption) *httpexpect.Value { + options := AssetQueryOptions{ + First: 20, + SortField: "DATE", + SortDir: "DESC", + } + + for _, opt := range opts { + opt(&options) + } + requestBody := GraphQLRequest{ OperationName: "GetAssets", Query: GetAssetsQuery, Variables: map[string]any{ "teamId": teamId, "pagination": map[string]any{ - "first": 20, + "first": options.First, }, "sort": map[string]string{ - "direction": "DESC", - "field": "DATE", + "direction": options.SortDir, + "field": options.SortField, }, }, } - return e.POST("/api/graphql"). + response := e.POST("/api/graphql"). WithHeader("Origin", "https://example.com"). WithHeader("X-Reearth-Debug-User", uID.String()). WithHeader("Content-Type", "application/json"). WithJSON(requestBody). Expect(). Status(http.StatusOK). JSON() + + // Validate response structure + response.Path("$.errors").Absent() + return response }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (16)
server/e2e/gql_asset_test.go(1 hunks)server/gql/asset.graphql(3 hunks)server/internal/adapter/gql/generated.go(12 hunks)server/internal/adapter/gql/gqlmodel/convert_asset.go(1 hunks)server/internal/adapter/gql/gqlmodel/models_gen.go(2 hunks)server/internal/adapter/gql/resolver_mutation_asset.go(1 hunks)server/internal/adapter/gql/resolver_mutation_property.go(1 hunks)server/internal/infrastructure/memory/asset.go(1 hunks)server/internal/infrastructure/mongo/asset.go(2 hunks)server/internal/infrastructure/mongo/mongodoc/asset.go(3 hunks)server/internal/usecase/interactor/asset.go(1 hunks)server/internal/usecase/interactor/asset_test.go(2 hunks)server/internal/usecase/interfaces/asset.go(1 hunks)server/pkg/asset/asset.go(2 hunks)server/pkg/asset/asset_test.go(4 hunks)server/pkg/asset/builder.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (15)
- server/gql/asset.graphql
- server/internal/adapter/gql/generated.go
- server/internal/adapter/gql/gqlmodel/convert_asset.go
- server/internal/adapter/gql/gqlmodel/models_gen.go
- server/internal/adapter/gql/resolver_mutation_asset.go
- server/internal/adapter/gql/resolver_mutation_property.go
- server/internal/infrastructure/memory/asset.go
- server/internal/infrastructure/mongo/asset.go
- server/internal/infrastructure/mongo/mongodoc/asset.go
- server/internal/usecase/interactor/asset.go
- server/internal/usecase/interactor/asset_test.go
- server/internal/usecase/interfaces/asset.go
- server/pkg/asset/asset.go
- server/pkg/asset/asset_test.go
- server/pkg/asset/builder.go
Overview
We will add a coresupport flag to assets to distinguish them.
What I've done
1. [GetAssets]The asset retrieval query will only return those with coresupport set to true.
var filter any = bson.M{ "team": id.String(), + "$or": []bson.M{ + {"coresupport": true}, + {"coresupport": bson.M{"$exists": false}}, + }, }Note: Data where coresupport is nil will be treated as false.
2. [CreateAsset]Added coresupport to the asset creation mutation.
input CreateAssetInput { teamId: ID! + coreSupport: Boolean! file: Upload! }3. When creating a new asset in UploadFileToProperty, coresupport will be set to true.
a, err := uc.Asset.Create(ctx, interfaces.CreateAssetParam{ WorkspaceID: prj[0].Workspace(), + coreSupport: true, File: gqlmodel.FromFile(&input.File), }, getOperator(ctx))What I haven't done
How I tested
Conducted in e2e testing
https://github.com/reearth/reearth-visualizer/blob/feat/exclusion-classic-assets/server/e2e/gql_asset_test.go
Which point I want you to review particularly
Past data is not taken into account. You must update the data directly.
Memo
1. You can retrieve assets by adding CoreFlag to the GET_ASSETS query:
export const GET_ASSETS = gql(` query GetAssets($teamId: ID!, $pagination: Pagination, $keyword: String, $sort: AssetSort) { assets(teamId: $teamId, pagination: $pagination, keyword: $keyword, sort: $sort) { edges { cursor node { ... + coreSupport } } nodes { ... + coreSupport } ... } } `);2. You need to specify coresupport flag in the CREATE_ASSET mutation:
3. generate graphql
Summary by CodeRabbit
New Features
CoreSupportfield for assets, enhancing asset management capabilities.Bug Fixes
Tests
CoreSupportfield in asset creation and retrieval.Documentation
CoreSupportfield.