Cache null dictionary values by key (#15576)

* Add CacheNullValues option to RepositoryCachePolicy

* Cache null values in DictionaryByKeyRepository

* Fixed issue with nullable reference.

* Updated logic for caching of null values.

* Update src/Umbraco.Infrastructure/Cache/DefaultRepositoryCachePolicy.cs

Co-authored-by: Sven Geusens <[email protected]>

* Made the NullValueRepresentation overwritable in a generic manner

* Improve generic NullValueCachePolicyResolver

* Revert Commits and clarify logic with comment

This reverts commit 8befb43 "Improve generic NullValueCachePolicyResolver"
Also reverts 8adf0a2 - Made the NullValueRepresentation overwritable in a generic manner
And 8adf0a2 - Made the NullValueRepresentation overwritable in a generic manner

* Update src/Umbraco.Infrastructure/Cache/DefaultRepositoryCachePolicy.cs

---------

Co-authored-by: Andy Butland <[email protected]>
Co-authored-by: Sven Geusens <[email protected]>
Co-authored-by: Sven Geusens <[email protected]>

Author: 3 people

src/Umbraco.Core/Cache/RepositoryCachePolicyOptions.cs

@@ -11,6 +11,7 @@ public class RepositoryCachePolicyOptions
     public RepositoryCachePolicyOptions(Func<int> performCount)
     {
         PerformCount = performCount;
+        CacheNullValues = false;
         GetAllCacheValidateCount = true;
         GetAllCacheAllowZeroCount = false;
     }
@@ -21,6 +22,7 @@ public RepositoryCachePolicyOptions(Func<int> performCount)
     public RepositoryCachePolicyOptions()
     {
         PerformCount = null;
+        CacheNullValues = false;
         GetAllCacheValidateCount = false;
         GetAllCacheAllowZeroCount = false;
     }
@@ -30,6 +32,11 @@ public RepositoryCachePolicyOptions()
     /// </summary>
     public Func<int>? PerformCount { get; set; }
 
+    /// <summary>
+    ///     True if the Get method will cache null results so that the db is not hit for repeated lookups
+    /// </summary>
+    public bool CacheNullValues { get; set; }
+
     /// <summary>
     ///     True/false as to validate the total item count when all items are returned from cache, the default is true but this
     ///     means that a db lookup will occur - though that lookup will probably be significantly less expensive than the

src/Umbraco.Infrastructure/Cache/DefaultRepositoryCachePolicy.cs

@@ -24,6 +24,8 @@ public class DefaultRepositoryCachePolicy<TEntity, TId> : RepositoryCachePolicyB
     private static readonly TEntity[] _emptyEntities = new TEntity[0]; // const
     private readonly RepositoryCachePolicyOptions _options;
 
+    private const string NullRepresentationInCache = "*NULL*";
+
     public DefaultRepositoryCachePolicy(IAppPolicyCache cache, IScopeAccessor scopeAccessor, RepositoryCachePolicyOptions options)
         : base(cache, scopeAccessor) =>
         _options = options ?? throw new ArgumentNullException(nameof(options));
@@ -116,6 +118,7 @@ public override void Delete(TEntity entity, Action<TEntity> persistDeleted)
         {
             // whatever happens, clear the cache
             var cacheKey = GetEntityCacheKey(entity.Id);
+
             Cache.Clear(cacheKey);
 
             // if there's a GetAllCacheAllowZeroCount cache, ensure it is cleared
@@ -127,20 +130,36 @@ public override void Delete(TEntity entity, Action<TEntity> persistDeleted)
     public override TEntity? Get(TId? id, Func<TId?, TEntity?> performGet, Func<TId[]?, IEnumerable<TEntity>?> performGetAll)
     {
         var cacheKey = GetEntityCacheKey(id);
+
         TEntity? fromCache = Cache.GetCacheItem<TEntity>(cacheKey);
 
-        // if found in cache then return else fetch and cache
-        if (fromCache != null)
+        // If found in cache then return immediately.
+        if (fromCache is not null)
         {
             return fromCache;
         }
 
+        // Because TEntity can never be a string, we will never be in a position where the proxy value collides withs a real value.
+        // Therefore this point can only be reached if there is a proxy null value => becomes null when cast to TEntity above OR the item simply does not exist.
+        // If we've cached a "null" value, return null.
+        if (_options.CacheNullValues && Cache.GetCacheItem<string>(cacheKey) == NullRepresentationInCache)
+        {
+            return null;
+        }
+
+        // Otherwise go to the database to retrieve.
         TEntity? entity = performGet(id);
 
         if (entity != null && entity.HasIdentity)
         {
+            // If we've found an identified entity, cache it for subsequent retrieval.
             InsertEntity(cacheKey, entity);
         }
+        else if (entity is null && _options.CacheNullValues)
+        {
+            // If we've not found an entity, and we're caching null values, cache a "null" value.
+            InsertNull(cacheKey);
+        }
 
         return entity;
     }
@@ -248,6 +267,15 @@ protected string GetEntityCacheKey(TId? id)
     protected virtual void InsertEntity(string cacheKey, TEntity entity)
         => Cache.Insert(cacheKey, () => entity, TimeSpan.FromMinutes(5), true);
 
+    protected virtual void InsertNull(string cacheKey)
+    {
+        // We can't actually cache a null value, as in doing so wouldn't be able to distinguish between
+        // a value that does exist but isn't yet cached, or a value that has been explicitly cached with a null value.
+        // Both would return null when we retrieve from the cache and we couldn't distinguish between the two.
+        // So we cache a special value that represents null, and then we can check for that value when we retrieve from the cache.
+        Cache.Insert(cacheKey, () => NullRepresentationInCache, TimeSpan.FromMinutes(5), true);
+    }
+
     protected virtual void InsertEntities(TId[]? ids, TEntity[]? entities)
     {
         if (ids?.Length == 0 && entities?.Length == 0 && _options.GetAllCacheAllowZeroCount)

src/Umbraco.Infrastructure/Persistence/Repositories/Implement/DictionaryRepository.cs

@@ -102,11 +102,10 @@ protected override IRepositoryCachePolicy<IDictionaryItem, int> CreateCachePolic
         var options = new RepositoryCachePolicyOptions
         {
             // allow zero to be cached
-            GetAllCacheAllowZeroCount = true,
+            GetAllCacheAllowZeroCount = true
         };
 
-        return new SingleItemsOnlyRepositoryCachePolicy<IDictionaryItem, int>(GlobalIsolatedCache, ScopeAccessor,
-            options);
+        return new SingleItemsOnlyRepositoryCachePolicy<IDictionaryItem, int>(GlobalIsolatedCache, ScopeAccessor, options);
     }
 
     protected IDictionaryItem ConvertFromDto(DictionaryDto dto)
@@ -190,11 +189,10 @@ protected override IRepositoryCachePolicy<IDictionaryItem, Guid> CreateCachePoli
             var options = new RepositoryCachePolicyOptions
             {
                 // allow zero to be cached
-                GetAllCacheAllowZeroCount = true,
+                GetAllCacheAllowZeroCount = true
             };
 
-            return new SingleItemsOnlyRepositoryCachePolicy<IDictionaryItem, Guid>(GlobalIsolatedCache, ScopeAccessor,
-                options);
+            return new SingleItemsOnlyRepositoryCachePolicy<IDictionaryItem, Guid>(GlobalIsolatedCache, ScopeAccessor, options);
         }
     }
 
@@ -228,12 +226,13 @@ protected override IRepositoryCachePolicy<IDictionaryItem, string> CreateCachePo
         {
             var options = new RepositoryCachePolicyOptions
             {
+                // allow null to be cached
+                CacheNullValues = true,
                 // allow zero to be cached
-                GetAllCacheAllowZeroCount = true,
+                GetAllCacheAllowZeroCount = true
             };
 
-            return new SingleItemsOnlyRepositoryCachePolicy<IDictionaryItem, string>(GlobalIsolatedCache, ScopeAccessor,
-                options);
+            return new SingleItemsOnlyRepositoryCachePolicy<IDictionaryItem, string>(GlobalIsolatedCache, ScopeAccessor, options);
         }
     }