IAppCache implementations should not cache null values (#14218)

* IAppCache implementations should not cache null values

* Add comment

src/Umbraco.Core/Cache/DictionaryAppCache.cs

@@ -24,7 +24,19 @@ public class DictionaryAppCache : IRequestCache
     public virtual object? Get(string key) => _items.TryGetValue(key, out var value) ? value : null;
 
     /// <inheritdoc />
-    public virtual object? Get(string key, Func<object?> factory) => _items.GetOrAdd(key, _ => factory());
+    public virtual object? Get(string key, Func<object?> factory)
+    {
+        var value = _items.GetOrAdd(key, _ => factory());
+
+        if (value is not null)
+        {
+            return value;
+        }
+
+        // do not cache null values
+        _items.TryRemove(key, out _);
+        return null;
+    }
 
     public bool Set(string key, object? value) => _items.TryAdd(key, value);
 

src/Umbraco.Core/Cache/FastDictionaryAppCache.cs

@@ -31,6 +31,14 @@ public class FastDictionaryAppCache : IAppCache
         Lazy<object?>? result = _items.GetOrAdd(cacheKey, k => SafeLazy.GetSafeLazy(getCacheItem));
 
         var value = result.Value; // will not throw (safe lazy)
+
+        if (value is null)
+        {
+            // do not cache null values
+            _items.TryRemove(cacheKey, out _);
+            return null;
+        }
+
         if (!(value is SafeLazy.ExceptionHolder eh))
         {
             return value;

src/Umbraco.Core/Cache/IAppCache.cs

@@ -18,6 +18,7 @@ public interface IAppCache
     /// <param name="key">The key of the item.</param>
     /// <param name="factory">A factory function that can create the item.</param>
     /// <returns>The item.</returns>
+    /// <remarks>Null values returned from the factory function are never cached.</remarks>
     object? Get(string key, Func<object?> factory);
 
     /// <summary>

tests/Umbraco.Tests.UnitTests/Umbraco.Core/Cache/AppCacheTests.cs

@@ -81,6 +81,25 @@ public abstract class AppCacheTests
         Assert.Greater(counter, 1);
     }
 
+    [Test]
+    public void Does_Not_Cache_Null_Values()
+    {
+        var counter = 0;
+
+        object? Factory()
+        {
+            counter++;
+            return counter == 3 ? "Not a null value" : null;
+        }
+
+        object? Get() => AppCache.Get("Blah", Factory);
+
+        Assert.IsNull(Get());
+        Assert.IsNull(Get());
+        Assert.AreEqual("Not a null value", Get());
+        Assert.AreEqual(3, counter);
+    }
+
     [Test]
     public void Ensures_Delegate_Result_Is_Cached_Once()
     {

tests/Umbraco.Tests.UnitTests/Umbraco.Core/Cache/FastDictionaryAppCacheTests.cs

@@ -0,0 +1,20 @@
+using NUnit.Framework;
+using Umbraco.Cms.Core.Cache;
+
+namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Core.Cache;
+
+[TestFixture]
+public class FastDictionaryAppCacheTests : AppCacheTests
+{
+    public override void Setup()
+    {
+        base.Setup();
+        _appCache = new FastDictionaryAppCache();
+    }
+
+    private FastDictionaryAppCache _appCache;
+
+    internal override IAppCache AppCache => _appCache;
+
+    protected override int GetTotalItemCount => _appCache.Count;
+}