Caching: Fixes regression of the caching of null representations for missing dictionary items (closes #20336 for 16) (#20349)

* Ports fix to regression of the caching of null representations for missing dictionary items.

* Fixed error raised in code review.

---------

Co-authored-by: Kenn Jacobsen <[email protected]>

Author: AndyButland

src/Umbraco.Core/Cache/AppCacheExtensions.cs

@@ -41,27 +41,39 @@ public static void InsertCacheItem<T>(
     public static T? GetCacheItem<T>(this IAppCache provider, string cacheKey)
     {
         var result = provider.Get(cacheKey);
-        if (IsRetrievedItemNull(result))
+        if (result == null)
         {
             return default;
         }
 
+        // If we've retrieved the specific string that represents null in the cache, return it only if we are requesting it (via a typed request for a string).
+        // Otherwise consider it a null value.
+        if (RetrievedNullRepresentationInCache(result))
+        {
+            return RequestedNullRepresentationInCache<T>() ? (T)result : default;
+        }
+
         return result.TryConvertTo<T>().Result;
     }
 
     public static T? GetCacheItem<T>(this IAppCache provider, string cacheKey, Func<T> getCacheItem)
     {
         var result = provider.Get(cacheKey, () => getCacheItem());
-        if (IsRetrievedItemNull(result))
+        if (result == null)
         {
             return default;
         }
 
+        // If we've retrieved the specific string that represents null in the cache, return it only if we are requesting it (via a typed request for a string).
+        // Otherwise consider it a null value.
+        if (RetrievedNullRepresentationInCache(result))
+        {
+            return RequestedNullRepresentationInCache<T>() ? (T)result : default;
+        }
+
         return result.TryConvertTo<T>().Result;
     }
 
-    private static bool IsRetrievedItemNull(object? result) => result is null or (object)Cms.Core.Constants.Cache.NullRepresentationInCache;
-
     public static async Task<T?> GetCacheItemAsync<T>(
         this IAppPolicyCache provider,
         string cacheKey,
@@ -77,9 +89,25 @@ public static void InsertCacheItem<T>(
             provider.Insert(cacheKey, () => result, timeout, isSliding);
         }
 
-        return result == null ? default : result.TryConvertTo<T>().Result;
+        if (result == null)
+        {
+            return default;
+        }
+
+        // If we've retrieved the specific string that represents null in the cache, return it only if we are requesting it (via a typed request for a string).
+        // Otherwise consider it a null value.
+        if (RetrievedNullRepresentationInCache(result))
+        {
+            return RequestedNullRepresentationInCache<T>() ? (T)result : default;
+        }
+
+        return result.TryConvertTo<T>().Result;
     }
 
+    private static bool RetrievedNullRepresentationInCache(object result) => result == (object)Cms.Core.Constants.Cache.NullRepresentationInCache;
+
+    private static bool RequestedNullRepresentationInCache<T>() => typeof(T) == typeof(string);
+
     public static async Task InsertCacheItemAsync<T>(
         this IAppPolicyCache provider,
         string cacheKey,

tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/DictionaryRepositoryTest.cs

@@ -1,13 +1,16 @@
 // Copyright (c) Umbraco.
 // See LICENSE for more details.
 
-using System.Collections.Generic;
-using System.Linq;
+using Microsoft.Extensions.Logging;
+using Moq;
 using NUnit.Framework;
 using Umbraco.Cms.Core;
+using Umbraco.Cms.Core.Cache;
 using Umbraco.Cms.Core.Models;
 using Umbraco.Cms.Core.Persistence.Repositories;
 using Umbraco.Cms.Core.Services;
+using Umbraco.Cms.Infrastructure.Persistence.Repositories.Implement;
+using Umbraco.Cms.Infrastructure.Scoping;
 using Umbraco.Cms.Tests.Common.Testing;
 using Umbraco.Cms.Tests.Integration.Testing;
 
@@ -22,6 +25,16 @@ internal sealed class DictionaryRepositoryTest : UmbracoIntegrationTest
 
     private IDictionaryRepository CreateRepository() => GetRequiredService<IDictionaryRepository>();
 
+    private IDictionaryRepository CreateRepositoryWithCache(AppCaches cache) =>
+
+        // Create a repository with a real runtime cache.
+        new DictionaryRepository(
+            GetRequiredService<IScopeAccessor>(),
+            cache,
+            GetRequiredService<ILogger<DictionaryRepository>>(),
+            GetRequiredService<ILoggerFactory>(),
+            GetRequiredService<ILanguageRepository>());
+
     [Test]
     public async Task Can_Perform_Get_By_Key_On_DictionaryRepository()
     {
@@ -396,6 +409,134 @@ public void Can_Perform_GetDictionaryItemKeyMap_On_DictionaryRepository()
         }
     }
 
+    [Test]
+    public void Can_Perform_Cached_Request_For_Existing_Value_By_Key_On_DictionaryRepository_With_Cache()
+    {
+        var cache = AppCaches.Create(Mock.Of<IRequestCache>());
+        var repository = CreateRepositoryWithCache(cache);
+
+        using (ScopeProvider.CreateScope())
+        {
+            var dictionaryItem = repository.Get("Read More");
+
+            Assert.AreEqual("Read More", dictionaryItem.Translations.Single(x => x.LanguageIsoCode == "en-US").Value);
+        }
+
+        // Modify the value directly in the database. This won't be reflected in the repository cache and hence if the cache
+        // is working as expected we should get the same value as above.
+        using (var scope = ScopeProvider.CreateScope())
+        {
+            scope.Database.Execute("UPDATE cmsLanguageText SET value = 'Read More (updated)' WHERE value = 'Read More' and LanguageId = 1");
+            scope.Complete();
+        }
+
+        using (ScopeProvider.CreateScope())
+        {
+            var dictionaryItem = repository.Get("Read More");
+
+            Assert.AreEqual("Read More", dictionaryItem.Translations.Single(x => x.LanguageIsoCode == "en-US").Value);
+        }
+
+        cache.IsolatedCaches.ClearCache<IDictionaryItem>();
+        using (ScopeProvider.CreateScope())
+        {
+            var dictionaryItem = repository.Get("Read More");
+
+            Assert.AreEqual("Read More (updated)", dictionaryItem.Translations.Single(x => x.LanguageIsoCode == "en-US").Value);
+        }
+    }
+
+    [Test]
+    public void Can_Perform_Cached_Request_For_NonExisting_Value_By_Key_On_DictionaryRepository_With_Cache()
+    {
+        var cache = AppCaches.Create(Mock.Of<IRequestCache>());
+        var repository = CreateRepositoryWithCache(cache);
+
+        using (ScopeProvider.CreateScope())
+        {
+            var dictionaryItem = repository.Get("Read More Updated");
+
+            Assert.IsNull(dictionaryItem);
+        }
+
+        // Modify the value directly in the database such that it now exists. This won't be reflected in the repository cache and hence if the cache
+        // is working as expected we should get the same null value as above.
+        using (var scope = ScopeProvider.CreateScope())
+        {
+            scope.Database.Execute("UPDATE cmsDictionary SET [key] = 'Read More Updated' WHERE [key] = 'Read More'");
+            scope.Complete();
+        }
+
+        using (ScopeProvider.CreateScope())
+        {
+            var dictionaryItem = repository.Get("Read More Updated");
+
+            Assert.IsNull(dictionaryItem);
+        }
+
+        cache.IsolatedCaches.ClearCache<IDictionaryItem>();
+        using (ScopeProvider.CreateScope())
+        {
+            var dictionaryItem = repository.Get("Read More Updated");
+
+            Assert.IsNotNull(dictionaryItem);
+        }
+    }
+
+    [Test]
+    public void Cannot_Perform_Cached_Request_For_Existing_Value_By_Key_On_DictionaryRepository_Without_Cache()
+    {
+        var repository = CreateRepository();
+
+        using (ScopeProvider.CreateScope())
+        {
+            var dictionaryItem = repository.Get("Read More");
+
+            Assert.AreEqual("Read More", dictionaryItem.Translations.Single(x => x.LanguageIsoCode == "en-US").Value);
+        }
+
+        // Modify the value directly in the database. As we don't have caching enabled on the repository we should get the new value.
+        using (var scope = ScopeProvider.CreateScope())
+        {
+            scope.Database.Execute("UPDATE cmsLanguageText SET value = 'Read More (updated)' WHERE value = 'Read More' and LanguageId = 1");
+            scope.Complete();
+        }
+
+        using (ScopeProvider.CreateScope())
+        {
+            var dictionaryItem = repository.Get("Read More");
+
+            Assert.AreEqual("Read More (updated)", dictionaryItem.Translations.Single(x => x.LanguageIsoCode == "en-US").Value);
+        }
+    }
+
+    [Test]
+    public void Cannot_Perform_Cached_Request_For_NonExisting_Value_By_Key_On_DictionaryRepository_Without_Cache()
+    {
+        var repository = CreateRepository();
+
+        using (ScopeProvider.CreateScope())
+        {
+            var dictionaryItem = repository.Get("Read More Updated");
+
+            Assert.IsNull(dictionaryItem);
+        }
+
+        // Modify the value directly in the database such that it now exists. As we don't have caching enabled on the repository we should get the new value.
+        using (var scope = ScopeProvider.CreateScope())
+        {
+            scope.Database.Execute("UPDATE cmsDictionary SET [key] = 'Read More Updated' WHERE [key] = 'Read More'");
+            scope.Complete();
+        }
+
+        using (ScopeProvider.CreateScope())
+        {
+            var dictionaryItem = repository.Get("Read More Updated");
+
+            Assert.IsNotNull(dictionaryItem);
+        }
+    }
+
     public async Task CreateTestData()
     {
         var languageService = GetRequiredService<ILanguageService>();