chore(server): refactoring id property schema [VIZ-BE-574] #1455
Conversation
hexaforce
requested review from
airslice,
mkumbobeaty,
pyshx and
soneda-yuya
as code owners
WalkthroughThe pull request introduces extensive refactoring across multiple modules. A new Makefile target is added to clear caches. Many GraphQL schema definitions, resolver functions, test cases, and interactor methods have been updated—signatures changed, unused types and legacy fields removed, and error handling streamlined. In addition, files related to layer, dataset, and tag management in both in‑memory and MongoDB implementations have been removed or simplified, while asset, project, and storytelling logic now incorporate project filtering and Google Analytics parameters. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AssetInteractor
participant AssetRepo
Client->>AssetInteractor: Update(assetID, newProjectID, operator)
AssetInteractor->>AssetRepo: Retrieve asset by assetID
AssetRepo-->>AssetInteractor: Asset data
AssetInteractor->>AssetRepo: Save asset (with updated ProjectID)
AssetRepo-->>AssetInteractor: Save confirmation
AssetInteractor-->>Client: Return updated assetID and ProjectID
sequenceDiagram
participant Client
participant ProjectInteractor
participant ProjectRepo
participant FileGateway
Client->>ProjectInteractor: ExportProjectData(projectID, zipWriter, operator)
ProjectInteractor->>ProjectRepo: Fetch project details
ProjectInteractor->>FileGateway: AddZipAsset(file, zipWriter, path)
FileGateway-->>ProjectInteractor: Asset added to zip
ProjectRepo-->>ProjectInteractor: Project data retrieved
ProjectInteractor-->>Client: Return exported project data
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
✅ Deploy Preview for reearth-web ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (21)
server/go.mod (1)
131-131: Dependency Refactoring: Use ofgithub.com/oklog/ulidas an Indirect DependencyThe dependency
github.com/oklog/ulid v1.3.1has been moved to the indirect block. Please ensure that:
- It is not imported directly in your codebase.
- This change does not break any dependency resolution or tooling that expects it as a direct dependency.
server/e2e/gql_user_test.go (1)
28-28: Unused parameter in baseSeederUser functionThe function signature has been updated to accept a new
gateway.Fileparameter, but this parameter is not used within the function implementation. This suggests that either:
- The parameter is intended for future functionality, or
- There's a missing implementation that should use this parameter
If the parameter is for future use, consider adding a comment explaining its purpose. If implementation is missing, ensure the parameter is properly utilized.
server/internal/adapter/gql/resolver_mutation_project.go (4)
10-10: Check new imports.Imports for
"net/url"and"path"are added. Ensure they are actually required—any leftover or unused imports can be removed if not in use.Also applies to: 12-12
130-133: Filename creation and case sensitivity.Here you create a ZIP file named with a lowercased project ID. If uppercase or out-of-band ID formatting is significant, consider removing the
strings.ToLowercall. Otherwise, this is fine.
178-182: Consider removing commented-out code.Lines 178-182 with commented code (
pid2, etc.) appear to be remnants. If they are no longer needed, removing them helps keep the codebase clean.
212-218: New import flow entrypoint.The
ImportProjectmethod now looks for a team workspace and uncompresses the exported ZIP. Consider explicit error handling foraccountdomain.WorkspaceIDFrominstead of ignoring with_.server/e2e/gql_asset_test.go (2)
15-120: Tests for coreSupport asset handling.These tests thoroughly validate:
- Creating assets with and without
coreSupport.- Direct DB insertion scenarios (lines 79–104).
- Retrieving only those assets marked
coreSupport.One nitpick: consider testing boundary cases (e.g., extremely large files) if relevant. Otherwise, this looks solid.
200-223: createAsset helper function.This function neatly wraps the multipart file execution. Watch out for potential large-file handling or timeouts, but as-is, it’s a helpful utility.
server/e2e/gql_storytelling_test.go (3)
15-55: Consider splitting this test into smaller, more focused ones.
This test function is large, covering multiple scenarios (fetching, creating, updating, deleting). Splitting it into smaller, dedicated tests makes it easier to maintain, read, and diagnose potential failures.
154-206: Validate negative scenarios for block operations.
Although the test exercises block creation, movement, and removal, consider adding a scenario for invalid block IDs or invalid positions to ensure complete robustness.
231-413: Ensure clarity and maintainability by extracting repeated checks.
This test includes extensive JSON validation and repeated steps for building expectations. Extracting common logic (e.g., building or verifying the JSON payload) into helper methods may improve readability and reduce duplication.server/internal/usecase/interactor/project.go (2)
497-516: ExportProjectData handles the project image via AddZipAsset.
When the image is an external URL,AddZipAssetsilently skips the asset if unreadable. Consider logging or surfacing that condition to give visibility if an asset isn't found.
611-633: Skipping external URLs in AddZipAsset.
Returningnilon reading errors helps skip invalid or external assets. If skipping assets might impact the final exported zip, consider logging the skip event or providing user feedback.server/internal/usecase/interactor/storytelling.go (1)
370-370: Hardcoded coreSupport as 'true' during publish.
The publish builder call setscoreSupporttotrueunconditionally. If the intention is to mirror the project’s or story’s coreSupport setting, consider passing that from the domain model for consistency.server/e2e/gql_project_export_import_test.go (4)
21-32: Test steps are clearly outlined, but watch out for the function spelling.
The sequence (retrieve data → export → import) is straightforward. However, note that the function call in lines 28-29 isexporProjectinstead of “exportProject,” which appears to be a typographical slip.Consider renaming
exporProjecttoexportProjectthroughout the file for clarity:- fileName := exporProject(t, e, pID.String()) + fileName := exportProject(t, e, pID.String())
45-45: Consider using httpexpect string extraction for clarity.
Currently, raw casting is used with(string). You can useValue("id").String().Raw()for a more typed approach.
50-57: Repeated calls to compareValue could be centralized.
All keys (styles,widgets, etc.) are tested individually usingcompareValue. This is fine, but you might consider a single loop mechanism if you add more fields.
113-120:isIDchecks only widget IDs.
This works if you only intend to ignore widget IDs. If other ID types can appear in the test data, consider making it generic or clarifying the function name.server/e2e/seeder.go (2)
120-162:addAssetproperly uploads the file and creates an asset object, but consider edge cases.
Overall, it’s robust, but be mindful of missing or invalid files in real scenarios. The function returns the uploaded URL and also saves the asset.
437-497: Consider removing debug print inaddGeoJson.
Line 453 printserrbefore returning it. Logging the error is fine, butfmt.Printmay be too minimal. A log statement or direct return is usually enough.- fmt.Print(err) return err + // log.Printf("geometry creation error: %v\n", err) return errserver/e2e/common.go (1)
63-63: Storing file reference globally (fr).
This design choice is working for tests but be cautious of concurrency if running parallel tests.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
server/e2e/test.zipis excluded by!**/*.zipserver/go.sumis excluded by!**/*.sumserver/pkg/layer/decoding/shapetest/shapes.zipis excluded by!**/*.zip
📒 Files selected for processing (107)
server/Makefile(3 hunks)server/e2e/common.go(7 hunks)server/e2e/gql_asset_test.go(6 hunks)server/e2e/gql_me_test.go(1 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(6 hunks)server/e2e/gql_scene_test.go(4 hunks)server/e2e/gql_storytelling_test.go(6 hunks)server/e2e/gql_user_test.go(2 hunks)server/e2e/gql_validate_geojson_test.go(1 hunks)server/e2e/mock_test.go(1 hunks)server/e2e/ping_test.go(1 hunks)server/e2e/seeder.go(5 hunks)server/go.mod(1 hunks)server/gql/asset.graphql(5 hunks)server/gql/layer.graphql(0 hunks)server/gql/property.graphql(0 hunks)server/gql/storytelling.graphql(3 hunks)server/gql/workspace.graphql(2 hunks)server/gqlgen.yml(0 hunks)server/internal/adapter/gql/gqldataloader/dataloader.go(0 hunks)server/internal/adapter/gql/gqldataloader/layergrouploader_gen.go(0 hunks)server/internal/adapter/gql/gqldataloader/layeritemloader_gen.go(0 hunks)server/internal/adapter/gql/gqldataloader/layerloader_gen.go(0 hunks)server/internal/adapter/gql/gqlmodel/convert_asset.go(1 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_project.go(2 hunks)server/internal/adapter/gql/gqlmodel/convert_property.go(3 hunks)server/internal/adapter/gql/gqlmodel/convert_storytelling.go(1 hunks)server/internal/adapter/gql/gqlmodel/models.go(0 hunks)server/internal/adapter/gql/gqlmodel/models_gen.go(5 hunks)server/internal/adapter/gql/gqlmodel/scalar_id.go(0 hunks)server/internal/adapter/gql/loader.go(0 hunks)server/internal/adapter/gql/loader_asset.go(2 hunks)server/internal/adapter/gql/loader_layer.go(0 hunks)server/internal/adapter/gql/loader_property.go(1 hunks)server/internal/adapter/gql/resolver_Storytelling.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_project.go(6 hunks)server/internal/adapter/gql/resolver_mutation_property.go(0 hunks)server/internal/adapter/gql/resolver_mutation_storytelling.go(1 hunks)server/internal/adapter/gql/resolver_property.go(0 hunks)server/internal/adapter/gql/resolver_query.go(1 hunks)server/internal/adapter/gql/resolver_team.go(1 hunks)server/internal/adapter/http/dataset.go(0 hunks)server/internal/app/app.go(2 hunks)server/internal/app/auth_client.go(1 hunks)server/internal/app/private.go(0 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/memory/container.go(0 hunks)server/internal/infrastructure/memory/dataset.go(0 hunks)server/internal/infrastructure/memory/dataset_schema.go(0 hunks)server/internal/infrastructure/memory/layer.go(0 hunks)server/internal/infrastructure/memory/layer_test.go(0 hunks)server/internal/infrastructure/memory/property.go(1 hunks)server/internal/infrastructure/memory/tag.go(0 hunks)server/internal/infrastructure/memory/tag_test.go(0 hunks)server/internal/infrastructure/mongo/asset.go(2 hunks)server/internal/infrastructure/mongo/container.go(0 hunks)server/internal/infrastructure/mongo/dataset.go(0 hunks)server/internal/infrastructure/mongo/dataset_schema.go(0 hunks)server/internal/infrastructure/mongo/layer.go(0 hunks)server/internal/infrastructure/mongo/layer_test.go(0 hunks)server/internal/infrastructure/mongo/mongodoc/asset.go(3 hunks)server/internal/infrastructure/mongo/mongodoc/dataset.go(0 hunks)server/internal/infrastructure/mongo/mongodoc/dataset_schema.go(0 hunks)server/internal/infrastructure/mongo/mongodoc/layer.go(0 hunks)server/internal/infrastructure/mongo/mongodoc/property.go(0 hunks)server/internal/infrastructure/mongo/mongodoc/property_schema.go(1 hunks)server/internal/infrastructure/mongo/mongodoc/scene.go(0 hunks)server/internal/infrastructure/mongo/mongodoc/storytelling.go(4 hunks)server/internal/infrastructure/mongo/mongodoc/tag.go(0 hunks)server/internal/infrastructure/mongo/mongodoc/tag_test.go(0 hunks)server/internal/infrastructure/mongo/property.go(0 hunks)server/internal/infrastructure/mongo/tag.go(0 hunks)server/internal/usecase/gateway/container.go(0 hunks)server/internal/usecase/gateway/datasouce.go(0 hunks)server/internal/usecase/interactor/asset.go(6 hunks)server/internal/usecase/interactor/common.go(1 hunks)server/internal/usecase/interactor/dataset.go(0 hunks)server/internal/usecase/interactor/layer.go(0 hunks)server/internal/usecase/interactor/layer_test.go(0 hunks)server/internal/usecase/interactor/nlslayer.go(13 hunks)server/internal/usecase/interactor/plugin.go(1 hunks)server/internal/usecase/interactor/plugin_upload.go(1 hunks)server/internal/usecase/interactor/plugin_upload_test.go(1 hunks)server/internal/usecase/interactor/project.go(7 hunks)server/internal/usecase/interactor/project_test.go(0 hunks)server/internal/usecase/interactor/property.go(0 hunks)server/internal/usecase/interactor/scene.go(6 hunks)server/internal/usecase/interactor/scene_plugin.go(3 hunks)server/internal/usecase/interactor/scene_plugin_test.go(2 hunks)server/internal/usecase/interactor/storytelling.go(5 hunks)server/internal/usecase/interactor/storytelling_test.go(1 hunks)server/internal/usecase/interactor/tag.go(0 hunks)server/internal/usecase/interfaces/asset.go(2 hunks)server/internal/usecase/interfaces/common.go(0 hunks)server/internal/usecase/interfaces/dataset.go(0 hunks)server/internal/usecase/interfaces/layer.go(0 hunks)server/internal/usecase/interfaces/project.go(2 hunks)server/internal/usecase/interfaces/property.go(0 hunks)server/internal/usecase/interfaces/scene.go(0 hunks)
⛔ Files not processed due to max files limit (54)
- server/internal/usecase/interfaces/story.go
- server/internal/usecase/interfaces/tag.go
- server/internal/usecase/repo/asset.go
- server/internal/usecase/repo/container.go
- server/internal/usecase/repo/dataset.go
- server/internal/usecase/repo/dataset_schema.go
- server/internal/usecase/repo/layer.go
- server/internal/usecase/repo/property.go
- server/internal/usecase/repo/tag.go
- server/pkg/asset/asset.go
- server/pkg/asset/builder.go
- server/pkg/asset/id.go
- server/pkg/builtin/main.go
- server/pkg/builtin/main_test.go
- server/pkg/dataset/builder.go
- server/pkg/dataset/csvparser.go
- server/pkg/dataset/csvparser_test.go
- server/pkg/dataset/dataset.go
- server/pkg/dataset/dataset_test.go
- server/pkg/dataset/diff.go
- server/pkg/dataset/export.go
- server/pkg/dataset/export_test.go
- server/pkg/dataset/field.go
- server/pkg/dataset/graph_iterator.go
- server/pkg/dataset/graph_iterator_test.go
- server/pkg/dataset/graph_loader.go
- server/pkg/dataset/id.go
- server/pkg/dataset/list.go
- server/pkg/dataset/list_test.go
- server/pkg/dataset/loader.go
- server/pkg/dataset/schema.go
- server/pkg/dataset/schema_builder.go
- server/pkg/dataset/schema_field.go
- server/pkg/dataset/schema_field_builder.go
- server/pkg/dataset/schema_field_diff.go
- server/pkg/dataset/schema_graph_iterator.go
- server/pkg/dataset/schema_graph_iterator_test.go
- server/pkg/dataset/schema_list.go
- server/pkg/dataset/schema_list_test.go
- server/pkg/dataset/schema_test.go
- server/pkg/dataset/value.go
- server/pkg/dataset/value_optional.go
- server/pkg/dataset/value_optional_test.go
- server/pkg/dataset/value_test.go
- server/pkg/id/id.go
- server/pkg/layer/builder.go
- server/pkg/layer/decoding/common.go
- server/pkg/layer/decoding/common_test.go
- server/pkg/layer/decoding/czml.go
- server/pkg/layer/decoding/czml_test.go
- server/pkg/layer/decoding/decoder.go
- server/pkg/layer/decoding/format.go
- server/pkg/layer/decoding/geojson.go
- server/pkg/layer/decoding/geojson_test.go
💤 Files with no reviewable changes (53)
- server/gql/property.graphql
- server/internal/usecase/gateway/container.go
- server/internal/usecase/interfaces/common.go
- server/internal/infrastructure/mongo/property.go
- server/internal/usecase/interactor/layer_test.go
- server/internal/adapter/gql/resolver_mutation_property.go
- server/e2e/gql_project_export_test.go
- server/internal/usecase/interactor/project_test.go
- server/internal/app/private.go
- server/internal/infrastructure/mongo/mongodoc/property.go
- server/internal/infrastructure/mongo/layer_test.go
- server/internal/adapter/gql/gqlmodel/scalar_id.go
- server/internal/adapter/http/dataset.go
- server/internal/infrastructure/memory/container.go
- server/internal/usecase/interfaces/property.go
- server/gqlgen.yml
- server/internal/usecase/gateway/datasouce.go
- server/internal/infrastructure/mongo/container.go
- server/internal/usecase/interfaces/scene.go
- server/internal/adapter/gql/resolver_property.go
- server/internal/infrastructure/mongo/mongodoc/scene.go
- server/internal/adapter/gql/gqldataloader/dataloader.go
- server/internal/infrastructure/mongo/mongodoc/tag_test.go
- server/internal/usecase/interactor/property.go
- server/internal/adapter/gql/loader.go
- server/internal/infrastructure/mongo/dataset.go
- server/internal/infrastructure/mongo/mongodoc/dataset_schema.go
- server/internal/usecase/interfaces/layer.go
- server/internal/adapter/gql/gqlmodel/models.go
- server/internal/infrastructure/mongo/mongodoc/layer.go
- server/internal/infrastructure/mongo/mongodoc/tag.go
- server/gql/layer.graphql
- server/internal/infrastructure/memory/tag_test.go
- server/internal/infrastructure/memory/layer_test.go
- server/internal/infrastructure/memory/dataset_schema.go
- server/internal/usecase/interfaces/dataset.go
- server/internal/adapter/gql/gqlmodel/convert_layer.go
- server/internal/infrastructure/memory/dataset.go
- server/internal/adapter/gql/resolver_Storytelling.go
- server/internal/usecase/interactor/dataset.go
- server/internal/adapter/gql/gqldataloader/layerloader_gen.go
- server/internal/adapter/gql/loader_layer.go
- server/internal/infrastructure/mongo/dataset_schema.go
- server/internal/adapter/gql/gqldataloader/layergrouploader_gen.go
- server/internal/adapter/gql/gqldataloader/layeritemloader_gen.go
- server/internal/infrastructure/mongo/tag.go
- server/internal/usecase/interactor/tag.go
- server/internal/usecase/interactor/layer.go
- server/internal/infrastructure/memory/layer.go
- server/internal/adapter/gql/resolver_layer.go
- server/internal/infrastructure/mongo/mongodoc/dataset.go
- server/internal/infrastructure/mongo/layer.go
- server/internal/infrastructure/memory/tag.go
✅ Files skipped from review due to trivial changes (2)
- server/internal/usecase/interactor/scene_plugin_test.go
- server/internal/usecase/interactor/plugin_upload_test.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 (185)
server/internal/usecase/interactor/common.go (1)
143-147: Good simplification of the SceneDeleter struct.The SceneDeleter struct has been simplified by removing the Layer, Dataset, and DatasetSchema fields, which aligns with the broader refactoring effort to streamline the id property schema. This change reduces the struct's responsibilities to focus only on Scene, SceneLock, and Property management.
The Delete method implementation (lines 149-181) correctly reflects this simplified structure by only handling property deletion, scene lock release, and scene removal. This suggests that layer, dataset, and schema deletion responsibilities have been moved elsewhere or are no longer needed in the system architecture.
server/internal/usecase/interactor/nlslayer.go (13)
38-54: Well-structured error handling improvements.The addition of specific error constants enhances error handling clarity and consistency throughout the codebase. This makes debugging easier and provides more meaningful error messages to clients.
388-388: Improved error specificity.Replacing generic errors with the more specific
ErrInfoboxAlreadyExistsimproves error handling and makes the code more maintainable.
446-446: Improved error specificity.Using
ErrInfoboxNotFoundinstead of a more generic error enhances clarity for error handling.
478-478: Improved error specificity.Using
ErrPluginNotFoundfor this specific error case improves clarity and consistency.
489-489: Improved error specificity.Using
ErrExtensionNotFoundenhances error handling clarity.
523-523: Improved error specificity.Using
ErrInfoboxNotFoundprovides better context for the error condition.
531-531: Improved error specificity.Using
ErrExtensionTypeMustBeBlockmakes the requirement more explicit.
601-601: Improved error specificity.Using
ErrInfoboxNotFoundprovides better context for the error condition.
648-648: Improved error specificity.Using
ErrInfoboxNotFoundprovides better context for the error condition.
774-774: Improved error specificity.Using
ErrSketchNotFoundprovides better error context.
840-840: Improved error specificity.Using
ErrSketchNotFoundprovides better error context.
905-907: Improved feature creation with proper ID generation.Using
nlslayer.NewFeatureID()ensures that each feature gets a proper unique identifier.
1155-1180: Streamlined feature collection handling in import process.The refactored code improves how features are processed during import, with better handling of feature creation, properties, and geometry.
server/internal/usecase/interactor/scene_plugin.go (4)
45-46: Standardized error handling for plugin not found scenarios.The error handling has been updated to consistently use
ErrPluginNotFoundinstead ofinterfaces.ErrPluginNotFoundthroughout theInstallPluginmethod. This change aligns with best practices for error standardization and simplifies error handling.Also applies to: 50-51, 53-54
110-111: Consistent error handling update in UninstallPlugin method.The error reference has been changed from
interfaces.ErrPluginNotFoundtoErrPluginNotFound, maintaining consistency with the changes made in theInstallPluginmethod.
192-193: Standardized error reference in UpgradePlugin method.The error handling has been updated to use
ErrPluginNotFoundinstead ofinterfaces.ErrPluginNotFound, completing the consistent error standardization across all plugin management methods.
3-15:✅ Verification successful
Verify import dependencies after layer-related code removal.
According to the AI summary, references to the
layeropspackage have been removed from this file. While I can see the error handling changes, I don't see the import statement changes in the provided code. Please verify that all necessary imports are present and any unused imports have been removed.
🏁 Script executed:
#!/bin/bash # Check if there are any unused imports in the file echo "Checking for unused imports in scene_plugin.go..." rg -n "^import" server/internal/usecase/interactor/scene_plugin.go -A 15Length of output: 868
Imports verified – no unused or extraneous imports remain.
After reviewing the file
server/internal/usecase/interactor/scene_plugin.go, the updated import block confirms that thelayeropspackage has been removed as expected. The remaining imports all appear necessary for the current implementation, and no unused imports were detected.server/internal/infrastructure/memory/property.go (2)
1-205:✅ Verification successful
Missing implementation for dataset-related functionality.
The
FindByDatasetmethod has been completely removed from this file. According to the AI summary, this method was responsible for retrieving properties linked to a specific dataset schema and dataset ID.Check if all code that previously called this method has been updated accordingly to prevent potential runtime errors. Also, verify if this removal is part of a broader refactoring related to datasets.
🏁 Script executed:
#!/bin/bash # Search for any references to FindByDataset in the codebase rg "FindByDataset" --type go # Check for any interface definitions that might require this method rg -A 10 -B 10 "type Property interface" --type goLength of output: 4572
Dataset-related method removal verified as intentional
After verifying the codebase, there are no references to a
FindByDatasetmethod in either the memory implementation or in the property interface definitions. This indicates that its removal is part of a broader refactoring and that no runtime errors should occur due to its absence. Please ensure that any required dataset functionality is addressed elsewhere in the code if needed.
- Verified absence of
FindByDatasetreferences in the repository.- Confirmed that the property interfaces do not require this method.
78-78:❓ Verification inconclusive
Behavior change in FindLinkedAll method.
The condition has been simplified to only check if the property belongs to the specified scene, removing the additional check for linked fields (
p.HasLinkedField()). This change will result in more properties being returned than before, as it now includes properties without linked fields.Ensure that all consumers of this method can handle the additional properties that will now be returned. This could affect performance or business logic in code that expects only properties with linked fields.
🏁 Script executed:
#!/bin/bash # Search for FindLinkedAll usage in the codebase to verify all consumers are updated rg -A 5 "FindLinkedAll" --type goLength of output: 1655
Action: Verify Consumers Compatibility for Updated FindLinkedAll Logic
The memory implementation now returns properties based solely on the scene equality without checking for linked fields—unlike the Mongo version which still applies an additional filter. Please verify that all consumers (e.g., those in
server/internal/usecase/repo/property.go) are prepared to handle the increased number of properties (including those without linked fields) and that this behavior change does not impact performance or business logic adversely.
- Review the usage of
FindLinkedAllin the repository layer (e.g., inserver/internal/usecase/repo/property.go) to ensure that returning properties without linked fields is acceptable.- Confirm that any downstream logic or tests expecting properties with linked fields are updated accordingly.
server/internal/usecase/interactor/scene.go (6)
21-21: Import addition looks good.
No issues detected; importingstorytellingappears necessary for handling story data in subsequent logic.
530-533: Consider verifying handling of multiple stories.
Currently, only the first story is retrieved from the list. If multiple stories are expected, confirm that ignoring the rest is intended behavior.
586-587: Button icon URL check is appropriate.
This validation properly confirms that the field is a URL type before proceeding. No issues detected.
611-613: Widget page block resource handling is appropriate.
This mirrors the logic for adding ZIP assets for widgets; ensuring the type is*url.URLbefore usage is sensible.
743-743: Clear plugin retrieval error.
ReturningErrPluginNotFoundinstead of the generic error clarifies the error source.
749-749: Distinct error for missing extensions.
UsingErrExtensionNotFoundimproves error granularity and helps distinguish plugin from extension issues.server/internal/adapter/gql/gqlmodel/convert_asset.go (2)
12-16: Good addition of project ID association to assetsThis change correctly extracts the project ID from the asset when available and stores it in a variable, properly handling the case where the project might be nil. The implementation follows good defensive programming practices.
22-22: LGTM: ProjectID field added to Asset structThe ProjectID field is now properly included in the returned Asset struct, which allows clients to identify which project an asset belongs to.
server/internal/app/auth_client.go (1)
105-107: Suppress debug logs for e2e test userThis change reduces log noise during automated testing by suppressing debug logs when the e2e test user is detected. This is a good practice for keeping test logs clean while maintaining detailed logging for real users.
Also applies to: 115-117
server/Makefile (3)
23-23: Good documentation for the new cache clearing targetThe help text clearly explains the purpose of the new
run-clear-cachetarget.
93-95: Useful addition of cache clearing functionalityThis target provides a convenient way to clear all Go caches, which is helpful during development when troubleshooting dependency issues or ensuring clean builds.
make run-clear-cache # To clear all Go caches
116-116: Correctly updated .PHONY declarationThe .PHONY declaration has been properly updated to include the new target, ensuring that Make doesn't look for a file named "run-clear-cache".
server/internal/infrastructure/mongo/mongodoc/property_schema.go (1)
297-297:✅ Verification successful
Updated ID type package reference
The code now uses
id.PropertySchemaGroupIDinstead ofproperty.SchemaGroupID, which appears to be part of the ID property schema refactoring effort. This change helps centralize ID types management.
🏁 Script executed:
#!/bin/bash # Check if id.PropertySchemaGroupID is used consistently across the codebase rg -A 1 "PropertySchemaGroupID" --type goLength of output: 30049
ID Type Update Consistently Applied
The change replacingproperty.SchemaGroupIDwithid.PropertySchemaGroupIDhas been verified across the codebase—including inserver/internal/infrastructure/mongo/mongodoc/property_schema.goat line 297—and is consistently applied as part of the centralized ID types refactoring effort.server/internal/adapter/gql/resolver_mutation_storytelling.go (1)
58-59: LGTM - Properly added Google Analytics fields.The addition of
EnableGaandTrackingIDfields to theUpdateStoryInputstruct allows integration of Google Analytics tracking features in stories. Fields are properly positioned within the existing input parameter construction.server/e2e/gql_validate_geojson_test.go (1)
186-186: LGTM - Updated function call with new projectId parameter.The
createAssetFromFileDatafunction call has been updated to include anilproject ID parameter, matching the new function signature. This is consistent with the refactoring of the ID property schema mentioned in the PR objectives.server/internal/infrastructure/mongo/mongodoc/asset.go (5)
17-17: LGTM - Added Project field to AssetDocument.Added a new optional
Projectfield as a pointer to string to allow assets to be associated with projects. This aligns with the ID property schema refactoring.
35-40: LGTM - Proper implementation of Project field handling.The implementation correctly extracts the project ID from the asset and converts it to a string pointer for storage in the MongoDB document. Using a pointer allows for nil values when no project is associated.
46-46: LGTM - Added Project field to document construction.Project field is properly included in the AssetDocument constructor, maintaining consistency with the struct definition.
65-72: LGTM - Properly implemented Project ID conversion with error handling.The implementation correctly converts the string project ID back to a typed ID with appropriate error handling. The error is properly bubbled up if the conversion fails.
78-78: LGTM - Added Project field to asset build chain.Project field is properly included in the asset build method chain.
server/e2e/mock_test.go (1)
11-11: LGTM - Simplified server initialization.Replaced detailed server configuration with a call to
ServerMockTest(t), which simplifies the test setup by abstracting server initialization details. This makes the test code more maintainable and readable.server/internal/adapter/gql/resolver_mutation_asset.go (2)
19-26: Project ID handling implementation looks good.This implementation correctly processes the optional ProjectID parameter from the input, converting it to the appropriate type for use in the Asset.Create call.
41-62: New UpdateAsset resolver implementation is clean and consistent.The implementation follows established patterns for handling IDs and optional parameters. The error handling is appropriate, and the method signature aligns with the existing code style.
server/internal/adapter/gql/gqlmodel/convert_nlslayer_test.go (2)
11-12: Good refactoring to separate ID generation from feature creation.The change from using
nlslayer.NewFeatureWithNewIdto explicitly providingnlslayer.NewFeatureID()as an argument tonlslayer.NewFeaturemakes the code more explicit and follows better separation of concerns.
21-22: Consistent application of the new feature creation pattern.The same pattern is applied consistently across all feature creation calls, which is good for code maintainability.
Also applies to: 31-32, 41-42, 51-52
server/internal/adapter/gql/gqlmodel/convert_storytelling.go (1)
35-36: Good addition of Google Analytics support to the Story model.The addition of
EnableGaandTrackingIDfields is implemented cleanly and follows the existing patterns of accessing data from the underlying model.server/internal/infrastructure/fs/file_test.go (3)
165-165: Consistent change to base URL in RemoveAsset test.The base URL has been updated to use the root path instead of the assets subdirectory, which is consistent with the changes in other tests.
316-316: Consistent change to base URL in GetAssetFileURL test.This change aligns with the other modifications to use root paths instead of asset-specific paths.
116-117:❓ Verification inconclusive
Verify that URL construction still works as expected with the new base path.
The test initialization has changed from using "/assets" to "/" as the base path. Ensure that this doesn't affect the expected URL construction behavior, especially considering the assertion on line 128 which still expects paths to start with "/assets/".
🏁 Script executed:
#!/bin/bash # Check how the base path is used in the File implementation rg -A 10 'func NewFile' server/internal/infrastructure/fs/file.go rg -A 5 'getAssetFileURL' server/internal/infrastructure/fs/file.goLength of output: 912
Ensure Consistency in Asset URL Construction
- The base path for file initialization has changed from
"/assets"to"/", meaning that theurlBasein the file repository is now derived from"/".- Since
getAssetFileURLsimply uses this base to construct asset URLs, the resulting URLs will now likely be prefixed with"/"rather than"/assets/".- Please verify that this change is intentional: if asset URLs should continue to include the
"/assets/"prefix (as expected by the assertion on line 128 in the test), either adjust the base path provided toNewFileor modify the URL assembly logic ingetAssetFileURLaccordingly.- If the change is deliberate, update the test assertions to match the new behavior.
server/e2e/ping_test.go (1)
9-9: Simplification of server initializationThe code has been refactored to use
ServerPingTest(t)instead of a more verboseStartServerfunction call with explicit configuration. This change improves maintainability by centralizing server setup logic for ping tests.server/e2e/gql_me_test.go (1)
8-8: Simplified server setup for testsThe server initialization has been streamlined to use
Server(t, baseSeeder)instead of the previous approach with explicit configuration. This is part of a broader refactoring to reduce boilerplate in test setup.server/internal/usecase/interactor/storytelling_test.go (1)
159-187: Updated expected JSON structure to match new implementationThe expected JSON structure has been updated to reflect changes in the storytelling implementation, including:
- Added new fields:
enableGaandtrackingId- Reorganized existing fields
- Simplified the blocks structure
- Set
layersIdsto nullThese changes align with the modifications made to the
Storystruct in related files.server/internal/app/app.go (2)
71-77: Improved authentication handling with conditional middlewareThe authentication logic has been enhanced to conditionally use a dummy middleware when authentication is disabled. This improves flexibility for testing and development scenarios.
161-165: New dummy authentication middlewareAdded a simple no-op middleware function that can be used when authentication needs to be bypassed. This is a clean implementation that allows for easier testing and configuration.
server/internal/infrastructure/fs/file.go (1)
113-113: Path validation corrected for asset removalThe path validation logic has been improved to ensure that files being removed are located within the expected asset directory.
server/e2e/gql_scene_test.go (4)
16-16: Improved object access chainThe code now correctly uses
r.Object().Value(...)instead ofr.Value(...), which makes the object access chain more explicit and consistent.
42-42: Improved object access chainThe code now correctly uses
r.Object().Value(...)instead ofr.Value(...), which makes the object access chain more explicit and consistent.
71-73: Improved object access chainThe code now correctly uses
r.Object().Value(...)andHasValue()methods, making the object access chain more explicit and consistent.
77-100: New utility function for project creation with external imageThis is a good addition that encapsulates the GraphQL request for creating a project with an external image, making the tests more maintainable and readable.
server/internal/usecase/interfaces/project.go (1)
74-75: Method renaming and type safety improvementsThe renaming of methods from
ExportProject/ImportProjecttoExportProjectData/ImportProjectDatabetter reflects their purpose and the parameter type change fromstringtoidx.ID[accountdomain.Workspace]enhances type safety.server/internal/adapter/gql/gqlmodel/convert_project.go (1)
108-137: Well-structured project export implementationThe new
ProjectExporttype and associated conversion functions provide a clean, type-safe structure for the project export/import functionality. The implementations properly handle error cases and nil values.server/internal/infrastructure/mongo/mongodoc/storytelling.go (2)
34-35: Added Google Analytics support to storytellingGood addition of Google Analytics tracking capabilities with the
EnableGaandTrackingIDfields, properly implemented throughout all relevant methods.Also applies to: 87-88, 197-198
226-231: Improved variable naming for clarityThe variable rename from
pagetoblockbetter reflects the actual content the variable represents.server/internal/infrastructure/mongo/asset.go (1)
67-80: Enhanced asset filtering capabilitiesThe method has been renamed from
FindByWorkspacetoFindByWorkspaceProjectand now includes an additionalprojectIdparameter, allowing assets to be filtered by either project ID or workspace ID. The filter construction is also improved with better type safety usingbson.Minstead ofany.server/gql/asset.graphql (6)
5-5: Good addition of the projectId field to the Asset type.This change enables association of assets with specific projects, providing better organization and filtering capabilities.
24-24: LGTM! Consistent extension of the CreateAssetInput.The projectId field aligns with the Asset type changes and enables project association during asset creation.
29-32: Well-structured UpdateAssetInput definition.The new input type follows GraphQL best practices with required ID fields and optional update fields.
49-52: Consistent UpdateAssetPayload structure.The payload structure properly returns the updated entity identifiers.
75-75: Good addition of projectId filter to the assets query.This enables filtering assets by project, improving query flexibility and performance.
84-84: Complete mutation implementation for asset updates.The updateAsset mutation integrates well with the new input and payload types.
server/internal/adapter/gql/resolver_team.go (1)
27-28: Method signature and implementation updated correctly for project filtering.The resolver correctly passes the new projectID parameter to the asset loader's FindByWorkspace method, enabling project-specific asset filtering within teams.
server/gql/storytelling.graphql (3)
23-24: Good addition of Google Analytics fields to the Story type.These fields will enable GA tracking for stories, enhancing analytics capabilities.
84-86: Well-organized Google Analytics section in UpdateStoryInput.The grouping with a clear comment improves code readability and maintainability.
73-73: Good documentation improvement.Converting the commented-out line to an active comment provides better clarity about the Publishment section.
server/internal/adapter/gql/loader_asset.go (4)
39-39: Method signature updated correctly to support project filtering.The FindByWorkspace method now includes a projectId parameter, enabling more targeted asset queries.
45-52: Good error handling for project ID conversion.The code properly converts the GraphQL ID to the internal type and handles potential errors.
54-54: Updated method call to renamed backend function.The call has been updated from FindByWorkspace to FindByWorkspaceProject to reflect the enhanced functionality.
60-66: Performance improvement in edge creation.Allocating a fixed-size slice upfront instead of using append operations is more efficient for known collection sizes.
server/gql/workspace.graphql (3)
8-14: Approve the addition of projectId parameter for improved asset filteringThe addition of the
projectIdparameter to theassetsfield enables more granular filtering of assets by project, which aligns well with the ID property schema refactoring described in the PR title. This is a backward-compatible change since the parameter is optional.
15-21: LGTM for consistent formattingThe reformatting of the
projectsfield maintains consistent argument presentation while preserving the existing functionality.
121-123: LGTM for improved readabilityThe multi-line formatting of the
removeMemberFromTeammutation input improves readability and consistency with other mutation definitions.server/internal/usecase/interactor/plugin.go (3)
37-47: LGTM for simplified constructorThe
NewPluginconstructor is now simpler with one less dependency, which reduces complexity.
197-198: Approve the consistent approach to ID handlingThe change from
property.SchemaGroupID(sg)toid.PropertySchemaGroupID(sg)standardizes ID handling across the codebase, moving to a more centralized approach. This aligns with the PR's focus on refactoring ID property schema.
26-35:❓ Verification inconclusive
Approve removal of layerRepo dependency
The removal of the
layerRepofield from thePluginstruct reduces coupling between components, which is generally a good practice for maintainability. Make sure this change doesn't break any functionality that might have been relying on this repository.
🏁 Script executed:
#!/bin/bash # Verify that layerRepo is not used in any method of the Plugin struct rg -A 5 -B 5 "i\.layerRepo" --type goLength of output: 39
Action Required: Manual Verification
The removal of the
layerRepodependency from thePluginstruct seems safe based on our automated search—no usage oflayerRepowas found in the Plugin-related methods. However, the verification script produced no output, so please ensure by conducting an additional, manual review of the repository that no indirect dependencies remain that rely onlayerRepo.
- File under review:
server/internal/usecase/interactor/plugin.go(Lines: 26-35)server/e2e/gql_project_test.go (5)
42-42: Approved update to expected project countThe increase in expected edge count from 1 to 2 reflects changes in the project data model or filtering logic, ensuring tests pass with the refactored code.
144-144: Approved update to expected edge countIncreasing the expected edge count from 4 to 5 maintains test accuracy with the updated project handling logic.
560-560: LGTM for improved test flexibilitySwitching to
ServerAndReposprovides direct access to the repository objects, enabling more comprehensive testing capabilities while maintaining the original test functionality.
585-585: Approved updates to expected countsThe updates to expected
totalCountvalues (20→21, 4→5) and edge counts reflect changes in the project filtering logic, ensuring tests remain valid with the refactored implementation.Also applies to: 617-617, 620-620
632-632: LGTM for consistent test setupConsistent application of the
ServerAndReposfunction across test cases improves maintainability.server/internal/usecase/interfaces/asset.go (4)
28-28: Approve the ProjectID field additionAdding the
ProjectIDfield toCreateAssetParamenables associating assets with specific projects during creation, providing more granular control over asset organization. The optional pointer type maintains backward compatibility.
39-39: Approve the method rename and enhanced filteringRenaming
FindByWorkspacetoFindByWorkspaceProjectclearly indicates the method's ability to filter assets by both workspace and project. The parameter type change from*stringto*id.ProjectIDimproves type safety and aligns with the ID property schema refactoring.
41-41: LGTM for added Update methodThe new
Updatemethod enhances the asset management capabilities by allowing assets to be reassigned to different projects, providing greater flexibility in organizing assets.
43-43: Approve the enhanced import functionalityRenaming
UploadAssetFiletoImportAssetFilesbetter reflects its purpose, and the addition of theProjectIDparameter enables project-specific asset imports, consistent with the overall approach to project-based asset management.server/internal/usecase/interactor/plugin_upload.go (2)
292-292: Verify no-op layer deletion logic.Replacing the layer deletion logic with an empty slice return effectively makes this function a no-op. If you intend to remove layer references or perform cleanup, this change may cause stale data references.
296-296: Verify no-op block deletion logic.Similarly, returning an empty slice here eliminates block deletion, which can lead to leftover references if not handled elsewhere. Confirm that your data remains consistent when removing plugin extensions.
server/internal/adapter/gql/gqlmodel/convert_property.go (3)
333-335: Retaining metadata references.The addition of
OriginalIDandParentIDfields in the returnedMergedPropertyhelps preserve references for merged metadata. Confirm that downstream consumers correctly handle these references.
345-347: Ensure consistent merged property references.Including
OriginalID,ParentID, andSchemaIDis beneficial for tracking property origins, parents, and schema usage. Double-check that all references to the old structure are updated accordingly.
389-389: Signature change forToPropertySchemaGroup.Switching the second parameter to
id.PropertySchemaIDmay break callers if they were passingproperty.SchemaID. Ensure all invocations are updated to match the new type.server/internal/adapter/gql/resolver_query.go (1)
15-16: AddedprojectIdparameter for asset listing.Introducing the
projectIdparameter provides more granular filtering of workspace assets. Verify all call sites have been updated and confirm no inadvertent behavior changes.server/internal/adapter/gql/resolver_mutation_project.go (19)
155-155: Refactored call toExportProjectData.Switching from a direct export to
ExportProjectDatacan be more modular. Confirm that all plugin, scene, and property references are handled inside the new export path.
183-183: Asset retrieval for export.Fetching assets via
FindByWorkspaceProjectis appropriate. Ensure you handle large sets of assets or potential pagination limits, if applicable.
189-194: Parse asset URL and record file names.Be mindful of invalid or malformed URLs. If
path.Basereturns an unexpected result, consider additional validation before mapping names.
197-199: Storing export metadata indata.Adding
project,plugins, andschemasto the export data is a clean approach. Confirm that all required fields (e.g., layering data, style references, etc.) are fully captured if needed.
223-226: Initialize operator and load project data.You’re retrieving the operator, unmarshalling project data, and calling
ImportProjectData. Ensure partial or invalid JSON (e.g., missing keys) is handled gracefully by these steps.
229-232: Transactional handling viadefer.Ending the transaction in a deferred block is correct. Verify you still have the necessary coverage for partial import failures and robust rollback logic if intermediate steps fail.
250-270: Asset loop with name remapping.• Real asset names are mapped from the old to new ones via the imported ZIP.
• The code replaces any references to the old name intempData.
• If the unmarshal or rename step fails, consider robust error handling or logs.
• Make sure the replaced substring doesn’t inadvertently match partial text (e.g., partial filenames).
278-278: Importing scene from exported data.
Scene.ImportScenerehydrates scene data from the export. Confirm that plugin or property references align with the newly generated IDs.
297-299: Plugin import step.
ImportPluginsloads and reconfigures plugin data, returning new plugin references. Make sure plugin dependencies within the scene are correctly re-linked if any ID changes occur.
303-304: Scene re-import logic.Re-importing the scene after plugin import can be beneficial, but watch for double updates if partial data is loaded. Confirm consistent state across the entire import sequence.
307-310: NLS layer import.
ImportNLSLayersmight replace or remap layer IDs. Validate that they don’t overlap with newly created plugin or property references.
312-315: Style import.Imported styles are re-linked to the new scene ID. Confirm style references to geometry or layer objects are also correctly adjusted if they rely on old IDs.
317-320: StoryTelling import.Similar to other imports, ensure that any references to layers or assets in story slides are fully updated to the new IDs.
322-322: Committing transaction.Once all scene, plugin, style, and story data is imported, finalizing the transaction is correct. If partial imports are a possibility, ensure early failures roll back progress.
333-334: Returning the combinedProjectData.Your final payload includes project, plugins, schema, scene, NLS layer, style, and story—comprehensive coverage. Looks good for client consumption.
338-347: Replacing old scene IDs in JSON.Using
bytes.Replaceto swap IDs is convenient but poses a risk of partial string collisions. For instance, ifoldSceneIDis a substring of another ID. Keep an eye on collisions or consider more structured replacement if necessary.
349-357: Unmarshal project data.You’re ignoring an error when casting to
map[string]interface{}. Consider verifying the type assertion before reference to avoid potential runtime issues.
359-373: Unmarshal assets data.Extracting asset metadata into a map is efficient. If asset references in the JSON are nested or differently typed, confirm no edge cases skip proper parsing.
375-385: Unmarshal plugins, scene, and schemas.Gathering all relevant data from the JSON in one place is neat. As with other sections, verify type assertions and handle any missing keys gracefully.
server/internal/usecase/interactor/asset.go (4)
39-44: Consider verifying project-level permissions.Currently,
Usecase().WithReadableWorkspaces(tid)only verifies workspace accessibility. Ifpidis not nil, you might want to confirm that the operator can also read from or write to the specified project before retrieving assets under that project.
91-91: Looks good for associating assets with a project.Adding the project reference here appears consistent with the broader refactor, aligning asset creation to be project-aware.
108-127: Ensure project permission checks in the Update method.While this method ensures the operator can write to the asset’s workspace, it does not confirm the operator’s rights on the target project. In some cases, project-level checks may be necessary.
154-182: Revisit permission checks for ImportAssetFiles.This method now accepts a project ID but does not verify that the caller has access to that project. If this is indirectly guaranteed by higher-level logic, no change is needed. Otherwise, consider enforcing a permission check here as well.
server/internal/adapter/gql/gqlmodel/models_gen.go (6)
120-120: Field addition: ProjectID in Asset.Adding a nullable
projectIdfield aligns with the new project-based asset flow. No issues spotted.
168-168: Field addition: ProjectID in CreateAssetInput.Enabling asset creation under specific projects is coherent with the refactoring.
1068-1069: New analytics fields in Story.The
enableGaandtrackingIdfields extend analytics capabilities for stories. Make sure to handle empty tracking IDs if necessary.
1177-1180: Added UpdateAssetInput.This new input struct properly captures the updated asset ID and its new (optional) project ID.
1182-1185: Added UpdateAssetPayload.Returning both
assetIdandprojectIdafter an update helps confirm the new association.
1304-1305: Optional GA fields in UpdateStoryInput.Using pointers for
enableGaandtrackingIdmakes sense for patch-style GraphQL updates, providing flexible partial updates.server/e2e/gql_asset_test.go (6)
122-180: Tests for project-based asset association.Great coverage for:
- Creating assets under different projects.
- Verifying retrieval by project.
- Updating (moving assets between projects or removing the project association).
- Cleaning up by removing assets.
No particular issues. Nicely done!
181-198: CreateAssetMutation GraphQL definition.Adding
projectIdaligns with the new backend logic and is clearly documented.
224-241: updateAsset helper function.Straightforward approach for updating an asset’s project ID. The GraphQL mutation appears well-structured.
242-257: removeAsset helper function.Properly encapsulates the GraphQL call for asset removal. This is coherent with the rest of the test utilities.
258-285: createAssetFromFileData helper function.Similar to
createAsset, this version reads from in-memory data. It’s well-managed with a temp file.
287-351: getAssets query helper method.Incorporates
teamIdandprojectId, offering pagination and sorting. This method nicely complements the test scenarios for verifying newly introduced features.server/e2e/gql_storytelling_test.go (2)
57-152: Good coverage of page-based operations.
This test thoroughly verifies page creation, duplication, and reordering, including negative scenarios (invalid page ID). The approach is sound and covers key functionality comprehensively.
208-229: Straightforward property updates.
This test successfully verifies property updates of various data types. Adding more boundary or edge-case checks (e.g., invalid data types or overflow values) might further strengthen test coverage.server/internal/usecase/interactor/project.go (4)
35-47: Struct field organization is consistent.
All required repositories and gateway implementations are declared clearly. Ensure that any unused dependencies are removed in the future to keep the struct streamlined.
52-66: Initialization of the Project interactor is clear.
Assigning each repository and gateway reference is straightforward. Consider verifying if all fields are truly necessary to avoid over-injection.
481-485: Validate the cleanup logic for SceneDeleter dependencies.
When deleting a project, you remove both the scene and associated locks/properties. Ensure that other references to the scene (e.g., from external modules) are handled separately or flagged for cleanup to prevent orphaned data.
551-579: Defaulting alias to empty and coreSupport to true.
The code sets a blank alias,archived=false, andcoreSupport=truefor imported projects, potentially overriding user-provided values. Make sure this reflects the intended import behavior or allow them to be overridden by the import data.server/internal/usecase/interactor/storytelling.go (3)
182-189: New GA tracking fields appear consistent with the rest of the story updates.
These additions allow toggling GA and setting the tracking ID. Confirm that dependent logic (e.g., analytics dashboards or user metrics) is fully integrated in subsequent steps or tests.
286-306: Enforce published story policy at the workspace level.
This block of code checks the workspace’s policy before switching from private to published. Confirm that the logic for counting public stories aligns with your overall policy approach (e.g., if stories and projects share the same publication limit).
1128-1140: Improved error handling with custom error variables.
Replacing generic errors withErrPluginNotFoundandErrExtensionNotFoundclarifies the intent. Ensure these errors are documented and surfaced properly to the user for debugging.server/e2e/gql_project_export_import_test.go (11)
4-8: Imports look good.
These imports (encoding/json,fmt,net/http,strings) are all used within the file, and there appear to be no redundancies or conflicts.
12-13: Additional package imports for ID handling.
Importing internal ID packages fromreearthandreearthxaligns with the new test logic.
19-19: Helpful test command comment.
This line clarifies how to run the specific test. No issues found.
34-43: Thorough checks on returned objects.
Verifying thatproject,plugins,schema,scene,nlsLayer,style, andstoryare non-null is a solid approach for ensuring the import was successful.
47-48: Lookup for imported data looks correct.
The call togetScenewith the newly imported ID is direct and clear.
59-62: File cleanup is properly deferred.
This ensures the exported zip is removed after the test completes.
66-70: Function for line conversion is concise.
convertLineaccurately marshals and splits JSON. No issues found.
72-102: Line-by-line comparison logic is robust.
Ignoring certain IDs is a practical approach for dynamic fields. The function is clear.
104-111:isIgnorefunction is straightforward.
This helper accurately checks if two lines start with the same property name.
143-173: Import logic is sound.
The multipart upload approach is valid, and the test checks forprojectData. No issues flagged.
175-189:getScenestructure is consistent with the test flow.
Retrieves data from the server for comparison. Approved.server/e2e/seeder.go (15)
5-13: Expanded imports for random, hex encoding, and file handling.
These additions support asset uploads, random UUID generation, and JSON encoding. No immediate issues noted.
15-29: New references to pkg fields for asset, property, scene, etc.
These reflect the new or expanded usage within seeder functions (full seeder, base seeder, etc.). Everything appears consistent with the domain models.
43-44: Updated project description.
Appending " desc" topDescis minor but clarifies the property.
45-45: Scene ID initialization.
Line sets a global ID for the scene. Nothing problematic here.
53-66:baseSeedernow takes agateway.File.
This parameter injection is consistent with the new asset workflow. No immediate concerns.
68-81: Add language to the user’s data inbaseSeederWithLang.
Carrying a language tag is helpful for i18n. The approach to callbaseSetupwith the file param looks correct.
83-118:baseSetupnow creates an asset for the project.
Attaching an image URL to the project is a practical demonstration. Implementation flow is logical.
164-178:fullSeederextends seeding functionality.
Builds onbaseSetup. Good separation of concerns.
219-265:addWidgetlogic.
This function cleanly builds a new widget, property, and updates the scene. No major concerns beyond ensuring the plugin extension is valid.
267-296:addStorysets up a story with property and page references.
Implementation is straightforward, with correct saving of property and story.
298-322:_createPagefor story pages.
Page creation is well-structured: property, blocks, ID. Looks good.
324-352:_createBlockcorrectly links a plugin extension property.
Checks the extension type and assigns the property. No issues noted.
354-378:addLayerStyle: minimal JSON unmarshal and style creation.
The function is straightforward, applying a scene style with a “marker” config.
380-435:addLayerSimplesets up a basic layer with config and a placeholder property schema.
Allows for geojson data to be attached later. Good separation withaddGeoJson.
499-514: UUID generation is standard.
Generates a compliant random UUIDv4. No issues found.server/e2e/common.go (16)
11-11: Reflection import usage.
reflectis used in theValueDumpfunction to identify unsupported types. This is a valid approach.
35-43: Introducing globalfranddisabledAuthConfig.
Allows skipping auth for tests and referencing a global file pointer. Straightforward for e2e convenience.
45-45: Acceptingf gateway.Filein theSeedertype.
This lines up with the new asset seeding approach in other files.
51-51: UpdatedinitReposto return afile.
Ensures that the file gateway is seeded properly for uploading assets.
65-68: Seeding logic.
If a seeder is given, we call it after setting up repos. Straightforward approach.
70-70: Returning both repos and file.
No issues with the final return signature.
74-82: Handling nilfrininitGateway.
Falls back to creating a new file gateway iffris nil. Clear fallback logic.
138-139: Mongo-based repos get seeded withbaseSeeder.
The call is consistent with your approach of passingtrueforuseMongo.
143-147:startServerfunction merges config, repo init, and server creation.
Centralizing the server start is a clean approach for e2e tests.
149-151: ExportedServerAndRepos: a convenience wrapper.
Offers the same multi-return approach for external usage.
153-156: ExportedServer: test server with default config.
EncapsulatesstartServerusage.
158-161:ServerPingTest: toggles off DB usage.
Handy for basic server availability checks without seeding.
163-174:ServerMockTest: includes mock auth.
Allows local dev testing. No issues found.
177-183:ServerLanguage: specialized seeder call.
InvokesbaseSeederWithLangto seed in a specific language.
266-271:JSONEqRegexpValue: no major concerns.
Ensures the output matches the expected JSON pattern using regex.
283-297:ValueDumpfunction for debugging.
Leverages reflection to handle different raw data types. Useful in test contexts.
| func (c *PropertyLoader) FetchMerged(ctx context.Context, org, parent, linked *gqlmodel.ID) (*gqlmodel.MergedProperty, error) { | ||
| res, err := c.usecase.FetchMerged( | ||
| ctx, | ||
| gqlmodel.ToIDRef[id.Property](org), | ||
| gqlmodel.ToIDRef[id.Property](parent), | ||
| gqlmodel.ToIDRef[id.Dataset](linked), | ||
| getOperator(ctx), | ||
| ) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return gqlmodel.ToMergedProperty(res), nil | ||
| return nil, nil | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Emptied implementation might impact dependent code.
The implementation of FetchMerged now just returns nil, nil rather than actually fetching and merging properties. This appears to be part of a larger refactoring effort to simplify property management logic across the codebase, but any code that depends on this method will now receive nil values instead of merged properties.
Consider adding a comment explaining this intentional change, such as whether the functionality has been deprecated, moved elsewhere, or is no longer needed. You can also check which code might be affected by this change:
🏁 Script executed:
#!/bin/bash
# Find references to FetchMerged in the codebase
rg "FetchMerged" --type goLength of output: 351
Action: Clarify the Intentional Stub in FetchMerged
We’ve confirmed that the FetchMerged method in server/internal/adapter/gql/loader_property.go is still being called in server/internal/adapter/gql/resolver_property.go. Returning nil, nil here means that any resolver relying on a proper merged property will instead receive nil values, which could lead to unexpected behavior. Please add a comment in the method (or a TODO) explaining whether this empty implementation is intentional—such as due to deprecation, a planned refactoring, or a temporary stub—and verify that dependent code can safely handle a nil result.
| u, _ := url.Parse(urlStr) | ||
| if err := AddZipAsset(ctx, i.file, zipWriter, u.Path); err != nil { | ||
| log.Infofc(ctx, "Fail nLayer addZipAsset :", err.Error()) |
There was a problem hiding this comment.
Avoid ignoring potential URL parse errors.
Using _ to ignore the error from url.Parse could lead to u being nil, causing unintended behavior when calling AddZipAsset. Properly handle or propagate the parse error:
- u, _ := url.Parse(urlStr)
- if err := AddZipAsset(ctx, i.file, zipWriter, u.Path); err != nil {
- log.Infofc(ctx, "Fail nLayer addZipAsset :", err.Error())
+ u, err := url.Parse(urlStr)
+ if err != nil {
+ log.Warnfc(ctx, "Invalid URL for nLayer: %s", urlStr)
+ } else {
+ if err := AddZipAsset(ctx, i.file, zipWriter, u.Path); err != nil {
+ log.Infofc(ctx, "Fail nLayer addZipAsset :", err.Error())
+ }
+ }📝 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.
| u, _ := url.Parse(urlStr) | |
| if err := AddZipAsset(ctx, i.file, zipWriter, u.Path); err != nil { | |
| log.Infofc(ctx, "Fail nLayer addZipAsset :", err.Error()) | |
| u, err := url.Parse(urlStr) | |
| if err != nil { | |
| log.Warnfc(ctx, "Invalid URL for nLayer: %s", urlStr) | |
| } else { | |
| if err := AddZipAsset(ctx, i.file, zipWriter, u.Path); err != nil { | |
| log.Infofc(ctx, "Fail nLayer addZipAsset :", err.Error()) | |
| } | |
| } |
| func (r *Asset) FindByWorkspaceProject(_ context.Context, wid accountdomain.WorkspaceID, pid *id.ProjectID, filter repo.AssetFilter) ([]*asset.Asset, *usecasex.PageInfo, error) { | ||
| if !r.f.CanRead(wid) { | ||
| return nil, usecasex.EmptyPageInfo(), nil | ||
| } | ||
|
|
||
| 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)) | ||
| } |
There was a problem hiding this comment.
Missing workspace check when filtering by project ID
When filtering assets by project ID (pid != nil), the implementation only checks if the asset's project matches the provided project ID, but it does not verify if the asset belongs to the given workspace (wid). This could potentially expose assets from other workspaces if they happen to have the same project ID.
Consider adding the workspace check to ensure proper access control:
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.Project() != nil && *v.Project() == *pid && v.CoreSupport() && (filter.Keyword == nil || strings.Contains(v.Name(), *filter.Keyword))
}📝 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 (r *Asset) FindByWorkspaceProject(_ context.Context, wid accountdomain.WorkspaceID, pid *id.ProjectID, filter repo.AssetFilter) ([]*asset.Asset, *usecasex.PageInfo, error) { | |
| if !r.f.CanRead(wid) { | |
| return nil, usecasex.EmptyPageInfo(), nil | |
| } | |
| 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)) | |
| } | |
| func (r *Asset) FindByWorkspaceProject(_ context.Context, wid accountdomain.WorkspaceID, pid *id.ProjectID, filter repo.AssetFilter) ([]*asset.Asset, *usecasex.PageInfo, error) { | |
| if !r.f.CanRead(wid) { | |
| return nil, usecasex.EmptyPageInfo(), nil | |
| } | |
| result := r.data.FindAll(func(k id.AssetID, v *asset.Asset) bool { | |
| if pid != nil { | |
| return v.Workspace() == wid && v.Project() != nil && *v.Project() == *pid && v.CoreSupport() && (filter.Keyword == nil || strings.Contains(v.Name(), *filter.Keyword)) | |
| } | |
| // ... potentially more code follows | |
| }) | |
| // ... remainder of function code | |
| } |
| // https://github.com/golang/go/issues/38351 | ||
| b := *base | ||
| b.Path = path.Join(b.Path, filename) | ||
| if b.Path == "/" { | ||
| b.Path = path.Join(b.Path, assetDir, filename) | ||
| } else { | ||
| b.Path = path.Join(b.Path, filename) | ||
| } | ||
| return &b |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent URL construction in getAssetFileURL
The function now handles two different scenarios based on the base path, but this creates an inconsistency. When the base path is root ("/"), it includes the asset directory in the constructed path, but when the base path is not root, it doesn't. This might lead to unexpected behavior where assets have different URL patterns depending on the base path.
Consider standardizing the URL construction to always include the asset directory:
func getAssetFileURL(base *url.URL, filename string) *url.URL {
if base == nil {
return nil
}
// https://github.com/golang/go/issues/38351
b := *base
- if b.Path == "/" {
- b.Path = path.Join(b.Path, assetDir, filename)
- } else {
- b.Path = path.Join(b.Path, filename)
- }
+ b.Path = path.Join(b.Path, assetDir, filename)
return &b
}Alternatively, if this is intentional, add a comment explaining the different behaviors.
📝 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.
| // https://github.com/golang/go/issues/38351 | |
| b := *base | |
| b.Path = path.Join(b.Path, filename) | |
| if b.Path == "/" { | |
| b.Path = path.Join(b.Path, assetDir, filename) | |
| } else { | |
| b.Path = path.Join(b.Path, filename) | |
| } | |
| return &b | |
| func getAssetFileURL(base *url.URL, filename string) *url.URL { | |
| if base == nil { | |
| return nil | |
| } | |
| // https://github.com/golang/go/issues/38351 | |
| b := *base | |
| b.Path = path.Join(b.Path, assetDir, filename) | |
| return &b | |
| } |
| 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
Typo in function name and potential rename.
The function is spelled exporProject. It couples well with the GraphQL call, but a rename to exportProject would improve readability.
-func exporProject(t *testing.T, e *httpexpect.Expect, p string) string {
+func exportProject(t *testing.T, e *httpexpect.Expect, p string) string {📝 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 exportProject(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 fullSetup(ctx context.Context, r *repo.Container) error { | ||
| schema := builtin.GetPropertySchemaByVisualizer(visualizer.VisualizerCesiumBeta) | ||
| prop, err := property.New().NewID().Schema(schema.ID()).Scene(sID).Build() | ||
| if err != nil { | ||
| return err | ||
| } | ||
| ps := scene.NewPlugins([]*scene.Plugin{ | ||
| scene.NewPlugin(id.OfficialPluginID, nil), | ||
| }) | ||
| s, err := scene.New().ID(sID). | ||
| Project(pID). | ||
| Workspace(wID). | ||
| Property(prop.ID()). | ||
| Plugins(ps). | ||
| Build() | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if err = r.Scene.Save(ctx, s); err != err { | ||
| return err | ||
| } | ||
| if err = r.Property.Save(ctx, prop); err != err { | ||
| return err | ||
| } | ||
| if err = addWidget(ctx, s, r); err != err { | ||
| return err | ||
| } | ||
| if err = addStory(ctx, r); err != err { | ||
| return err | ||
| } | ||
| if err = addLayerSimple(ctx, r); err != err { | ||
| return err | ||
| } | ||
| if err = addLayerStyle(s.ID(), ctx, r); err != err { | ||
| return err | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Critical bug in repeated conditions: if err = r.Scene.Save(ctx, s); err != err { ... }.
You likely meant err != nil. As written, err != err is always false and will never handle the error case. Apply the fix in all similar lines here (e.g., lines 198, 201, 204, 207, 210, 213).
- if err = r.Scene.Save(ctx, s); err != err {
+ if err = r.Scene.Save(ctx, s); err != nil {
return err
}📝 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 fullSetup(ctx context.Context, r *repo.Container) error { | |
| schema := builtin.GetPropertySchemaByVisualizer(visualizer.VisualizerCesiumBeta) | |
| prop, err := property.New().NewID().Schema(schema.ID()).Scene(sID).Build() | |
| if err != nil { | |
| return err | |
| } | |
| ps := scene.NewPlugins([]*scene.Plugin{ | |
| scene.NewPlugin(id.OfficialPluginID, nil), | |
| }) | |
| s, err := scene.New().ID(sID). | |
| Project(pID). | |
| Workspace(wID). | |
| Property(prop.ID()). | |
| Plugins(ps). | |
| Build() | |
| if err != nil { | |
| return err | |
| } | |
| if err = r.Scene.Save(ctx, s); err != err { | |
| return err | |
| } | |
| if err = r.Property.Save(ctx, prop); err != err { | |
| return err | |
| } | |
| if err = addWidget(ctx, s, r); err != err { | |
| return err | |
| } | |
| if err = addStory(ctx, r); err != err { | |
| return err | |
| } | |
| if err = addLayerSimple(ctx, r); err != err { | |
| return err | |
| } | |
| if err = addLayerStyle(s.ID(), ctx, r); err != err { | |
| return err | |
| } | |
| return nil | |
| } | |
| func fullSetup(ctx context.Context, r *repo.Container) error { | |
| schema := builtin.GetPropertySchemaByVisualizer(visualizer.VisualizerCesiumBeta) | |
| prop, err := property.New().NewID().Schema(schema.ID()).Scene(sID).Build() | |
| if err != nil { | |
| return err | |
| } | |
| ps := scene.NewPlugins([]*scene.Plugin{ | |
| scene.NewPlugin(id.OfficialPluginID, nil), | |
| }) | |
| s, err := scene.New().ID(sID). | |
| Project(pID). | |
| Workspace(wID). | |
| Property(prop.ID()). | |
| Plugins(ps). | |
| Build() | |
| if err != nil { | |
| return err | |
| } | |
| if err = r.Scene.Save(ctx, s); err != nil { | |
| return err | |
| } | |
| if err = r.Property.Save(ctx, prop); err != nil { | |
| return err | |
| } | |
| if err = addWidget(ctx, s, r); err != nil { | |
| return err | |
| } | |
| if err = addStory(ctx, r); err != nil { | |
| return err | |
| } | |
| if err = addLayerSimple(ctx, r); err != nil { | |
| return err | |
| } | |
| if err = addLayerStyle(s.ID(), ctx, r); err != nil { | |
| return err | |
| } | |
| return nil | |
| } |
hexaforce
changed the title
Overview
Refactoring ids
What I've done
This pull request unifies the Property and PropertySchema IDs to the ones in ./server/pkg/id/id.go.
I removed the parts that were redefining the ID from id.go.
type PropertyID
type PropertySchemaID
type PropertySchemaGroupID
What I haven't done
How I tested
Which point I want you to review particularly
Memo
Summary by CodeRabbit
New Features
Improvements