feat(ci,server,worker): implement schemata and items copy #1331
Conversation
WalkthroughThis pull request introduces a comprehensive feature for copying models in the ReEarth CMS system. The implementation spans multiple components, including server-side integration API, worker processes, and infrastructure layers. The changes enable users to duplicate models along with their schemas, items, and metadata while also adding new GitHub Actions workflows to support the build and deployment of the copier service. The feature introduces new endpoints, controllers, repositories, and test cases to ensure robust functionality across the system. Changes
Sequence DiagramsequenceDiagram
participant Client
participant ModelController
participant ModelInteractor
participant TaskRunner
participant CopierWorker
participant MongoDB
Client->>ModelController: POST /models/{modelId}/copy
ModelController->>ModelInteractor: Copy Model
ModelInteractor->>ModelInteractor: Copy Schema
ModelInteractor->>ModelInteractor: Copy Items
ModelInteractor->>TaskRunner: Trigger Copy Event
TaskRunner->>CopierWorker: Dispatch Copy Task
CopierWorker->>MongoDB: Retrieve Documents
CopierWorker->>MongoDB: Copy Documents
MongoDB-->>CopierWorker: Confirm Copy
CopierWorker-->>TaskRunner: Task Complete
TaskRunner-->>ModelInteractor: Copy Successful
ModelInteractor-->>ModelController: Return Copied Model
ModelController-->>Client: 200 OK with Copied Model
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
CodeRabbit Configuration File (
|
✅ Deploy Preview for reearth-cms canceled.
|
nourbalaha
changed the title
nourbalaha
changed the title
nourbalaha
changed the title
![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
♻️ Duplicate comments (2)
server/internal/infrastructure/gcp/taskrunner.go (2)
167-174: 🛠️ Refactor suggestionExtract hardcoded build arguments to configuration
Similar to the decompressAsset function, these build arguments should be configurable.
+type CopierConfig struct { + ThreadCount string + GCLimit string + ChunkSize string + DiskLimit string + SkipTop bool + OldWindows bool +} type TaskConfig struct { // ... existing fields ... + CopierConfig *CopierConfig } -Args: []string{ - "-v", - "-n=192", - "-gc=5000", - "-chunk=1m", - "-disk-limit=20g", - "-skip-top", - "-old-windows", -}, +args := []string{"-v"} +if conf.CopierConfig != nil { + if conf.CopierConfig.ThreadCount != "" { + args = append(args, "-n="+conf.CopierConfig.ThreadCount) + } + // ... add other configurable arguments +}
176-178:⚠️ Potential issueAdd input validation for environment variables
The Copy payload fields are used directly in environment variables without validation. This could lead to injection vulnerabilities.
+func sanitizeEnvValue(value string) string { + // Remove any characters that could cause issues in environment variables + // This is a basic example - implement proper sanitization based on your requirements + return strings.Map(func(r rune) rune { + if unicode.IsLetter(r) || unicode.IsNumber(r) || r == '-' || r == '_' || r == '.' { + return r + } + return -1 + }, value) +} Env: []string{ - "REEARTH_CMS_COPIER_COLLECTION=" + p.Copy.Collection, - "REEARTH_CMS_COPIER_FILTER=" + p.Copy.Filter, - "REEARTH_CMS_COPIER_CHANGES=" + p.Copy.Changes, + "REEARTH_CMS_COPIER_COLLECTION=" + sanitizeEnvValue(p.Copy.Collection), + "REEARTH_CMS_COPIER_FILTER=" + sanitizeEnvValue(p.Copy.Filter), + "REEARTH_CMS_COPIER_CHANGES=" + sanitizeEnvValue(p.Copy.Changes), },
🧹 Nitpick comments (5)
server/internal/infrastructure/gcp/taskrunner.go (1)
182-184: Consider making disk size configurable for copy operationsThe copy operation uses a hardcoded defaultDiskSizeGb value. Consider making this configurable similar to the decompressor.
Options: &cloudbuild.BuildOptions{ - DiskSizeGb: defaultDiskSizeGb, + DiskSizeGb: conf.CopierDiskSizeGb, },server/pkg/task/task.go (2)
64-68: Add documentation for Changes and Change typesThe types lack documentation explaining their purpose and usage.
Add documentation:
+// Changes represents a map of field changes to be applied during copy operations type Changes map[string]Change + +// Change represents a single field modification with its type and new value type Change struct { Type ChangeType Value string }
71-74: Consider adding validation for ChangeType valuesThe ChangeType constants should be validated when used.
Add validation:
const ( ChangeTypeSet ChangeType = "set" ChangeTypeNew ChangeType = "new" ) + +// IsValid checks if the ChangeType is a valid value +func (ct ChangeType) IsValid() bool { + switch ct { + case ChangeTypeSet, ChangeTypeNew: + return true + default: + return false + } +}.github/workflows/build_copier.yml (1)
7-10: Consider adding timeout to concurrency configurationThe concurrency configuration lacks a timeout, which could lead to stuck workflows.
Add timeout:
concurrency: group: ${{ github.workflow }}-${{ github.event.workflow_run.head_branch }} cancel-in-progress: true + timeout-minutes: 60server/internal/usecase/interactor/model.go (1)
415-439: Add retry mechanism for copyItemsThe copyItems method should handle transient failures during the copy operation.
Add retry logic:
func (i Model) copyItems(ctx context.Context, oldSchemaID, newSchemaID id.SchemaID, newModelID id.ModelID) error { + maxRetries := 3 + var lastErr error + + for attempt := 1; attempt <= maxRetries; attempt++ { collection := "item" filter, err := json.Marshal(bson.M{"schema": oldSchemaID.String()}) if err != nil { return err } changes, err := json.Marshal(task.Changes{ "id": { Type: task.ChangeTypeNew, Value: "item", }, "schema": { Type: task.ChangeTypeSet, Value: newSchemaID.String(), }, "model": { Type: task.ChangeTypeSet, Value: newModelID.String(), }, }) if err != nil { return err } - return i.triggerCopyEvent(ctx, collection, string(filter), string(changes)) + if err := i.triggerCopyEvent(ctx, collection, string(filter), string(changes)); err != nil { + lastErr = err + log.Warnf("attempt %d failed: %v", attempt, err) + continue + } + return nil + } + return fmt.Errorf("failed after %d attempts: %v", maxRetries, lastErr) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/build_copier.yml(1 hunks)server/internal/infrastructure/gcp/taskrunner.go(4 hunks)server/internal/usecase/interactor/model.go(3 hunks)server/pkg/task/task.go(2 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/build_copier.yml
31-31: shellcheck reported issue in this script: SC2236:style:4:9: Use -n instead of ! -z
(shellcheck)
78-78: shellcheck reported issue in this script: SC2129:style:15:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
🔇 Additional comments (3)
server/internal/infrastructure/gcp/taskrunner.go (1)
119-120: 🛠️ Refactor suggestion
Extract hardcoded build step arguments to configuration
The build step arguments contain multiple hardcoded values that should be configurable. This makes the application less flexible and harder to maintain.
+type DecompressorConfig struct {
+ ThreadCount string
+ GCLimit string
+ ChunkSize string
+ DiskLimit string
+ SkipTop bool
+ OldWindows bool
+}
type TaskConfig struct {
// ... existing fields ...
+ DecompressorConfig *DecompressorConfig
}
-Args: []string{"-v", "-n=192", "-gc=5000", "-chunk=1m", "-disk-limit=20g", "-gzip-ext=" + conf.DecompressorGzipExt, "-skip-top", "-old-windows", src, dest},
+args := []string{"-v"}
+if conf.DecompressorConfig != nil {
+ if conf.DecompressorConfig.ThreadCount != "" {
+ args = append(args, "-n="+conf.DecompressorConfig.ThreadCount)
+ }
+ // ... add other configurable arguments
+}
+args = append(args, "-gzip-ext="+conf.DecompressorGzipExt, src, dest)Likely invalid or redundant comment.
server/pkg/task/task.go (1)
48-52: 🛠️ Refactor suggestion
Consider enhancing CopyPayload validation
The CopyPayload struct contains critical fields for copy operations but lacks comprehensive validation.
Consider adding field-level validation:
type CopyPayload struct {
Collection string
Filter string
Changes string
+
+ // Validate checks if the payload fields are valid
+ func (p *CopyPayload) Validate() error {
+ if p == nil {
+ return errors.New("payload cannot be nil")
+ }
+ if p.Collection == "" {
+ return errors.New("collection is required")
+ }
+ if p.Filter == "" {
+ return errors.New("filter is required")
+ }
+ if !json.Valid([]byte(p.Changes)) {
+ return errors.New("changes must be valid JSON")
+ }
+ return nil
+ }
}Likely invalid or redundant comment.
.github/workflows/build_copier.yml (1)
97-106: 🛠️ Refactor suggestion
Add resource limits to Docker build step
The Docker build step lacks resource constraints, which could impact CI/CD performance.
Add resource limits:
uses: docker/build-push-action@v6
with:
context: ./worker
file: ./worker/copier.Dockerfile
platforms: ${{ steps.options.outputs.platforms }}
push: true
build-args: VERSION=${{ steps.options.outputs.version }}
tags: ${{ steps.options.outputs.tags }}
cache-from: type=gha
cache-to: type=gha,mode=max
+ timeout-minutes: 60
+ memory: 6G
+ cpu-limit: 2Likely invalid or redundant comment.
nourbalaha
force-pushed
the
feat-server/schemata-item-copy
branch
from
8fbdb8a to
e80cf07
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
server/internal/usecase/interactor/model_test.go (3)
540-575: LGTM! Consider adding test cleanup.The test setup is thorough and follows best practices. However, consider adding cleanup of test data after each test case.
func TestModel_Copy(t *testing.T) { + t.Cleanup(func() { + ctx := context.Background() + _ = db.Model.DeleteAll(ctx) + _ = db.Schema.DeleteAll(ctx) + _ = db.Project.DeleteAll(ctx) + })
576-629: Add test cases for additional scenarios.The current test cases cover basic scenarios. Consider adding test cases for:
- Invalid project permissions
- Schema copy failures
- Metadata schema copy failures
Example test case:
+ { + name: "invalid project permissions", + param: interfaces.CopyModelParam{ + ModelId: m.ID(), + Name: lo.ToPtr("Copied Model"), + }, + setupMock: func() { + mRunner.EXPECT().Run(ctx, gomock.Any()).Times(0) + }, + wantErr: true, + validate: func(t *testing.T, got *model.Model) { + assert.Nil(t, got) + }, + operator: &usecase.Operator{}, // operator without project permissions + },
631-644: Enhance test execution with parallel runs and error validation.Consider these improvements:
- Run test cases in parallel for faster execution
- Validate error messages for better test coverage
for _, tt := range tests { + tt := tt t.Run(tt.name, func(t *testing.T) { + t.Parallel() tt.setupMock() got, err := u.Copy(ctx, tt.param, op) if tt.wantErr { assert.Error(t, err) + if tt.expectedError != nil { + assert.ErrorIs(t, err, tt.expectedError) + } tt.validate(t, nil) } else {server/e2e/integration_model_test.go (2)
79-121: Add tests for error responses and additional parameters.While the happy path is well tested, consider adding:
- Tests for invalid request parameters
- Validation of error response structure
- Tests for additional request parameters (e.g., description, key)
Example test case:
+ // Test invalid request + e.POST(endpoint, oldModelId). + WithHeader("authorization", "Bearer "+secret). + WithJSON(map[string]interface{}{ + "name": "", // Invalid empty name + }). + Expect(). + Status(http.StatusBadRequest). + JSON(). + Object(). + HasValue("error", "invalid_request"). + HasValue("message", "name cannot be empty")
122-140: Enhance schema validation coverage.Consider adding more detailed schema validation:
- Validate individual field properties
- Check field types and configurations
- Verify schema metadata and settings
Example validation:
copiedSchema.Value("fields").Array().Length().IsEqual(oldSchema.Value("fields").Array().Length().Raw()) + // Validate first field properties + firstField := copiedSchema.Value("fields").Array().First().Object() + firstField.HasValue("type", "text") + firstField.HasValue("required", true) + firstField.Value("title").NotNull() + firstField.Value("key").NotNull()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
server/e2e/integration_model_test.go(1 hunks)server/internal/usecase/interactor/model_test.go(2 hunks)server/internal/usecase/interfaces/model.go(2 hunks)server/pkg/schema/schema_test.go(1 hunks)worker/cmd/copier/main.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- server/internal/usecase/interfaces/model.go
- server/pkg/schema/schema_test.go
- worker/cmd/copier/main.go
🔇 Additional comments (1)
server/e2e/integration_model_test.go (1)
60-78: LGTM! Authorization tests are comprehensive.The authorization tests cover all necessary scenarios and follow the established pattern.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
worker/internal/adapter/http/copy.go (1)
28-38:⚠️ Potential issueAdd input validation and improve error handling
The current implementation lacks proper input validation and error handling, which could lead to security vulnerabilities.
- Add input validation:
func (c *CopyController) Copy(ctx context.Context, input CopyInput) error { + if len(input.Filter) == 0 || len(input.Changes) == 0 { + return rerror.ErrInvalidRequestBy(fmt.Errorf("filter and changes cannot be empty")) + } + if len(input.Filter) > 1024*1024 || len(input.Changes) > 1024*1024 { + return rerror.ErrInvalidRequestBy(fmt.Errorf("input size exceeds limit")) + } var filter bson.M if err := json.Unmarshal([]byte(input.Filter), &filter); err != nil { - return rerror.ErrInternalBy(err) + return rerror.ErrInvalidRequestBy(fmt.Errorf("invalid filter format: %w", err)) } var changes task.Changes if err := json.Unmarshal([]byte(input.Changes), &changes); err != nil { - return rerror.ErrInternalBy(err) + return rerror.ErrInvalidRequestBy(fmt.Errorf("invalid changes format: %w", err)) } + if err := validateMongoFilter(filter); err != nil { + return rerror.ErrInvalidRequestBy(fmt.Errorf("invalid filter: %w", err)) + } return c.usecase.Copy(ctx, filter, changes) }worker/internal/infrastructure/mongo/copier_test.go (1)
25-61: 🛠️ Refactor suggestionEnhance test coverage
The current test coverage is insufficient. Consider adding the following test cases:
- Error scenarios (invalid filter, changes)
- Verification of copied documents
- Edge cases (empty filter, null values)
Example additional test cases:
t.Run("Copy_InvalidFilter", func(t *testing.T) { err := w.Copy(ctx, bson.M{"$invalid": 1}, changes) assert.Error(t, err) }) t.Run("Copy_VerifyCopiedDocs", func(t *testing.T) { err := w.Copy(ctx, filter, changes) assert.NoError(t, err) // Verify copied documents var copied []bson.M cursor, err := iCol.Find(ctx, bson.M{"schema": sid2.String()}) assert.NoError(t, err) err = cursor.All(ctx, &copied) assert.NoError(t, err) assert.Len(t, copied, 2) })server/internal/usecase/interactor/model.go (2)
322-353:⚠️ Potential issueAdd transaction rollback mechanism
The Copy method should handle cleanup if any step fails during the copy process.
Previous suggestion about implementing rollback logic is still valid. This is crucial for maintaining data consistency in case of failures during the copy process.
445-463:⚠️ Potential issueEnhance error handling and add timeout
The method needs better error handling and timeout management.
Previous suggestions about adding context timeout and validating taskPayload are still valid. Additionally:
if err := i.gateways.TaskRunner.Run(ctx, taskPayload.Payload()); err != nil { - return fmt.Errorf("failed to trigger copy event: %w", err) + return fmt.Errorf("failed to trigger copy event for collection %s: %w", collection, err) }
🧹 Nitpick comments (9)
worker/internal/adapter/http/copy.go (1)
23-26: Add JSON validation tags to input structConsider adding validation tags to enforce input constraints at the struct level.
type CopyInput struct { - Filter string `json:"filter"` - Changes string `json:"changes"` + Filter string `json:"filter" validate:"required,max=1048576"` + Changes string `json:"changes" validate:"required,max=1048576"` }worker/internal/usecase/repo/copier.go (1)
10-12: Add interface documentationThe Copier interface would benefit from documentation explaining its purpose and the expected behavior of the Copy method.
+// Copier defines the interface for copying documents based on a filter and applying specified changes type Copier interface { + // Copy applies the specified changes to documents matching the filter + // Returns an error if the operation fails Copy(context.Context, bson.M, task.Changes) error }worker/internal/usecase/interactor/copier.go (1)
10-12: Improve error handlingConsider wrapping the repository error with context to help with debugging and error tracking.
func (u *Usecase) Copy(ctx context.Context, filter bson.M, changes task.Changes) error { - return u.repos.Copier.Copy(ctx, filter, changes) + if err := u.repos.Copier.Copy(ctx, filter, changes); err != nil { + return fmt.Errorf("failed to copy documents: %w", err) + } + return nil }worker/internal/infrastructure/mongo/copier_test.go (2)
41-41: Remove debug print statementThe fmt.Print statement appears to be used for debugging and should be removed.
- fmt.Print(res)
16-23: Add cleanup between testsAdd cleanup to prevent test data from affecting other tests.
func TestCopier(t *testing.T) { db := mongotest.Connect(t)(t) + defer db.Drop(context.Background()) w := NewCopier(db) w.SetCollection(db.Collection("item"))worker/internal/infrastructure/mongo/copier.go (1)
16-17: Move batch size to configuration.The TODO comment indicates that the batch size should be configurable. Consider moving this to the application's configuration system for better maintainability.
-// TODO: this should be taken from config -var batchSize int32 = 1000 +type Config struct { + BatchSize int32 `env:"BATCH_SIZE" envDefault:"1000"` +}server/e2e/integration_model_test.go (1)
60-143: Add edge case tests for model copying.The test suite should include additional cases:
- Copying with invalid name/key formats
- Copying non-existent models
- Copying with duplicate keys
Add these test cases:
// Test invalid name e.POST(endpoint, oldModelId). WithHeader("authorization", "Bearer "+secret). WithJSON(map[string]interface{}{ "name": "", }). Expect(). Status(http.StatusBadRequest) // Test invalid key e.POST(endpoint, oldModelId). WithHeader("authorization", "Bearer "+secret). WithJSON(map[string]interface{}{ "key": "invalid key", }). Expect(). Status(http.StatusBadRequest) // Test non-existent model e.POST(endpoint, id.NewModelID()). WithHeader("authorization", "Bearer "+secret). WithJSON(map[string]interface{}{ "name": "new name", }). Expect(). Status(http.StatusNotFound)server/schemas/integration.yml (1)
452-486: Enhance the endpoint documentation.While the endpoint definition is well-structured, consider adding more detailed documentation to help API consumers:
- Describe what gets copied (schema, items, metadata)
- Explain the purpose of the optional
keyfield- Document any constraints on the
namefield- Specify if the copy operation is asynchronous or synchronous
'/models/{modelId}/copy': parameters: - $ref: '#/components/parameters/modelIdParam' post: operationId: CopyModel - summary: Copy schema and items of a selected model + summary: Create a copy of an existing model + description: | + Creates a new model by copying the schema and items from an existing model. + The operation copies: + - Model schema + - Model items + - Model metadata + + The name field is required and must be unique within the project. + The key field is optional and must be unique if provided. + + This is a synchronous operation that returns the newly created model. tags: - Modelsserver/internal/usecase/interactor/model.go (1)
355-379: Enhance model copy implementationA few suggestions for improvement:
- Consider making the "Copy" suffix configurable or localizable.
- Add explicit project access validation before proceeding with the copy operation.
func (i Model) copyModel(ctx context.Context, params interfaces.CopyModelParam, operator *usecase.Operator) (*model.Model, *model.Model, error) { oldModel, err := i.repos.Model.FindByID(ctx, params.ModelId) if err != nil { return nil, nil, err } + if !operator.IsMaintainingProject(oldModel.Project()) { + return nil, nil, interfaces.ErrOperationDenied + } - name := lo.ToPtr(oldModel.Name() + " Copy") + name := lo.ToPtr(oldModel.Name() + i18n.T(" Copy"))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
server/e2e/integration_model_test.go(1 hunks)server/internal/adapter/integration/model.go(1 hunks)server/internal/adapter/integration/server.gen.go(7 hunks)server/internal/usecase/interactor/model.go(3 hunks)server/internal/usecase/interfaces/model.go(2 hunks)server/pkg/integrationapi/types.gen.go(3 hunks)server/schemas/integration.yml(2 hunks)worker/internal/adapter/http/copy.go(1 hunks)worker/internal/infrastructure/mongo/copier.go(1 hunks)worker/internal/infrastructure/mongo/copier_test.go(1 hunks)worker/internal/usecase/interactor/copier.go(1 hunks)worker/internal/usecase/repo/copier.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server/pkg/integrationapi/types.gen.go
🔇 Additional comments (6)
worker/internal/infrastructure/mongo/copier.go (1)
37-84:⚠️ Potential issueAdd error handling for bulk write failures.
The bulk write operation doesn't handle partial failures. MongoDB's BulkWrite can fail for individual documents while succeeding for others.
if len(bulkModels) > 0 { - _, err = r.c.BulkWrite(ctx, bulkModels) + result, err := r.c.BulkWrite(ctx, bulkModels, options.BulkWrite().SetOrdered(false)) if err != nil { + if bulkErr, ok := err.(mongo.BulkWriteException); ok { + log.Errorf("partial bulk write failure: %d/%d operations failed", + len(bulkErr.WriteErrors), len(bulkModels)) + return fmt.Errorf("partial bulk write failure: %w", err) + } return rerror.ErrInternalBy(err) } + if result.InsertedCount != int64(len(bulkModels)) { + return fmt.Errorf("expected to insert %d documents, but inserted %d", + len(bulkModels), result.InsertedCount) + } }Likely invalid or redundant comment.
server/internal/adapter/integration/model.go (1)
115-145: 🛠️ Refactor suggestionImprove error handling and add request validation.
The handler should validate the request body and provide more specific error responses.
func (s *Server) CopyModel(ctx context.Context, request CopyModelRequestObject) (CopyModelResponseObject, error) { uc := adapter.Usecases(ctx) op := adapter.Operator(ctx) + if request.Body.Name != nil && *request.Body.Name == "" { + return CopyModel400Response{}, rerror.NewE(rerror.ErrInvalidParams, "name cannot be empty") + } + + if request.Body.Key != nil && !id.NewKey(*request.Body.Key).IsValid() { + return CopyModel400Response{}, rerror.NewE(rerror.ErrInvalidParams, "invalid key format") + } m, err := uc.Model.Copy(ctx, interfaces.CopyModelParam{Likely invalid or redundant comment.
server/schemas/integration.yml (1)
1861-1861: LGTM! Property rename follows JSON conventions.The rename from
TitleFieldtotitleFieldfollows camelCase convention which is more idiomatic in JSON/OpenAPI specifications.server/internal/adapter/integration/server.gen.go (1)
78-80: LGTM! Generated code matches the API specification.The generated code for the new
CopyModelendpoint is consistent with other endpoints and correctly implements:
- Interface method
- Request/response types
- Error handling
- Authentication
- Parameter binding
Also applies to: 513-529, 2278-2324, 3814-3816, 4365-4394
server/internal/usecase/interactor/model.go (2)
419-443: Improve items copy configuration and validationThe method could be enhanced in several ways:
- Make the collection name configurable.
- Validate the task payload before triggering the copy event.
Previous suggestion about validating taskPayload is still valid. Additionally:
- collection := "item" + collection := i.config.Collections.Item // Add configuration for collection names
381-392: Add schema compatibility validationThe schema copy operation should validate compatibility and handle potential errors from the CopyFrom operation.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
worker/internal/infrastructure/mongo/copier_test.go (1)
23-59: 🛠️ Refactor suggestionEnhance test coverage and robustness.
The test needs significant improvements:
- Missing verification of the copied items.
- No cleanup of test resources.
- No error cases tested.
- Not running in parallel.
Here's a suggested improvement:
func TestCopier_Copy(t *testing.T) { + t.Parallel() ctx := context.Background() db := mongotest.Connect(t)(t) + defer db.Drop(ctx) w := NewCopier(db) iCol := db.Collection("item") w.SetCollection(iCol) - mid1 := id.NewModelID() - sid1 := id.NewSchemaID() - mid2 := id.NewModelID() - sid2 := id.NewSchemaID() - i1 := item.New().ID(id.NewItemID()).Schema(sid1).Model(mid1).Project(id.NewProjectID()).Thread(id.NewThreadID()).MustBuild() - i2 := item.New().ID(id.NewItemID()).Schema(sid1).Model(mid1).Project(id.NewProjectID()).Thread(id.NewThreadID()).MustBuild() - - res, err := iCol.InsertMany(ctx, []any{i1, i2}) - assert.NoError(t, err) - fmt.Print(res) - - filter := bson.M{"schema": sid1.String()} - changes := task.Changes{ - "id": { - Type: task.ChangeTypeNew, - Value: "item", - }, - "schema": { - Type: task.ChangeTypeSet, - Value: sid2.String(), - }, - "model": { - Type: task.ChangeTypeSet, - Value: mid2.String(), - }, - } - - err = w.Copy(ctx, filter, changes) - assert.NoError(t, err) + tests := []struct { + name string + setup func(t *testing.T) (filter bson.M, changes task.Changes) + wantErr bool + }{ + { + name: "success", + setup: func(t *testing.T) (bson.M, task.Changes) { + mid1 := id.NewModelID() + sid1 := id.NewSchemaID() + mid2 := id.NewModelID() + sid2 := id.NewSchemaID() + i1 := item.New().ID(id.NewItemID()).Schema(sid1).Model(mid1).Project(id.NewProjectID()).Thread(id.NewThreadID()).MustBuild() + i2 := item.New().ID(id.NewItemID()).Schema(sid1).Model(mid1).Project(id.NewProjectID()).Thread(id.NewThreadID()).MustBuild() + + _, err := iCol.InsertMany(ctx, []any{i1, i2}) + assert.NoError(t, err) + + return bson.M{"schema": sid1.String()}, task.Changes{ + "id": {Type: task.ChangeTypeNew, Value: "item"}, + "schema": {Type: task.ChangeTypeSet, Value: sid2.String()}, + "model": {Type: task.ChangeTypeSet, Value: mid2.String()}, + } + }, + wantErr: false, + }, + { + name: "invalid_filter", + setup: func(t *testing.T) (bson.M, task.Changes) { + return bson.M{"invalid": "filter"}, task.Changes{} + }, + wantErr: true, + }, + { + name: "invalid_changes", + setup: func(t *testing.T) (bson.M, task.Changes) { + return bson.M{}, task.Changes{ + "invalid": {Type: "invalid", Value: nil}, + } + }, + wantErr: true, + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + filter, changes := tt.setup(t) + + err := w.Copy(ctx, filter, changes) + if tt.wantErr { + assert.Error(t, err) + return + } + assert.NoError(t, err) + + // Verify copied items + var copied []item.Item + cursor, err := iCol.Find(ctx, changes) + assert.NoError(t, err) + err = cursor.All(ctx, &copied) + assert.NoError(t, err) + assert.NotEmpty(t, copied) + }) + } }server/internal/usecase/interactor/model.go (3)
322-353: 🛠️ Refactor suggestionAdd rollback mechanism for partial failures.
The Copy method should ensure proper cleanup if any step fails to maintain data consistency.
This is a duplicate of a previous review comment. The suggestion to implement rollback logic remains valid.
381-392: 🛠️ Refactor suggestionAdd workspace and project validation.
The method should validate that both schemas belong to the same workspace and project before copying.
This is a duplicate of a previous review comment. The suggestion to validate workspace consistency remains valid.
445-463: 🛠️ Refactor suggestionAdd context timeout and payload validation.
The method needs a timeout to prevent long-running operations and should validate the task payload.
This is a duplicate of previous review comments. The suggestions to add context timeout and validate task payload remain valid.
🧹 Nitpick comments (4)
worker/internal/infrastructure/mongo/copier_test.go (2)
16-21: Add cleanup and error handling.While the test is functional, it could be improved by:
- Adding database cleanup using
defer db.Drop(ctx).- Adding error handling for database connection.
func TestCopier_SetCollection(t *testing.T) { + ctx := context.Background() db := mongotest.Connect(t)(t) + defer db.Drop(ctx) + w := NewCopier(db) w.SetCollection(db.Collection("item")) assert.Equal(t, "item", w.c.Name()) }
39-39: Remove debug print statement.The
fmt.Print(res)statement appears to be left from debugging and should be removed.server/internal/usecase/interactor/model.go (2)
394-417: Improve error handling for model updates.Consider wrapping the model and schema saves in a transaction to ensure atomicity.
+ if err := i.repos.Schema.Save(ctx, newMetaSchema); err != nil { + return nil, fmt.Errorf("failed to save meta schema: %w", err) + } newModel.SetMetadata(newMetaSchema.ID()) - if err := i.repos.Model.Save(ctx, newModel); err != nil { - return nil, err - } - if err := i.repos.Schema.Save(ctx, newMetaSchema); err != nil { - return nil, err + if err := i.repos.Model.Save(ctx, newModel); err != nil { + return nil, fmt.Errorf("failed to update model with meta schema: %w", err) }
419-443: Improve error handling and add constants.The method needs better error handling and should use constants for collection names.
+const ( + itemCollection = "item" + idField = "id" + schemaField = "schema" + modelIDField = "modelid" +) func (i Model) copyItems(ctx context.Context, oldSchemaID, newSchemaID id.SchemaID, newModelID id.ModelID) error { - collection := "item" + collection := itemCollection filter, err := json.Marshal(bson.M{"schema": oldSchemaID.String()}) if err != nil { - return err + return fmt.Errorf("failed to marshal filter: %w", err) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
server/internal/usecase/interactor/model.go(3 hunks)worker/cmd/copier/main.go(1 hunks)worker/internal/infrastructure/mongo/container.go(1 hunks)worker/internal/infrastructure/mongo/copier.go(1 hunks)worker/internal/infrastructure/mongo/copier_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- worker/internal/infrastructure/mongo/container.go
- worker/cmd/copier/main.go
- worker/internal/infrastructure/mongo/copier.go
🔇 Additional comments (2)
worker/internal/infrastructure/mongo/copier_test.go (1)
1-14: LGTM!The package declaration and imports are well-organized and all imports are being utilized.
server/internal/usecase/interactor/model.go (1)
355-379: Add name uniqueness validation.The method should validate that the new name doesn't conflict with existing models in the project.
Run this script to check if the codebase has name uniqueness validation:
✅ Verification successful
Add name uniqueness validation.
The codebase has key uniqueness validation but lacks name uniqueness checks. While the
copyModelmethod appends "Copy" to duplicate names, there's no guarantee of uniqueness:
- Multiple copies could create models with the same name
- Custom names provided via
params.Namearen't validated for uniqueness🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for name uniqueness validation in model-related files rg -A 5 'func.*Create.*Model' | rg -A 5 'name.*unique|duplicate.*name'Length of output: 72
Script:
#!/bin/bash # Search for model validation and uniqueness checks echo "=== Searching for model validation ===" rg -g '!*.test.*' -g '!*test*' "func.*Validate.*Model|func.*Check.*Model" -A 5 echo -e "\n=== Searching for name uniqueness checks in repository layer ===" ast-grep --pattern 'func ($VAR *ModelRepository) Create($ARGS) { $$$ }' echo -e "\n=== Searching for database schema or constraints ===" fd -e sql . | xargs catLength of output: 3138
Script:
#!/bin/bash # Search for model repository implementation echo "=== Searching for model repository implementation ===" fd -t f "model.*repository.*go" -X cat echo -e "\n=== Searching for model table schema ===" fd -t f ".*model.*sql" -X cat echo -e "\n=== Searching for Create method implementation ===" ast-grep --pattern 'func ($VAR *Model) Create(ctx context.Context, $ARGS) ($RET, error) { $$$ }'Length of output: 504
Script:
#!/bin/bash echo "=== Searching for repository files ===" rg -g "*.go" -l "type.*ModelRepository" echo -e "\n=== Searching for model creation implementation ===" rg -g "*.go" -B 5 -A 10 "func.*Create.*Model.*Context" echo -e "\n=== Searching for database schema files ===" fd . -e sql -e ddl -e schemaLength of output: 4637
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
worker/internal/infrastructure/mongo/container.go (2)
10-22:⚠️ Potential issueAdd validation for required components
The
Newfunction should validate its input parameters to prevent potential nil pointer dereferences later.Apply this diff to add validation:
func New(ctx context.Context, webhook *Webhook, copier *Copier) (*repo.Container, error) { + if webhook == nil { + return nil, fmt.Errorf("webhook is required") + } + if copier == nil { + return nil, fmt.Errorf("copier is required") + } + r := &repo.Container{ Webhook: webhook, Copier: copier, }
24-34:⚠️ Potential issueAdd type assertion safety checks
The initialization should include type assertions to ensure the correct implementation types are provided.
Apply this diff to add type safety:
func Init(r *repo.Container) error { if r == nil { return nil } if r.Webhook == nil && r.Copier == nil { return errors.New("invalid repository container") } + if r.Webhook != nil { + if _, ok := r.Webhook.(*Webhook); !ok { + return fmt.Errorf("invalid webhook implementation type") + } + } + + if r.Copier != nil { + if _, ok := r.Copier.(*Copier); !ok { + return fmt.Errorf("invalid copier implementation type") + } + } + return nil }
🧹 Nitpick comments (1)
worker/internal/infrastructure/mongo/container.go (1)
1-34: Consider enhancing the container for schema and item copy operationsGiven that this PR aims to implement schemata and items copy functionality, consider extending the container to include:
- Validation of copy operations (e.g., checking for circular dependencies)
- Rate limiting or batching for large copy operations
- Progress tracking for long-running copy operations
- Rollback mechanisms for failed copy operations
This would make the implementation more robust and production-ready.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
worker/cmd/copier/main.go(1 hunks)worker/internal/app/main.go(4 hunks)worker/internal/infrastructure/mongo/container.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- worker/cmd/copier/main.go
- worker/internal/app/main.go
🔇 Additional comments (1)
worker/internal/infrastructure/mongo/container.go (1)
1-9: LGTM!The package declaration and imports are clean and well-organized.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
server/internal/usecase/interactor/model.go (4)
322-353:⚠️ Potential issueImplement rollback mechanism for partial failures.
The
Copymethod performs multiple operations but lacks cleanup mechanisms for partial failures. If an error occurs after creating the model but before copying items, the system could be left in an inconsistent state.Apply the rollback mechanism as suggested in the previous review:
func (i Model) Copy(ctx context.Context, params interfaces.CopyModelParam, operator *usecase.Operator) (*model.Model, error) { return Run1(ctx, operator, i.repos, Usecase().Transaction(), func(ctx context.Context) (*model.Model, error) { + // Create a cleanup function + var cleanup func() error + defer func() { + if cleanup != nil { + if err := cleanup(); err != nil { + log.Errorf("failed to cleanup after copy error: %v", err) + } + } + }() + // copy model oldModel, newModel, err := i.copyModel(ctx, params, operator) if err != nil { return nil, err } + cleanup = func() error { + return i.repos.Model.Remove(ctx, newModel.ID()) + }
381-392: 🛠️ Refactor suggestionEnsure workspace consistency and proper schema copying.
The method should validate workspace consistency and ensure proper deep copying of schema fields.
Apply the workspace validation as suggested in the previous review:
func (i Model) copySchema(ctx context.Context, oldSchemaId, newSchemaId id.SchemaID) error { oldSchema, err := i.repos.Schema.FindByID(ctx, oldSchemaId) if err != nil { return err } newSchema, err := i.repos.Schema.FindByID(ctx, newSchemaId) if err != nil { return err } + if oldSchema.Workspace() != newSchema.Workspace() { + return fmt.Errorf("workspace mismatch: cannot copy schema across workspaces") + } newSchema.CopyFrom(oldSchema) return i.repos.Schema.Save(ctx, newSchema) }
419-443: 🛠️ Refactor suggestionValidate task payload before execution.
The method should validate the task payload before triggering the copy event.
Apply the validation as suggested in the previous review:
taskPayload := task.CopyPayload{ Collection: collection, Filter: filter, Changes: changes, } + + if err := taskPayload.Validate(); err != nil { + return fmt.Errorf("invalid task payload: %w", err) + }
445-463: 🛠️ Refactor suggestionAdd context timeout to prevent long-running operations.
The method should have a timeout to prevent long-running copy operations.
Apply the timeout as suggested in the previous review:
func (i Model) triggerCopyEvent(ctx context.Context, collection, filter, changes string) error { + ctx, cancel := context.WithTimeout(ctx, 30*time.Second) + defer cancel() + if i.gateways.TaskRunner == nil { log.Infof("model: copy of %s skipped because task runner is not configured", collection) return nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/internal/usecase/interactor/model.go(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: ci-server / test
🔇 Additional comments (1)
server/internal/usecase/interactor/model.go (1)
394-417: LGTM with previous suggestions applied.The implementation looks correct, but ensure to apply the workspace validation and atomic operation handling suggested in the previous review.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
worker/internal/infrastructure/mongo/copier_test.go (1)
22-57: 🛠️ Refactor suggestionEnhance test coverage and add data verification.
The test needs improvement in several areas:
- No verification of the copied data
- Missing cleanup after test execution
- No error cases tested
Apply this diff to improve the test:
func TestCopier_Copy(t *testing.T) { ctx := context.Background() db := mongotest.Connect(t)(t) + defer db.Drop(ctx) w := NewCopier(db) iCol := db.Collection("item") w.SetCollection(iCol) mid1 := id.NewModelID() sid1 := id.NewSchemaID() mid2 := id.NewModelID() sid2 := id.NewSchemaID() i1 := item.New().ID(id.NewItemID()).Schema(sid1).Model(mid1).Project(id.NewProjectID()).Thread(id.NewThreadID()).MustBuild() i2 := item.New().ID(id.NewItemID()).Schema(sid1).Model(mid1).Project(id.NewProjectID()).Thread(id.NewThreadID()).MustBuild() _, err := iCol.InsertMany(ctx, []any{i1, i2}) assert.NoError(t, err) filter := bson.M{"schema": sid1.String()} changes := task.Changes{ "id": { Type: task.ChangeTypeNew, Value: "item", }, "schema": { Type: task.ChangeTypeSet, Value: sid2.String(), }, "model": { Type: task.ChangeTypeSet, Value: mid2.String(), }, } err = w.Copy(ctx, filter, changes) assert.NoError(t, err) + + // Verify copied data + var results []bson.M + cursor, err := iCol.Find(ctx, bson.M{"schema": sid2.String()}) + assert.NoError(t, err) + err = cursor.All(ctx, &results) + assert.NoError(t, err) + assert.Len(t, results, 2) + + // Test error cases + err = w.Copy(ctx, bson.M{"invalid": "filter"}, changes) + assert.Error(t, err) + + invalidChanges := task.Changes{ + "invalid": { + Type: task.ChangeTypeSet, + Value: "value", + }, + } + err = w.Copy(ctx, filter, invalidChanges) + assert.Error(t, err) }
🧹 Nitpick comments (2)
worker/internal/infrastructure/mongo/copier_test.go (2)
15-20: Add cleanup after test execution.While the test correctly verifies the collection name setting, it should clean up after execution to prevent interference with other tests.
func TestCopier_SetCollection(t *testing.T) { db := mongotest.Connect(t)(t) + defer db.Drop(context.Background()) w := NewCopier(db) w.SetCollection(db.Collection("item")) assert.Equal(t, "item", w.c.Name()) }
59-98: Enhance test cases to verify generated ID values.The test only verifies if ID generation succeeds but doesn't check the actual generated ID values. Add verification of the generated IDs to ensure they match the expected format for each type.
func TestCopier_GenerateId(t *testing.T) { tests := []struct { name string input string expectOk bool + expectType string }{ { name: "Valid input - item", input: "item", expectOk: true, + expectType: "item", }, // ... other test cases ... } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - _, ok := generateId(tt.input) + id, ok := generateId(tt.input) assert.Equal(t, tt.expectOk, ok) + if ok { + assert.Contains(t, id.String(), tt.expectType) + } }) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
worker/internal/infrastructure/mongo/common_test.go(1 hunks)worker/internal/infrastructure/mongo/copier_test.go(1 hunks)
🔇 Additional comments (1)
worker/internal/infrastructure/mongo/common_test.go (1)
1-7: LGTM! Environment setup is correctly implemented.The initialization of the test environment follows Go conventions and uses consistent naming.
nourbalaha
force-pushed
the
feat-server/schemata-item-copy
branch
3 times, most recently
from
2e0c104 to
90b5277
Compare
Overview
This PR implements schemata and items copy
Summary by CodeRabbit
Release Notes
New Features
/models/{modelId}/copyfor model duplication.CopyControllerfor handling copy operations via HTTP.Improvements
Technical Updates
Containerfor managing related functionalities.Bug Fixes