fix: fix cache store leaving orphan tenant on some update scenarios (#946)

Author: AndrewTriesToCode

src/Finbuckle.MultiTenant/Stores/DistributedCacheStore/DistributedCacheStore.cs

@@ -11,7 +11,8 @@ namespace Finbuckle.MultiTenant.Stores.DistributedCacheStore;
 /// Basic store that uses an IDistributedCache instance as its backing. Note that GetAllAsync is not implemented.
 /// </summary>
 /// <typeparam name="TTenantInfo">The ITenantInfo implementation type.</typeparam>
-public class DistributedCacheStore<TTenantInfo> : IMultiTenantStore<TTenantInfo> where TTenantInfo : class, ITenantInfo, new()
+public class DistributedCacheStore<TTenantInfo> : IMultiTenantStore<TTenantInfo>
+    where TTenantInfo : class, ITenantInfo, new()
 {
     private readonly IDistributedCache cache;
     private readonly string keyPrefix;
@@ -38,7 +39,8 @@ public async Task<bool> TryAddAsync(TTenantInfo tenantInfo)
         var bytes = JsonSerializer.Serialize(tenantInfo);
 
         await cache.SetStringAsync($"{keyPrefix}id__{tenantInfo.Id}", bytes, options).ConfigureAwait(false);
-        await cache.SetStringAsync($"{keyPrefix}identifier__{tenantInfo.Identifier}", bytes, options).ConfigureAwait(false);
+        await cache.SetStringAsync($"{keyPrefix}identifier__{tenantInfo.Identifier}", bytes, options)
+            .ConfigureAwait(false);
 
         return true;
     }
@@ -96,9 +98,17 @@ public async Task<bool> TryRemoveAsync(string identifier)
     }
 
     /// <inheritdoc />
-    public Task<bool> TryUpdateAsync(TTenantInfo tenantInfo)
+    public async Task<bool> TryUpdateAsync(TTenantInfo tenantInfo)
     {
-        // Same as adding for distributed cache.
-        return TryAddAsync(tenantInfo);
+        if (tenantInfo.Id is null)
+            return false;
+        
+        var current = await TryGetAsync(tenantInfo.Id).ConfigureAwait(false);
+
+        if (current is null)
+            return false;
+
+        return await TryRemoveAsync(current.Identifier!).ConfigureAwait(false) &&
+               await TryAddAsync(tenantInfo).ConfigureAwait(false);
     }
 }

test/Finbuckle.MultiTenant.Test/Stores/DistributedCacheStoreShould.cs

@@ -1,22 +1,25 @@
 // Copyright Finbuckle LLC, Andrew White, and Contributors.
 // Refer to the solution LICENSE file for more information.
 
+using System.Text;
+using System.Text.Json;
 using Finbuckle.MultiTenant.Abstractions;
 using Finbuckle.MultiTenant.Internal;
 using Finbuckle.MultiTenant.Stores.DistributedCacheStore;
 using Microsoft.Extensions.Caching.Distributed;
 using Microsoft.Extensions.DependencyInjection;
+using Moq;
 using Xunit;
 
 namespace Finbuckle.MultiTenant.Test.Stores;
 
 public class DistributedCacheStoreShould : MultiTenantStoreTestBase
 {
     [Fact]
-    public void ThrownOnGetAllTenantsFromStoreAsync()
+    public async Task ThrowOnGetAllTenantsFromStoreAsync()
     {
         var store = CreateTestStore();
-        Assert.Throws<NotImplementedException>(() => store.GetAllAsync().Wait());
+        await Assert.ThrowsAsync<NotImplementedException>(async () => await store.GetAllAsync());
     }
 
     [Fact]
@@ -45,7 +48,7 @@ public async Task RemoveReturnsFalseWhenNoMatchingIdentifierFound()
     }
 
     [Fact]
-    public async Task AddDualEntriesOnAddOrUpdate()
+    public async Task AddDualEntriesOnAdd()
     {
         var store = CreateTestStore();
 
@@ -61,34 +64,69 @@ public async Task AddDualEntriesOnAddOrUpdate()
     }
 
     [Fact]
-    public async Task RefreshDualEntriesOnAddOrUpdate()
+    public async Task RefreshDualEntriesOnTryGet()
     {
-        var store = CreateTestStore();
-        await Task.Delay(1500);
+        var cache = new Mock<IDistributedCache>();
+        cache.Setup(c => c.GetAsync(It.IsAny<string>(), It.IsAny<CancellationToken>()))
+            .ReturnsAsync(Encoding.UTF8.GetBytes(JsonSerializer.Serialize(new TenantInfo
+                { Id = "lol-id", Identifier = "lol" })));
+        
+        var store = new DistributedCacheStore<TenantInfo>(cache.Object, Constants.TenantToken, TimeSpan.FromSeconds(1));
+
         var t1 = await store.TryGetAsync("lol-id");
-        await Task.Delay(1500);
-        var t2 = await store.TryGetByIdentifierAsync("lol");
-        await Task.Delay(1500);
-        var t3 = await store.TryGetAsync("lol-id");
+        cache.Verify(c => c.RefreshAsync(It.IsAny<string>(), CancellationToken.None), Times.Once);
+    }
 
