feat(server): associate project asset #1410
Conversation
hexaforce
requested review from
airslice,
mkumbobeaty and
pyshx
as code owners
WalkthroughThis pull request implements extensive changes across asset and project management. Many functions and resolvers have been updated to accept a new file parameter (gateway.File) and an optional project identifier. GraphQL schemas, models, and queries now support filtering and mutating assets by projectId, and numerous seeder and test functions have been adjusted accordingly. Additionally, methods for asset and project export/import have been renamed and refactored to improve clarity and consistency, while minor formatting and logging improvements have been made. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant MutationResolver
participant AssetService
participant Repo
Client->>MutationResolver: UpdateAsset(input: {assetId, projectId})
MutationResolver->>AssetService: Convert input parameters
AssetService->>Repo: Update(assetID, projectId)
Repo-->>AssetService: Return updated asset
AssetService-->>MutationResolver: Build UpdateAssetPayload
MutationResolver-->>Client: Return payload with assetId and projectId
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
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: 4
🔭 Outside diff range comments (2)
web/src/beta/features/AssetsManager/hooks.ts (2)
184-184: Pass the projectId parameter to FindByWorkspace.The
projectIdparameter should be passed to theFindByWorkspacefunction to enable filtering assets by project.Apply this diff to fix the issue:
- assets, _, err := usecases(ctx).Asset.FindByWorkspace(ctx, prj.Workspace(), nil, nil, sort, page, getOperator(ctx)) + assets, _, err := usecases(ctx).Asset.FindByWorkspace(ctx, prj.Workspace(), &projectId, nil, sort, page, getOperator(ctx))
202-213: Add missing dependency to useCallback hook.The
projectIdparameter is used within the callback but is missing from the dependency array.Apply this diff to fix the issue:
const handleAssetsCreate = useCallback( async (files?: FileList) => { if (!files) return; await useCreateAssets({ teamId: workspaceId ?? "", projectId: projectId ?? "", file: files, coreSupport: true }); }, - [workspaceId, useCreateAssets] + [workspaceId, projectId, useCreateAssets] );🧰 Tools
🪛 GitHub Check: ci-web / ci
[failure] 212-212:
React Hook useCallback has a missing dependency: 'projectId'. Either include it or remove the dependency array🪛 GitHub Actions: ci
[error] 212-212: React Hook useCallback has a missing dependency: 'projectId'. Either include it or remove the dependency array.
🧹 Nitpick comments (12)
server/pkg/scene/builder/builder.go (2)
46-55: Consider documenting the exporter configuration.The exporter setup is crucial for layer processing but lacks documentation about its role and configuration. Consider adding comments explaining:
- The merger's role in layer/property loading
- The sealer's role in dataset/tag processing
133-159: Improve error message consistency and clarity.The error messages in BuildResult could be more descriptive and consistent:
- Error prefixes vary between "Fail" and "Fail build"
- Error messages use inconsistent separators
Consider applying this diff to improve error messages:
- return nil, errors.New("Fail buildScene :" + err.Error()) + return nil, fmt.Errorf("failed to build scene: %w", err) - return nil, errors.New("Fail buildStory :" + err.Error()) + return nil, fmt.Errorf("failed to build story: %w", err) - return nil, errors.New("Fail buildNLSLayers :" + err.Error()) + return nil, fmt.Errorf("failed to build NLS layers: %w", err) - return nil, errors.New("Fail buildLayerStyles :" + err.Error()) + return nil, fmt.Errorf("failed to build layer styles: %w", err)Also add the following import:
+ "fmt"server/e2e/gql_asset_test.go (2)
29-83: Repeated JSON checks hint at code duplication.Lines creating assets and verifying JSON structures look repetitive. You might consider factoring out a helper function that wraps creation of the asset plus validation of its fields to avoid duplication and reduce potential for inconsistencies.
216-243: Synchronized logic for file data creation.
createAssetFromFileDatamirrorscreateAsset. Consider deduplicating these if possible—using a shared helper for request building and file handling avoids divergences.server/internal/adapter/gql/gqlmodel/convert_asset.go (1)
12-16: Consider a more concise implementation.While the current implementation is correct, it could be simplified for better readability.
- var pid *ID - if project := a.Project(); project != nil { - pidValue := IDFrom(*a.Project()) - pid = &pidValue - } + var pid *ID + if p := a.Project(); p != nil { + id := IDFrom(*p) + pid = &id + }Also applies to: 22-22
server/internal/adapter/gql/resolver_mutation_asset.go (1)
19-26: Improve error variable naming for consistency.The implementation is correct, but consider improving error variable naming for consistency with the rest of the file.
- pidValue, err := gqlmodel.ToID[id.Project](*input.ProjectID) - if err != nil { + pidValue, err2 := gqlmodel.ToID[id.Project](*input.ProjectID) + if err2 != nil {Note: This matches the error variable naming pattern used in the RemoveAsset function (line 48).
Also applies to: 30-30
server/internal/infrastructure/memory/asset.go (1)
49-59: Consider extracting the filter condition for better maintainability.The implementation is correct, but the filter conditions could be extracted into a separate function to improve readability and maintainability.
func (r *Asset) FindByWorkspace(_ context.Context, wid accountdomain.WorkspaceID, pid *id.ProjectID, filter repo.AssetFilter) ([]*asset.Asset, *usecasex.PageInfo, error) { + filterAsset := func(v *asset.Asset) bool { + baseCondition := v.CoreSupport() && (filter.Keyword == nil || strings.Contains(v.Name(), *filter.Keyword)) + if pid != nil { + return v.Project() != nil && *v.Project() == *pid && baseCondition + } + return v.Workspace() == wid && baseCondition + } + result := r.data.FindAll(func(k id.AssetID, v *asset.Asset) bool { - if pid != nil { - return v.Project() != nil && *v.Project() == *pid && v.CoreSupport() && (filter.Keyword == nil || strings.Contains(v.Name(), *filter.Keyword)) - } - return v.Workspace() == wid && v.CoreSupport() && (filter.Keyword == nil || strings.Contains(v.Name(), *filter.Keyword)) + return filterAsset(v) })web/src/services/api/assetsApi.ts (1)
116-116: Consider implementing error tracking.The TODO comment indicates missing error tracking implementation.
Would you like me to help implement error tracking using a standard error tracking library?
server/e2e/common.go (2)
288-293: Consider handling additional JSON value types.The function only handles
map[string]interface{}but could be enhanced to support arrays and primitive types.Apply this diff to improve type handling:
func JSONEqRegexpValue(t *testing.T, actual *httpexpect.Value, expected string) bool { - if actualData, ok := actual.Raw().(map[string]interface{}); ok { + switch actualData := actual.Raw().(type) { + case map[string]interface{}, []interface{}, string, float64, bool: return JSONEqRegexpInterface(t, actualData, expected) + default: + return false } - return false }
305-319: Enhance error handling in ValueDump function.Consider logging errors when JSON marshaling fails and handling nil values.
Apply this diff to improve error handling:
func ValueDump(val *httpexpect.Value) { + if val == nil { + fmt.Println("Value is nil") + return + } raw := val.Raw() switch data := raw.(type) { case map[string]interface{}: if text, err := json.MarshalIndent(data, "", " "); err == nil { fmt.Println(string(text)) + } else { + fmt.Printf("Error marshaling map: %v\n", err) } case []interface{}: if text, err := json.MarshalIndent(data, "", " "); err == nil { fmt.Println(string(text)) + } else { + fmt.Printf("Error marshaling array: %v\n", err) } default: fmt.Println("Unsupported type:", reflect.TypeOf(raw)) } }server/internal/adapter/gql/resolver_mutation_project.go (2)
262-272: Improve error message clarity.The error messages could be more descriptive to help with debugging.
Apply this diff to improve error messages:
projectData, _ := unmarshalProject(tempData) prj, tx, err := usecases(ctx).Project.ImportProject(ctx, workspace.String(), projectData) if err != nil { - return nil, errors.New("Fail ImportProject :" + err.Error()) + return nil, fmt.Errorf("failed to import project (workspace: %s): %w", workspace.String(), err) } defer func() { if err2 := tx.End(ctx); err == nil && err2 != nil { err = err2 } }()
287-292: Improve error handling for asset upload.The error handling could be more descriptive and consistent with other error messages.
Apply this diff to improve error handling:
pid, err := id.ProjectIDFrom(prj.ID().String()) if err != nil { - return nil, errors.New("Fail UploadAssetFile :" + err.Error()) + return nil, fmt.Errorf("failed to parse project ID %s: %w", prj.ID().String(), err) } url, _, err := usecases(ctx).Asset.UploadAssetFile(ctx, realName, file, workspace, &pid)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
web/src/services/gql/__gen__/fragmentMatcher.jsonis excluded by!**/__gen__/**web/src/services/gql/__gen__/gql.tsis excluded by!**/__gen__/**web/src/services/gql/__gen__/graphql.tsis excluded by!**/__gen__/**
📒 Files selected for processing (82)
server/Makefile(3 hunks)server/e2e/common.go(3 hunks)server/e2e/dataset_export_test.go(0 hunks)server/e2e/gql_asset_test.go(7 hunks)server/e2e/gql_custom_property_test.go(1 hunks)server/e2e/gql_layer_test.go(0 hunks)server/e2e/gql_nlslayer_test.go(0 hunks)server/e2e/gql_project_import_test.go(1 hunks)server/e2e/gql_storytelling_test.go(2 hunks)server/e2e/gql_validate_geojson_test.go(1 hunks)server/e2e/seeder.go(1 hunks)server/gql/asset.graphql(3 hunks)server/gql/cluster.graphql(0 hunks)server/gql/dataset.graphql(0 hunks)server/gql/layer.graphql(0 hunks)server/gql/plugin.graphql(2 hunks)server/gql/property.graphql(0 hunks)server/gql/scene.graphql(1 hunks)server/gql/tag.graphql(0 hunks)server/gql/workspace.graphql(2 hunks)server/gqlgen.yml(0 hunks)server/internal/adapter/gql/context.go(0 hunks)server/internal/adapter/gql/gqldataloader/dataloader.go(0 hunks)server/internal/adapter/gql/gqldataloader/datasetloader_gen.go(0 hunks)server/internal/adapter/gql/gqldataloader/datasetschemaloader_gen.go(0 hunks)server/internal/adapter/gql/gqldataloader/taggrouploader_gen.go(0 hunks)server/internal/adapter/gql/gqldataloader/tagitemloader_gen.go(0 hunks)server/internal/adapter/gql/gqldataloader/tagloader_gen.go(0 hunks)server/internal/adapter/gql/gqlmodel/convert.go(0 hunks)server/internal/adapter/gql/gqlmodel/convert_asset.go(1 hunks)server/internal/adapter/gql/gqlmodel/convert_dataset.go(0 hunks)server/internal/adapter/gql/gqlmodel/convert_layer.go(0 hunks)server/internal/adapter/gql/gqlmodel/convert_plugin.go(0 hunks)server/internal/adapter/gql/gqlmodel/convert_scene.go(0 hunks)server/internal/adapter/gql/gqlmodel/convert_tag.go(0 hunks)server/internal/adapter/gql/gqlmodel/models.go(0 hunks)server/internal/adapter/gql/gqlmodel/models_gen.go(6 hunks)server/internal/adapter/gql/loader.go(0 hunks)server/internal/adapter/gql/loader_asset.go(2 hunks)server/internal/adapter/gql/loader_dataset.go(0 hunks)server/internal/adapter/gql/loader_tag.go(0 hunks)server/internal/adapter/gql/resolver_dataset.go(0 hunks)server/internal/adapter/gql/resolver_dataset_schema.go(0 hunks)server/internal/adapter/gql/resolver_layer.go(0 hunks)server/internal/adapter/gql/resolver_mutation_asset.go(2 hunks)server/internal/adapter/gql/resolver_mutation_dataset.go(0 hunks)server/internal/adapter/gql/resolver_mutation_layer.go(0 hunks)server/internal/adapter/gql/resolver_mutation_project.go(3 hunks)server/internal/adapter/gql/resolver_mutation_scene.go(0 hunks)server/internal/adapter/gql/resolver_mutation_tag.go(0 hunks)server/internal/adapter/gql/resolver_property.go(0 hunks)server/internal/adapter/gql/resolver_property_test.go(0 hunks)server/internal/adapter/gql/resolver_query.go(1 hunks)server/internal/adapter/gql/resolver_scene.go(0 hunks)server/internal/adapter/gql/resolver_tag.go(0 hunks)server/internal/adapter/gql/resolver_team.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(4 hunks)server/internal/usecase/interactor/layer.go(0 hunks)server/internal/usecase/interactor/scene.go(0 hunks)server/internal/usecase/interactor/scene_test.go(0 hunks)server/internal/usecase/interfaces/asset.go(2 hunks)server/internal/usecase/repo/asset.go(1 hunks)server/pkg/asset/asset.go(2 hunks)server/pkg/asset/builder.go(1 hunks)server/pkg/asset/id.go(1 hunks)server/pkg/layer/encoding/exporter.go(1 hunks)server/pkg/layer/layerops/processor_test.go(0 hunks)server/pkg/scene/builder/builder.go(1 hunks)server/pkg/scene/builder/builder_test.go(0 hunks)server/pkg/scene/builder/encoder.go(1 hunks)server/pkg/scene/builder/encoder_test.go(0 hunks)server/pkg/scene/builder/scene.go(1 hunks)web/src/beta/features/AssetsManager/hooks.ts(2 hunks)web/src/beta/features/AssetsManager/index.tsx(3 hunks)web/src/beta/features/Dashboard/ContentsContainer/Assets/index.tsx(2 hunks)web/src/services/api/assetsApi.ts(2 hunks)web/src/services/api/propertyApi/utils.ts(1 hunks)web/src/services/api/sceneApi.ts(0 hunks)web/src/services/gql/fragments/dataset.ts(0 hunks)
⛔ Files not processed due to max files limit (5)
- web/src/services/gql/fragments/index.ts
- web/src/services/gql/fragments/layer.ts
- web/src/services/gql/fragments/property.ts
- web/src/services/gql/queries/asset.ts
- web/src/services/gql/queries/scene.ts
💤 Files with no reviewable changes (45)
- server/internal/adapter/gql/context.go
- server/internal/adapter/gql/gqlmodel/convert_scene.go
- server/internal/adapter/gql/gqldataloader/dataloader.go
- web/src/services/gql/fragments/dataset.ts
- server/internal/adapter/gql/gqlmodel/convert.go
- server/internal/usecase/interactor/scene_test.go
- server/pkg/scene/builder/builder_test.go
- web/src/services/api/sceneApi.ts
- server/pkg/layer/layerops/processor_test.go
- server/internal/adapter/gql/gqlmodel/convert_plugin.go
- server/gql/property.graphql
- server/internal/adapter/gql/resolver_property_test.go
- server/pkg/scene/builder/encoder_test.go
- server/internal/usecase/interactor/scene.go
- server/gqlgen.yml
- server/internal/adapter/gql/gqlmodel/convert_layer.go
- server/gql/cluster.graphql
- server/internal/adapter/gql/resolver_scene.go
- server/e2e/gql_layer_test.go
- server/internal/adapter/gql/resolver_layer.go
- server/internal/adapter/gql/gqlmodel/convert_tag.go
- server/e2e/gql_nlslayer_test.go
- server/e2e/dataset_export_test.go
- server/internal/usecase/interactor/layer.go
- server/gql/dataset.graphql
- server/internal/adapter/gql/gqlmodel/convert_dataset.go
- server/internal/adapter/gql/gqlmodel/models.go
- server/internal/adapter/gql/resolver_mutation_scene.go
- server/internal/adapter/gql/resolver_mutation_tag.go
- server/internal/adapter/gql/loader.go
- server/internal/adapter/gql/resolver_mutation_layer.go
- server/internal/adapter/gql/resolver_dataset.go
- server/internal/adapter/gql/resolver_mutation_dataset.go
- server/internal/adapter/gql/resolver_dataset_schema.go
- server/internal/adapter/gql/gqldataloader/taggrouploader_gen.go
- server/internal/adapter/gql/gqldataloader/tagloader_gen.go
- server/internal/adapter/gql/gqldataloader/tagitemloader_gen.go
- server/internal/adapter/gql/resolver_tag.go
- server/gql/tag.graphql
- server/internal/adapter/gql/resolver_property.go
- server/internal/adapter/gql/gqldataloader/datasetloader_gen.go
- server/gql/layer.graphql
- server/internal/adapter/gql/loader_tag.go
- server/internal/adapter/gql/gqldataloader/datasetschemaloader_gen.go
- server/internal/adapter/gql/loader_dataset.go
✅ Files skipped from review due to trivial changes (1)
- server/pkg/scene/builder/encoder.go
🧰 Additional context used
📓 Learnings (1)
server/internal/adapter/gql/resolver_mutation_project.go (1)
Learnt from: hexaforce
PR: reearth/reearth-visualizer#1151
File: server/internal/adapter/gql/resolver_mutation_project.go:215-217
Timestamp: 2024-11-12T15:21:04.150Z
Learning: In the project import functionality, the file names are unique and can be safely replaced uniformly using `bytes.Replace`.
🪛 GitHub Actions: ci
web/src/beta/features/AssetsManager/hooks.ts
[error] 212-212: React Hook useCallback has a missing dependency: 'projectId'. Either include it or remove the dependency array.
server/e2e/seeder.go
[warning] 28-28: var sID is unused.
🪛 golangci-lint (1.62.2)
server/e2e/seeder.go
28-28: var sID is unused
(unused)
🪛 GitHub Check: ci-server / ci-server-lint
server/e2e/seeder.go
[failure] 28-28:
var sID is unused (unused)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Redirect rules - reearth-web
- GitHub Check: Header rules - reearth-web
- GitHub Check: Pages changed - reearth-web
🔇 Additional comments (55)
server/pkg/scene/builder/scene.go (2)
11-25: LGTM! Clean removal of deprecated fields.The removal of Layers, Tags, and Clusters fields from the sceneJSON struct aligns with the broader refactoring effort to simplify the data model.
27-40: LGTM! Method signature updated correctly.The sceneJSON method has been updated to remove the layers parameter while maintaining all essential functionality.
server/e2e/gql_project_import_test.go (1)
80-82: LGTM! Clean formatting adjustment.The formatting change improves code readability without affecting functionality.
server/gql/scene.graphql (2)
1-17: LGTM! Scene type simplified effectively.The removal of deprecated fields (rootLayerId, rootLayer, datasetSchemas, tagIds, tags, clusters) streamlines the Scene type while maintaining all essential functionality.
50-52: LGTM! Query formatting improved.The formatting change in the Query type extension improves readability.
server/gql/plugin.graphql (2)
20-30: LGTM! Removed deprecated extension type.The removal of the Cluster extension type is consistent with the broader refactoring effort.
99-109: LGTM! Improved formatting.The formatting changes in the Query and Mutation type extensions improve code readability.
server/e2e/seeder.go (1)
1-91: Verify alignment with PR objectives.The changes in this file primarily involve removing dataset-related code, but the PR objectives focus on adding project asset association functionality. Please verify if additional test setup code is needed to cover the new project asset association feature.
Would you like me to help generate test setup code for the new project asset association feature?
🧰 Tools
🪛 golangci-lint (1.62.2)
28-28: var
sIDis unused(unused)
🪛 GitHub Check: ci-server / ci-server-lint
[failure] 28-28:
varsIDis unused (unused)🪛 GitHub Actions: ci
[warning] 28-28: var
sIDis unused.server/internal/adapter/gql/gqlmodel/models_gen.go (5)
138-138: Ensure optional field is properly handled.
The addition ofProjectIDas an optional field aligns with the PR objectives for associating assets with projects. Please verify that usage in resolvers or services properly accounts for the possibility of a nil project reference.
186-186: Confirm validation forprojectIdin creation flow.
AddingprojectIdtoCreateAssetInputis consistent with the new association feature. Ensure relevant logic (e.g., create mutation) properly validates whenprojectIdis omitted or points to a non-existent project.
581-588: Struct changes look good.
These newly introduced fields inMergedPropertyFieldseem consistent with the merged property concept. No issues detected.
919-921: New fields appear consistent.
These new fields inPropertyFieldLinkcorrectly reflect optional vs. required attributes.
1834-1834: Enum coverage update looks valid.
IncludingPluginExtensionTypeInfoboxBlockin theIsValidswitch branch is appropriate.server/pkg/scene/builder/builder.go (1)
176-181: Verify layer export functionality after encoder removal.The removal of the encoder and direct usage of the exporter changes how layers are processed. While the code looks correct, we should verify that:
- All layer data is properly exported via
ExportLayerByID- The scene JSON generation receives all necessary layer information through the context
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for any direct encoder usage that might have been missed rg -A 5 "encoder\." server/pkg/scene/ # Check for any layer-related tests that might need updates fd -e go -e test.go -x rg -l "TestBuild.*Scene|TestExport.*Layer" {} \;Length of output: 154
🏁 Script executed:
#!/bin/bash # Check the definition of ExportLayerByID in the repository to inspect how it handles layer data. rg -A 20 "func.*ExportLayerByID" -n . # Check the implementation of sceneJSON within the Builder to verify that it processes the exported layer data correctly. rg -A 20 "func (b *Builder) sceneJSON" -n server/pkg/scene/builder/builder.goLength of output: 1450
Layer export functionality verified post-encoder removal.
The exported layer data is now processed via
b.exporter.ExportLayerByID, which internally merges and encodes the layer correctly. No residual usage of the encoder was found, and the existing tests (e.g., inserver/pkg/property/builder_test.go) confirm that scene construction viasceneJSONreceives the necessary layer data.server/e2e/gql_asset_test.go (7)
16-18: Rename is appropriate but consider clarifying doc comments.Renaming the test function to
TestCoreSupportGetAssetsaligns better with its purpose of testing core support for created assets. Consider adding a short comment describing the test's high-level objective for future maintainers.
113-113: Enforce domain constraints on workspace searching.When calling
FindByWorkspace()with anilproject filter, ensure the rest of the domain logic still enforces correctness (like workspace membership, valid statuses, etc.).
121-122: Verify consistency of coreSupport in results.The test strictly checks
coreSupport == truefor every returned asset. If some assets are intended to havefalse, confirm this query or filter is correct; otherwise, the test might mask unexpected data.
129-171: Test covers multiple project associations thoroughly.The
TestAssociateProjectGetAssetsfunction offers good coverage for assets without a project and with different projects. Consider adding error-path checks (e.g., invalid project IDs) to bolster reliability.
173-190: GraphQL mutation structure looks correct.Adding
projectIdtoCreateAssetInputis consistent with the PR goal. Confirm all references to this mutation in the codebase or frontend are updated to avoid signature mismatches.
192-214: OptionalprojectIdparameter is handled properly.Allowing
nilforprojectIdis logical for workspace-level assets. Validating it in the create function can prevent invalid references (e.g., unknown project).
245-308: Query is comprehensive but watch performance.The
getAssetsquery includes pagination, filtering, and many fields. While thorough, ensure performance is acceptable under large data loads; you might consider smaller queries or only requesting necessary fields in certain contexts.server/e2e/gql_custom_property_test.go (1)
347-762: Extensive multi-fragment GraphQL query is well-structured.Using fragments for
PropertyFragment,WidgetAlignSystemFragment, etc., helps keep the query organized. However, it's quite large. If performance or clarity issues arise, consider splitting queries by domain area or introducing smaller resolvers.server/e2e/gql_storytelling_test.go (2)
735-741: Refactored configuration for test initialization.Separating configuration into a variable
cbefore callingStartServerAndReposimproves readability and maintainability. Ensure any additional test config toggles remain consistent across all tests.
770-840: Confirm published JSON matches updated schema.The large JSON structure checks many fields, including new properties like
coreSupport. If your schema evolves, remember to adjust these expected values to prevent brittle tests. Also, consider verifying the final published file includes all required fields (e.g.,property.tiles).server/internal/usecase/repo/asset.go (1)
20-20: LGTM! The interface change aligns with PR objectives.The updated
FindByWorkspacemethod signature correctly adds project filtering capability while maintaining backward compatibility with an optional project ID parameter.server/pkg/layer/encoding/exporter.go (2)
37-42: Verify the intentional removal of encoding functionality.The
Encodemethod now returnsnilwithout performing any operations. This could lead to silent failures as callers expect encoding to happen.Please confirm:
- Is this change intentional?
- Should we add a warning log or error return to indicate that encoding is not implemented?
- Should we update the method documentation to reflect this change?
10-13: Consider documenting the removal of encoding functionality.The
Encoderfield has been removed from theExporterstruct, significantly changing its behavior.Let's verify if any callers of this struct need to be updated:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for usages of Exporter struct and Encode method rg -A 5 "Exporter.*{|Encode\(" --type goLength of output: 156
🏁 Script executed:
#!/bin/bash # Search for the Exporter struct definition to confirm its current fields rg -A 5 "type Exporter struct" --type go # Search for any calls to an Encode method (which might be referencing the removed Encoder functionality) rg -A 5 "\.Encode\(" --type go # Additionally, search for any references to the removed 'Encoder' field in usages of Exporter rg -A 5 "Exporter.*Encoder" --type goLength of output: 5000
No caller updates are necessary since the new
Encodemethod is properly used and there are no references to the previously removedEncoderfield.
- The grep output confirms that the
Exporterstruct now only contains theMergerandSealerfields.- All invocations of encoding functionality directly call the
Encodemethod on theExporterinstance.- There are no lingering references to an
Encoderfield anywhere in the codebase.It would still be beneficial to add a documentation note describing the removal of the
Encoderfield and how encoding is now handled.server/pkg/asset/id.go (1)
10-10: LGTM! Well-structured ID type definitions.The new ProjectID type and associated helper functions follow consistent naming patterns and maintain code organization.
Also applies to: 14-14, 18-18, 22-22, 26-26
web/src/beta/features/Dashboard/ContentsContainer/Assets/index.tsx (1)
7-10: LGTM! Clean implementation of project association in the UI.The changes correctly implement project filtering while maintaining backward compatibility with the optional
projectIdprop.Also applies to: 12-12, 29-31
server/pkg/asset/asset.go (1)
18-18: LGTM! Clean implementation of project association.The optional project field and its getter method are well-implemented, following the established patterns in the codebase.
Also applies to: 34-36
server/internal/usecase/interfaces/asset.go (1)
28-28: Verify implementations of the updated interface methods.The interface changes look good, but please ensure all implementations of the Asset interface have been updated to match these changes:
- FindByWorkspace with new ProjectID parameter
- UploadAssetFile with new ProjectID parameter
Also applies to: 39-39, 42-42
server/internal/adapter/gql/resolver_team.go (1)
27-34: LGTM! The changes correctly implement project filtering for team assets.The updated
Assetsmethod signature and implementation properly handle the optional project ID parameter, aligning with the PR objectives.server/pkg/asset/builder.go (1)
57-60: LGTM! The Project method follows the builder pattern consistently.The implementation maintains the fluent interface pattern used throughout the builder, correctly handling the optional project ID.
server/internal/infrastructure/mongo/mongodoc/asset.go (3)
17-17: LGTM! Project field added to AssetDocument.The field is correctly defined as an optional string pointer, consistent with MongoDB document patterns.
35-40: LGTM! Project ID conversion is properly handled in NewAsset.The implementation correctly handles the optional project ID conversion to string format for MongoDB storage.
65-72: LGTM! Model method properly handles project ID conversion.The implementation includes proper error handling for project ID conversion from MongoDB format.
server/internal/adapter/gql/loader_asset.go (2)
39-54: LGTM! Project ID handling is properly implemented.The implementation includes proper error handling for project ID conversion and maintains consistency with other ID handling patterns.
60-66: Nice optimization of edge creation!Pre-allocating the edges slice and using an indexed loop improves performance by avoiding slice growth operations.
server/e2e/gql_validate_geojson_test.go (1)
186-186: LGTM! Test updated correctly for new API signature.The test has been properly updated to include the new
projectIdparameter as nil, maintaining backward compatibility.web/src/services/api/assetsApi.ts (1)
74-76: LGTM! Correctly added projectId to asset creation.The projectId parameter has been properly integrated into the asset creation flow.
server/internal/usecase/interactor/asset.go (1)
39-51: Consider adding project ID validation.The FindByWorkspace implementation should validate that the project belongs to the workspace when a project ID is provided.
server/internal/infrastructure/mongo/asset.go (2)
67-67: LGTM! Method signature updated to support project-based filtering.The addition of
projectIdparameter aligns with the PR objective of associating assets with projects.
72-80: LGTM! Filter construction handles both project and workspace scenarios.The implementation correctly:
- Uses project ID for filtering when provided
- Falls back to workspace ID when project ID is not provided
server/internal/adapter/gql/resolver_query.go (1)
15-16: LGTM! GraphQL resolver updated to support project-based asset filtering.The changes correctly propagate the
projectIdparameter to the asset loader.web/src/beta/features/AssetsManager/index.tsx (2)
24-24: LGTM! Type definition updated to support project association.The optional
projectIdprop allows the component to be used in both project-specific and workspace-wide contexts.
37-37: LGTM! Component correctly handles the projectId prop.The implementation properly:
- Destructures
projectIdfrom props- Passes it to
useHooksfor handling asset operationsAlso applies to: 69-69
web/src/beta/features/AssetsManager/hooks.ts (1)
30-31: LGTM!The optional
projectIdparameter has been correctly added to both the function signature and type definition.Also applies to: 38-39
server/gql/asset.graphql (3)
5-5: LGTM!The optional
projectIdfield has been correctly added to the Asset type.
24-24: LGTM!The optional
projectIdfield has been correctly added to the CreateAssetInput type.
65-65: LGTM!The optional
projectIdparameter has been correctly added to the assets query.server/gql/workspace.graphql (3)
8-14: Enhance Asset Query with Optional 'projectId' Parameter
The assets field now accepts an optionalprojectIdparameter, which will enable filtering assets by their associated project. Please ensure that the corresponding resolver logic is updated to handle this parameter.
15-21: Consistent Formatting for Projects Field
The parameters for the projects field have been reformatted for improved readability. There are no functional changes, so this update is acceptable.
121-123: Refactored Mutation Field for removeMemberFromTeam
The formatting for theremoveMemberFromTeammutation has been updated to a multi-line structure. This change improves readability while preserving functionality.server/Makefile (2)
85-90: New 'run-reset' Target Added
Therun-resettarget has been introduced to reset the MongoDB environment by stopping the container (reearth-visualizer-reearth-mongo-1), removing old data directories, and restarting the database along with initializing a mock user. Please verify that the Docker container name is consistent with your deployment configuration. As a future improvement, consider adding error handling (e.g., checking if the container is running before attempting to stop it) to make the process more robust.
112-112: Updated .PHONY Declaration
Includingrun-resetin the .PHONY declaration ensures that the target is always executed correctly. This update is correct and helps prevent potential issues with file conflicts.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
web/src/services/gql/queries/asset.ts (1)
58-66: Consider adding error handling to the UPDATE_ASSET mutation.The new mutation looks good, but it would be beneficial to include error information in the response.
export const UPDATE_ASSET = gql(` mutation UpdateAsset($assetId: ID!, $projectId: ID) { updateAsset(input: { assetId: $assetId projectId: $projectId }) { assetId projectId + errors { + message + path + } __typename } } `);server/e2e/common.go (2)
288-293: Enhance error handling in JSONEqRegexpValue.The function could be more robust by handling additional types and providing better error feedback.
func JSONEqRegexpValue(t *testing.T, actual *httpexpect.Value, expected string) bool { + if actual == nil { + t.Error("actual value is nil") + return false + } if actualData, ok := actual.Raw().(map[string]interface{}); ok { return JSONEqRegexpInterface(t, actualData, expected) } + t.Errorf("expected map[string]interface{}, got %T", actual.Raw()) return false }
305-319: Improve error handling in ValueDump.The function silently ignores marshaling errors and could provide better feedback.
func ValueDump(val *httpexpect.Value) { + if val == nil { + fmt.Println("Error: nil value") + return + } raw := val.Raw() switch data := raw.(type) { case map[string]interface{}: if text, err := json.MarshalIndent(data, "", " "); err == nil { fmt.Println(string(text)) + } else { + fmt.Printf("Error marshaling map: %v\n", err) } case []interface{}: if text, err := json.MarshalIndent(data, "", " "); err == nil { fmt.Println(string(text)) + } else { + fmt.Printf("Error marshaling array: %v\n", err) } default: fmt.Println("Unsupported type:", reflect.TypeOf(raw)) } }server/e2e/gql_asset_test.go (1)
268-271: Remove commented code.The commented code block is unnecessary and should be removed for better code cleanliness.
- // "input": map[string]interface{}{ - // "assetId": assetId, - // },
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
web/src/services/gql/__gen__/gql.tsis excluded by!**/__gen__/**web/src/services/gql/__gen__/graphql.tsis excluded by!**/__gen__/**
📒 Files selected for processing (11)
server/e2e/common.go(4 hunks)server/e2e/gql_asset_test.go(7 hunks)server/gql/asset.graphql(5 hunks)server/internal/adapter/gql/gqlmodel/models_gen.go(7 hunks)server/internal/adapter/gql/resolver_mutation_asset.go(3 hunks)server/internal/usecase/interactor/asset.go(6 hunks)server/internal/usecase/interfaces/asset.go(2 hunks)server/pkg/asset/asset.go(2 hunks)web/src/beta/features/AssetsManager/hooks.ts(2 hunks)web/src/beta/ui/fields/AssetField/useAssetUpload.ts(1 hunks)web/src/services/gql/queries/asset.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web/src/beta/features/AssetsManager/hooks.ts
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Redirect rules - reearth-web
- GitHub Check: Header rules - reearth-web
- GitHub Check: Pages changed - reearth-web
🔇 Additional comments (14)
server/internal/adapter/gql/gqlmodel/models_gen.go (5)
906-915: Use a consistent type forIDinPropertyField.The
IDfield in thePropertyFieldstruct is declared asstring, while other ID fields use the customIDtype. For consistency and type safety, consider changing it to use theIDtype.
138-138: Addition ofProjectIDtoAssetstruct.The optional
ProjectIDfield has been added to theAssetstruct to associate assets with projects. Ensure that this addition aligns with the overall data model and that any serialization or validation logic accounts for this new field.
186-186: UpdateCreateAssetInputwith optionalprojectId.The
CreateAssetInputstruct now includes an optionalprojectIdfield. This allows for the creation of assets associated with a specific project. Confirm that all GraphQL mutations using this input handle the new field correctly and that appropriate validations are in place.
1330-1333: IntroduceUpdateAssetInputwith optionalprojectId.A new
UpdateAssetInputtype with an optionalprojectIdfield has been added. This enables updating an asset's associated project. Verify that the update operations correctly handle changes to theprojectIdand maintain data integrity.
581-589: Verify the correctness of added fields inMergedPropertyField.New fields have been added to the
MergedPropertyFieldstruct, includingSchemaID,FieldID,Value,Type,Links,Overridden,Schema, andField. Ensure that these fields are properly initialized and integrated within the system's property merging logic.server/pkg/asset/asset.go (2)
18-18: Addition of optionalprojectfield toAssetstruct.The
Assetstruct now includes an optionalprojectfield of type*ProjectID, allowing assets to be associated with a project. Verify that this new field is correctly handled throughout the codebase, especially during asset creation and serialization.
34-40: ImplementProject()andSetProject()methods inAsset.The new methods
Project()andSetProject()provide access to theprojectfield. Ensure that these methods are used appropriately and that they do not introduce unintended side effects. Additionally, confirm that any concurrency considerations are addressed if the asset may be accessed by multiple goroutines.server/internal/usecase/interfaces/asset.go (2)
28-28: IncludeProjectIDinCreateAssetParam.The
CreateAssetParamstruct now includes aProjectIDfield. Ensure that this parameter is correctly utilized in asset creation workflows and that necessary validations are in place when associating an asset with a project.
41-41: AddUpdatemethod toAssetinterface.A new
Updatemethod has been added to theAssetinterface to allow updating an asset'sProjectID. Ensure that this method is implemented consistently across all implementations of the interface and that it properly handles concurrent access and error conditions.web/src/services/gql/queries/asset.ts (1)
4-5: LGTM! The query parameters have been updated correctly.The
GET_ASSETSquery has been enhanced with an optionalprojectIdparameter, allowing for project-specific asset filtering.server/internal/adapter/gql/resolver_mutation_asset.go (1)
19-26: Add validation for project-workspace relationship.The CreateAsset resolver should verify that the provided project belongs to the workspace before creating the asset.
server/internal/usecase/interactor/asset.go (1)
108-127: Consider adding transaction rollback on error.The Update method uses a transaction but doesn't explicitly handle rollback on error. While the transaction wrapper might handle this, it would be good to verify the behavior.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Check how transactions are handled in the codebase rg "Transaction\(\)" -A 10 --type goLength of output: 7950
Transaction Rollback Is Handled by the Transaction Wrapper
The transaction infrastructure (using
Usecase().Transaction()in combination withRun2) automatically rolls back the transaction upon error. This behavior is confirmed by tests inserver/internal/usecase/interactor/usecase_test.gowhere error cases assert that the transaction is not committed.server/e2e/gql_asset_test.go (1)
131-195: LGTM! Comprehensive test coverage for project association.The test cases thoroughly cover:
- Asset creation with project association
- Asset retrieval by project
- Asset movement between projects
- Asset removal from projects
- Asset deletion
server/gql/asset.graphql (1)
1-87: LGTM! Well-structured GraphQL schema changes.The changes consistently implement project association across all relevant types and operations:
- Asset type includes optional projectId
- Input types properly handle projectId
- Query and mutation operations support project filtering
hexaforce
force-pushed
the
feat/associate-project-asset
branch
from
d6b5ab7 to
ae2ce12
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
server/internal/adapter/gql/gqlmodel/models_gen.go (1)
906-915: 🛠️ Refactor suggestionUse consistent type for
IDinPropertyFieldThe
IDfield in thePropertyFieldstruct is declared asstring, whereas other ID fields use the customIDtype. For consistency and improved type safety, consider changing theIDfield to use theIDtype.Apply this diff to align the
IDtype:- ID string `json:"id"` + ID ID `json:"id"`
🧹 Nitpick comments (6)
server/internal/adapter/gql/resolver_team.go (1)
27-34: Consider documenting pagination parameters.The implementation correctly handles the new projectID parameter, but the pagination parameters (first, last, after, before) could benefit from documentation explaining their usage and limitations.
server/internal/infrastructure/mongo/asset.go (1)
67-103: Improve filter construction for better maintainability.The current implementation has separate branches for project and team filtering. Consider using a more maintainable approach by building the filter dynamically.
func (r *Asset) FindByWorkspace(ctx context.Context, id accountdomain.WorkspaceID, projectId *id.ProjectID, uFilter repo.AssetFilter) ([]*asset.Asset, *usecasex.PageInfo, error) { if !r.f.CanRead(id) { return nil, usecasex.EmptyPageInfo(), nil } - filter := bson.M{ - "coresupport": true, - } - - if projectId != nil { - filter["project"] = projectId.String() - } else { - filter["team"] = id.String() - } + filter := bson.M{"coresupport": true} + + // Build filter based on project or team + if projectId != nil { + filter["project"] = projectId.String() + filter["team"] = id.String() // Always include team check for security + } else { + filter["team"] = id.String() + }server/e2e/common.go (2)
288-294: Enhance JSON comparison to handle more types.The function only handles map[string]interface{} but should support arrays and other JSON types for better test coverage.
func JSONEqRegexpValue(t *testing.T, actual *httpexpect.Value, expected string) bool { - if actualData, ok := actual.Raw().(map[string]interface{}); ok { - return JSONEqRegexpInterface(t, actualData, expected) + switch actualData := actual.Raw().(type) { + case map[string]interface{}, []interface{}: + return JSONEqRegexpInterface(t, actualData, expected) + default: + t.Errorf("Unsupported type for JSON comparison: %T", actualData) + return false } - return false }
305-319: Reduce code duplication in JSON marshaling.The JSON marshaling logic is duplicated for map and slice types. Consider extracting it to a helper function.
+func marshalAndPrint(data interface{}) { + if text, err := json.MarshalIndent(data, "", " "); err == nil { + fmt.Println(string(text)) + } +} + func ValueDump(val *httpexpect.Value) { raw := val.Raw() switch data := raw.(type) { case map[string]interface{}: - if text, err := json.MarshalIndent(data, "", " "); err == nil { - fmt.Println(string(text)) - } + marshalAndPrint(data) case []interface{}: - if text, err := json.MarshalIndent(data, "", " "); err == nil { - fmt.Println(string(text)) - } + marshalAndPrint(data) default: fmt.Println("Unsupported type:", reflect.TypeOf(raw)) } }server/internal/adapter/gql/resolver_mutation_project.go (1)
287-292: Consider enhancing error message.The error message could be more descriptive to help with debugging.
- return nil, errors.New("Fail UploadAssetFile :" + err.Error()) + return nil, errors.New("Failed to parse project ID during asset upload: " + err.Error())server/internal/adapter/gql/gqlmodel/models_gen.go (1)
1844-1845: Ensure consistent string values inPluginExtensionTypeconstantsThe new
PluginExtensionTypeconstants have mixed-case string values (e.g.,"Story","StoryPage"), while existing constants use uppercase (e.g.,"PRIMITIVE","WIDGET"). For consistency and to prevent potential issues during serialization, consider updating the string values of the new constants to uppercase.Apply this diff to standardize the string values:
- PluginExtensionTypeStory PluginExtensionType = "Story" - PluginExtensionTypeStoryPage PluginExtensionType = "StoryPage" - PluginExtensionTypeStoryBlock PluginExtensionType = "StoryBlock" - PluginExtensionTypeInfoboxBlock PluginExtensionType = "InfoboxBlock" + PluginExtensionTypeStory PluginExtensionType = "STORY" + PluginExtensionTypeStoryPage PluginExtensionType = "STORYPAGE" + PluginExtensionTypeStoryBlock PluginExtensionType = "STORYBLOCK" + PluginExtensionTypeInfoboxBlock PluginExtensionType = "INFOBOXBLOCK"Also, update the
IsValidmethod and any references to these constants accordingly.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
server/e2e/common.go(4 hunks)server/e2e/gql_asset_test.go(7 hunks)server/e2e/gql_validate_geojson_test.go(1 hunks)server/e2e/seeder.go(0 hunks)server/gql/asset.graphql(5 hunks)server/gql/workspace.graphql(2 hunks)server/internal/adapter/gql/gqlmodel/convert_asset.go(1 hunks)server/internal/adapter/gql/gqlmodel/models_gen.go(7 hunks)server/internal/adapter/gql/loader_asset.go(2 hunks)server/internal/adapter/gql/resolver_mutation_asset.go(3 hunks)server/internal/adapter/gql/resolver_mutation_project.go(3 hunks)server/internal/adapter/gql/resolver_query.go(1 hunks)server/internal/adapter/gql/resolver_team.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(5 hunks)server/internal/usecase/interfaces/asset.go(2 hunks)server/internal/usecase/repo/asset.go(1 hunks)server/pkg/asset/asset.go(2 hunks)server/pkg/asset/builder.go(1 hunks)server/pkg/asset/id.go(1 hunks)server/pkg/layer/layerops/processor_test.go(0 hunks)
💤 Files with no reviewable changes (2)
- server/pkg/layer/layerops/processor_test.go
- server/e2e/seeder.go
🚧 Files skipped from review as they are similar to previous changes (9)
- server/internal/adapter/gql/gqlmodel/convert_asset.go
- server/pkg/asset/builder.go
- server/internal/usecase/repo/asset.go
- server/internal/infrastructure/memory/asset.go
- server/pkg/asset/asset.go
- server/gql/asset.graphql
- server/internal/infrastructure/mongo/mongodoc/asset.go
- server/e2e/gql_validate_geojson_test.go
- server/internal/adapter/gql/loader_asset.go
🧰 Additional context used
📓 Learnings (1)
server/internal/adapter/gql/resolver_mutation_project.go (1)
Learnt from: hexaforce
PR: reearth/reearth-visualizer#1151
File: server/internal/adapter/gql/resolver_mutation_project.go:215-217
Timestamp: 2024-11-12T15:21:04.150Z
Learning: In the project import functionality, the file names are unique and can be safely replaced uniformly using `bytes.Replace`.
🔇 Additional comments (19)
server/pkg/asset/id.go (1)
10-10: LGTM! Well-structured ID type implementation.The new ProjectID type and its associated functions follow the established patterns consistently.
Also applies to: 14-14, 18-18, 22-22, 26-26
server/internal/usecase/interfaces/asset.go (2)
28-29: LGTM! ProjectID field added to CreateAssetParam.The addition follows the struct's existing pattern.
39-39: Verify interface implementations for updated method signatures.The Asset interface has been updated with ProjectID parameters. Ensure all implementations (mongo, memory, etc.) are updated accordingly.
Also applies to: 41-41, 43-43
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for Asset interface implementations ast-grep --pattern 'type $_ struct { $$$ } func (r $_) FindByWorkspace(ctx context.Context, wid accountdomain.WorkspaceID, pid *id.ProjectID, $$$) ($$$) { $$$ }'Length of output: 168
I'm going to run an additional search using
rgto list all occurrences ofFindByWorkspacemethod definitions in the repository. This should help us verify if the implementations have been updated to the new signature with the ProjectID parameter.
🏁 Script executed:
#!/bin/bash # Search for all FindByWorkspace definitions with context. rg "func .*FindByWorkspace\(.*\)" -A10 -B10 .Length of output: 16701
Interface implementations for FindByWorkspace have been updated correctly.
- The memory implementation in
server/internal/infrastructure/memory/asset.goaccepts the new*id.ProjectIDparameter.- The Mongo implementation in
server/internal/infrastructure/mongo/asset.gosimilarly reflects the updated signature.- The usecase and gateway layers (e.g., in
server/internal/usecase/interactor/asset.goand the GraphQL loader) also pass the parameter as expected.All implementations appear to be consistent with the new interface signature.
server/internal/adapter/gql/resolver_mutation_asset.go (1)
41-62: LGTM! UpdateAsset implementation with validation.The implementation includes proper validation of project-workspace relationship as suggested in past reviews.
server/internal/usecase/interactor/asset.go (3)
39-51: LGTM! Method signature updated to support project filtering.The changes correctly implement project-based filtering for assets by passing the project ID to the repository layer.
88-92: LGTM! Asset creation updated to include project association.The asset builder chain now correctly includes the project ID when creating new assets.
154-182: Add validation for project-workspace relationship.The UploadAssetFile method should verify that the provided project belongs to the workspace before creating the asset.
func (i *Asset) UploadAssetFile(ctx context.Context, name string, zipFile *zip.File, workspace idx.ID[accountdomain.Workspace], project *id.ProjectID) (*url.URL, int64, error) { + if project != nil { + p, err := i.repos.Project.FindByID(ctx, *project) + if err != nil { + return nil, 0, fmt.Errorf("failed to validate project: %w", err) + } + if p.Workspace() != workspace { + return nil, 0, fmt.Errorf("project does not belong to workspace") + } + }server/internal/adapter/gql/resolver_query.go (1)
15-17: LGTM! GraphQL resolver updated to support project filtering.The Assets query resolver correctly handles the new projectId parameter.
server/e2e/gql_asset_test.go (4)
16-17: LGTM! Clear test description.The test description in the comment clearly indicates the test's purpose.
131-195: Well-structured test coverage for project association.The test comprehensively covers:
- Asset creation with project association
- Asset retrieval by project
- Moving assets between projects
- Removing project association
- Asset deletion
197-214: LGTM! GraphQL mutation updated correctly.The CreateAssetMutation has been properly updated to include the projectId parameter.
240-256: LGTM! Clear implementation of asset update mutation.The updateAsset function is well-implemented with proper error handling.
server/internal/adapter/gql/resolver_mutation_project.go (1)
262-271: LGTM! Improved transaction handling.The deferred transaction handling ensures proper cleanup in case of errors.
server/gql/workspace.graphql (2)
8-14: LGTM! Well-structured assets query.The assets query has been properly updated to include the optional projectId parameter, maintaining backward compatibility.
15-21: LGTM! Consistent formatting.The projects query formatting matches the assets query, maintaining consistency.
server/internal/adapter/gql/gqlmodel/models_gen.go (4)
138-138: Verify integration of the newProjectIDfield inAssetandCreateAssetInputstructsThe addition of the optional
ProjectIDfield to theAssetandCreateAssetInputstructs appears to be correctly implemented. Ensure that all parts of the codebase that create or manipulate assets handle this new field appropriately. This includes validation, serialization, and updates to any API clients that interact with these structs.Also applies to: 186-186
1330-1337: Ensure correct implementation ofUpdateAssetInputandUpdateAssetPayloadstructsThe introduction of
UpdateAssetInputandUpdateAssetPayloadstructs with an optionalProjectIDfield extends the asset update functionality. Verify that the mutation resolvers correctly utilize these structs and that theProjectIDfield is properly handled during asset updates.
1135-1149: Verify handling of new fields in theScenestructThe addition of multiple fields to the
Scenestruct, such asProjectID,TeamID,Widgets,Plugins,NewLayers,Stories, andStyles, enhances the scene's capabilities. Ensure that these fields are correctly initialized, managed, and serialized throughout the codebase. Also, verify that any associated business logic and database migrations are updated to accommodate these changes.
581-588: Check compatibility of usinganyas a typeThe
MergedPropertyFieldstruct usesany(an alias forinterface{}introduced in Go 1.18) for theValuefield. If the project aims to maintain compatibility with versions of Go prior to 1.18, consider replacinganywithinterface{}. Ensure consistency across all structs whereanyis used.Apply this diff if compatibility with earlier Go versions is required:
- Value any `json:"value,omitempty"` + Value interface{} `json:"value,omitempty"`✅ Verification successful
I'm going to start by checking the Go version required by the project (as specified in the
go.modfile) to determine whether using Go 1.18'sanyalias is acceptable. This will help us know if we need backward compatibility.
🏁 Script executed:
#!/bin/bash # Check the Go version specified in go.mod grep "^go " go.modLength of output: 60
Let's get more context on the Go version used by the project by checking for Dockerfiles that might specify a Go version.
🏁 Script executed:
#!/bin/bash # Check for indications of the Go version used in Dockerfiles. grep -R "FROM golang:" .Length of output: 81
Compatibility of
anyis acceptable given the project's Go version
- The Dockerfile specifies
golang:1.23.5-alpine, ensuring the project is built on Go 1.23.5, which fully supports theanyalias.- No backward compatibility concerns arise as Go versions prior to 1.18 are not targeted.
- The use of
anyin theMergedPropertyFieldstruct is consistent with the project's environment.
hexaforce
force-pushed
the
feat/associate-project-asset
branch
from
5e732cf to
c76e941
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
server/internal/adapter/gql/gqlmodel/models_gen.go (1)
906-915: 🛠️ Refactor suggestionInconsistent type for 'ID' in 'PropertyField' struct.
The
IDfield is declared asstring, whereas other identifier fields use the customIDtype. For consistency and type safety, consider changingIDto use theIDtype.Apply this diff to fix the inconsistency:
-ID string `json:"id"` +ID ID `json:"id"`server/internal/usecase/interactor/asset.go (1)
108-127:⚠️ Potential issueAdd missing project-workspace validation.
The project-workspace validation is missing from the Update method.
As previously suggested in past reviews, add the validation before updating the asset:
if ok := operator.IsWritableWorkspace(asset.Workspace()); !ok { return aid, pid, interfaces.ErrOperationDenied } +if pid != nil { + project, err := i.repos.Project.FindByID(ctx, *pid) + if err != nil { + return aid, pid, fmt.Errorf("failed to validate project: %w", err) + } + if project.Workspace() != asset.Workspace() { + return aid, pid, fmt.Errorf("project does not belong to asset's workspace") + } +} + asset.SetProject(pid)
🧹 Nitpick comments (3)
server/internal/adapter/gql/resolver_mutation_project.go (1)
287-292: Improve error message specificity.The current error message "Fail UploadAssetFile" doesn't indicate whether the failure was due to project ID conversion or the actual file upload.
- return nil, errors.New("Fail UploadAssetFile :" + err.Error()) + return nil, errors.New("Failed to convert project ID during asset upload: " + err.Error())server/e2e/common.go (1)
305-319: Improve error handling inValueDump.While the type handling has been improved, the function silently ignores JSON marshaling errors.
Consider this enhancement:
func ValueDump(val *httpexpect.Value) { raw := val.Raw() switch data := raw.(type) { case map[string]interface{}: if text, err := json.MarshalIndent(data, "", " "); err == nil { fmt.Println(string(text)) + } else { + fmt.Printf("Error marshaling map: %v\n", err) } case []interface{}: if text, err := json.MarshalIndent(data, "", " "); err == nil { fmt.Println(string(text)) + } else { + fmt.Printf("Error marshaling array: %v\n", err) } default: fmt.Println("Unsupported type:", reflect.TypeOf(raw)) } }server/internal/adapter/gql/gqlmodel/models_gen.go (1)
1147-1149: Consider renaming 'NewLayers' field in 'Scene' struct for clarity.The field
NewLayersmay cause confusion regarding its purpose. If it represents the primary collection of layers in a scene, consider renaming it toLayersfor consistency. If it coexists with an existingLayersfield, a more descriptive name likeUpdatedLayersorAdditionalLayersmight enhance clarity.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
server/e2e/common.go(4 hunks)server/e2e/gql_asset_test.go(7 hunks)server/e2e/gql_validate_geojson_test.go(1 hunks)server/e2e/seeder.go(0 hunks)server/gql/asset.graphql(5 hunks)server/gql/workspace.graphql(2 hunks)server/internal/adapter/gql/gqlmodel/convert_asset.go(1 hunks)server/internal/adapter/gql/gqlmodel/models_gen.go(7 hunks)server/internal/adapter/gql/loader_asset.go(2 hunks)server/internal/adapter/gql/resolver_mutation_asset.go(3 hunks)server/internal/adapter/gql/resolver_mutation_project.go(3 hunks)server/internal/adapter/gql/resolver_query.go(1 hunks)server/internal/adapter/gql/resolver_team.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(5 hunks)server/internal/usecase/interfaces/asset.go(2 hunks)server/internal/usecase/repo/asset.go(1 hunks)server/pkg/asset/asset.go(2 hunks)server/pkg/asset/builder.go(1 hunks)server/pkg/asset/id.go(1 hunks)server/pkg/layer/layerops/processor_test.go(0 hunks)server/pkg/scene/builder_test.go(0 hunks)
💤 Files with no reviewable changes (3)
- server/pkg/scene/builder_test.go
- server/pkg/layer/layerops/processor_test.go
- server/e2e/seeder.go
🚧 Files skipped from review as they are similar to previous changes (14)
- server/internal/adapter/gql/gqlmodel/convert_asset.go
- server/internal/usecase/repo/asset.go
- server/internal/adapter/gql/resolver_team.go
- server/e2e/gql_validate_geojson_test.go
- server/pkg/asset/builder.go
- server/pkg/asset/id.go
- server/gql/asset.graphql
- server/internal/infrastructure/memory/asset.go
- server/gql/workspace.graphql
- server/internal/adapter/gql/loader_asset.go
- server/pkg/asset/asset.go
- server/internal/infrastructure/mongo/mongodoc/asset.go
- server/internal/infrastructure/mongo/asset.go
- server/internal/adapter/gql/resolver_mutation_asset.go
🧰 Additional context used
📓 Learnings (1)
server/internal/adapter/gql/resolver_mutation_project.go (1)
Learnt from: hexaforce
PR: reearth/reearth-visualizer#1151
File: server/internal/adapter/gql/resolver_mutation_project.go:215-217
Timestamp: 2024-11-12T15:21:04.150Z
Learning: In the project import functionality, the file names are unique and can be safely replaced uniformly using `bytes.Replace`.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: ci-server / ci-server-lint
- GitHub Check: ci-web / ci
- GitHub Check: ci-server / ci-server-test
🔇 Additional comments (17)
server/internal/usecase/interfaces/asset.go (4)
26-31: LGTM! TheProjectIDfield is well-defined.The optional
ProjectIDfield is correctly implemented as a pointer type, allowing assets to be created with or without project association.
39-39: LGTM! TheFindByWorkspacemethod signature is updated correctly.The method now accepts a
*id.ProjectIDparameter, enabling filtering of assets by project.
41-41: LGTM! TheUpdatemethod is well-designed.The method returns both
AssetIDandProjectID, providing clear feedback about the update operation's result.
43-43: LGTM! TheUploadAssetFilemethod signature is updated correctly.The method now accepts a
*id.ProjectIDparameter, allowing project association during file upload.server/internal/adapter/gql/resolver_query.go (1)
15-16: LGTM! TheAssetsresolver is updated correctly.The resolver now accepts and forwards the optional
projectIdparameter to the asset loader, enabling project-based filtering.server/e2e/gql_asset_test.go (3)
18-127: LGTM! The core support test cases are comprehensive.The test cases thoroughly validate asset creation and retrieval with core support flags, including proper handling of the new
projectIdfield.
131-194: LGTM! The project association test cases are thorough.The test suite comprehensively validates the project association functionality:
- Asset creation with project association
- Project-specific asset retrieval
- Moving assets between projects
- Removing project associations
- Asset removal
215-365: LGTM! The helper functions are well-implemented.The helper functions provide comprehensive support for testing asset management:
- Asset creation with project association
- Asset retrieval with project filtering
- Project association updates
- Asset removal
server/internal/adapter/gql/resolver_mutation_project.go (1)
262-271: LGTM! Proper transaction handling.The transaction management is well-implemented with proper cleanup and error propagation.
server/internal/adapter/gql/gqlmodel/models_gen.go (6)
138-138: Addition of 'ProjectID' field to 'Asset' struct is appropriate.The
ProjectIDfield allows assets to be associated with projects, enhancing asset management functionality as per the PR objectives. The field is optional and correctly handled using a pointer, and the JSON tag follows the correct naming convention.
186-186: Proper inclusion of 'ProjectID' in 'CreateAssetInput' struct.Adding
ProjectIDtoCreateAssetInputenables associating assets with projects during creation. The implementation correctly uses a pointer for the optional field, and the JSON tag is appropriately named.
581-588: Fields added to 'MergedPropertyField' struct are properly defined.The addition of fields such as
SchemaID,FieldID,Value,Type,Links,Overridden,Schema, andFieldenhances the functionality ofMergedPropertyField. The types are correctly specified, and the use ofanyfor theValuefield is appropriate.
1331-1332: 'ProjectID' correctly added to 'UpdateAssetInput' struct.Including
ProjectIDinUpdateAssetInputallows updating the association between an asset and a project. The optional nature of the field is properly handled, and the JSON tag follows conventions.
1336-1337: 'ProjectID' correctly added to 'UpdateAssetPayload' struct.This ensures the response after updating an asset includes the
ProjectID, reflecting any changes made to the asset's project association.
1844-1844: Updated 'PluginExtensionType.IsValid' method to include new extension types.The addition of new extension types like
PluginExtensionTypeStory,PluginExtensionTypeStoryPage,PluginExtensionTypeStoryBlock, andPluginExtensionTypeInfoboxBlockto the validation ensures these types are recognized as valid, aligning with the new functionalities introduced.server/internal/usecase/interactor/asset.go (2)
39-44: LGTM! Well-structured project filtering implementation.The addition of project ID filtering in
FindByWorkspaceis clean and consistent with the existing filtering pattern.
88-92: 🛠️ Refactor suggestionAdd project-workspace validation before asset creation.
While setting the project ID is implemented correctly, you should validate that the project belongs to the workspace before creating the asset.
Add this validation after the workspace check:
if !operator.IsWritableWorkspace(ws.ID()) { return nil, interfaces.ErrOperationDenied } + +if inp.ProjectID != nil { + project, err := i.repos.Project.FindByID(ctx, *inp.ProjectID) + if err != nil { + return nil, fmt.Errorf("failed to validate project: %w", err) + } + if project.Workspace() != ws.ID() { + return nil, fmt.Errorf("project does not belong to workspace") + } +}
hexaforce
force-pushed
the
feat/associate-project-asset
branch
from
76da26f to
4c733af
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
server/e2e/common.go (1)
288-293:⚠️ Potential issueEnhance error handling and type support in
JSONEqRegexpValue.The current implementation has several limitations that could lead to silent failures and missed test cases:
- Only handles map types, silently failing for other JSON-compatible values
- No error handling for JSON marshaling failures
- Missing documentation explaining the function's purpose
The previous review comment's suggestions for improvement are still valid. Please refer to that comment for the recommended implementation.
🧹 Nitpick comments (5)
server/e2e/common.go (1)
305-319: Improve error handling and add documentation toValueDump.The type handling improvements are good, but there are a few enhancements that would make the function more robust:
+// ValueDump pretty prints the JSON representation of an httpexpect.Value. +// Supports maps, slices, and reports unsupported types. func ValueDump(val *httpexpect.Value) { raw := val.Raw() switch data := raw.(type) { case map[string]interface{}: if text, err := json.MarshalIndent(data, "", " "); err == nil { fmt.Println(string(text)) + } else { + fmt.Printf("Error marshaling map: %v\n", err) } case []interface{}: if text, err := json.MarshalIndent(data, "", " "); err == nil { fmt.Println(string(text)) + } else { + fmt.Printf("Error marshaling slice: %v\n", err) } default: fmt.Println("Unsupported type:", reflect.TypeOf(raw)) } }server/internal/infrastructure/mongo/asset.go (1)
67-101: Consider adding index for project field.Since the code now filters by project ID, adding a MongoDB index for the "project" field would improve query performance.
var ( - assetIndexes = []string{"team"} + assetIndexes = []string{"team", "project"} assetUniqueIndexes = []string{"id"} )server/e2e/gql_asset_test.go (3)
16-18: Consider renaming the test function to better reflect its purpose.The test name
TestCoreSupportGetAssetsemphasizes "CoreSupport" testing, but the function also tests project association functionality. Consider a more inclusive name likeTestAssetCreationAndFilteringto better reflect all aspects being tested.
131-194: LGTM! Consider adding edge cases.The test function provides excellent coverage of project association functionality. Consider adding these edge cases:
- Attempt to associate an asset with a non-existent project
- Test concurrent updates to project associations
- Verify behavior when moving multiple assets between projects
215-237: Add function documentation.Consider adding GoDoc comments to the helper functions to describe:
- Parameter requirements and constraints
- Return value structure
- Error conditions
- Usage examples
+// createAsset creates a new asset with the given parameters and returns the GraphQL response. +// Parameters: +// - t: testing.T instance for test assertions +// - e: httpexpect.Expect instance for making HTTP requests +// - filePath: path to the file to be uploaded +// - coreSupport: whether the asset is core supported +// - teamId: ID of the team owning the asset +// - projectId: optional ID of the project to associate with the asset func createAsset(t *testing.T, e *httpexpect.Expect, filePath string, coreSupport bool, teamId string, projectId *string) *httpexpect.Value {Also applies to: 273-300
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
server/e2e/common.go(4 hunks)server/e2e/gql_asset_test.go(7 hunks)server/e2e/gql_validate_geojson_test.go(1 hunks)server/gql/asset.graphql(5 hunks)server/gql/workspace.graphql(2 hunks)server/internal/adapter/gql/gqlmodel/convert_asset.go(1 hunks)server/internal/adapter/gql/gqlmodel/models_gen.go(7 hunks)server/internal/adapter/gql/loader_asset.go(2 hunks)server/internal/adapter/gql/resolver_mutation_asset.go(3 hunks)server/internal/adapter/gql/resolver_mutation_project.go(3 hunks)server/internal/adapter/gql/resolver_query.go(1 hunks)server/internal/adapter/gql/resolver_team.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(5 hunks)server/internal/usecase/interfaces/asset.go(2 hunks)server/internal/usecase/repo/asset.go(1 hunks)server/pkg/asset/asset.go(2 hunks)server/pkg/asset/builder.go(1 hunks)server/pkg/asset/id.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (14)
- server/internal/adapter/gql/gqlmodel/convert_asset.go
- server/internal/adapter/gql/resolver_team.go
- server/internal/usecase/repo/asset.go
- server/internal/infrastructure/memory/asset.go
- server/e2e/gql_validate_geojson_test.go
- server/internal/adapter/gql/loader_asset.go
- server/internal/infrastructure/mongo/mongodoc/asset.go
- server/internal/adapter/gql/resolver_mutation_asset.go
- server/internal/adapter/gql/resolver_mutation_project.go
- server/pkg/asset/builder.go
- server/pkg/asset/asset.go
- server/internal/usecase/interfaces/asset.go
- server/gql/asset.graphql
- server/gql/workspace.graphql
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Redirect rules - reearth-web
- GitHub Check: Header rules - reearth-web
- GitHub Check: Pages changed - reearth-web
🔇 Additional comments (9)
server/internal/adapter/gql/gqlmodel/models_gen.go (3)
134-145: LGTM! The Asset struct changes look good.The addition of the optional
ProjectIDfield enables associating assets with projects while maintaining backward compatibility.
184-189: LGTM! The CreateAssetInput struct changes look good.The addition of the optional
ProjectIDfield allows specifying project association during asset creation.
1330-1338: LGTM! The UpdateAssetInput and UpdateAssetPayload struct changes look good.The addition of the optional
ProjectIDfield to both structs enables updating the project association of existing assets.server/pkg/asset/id.go (1)
10-10: LGTM! Clean and consistent implementation.The additions of ProjectID type alias and related variables follow the existing patterns and are well-structured.
Also applies to: 14-14, 18-18, 22-22, 26-26
server/internal/usecase/interactor/asset.go (3)
39-51: LGTM! FindByWorkspace implementation is correct.The method signature and implementation have been updated to support filtering assets by project ID.
108-127: Verify project-workspace relationship in Update method.The Update method should validate that the provided project belongs to the asset's workspace before updating the association.
Based on past review comments, this validation was previously suggested and fixed. Please ensure the fix is in place.
154-195: Verify project-workspace relationship in UploadAssetFile.The UploadAssetFile method should validate that the provided project belongs to the workspace before creating the asset.
Based on past review comments, this validation was previously suggested and fixed. Please ensure the fix is in place.
server/internal/adapter/gql/resolver_query.go (1)
15-17: LGTM! GraphQL resolver implementation is correct.The Assets query resolver has been properly updated to support filtering by project ID.
server/e2e/gql_asset_test.go (1)
196-213: Add error response validation.While the GraphQL operations are well-structured, the tests should also verify error responses for invalid scenarios:
- Invalid project IDs
- Permission-related errors
- Concurrent modification conflicts
Also applies to: 239-271
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Check if error handling tests exist in the codebase rg -l 'func.*Test.*Error' server/e2e/Length of output: 39
GraphQL Error Handling Tests Are Missing
The recent search for functions matching
Test.*Errorwithin theserver/e2e/directory produced no matches, confirming that there are currently no tests validating error responses for the GraphQL operations.
- Ensure that tests are added to verify error responses for scenarios including invalid project IDs, permission-related errors, and concurrent modification conflicts.
- Review both sections (lines 196–213 and 239–271 in
server/e2e/gql_asset_test.go) and augment them with assertions against expected error responses.
hexaforce
force-pushed
the
feat/associate-project-asset
branch
from
4c733af to
6d8f26b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
server/internal/usecase/interactor/asset.go (1)
53-106: 🛠️ Refactor suggestionAdd project-workspace validation in Create method.
Similar to other methods, we should validate that the project belongs to the workspace before creating the asset.
Add this validation before creating the asset:
if !operator.IsWritableWorkspace(ws.ID()) { return nil, interfaces.ErrOperationDenied } + + if inp.ProjectID != nil { + project, err := i.repos.Project.FindByID(ctx, *inp.ProjectID) + if err != nil { + return nil, fmt.Errorf("failed to validate project: %w", err) + } + if project.Workspace() != ws.ID() { + return nil, fmt.Errorf("project does not belong to workspace") + } + }
♻️ Duplicate comments (1)
server/e2e/common.go (1)
288-293: 🛠️ Refactor suggestionEnhance error handling and type support in
JSONEqRegexpValue.The function has limited type support and lacks error handling:
- Only handles map types, but could receive arrays or other JSON-compatible values
- No error handling for JSON marshaling failures
- Missing documentation
Consider this improved implementation:
+// JSONEqRegexpValue compares the JSON representation of an httpexpect.Value against a regexp pattern. +// Returns true if the pattern matches, false otherwise. func JSONEqRegexpValue(t *testing.T, actual *httpexpect.Value, expected string) bool { - if actualData, ok := actual.Raw().(map[string]interface{}); ok { + actualData := actual.Raw() + if actualData == nil { + t.Error("actual value is nil") + return false + } + switch actualData.(type) { + case map[string]interface{}, []interface{}, string, float64, bool: return JSONEqRegexpInterface(t, actualData, expected) + default: + t.Errorf("unsupported type for JSON comparison: %T", actualData) + return false } - return false }
🧹 Nitpick comments (4)
server/internal/infrastructure/mongo/asset.go (1)
23-23: Add index for project field.Since we're now filtering by project, adding an index will improve query performance.
Update the indexes:
- assetIndexes = []string{"team"} + assetIndexes = []string{"team", "project"}server/e2e/gql_asset_test.go (3)
131-194: Add error case tests for project asset operations.While the happy path is well tested, consider adding test cases for:
- Invalid project IDs
- Unauthorized project access
- Moving assets to non-existent projects
- Concurrent project updates
239-256: Add error handling for the GraphQL response.Consider checking for errors in the response to handle cases like:
- Invalid asset ID
- Invalid project ID
- GraphQL execution errors
257-272: Add error handling for the GraphQL response.Consider checking for errors in the response to handle cases like:
- Invalid asset ID
- Asset already removed
- GraphQL execution errors
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
server/e2e/common.go(4 hunks)server/e2e/gql_asset_test.go(7 hunks)server/e2e/gql_validate_geojson_test.go(1 hunks)server/gql/asset.graphql(5 hunks)server/gql/workspace.graphql(2 hunks)server/internal/adapter/gql/gqlmodel/convert_asset.go(1 hunks)server/internal/adapter/gql/gqlmodel/models_gen.go(7 hunks)server/internal/adapter/gql/loader_asset.go(2 hunks)server/internal/adapter/gql/resolver_mutation_asset.go(3 hunks)server/internal/adapter/gql/resolver_mutation_project.go(3 hunks)server/internal/adapter/gql/resolver_query.go(1 hunks)server/internal/adapter/gql/resolver_team.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(5 hunks)server/internal/usecase/interfaces/asset.go(2 hunks)server/internal/usecase/repo/asset.go(1 hunks)server/pkg/asset/asset.go(2 hunks)server/pkg/asset/builder.go(1 hunks)server/pkg/asset/id.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (14)
- server/pkg/asset/builder.go
- server/internal/adapter/gql/gqlmodel/convert_asset.go
- server/internal/usecase/repo/asset.go
- server/internal/adapter/gql/resolver_team.go
- server/internal/infrastructure/memory/asset.go
- server/e2e/gql_validate_geojson_test.go
- server/internal/infrastructure/mongo/mongodoc/asset.go
- server/gql/asset.graphql
- server/internal/adapter/gql/loader_asset.go
- server/internal/adapter/gql/resolver_mutation_asset.go
- server/gql/workspace.graphql
- server/pkg/asset/asset.go
- server/internal/adapter/gql/resolver_query.go
- server/internal/adapter/gql/resolver_mutation_project.go
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: deploy-server / deploy_test
- GitHub Check: Redirect rules - reearth-web
- GitHub Check: Header rules - reearth-web
- GitHub Check: Pages changed - reearth-web
🔇 Additional comments (22)
server/pkg/asset/id.go (1)
10-10: LGTM! Clean addition of ProjectID type and related functions.The new ProjectID type and its associated functions follow the established pattern, maintaining consistency with existing ID types.
Also applies to: 14-14, 18-18, 22-22, 26-26
server/internal/usecase/interfaces/asset.go (2)
28-28: LGTM! ProjectID field added to CreateAssetParam.The optional ProjectID field allows assets to be associated with projects during creation.
39-39: Verify interface implementation consistency.The interface methods have been updated to include ProjectID. Let's verify that all implementations match this interface.
Also applies to: 41-41, 43-43
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Find all implementations of the Asset interface to verify consistency # Search for struct types that implement the Asset interface ast-grep --pattern 'type $_ struct { $$$ } func ($_ *$_) FindByWorkspace(ctx context.Context, $$$) ($$$) { $$$ }' # Search for interface method implementations rg -A 5 'func \(.*\) FindByWorkspace\('Length of output: 5919
Interface Implementation Consistency Verified: No discrepancies found.
- The asset interface in
server/internal/usecase/interfaces/asset.gonow includes the*id.ProjectIDparameter.- The memory implementation (
server/internal/infrastructure/memory/asset.go) and the mongo implementation (server/internal/infrastructure/mongo/asset.go) both include this parameter aspid *id.ProjectID(or equivalent).- The usecase interactor (
server/internal/usecase/interactor/asset.go) implements the updated signature with the extra parameter alongside the additional fields, and the GraphQL adapter appropriately adapts types.All implementations consistently reflect the updated interface.
server/internal/usecase/interactor/asset.go (3)
39-51: LGTM! FindByWorkspace implementation updated.The method correctly handles the new ProjectID parameter and passes it to the repository layer.
108-127: LGTM! Update method implementation.The method correctly handles project association updates with proper permission checks.
154-195: LGTM! UploadAssetFile implementation updated.The method correctly handles the new ProjectID parameter.
server/internal/infrastructure/mongo/asset.go (1)
67-103: LGTM! FindByWorkspace implementation updated.The method correctly handles filtering by project ID while maintaining existing functionality.
server/e2e/common.go (1)
305-319: LGTM! Improved type handling inValueDump.The changes enhance the function by:
- Adding support for array types
- Better type handling with switch statement
- Improved error reporting for unsupported types
server/e2e/gql_asset_test.go (5)
18-127: LGTM! Test cases updated to handle projectId field.The changes correctly validate the new projectId field while maintaining backward compatibility by testing the nil case.
196-213: LGTM! GraphQL mutation updated to handle projectId.The mutation correctly includes projectId in both input and response fields.
215-237: LGTM! Asset creation updated to handle projectId.The function correctly handles the optional projectId parameter while maintaining backward compatibility.
273-300: LGTM! Asset creation from file data updated to handle projectId.The function correctly handles the optional projectId parameter while maintaining backward compatibility.
302-365: LGTM! Asset retrieval updated to handle projectId filtering.The function correctly includes projectId in both query parameters and response fields while maintaining backward compatibility.
server/internal/adapter/gql/gqlmodel/models_gen.go (9)
138-138: Introduce optionalprojectIdfield inAsset.
Good addition for linking assets to projects. No obvious issues found.
186-186: AddprojectIdtoCreateAssetInput.
The change aligns with theAssetstruct. Consider verifying upstream validation to ensure correct handling of this optional field.
580-589:MergedPropertyFieldusesanyforValue.
This seems normal for GraphQL-dynamic properties, though it sacrifices strict type checking. No immediate concerns.
905-915: Use a consistent type forID.
This comment mirrors a prior review remark:PropertyField.IDis astring, whereas related fields in this struct use the customIDtype. Aligning them would improve consistency and type safety.
919-921:PropertyFieldLinkreferences appear consistent.
All references are typed correctly asIDand*ID. The struct looks coherent with the rest of the model.
1135-1149: Verify cross-references in theScenestruct.
New references toProject,Team, andPropertycan introduce potential nulls or circular dependencies. Confirm that data loaders or resolvers properly handle these references.
1330-1333: Support optionalprojectIdinUpdateAssetInput.
Incorporating an optional project association consistently matches the earlier additions. No concerns observed.
1335-1337: IncludeprojectIdinUpdateAssetPayload.
The payload now includes the linkedprojectId, which aligns with the updated asset structure. Looks good.
1844-1844: UpdatedPluginExtensionTypeswitch branch.
The added or modified case maintains consistency with other extension types.
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (1)
server/pkg/storytelling/story.go (1)
202-210:⚠️ Potential issueFix duplicate property ID in the list.
The property ID is being added twice to the list:
- Line 206:
ids := []PropertyID{s.property}- Line 207:
ids = append(ids, s.Property())Both lines add the same property ID, as
s.Property()returnss.property.Apply this diff to fix the duplication:
- ids := []PropertyID{s.property} - ids = append(ids, s.Property()) + ids := []PropertyID{s.property}
♻️ Duplicate comments (1)
server/internal/usecase/interactor/asset.go (1)
108-127:⚠️ Potential issueAdd validation for project-workspace relationship.
The function should verify that the provided project belongs to the workspace before updating the asset's project association.
🧹 Nitpick comments (17)
server/pkg/scene/builder/story.go (2)
38-56: Consider improving error handling and input validation.The method could be enhanced in two ways:
- Add validation for the input property slice
- Simplify the error handling since it's only used for the nil case
Consider this refactoring:
-func (b *Builder) storyJSON(ctx context.Context, p []*property.Property) (*storyJSON, error) { +func (b *Builder) storyJSON(ctx context.Context, p []*property.Property) *storyJSON { if b.story == nil { - return nil, nil + return nil } + + if p == nil { + p = []*property.Property{} + } return &storyJSON{ ID: b.story.Id().String(), Title: b.story.Title(), Property: b.property(ctx, findProperty(p, b.story.Property())), Pages: lo.FilterMap(b.story.Pages().Pages(), func(page *storytelling.Page, _ int) (pageJSON, bool) { if page == nil { return pageJSON{}, false } return b.pageJSON(ctx, *page, p), true }), PanelPosition: string(b.story.PanelPosition()), BgColor: b.story.BgColor(), - }, nil + } }
75-83: Remove unnecessary nil assignment.The explicit assignment of
Plugins: nilis redundant as it's already the zero value for the field.func (b *Builder) blockJSON(ctx context.Context, block storytelling.Block, p []*property.Property) blockJSON { return blockJSON{ ID: block.ID().String(), Property: b.property(ctx, findProperty(p, block.Property())), - Plugins: nil, ExtensionId: block.Extension().String(), PluginId: block.Plugin().String(), } }server/e2e/gql_project_export_import_test.go (2)
29-29: Rename "exporProject" to "exportProject" for clarity.The function name "exporProject" is likely a typo and may cause confusion. Here’s a suggested fix:
-// Step 2: Export the data -fileName := exporProject(t, e, pID.String()) +// Step 2: Export the data +fileName := exportProject(t, e, pID.String()) -func exporProject(t *testing.T, e *httpexpect.Expect, p string) string { +func exportProject(t *testing.T, e *httpexpect.Expect, p string) string {Also applies to: 121-121
41-42: Consider validating the project ID in addition to name and description.The test currently asserts only the name and description under
"project". Verifying the project ID helps ensure correct project association after import.server/pkg/nlslayer/feature_test.go (1)
13-14: Rename the test method to match the new function usage.The function is no longer calling
NewFeatureWithNewIddirectly. Consider updating the test name for clarity:-func TestNewFeatureWithNewId(t *testing.T) { +func TestNewFeatureAutoID(t *testing.T) {server/e2e/gql_scene_test.go (1)
77-100: Consider enhancing error handling in project creation utility.While the function is well-structured, it could benefit from error handling for the GraphQL response.
func createProjectWithExternalImage(e *httpexpect.Expect, name string) string { // ... existing code ... res := Request(e, uID.String(), requestBody) + if err := res.Path("$.errors").NotNull().Raw(); err != nil { + panic(fmt.Sprintf("Failed to create project: %v", err)) + } return res.Path("$.data.createProject.project.id").Raw().(string) }server/internal/adapter/gql/gqlmodel/convert_project.go (1)
115-137: Consider adding validation in JSON conversion.While the conversion functions are well-implemented, they could benefit from additional validation.
func ToProjectExportFromJSON(data map[string]any) *ProjectExport { var p ProjectExport bytes, err := json.MarshalIndent(data, "", " ") if err != nil { return nil } if err := json.Unmarshal(bytes, &p); err != nil { return nil } + // Validate required fields + if p.Name == "" || p.Description == "" { + return nil + } return &p }server/internal/app/app.go (1)
164-168: Consider adding documentation for the dummy middleware.While the implementation is straightforward, it would be helpful to document:
- The purpose of this middleware
- When it should be used
- Security implications of disabling authentication
Add documentation:
+// AuthMiddlewareDummy returns a no-op middleware that bypasses authentication. +// This should only be used in development or testing environments where authentication is not required. +// WARNING: Using this in production would bypass all authentication checks. func AuthMiddlewareDummy() (func(http.Handler) http.Handler, error) {server/e2e/common.go (1)
35-37: Consider using a more descriptive variable name thanfr.The variable name
fris not descriptive enough. Consider renaming it to something more meaningful likefileRepoorfileGateway.-var fr *gateway.File +var fileGateway *gateway.File -type Seeder func(ctx context.Context, r *repo.Container, f gateway.File) error +type Seeder func(ctx context.Context, r *repo.Container, fileRepo gateway.File) errorserver/e2e/gql_asset_test.go (1)
196-213: Consider adding input validation tests.The mutation accepts optional
projectIdbut lacks tests for invalid project IDs or permissions.Add test cases for:
- Invalid project ID
- Project from different workspace
- Missing permissions
server/internal/adapter/gql/resolver_mutation_project.go (1)
189-194: Improve error message formatting.The error messages could be more descriptive and consistent.
-return nil, errors.New("Error parsing URL:" + err.Error()) +return nil, fmt.Errorf("failed to parse asset URL: %w", err)server/e2e/seeder.go (1)
305-306: Return nil explicitly for consistency.For better readability and consistency with the rest of the codebase, return nil explicitly instead of returning the err variable when there are no errors.
Apply this diff:
- return err + return nilserver/internal/usecase/interactor/project.go (2)
630-632: Log errors for external URLs.The function silently ignores errors when reading external URLs. This could hide issues that might need attention.
Apply this diff:
stream, err := file.ReadAsset(ctx, fileName) if err != nil { + fmt.Printf("Warning: Failed to read asset %s: %v (skipping external URL)\n", fileName, err) return nil // skip if external URL }
644-646: Remove redundant stream close.The stream is already closed by the deferred function, making this close redundant.
Apply this diff:
_, err = io.Copy(zipEntry, stream) if err != nil { - _ = stream.Close() return err }server/e2e/gql_project_test.go (2)
570-571: Use constants for test data setup.Using magic numbers makes the test harder to understand and maintain. Consider using named constants to make the test's intent clearer.
Apply this diff:
+const ( + testProjectsPerWorkspace = 20 + testPageSize = 16 +) - projects(t, ctx, r, 20, wID, "[wID]project", "ALIAS1", true) - projects(t, ctx, r, 20, wId1, "[wId1]project", "ALIAS2", true) + projects(t, ctx, r, testProjectsPerWorkspace, wID, "[wID]project", "ALIAS1", true) + projects(t, ctx, r, testProjectsPerWorkspace, wId1, "[wId1]project", "ALIAS2", true)
586-587: Remove or document commented code.The commented "keyword" filter lacks context. Either remove it if it's not needed or document why it's commented out.
Either remove the commented line:
- // "keyword": "Project",Or document why it's commented out:
+ // "keyword": "Project", // Commented out to test pagination without filteringserver/e2e/gql_storytelling_test.go (1)
769-847: Move expected JSON to test data file.Large inline JSON strings make tests hard to maintain. Consider moving this to a separate test data file.
- Create a new file
testdata/story_publishing_expected.jsonwith the JSON content.- Apply this diff:
+ expectedJSON, err := os.ReadFile("testdata/story_publishing_expected.json") + assert.NoError(t, err) + expected := fmt.Sprintf(string(expectedJSON), blockID) - expected := fmt.Sprintf(`{ - "coreSupport": true, - ... - }`, blockID)This makes the test more maintainable and easier to read.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
server/e2e/test.zipis excluded by!**/*.zip
📒 Files selected for processing (34)
server/e2e/common.go(9 hunks)server/e2e/gql_asset_test.go(7 hunks)server/e2e/gql_project_export_import_test.go(3 hunks)server/e2e/gql_project_export_test.go(0 hunks)server/e2e/gql_project_test.go(4 hunks)server/e2e/gql_scene_test.go(4 hunks)server/e2e/gql_storytelling_test.go(2 hunks)server/e2e/gql_user_test.go(2 hunks)server/e2e/seeder.go(5 hunks)server/go.mod(1 hunks)server/internal/adapter/gql/gqlmodel/convert_nlslayer_test.go(5 hunks)server/internal/adapter/gql/gqlmodel/convert_project.go(2 hunks)server/internal/adapter/gql/loader_asset.go(2 hunks)server/internal/adapter/gql/resolver_mutation_project.go(6 hunks)server/internal/app/app.go(2 hunks)server/internal/app/auth_client.go(1 hunks)server/internal/infrastructure/fs/file.go(1 hunks)server/internal/infrastructure/fs/file_test.go(2 hunks)server/internal/infrastructure/memory/asset.go(1 hunks)server/internal/infrastructure/mongo/asset.go(2 hunks)server/internal/infrastructure/mongo/mongodoc/scene.go(0 hunks)server/internal/infrastructure/mongo/mongodoc/storytelling.go(1 hunks)server/internal/usecase/interactor/asset.go(6 hunks)server/internal/usecase/interactor/nlslayer.go(3 hunks)server/internal/usecase/interactor/project.go(5 hunks)server/internal/usecase/interactor/project_test.go(0 hunks)server/internal/usecase/interactor/scene.go(5 hunks)server/internal/usecase/interfaces/asset.go(2 hunks)server/internal/usecase/interfaces/project.go(2 hunks)server/internal/usecase/repo/asset.go(1 hunks)server/pkg/nlslayer/feature.go(0 hunks)server/pkg/nlslayer/feature_test.go(1 hunks)server/pkg/scene/builder/story.go(1 hunks)server/pkg/storytelling/story.go(1 hunks)
💤 Files with no reviewable changes (4)
- server/internal/usecase/interactor/project_test.go
- server/pkg/nlslayer/feature.go
- server/internal/infrastructure/mongo/mongodoc/scene.go
- server/e2e/gql_project_export_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- server/internal/infrastructure/memory/asset.go
- server/internal/adapter/gql/loader_asset.go
🧰 Additional context used
🧠 Learnings (2)
server/e2e/gql_project_export_import_test.go (2)
Learnt from: hexaforce
PR: reearth/reearth-visualizer#1141
File: server/e2e/gql_import_export_test.go:48-80
Timestamp: 2024-11-12T15:21:04.151Z
Learning: In `server/e2e/gql_import_export_test.go`, test functions like `TestCallImportProject` may have disabled assertions due to problems with GraphQL.
Learnt from: hexaforce
PR: reearth/reearth-visualizer#1141
File: server/e2e/gql_import_export_test.go:12-44
Timestamp: 2024-11-12T15:21:04.150Z
Learning: The response body assertions in `TestCallExportProject` are disabled due to existing problems with GraphQL.
server/internal/adapter/gql/resolver_mutation_project.go (2)
Learnt from: hexaforce
PR: reearth/reearth-visualizer#1151
File: server/internal/adapter/gql/resolver_mutation_project.go:215-217
Timestamp: 2024-11-12T15:21:04.150Z
Learning: In the project import functionality, the file names are unique and can be safely replaced uniformly using `bytes.Replace`.
Learnt from: hexaforce
PR: reearth/reearth-visualizer#1151
File: server/internal/adapter/gql/resolver_mutation_project.go:0-0
Timestamp: 2024-11-12T15:21:04.150Z
Learning: In the project export functionality, the ZIP file is deleted after saving to storage.
🔇 Additional comments (28)
server/internal/usecase/interactor/scene.go (2)
22-22: LGTM!The addition of the storytelling package import aligns with the new story handling functionality.
680-683: LGTM!The new story handling logic properly checks for nil and array bounds before accessing the story list, preventing potential runtime panics.
server/internal/adapter/gql/gqlmodel/convert_nlslayer_test.go (2)
135-194: LGTM! Test structure is comprehensive and well-organized.The test cases provide good coverage of edge cases, use proper test parallelization, and maintain clear organization.
11-12: LGTM! Feature creation changes improve separation of concerns.The changes to use explicit ID generation with
NewFeatureID()improve code clarity and follow the single responsibility principle.Let's verify that all feature creation calls have been updated consistently:
Also applies to: 21-22, 31-32, 41-42, 51-52
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Search for any remaining instances of NewFeatureWithNewId # and verify all NewFeature calls have an ID parameter # Check for any remaining NewFeatureWithNewId calls echo "Checking for remaining NewFeatureWithNewId calls:" rg "NewFeatureWithNewId" # Check NewFeature usage pattern echo -e "\nVerifying NewFeature calls have NewFeatureID:" rg -A 1 "NewFeature\("Length of output: 5942
LGTM! Feature creation in
convert_nlslayer_test.gofollows the updated pattern.
- In
server/internal/adapter/gql/gqlmodel/convert_nlslayer_test.go, all feature creation calls now invokenlslayer.NewFeaturewith an explicit ID generated bynlslayer.NewFeatureID().- A repository-wide search confirms that the replacement is correctly applied in this file.
- Note: One occurrence of
NewFeatureWithNewIdstill exists inserver/pkg/nlslayer/feature_test.go. If the intent was to refactor all feature creations, this file might need a review too.server/internal/usecase/interfaces/asset.go (2)
28-28: LGTM! Project association field added.The addition of
ProjectIDfield toCreateAssetParamaligns with the PR objective of associating assets with projects.
39-43: Method signatures updated for project association.The changes enhance asset management by:
- Renaming
FindByWorkspacetoFindByWorkspaceProjectwith project filtering- Adding
Updatemethod for modifying project associations- Renaming
UploadAssetFiletoImportAssetFilesfor clarityserver/internal/usecase/interfaces/project.go (1)
74-75: LGTM! Enhanced type safety in project data methods.The changes improve the interface by:
- Using clearer method names with
Datasuffix- Enhancing type safety by using
idx.ID[accountdomain.Workspace]instead ofstringserver/e2e/gql_scene_test.go (1)
16-74: LGTM! Improved response object access.The changes enhance test readability by consistently using
Object().Value()for accessing response properties.server/internal/adapter/gql/gqlmodel/convert_project.go (1)
108-113: LGTM! Well-structured export type.The
ProjectExportstruct is well-defined with appropriate JSON tags and field types.server/pkg/storytelling/story.go (2)
55-60: LGTM! Good defensive programming practice.The nil check prevents potential nil pointer dereference and ensures safe behavior.
233-238: LGTM! Good input validation.The function now properly handles:
- Nil receiver
- Empty name parameter
- Private story status
server/internal/usecase/interactor/asset.go (3)
39-51: LGTM! Good workspace access control.The function properly:
- Validates workspace readability
- Passes project ID to repository layer
88-97:⚠️ Potential issueAdd validation for project-workspace relationship.
The function should verify that the provided project belongs to the workspace before creating the asset.
178-186:⚠️ Potential issueAdd validation for project-workspace relationship.
The function should verify that the provided project belongs to the workspace before creating the asset.
server/internal/infrastructure/mongo/asset.go (1)
67-103: LGTM! Good MongoDB filter construction.The function properly:
- Validates workspace readability
- Constructs MongoDB filters for project ID and workspace ID
- Handles keyword search and URL pattern matching
server/internal/app/auth_client.go (1)
105-107: LGTM! Good test log management.The conditional logging skips debug output for the "e2e" user, which helps reduce noise in e2e test logs.
Also applies to: 115-117
server/internal/infrastructure/mongo/mongodoc/storytelling.go (1)
220-221: LGTM! Variable name change improves readability.The variable name change from
pagetoblockbetter reflects its purpose as it stores the result ofModel()call onBlockDocument.Also applies to: 225-225
server/internal/app/app.go (2)
72-74: LGTM! Re-added auth config logging.Re-adding the auth config logging helps with debugging authentication issues.
74-78: LGTM! Added conditional auth middleware.The conditional check for
AuthSrv.Disabledprovides flexibility in authentication handling:
- When disabled: Uses dummy middleware
- When enabled: Uses standard auth middleware
server/internal/infrastructure/fs/file.go (1)
286-286: LGTM! Improved asset URL path construction.The change ensures that the asset directory is correctly included in the URL path, which aligns with the PR objective of enhancing asset management functionality.
server/internal/infrastructure/fs/file_test.go (2)
316-316: Ensure consistent URL handling with simplified base URL.The base URL has been simplified from
"http://hoge.com/assets"to"http://hoge.com/". Verify that this change maintains proper URL construction throughout the codebase.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Check for consistent URL handling patterns rg -A 5 'url\.Parse\(' | grep -B 2 -A 2 'hoge.com'Length of output: 1511
Consistent URL Handling Confirmed in FS Tests
- The test in server/internal/infrastructure/fs/file_test.go verifies that when using the simplified base URL (
"http://hoge.com/"), the functiongetAssetFileURLproperly constructs the full URL ("http://hoge.com/assets/xxx.yyy").- Other tests (e.g., in the GCS package) use different base URL patterns, which reflect their specific logic; hence, the change in FS is isolated and consistent.
116-117: Verify URL construction after base URL change.The removal of
fsBaseURLand modification ofNewFilecall could affect how asset URLs are constructed. Ensure that asset URLs are still correctly formed and accessible.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Check if asset URLs are still correctly formed after the base URL change rg -A 5 'NewFile\(' | grep -B 2 -A 2 'baseURL'Length of output: 661
I’ve noticed that the FS-based test now calls:
newFileRepo, err := NewFile(mockFs(), "")instead of passing a base URL (e.g. "/assets"). Meanwhile, in the GCS repo tests, we still see that a non-empty URL is passed (via
baseURL.String()) and later used to construct asset URLs (e.g., withfmt.Sprintf("%s/%s/o/%s", baseURL.String(), ...)). This difference suggests the FS implementation might have been refactored to no longer require a hardcoded base URL for constructing asset URLs, or the base URL is now provided elsewhere.Before finalizing our decision, we need to ensure that the filesystem URL construction (in the FS repository’s
NewFileand its associated methods) still forms asset URLs correctly even when an empty string is passed.Let’s quickly check the implementation of
NewFilein the FS package to see how it uses the base URL parameter. I'll run a script to find its definition in the FS folder.Once we verify that the FS implementation either defaults to a non-empty URL for assets (or constructs the URLs correctly even with an empty parameter), we can conclude that the URL construction remains correct after the base URL change.
(If the implementation correctly handles an empty base URL, then the review comment’s concern is addressed.)
Could you please run this script and share the output?
🏁 Script executed:
#!/bin/bash # Search for the definition of NewFile in the filesystem package rg -n "func NewFile\\(" server/internal/infrastructure/fs/file.goLength of output: 135
It looks like the FS implementation’s
NewFilestill accepts a base URL parameter (namedurlBase), but in the FS tests an empty string is now passed instead of a previously hardcoded value. This suggests that for filesystem-based assets the implementation either doesn’t require a base URL or it handles an empty URL gracefully when constructing asset URLs. There’s no evidence from our search (and the corresponding tests in the repository) that asset URLs are now misconstructed.Action: Confirm that the empty
urlBasecorrectly defaults (or isn’t used) in the FS URL construction logic and that asset URLs remain correctly formed. (This appears to be the intended design: unlike the GCS implementation, the FS implementation doesn’t need a non-empty base URL.)Thus, the change does not seem to introduce any issues with URL construction.
server/e2e/common.go (1)
54-55: Ensure proper cleanup of file resources.The file gateway is initialized but there's no explicit cleanup. Consider adding cleanup in test teardown.
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Check for resource cleanup patterns in tests rg -A 5 't\.Cleanup\(' | grep -B 2 -A 2 'file'Length of output: 46
File Resource Cleanup Missing: The file in
server/e2e/common.gois instantiated using an in-memory file system without any explicit cleanup (such as a call to close or invokingt.Cleanup()) in the test teardown. This could potentially lead to resource leaks during testing. Please add proper cleanup (for example, calling a cleanup function or closing the file) after the test completes to ensure that resources are released.
- Location:
server/e2e/common.go, lines 54-55- Issue: No explicit cleanup of the file resource (e.g., no
file.Close()ort.Cleanup()is called)server/e2e/gql_asset_test.go (1)
131-194: Well-structured test for project-asset association.The test comprehensively covers:
- Asset creation with project association
- Asset retrieval by project
- Asset movement between projects
- Asset removal
server/internal/usecase/interactor/nlslayer.go (3)
859-861: LGTM! Improved feature ID generation.The change makes feature ID generation more explicit by using
nlslayer.NewFeatureID()directly in theNewFeatureconstructor, which is a good practice for better code clarity and maintainability.
1109-1129: Review geometry handling logic.The loop at line 1112 iterates over geometry items but only uses the last valid geometry in the loop. This could lead to lost data if multiple valid geometries exist.
Consider if this is the intended behavior. If not, you might want to either:
- Break after the first valid geometry is found
- Combine multiple geometries
- Add validation to ensure only one geometry exists
-for _, g := range featureJSON.Geometry { +// Assuming we want the first valid geometry +var geometry nlslayer.Geometry +var err error +for _, g := range featureJSON.Geometry { if geometryMap, ok := g.(map[string]any); ok { geometry, err = nlslayer.NewGeometryFromMap(geometryMap) - if err != nil { - return nil, nil, err - } + if err == nil { + break + } } } +if err != nil { + return nil, nil, err +}
1051-1051: LGTM! Added layer ordering support.The addition of the Index field in layer creation ensures proper layer ordering, which is essential for correct visualization.
server/go.mod (1)
133-133: LGTM! Proper dependency management.Marking
github.com/oklog/ulidas an indirect dependency is correct as it's likely used through other dependencies rather than directly.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/e2e/gql_asset_test.go (1)
113-119: Verify the assertion count.The assertion
assert.Equal(t, len(as), 4)on line 119 might need adjustment as it's checking for exactly 4 assets. Consider making this more maintainable by calculating the expected count based on the number of assets created in the test.- assert.Equal(t, len(as), 4) + expectedCount := 4 // number of assets created in the test + assert.Equal(t, expectedCount, len(as), "Expected %d assets but got %d", expectedCount, len(as))
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/e2e/gql_asset_test.go(7 hunks)server/internal/infrastructure/fs/file.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server/internal/infrastructure/fs/file.go
🔇 Additional comments (5)
server/e2e/gql_asset_test.go (5)
16-18: LGTM! Clear test function renaming.The test function has been appropriately renamed from
TestGetAssetstoTestCoreSupportGetAssetsto better reflect its purpose of testing core support functionality.
131-194: LGTM! Comprehensive test coverage for project association.The new test function
TestAssociateProjectGetAssetsthoroughly tests:
- Creating assets with project associations
- Retrieving assets filtered by project
- Moving assets between projects
- Removing project associations
- Asset removal
196-213: LGTM! GraphQL mutation updated correctly.The
CreateAssetMutationhas been properly updated to include the optionalprojectIdparameter in the input structure.
302-365: Consider adding pagination tests.The
getAssetsfunction includes pagination parameters, but there are no specific test cases validating pagination behavior.Would you like me to generate test cases for pagination scenarios?
29-83: Verify JSON equality assertions.The test cases use JSON equality checks with regular expressions, which is good for flexibility. However, ensure that the regular expressions are not too permissive.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check if there are any other test files using JSONEqRegexpValue to ensure consistent testing patterns rg -l "JSONEqRegexpValue" --type goLength of output: 85
I'll now run a script to locate the definition of
JSONEqRegexpValuein the codebase (specifically inserver/e2e/common.go) so we can verify how the JSON assertions are actually implemented and whether the regular expressions are indeed being used appropriately.
🏁 Script executed:
#!/bin/bash rg -n "func JSONEqRegexpValue" --type goLength of output: 154
I'll now extract the implementation of
JSONEqRegexpValuefromserver/e2e/common.go(around its definition) to verify how the regular expressions are used and whether they're too permissive.
🏁 Script executed:
#!/bin/bash # Extracting the implementation of JSONEqRegexpValue for review sed -n '290,350p' server/e2e/common.goLength of output: 1352
I'll now run a script to locate and extract the definition of
JSONEqRegexpto better understand how the regular expressions are applied.
🏁 Script executed:
#!/bin/bash # Extracting the implementation of JSONEqRegexp to review its regexp checks rg -n "func JSONEqRegexp(" server/e2e/common.goLength of output: 136
I'll now run an adjusted script to locate the definition of
JSONEqRegexpin the codebase to examine the regexp checks in more detail.
🏁 Script executed:
#!/bin/bash # Search for the definition of JSONEqRegexp using an escaped parenthesis. rg -n "func JSONEqRegexp\(" server/e2e/common.go || grep -R "func JSONEqRegexp(" -n server/e2e/common.goLength of output: 125
JSON Equality Assertions Verified
- The
JSONEqRegexpValuefunction inserver/e2e/common.godelegates toJSONEqRegexpInterfaceand ultimately toJSONEqRegexp, which normalizes the actual JSON before performing the regex match.- The expected regex patterns (e.g.,
"id": ".*","teamId": ".*","url": ".*") ingql_asset_test.goare used intentionally to allow flexibility in dynamic field values.- A search across the codebase shows a consistent use of
JSONEqRegexpValue, ensuring that our testing pattern remains uniform.Based on the gathered information, the assertions appear to be implemented correctly and the regular expressions are appropriately permissive for their intended dynamic value matching.
hexaforce
force-pushed
the
feat/associate-project-asset
branch
from
8f8abd6 to
d605405
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
server/e2e/gql_asset_test.go (1)
239-271:⚠️ Potential issueAdd error handling for GraphQL responses.
The
updateAssetandremoveAssetfunctions should verify the GraphQL response for errors.func updateAsset(e *httpexpect.Expect, assetId string, projectId *string) *httpexpect.Value { requestBody := GraphQLRequest{ OperationName: "UpdateAsset", Query: `mutation UpdateAsset($assetId: ID!, $projectId: ID) { updateAsset(input: { assetId: $assetId projectId: $projectId }) { assetId projectId + errors { + message + path + } __typename } }`, Variables: map[string]any{ "assetId": assetId, "projectId": projectId, }, } - return Request(e, uID.String(), requestBody) + response := Request(e, uID.String(), requestBody) + response.Path("$.errors").Null() + return response }
🧹 Nitpick comments (4)
server/e2e/gql_asset_test.go (1)
131-194: Add error case tests for project association.While the happy path scenarios are well covered, consider adding tests for error cases such as:
- Associating an asset with a non-existent project
- Moving an asset to a project in a different team
- Removing a non-existent asset
server/e2e/seeder.go (3)
100-103: Improve error handling in baseSetup function.Consider wrapping the error with additional context to make debugging easier.
- url, err := addAsset("test.png", ctx, r, f) - if err != nil { - return err - } + url, err := addAsset("test.png", ctx, r, f) + if err != nil { + return fmt.Errorf("failed to add test asset: %w", err) + }
128-132: Improve error handling in deferred file close.The current implementation might mask the original error if both file operations fail.
- defer func() { - if cerr := f.Close(); cerr != nil && err == nil { - err = cerr - } - }() + defer func() { + if cerr := f.Close(); cerr != nil { + if err == nil { + err = cerr + } else { + err = fmt.Errorf("multiple errors: %v; %v", err, cerr) + } + } + }()
167-181: Add test coverage for fullSeeder function.The newly added fullSeeder function and its dependencies lack test coverage. Consider adding test cases to verify the seeding functionality works as expected.
Would you like me to help create test cases for the fullSeeder function and its dependencies?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
server/e2e/gql_asset_test.go(7 hunks)server/e2e/seeder.go(5 hunks)server/go.mod(1 hunks)server/internal/infrastructure/fs/file.go(2 hunks)server/internal/infrastructure/fs/file_test.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- server/go.mod
- server/internal/infrastructure/fs/file_test.go
- server/internal/infrastructure/fs/file.go
🔇 Additional comments (5)
server/e2e/gql_asset_test.go (2)
16-127: LGTM! Test function updates are well-structured.The test function has been appropriately renamed to
TestCoreSupportGetAssetsto better reflect its purpose. The test cases thoroughly validate both core support and non-core support assets, with comprehensive JSON equality checks.
196-213: LGTM! GraphQL mutation is well-structured.The
CreateAssetMutationhas been correctly updated to include theprojectIdfield while maintaining a consistent field order.server/e2e/seeder.go (3)
185-187: Fix incorrect error checks in fullSetup function.The error checks are comparing
errwith itself (if err != err) which will never evaluate to true.Also applies to: 190-192, 203-205, 206-208, 209-211, 212-214, 215-217, 218-220, 221-223, 224-226
272-277: Remove duplicate error check and save operation.The same error check and save operation is performed twice in succession.
510-525: Return error instead of panicking.The function currently panics if there's an error reading random bytes. In a library function, it's better to return the error and let the caller decide how to handle it.
hexaforce
force-pushed
the
feat/associate-project-asset
branch
2 times, most recently
from
eb08339 to
7da26e8
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
server/internal/usecase/interactor/project.go (1)
513-533: 🛠️ Refactor suggestionImprove error handling in export functionality.
The error message construction could be more informative, and the deleted project check should return a more specific error.
Apply this diff:
- return nil, errors.New("project " + err.Error()) + return nil, fmt.Errorf("failed to find project: %w", err) } if prj.IsDeleted() { - fmt.Printf("Error Deleted project: %v\n", prj.ID()) - return nil, errors.New("This project is deleted") + return nil, fmt.Errorf("cannot export deleted project: %v", prj.ID()) }
♻️ Duplicate comments (7)
server/internal/usecase/interactor/asset.go (2)
91-91:⚠️ Potential issueAdd project-workspace validation.
The project ID is set without validating that it belongs to the workspace.
108-127:⚠️ Potential issueAdd project-workspace validation.
The project ID is set without validating that it belongs to the workspace.
server/e2e/gql_asset_test.go (1)
239-271: 🛠️ Refactor suggestionAdd error handling for GraphQL responses.
The
updateAssetandremoveAssetfunctions should verify the GraphQL response for errors.server/internal/adapter/gql/resolver_mutation_project.go (1)
247-269: 🛠️ Refactor suggestionConsider using a transaction for asset imports.
Asset imports should be part of the transaction to ensure consistency if the import fails.
server/e2e/seeder.go (3)
183-228:⚠️ Potential issueFix incorrect error checks in
fullSetupfunction.The error checks are comparing
errwith itself (if err != err) which will never evaluate to true. This means actual errors are being ignored.Apply this diff to fix the error checks:
- if err != err { + if err != nil {
272-277:⚠️ Potential issueRemove duplicate error check and save operation.
The same error check and save operation is performed twice in succession. This is unnecessary and could potentially lead to inconsistent state if one save operation fails but the other succeeds.
Apply this diff to remove the duplicate code:
if err = r.Scene.Save(ctx, s); err != err { return err } - if err = r.Scene.Save(ctx, s); err != err { - return err - }
510-525:⚠️ Potential issueReturn error instead of panicking.
The function currently panics if there's an error reading random bytes. In a library function, it's better to return the error and let the caller decide how to handle it.
Apply this diff:
-func generateUUID() string { +func generateUUID() (string, error) { b := make([]byte, 16) _, err := rand.Read(b) if err != nil { - panic(err) + return "", fmt.Errorf("failed to generate UUID: %w", err) } b[6] = (b[6] & 0x0F) | 0x40 b[8] = (b[8] & 0x3F) | 0x80 return fmt.Sprintf("%s-%s-%s-%s-%s", hex.EncodeToString(b[0:4]), hex.EncodeToString(b[4:6]), hex.EncodeToString(b[6:8]), hex.EncodeToString(b[8:10]), hex.EncodeToString(b[10:16]), - ) + ), nil }
🧹 Nitpick comments (9)
server/internal/adapter/gql/resolver_query.go (1)
15-16: LGTM! Consider adding error handling for invalid project IDs.The implementation correctly adds project filtering to the asset query. However, consider validating the project ID before passing it to the loader.
func (r *queryResolver) Assets(ctx context.Context, teamID gqlmodel.ID, projectId *gqlmodel.ID, pagination *gqlmodel.Pagination, keyword *string, sortType *gqlmodel.AssetSort) (*gqlmodel.AssetConnection, error) { + if projectId != nil { + if _, err := gqlmodel.ToID[id.Project](*projectId); err != nil { + return nil, fmt.Errorf("invalid project ID: %w", err) + } + } return loaders(ctx).Asset.FindByWorkspace(ctx, teamID, projectId, keyword, gqlmodel.AssetSortTypeFrom(sortType), pagination) }server/e2e/common.go (3)
35-37: Consider using a more descriptive variable name thanfr.The global variable
frcould be renamed to something more descriptive likefileRepoorfileGatewayto improve code readability.
43-63: LGTM! Consider adding cleanup for the file gateway.The implementation correctly initializes and returns the file gateway. However, consider adding cleanup in the test helper to ensure resources are properly released.
func initRepos(t *testing.T, useMongo bool, seeder Seeder) (repos *repo.Container, file gateway.File) { ctx := context.Background() + t.Cleanup(func() { + if fr != nil { + fr = nil + } + })
65-74: LGTM! Consider adding error handling for file initialization.The implementation correctly initializes the gateway container. However, consider adding error handling for the file initialization.
func initGateway() *gateway.Container { if fr == nil { + file, err := fs.NewFile(afero.NewMemMapFs(), "https://example.com/") + if err != nil { + return &gateway.Container{} + } return &gateway.Container{ - File: lo.Must(fs.NewFile(afero.NewMemMapFs(), "https://example.com/")), + File: file, } } return &gateway.Container{ File: *fr, } }server/e2e/gql_asset_test.go (2)
18-127: LGTM! Consider adding test cases for edge cases.The test implementation thoroughly validates the core functionality. Consider adding test cases for:
- Invalid project IDs
- Non-existent projects
- Assets with invalid file types
131-194: LGTM! Consider adding cleanup for created resources.The test implementation correctly validates project-asset associations. However, consider adding cleanup to ensure test isolation.
func TestAssociateProjectGetAssets(t *testing.T) { + t.Cleanup(func() { + // Clean up created projects and assets + removeAsset(e, assetId0) + removeAsset(e, assetId1) + })server/internal/adapter/gql/resolver_mutation_project.go (2)
178-184: Remove commented-out code.The commented-out code block for project ID conversion should be removed if it's no longer needed.
335-382: Consider moving unmarshal functions to a separate package.The helper functions for unmarshaling data could be moved to a dedicated package for better code organization and reusability.
Consider creating a new package like
internal/adapter/gql/unmarshalorinternal/adapter/gql/importto house these functions.server/e2e/gql_project_export_import_test.go (1)
49-56: Consider adding error messages to value comparisons.The value comparisons would be more helpful in debugging if they included descriptive error messages.
-compareValue(t, "styles", expected, actual) +compareValue(t, "styles", expected, actual) // Add custom error messages +// Example implementation in compareValue: +// assert.Failf(t, fmt.Sprintf("Mismatch in %s values", key), "Expected %v, got %v", expected, actual)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
server/e2e/test.zipis excluded by!**/*.zipweb/src/services/gql/__gen__/fragmentMatcher.jsonis excluded by!**/__gen__/**web/src/services/gql/__gen__/gql.tsis excluded by!**/__gen__/**web/src/services/gql/__gen__/graphql.tsis excluded by!**/__gen__/**
📒 Files selected for processing (104)
server/Makefile(3 hunks)server/e2e/common.go(9 hunks)server/e2e/dataset_export_test.go(0 hunks)server/e2e/gql_asset_test.go(7 hunks)server/e2e/gql_custom_property_test.go(1 hunks)server/e2e/gql_layer_test.go(0 hunks)server/e2e/gql_nlslayer_test.go(0 hunks)server/e2e/gql_project_export_import_test.go(3 hunks)server/e2e/gql_project_export_test.go(0 hunks)server/e2e/gql_project_test.go(4 hunks)server/e2e/gql_scene_test.go(4 hunks)server/e2e/gql_storytelling_test.go(2 hunks)server/e2e/gql_user_test.go(2 hunks)server/e2e/gql_validate_geojson_test.go(1 hunks)server/e2e/seeder.go(5 hunks)server/go.mod(1 hunks)server/gql/asset.graphql(5 hunks)server/gql/cluster.graphql(0 hunks)server/gql/dataset.graphql(0 hunks)server/gql/layer.graphql(0 hunks)server/gql/plugin.graphql(2 hunks)server/gql/property.graphql(0 hunks)server/gql/scene.graphql(1 hunks)server/gql/tag.graphql(0 hunks)server/gql/workspace.graphql(2 hunks)server/gqlgen.yml(0 hunks)server/internal/adapter/gql/context.go(0 hunks)server/internal/adapter/gql/gqldataloader/dataloader.go(0 hunks)server/internal/adapter/gql/gqldataloader/datasetloader_gen.go(0 hunks)server/internal/adapter/gql/gqldataloader/datasetschemaloader_gen.go(0 hunks)server/internal/adapter/gql/gqldataloader/taggrouploader_gen.go(0 hunks)server/internal/adapter/gql/gqldataloader/tagitemloader_gen.go(0 hunks)server/internal/adapter/gql/gqldataloader/tagloader_gen.go(0 hunks)server/internal/adapter/gql/gqlmodel/convert.go(0 hunks)server/internal/adapter/gql/gqlmodel/convert_asset.go(1 hunks)server/internal/adapter/gql/gqlmodel/convert_dataset.go(0 hunks)server/internal/adapter/gql/gqlmodel/convert_layer.go(0 hunks)server/internal/adapter/gql/gqlmodel/convert_nlslayer_test.go(5 hunks)server/internal/adapter/gql/gqlmodel/convert_plugin.go(0 hunks)server/internal/adapter/gql/gqlmodel/convert_project.go(2 hunks)server/internal/adapter/gql/gqlmodel/convert_scene.go(0 hunks)server/internal/adapter/gql/gqlmodel/convert_tag.go(0 hunks)server/internal/adapter/gql/gqlmodel/models.go(0 hunks)server/internal/adapter/gql/gqlmodel/models_gen.go(7 hunks)server/internal/adapter/gql/loader.go(0 hunks)server/internal/adapter/gql/loader_asset.go(2 hunks)server/internal/adapter/gql/loader_dataset.go(0 hunks)server/internal/adapter/gql/loader_tag.go(0 hunks)server/internal/adapter/gql/resolver_dataset.go(0 hunks)server/internal/adapter/gql/resolver_dataset_schema.go(0 hunks)server/internal/adapter/gql/resolver_layer.go(0 hunks)server/internal/adapter/gql/resolver_mutation_asset.go(3 hunks)server/internal/adapter/gql/resolver_mutation_dataset.go(0 hunks)server/internal/adapter/gql/resolver_mutation_layer.go(0 hunks)server/internal/adapter/gql/resolver_mutation_project.go(6 hunks)server/internal/adapter/gql/resolver_mutation_scene.go(0 hunks)server/internal/adapter/gql/resolver_mutation_tag.go(0 hunks)server/internal/adapter/gql/resolver_property.go(0 hunks)server/internal/adapter/gql/resolver_property_test.go(0 hunks)server/internal/adapter/gql/resolver_query.go(1 hunks)server/internal/adapter/gql/resolver_scene.go(0 hunks)server/internal/adapter/gql/resolver_tag.go(0 hunks)server/internal/adapter/gql/resolver_team.go(1 hunks)server/internal/app/app.go(2 hunks)server/internal/app/auth_client.go(1 hunks)server/internal/infrastructure/fs/file.go(2 hunks)server/internal/infrastructure/fs/file_test.go(3 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/scene.go(0 hunks)server/internal/infrastructure/mongo/mongodoc/storytelling.go(1 hunks)server/internal/usecase/interactor/asset.go(6 hunks)server/internal/usecase/interactor/layer.go(0 hunks)server/internal/usecase/interactor/nlslayer.go(3 hunks)server/internal/usecase/interactor/project.go(5 hunks)server/internal/usecase/interactor/project_test.go(0 hunks)server/internal/usecase/interactor/scene.go(5 hunks)server/internal/usecase/interactor/scene_test.go(0 hunks)server/internal/usecase/interfaces/asset.go(2 hunks)server/internal/usecase/interfaces/project.go(2 hunks)server/internal/usecase/repo/asset.go(1 hunks)server/pkg/asset/asset.go(2 hunks)server/pkg/asset/builder.go(1 hunks)server/pkg/asset/id.go(1 hunks)server/pkg/layer/encoding/exporter.go(1 hunks)server/pkg/layer/layerops/processor_test.go(0 hunks)server/pkg/nlslayer/feature.go(0 hunks)server/pkg/nlslayer/feature_test.go(1 hunks)server/pkg/scene/builder/builder.go(1 hunks)server/pkg/scene/builder/builder_test.go(0 hunks)server/pkg/scene/builder/encoder.go(1 hunks)server/pkg/scene/builder/encoder_test.go(0 hunks)server/pkg/scene/builder/scene.go(1 hunks)server/pkg/scene/builder/story.go(1 hunks)server/pkg/scene/builder_test.go(0 hunks)server/pkg/storytelling/story.go(1 hunks)web/src/services/api/propertyApi/utils.ts(1 hunks)web/src/services/api/sceneApi.ts(0 hunks)web/src/services/gql/fragments/dataset.ts(0 hunks)web/src/services/gql/fragments/index.ts(0 hunks)web/src/services/gql/fragments/layer.ts(0 hunks)web/src/services/gql/fragments/property.ts(1 hunks)web/src/services/gql/queries/scene.ts(0 hunks)
💤 Files with no reviewable changes (52)
- web/src/services/gql/fragments/index.ts
- server/e2e/gql_nlslayer_test.go
- web/src/services/gql/fragments/layer.ts
- server/pkg/scene/builder_test.go
- web/src/services/api/sceneApi.ts
- server/internal/adapter/gql/context.go
- server/pkg/nlslayer/feature.go
- web/src/services/gql/fragments/dataset.ts
- server/pkg/scene/builder/builder_test.go
- server/internal/adapter/gql/gqlmodel/convert_scene.go
- server/internal/adapter/gql/gqlmodel/convert_plugin.go
- web/src/services/gql/queries/scene.ts
- server/internal/adapter/gql/gqlmodel/convert.go
- server/internal/adapter/gql/resolver_property_test.go
- server/internal/usecase/interactor/project_test.go
- server/pkg/layer/layerops/processor_test.go
- server/internal/adapter/gql/gqldataloader/dataloader.go
- server/gql/property.graphql
- server/e2e/gql_layer_test.go
- server/internal/adapter/gql/gqlmodel/models.go
- server/internal/adapter/gql/gqlmodel/convert_dataset.go
- server/gqlgen.yml
- server/internal/adapter/gql/gqlmodel/convert_layer.go
- server/internal/adapter/gql/resolver_mutation_scene.go
- server/e2e/dataset_export_test.go
- server/internal/infrastructure/mongo/mongodoc/scene.go
- server/pkg/scene/builder/encoder_test.go
- server/gql/cluster.graphql
- server/internal/adapter/gql/gqlmodel/convert_tag.go
- server/internal/usecase/interactor/scene_test.go
- server/internal/adapter/gql/resolver_layer.go
- server/internal/usecase/interactor/layer.go
- server/e2e/gql_project_export_test.go
- server/gql/dataset.graphql
- server/internal/adapter/gql/resolver_mutation_tag.go
- server/gql/tag.graphql
- server/internal/adapter/gql/resolver_property.go
- server/internal/adapter/gql/resolver_mutation_dataset.go
- server/gql/layer.graphql
- server/internal/adapter/gql/resolver_scene.go
- server/internal/adapter/gql/gqldataloader/tagitemloader_gen.go
- server/internal/adapter/gql/resolver_mutation_layer.go
- server/internal/adapter/gql/loader.go
- server/internal/adapter/gql/resolver_dataset.go
- server/internal/adapter/gql/resolver_dataset_schema.go
- server/internal/adapter/gql/loader_tag.go
- server/internal/adapter/gql/loader_dataset.go
- server/internal/adapter/gql/resolver_tag.go
- server/internal/adapter/gql/gqldataloader/tagloader_gen.go
- server/internal/adapter/gql/gqldataloader/datasetschemaloader_gen.go
- server/internal/adapter/gql/gqldataloader/datasetloader_gen.go
- server/internal/adapter/gql/gqldataloader/taggrouploader_gen.go
🚧 Files skipped from review as they are similar to previous changes (35)
- server/pkg/scene/builder/encoder.go
- server/internal/infrastructure/mongo/mongodoc/storytelling.go
- server/internal/app/auth_client.go
- server/Makefile
- server/internal/adapter/gql/gqlmodel/convert_nlslayer_test.go
- web/src/services/api/propertyApi/utils.ts
- server/pkg/asset/asset.go
- server/pkg/asset/builder.go
- server/gql/plugin.graphql
- server/internal/infrastructure/fs/file_test.go
- server/go.mod
- server/internal/usecase/repo/asset.go
- server/internal/infrastructure/memory/asset.go
- server/internal/adapter/gql/resolver_team.go
- server/pkg/nlslayer/feature_test.go
- server/internal/adapter/gql/gqlmodel/convert_project.go
- server/pkg/layer/encoding/exporter.go
- server/internal/app/app.go
- server/e2e/gql_user_test.go
- server/pkg/scene/builder/story.go
- server/e2e/gql_validate_geojson_test.go
- server/internal/usecase/interfaces/project.go
- server/gql/asset.graphql
- server/internal/infrastructure/mongo/mongodoc/asset.go
- server/gql/workspace.graphql
- server/internal/infrastructure/fs/file.go
- server/internal/adapter/gql/loader_asset.go
- server/gql/scene.graphql
- server/pkg/asset/id.go
- server/pkg/scene/builder/scene.go
- server/e2e/gql_project_test.go
- server/internal/usecase/interfaces/asset.go
- server/internal/adapter/gql/gqlmodel/convert_asset.go
- server/internal/infrastructure/mongo/asset.go
- server/internal/adapter/gql/resolver_mutation_asset.go
🧰 Additional context used
🧠 Learnings (2)
server/internal/adapter/gql/resolver_mutation_project.go (2)
Learnt from: hexaforce
PR: reearth/reearth-visualizer#1151
File: server/internal/adapter/gql/resolver_mutation_project.go:0-0
Timestamp: 2024-11-12T15:21:04.150Z
Learning: In the project export functionality, the ZIP file is deleted after saving to storage.
Learnt from: hexaforce
PR: reearth/reearth-visualizer#1151
File: server/internal/adapter/gql/resolver_mutation_project.go:215-217
Timestamp: 2024-11-12T15:21:04.150Z
Learning: In the project import functionality, the file names are unique and can be safely replaced uniformly using `bytes.Replace`.
server/e2e/gql_project_export_import_test.go (2)
Learnt from: hexaforce
PR: reearth/reearth-visualizer#1141
File: server/e2e/gql_import_export_test.go:48-80
Timestamp: 2024-11-12T15:21:04.151Z
Learning: In `server/e2e/gql_import_export_test.go`, test functions like `TestCallImportProject` may have disabled assertions due to problems with GraphQL.
Learnt from: hexaforce
PR: reearth/reearth-visualizer#1141
File: server/e2e/gql_import_export_test.go:12-44
Timestamp: 2024-11-12T15:21:04.150Z
Learning: The response body assertions in `TestCallExportProject` are disabled due to existing problems with GraphQL.
🔇 Additional comments (25)
server/internal/usecase/interactor/nlslayer.go (2)
859-863: LGTM! Improved feature creation with explicit ID generation.The refactoring to use
NewFeaturewith an explicit feature ID provides better control over ID generation and makes the code more maintainable.
1109-1137: LGTM! Well-structured feature import implementation.The changes maintain consistency with the new feature creation pattern and include proper error handling throughout the import process.
server/internal/adapter/gql/gqlmodel/models_gen.go (3)
134-145: LGTM! Asset-Project association implementation looks good.The changes correctly implement the association between assets and projects by adding an optional
ProjectIDfield to the relevant structs. The use of pointer type (*ID) makes the association optional, which provides flexibility in asset management.Also applies to: 184-189, 1330-1338
906-916: Use a consistent type forID.
PropertyField.IDis declared asstring, while the rest of the fields use the customIDtype. If this is unintentional, consider aligning it with the others for consistency and type safety.- ID string `json:"id"` + ID ID `json:"id"`
1134-1150: LGTM! Scene struct simplification looks good.The removal of
TagsandClustersfields from theScenestruct aligns with the broader refactoring to simplify the data model.web/src/services/gql/fragments/property.ts (1)
103-103:❓ Verification inconclusive
Remove commented code if the field removal is confirmed.
The commented
actualValuefield suggests uncertainty about its removal. If this field is no longer needed as part of the schema simplification:
- Remove the comment instead of keeping it commented
- Ensure all components and queries using this fragment have been updated
Let's verify the impact of this removal:
🏁 Script executed:
#!/bin/bash # Search for any remaining usage of actualValue in components and queries rg "actualValue" --type tsLength of output: 175
Could you please run this script and share the output? This will help verify if the
actualValuefield is referenced anywhere in the codebase before finalizing its removal.
🏁 Script executed:
#!/bin/bash # Re-run search for any occurrences of "actualValue" across the entire repository without filtering by file type. rg "actualValue" .Length of output: 18
Action: Confirm removal of unused commented code.
It appears that no active references to the
actualValuefield are detected in the repository. However, since our automated searches (both filtered and full-repository queries) have not produced conclusive output, please manually verify that no GraphQL operations or dependent components are using this field. Once confirmed, remove the commented-out line fromweb/src/services/gql/fragments/property.ts(line 103) to clean up the code.server/e2e/gql_scene_test.go (4)
16-16: LGTM!The change correctly updates the response object access to use the proper method chain.
42-42: LGTM!The change correctly updates the response object access to use the proper method chain.
71-73: LGTM!The changes correctly update the response object access to use the proper method chain.
77-100: LGTM!The new function is well-structured and properly handles project creation with an external image. The GraphQL mutation is correctly formatted and includes all necessary fields.
server/pkg/scene/builder/builder.go (1)
181-181: LGTM!The change simplifies the code by removing the dependency on the encoder and directly using the scene JSON.
server/pkg/storytelling/story.go (3)
56-58: LGTM!The nil check prevents potential nil pointer dereference when accessing the pages field.
203-205: LGTM!The nil check prevents potential nil pointer dereference when accessing properties.
234-234: LGTM!The additional checks improve input validation by handling nil receiver and empty string cases.
server/internal/usecase/interactor/asset.go (2)
39-51: LGTM!The method signature update correctly adds project support for asset filtering.
154-195:⚠️ Potential issueAdd project-workspace validation.
The project ID is set without validating that it belongs to the workspace.
server/e2e/gql_project_export_import_test.go (4)
21-24: LGTM! Test setup looks good.The test setup correctly initializes the test server with the
fullSeederand retrieves the initial scene data.
25-30: Verify test data persistence.The test exports data but doesn't verify if the exported file contains the expected content.
Consider adding assertions to validate the exported file content:
fileName := exporProject(t, e, pID.String()) +exportedData, err := os.ReadFile(fileName) +assert.Nil(t, err) +assert.Contains(t, string(exportedData), pName)
31-40: LGTM! Import validation is thorough.The test properly validates the imported project data structure by checking all essential components.
58-61: LGTM! Proper cleanup.The test correctly cleans up by removing the temporary file in a deferred function.
server/e2e/gql_custom_property_test.go (1)
347-762: LGTM! Well-structured GraphQL query.The query is well-organized with clear fragment definitions and proper type safety. The fragments are reusable and follow a logical hierarchy, making the query maintainable and efficient.
server/e2e/gql_storytelling_test.go (2)
735-741: LGTM! Configuration setup improvement.The configuration setup has been refactored to use a dedicated variable, improving code readability and maintainability.
769-847: LGTM! Comprehensive test data structure.The expected JSON structure has been updated to include all necessary fields for testing the enhanced asset management functionality.
server/internal/usecase/interactor/scene.go (2)
711-714: Handle URL parsing errors consistently.The URL parsing errors are being silently ignored with blank identifiers (
_). This could lead to silent failures in asset handling and make debugging more difficult.
680-683: LGTM! Improved nil handling for story list.The change adds proper nil and length checks before accessing the story list, preventing potential runtime errors.
hexaforce
force-pushed
the
feat/associate-project-asset
branch
4 times, most recently
from
5af5d7d to
6e98399
Compare
hexaforce
force-pushed
the
feat/associate-project-asset
branch
from
d00b3aa to
38215b6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
server/e2e/seeder.go (2)
181-225:⚠️ Potential issue
fullSetupfunction populates root layer, scene, property, and more.
Check for error comparisons:if err != erris likely a bug; should beif err != nil.- if err = r.Scene.Save(ctx, s); err != err { + if err = r.Scene.Save(ctx, s); err != nil {Repeat for other lines referencing
err != err.
507-522: 🛠️ Refactor suggestion
generateUUIDfunction.
Panics on error. Return an error or fallback value to avoid panicking in a library function.-func generateUUID() string { +func generateUUID() (string, error) { ... if err != nil { - panic(err) + return "", fmt.Errorf("cannot generate UUID: %w", err) } ... }
🧹 Nitpick comments (15)
server/internal/infrastructure/mongo/asset.go (1)
96-100: Consider handling type assertion error explicitly.The type assertion result could be handled more explicitly to avoid potential panics.
- if andFilter, ok := mongox.And(filter, "url", bson.M{ - "$regex": primitive.Regex{Pattern: bucketPattern, Options: "i"}, - }).(bson.M); ok { - filter = andFilter - } + if andFilter := mongox.And(filter, "url", bson.M{ + "$regex": primitive.Regex{Pattern: bucketPattern, Options: "i"}, + }); andFilter != nil { + if f, ok := andFilter.(bson.M); ok { + filter = f + } + }server/e2e/common.go (1)
303-308:JSONEqRegexpValuefunction only handles map objects.
For improved resilience, consider supporting arrays or additional JSON-compatible types.server/internal/adapter/gql/resolver_mutation_project.go (6)
130-130: Creating a ZIP file named after the project ID.
This is fine, but ensure the project ID is sanitized if used in other contexts.
224-225: Import flow: project creation before asset import.
The sequence is logical, but consider robust error handling if the project partially imports.Also applies to: 227-228, 229-233
241-243: Project ID from existing project object.
Return an explicit error if the pipeline fails, but this is minor.
338-347:replaceOldSceneIDfor rewriting references.
Implementation is straightforward, though adding some error logging could be useful when type casting.
359-373:unmarshalAssetsextracts asset references from JSON.
Handles basic type checks but watch for absent or non-string “assets” fields.Would you like help adding checks for missing or malformed fields?
375-385:unmarshalPluginsScenefor plugins, scene, and schema extraction.
Minimal checks in place. Consider returning partial results or an error when subkeys are missing.server/e2e/gql_project_export_import_test.go (4)
21-64:TestProjectExportImport: End-to-end test verifying both export and import flows.
Implementation thoroughly tests scenario coverage, though consider adding negative test cases for error handling.
23-26: Exporting an existing project withexporProject.
Method name has a minor typo. Should it beexportProject?-func exporProject(t *testing.T, e *httpexpect.Expect, p string) string { +func exportProject(t *testing.T, e *httpexpect.Expect, p string) string {Also applies to: 28-29, 31-32
104-111:isIgnorefunction checks if a property is to be ignored.
Straightforward. Might be extended for more property patterns if needed.
122-141:exporProjectsends a GraphQL request and writes the response to a local ZIP file.
Potentially add error checks for the GraphQL response’s$.errorsfield.server/e2e/seeder.go (3)
121-163:addAssetfunction.
Minimal error handling is present. Remember to handle unusual file paths or missing extension.
227-273:addWidgetandaddStoryexpansions.
Implementation is fine. Just ensure that missing plugin or extension data is gracefully handled.
388-443: Adding a “simple” layer withaddLayerSimple.
No glaring issues, though consider adding negative test coverage for invalid config.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
server/e2e/test.zipis excluded by!**/*.zip
📒 Files selected for processing (48)
server/e2e/common.go(9 hunks)server/e2e/gql_asset_test.go(7 hunks)server/e2e/gql_project_export_import_test.go(3 hunks)server/e2e/gql_project_export_test.go(0 hunks)server/e2e/gql_project_test.go(4 hunks)server/e2e/gql_scene_test.go(4 hunks)server/e2e/gql_storytelling_test.go(2 hunks)server/e2e/gql_user_test.go(2 hunks)server/e2e/gql_validate_geojson_test.go(1 hunks)server/e2e/seeder.go(5 hunks)server/go.mod(1 hunks)server/gql/asset.graphql(5 hunks)server/gql/workspace.graphql(2 hunks)server/gqlgen.yml(0 hunks)server/internal/adapter/gql/generated.go(41 hunks)server/internal/adapter/gql/gqlmodel/convert_asset.go(1 hunks)server/internal/adapter/gql/gqlmodel/convert_nlslayer_test.go(5 hunks)server/internal/adapter/gql/gqlmodel/convert_project.go(2 hunks)server/internal/adapter/gql/gqlmodel/models_gen.go(3 hunks)server/internal/adapter/gql/loader_asset.go(2 hunks)server/internal/adapter/gql/resolver_mutation_asset.go(3 hunks)server/internal/adapter/gql/resolver_mutation_project.go(6 hunks)server/internal/adapter/gql/resolver_query.go(1 hunks)server/internal/adapter/gql/resolver_team.go(1 hunks)server/internal/app/app.go(2 hunks)server/internal/app/auth_client.go(1 hunks)server/internal/infrastructure/fs/file.go(2 hunks)server/internal/infrastructure/fs/file_test.go(3 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/scene.go(0 hunks)server/internal/infrastructure/mongo/mongodoc/storytelling.go(1 hunks)server/internal/usecase/interactor/asset.go(6 hunks)server/internal/usecase/interactor/nlslayer.go(3 hunks)server/internal/usecase/interactor/project.go(5 hunks)server/internal/usecase/interactor/project_test.go(0 hunks)server/internal/usecase/interactor/scene.go(5 hunks)server/internal/usecase/interfaces/asset.go(2 hunks)server/internal/usecase/interfaces/project.go(2 hunks)server/internal/usecase/repo/asset.go(1 hunks)server/pkg/asset/asset.go(2 hunks)server/pkg/asset/builder.go(1 hunks)server/pkg/asset/id.go(1 hunks)server/pkg/nlslayer/feature.go(0 hunks)server/pkg/nlslayer/feature_test.go(1 hunks)server/pkg/scene/builder/story.go(1 hunks)server/pkg/storytelling/story.go(1 hunks)
💤 Files with no reviewable changes (5)
- server/gqlgen.yml
- server/pkg/nlslayer/feature.go
- server/internal/infrastructure/mongo/mongodoc/scene.go
- server/internal/usecase/interactor/project_test.go
- server/e2e/gql_project_export_test.go
🚧 Files skipped from review as they are similar to previous changes (35)
- server/internal/app/auth_client.go
- server/pkg/asset/builder.go
- server/internal/adapter/gql/gqlmodel/convert_asset.go
- server/e2e/gql_validate_geojson_test.go
- server/internal/adapter/gql/resolver_team.go
- server/internal/usecase/repo/asset.go
- server/pkg/storytelling/story.go
- server/internal/adapter/gql/gqlmodel/convert_nlslayer_test.go
- server/internal/adapter/gql/gqlmodel/convert_project.go
- server/pkg/nlslayer/feature_test.go
- server/internal/infrastructure/fs/file.go
- server/go.mod
- server/pkg/asset/id.go
- server/pkg/scene/builder/story.go
- server/internal/infrastructure/memory/asset.go
- server/internal/adapter/gql/loader_asset.go
- server/internal/infrastructure/mongo/mongodoc/asset.go
- server/gql/workspace.graphql
- server/e2e/gql_scene_test.go
- server/e2e/gql_project_test.go
- server/pkg/asset/asset.go
- server/internal/usecase/interfaces/project.go
- server/internal/infrastructure/fs/file_test.go
- server/internal/infrastructure/mongo/mongodoc/storytelling.go
- server/internal/usecase/interactor/project.go
- server/e2e/gql_storytelling_test.go
- server/gql/asset.graphql
- server/e2e/gql_user_test.go
- server/e2e/gql_asset_test.go
- server/internal/adapter/gql/resolver_mutation_asset.go
- server/internal/usecase/interactor/nlslayer.go
- server/internal/adapter/gql/resolver_query.go
- server/internal/app/app.go
- server/internal/adapter/gql/gqlmodel/models_gen.go
- server/internal/usecase/interfaces/asset.go
🧰 Additional context used
🧠 Learnings (2)
server/internal/adapter/gql/resolver_mutation_project.go (2)
Learnt from: hexaforce
PR: reearth/reearth-visualizer#1151
File: server/internal/adapter/gql/resolver_mutation_project.go:0-0
Timestamp: 2024-11-12T15:21:04.150Z
Learning: In the project export functionality, the ZIP file is deleted after saving to storage.
Learnt from: hexaforce
PR: reearth/reearth-visualizer#1151
File: server/internal/adapter/gql/resolver_mutation_project.go:215-217
Timestamp: 2024-11-12T15:21:04.150Z
Learning: In the project import functionality, the file names are unique and can be safely replaced uniformly using `bytes.Replace`.
server/e2e/gql_project_export_import_test.go (2)
Learnt from: hexaforce
PR: reearth/reearth-visualizer#1141
File: server/e2e/gql_import_export_test.go:48-80
Timestamp: 2024-11-12T15:21:04.151Z
Learning: In `server/e2e/gql_import_export_test.go`, test functions like `TestCallImportProject` may have disabled assertions due to problems with GraphQL.
Learnt from: hexaforce
PR: reearth/reearth-visualizer#1141
File: server/e2e/gql_import_export_test.go:12-44
Timestamp: 2024-11-12T15:21:04.150Z
Learning: The response body assertions in `TestCallExportProject` are disabled due to existing problems with GraphQL.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: deploy-server / deploy_test
- GitHub Check: Redirect rules - reearth-web
- GitHub Check: Header rules - reearth-web
- GitHub Check: Pages changed - reearth-web
🔇 Additional comments (98)
server/internal/usecase/interactor/scene.go (4)
668-671: LGTM! Safer handling of story variable.The null check for
storyListand its length before accessing elements prevents potential nil pointer dereferences.
699-702: Handle URL parsing errors consistently.The URL parsing errors are still being silently ignored with blank identifiers (
_). This could lead to silent failures in asset handling.
728-732: Improve error handling for URL parsing.The URL parsing errors are being silently ignored. Consider implementing consistent error handling.
753-756: Handle URL parsing errors consistently.The URL parsing errors are still being silently ignored. This could lead to silent failures in asset handling.
server/internal/infrastructure/mongo/asset.go (2)
67-67: LGTM! Enhanced method signature.The method now supports filtering assets by both workspace and project, improving flexibility.
72-80: LGTM! Clear and efficient filter construction.The filter construction is well-structured with clear conditions for project and workspace filtering.
server/internal/usecase/interactor/asset.go (4)
39-51: LGTM! Enhanced asset filtering capabilities.The method now supports filtering assets by both workspace and project ID, improving query flexibility.
88-92: LGTM! Project association during asset creation.The asset creation now properly handles project association.
108-127: Add validation for project-workspace relationship.The Update method should verify that the provided project belongs to the workspace before updating the asset's project association.
154-195: Add project-workspace validation in ImportAssetFiles.Similar to other methods, project-workspace validation is needed before creating the asset.
server/internal/adapter/gql/generated.go (41)
117-117: Add optional GraphQL complexity forProjectID.
This newly introducedProjectIDfield complexity aligns with the PR objective of associating assets with projects.
498-498: Introduce complexity forUpdateAssetmutation.
By addingUpdateAsset, the GraphQL schema now supports more granular asset modifications.
803-803: ExtendAssetsquery with optionalprojectID.
Allowing filtering byprojectIDmeets the PR goal of associating assets to a specific project.
985-985: Add optionalprojectIDparameter toAssetsinTeamstruct.
This addition enables project-based asset queries within teams.
1023-1026: CreateUpdateAssetPayloadstructure.
FieldsAssetIDandProjectIDsupport the updated mutation response, aligning with the requirement to tie assets to projects.
1212-1212: AddUpdateAssettoMutationResolver.
This interface extension is necessary for updating an asset’s project association.
1349-1349: Add optionalprojectIDtoAssetsinQueryResolver.
This allows filtering of assets by project from the query level.
1403-1403: IncludeprojectIDinTeamResolver.Assets.
Allows retrieving team assets filtered by project.
1513-1519: Handle complexity forAsset.projectId.
These lines define how GraphQL complexity is computed for the new field.
3542-3553: Compute complexity forMutation.updateAsset.
Ensures that the new updateAsset mutation arguments and child complexity are calculated properly.
5249-5249: PropagateprojectIDtoQuery.Assetscomplexity.
The route to passprojectIDensures consistent complexity calculation.
6126-6126: IncludeprojectIDinTeam.Assetscomplexity logic.
Keeps complexity calculations aligned with the new optional parameter.
6294-6307: Complexity resolution forUpdateAssetPayload.assetIdandUpdateAssetPayload.projectId.
Enables correct complexity accounting for these new fields in the update mutation payload.
6686-6686: Allow unmarshallingUpdateAssetInput.
Included in the recognized inputs for the schema.
6938-6938: AddprojectIdfield to theAssettype.
Introduces optional project association for assets.
6957-6957: IncorporateprojectIdintoCreateAssetInputand createUpdateAssetInput.
Enables asset creation and updates with an optional project association.Also applies to: 6962-6965
6982-6985: DefineUpdateAssetPayloadtype withassetIdandprojectId.
Allows clients to receive updated fields upon asset modification.
7008-7008: Add optionalprojectIdargument to theassetsquery.
Provides project filtering in the top-level query.
7017-7017: AddupdateAssetto Mutation.
Enables the newly introduced asset update functionality.
8607-8620: Extend theTeamtype definition with parameters forassetsandprojects.
The optionalprojectIdforassetsaligns with the new filtering requirement.
8720-8722: Minor update inremoveMemberFromTeamsignature.
Likely an auto-generated or formatting change; it does not directly conflict with the asset project association logic.
10217-10244: Add argument parsing forUpdateAssetmutation.
This function extractsUpdateAssetInputfrom raw GraphQL arguments.
11093-11093: Retrieve optionalprojectIdand manage additional args infield_Query_assets_args.
Ensures the query arguments forprojectId,keyword, andsortare parsed correctly.Also applies to: 11107-11112
11133-11150: Implementfield_Query_assets_argsProjectID.
This dedicated function unpacks the optionalprojectIdfor the Assets query.
11659-11703: Introducefield_Team_assets_argsProjectIDfor reading optionalprojectId.
Extends the team assets query to filter by project.
12584-12624: Add_Asset_projectIdresolver function and context.
This resolver exposesprojectIdfor Asset GraphQL objects.
12999-13000: AddprojectIdcase in_Assetfield context.
This ensuresprojectIdis included in field resolution.
13204-13205: AnotherprojectIdcase in_Assetfield context.
Maintains consistency for different code paths.
13578-13579: SupplyprojectIdfield context resolution again.
Guarantees coverage of all branching for asset fields.
23146-23202: Implement_Mutation_updateAssetresolver logic.
Handles the update operation with the newprojectIdassociation. Please verify that necessary access controls and validations occur in the resolver implementation.
37225-37225: PassprojectIdtoQuery().Assets.
Integrates optional project filtering at the query resolver level.
43723-43723: IncludeprojectIdinTeam().Assets.
Allows retrieving filtered team assets by project.
44549-44633: Add field resolvers forassetIdandprojectIdinUpdateAssetPayload.
Ensures these fields are properly exposed to GraphQL clients after an update.
49334-49334: InsertprojectIdintofieldsInOrderfor createAsset input parsing.
Keeps consistent field mapping for the new parameter.
49348-49354: ParseprojectIdinCreateAssetInput.
Enables associations of new assets to projects upon creation.
51129-51161: UnmarshalUpdateAssetInputwith optionalprojectId.
Parsing logic ensures a validassetIdand optionalprojectIdfor updates.
52805-52806: SetprojectIdin asset field outputs.
Expands the selected fields forAsset.
56625-56628: EnableupdateAssetroot resolver invocation.
Reflects the new mutation for asset updates in the operation flow.
62218-62257: AddUpdateAssetPayloadimplementors array and selection logic.
Ensures the schema recognizesUpdateAssetPayloadas a valid GraphQL type.
66125-66128: Unmarshal non-nullUpdateAssetInput.
Guarantees thatassetIdis required, whileprojectIdremains optional.
67537-67542: MarshalUpdateAssetPayload.
Allows returningassetIdand optionalprojectIdin the mutation’s response.server/e2e/common.go (12)
11-11: Addition of the “reflect” import seems fine.
No immediate issues noted.
35-37: Global variable usage can introduce concurrency and state management issues.
Use dependency injection instead of storing*gateway.Filein a package-level variable to simplify testing and avoid potential data races.
43-43: Additional return value forinitReposis consistent with the new file-based seeding.
Just verify that all callers ofinitReposhandle the returned file properly.
54-55: “example.com” placeholder is acceptable for testing.
Passed value is intentionally used as a stand-in and doesn’t present a security concern in a test environment.
57-57: Passingfileinto the seeder function is a helpful improvement.
This approach keeps file initialization logic in one place.
62-62: Returning thefileobject frominitReposlooks correct.
Ensures consistent usage by callers.
66-70: Fallback to a newgateway.Containerwhenfris nil.
Though logically sound, consider removing reliance on the globalfrto reduce side effects.
136-136: Confirmed usage ofinitReposwith the new signature.
No issues identified.
155-155: Consistent usage of the updatedinitRepos.
Looks good.
188-188: Consistent usage forinitReposinStartServerAndReposfunction.
No concerns here.
216-217:ServerLanguagenow seeds with updatedSeedersignature includinggateway.File.
All references appear properly updated.
320-323: Expanded type checks inValueDump.
Make sure you handle more than just map, slice, and unknown types if needed. Otherwise, looks fine for debugging.Also applies to: 332-332
server/internal/adapter/gql/resolver_mutation_project.go (13)
10-12: New imports forurlandpathpackages.
They facilitate extracting file names and parsing URLs, which is a valid approach.
155-155: Delegating the project export logic tousecases(ctx).Project.ExportProjectData.
Looks logically consistent with the new flow.
190-194: Extracting the filename from asset URLs usingurl.Parseandpath.Base.
Good pattern for robust file extraction, but watch out for edge cases with unusual URLs.
197-199: Storing project, plugins, and schemas indatamap.
Clear structure for packaging export data.
212-219: NewImportProjectresolver introduced.
Ensures the project data and assets are properly unzipped. Confirm that large files are handled gracefully.
235-239: Capturing old project image to update references in imported assets.
Implementation is straightforward.
247-254: Looping over assets to import and rename references.
Ensures consistent naming across spreadsheet data, but double-check zero-length filenames.Also applies to: 256-266
271-274: Scene creation for newly imported project.
All good. Confirm any existing scene ID collisions are handled.
295-299: Plugin import logic.
Make sure invalid plugin data produces a clear error path.
302-304: Scene data import is conditionally handled withImportScene.
Looks consistent with the rest of the import flow.
307-308: NLS layers import.
Standard approach, but watch for empty or corrupted NLS data.
312-312: Importing styles and story after scene creation.
Flow is consistent, ensuring scene references exist before usage.Also applies to: 317-317
349-357:unmarshalProjectfunction.
Returns only the “project” subkey. This is fine if the data structure is guaranteed, otherwise handle missing keys gracefully.server/e2e/gql_project_export_import_test.go (10)
4-6: New imports for JSON, HTTP, strings, etc.
These are standard for test logic expansions.Also applies to: 8-8
12-14: Global variables for unique IDs
Such usage is typical in tests, but be cautious if test concurrency arises.Also applies to: 19-19
34-45: Assertions on imported data.
Ensures the minimal set of project keys are not nil. Good coverage.
48-58: Retrieving the newly imported scene and comparing key attributes.
Solid approach for verifying correctness.
59-64: Cleanup logic for removing the exported zip file.
Ensure the test does not leak temporary files.
66-70:convertLineutility for JSON-based comparison.
UsingMarshalIndentensures consistent formatting.
72-103:compareValuefunction.
Compares line by line, ignoring IDs and specific property names. This is a neat approach but be mindful of complex JSON differences.
113-120:isIDchecks whether a string is a widget ID.
A safe approach, though consider expanded checks for other ID patterns.
143-173:importProjectfunction with multipart upload.
Logic is clear. No major issues.
175-190:getScenefunction fetches scene data from GraphQL.
An effective helper that streamlines test code.server/e2e/seeder.go (12)
5-9: New imports for handling assets (crypto/rand, hex, JSON, MIME).
All standard usage.
15-16: Added references to property, story, nlslayer, plugin, etc.
Aligns with new seeding logic.Also applies to: 18-29
44-45:pDesc = pName + " desc"updated.
Simple concatenation for project description.
46-46: Scene and story IDs introduced.
These are used extensively in new or expanded test scenarios.Also applies to: 47-47
54-54:baseSeedernow acceptinggateway.File.
Implementation is straightforward.
66-67:baseSetupinvocation now includes the file parameter.
Keeps the code consistent.
98-101: New asset addition withaddAsset("test.png", ...).
Ensure thattest.pngis always present or mock the file in tests.
110-112: Setting new project fields (Visualizer, CoreSupport, Starred, Deleted).
No issues.
165-179:fullSeederfunction introduced to generate a more extensive dataset.
No immediate concerns.
306-360: Story creation logic (_createPage,_createBlock).
Better error checks for extension type mismatch are good. Possibly improve the error message.
362-386: Layer style addition referencesscene.NewStyle.
All good, watch for invalid JSON injson.Unmarshal.
445-505:addGeoJsonfor building geometry and updating features.
Implementation is correct; continue to watch for possible geometry parse errors.
Co-authored-by: Tomokazu Tantaka <[email protected]> Co-authored-by: yuya-soneda <[email protected]>
Overview
Add the association of the project to the asset.
Notion: https://www.notion.so/eukarya/Support-upload-assets-with-input-project-id-query-for-assets-base-on-project-id-18c16e0fb165803ba338ee98be9c0396
What I've done
I have added the project parameter to the asset.
Added updateAsset, which allows you to change the project.
You can also specify null to make it a workspace file.
The changes to the API are as follows:
type Asset implements Node { id: ID! createdAt: DateTime! teamId: ID! + projectId: ID name: String! size: FileSize! url: String! contentType: String! team: Team coreSupport: Boolean! } input CreateAssetInput { teamId: ID! + projectId: ID coreSupport: Boolean! file: Upload! } +input UpdateAssetInput { + assetId: ID! + projectId: ID +} +type UpdateAssetPayload { + assetId: ID! + projectId: ID +} extend type Query { assets( teamId: ID! + projectId: ID pagination: Pagination keyword: String sort: AssetSort ): AssetConnection! } extend type Mutation { createAsset(input: CreateAssetInput!): CreateAssetPayload + updateAsset(input: UpdateAssetInput!): UpdateAssetPayload removeAsset(input: RemoveAssetInput!): RemoveAssetPayload }The projectId parameter for createAsset is optional, while teamId remains required as before.
teamId is necessary because, if the owner is not present, the data would remain as unmanaged.
For the Query, when both teamId and projectId are specified, the search will be performed based on projectId (and teamId will be ignored).
The data model for the asset is as shown in the diagram below.
What I haven't done
How I tested
e2e test
https://github.com/reearth/reearth-visualizer/pull/1410/files#diff-be135cdb0bc1a5b8a184c356c9664cee68bcc8220f9d2097ad6b5348ac25c425R131
Which point I want you to review particularly
* Added fullSeeder to the e2e test seeder.
This seeder handles all the data used in reearth-visualizer.
https://github.com/reearth/reearth-visualizer/blob/feat/associate-project-asset/server/e2e/seeder.go#L163
* Fixed asset handling in project export/import.
Updated project export/import tests to cover all parameters.
Now, project export/import tests compare the original project with GetScene to verify correctness.
This ensures that any future modifications to GetScene will not introduce issues in project export/import.
Note: Currently, project export searches within the workspace. Changes to the project can be made at any time.
https://github.com/reearth/reearth-visualizer/blob/feat/associate-project-asset/server/e2e/gql_project_export_import_test.go#L21
Memo
Fix the front-end query here
web/src/services/gql/queries/asset.ts
import { gql } from "@reearth/services/gql/__gen__"; export const GET_ASSETS = gql(` - query GetAssets($teamId: ID!, $pagination: Pagination, $keyword: String, $sort: AssetSort) { - assets(teamId: $teamId, pagination: $pagination, keyword: $keyword, sort: $sort) { + query GetAssets($teamId: ID!, $projectId: ID, $pagination: Pagination, $keyword: String, $sort: AssetSort) { + assets(teamId: $teamId, projectId: $projectId, pagination: $pagination, keyword: $keyword, sort: $sort) { edges { cursor node { id teamId + projectId name size url createdAt contentType coreSupport } } nodes { id teamId + projectId name size url createdAt contentType coreSupport } pageInfo { endCursor hasNextPage hasPreviousPage startCursor } totalCount } } `); export const CREATE_ASSET = gql(` - mutation CreateAsset($teamId: ID!, $file: Upload!, $coreSupport: Boolean!) { - createAsset(input: { teamId: $teamId, file: $file, coreSupport: $coreSupport }) { + mutation CreateAsset($teamId: ID!, $projectId: ID, $file: Upload!, $coreSupport: Boolean!) { + createAsset(input: { teamId: $teamId, projectId: $projectId, file: $file, coreSupport: $coreSupport }) { asset { id teamId + projectId name size url contentType } } } `); +export const UPDATE_ASSET = gql(` + mutation UpdateAsset($assetId: ID!, $projectId: ID) { + updateAsset(input: { assetId: $assetId projectId: $projectId }) { + assetId + projectId + __typename + } + } +`); export const REMOVE_ASSET = gql(` mutation RemoveAsset($assetId: ID!) { removeAsset(input: { assetId: $assetId }) { assetId } } `);Summary by CodeRabbit
New Features
Improvements