[RISE] - Only fetch categories that needs migration #36
Conversation
WalkthroughThe change refines DynamoDB scan filtering to target legacy category IDs outside the 1-19 range using new constants and OR-like conditions, moving filter logic from in-memory processing to the data source level while maintaining overall control flow. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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: 2
📜 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(1 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
🔇 Additional comments (1)
CategoryMigrationLambda/Services/CategoryMigrationService.cs (1)
22-35: DynamoDB client properly injected.The DynamoDB client is correctly injected via the constructor as an interface, following the coding guidelines. Disposal will be handled by the DI container.
| const int newCategoryMin = 1; | ||
| const int newCategoryMax = 19; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Extract duplicated constants to class level.
The newCategoryMin and newCategoryMax constants are duplicated between the NeedsMigration method (lines 116-117) and here. This violates the DRY principle and creates a maintenance risk if the range ever needs to change.
Define these constants at the class level:
public class CategoryMigrationService : ICategoryMigrationService
{
private readonly IAmazonDynamoDB _dynamoDbClient;
private readonly IDynamoDBContext _dynamoDbContext;
private readonly ILogger<CategoryMigrationService> _logger;
private readonly IConfiguration _configuration;
private readonly string _tableName;
private readonly int _batchSize;
+
+ // New category IDs are in the range 1-19
+ // Legacy category IDs are outside this range (146-157, 390-391, 1001-1018, etc.)
+ private const int NewCategoryMin = 1;
+ private const int NewCategoryMax = 19;Then remove the local constant declarations and use the class-level constants in both methods.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In CategoryMigrationLambda/Services/CategoryMigrationService.cs around lines
139-140 (and also referenced at lines ~116-117), extract the duplicated local
constants newCategoryMin and newCategoryMax to private (or private const)
class-level fields (e.g., private const int NewCategoryMin = 1; private const
int NewCategoryMax = 19;) and replace the local declarations in NeedsMigration
and the current method with references to these class-level constants; ensure
naming follows PascalCase for constants and update both methods to use the
shared constants so the range is defined in one place.
| var scanFilter = new ScanFilter(); | ||
| scanFilter.AddCondition("CategoryId", ScanOperator.IsNotNull); | ||
| // Add OR conditions to find legacy categories that need migration | ||
| // This filters at the DynamoDB level, so we only fetch records that need migration | ||
| scanFilter.AddCondition("CategoryId", ScanOperator.LessThan, newCategoryMin); | ||
| scanFilter.AddCondition("CategoryId", ScanOperator.GreaterThan, newCategoryMax); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify that ScanFilter correctly implements OR logic for multiple conditions on the same attribute.
The code adds two conditions on the same CategoryId attribute, intending to create an OR filter (CategoryId < 1 OR CategoryId > 19). However, in the AWS SDK's Document Model API, multiple AddCondition calls on the same attribute may be combined with AND semantics rather than OR, resulting in an impossible condition (CategoryId < 1 AND CategoryId > 19) that matches no records.
Consider using FilterExpression with explicit OR logic instead:
-var scanFilter = new ScanFilter();
-// Add OR conditions to find legacy categories that need migration
-// This filters at the DynamoDB level, so we only fetch records that need migration
-scanFilter.AddCondition("CategoryId", ScanOperator.LessThan, newCategoryMin);
-scanFilter.AddCondition("CategoryId", ScanOperator.GreaterThan, newCategoryMax);
+var scanConfig = new ScanOperationConfig
+{
+ FilterExpression = "CategoryId < :min OR CategoryId > :max",
+ ExpressionAttributeValues = new Dictionary<string, DynamoDBEntry>
+ {
+ { ":min", newCategoryMin },
+ { ":max", newCategoryMax }
+ }
+};Then update the scan call:
-var search = table.Scan(scanFilter);
+var search = table.Scan(scanConfig);Test the current implementation to verify it returns records with legacy CategoryIds. Run a test migration with logging to confirm that records outside the 1-19 range are actually being retrieved:
🏁 Script executed:
#!/bin/bash
# Description: Check AWS SDK documentation for ScanFilter behavior with multiple conditions on same attribute
# Search for ScanFilter usage patterns in the codebase to see if there are other examples
rg -n "ScanFilter" -A 10 -B 2
# Search for FilterExpression usage to see if it's used elsewhere
rg -n "FilterExpression" -A 5 -B 2Length of output: 1577
Fix ScanFilter logic that incorrectly ANDs conditions instead of ORing them.
Multiple conditions in a ScanFilter map are ANDed together by default, creating an impossible filter (CategoryId < 1 AND CategoryId > 19) that returns no results. Your code comment says "OR conditions" but the implementation does not achieve this.
To OR conditions with ScanFilter, you must use the ConditionalOperator (legacy) parameter, or better yet, switch to FilterExpression for clearer OR semantics:
-var scanFilter = new ScanFilter();
-// Add OR conditions to find legacy categories that need migration
-// This filters at the DynamoDB level, so we only fetch records that need migration
-scanFilter.AddCondition("CategoryId", ScanOperator.LessThan, newCategoryMin);
-scanFilter.AddCondition("CategoryId", ScanOperator.GreaterThan, newCategoryMax);
+var scanConfig = new ScanOperationConfig
+{
+ FilterExpression = "CategoryId < :min OR CategoryId > :max",
+ ExpressionAttributeValues = new Dictionary<string, DynamoDBEntry>
+ {
+ { ":min", newCategoryMin },
+ { ":max", newCategoryMax }
+ }
+};Then update line 151:
-var search = table.Scan(scanFilter);
+var search = table.Scan(scanConfig);Committable suggestion skipped: line range outside the PR's diff.
1 participant
Summary by CodeRabbit