From 97e0c79d94b762d6731fdb1000209f84f9659b52 Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Thu, 2 Oct 2025 20:10:16 +0200 Subject: [PATCH 1/6] 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 --- src/Umbraco.Core/Cache/AppCacheExtensions.cs | 38 ++++- .../Repositories/DictionaryRepositoryTest.cs | 145 +++++++++++++++++- 2 files changed, 176 insertions(+), 7 deletions(-) diff --git a/src/Umbraco.Core/Cache/AppCacheExtensions.cs b/src/Umbraco.Core/Cache/AppCacheExtensions.cs index 480b677f24..32c1b772f0 100644 --- a/src/Umbraco.Core/Cache/AppCacheExtensions.cs +++ b/src/Umbraco.Core/Cache/AppCacheExtensions.cs @@ -41,27 +41,39 @@ public static class AppCacheExtensions public static T? GetCacheItem(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)result : default; + } + return result.TryConvertTo().Result; } public static T? GetCacheItem(this IAppCache provider, string cacheKey, Func 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)result : default; + } + return result.TryConvertTo().Result; } - private static bool IsRetrievedItemNull(object? result) => result is null or (object)Cms.Core.Constants.Cache.NullRepresentationInCache; - public static async Task GetCacheItemAsync( this IAppPolicyCache provider, string cacheKey, @@ -77,9 +89,25 @@ public static class AppCacheExtensions provider.Insert(cacheKey, () => result, timeout, isSliding); } - return result == null ? default : result.TryConvertTo().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)result : default; + } + + return result.TryConvertTo().Result; } + private static bool RetrievedNullRepresentationInCache(object result) => result == (object)Cms.Core.Constants.Cache.NullRepresentationInCache; + + private static bool RequestedNullRepresentationInCache() => typeof(T) == typeof(string); + public static async Task InsertCacheItemAsync( this IAppPolicyCache provider, string cacheKey, diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/DictionaryRepositoryTest.cs b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/DictionaryRepositoryTest.cs index cb3414ad3a..acdda16406 100644 --- a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/DictionaryRepositoryTest.cs +++ b/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(); + private IDictionaryRepository CreateRepositoryWithCache(AppCaches cache) => + + // Create a repository with a real runtime cache. + new DictionaryRepository( + GetRequiredService(), + cache, + GetRequiredService>(), + GetRequiredService(), + GetRequiredService()); + [Test] public async Task Can_Perform_Get_By_Key_On_DictionaryRepository() { @@ -396,6 +409,134 @@ internal sealed class DictionaryRepositoryTest : UmbracoIntegrationTest } } + [Test] + public void Can_Perform_Cached_Request_For_Existing_Value_By_Key_On_DictionaryRepository_With_Cache() + { + var cache = AppCaches.Create(Mock.Of()); + 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(); + 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()); + 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(); + 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(); From d9592aa26d99d0c8298c218414e12ea479c5afa1 Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Thu, 2 Oct 2025 21:16:25 +0200 Subject: [PATCH 2/6] Bumped version to 16.3.0-rc2. --- version.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/version.json b/version.json index 0b4f9c3997..a3c728eba2 100644 --- a/version.json +++ b/version.json @@ -1,6 +1,6 @@ { "$schema": "https://raw.githubusercontent.com/dotnet/Nerdbank.GitVersioning/main/src/NerdBank.GitVersioning/version.schema.json", - "version": "16.3.0-rc", + "version": "16.3.0-rc2", "assemblyVersion": { "precision": "build" }, From bfd2594c7be86295f46ae5022498b7dd1e6d87db Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Mon, 6 Oct 2025 21:20:16 +0200 Subject: [PATCH 3/6] Hybrid cache: Check for `ContentCacheNode` instead of object on exists for hybrid cache to ensure correct deserialization (closes #20352) (#20383) Checked for ContentCacheNode instead of object on exists for hybrid cache to ensure correct deserialization. (cherry picked from commit 184c17e2c8a5f45557bfe66ee1444793a55d46d0) --- .../Extensions/HybridCacheExtensions.cs | 4 ++-- .../Services/DocumentCacheService.cs | 4 ++-- .../Services/MediaCacheService.cs | 4 ++-- .../Extensions/HybridCacheExtensionsTests.cs | 13 +++++++------ 4 files changed, 13 insertions(+), 12 deletions(-) diff --git a/src/Umbraco.PublishedCache.HybridCache/Extensions/HybridCacheExtensions.cs b/src/Umbraco.PublishedCache.HybridCache/Extensions/HybridCacheExtensions.cs index 427bc67d3f..ee1b7aefda 100644 --- a/src/Umbraco.PublishedCache.HybridCache/Extensions/HybridCacheExtensions.cs +++ b/src/Umbraco.PublishedCache.HybridCache/Extensions/HybridCacheExtensions.cs @@ -17,9 +17,9 @@ internal static class HybridCacheExtensions /// Hat-tip: https://github.com/dotnet/aspnetcore/discussions/57191 /// Will never add or alter the state of any items in the cache. /// - public static async Task ExistsAsync(this Microsoft.Extensions.Caching.Hybrid.HybridCache cache, string key) + public static async Task ExistsAsync(this Microsoft.Extensions.Caching.Hybrid.HybridCache cache, string key) { - (bool exists, _) = await TryGetValueAsync(cache, key); + (bool exists, _) = await TryGetValueAsync(cache, key); return exists; } diff --git a/src/Umbraco.PublishedCache.HybridCache/Services/DocumentCacheService.cs b/src/Umbraco.PublishedCache.HybridCache/Services/DocumentCacheService.cs index 2d41bc0a12..1675cb05cf 100644 --- a/src/Umbraco.PublishedCache.HybridCache/Services/DocumentCacheService.cs +++ b/src/Umbraco.PublishedCache.HybridCache/Services/DocumentCacheService.cs @@ -205,7 +205,7 @@ internal sealed class DocumentCacheService : IDocumentCacheService var cacheKey = GetCacheKey(key, false); - var existsInCache = await _hybridCache.ExistsAsync(cacheKey); + var existsInCache = await _hybridCache.ExistsAsync(cacheKey); if (existsInCache is false) { uncachedKeys.Add(key); @@ -278,7 +278,7 @@ internal sealed class DocumentCacheService : IDocumentCacheService return false; } - return await _hybridCache.ExistsAsync(GetCacheKey(keyAttempt.Result, preview)); + return await _hybridCache.ExistsAsync(GetCacheKey(keyAttempt.Result, preview)); } public async Task RefreshContentAsync(IContent content) diff --git a/src/Umbraco.PublishedCache.HybridCache/Services/MediaCacheService.cs b/src/Umbraco.PublishedCache.HybridCache/Services/MediaCacheService.cs index 29095b1d04..65b8f91945 100644 --- a/src/Umbraco.PublishedCache.HybridCache/Services/MediaCacheService.cs +++ b/src/Umbraco.PublishedCache.HybridCache/Services/MediaCacheService.cs @@ -133,7 +133,7 @@ internal sealed class MediaCacheService : IMediaCacheService return false; } - return await _hybridCache.ExistsAsync($"{keyAttempt.Result}"); + return await _hybridCache.ExistsAsync($"{keyAttempt.Result}"); } public async Task RefreshMediaAsync(IMedia media) @@ -170,7 +170,7 @@ internal sealed class MediaCacheService : IMediaCacheService var cacheKey = GetCacheKey(key, false); - var existsInCache = await _hybridCache.ExistsAsync(cacheKey); + var existsInCache = await _hybridCache.ExistsAsync(cacheKey); if (existsInCache is false) { uncachedKeys.Add(key); diff --git a/tests/Umbraco.Tests.UnitTests/Umbraco.PublishedCache.HybridCache/Extensions/HybridCacheExtensionsTests.cs b/tests/Umbraco.Tests.UnitTests/Umbraco.PublishedCache.HybridCache/Extensions/HybridCacheExtensionsTests.cs index 27e0cb0d0a..152fe28b4e 100644 --- a/tests/Umbraco.Tests.UnitTests/Umbraco.PublishedCache.HybridCache/Extensions/HybridCacheExtensionsTests.cs +++ b/tests/Umbraco.Tests.UnitTests/Umbraco.PublishedCache.HybridCache/Extensions/HybridCacheExtensionsTests.cs @@ -1,6 +1,7 @@ using Microsoft.Extensions.Caching.Hybrid; using Moq; using NUnit.Framework; +using Umbraco.Cms.Infrastructure.HybridCache; using Umbraco.Cms.Infrastructure.HybridCache.Extensions; namespace Umbraco.Cms.Tests.UnitTests.Umbraco.PublishedCache.HybridCache.Extensions; @@ -27,20 +28,20 @@ public class HybridCacheExtensionsTests { // Arrange string key = "test-key"; - var expectedValue = "test-value"; + var expectedValue = new ContentCacheNode { Id = 1234 }; _cacheMock .Setup(cache => cache.GetOrCreateAsync( key, null!, - It.IsAny>>(), + It.IsAny>>(), It.IsAny(), null, CancellationToken.None)) .ReturnsAsync(expectedValue); // Act - var exists = await HybridCacheExtensions.ExistsAsync(_cacheMock.Object, key); + var exists = await HybridCacheExtensions.ExistsAsync(_cacheMock.Object, key); // Assert Assert.IsTrue(exists); @@ -56,14 +57,14 @@ public class HybridCacheExtensionsTests .Setup(cache => cache.GetOrCreateAsync( key, null!, - It.IsAny>>(), + It.IsAny>>(), It.IsAny(), null, CancellationToken.None)) .Returns(( string key, object? state, - Func> factory, + Func> factory, HybridCacheEntryOptions? options, IEnumerable? tags, CancellationToken token) => @@ -72,7 +73,7 @@ public class HybridCacheExtensionsTests }); // Act - var exists = await HybridCacheExtensions.ExistsAsync(_cacheMock.Object, key); + var exists = await HybridCacheExtensions.ExistsAsync(_cacheMock.Object, key); // Assert Assert.IsFalse(exists); From 629e9051870ba4cd5e5e539b96fae91e0cd628a2 Mon Sep 17 00:00:00 2001 From: Nikolaj Geisle <70372949+Zeegaan@users.noreply.github.com> Date: Mon, 6 Oct 2025 21:21:14 +0200 Subject: [PATCH 4/6] Bump version --- version.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/version.json b/version.json index a3c728eba2..cf25997430 100644 --- a/version.json +++ b/version.json @@ -1,6 +1,6 @@ { "$schema": "https://raw.githubusercontent.com/dotnet/Nerdbank.GitVersioning/main/src/NerdBank.GitVersioning/version.schema.json", - "version": "16.3.0-rc2", + "version": "16.3.0-rc3", "assemblyVersion": { "precision": "build" }, From b036eb3a759ba1972b3b0e8e0a6b234f98b1e8ae Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Wed, 8 Oct 2025 07:56:49 +0200 Subject: [PATCH 5/6] Performance: Added request cache to media type retrieval in media picker validation (#20405) * Added request cache to media type retrieval in media picker validation. * Applied suggestions from code review. --- .../MediaPicker3PropertyEditor.cs | 26 +++++++++++++++---- ...ataValueReferenceFactoryCollectionTests.cs | 3 ++- .../MediaPicker3ValueEditorValidationTests.cs | 3 ++- 3 files changed, 25 insertions(+), 7 deletions(-) diff --git a/src/Umbraco.Infrastructure/PropertyEditors/MediaPicker3PropertyEditor.cs b/src/Umbraco.Infrastructure/PropertyEditors/MediaPicker3PropertyEditor.cs index ae43c493ef..26efa02f10 100644 --- a/src/Umbraco.Infrastructure/PropertyEditors/MediaPicker3PropertyEditor.cs +++ b/src/Umbraco.Infrastructure/PropertyEditors/MediaPicker3PropertyEditor.cs @@ -82,7 +82,8 @@ public class MediaPicker3PropertyEditor : DataEditor IDataTypeConfigurationCache dataTypeReadCache, ILocalizedTextService localizedTextService, IMediaTypeService mediaTypeService, - IMediaNavigationQueryService mediaNavigationQueryService) + IMediaNavigationQueryService mediaNavigationQueryService, + AppCaches appCaches) : base(shortStringHelper, jsonSerializer, ioHelper, attribute) { _jsonSerializer = jsonSerializer; @@ -95,7 +96,7 @@ public class MediaPicker3PropertyEditor : DataEditor var validators = new TypedJsonValidatorRunner, MediaPicker3Configuration>( jsonSerializer, new MinMaxValidator(localizedTextService), - new AllowedTypeValidator(localizedTextService, mediaTypeService, _mediaService), + new AllowedTypeValidator(localizedTextService, mediaTypeService, _mediaService, appCaches), new StartNodeValidator(localizedTextService, mediaNavigationQueryService)); Validators.Add(validators); @@ -401,18 +402,22 @@ public class MediaPicker3PropertyEditor : DataEditor /// internal sealed class AllowedTypeValidator : ITypedJsonValidator, MediaPicker3Configuration> { + private const string MediaTypeCacheKeyFormat = nameof(AllowedTypeValidator) + "_MediaTypeKey_{0}"; + private readonly ILocalizedTextService _localizedTextService; private readonly IMediaTypeService _mediaTypeService; private readonly IMediaService _mediaService; + private readonly AppCaches _appCaches; /// /// Initializes a new instance of the class. /// - public AllowedTypeValidator(ILocalizedTextService localizedTextService, IMediaTypeService mediaTypeService, IMediaService mediaService) + public AllowedTypeValidator(ILocalizedTextService localizedTextService, IMediaTypeService mediaTypeService, IMediaService mediaService, AppCaches appCaches) { _localizedTextService = localizedTextService; _mediaTypeService = mediaTypeService; _mediaService = mediaService; + _appCaches = appCaches; } /// @@ -452,9 +457,20 @@ public class MediaPicker3PropertyEditor : DataEditor foreach (var typeAlias in distinctTypeAliases) { - IMediaType? type = _mediaTypeService.Get(typeAlias); + // Cache media type lookups since the same media type is likely to be used multiple times in validation, + // particularly if we have multiple languages and blocks. + var cacheKey = string.Format(MediaTypeCacheKeyFormat, typeAlias); + string? typeKey = _appCaches.RequestCache.GetCacheItem(cacheKey); + if (typeKey is null) + { + typeKey = _mediaTypeService.Get(typeAlias)?.Key.ToString(); + if (typeKey is not null) + { + _appCaches.RequestCache.Set(cacheKey, typeKey); + } + } - if (type is null || allowedTypes.Contains(type.Key.ToString()) is false) + if (typeKey is null || allowedTypes.Contains(typeKey) is false) { return [ diff --git a/tests/Umbraco.Tests.UnitTests/Umbraco.Core/PropertyEditors/DataValueReferenceFactoryCollectionTests.cs b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/PropertyEditors/DataValueReferenceFactoryCollectionTests.cs index 953301f74f..a5fed60fec 100644 --- a/tests/Umbraco.Tests.UnitTests/Umbraco.Core/PropertyEditors/DataValueReferenceFactoryCollectionTests.cs +++ b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/PropertyEditors/DataValueReferenceFactoryCollectionTests.cs @@ -41,7 +41,8 @@ public class DataValueReferenceFactoryCollectionTests Mock.Of(), Mock.Of(), Mock.Of(), - Mock.Of())); + Mock.Of(), + AppCaches.Disabled)); private IIOHelper IOHelper { get; } = Mock.Of(); diff --git a/tests/Umbraco.Tests.UnitTests/Umbraco.Core/PropertyEditors/MediaPicker3ValueEditorValidationTests.cs b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/PropertyEditors/MediaPicker3ValueEditorValidationTests.cs index 6816b17615..8bd7f3b187 100644 --- a/tests/Umbraco.Tests.UnitTests/Umbraco.Core/PropertyEditors/MediaPicker3ValueEditorValidationTests.cs +++ b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/PropertyEditors/MediaPicker3ValueEditorValidationTests.cs @@ -218,7 +218,8 @@ internal class MediaPicker3ValueEditorValidationTests Mock.Of(), Mock.Of(), mediaTypeServiceMock.Object, - mediaNavigationQueryServiceMock.Object) + mediaNavigationQueryServiceMock.Object, + AppCaches.Disabled) { ConfigurationObject = new MediaPicker3Configuration() }; From dab9df3f1089f8144614bc1f5465b91fff84c0c4 Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Wed, 8 Oct 2025 07:59:10 +0200 Subject: [PATCH 6/6] Bumped version to 16.3.0-rc4. --- version.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/version.json b/version.json index cf25997430..cd1e47b49d 100644 --- a/version.json +++ b/version.json @@ -1,6 +1,6 @@ { "$schema": "https://raw.githubusercontent.com/dotnet/Nerdbank.GitVersioning/main/src/NerdBank.GitVersioning/version.schema.json", - "version": "16.3.0-rc3", + "version": "16.3.0-rc4", "assemblyVersion": { "precision": "build" },