feat: Implement transaction handling and add AvatarUrl to User entity#46
Conversation
WalkthroughThe PR refactors transaction handling by introducing an Changes
Sequence Diagram(s)sequenceDiagram
participant Mediator
participant TransactionBehavior
participant UnitOfWork
participant DataContext
participant Database
Mediator->>TransactionBehavior: Handle(request)
TransactionBehavior->>UnitOfWork: ExecuteInTransactionAsync(action)
rect rgb(220, 240, 255)
Note over DataContext,Database: Transaction Wrapper
UnitOfWork->>DataContext: Create ExecutionStrategy
DataContext->>Database: BeginTransaction()
activate Database
end
UnitOfWork->>UnitOfWork: Execute action (business logic)
UnitOfWork->>Database: SaveChangesAsync()
rect rgb(220, 255, 220)
Note over DataContext,Database: Commit Phase
DataContext->>Database: CommitTransaction()
deactivate Database
end
UnitOfWork-->>TransactionBehavior: return result<T>
TransactionBehavior-->>Mediator: return response
alt On Exception
rect rgb(255, 220, 220)
Note over DataContext,Database: Error & Rollback
DataContext->>Database: RollbackTransaction()
DataContext-->>UnitOfWork: rethrow exception
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
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
🧹 Nitpick comments (4)
src/Infrastructure/Data/Contexts/DataContext.cs (1)
7-7: Execution strategy helper looks good; consider narrowing visibilityWrapping
Database.CreateExecutionStrategy()behindCreateExecutionStrategy()keeps strategy creation centralized and makes the UnitOfWork implementation cleaner. If this is only ever used from infrastructure code (e.g., otherDataContextpartials), consider making itinternalto avoid exposing EF‑specific details unnecessarily on the public surface.Also applies to: 58-61
src/Application/Common/IUnitOfWork.cs (1)
40-40: Clarify the contract and semantics ofExecuteInTransactionAsyncThe new method is a good abstraction point, but its behavior is ambiguous (e.g., does it always start a new transaction, reuse an existing one when
HasActiveTransactionis true, and callSaveChangesAsyncinternally; can the action be retried via an execution strategy, etc.). Consider documenting these expectations on the interface to avoid divergent implementations and misuse.Example doc comment:
- Task<T> ExecuteInTransactionAsync<T>(Func<Task<T>> action, CancellationToken cancellationToken = default); + /// <summary> + /// Executes the given asynchronous <paramref name="action"/> within a database transaction. + /// Implementations should reuse an existing transaction when <see cref="HasActiveTransaction"/> is true, + /// and are responsible for saving changes and committing/rolling back as appropriate. + /// If an execution strategy with retries is used, callers must ensure that <paramref name="action"/> is idempotent. + /// </summary> + Task<T> ExecuteInTransactionAsync<T>(Func<Task<T>> action, CancellationToken cancellationToken = default);src/Infrastructure/Data/Migrations/DataContextModelSnapshot.cs (1)
158-160: AvatarUrl snapshot mapping is fine; confirm nullable & unbounded length are intentionalThe new
AvatarUrlcolumn onDomain.Entities.Useris mapped as nullabletextwith no max length. That’s valid for URLs, but you may want to align constraints withUserExternalLogin.AvatarUrl(which usesHasMaxLength(500)) if you prefer consistent limits and more predictable schema. If you expect arbitrary‑length data here, current mapping is OK.src/Application/Common/Behaviors/TransactionBehavior.cs (1)
25-43: Tighten transaction logging, verify retry/idempotency behavior, and checknext(cancellationToken)usageA few points around the new transactional wrapper:
Duplicate "Starting transaction" log
You now log"Starting transaction..."both before callingExecuteInTransactionAsync(Line 32) and again inside the lambda (Line 36). This is redundant and will clutter logs. Consider keeping a single log, e.g.:
logger.LogDebug("Starting transaction for {RequestName}", typeof(TRequest).Name);return await unitOfWork.ExecuteInTransactionAsync(async () =>{logger.LogDebug("Starting transaction for {RequestName}", typeof(TRequest).Name);
logger.LogDebug("Executing transactional command {RequestName}", typeof(TRequest).Name);return await unitOfWork.ExecuteInTransactionAsync(async () =>{ var response = await next(cancellationToken); logger.LogDebug("Transaction committed for {RequestName}", typeof(TRequest).Name); return response; }, cancellationToken);
Interaction with existing transactions
The behavior no longer checksunitOfWork.HasActiveTransactionbefore starting a transactional scope. Ensure theExecuteInTransactionAsyncimplementation itself handles the “already in a transaction” case correctly (reusing or nesting appropriately) so that commands executed inside broader transactional flows don’t accidentally start a conflicting transaction.Execution strategy and idempotency
IfExecuteInTransactionAsyncuses EF Core’s execution strategy (as suggested byCreateExecutionStrategy), the lambda may be retried on transient failures. Any side effects in handlers (e.g., sending emails, publishing messages) could run multiple times. Consider documenting and enforcing that only DB work lives inside this wrapper, or otherwise make handlers idempotent with respect to retries.
next(cancellationToken)correctness
In standard MediatR,RequestHandlerDelegate<TResponse>does not take aCancellationToken, and handlers are usually invoked viaawait next(). If you are not intentionally relying on an extension method that adds this overload, it’s safer to callawait next()in both branches and pass the token only via the pipeline’sHandlesignature.[highlighted issues collectively merit verification but are not necessarily blockers if your existing infrastructure already accounts for them.]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/Application/Common/Behaviors/TransactionBehavior.cs(1 hunks)src/Application/Common/IUnitOfWork.cs(1 hunks)src/Infrastructure/Data/Contexts/DataContext.UnitOfWork.cs(2 hunks)src/Infrastructure/Data/Contexts/DataContext.cs(2 hunks)src/Infrastructure/Data/Migrations/20251114111233_AddAvatarUrlToUser.Designer.cs(1 hunks)src/Infrastructure/Data/Migrations/20251114111233_AddAvatarUrlToUser.cs(1 hunks)src/Infrastructure/Data/Migrations/DataContextModelSnapshot.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Infrastructure/Data/Contexts/DataContext.UnitOfWork.cs (2)
src/Application/Common/Behaviors/TransactionBehavior.cs (1)
Task(25-43)src/Application/Common/IUnitOfWork.cs (5)
Task(23-23)Task(28-28)Task(33-33)Task(38-38)Task(40-40)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-deploy
🔇 Additional comments (3)
src/Infrastructure/Data/Migrations/20251114111233_AddAvatarUrlToUser.Designer.cs (1)
1-598: Migration designer forAddAvatarUrlToUserlooks consistentThe designer’s target model correctly includes
User.AvatarUrlas a nullabletextcolumn and otherwise mirrors the currentDataContextModelSnapshot. Nothing to adjust here as long as this file was generated by the same EF Core version used elsewhere.src/Infrastructure/Data/Migrations/20251114111233_AddAvatarUrlToUser.cs (1)
1-30: LGTM! Migration is well-structured.The migration correctly adds a nullable
AvatarUrlcolumn to the Users table and provides a proper rollback in the Down method. Since the column is nullable, there are no concerns with existing data.src/Infrastructure/Data/Contexts/DataContext.UnitOfWork.cs (1)
2-2: The using statement is not necessary for CreateExecutionStrategy().DatabaseFacade.CreateExecutionStrategy() is defined in the Microsoft.EntityFrameworkCore.Infrastructure namespace, and the method can be called through the Database property without adding
using Microsoft.EntityFrameworkCore. The existingusing Microsoft.EntityFrameworkCore.Storage;provides sufficient access to the method.Likely an incorrect or invalid review comment.
| public async Task<T> ExecuteInTransactionAsync<T>(Func<Task<T>> action, CancellationToken cancellationToken = default) | ||
| { | ||
| var strategy = Database.CreateExecutionStrategy(); | ||
|
|
||
| return await strategy.ExecuteAsync(async () => | ||
| { | ||
| await using var tx = await Database.BeginTransactionAsync(cancellationToken); | ||
|
|
||
| try | ||
| { | ||
| var result = await action(); | ||
|
|
||
| await SaveChangesAsync(cancellationToken); | ||
| await tx.CommitAsync(cancellationToken); | ||
|
|
||
| return result; | ||
| } | ||
| catch | ||
| { | ||
| await tx.RollbackAsync(cancellationToken); | ||
| throw; | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
Note: Duplicate logging in TransactionBehavior.cs.
The TransactionBehavior.cs file (shown in relevant snippets) has duplicate "Starting transaction" log statements at lines 29 and 31. One should be removed to avoid redundant log entries.
🤖 Prompt for AI Agents
In TransactionBehavior.cs around lines 29 to 31 there are duplicate "Starting
transaction" log statements; remove the redundant one (keep a single,
appropriately leveled log) so the message is only emitted once when a
transaction begins, and ensure any contextual data passed to the logger remains
on the retained call.
Transaction state inconsistency: ExecuteInTransactionAsync doesn't coordinate with _currentTransaction.
The new ExecuteInTransactionAsync method creates its own transaction (line 42) without checking or updating the _currentTransaction field that's used by the existing transaction methods. This creates several problems:
HasActiveTransaction(line 13) will returnfalseeven when a transaction is active viaExecuteInTransactionAsync- If code calls
BeginTransactionAsyncwhile insideExecuteInTransactionAsync, it will silently succeed (line 30 returns early) but won't actually start a nested transaction - Mixing the new wrapper pattern with manual
BeginTransactionAsync/CommitTransactionAsynccalls could lead to unexpected behavior
Consider one of these approaches:
Option 1: Make ExecuteInTransactionAsync coordinate with _currentTransaction
public async Task<T> ExecuteInTransactionAsync<T>(Func<Task<T>> action, CancellationToken cancellationToken = default)
{
+ if (_currentTransaction != null)
+ {
+ throw new InvalidOperationException("Cannot execute in transaction when another transaction is already active");
+ }
+
var strategy = Database.CreateExecutionStrategy();
return await strategy.ExecuteAsync(async () =>
{
- await using var tx = await Database.BeginTransactionAsync(cancellationToken);
+ _currentTransaction = await Database.BeginTransactionAsync(cancellationToken);
try
{
var result = await action();
await SaveChangesAsync(cancellationToken);
- await tx.CommitAsync(cancellationToken);
+ await _currentTransaction.CommitAsync(cancellationToken);
return result;
}
catch
{
- await tx.RollbackAsync(cancellationToken);
+ await _currentTransaction.RollbackAsync(cancellationToken);
throw;
}
+ finally
+ {
+ if (_currentTransaction != null)
+ {
+ await _currentTransaction.DisposeAsync();
+ _currentTransaction = null;
+ }
+ }
});
}Option 2: Deprecate manual transaction methods
If the new pattern is preferred, consider marking BeginTransactionAsync, CommitTransactionAsync, and RollbackTransactionAsync as [Obsolete] and migrating all usage to ExecuteInTransactionAsync.
📝 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 async Task<T> ExecuteInTransactionAsync<T>(Func<Task<T>> action, CancellationToken cancellationToken = default) | |
| { | |
| var strategy = Database.CreateExecutionStrategy(); | |
| return await strategy.ExecuteAsync(async () => | |
| { | |
| await using var tx = await Database.BeginTransactionAsync(cancellationToken); | |
| try | |
| { | |
| var result = await action(); | |
| await SaveChangesAsync(cancellationToken); | |
| await tx.CommitAsync(cancellationToken); | |
| return result; | |
| } | |
| catch | |
| { | |
| await tx.RollbackAsync(cancellationToken); | |
| throw; | |
| } | |
| }); | |
| } | |
| public async Task<T> ExecuteInTransactionAsync<T>(Func<Task<T>> action, CancellationToken cancellationToken = default) | |
| { | |
| if (_currentTransaction != null) | |
| { | |
| throw new InvalidOperationException("Cannot execute in transaction when another transaction is already active"); | |
| } | |
| var strategy = Database.CreateExecutionStrategy(); | |
| return await strategy.ExecuteAsync(async () => | |
| { | |
| _currentTransaction = await Database.BeginTransactionAsync(cancellationToken); | |
| try | |
| { | |
| var result = await action(); | |
| await SaveChangesAsync(cancellationToken); | |
| await _currentTransaction.CommitAsync(cancellationToken); | |
| return result; | |
| } | |
| catch | |
| { | |
| await _currentTransaction.RollbackAsync(cancellationToken); | |
| throw; | |
| } | |
| finally | |
| { | |
| if (_currentTransaction != null) | |
| { | |
| await _currentTransaction.DisposeAsync(); | |
| _currentTransaction = null; | |
| } | |
| } | |
| }); | |
| } |
1 participant
Summary by CodeRabbit
New Features
Refactor