From e2bb34b4297adfa5082de1519e218724b8529821 Mon Sep 17 00:00:00 2001 From: Callum Whyte Date: Mon, 15 Jan 2024 10:09:10 +0000 Subject: [PATCH 1/9] Add CacheNullValues option to RepositoryCachePolicy --- src/Umbraco.Core/Cache/RepositoryCachePolicyOptions.cs | 7 +++++++ .../Cache/DefaultRepositoryCachePolicy.cs | 4 +++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/src/Umbraco.Core/Cache/RepositoryCachePolicyOptions.cs b/src/Umbraco.Core/Cache/RepositoryCachePolicyOptions.cs index ba7b251aa0fc..fa334e5c4a21 100644 --- a/src/Umbraco.Core/Cache/RepositoryCachePolicyOptions.cs +++ b/src/Umbraco.Core/Cache/RepositoryCachePolicyOptions.cs @@ -11,6 +11,7 @@ public class RepositoryCachePolicyOptions public RepositoryCachePolicyOptions(Func performCount) { PerformCount = performCount; + CacheNullValues = false; GetAllCacheValidateCount = true; GetAllCacheAllowZeroCount = false; } @@ -21,6 +22,7 @@ public RepositoryCachePolicyOptions(Func performCount) public RepositoryCachePolicyOptions() { PerformCount = null; + CacheNullValues = false; GetAllCacheValidateCount = false; GetAllCacheAllowZeroCount = false; } @@ -30,6 +32,11 @@ public RepositoryCachePolicyOptions() /// public Func? PerformCount { get; set; } + /// + /// True if the Get method will cache null results so that the db is not hit for repeated lookups + /// + public bool CacheNullValues { get; set; } + /// /// 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 diff --git a/src/Umbraco.Infrastructure/Cache/DefaultRepositoryCachePolicy.cs b/src/Umbraco.Infrastructure/Cache/DefaultRepositoryCachePolicy.cs index 7f7f8d678422..def115e130a7 100644 --- a/src/Umbraco.Infrastructure/Cache/DefaultRepositoryCachePolicy.cs +++ b/src/Umbraco.Infrastructure/Cache/DefaultRepositoryCachePolicy.cs @@ -116,6 +116,7 @@ public override void Delete(TEntity entity, Action 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,6 +128,7 @@ public override void Delete(TEntity entity, Action persistDeleted) public override TEntity? Get(TId? id, Func performGet, Func?> performGetAll) { var cacheKey = GetEntityCacheKey(id); + TEntity? fromCache = Cache.GetCacheItem(cacheKey); // if found in cache then return else fetch and cache @@ -137,7 +139,7 @@ public override void Delete(TEntity entity, Action persistDeleted) TEntity? entity = performGet(id); - if (entity != null && entity.HasIdentity) + if ((entity != null && entity.HasIdentity) || _options.CacheNullValues) { InsertEntity(cacheKey, entity); } From 16f324e6a5c9e7639a148ea47dcc8bce41583db2 Mon Sep 17 00:00:00 2001 From: Callum Whyte Date: Mon, 15 Jan 2024 10:09:37 +0000 Subject: [PATCH 2/9] Cache null values in DictionaryByKeyRepository --- .../Implement/DictionaryRepository.cs | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/DictionaryRepository.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/DictionaryRepository.cs index 909c9cfec23e..bf4799e938a0 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/DictionaryRepository.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/DictionaryRepository.cs @@ -102,11 +102,10 @@ protected override IRepositoryCachePolicy CreateCachePolic var options = new RepositoryCachePolicyOptions { // allow zero to be cached - GetAllCacheAllowZeroCount = true, + GetAllCacheAllowZeroCount = true }; - return new SingleItemsOnlyRepositoryCachePolicy(GlobalIsolatedCache, ScopeAccessor, - options); + return new SingleItemsOnlyRepositoryCachePolicy(GlobalIsolatedCache, ScopeAccessor, options); } protected IDictionaryItem ConvertFromDto(DictionaryDto dto) @@ -190,11 +189,10 @@ protected override IRepositoryCachePolicy CreateCachePoli var options = new RepositoryCachePolicyOptions { // allow zero to be cached - GetAllCacheAllowZeroCount = true, + GetAllCacheAllowZeroCount = true }; - return new SingleItemsOnlyRepositoryCachePolicy(GlobalIsolatedCache, ScopeAccessor, - options); + return new SingleItemsOnlyRepositoryCachePolicy(GlobalIsolatedCache, ScopeAccessor, options); } } @@ -228,12 +226,13 @@ protected override IRepositoryCachePolicy CreateCachePo { var options = new RepositoryCachePolicyOptions { + // allow null to be cached + CacheNullValues = true, // allow zero to be cached - GetAllCacheAllowZeroCount = true, + GetAllCacheAllowZeroCount = true }; - return new SingleItemsOnlyRepositoryCachePolicy(GlobalIsolatedCache, ScopeAccessor, - options); + return new SingleItemsOnlyRepositoryCachePolicy(GlobalIsolatedCache, ScopeAccessor, options); } } From deec4c87b1f4c27ec7f3cd6511279afd62fb8e70 Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Tue, 28 Jan 2025 13:35:09 +0100 Subject: [PATCH 3/9] Fixed issue with nullable reference. --- .../Cache/DefaultRepositoryCachePolicy.cs | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/Umbraco.Infrastructure/Cache/DefaultRepositoryCachePolicy.cs b/src/Umbraco.Infrastructure/Cache/DefaultRepositoryCachePolicy.cs index def115e130a7..51c3d6ea0030 100644 --- a/src/Umbraco.Infrastructure/Cache/DefaultRepositoryCachePolicy.cs +++ b/src/Umbraco.Infrastructure/Cache/DefaultRepositoryCachePolicy.cs @@ -139,10 +139,14 @@ public override void Delete(TEntity entity, Action persistDeleted) TEntity? entity = performGet(id); - if ((entity != null && entity.HasIdentity) || _options.CacheNullValues) + if (entity != null && entity.HasIdentity) { InsertEntity(cacheKey, entity); } + else if (entity is null && _options.CacheNullValues) + { + InsertNull(cacheKey); + } return entity; } @@ -248,6 +252,12 @@ protected string GetEntityCacheKey(TId? id) } protected virtual void InsertEntity(string cacheKey, TEntity entity) + => InsertNullableEntity(cacheKey, entity); + + protected virtual void InsertNull(string cacheKey) + => InsertNullableEntity(cacheKey, null); + + private void InsertNullableEntity(string cacheKey, TEntity? entity) => Cache.Insert(cacheKey, () => entity, TimeSpan.FromMinutes(5), true); protected virtual void InsertEntities(TId[]? ids, TEntity[]? entities) From 2acb28c4ccce20fd297df2e52fcb7e918c849dc4 Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Tue, 28 Jan 2025 14:03:59 +0100 Subject: [PATCH 4/9] Updated logic for caching of null values. --- .../Cache/DefaultRepositoryCachePolicy.cs | 28 ++++++++++++++----- 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/src/Umbraco.Infrastructure/Cache/DefaultRepositoryCachePolicy.cs b/src/Umbraco.Infrastructure/Cache/DefaultRepositoryCachePolicy.cs index 51c3d6ea0030..cb44208c4e53 100644 --- a/src/Umbraco.Infrastructure/Cache/DefaultRepositoryCachePolicy.cs +++ b/src/Umbraco.Infrastructure/Cache/DefaultRepositoryCachePolicy.cs @@ -24,6 +24,8 @@ public class DefaultRepositoryCachePolicy : 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)); @@ -131,20 +133,29 @@ public override void Delete(TEntity entity, Action persistDeleted) TEntity? fromCache = Cache.GetCacheItem(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; } + // If we've cached a "null" value, return null. + if (_options.CacheNullValues && Cache.GetCacheItem(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); } @@ -252,13 +263,16 @@ protected string GetEntityCacheKey(TId? id) } protected virtual void InsertEntity(string cacheKey, TEntity entity) - => InsertNullableEntity(cacheKey, entity); + => Cache.Insert(cacheKey, () => entity, TimeSpan.FromMinutes(5), true); protected virtual void InsertNull(string cacheKey) - => InsertNullableEntity(cacheKey, null); - - private void InsertNullableEntity(string cacheKey, TEntity? entity) - => Cache.Insert(cacheKey, () => entity, TimeSpan.FromMinutes(5), true); + { + // 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) { From f2e252a9921988ed7a0f0eec77193d41ca3301e0 Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Mon, 3 Feb 2025 16:01:37 +0100 Subject: [PATCH 5/9] Update src/Umbraco.Infrastructure/Cache/DefaultRepositoryCachePolicy.cs Co-authored-by: Sven Geusens --- .../Cache/DefaultRepositoryCachePolicy.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Umbraco.Infrastructure/Cache/DefaultRepositoryCachePolicy.cs b/src/Umbraco.Infrastructure/Cache/DefaultRepositoryCachePolicy.cs index cb44208c4e53..79f8a2a1104e 100644 --- a/src/Umbraco.Infrastructure/Cache/DefaultRepositoryCachePolicy.cs +++ b/src/Umbraco.Infrastructure/Cache/DefaultRepositoryCachePolicy.cs @@ -24,7 +24,7 @@ public class DefaultRepositoryCachePolicy : RepositoryCachePolicyB private static readonly TEntity[] _emptyEntities = new TEntity[0]; // const private readonly RepositoryCachePolicyOptions _options; - private const string NullRepresentationInCache = "*NULL*"; + private const string NullRepresentationInCache = "Umbraco.Cms.Core.Cache.DefaultRepositoryCachePolicy.NullRepresentationInCache"; public DefaultRepositoryCachePolicy(IAppPolicyCache cache, IScopeAccessor scopeAccessor, RepositoryCachePolicyOptions options) : base(cache, scopeAccessor) => From 8adf0a28ced5f489d7625da7417f543ca67783d1 Mon Sep 17 00:00:00 2001 From: Sven Geusens Date: Mon, 3 Feb 2025 17:17:51 +0100 Subject: [PATCH 6/9] Made the NullValueRepresentation overwritable in a generic manner --- .../DictionaryNullValueCachePolicyResolver.cs | 6 +++ .../Cache/INullValueCachePolicyResolver.cs | 6 +++ .../Cache/RepositoryCachePolicyOptions.cs | 7 ++- .../Cache/DefaultRepositoryCachePolicy.cs | 10 ++-- .../UmbracoBuilder.Repositories.cs | 2 + .../Implement/DictionaryRepository.cs | 52 ++++++++++++++----- 6 files changed, 61 insertions(+), 22 deletions(-) create mode 100644 src/Umbraco.Core/Cache/DictionaryNullValueCachePolicyResolver.cs create mode 100644 src/Umbraco.Core/Cache/INullValueCachePolicyResolver.cs diff --git a/src/Umbraco.Core/Cache/DictionaryNullValueCachePolicyResolver.cs b/src/Umbraco.Core/Cache/DictionaryNullValueCachePolicyResolver.cs new file mode 100644 index 000000000000..8dee9c07a1c7 --- /dev/null +++ b/src/Umbraco.Core/Cache/DictionaryNullValueCachePolicyResolver.cs @@ -0,0 +1,6 @@ +namespace Umbraco.Cms.Core.Cache; + +public class DictionaryNullValueCachePolicyResolver : INullValueCachePolicyResolver +{ + public string GetNullValue() => "Umbraco.Cms.Core.Cache.DefaultRepositoryCachePolicy.NullRepresentationInCache"; +} diff --git a/src/Umbraco.Core/Cache/INullValueCachePolicyResolver.cs b/src/Umbraco.Core/Cache/INullValueCachePolicyResolver.cs new file mode 100644 index 000000000000..5a564d23ef62 --- /dev/null +++ b/src/Umbraco.Core/Cache/INullValueCachePolicyResolver.cs @@ -0,0 +1,6 @@ +namespace Umbraco.Cms.Core.Cache; + +public interface INullValueCachePolicyResolver +{ + public string? GetNullValue(); +} diff --git a/src/Umbraco.Core/Cache/RepositoryCachePolicyOptions.cs b/src/Umbraco.Core/Cache/RepositoryCachePolicyOptions.cs index fa334e5c4a21..525bb7c3ba7e 100644 --- a/src/Umbraco.Core/Cache/RepositoryCachePolicyOptions.cs +++ b/src/Umbraco.Core/Cache/RepositoryCachePolicyOptions.cs @@ -11,7 +11,6 @@ public class RepositoryCachePolicyOptions public RepositoryCachePolicyOptions(Func performCount) { PerformCount = performCount; - CacheNullValues = false; GetAllCacheValidateCount = true; GetAllCacheAllowZeroCount = false; } @@ -22,7 +21,6 @@ public RepositoryCachePolicyOptions(Func performCount) public RepositoryCachePolicyOptions() { PerformCount = null; - CacheNullValues = false; GetAllCacheValidateCount = false; GetAllCacheAllowZeroCount = false; } @@ -33,9 +31,10 @@ public RepositoryCachePolicyOptions() public Func? PerformCount { get; set; } /// - /// True if the Get method will cache null results so that the db is not hit for repeated lookups + /// If not null, the Get method will cache null results so that the db is not hit for repeated lookups + /// The value of this property will be used as the proxy value for the given key to populate the cache, but the return value will be null /// - public bool CacheNullValues { get; set; } + public string? NullValueRepresentation { get; set; } /// /// True/false as to validate the total item count when all items are returned from cache, the default is true but this diff --git a/src/Umbraco.Infrastructure/Cache/DefaultRepositoryCachePolicy.cs b/src/Umbraco.Infrastructure/Cache/DefaultRepositoryCachePolicy.cs index 79f8a2a1104e..0d6d04062638 100644 --- a/src/Umbraco.Infrastructure/Cache/DefaultRepositoryCachePolicy.cs +++ b/src/Umbraco.Infrastructure/Cache/DefaultRepositoryCachePolicy.cs @@ -1,6 +1,8 @@ // Copyright (c) Umbraco. // See LICENSE for more details. +using System.Diagnostics; +using Umbraco.Cms.Core.Models; using Umbraco.Cms.Core.Models.Entities; using Umbraco.Cms.Infrastructure.Scoping; using Umbraco.Extensions; @@ -24,8 +26,6 @@ public class DefaultRepositoryCachePolicy : RepositoryCachePolicyB private static readonly TEntity[] _emptyEntities = new TEntity[0]; // const private readonly RepositoryCachePolicyOptions _options; - private const string NullRepresentationInCache = "Umbraco.Cms.Core.Cache.DefaultRepositoryCachePolicy.NullRepresentationInCache"; - public DefaultRepositoryCachePolicy(IAppPolicyCache cache, IScopeAccessor scopeAccessor, RepositoryCachePolicyOptions options) : base(cache, scopeAccessor) => _options = options ?? throw new ArgumentNullException(nameof(options)); @@ -140,7 +140,7 @@ public override void Delete(TEntity entity, Action persistDeleted) } // If we've cached a "null" value, return null. - if (_options.CacheNullValues && Cache.GetCacheItem(cacheKey) == NullRepresentationInCache) + if (_options.NullValueRepresentation is not null && Cache.GetCacheItem(cacheKey) == _options.NullValueRepresentation) { return null; } @@ -153,7 +153,7 @@ public override void Delete(TEntity entity, Action persistDeleted) // If we've found an identified entity, cache it for subsequent retrieval. InsertEntity(cacheKey, entity); } - else if (entity is null && _options.CacheNullValues) + else if (entity is null && _options.NullValueRepresentation is not null) { // If we've not found an entity, and we're caching null values, cache a "null" value. InsertNull(cacheKey); @@ -271,7 +271,7 @@ protected virtual void InsertNull(string cacheKey) // 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); + Cache.Insert(cacheKey, () => _options.NullValueRepresentation, TimeSpan.FromMinutes(5), true); } protected virtual void InsertEntities(TId[]? ids, TEntity[]? entities) diff --git a/src/Umbraco.Infrastructure/DependencyInjection/UmbracoBuilder.Repositories.cs b/src/Umbraco.Infrastructure/DependencyInjection/UmbracoBuilder.Repositories.cs index 8c9f4b119378..e488a729d5d8 100644 --- a/src/Umbraco.Infrastructure/DependencyInjection/UmbracoBuilder.Repositories.cs +++ b/src/Umbraco.Infrastructure/DependencyInjection/UmbracoBuilder.Repositories.cs @@ -1,4 +1,5 @@ using Microsoft.Extensions.DependencyInjection; +using Umbraco.Cms.Core.Cache; using Umbraco.Cms.Core.DependencyInjection; using Umbraco.Cms.Core.Persistence.Repositories; using Umbraco.Cms.Core.DynamicRoot; @@ -27,6 +28,7 @@ internal static IUmbracoBuilder AddRepositories(this IUmbracoBuilder builder) builder.Services.AddUnique(); builder.Services.AddUnique(); builder.Services.AddUnique(); + builder.Services.AddUnique, DictionaryNullValueCachePolicyResolver>(); builder.Services.AddUnique(); builder.Services.AddUnique(); builder.Services.AddUnique(); diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/DictionaryRepository.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/DictionaryRepository.cs index bf4799e938a0..bfcb9e754a87 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/DictionaryRepository.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/DictionaryRepository.cs @@ -1,7 +1,9 @@ +using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; using NPoco; using Umbraco.Cms.Core; using Umbraco.Cms.Core.Cache; +using Umbraco.Cms.Core.DependencyInjection; using Umbraco.Cms.Core.Models; using Umbraco.Cms.Core.Models.Entities; using Umbraco.Cms.Core.Persistence.Querying; @@ -20,11 +22,19 @@ namespace Umbraco.Cms.Infrastructure.Persistence.Repositories.Implement; internal class DictionaryRepository : EntityRepositoryBase, IDictionaryRepository { private readonly ILoggerFactory _loggerFactory; - - public DictionaryRepository(IScopeAccessor scopeAccessor, AppCaches cache, ILogger logger, - ILoggerFactory loggerFactory) - : base(scopeAccessor, cache, logger) => + private readonly INullValueCachePolicyResolver _nullValueCachePolicyResolver; + + public DictionaryRepository( + IScopeAccessor scopeAccessor, + AppCaches cache, + ILogger logger, + ILoggerFactory loggerFactory, + INullValueCachePolicyResolver nullValueCachePolicyResolver) + : base(scopeAccessor, cache, logger) + { _loggerFactory = loggerFactory; + _nullValueCachePolicyResolver = nullValueCachePolicyResolver; + } public IDictionaryItem? Get(Guid uniqueId) { @@ -42,15 +52,23 @@ public IEnumerable GetMany(params Guid[] uniqueIds) public IDictionaryItem? Get(string key) { - var keyRepo = new DictionaryByKeyRepository(this, ScopeAccessor, AppCaches, - _loggerFactory.CreateLogger()); + var keyRepo = new DictionaryByKeyRepository( + this, + ScopeAccessor, + AppCaches, + _loggerFactory.CreateLogger(), + _nullValueCachePolicyResolver); return keyRepo.Get(key); } public IEnumerable GetManyByKeys(string[] keys) { - var keyRepo = new DictionaryByKeyRepository(this, ScopeAccessor, AppCaches, - _loggerFactory.CreateLogger()); + var keyRepo = new DictionaryByKeyRepository( + this, + ScopeAccessor, + AppCaches, + _loggerFactory.CreateLogger(), + _nullValueCachePolicyResolver); return keyRepo.GetMany(keys); } @@ -199,11 +217,19 @@ protected override IRepositoryCachePolicy CreateCachePoli private class DictionaryByKeyRepository : SimpleGetRepository { private readonly DictionaryRepository _dictionaryRepository; - - public DictionaryByKeyRepository(DictionaryRepository dictionaryRepository, IScopeAccessor scopeAccessor, - AppCaches cache, ILogger logger) - : base(scopeAccessor, cache, logger) => + private readonly INullValueCachePolicyResolver _nullValueCachePolicyResolver; + + public DictionaryByKeyRepository( + DictionaryRepository dictionaryRepository, + IScopeAccessor scopeAccessor, + AppCaches cache, + ILogger logger, + INullValueCachePolicyResolver nullValueCachePolicyResolver) + : base(scopeAccessor, cache, logger) + { _dictionaryRepository = dictionaryRepository; + _nullValueCachePolicyResolver = nullValueCachePolicyResolver; + } protected override IEnumerable PerformFetch(Sql sql) => Database @@ -227,7 +253,7 @@ protected override IRepositoryCachePolicy CreateCachePo var options = new RepositoryCachePolicyOptions { // allow null to be cached - CacheNullValues = true, + NullValueRepresentation = _nullValueCachePolicyResolver.GetNullValue(), // allow zero to be cached GetAllCacheAllowZeroCount = true }; From 8befb437921cb6e3b87725cefb92a6afbf3d28fb Mon Sep 17 00:00:00 2001 From: Sven Geusens Date: Mon, 3 Feb 2025 17:36:40 +0100 Subject: [PATCH 7/9] Improve generic NullValueCachePolicyResolver --- .../Cache/DictionaryNullValueCachePolicyResolver.cs | 4 +++- .../DependencyInjection/UmbracoBuilder.Repositories.cs | 2 +- .../Repositories/Implement/DictionaryRepository.cs | 8 ++++---- 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/Umbraco.Core/Cache/DictionaryNullValueCachePolicyResolver.cs b/src/Umbraco.Core/Cache/DictionaryNullValueCachePolicyResolver.cs index 8dee9c07a1c7..644142070479 100644 --- a/src/Umbraco.Core/Cache/DictionaryNullValueCachePolicyResolver.cs +++ b/src/Umbraco.Core/Cache/DictionaryNullValueCachePolicyResolver.cs @@ -1,6 +1,8 @@ +using Umbraco.Cms.Core.Persistence.Repositories; + namespace Umbraco.Cms.Core.Cache; -public class DictionaryNullValueCachePolicyResolver : INullValueCachePolicyResolver +public class DictionaryNullValueCachePolicyResolver: INullValueCachePolicyResolver { public string GetNullValue() => "Umbraco.Cms.Core.Cache.DefaultRepositoryCachePolicy.NullRepresentationInCache"; } diff --git a/src/Umbraco.Infrastructure/DependencyInjection/UmbracoBuilder.Repositories.cs b/src/Umbraco.Infrastructure/DependencyInjection/UmbracoBuilder.Repositories.cs index e488a729d5d8..7a3ec9032dec 100644 --- a/src/Umbraco.Infrastructure/DependencyInjection/UmbracoBuilder.Repositories.cs +++ b/src/Umbraco.Infrastructure/DependencyInjection/UmbracoBuilder.Repositories.cs @@ -28,7 +28,7 @@ internal static IUmbracoBuilder AddRepositories(this IUmbracoBuilder builder) builder.Services.AddUnique(); builder.Services.AddUnique(); builder.Services.AddUnique(); - builder.Services.AddUnique, DictionaryNullValueCachePolicyResolver>(); + builder.Services.AddUnique, DictionaryNullValueCachePolicyResolver>(); builder.Services.AddUnique(); builder.Services.AddUnique(); builder.Services.AddUnique(); diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/DictionaryRepository.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/DictionaryRepository.cs index bfcb9e754a87..1af27a5614b3 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/DictionaryRepository.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/DictionaryRepository.cs @@ -22,14 +22,14 @@ namespace Umbraco.Cms.Infrastructure.Persistence.Repositories.Implement; internal class DictionaryRepository : EntityRepositoryBase, IDictionaryRepository { private readonly ILoggerFactory _loggerFactory; - private readonly INullValueCachePolicyResolver _nullValueCachePolicyResolver; + private readonly INullValueCachePolicyResolver _nullValueCachePolicyResolver; public DictionaryRepository( IScopeAccessor scopeAccessor, AppCaches cache, ILogger logger, ILoggerFactory loggerFactory, - INullValueCachePolicyResolver nullValueCachePolicyResolver) + INullValueCachePolicyResolver nullValueCachePolicyResolver) : base(scopeAccessor, cache, logger) { _loggerFactory = loggerFactory; @@ -217,14 +217,14 @@ protected override IRepositoryCachePolicy CreateCachePoli private class DictionaryByKeyRepository : SimpleGetRepository { private readonly DictionaryRepository _dictionaryRepository; - private readonly INullValueCachePolicyResolver _nullValueCachePolicyResolver; + private readonly INullValueCachePolicyResolver _nullValueCachePolicyResolver; public DictionaryByKeyRepository( DictionaryRepository dictionaryRepository, IScopeAccessor scopeAccessor, AppCaches cache, ILogger logger, - INullValueCachePolicyResolver nullValueCachePolicyResolver) + INullValueCachePolicyResolver nullValueCachePolicyResolver) : base(scopeAccessor, cache, logger) { _dictionaryRepository = dictionaryRepository; From 4c20055eea2cf8080956a7126f556f0617f9b5d3 Mon Sep 17 00:00:00 2001 From: Sven Geusens Date: Tue, 4 Feb 2025 10:50:48 +0100 Subject: [PATCH 8/9] Revert Commits and clarify logic with comment This reverts commit 8befb437921cb6e3b87725cefb92a6afbf3d28fb "Improve generic NullValueCachePolicyResolver" Also reverts 8adf0a2 - Made the NullValueRepresentation overwritable in a generic manner And 8adf0a2 - Made the NullValueRepresentation overwritable in a generic manner --- .../DictionaryNullValueCachePolicyResolver.cs | 8 --- .../Cache/INullValueCachePolicyResolver.cs | 6 --- .../Cache/RepositoryCachePolicyOptions.cs | 7 +-- .../Cache/DefaultRepositoryCachePolicy.cs | 12 +++-- .../UmbracoBuilder.Repositories.cs | 2 - .../Implement/DictionaryRepository.cs | 52 +++++-------------- 6 files changed, 24 insertions(+), 63 deletions(-) delete mode 100644 src/Umbraco.Core/Cache/DictionaryNullValueCachePolicyResolver.cs delete mode 100644 src/Umbraco.Core/Cache/INullValueCachePolicyResolver.cs diff --git a/src/Umbraco.Core/Cache/DictionaryNullValueCachePolicyResolver.cs b/src/Umbraco.Core/Cache/DictionaryNullValueCachePolicyResolver.cs deleted file mode 100644 index 644142070479..000000000000 --- a/src/Umbraco.Core/Cache/DictionaryNullValueCachePolicyResolver.cs +++ /dev/null @@ -1,8 +0,0 @@ -using Umbraco.Cms.Core.Persistence.Repositories; - -namespace Umbraco.Cms.Core.Cache; - -public class DictionaryNullValueCachePolicyResolver: INullValueCachePolicyResolver -{ - public string GetNullValue() => "Umbraco.Cms.Core.Cache.DefaultRepositoryCachePolicy.NullRepresentationInCache"; -} diff --git a/src/Umbraco.Core/Cache/INullValueCachePolicyResolver.cs b/src/Umbraco.Core/Cache/INullValueCachePolicyResolver.cs deleted file mode 100644 index 5a564d23ef62..000000000000 --- a/src/Umbraco.Core/Cache/INullValueCachePolicyResolver.cs +++ /dev/null @@ -1,6 +0,0 @@ -namespace Umbraco.Cms.Core.Cache; - -public interface INullValueCachePolicyResolver -{ - public string? GetNullValue(); -} diff --git a/src/Umbraco.Core/Cache/RepositoryCachePolicyOptions.cs b/src/Umbraco.Core/Cache/RepositoryCachePolicyOptions.cs index 525bb7c3ba7e..fa334e5c4a21 100644 --- a/src/Umbraco.Core/Cache/RepositoryCachePolicyOptions.cs +++ b/src/Umbraco.Core/Cache/RepositoryCachePolicyOptions.cs @@ -11,6 +11,7 @@ public class RepositoryCachePolicyOptions public RepositoryCachePolicyOptions(Func performCount) { PerformCount = performCount; + CacheNullValues = false; GetAllCacheValidateCount = true; GetAllCacheAllowZeroCount = false; } @@ -21,6 +22,7 @@ public RepositoryCachePolicyOptions(Func performCount) public RepositoryCachePolicyOptions() { PerformCount = null; + CacheNullValues = false; GetAllCacheValidateCount = false; GetAllCacheAllowZeroCount = false; } @@ -31,10 +33,9 @@ public RepositoryCachePolicyOptions() public Func? PerformCount { get; set; } /// - /// If not null, the Get method will cache null results so that the db is not hit for repeated lookups - /// The value of this property will be used as the proxy value for the given key to populate the cache, but the return value will be null + /// True if the Get method will cache null results so that the db is not hit for repeated lookups /// - public string? NullValueRepresentation { get; set; } + public bool CacheNullValues { get; set; } /// /// True/false as to validate the total item count when all items are returned from cache, the default is true but this diff --git a/src/Umbraco.Infrastructure/Cache/DefaultRepositoryCachePolicy.cs b/src/Umbraco.Infrastructure/Cache/DefaultRepositoryCachePolicy.cs index 0d6d04062638..467607c316d4 100644 --- a/src/Umbraco.Infrastructure/Cache/DefaultRepositoryCachePolicy.cs +++ b/src/Umbraco.Infrastructure/Cache/DefaultRepositoryCachePolicy.cs @@ -1,8 +1,6 @@ // Copyright (c) Umbraco. // See LICENSE for more details. -using System.Diagnostics; -using Umbraco.Cms.Core.Models; using Umbraco.Cms.Core.Models.Entities; using Umbraco.Cms.Infrastructure.Scoping; using Umbraco.Extensions; @@ -26,6 +24,8 @@ public class DefaultRepositoryCachePolicy : 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)); @@ -139,8 +139,10 @@ public override void Delete(TEntity entity, Action persistDeleted) 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 + // There for 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.NullValueRepresentation is not null && Cache.GetCacheItem(cacheKey) == _options.NullValueRepresentation) + if (_options.CacheNullValues && Cache.GetCacheItem(cacheKey) == NullRepresentationInCache) { return null; } @@ -153,7 +155,7 @@ public override void Delete(TEntity entity, Action persistDeleted) // If we've found an identified entity, cache it for subsequent retrieval. InsertEntity(cacheKey, entity); } - else if (entity is null && _options.NullValueRepresentation is not null) + 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); @@ -271,7 +273,7 @@ protected virtual void InsertNull(string cacheKey) // 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, () => _options.NullValueRepresentation, TimeSpan.FromMinutes(5), true); + Cache.Insert(cacheKey, () => NullRepresentationInCache, TimeSpan.FromMinutes(5), true); } protected virtual void InsertEntities(TId[]? ids, TEntity[]? entities) diff --git a/src/Umbraco.Infrastructure/DependencyInjection/UmbracoBuilder.Repositories.cs b/src/Umbraco.Infrastructure/DependencyInjection/UmbracoBuilder.Repositories.cs index 7a3ec9032dec..8c9f4b119378 100644 --- a/src/Umbraco.Infrastructure/DependencyInjection/UmbracoBuilder.Repositories.cs +++ b/src/Umbraco.Infrastructure/DependencyInjection/UmbracoBuilder.Repositories.cs @@ -1,5 +1,4 @@ using Microsoft.Extensions.DependencyInjection; -using Umbraco.Cms.Core.Cache; using Umbraco.Cms.Core.DependencyInjection; using Umbraco.Cms.Core.Persistence.Repositories; using Umbraco.Cms.Core.DynamicRoot; @@ -28,7 +27,6 @@ internal static IUmbracoBuilder AddRepositories(this IUmbracoBuilder builder) builder.Services.AddUnique(); builder.Services.AddUnique(); builder.Services.AddUnique(); - builder.Services.AddUnique, DictionaryNullValueCachePolicyResolver>(); builder.Services.AddUnique(); builder.Services.AddUnique(); builder.Services.AddUnique(); diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/DictionaryRepository.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/DictionaryRepository.cs index 1af27a5614b3..bf4799e938a0 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/DictionaryRepository.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/DictionaryRepository.cs @@ -1,9 +1,7 @@ -using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; using NPoco; using Umbraco.Cms.Core; using Umbraco.Cms.Core.Cache; -using Umbraco.Cms.Core.DependencyInjection; using Umbraco.Cms.Core.Models; using Umbraco.Cms.Core.Models.Entities; using Umbraco.Cms.Core.Persistence.Querying; @@ -22,19 +20,11 @@ namespace Umbraco.Cms.Infrastructure.Persistence.Repositories.Implement; internal class DictionaryRepository : EntityRepositoryBase, IDictionaryRepository { private readonly ILoggerFactory _loggerFactory; - private readonly INullValueCachePolicyResolver _nullValueCachePolicyResolver; - - public DictionaryRepository( - IScopeAccessor scopeAccessor, - AppCaches cache, - ILogger logger, - ILoggerFactory loggerFactory, - INullValueCachePolicyResolver nullValueCachePolicyResolver) - : base(scopeAccessor, cache, logger) - { + + public DictionaryRepository(IScopeAccessor scopeAccessor, AppCaches cache, ILogger logger, + ILoggerFactory loggerFactory) + : base(scopeAccessor, cache, logger) => _loggerFactory = loggerFactory; - _nullValueCachePolicyResolver = nullValueCachePolicyResolver; - } public IDictionaryItem? Get(Guid uniqueId) { @@ -52,23 +42,15 @@ public IEnumerable GetMany(params Guid[] uniqueIds) public IDictionaryItem? Get(string key) { - var keyRepo = new DictionaryByKeyRepository( - this, - ScopeAccessor, - AppCaches, - _loggerFactory.CreateLogger(), - _nullValueCachePolicyResolver); + var keyRepo = new DictionaryByKeyRepository(this, ScopeAccessor, AppCaches, + _loggerFactory.CreateLogger()); return keyRepo.Get(key); } public IEnumerable GetManyByKeys(string[] keys) { - var keyRepo = new DictionaryByKeyRepository( - this, - ScopeAccessor, - AppCaches, - _loggerFactory.CreateLogger(), - _nullValueCachePolicyResolver); + var keyRepo = new DictionaryByKeyRepository(this, ScopeAccessor, AppCaches, + _loggerFactory.CreateLogger()); return keyRepo.GetMany(keys); } @@ -217,19 +199,11 @@ protected override IRepositoryCachePolicy CreateCachePoli private class DictionaryByKeyRepository : SimpleGetRepository { private readonly DictionaryRepository _dictionaryRepository; - private readonly INullValueCachePolicyResolver _nullValueCachePolicyResolver; - - public DictionaryByKeyRepository( - DictionaryRepository dictionaryRepository, - IScopeAccessor scopeAccessor, - AppCaches cache, - ILogger logger, - INullValueCachePolicyResolver nullValueCachePolicyResolver) - : base(scopeAccessor, cache, logger) - { + + public DictionaryByKeyRepository(DictionaryRepository dictionaryRepository, IScopeAccessor scopeAccessor, + AppCaches cache, ILogger logger) + : base(scopeAccessor, cache, logger) => _dictionaryRepository = dictionaryRepository; - _nullValueCachePolicyResolver = nullValueCachePolicyResolver; - } protected override IEnumerable PerformFetch(Sql sql) => Database @@ -253,7 +227,7 @@ protected override IRepositoryCachePolicy CreateCachePo var options = new RepositoryCachePolicyOptions { // allow null to be cached - NullValueRepresentation = _nullValueCachePolicyResolver.GetNullValue(), + CacheNullValues = true, // allow zero to be cached GetAllCacheAllowZeroCount = true }; From 1fd5b43e52d115d3dd01e977498c6f083639a61c Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Tue, 4 Feb 2025 10:53:43 +0100 Subject: [PATCH 9/9] Update src/Umbraco.Infrastructure/Cache/DefaultRepositoryCachePolicy.cs --- .../Cache/DefaultRepositoryCachePolicy.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Umbraco.Infrastructure/Cache/DefaultRepositoryCachePolicy.cs b/src/Umbraco.Infrastructure/Cache/DefaultRepositoryCachePolicy.cs index 467607c316d4..9494ed2eea58 100644 --- a/src/Umbraco.Infrastructure/Cache/DefaultRepositoryCachePolicy.cs +++ b/src/Umbraco.Infrastructure/Cache/DefaultRepositoryCachePolicy.cs @@ -139,8 +139,8 @@ public override void Delete(TEntity entity, Action persistDeleted) 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 - // There for 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. + // 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(cacheKey) == NullRepresentationInCache) {