From 3b626cca58e92aec5701a2f69803af5e0768c428 Mon Sep 17 00:00:00 2001 From: Alexandre Brandizzi Date: Wed, 23 Sep 2026 03:35:23 -0300 Subject: [PATCH] fix: make team member creation atomic --- .../TeamMemberServiceTests.cs | 54 +++++- .../Implementation/UserDataService.cs | 19 ++ .../Interfaces/IUserDataService.cs | 1 + .../Implementation/TeamMemberService.cs | 68 ++++---- .../TeamMemberCreateTransactionTests.cs | 162 ++++++++++++++++++ 5 files changed, 273 insertions(+), 31 deletions(-) create mode 100644 SeaHavenIndustries.Tests/TeamMemberCreateTransactionTests.cs diff --git a/Api.SeaHavenIndustries.Tests/TeamMemberServiceTests.cs b/Api.SeaHavenIndustries.Tests/TeamMemberServiceTests.cs index 5f5204b..cd2ced8 100644 --- a/Api.SeaHavenIndustries.Tests/TeamMemberServiceTests.cs +++ b/Api.SeaHavenIndustries.Tests/TeamMemberServiceTests.cs @@ -319,7 +319,7 @@ public sealed class TeamMemberServiceTests [Fact] public async Task Create_ConcurrentDuplicateEmail_ReturnsAlreadyInUseInsteadOf500() { - var service = NewService(out var userManager, out var roleManager, out _, out _, out _, out _); + var service = NewService(out var userManager, out var roleManager, out _, out _, out var userData, out _); userManager .Setup(manager => manager.FindByEmailAsync(It.IsAny())) .ReturnsAsync((ApplicationUser?)null); @@ -332,10 +332,57 @@ public sealed class TeamMemberServiceTests result.Success.Should().BeFalse(); result.Error.Should().Be("Email is already in use."); + userData.Verify( + data => data.ExecuteTransactionalAsync(It.IsAny>(), It.IsAny()), + Times.Once); userManager.Verify(manager => manager.AddToRoleAsync(It.IsAny(), It.IsAny()), Times.Never); userManager.Verify(manager => manager.DeleteAsync(It.IsAny()), Times.Never); } + [Fact] + public async Task Create_MidSequencePersistenceFailure_PropagatesWithoutCompensation() + { + var service = NewService(out var userManager, out var roleManager, out var areas, out var overrides, out _, out _); + ApplicationUser? created = null; + userManager + .Setup(manager => manager.FindByEmailAsync(It.IsAny())) + .ReturnsAsync((ApplicationUser?)null); + userManager + .Setup(manager => manager.CreateAsync(It.IsAny())) + .Callback(user => + { + user.Id = "mid-fail-user"; + created = user; + }) + .ReturnsAsync(IdentityResult.Success); + userManager + .Setup(manager => manager.AddToRoleAsync(It.IsAny(), "Dispatcher")) + .ReturnsAsync(IdentityResult.Success); + roleManager.Setup(manager => manager.RoleExistsAsync("Dispatcher")).ReturnsAsync(true); + areas + .Setup(data => data.ReplaceAsync( + "mid-fail-user", + It.Is>(value => value.SequenceEqual(new[] { "East", "West" })), + It.IsAny())) + .Returns(Task.CompletedTask); + overrides + .Setup(data => data.SetOverridesAsync( + "mid-fail-user", + It.IsAny>(), + It.IsAny())) + .ThrowsAsync(new InvalidOperationException("injected mid-sequence failure")); + + var failure = await Record.ExceptionAsync(() => service.CreateAsync(ValidRequest(), Admin(), CancellationToken.None)); + + failure.Should().BeOfType() + .Which.Message.Should().Be("injected mid-sequence failure"); + created.Should().NotBeNull(); + areas.Verify( + data => data.ReplaceAsync("mid-fail-user", It.IsAny>(), It.IsAny()), + Times.Once); + userManager.Verify(manager => manager.DeleteAsync(It.IsAny()), Times.Never); + } + private static TeamMemberService NewService( out Mock> userManager, out Mock> roleManager, @@ -350,6 +397,11 @@ public sealed class TeamMemberServiceTests overrides = new Mock(); userData = new Mock(); permissions = new Mock(); + userData + .Setup(data => data.ExecuteTransactionalAsync( + It.IsAny>(), + It.IsAny())) + .Returns, CancellationToken>((callback, token) => callback(token)); return new TeamMemberService( userManager.Object, roleManager.Object, diff --git a/SeaHaven.DataServices/Implementation/UserDataService.cs b/SeaHaven.DataServices/Implementation/UserDataService.cs index 374d34e..32ac851 100644 --- a/SeaHaven.DataServices/Implementation/UserDataService.cs +++ b/SeaHaven.DataServices/Implementation/UserDataService.cs @@ -166,6 +166,25 @@ namespace SeaHaven.DataServices.Implementation await transaction.CommitAsync(cancellationToken); } + public async Task ExecuteTransactionalAsync( + Func callback, + CancellationToken cancellationToken) + { + ArgumentNullException.ThrowIfNull(callback); + + await using var transaction = await _context.Database.BeginTransactionAsync(cancellationToken); + try + { + await callback(cancellationToken); + await transaction.CommitAsync(cancellationToken); + } + catch + { + await transaction.RollbackAsync(CancellationToken.None); + throw; + } + } + public async Task GetEmailByIdAsync( string userId, CancellationToken cancellationToken) { diff --git a/SeaHaven.DataServices/Interfaces/IUserDataService.cs b/SeaHaven.DataServices/Interfaces/IUserDataService.cs index 0a60d5c..3c74dcf 100644 --- a/SeaHaven.DataServices/Interfaces/IUserDataService.cs +++ b/SeaHaven.DataServices/Interfaces/IUserDataService.cs @@ -18,6 +18,7 @@ namespace SeaHaven.DataServices.Interfaces Task GetByEmailNormalizedAsync(string email, CancellationToken cancellationToken); Task UpdateUserAsync(ApplicationUser user, CancellationToken cancellationToken); Task DeleteUserWithCascadeAsync(ApplicationUser user, CancellationToken cancellationToken); + Task ExecuteTransactionalAsync(Func callback, CancellationToken cancellationToken); Task GetEmailByIdAsync(string userId, CancellationToken cancellationToken); Task> GetDisplayNamesByIdsAsync(IEnumerable ids); } diff --git a/SeaHaven.Services/Implementation/TeamMemberService.cs b/SeaHaven.Services/Implementation/TeamMemberService.cs index ba2cd2b..faf64f3 100644 --- a/SeaHaven.Services/Implementation/TeamMemberService.cs +++ b/SeaHaven.Services/Implementation/TeamMemberService.cs @@ -52,10 +52,6 @@ public sealed class TeamMemberService : ITeamMemberService if (await _userManager.FindByEmailAsync(email!) is not null) return Failure("Email is already in use."); - var roleError = await EnsureRoleAsync(role!); - if (roleError is not null) - return Failure(roleError); - var now = DateTime.UtcNow; var user = new ApplicationUser { @@ -72,39 +68,44 @@ public sealed class TeamMemberService : ITeamMemberService PendingRegistrationCreatedDate = now }; - IdentityResult createResult; try { - createResult = await _userManager.CreateAsync(user); - } - catch (DbUpdateException) - { - // A concurrent create won the race on the unique user-name index - // between the FindByEmailAsync check above and this insert. Surface - // the same conflict message instead of letting the database - // exception bubble up as a 500. - return Failure("Email is already in use."); - } + await _userDataService.ExecuteTransactionalAsync( + async transactionCancellationToken => + { + var roleError = await EnsureRoleAsync(role!); + if (roleError is not null) + throw new TeamMemberCreateException(roleError); - if (!createResult.Succeeded) - return Failure(createResult.Errors.FirstOrDefault()?.Description ?? "Unable to create team member."); + IdentityResult createResult; + try + { + createResult = await _userManager.CreateAsync(user); + } + catch (DbUpdateException) + { + // A concurrent create won the race on the unique user-name index + // between the FindByEmailAsync check above and this insert. Surface + // the same conflict message instead of letting the database + // exception bubble up as a 500. + throw new TeamMemberCreateException("Email is already in use."); + } - var addRoleResult = await _userManager.AddToRoleAsync(user, role!); - if (!addRoleResult.Succeeded) - { - await _userManager.DeleteAsync(user); - return Failure(addRoleResult.Errors.FirstOrDefault()?.Description ?? "Unable to assign role."); - } + if (!createResult.Succeeded) + throw new TeamMemberCreateException(createResult.Errors.FirstOrDefault()?.Description ?? "Unable to create team member."); - try - { - await _areaDataService.ReplaceAsync(user.Id, areas!, cancellationToken); - await _permissionDataService.SetOverridesAsync(user.Id, overrides!, cancellationToken); + var addRoleResult = await _userManager.AddToRoleAsync(user, role!); + if (!addRoleResult.Succeeded) + throw new TeamMemberCreateException(addRoleResult.Errors.FirstOrDefault()?.Description ?? "Unable to assign role."); + + await _areaDataService.ReplaceAsync(user.Id, areas!, transactionCancellationToken); + await _permissionDataService.SetOverridesAsync(user.Id, overrides!, transactionCancellationToken); + }, + cancellationToken); } - catch + catch (TeamMemberCreateException exception) { - await _userManager.DeleteAsync(user); - throw; + return Failure(exception.Message); } return new CreateTeamMemberOutcomeDTO @@ -391,6 +392,13 @@ public sealed class TeamMemberService : ITeamMemberService private static CreateTeamMemberOutcomeDTO Failure(string error) => new() { Success = false, Error = error }; + private sealed class TeamMemberCreateException : Exception + { + public TeamMemberCreateException(string error) : base(error) + { + } + } + private static TeamMemberOperationOutcomeDTO OperationFailure(string error) => new() { Success = false, Error = error }; } diff --git a/SeaHavenIndustries.Tests/TeamMemberCreateTransactionTests.cs b/SeaHavenIndustries.Tests/TeamMemberCreateTransactionTests.cs new file mode 100644 index 0000000..43c2d40 --- /dev/null +++ b/SeaHavenIndustries.Tests/TeamMemberCreateTransactionTests.cs @@ -0,0 +1,162 @@ +using System.Security.Claims; +using Data.SeaHavenIndustries; +using Data.SeaHavenIndustries.Enums; +using Microsoft.AspNetCore.Identity; +using Microsoft.AspNetCore.Identity.EntityFrameworkCore; +using Microsoft.Data.Sqlite; +using Microsoft.EntityFrameworkCore; +using Microsoft.EntityFrameworkCore.Metadata; +using Microsoft.Extensions.Logging.Abstractions; +using SeaHaven.DataServices.Dto; +using SeaHaven.DataServices.Implementation; +using SeaHaven.DataServices.Interfaces; +using SeaHaven.Services.DTOs; +using SeaHaven.Services.Implementation; + +namespace SeaHavenIndustries.Tests; + +public sealed class TeamMemberCreateTransactionTests +{ + [Fact] + public async Task Create_MidSequencePersistenceFailure_RollsBackAllMemberRows() + { + await using var connection = new SqliteConnection("Data Source=:memory:;Foreign Keys=True"); + await connection.OpenAsync(); + + var options = new DbContextOptionsBuilder() + .UseSqlite(connection) + .Options; + + await using (var setup = new SqliteTeamMemberTestDbContext(options)) + await setup.Database.EnsureCreatedAsync(); + + await using (var createScope = new SqliteTeamMemberTestDbContext(options)) + { + var permissionData = new ThrowingAfterPersistPermissionDataService( + new TeamPermissionOverrideDataService(createScope)); + var service = new TeamMemberService( + CreateUserManager(createScope), + CreateRoleManager(createScope), + new UserServiceAreaDataService(createScope), + permissionData, + new UserDataService(createScope), + permissionService: null!); + + var failure = await Record.ExceptionAsync(() => service.CreateAsync( + ValidRequest(), + Admin(), + CancellationToken.None)); + + Assert.IsType(failure); + Assert.Equal("injected mid-sequence failure", failure!.Message); + } + + await using var verify = new SqliteTeamMemberTestDbContext(options); + Assert.Empty(await verify.Users.AsNoTracking().ToListAsync()); + Assert.Empty(await verify.Roles.AsNoTracking().ToListAsync()); + Assert.Empty(await verify.UserRoles.AsNoTracking().ToListAsync()); + Assert.Empty(await verify.UserServiceAreas.AsNoTracking().ToListAsync()); + Assert.Empty(await verify.UserPermissionOverrides.AsNoTracking().ToListAsync()); + } + + private static UserManager CreateUserManager(ApplicationDbContext context) => + new( + new UserStore(context), + Microsoft.Extensions.Options.Options.Create(new IdentityOptions()), + new PasswordHasher(), + Array.Empty>(), + Array.Empty>(), + new UpperInvariantLookupNormalizer(), + new IdentityErrorDescriber(), + new EmptyServiceProvider(), + NullLogger>.Instance); + + private static RoleManager CreateRoleManager(ApplicationDbContext context) => + new( + new RoleStore(context), + Array.Empty>(), + new UpperInvariantLookupNormalizer(), + new IdentityErrorDescriber(), + NullLogger>.Instance); + + private static CreateTeamMemberRequestDTO ValidRequest() => new() + { + Name = "Taylor Dispatcher", + Role = "dispatcher", + Color = "#F59E0B", + Email = "taylor@example.com", + Phone = "555-0100", + ServiceAreas = new[] { "east", "West" }, + PermissionOverrides = new Dictionary + { + ["deleteSites"] = UserPermissionState.Allow + } + }; + + private static ClaimsPrincipal Admin() => + new(new ClaimsIdentity(new[] { new Claim(ClaimTypes.Role, "Admin") }, "test")); + + private sealed class SqliteTeamMemberTestDbContext : ApplicationDbContext + { + public SqliteTeamMemberTestDbContext(DbContextOptions options) + : base(options) + { + } + + protected override void OnModelCreating(ModelBuilder builder) + { + base.OnModelCreating(builder); + + foreach (var index in builder.Model.GetEntityTypes().SelectMany(entity => entity.GetIndexes())) + { + if (index.GetFilter() is not null) + index.SetFilter(null); + } + + foreach (var property in builder.Model.GetEntityTypes() + .SelectMany(entity => entity.GetProperties()) + .Where(property => property.Name == "RowVersion" && property.ClrType == typeof(byte[]))) + { + property.ValueGenerated = ValueGenerated.Never; + property.IsConcurrencyToken = false; + } + } + } + + private sealed class ThrowingAfterPersistPermissionDataService : ITeamPermissionOverrideDataService + { + private readonly ITeamPermissionOverrideDataService _inner; + + public ThrowingAfterPersistPermissionDataService(ITeamPermissionOverrideDataService inner) + { + _inner = inner; + } + + public Task GetUserAsync(string userId, CancellationToken cancellationToken) => + _inner.GetUserAsync(userId, cancellationToken); + + public Task SetOverrideAsync( + string userId, + string permissionKey, + UserPermissionState state, + CancellationToken cancellationToken) => + _inner.SetOverrideAsync(userId, permissionKey, state, cancellationToken); + + public async Task SetOverridesAsync( + string userId, + IReadOnlyDictionary overrides, + CancellationToken cancellationToken) + { + await _inner.SetOverridesAsync(userId, overrides, cancellationToken); + throw new InvalidOperationException("injected mid-sequence failure"); + } + + public Task ClearOverridesAsync(string userId, CancellationToken cancellationToken) => + _inner.ClearOverridesAsync(userId, cancellationToken); + } + + private sealed class EmptyServiceProvider : IServiceProvider + { + public object? GetService(Type serviceType) => null; + } +}