From 66de542f7a89eb243888b4047c16405a9d1ede1e Mon Sep 17 00:00:00 2001 From: Donald Gray Date: Mon, 10 Feb 2025 11:59:36 +0000 Subject: [PATCH] Make EntityCounter create tolerant of existing Avoids issues where failed space delete can prevent creation of space. This logic of "check then update" is already used when inc/dec counter --- .../Assets/ApiAssetRepositoryTests.cs | 8 +-- .../Application/Requests/SetupApplication.cs | 2 +- .../Customer/Requests/CreateCustomer.cs | 2 +- .../DLCS.Model/IEntityCounterRepository.cs | 3 +- .../Entities/EntityCounterRepositoryTests.cs | 30 +++++---- .../Entities/EntityCounterRepository.cs | 47 ++++++++------ .../DLCS.Repository/Spaces/SpaceRepository.cs | 65 ++++++++----------- 7 files changed, 76 insertions(+), 81 deletions(-) diff --git a/src/protagonist/API.Tests/Features/Assets/ApiAssetRepositoryTests.cs b/src/protagonist/API.Tests/Features/Assets/ApiAssetRepositoryTests.cs index 3d4e88a16..445247a0f 100644 --- a/src/protagonist/API.Tests/Features/Assets/ApiAssetRepositoryTests.cs +++ b/src/protagonist/API.Tests/Features/Assets/ApiAssetRepositoryTests.cs @@ -43,12 +43,8 @@ public ApiAssetRepositoryTests(DlcsDatabaseFixture dbFixture) ); // We want this turned on to match live behaviour dbContext.ChangeTracker.QueryTrackingBehavior = QueryTrackingBehavior.TrackAll; - var config = new ConfigurationBuilder() - .AddInMemoryCollection(new[] - { new KeyValuePair("ConnectionStrings:PostgreSQLConnection", dbFixture.ConnectionString) }) - .Build(); - - var entityCounterRepo = new EntityCounterRepository(dbContext); + + var entityCounterRepo = new EntityCounterRepository(dbContext, new NullLogger()); var assetRepositoryCachingHelper = new AssetCachingHelper( new MockCachingService(), diff --git a/src/protagonist/API/Features/Application/Requests/SetupApplication.cs b/src/protagonist/API/Features/Application/Requests/SetupApplication.cs index 84ee29893..3b3613f0e 100644 --- a/src/protagonist/API/Features/Application/Requests/SetupApplication.cs +++ b/src/protagonist/API/Features/Application/Requests/SetupApplication.cs @@ -59,7 +59,7 @@ public async Task Handle(SetupApplication request, Cancellat await CreateDefaultStoragePolicy(cancellationToken); var updateCount = await dbContext.SaveChangesAsync(cancellationToken); - await entityCounterRepository.Create(adminCustomer.Id, KnownEntityCounters.CustomerSpaces, adminCustomer.Id.ToString()); + await entityCounterRepository.TryCreate(adminCustomer.Id, KnownEntityCounters.CustomerSpaces, adminCustomer.Id.ToString()); return updateCount == 2 ? CreateApiKeyResult.Success(apiKey, apiSecret) diff --git a/src/protagonist/API/Features/Customer/Requests/CreateCustomer.cs b/src/protagonist/API/Features/Customer/Requests/CreateCustomer.cs index 846478297..1cf2f4beb 100644 --- a/src/protagonist/API/Features/Customer/Requests/CreateCustomer.cs +++ b/src/protagonist/API/Features/Customer/Requests/CreateCustomer.cs @@ -85,7 +85,7 @@ public async Task> Handle(CreateCustomer requ var newCustomerId = customer.Id; // create an entity counter for space IDs - await entityCounterRepository.Create(newCustomerId, KnownEntityCounters.CustomerSpaces, + await entityCounterRepository.TryCreate(newCustomerId, KnownEntityCounters.CustomerSpaces, newCustomerId.ToString()); await CreateAuthServices(cancellationToken, newCustomerId); diff --git a/src/protagonist/DLCS.Model/IEntityCounterRepository.cs b/src/protagonist/DLCS.Model/IEntityCounterRepository.cs index 874939754..2b7427ac7 100644 --- a/src/protagonist/DLCS.Model/IEntityCounterRepository.cs +++ b/src/protagonist/DLCS.Model/IEntityCounterRepository.cs @@ -11,7 +11,8 @@ public interface IEntityCounterRepository /// /// Create a new EntityCounter record with specified value. /// - Task Create(int customer, string entityType, string scope, long initialValue = 1); + /// True if EntityCounter created, else false + Task TryCreate(int customer, string entityType, string scope, long initialValue = 1); /// /// Removes an EntityCounter from the database diff --git a/src/protagonist/DLCS.Repository.Tests/Entities/EntityCounterRepositoryTests.cs b/src/protagonist/DLCS.Repository.Tests/Entities/EntityCounterRepositoryTests.cs index a0e08739d..13811f3f4 100644 --- a/src/protagonist/DLCS.Repository.Tests/Entities/EntityCounterRepositoryTests.cs +++ b/src/protagonist/DLCS.Repository.Tests/Entities/EntityCounterRepositoryTests.cs @@ -1,7 +1,7 @@ -using System; -using System.Linq; +using System.Linq; using DLCS.Repository.Entities; using Microsoft.EntityFrameworkCore; +using Microsoft.Extensions.Logging.Abstractions; using Test.Helpers.Integration; namespace DLCS.Repository.Tests.Entities; @@ -17,47 +17,49 @@ public EntityCounterRepositoryTests(DlcsDatabaseFixture dbFixture) { dbContext = dbFixture.DbContext; - sut = new EntityCounterRepository(dbContext); + sut = new EntityCounterRepository(dbContext, new NullLogger()); dbFixture.CleanUp(); } [Fact] - public async Task Create_AddsEntityCounter() + public async Task TryCreate_AddsEntityCounter() { // Arrange - const string scope = nameof(Create_AddsEntityCounter); + const string scope = nameof(TryCreate_AddsEntityCounter); var expected = new EntityCounter { Customer = 1, Next = 1, Scope = scope, Type = $"{scope}_type" }; // Act - await sut.Create(1, $"{scope}_type", scope); + var result = await sut.TryCreate(1, $"{scope}_type", scope); // Assert + result.Should().BeTrue("Counter created"); var saved = dbContext.EntityCounters.Single(ec => ec.Scope == scope); saved.Should().BeEquivalentTo(expected); } [Fact] - public async Task Create_Throws_IfRecordAlreadyExists_SameTypeScopeCustomer() + public async Task TryCreate_NoOp_ReturnsFalse_IfRecordAlreadyExists_SameTypeScopeCustomer() { - // NOTE - this tests behaviour rather than the behaviour being correct - // Arrange - const string scope = nameof(Create_Throws_IfRecordAlreadyExists_SameTypeScopeCustomer); - dbContext.EntityCounters.Add(new EntityCounter + const string scope = nameof(TryCreate_NoOp_ReturnsFalse_IfRecordAlreadyExists_SameTypeScopeCustomer); + var entityCounter = new EntityCounter { Customer = 1, Next = 9999, Scope = scope, Type = $"{scope}_type" - }); + }; + dbContext.EntityCounters.Add(entityCounter); await dbContext.SaveChangesAsync(); // Act - Func action = () => sut.Create(1, $"{scope}_type", scope); + var result = await sut.TryCreate(1, $"{scope}_type", scope); // Assert - await action.Should().ThrowAsync(); + result.Should().BeFalse("Counter already exists"); + var dbCounter = dbContext.EntityCounters.Single(ec => ec.Scope == scope); + dbCounter.Should().BeEquivalentTo(entityCounter); } [Fact] diff --git a/src/protagonist/DLCS.Repository/Entities/EntityCounterRepository.cs b/src/protagonist/DLCS.Repository/Entities/EntityCounterRepository.cs index 33980ba7d..2edd2cc12 100644 --- a/src/protagonist/DLCS.Repository/Entities/EntityCounterRepository.cs +++ b/src/protagonist/DLCS.Repository/Entities/EntityCounterRepository.cs @@ -1,31 +1,32 @@ using System.Threading.Tasks; using DLCS.Model; using Microsoft.EntityFrameworkCore; +using Microsoft.Extensions.Logging; namespace DLCS.Repository.Entities; public class EntityCounterRepository : IDapperContextRepository, IEntityCounterRepository { + private readonly ILogger logger; public DlcsContext DlcsContext { get; } - public EntityCounterRepository(DlcsContext dlcsContext) + public EntityCounterRepository(DlcsContext dlcsContext, ILogger logger) { + this.logger = logger; DlcsContext = dlcsContext; } - public async Task Create(int customer, string entityType, string scope, long initialValue = 1) + public async Task TryCreate(int customer, string entityType, string scope, long initialValue = 1) { - var ec = new EntityCounter - { - Customer = customer, - Type = entityType, - Scope = scope, - Next = initialValue - }; - await DlcsContext.EntityCounters.AddAsync(ec); - await DlcsContext.SaveChangesAsync(); + var exists = await Exists(customer, entityType, scope); + if (exists) return false; + + logger.LogInformation("Creating entityCounter {EntityType}:{Scope} for Customer {Customer}", entityType, + scope, customer); + await CreateEntity(customer, entityType, scope, initialValue); + return true; } - + public async Task Remove(int customer, string entityType, string scope, long nextValue) { var ec = new EntityCounter @@ -59,19 +60,23 @@ private Task Exists(int customer, string entityType, string scope) => DlcsContext.EntityCounters.AnyAsync(ec => ec.Customer == customer && ec.Type == entityType && ec.Scope == scope); - private async Task EnsureCounter(int customer, string entityType, string scope, long initialValue) + private async Task LongUpdate(string sql, int customer, string entityType, string scope, long initialValue = 1) { - var exists = await Exists(customer, entityType, scope); - if (!exists) - { - await Create(customer, entityType, scope, initialValue); - } + await TryCreate(customer, entityType, scope, initialValue); + return await this.QuerySingleAsync(sql, new { customer, entityType, scope }); } - private async Task LongUpdate(string sql, int customer, string entityType, string scope, long initialValue = 1) + private async Task CreateEntity(int customer, string entityType, string scope, long initialValue = 1) { - await EnsureCounter(customer, entityType, scope, initialValue); - return await this.QuerySingleAsync(sql, new { customer, entityType, scope }); + var ec = new EntityCounter + { + Customer = customer, + Type = entityType, + Scope = scope, + Next = initialValue + }; + await DlcsContext.EntityCounters.AddAsync(ec); + await DlcsContext.SaveChangesAsync(); } private const string GetNextSql = @" diff --git a/src/protagonist/DLCS.Repository/Spaces/SpaceRepository.cs b/src/protagonist/DLCS.Repository/Spaces/SpaceRepository.cs index d01ae7564..d42744318 100644 --- a/src/protagonist/DLCS.Repository/Spaces/SpaceRepository.cs +++ b/src/protagonist/DLCS.Repository/Spaces/SpaceRepository.cs @@ -21,7 +21,7 @@ public class SpaceRepository : ISpaceRepository private readonly IEntityCounterRepository entityCounterRepository; private readonly CacheSettings cacheSettings; private readonly IAppCache appCache; - private ILogger logger; + private readonly ILogger logger; private const int DefaultMaxUnauthorised = -1; @@ -88,7 +88,7 @@ public async Task CreateSpace(int customer, string name, string? imageBuc }; await dlcsContext.Spaces.AddAsync(space, cancellationToken); - await entityCounterRepository.Create(customer, KnownEntityCounters.SpaceImages, space.Id.ToString()); + await entityCounterRepository.TryCreate(customer, KnownEntityCounters.SpaceImages, space.Id.ToString()); await dlcsContext.SaveChangesAsync(cancellationToken); return space; } @@ -108,7 +108,6 @@ private async Task GetIdForNewSpace(int requestCustomer) return newModelId; } - private async Task GetSpaceInternal(int customerId, int spaceId, CancellationToken cancellationToken, string? name = null, @@ -261,7 +260,7 @@ public async Task PutSpace( }; await dlcsContext.Spaces.AddAsync(space, cancellationToken); - await entityCounterRepository.Create(customerId, KnownEntityCounters.SpaceImages, space.Id.ToString()); + await entityCounterRepository.TryCreate(customerId, KnownEntityCounters.SpaceImages, space.Id.ToString()); } await dlcsContext.SaveChangesAsync(cancellationToken); @@ -269,51 +268,43 @@ public async Task PutSpace( var retrievedSpace = await GetSpaceInternal(customerId, spaceId, cancellationToken, noCache:true); return retrievedSpace; } - - public async Task> DeleteSpace(int customerId, int spaceId, CancellationToken cancellationToken) - { - var deleteResult = DeleteResult.NotFound; - var message = string.Empty; + public async Task> DeleteSpace(int customerId, int spaceId, + CancellationToken cancellationToken) + { try { var space = await dlcsContext.Spaces.SingleOrDefaultAsync(s => s.Customer == customerId && s.Id == spaceId, cancellationToken: cancellationToken); - if (space != null) + if (space == null) { - var images = await dlcsContext.Images.AsNoTracking().FirstOrDefaultAsync(i => - i.Customer == customerId && i.Space == spaceId, cancellationToken); - - if (images == null) - { - dlcsContext.Spaces.Remove(space); - await dlcsContext.SaveChangesAsync(cancellationToken); - - await entityCounterRepository.Decrement(customerId, KnownEntityCounters.CustomerSpaces, - customerId.ToString()); - await entityCounterRepository.Remove(customerId, KnownEntityCounters.SpaceImages, - space.Id.ToString(), 1); - deleteResult = DeleteResult.Deleted; - } - else - { - deleteResult = DeleteResult.Conflict; - message = "Cannot delete a space with images"; - } + return new ResultMessage($"Space {spaceId} not found", DeleteResult.NotFound); } + + var hasImages = await dlcsContext.Images.AnyAsync(i => + i.Customer == customerId && i.Space == spaceId, cancellationToken); + + if (hasImages) + { + return new ResultMessage("Cannot delete a space with images", DeleteResult.Conflict); + } + + dlcsContext.Spaces.Remove(space); + await dlcsContext.SaveChangesAsync(cancellationToken); + + await entityCounterRepository.Decrement(customerId, KnownEntityCounters.CustomerSpaces, + customerId.ToString()); + await entityCounterRepository.Remove(customerId, KnownEntityCounters.SpaceImages, + space.Id.ToString(), 1); + return new ResultMessage(string.Empty, DeleteResult.Deleted); } catch (Exception exception) { - message = "Error deleting space"; - deleteResult = DeleteResult.Error; - logger.LogError(exception, "Failed to delete space"); + logger.LogError(exception, "Failed to delete space {Customer}/{Space}", customerId, spaceId); + return new ResultMessage("Error deleting space", DeleteResult.Error); } - - var resultMessage = new ResultMessage(message, deleteResult); - - return resultMessage; } - + private static long GetApproximateImages(long entityCounterNext) => Math.Max(entityCounterNext - 1, 0); } \ No newline at end of file