Merge pull request #956 from dlcs/bugfix/createspace_ec

Make EntityCounter create tolerant of existing

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<string, string>("ConnectionStrings:PostgreSQLConnection", dbFixture.ConnectionString) })
-            .Build();
-
-        var entityCounterRepo = new EntityCounterRepository(dbContext);
+
+        var entityCounterRepo = new EntityCounterRepository(dbContext, new NullLogger<EntityCounterRepository>());
 
         var assetRepositoryCachingHelper = new AssetCachingHelper(
             new MockCachingService(),

src/protagonist/API/Features/Application/Requests/SetupApplication.cs

@@ -59,7 +59,7 @@ public async Task<CreateApiKeyResult> 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)

src/protagonist/API/Features/Customer/Requests/CreateCustomer.cs

@@ -85,7 +85,7 @@ public async Task<ModifyEntityResult<CustomerEntity>> 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);

src/protagonist/DLCS.Model/IEntityCounterRepository.cs

@@ -11,7 +11,8 @@ public interface IEntityCounterRepository
     /// <summary>
     /// Create a new EntityCounter record with specified value.
     /// </summary>
-    Task Create(int customer, string entityType, string scope, long initialValue = 1);
+    /// <returns>True if EntityCounter created, else false</returns>
+    Task<bool> TryCreate(int customer, string entityType, string scope, long initialValue = 1);
 
     /// <summary>
     /// Removes an EntityCounter from the database

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<EntityCounterRepository>());
 
         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<Task> action = () => sut.Create(1, $"{scope}_type", scope);
+        var result = await sut.TryCreate(1, $"{scope}_type", scope);
 
         // Assert
-        await action.Should().ThrowAsync<InvalidOperationException>();
+        result.Should().BeFalse("Counter already exists");
+        var dbCounter = dbContext.EntityCounters.Single(ec => ec.Scope == scope);
+        dbCounter.Should().BeEquivalentTo(entityCounter);
     }
 
     [Fact]

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<EntityCounterRepository> logger;
     public DlcsContext DlcsContext { get; }
 
-    public EntityCounterRepository(DlcsContext dlcsContext)
+    public EntityCounterRepository(DlcsContext dlcsContext, ILogger<EntityCounterRepository> logger)
     {
+        this.logger = logger;
         DlcsContext = dlcsContext;
     }
 
-    public async Task Create(int customer, string entityType, string scope, long initialValue = 1)
+    public async Task<bool> 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<bool> 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<long> 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<long>(sql, new { customer, entityType, scope });
     }
 
-    private async Task<long> 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<long>(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 = @"

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<SpaceRepository> logger;
+    private readonly ILogger<SpaceRepository> logger;
 
     private const int DefaultMaxUnauthorised = -1;
 
@@ -88,7 +88,7 @@ public async Task<Space> 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<int> GetIdForNewSpace(int requestCustomer)
 
         return newModelId;
     }
-
 
     private async Task<Space?> GetSpaceInternal(int customerId, int spaceId, 
         CancellationToken cancellationToken, string? name = null,
@@ -261,59 +260,51 @@ public async Task<Space> 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);
 
         var retrievedSpace = await GetSpaceInternal(customerId, spaceId, cancellationToken, noCache:true);
         return retrievedSpace;
     }
-
-    public async Task<ResultMessage<DeleteResult>> DeleteSpace(int customerId, int spaceId, CancellationToken cancellationToken)
-    {
-        var deleteResult = DeleteResult.NotFound;
-        var message = string.Empty;
 
+    public async Task<ResultMessage<DeleteResult>> 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<DeleteResult>($"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<DeleteResult>("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<DeleteResult>(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<DeleteResult>("Error deleting space", DeleteResult.Error);
         }
-
-        var resultMessage = new ResultMessage<DeleteResult>(message, deleteResult);
-
-        return resultMessage;
     }
-    
+
     private static long GetApproximateImages(long entityCounterNext) => Math.Max(entityCounterNext - 1, 0);
 }