chore(server): reduce importer transaction time #1380
Conversation
WalkthroughThe changes focus on refactoring the Changes
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.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
server/internal/usecase/interactor/item.go (3)
374-409: Consider consolidating error handling for data retrieval.The data retrieval section could benefit from a helper function to reduce repetitive error handling patterns. Consider extracting the project, model, and items retrieval into a helper function.
+func (i Item) retrieveImportData(ctx context.Context, param interfaces.ImportItemsParam) ( + *schema.Schema, + *project.Project, + *model.Model, + item.VersionedList, + item.VersionedList, + error, +) { + s := param.SP.Schema() + prj, err := i.repos.Project.FindByID(ctx, s.Project()) + if err != nil { + return nil, nil, nil, nil, nil, err + } + + m, err := i.repos.Model.FindByID(ctx, param.ModelID) + if err != nil { + return nil, nil, nil, nil, nil, err + } + + itemsIds := lo.FilterMap(param.Items, func(i interfaces.ImportItemParam, _ int) (item.ID, bool) { + if i.ItemId != nil { + return *i.ItemId, true + } + return item.ID{}, false + }) + oldItems, err := i.repos.Item.FindByIDs(ctx, itemsIds, nil) + if err != nil { + return nil, nil, nil, nil, nil, err + } + + metaItemsIds := lo.FilterMap(param.Items, func(i interfaces.ImportItemParam, _ int) (item.ID, bool) { + if i.MetadataID != nil { + return *i.MetadataID, true + } + return item.ID{}, false + }) + oldMetaItems, err := i.repos.Item.FindByIDs(ctx, metaItemsIds, nil) + if err != nil { + return nil, nil, nil, nil, nil, err + } + + return s, prj, m, oldItems, oldMetaItems, nil +}
Line range hint
450-599: Consider breaking down the import logic into smaller functions.The import logic is complex and could benefit from being split into smaller, focused functions for better maintainability and testability. Consider extracting the following operations:
- Strategy handling
- Metadata processing
- Reference field updates
+func (i Item) handleImportStrategy( + param interfaces.ImportItemsParam, + oldItem *item.Item, +) (interfaces.ImportStrategyType, error) { + if param.Strategy == interfaces.ImportStrategyTypeInsert && oldItem != nil { + return param.Strategy, nil + } + if param.Strategy == interfaces.ImportStrategyTypeUpdate && oldItem == nil { + return param.Strategy, nil + } + if param.Strategy == interfaces.ImportStrategyTypeUpsert { + if oldItem != nil { + return interfaces.ImportStrategyTypeUpdate, nil + } + return interfaces.ImportStrategyTypeInsert, nil + } + return param.Strategy, nil +}
604-644: Consider batching event creation.The event creation loop could be optimized by batching the database operations for referenced items.
+func (i Item) createBatchEvents( + ctx context.Context, + items item.VersionedList, + itemsEvent map[item.ID]itemChanges, + prj *project.Project, + s *schema.Schema, + m *model.Model, +) ([]Event, error) { + var events []Event + // Batch fetch all referenced items + allFields := items.Fields() + refItems, err := i.getReferencedItems(ctx, allFields) + if err != nil { + return nil, err + } + + for k, changes := range itemsEvent { + vi := items.Item(k) + it := vi.Value() + + eType := event.ItemCreate + if changes.action == interfaces.ImportStrategyTypeUpdate { + eType = event.ItemUpdate + } + + events = append(events, Event{ + Project: prj, + Workspace: s.Workspace(), + Type: eType, + Object: vi, + WebhookObject: item.ItemModelSchema{ + Item: it, + Model: m, + Schema: s, + GroupSchemas: param.SP.GroupSchemas(), + ReferencedItems: refItems, + Changes: item.CompareFields(it.Fields(), changes.oldFields), + }, + Operator: operator.Operator(), + }) + } + return events, nil +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/internal/usecase/interactor/item.go(3 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
server/internal/usecase/interactor/item.go
602-602: ineffectual assignment to err
(ineffassign)
🪛 GitHub Check: ci-server / lint
server/internal/usecase/interactor/item.go
[failure] 602-602:
ineffectual assignment to err (ineffassign)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: ci-server / test
🔇 Additional comments (2)
server/internal/usecase/interactor/item.go (2)
368-371: LGTM! Proper permission validation.The permission check is correctly placed at the start of the function, ensuring early validation of workspace access rights.
420-448: Enhance schema mutation validation.The schema mutation logic should validate field types more thoroughly and ensure compatibility with existing data.
Run this script to check for potential field type conflicts:
| for k, changes := range itemsEvent { | ||
| vi := items.Item(k) | ||
| it := vi.Value() | ||
| runRes, err := Run1(ctx, operator, i.repos, Usecase().Transaction(), f) |
There was a problem hiding this comment.
Fix ineffectual error assignment.
The error assignment on line 602 is ineffectual as it's immediately overwritten.
-runRes, err := Run1(ctx, operator, i.repos, Usecase().Transaction(), f)
+runRes, runErr := Run1(ctx, operator, i.repos, Usecase().Transaction(), f)📝 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.
| runRes, err := Run1(ctx, operator, i.repos, Usecase().Transaction(), f) | |
| runRes, runErr := Run1(ctx, operator, i.repos, Usecase().Transaction(), f) |
🧰 Tools
🪛 golangci-lint (1.62.2)
602-602: ineffectual assignment to err
(ineffassign)
🪛 GitHub Check: ci-server / lint
[failure] 602-602:
ineffectual assignment to err (ineffassign)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
server/internal/usecase/interactor/item.go (1)
602-605:⚠️ Potential issueFix ineffectual error assignment.
The error assignment is ineffectual as noted by the linter.
Apply this fix:
-runRes, err := Run1(ctx, operator, i.repos, Usecase().Transaction(), f) +runRes, runErr := Run1(ctx, operator, i.repos, Usecase().Transaction(), f) if err != nil { return interfaces.ImportItemsResponse{}, err }
🧹 Nitpick comments (3)
server/internal/usecase/interactor/item.go (3)
374-409: Consider enhancing error handling for database operations.While the error handling is generally good, consider wrapping database errors with more context to aid in debugging:
prj, err := i.repos.Project.FindByID(ctx, s.Project()) if err != nil { - return interfaces.ImportItemsResponse{}, err + return interfaces.ImportItemsResponse{}, fmt.Errorf("failed to find project: %w", err) }
Line range hint
450-599: Enhance error messages for better debugging.Consider adding more descriptive error messages for common failure scenarios during import:
if param.Strategy == interfaces.ImportStrategyTypeUpdate && !operator.CanUpdate(oldItem) { - return interfaces.ImportItemsResponse{}, interfaces.ErrOperationDenied + return interfaces.ImportItemsResponse{}, fmt.Errorf("%w: user cannot update item %s", interfaces.ErrOperationDenied, oldItem.ID()) }
607-649: Consider optimizing event processing.The event handling could be optimized by:
- Batching database queries for referenced items
- Processing events in parallel where possible
Example optimization:
-for k, changes := range itemsEvent { +// Batch fetch all referenced items +allRefItems, err := i.getReferencedItems(ctx, lo.FlatMap(items, func(item item.Versioned) []*item.Field { + return item.Value().Fields() +})) +if err != nil { + return interfaces.ImportItemsResponse{}, err +} + +for k, changes := range itemsEvent {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/internal/usecase/interactor/item.go(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: ci-server / test
🔇 Additional comments (3)
server/internal/usecase/interactor/item.go (3)
368-371: LGTM! Early permission validation is well implemented.The code properly validates workspace permissions before proceeding with the import operation.
411-418: LGTM! Well-structured change tracking implementation.The use of a custom struct to track item changes is a clean and efficient approach.
420-448: Ensure schema mutations are transaction-safe.While the schema mutation logic is thorough, consider adding explicit checks to ensure schema updates are atomic and consistent within the transaction.
Run this script to check for potential transaction issues:
Summary by CodeRabbit