feat(server): Add data deletion process [VIZ-1419] #1520
Conversation
WalkthroughThis pull request introduces new removal methods across various layers of the application. In the infrastructure packages (adapter, fs, memory, mongo), methods for removing plugins, property schemas, and assets by scene and file (or project and file) have been added. The deletion processes in the usecase layer (SceneDeleter and ProjectDeleter) have been expanded to incorporate these new methods and repositories. Corresponding interface definitions in the usecase repo have also been updated, along with necessary import adjustments. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SceneDeleter
participant PluginRepo
participant PropertySchemaRepo
participant NLSLayerRepo
participant StorytellingRepo
participant StyleRepo
participant FileGateway
Client->>SceneDeleter: Request scene deletion
SceneDeleter->>PluginRepo: RemoveBySceneWithFile(ctx, scene, file)
SceneDeleter->>PropertySchemaRepo: RemoveByScene(ctx, scene)
SceneDeleter->>NLSLayerRepo: Remove(ctx, scene)
SceneDeleter->>StorytellingRepo: Remove(ctx, scene)
SceneDeleter->>StyleRepo: Remove(ctx, scene)
SceneDeleter->>FileGateway: Process file removal
SceneDeleter->>Client: Return deletion result
sequenceDiagram
participant Client
participant ProjectDeleter
participant AssetRepo
participant PluginRepo
participant PropertySchemaRepo
participant OtherDeleters
Client->>ProjectDeleter: Request project deletion
ProjectDeleter->>AssetRepo: RemoveByProjectWithFile(ctx, project, file)
ProjectDeleter->>PluginRepo: RemoveBySceneWithFile(ctx, scene, file)
ProjectDeleter->>PropertySchemaRepo: RemoveByScene(ctx, scene)
ProjectDeleter->>OtherDeleters: Delete remaining components
ProjectDeleter->>Client: Return deletion result
Possibly related PRs
Poem
✨ Finishing Touches
🪧 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: 5
🧹 Nitpick comments (5)
server/internal/infrastructure/mongo/plugin.go (1)
107-130: Consider consistent error handling approach.The new
RemoveBySceneWithFilemethod logs errors and continues execution instead of returning early, which is a different pattern from other methods in this codebase. Additionally, the method doesn't check write permissions usingr.f.CanWrite(sid)as seen in other methods likeSave.Consider adding permission checking and possibly refactoring error handling to be more consistent with the rest of the codebase:
func (r *Plugin) RemoveBySceneWithFile(ctx context.Context, sid id.SceneID, f gateway.File) error { + // Check if we have permission to write to this scene + if !r.f.CanWrite(sid) { + return repo.ErrOperationDenied + } + plugins, err := r.find(ctx, bson.M{"scene": sid.String()}) if err != nil { return err } + var errs []error for _, pl := range plugins { if p := builtin.GetPlugin(pl.ID()); p != nil { continue } if err := f.RemovePlugin(ctx, pl.ID()); err != nil { - log.Print(err.Error()) + errs = append(errs, err) + log.Printf("Error removing plugin from file: %s", err.Error()) } if err := r.Remove(ctx, pl.ID()); err != nil { - log.Print(err.Error()) + errs = append(errs, err) + log.Printf("Error removing plugin from database: %s", err.Error()) } } + if len(errs) > 0 { + return fmt.Errorf("encountered %d errors during plugin removal", len(errs)) + } return nil }This would require also importing the
fmtpackage.server/internal/infrastructure/mongo/asset.go (2)
175-178: Error handling could be improved for URL parsing.The current implementation silently continues to the next asset when URL parsing fails, without any logging or error reporting.
Consider logging parsing errors to help with debugging:
aPath, err := url.Parse(a.URL()) if err != nil { + log.Printf("Error parsing asset URL %s: %v", a.URL(), err) continue }
180-188: Non-fatal error handling approach for asset removal operations.The implementation logs errors but continues execution when asset removal operations fail. This is a reasonable approach for a cleanup operation where we want to remove as many assets as possible, but could be enhanced.
Consider collecting errors in a slice and returning a summarized error at the end:
+var errs []error err = f.RemoveAsset(ctx, aPath) if err != nil { log.Print(err.Error()) + errs = append(errs, fmt.Errorf("failed to remove asset file for %s: %w", a.ID(), err)) } err = r.Remove(ctx, a.ID()) if err != nil { log.Print(err.Error()) + errs = append(errs, fmt.Errorf("failed to remove asset record for %s: %w", a.ID(), err)) } +if len(errs) > 0 { + return fmt.Errorf("failed to remove %d assets: %v", len(errs), errs) +}server/internal/usecase/interactor/project.go (1)
484-499: Enhanced ProjectDeleter initialization with additional repositories.The ProjectDeleter now includes additional repositories in the SceneDeleter struct, along with the Asset repository. This is a good enhancement that ensures all related resources are properly cleaned up during project deletion.
This change improves the system's consistency by making sure that all associated resources are properly cleaned up when a project is deleted. The architecture follows a proper dependency injection pattern, making the code more maintainable and testable.
server/internal/usecase/interactor/common.go (1)
173-202: Comprehensive scene resource cleanup implementation.This implementation properly deletes all associated resources (NLSLayer, Plugin, PropertySchema, Storytelling, Style) when a scene is deleted. The code follows a consistent pattern for each resource type and includes appropriate error handling.
The sequential deletion approach ensures that all resources are properly cleaned up. If performance becomes an issue with large scenes, consider implementing parallel deletion operations with proper error aggregation.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
server/internal/infrastructure/adapter/plugin.go(2 hunks)server/internal/infrastructure/adapter/property_schema.go(1 hunks)server/internal/infrastructure/fs/plugin.go(2 hunks)server/internal/infrastructure/fs/property_schema.go(1 hunks)server/internal/infrastructure/memory/asset.go(2 hunks)server/internal/infrastructure/memory/plugin.go(2 hunks)server/internal/infrastructure/memory/property_schema.go(1 hunks)server/internal/infrastructure/mongo/asset.go(2 hunks)server/internal/infrastructure/mongo/plugin.go(2 hunks)server/internal/infrastructure/mongo/property_schema.go(1 hunks)server/internal/usecase/interactor/common.go(4 hunks)server/internal/usecase/interactor/project.go(2 hunks)server/internal/usecase/repo/asset.go(2 hunks)server/internal/usecase/repo/plugin.go(2 hunks)server/internal/usecase/repo/property_schema.go(1 hunks)
🧰 Additional context used
🧬 Code Definitions (6)
server/internal/infrastructure/memory/asset.go (7)
server/internal/infrastructure/mongo/asset.go (15)
r(40-42)r(44-49)r(51-55)r(57-61)r(63-75)r(77-113)r(115-143)r(145-151)r(153-157)r(159-193)r(195-210)r(212-218)r(220-229)r(250-252)Asset(31-34)server/internal/usecase/repo/asset.go (1)
Asset(19-29)server/pkg/asset/asset.go (11)
Asset(17-27)a(29-31)a(33-35)a(37-39)a(41-43)a(45-47)a(49-51)a(53-55)a(57-59)a(61-63)a(65-70)server/internal/usecase/interactor/asset.go (1)
Asset(25-28)server/internal/usecase/interactor/project.go (1)
Project(34-51)server/internal/usecase/interfaces/project.go (1)
Project(63-76)server/internal/infrastructure/memory/project.go (1)
Project(18-22)
server/internal/infrastructure/fs/property_schema.go (1)
server/internal/infrastructure/adapter/property_schema.go (9)
r(27-36)r(38-51)r(53-63)r(65-70)r(72-77)r(79-84)r(86-91)r(93-98)propertySchema(14-17)
server/internal/usecase/interactor/project.go (11)
server/internal/usecase/interactor/asset.go (1)
Asset(25-28)server/internal/usecase/repo/asset.go (1)
Asset(19-29)server/internal/usecase/repo/project.go (1)
Project(19-32)server/internal/usecase/interactor/storytelling.go (1)
Storytelling(27-43)server/internal/usecase/interactor/scene.go (1)
Scene(28-43)server/internal/usecase/repo/property_schema.go (1)
PropertySchema(10-19)server/internal/usecase/interactor/nlslayer.go (17)
NLSLayer(52-67)i(87-89)i(91-93)i(95-97)i(99-101)i(103-224)i(226-244)i(246-302)i(304-358)i(360-415)i(417-472)i(474-525)i(527-578)i(580-609)i(611-629)i(631-646)i(648-717)server/internal/usecase/interactor/style.go (1)
Style(17-25)server/internal/usecase/interactor/plugin.go (1)
Plugin(28-37)server/internal/usecase/repo/container.go (1)
Container(21-40)server/internal/usecase/interactor/common.go (1)
commonSceneLock(107-109)
server/internal/infrastructure/mongo/asset.go (5)
server/internal/adapter/gql/gqlmodel/models_gen.go (2)
Asset(117-128)Asset(130-130)server/internal/usecase/repo/asset.go (1)
Asset(19-29)server/internal/adapter/gql/loader_asset.go (1)
pid(45-45)server/internal/adapter/gql/resolver_mutation_asset.go (2)
pid(19-19)pid(47-47)server/internal/infrastructure/mongo/mongodoc/asset.go (2)
pid(36-36)pid(65-65)
server/internal/usecase/interactor/common.go (8)
server/internal/infrastructure/memory/property_schema.go (1)
PropertySchema(15-19)server/internal/infrastructure/mongo/property_schema.go (2)
PropertySchema(21-24)err(69-69)server/internal/usecase/repo/property_schema.go (1)
PropertySchema(10-19)server/internal/infrastructure/memory/plugin.go (1)
Plugin(16-20)server/internal/infrastructure/mongo/plugin.go (2)
Plugin(25-28)err(78-78)server/internal/usecase/repo/plugin.go (1)
Plugin(11-18)server/internal/usecase/repo/asset.go (1)
Asset(19-29)server/internal/infrastructure/mongo/asset.go (1)
Asset(31-34)
server/internal/infrastructure/fs/plugin.go (1)
server/internal/infrastructure/adapter/plugin.go (7)
r(28-37)r(39-52)r(54-64)r(66-71)r(73-78)r(80-85)pluginRepo(15-18)
🔇 Additional comments (24)
server/internal/infrastructure/adapter/property_schema.go (1)
93-98: Implementation follows established patterns correctly.This implementation of
RemoveBySceneis consistent with other removal methods in the file. It properly checks for a nil writer before delegating to it, maintaining error handling patterns from existing methods.server/internal/usecase/repo/property_schema.go (1)
18-18: Interface contract addition is appropriate.The addition of the
RemoveByScenemethod to thePropertySchemainterface is consistent with other methods and provides a clean way to remove property schemas associated with a specific scene.server/internal/usecase/repo/plugin.go (2)
6-6: Import addition is necessary for the new method.The addition of the gateway import supports the new method parameter type.
17-17: Interface extension to support scene-based deletion with file handling.This new method enhances the Plugin interface to support deletion of plugins associated with a specific scene while also handling file resources, which is important for proper resource cleanup.
server/internal/infrastructure/fs/property_schema.go (1)
84-86: Read-only implementation is consistent.This implementation correctly maintains the read-only nature of the file system repository by returning an appropriate error message, following the same pattern as other write operations in this file.
server/internal/infrastructure/mongo/property_schema.go (1)
127-129: RemoveByScene implementation looks goodThe
RemoveByScenemethod properly handles the removal of property schemas associated with a specific scene ID. It correctly uses thewriteFilterto enforce permissions and theRemoveAllmethod to perform the deletion. This implementation is consistent with other removal methods in the struct.server/internal/usecase/repo/asset.go (2)
6-6: Import added correctly for the new methodThe import statement for the
gatewaypackage is correctly added to support the newRemoveByBrojectWithFilemethod parameter.
28-28:❓ Verification inconclusive
Potential typo in method name "RemoveByBrojectWithFile"
There's a possible typo in the method name: "Broject" instead of "Project". However, since this seems to be consistent across multiple files in the codebase, please verify if this naming is intentional or if it should be corrected across all implementations.
🏁 Script executed:
#!/bin/bash # Search for occurrences of "Broject" in the codebase to determine if this is consistent terminology rg "Broject" --type goLength of output: 560
Action Required: Verify the naming of "RemoveByBrojectWithFile" across the codebase
After searching for "Broject" in the repository, it appears in multiple files:
server/internal/usecase/interactor/common.goserver/internal/usecase/repo/asset.goserver/internal/infrastructure/memory/asset.goserver/internal/infrastructure/mongo/asset.goThis consistent usage could indicate either an intentional naming convention or a systematic typo. Please confirm if "Broject" is meant to be "Project," and if so, update the method name across all implementations accordingly.
server/internal/infrastructure/memory/plugin.go (1)
8-8: Import added correctly for gateway.FileThe import statement for the
gatewaypackage is correctly added to support theFileparameter in the new method.server/internal/infrastructure/memory/asset.go (1)
8-8: Import added correctly for gatewayThe import statement for the
gatewaypackage is correctly added to support theFileparameter in the new method.server/internal/infrastructure/fs/plugin.go (2)
9-9: Import added correctly for the new method.The
gatewaypackage is imported to support the newRemoveBySceneWithFilemethod that uses thegateway.Filetype.
69-71: Implementation consistent with existing pattern.The new
RemoveBySceneWithFilemethod follows the same approach as the existingRemovemethod, returning a "read only" error since this is a file system-based repository intended for read-only operations.server/internal/infrastructure/adapter/plugin.go (2)
7-7: Import added correctly for the new method.The
gatewaypackage is imported to support the newRemoveBySceneWithFilemethod that uses thegateway.Filetype.
80-85: Implementation follows established pattern.The new
RemoveBySceneWithFilemethod correctly checks if thewriterisnilbefore delegating the operation, which is consistent with the pattern used in other methods likeRemoveandSave.server/internal/infrastructure/mongo/plugin.go (1)
6-6: Imports added correctly for the new method.The
logpackage is imported for error logging, and thegatewaypackage is imported to support the newRemoveBySceneWithFilemethod that uses thegateway.Filetype.Also applies to: 11-11
server/internal/infrastructure/mongo/asset.go (4)
7-8: New imports added for logging and URL handling.These imports are necessary for the new
RemoveByBrojectWithFilemethod implementation.
14-14: New gateway package import for file operations.This import is required to access the
gateway.Fileinterface that will be used in the new method.
161-167: The query logic correctly filters for core-supported assets by project ID.The implementation properly queries for assets with the
coresupportflag set to true that belong to the specified project.
169-173: Permission check for workspace write access is appropriate.The code correctly validates that the user has write permissions for each asset's workspace before proceeding with deletion.
server/internal/usecase/interactor/project.go (2)
34-51: New propertySchemaRepo field added to Project struct.The
propertySchemaRepofield has been added to the Project struct to handle property schema operations. This is a good addition as it maintains consistency with other repositories and allows for proper cleanup during project deletion.
53-71: PropertySchema repository initialization added to NewProject function.The PropertySchema repository is now properly initialized in the NewProject constructor function, ensuring it's available for project operations.
server/internal/usecase/interactor/common.go (3)
145-155: Enhanced SceneDeleter struct with additional repositories.The SceneDeleter struct has been expanded to include additional repositories for PropertySchema, NLSLayer, Plugin, Storytelling, Style, and File. This enhancement allows for comprehensive cleanup of all associated resources when a scene is deleted.
216-221: Asset repository added to ProjectDeleter.The ProjectDeleter struct has been enhanced with an Asset repository field to support asset deletion during project removal.
234-238:✅ Verification successful
Asset deletion implemented in ProjectDeleter.
The implementation now properly cleans up assets associated with a project during project deletion. This ensures that no orphaned assets remain in the system after a project is deleted.
Note that this calls
RemoveByBrojectWithFilewhich has a typo in the method name. Verify that this method name is updated consistently across the codebase when fixing the typo.
🏁 Script executed:
#!/bin/bash # Search for any references to "RemoveByBrojectWithFile" in the codebase rg "RemoveByBrojectWithFile" --type goLength of output: 576
Asset deletion in ProjectDeleter verified; method naming confirmed across the codebase.
The asset deletion functionality now correctly cleans up all project assets. We verified that the method
RemoveByBrojectWithFile(which contains a typo in “Broject”) is used consistently in the following locations:
server/internal/infrastructure/memory/asset.goserver/internal/infrastructure/mongo/asset.goserver/internal/usecase/repo/asset.goIf you decide to fix this typo, please update all these references accordingly.
hexaforce
force-pushed
the
feat/missed-project-data-deletion
branch
from
2135666 to
ca54940
Compare
hexaforce
force-pushed
the
feat/missed-project-data-deletion
branch
from
ca54940 to
55d59e4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
server/internal/infrastructure/mongo/asset.go (1)
159-193: 🛠️ Refactor suggestionError handling needs improvement in the RemoveByProjectWithFile method.
The method logs errors but continues execution, potentially leading to inconsistent states if some operations fail while others succeed. Additionally, it always returns nil regardless of individual operation failures, which masks errors from callers.
Consider implementing one of these approaches:
- Fail-fast approach (return on first error):
func (r *Asset) RemoveByProjectWithFile(ctx context.Context, pid id.ProjectID, f gateway.File) error { projectAssets, err := r.find(ctx, bson.M{ "coresupport": true, "project": pid.String(), }) if err != nil { return err } for _, a := range projectAssets { if !r.f.CanWrite(a.Workspace()) { return repo.ErrOperationDenied } aPath, err := url.Parse(a.URL()) if err != nil { - continue + return fmt.Errorf("failed to parse asset URL %s: %w", a.URL(), err) } err = f.RemoveAsset(ctx, aPath) if err != nil { - log.Print(err.Error()) + return fmt.Errorf("failed to remove asset file for %s: %w", a.ID(), err) } err = r.Remove(ctx, a.ID()) if err != nil { - log.Print(err.Error()) + return fmt.Errorf("failed to remove asset record for %s: %w", a.ID(), err) } } return nil }
- Best-effort approach (collect errors and return them as a composite):
func (r *Asset) RemoveByProjectWithFile(ctx context.Context, pid id.ProjectID, f gateway.File) error { projectAssets, err := r.find(ctx, bson.M{ "coresupport": true, "project": pid.String(), }) if err != nil { return err } + var errs []error for _, a := range projectAssets { if !r.f.CanWrite(a.Workspace()) { return repo.ErrOperationDenied } aPath, err := url.Parse(a.URL()) if err != nil { - continue + errs = append(errs, fmt.Errorf("failed to parse asset URL %s: %w", a.URL(), err)) + continue } err = f.RemoveAsset(ctx, aPath) if err != nil { - log.Print(err.Error()) + errs = append(errs, fmt.Errorf("failed to remove asset file for %s: %w", a.ID(), err)) } err = r.Remove(ctx, a.ID()) if err != nil { - log.Print(err.Error()) + errs = append(errs, fmt.Errorf("failed to remove asset record for %s: %w", a.ID(), err)) } } + if len(errs) > 0 { + return fmt.Errorf("failed to remove %d assets: %v", len(errs), errs) + } return nil }
- If suppressing errors is intended behavior, add a comment explaining this design choice:
func (r *Asset) RemoveByProjectWithFile(ctx context.Context, pid id.ProjectID, f gateway.File) error { + // This method attempts to remove all assets associated with the project, + // continuing even if individual operations fail. Errors are logged but not returned + // to ensure maximum cleanup is attempted regardless of individual failures. projectAssets, err := r.find(ctx, bson.M{ "coresupport": true, "project": pid.String(), }) if err != nil { return err } for _, a := range projectAssets { // Rest of the code remains the same
🧹 Nitpick comments (6)
server/internal/infrastructure/mongo/asset.go (4)
171-173: Consider moving permission check before asset iteration.The current implementation checks permissions for each asset individually and returns immediately if any permission check fails, potentially leaving the operation in an incomplete state. Consider checking permissions beforehand or implementing a two-phase approach.
func (r *Asset) RemoveByProjectWithFile(ctx context.Context, pid id.ProjectID, f gateway.File) error { projectAssets, err := r.find(ctx, bson.M{ "coresupport": true, "project": pid.String(), }) if err != nil { return err } + // Check permissions for all assets first + for _, a := range projectAssets { + if !r.f.CanWrite(a.Workspace()) { + return repo.ErrOperationDenied + } + } for _, a := range projectAssets { - if !r.f.CanWrite(a.Workspace()) { - return repo.ErrOperationDenied - } aPath, err := url.Parse(a.URL()) // Rest of the method... } return nil }
175-178: Improve handling of URL parsing errors.URL parsing errors are silently ignored and the asset is skipped. Consider logging these errors to help with debugging.
aPath, err := url.Parse(a.URL()) if err != nil { + log.Printf("failed to parse asset URL %s: %v", a.URL(), err) continue }
162-163: Consider adding documentation for the "coresupport" filter purpose.The method only processes assets with "coresupport" set to true. Add a comment explaining the significance of this filter to improve code maintainability.
func (r *Asset) RemoveByProjectWithFile(ctx context.Context, pid id.ProjectID, f gateway.File) error { + // Find assets that belong to the project and have coresupport flag enabled. + // Assets with coresupport=true are managed by the core system and need special handling. projectAssets, err := r.find(ctx, bson.M{ "coresupport": true, "project": pid.String(), })
159-159: Add a documentation comment for the method.This public method lacks documentation explaining its purpose, behavior, and return value semantics.
+// RemoveByProjectWithFile finds all assets associated with the given project ID that have +// coreSupport flag set to true, then removes each asset both from the file storage (using +// the provided File interface) and from the database. Errors during individual asset +// removals are logged but do not halt the operation. func (r *Asset) RemoveByProjectWithFile(ctx context.Context, pid id.ProjectID, f gateway.File) error {server/internal/infrastructure/memory/plugin.go (2)
122-138: Consider returning specific errors in error casesThe method currently returns
nilin all cases, including when permission is denied. It would be more informative to return a specific error (likerepo.ErrOperationDenied) in permission failure cases, similar to how theSavemethod works. This would help callers better handle different failure scenarios.func (r *Plugin) RemoveBySceneWithFile(cts context.Context, sid id.SceneID, f gateway.File) error { if !r.f.CanWrite(sid) { - return nil + return repo.ErrOperationDenied } r.lock.Lock() defer r.lock.Unlock() newData := make([]*plugin.Plugin, 0, len(r.data)) for _, p := range r.data { if s := p.Scene(); s == nil || *s != sid { newData = append(newData, p) } } r.data = newData return nil }
130-136: Consider error tracking for reportingConsider adding a mechanism to track how many plugins were removed, which could be useful for logging or returning to the caller. This would help with debugging and monitoring system behavior.
func (r *Plugin) RemoveBySceneWithFile(cts context.Context, sid id.SceneID, f gateway.File) error { if !r.f.CanWrite(sid) { return nil } r.lock.Lock() defer r.lock.Unlock() newData := make([]*plugin.Plugin, 0, len(r.data)) + removed := 0 for _, p := range r.data { if s := p.Scene(); s == nil || *s != sid { newData = append(newData, p) + } else { + removed++ } } r.data = newData + // Could log or return metadata about removed plugins return nil }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
server/internal/infrastructure/adapter/plugin.go(2 hunks)server/internal/infrastructure/adapter/property_schema.go(1 hunks)server/internal/infrastructure/fs/plugin.go(2 hunks)server/internal/infrastructure/fs/property_schema.go(1 hunks)server/internal/infrastructure/memory/asset.go(2 hunks)server/internal/infrastructure/memory/plugin.go(2 hunks)server/internal/infrastructure/memory/property_schema.go(1 hunks)server/internal/infrastructure/mongo/asset.go(2 hunks)server/internal/infrastructure/mongo/plugin.go(2 hunks)server/internal/infrastructure/mongo/property_schema.go(1 hunks)server/internal/usecase/interactor/common.go(4 hunks)server/internal/usecase/interactor/project.go(2 hunks)server/internal/usecase/repo/asset.go(2 hunks)server/internal/usecase/repo/plugin.go(2 hunks)server/internal/usecase/repo/property_schema.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- server/internal/usecase/repo/property_schema.go
- server/internal/usecase/repo/plugin.go
- server/internal/infrastructure/mongo/property_schema.go
- server/internal/infrastructure/fs/plugin.go
- server/internal/infrastructure/adapter/property_schema.go
- server/internal/infrastructure/memory/asset.go
- server/internal/usecase/repo/asset.go
- server/internal/infrastructure/memory/property_schema.go
- server/internal/infrastructure/adapter/plugin.go
- server/internal/infrastructure/mongo/plugin.go
- server/internal/usecase/interactor/common.go
- server/internal/usecase/interactor/project.go
🧰 Additional context used
🧬 Code Definitions (2)
server/internal/infrastructure/fs/property_schema.go (2)
server/internal/infrastructure/fs/plugin.go (6)
r(29-34)r(36-47)r(49-59)r(61-63)r(65-67)r(69-71)server/internal/infrastructure/adapter/property_schema.go (5)
r(27-36)r(38-51)r(53-63)r(65-70)propertySchema(14-17)
server/internal/infrastructure/mongo/asset.go (8)
server/internal/usecase/interactor/asset.go (1)
Asset(25-28)server/internal/adapter/gql/gqlmodel/models_gen.go (2)
Asset(117-128)Asset(130-130)server/internal/usecase/repo/asset.go (1)
Asset(19-29)server/pkg/asset/asset.go (11)
Asset(17-27)a(29-31)a(33-35)a(37-39)a(41-43)a(45-47)a(49-51)a(53-55)a(57-59)a(61-63)a(65-70)server/internal/adapter/gql/loader_asset.go (1)
pid(45-45)server/internal/adapter/gql/resolver_mutation_asset.go (2)
pid(19-19)pid(47-47)server/internal/infrastructure/mongo/mongodoc/asset.go (2)
pid(36-36)pid(65-65)server/internal/infrastructure/fs/file.go (10)
f(50-52)f(54-61)f(63-106)f(108-117)f(121-123)f(125-128)f(130-132)f(136-138)f(140-143)f(145-151)
🔇 Additional comments (4)
server/internal/infrastructure/fs/property_schema.go (1)
84-86: Method implementation consistent with read-only repository pattern.The new
RemoveByScenemethod follows the same pattern as other mutation methods in this read-only implementation, correctly returning an error with "read only" message. This is consistent with the repository's design pattern where file system implementations are read-only.server/internal/infrastructure/mongo/asset.go (1)
7-8: LGTM: Required imports added for the new functionality.The necessary imports for log, url parsing, and the gateway package have been correctly added to support the new functionality in the
RemoveByProjectWithFilemethod.Also applies to: 14-14
server/internal/infrastructure/memory/plugin.go (2)
122-138: Clarify Handling of the File Parameter in RemoveBySceneWithFileThe implementation doesn't use the provided
gateway.Fileparameter. This contradicts the method's name which suggests that file operations are performed during deletion.Consider:
- Implementing file-related cleanup operations if needed
- Adding a comment explaining why the parameter is unused in this implementation
- Renaming the method if the file parameter is irrelevant for in-memory storage
132-132:❓ Verification inconclusive
Inconsistent scene ID access pattern
Other methods in this file (like
FindByIDandRemove) access the scene ID usingp.ID().Scene(), but this method usesp.Scene(). Verify this is intentional and not a potential bug.
🏁 Script executed:
#!/bin/bash # Check how scene IDs are accessed across the codebase echo "Checking p.Scene() usage:" rg "p\.Scene\(\)" --type go -A 2 -B 2 echo -e "\nChecking p.ID().Scene() usage:" rg "p\.ID\(\)\.Scene\(\)" --type go -A 2 -B 2 echo -e "\nChecking plugin.Scene() method definition:" rg "func \(\w+ \*plugin\)\.Scene\(\)" --type go -A 5Length of output: 16972
Attention: Inconsistent Scene ID Access Pattern in Plugin Memory Store
The code at line 132 in
server/internal/infrastructure/memory/plugin.gousesp.Scene()for scene ID retrieval, yet elsewhere in the file (e.g., in the implementations ofFindByIDandRemove) the patternp.ID().Scene()is used. Note that the plugin’sScene()method defined inserver/pkg/plugin/plugin.gointernally callsp.ID().Scene(), so while both approaches currently yield the scene ID, the inconsistency might lead to confusion or subtle bugs.Please verify whether using
p.Scene()directly here is intentional. If not, consider updating the code to consistently usep.ID().Scene()across the file.
hexaforce
changed the title
pyshx
changed the title
Overview
The data deletion process is currently incomplete.
As a result, junk data continues to accumulate.
What I've done
At the moment, when a project is deleted from the trash, only the following items are removed:
The following items are not being deleted properly:
In addition, the following files should also be deleted from file storage:
What I haven't done
How I tested
Which point I want you to review particularly
Memo
Summary by CodeRabbit