Merge pull request #170 from Sea-Haven-Industries/fix/ab/sh-377-atomic-team-member-create

fix: make team member creation atomic

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<string>()))
             .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<Func<CancellationToken, Task>>(), It.IsAny<CancellationToken>()),
+            Times.Once);
         userManager.Verify(manager => manager.AddToRoleAsync(It.IsAny<ApplicationUser>(), It.IsAny<string>()), Times.Never);
         userManager.Verify(manager => manager.DeleteAsync(It.IsAny<ApplicationUser>()), 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<string>()))
+            .ReturnsAsync((ApplicationUser?)null);
+        userManager
+            .Setup(manager => manager.CreateAsync(It.IsAny<ApplicationUser>()))
+            .Callback<ApplicationUser>(user =>
+            {
+                user.Id = "mid-fail-user";
+                created = user;
+            })
+            .ReturnsAsync(IdentityResult.Success);
+        userManager
+            .Setup(manager => manager.AddToRoleAsync(It.IsAny<ApplicationUser>(), "Dispatcher"))
+            .ReturnsAsync(IdentityResult.Success);
+        roleManager.Setup(manager => manager.RoleExistsAsync("Dispatcher")).ReturnsAsync(true);
+        areas
+            .Setup(data => data.ReplaceAsync(
+                "mid-fail-user",
+                It.Is<IReadOnlyCollection<string>>(value => value.SequenceEqual(new[] { "East", "West" })),
+                It.IsAny<CancellationToken>()))
+            .Returns(Task.CompletedTask);
+        overrides
+            .Setup(data => data.SetOverridesAsync(
+                "mid-fail-user",
+                It.IsAny<IReadOnlyDictionary<string, UserPermissionState>>(),
+                It.IsAny<CancellationToken>()))
+            .ThrowsAsync(new InvalidOperationException("injected mid-sequence failure"));
+
+        var failure = await Record.ExceptionAsync(() => service.CreateAsync(ValidRequest(), Admin(), CancellationToken.None));
+
+        failure.Should().BeOfType<InvalidOperationException>()
+            .Which.Message.Should().Be("injected mid-sequence failure");
+        created.Should().NotBeNull();
+        areas.Verify(
+            data => data.ReplaceAsync("mid-fail-user", It.IsAny<IReadOnlyCollection<string>>(), It.IsAny<CancellationToken>()),
+            Times.Once);
+        userManager.Verify(manager => manager.DeleteAsync(It.IsAny<ApplicationUser>()), Times.Never);
+    }
+
     private static TeamMemberService NewService(
         out Mock<UserManager<ApplicationUser>> userManager,
         out Mock<RoleManager<IdentityRole>> roleManager,
@@ -350,6 +397,11 @@ public sealed class TeamMemberServiceTests
         overrides = new Mock<ITeamPermissionOverrideDataService>();
         userData = new Mock<IUserDataService>();
         permissions = new Mock<ITeamPermissionService>();
+        userData
+            .Setup(data => data.ExecuteTransactionalAsync(
+                It.IsAny<Func<CancellationToken, Task>>(),
+                It.IsAny<CancellationToken>()))
+            .Returns<Func<CancellationToken, Task>, CancellationToken>((callback, token) => callback(token));
         return new TeamMemberService(
             userManager.Object,
             roleManager.Object,

SeaHaven.DataServices/Implementation/UserDataService.cs

@@ -166,6 +166,25 @@ namespace SeaHaven.DataServices.Implementation
             await transaction.CommitAsync(cancellationToken);
         }
 
+        public async Task ExecuteTransactionalAsync(
+            Func<CancellationToken, Task> 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<string?> GetEmailByIdAsync(
             string userId, CancellationToken cancellationToken)
         {

SeaHaven.DataServices/Interfaces/IUserDataService.cs

@@ -18,6 +18,7 @@ namespace SeaHaven.DataServices.Interfaces
         Task<ApplicationUser?> GetByEmailNormalizedAsync(string email, CancellationToken cancellationToken);
         Task UpdateUserAsync(ApplicationUser user, CancellationToken cancellationToken);
         Task DeleteUserWithCascadeAsync(ApplicationUser user, CancellationToken cancellationToken);
+        Task ExecuteTransactionalAsync(Func<CancellationToken, Task> callback, CancellationToken cancellationToken);
         Task<string?> GetEmailByIdAsync(string userId, CancellationToken cancellationToken);
         Task<IReadOnlyDictionary<string, string>> GetDisplayNamesByIdsAsync(IEnumerable<string> ids);
     }

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);
+            await _userDataService.ExecuteTransactionalAsync(
-        }
+                async transactionCancellationToken =>
-        catch (DbUpdateException)
+                {
-        {
+                    var roleError = await EnsureRoleAsync(role!);
-            // A concurrent create won the race on the unique user-name index
+                    if (roleError is not null)
-            // between the FindByEmailAsync check above and this insert. Surface
+                        throw new TeamMemberCreateException(roleError);
-            // the same conflict message instead of letting the database
-            // exception bubble up as a 500.
-            return Failure("Email is already in use.");
-        }
 
-        if (!createResult.Succeeded)
+                    IdentityResult createResult;
-            return Failure(createResult.Errors.FirstOrDefault()?.Description ?? "Unable to create team member.");
+                    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 (!createResult.Succeeded)
-        if (!addRoleResult.Succeeded)
+                        throw new TeamMemberCreateException(createResult.Errors.FirstOrDefault()?.Description ?? "Unable to create team member.");
-        {
-            await _userManager.DeleteAsync(user);
-            return Failure(addRoleResult.Errors.FirstOrDefault()?.Description ?? "Unable to assign role.");
-        }
 
-        try
+                    var addRoleResult = await _userManager.AddToRoleAsync(user, role!);
-        {
+                    if (!addRoleResult.Succeeded)
-            await _areaDataService.ReplaceAsync(user.Id, areas!, cancellationToken);
+                        throw new TeamMemberCreateException(addRoleResult.Errors.FirstOrDefault()?.Description ?? "Unable to assign role.");
-            await _permissionDataService.SetOverridesAsync(user.Id, overrides!, cancellationToken);
+
+                    await _areaDataService.ReplaceAsync(user.Id, areas!, transactionCancellationToken);
+                    await _permissionDataService.SetOverridesAsync(user.Id, overrides!, transactionCancellationToken);
+                },
+                cancellationToken);
         }
