feat(cqrs): project orders into durable read model#121
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a complete event-driven order read model system. It adds four new domain events (OrderCanceledEvent, OrderRejectedEvent, OrderLinkedToPositionEvent) and enhances OrderPlacedEvent with additional context fields, updates OrderLifecycle to emit these events on transitions, replaces a repository-backed read model with a JSON-file-backed JsonOrderReadModel that projects events into a queryable order state, and wires these components into the dependency injection container. Changes
Sequence DiagramsequenceDiagram
participant OL as OrderLifecycle<br/>(Domain)
participant DD as DomainEventDispatcher
participant PH as OrderReadModelProjectionHandler
participant JORM as JsonOrderReadModel<br/>(Persistence)
participant FS as File System
OL->>OL: Submit Order
OL->>DD: Publish OrderPlacedEvent
OL->>OL: Receive Fill
OL->>DD: Publish OrderFilledEvent
OL->>OL: Cancel Order
OL->>DD: Publish OrderCanceledEvent
DD->>PH: Dispatch all events
PH->>JORM: ProjectAsync(OrderPlacedEvent)
JORM->>JORM: Upsert DTO to cache
JORM->>FS: Persist to JSON
PH->>JORM: ProjectAsync(OrderFilledEvent)
JORM->>JORM: Update quantity & status
JORM->>FS: Persist to JSON
PH->>JORM: ProjectAsync(OrderCanceledEvent)
JORM->>JORM: Mark as canceled
JORM->>FS: Persist to JSON
Note over JORM: Read queries now serve<br/>cached, projected state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes The JsonOrderReadModel implementation (531 lines) introduces thread-safe in-memory caching with file-backed persistence, complex event projection logic with state derivation for order fills and cancellations, and multi-method filtering/mapping. Combined with substantial domain logic changes, new event types, comprehensive test coverage, and integration test wiring across multiple test files, this requires careful review of synchronization, event handling edge cases, and persistence correctness. Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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.
Actionable comments posted: 2
🧹 Nitpick comments (4)
tests/IntelliTrader.Application.Tests/Trading/Handlers/ExchangeOrderLifecycleFactoryTests.cs (1)
221-222: Optional: pin the exact event set, not just presence.Two
.Containassertions allow extra events to slip through unnoticed. Tightening the assertion to the expected set guards against regressions where the cancel-after-fill transition starts emitting additional events.♻️ Suggested tightening
- lifecycle.DomainEvents.Should().Contain(e => e is OrderFilledEvent); - lifecycle.DomainEvents.Should().Contain(e => e is OrderCanceledEvent); + lifecycle.DomainEvents.Should().HaveCount(2); + lifecycle.DomainEvents.Should().ContainSingle(e => e is OrderFilledEvent); + lifecycle.DomainEvents.Should().ContainSingle(e => e is OrderCanceledEvent);Also worth confirming the emission order is intentionally Filled-then-Canceled (the sibling test at lines 250–251 still expects only
OrderFilledEventfor an already-canceled lifecycle that later reports executed quantity, which seems consistent — just flagging for awareness).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/IntelliTrader.Application.Tests/Trading/Handlers/ExchangeOrderLifecycleFactoryTests.cs` around lines 221 - 222, Replace the two loose presence assertions on lifecycle.DomainEvents with a precise assertion that the sequence equals the expected events (both content and order) — e.g., assert that lifecycle.DomainEvents.Select(d => d.GetType()) (or map to types) is exactly the sequence [typeof(OrderFilledEvent), typeof(OrderCanceledEvent)] using an equality/equivalence assertion (BeEquivalentTo or Should().Equal depending on style) so no extra events can appear and order is verified; target the DomainEvents collection and the OrderFilledEvent and OrderCanceledEvent types when making the replacement.IntelliTrader.Infrastructure/Adapters/Persistence/ReadModels/JsonOrderReadModel.cs (3)
18-32: Document thatKnownQuoteCurrenciesordering is significant.The match in
ResolveQuoteCurrency(line 489) is aFirstOrDefaultoverEndsWith, so longer/more-specific suffixes must come first (e.g.,USDT/USDC/BUSDbeforeUSD,FDUSDbeforeUSD,BNB/ETH/BTCafter the stablecoin suffixes since the ambiguous "ETH"/"BTC" base case is rare). The current order is correct, but a one-line comment will save a future contributor from re-sorting alphabetically and quietly breaking pair resolution for things likeBTCUSDT→USDT.📝 Suggested comment
- private static readonly string[] KnownQuoteCurrencies = - [ + // Ordering matters: ResolveQuoteCurrency does a longest-suffix match via FirstOrDefault+EndsWith, + // so multi-character stablecoin suffixes (e.g. USDT/FDUSD) must appear before shorter ones (USD). + private static readonly string[] KnownQuoteCurrencies = + [ "USDT",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@IntelliTrader.Infrastructure/Adapters/Persistence/ReadModels/JsonOrderReadModel.cs` around lines 18 - 32, The KnownQuoteCurrencies array ordering is significant because ResolveQuoteCurrency uses FirstOrDefault with EndsWith to pick the first matching suffix; update the KnownQuoteCurrencies declaration to include a one-line comment above the array explaining that entries must be ordered from longest/most-specific to shortest to avoid incorrect matches (e.g., USDT/USDC/BUSD/FDUSD before USD), and reference ResolveQuoteCurrency in the comment so future contributors understand why the ordering must not be alphabetized or changed.
437-441: Readability nit: parenthesize the mixedis/==/&&/||expression.Operator precedence makes the current expression correct (
||binds looser than&&), but mixing pattern-matchingis … or …with==/&&/||on the same line is easy to misread during future edits. A small refactor pays off in clarity.♻️ Suggested readability tweak
- return status is OrderLifecycleStatus.PartiallyFilled or OrderLifecycleStatus.Filled || - status == OrderLifecycleStatus.Canceled && filledQuantity > 0m; + return status is OrderLifecycleStatus.PartiallyFilled or OrderLifecycleStatus.Filled + || (status == OrderLifecycleStatus.Canceled && filledQuantity > 0m);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@IntelliTrader.Infrastructure/Adapters/Persistence/ReadModels/JsonOrderReadModel.cs` around lines 437 - 441, The expression in CanAffectPosition mixes pattern-matching ("is ... or ...") with ==, && and || which is correct but hard to read; refactor by adding parentheses to make intent explicit—e.g., group the filled status check together and parenthesize the canceled-with-fill check (so it's clear the canceled check is combined with filledQuantity > 0m) or rewrite the pattern into explicit comparisons—update the return expression in CanAffectPosition accordingly to improve readability.
338-361: Harden cache load against malformed JSON and duplicate ids.
JsonSerializer.Deserializewill throw on a corrupted/partial file (very possible given the non-atomic write above), anddtos.ToDictionary(dto => dto.Id, …)will throwArgumentExceptionif the file ever ends up with two entries sharing anId(case-insensitively). Either failure surfaces on the very first read/projection call after restart and prevents the read model from recovering.Consider catching
JsonException(andArgumentExceptionfrom the dictionary build) and degrading gracefully — e.g., logging, optionally backing up the bad file, and starting from an empty cache so subsequent projections heal the state.♻️ Suggested resiliency around load
- var dtos = JsonSerializer.Deserialize<List<OrderReadModelDto>>(json, _jsonOptions) - ?? new List<OrderReadModelDto>(); - _cache = new ConcurrentDictionary<string, OrderReadModelDto>( - dtos.ToDictionary(dto => dto.Id, StringComparer.OrdinalIgnoreCase), - StringComparer.OrdinalIgnoreCase); - _cacheLoaded = true; + List<OrderReadModelDto> dtos; + try + { + dtos = JsonSerializer.Deserialize<List<OrderReadModelDto>>(json, _jsonOptions) + ?? new List<OrderReadModelDto>(); + } + catch (JsonException) + { + // Corrupt or partially-written file; start from empty and let projections heal. + dtos = new List<OrderReadModelDto>(); + } + + var dict = new ConcurrentDictionary<string, OrderReadModelDto>(StringComparer.OrdinalIgnoreCase); + foreach (var dto in dtos) + { + if (!string.IsNullOrEmpty(dto.Id)) + { + dict[dto.Id] = dto; // last-write-wins instead of throwing on duplicates + } + } + _cache = dict; + _cacheLoaded = true;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@IntelliTrader.Infrastructure/Adapters/Persistence/ReadModels/JsonOrderReadModel.cs` around lines 338 - 361, LoadCacheUnlockedAsync should be hardened against malformed JSON and duplicate ids: wrap the JsonSerializer.Deserialize and the dtos.ToDictionary logic in a try/catch that catches JsonException, ArgumentException (and optionally IOException), logs the error (include the problematic _filePath and exception), moves/backs up the bad file (e.g., rename to a .bad timestamped copy) and then initialize _cache = new ConcurrentDictionary<string, OrderReadModelDto>(StringComparer.OrdinalIgnoreCase) and set _cacheLoaded = true so the read model can recover; alternatively when building the dictionary avoid ToDictionary (which throws on duplicates) by grouping dtos by dto.Id with StringComparer.OrdinalIgnoreCase and choosing a deterministic survivor (e.g., Last()) before creating the ConcurrentDictionary, and keep JsonSerializer.Deserialize, _filePath, _cache, _cacheLoaded and _jsonOptions as the referenced symbols to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@IntelliTrader.Infrastructure/Adapters/Persistence/ReadModels/JsonOrderReadModel.cs`:
- Around line 237-248: ProjectAsync quietly drops OrderLinkedToPositionEvent
when the DTO is missing; change it to defensively upsert a minimal DTO instead
of doing nothing: inside ProjectAsync (the lambda passed to MutateAsync) detect
when cache.TryGetValue(domainEvent.OrderId, out var dto) is false and create a
new minimal OrderReadModel/DTO with OrderId = domainEvent.OrderId and
RelatedPositionId = domainEvent.PositionId (and any required key defaults
similar to ProjectTerminalStatusAsync), then add it to the cache so later events
can fill remaining fields; preserve the existing path when dto exists and keep
using MutateAsync with the cancellationToken.
- Around line 363-376: PersistCacheUnlockedAsync currently writes directly to
_filePath which can leave a truncated/partial file on crash; change it to write
atomically by serializing dtos to a sibling temp file (e.g., same directory +
unique suffix), flush and close that temp file, then if the destination exists
call File.Replace(tempPath, _filePath, null) to atomically swap, otherwise call
File.Move(tempPath, _filePath); keep this logic inside PersistCacheUnlockedAsync
and ensure any exceptions clean up the temp file so LoadCacheUnlockedAsync
always reads a fully-written JSON file.
---
Nitpick comments:
In
`@IntelliTrader.Infrastructure/Adapters/Persistence/ReadModels/JsonOrderReadModel.cs`:
- Around line 18-32: The KnownQuoteCurrencies array ordering is significant
because ResolveQuoteCurrency uses FirstOrDefault with EndsWith to pick the first
matching suffix; update the KnownQuoteCurrencies declaration to include a
one-line comment above the array explaining that entries must be ordered from
longest/most-specific to shortest to avoid incorrect matches (e.g.,
USDT/USDC/BUSD/FDUSD before USD), and reference ResolveQuoteCurrency in the
comment so future contributors understand why the ordering must not be
alphabetized or changed.
- Around line 437-441: The expression in CanAffectPosition mixes
pattern-matching ("is ... or ...") with ==, && and || which is correct but hard
to read; refactor by adding parentheses to make intent explicit—e.g., group the
filled status check together and parenthesize the canceled-with-fill check (so
it's clear the canceled check is combined with filledQuantity > 0m) or rewrite
the pattern into explicit comparisons—update the return expression in
CanAffectPosition accordingly to improve readability.
- Around line 338-361: LoadCacheUnlockedAsync should be hardened against
malformed JSON and duplicate ids: wrap the JsonSerializer.Deserialize and the
dtos.ToDictionary logic in a try/catch that catches JsonException,
ArgumentException (and optionally IOException), logs the error (include the
problematic _filePath and exception), moves/backs up the bad file (e.g., rename
to a .bad timestamped copy) and then initialize _cache = new
ConcurrentDictionary<string,
OrderReadModelDto>(StringComparer.OrdinalIgnoreCase) and set _cacheLoaded = true
so the read model can recover; alternatively when building the dictionary avoid
ToDictionary (which throws on duplicates) by grouping dtos by dto.Id with
StringComparer.OrdinalIgnoreCase and choosing a deterministic survivor (e.g.,
Last()) before creating the ConcurrentDictionary, and keep
JsonSerializer.Deserialize, _filePath, _cache, _cacheLoaded and _jsonOptions as
the referenced symbols to locate the change.
In
`@tests/IntelliTrader.Application.Tests/Trading/Handlers/ExchangeOrderLifecycleFactoryTests.cs`:
- Around line 221-222: Replace the two loose presence assertions on
lifecycle.DomainEvents with a precise assertion that the sequence equals the
expected events (both content and order) — e.g., assert that
lifecycle.DomainEvents.Select(d => d.GetType()) (or map to types) is exactly the
sequence [typeof(OrderFilledEvent), typeof(OrderCanceledEvent)] using an
equality/equivalence assertion (BeEquivalentTo or Should().Equal depending on
style) so no extra events can appear and order is verified; target the
DomainEvents collection and the OrderFilledEvent and OrderCanceledEvent types
when making the replacement.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b2b411eb-3740-495f-83f0-21467fff704a
📒 Files selected for processing (19)
IntelliTrader.Domain/Events/OrderCanceledEvent.csIntelliTrader.Domain/Events/OrderLinkedToPositionEvent.csIntelliTrader.Domain/Events/OrderPlacedEvent.csIntelliTrader.Domain/Events/OrderRejectedEvent.csIntelliTrader.Domain/Trading/Orders/OrderLifecycle.csIntelliTrader.Infrastructure/Adapters/Persistence/ReadModels/IOrderReadModelProjectionWriter.csIntelliTrader.Infrastructure/Adapters/Persistence/ReadModels/JsonOrderReadModel.csIntelliTrader.Infrastructure/Adapters/Persistence/ReadModels/OrderReadModelProjectionHandler.csIntelliTrader.Infrastructure/Adapters/Persistence/ReadModels/RepositoryOrderReadModel.csIntelliTrader.Infrastructure/AppModule.cstests/IntelliTrader.Application.Tests/Trading/Handlers/ExchangeOrderLifecycleFactoryTests.cstests/IntelliTrader.Application.Tests/Trading/Handlers/RefreshOrderStatusHandlerTests.cstests/IntelliTrader.Infrastructure.Tests/Adapters/Persistence/OrderReadModelProjectionTests.cstests/IntelliTrader.Infrastructure.Tests/AssemblyInfo.cstests/IntelliTrader.Infrastructure.Tests/Integration/InfrastructureTestFixture.cstests/IntelliTrader.Infrastructure.Tests/Integration/OpenPositionCommandDispatchIntegrationTests.cstests/IntelliTrader.Infrastructure.Tests/Integration/OrderLifecycleCommandDispatchIntegrationTests.cstests/IntelliTrader.Infrastructure.Tests/Integration/RefreshOrderStatusCommandDispatchIntegrationTests.cstests/IntelliTrader.Infrastructure.Tests/Integration/RefreshSubmittedOrdersCommandDispatchIntegrationTests.cs
💤 Files with no reviewable changes (1)
- IntelliTrader.Infrastructure/Adapters/Persistence/ReadModels/RepositoryOrderReadModel.cs
| public Task ProjectAsync(OrderLinkedToPositionEvent domainEvent, CancellationToken cancellationToken = default) | ||
| { | ||
| ArgumentNullException.ThrowIfNull(domainEvent); | ||
|
|
||
| return MutateAsync(cache => | ||
| { | ||
| if (cache.TryGetValue(domainEvent.OrderId, out var dto)) | ||
| { | ||
| dto.RelatedPositionId = domainEvent.PositionId; | ||
| } | ||
| }, cancellationToken); | ||
| } |
There was a problem hiding this comment.
OrderLinkedToPositionEvent is silently dropped if the DTO is missing.
If the link event is ever processed before its corresponding OrderPlacedEvent (recovery/replay scenarios, out-of-order delivery, or projection lag), the else branch is implicit and the position link is permanently lost — the order will never appear in GetTradingHistoryAsync because the filter requires RelatedPositionId to be non-empty. Consider creating a minimal DTO when missing, mirroring the pattern in ProjectTerminalStatusAsync, so the link still lands and later events fill in the rest.
🛡️ Suggested defensive upsert
return MutateAsync(cache =>
{
- if (cache.TryGetValue(domainEvent.OrderId, out var dto))
- {
- dto.RelatedPositionId = domainEvent.PositionId;
- }
+ var dto = cache.GetValueOrDefault(domainEvent.OrderId) ?? new OrderReadModelDto
+ {
+ Id = domainEvent.OrderId,
+ SubmittedAt = domainEvent.OccurredAt
+ };
+ dto.RelatedPositionId = domainEvent.PositionId;
+ cache[domainEvent.OrderId] = dto;
}, cancellationToken);📝 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.
| public Task ProjectAsync(OrderLinkedToPositionEvent domainEvent, CancellationToken cancellationToken = default) | |
| { | |
| ArgumentNullException.ThrowIfNull(domainEvent); | |
| return MutateAsync(cache => | |
| { | |
| if (cache.TryGetValue(domainEvent.OrderId, out var dto)) | |
| { | |
| dto.RelatedPositionId = domainEvent.PositionId; | |
| } | |
| }, cancellationToken); | |
| } | |
| public Task ProjectAsync(OrderLinkedToPositionEvent domainEvent, CancellationToken cancellationToken = default) | |
| { | |
| ArgumentNullException.ThrowIfNull(domainEvent); | |
| return MutateAsync(cache => | |
| { | |
| var dto = cache.GetValueOrDefault(domainEvent.OrderId) ?? new OrderReadModelDto | |
| { | |
| Id = domainEvent.OrderId, | |
| SubmittedAt = domainEvent.OccurredAt | |
| }; | |
| dto.RelatedPositionId = domainEvent.PositionId; | |
| cache[domainEvent.OrderId] = dto; | |
| }, cancellationToken); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@IntelliTrader.Infrastructure/Adapters/Persistence/ReadModels/JsonOrderReadModel.cs`
around lines 237 - 248, ProjectAsync quietly drops OrderLinkedToPositionEvent
when the DTO is missing; change it to defensively upsert a minimal DTO instead
of doing nothing: inside ProjectAsync (the lambda passed to MutateAsync) detect
when cache.TryGetValue(domainEvent.OrderId, out var dto) is false and create a
new minimal OrderReadModel/DTO with OrderId = domainEvent.OrderId and
RelatedPositionId = domainEvent.PositionId (and any required key defaults
similar to ProjectTerminalStatusAsync), then add it to the cache so later events
can fill remaining fields; preserve the existing path when dto exists and keep
using MutateAsync with the cancellationToken.
| private async Task PersistCacheUnlockedAsync(CancellationToken cancellationToken) | ||
| { | ||
| var directory = Path.GetDirectoryName(_filePath); | ||
| if (!string.IsNullOrEmpty(directory) && !Directory.Exists(directory)) | ||
| { | ||
| Directory.CreateDirectory(directory); | ||
| } | ||
|
|
||
| var dtos = _cache.Values | ||
| .OrderByDescending(dto => dto.SubmittedAt) | ||
| .ToList(); | ||
| var json = JsonSerializer.Serialize(dtos, _jsonOptions); | ||
| await File.WriteAllTextAsync(_filePath, json, cancellationToken).ConfigureAwait(false); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
File.Replace vs File.Move atomic file replacement semantics on Linux and Windows .NET
💡 Result:
File.Replace provides atomic file replacement semantics on both Linux and Windows in .NET, while File.Move does not guarantee atomic replacement when overwriting. File.Replace replaces the destination file with the source file atomically, creating a backup if specified. It requires the destination to exist and source/destination on the same volume. On Windows, it uses Win32 ReplaceFile, which performs an atomic swap of file IDs on NTFS, ensuring no intermediate state where the destination is missing (though not contractually documented as atomic, it's the recommended replacement for transactional operations). On Linux/Unix, it uses POSIX rename(2) after checks (stat destination if no backup, link/copy for backup), which is atomic: the destination remains accessible, switching instantly from old to new content. File.Move (with overwrite=true, .NET Core 3.0+) primarily uses rename on Unix (atomic within filesystem) or MoveFileEx on Windows (atomic on same-volume NTFS with REPLACE_EXISTING). However, cross-volume it falls back to copy+delete, which is non-atomic (partial writes possible on crash). File.Move also works when destination doesn't exist, unlike Replace. Key differences: - Replace: Requires destination exists; always atomic replacement (fails cross-volume); backup support. - Move: Handles non-existing destination; non-atomic cross-volume; no backup. For atomic replacement assuming destination exists and same volume, prefer File.Replace(null backup for no backup). Use if (!File.Exists(dest)) File.Move else File.Replace for full POSIX rename semantics.
Citations:
- 1: Implement File.Replace() as a documented safe atomic API dotnet/runtime#18034
- 2: https://learn.microsoft.com/en-us/dotnet/api/system.io.file.replace?view=net-9.0
- 3: https://learn.microsoft.com/en-us/dotnet/api/system.io.file.move?view=net-9.0
- 4: File move operation with semantics of Unix rename dotnet/runtime#22157
- 5: https://zenn.dev/arika/articles/20251010-csharp-replace-is-atomic?locale=en
- 6: File.Unix: Replace: increase Windows compatibility dotnet/runtime#50234
- 7: https://stackoverflow.com/questions/34973323/how-do-i-rename-a-file-over-an-existing-file-in-net-without-deleting-the-desitn
- 8: File.Move relies on the UnixFileSystem CopyFile method that overwrites only when it shouldn't dotnet/runtime#14885
- 9: Make File.Move(..., ..., overwrite: true) more efficient on Unix. dotnet/runtime#47118
- 10: Remove race conditions from File.Move and Directory.Move dotnet/runtime#33370
- 11: https://stackoverflow.com/questions/5989987/difference-between-file-replace-and-file-deletefile-move-in-c-sharp
Persisting via a single File.WriteAllTextAsync is not crash-safe — write atomically.
This is the durability point of the new "durable read-side projection." File.WriteAllTextAsync truncates and rewrites the destination in place, so a process crash, host kill, or power loss mid-write can leave order-read-model.json truncated or partially written. On the next start, LoadCacheUnlockedAsync will then either deserialize a partial document and throw, or silently lose every projected order recorded since the last successful flush.
Given every projection mutation calls this on the hot path, the exposure is non-trivial. Write to a sibling temp file and atomically replace using File.Replace (when destination exists) or File.Move (when it doesn't).
🛡️ Suggested atomic-write pattern
private async Task PersistCacheUnlockedAsync(CancellationToken cancellationToken)
{
var directory = Path.GetDirectoryName(_filePath);
if (!string.IsNullOrEmpty(directory) && !Directory.Exists(directory))
{
Directory.CreateDirectory(directory);
}
var dtos = _cache.Values
.OrderByDescending(dto => dto.SubmittedAt)
.ToList();
var json = JsonSerializer.Serialize(dtos, _jsonOptions);
- await File.WriteAllTextAsync(_filePath, json, cancellationToken).ConfigureAwait(false);
+
+ var tempPath = _filePath + ".tmp";
+ await File.WriteAllTextAsync(tempPath, json, cancellationToken).ConfigureAwait(false);
+ if (File.Exists(_filePath))
+ {
+ File.Replace(tempPath, _filePath, destinationBackupFileName: null);
+ }
+ else
+ {
+ File.Move(tempPath, _filePath);
+ }
}📝 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.
| private async Task PersistCacheUnlockedAsync(CancellationToken cancellationToken) | |
| { | |
| var directory = Path.GetDirectoryName(_filePath); | |
| if (!string.IsNullOrEmpty(directory) && !Directory.Exists(directory)) | |
| { | |
| Directory.CreateDirectory(directory); | |
| } | |
| var dtos = _cache.Values | |
| .OrderByDescending(dto => dto.SubmittedAt) | |
| .ToList(); | |
| var json = JsonSerializer.Serialize(dtos, _jsonOptions); | |
| await File.WriteAllTextAsync(_filePath, json, cancellationToken).ConfigureAwait(false); | |
| } | |
| private async Task PersistCacheUnlockedAsync(CancellationToken cancellationToken) | |
| { | |
| var directory = Path.GetDirectoryName(_filePath); | |
| if (!string.IsNullOrEmpty(directory) && !Directory.Exists(directory)) | |
| { | |
| Directory.CreateDirectory(directory); | |
| } | |
| var dtos = _cache.Values | |
| .OrderByDescending(dto => dto.SubmittedAt) | |
| .ToList(); | |
| var json = JsonSerializer.Serialize(dtos, _jsonOptions); | |
| var tempPath = _filePath + ".tmp"; | |
| await File.WriteAllTextAsync(tempPath, json, cancellationToken).ConfigureAwait(false); | |
| if (File.Exists(_filePath)) | |
| { | |
| File.Replace(tempPath, _filePath, destinationBackupFileName: null); | |
| } | |
| else | |
| { | |
| File.Move(tempPath, _filePath); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@IntelliTrader.Infrastructure/Adapters/Persistence/ReadModels/JsonOrderReadModel.cs`
around lines 363 - 376, PersistCacheUnlockedAsync currently writes directly to
_filePath which can leave a truncated/partial file on crash; change it to write
atomically by serializing dtos to a sibling temp file (e.g., same directory +
unique suffix), flush and close that temp file, then if the destination exists
call File.Replace(tempPath, _filePath, null) to atomically swap, otherwise call
File.Move(tempPath, _filePath); keep this logic inside PersistCacheUnlockedAsync
and ensure any exceptions clean up the temp file so LoadCacheUnlockedAsync
always reads a fully-written JSON file.
1 participant
Summary
Acceptance Criteria
Changes
JsonOrderReadModelplus projection writer and domain event handler.OrderCanceledEvent,OrderRejectedEvent, andOrderLinkedToPositionEvent; enrichedOrderPlacedEventwith quote currency, intent, and related position metadata.RepositoryOrderReadModeladapter and registered the projected read model inAppModule.Testing
dotnet test tests/IntelliTrader.Infrastructure.Tests/IntelliTrader.Infrastructure.Tests.csproj --no-restore -c Release --filter "FullyQualifiedName~OpenPositionCommandDispatchIntegrationTests|FullyQualifiedName~OrderLifecycleCommandDispatchIntegrationTests|FullyQualifiedName~RefreshOrderStatusCommandDispatchIntegrationTests|FullyQualifiedName~RefreshSubmittedOrdersCommandDispatchIntegrationTests|FullyQualifiedName~OrderReadModelProjectionTests"dotnet test tests/IntelliTrader.Infrastructure.Tests/IntelliTrader.Infrastructure.Tests.csproj --no-restore -c Releasedotnet test tests/IntelliTrader.Application.Tests/IntelliTrader.Application.Tests.csproj --no-restore -c Release --filter "FullyQualifiedName~ExchangeOrderLifecycleFactoryTests|FullyQualifiedName~RefreshOrderStatusHandlerTests"dotnet test IntelliTrader.sln --no-restore -c Releasegit diff --checkChecklist
Summary by CodeRabbit
New Features
Improvements