-        Assert.NotNull(t1);
-        Assert.NotNull(t2);
-        Assert.NotNull(t3);
-        Assert.Equal("lol-id", t1.Id);
-        Assert.Equal("lol-id", t2.Id);
-        Assert.Equal("lol-id", t3.Id);
+    [Fact]
+    public async Task RefreshDualEntriesOnTryGetByIdentifier()
+    {
+        var cache = new Mock<IDistributedCache>();
+        cache.Setup(c => c.GetAsync(It.IsAny<string>(), It.IsAny<CancellationToken>()))
+            .ReturnsAsync(Encoding.UTF8.GetBytes(JsonSerializer.Serialize(new TenantInfo
+                { Id = "lol-id", Identifier = "lol" })));
+        
+        var store = new DistributedCacheStore<TenantInfo>(cache.Object, Constants.TenantToken, TimeSpan.FromSeconds(1));
+
+        var t1 = await store.TryGetByIdentifierAsync("lol-id");
+        cache.Verify(c => c.RefreshAsync(It.IsAny<string>(), CancellationToken.None), Times.Once);
     }
+    
+    [Fact]
+    public async Task SetSlidingExpirationOnAdd()
+    {
+        var cache = new Mock<IDistributedCache>();
+        var options = new DistributedCacheEntryOptions { SlidingExpiration = TimeSpan.FromSeconds(1) };
+
+        cache.Setup(c => c.SetAsync(It.IsAny<string>(), It.IsAny<byte[]>(),
+                It.IsAny<DistributedCacheEntryOptions>(), It.IsAny<CancellationToken>()))
+            .Callback<string, byte[], DistributedCacheEntryOptions, CancellationToken>((key, value, opts, token) =>
+            {
+                Assert.Equal(options.SlidingExpiration, opts.SlidingExpiration);
+            })
+            .Returns(Task.CompletedTask);
+
+        var store = new DistributedCacheStore<TenantInfo>(cache.Object, Constants.TenantToken, TimeSpan.FromSeconds(1));
 
+        await store.TryAddAsync(new TenantInfo { Id = "test-id", Identifier = "test", Name = "Test Tenant" });
+    }
+    
     [Fact]
-    public async Task ExpireDualEntriesAfterTimespan()
+    public async Task SetSlidingExpirationOnUpdate()
     {
-        var store = CreateTestStore();
-        await Task.Delay(2100);
-        var t1 = await store.TryGetAsync("lol-id");
-        var t2 = await store.TryGetByIdentifierAsync("lol");
+        var cache = new Mock<IDistributedCache>();
+        var options = new DistributedCacheEntryOptions { SlidingExpiration = TimeSpan.FromSeconds(1) };
 
-        Assert.Null(t1);
-        Assert.Null(t2);
+        cache.Setup(c => c.SetAsync(It.IsAny<string>(), It.IsAny<byte[]>(),
+                It.IsAny<DistributedCacheEntryOptions>(), It.IsAny<CancellationToken>()))
+            .Callback<string, byte[], DistributedCacheEntryOptions, CancellationToken>((key, value, opts, token) =>
+            {
+                Assert.Equal(options.SlidingExpiration, opts.SlidingExpiration);
+            })
+            .Returns(Task.CompletedTask);
+
+        var store = new DistributedCacheStore<TenantInfo>(cache.Object, Constants.TenantToken, TimeSpan.FromSeconds(1));
+
+        await store.TryAddAsync(new TenantInfo { Id = "test-id", Identifier = "test", Name = "Test Tenant" });
     }
 
     // Basic store functionality tested in MultiTenantStoresShould.cs
@@ -99,7 +137,8 @@ protected override IMultiTenantStore<TenantInfo> CreateTestStore()
         services.AddOptions().AddDistributedMemoryCache();
         var sp = services.BuildServiceProvider();
 
-        var store = new DistributedCacheStore<TenantInfo>(sp.GetRequiredService<IDistributedCache>(), Constants.TenantToken, TimeSpan.FromMilliseconds(2000));
+        var store = new DistributedCacheStore<TenantInfo>(sp.GetRequiredService<IDistributedCache>(),
+            Constants.TenantToken, TimeSpan.MaxValue);
 
         return PopulateTestStore(store);
     }