Conversation
Replace Console.WriteLine with structured logging and add robust concurrency handling to SavePlayer. Introduces GetLastModifiedTimestamps to fetch last-modified timestamps, a retry loop (max 5 attempts) for DbUpdateConcurrencyException, per-entity diff logging, normalization of CreationTime, and suppression of noisy JSON property diffs via IsJsonStructurallyEqual. Adds helper methods (FormatValue, JsonNoiseProperties) and necessary using directives for EF Core change tracking and System.Text.Json. These changes improve diagnostics and reliability when saving player/account/unlock entities.
Change LastModified in Account.cs and Character.cs from mutable setters to init-only properties (get; init;) so the timestamp can only be set during object initialization. This enforces immutability after construction and helps prevent accidental updates to the LastModified field.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughChanged LastModified properties from mutable to init-only in Account and Character models. Enhanced SavePlayer method with concurrency-resilient retry logic, JSON diff suppression, and improved logging. Added GetLastModifiedTimestamps method for timestamp retrieval. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SavePlayer as SavePlayer()
participant DbContext
participant Logger
participant Database
Client->>SavePlayer: SavePlayer(characterId)
loop Retry up to 5 times
SavePlayer->>DbContext: SaveChanges()
alt DbUpdateConcurrencyException caught
SavePlayer->>Logger: Log concurrency conflict details
SavePlayer->>DbContext: Get affected entries metadata
loop For each affected entry
alt Entity is Account/Character/CharacterUnlock
SavePlayer->>DbContext: Refresh entry from database
SavePlayer->>SavePlayer: Compare & suppress noisy diffs<br/>(Cooldown, Currency, etc.)
SavePlayer->>Logger: Log actual value changes
else Other entity type
SavePlayer->>SavePlayer: Rethrow exception
end
end
SavePlayer->>DbContext: Reset OriginalValues to database
else Success
break SaveChanges succeeded
SavePlayer->>DbContext: GetLastModifiedTimestamps()
DbContext->>SavePlayer: Return updated timestamps
SavePlayer->>Logger: Log save completion with timestamps
SavePlayer->>Client: Return
end
end
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@Maple2.Database/Storage/Game/GameStorage.User.cs`:
- Around line 348-355: The DbUpdateConcurrencyException handling in SavePlayer
currently rethrows for unsupported entity types (inside the foreach over
ex.Entries), which causes SavePlayer to throw instead of returning false; change
that behavior by removing the throw and instead logging the unsupported entity
(using Logger.LogInformation or LogWarning) and returning false from SavePlayer
so the method follows its error-handling contract; update the catch block
handling in the SavePlayer method (the catch (DbUpdateConcurrencyException ex)
and its foreach over ex.Entries) to log the unsupported entity name
(entry.Metadata.ClrType.Name) and immediately return false rather than
rethrowing.
- Around line 339-420: GetLastModifiedTimestamps returns fresh concurrency
tokens but they are never written back, leaving in-memory
player.Character/account/unlock with stale init-only LastModified values; fix by
applying the returned timestamps after a successful save: either reconstruct and
replace the in-memory entities (create new
Model.Character/Model.Account/CharacterUnlock instances copying all existing
fields but using the new LastModified values and assign them to
player.Character/player.Account/player.Unlock) or, if you prefer to keep the
same instances, update EF's tracked values for the concurrency token via
Context.Entry(entity).Property("LastModified").CurrentValue and .OriginalValue =
newTimestamp (so EF and the token are synchronized) for each of Character,
Account and Unlock when GetLastModifiedTimestamps returns non-null (use the
returned tuple fields e.g.
CharacterLastModified/AccountLastModified/UnlockLastModified).
🧹 Nitpick comments (2)
Maple2.Database/Storage/Game/GameStorage.User.cs (2)
380-398: Redundant secondCreationTimeguard (lines 393–398).The first block (lines 381–391) already matches
property.Name.Equals("CreationTime", OrdinalIgnoreCase)andcontinues. The second block (lines 393–398) usesContains("CreationTime"), which would only match properties whose name contains "CreationTime" as a substring but is not exactly "CreationTime" (since that was already handled). If no such properties exist today, this is dead code. If they might exist in the future, consider adding a comment explaining the intent.
302-303: Logging level escalation in concurrency handler may cause alert fatigue.Line 349 uses
LogWarningfor the concurrency conflict, line 364 usesLogWarningfor diff headers, and line 370 usesLogErrorfor concurrency token details. Since concurrency conflicts are expected and handled (up to 5 retries), consider usingLogWarningorLogInformationconsistently for the token details rather thanLogError, reservingLogErrorfor actual failures (lines 413, 418, 425).Suggested level adjustment
- Logger.LogError(" {PropertyName}: original='{S}' current='{FormatValue1}' db='{S1}' <concurrency token>", property.Name, FormatValue(originalValue), FormatValue(currentValue), FormatValue(databaseValue2)); + Logger.LogWarning(" {PropertyName}: original='{S}' current='{FormatValue1}' db='{S1}' <concurrency token>", property.Name, FormatValue(originalValue), FormatValue(currentValue), FormatValue(databaseValue2));
Change LastModified properties from init-only to settable on Account, Character, Unlock, and CharacterUnlock so in-memory models can be updated after DB saves. In GameStorage.User.SavePlayer add a fail-fast rethrow/log for unexpected concurrency entities, return false when fetching updated timestamps fails, and assign the fetched LastModified timestamps back to the player Account/Character/Unlock. These changes ensure saved timestamps are propagated to runtime objects and surface logic errors for unsupported entities.
1 participant
Summary by CodeRabbit
Release Notes