-        catch
+        catch (TeamMemberCreateException exception)
         {
-            await _userManager.DeleteAsync(user);
+            return Failure(exception.Message);
-            throw;
         }
 
         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 };
 }

SeaHavenIndustries.Tests/TeamMemberCreateTransactionTests.cs

@@ -0,0 +1,230 @@
+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.Configuration;
+using Microsoft.Extensions.DependencyInjection;
+using Microsoft.Extensions.DependencyInjection.Extensions;
+using SeaHaven.DataServices.Dto;
+using SeaHaven.DataServices.Implementation;
+using SeaHaven.DataServices.DependencyInjection;
+using SeaHaven.DataServices.Interfaces;
+using SeaHaven.Services.DTOs;
+using SeaHaven.Services.DependencyInjection;
+using SeaHaven.Services.Implementation;
+using SeaHaven.Services.Interfaces;
+
+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<ApplicationDbContext>()
+            .UseSqlite(connection)
+            .Options;
+
+        await using (var setup = new SqliteTeamMemberTestDbContext(options))
+            await setup.Database.EnsureCreatedAsync();
+
+        await using var serviceProvider = BuildServiceProvider(connection, injectFailure: true);
+        ThrowingAfterPersistPermissionDataService failingPermissionData;
+        await using (var createScope = serviceProvider.CreateAsyncScope())
+        {
+            var service = createScope.ServiceProvider.GetRequiredService<ITeamMemberService>();
+            failingPermissionData = (ThrowingAfterPersistPermissionDataService)createScope.ServiceProvider
+                .GetRequiredService<ITeamPermissionOverrideDataService>();
+            var failure = await Record.ExceptionAsync(() => service.CreateAsync(
+                ValidRequest(),
+                Admin(),
+                CancellationToken.None));
+
+            Assert.IsType<InvalidOperationException>(failure);
+            Assert.Equal("injected mid-sequence failure", failure!.Message);
+        }
+
+        Assert.NotNull(failingPermissionData.RowsAtFailure);
+        var rowsAtFailure = failingPermissionData.RowsAtFailure!.Value;
+        Assert.Equal(1, rowsAtFailure.Users);
+        Assert.Equal(1, rowsAtFailure.Roles);
+        Assert.Equal(1, rowsAtFailure.UserRoles);
+        Assert.Equal(2, rowsAtFailure.ServiceAreas);
+        Assert.Equal(1, rowsAtFailure.PermissionOverrides);
+
+        await using var verifyScope = serviceProvider.CreateAsyncScope();
+        var verify = verifyScope.ServiceProvider.GetRequiredService<ApplicationDbContext>();
+        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());
+    }
+
+    [Fact]
+    public async Task Create_SuccessfulTransaction_CommitsPendingMemberAndAssociations()
+    {
+        await using var connection = new SqliteConnection("Data Source=:memory:;Foreign Keys=True");
+        await connection.OpenAsync();
+
+        var options = new DbContextOptionsBuilder<ApplicationDbContext>()
+            .UseSqlite(connection)
+            .Options;
+
+        await using (var setup = new SqliteTeamMemberTestDbContext(options))
+            await setup.Database.EnsureCreatedAsync();
+
+        await using var serviceProvider = BuildServiceProvider(connection);
+        await using (var createScope = serviceProvider.CreateAsyncScope())
+        {
+            var service = createScope.ServiceProvider.GetRequiredService<ITeamMemberService>();
+            var outcome = await service.CreateAsync(ValidRequest(), Admin(), CancellationToken.None);
+
+            Assert.True(outcome.Success);
+            Assert.True(outcome.Member!.PendingRegistration);
+            Assert.Equal("taylor@example.com", outcome.Member.Email);
+        }
+
+        await using var verifyScope = serviceProvider.CreateAsyncScope();
+        var verify = verifyScope.ServiceProvider.GetRequiredService<ApplicationDbContext>();
+        var user = await verify.Users.AsNoTracking().SingleAsync(user => user.Email == "taylor@example.com");
+        Assert.True(user.PendingRegistration);
+
+        var role = await verify.Roles.AsNoTracking().SingleAsync(role => role.Name == "Dispatcher");
+        Assert.Contains(await verify.UserRoles.AsNoTracking().ToListAsync(), row => row.UserId == user.Id && row.RoleId == role.Id);
+
+        var areas = await verify.UserServiceAreas.AsNoTracking()
+            .Where(area => area.UserId == user.Id)
+            .Select(area => area.Area)
+            .OrderBy(area => area)
+            .ToListAsync();
+        Assert.Equal(new[] { "East", "West" }, areas);
+
+        var permissionOverride = await verify.UserPermissionOverrides.AsNoTracking()
+            .SingleAsync(permission => permission.UserId == user.Id);
+        Assert.Equal("deleteSites", permissionOverride.PermissionKey);
+    }
+
+    private static ServiceProvider BuildServiceProvider(SqliteConnection connection, bool injectFailure = false)
+    {
+        var configuration = new ConfigurationBuilder().Build();
+        var services = new ServiceCollection();
+        services.AddLogging();
+        services.AddDbContext<ApplicationDbContext>(builder => builder.UseSqlite(connection));
+        services.Replace(ServiceDescriptor.Scoped<ApplicationDbContext>(provider =>
+            new SqliteTeamMemberTestDbContext(
+                provider.GetRequiredService<DbContextOptions<ApplicationDbContext>>())));
+        services.AddIdentity<ApplicationUser, IdentityRole>()
+            .AddEntityFrameworkStores<ApplicationDbContext>()
+            .AddDefaultTokenProviders();
+        services.AddDataServices();
+        services.AddBusinessServices(configuration);
+
+        if (injectFailure)
+        {
+            services.AddScoped<TeamPermissionOverrideDataService>();
+            services.AddScoped<ITeamPermissionOverrideDataService>(provider =>
+                new ThrowingAfterPersistPermissionDataService(
+                    provider.GetRequiredService<TeamPermissionOverrideDataService>(),
+                    provider.GetRequiredService<ApplicationDbContext>()));
+        }
+
+        return services.BuildServiceProvider();
+    }
+
+    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<string, UserPermissionState>
+        {
+            ["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<ApplicationDbContext> 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;
+        private readonly ApplicationDbContext _context;
+
+        public ThrowingAfterPersistPermissionDataService(
+            ITeamPermissionOverrideDataService inner,
+            ApplicationDbContext context)
+        {
+            _inner = inner;
+            _context = context;
+        }
+
+        public (int Users, int Roles, int UserRoles, int ServiceAreas, int PermissionOverrides)? RowsAtFailure { get; private set; }
+
+        public Task<TeamPermissionUserData?> 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<string, UserPermissionState> overrides,
+            CancellationToken cancellationToken)
+        {
+            await _inner.SetOverridesAsync(userId, overrides, cancellationToken);
+            RowsAtFailure = (
+                await _context.Users.CountAsync(cancellationToken),
+                await _context.Roles.CountAsync(cancellationToken),
+                await _context.UserRoles.CountAsync(cancellationToken),
+                await _context.UserServiceAreas.CountAsync(cancellationToken),
+                await _context.UserPermissionOverrides.CountAsync(cancellationToken));
+            throw new InvalidOperationException("injected mid-sequence failure");
+        }
+
+        public Task ClearOverridesAsync(string userId, CancellationToken cancellationToken) =>
+            _inner.ClearOverridesAsync(userId, cancellationToken);
+    }
+
+}