Fix data mapping regression and increase import timeout#34
Conversation
|
Warning Review limit reached
Next review available in: 22 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR splits the single ChangesAppDbContext/DataDbContext split
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Program
participant DataDbContext
participant DbInitializer
participant HierarchySeed
participant ChartStringSegmentSeed
Program->>DataDbContext: register with resolved data connection
Program->>DbInitializer: construct with AppDbContext and DataDbContext
DbInitializer->>HierarchySeed: EnsureSeededAsync(DataDbContext)
DbInitializer->>ChartStringSegmentSeed: EnsureSeededAsync(DataDbContext)
HierarchySeed-->>DataDbContext: save hierarchy rows
ChartStringSegmentSeed-->>DataDbContext: save chart string segments
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
server.core/Data/DataDbContext.cs (1)
39-61: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winNaming-convention-based
MaxLengthsizing is fragile for future properties.
ConfigureHierarchyderives max length purely from property name suffix (Description→ 1000, ends-withName→ 1000, else → 20). Any future string property on a hierarchy entity that isn't namedDescription/*Name(e.g. aParentCode,ShortCode, or similar lookup key) silently falls into the 20-char default, which could truncate data or mismatch a relatedCodecolumn (already explicitly sized at 50). Since this configuration is convention-driven rather than explicit per-property, it's easy for a new column to get an unintended narrow length without anyone noticing at compile time.Consider making the length explicit per-property (or adding an allow-list/deny-list with a fail-fast default) rather than relying on naming conventions to select the width.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server.core/Data/DataDbContext.cs` around lines 39 - 61, The MaxLength setup in ConfigureHierarchy is too dependent on property-name conventions and can silently assign the wrong size to future string properties. Update the string-property configuration in ConfigureHierarchy<T> to use explicit per-property sizing (or a strict allow-list with a fail-fast default) instead of the current switch on property.Name, and make sure known fields like ISegmentHierarchy.Code, Description, and any other hierarchy lookup/code fields are intentionally sized.server.core/Import/PgmProjectsImportService.cs (1)
16-16: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated 600s timeout constant across import services.
CommandTimeoutSecondshere andDatabaseCommandTimeoutSecondsinFlatFileImportServiceboth hardcode 600s independently. Consider a shared constant (e.g., onDataDbConnection) if these are meant to stay in sync.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server.core/Import/PgmProjectsImportService.cs` at line 16, The 600-second command timeout is duplicated across import services, so update PgmProjectsImportService to stop hardcoding its own timeout and instead reuse a shared timeout source used by FlatFileImportService. Move the timeout value to a common symbol such as DataDbConnection (or another shared constant location) and have both CommandTimeoutSeconds and DatabaseCommandTimeoutSeconds reference that single definition so they stay in sync.tests/server.tests/Import/FlatFileImportServiceTests.cs (1)
451-461: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
dataDbis never disposed inCreateService.Every call creates a new InMemory
DataDbContextthat's never released, unlike thedbcontext in each test method (await using var db = ...). With ~15+ call sites in this file, these accumulate for the life of the test process.♻️ Proposed fix
- private static FlatFileImportService CreateService(Server.Core.Data.AppDbContext db) + private static FlatFileImportService CreateService(Server.Core.Data.AppDbContext db, out DataDbContext dataDb) { - var dataDb = TestDbContextFactory.CreateDataInMemory(); + dataDb = TestDbContextFactory.CreateDataInMemory(); return new FlatFileImportService( db, dataDb, new FlatFileImportRegistry(), new ConfigurationBuilder().Build(), NullLogger<FlatFileImportService>.Instance); }Then update each call site to capture and dispose
dataDb(e.g.await using var dataDb; var service = CreateService(db, out dataDb);).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/server.tests/Import/FlatFileImportServiceTests.cs` around lines 451 - 461, CreateService currently hides a disposable DataDbContext by constructing dataDb internally and never releasing it, so refactor FlatFileImportServiceTests to make disposal explicit. Update CreateService to take an out/returned dataDb (or otherwise expose the created context), and adjust each test call site that uses CreateService so it captures dataDb and disposes it with await using alongside db. Keep the changes centered around CreateService and the FlatFileImportService test helpers so every in-memory context created in this file is cleaned up.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@server.core/Data/DataDbContext.cs`:
- Around line 39-61: The MaxLength setup in ConfigureHierarchy is too dependent
on property-name conventions and can silently assign the wrong size to future
string properties. Update the string-property configuration in
ConfigureHierarchy<T> to use explicit per-property sizing (or a strict
allow-list with a fail-fast default) instead of the current switch on
property.Name, and make sure known fields like ISegmentHierarchy.Code,
Description, and any other hierarchy lookup/code fields are intentionally sized.
In `@server.core/Import/PgmProjectsImportService.cs`:
- Line 16: The 600-second command timeout is duplicated across import services,
so update PgmProjectsImportService to stop hardcoding its own timeout and
instead reuse a shared timeout source used by FlatFileImportService. Move the
timeout value to a common symbol such as DataDbConnection (or another shared
constant location) and have both CommandTimeoutSeconds and
DatabaseCommandTimeoutSeconds reference that single definition so they stay in
sync.
In `@tests/server.tests/Import/FlatFileImportServiceTests.cs`:
- Around line 451-461: CreateService currently hides a disposable DataDbContext
by constructing dataDb internally and never releasing it, so refactor
FlatFileImportServiceTests to make disposal explicit. Update CreateService to
take an out/returned dataDb (or otherwise expose the created context), and
adjust each test call site that uses CreateService so it captures dataDb and
disposes it with await using alongside db. Keep the changes centered around
CreateService and the FlatFileImportService test helpers so every in-memory
context created in this file is cleaned up.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e604f530-0c4f-44ad-848e-ac9089eed5d2
📒 Files selected for processing (17)
server.core/Data/AppDbContext.csserver.core/Data/ChartStringSegmentSeed.csserver.core/Data/DataDbContext.csserver.core/Data/DbInitializer.csserver.core/Data/HierarchySeed.csserver.core/Import/PgmProjectsImportService.csserver.core/createMigration.shserver/Controllers/ChartStringSegmentsController.csserver/Import/FlatFileImportService.csserver/Program.cstests/server.tests/ChartStringSegments/ChartStringSegmentMappingTests.cstests/server.tests/ChartStringSegments/ChartStringSegmentSeedTests.cstests/server.tests/ChartStringSegments/ChartStringSegmentsControllerTests.cstests/server.tests/ChartStringSegments/HierarchyMappingTests.cstests/server.tests/ChartStringSegments/HierarchySeedTests.cstests/server.tests/Import/FlatFileImportServiceTests.cstests/server.tests/TestDbContextFactory.cs
💤 Files with no reviewable changes (1)
- server.core/Data/AppDbContext.cs
PR #34 split the data schema entities into DataDbContext, so the PurposeHierarchy DbSet, mapping, seeds, and controller lookup follow. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Tests