Use transactions to coordinate updates to FrontierCollection, WordsCollection#4189
Use transactions to coordinate updates to FrontierCollection, WordsCollection#4189imnasnainaec wants to merge 3 commits intomasterfrom
Conversation
📝 WalkthroughWalkthroughThis PR refactors MongoDB transaction handling by introducing an explicit transaction abstraction (IMongoTransaction), updating the MongoDbContext to use session-based transactions, and modifying WordRepository to execute multi-collection operations atomically. New combined frontier operations (CreateAndDeleteFrontier, AddAndDeleteFrontier) are introduced to the repository layer, and WordService is simplified to use these atomic operations. Changes
Sequence DiagramsequenceDiagram
actor Client
participant WordService as WordService
participant WordRepository as WordRepository
participant MongoDbContext as MongoDbContext
participant MongoDB as MongoDB<br/>(Session/Transaction)
Client->>WordService: Update(userId, word)
WordService->>WordRepository: CreateAndDeleteFrontier(newWord, oldWordId)
WordRepository->>MongoDbContext: BeginTransaction()
MongoDbContext->>MongoDB: StartSessionAsync()
MongoDB-->>MongoDbContext: IClientSessionHandle
MongoDbContext->>MongoDB: Start Transaction
MongoDB-->>MongoDbContext: IMongoTransaction
MongoDbContext-->>WordRepository: IMongoTransaction
WordRepository->>MongoDB: InsertOne(WordsCollection, newWord, session)
MongoDB-->>WordRepository: Success
WordRepository->>MongoDB: InsertOne(Frontier, newWord, session)
MongoDB-->>WordRepository: Success
WordRepository->>MongoDB: DeleteOne(Frontier, oldWord, session)
MongoDB-->>WordRepository: Success
WordRepository->>MongoDbContext: CommitTransactionAsync()
MongoDbContext->>MongoDB: Commit Transaction
MongoDB-->>MongoDbContext: Success
MongoDbContext-->>WordRepository: Task Complete
WordRepository-->>WordService: Word (committed)
WordService-->>Client: Updated Word
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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 |
This comment was marked as resolved.
This comment was marked as resolved.
|
@imnasnainaec I've opened a new pull request, #4190, to work on those changes. Once the pull request is ready, I'll request review from you. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4189 +/- ##
===========================================
+ Coverage 75.15% 86.09% +10.94%
===========================================
Files 302 57 -245
Lines 11099 4862 -6237
Branches 1394 603 -791
===========================================
- Hits 8341 4186 -4155
+ Misses 2357 534 -1823
+ Partials 401 142 -259
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@copilot Open a pull request onto this branch that resolves the final goal of this pr:
|
|
@imnasnainaec I've opened a new pull request, #4191, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
Backend/Contexts/MongoDbContext.cs (1)
25-45: Consider implementingIAsyncDisposablefor cleaner async cleanup.The
MongoTransactionWrapperuses synchronousDispose()to clean up the session. Since all transaction operations are async, consider also implementingIAsyncDisposableto allow callers to useawait usingfor more idiomatic async resource management.♻️ Optional: Add IAsyncDisposable support
- private class MongoTransactionWrapper(IClientSessionHandle session) : IMongoTransaction + private class MongoTransactionWrapper(IClientSessionHandle session) : IMongoTransaction, IAsyncDisposable { private readonly IClientSessionHandle _session = session; public IClientSessionHandle Session => _session; public Task CommitTransactionAsync() { return _session.CommitTransactionAsync(); } public Task AbortTransactionAsync() { return _session.AbortTransactionAsync(); } public void Dispose() { _session.Dispose(); } + + public ValueTask DisposeAsync() + { + _session.Dispose(); + return ValueTask.CompletedTask; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Backend/Contexts/MongoDbContext.cs` around lines 25 - 45, The MongoTransactionWrapper currently only implements Dispose() synchronously; update the class declaration for MongoTransactionWrapper to also implement IAsyncDisposable and add a DisposeAsync method that calls and awaits _session.DisposeAsync() (returning a ValueTask), keeping the existing Dispose() for backward compatibility (it can call DisposeAsync().AsTask().GetAwaiter().GetResult() or simply call _session.Dispose()); ensure signatures and references to Session, CommitTransactionAsync, and AbortTransactionAsync remain unchanged so callers can use await using with MongoTransactionWrapper.Backend.Tests/Mocks/MongoDbContextMock.cs (1)
16-33: Consider the implications ofSessionreturningnull!.The
Sessionproperty returnsnull!, which could causeNullReferenceExceptionif any test code attempts to use the session directly. Since theWordRepositoryMockdoesn't use the session (it operates on in-memory collections), this works for current tests. However, if future tests need to verify session-based behavior, this mock would need enhancement.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Backend.Tests/Mocks/MongoDbContextMock.cs` around lines 16 - 33, The MongoTransactionMock's Session property currently returns null! which risks NullReferenceException in tests that access the session; update MongoTransactionMock to return a minimal non-null IClientSessionHandle implementation instead (e.g., add an inner sealed class MockClientSessionHandle implementing IClientSessionHandle with no-op/throwing members as appropriate) and have the Session property return an instance of that mock; this preserves current behavior for in-memory tests while allowing future tests to inspect or interact with the session via the new MockClientSessionHandle, and keep the class name MongoTransactionMock and the Session property as the wiring point.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Backend.Tests/Mocks/MongoDbContextMock.cs`:
- Around line 16-33: The MongoTransactionMock's Session property currently
returns null! which risks NullReferenceException in tests that access the
session; update MongoTransactionMock to return a minimal non-null
IClientSessionHandle implementation instead (e.g., add an inner sealed class
MockClientSessionHandle implementing IClientSessionHandle with no-op/throwing
members as appropriate) and have the Session property return an instance of that
mock; this preserves current behavior for in-memory tests while allowing future
tests to inspect or interact with the session via the new
MockClientSessionHandle, and keep the class name MongoTransactionMock and the
Session property as the wiring point.
In `@Backend/Contexts/MongoDbContext.cs`:
- Around line 25-45: The MongoTransactionWrapper currently only implements
Dispose() synchronously; update the class declaration for
MongoTransactionWrapper to also implement IAsyncDisposable and add a
DisposeAsync method that calls and awaits _session.DisposeAsync() (returning a
ValueTask), keeping the existing Dispose() for backward compatibility (it can
call DisposeAsync().AsTask().GetAwaiter().GetResult() or simply call
_session.Dispose()); ensure signatures and references to Session,
CommitTransactionAsync, and AbortTransactionAsync remain unchanged so callers
can use await using with MongoTransactionWrapper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d608bccf-62e1-4a1a-a466-19db284a3a64
📒 Files selected for processing (8)
Backend.Tests/Mocks/MongoDbContextMock.csBackend.Tests/Mocks/WordRepositoryMock.csBackend/Contexts/MongoDbContext.csBackend/Interfaces/IMongoDbContext.csBackend/Interfaces/IWordRepository.csBackend/Repositories/WordRepository.csBackend/Services/WordService.csBackend/Startup.cs
Goals:
MergeService,WordService: move the core of methods that involve multiple database updates into new methods in theWordRepositoryWordRepository: Use a transaction around every coordinated change to both_wordsand_frontierThis change is
Summary by CodeRabbit
Tests
Refactor