[RISE-3309] - Split migrations #35
Conversation
WalkthroughRefactored migration logic to support split category scenarios where one source category maps to multiple target categories. Migration now uses grouped mapping approach, updates the primary category on the original document, and creates new preference documents for additional target categories with batch writing. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![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 (1)
CategoryMigrationLambda/Services/CategoryMigrationService.cs (1)
334-389: LGTM! New document creation logic is well-structured.The method correctly clones the original document, generates a new unique EntityId, updates the necessary key fields (SK, Gsi1SK), and sets appropriate timestamps. The defensive check for
IsPrimary(lines 380-383) is good practice.Optional: Consider more explicit SK parsing.
Line 355 uses string replacement to update the EntityId in the SK. While this should be safe given the documented format
ENTITYID#{EntityId}and GUID-based EntityIds, you could make it more explicit:// More explicit parsing (optional) if (originalDocument.ContainsKey("SK")) { newDocument["SK"] = $"ENTITYID#{newEntityId}"; }This assumes the SK format is always exactly
ENTITYID#{EntityId}without additional segments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
CategoryMigrationLambda/Services/CategoryMigrationService.cs(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cs,cshtml}
⚙️ CodeRabbit configuration file
**/*.{cs,cshtml}: For any public or protected asynchronous method name that does not end in "Async", suggest changing the name to end in "Async".
Recommend using "await" instead of "Result" when calling asynchronous methods.When HttpClient is used, verify that it is injected and not created manually. When HttpRequestMessage or HttpResponseMessage is used, verify that they are disposed correctly.
When DynamoDbClient is used, verify that it is injected or created by IDataClientFactory and not created manually. Also verify that the DynamoDbClient is disposed correctly since it implements IDisposable.
Files:
CategoryMigrationLambda/Services/CategoryMigrationService.cs
🧬 Code graph analysis (1)
CategoryMigrationLambda/Services/CategoryMigrationService.cs (1)
CategoryMigrationLambda/Services/CategoryMappings.cs (1)
CategoryMappings(6-77)
🔇 Additional comments (5)
CategoryMigrationLambda/Services/CategoryMigrationService.cs (5)
210-218: LGTM! Good defensive check for empty migrations.The code appropriately handles the case where no valid migration mappings are found by logging a warning and skipping the document.
220-246: LGTM! Primary category migration logic is correct.The code correctly uses the first grouped migration as the primary category to update the existing preference document, with appropriate logging.
402-404: LGTM! Timestamp update is appropriate.Adding the
UpdatedAttimestamp ensures proper tracking of when the migration was performed on each document.
407-479: LGTM! Grouped migration logic is well-designed.The method correctly groups mappings by target category, handles null mappings appropriately (lines 438-442), and ensures clean data by deduplicating and sorting subcategories (lines 472-476). The fallback to category-only mapping when no subcategory mappings are found is a good safeguard.
481-490: LGTM! Backward compatibility maintained.The refactoring correctly delegates to the grouped method while maintaining the original return signature by returning the first (primary) category mapping.
| // Handle additional categories (create new preferences) | ||
| if (groupedMigrations.Count > 1) | ||
| { | ||
| var additionalCategories = groupedMigrations.Skip(1); | ||
|
|
||
| foreach (var additionalCategory in additionalCategories) | ||
| { | ||
| var newCategoryId = additionalCategory.Key; | ||
| var newSubcategoryIds = additionalCategory.Value; | ||
|
|
||
| if (!result.DryRun) | ||
| { | ||
| // Create a new document (preference) for this additional category | ||
| var newDocument = CreateNewPreferenceDocument(document, newCategoryId, newSubcategoryIds); | ||
| batch.Add(newDocument); | ||
| } | ||
|
|
||
| result.MigratedCount++; | ||
|
|
||
| // Log the new preference creation | ||
| var newEntityId = !result.DryRun ? document["EntityId"].AsString() + "_split_" + newCategoryId : "(dry-run)"; | ||
| if (userContext != null) | ||
| { | ||
| _logger.LogInformation("Created NEW preference {NewEntityId} for user {UserContext} from {OriginalEntityId}: CategoryId -> {NewCategoryId}, SubcategoryIds -> [{NewSubcategoryIds}]", | ||
| newEntityId, userContext, preference.EntityId, newCategoryId, | ||
| string.Join(",", newSubcategoryIds)); | ||
| } | ||
| else | ||
| { | ||
| _logger.LogInformation("Created NEW preference {NewEntityId} from {OriginalEntityId}: CategoryId -> {NewCategoryId}, SubcategoryIds -> [{NewSubcategoryIds}]", | ||
| newEntityId, preference.EntityId, newCategoryId, | ||
| string.Join(",", newSubcategoryIds)); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix incorrect EntityId in log messages.
The logged newEntityId on line 268 doesn't match the actual EntityId created in CreateNewPreferenceDocument. Line 268 constructs a descriptive string (originalId_split_categoryId), but CreateNewPreferenceDocument generates a completely new GUID (line 346). This makes the logs misleading and could hinder debugging.
Apply this diff to log the actual EntityId:
if (!result.DryRun)
{
// Create a new document (preference) for this additional category
var newDocument = CreateNewPreferenceDocument(document, newCategoryId, newSubcategoryIds);
batch.Add(newDocument);
+ var newEntityId = newDocument["EntityId"].AsString();
- var newEntityId = !result.DryRun ? document["EntityId"].AsString() + "_split_" + newCategoryId : "(dry-run)";
if (userContext != null)
{
_logger.LogInformation("Created NEW preference {NewEntityId} for user {UserContext} from {OriginalEntityId}: CategoryId -> {NewCategoryId}, SubcategoryIds -> [{NewSubcategoryIds}]",
newEntityId, userContext, preference.EntityId, newCategoryId,
string.Join(",", newSubcategoryIds));
}
else
{
_logger.LogInformation("Created NEW preference {NewEntityId} from {OriginalEntityId}: CategoryId -> {NewCategoryId}, SubcategoryIds -> [{NewSubcategoryIds}]",
newEntityId, preference.EntityId, newCategoryId,
string.Join(",", newSubcategoryIds));
}
+ }
+ else
+ {
+ var newEntityId = "(dry-run)";
+ if (userContext != null)
+ {
+ _logger.LogInformation("Would create NEW preference for user {UserContext} from {OriginalEntityId}: CategoryId -> {NewCategoryId}, SubcategoryIds -> [{NewSubcategoryIds}]",
+ userContext, preference.EntityId, newCategoryId,
+ string.Join(",", newSubcategoryIds));
+ }
+ else
+ {
+ _logger.LogInformation("Would create NEW preference from {OriginalEntityId}: CategoryId -> {NewCategoryId}, SubcategoryIds -> [{NewSubcategoryIds}]",
+ preference.EntityId, newCategoryId,
+ string.Join(",", newSubcategoryIds));
+ }
}Committable suggestion skipped: line range outside the PR's diff.
1 participant
Summary by CodeRabbit