feat(server): associate project asset #1410
Conversation
hexaforce
requested review from
airslice,
mkumbobeaty and
pyshx
as code owners
|
Warning Rate limit exceeded@hexaforce has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 16 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughA broad refactoring is underway across backend and frontend code. Significant changes include the introduction of new asset and project handling features (e.g. new project ID parameters in asset creation and queries), removal of cluster, dataset, and tagging functionalities from the GraphQL schema and resolvers, and updates to several test files and seeder functions. Various function signatures and control flows have been streamlined by removing root layer handling and redundant fields. In addition, middleware logic has been adjusted to support disabled authentication, and file URL construction now explicitly uses asset directories. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant R as GraphQL Resolver
participant A as Asset Interactor
participant Repo as Asset Repository
participant DB as Database
C->>R: CreateAsset request (with optional projectId)
R->>A: Call ImportAssetFiles(ctx, name, file, workspace, projectId)
A->>Repo: FindByWorkspaceProject(workspace, projectId, filter)
Repo-->>A: Return matching assets / create new record
A->>Repo: Create asset record with project association
Repo-->>A: Asset record created
A->>R: Return asset details
R-->>C: Respond with asset data including projectId
sequenceDiagram
participant S as Server
participant M as Auth Middleware
participant N as Next Handler
S->>M: Incoming request
alt Auth Disabled
M->>N: Pass request unmodified (dummy middleware)
else Auth Enabled
M->>N: Validate token and attach user context
end
N-->>S: Continue processing request
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
✅ Deploy Preview for reearth-web canceled.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 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
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
server/e2e/gql_project_export_import_test.go (1)
141-170: 🛠️ Refactor suggestionAdd cleanup for temporary files.
The
importProjectfunction should ensure cleanup of temporary files in case of errors.func importProject(t *testing.T, e *httpexpect.Expect, filePath string) *httpexpect.Value { + // Ensure cleanup + defer func() { + if err := os.Remove(filePath); err != nil { + t.Logf("Failed to cleanup temporary file %s: %v", filePath, err) + } + }()
♻️ Duplicate comments (9)
server/internal/usecase/interactor/asset.go (2)
88-92: 🛠️ Refactor suggestionAdd validation for project-workspace relationship.
The function should verify that the provided project belongs to the workspace before creating the asset.
Apply this diff to add validation:
a, err := asset.New(). NewID(). Workspace(inp.WorkspaceID). + // Validate project belongs to workspace + if inp.ProjectID != nil { + p, err := i.repos.Project.FindByID(ctx, *inp.ProjectID) + if err != nil { + return nil, fmt.Errorf("failed to validate project: %w", err) + } + if p.Workspace() != inp.WorkspaceID { + return nil, fmt.Errorf("project does not belong to workspace") + } + } Project(inp.ProjectID). Name(path.Base(inp.File.Path)).
108-127: 🛠️ Refactor suggestionAdd validation for project-workspace relationship.
The function should verify that the provided project belongs to the workspace before updating the asset's project association.
Apply this diff to add validation:
func (i *Asset) Update(ctx context.Context, aid id.AssetID, pid *id.ProjectID, operator *usecase.Operator) (id.AssetID, *id.ProjectID, error) { return Run2( ctx, operator, i.repos, Usecase().Transaction(), func(ctx context.Context) (id.AssetID, *id.ProjectID, error) { asset, err := i.repos.Asset.FindByID(ctx, aid) if err != nil { return aid, pid, err } 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) return aid, pid, i.repos.Asset.Save(ctx, asset) }, ) }server/e2e/common.go (1)
303-308: 🛠️ Refactor suggestionEnhance type support in
JSONEqRegexpValue.The function only handles map types, but could receive arrays or other JSON-compatible values.
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 }server/e2e/gql_asset_test.go (2)
239-255: 🛠️ Refactor suggestionAdd error handling for GraphQL responses.
The
updateAssetfunction 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 }
257-271: 🛠️ Refactor suggestionAdd error handling for GraphQL responses.
The
removeAssetfunction should verify the GraphQL response for errors.func removeAsset(e *httpexpect.Expect, assetId string) *httpexpect.Value { requestBody := GraphQLRequest{ OperationName: "RemoveAsset", Query: `mutation RemoveAsset($assetId: ID!) { removeAsset(input: { assetId: $assetId }) { assetId + errors { + message + path + } __typename } }`, Variables: map[string]any{ "assetId": assetId, }, } - return Request(e, uID.String(), requestBody) + response := Request(e, uID.String(), requestBody) + response.Path("$.errors").Null() + return response }server/e2e/seeder.go (3)
272-277:⚠️ Potential issueRemove duplicate error check and save operation.
The same error check and save operation is performed twice in succession.
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 - }
185-224:⚠️ Potential issueFix incorrect error checks in
fullSetupfunction.Multiple 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 {This change needs to be applied to all error checks in the function.
🧰 Tools
🪛 GitHub Check: ci-server / ci-server-lint
[failure] 200-200:
scene.New().ID(sID).Project(pID).Workspace(wID).Property(prop.ID()).RootLayer undefined (type *scene.Builder has no field or method RootLayer) (typecheck)
510-525: 🛠️ Refactor suggestionReturn 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 }Update the caller at line 484 to handle the error:
- err = json.Unmarshal([]byte(fmt.Sprintf(`{ + uuid, err := generateUUID() + if err != nil { + return err + } + err = json.Unmarshal([]byte(fmt.Sprintf(`{ "extrudedHeight": 0, - "id": "%s", + "id": "%s", "positions": [ [ -3958794.1421583104, 3350991.8464303534, 3699620.1697127568 ] ], "type": "marker" - }`, generateUUID())), &p) + }`, uuid)), &p)server/internal/usecase/interactor/project.go (1)
627-649: 🛠️ Refactor suggestionImprove error handling in
AddZipAsset.The function silently ignores errors for external URLs and has a redundant stream close in the error path.
Apply this diff:
if err != nil { - return nil // skip if external URL + if strings.HasPrefix(path, "http://") || strings.HasPrefix(path, "https://") { + return nil // skip external URLs + } + return fmt.Errorf("failed to read asset: %w", err) } defer func() { if cerr := stream.Close(); cerr != nil { - fmt.Printf("Error closing file: %v\n", cerr) + err = errors.Join(err, fmt.Errorf("failed to close stream: %w", cerr)) } }() zipEntryPath := fmt.Sprintf("assets/%s", fileName) zipEntry, err := zipWriter.Create(zipEntryPath) if err != nil { return err } _, err = io.Copy(zipEntry, stream) - if err != nil { - _ = stream.Close() - return err - } - return nil + return err
🧹 Nitpick comments (13)
server/pkg/scene/builder/scene.go (2)
27-40: Add method documentation to improve code clarity.Consider adding a documentation comment explaining the method's purpose, parameters, and return values. This would help other developers understand the method's role in scene building.
Add this documentation:
+// sceneJSON creates a new sceneJSON instance with the given parameters. +// It initializes all scene properties including plugins, widgets, and alignment systems. +// Parameters: +// - ctx: Context for the operation +// - publishedAt: Time when the scene was published +// - p: Slice of property configurations +// - coreSupport: Flag indicating core feature support +// - enableGa: Flag for Google Analytics +// - trackingId: Analytics tracking identifier +// Returns the constructed sceneJSON and any error encountered. func (b *Builder) sceneJSON(ctx context.Context, publishedAt time.Time, p []*property.Property, coreSupport bool, enableGa bool, trackingId string) (*sceneJSON, error) {
42-167: Add documentation to helper methods for better maintainability.The helper methods are well-structured with clear responsibilities. Consider adding documentation to explain their purpose and behavior.
Add documentation to key helper methods:
+// plugins builds a map of plugin properties for the scene. +// It processes each plugin and its associated property. func (b *Builder) plugins(ctx context.Context, p []*property.Property) map[string]propertyJSON { +// widgets constructs widget configurations for the scene. +// It processes enabled widgets and their properties. func (b *Builder) widgets(ctx context.Context, p []*property.Property) []*widgetJSON { +// property seals and returns the property configuration. func (b *Builder) property(ctx context.Context, p *property.Property) propertyJSON { +// findProperty locates a property by its ID in a slice of properties. func findProperty(pp []*property.Property, i property.ID) *property.Property {server/pkg/layer/encoding/exporter.go (3)
10-13: Consider removing unused Sealer field.The
Sealerfield is retained in the struct but is no longer used in theEncodemethod. This creates an inconsistency in the design.Consider applying this diff to maintain consistency with the simplified encoding approach:
type Exporter struct { Merger *merging.Merger - Sealer *merging.Sealer }
37-42: Document the simplified Encode method.The
Encodemethod has been simplified to return nil without performing any encoding. This significant change in functionality should be documented to explain the current behavior and future plans.Consider adding a comment to explain the current implementation:
+// Encode is currently a no-op method retained for future implementation func (e *Exporter) Encode(ctx context.Context, m merging.MergedLayer) error { if e == nil { return nil } return nil }
1-2: Add package documentation to explain the transition.Given the significant changes in the encoding functionality, it would be helpful to add package documentation explaining the current state and future direction of the layer encoding package.
Consider adding package documentation:
package encoding + +// Package encoding handles layer export functionality. +// Note: The encoding functionality is currently being refactored as part of +// the project asset association changes.server/pkg/scene/builder/builder.go (1)
40-57: Consider documenting the Builder initialization changes.The removal of the encoder initialization simplifies the Builder, but consider adding documentation to explain the new scene building process for future maintainers.
func New(ll layer.Loader, pl property.Loader, dl dataset.GraphLoader, tl tag.Loader, tsl tag.SceneLoader, nlsl nlslayer.Loader, exp bool) *Builder { + // Builder constructs scene JSON without an encoder, directly using the exporter + // for layer processing and the sceneJSON method for final assembly return &Builder{ ploader: pl, tloader: tsl,server/internal/app/app.go (1)
164-168: Simplify the dummy auth middleware.The current implementation can be simplified since it's just passing through the handler.
-func AuthMiddlewareDummy() (func(http.Handler) http.Handler, error) { - return func(next http.Handler) http.Handler { - return next - }, nil -} +func AuthMiddlewareDummy() (func(http.Handler) http.Handler, error) { + return func(next http.Handler) http.Handler { return next }, nil +}server/e2e/common.go (1)
288-293: Remove commented-out code.The commented-out code in
RegexpJSONEReadClosershould be removed as it's not being used.- // var data map[string]interface{} - // err = json.Unmarshal(actualBuf.Bytes(), &data) - // assert.NoError(t, err) - // if text, err := json.MarshalIndent(data, "", " "); err == nil { - // fmt.Println(string(text)) - // }server/e2e/gql_asset_test.go (1)
355-357: Document magic numbers in pagination.The hardcoded value
20in pagination should be documented or moved to a constant.+const defaultPageSize = 20 + func getAssets(e *httpexpect.Expect, teamId string, projectId *string) *httpexpect.Value { // ... "pagination": map[string]any{ - "first": 20, + "first": defaultPageSize, // Maximum number of assets to return per page }, // ... }server/internal/adapter/gql/resolver_mutation_project.go (2)
130-131: Use a more descriptive file name for the ZIP file.The current implementation uses a simple lowercase project ID for the ZIP file name. Consider adding a timestamp or project name for better identification.
- zipFile, err := fs.Create(fmt.Sprintf("%s.zip", strings.ToLower(string(input.ProjectID)))) + zipFile, err := fs.Create(fmt.Sprintf("%s_%s.zip", strings.ToLower(string(input.ProjectID)), time.Now().Format("20060102_150405")))
189-195: Improve error handling for URL parsing.The URL parsing error handling could be more descriptive. Consider including the problematic URL in the error message.
- return nil, errors.New("Error parsing URL:" + err.Error()) + return nil, fmt.Errorf("Error parsing URL %q: %w", a.URL, err)server/e2e/gql_project_export_import_test.go (2)
71-101: Consider parameterizing the comparison logic.The comparison function could be more flexible by accepting a list of properties to ignore or custom comparison functions.
-func compareValue(t *testing.T, key string, e, a *httpexpect.Value) { +func compareValue(t *testing.T, key string, e, a *httpexpect.Value, opts ...CompareOption) { + config := &compareConfig{ + ignoreProps: []string{"id", "propertyId", "sceneId"}, + customCompareFuncs: map[string]func(string, string) bool{}, + } + for _, opt := range opts { + opt(config) + }
112-119: Add test cases for ID validation.The
isIDfunction should have unit tests to verify its behavior with various input formats.func TestIsID(t *testing.T) { tests := []struct { name string input string want bool }{ {"valid ID", `"01234567-89ab-cdef-0123-456789abcdef"`, true}, {"invalid format", `"invalid"`, false}, {"empty string", `""`, false}, } // ... test implementation }
📜 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 (107)
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(1 hunks)server/internal/usecase/interactor/layer_test.go(1 hunks)server/internal/usecase/interactor/nlslayer.go(3 hunks)server/internal/usecase/interactor/nlslayer_test.go(5 hunks)server/internal/usecase/interactor/plugin_test.go(1 hunks)server/internal/usecase/interactor/plugin_upload_test.go(5 hunks)server/internal/usecase/interactor/project.go(5 hunks)server/internal/usecase/interactor/project_test.go(0 hunks)server/internal/usecase/interactor/property_test.go(3 hunks)server/internal/usecase/interactor/scene.go(6 hunks)server/internal/usecase/interactor/scene_plugin.go(0 hunks)server/internal/usecase/interactor/scene_plugin_test.go(3 hunks)server/internal/usecase/interactor/scene_test.go(1 hunks)server/internal/usecase/interactor/storytelling_test.go(1 hunks)server/internal/usecase/interactor/style_test.go(1 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/initializer_test.go(0 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.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(0 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(1 hunks)
⛔ Files not processed due to max files limit (11)
- server/pkg/scene/scene.go
- server/pkg/scene/scene_test.go
- server/pkg/scene/sceneops/plugin_migrator_test.go
- server/pkg/storytelling/story.go
- web/src/services/api/propertyApi/utils.ts
- web/src/services/api/sceneApi.ts
- web/src/services/gql/fragments/dataset.ts
- 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/scene.ts
💤 Files with no reviewable changes (48)
- server/pkg/layer/initializer_test.go
- server/internal/adapter/gql/gqlmodel/convert_scene.go
- server/pkg/scene/builder/encoder.go
- server/pkg/scene/builder/builder_test.go
- server/internal/adapter/gql/gqlmodel/convert.go
- server/internal/adapter/gql/resolver_property_test.go
- server/pkg/scene/builder.go
- server/internal/adapter/gql/gqlmodel/convert_plugin.go
- server/internal/usecase/interactor/scene_plugin.go
- server/internal/adapter/gql/gqlmodel/models.go
- server/internal/adapter/gql/gqlmodel/convert_layer.go
- server/e2e/gql_nlslayer_test.go
- server/internal/adapter/gql/gqlmodel/convert_tag.go
- server/e2e/gql_layer_test.go
- server/internal/adapter/gql/context.go
- server/internal/adapter/gql/gqlmodel/convert_dataset.go
- server/internal/adapter/gql/gqldataloader/dataloader.go
- server/internal/adapter/gql/resolver_mutation_scene.go
- server/internal/usecase/interactor/project_test.go
- server/pkg/nlslayer/feature.go
- server/gql/property.graphql
- server/pkg/scene/builder/encoder_test.go
- server/internal/infrastructure/mongo/mongodoc/scene.go
- server/internal/adapter/gql/resolver_layer.go
- server/gqlgen.yml
- server/pkg/layer/layerops/processor_test.go
- server/internal/adapter/gql/loader.go
- server/gql/tag.graphql
- server/internal/adapter/gql/resolver_scene.go
- server/e2e/dataset_export_test.go
- server/internal/adapter/gql/resolver_dataset.go
- server/internal/adapter/gql/resolver_dataset_schema.go
- server/internal/adapter/gql/resolver_property.go
- server/internal/adapter/gql/resolver_mutation_dataset.go
- server/internal/adapter/gql/resolver_tag.go
- server/internal/adapter/gql/gqldataloader/tagloader_gen.go
- server/gql/cluster.graphql
- server/gql/dataset.graphql
- server/internal/adapter/gql/gqldataloader/datasetschemaloader_gen.go
- server/e2e/gql_project_export_test.go
- server/internal/adapter/gql/gqldataloader/taggrouploader_gen.go
- server/internal/adapter/gql/gqldataloader/datasetloader_gen.go
- server/internal/adapter/gql/loader_dataset.go
- server/gql/layer.graphql
- server/internal/adapter/gql/resolver_mutation_tag.go
- server/internal/adapter/gql/loader_tag.go
- server/internal/adapter/gql/gqldataloader/tagitemloader_gen.go
- server/internal/adapter/gql/resolver_mutation_layer.go
🚧 Files skipped from review as they are similar to previous changes (33)
- server/pkg/asset/builder.go
- server/internal/infrastructure/mongo/mongodoc/storytelling.go
- server/internal/infrastructure/fs/file.go
- server/internal/adapter/gql/resolver_team.go
- server/pkg/scene/builder/story.go
- server/gql/plugin.graphql
- server/internal/usecase/interactor/scene_test.go
- server/internal/infrastructure/memory/asset.go
- server/internal/usecase/repo/asset.go
- server/e2e/gql_user_test.go
- server/internal/adapter/gql/gqlmodel/convert_nlslayer_test.go
- server/internal/infrastructure/fs/file_test.go
- server/gql/workspace.graphql
- server/pkg/asset/asset.go
- server/Makefile
- server/gql/scene.graphql
- server/pkg/scene/builder_test.go
- server/internal/app/auth_client.go
- server/pkg/nlslayer/feature_test.go
- server/internal/adapter/gql/loader_asset.go
- server/internal/adapter/gql/gqlmodel/convert_asset.go
- server/gql/asset.graphql
- server/go.mod
- server/internal/usecase/interactor/nlslayer.go
- server/e2e/gql_validate_geojson_test.go
- server/internal/adapter/gql/gqlmodel/convert_project.go
- server/internal/adapter/gql/resolver_mutation_asset.go
- server/internal/usecase/interfaces/asset.go
- server/e2e/gql_custom_property_test.go
- server/internal/infrastructure/mongo/mongodoc/asset.go
- server/e2e/gql_project_test.go
- server/internal/usecase/interfaces/project.go
- server/internal/usecase/interactor/layer.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: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`.
🪛 GitHub Actions: ci
server/e2e/common.go
[error] 49-49: not enough arguments in call to lo.Must
server/e2e/seeder.go
[error] 24-24: no required module provides package github.com/reearth/reearth/server/pkg/nlslayer/nlslayerops; to add it:
🪛 GitHub Check: ci-server / ci-server-lint
server/e2e/seeder.go
[failure] 200-200:
scene.New().ID(sID).Project(pID).Workspace(wID).Property(prop.ID()).RootLayer undefined (type *scene.Builder has no field or method RootLayer) (typecheck)
[failure] 379-379:
undefined: sceneops.Style (typecheck)
⏰ 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 (37)
server/pkg/scene/builder/scene.go (1)
11-25: LGTM! Clean struct definition with proper field organization.The
sceneJSONstruct maintains a clear separation of concerns with well-defined fields and appropriate JSON tags.server/internal/usecase/interactor/style_test.go (1)
30-30:❓ Verification inconclusive
Verify consistent scene initialization across test files.
The removal of
RootLayerfrom scene creation aligns with the broader refactoring effort. However, let's ensure this pattern is consistently applied across all test files.
🏁 Script executed:
#!/bin/bash # Description: Check for any remaining RootLayer calls in test files # to ensure consistent scene initialization echo "Checking for RootLayer usage in test files..." rg -l "RootLayer" "server/internal/usecase/interactor/*_test.go" echo "Checking scene creation patterns in test files..." ast-grep --pattern 'scene.New().NewID().Workspace($_).Project($_).Build()' "server/internal/usecase/interactor/*_test.go"Length of output: 575
Action Required: Verify Consistent Scene Initialization Across Test Files
The automated checks did not locate any test files matching the specified pattern, so we couldn’t conclusively verify the removal of
RootLayerin all relevant tests. The change inserver/internal/usecase/interactor/style_test.go(line 30) aligns with the broader refactoring effort, but please ensure by manual inspection that all test files use the updated scene creation pattern withoutRootLayer.
- Action Item: Manually review other tests (or update search scripts with the correct paths) to confirm consistent usage of:
scene.New().NewID().Workspace(accountdomain.NewWorkspaceID()).Project(prj.ID()).Build()server/pkg/layer/encoding/exporter.go (1)
15-35:❓ Verification inconclusive
Verify the necessity of merge operations.
Both
ExportLayerByIDandExportLayermethods perform merging operations but the results are effectively discarded sinceEncodenow does nothing. This could be unnecessary processing.Let's verify if these methods are still needed in their current form:
🏁 Script executed:
#!/bin/bash # Search for usages of ExportLayerByID and ExportLayer echo "Searching for ExportLayerByID usages:" rg "ExportLayerByID" -A 3 echo -e "\nSearching for ExportLayer usages:" rg "ExportLayer\b" -A 3Length of output: 517
Performance Concern: Verify Unnecessary Merge Operations
Both
ExportLayerByIDandExportLayerstill perform merge operations before callingEncode, which currently does nothing. This leads to potentially unnecessary processing that might impact performance. Please verify the following:
- Confirm whether the merge results are used for side effects or any implicit behavior.
- Review if these methods' merge operations are actually needed given that
Encodeis now a no-op.- Check if the merge work could be conditionally bypassed to avoid performance overhead.
server/internal/adapter/gql/gqlmodel/models_gen.go (8)
138-138: No concerns about optional project association here.
The newProjectIDfield in theAssetstruct properly aligns with the PR goal of associating assets with projects.
186-186: Ensure user authorization for project references during asset creation.
When creating assets with aprojectId, confirm that the user has adequate permissions to associate assets with that project.
581-588: MergedPropertyField structure looks consistent.
These fields, including the use ofany, appear aligned with the flexible nature of property values across the schema.
906-915: Use a consistent type forIDinPropertyField.
PropertyField.IDis declared asstring, whereas other ID fields use the customIDtype. If this is unintentional, consider aligning it for consistency.
919-921: Dataset references are well-typed.
Using a pointer forDatasetIDand non-pointer fields for the others seems appropriate for optional vs required references.
1135-1149: Scene struct expansion aligns well with new linkages.
These new or updated references—Project,Team, andProperty—should be manageable in GraphQL with resolvers. No immediate issues.
1330-1332: Confirm authorization for updating asset's project association.
When changing an asset’sprojectIdviaUpdateAssetInput, ensure that only authorized users can reassign assets to different projects.
1335-1337: Exposing the updated project ID in the payload is sensible.
Returning theProjectIDinUpdateAssetPayloadhelps the client synchronize the new project association.server/pkg/asset/id.go (1)
10-10: LGTM! Well-structured additions for project ID support.The new ProjectID type and its associated functions are consistently implemented, following the established patterns in the codebase. These additions align well with the PR's objective of enabling project-asset associations.
Also applies to: 14-14, 18-18, 22-22, 26-26
server/internal/usecase/interactor/layer_test.go (1)
20-20:❓ Verification inconclusive
Verify scene creation without RootLayer.
The removal of
RootLayercall from scene creation appears intentional based on the broader changes. However, let's verify that scene creation and layer operations work correctly without a root layer.
🏁 Script executed:
#!/bin/bash # Search for other test files that might be affected by RootLayer removal rg -l "RootLayer.*NewLayerID" --type goLength of output: 190
Manual Verification Needed: Scene Creation Without RootLayer
The removal of the
RootLayercall from scene creation inserver/internal/usecase/interactor/layer_test.goappears intentional and the test passes. However, the initial search using the pattern"RootLayer.*NewLayerID"with a type filter did not search any files. Please verify there are no unexpected references toRootLayerby running the above command (or a similar variant) to ensure that no related test or implementation is inadvertently affected.
🏁 Script executed:
[run_scripts] #!/bin/bash # Re-run a comprehensive search for any occurrences of "RootLayer" in the repository. rg -l "RootLayer" .Length of output: 141
Manual Verification Required: Scene Creation and RootLayer Removal
The removal of the
RootLayercall in scene creation seems intentional and aligns with the focus on infobox creation in the test. However, our automated search results were inconclusive. Please perform a manual check to ensure no other parts of the codebase depend on theRootLayerfunctionality.
- Verify in tests and implementation: Confirm that no other tests or scene-building operations rely on the removed
RootLayercall (e.g., inserver/internal/usecase/interactor/layer_test.goor related files).- Double-check dependencies: Ensure that scene creation continues to function correctly without
RootLayer.server/e2e/gql_scene_test.go (2)
16-16: Improved type safety in response handling.The change to use
Object().Value()instead of directValue()calls improves type safety and makes the code more robust.Also applies to: 42-42, 71-73
77-100: LGTM! Well-structured test helper function.The new
createProjectWithExternalImagefunction is well-implemented with:
- Clear GraphQL mutation structure
- Proper error handling through httpexpect
- Consistent return type handling
server/pkg/scene/builder/builder.go (1)
181-181:❓ Verification inconclusive
Verify scene JSON generation after encoder removal.
The removal of the encoder and direct call to
sceneJSONrepresents a significant change in scene building logic. Let's verify that all scene data is still properly encoded.
🏁 Script executed:
#!/bin/bash # Search for sceneJSON method implementation and its test coverage ast-grep --pattern 'func $_(b *Builder) sceneJSON($_, $_, $_, $_, $_, $_) (*sceneJSON, error) { $$$ }'Length of output: 106
Action: Verify Scene Data Encoding via
sceneJSONFunctionThe latest changes now bypass the encoder by directly calling:
return b.sceneJSON(ctx, publishedAt, p, coreSupport, enableGa, trackingId)Please ensure that:
- The
sceneJSONfunction is correctly implemented to encode all scene data as expected.- There is sufficient test coverage verifying that its output meets the required formatting and error-handling without the encoder.
To assist with verification, please run the following script to locate the implementation of
sceneJSONwithin the repository:Once you confirm that the
sceneJSONfunction is present and behaves correctly, feel free to update this review comment accordingly.
🏁 Script executed:
[run_scripts] #!/bin/bash # Search for all occurrences of the sceneJSON function definition in Go source files rg -n "func (b \\*Builder) sceneJSON" -tgo .Length of output: 167
Action: Manually Verify Scene JSON Functionality
The direct call to
sceneJSONafter removing the encoder is a significant change. Since our automated search commands did not conclusively locate thesceneJSONfunction implementation, please verify manually that:
- The
sceneJSONmethod is implemented and exposed as expected.- All scene data is correctly encoded by this function.
- Appropriate test coverage exists to validate the scene JSON output.
server/internal/usecase/interactor/asset.go (2)
39-51: LGTM! Function renamed and updated to support project filtering.The changes correctly implement project-based asset filtering while maintaining workspace authorization checks.
154-195: 🛠️ Refactor suggestionAdd project-workspace validation in ImportAssetFiles.
The function should verify that the provided project belongs to the workspace before creating the asset.
Apply this diff to add validation:
func (i *Asset) ImportAssetFiles(ctx context.Context, realName 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/infrastructure/mongo/asset.go (1)
67-103: LGTM! MongoDB query updated to support project filtering.The changes correctly implement project-based asset filtering in MongoDB while maintaining workspace authorization and existing filter functionality.
server/internal/usecase/interactor/property_test.go (3)
23-23: LGTM! Simplified scene creation in test.The removal of
RootLayerdoesn't affect the test's functionality as it's not relevant for property item operations.
75-75: LGTM! Simplified scene creation in test.The removal of
RootLayerdoesn't affect the test's functionality as it's not relevant for property item operations.
120-120: LGTM! Simplified scene creation in test.The removal of
RootLayerdoesn't affect the test's functionality as it's not relevant for property value operations.server/internal/usecase/interactor/storytelling_test.go (1)
35-35: LGTM! Simplified scene creation in test.The removal of
RootLayerdoesn't affect the test's functionality as it's not relevant for story import operations.server/internal/adapter/gql/resolver_query.go (1)
15-16: LGTM! The asset query resolver now supports project filtering.The implementation correctly handles the new
projectIdparameter and passes it to the asset loader, enabling asset filtering by project.server/internal/usecase/interactor/nlslayer_test.go (1)
34-34: LGTM! Consistent simplification of scene creation.The removal of
RootLayer()from scene creation is consistent across all test functions and aligns with the simplified scene initialization approach.Also applies to: 85-85, 142-142, 221-221, 289-289
server/internal/adapter/gql/resolver_mutation_project.go (2)
183-183: Filter assets by project ID during export.The current implementation uses
FindByWorkspaceProjectbut doesn't filter by project ID. Consider filtering to ensure only relevant assets are exported.
251-255: Consider using a transaction for asset imports.Asset imports should be part of the transaction to ensure consistency if the import fails.
server/internal/usecase/interactor/scene_plugin_test.go (1)
101-101: LGTM! Consistent scene creation approach.The removal of
RootLayer()from scene creation is consistent with other test files and maintains test functionality.Also applies to: 233-233, 381-381
server/e2e/gql_project_export_import_test.go (1)
21-63: Well-structured test with clear steps.The test is well-organized with clear steps for export, import, and validation. The use of
fullSeederprovides better test coverage.server/internal/usecase/interactor/plugin_upload_test.go (1)
98-99: LGTM!The scene creation has been simplified by removing the
RootLayermethod, which aligns with the broader refactoring of scene management in the codebase.server/e2e/gql_storytelling_test.go (1)
735-741: LGTM!Moving the server configuration to a variable improves readability and maintainability. The updated JSON structure correctly reflects the current state of the storytelling functionality.
server/internal/usecase/interactor/scene.go (5)
21-21: LGTM!The addition of the storytelling package import is necessary for handling story-related functionality in the ExportScene method.
150-150: LGTM!The property save operation is correctly placed after property creation, ensuring proper sequencing of operations.
667-670: LGTM!The addition of null and length checks before accessing the story list prevents potential null pointer dereferences.
698-701: Handle URL parsing errors consistently.The URL parsing errors are currently being silently ignored with blank identifiers (
_). This could lead to silent failures in asset handling and make debugging more difficult.
768-866: LGTM!The removal of cluster-related code from the ImportScene method aligns with the broader refactoring to simplify the codebase.
server/internal/usecase/interactor/plugin_test.go (1)
33-33: LGTM!The removal of the RootLayer method call simplifies scene initialization and aligns with the broader refactoring of scene management.
hexaforce
force-pushed
the
feat/associate-project-asset
branch
from
6e98399 to
cc449c1
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (8)
server/internal/adapter/gql/resolver_mutation_asset.go (2)
19-26:⚠️ Potential issueAdd project-workspace validation in CreateAsset.
The project-workspace relationship should be validated before creating the asset.
Apply this diff to add validation:
var pid *idx.ID[id.Project] if input.ProjectID != nil { pidValue, err := gqlmodel.ToID[id.Project](*input.ProjectID) if err != nil { return nil, err } + // Validate project exists and belongs to the workspace + project, err := usecases(ctx).Project.FindByID(ctx, pidValue) + if err != nil { + return nil, err + } + if project.Workspace() != tid { + return nil, fmt.Errorf("project does not belong to specified workspace") + } pid = &pidValue }
41-62:⚠️ Potential issueAdd validation for project-workspace relationship in UpdateAsset.
The project-workspace relationship should be validated before updating the asset.
Apply this diff to add validation:
var pid *id.ProjectID if project := input.ProjectID; project != nil { pidValue, err := gqlmodel.ToID[id.Project](*input.ProjectID) if err != nil { return nil, err } + // Validate project exists and belongs to the asset's workspace + asset, err := usecases(ctx).Asset.FindByID(ctx, aid) + if err != nil { + return nil, err + } + project, err := usecases(ctx).Project.FindByID(ctx, pidValue) + if err != nil { + return nil, err + } + if project.Workspace() != asset.Workspace() { + return nil, fmt.Errorf("project does not belong to asset's workspace") + } pid = &pidValue }server/internal/usecase/interactor/asset.go (1)
108-127:⚠️ Potential issueAdd validation for project-workspace relationship.
The project-workspace relationship should be validated before updating the asset.
Apply this diff to add validation:
func (i *Asset) Update(ctx context.Context, aid id.AssetID, pid *id.ProjectID, operator *usecase.Operator) (id.AssetID, *id.ProjectID, error) { return Run2( ctx, operator, i.repos, Usecase().Transaction(), func(ctx context.Context) (id.AssetID, *id.ProjectID, error) { asset, err := i.repos.Asset.FindByID(ctx, aid) if err != nil { return aid, pid, err } 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) return aid, pid, i.repos.Asset.Save(ctx, asset) }, ) }server/e2e/gql_asset_test.go (1)
239-271: 🛠️ Refactor suggestionAdd error handling for GraphQL mutations.
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 }Apply similar changes to the
removeAssetfunction.server/e2e/seeder.go (2)
203-223:⚠️ Potential issueFix incorrect error handling.
The error checks are comparing
errwith itself, which will never evaluate to true.Apply this diff:
- if err = r.Scene.Save(ctx, s); err != err { + if err = r.Scene.Save(ctx, s); err != nil { return err } - if err = r.Property.Save(ctx, prop); err != err { + if err = r.Property.Save(ctx, prop); err != nil { return err } - if err = r.Layer.Save(ctx, rootLayer); err != err { + if err = r.Layer.Save(ctx, rootLayer); err != nil { return err } - if err = addWidget(ctx, s, r); err != err { + if err = addWidget(ctx, s, r); err != nil { return err } - if err = addStory(ctx, r); err != err { + if err = addStory(ctx, r); err != nil { return err } - if err = addLayerSimple(ctx, r); err != err { + if err = addLayerSimple(ctx, r); err != nil { return err } - if err = addLayerStyle(s.ID(), ctx, r); err != err { + if err = addLayerStyle(s.ID(), ctx, r); err != nil { return err }
507-522: 🛠️ Refactor suggestionReturn error instead of panicking in UUID generation.
The function should return an error instead of panicking when random number generation fails.
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]), - ) + ), nilUpdate the caller at line 486:
- }`, generateUUID())), &p) + uuid, err := generateUUID() + if err != nil { + return err + } + }`, uuid)), &p)server/internal/usecase/interactor/project.go (1)
627-649: 🛠️ Refactor suggestionImprove error handling in AddZipAsset.
The function silently ignores errors for external URLs and has a redundant stream close in the error path.
Apply this diff:
if err != nil { - return nil // skip if external URL + if strings.HasPrefix(path, "http://") || strings.HasPrefix(path, "https://") { + return nil // skip external URLs + } + return fmt.Errorf("failed to read asset: %w", err) } defer func() { if cerr := stream.Close(); cerr != nil { - fmt.Printf("Error closing file: %v\n", cerr) + err = errors.Join(err, fmt.Errorf("failed to close stream: %w", cerr)) } }() zipEntryPath := fmt.Sprintf("assets/%s", fileName) zipEntry, err := zipWriter.Create(zipEntryPath) if err != nil { return err } _, err = io.Copy(zipEntry, stream) - if err != nil { - _ = stream.Close() - return err - } - return nil + return errserver/internal/usecase/interactor/scene.go (1)
698-701:⚠️ Potential issueHandle 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 and make debugging more difficult.Apply this pattern to all URL parsing locations:
- u, _ := url.Parse(urlStr) - if err := AddZipAsset(ctx, i.file, zipWriter, u.Path); err != nil { + u, err := url.Parse(urlStr) + if err != nil { + log.Errorfc(ctx, "Failed to parse URL: %v", err) + return nil, nil, fmt.Errorf("failed to parse URL: %w", err) + } + if err := AddZipAsset(ctx, i.file, zipWriter, u.Path); err != nil {Also applies to: 727-731, 752-755
🧹 Nitpick comments (11)
server/e2e/gql_scene_test.go (1)
77-100: Enhance the project creation utility function.While the function works, consider these improvements for better maintainability and reusability:
- Make the function more flexible by accepting optional parameters for description, imageUrl, visualizer, and coreSupport instead of hardcoding them.
- Add validation for required parameters.
- Add error handling for failed requests.
Here's a suggested implementation:
-func createProjectWithExternalImage(e *httpexpect.Expect, name string) string { +type ProjectOptions struct { + Description string + ImageURL string + Visualizer string + CoreSupport bool +} + +func createProjectWithExternalImage(e *httpexpect.Expect, name string, opts *ProjectOptions) (string, error) { + if e == nil { + return "", fmt.Errorf("httpexpect.Expect is required") + } + if name == "" { + return "", fmt.Errorf("project name is required") + } + + // Set default options if not provided + if opts == nil { + opts = &ProjectOptions{ + Description: "abc", + ImageURL: "https://test.com/project.jpg", + Visualizer: "CESIUM", + CoreSupport: true, + } + } + requestBody := GraphQLRequest{ OperationName: "CreateProject", Query: `mutation CreateProject($teamId: ID!, $visualizer: Visualizer!, $name: String!, $description: String!, $imageUrl: URL, $coreSupport: Boolean) { createProject( input: {teamId: $teamId, visualizer: $visualizer, name: $name, description: $description, imageUrl: $imageUrl, coreSupport: $coreSupport} ) { project { id + name __typename } __typename } }`, Variables: map[string]any{ "name": name, - "description": "abc", - "imageUrl": "https://test.com/project.jpg", + "description": opts.Description, + "imageUrl": opts.ImageURL, "teamId": wID.String(), - "visualizer": "CESIUM", - "coreSupport": true, + "visualizer": opts.Visualizer, + "coreSupport": opts.CoreSupport, }, } res := Request(e, uID.String(), requestBody) - return res.Path("$.data.createProject.project.id").Raw().(string) + + // Check for GraphQL errors + if errors := res.Path("$.errors").Raw(); errors != nil { + return "", fmt.Errorf("GraphQL error: %v", errors) + } + + return res.Path("$.data.createProject.project.id").Raw().(string), nil }Update the test functions to use the enhanced version:
- pID := createProjectWithExternalImage(e, "test") + pID, err := createProjectWithExternalImage(e, "test", nil) + if err != nil { + t.Fatalf("Failed to create project: %v", err) + }server/internal/adapter/gql/resolver_query.go (1)
15-16: RenameFindByWorkspaceto better reflect its functionality.The method name
FindByWorkspaceis misleading as it now filters assets by both workspace and project. Consider renaming it toFindByWorkspaceProjectto better reflect its functionality.- return loaders(ctx).Asset.FindByWorkspace(ctx, teamID, projectId, keyword, gqlmodel.AssetSortTypeFrom(sortType), pagination) + return loaders(ctx).Asset.FindByWorkspaceProject(ctx, teamID, projectId, keyword, gqlmodel.AssetSortTypeFrom(sortType), pagination)server/e2e/common.go (1)
303-308: Enhance type support inJSONEqRegexpValue.The function only handles
map[string]interface{}types. Consider supporting other JSON types like arrays, strings, numbers, and booleans.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) + actualData := actual.Raw() + switch data := actualData.(type) { + case map[string]interface{}, []interface{}, string, float64, bool: + return JSONEqRegexpInterface(t, data, expected) + default: + t.Errorf("unsupported type for JSON comparison: %T", actualData) + return false } - return false }server/internal/adapter/gql/resolver_mutation_project.go (2)
189-195: Improve error handling for URL parsing.The URL parsing error handling could be more robust and informative.
Apply this diff:
- parsedURL, err := url.Parse(a.URL) - if err != nil { - return nil, errors.New("Error parsing URL:" + err.Error()) - } - fileName := path.Base(parsedURL.Path) - assetNames[fileName] = a.Name + parsedURL, err := url.Parse(a.URL) + if err != nil { + return nil, fmt.Errorf("failed to parse asset URL %q: %w", a.URL, err) + } + if parsedURL.Path == "" { + return nil, fmt.Errorf("asset URL %q has no path component", a.URL) + } + fileName := path.Base(parsedURL.Path) + if fileName == "." || fileName == "/" { + return nil, fmt.Errorf("invalid file name in asset URL %q", a.URL) + } + assetNames[fileName] = a.Name
237-239: Validate project image URL path.The old project image path extraction could fail if the URL is invalid.
Apply this diff:
+ var oldProjectImage string - if prj.ImageURL() != nil { - oldProjectImage = path.Base(prj.ImageURL().Path) + if url := prj.ImageURL(); url != nil { + if url.Path == "" { + return nil, fmt.Errorf("project image URL %q has no path component", url) + } + fileName := path.Base(url.Path) + if fileName == "." || fileName == "/" { + return nil, fmt.Errorf("invalid file name in project image URL %q", url) + } + oldProjectImage = fileNameserver/e2e/gql_project_export_import_test.go (4)
32-39: Add error assertions for import response.The test should verify that the import operation succeeds without errors.
Apply this diff:
r := importProject(t, e, fileName) + r.Object().Path("$.errors").Absent() r.Object().Value("project").NotNull() r.Object().Value("plugins").Array() r.Object().Value("schema").Array() r.Object().Value("scene").NotNull() r.Object().Value("nlsLayer").Array() r.Object().Value("style").Array() r.Object().Value("story").NotNull()
58-61: Improve file cleanup error handling.The file cleanup error should be logged for debugging purposes.
Apply this diff:
defer func() { err := os.Remove(fileName) - assert.Nil(t, err) + if err != nil { + t.Logf("Failed to cleanup test file %q: %v", fileName, err) + } }()
71-101: Enhance value comparison logic.The comparison function could be more robust by handling edge cases and providing better error messages.
Apply this diff:
func compareValue(t *testing.T, key string, e, a *httpexpect.Value) { + t.Helper() expected := convertLine(t, key, e) actual := convertLine(t, key, a) maxLines := len(expected) if len(actual) > maxLines { maxLines = len(actual) } + if len(expected) == 0 && len(actual) == 0 { + return + } + + if len(expected) == 0 { + t.Errorf("No expected values for %s, but got %d actual values", key, len(actual)) + return + } + + if len(actual) == 0 { + t.Errorf("Expected %d values for %s, but got none", len(expected), key) + return + } + for i := 0; i < maxLines; i++ { var expectedLine, actualLine string if i < len(expected) { expectedLine = expected[i] } if i < len(actual) { actualLine = actual[i] } if expectedLine != actualLine { if isIgnore(`"id":`, expectedLine, actualLine) || isIgnore(`"propertyId":`, expectedLine, actualLine) || isIgnore(`"sceneId":`, expectedLine, actualLine) || (isID(expectedLine) && isID(actualLine)) { continue } - assert.Failf(t, "Mismatch in %s at line %d", key, i+1, - "Expected: %s\nActual: %s", expectedLine, actualLine) + t.Errorf("Mismatch in %s at line %d:\nExpected: %s\nActual: %s\nDiff: %s", + key, i+1, expectedLine, actualLine, + diffString(expectedLine, actualLine)) } } } + +func diffString(expected, actual string) string { + if len(expected) == 0 { + return fmt.Sprintf("Extra line: %s", actual) + } + if len(actual) == 0 { + return fmt.Sprintf("Missing line: %s", expected) + } + return fmt.Sprintf("Changed from %s to %s", expected, actual) +}
135-139: Improve file write error handling.The file write operation should handle potential disk space issues.
Apply this diff:
- err := os.WriteFile(fileName, []byte(downloadResponse), os.ModePerm) - assert.Nil(t, err) + if err := os.WriteFile(fileName, []byte(downloadResponse), os.ModePerm); err != nil { + t.Fatalf("Failed to write export file %q: %v", fileName, err) + } + t.Cleanup(func() { + if err := os.Remove(fileName); err != nil { + t.Logf("Failed to cleanup test file %q: %v", fileName, err) + } + })server/internal/usecase/interactor/project.go (2)
516-522: Improve error handling for project retrieval and deletion check.The error messages could be more descriptive and the deletion check could be more robust.
Apply this diff:
prj, err := i.projectRepo.FindByID(ctx, projectID) if err != nil { - return nil, errors.New("project " + err.Error()) + return nil, fmt.Errorf("failed to find project %s: %w", projectID, 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 %s", prj.ID()) }
589-592: Remove debug print and improve error handling.The debug print statement should be removed and error handling improved.
Apply this diff:
if err != nil { - fmt.Printf("err:%v\n", err) - return nil, tx, err + return nil, tx, fmt.Errorf("failed to create project: %w", err) }
📜 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 (47)
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(2 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/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(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(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(7 hunks)server/internal/usecase/interactor/project.go(5 hunks)server/internal/usecase/interactor/project_test.go(0 hunks)server/internal/usecase/interactor/scene.go(6 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/e2e/gql_project_export_test.go
- server/internal/usecase/interactor/project_test.go
- server/gqlgen.yml
- server/pkg/nlslayer/feature.go
- server/internal/infrastructure/mongo/mongodoc/scene.go
🚧 Files skipped from review as they are similar to previous changes (24)
- server/internal/infrastructure/mongo/mongodoc/storytelling.go
- server/e2e/gql_user_test.go
- server/internal/adapter/gql/gqlmodel/convert_asset.go
- server/internal/infrastructure/fs/file_test.go
- server/internal/usecase/repo/asset.go
- server/internal/adapter/gql/resolver_team.go
- server/pkg/asset/builder.go
- server/pkg/asset/asset.go
- server/internal/usecase/interfaces/project.go
- server/internal/adapter/gql/gqlmodel/convert_project.go
- server/pkg/nlslayer/feature_test.go
- server/go.mod
- server/internal/app/app.go
- server/gql/workspace.graphql
- server/gql/asset.graphql
- server/internal/infrastructure/memory/asset.go
- server/internal/adapter/gql/gqlmodel/convert_nlslayer_test.go
- server/pkg/scene/builder/story.go
- server/pkg/storytelling/story.go
- server/internal/adapter/gql/loader_asset.go
- server/internal/infrastructure/mongo/mongodoc/asset.go
- server/internal/usecase/interfaces/asset.go
- server/internal/app/auth_client.go
- server/internal/infrastructure/fs/file.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 (20)
server/internal/adapter/gql/gqlmodel/models_gen.go (4)
134-145: LGTM! Asset struct changes look good.The addition of the optional
ProjectIDfield to theAssetstruct is well-implemented and aligns with the PR objectives to associate assets with projects.
184-189: LGTM! CreateAssetInput changes look good.The addition of the optional
ProjectIDfield to theCreateAssetInputstruct is consistent with the Asset struct changes.
1330-1338: LGTM! UpdateAssetInput/Payload changes look good.The addition of the optional
ProjectIDfield to bothUpdateAssetInputandUpdateAssetPayloadstructs is consistent with the Asset struct changes.
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"`server/e2e/gql_scene_test.go (3)
16-16: LGTM! Consistent response object access.The change to use
r.Object().Value(...)ensures a more consistent approach to accessing response properties.
42-42: LGTM! Consistent response object access.The change to use
r.Object().Value(...)aligns with the English test implementation.
71-73: LGTM! Consistent response object access.The changes maintain consistency with other test functions in accessing response properties.
server/pkg/asset/id.go (1)
10-10: LGTM!The new
ProjectIDtype alias and associated variables follow the established pattern and are correctly implemented.Also applies to: 14-14, 18-18, 22-22, 26-26
server/internal/usecase/interactor/asset.go (1)
154-195:⚠️ Potential issueAdd project-workspace validation in ImportAssetFiles.
The project-workspace relationship should be validated before creating the asset.
Apply this diff to add validation:
func (i *Asset) ImportAssetFiles(ctx context.Context, realName 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/infrastructure/mongo/asset.go (1)
67-103: LGTM!The implementation of
FindByWorkspaceProjectcorrectly handles project-based filtering and maintains the existing functionality for workspace-based filtering.server/e2e/gql_validate_geojson_test.go (1)
186-186: LGTM!The test cases comprehensively cover GeoJSON validation scenarios, and the changes correctly incorporate the new
projectIdparameter.server/e2e/gql_asset_test.go (1)
131-194: LGTM! Comprehensive test coverage for asset-project associations.The test cases thoroughly validate:
- Asset creation with project associations
- Asset retrieval filtered by project
- Moving assets between projects
- Removing project associations
- Asset deletion
server/e2e/gql_project_test.go (2)
43-43:✅ Verification successful
Verify the updated test expectation.
The test now expects 2 projects instead of 1 in the edges array. This change should be verified against the test data setup in
testData(e)to ensure the new expectation is correct.
🏁 Script executed:
#!/bin/bash # Search for project creation calls in testData function rg -A 5 'testData\(e\)' server/e2e/gql_project_test.goLength of output: 1116
Verified Test Expectation: The updated test now correctly expects two projects.
Based on the search results, the
testData(e)function is invoked twice within the test file, which confirms that the test data setup is creating two projects. The expectation usingedges.Length().IsEqual(2)is thus accurate and consistent with how the data is prepared.
592-592:❓ Verification inconclusive
Verify pagination test expectations.
The test now expects:
- First request: 21 total projects
- Second request: 5 projects in total count and edges array
These changes should be validated against the test setup inTestGetProjectPagination.Also applies to: 624-624, 627-627
🏁 Script executed:
#!/bin/bash # Search for project creation in pagination test rg -A 5 'projects\(t, ctx, r, \d+,' server/e2e/gql_project_test.goLength of output: 817
Pagination Test Expectations – Please Verify Against Test Setup
- Confirm that the first GraphQL request correctly verifies a total of 21 projects (as expected by the assertion
projects.HasValue("totalCount", 21)on line 592).- Ensure that the subsequent request returns both a total count and an edges array containing 5 projects.
- Cross-check these expectations with the setup in
TestGetProjectPagination(also impacting lines 624 and 627); note that the test setup uses project creation calls with numeric parameters (e.g., 20 and 10 as seen in the grep output), so verify that these align with the intended pagination behavior.server/e2e/gql_storytelling_test.go (2)
735-741: LGTM! Improved test configuration setup.The server configuration has been refactored to use a separate config variable, making it clearer and more maintainable.
770-846:❓ Verification inconclusive
Verify the updated JSON structure.
The expected JSON structure has been updated with several changes:
- Added
coreSupportandenableGafields- Removed
position,tags, andtrackingIdfields- Updated widget alignment system structure
These changes should be validated against the actual API response structure.
🏁 Script executed:
#!/bin/bash # Search for story-related GraphQL schema definitions rg -A 10 'type Story' server/gql/story.graphqlLength of output: 113
Action Required: Verify Updated JSON Structure
The expected JSON structure update indicates that the new API response should include the
coreSupportandenableGafields while removing fields such asposition,tags, andtrackingId, and it should reflect an updated widget alignment system structure. However, the snapshot inserver/e2e/gql_storytelling_test.go(lines 770–846) still shows"trackingId": ""and a"position": "left"within thestoryobject. Moreover, our attempt to locate the GraphQL schema file (server/gql/story.graphql) returned a “No such file or directory” error, suggesting that either the file has moved or it isn’t present as expected.
- Verify whether the inclusion of
trackingIdandpositionis intentional or if these fields need to be removed per the updated schema.- Confirm the updated widget alignment system structure meets the new requirements.
- Check if the GraphQL schema is defined elsewhere or if the file location needs correction.
Please review and verify these points to ensure the JSON response structure conforms to the intended changes.
server/internal/usecase/interactor/scene.go (1)
667-670: LGTM! Improved story validation.The code now properly checks for nil and empty story list before accessing the first story, preventing potential nil pointer dereferences.
server/internal/usecase/interactor/nlslayer.go (3)
156-167: LGTM! Improved GeoJSON validation with size limits.The code now properly validates GeoJSON data with a 10MB size limit and falls back to asset repository validation if download fails. This helps prevent memory issues from large files.
1192-1221: LGTM! Well-implemented download function.The new
downloadToBufferfunction includes:
- Proper timeout handling
- Size limit enforcement
- Response status code checking
- Resource cleanup with deferred close
1257-1262: LGTM! Enhanced GeoJSON validation.The validation now includes additional checks for:
- Invalid GeoJSON data
- Invalid BBox values
This helps catch malformed GeoJSON earlier in the process.
| entropy := ulid.Monotonic(rand.New(rand.NewSource(uint64(t.UnixNano()))), 0) | ||
| name := ulid.MustNew(ulid.Timestamp(t), entropy) | ||
| zipFile, err := fs.Create(fmt.Sprintf("%s.zip", strings.ToLower(name.String()))) | ||
| zipFile, err := fs.Create(fmt.Sprintf("%s.zip", strings.ToLower(string(input.ProjectID)))) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Sanitize the project ID before using it in the file name.
The project ID is used directly in the file name without sanitization, which could lead to issues with special characters or path traversal.
Apply this diff:
- zipFile, err := fs.Create(fmt.Sprintf("%s.zip", strings.ToLower(string(input.ProjectID))))
+ sanitizedID := strings.Map(func(r rune) rune {
+ if r >= 'a' && r <= 'z' || r >= '0' && r <= '9' || r == '-' {
+ return r
+ }
+ return '_'
+ }, strings.ToLower(string(input.ProjectID)))
+ zipFile, err := fs.Create(fmt.Sprintf("%s.zip", sanitizedID))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| zipFile, err := fs.Create(fmt.Sprintf("%s.zip", strings.ToLower(string(input.ProjectID)))) | |
| sanitizedID := strings.Map(func(r rune) rune { | |
| if r >= 'a' && r <= 'z' || r >= '0' && r <= '9' || r == '-' { | |
| return r | |
| } | |
| return '_' | |
| }, strings.ToLower(string(input.ProjectID))) | |
| zipFile, err := fs.Create(fmt.Sprintf("%s.zip", sanitizedID)) |
| defer func() { | ||
| if err2 := tx.End(ctx); err == nil && err2 != nil { | ||
| err = err2 | ||
| } | ||
| }() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve transaction rollback handling.
The transaction rollback in the deferred function could be more robust.
Apply this diff:
defer func() {
- if err2 := tx.End(ctx); err == nil && err2 != nil {
+ if err2 := tx.End(ctx); err2 != nil {
+ if err == nil {
+ err = err2
+ } else {
+ err = fmt.Errorf("multiple errors: %v; %v", err, err2)
+ }
+ }
+ if err != nil && tx.IsActive() {
+ _ = tx.Rollback(ctx)
+ }
}()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| defer func() { | |
| if err2 := tx.End(ctx); err == nil && err2 != nil { | |
| err = err2 | |
| } | |
| }() | |
| defer func() { | |
| if err2 := tx.End(ctx); err2 != nil { | |
| if err == nil { | |
| err = err2 | |
| } else { | |
| err = fmt.Errorf("multiple errors: %v; %v", err, err2) | |
| } | |
| } | |
| if err != nil && tx.IsActive() { | |
| _ = tx.Rollback(ctx) | |
| } | |
| }() |
| func addAsset(path string, ctx context.Context, r *repo.Container, gf gateway.File) (*url.URL, error) { | ||
| f, err := os.Open(path) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| defer func() { | ||
| if cerr := f.Close(); cerr != nil && err == nil { | ||
| err = cerr | ||
| } | ||
| }() | ||
| stat, err := f.Stat() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| contentType := mime.TypeByExtension(filepath.Ext(path)) | ||
| if contentType == "" { | ||
| contentType = "application/octet-stream" | ||
| } | ||
|
|
||
| u, size, err := gf.UploadAsset(ctx, &file.File{ | ||
| Content: f, | ||
| Path: path, | ||
| Size: int64(stat.Size()), | ||
| ContentType: contentType, | ||
| }) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| aid := asset.NewID() | ||
| a := asset.New(). | ||
| ID(aid). | ||
| Workspace(wID). | ||
| Project(&pID). | ||
| URL(u.String()). | ||
| CreatedAt(aid.Timestamp()). | ||
| Name(path). | ||
| Size(size). | ||
| ContentType(contentType). | ||
| CoreSupport(true). | ||
| MustBuild() | ||
| if err := r.DatasetSchema.Save(ctx, dss); err != nil { | ||
| return u, r.Asset.Save(ctx, a) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve asset handling robustness.
The asset handling could be more robust with better error handling and validation.
Apply this diff:
func addAsset(path string, ctx context.Context, r *repo.Container, gf gateway.File) (*url.URL, error) {
+ if path == "" {
+ return nil, fmt.Errorf("empty asset path")
+ }
+
f, err := os.Open(path)
if err != nil {
- return nil, err
+ return nil, fmt.Errorf("failed to open asset file %q: %w", path, err)
}
defer func() {
if cerr := f.Close(); cerr != nil && err == nil {
- err = cerr
+ err = fmt.Errorf("failed to close asset file %q: %w", path, cerr)
}
}()
stat, err := f.Stat()
if err != nil {
- return nil, err
+ return nil, fmt.Errorf("failed to stat asset file %q: %w", path, err)
}
+ if stat.Size() == 0 {
+ return nil, fmt.Errorf("empty asset file %q", path)
+ }
+
contentType := mime.TypeByExtension(filepath.Ext(path))
if contentType == "" {
contentType = "application/octet-stream"
}
u, size, err := gf.UploadAsset(ctx, &file.File{
Content: f,
Path: path,
Size: int64(stat.Size()),
ContentType: contentType,
})
if err != nil {
- return nil, err
+ return nil, fmt.Errorf("failed to upload asset %q: %w", path, err)
}
aid := asset.NewID()
a := asset.New().
ID(aid).
Workspace(wID).
Project(&pID).
URL(u.String()).
CreatedAt(aid.Timestamp()).
Name(path).
Size(size).
ContentType(contentType).
CoreSupport(true).
MustBuild()
- return u, r.Asset.Save(ctx, a)
+ if err := r.Asset.Save(ctx, a); err != nil {
+ return nil, fmt.Errorf("failed to save asset %q: %w", path, err)
+ }
+ return u, nil📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func addAsset(path string, ctx context.Context, r *repo.Container, gf gateway.File) (*url.URL, error) { | |
| f, err := os.Open(path) | |
| if err != nil { | |
| return nil, err | |
| } | |
| defer func() { | |
| if cerr := f.Close(); cerr != nil && err == nil { | |
| err = cerr | |
| } | |
| }() | |
| stat, err := f.Stat() | |
| if err != nil { | |
| return nil, err | |
| } | |
| contentType := mime.TypeByExtension(filepath.Ext(path)) | |
| if contentType == "" { | |
| contentType = "application/octet-stream" | |
| } | |
| u, size, err := gf.UploadAsset(ctx, &file.File{ | |
| Content: f, | |
| Path: path, | |
| Size: int64(stat.Size()), | |
| ContentType: contentType, | |
| }) | |
| if err != nil { | |
| return nil, err | |
| } | |
| aid := asset.NewID() | |
| a := asset.New(). | |
| ID(aid). | |
| Workspace(wID). | |
| Project(&pID). | |
| URL(u.String()). | |
| CreatedAt(aid.Timestamp()). | |
| Name(path). | |
| Size(size). | |
| ContentType(contentType). | |
| CoreSupport(true). | |
| MustBuild() | |
| if err := r.DatasetSchema.Save(ctx, dss); err != nil { | |
| return u, r.Asset.Save(ctx, a) | |
| } | |
| func addAsset(path string, ctx context.Context, r *repo.Container, gf gateway.File) (*url.URL, error) { | |
| if path == "" { | |
| return nil, fmt.Errorf("empty asset path") | |
| } | |
| f, err := os.Open(path) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to open asset file %q: %w", path, err) | |
| } | |
| defer func() { | |
| if cerr := f.Close(); cerr != nil && err == nil { | |
| err = fmt.Errorf("failed to close asset file %q: %w", path, cerr) | |
| } | |
| }() | |
| stat, err := f.Stat() | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to stat asset file %q: %w", path, err) | |
| } | |
| if stat.Size() == 0 { | |
| return nil, fmt.Errorf("empty asset file %q", path) | |
| } | |
| contentType := mime.TypeByExtension(filepath.Ext(path)) | |
| if contentType == "" { | |
| contentType = "application/octet-stream" | |
| } | |
| u, size, err := gf.UploadAsset(ctx, &file.File{ | |
| Content: f, | |
| Path: path, | |
| Size: int64(stat.Size()), | |
| ContentType: contentType, | |
| }) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to upload asset %q: %w", path, err) | |
| } | |
| aid := asset.NewID() | |
| a := asset.New(). | |
| ID(aid). | |
| Workspace(wID). | |
| Project(&pID). | |
| URL(u.String()). | |
| CreatedAt(aid.Timestamp()). | |
| Name(path). | |
| Size(size). | |
| ContentType(contentType). | |
| CoreSupport(true). | |
| MustBuild() | |
| if err := r.Asset.Save(ctx, a); err != nil { | |
| return nil, fmt.Errorf("failed to save asset %q: %w", path, err) | |
| } | |
| return u, nil | |
| } |
| @@ -142,7 +142,7 @@ func TestSortByName(t *testing.T) { | |||
| Value("projects").Object(). | |||
| Value("edges").Array() | |||
|
|
|||
| edges.Length().IsEqual(4) | |||
| edges.Length().IsEqual(5) | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the updated test expectation.
The test now expects 5 projects in the edges array for name-based sorting. This should be validated against the test setup which creates projects with names "a-project", "b-project", "A-project", "B-project".
🏁 Script executed:
#!/bin/bash
# Search for project creation calls in test setup
rg -A 5 'createProject\(e, "' server/e2e/gql_project_test.goLength of output: 977
Action: Resolve the discrepancy in the expected project count for name-based sorting.
The test now asserts that the edges array length is 5:
edges.Length().IsEqual(5)However, the test setup for name-based sorting only creates four projects with the names "a-project", "b-project", "A-project", and "B-project". Please confirm whether an additional (fifth) project is expected from another source or if the test expectation should be updated to match the four projects created in this scenario.
hexaforce
force-pushed
the
feat/associate-project-asset
branch
from
6a2cc60 to
9288099
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (2)
server/internal/adapter/gql/resolver_mutation_scene.go (1)
25-30: 🛠️ Refactor suggestionRemove commented out code.
If this code is no longer needed, it should be removed rather than commented out. The version control system preserves the history if needed later. If there's a specific reason to keep it commented, please add a comment explaining why.
server/e2e/common.go (1)
43-63:⚠️ Potential issueAdd error handling for
lo.Mustcalls and avoid global state mutation.The function has two issues:
- Missing error handling in
lo.Mustcalls- Global state mutation via
fr = &filecould cause race conditions in concurrent tests-func initRepos(t *testing.T, useMongo bool, seeder Seeder) (repos *repo.Container, file gateway.File) { +func initRepos(t *testing.T, useMongo bool, seeder Seeder) (*repo.Container, gateway.File, error) { ctx := context.Background() if useMongo { db := mongotest.Connect(t)(t) - accountRepos := lo.Must(accountmongo.New(ctx, db.Client(), db.Name(), false, false, nil)) - repos = lo.Must(mongo.New(ctx, db, accountRepos, false)) + accountRepos, err := accountmongo.New(ctx, db.Client(), db.Name(), false, false, nil) + if err != nil { + return nil, gateway.File{}, fmt.Errorf("failed to create account repos: %w", err) + } + repos, err = mongo.New(ctx, db, accountRepos, false) + if err != nil { + return nil, gateway.File{}, fmt.Errorf("failed to create mongo repos: %w", err) + } } else { repos = memory.New() } - file = lo.Must(fs.NewFile(afero.NewMemMapFs(), "https://example.com/")) - fr = &file + file, err := fs.NewFile(afero.NewMemMapFs(), "https://example.com/") + if err != nil { + return nil, gateway.File{}, fmt.Errorf("failed to create file: %w", err) + } if seeder != nil { if err := seeder(ctx, repos, file); err != nil { - t.Fatalf("failed to seed the db: %s", err) + return nil, gateway.File{}, fmt.Errorf("failed to seed the db: %w", err) } } - return repos, file + return repos, file, nil }
♻️ Duplicate comments (10)
server/internal/adapter/gql/gqlmodel/models_gen.go (1)
906-915: 🛠️ Refactor suggestionRetain consistent usage of the custom
IDtype
PropertyField.IDis declared asstringwhile other fields use the customIDtype. As noted in a past review, aligning them helps with consistency and type safety.Apply this diff to unify the types:
- ID string `json:"id"` + ID ID `json:"id"`server/internal/adapter/gql/resolver_mutation_asset.go (2)
19-26:⚠️ Potential issueAdd project-workspace validation.
Similar to the validation added in UpdateAsset (from past reviews), the CreateAsset resolver should also validate the project-workspace relationship.
Apply this diff to add the validation:
var pid *idx.ID[id.Project] if input.ProjectID != nil { pidValue, err := gqlmodel.ToID[id.Project](*input.ProjectID) if err != nil { return nil, err } + // Validate project exists and belongs to the workspace + project, err := usecases(ctx).Project.FindByID(ctx, pidValue) + if err != nil { + return nil, err + } + if project.Workspace() != tid { + return nil, fmt.Errorf("project does not belong to specified workspace") + } pid = &pidValue }
47-54:⚠️ Potential issueAdd project-workspace validation.
Similar to the validation added in previous reviews, the UpdateAsset resolver should verify the project-workspace relationship.
Apply this diff to add the validation:
var pid *id.ProjectID if project := input.ProjectID; project != nil { pidValue, err := gqlmodel.ToID[id.Project](*input.ProjectID) if err != nil { return nil, err } + // Validate project exists and belongs to the asset's workspace + asset, err := usecases(ctx).Asset.FindByID(ctx, aid) + if err != nil { + return nil, err + } + project, err := usecases(ctx).Project.FindByID(ctx, pidValue) + if err != nil { + return nil, err + } + if project.Workspace() != asset.Workspace() { + return nil, fmt.Errorf("project does not belong to asset's workspace") + } pid = &pidValue }server/internal/usecase/interactor/asset.go (1)
108-127:⚠️ Potential issueAdd project-workspace validation.
Similar to the validation added in previous reviews, the Update method should verify that the provided project belongs to the workspace before updating the asset's project association.
Apply this diff to add the validation:
func (i *Asset) Update(ctx context.Context, aid id.AssetID, pid *id.ProjectID, operator *usecase.Operator) (id.AssetID, *id.ProjectID, error) { return Run2( ctx, operator, i.repos, Usecase().Transaction(), func(ctx context.Context) (id.AssetID, *id.ProjectID, error) { asset, err := i.repos.Asset.FindByID(ctx, aid) if err != nil { return aid, pid, err } 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) return aid, pid, i.repos.Asset.Save(ctx, asset) }, ) }server/e2e/seeder.go (3)
203-209:⚠️ Potential issueFix incorrect error checks in
fullSetupfunction.Multiple error checks in this function 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 = r.Scene.Save(ctx, s); err != err { + if err = r.Scene.Save(ctx, s); err != nil { return err } - if err = r.Property.Save(ctx, prop); err != err { + if err = r.Property.Save(ctx, prop); err != nil { return err }
121-163: 🛠️ Refactor suggestionImprove asset handling robustness.
The asset handling could be more robust with better error handling and validation.
Apply this diff:
func addAsset(path string, ctx context.Context, r *repo.Container, gf gateway.File) (*url.URL, error) { + if path == "" { + return nil, fmt.Errorf("empty asset path") + } + f, err := os.Open(path) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to open asset file %q: %w", path, err) } defer func() { if cerr := f.Close(); cerr != nil && err == nil { - err = cerr + err = fmt.Errorf("failed to close asset file %q: %w", path, cerr) } }() stat, err := f.Stat() if err != nil { - return nil, err + return nil, fmt.Errorf("failed to stat asset file %q: %w", path, err) } + if stat.Size() == 0 { + return nil, fmt.Errorf("empty asset file %q", path) + }
507-522: 🛠️ Refactor suggestionReturn 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 }server/internal/usecase/interactor/project.go (1)
627-649: 🛠️ Refactor suggestionImprove error handling in
AddZipAsset.The function silently ignores errors for external URLs and has a redundant stream close in the error path.
Apply this diff:
if err != nil { - return nil // skip if external URL + if strings.HasPrefix(path, "http://") || strings.HasPrefix(path, "https://") { + return nil // skip external URLs + } + return fmt.Errorf("failed to read asset: %w", err) } defer func() { if cerr := stream.Close(); cerr != nil { - fmt.Printf("Error closing file: %v\n", cerr) + err = errors.Join(err, fmt.Errorf("failed to close stream: %w", cerr)) } }() zipEntryPath := fmt.Sprintf("assets/%s", fileName) zipEntry, err := zipWriter.Create(zipEntryPath) if err != nil { return err } _, err = io.Copy(zipEntry, stream) - if err != nil { - _ = stream.Close() - return err - } - return nil + return errserver/internal/usecase/interactor/scene.go (2)
705-708: 🛠️ Refactor suggestionHandle URL parsing errors consistently.
The URL parsing errors are currently being silently ignored with blank identifiers (
_). This could lead to silent failures in asset handling and make debugging more difficult.Apply this pattern to all URL parsing locations:
- u, _ := url.Parse(urlStr) - if err := AddZipAsset(ctx, i.file, zipWriter, u.Path); err != nil { + u, err := url.Parse(urlStr) + if err != nil { + log.Infofc(ctx, "Failed to parse URL:", err.Error()) + continue + } + if err := AddZipAsset(ctx, i.file, zipWriter, u.Path); err != nil {
734-738: 🛠️ Refactor suggestionImprove error handling for URL parsing.
The URL parsing errors are being silently ignored in multiple locations. Consider implementing consistent error handling across all URL parsing operations.
Apply this pattern to all URL parsing locations:
-if u, ok := field.Value().Value().(*url.URL); ok { - if err := AddZipAsset(ctx, i.file, zipWriter, u.Path); err != nil { - log.Infofc(ctx, "Fail widget addZipAsset :", err.Error()) - } -} +if u, ok := field.Value().Value().(*url.URL); ok { + if err := AddZipAsset(ctx, i.file, zipWriter, u.Path); err != nil { + log.Errorfc(ctx, "Failed to add widget asset: %v", err) + return nil, nil, fmt.Errorf("failed to add widget asset: %w", err) + } +}
🧹 Nitpick comments (8)
server/internal/adapter/gql/resolver_mutation_scene.go (1)
20-20: Document the purpose of the boolean parameter.The
falseparameter inScene.Createis not self-documenting. Please add a comment explaining its purpose or consider using a named parameter for better clarity.server/e2e/gql_scene_test.go (1)
77-100: Consider improving the test helper function.The new
createProjectWithExternalImagefunction could be enhanced in several ways:
- Make the image URL configurable to avoid test environment dependencies
- Remove unnecessary
__typenamefields from the GraphQL query- Add error handling for response parsing
- Parameterize hardcoded values like
descriptionandcoreSupportHere's a suggested improvement:
-func createProjectWithExternalImage(e *httpexpect.Expect, name string) string { +func createProjectWithExternalImage(e *httpexpect.Expect, name, imageURL string, opts ...ProjectOption) string { + // Default options + options := ProjectOptions{ + description: "abc", + coreSupport: true, + visualizer: "CESIUM", + } + // Apply custom options + for _, opt := range opts { + opt(&options) + } requestBody := GraphQLRequest{ OperationName: "CreateProject", Query: `mutation CreateProject($teamId: ID!, $visualizer: Visualizer!, $name: String!, $description: String!, $imageUrl: URL, $coreSupport: Boolean) { createProject( input: {teamId: $teamId, visualizer: $visualizer, name: $name, description: $description, imageUrl: $imageUrl, coreSupport: $coreSupport} ) { project { - id - __typename + id } - __typename } }`, Variables: map[string]any{ "name": name, - "description": "abc", - "imageUrl": "https://test.com/project.jpg", + "description": options.description, + "imageUrl": imageURL, "teamId": wID.String(), - "visualizer": "CESIUM", - "coreSupport": true, + "visualizer": options.visualizer, + "coreSupport": options.coreSupport, }, } res := Request(e, uID.String(), requestBody) - return res.Path("$.data.createProject.project.id").Raw().(string) + projectID := res.Path("$.data.createProject.project.id") + if projectID.Raw() == nil { + panic("Failed to create project: response does not contain project ID") + } + return projectID.Raw().(string) } + +type ProjectOptions struct { + description string + coreSupport bool + visualizer string +} + +type ProjectOption func(*ProjectOptions) + +func WithDescription(desc string) ProjectOption { + return func(o *ProjectOptions) { + o.description = desc + } +} + +func WithCoreSupport(enabled bool) ProjectOption { + return func(o *ProjectOptions) { + o.coreSupport = enabled + } }Usage example:
pID := createProjectWithExternalImage(e, "test", "https://test.com/project.jpg", WithDescription("Custom description"), WithCoreSupport(false), )server/internal/adapter/gql/gqlmodel/models_gen.go (1)
581-588: Consider stronger typing forValueinMergedPropertyField
Usingany(line 583) can loosen type safety and may produce runtime errors if the value is used incorrectly. Consider introducing a more robust type or a union-like structure to maintain clarity and ensure correctness.server/e2e/gql_validate_geojson_test.go (2)
201-236: Consider adding more edge cases to the test suite.The test suite could be enhanced by adding the following test cases:
- URL with invalid GeoJSON content
- URL with valid GeoJSON but invalid geometry
- URL with valid GeoJSON but unsupported geometry type
- URL with valid GeoJSON but missing required properties
238-313: Consider adding more geometry types to the test suite.The test suite could be enhanced by adding test cases for:
- LineString geometry
- Polygon geometry
- MultiPoint geometry
- MultiLineString geometry
- MultiPolygon geometry
- GeometryCollection geometry
server/e2e/gql_storytelling_test.go (1)
735-741: Improve test configuration readability.The test configuration could be more readable with descriptive variable names and comments explaining the purpose of each setting.
Apply this diff:
- c := &config.Config{ + testConfig := &config.Config{ + // Allow CORS requests from example.com for testing Origins: []string{"https://example.com"}, + // Disable authentication for testing purposes AuthSrv: config.AuthSrvConfig{ Disabled: true, }, } - e, _, g := StartServerAndRepos(t, c, true, baseSeeder) + e, _, g := StartServerAndRepos(t, testConfig, true, baseSeeder)server/internal/usecase/interactor/nlslayer.go (2)
156-157: Consider extracting the size limit as a configurable constant.The 10MB size limit is hardcoded. Consider moving it to a configuration file or constant for better maintainability.
-maxDownloadSize := 10 * 1024 * 1024 // 10MB +const maxGeoJSONDownloadSize = 10 * 1024 * 1024 // 10MB +maxDownloadSize := maxGeoJSONDownloadSize
1192-1221: Add content type validation and context support.The download function could be enhanced with:
- Content type validation to ensure GeoJSON files
- Context support for cancellation
- Configurable timeout
-func downloadToBuffer(url string, maxDownloadSize int64) (*bytes.Buffer, error) { +func downloadToBuffer(ctx context.Context, url string, maxDownloadSize int64, timeout time.Duration) (*bytes.Buffer, error) { client := &http.Client{ - Timeout: 30 * time.Second, + Timeout: timeout, } - resp, err := client.Get(url) + req, err := http.NewRequestWithContext(ctx, "GET", url, nil) + if err != nil { + return nil, err + } + resp, err := client.Do(req) if err != nil { return nil, err } defer func() { if err2 := resp.Body.Close(); err2 != nil && err == nil { err = err2 } }() + if contentType := resp.Header.Get("Content-Type"); !strings.Contains(contentType, "application/geo+json") { + return nil, fmt.Errorf("invalid content type: %s", contentType) + } + if resp.StatusCode != http.StatusOK { return nil, fmt.Errorf("failed to download file, status code: %d", resp.StatusCode) }
📜 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 (49)
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(2 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/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(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(6 hunks)server/internal/adapter/gql/resolver_mutation_scene.go(1 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(7 hunks)server/internal/usecase/interactor/project.go(5 hunks)server/internal/usecase/interactor/project_test.go(0 hunks)server/internal/usecase/interactor/scene.go(7 hunks)server/internal/usecase/interfaces/asset.go(2 hunks)server/internal/usecase/interfaces/project.go(2 hunks)server/internal/usecase/interfaces/scene.go(1 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/pkg/nlslayer/feature.go
- server/internal/usecase/interactor/project_test.go
- server/e2e/gql_project_export_test.go
- server/gqlgen.yml
- server/internal/infrastructure/mongo/mongodoc/scene.go
🚧 Files skipped from review as they are similar to previous changes (27)
- server/pkg/asset/builder.go
- server/internal/app/auth_client.go
- server/internal/adapter/gql/gqlmodel/convert_nlslayer_test.go
- server/go.mod
- server/internal/adapter/gql/resolver_team.go
- server/internal/usecase/repo/asset.go
- server/e2e/gql_user_test.go
- server/internal/app/app.go
- server/pkg/nlslayer/feature_test.go
- server/internal/infrastructure/mongo/mongodoc/storytelling.go
- server/internal/adapter/gql/gqlmodel/convert_asset.go
- server/internal/infrastructure/fs/file.go
- server/internal/infrastructure/memory/asset.go
- server/internal/adapter/gql/gqlmodel/convert_project.go
- server/gql/asset.graphql
- server/internal/usecase/interfaces/project.go
- server/internal/infrastructure/fs/file_test.go
- server/e2e/gql_project_test.go
- server/pkg/storytelling/story.go
- server/pkg/asset/asset.go
- server/pkg/asset/id.go
- server/internal/adapter/gql/loader_asset.go
- server/pkg/scene/builder/story.go
- server/gql/workspace.graphql
- server/internal/infrastructure/mongo/mongodoc/asset.go
- server/internal/infrastructure/mongo/asset.go
- server/internal/usecase/interfaces/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: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`.
🔇 Additional comments (24)
server/e2e/gql_scene_test.go (2)
6-6: LGTM! Using versioned import path.The import path correctly uses the
/v2suffix, following Go modules versioning best practices.
16-16: LGTM! Improved response object handling.The changes make type assertions more explicit and safer by using
Object().Value()instead of directValue()calls, which is the recommended approach in httpexpect v2.Also applies to: 42-42, 71-73
server/internal/usecase/interfaces/scene.go (1)
25-25:❓ Verification inconclusive
Document the purpose of the new boolean parameter.
The
Createmethod signature has been updated to include an undocumented boolean parameter. This could lead to confusion for implementers and callers of the interface.
- Please add documentation explaining the purpose and expected behavior of this parameter.
- Consider using a named parameter in the interface definition for better clarity, e.g.,
isImport boolorskipValidation bool.Let's verify the impact of this change:
🏁 Script executed:
#!/bin/bash # Find all implementations of the Scene interface ast-grep --pattern $'type $_ struct { $$$ } func \($_\) Create\(context.Context, id.ProjectID, \*usecase.Operator, bool\) \(\*scene.Scene, error\) { $$$ }'Length of output: 161
Clarify the boolean parameter in the
Createmethod signatureThe updated signature in
server/internal/usecase/interfaces/scene.gonow includes a boolean parameter that lacks documentation, which can be confusing for both implementers and callers. Please update the interface by:
- Documenting the purpose and expected behavior of this boolean parameter.
- Considering a renaming to a more descriptive identifier (for example,
isImportorskipValidation).Additionally, please ensure that all implementations and callers of the
Createmethod are manually verified to account for this change.server/internal/adapter/gql/gqlmodel/models_gen.go (7)
138-138: AddprojectIdfield for asset-project association
This new pointer field aligns perfectly with the PR objective of linking an asset to a specific project.
186-186: EnhanceCreateAssetInputwith aprojectIdparameter
IncludingprojectIdin the input structure cleanly supports creating assets that are associated with a project.
919-921: No issues inPropertyFieldLink
The optionalDatasetIDpointer and mandatoryDatasetSchemaIDfields are logically consistent.
1135-1149: Scene struct changes align with project-based ownership
All newly introduced fields support a closer relationship between scenes and their parent project/team. This is in line with the PR's goals and appears consistent across references.
1330-1332: UpdateAssetInput extended to includeprojectId
This addition provides flexibility for reassigning or updating asset-project relationships.
1335-1337: UpdateAssetPayload includesprojectId
The payload structure now reflects any updated project association for an asset.
1844-1844: AddedPluginExtensionTypeInfoboxBlockin switch
Recognizing the new infobox block type ensures proper validation inIsValid().server/internal/usecase/interactor/asset.go (3)
39-51: LGTM!The function has been correctly updated to support project-specific asset retrieval while maintaining the existing error handling and operator validation.
88-96:⚠️ Potential issueAdd project-workspace validation.
Similar to the validation added in previous reviews, the Create method should verify that the provided project belongs to the workspace before creating the asset.
Apply this diff to add the validation:
+ 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() != inp.WorkspaceID { + return nil, fmt.Errorf("project does not belong to workspace") + } + } a, err := asset.New(). NewID(). Workspace(inp.WorkspaceID). Project(inp.ProjectID). Name(path.Base(inp.File.Path)). Size(size). URL(url.String()). CoreSupport(inp.CoreSupport). Build()
154-195:⚠️ Potential issueAdd project-workspace validation.
Similar to the validation added in previous reviews, the ImportAssetFiles method should verify that the provided project belongs to the workspace before creating the asset.
Apply this diff to add the validation:
func (i *Asset) ImportAssetFiles(ctx context.Context, realName string, zipFile *zip.File, workspace idx.ID[accountdomain.Workspace], project *id.ProjectID) (*url.URL, int64, error) { + if project != nil { + // Validate that project belongs to workspace + 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!The function has been correctly updated to support project-specific asset retrieval while maintaining the existing error handling and pagination.
server/e2e/gql_validate_geojson_test.go (1)
186-186: LGTM!The function has been correctly updated to support the new project ID parameter while maintaining the existing test cases and assertions.
server/e2e/common.go (2)
303-308: Enhance error handling and type support inJSONEqRegexpValue.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
320-334: LGTM! Well-structured type handling inValueDump.The function properly handles different types using a switch statement and includes clear error handling for JSON marshaling.
server/e2e/gql_asset_test.go (2)
16-127: LGTM! Well-structured test coverage for core support functionality.The test cases thoroughly validate:
- Asset creation with and without core support
- Asset properties validation
- Database operations
- Asset retrieval and filtering
239-271: Add error handling for GraphQL responses.The
updateAssetandremoveAssetfunctions should verify the GraphQL response for errors.server/internal/adapter/gql/resolver_mutation_project.go (3)
127-210: Sanitize project ID and improve file cleanup inExportProject.Two issues need to be addressed:
- Project ID is used directly in file name without sanitization
- File cleanup in defer could fail silently
212-335: Consider using a transaction for asset imports.Asset imports should be part of the transaction to ensure consistency if the import fails.
338-385: Add error handling for JSON unmarshaling in helper functions.The helper functions should handle JSON unmarshaling errors more gracefully and provide context about which part of the data failed to unmarshal.
server/internal/usecase/interactor/nlslayer.go (2)
893-895: LGTM! Good use of unique feature IDs.The change ensures each feature gets a unique identifier, which is a good practice for data integrity.
1257-1262: LGTM! Enhanced GeoJSON validation.The changes improve error handling by:
- Adding explicit BBox validation
- Providing clearer error messages
| var fr *gateway.File | ||
|
|
||
| type Seeder func(ctx context.Context, r *repo.Container, f gateway.File) error |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider removing the global variable and using dependency injection.
The global variable fr could lead to state management issues in concurrent tests. Consider passing the gateway.File directly to functions that need it.
-var fr *gateway.File
+type TestContext struct {
+ file gateway.File
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var fr *gateway.File | |
| type Seeder func(ctx context.Context, r *repo.Container, f gateway.File) error | |
| type TestContext struct { | |
| file gateway.File | |
| } | |
| type Seeder func(ctx context.Context, r *repo.Container, f gateway.File) error |
| // go test -v -run TestAssociateProjectGetAssets ./e2e/... | ||
|
|
||
| func TestAssociateProjectGetAssets(t *testing.T) { | ||
|
|
||
| c := &config.Config{ | ||
| Origins: []string{"https://example.com"}, | ||
| AuthSrv: config.AuthSrvConfig{ | ||
| Disabled: true, | ||
| }, | ||
| } | ||
|
|
||
| e, _, _ := StartServerAndRepos(t, c, true, baseSeeder) | ||
| teamId := wID.String() | ||
|
|
||
| // Create projectA >>> test.png | ||
| pidA := createProject(e, "projectA") | ||
| createAsset(t, e, "test.png", true, teamId, &pidA) | ||
|
|
||
| // Create projectB >>> test.csv | ||
| pidB := createProject(e, "projectB") | ||
| createAsset(t, e, "test.csv", true, teamId, &pidB) | ||
|
|
||
| // Get projectA >>> test.png | ||
| res := getAssets(e, teamId, &pidA) | ||
| assets := res.Path("$.data.assets.nodes").Array() | ||
| assets.Length().IsEqual(1) | ||
| assets.Value(0).Object().HasValue("name", "test.png") | ||
|
|
||
| // Get projectB >>> test.csv | ||
| res = getAssets(e, teamId, &pidB) | ||
| assets = res.Path("$.data.assets.nodes").Array() | ||
| assets.Length().IsEqual(1) | ||
| assets.Value(0).Object().HasValue("name", "test.csv") | ||
|
|
||
| // Get none project(Workspace) >>> test.png, test.csv | ||
| res = getAssets(e, teamId, nil) | ||
| assets = res.Path("$.data.assets.nodes").Array() | ||
| assets.Length().IsEqual(3) | ||
| assets.Value(0).Object().HasValue("name", "test.csv") | ||
| assets.Value(1).Object().HasValue("name", "test.png") | ||
|
|
||
| assetId0 := assets.Value(0).Object().Value("id").Raw().(string) | ||
| assetId1 := assets.Value(1).Object().Value("id").Raw().(string) | ||
|
|
||
| // Move projectA(test.png) >>> projectB | ||
| updateAsset(e, assetId1, &pidB) | ||
| res = getAssets(e, teamId, &pidB) | ||
| assets = res.Path("$.data.assets.nodes").Array() | ||
| assets.Length().IsEqual(2) // projectB >>> test.png, test.csv | ||
|
|
||
| // Remove projectID test | ||
| updateAsset(e, assetId0, nil) // projectID > null | ||
| updateAsset(e, assetId1, nil) // projectID > null | ||
|
|
||
| res = getAssets(e, teamId, &pidB) | ||
| assets = res.Path("$.data.assets.nodes").Array() | ||
| assets.Length().IsEqual(0) // projectB >>> none | ||
|
|
||
| // Remove asset test | ||
| removeAsset(e, assetId0) | ||
| removeAsset(e, assetId1) | ||
|
|
||
| res = getAssets(e, teamId, nil) | ||
| assets = res.Path("$.data.assets.nodes").Array() | ||
| assets.Length().IsEqual(1) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for asset operations in TestAssociateProjectGetAssets.
The test should verify error responses for:
- Invalid project IDs
- Non-existent assets
- Permission issues
func TestAssociateProjectGetAssets(t *testing.T) {
e := Server(t, fullSeeder)
teamId := wID.String()
+ // Test invalid project ID
+ invalidPID := "invalid_id"
+ res := createAsset(t, e, "test.png", true, teamId, &invalidPID)
+ res.Path("$.errors").NotNull()
+
+ // Test non-existent asset
+ nonExistentAssetID := "non_existent_id"
+ res = updateAsset(e, nonExistentAssetID, nil)
+ res.Path("$.errors").NotNull()
// Existing test cases...
}Committable suggestion skipped: line range outside the PR's diff.
| func exporProject(t *testing.T, e *httpexpect.Expect, p string) string { | ||
| requestBody := GraphQLRequest{ | ||
| OperationName: "ExportProject", | ||
| Query: "mutation ExportProject($projectId: ID!) { exportProject(input: {projectId: $projectId}) { projectDataPath __typename } }", | ||
| Variables: map[string]any{ | ||
| "projectId": p, | ||
| }, | ||
| } | ||
| r := Request(e, uID.String(), requestBody) | ||
| // ValueDump(r) | ||
| downloadPath := r.Path("$.data.exportProject.projectDataPath").Raw().(string) | ||
| downloadResponse := e.GET(fmt.Sprintf("http://localhost:8080%s", downloadPath)). | ||
| Expect(). | ||
| Status(http.StatusOK). | ||
| Body().Raw() | ||
| fileName := "project_data.zip" | ||
| err := os.WriteFile(fileName, []byte(downloadResponse), os.ModePerm) | ||
| assert.Nil(t, err) | ||
| return fileName | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and use temporary directory in exporProject.
The function should:
- Handle HTTP errors
- Use
os.CreateTempfor file creation
-func exporProject(t *testing.T, e *httpexpect.Expect, p string) string {
+func exporProject(t *testing.T, e *httpexpect.Expect, p string) (string, error) {
requestBody := GraphQLRequest{
OperationName: "ExportProject",
Query: "mutation ExportProject($projectId: ID!) { exportProject(input: {projectId: $projectId}) { projectDataPath __typename } }",
Variables: map[string]any{
"projectId": p,
},
}
r := Request(e, uID.String(), requestBody)
+ if r.Path("$.errors").Raw() != nil {
+ return "", fmt.Errorf("GraphQL error: %v", r.Path("$.errors").Raw())
+ }
downloadPath := r.Path("$.data.exportProject.projectDataPath").Raw().(string)
- downloadResponse := e.GET(fmt.Sprintf("http://localhost:8080%s", downloadPath)).
+ resp := e.GET(fmt.Sprintf("http://localhost:8080%s", downloadPath)).
Expect().
- Status(http.StatusOK).
- Body().Raw()
+ Status(http.StatusOK)
+ if resp.Raw().StatusCode != http.StatusOK {
+ return "", fmt.Errorf("download failed with status: %d", resp.Raw().StatusCode)
+ }
+ downloadResponse := resp.Body().Raw()
- fileName := "project_data.zip"
+ tmpFile, err := os.CreateTemp("", "project_*.zip")
+ if err != nil {
+ return "", fmt.Errorf("failed to create temp file: %w", err)
+ }
+ fileName := tmpFile.Name()
- err := os.WriteFile(fileName, []byte(downloadResponse), os.ModePerm)
+ if err := os.WriteFile(fileName, []byte(downloadResponse), 0600); err != nil {
+ os.Remove(fileName)
+ return "", fmt.Errorf("failed to write file: %w", err)
+ }
- return fileName
+ return fileName, nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func exporProject(t *testing.T, e *httpexpect.Expect, p string) string { | |
| requestBody := GraphQLRequest{ | |
| OperationName: "ExportProject", | |
| Query: "mutation ExportProject($projectId: ID!) { exportProject(input: {projectId: $projectId}) { projectDataPath __typename } }", | |
| Variables: map[string]any{ | |
| "projectId": p, | |
| }, | |
| } | |
| r := Request(e, uID.String(), requestBody) | |
| // ValueDump(r) | |
| downloadPath := r.Path("$.data.exportProject.projectDataPath").Raw().(string) | |
| downloadResponse := e.GET(fmt.Sprintf("http://localhost:8080%s", downloadPath)). | |
| Expect(). | |
| Status(http.StatusOK). | |
| Body().Raw() | |
| fileName := "project_data.zip" | |
| err := os.WriteFile(fileName, []byte(downloadResponse), os.ModePerm) | |
| assert.Nil(t, err) | |
| return fileName | |
| } | |
| func exporProject(t *testing.T, e *httpexpect.Expect, p string) (string, error) { | |
| requestBody := GraphQLRequest{ | |
| OperationName: "ExportProject", | |
| Query: "mutation ExportProject($projectId: ID!) { exportProject(input: {projectId: $projectId}) { projectDataPath __typename } }", | |
| Variables: map[string]any{ | |
| "projectId": p, | |
| }, | |
| } | |
| r := Request(e, uID.String(), requestBody) | |
| if r.Path("$.errors").Raw() != nil { | |
| return "", fmt.Errorf("GraphQL error: %v", r.Path("$.errors").Raw()) | |
| } | |
| downloadPath := r.Path("$.data.exportProject.projectDataPath").Raw().(string) | |
| resp := e.GET(fmt.Sprintf("http://localhost:8080%s", downloadPath)). | |
| Expect(). | |
| Status(http.StatusOK) | |
| if resp.Raw().StatusCode != http.StatusOK { | |
| return "", fmt.Errorf("download failed with status: %d", resp.Raw().StatusCode) | |
| } | |
| downloadResponse := resp.Body().Raw() | |
| tmpFile, err := os.CreateTemp("", "project_*.zip") | |
| if err != nil { | |
| return "", fmt.Errorf("failed to create temp file: %w", err) | |
| } | |
| fileName := tmpFile.Name() | |
| if err := os.WriteFile(fileName, []byte(downloadResponse), 0600); err != nil { | |
| os.Remove(fileName) | |
| return "", fmt.Errorf("failed to write file: %w", err) | |
| } | |
| return fileName, nil | |
| } |
| func TestProjectExportImport(t *testing.T) { | ||
|
|
||
| e := Server(t, baseSeeder) | ||
| e := Server(t, fullSeeder) | ||
|
|
||
| filePath := "test.zip" | ||
| // 1.Retrieve the data created by fullSeeder. | ||
| expected := getScene(e, sID.String(), language.English.String()) | ||
|
|
||
| r := importProject(t, e, filePath) | ||
| // 2.Export the data. | ||
| fileName := exporProject(t, e, pID.String()) | ||
|
|
||
| r.Value("project").NotNull() | ||
| r.Value("plugins").Array() | ||
| r.Value("schema").Array() | ||
| r.Value("scene").NotNull() | ||
| r.Value("nlsLayer").Array() | ||
| r.Value("style").Array() | ||
| r.Value("story").NotNull() | ||
| // 3.Import the exported data. | ||
| r := importProject(t, e, fileName) | ||
|
|
||
| sid := r.Value("scene").Object().Value("id").Raw().(string) | ||
| r.Object().Value("project").NotNull() | ||
| r.Object().Value("plugins").Array() | ||
| r.Object().Value("schema").Array() | ||
| r.Object().Value("scene").NotNull() | ||
| r.Object().Value("nlsLayer").Array() | ||
| r.Object().Value("style").Array() | ||
| r.Object().Value("story").NotNull() | ||
|
|
||
| r = getScene(e, sid, language.English.String()) | ||
| // fmt.Println(toJSONString(r.Raw())) | ||
| r.Object().Value("project").Object().HasValue("name", pName) | ||
| r.Object().Value("project").Object().HasValue("description", pDesc) | ||
|
|
||
| r.Value("id").IsEqual(sid) | ||
| newId := r.Object().Value("scene").Object().Value("id").Raw().(string) | ||
|
|
||
| // 4.Retrieve the imported data. | ||
| actual := getScene(e, newId, language.English.String()) | ||
|
|
||
| // 5. Compare and check each value individually. | ||
| compareValue(t, "styles", expected, actual) | ||
| compareValue(t, "widgets", expected, actual) | ||
| compareValue(t, "widgetAlignSystem", expected, actual) | ||
| compareValue(t, "stories", expected, actual) | ||
| compareValue(t, "newLayers", expected, actual) | ||
| compareValue(t, "plugins", expected, actual) | ||
| compareValue(t, "property", expected, actual) | ||
|
|
||
| defer func() { | ||
| err := os.Remove(fileName) | ||
| assert.Nil(t, err) | ||
| }() | ||
|
|
||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add cleanup in error cases for TestProjectExportImport.
The test should ensure cleanup even if assertions fail.
func TestProjectExportImport(t *testing.T) {
+ var fileName string
+ defer func() {
+ if fileName != "" {
+ if err := os.Remove(fileName); err != nil {
+ t.Logf("failed to cleanup test file: %v", err)
+ }
+ }
+ }()
e := Server(t, fullSeeder)
expected := getScene(e, sID.String(), language.English.String())
- fileName := exporProject(t, e, pID.String())
+ var err error
+ fileName, err = exporProject(t, e, pID.String())
+ assert.NoError(t, err)
// Rest of the test...
- defer func() {
- err := os.Remove(fileName)
- assert.Nil(t, err)
- }()
}Committable suggestion skipped: line range outside the PR's diff.
hexaforce
force-pushed
the
feat/associate-project-asset
branch
from
9288099 to
a03726c
Compare
hexaforce
force-pushed
the
feat/associate-project-asset
branch
from
a03726c to
47a97b9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (8)
server/e2e/common.go (3)
35-36: 🛠️ Refactor suggestionConsider removing the global variable and using dependency injection.
The global variable
frcould lead to state management issues in concurrent tests. Consider passing thegateway.Filedirectly to functions that need it.-var fr *gateway.File +type TestContext struct { + file gateway.File +}
303-308: 🛠️ 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 explaining the function's purpose
+// 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 }
54-55:⚠️ Potential issueFix pipeline failure in
initReposfunction.The
lo.Mustcall is missing an argument.- file = lo.Must(fs.NewFile(afero.NewMemMapFs(), "https://example.com/")) + file = lo.Must(fs.NewFile(afero.NewMemMapFs(), "https://example.com/"), nil)server/internal/adapter/gql/resolver_mutation_project.go (2)
130-130:⚠️ Potential issueSanitize the project ID before using it in the file name.
The project ID is used directly in the file name without sanitization, which could lead to issues with special characters or path traversal.
- zipFile, err := fs.Create(fmt.Sprintf("%s.zip", strings.ToLower(string(input.ProjectID)))) + sanitizedID := strings.Map(func(r rune) rune { + if r >= 'a' && r <= 'z' || r >= '0' && r <= '9' || r == '-' { + return r + } + return '_' + }, strings.ToLower(string(input.ProjectID))) + zipFile, err := fs.Create(fmt.Sprintf("%s.zip", sanitizedID))
229-233: 🛠️ Refactor suggestionImprove transaction rollback handling.
The transaction rollback in the deferred function could be more robust.
defer func() { - if err2 := tx.End(ctx); err == nil && err2 != nil { + if err2 := tx.End(ctx); err2 != nil { + if err == nil { + err = err2 + } else { + err = fmt.Errorf("multiple errors: %v; %v", err, err2) + } + } + if err != nil && tx.IsActive() { + _ = tx.Rollback(ctx) + } }()server/e2e/gql_project_export_import_test.go (2)
122-141: 🛠️ Refactor suggestionAdd error handling and use temporary directory in
exporProject.The function should:
- Handle HTTP errors
- Use
os.CreateTempfor file creation-func exporProject(t *testing.T, e *httpexpect.Expect, p string) string { +func exporProject(t *testing.T, e *httpexpect.Expect, p string) (string, error) { requestBody := GraphQLRequest{ OperationName: "ExportProject", Query: "mutation ExportProject($projectId: ID!) { exportProject(input: {projectId: $projectId}) { projectDataPath __typename } }", Variables: map[string]any{ "projectId": p, }, } r := Request(e, uID.String(), requestBody) + if r.Path("$.errors").Raw() != nil { + return "", fmt.Errorf("GraphQL error: %v", r.Path("$.errors").Raw()) + } downloadPath := r.Path("$.data.exportProject.projectDataPath").Raw().(string) - downloadResponse := e.GET(fmt.Sprintf("http://localhost:8080%s", downloadPath)). + resp := e.GET(fmt.Sprintf("http://localhost:8080%s", downloadPath)). Expect(). - Status(http.StatusOK). - Body().Raw() + Status(http.StatusOK) + if resp.Raw().StatusCode != http.StatusOK { + return "", fmt.Errorf("download failed with status: %d", resp.Raw().StatusCode) + } + downloadResponse := resp.Body().Raw() - fileName := "project_data.zip" + tmpFile, err := os.CreateTemp("", "project_*.zip") + if err != nil { + return "", fmt.Errorf("failed to create temp file: %w", err) + } + fileName := tmpFile.Name() - err := os.WriteFile(fileName, []byte(downloadResponse), os.ModePerm) + if err := os.WriteFile(fileName, []byte(downloadResponse), 0600); err != nil { + os.Remove(fileName) + return "", fmt.Errorf("failed to write file: %w", err) + } - return fileName + return fileName, nil }
21-64: 🛠️ Refactor suggestionAdd cleanup in error cases for
TestProjectExportImport.The test should ensure cleanup even if assertions fail.
func TestProjectExportImport(t *testing.T) { + var fileName string + defer func() { + if fileName != "" { + if err := os.Remove(fileName); err != nil { + t.Logf("failed to cleanup test file: %v", err) + } + } + }() e := Server(t, fullSeeder) expected := getScene(e, sID.String(), language.English.String()) - fileName := exporProject(t, e, pID.String()) + var err error + fileName, err = exporProject(t, e, pID.String()) + assert.NoError(t, err) // Rest of the test... - defer func() { - err := os.Remove(fileName) - assert.Nil(t, err) - }() }server/e2e/seeder.go (1)
507-522: 🛠️ Refactor suggestionReturn 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.
-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 }Update the caller at line 484 to handle the error:
- err = json.Unmarshal([]byte(fmt.Sprintf(`{ + uuid, err := generateUUID() + if err != nil { + return err + } + err = json.Unmarshal([]byte(fmt.Sprintf(`{ "extrudedHeight": 0, - "id": "%s", + "id": "%s", "positions": [ [ -3958794.1421583104, 3350991.8464303534, 3699620.1697127568 ] ], "type": "marker" - }`, generateUUID())), &p) + }`, uuid)), &p)
🧹 Nitpick comments (13)
server/internal/usecase/interactor/layer.go (2)
31-31: Consider translating the TODO comment to English.The TODO comment is in Japanese, which might not be accessible to all developers. Consider translating it to English for better maintainability.
-// TODO: レイヤー作成のドメインロジックがここに多く漏れ出しているのでドメイン層に移す +// TODO: Move layer creation domain logic to the domain layer as it's leaking into the interactor layer
1001-1033: Consider refactoring the format-specific decoder initialization.The decoder initialization logic could be simplified by extracting it into a separate factory method. This would improve maintainability and make it easier to add new formats in the future.
Here's a suggested approach:
// Create a new file at: server/internal/usecase/interactor/layer_decoder.go type decoderFactory struct { scene id.SceneID } func newDecoderFactory(scene id.SceneID) *decoderFactory { return &decoderFactory{scene: scene} } func (f *decoderFactory) createDecoder(format decoding.LayerEncodingFormat, file *File) (decoding.Decoder, error) { switch format { case decoding.LayerEncodingFormatKML: d := xml.NewDecoder(file.Content) return decoding.NewKMLDecoder(d, f.scene), nil case decoding.LayerEncodingFormatGEOJSON: return decoding.NewGeoJSONDecoder(file.Content, f.scene), nil case decoding.LayerEncodingFormatCZML: d := json.NewDecoder(file.Content) return decoding.NewCZMLDecoder(d, f.scene), nil case decoding.LayerEncodingFormatREEARTH: d := json.NewDecoder(file.Content) return decoding.NewReearthDecoder(d, f.scene), nil case decoding.LayerEncodingFormatSHAPE: return f.createShapeDecoder(file) default: return nil, errors.New("unsupported format") } } func (f *decoderFactory) createShapeDecoder(file *File) (decoding.Decoder, error) { if file.Size > 2097152 { return nil, errors.New("file is too big") } var reader decoding.ShapeReader var err error if file.ContentType == "application/octet-stream" && strings.HasSuffix(file.Path, ".shp") { reader, err = shp.ReadFrom(file.Content) } else if file.ContentType == "application/zip" && strings.HasSuffix(file.Path, ".zip") { reader, err = shp.ReadZipFrom(file.Content) } if err != nil { return nil, err } return decoding.NewShapeDecoder(reader, f.scene), nil }Then update the
ImportLayermethod to use the factory:-var decoder decoding.Decoder -switch inp.Format { - case decoding.LayerEncodingFormatKML: - d := xml.NewDecoder(inp.File.Content) - decoder = decoding.NewKMLDecoder(d, parent.Scene()) - // ... other cases ... -} -if decoder == nil { - return nil, nil, errors.New("unsupported format") -} +factory := newDecoderFactory(parent.Scene()) +decoder, err := factory.createDecoder(inp.Format, inp.File) +if err != nil { + return nil, nil, err +}server/internal/infrastructure/mongo/asset.go (1)
72-80: Refactor filter construction for better readability.Consider extracting the filter construction logic into a separate helper function to improve code organization and maintainability.
+func buildAssetFilter(id accountdomain.WorkspaceID, projectId *id.ProjectID) bson.M { + filter := bson.M{ + "coresupport": true, + } + if projectId != nil { + filter["project"] = projectId.String() + } else { + filter["team"] = id.String() + } + return filter +} func (r *Asset) FindByWorkspaceProject(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 := buildAssetFilter(id, projectId)server/e2e/gql_validate_geojson_test.go (2)
201-236: Consider adding more edge cases to GeoJSON URL validation tests.The test covers basic scenarios but could be enhanced with additional cases:
- URL with invalid GeoJSON content
- URL with large GeoJSON file
- URL with redirects
- URL with different content types
238-313: Add test cases for additional GeoJSON geometry types.Consider adding test cases for:
- LineString
- Polygon
- MultiPoint
- MultiLineString
- MultiPolygon
- GeometryCollection
server/e2e/gql_asset_test.go (2)
31-41: Consider extracting repeated JSON test data into reusable fixtures.The test contains multiple similar JSON structures that could be refactored into reusable test fixtures to improve maintainability and reduce duplication.
Create a helper function to generate the expected JSON structure:
+func expectedAssetJSON(name string, coreSupport bool, size int, projectId *string) string { + return fmt.Sprintf(`{ + "__typename": "Asset", + "contentType": "", + "coreSupport": %t, + "id": ".*", + "name": "%s", + "projectId": %s, + "size": %d, + "teamId": ".*", + "url": ".*" + }`, coreSupport, name, projectId, size) +}Then use it in the tests:
-JSONEqRegexpValue(t, a, `{...}`) +JSONEqRegexpValue(t, a, expectedAssetJSON("test.png", true, 30438, nil))Also applies to: 45-55, 59-69, 73-83
302-365: Consider pagination parameters validation in getAssets query.The function accepts pagination parameters but doesn't validate their values. Consider adding validation for:
- Maximum page size
- Sort direction values
- Sort field values
Add parameter validation:
func getAssets(e *httpexpect.Expect, teamId string, projectId *string) *httpexpect.Value { + const maxPageSize = 100 + pageSize := 20 + if pageSize > maxPageSize { + pageSize = maxPageSize + } + + validSortFields := map[string]bool{"DATE": true, "NAME": true, "SIZE": true} + sortField := "DATE" + if _, valid := validSortFields[sortField]; !valid { + sortField = "DATE" + } + requestBody := GraphQLRequest{ // ... existing code ... Variables: map[string]any{ "teamId": teamId, "projectId": projectId, "pagination": map[string]any{ - "first": 20, + "first": pageSize, }, "sort": map[string]string{ "direction": "DESC", - "field": "DATE", + "field": sortField, }, }, } return Request(e, uID.String(), requestBody) }server/e2e/gql_scene_test.go (1)
77-100: Improve test maintainability and error handling.The new helper function could benefit from the following improvements:
- Move the GraphQL query to a constant for reusability
- Use meaningful test data instead of magic values
- Add error handling for the type assertion
Consider applying these changes:
+const createProjectMutation = `mutation CreateProject($teamId: ID!, $visualizer: Visualizer!, $name: String!, $description: String!, $imageUrl: URL, $coreSupport: Boolean) { + createProject( input: {teamId: $teamId, visualizer: $visualizer, name: $name, description: $description, imageUrl: $imageUrl, coreSupport: $coreSupport} ) { + project { + id + __typename + } + __typename + } +}` func createProjectWithExternalImage(e *httpexpect.Expect, name string) string { requestBody := GraphQLRequest{ OperationName: "CreateProject", - Query: `mutation CreateProject($teamId: ID!, $visualizer: Visualizer!, $name: String!, $description: String!, $imageUrl: URL, $coreSupport: Boolean) { - createProject( input: {teamId: $teamId, visualizer: $visualizer, name: $name, description: $description, imageUrl: $imageUrl, coreSupport: $coreSupport} ) { - project { - id - __typename - } - __typename - } - }`, + Query: createProjectMutation, Variables: map[string]any{ "name": name, - "description": "abc", - "imageUrl": "https://test.com/project.jpg", + "description": "Test project description", + "imageUrl": "https://example.com/test-project-image.jpg", "teamId": wID.String(), "visualizer": "CESIUM", "coreSupport": true, }, } res := Request(e, uID.String(), requestBody) - return res.Path("$.data.createProject.project.id").Raw().(string) + projectID, ok := res.Path("$.data.createProject.project.id").Raw().(string) + if !ok { + panic("Failed to get project ID from response") + } + return projectID }server/internal/adapter/gql/resolver_mutation_project.go (1)
338-347: Add error handling for JSON unmarshaling in helper functions.The helper functions should handle JSON unmarshaling errors more gracefully and provide context about which part of the data failed to unmarshal.
- if err := json.Unmarshal(data, &jsonData); err != nil { - return "", nil, err + if err := json.Unmarshal(data, &jsonData); err != nil { + return "", nil, fmt.Errorf("failed to unmarshal scene data: %w", err)server/e2e/seeder.go (1)
121-163: Improve asset handling robustness.The asset handling could be more robust with better error handling and validation.
func addAsset(path string, ctx context.Context, r *repo.Container, gf gateway.File) (*url.URL, error) { + if path == "" { + return nil, fmt.Errorf("empty asset path") + } + f, err := os.Open(path) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to open asset file %q: %w", path, err) } defer func() { if cerr := f.Close(); cerr != nil && err == nil { - err = cerr + err = fmt.Errorf("failed to close asset file %q: %w", path, cerr) } }() stat, err := f.Stat() if err != nil { - return nil, err + return nil, fmt.Errorf("failed to stat asset file %q: %w", path, err) } + if stat.Size() == 0 { + return nil, fmt.Errorf("empty asset file %q", path) + } + contentType := mime.TypeByExtension(filepath.Ext(path)) if contentType == "" { contentType = "application/octet-stream" } u, size, err := gf.UploadAsset(ctx, &file.File{ Content: f, Path: path, Size: int64(stat.Size()), ContentType: contentType, }) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to upload asset %q: %w", path, err) } aid := asset.NewID() a := asset.New(). ID(aid). Workspace(wID). Project(&pID). URL(u.String()). CreatedAt(aid.Timestamp()). Name(path). Size(size). ContentType(contentType). CoreSupport(true). MustBuild() - return u, r.Asset.Save(ctx, a) + if err := r.Asset.Save(ctx, a); err != nil { + return nil, fmt.Errorf("failed to save asset %q: %w", path, err) + } + return u, nilserver/internal/usecase/interactor/project.go (2)
524-530: Improve error handling for project image export.The error handling for project image export could be more descriptive.
Apply this diff to improve error handling:
if prj.ImageURL() != nil { err := AddZipAsset(ctx, i.file, zipWriter, prj.ImageURL().Path) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to export project image: %w", err) } }
579-588: Consider making default values configurable.The hardcoded default values for
alias,archived, andcoreSupportcould be made configurable through parameters or configuration.Apply this diff to make the values configurable:
+type ProjectDefaults struct { + Alias string + Archived bool + CoreSupport bool +} + func (i *Project) ImportProjectData(ctx context.Context, workspace idx.ID[accountdomain.Workspace], projectData map[string]interface{}, op *usecase.Operator) (*project.Project, usecasex.Tx, error) { + defaults := ProjectDefaults{ + Alias: "", + Archived: false, + CoreSupport: true, + } // ... - alias := "" - archived := false - coreSupport := true + alias := defaults.Alias + archived := defaults.Archived + coreSupport := defaults.CoreSupportserver/e2e/gql_storytelling_test.go (1)
769-847: Consider using a test helper for JSON comparison.The expected JSON structure is quite large and could be moved to a test helper or fixture file to improve test readability.
Create a helper function to load the expected JSON from a fixture file:
+func loadExpectedJSON(t *testing.T, filename string, params ...interface{}) string { + data, err := os.ReadFile(filepath.Join("testdata", filename)) + assert.NoError(t, err) + return fmt.Sprintf(string(data), params...) +} + func TestStoryPublishing(t *testing.T) { // ... - expected := fmt.Sprintf(`{...}`, blockID) + expected := loadExpectedJSON(t, "story_publishing.json", blockID)
📜 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 (107)
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(2 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(1 hunks)server/internal/usecase/interactor/layer_test.go(1 hunks)server/internal/usecase/interactor/nlslayer.go(7 hunks)server/internal/usecase/interactor/nlslayer_test.go(5 hunks)server/internal/usecase/interactor/plugin_test.go(1 hunks)server/internal/usecase/interactor/plugin_upload_test.go(5 hunks)server/internal/usecase/interactor/project.go(5 hunks)server/internal/usecase/interactor/project_test.go(0 hunks)server/internal/usecase/interactor/property_test.go(3 hunks)server/internal/usecase/interactor/scene.go(6 hunks)server/internal/usecase/interactor/scene_plugin.go(0 hunks)server/internal/usecase/interactor/scene_plugin_test.go(3 hunks)server/internal/usecase/interactor/scene_test.go(1 hunks)server/internal/usecase/interactor/storytelling_test.go(1 hunks)server/internal/usecase/interactor/style_test.go(1 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/initializer_test.go(0 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.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(0 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(1 hunks)
⛔ Files not processed due to max files limit (11)
- server/pkg/scene/scene.go
- server/pkg/scene/scene_test.go
- server/pkg/scene/sceneops/plugin_migrator_test.go
- server/pkg/storytelling/story.go
- web/src/services/api/propertyApi/utils.ts
- web/src/services/api/sceneApi.ts
- web/src/services/gql/fragments/dataset.ts
- 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/scene.ts
💤 Files with no reviewable changes (48)
- server/e2e/gql_nlslayer_test.go
- server/pkg/layer/initializer_test.go
- server/internal/adapter/gql/resolver_property_test.go
- server/internal/adapter/gql/gqlmodel/convert.go
- server/internal/adapter/gql/gqlmodel/convert_plugin.go
- server/pkg/scene/builder/builder_test.go
- server/pkg/layer/layerops/processor_test.go
- server/pkg/scene/builder/encoder.go
- server/internal/usecase/interactor/project_test.go
- server/internal/usecase/interactor/scene_plugin.go
- server/internal/adapter/gql/gqldataloader/dataloader.go
- server/internal/adapter/gql/gqlmodel/convert_scene.go
- server/internal/adapter/gql/context.go
- server/pkg/nlslayer/feature.go
- server/internal/adapter/gql/gqlmodel/models.go
- server/e2e/dataset_export_test.go
- server/internal/adapter/gql/resolver_mutation_scene.go
- server/gql/property.graphql
- server/internal/adapter/gql/gqlmodel/convert_dataset.go
- server/e2e/gql_project_export_test.go
- server/gql/dataset.graphql
- server/pkg/scene/builder/encoder_test.go
- server/internal/adapter/gql/gqlmodel/convert_layer.go
- server/e2e/gql_layer_test.go
- server/internal/adapter/gql/resolver_layer.go
- server/internal/adapter/gql/resolver_mutation_tag.go
- server/gqlgen.yml
- server/internal/adapter/gql/gqlmodel/convert_tag.go
- server/internal/adapter/gql/resolver_scene.go
- server/internal/adapter/gql/resolver_dataset.go
- server/gql/cluster.graphql
- server/internal/adapter/gql/loader.go
- server/internal/infrastructure/mongo/mongodoc/scene.go
- server/internal/adapter/gql/gqldataloader/tagloader_gen.go
- server/pkg/scene/builder.go
- server/internal/adapter/gql/resolver_dataset_schema.go
- server/internal/adapter/gql/resolver_property.go
- server/gql/tag.graphql
- server/internal/adapter/gql/resolver_mutation_dataset.go
- server/internal/adapter/gql/gqldataloader/taggrouploader_gen.go
- server/gql/layer.graphql
- server/internal/adapter/gql/gqldataloader/datasetschemaloader_gen.go
- server/internal/adapter/gql/resolver_tag.go
- server/internal/adapter/gql/gqldataloader/datasetloader_gen.go
- server/internal/adapter/gql/loader_tag.go
- server/internal/adapter/gql/gqldataloader/tagitemloader_gen.go
- server/internal/adapter/gql/loader_dataset.go
- server/internal/adapter/gql/resolver_mutation_layer.go
🚧 Files skipped from review as they are similar to previous changes (41)
- server/Makefile
- server/go.mod
- server/pkg/scene/builder/story.go
- server/pkg/asset/builder.go
- server/internal/infrastructure/mongo/mongodoc/storytelling.go
- server/internal/usecase/interactor/style_test.go
- server/pkg/layer/encoding/exporter.go
- server/internal/usecase/interactor/scene_test.go
- server/internal/usecase/interactor/layer_test.go
- server/internal/usecase/repo/asset.go
- server/gql/plugin.graphql
- server/internal/adapter/gql/gqlmodel/convert_asset.go
- server/internal/adapter/gql/gqlmodel/convert_nlslayer_test.go
- server/internal/app/app.go
- server/e2e/gql_user_test.go
- server/internal/app/auth_client.go
- server/internal/infrastructure/memory/asset.go
- server/pkg/nlslayer/feature_test.go
- server/internal/infrastructure/fs/file_test.go
- server/pkg/asset/id.go
- server/gql/scene.graphql
- server/internal/adapter/gql/gqlmodel/convert_project.go
- server/internal/usecase/interactor/property_test.go
- server/internal/adapter/gql/resolver_team.go
- server/internal/usecase/interactor/scene_plugin_test.go
- server/internal/usecase/interactor/storytelling_test.go
- server/internal/infrastructure/mongo/mongodoc/asset.go
- server/gql/asset.graphql
- server/internal/infrastructure/fs/file.go
- server/e2e/gql_custom_property_test.go
- server/internal/adapter/gql/loader_asset.go
- server/pkg/asset/asset.go
- server/internal/usecase/interactor/plugin_test.go
- server/e2e/gql_project_test.go
- server/pkg/scene/builder/scene.go
- server/internal/usecase/interfaces/project.go
- server/internal/usecase/interfaces/asset.go
- server/gql/workspace.graphql
- server/internal/adapter/gql/resolver_mutation_asset.go
- server/internal/usecase/interactor/nlslayer.go
- server/internal/usecase/interactor/nlslayer_test.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 (29)
server/internal/usecase/interactor/plugin_upload_test.go (3)
98-98: LGTM! Scene creation simplified.The removal of the
RootLayermethod aligns with the broader refactoring to simplify scene management.
172-176: LGTM! Scene creation and layer management simplified.The changes maintain test coverage while:
- Removing unnecessary root layer handling
- Simplifying layer saving with
SaveAllAlso applies to: 181-181
296-300: LGTM! Consistent simplification of scene and layer management.The changes maintain test coverage while:
- Removing unnecessary root layer handling
- Simplifying layer saving with
SaveAllAlso applies to: 305-305
server/pkg/scene/builder/builder.go (1)
176-176: LGTM! Simplified scene building logic.The removal of the intermediate encoding step and direct call to
sceneJSONimproves code maintainability by reducing complexity.server/pkg/scene/builder_test.go (1)
13-13: LGTM! Test cases updated to reflect simplified scene building.The removal of
RootLayercalls from test cases aligns with the architectural changes in the scene builder. The test coverage remains comprehensive, ensuring proper validation of scene construction.Also applies to: 17-17, 23-23, 29-29, 37-37, 43-43, 51-51
server/internal/adapter/gql/gqlmodel/models_gen.go (4)
134-145: LGTM! The Asset struct now supports project association.The addition of the optional
ProjectIDfield enables assets to be associated with specific projects, which aligns with the PR objectives.
184-189: LGTM! The CreateAssetInput struct supports project association during asset creation.The addition of the optional
ProjectIDfield allows specifying a project when creating new assets.
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"`
1330-1338: LGTM! The UpdateAssetInput/Payload structs support modifying project associations.The addition of the optional
ProjectIDfield enables updating the project association of existing assets.server/internal/usecase/interactor/scene.go (4)
21-21: LGTM: Import aligns with storytelling feature.The addition of the storytelling package import is consistent with the PR objectives.
150-151: LGTM: Improved permission handling.The introduction of writableFilter enhances security by ensuring proper access control during property saving.
668-671: LGTM: Safe story data handling.The implementation properly handles nil checks and empty list conditions before accessing story data.
699-702: Handle URL parsing errors consistently.The URL parsing errors are currently being silently ignored with blank identifiers (
_). This could lead to silent failures in asset handling and make debugging more difficult.Also applies to: 728-732, 753-756
server/internal/usecase/interactor/layer.go (1)
1062-1062: LGTM! Simplifying layer management.The change to return
nilfor the layers list aligns with the broader refactoring to remove root layer handling across the codebase.server/internal/usecase/interactor/asset.go (4)
39-51: LGTM! The workspace-project filtering implementation looks good.The method correctly implements project-based asset filtering while maintaining workspace access control through the
WithReadableWorkspacesmiddleware.
88-96: Add validation for project-workspace relationship.The asset creation should verify that the provided project belongs to the workspace before associating them.
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/resolver_query.go (1)
15-17: LGTM! The project ID parameter is correctly handled.The Assets query resolver properly integrates project-based filtering by passing the projectId to the asset loader.
server/e2e/gql_validate_geojson_test.go (1)
186-186: LGTM! Test updated to handle project ID parameter.The test correctly adapts to the new API by passing nil for the optional projectId parameter.
server/e2e/gql_asset_test.go (2)
131-194: Add error handling for edge cases in project association tests.The test should verify error responses for invalid project IDs, non-existent assets, and permission issues.
This was previously suggested in a past review. The recommendation remains valid for comprehensive test coverage.
239-271: Add error handling for GraphQL responses in asset operations.The
updateAssetandremoveAssetfunctions should verify the GraphQL response for errors.This was previously suggested in a past review. The recommendation remains valid for robust error handling.
server/e2e/gql_scene_test.go (3)
6-6: LGTM! Using versioned import for httpexpect.The explicit version import of httpexpect/v2 ensures consistent test behavior.
77-100: Consider adding tests for project-asset association.While this helper function supports the new project creation flow, consider adding test cases that explicitly verify the new project-asset association functionality mentioned in the PR objectives.
Would you like me to help generate additional test cases for:
- Creating an asset with a project association
- Querying assets filtered by project ID
- Verifying asset behavior during project export/import
16-16:✅ Verification successful
Verify the necessity of adding .Object() calls.
The consistent pattern of adding
.Object()before.Value()calls suggests a breaking change in the httpexpect library or a move towards more explicit type handling.Also applies to: 42-42, 71-73
🌐 Web query:
What are the differences between Value() and Object().Value() in httpexpect v2?💡 Result:
The differences between
Value()andObject().Value()in httpexpect v2 stem from their usage contexts and assertion targets:
Object().Value()
- Purpose: Accesses nested values within JSON objects by key
- Usage:
obj := e.GET("/fruits/apple").Expect().JSON().Object() obj.Value("colors").Array().ConsistsOf("green", "red")- Key Features:
- Operates on objects created via
JSON().Object()- Takes string keys (
"colors","weight")- Returns chainable assertions for nested values
- Enables deep validation of structured data
Value()(Standalone)
- Purpose: Asserts properties of primitive values or root responses
- Usage:
// For primitives e.GET("/status").Expect().Body().Int32().IsEqual(200) // For root values e.GET("/version").Expect().JSON().String()- Key Features:
- Works directly on primitives (
string,number,bool)- Applied to entire response bodies via
.Body()- No key parameter required
Key Differences Summary
Feature Object().Value()Standalone Value()Target Nested object properties Root responses/primitives Chain Origin Requires JSON().Object()Starts with Body()/JSON()Key Handling Mandatory key parameter No keys used Typical Use Validating complex API payloads Simple endpoint responses [1][7][4]
Citations:
- 1: https://pkg.go.dev/github.com/gavv/httpexpect/v2
- 2: https://eclipse-ee4j.github.io/jersey.github.io/apidocs/2.45/jersey/org/glassfish/jersey/client/ClientProperties.html
- 3: https://metacpan.org/release/RGIERSIG/Expect-1.15/view/Expect.pod
- 4: https://pkg.go.dev/github.com/iris-contrib/httpexpect/v2
- 5: https://docs.spring.io/spring-framework/docs/current/javadoc-api/org/springframework/http/HttpHeaders.html
- 6: https://www.chaijs.com/api/bdd/
- 7: https://github.com/gavv/httpexpect
- 8: https://github.com/gavv/httpexpect/blob/master/_examples/iris_test.go
- 9: https://github.com/gavv/httpexpect/blob/master/value.go
- 10: https://nodejs.org/api/http2.html
Confirmed: The addition of
.Object()calls is necessary.The verification shows that in httpexpect v2, using
.Object().Value()is required when accessing nested object properties—this enforces explicit type handling. The changes in lines 16, 42, and 71-73 correctly follow this pattern to validate nested JSON objects, unlike usingValue()alone (which is meant for primitives or root-level values). No further modifications are needed.server/internal/usecase/interactor/project.go (3)
513-533: LGTM! The function signature change improves clarity.The renaming from
ExportProjecttoExportProjectDatabetter reflects the function's purpose of exporting project data rather than the project itself.
566-594: LGTM! The function signature change improves type safety.The change from
teamID stringtoworkspace idx.ID[accountdomain.Workspace]improves type safety by using a strongly typed ID.
627-649: Fix error handling inAddZipAsset.The function silently ignores errors for external URLs and has a redundant stream close in the error path.
server/e2e/gql_storytelling_test.go (1)
735-741: LGTM! Configuration initialization is now more maintainable.Moving the configuration to a separate variable improves readability and maintainability.
Overview
Add the association of the project to the asset.
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