From dfc52d642857a3e41614c00a30df452aa2896838 Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Wed, 25 Mar 2026 21:00:46 +0100 Subject: [PATCH 1/5] Optimize ContentTypeRepository to avoid unnecessary deep-cloning on cache reads. --- .../Cache/FullDataSetRepositoryCachePolicy.cs | 52 +++ .../Implement/ContentTypeRepository.cs | 29 +- .../Implement/ContentTypeRepositoryBase.cs | 75 +++- .../Implement/MediaTypeRepository.cs | 24 +- .../Implement/MemberTypeRepository.cs | 24 +- .../ContentTypeCachePolicyBenchmarks.cs | 139 +++++++ .../Repositories/ContentTypeRepositoryTest.cs | 104 ++++++ .../Repositories/MediaTypeRepositoryTest.cs | 35 ++ .../Cache/FullDataSetCachePolicyFindTests.cs | 341 ++++++++++++++++++ .../Cache/FullDataSetCachePolicyTests.cs | 55 --- .../Umbraco.Core/Cache/NonLockingCache.cs | 61 ++++ 11 files changed, 815 insertions(+), 124 deletions(-) create mode 100644 tests/Umbraco.Tests.Benchmarks/ContentTypeCachePolicyBenchmarks.cs create mode 100644 tests/Umbraco.Tests.UnitTests/Umbraco.Core/Cache/FullDataSetCachePolicyFindTests.cs create mode 100644 tests/Umbraco.Tests.UnitTests/Umbraco.Core/Cache/NonLockingCache.cs diff --git a/src/Umbraco.Infrastructure/Cache/FullDataSetRepositoryCachePolicy.cs b/src/Umbraco.Infrastructure/Cache/FullDataSetRepositoryCachePolicy.cs index 2ac1af1e181f..3dbd5f2e3ad5 100644 --- a/src/Umbraco.Infrastructure/Cache/FullDataSetRepositoryCachePolicy.cs +++ b/src/Umbraco.Infrastructure/Cache/FullDataSetRepositoryCachePolicy.cs @@ -191,6 +191,58 @@ public override TEntity[] GetAll(TId[]? ids, Func?> /// public override void ClearAll() => Cache.Clear(GetEntityTypeCacheKey()); + /// + /// Gets a single entity matching a predicate from cache, cloning only the match. + /// If cache is empty, populates it first via . + /// + /// The predicate to match against cached entities. + /// The repository PerformGetAll method, used to populate the cache on a miss. + /// A deep-cloned copy of the matching entity, or null if not found. + internal TEntity? FindCached(Func predicate, Func?> performGetAll) + { + EnsureCacheIsSynced(); + + IEnumerable all = GetAllCached(performGetAll); + TEntity? entity = all.FirstOrDefault(predicate); + + // See note in InsertEntities - what we get here is the original + // cached entity, not a clone, so we need to manually ensure it is deep-cloned. + return (TEntity?)entity?.DeepClone(); + } + + /// + /// Gets all entities matching a predicate from cache, cloning only the matches. + /// If cache is empty, populates it first via . + /// + /// The predicate to match against cached entities. + /// The repository PerformGetAll method, used to populate the cache on a miss. + /// An array of deep-cloned copies of matching entities. + internal TEntity[] FindAllCached(Func predicate, Func?> performGetAll) + { + EnsureCacheIsSynced(); + + IEnumerable all = GetAllCached(performGetAll); + + // See note in InsertEntities - what we get here are the original + // cached entities, not clones, so we need to manually ensure they are deep-cloned. + return all.Where(predicate).Select(x => (TEntity)x.DeepClone()).ToArray(); + } + + /// + /// Checks whether any cached entity matches the predicate, without cloning. + /// If cache is empty, populates it first via . + /// + /// The predicate to match against cached entities. + /// The repository PerformGetAll method, used to populate the cache on a miss. + /// True if any entity matches the predicate; otherwise false. + internal bool ExistsCached(Func predicate, Func?> performGetAll) + { + EnsureCacheIsSynced(); + + IEnumerable all = GetAllCached(performGetAll); + return all.Any(predicate); + } + /// /// Gets all cached entities, or retrieves them from the repository if not cached. /// diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/ContentTypeRepository.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/ContentTypeRepository.cs index 844b7253d2d3..30b2764dfa6d 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/ContentTypeRepository.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/ContentTypeRepository.cs @@ -134,37 +134,16 @@ public IEnumerable GetAllContentTypeIds(string[] aliases) protected override IRepositoryCachePolicy CreateCachePolicy() => new FullDataSetRepositoryCachePolicy(GlobalIsolatedCache, ScopeAccessor, _repositoryCacheVersionService, _cacheSyncService, GetEntityId, /*expires:*/ true); - // every GetExists method goes cachePolicy.GetSomething which in turns goes PerformGetAll, - // since this is a FullDataSet policy - and everything is cached - // so here, - // every PerformGet/Exists just GetMany() and then filters - // except PerformGetAll which is the one really doing the job - - // TODO: the filtering is highly inefficient as we deep-clone everything - // there should be a way to GetMany(predicate) right from the cache policy! - // and ah, well, this all caching should be refactored + the cache refreshers - // should to repository.Clear() not deal with magic caches by themselves + // Note: PerformGet(int) is passed as a callback to the cache policy's Get(TId) method, + // but FullDataSetRepositoryCachePolicy.Get() never invokes it — it uses GetAllCached() + // internally and clones only the matched entity. This override exists only as a required + // implementation of the abstract base and as a fallback for non-FullDataSet policies. protected override IContentType? PerformGet(int id) => GetMany().FirstOrDefault(x => x.Id == id); - protected override IContentType? PerformGet(Guid id) - => GetMany().FirstOrDefault(x => x.Key == id); - - protected override IContentType? PerformGet(string alias) - => GetMany().FirstOrDefault(x => x.Alias.InvariantEquals(alias)); - - protected override bool PerformExists(Guid id) - => GetMany().FirstOrDefault(x => x.Key == id) != null; - protected override IEnumerable? GetAllWithFullCachePolicy() => CommonRepository.GetAllTypes()?.OfType(); - protected override IEnumerable PerformGetAll(params Guid[]? ids) - { - IEnumerable all = GetMany(); - return ids?.Any() ?? false ? all.Where(x => ids.Contains(x.Key)) : all; - } - protected override IEnumerable PerformGetByQuery(IQuery query) { Sql baseQuery = GetBaseQuery(false); diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/ContentTypeRepositoryBase.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/ContentTypeRepositoryBase.cs index 7d62a9858193..e5e8e526bae4 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/ContentTypeRepositoryBase.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/ContentTypeRepositoryBase.cs @@ -58,6 +58,13 @@ protected ContentTypeRepositoryBase( protected ILanguageRepository LanguageRepository { get; } + /// + /// Gets the cache policy as for predicate-based lookups. + /// Returns null if the repository uses a different cache policy type. + /// + protected FullDataSetRepositoryCachePolicy? TypedCachePolicy + => CachePolicy as FullDataSetRepositoryCachePolicy; + protected abstract bool SupportsPublishing { get; } /// @@ -1559,7 +1566,20 @@ protected void ValidateAlias(TEntity entity) } } - protected abstract TEntity? PerformGet(Guid id); + /// + /// Gets a content type by its unique key (GUID), using the full-dataset cache policy + /// to clone only the matched entity rather than the entire cached collection. + /// + protected virtual TEntity? PerformGet(Guid id) + { + if (TypedCachePolicy is { } policy) + { + return policy.FindCached(x => x.Key == id, PerformGetAll); + } + + // Fallback for non-FullDataSet policies. + return GetMany().FirstOrDefault(x => x.Key == id); + } /// /// Try to set the data type Id based on the provided key or property editor alias. @@ -1615,11 +1635,58 @@ private void AssignDataTypeIdFromProvidedKeyOrPropertyEditor(IPropertyType prope } } - protected abstract TEntity? PerformGet(string alias); + /// + /// Gets a content type by its alias, using the full-dataset cache policy + /// to clone only the matched entity rather than the entire cached collection. + /// + protected virtual TEntity? PerformGet(string alias) + { + if (TypedCachePolicy is { } policy) + { + return policy.FindCached(x => x.Alias.InvariantEquals(alias), PerformGetAll); + } + + // Fallback for non-FullDataSet policies. + return GetMany().FirstOrDefault(x => x.Alias.InvariantEquals(alias)); + } + + /// + /// Gets content types by their unique keys (GUIDs), using the full-dataset cache policy + /// to clone only the matched entities rather than the entire cached collection. + /// When is null or empty, returns all content types. + /// + protected virtual IEnumerable? PerformGetAll(params Guid[]? ids) + { + if (TypedCachePolicy is { } policy) + { + if (ids?.Any() ?? false) + { + return policy.FindAllCached(x => ids.Contains(x.Key), PerformGetAll); + } - protected abstract IEnumerable? PerformGetAll(params Guid[]? ids); + // No filter — need all, clone everything (same as GetAll). + return policy.GetAll(null, PerformGetAll); + } - protected abstract bool PerformExists(Guid id); + // Fallback for non-FullDataSet policies. + IEnumerable all = GetMany(); + return ids?.Any() ?? false ? all.Where(x => ids.Contains(x.Key)) : all; + } + + /// + /// Checks whether a content type with the specified unique key (GUID) exists, + /// using the full-dataset cache policy to avoid any deep-cloning. + /// + protected virtual bool PerformExists(Guid id) + { + if (TypedCachePolicy is { } policy) + { + return policy.ExistsCached(x => x.Key == id, PerformGetAll); + } + + // Fallback for non-FullDataSet policies. + return GetMany().Any(x => x.Key == id); + } /// /// Generates a unique alias for a content type by appending an incrementing number to the provided base alias if necessary. diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/MediaTypeRepository.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/MediaTypeRepository.cs index c77d7e00d1c6..e95ba9087879 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/MediaTypeRepository.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/MediaTypeRepository.cs @@ -66,32 +66,16 @@ public MediaTypeRepository( protected override IRepositoryCachePolicy CreateCachePolicy() => new FullDataSetRepositoryCachePolicy(GlobalIsolatedCache, ScopeAccessor, _repositoryCacheVersionService, _cacheSyncService, GetEntityId, /*expires:*/ true); - // every GetExists method goes cachePolicy.GetSomething which in turns goes PerformGetAll, - // since this is a FullDataSet policy - and everything is cached - // so here, - // every PerformGet/Exists just GetMany() and then filters - // except PerformGetAll which is the one really doing the job + // Note: PerformGet(int) is passed as a callback to the cache policy's Get(TId) method, + // but FullDataSetRepositoryCachePolicy.Get() never invokes it — it uses GetAllCached() + // internally and clones only the matched entity. This override exists only as a required + // implementation of the abstract base and as a fallback for non-FullDataSet policies. protected override IMediaType? PerformGet(int id) => GetMany().FirstOrDefault(x => x.Id == id); - protected override IMediaType? PerformGet(Guid id) - => GetMany().FirstOrDefault(x => x.Key == id); - - protected override bool PerformExists(Guid id) - => GetMany().FirstOrDefault(x => x.Key == id) != null; - - protected override IMediaType? PerformGet(string alias) - => GetMany().FirstOrDefault(x => x.Alias.InvariantEquals(alias)); - protected override IEnumerable? GetAllWithFullCachePolicy() => CommonRepository.GetAllTypes()?.OfType(); - protected override IEnumerable PerformGetAll(params Guid[]? ids) - { - IEnumerable all = GetMany(); - return ids?.Any() ?? false ? all.Where(x => ids.Contains(x.Key)) : all; - } - protected override IEnumerable PerformGetByQuery(IQuery query) { Sql baseQuery = GetBaseQuery(false); diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/MemberTypeRepository.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/MemberTypeRepository.cs index b59f23de657f..7cfa74314205 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/MemberTypeRepository.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/MemberTypeRepository.cs @@ -71,29 +71,13 @@ public MemberTypeRepository( protected override IRepositoryCachePolicy CreateCachePolicy() => new FullDataSetRepositoryCachePolicy(GlobalIsolatedCache, ScopeAccessor, _repositoryCacheVersionService, _cacheSyncService, GetEntityId, /*expires:*/ true); - // every GetExists method goes cachePolicy.GetSomething which in turns goes PerformGetAll, - // since this is a FullDataSet policy - and everything is cached - // so here, - // every PerformGet/Exists just GetMany() and then filters - // except PerformGetAll which is the one really doing the job + // Note: PerformGet(int) is passed as a callback to the cache policy's Get(TId) method, + // but FullDataSetRepositoryCachePolicy.Get() never invokes it — it uses GetAllCached() + // internally and clones only the matched entity. This override exists only as a required + // implementation of the abstract base and as a fallback for non-FullDataSet policies. protected override IMemberType? PerformGet(int id) => GetMany().FirstOrDefault(x => x.Id == id); - protected override IMemberType? PerformGet(Guid id) - => GetMany().FirstOrDefault(x => x.Key == id); - - protected override IEnumerable PerformGetAll(params Guid[]? ids) - { - IEnumerable all = GetMany(); - return ids?.Any() ?? false ? all.Where(x => ids.Contains(x.Key)) : all; - } - - protected override bool PerformExists(Guid id) - => GetMany().FirstOrDefault(x => x.Key == id) != null; - - protected override IMemberType? PerformGet(string alias) - => GetMany().FirstOrDefault(x => x.Alias.InvariantEquals(alias)); - protected override IEnumerable? GetAllWithFullCachePolicy() => CommonRepository.GetAllTypes()?.OfType(); diff --git a/tests/Umbraco.Tests.Benchmarks/ContentTypeCachePolicyBenchmarks.cs b/tests/Umbraco.Tests.Benchmarks/ContentTypeCachePolicyBenchmarks.cs new file mode 100644 index 000000000000..103cd98ab392 --- /dev/null +++ b/tests/Umbraco.Tests.Benchmarks/ContentTypeCachePolicyBenchmarks.cs @@ -0,0 +1,139 @@ +// Copyright (c) Umbraco. +// See LICENSE for more details. + +using BenchmarkDotNet.Attributes; +using Moq; +using Umbraco.Cms.Core.Cache; +using Umbraco.Cms.Core.Models; +using Umbraco.Cms.Core.Scoping; +using Umbraco.Cms.Infrastructure.Scoping; +using IScope = Umbraco.Cms.Infrastructure.Scoping.IScope; + +namespace Umbraco.Tests.Benchmarks; + +/// +/// Benchmarks comparing the performance of +/// (clones only the matched entity) vs +/// (clones all cached entities then filters). +/// +/// +/// Run with: dotnet run -c Release --project tests/Umbraco.Tests.Benchmarks -- --filter "*ContentTypeCachePolicy*" +/// +[MemoryDiagnoser] +public class ContentTypeCachePolicyBenchmarks +{ + private FullDataSetRepositoryCachePolicy _policy = null!; + private AuditItem[] _entities = null!; + + [Params(10, 50, 200)] + public int EntityCount { get; set; } + + [GlobalSetup] + public void Setup() + { + _entities = Enumerable.Range(1, EntityCount) + .Select(i => new AuditItem(i, AuditType.Copy, 123, "test", $"item{i}")) + .ToArray(); + + var accessor = new Mock(); + var scope = new Mock(); + scope.Setup(x => x.RepositoryCacheMode).Returns(RepositoryCacheMode.Default); + accessor.Setup(x => x.AmbientScope).Returns(scope.Object); + + var cache = new BenchmarkCache(); + + _policy = new FullDataSetRepositoryCachePolicy( + cache, + accessor.Object, + new SingleServerCacheVersionService(), + Mock.Of(), + item => item.Id, + false); + + // Warm the cache. + _policy.GetAll(null, _ => _entities); + } + + /// + /// Baseline: simulates the old code path where GetAll() clones every entity, + /// then the caller filters with FirstOrDefault to find one. + /// + [Benchmark(Baseline = true)] + public AuditItem? GetAll_ThenFilter() + { + var all = _policy.GetAll(null, _ => _entities); + return all.FirstOrDefault(x => x.Id.Equals(EntityCount / 2)); + } + + /// + /// Optimized: FindCached searches non-cloned cache and clones only the single match. + /// + [Benchmark] + public AuditItem? FindCached_SingleClone() + { + return _policy.FindCached(x => x.Id.Equals(EntityCount / 2), _ => _entities); + } + + /// + /// Optimized: ExistsCached searches non-cloned cache without cloning at all. + /// + [Benchmark] + public bool ExistsCached_NoClone() + { + return _policy.ExistsCached(x => x.Id.Equals(EntityCount / 2), _ => _entities); + } + + /// + /// A simple cache backed by a dictionary, suitable for benchmarking. + /// + private sealed class BenchmarkCache : IAppPolicyCache + { + private readonly Dictionary _cache = []; + + public object? Get(string key) + => _cache.TryGetValue(key, out var value) ? value : null; + + public object? Get(string key, Func factory) => Get(key, factory, null, false); + + public object? Get(string key, Func factory, TimeSpan? timeout, bool isSliding = false) + { + if (_cache.TryGetValue(key, out var value)) + { + return value; + } + + value = factory(); + _cache[key] = value; + return value; + } + + public IEnumerable SearchByKey(string keyStartsWith) + => _cache.Where(kvp => kvp.Key.StartsWith(keyStartsWith)).Select(kvp => kvp.Value!); + + public IEnumerable SearchByRegex(string regex) => throw new NotImplementedException(); + + public void Clear() => _cache.Clear(); + + public void Clear(string key) => _cache.Remove(key); + + public void ClearOfType(Type type) => throw new NotImplementedException(); + + public void ClearOfType() => throw new NotImplementedException(); + + public void ClearOfType(Func predicate) => throw new NotImplementedException(); + + public void ClearByKey(string keyStartsWith) + { + var keysToRemove = _cache.Keys.Where(k => k.StartsWith(keyStartsWith)).ToList(); + foreach (var key in keysToRemove) + { + _cache.Remove(key); + } + } + + public void ClearByRegex(string regex) => throw new NotImplementedException(); + + public void Insert(string key, Func factory, TimeSpan? timeout = null, bool isSliding = false) + => _cache[key] = factory(); + } +} diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/ContentTypeRepositoryTest.cs b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/ContentTypeRepositoryTest.cs index 997961f4f417..e00991458aa3 100644 --- a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/ContentTypeRepositoryTest.cs +++ b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/ContentTypeRepositoryTest.cs @@ -1168,4 +1168,108 @@ DocumentPropertyValueGranularPermission CreatePermission(IPropertyType propertyT Assert.AreEqual(4, userGroup.GranularPermissions.Count); return userGroup; } + + [Test] + public void Get_By_Guid_Returns_Deep_Clone_Not_Cached_Instance() + { + var provider = ScopeProvider; + using (var scope = provider.CreateScope()) + { + var repository = ContentTypeRepository; + + var first = repository.Get(_simpleContentType.Key); + var second = repository.Get(_simpleContentType.Key); + + Assert.IsNotNull(first); + Assert.IsNotNull(second); + Assert.AreEqual(first.Id, second.Id); + + // Must be different object references (deep clones, not the cached original). + Assert.AreNotSame(first, second); + } + } + + [Test] + public void Get_By_Alias_Returns_Correct_ContentType() + { + var provider = ScopeProvider; + using (var scope = provider.CreateScope()) + { + var repository = ContentTypeRepository; + + // Case-insensitive alias lookup. + var result = repository.Get("UMBTEXTPAGE"); + + Assert.IsNotNull(result); + Assert.AreEqual(_simpleContentType.Id, result!.Id); + Assert.AreEqual("umbTextpage", result.Alias); + } + } + + [Test] + public void Get_By_Alias_Returns_Deep_Clone_Not_Cached_Instance() + { + var provider = ScopeProvider; + using (var scope = provider.CreateScope()) + { + var repository = ContentTypeRepository; + + var first = repository.Get("umbTextpage"); + var second = repository.Get("umbTextpage"); + + Assert.IsNotNull(first); + Assert.IsNotNull(second); + Assert.AreEqual(first!.Id, second!.Id); + Assert.AreNotSame(first, second); + } + } + + [Test] + public void Exists_By_Guid_Returns_True_For_Existing_Type() + { + var provider = ScopeProvider; + using (var scope = provider.CreateScope()) + { + var repository = ContentTypeRepository; + + var exists = repository.Exists(_simpleContentType.Key); + + Assert.IsTrue(exists); + } + } + + [Test] + public void Exists_By_Guid_Returns_False_For_NonExisting_Type() + { + var provider = ScopeProvider; + using (var scope = provider.CreateScope()) + { + var repository = ContentTypeRepository; + + var exists = repository.Exists(Guid.NewGuid()); + + Assert.IsFalse(exists); + } + } + + [Test] + public void Get_By_Guid_Mutation_Does_Not_Affect_Subsequent_Get() + { + var provider = ScopeProvider; + using (var scope = provider.CreateScope()) + { + var repository = ContentTypeRepository; + + // Get a content type and mutate its name. + var first = repository.Get(_simpleContentType.Key); + Assert.IsNotNull(first); + var originalName = first!.Name; + first.Name = "MUTATED_NAME_" + Guid.NewGuid(); + + // A subsequent Get should return an unmodified clone. + var second = repository.Get(_simpleContentType.Key); + Assert.IsNotNull(second); + Assert.AreEqual(originalName, second!.Name, "Mutation of a returned entity should not affect the cached copy"); + } + } } diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/MediaTypeRepositoryTest.cs b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/MediaTypeRepositoryTest.cs index f5551d751a62..25d8c17ff24a 100644 --- a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/MediaTypeRepositoryTest.cs +++ b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/MediaTypeRepositoryTest.cs @@ -412,6 +412,41 @@ public void Can_Verify_PropertyTypes_On_File_MediaType() } } + [Test] + public void Get_By_Guid_Returns_Deep_Clone_Not_Cached_Instance() + { + var provider = ScopeProvider; + using (var scope = provider.CreateScope()) + { + var repository = CreateRepository(provider); + IMediaType mediaType = MediaTypeBuilder.CreateNewMediaType(); + repository.Save(mediaType); + + var first = repository.Get(mediaType.Key); + var second = repository.Get(mediaType.Key); + + Assert.IsNotNull(first); + Assert.IsNotNull(second); + Assert.AreEqual(first.Id, second.Id); + Assert.AreNotSame(first, second); + } + } + + [Test] + public void Exists_By_Guid_Returns_Correct_Result() + { + var provider = ScopeProvider; + using (var scope = provider.CreateScope()) + { + var repository = CreateRepository(provider); + IMediaType mediaType = MediaTypeBuilder.CreateNewMediaType(); + repository.Save(mediaType); + + Assert.IsTrue(repository.Exists(mediaType.Key)); + Assert.IsFalse(repository.Exists(Guid.NewGuid())); + } + } + private MediaTypeRepository CreateRepository(IScopeProvider provider) => new((IScopeAccessor)provider, AppCaches.Disabled, LoggerFactory.CreateLogger(), CommonRepository, LanguageRepository, ShortStringHelper, Mock.Of(), IdKeyMap, Mock.Of()); diff --git a/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Cache/FullDataSetCachePolicyFindTests.cs b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Cache/FullDataSetCachePolicyFindTests.cs new file mode 100644 index 000000000000..ce8b6eda1bc3 --- /dev/null +++ b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Cache/FullDataSetCachePolicyFindTests.cs @@ -0,0 +1,341 @@ +// Copyright (c) Umbraco. +// See LICENSE for more details. + +using System.Collections.Concurrent; +using Moq; +using NUnit.Framework; +using Umbraco.Cms.Core.Cache; +using Umbraco.Cms.Core.Models.Entities; +using Umbraco.Cms.Core.Scoping; +using Umbraco.Cms.Infrastructure.Scoping; +using Umbraco.Cms.Tests.Common.Attributes; +using IScope = Umbraco.Cms.Infrastructure.Scoping.IScope; + +namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Core.Cache; + +/// +/// Tests for and +/// methods. +/// +[TestFixture] +public class FullDataSetCachePolicyFindTests +{ + private IScopeAccessor DefaultAccessor + { + get + { + var accessor = new Mock(); + var scope = new Mock(); + scope.Setup(x => x.RepositoryCacheMode).Returns(RepositoryCacheMode.Default); + accessor.Setup(x => x.AmbientScope).Returns(scope.Object); + return accessor.Object; + } + } + + private FullDataSetRepositoryCachePolicy CreatePolicy( + IAppPolicyCache? cache = null, + bool expires = false) + { + cache ??= new NonLockingCache(); + return new FullDataSetRepositoryCachePolicy( + cache, + DefaultAccessor, + new SingleServerCacheVersionService(), + Mock.Of(), + item => item.Id, + expires); + } + + private static CloneCountingEntity[] CreateEntities(int count) + { + var entities = new CloneCountingEntity[count]; + for (var i = 0; i < count; i++) + { + entities[i] = new CloneCountingEntity(i + 1, $"entity-{i + 1}"); + } + + return entities; + } + + [Test] + public void FindCached_Returns_Matching_Entity() + { + var entities = CreateEntities(5); + var policy = CreatePolicy(); + + // Populate cache. + policy.GetAll(null, _ => entities); + + CloneCountingEntity.ResetCloneCount(); + + var found = policy.FindCached(x => x.Id == 3, _ => entities); + + Assert.IsNotNull(found); + Assert.AreEqual(3, found!.Id); + Assert.AreEqual("entity-3", found.Name); + } + + [Test] + public void FindCached_Returns_Deep_Clone() + { + var entities = CreateEntities(3); + var policy = CreatePolicy(); + + // Populate cache. + policy.GetAll(null, _ => entities); + + var found = policy.FindCached(x => x.Id == 2, _ => entities); + + Assert.IsNotNull(found); + Assert.AreEqual(2, found!.Id); + + // Must be a different reference than the cached original. + var cachedOriginal = entities.First(x => x.Id == 2); + Assert.AreNotSame(cachedOriginal, found); + } + + [Test] + public void FindCached_Returns_Null_When_No_Match() + { + var entities = CreateEntities(3); + var policy = CreatePolicy(); + + // Populate cache. + policy.GetAll(null, _ => entities); + + var found = policy.FindCached(x => x.Id == 999, _ => entities); + + Assert.IsNull(found); + } + + [Test] + public void FindCached_Populates_Cache_On_Miss() + { + var entities = CreateEntities(3); + var performGetAllCallCount = 0; + + var policy = CreatePolicy(); + + // Cache is empty — FindCached should trigger performGetAll. + var found = policy.FindCached(x => x.Id == 2, _ => + { + performGetAllCallCount++; + return entities; + }); + + Assert.IsNotNull(found); + Assert.AreEqual(2, found!.Id); + Assert.AreEqual(1, performGetAllCallCount, "performGetAll should be called once to populate cache"); + + // Second call should NOT trigger performGetAll again. + var found2 = policy.FindCached(x => x.Id == 1, _ => + { + performGetAllCallCount++; + return entities; + }); + + Assert.IsNotNull(found2); + Assert.AreEqual(1, performGetAllCallCount, "performGetAll should not be called again — cache is warm"); + } + + [Test] + public void ExistsCached_Returns_True_For_Match() + { + var entities = CreateEntities(5); + var policy = CreatePolicy(); + + // Populate cache. + policy.GetAll(null, _ => entities); + + var exists = policy.ExistsCached(x => x.Id == 3, _ => entities); + + Assert.IsTrue(exists); + } + + [Test] + public void ExistsCached_Returns_False_For_No_Match() + { + var entities = CreateEntities(5); + var policy = CreatePolicy(); + + // Populate cache. + policy.GetAll(null, _ => entities); + + var exists = policy.ExistsCached(x => x.Id == 999, _ => entities); + + Assert.IsFalse(exists); + } + + [Test] + public void FindCached_Clones_Only_Matched_Entity() + { + const int entityCount = 50; + var entities = CreateEntities(entityCount); + var policy = CreatePolicy(); + + // Populate cache. + policy.GetAll(null, _ => entities); + + CloneCountingEntity.ResetCloneCount(); + + // FindCached should clone only the 1 matched entity. + var found = policy.FindCached(x => x.Id == 25, _ => entities); + + Assert.IsNotNull(found); + Assert.AreEqual(25, found!.Id); + Assert.AreEqual(1, CloneCountingEntity.CloneCount, "FindCached should trigger exactly 1 DeepClone call"); + } + + [Test] + public void GetAll_Clones_All_Entities() + { + // Verify the baseline: GetAll clones every entity. + const int entityCount = 50; + var entities = CreateEntities(entityCount); + var policy = CreatePolicy(); + + // Populate cache. + policy.GetAll(null, _ => entities); + + CloneCountingEntity.ResetCloneCount(); + + // GetAll clones every entity. + var all = policy.GetAll(null, _ => entities); + + Assert.AreEqual(entityCount, all.Length); + Assert.AreEqual(entityCount, CloneCountingEntity.CloneCount, "GetAll should trigger N DeepClone calls"); + } + + [Test] + public void ExistsCached_Does_Not_Clone() + { + const int entityCount = 50; + var entities = CreateEntities(entityCount); + var policy = CreatePolicy(); + + // Populate cache. + policy.GetAll(null, _ => entities); + + CloneCountingEntity.ResetCloneCount(); + + var exists = policy.ExistsCached(x => x.Id == 25, _ => entities); + + Assert.IsTrue(exists); + Assert.AreEqual(0, CloneCountingEntity.CloneCount, "ExistsCached should trigger zero DeepClone calls"); + } + + [Test] + [LongRunning] + public void FindCached_Is_ThreadSafe() + { + const int threadCount = 20; + var threads = new Thread[threadCount]; + var barrier = new ManualResetEventSlim(false); + var exceptions = new ConcurrentBag(); + + var entities = CreateEntities(100); + var policy = CreatePolicy(); + + // Populate cache. + policy.GetAll(null, _ => entities); + + for (var i = 0; i < threadCount; i++) + { + var targetId = (i % 100) + 1; + threads[i] = new Thread(() => + { + barrier.Wait(); + try + { + var result = policy.FindCached(x => x.Id == targetId, _ => entities); + Assert.IsNotNull(result); + Assert.AreEqual(targetId, result!.Id); + } + catch (Exception e) + { + exceptions.Add(e); + } + }); + } + + using (ExecutionContext.SuppressFlow()) + { + foreach (var t in threads) + { + t.Start(); + } + } + + barrier.Set(); + foreach (var t in threads) + { + t.Join(); + } + + Assert.IsEmpty( + exceptions, + $"Thread safety violation: {string.Join(Environment.NewLine, exceptions.Select(e => e.Message))}"); + } + + [Test] + public void FindAllCached_Clones_Only_Matched_Entities() + { + const int entityCount = 50; + var entities = CreateEntities(entityCount); + var policy = CreatePolicy(); + + // Populate cache. + policy.GetAll(null, _ => entities); + + CloneCountingEntity.ResetCloneCount(); + + // Find 5 entities out of 50. + var targetIds = new HashSet { 5, 10, 15, 20, 25 }; + var found = policy.FindAllCached(x => targetIds.Contains(x.Id), _ => entities); + + Assert.AreEqual(5, found.Length); + Assert.AreEqual(5, CloneCountingEntity.CloneCount, "FindAllCached should clone only the 5 matched entities, not all 50"); + } + + [Test] + public void FindAllCached_Returns_Empty_When_No_Match() + { + var entities = CreateEntities(10); + var policy = CreatePolicy(); + + // Populate cache. + policy.GetAll(null, _ => entities); + + var found = policy.FindAllCached(x => x.Id > 999, _ => entities); + + Assert.IsEmpty(found); + } + + /// + /// A test entity that counts how many times DeepClone is called across all instances. + /// + private sealed class CloneCountingEntity : EntityBase + { + private static int _cloneCount; + + public CloneCountingEntity(int id, string name) + { + DisableChangeTracking(); + Id = id; + Name = name; + EnableChangeTracking(); + } + + public string Name { get; set; } + + public static int CloneCount => _cloneCount; + + public static void ResetCloneCount() => Interlocked.Exchange(ref _cloneCount, 0); + + protected override void PerformDeepClone(object clone) + { + base.PerformDeepClone(clone); + Interlocked.Increment(ref _cloneCount); + } + } +} diff --git a/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Cache/FullDataSetCachePolicyTests.cs b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Cache/FullDataSetCachePolicyTests.cs index 5158c6862654..d8569876f988 100644 --- a/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Cache/FullDataSetCachePolicyTests.cs +++ b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Cache/FullDataSetCachePolicyTests.cs @@ -407,59 +407,4 @@ public void GetAllCached_Is_ThreadSafe_During_Iteration_And_Cache_Clear() threadSafetyExceptions, $"Thread safety violation detected: {string.Join(Environment.NewLine, threadSafetyExceptions.Select(e => e.Message))}"); } - - /// - /// A simple non-thread-safe cache for testing purposes. - /// This allows race conditions to manifest that would be hidden by ObjectCacheAppCache's internal locking. - /// - private sealed class NonLockingCache : IAppPolicyCache - { - private readonly Dictionary _cache = []; - - public object? Get(string key) - => _cache.TryGetValue(key, out var value) ? value : null; - - public object? Get(string key, Func factory) => Get(key, factory, null, false); - - public object? Get(string key, Func factory, TimeSpan? timeout, bool isSliding = false) - { - if (_cache.TryGetValue(key, out var value)) - { - return value; - } - - value = factory(); - _cache[key] = value; - return value; - } - - public IEnumerable SearchByKey(string keyStartsWith) - => _cache.Where(kvp => kvp.Key.StartsWith(keyStartsWith)).Select(kvp => kvp.Value!); - - public IEnumerable SearchByRegex(string regex) => throw new NotImplementedException(); - - public void Clear() => _cache.Clear(); - - public void Clear(string key) => _cache.Remove(key); - - public void ClearOfType(Type type) => throw new NotImplementedException(); - - public void ClearOfType() => throw new NotImplementedException(); - - public void ClearOfType(Func predicate) => throw new NotImplementedException(); - - public void ClearByKey(string keyStartsWith) - { - var keysToRemove = _cache.Keys.Where(k => k.StartsWith(keyStartsWith)).ToList(); - foreach (var key in keysToRemove) - { - _cache.Remove(key); - } - } - - public void ClearByRegex(string regex) => throw new NotImplementedException(); - - public void Insert(string key, Func factory, TimeSpan? timeout = null, bool isSliding = false) - => _cache[key] = factory(); - } } diff --git a/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Cache/NonLockingCache.cs b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Cache/NonLockingCache.cs new file mode 100644 index 000000000000..2e4dcf53607e --- /dev/null +++ b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Cache/NonLockingCache.cs @@ -0,0 +1,61 @@ +// Copyright (c) Umbraco. +// See LICENSE for more details. + +using Umbraco.Cms.Core.Cache; + +namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Core.Cache; + +/// +/// A simple non-thread-safe cache for testing purposes. +/// This allows race conditions to manifest that would be hidden by ObjectCacheAppCache's internal locking. +/// +internal sealed class NonLockingCache : IAppPolicyCache +{ + private readonly Dictionary _cache = []; + + public object? Get(string key) + => _cache.TryGetValue(key, out var value) ? value : null; + + public object? Get(string key, Func factory) => Get(key, factory, null, false); + + public object? Get(string key, Func factory, TimeSpan? timeout, bool isSliding = false) + { + if (_cache.TryGetValue(key, out var value)) + { + return value; + } + + value = factory(); + _cache[key] = value; + return value; + } + + public IEnumerable SearchByKey(string keyStartsWith) + => _cache.Where(kvp => kvp.Key.StartsWith(keyStartsWith)).Select(kvp => kvp.Value!); + + public IEnumerable SearchByRegex(string regex) => throw new NotImplementedException(); + + public void Clear() => _cache.Clear(); + + public void Clear(string key) => _cache.Remove(key); + + public void ClearOfType(Type type) => throw new NotImplementedException(); + + public void ClearOfType() => throw new NotImplementedException(); + + public void ClearOfType(Func predicate) => throw new NotImplementedException(); + + public void ClearByKey(string keyStartsWith) + { + var keysToRemove = _cache.Keys.Where(k => k.StartsWith(keyStartsWith)).ToList(); + foreach (var key in keysToRemove) + { + _cache.Remove(key); + } + } + + public void ClearByRegex(string regex) => throw new NotImplementedException(); + + public void Insert(string key, Func factory, TimeSpan? timeout = null, bool isSliding = false) + => _cache[key] = factory(); +} From cd44f3e0a36dc9a6d27f18d32720cc90db98e717 Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Thu, 26 Mar 2026 06:44:38 +0100 Subject: [PATCH 2/5] Used lightweight benchmark and addressed code review comments. --- .../Implement/ContentTypeRepositoryBase.cs | 11 +++++++++-- .../ContentTypeCachePolicyBenchmarks.cs | 7 ++++--- .../Umbraco.Core/Cache/NonLockingCache.cs | 4 ++-- 3 files changed, 15 insertions(+), 7 deletions(-) diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/ContentTypeRepositoryBase.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/ContentTypeRepositoryBase.cs index e5e8e526bae4..7c1af544a51d 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/ContentTypeRepositoryBase.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/ContentTypeRepositoryBase.cs @@ -1661,7 +1661,8 @@ private void AssignDataTypeIdFromProvidedKeyOrPropertyEditor(IPropertyType prope { if (ids?.Any() ?? false) { - return policy.FindAllCached(x => ids.Contains(x.Key), PerformGetAll); + var idSet = ids.ToHashSet(); + return policy.FindAllCached(x => idSet.Contains(x.Key), PerformGetAll); } // No filter — need all, clone everything (same as GetAll). @@ -1670,7 +1671,13 @@ private void AssignDataTypeIdFromProvidedKeyOrPropertyEditor(IPropertyType prope // Fallback for non-FullDataSet policies. IEnumerable all = GetMany(); - return ids?.Any() ?? false ? all.Where(x => ids.Contains(x.Key)) : all; + if (ids?.Any() ?? false) + { + var idSet = ids.ToHashSet(); + return all.Where(x => idSet.Contains(x.Key)); + } + + return all; } /// diff --git a/tests/Umbraco.Tests.Benchmarks/ContentTypeCachePolicyBenchmarks.cs b/tests/Umbraco.Tests.Benchmarks/ContentTypeCachePolicyBenchmarks.cs index 103cd98ab392..d261a391f19a 100644 --- a/tests/Umbraco.Tests.Benchmarks/ContentTypeCachePolicyBenchmarks.cs +++ b/tests/Umbraco.Tests.Benchmarks/ContentTypeCachePolicyBenchmarks.cs @@ -7,6 +7,7 @@ using Umbraco.Cms.Core.Models; using Umbraco.Cms.Core.Scoping; using Umbraco.Cms.Infrastructure.Scoping; +using Umbraco.Tests.Benchmarks.Config; using IScope = Umbraco.Cms.Infrastructure.Scoping.IScope; namespace Umbraco.Tests.Benchmarks; @@ -19,7 +20,7 @@ namespace Umbraco.Tests.Benchmarks; /// /// Run with: dotnet run -c Release --project tests/Umbraco.Tests.Benchmarks -- --filter "*ContentTypeCachePolicy*" /// -[MemoryDiagnoser] +[QuickRunWithMemoryDiagnoserConfig] public class ContentTypeCachePolicyBenchmarks { private FullDataSetRepositoryCachePolicy _policy = null!; @@ -108,7 +109,7 @@ private sealed class BenchmarkCache : IAppPolicyCache } public IEnumerable SearchByKey(string keyStartsWith) - => _cache.Where(kvp => kvp.Key.StartsWith(keyStartsWith)).Select(kvp => kvp.Value!); + => _cache.Where(kvp => kvp.Key.StartsWith(keyStartsWith, StringComparison.InvariantCultureIgnoreCase)).Select(kvp => kvp.Value!); public IEnumerable SearchByRegex(string regex) => throw new NotImplementedException(); @@ -124,7 +125,7 @@ public IEnumerable SearchByKey(string keyStartsWith) public void ClearByKey(string keyStartsWith) { - var keysToRemove = _cache.Keys.Where(k => k.StartsWith(keyStartsWith)).ToList(); + var keysToRemove = _cache.Keys.Where(k => k.StartsWith(keyStartsWith, StringComparison.InvariantCultureIgnoreCase)).ToList(); foreach (var key in keysToRemove) { _cache.Remove(key); diff --git a/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Cache/NonLockingCache.cs b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Cache/NonLockingCache.cs index 2e4dcf53607e..bc9e1ea678be 100644 --- a/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Cache/NonLockingCache.cs +++ b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Cache/NonLockingCache.cs @@ -31,7 +31,7 @@ internal sealed class NonLockingCache : IAppPolicyCache } public IEnumerable SearchByKey(string keyStartsWith) - => _cache.Where(kvp => kvp.Key.StartsWith(keyStartsWith)).Select(kvp => kvp.Value!); + => _cache.Where(kvp => kvp.Key.StartsWith(keyStartsWith, StringComparison.InvariantCultureIgnoreCase)).Select(kvp => kvp.Value!); public IEnumerable SearchByRegex(string regex) => throw new NotImplementedException(); @@ -47,7 +47,7 @@ public IEnumerable SearchByKey(string keyStartsWith) public void ClearByKey(string keyStartsWith) { - var keysToRemove = _cache.Keys.Where(k => k.StartsWith(keyStartsWith)).ToList(); + var keysToRemove = _cache.Keys.Where(k => k.StartsWith(keyStartsWith, StringComparison.InvariantCultureIgnoreCase)).ToList(); foreach (var key in keysToRemove) { _cache.Remove(key); From 198227239e061f0143badff1e856df311ff5dbe8 Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Thu, 26 Mar 2026 07:06:39 +0100 Subject: [PATCH 3/5] Optimize TemplateRepository to avoid unnecessary deep-cloning on cache reads. --- .../Implement/TemplateRepository.cs | 82 +++++++++++----- .../Repositories/TemplateRepositoryTest.cs | 94 +++++++++++++++++++ 2 files changed, 155 insertions(+), 21 deletions(-) diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/TemplateRepository.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/TemplateRepository.cs index a7796327c496..436870a1ff9a 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/TemplateRepository.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/TemplateRepository.cs @@ -104,18 +104,32 @@ public TemplateRepository( { } + /// + /// Gets the cache policy as for predicate-based lookups. + /// Returns null when caching is disabled (e.g. ). + /// + private FullDataSetRepositoryCachePolicy? TypedCachePolicy + => CachePolicy as FullDataSetRepositoryCachePolicy; + /// /// Gets the template with the specified unique identifier. /// /// The unique identifier of the template. /// The template if found; otherwise, null. - /// GUID-based lookups delegate to GetMany() which is served from FullDataSetRepositoryCachePolicy. - public ITemplate? Get(Guid key) => GetMany().FirstOrDefault(x => x.Key == key); + public ITemplate? Get(Guid key) + => TypedCachePolicy?.FindCached(x => x.Key == key, PerformGetAll) + ?? GetMany().FirstOrDefault(x => x.Key == key); IEnumerable IReadRepository.GetMany(params Guid[]? keys) { - IEnumerable all = GetMany(); - return keys?.Length > 0 ? all.Where(x => keys.Contains(x.Key)).ToArray() : all; + if (keys?.Length > 0) + { + var keySet = keys.ToHashSet(); + return TypedCachePolicy?.FindAllCached(x => keySet.Contains(x.Key), PerformGetAll) + ?? GetMany().Where(x => keySet.Contains(x.Key)).ToArray(); + } + + return GetMany(); } /// @@ -123,7 +137,9 @@ IEnumerable IReadRepository.GetMany(params Guid[]? k /// /// The unique identifier of the template. /// True if the template exists; otherwise, false. - public bool Exists(Guid id) => GetMany().Any(x => x.Key == id); + public bool Exists(Guid id) + => TypedCachePolicy?.ExistsCached(x => x.Key == id, PerformGetAll) + ?? GetMany().Any(x => x.Key == id); /// /// Saves the specified template entity to the repository. @@ -432,13 +448,12 @@ private string EnsureUniqueAlias(ITemplate template, int attempts) #region Overrides of RepositoryBase + // Note: PerformGet(int) is passed as a callback to the cache policy's Get(TId) method, + // but FullDataSetRepositoryCachePolicy.Get() never invokes it — it uses GetAllCached() + // internally and clones only the matched entity. This override exists only as a required + // implementation of the abstract base and as a fallback for non-FullDataSet policies. protected override ITemplate? PerformGet(int id) - { - //use the underlying GetAll which will force cache all templates - ITemplate? template = GetMany().FirstOrDefault(x => x.Id == id); - - return template; - } + => GetMany().FirstOrDefault(x => x.Id == id); protected override IEnumerable PerformGetAll(params int[]? ids) { @@ -703,7 +718,16 @@ protected override void PersistDeletedItem(ITemplate entity) /// Retrieves a template by its alias. /// The alias of the template to retrieve. /// The template matching the specified alias, or null if not found. - public ITemplate? Get(string? alias) => GetAll(alias).FirstOrDefault(); + public ITemplate? Get(string? alias) + { + if (alias is null) + { + return null; + } + + return TypedCachePolicy?.FindCached(x => x.Alias.InvariantEquals(alias), PerformGetAll) + ?? GetMany().FirstOrDefault(x => x.Alias.InvariantEquals(alias)); + } /// /// Retrieves all templates, or filters them by the specified aliases if provided. @@ -712,16 +736,17 @@ protected override void PersistDeletedItem(ITemplate entity) /// An enumerable collection of instances matching the specified aliases, or all templates if no aliases are given. public IEnumerable GetAll(params string?[] aliases) { - //We must call the base (normal) GetAll method - // which is cached. This is a specialized method and unfortunately with the params[] it + // We must call the base (normal) GetAll method which is cached. + // This is a specialized method and unfortunately with the params[] it // overlaps with the normal GetAll method. if (aliases.Any() == false) { return GetMany(); } - //return from base.GetAll, this is all cached - return GetMany().Where(x => aliases.WhereNotNull().InvariantContains(x.Alias)); + var nonNullAliases = aliases.WhereNotNull().ToArray(); + return TypedCachePolicy?.FindAllCached(x => nonNullAliases.InvariantContains(x.Alias), PerformGetAll) + ?? GetMany().Where(x => nonNullAliases.InvariantContains(x.Alias)); } /// @@ -731,7 +756,23 @@ public IEnumerable GetAll(params string?[] aliases) /// An enumerable collection of child templates. If the specified master template does not exist, returns an empty collection. public IEnumerable GetChildren(int masterTemplateId) { - //return from base.GetAll, this is all cached + if (TypedCachePolicy is { } policy) + { + if (masterTemplateId <= 0) + { + return policy.FindAllCached(x => x.MasterTemplateAlias.IsNullOrWhiteSpace(), PerformGetAll); + } + + ITemplate? parent = policy.FindCached(x => x.Id == masterTemplateId, PerformGetAll); + if (parent == null) + { + return Enumerable.Empty(); + } + + return policy.FindAllCached(x => x.MasterTemplateAlias.InvariantEquals(parent.Alias), PerformGetAll); + } + + // Fallback when caching is disabled. ITemplate[] all = GetMany().ToArray(); if (masterTemplateId <= 0) @@ -739,14 +780,13 @@ public IEnumerable GetChildren(int masterTemplateId) return all.Where(x => x.MasterTemplateAlias.IsNullOrWhiteSpace()); } - ITemplate? parent = all.FirstOrDefault(x => x.Id == masterTemplateId); - if (parent == null) + ITemplate? fallbackParent = all.FirstOrDefault(x => x.Id == masterTemplateId); + if (fallbackParent == null) { return Enumerable.Empty(); } - IEnumerable children = all.Where(x => x.MasterTemplateAlias.InvariantEquals(parent.Alias)); - return children; + return all.Where(x => x.MasterTemplateAlias.InvariantEquals(fallbackParent.Alias)); } /// diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/TemplateRepositoryTest.cs b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/TemplateRepositoryTest.cs index c2b1a11c523c..9f26dece87d2 100644 --- a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/TemplateRepositoryTest.cs +++ b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/TemplateRepositoryTest.cs @@ -986,4 +986,98 @@ private IEnumerable CreateHierarchy(ITemplateRepository repository) return new[] { parent, child1, child2, toddler1, toddler2, toddler3, toddler4, baby1, baby2 }; } + + [Test] + public void Get_By_Guid_Returns_Deep_Clone_Not_Cached_Instance() + { + var provider = ScopeProvider; + using (provider.CreateScope()) + { + var repository = CreateRepository(provider); + var template = new Template(ShortStringHelper, "deepCloneTest", "deepCloneTest"); + repository.Save(template); + + var first = repository.Get(template.Key); + var second = repository.Get(template.Key); + + Assert.IsNotNull(first); + Assert.IsNotNull(second); + Assert.AreEqual(first!.Id, second!.Id); + Assert.AreNotSame(first, second); + } + } + + [Test] + public void Get_By_Alias_Returns_Correct_Template() + { + var provider = ScopeProvider; + using (provider.CreateScope()) + { + var repository = CreateRepository(provider); + var template = new Template(ShortStringHelper, "aliasLookupTest", "aliasLookupTest"); + repository.Save(template); + + // Case-insensitive alias lookup. + var result = repository.Get("ALIASLOOKUPTEST"); + + Assert.IsNotNull(result); + Assert.AreEqual(template.Id, result!.Id); + } + } + + [Test] + public void Get_By_Alias_Returns_Deep_Clone_Not_Cached_Instance() + { + var provider = ScopeProvider; + using (provider.CreateScope()) + { + var repository = CreateRepository(provider); + var template = new Template(ShortStringHelper, "aliasCloneTest", "aliasCloneTest"); + repository.Save(template); + + var first = repository.Get("aliasCloneTest"); + var second = repository.Get("aliasCloneTest"); + + Assert.IsNotNull(first); + Assert.IsNotNull(second); + Assert.AreEqual(first!.Id, second!.Id); + Assert.AreNotSame(first, second); + } + } + + [Test] + public void Exists_By_Guid_Returns_Correct_Result() + { + var provider = ScopeProvider; + using (provider.CreateScope()) + { + var repository = CreateRepository(provider); + var template = new Template(ShortStringHelper, "existsTest", "existsTest"); + repository.Save(template); + + Assert.IsTrue(repository.Exists(template.Key)); + Assert.IsFalse(repository.Exists(Guid.NewGuid())); + } + } + + [Test] + public void Get_By_Guid_Mutation_Does_Not_Affect_Subsequent_Get() + { + var provider = ScopeProvider; + using (provider.CreateScope()) + { + var repository = CreateRepository(provider); + var template = new Template(ShortStringHelper, "mutationTest", "mutationTest"); + repository.Save(template); + + var first = repository.Get(template.Key); + Assert.IsNotNull(first); + var originalName = first!.Name; + first.Name = "MUTATED_" + Guid.NewGuid(); + + var second = repository.Get(template.Key); + Assert.IsNotNull(second); + Assert.AreEqual(originalName, second!.Name, "Mutation of a returned entity should not affect the cached copy"); + } + } } From f8d2fa4b720f689c8a4e6f64963270c2bda9ab3d Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Thu, 26 Mar 2026 07:33:04 +0100 Subject: [PATCH 4/5] Optimize DomainRepository to avoid unnecessary deep-cloning on cache reads. --- .../Implement/DomainRepository.cs | 31 +++++-- .../Repositories/DomainRepositoryTest.cs | 87 +++++++++++++++++++ 2 files changed, 113 insertions(+), 5 deletions(-) diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/DomainRepository.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/DomainRepository.cs index 37bf554ff6ae..e589ad7abe3b 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/DomainRepository.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/DomainRepository.cs @@ -38,13 +38,21 @@ public DomainRepository( { } + /// + /// Gets the cache policy as for predicate-based lookups. + /// Returns null when caching is disabled (e.g. ). + /// + private FullDataSetRepositoryCachePolicy? TypedCachePolicy + => CachePolicy as FullDataSetRepositoryCachePolicy; + /// /// Gets a domain by its name. /// /// The name of the domain to retrieve. /// The domain matching the specified name, or null if none found. public IDomain? GetByName(string domainName) - => GetMany().FirstOrDefault(x => x.DomainName.InvariantEquals(domainName)); + => TypedCachePolicy?.FindCached(x => x.DomainName.InvariantEquals(domainName), PerformGetAll) + ?? GetMany().FirstOrDefault(x => x.DomainName.InvariantEquals(domainName)); /// /// Determines whether a domain with the specified name exists. @@ -52,7 +60,8 @@ public DomainRepository( /// The name of the domain to check for existence. /// True if the domain exists; otherwise, false. public bool Exists(string domainName) - => GetMany().Any(x => x.DomainName.InvariantEquals(domainName)); + => TypedCachePolicy?.ExistsCached(x => x.DomainName.InvariantEquals(domainName), PerformGetAll) + ?? GetMany().Any(x => x.DomainName.InvariantEquals(domainName)); /// /// Gets all domains, optionally including wildcard domains. @@ -60,7 +69,15 @@ public bool Exists(string domainName) /// If true, includes wildcard domains in the result; otherwise, excludes them. /// An enumerable collection of domains. public IEnumerable GetAll(bool includeWildcards) - => GetMany().Where(x => includeWildcards || x.IsWildcard == false); + { + if (includeWildcards) + { + return GetMany(); + } + + return TypedCachePolicy?.FindAllCached(x => x.IsWildcard == false, PerformGetAll) + ?? GetMany().Where(x => x.IsWildcard == false); + } /// /// Retrieves the domains assigned to the specified content item. @@ -69,13 +86,17 @@ public IEnumerable GetAll(bool includeWildcards) /// If true, includes wildcard domains in the results; if false, only non-wildcard domains are returned. /// An containing the domains assigned to the specified content item. public IEnumerable GetAssignedDomains(int contentId, bool includeWildcards) - => GetMany().Where(x => x.RootContentId == contentId).Where(x => includeWildcards || x.IsWildcard == false); + => TypedCachePolicy?.FindAllCached(x => x.RootContentId == contentId && (includeWildcards || x.IsWildcard == false), PerformGetAll) + ?? GetMany().Where(x => x.RootContentId == contentId).Where(x => includeWildcards || x.IsWildcard == false); protected override IRepositoryCachePolicy CreateCachePolicy() => new FullDataSetRepositoryCachePolicy(GlobalIsolatedCache, ScopeAccessor, RepositoryCacheVersionService, CacheSyncService, GetEntityId, false); + // Note: PerformGet(int) is passed as a callback to the cache policy's Get(TId) method, + // but FullDataSetRepositoryCachePolicy.Get() never invokes it — it uses GetAllCached() + // internally and clones only the matched entity. This override exists only as a required + // implementation of the abstract base and as a fallback for non-FullDataSet policies. protected override IDomain? PerformGet(int id) - // Use the underlying GetAll which will force cache all domains => GetMany().FirstOrDefault(x => x.Id == id); protected override IEnumerable PerformGetAll(params int[]? ids) diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/DomainRepositoryTest.cs b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/DomainRepositoryTest.cs index c8e0e9fed86b..52fd993dba08 100644 --- a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/DomainRepositoryTest.cs +++ b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/DomainRepositoryTest.cs @@ -417,4 +417,91 @@ public void Get_All_For_Content_Without_Wildcards() Assert.AreEqual(0, all2.Count()); } } + + [Test] + public void GetByName_Returns_Deep_Clone_Not_Cached_Instance() + { + var contentId = CreateTestData("en-AU", out _); + var provider = ScopeProvider; + using (var scope = provider.CreateScope()) + { + var repo = CreateRepository(provider); + var content = DocumentRepository.Get(contentId); + var lang = LanguageRepository.GetByIsoCode("en-AU"); + var domain = (IDomain)new UmbracoDomain("clone-test.com") { RootContentId = content.Id, LanguageId = lang.Id }; + repo.Save(domain); + + var first = repo.GetByName("clone-test.com"); + var second = repo.GetByName("clone-test.com"); + + Assert.IsNotNull(first); + Assert.IsNotNull(second); + Assert.AreEqual(first!.Id, second!.Id); + Assert.AreNotSame(first, second); + } + } + + [Test] + public void Exists_By_Name_Returns_Correct_Result() + { + var contentId = CreateTestData("en-AU", out _); + var provider = ScopeProvider; + using (var scope = provider.CreateScope()) + { + var repo = CreateRepository(provider); + var content = DocumentRepository.Get(contentId); + var lang = LanguageRepository.GetByIsoCode("en-AU"); + var domain = (IDomain)new UmbracoDomain("exists-test.com") { RootContentId = content.Id, LanguageId = lang.Id }; + repo.Save(domain); + + Assert.IsTrue(repo.Exists("exists-test.com")); + Assert.IsFalse(repo.Exists("nonexistent.com")); + } + } + + [Test] + public void GetByName_Mutation_Does_Not_Affect_Subsequent_Get() + { + var contentId = CreateTestData("en-AU", out _); + var provider = ScopeProvider; + using (var scope = provider.CreateScope()) + { + var repo = CreateRepository(provider); + var content = DocumentRepository.Get(contentId); + var lang = LanguageRepository.GetByIsoCode("en-AU"); + var domain = (IDomain)new UmbracoDomain("mutation-test.com") { RootContentId = content.Id, LanguageId = lang.Id }; + repo.Save(domain); + + var first = repo.GetByName("mutation-test.com"); + Assert.IsNotNull(first); + var originalName = first!.DomainName; + first.DomainName = "MUTATED_" + Guid.NewGuid(); + + var second = repo.GetByName("mutation-test.com"); + Assert.IsNotNull(second); + Assert.AreEqual(originalName, second!.DomainName, "Mutation of a returned entity should not affect the cached copy"); + } + } + + [Test] + public void GetAssignedDomains_Returns_Only_Matching_Domains() + { + var contentId = CreateTestData("en-AU", out _); + var provider = ScopeProvider; + using (var scope = provider.CreateScope()) + { + var repo = CreateRepository(provider); + var content = DocumentRepository.Get(contentId); + var lang = LanguageRepository.GetByIsoCode("en-AU"); + + repo.Save((IDomain)new UmbracoDomain("assigned1.com") { RootContentId = content.Id, LanguageId = lang.Id }); + repo.Save((IDomain)new UmbracoDomain("assigned2.com") { RootContentId = content.Id, LanguageId = lang.Id }); + + var assigned = repo.GetAssignedDomains(content.Id, true).ToArray(); + Assert.AreEqual(2, assigned.Length); + + var unassigned = repo.GetAssignedDomains(-999, true).ToArray(); + Assert.AreEqual(0, unassigned.Length); + } + } } From 58dae9f0e4401a66f11de92324215468717ea29f Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Thu, 26 Mar 2026 07:39:31 +0100 Subject: [PATCH 5/5] Optimize remaining repositories to avoid unnecessary deep-cloning on cache reads. --- .../Implement/LogViewerQueryRepository.cs | 18 +++- .../Implement/PublicAccessRepository.cs | 6 +- .../Implement/RelationTypeRepository.cs | 22 ++-- .../Implement/ServerRegistrationRepository.cs | 8 +- .../LogViewerQueryRepositoryTest.cs | 101 ++++++++++++++++++ .../PublicAccessRepositoryTest.cs | 46 ++++++++ .../RelationTypeRepositoryTest.cs | 56 ++++++++++ .../ServerRegistrationRepositoryTest.cs | 35 ++++++ 8 files changed, 275 insertions(+), 17 deletions(-) create mode 100644 tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/LogViewerQueryRepositoryTest.cs diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/LogViewerQueryRepository.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/LogViewerQueryRepository.cs index 331478782395..b56a08246a41 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/LogViewerQueryRepository.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/LogViewerQueryRepository.cs @@ -37,13 +37,19 @@ public LogViewerQueryRepository( { } + /// + /// Gets the cache policy as for predicate-based lookups. + /// Returns null when caching is disabled (e.g. ). + /// + private FullDataSetRepositoryCachePolicy? TypedCachePolicy + => CachePolicy as FullDataSetRepositoryCachePolicy; + /// Retrieves a log viewer query by its name. /// The name of the log viewer query to retrieve. /// The log viewer query with the specified name, or null if not found. public ILogViewerQuery? GetByName(string name) => - - // use the underlying GetAll which will force cache all log queries - GetMany().FirstOrDefault(x => x.Name == name); + TypedCachePolicy?.FindCached(x => x.Name == name, PerformGetAll) + ?? GetMany().FirstOrDefault(x => x.Name == name); protected override IRepositoryCachePolicy CreateCachePolicy() => new FullDataSetRepositoryCachePolicy(GlobalIsolatedCache, ScopeAccessor, RepositoryCacheVersionService, CacheSyncService, GetEntityId, /*expires:*/ false); @@ -120,9 +126,11 @@ protected override void PersistUpdatedItem(ILogViewerQuery entity) Database.Update(dto); } + // Note: PerformGet(int) is passed as a callback to the cache policy's Get(TId) method, + // but FullDataSetRepositoryCachePolicy.Get() never invokes it — it uses GetAllCached() + // internally and clones only the matched entity. This override exists only as a required + // implementation of the abstract base and as a fallback for non-FullDataSet policies. protected override ILogViewerQuery? PerformGet(int id) => - - // use the underlying GetAll which will force cache all log queries GetMany().FirstOrDefault(x => x.Id == id); private static ILogViewerQuery ConvertFromDto(LogViewerQueryDto dto) diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/PublicAccessRepository.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/PublicAccessRepository.cs index 6758e8cb92ca..fc696eac8f66 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/PublicAccessRepository.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/PublicAccessRepository.cs @@ -41,9 +41,11 @@ public PublicAccessRepository( protected override IRepositoryCachePolicy CreateCachePolicy() => new FullDataSetRepositoryCachePolicy(GlobalIsolatedCache, ScopeAccessor, RepositoryCacheVersionService, CacheSyncService, GetEntityId, /*expires:*/ false); + // Note: PerformGet(Guid) is passed as a callback to the cache policy's Get(TId) method, + // but FullDataSetRepositoryCachePolicy.Get() never invokes it — it uses GetAllCached() + // internally and clones only the matched entity. This override exists only as a required + // implementation of the abstract base and as a fallback for non-FullDataSet policies. protected override PublicAccessEntry? PerformGet(Guid id) => - - // return from GetAll - this will be cached as a collection GetMany().FirstOrDefault(x => x.Key == id); protected override IEnumerable PerformGetAll(params Guid[]? ids) diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/RelationTypeRepository.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/RelationTypeRepository.cs index 08c8d95cc715..32f2a3a9ae4b 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/RelationTypeRepository.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/RelationTypeRepository.cs @@ -60,9 +60,18 @@ private static void CheckNullObjectTypeValues(IRelationType entity) #region Overrides of RepositoryBase - protected override IRelationType? PerformGet(int id) => + /// + /// Gets the cache policy as for predicate-based lookups. + /// Returns null when caching is disabled (e.g. ). + /// + private FullDataSetRepositoryCachePolicy? TypedCachePolicy + => CachePolicy as FullDataSetRepositoryCachePolicy; - // use the underlying GetAll which will force cache all content types + // Note: PerformGet(int) is passed as a callback to the cache policy's Get(TId) method, + // but FullDataSetRepositoryCachePolicy.Get() never invokes it — it uses GetAllCached() + // internally and clones only the matched entity. This override exists only as a required + // implementation of the abstract base and as a fallback for non-FullDataSet policies. + protected override IRelationType? PerformGet(int id) => GetMany()?.FirstOrDefault(x => x.Id == id); /// @@ -71,16 +80,17 @@ private static void CheckNullObjectTypeValues(IRelationType entity) /// The unique identifier of the relation type. /// The relation type matching the specified identifier, or null if not found. public IRelationType? Get(Guid id) => - - // use the underlying GetAll which will force cache all content types - GetMany()?.FirstOrDefault(x => x.Key == id); + TypedCachePolicy?.FindCached(x => x.Key == id, PerformGetAll) + ?? GetMany()?.FirstOrDefault(x => x.Key == id); /// /// Determines whether a relation type with the specified identifier exists. /// /// The unique identifier of the relation type. /// True if the relation type exists; otherwise, false. - public bool Exists(Guid id) => Get(id) != null; + public bool Exists(Guid id) => + TypedCachePolicy?.ExistsCached(x => x.Key == id, PerformGetAll) + ?? GetMany()?.Any(x => x.Key == id) == true; protected override IEnumerable PerformGetAll(params int[]? ids) { diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/ServerRegistrationRepository.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/ServerRegistrationRepository.cs index cf332222fd88..8287be419ca1 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/ServerRegistrationRepository.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/ServerRegistrationRepository.cs @@ -72,14 +72,14 @@ protected override IRepositoryCachePolicy CreateCacheP protected override int PerformCount(IQuery? query) => throw new NotSupportedException("This repository does not support this method."); + // Note: PerformExists(int) and PerformGet(int) are passed as callbacks to the cache + // policy, but FullDataSetRepositoryCachePolicy never invokes them — it uses GetAllCached() + // internally. These overrides exist only as required implementations of the abstract base + // and as fallbacks for non-FullDataSet policies. protected override bool PerformExists(int id) => - - // use the underlying GetAll which force-caches all registrations GetMany().Any(x => x.Id == id); protected override IServerRegistration? PerformGet(int id) => - - // use the underlying GetAll which force-caches all registrations GetMany().FirstOrDefault(x => x.Id == id); protected override IEnumerable PerformGetAll(params int[]? ids) => diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/LogViewerQueryRepositoryTest.cs b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/LogViewerQueryRepositoryTest.cs new file mode 100644 index 000000000000..69bfce26467d --- /dev/null +++ b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/LogViewerQueryRepositoryTest.cs @@ -0,0 +1,101 @@ +// Copyright (c) Umbraco. +// See LICENSE for more details. + +using Microsoft.Extensions.Logging; +using Moq; +using NUnit.Framework; +using Umbraco.Cms.Core.Cache; +using Umbraco.Cms.Core.Models; +using Umbraco.Cms.Core.Persistence.Repositories; +using Umbraco.Cms.Infrastructure.Persistence.Repositories.Implement; +using Umbraco.Cms.Infrastructure.Scoping; +using Umbraco.Cms.Tests.Common.Testing; +using Umbraco.Cms.Tests.Integration.Testing; + +namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Persistence.Repositories; + +[TestFixture] +[UmbracoTest(Database = UmbracoTestOptions.Database.NewSchemaPerTest)] +internal sealed class LogViewerQueryRepositoryTest : UmbracoIntegrationTest +{ + private LogViewerQueryRepository CreateRepository(IScopeProvider provider) => + new( + (IScopeAccessor)provider, + AppCaches.Disabled, + LoggerFactory.CreateLogger(), + Mock.Of(), + Mock.Of()); + + [Test] + public void Can_Create_And_Get_By_Name() + { + var provider = ScopeProvider; + using (provider.CreateScope()) + { + var repository = CreateRepository(provider); + var query = new LogViewerQuery("Test Query", "@Level='Error'"); + repository.Save(query); + + var found = repository.GetByName("Test Query"); + + Assert.IsNotNull(found); + Assert.AreEqual(query.Id, found!.Id); + Assert.AreEqual("Test Query", found.Name); + Assert.AreEqual("@Level='Error'", found.Query); + } + } + + [Test] + public void GetByName_Returns_Null_For_NonExisting() + { + var provider = ScopeProvider; + using (provider.CreateScope()) + { + var repository = CreateRepository(provider); + + var found = repository.GetByName("nonexistent"); + + Assert.IsNull(found); + } + } + + [Test] + public void GetByName_Returns_Deep_Clone_Not_Cached_Instance() + { + var provider = ScopeProvider; + using (provider.CreateScope()) + { + var repository = CreateRepository(provider); + var query = new LogViewerQuery("Clone Test", "@Level='Warning'"); + repository.Save(query); + + var first = repository.GetByName("Clone Test"); + var second = repository.GetByName("Clone Test"); + + Assert.IsNotNull(first); + Assert.IsNotNull(second); + Assert.AreEqual(first!.Id, second!.Id); + Assert.AreNotSame(first, second); + } + } + + [Test] + public void GetByName_Mutation_Does_Not_Affect_Subsequent_Get() + { + var provider = ScopeProvider; + using (provider.CreateScope()) + { + var repository = CreateRepository(provider); + var query = new LogViewerQuery("Mutation Test", "@Level='Fatal'"); + repository.Save(query); + + var first = repository.GetByName("Mutation Test"); + Assert.IsNotNull(first); + first!.Name = "MUTATED_" + Guid.NewGuid(); + + var second = repository.GetByName("Mutation Test"); + Assert.IsNotNull(second); + Assert.AreEqual("Mutation Test", second!.Name, "Mutation of a returned entity should not affect the cached copy"); + } + } +} diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/PublicAccessRepositoryTest.cs b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/PublicAccessRepositoryTest.cs index 9fcbf9c5980b..92b4f369a38e 100644 --- a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/PublicAccessRepositoryTest.cs +++ b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/PublicAccessRepositoryTest.cs @@ -271,4 +271,50 @@ private IEnumerable CreateTestData(int count) return result; } } + + [Test] + public void Get_By_Guid_Returns_Deep_Clone_Not_Cached_Instance() + { + var content = CreateTestData(3).ToArray(); + var provider = ScopeProvider; + using (var scope = provider.CreateScope()) + { + var repo = new PublicAccessRepository((IScopeAccessor)provider, AppCaches, LoggerFactory.CreateLogger(), Mock.Of(), Mock.Of()); + PublicAccessRule[] rules = { new PublicAccessRule { RuleValue = "test", RuleType = "RoleName" } }; + var entry = new PublicAccessEntry(content[0], content[1], content[2], rules); + repo.Save(entry); + + var first = repo.Get(entry.Key); + var second = repo.Get(entry.Key); + + Assert.IsNotNull(first); + Assert.IsNotNull(second); + Assert.AreEqual(first!.Key, second!.Key); + Assert.AreNotSame(first, second); + } + } + + [Test] + public void Get_By_Guid_Mutation_Does_Not_Affect_Subsequent_Get() + { + var content = CreateTestData(3).ToArray(); + var provider = ScopeProvider; + using (var scope = provider.CreateScope()) + { + var repo = new PublicAccessRepository((IScopeAccessor)provider, AppCaches, LoggerFactory.CreateLogger(), Mock.Of(), Mock.Of()); + PublicAccessRule[] rules = { new PublicAccessRule { RuleValue = "test", RuleType = "RoleName" } }; + var entry = new PublicAccessEntry(content[0], content[1], content[2], rules); + repo.Save(entry); + + var first = repo.Get(entry.Key); + Assert.IsNotNull(first); + var originalNodeId = first!.ProtectedNodeId; + first.ClearRules(); + + var second = repo.Get(entry.Key); + Assert.IsNotNull(second); + Assert.AreEqual(originalNodeId, second!.ProtectedNodeId); + Assert.AreEqual(1, second.Rules.Count(), "Mutation of a returned entity should not affect the cached copy"); + } + } } diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/RelationTypeRepositoryTest.cs b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/RelationTypeRepositoryTest.cs index becaf5898c83..2fb5a7c2239c 100644 --- a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/RelationTypeRepositoryTest.cs +++ b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/RelationTypeRepositoryTest.cs @@ -249,4 +249,60 @@ public void CreateTestData() scope.Complete(); } } + + [Test] + public void Get_By_Guid_Returns_Deep_Clone_Not_Cached_Instance() + { + ICoreScopeProvider provider = ScopeProvider; + using (provider.CreateCoreScope()) + { + var repository = CreateRepository(provider); + var relationType = new RelationType("Test Clone", "testClone", true, Constants.ObjectTypes.Document, Constants.ObjectTypes.Document, false); + repository.Save(relationType); + + var first = repository.Get(relationType.Key); + var second = repository.Get(relationType.Key); + + Assert.IsNotNull(first); + Assert.IsNotNull(second); + Assert.AreEqual(first!.Id, second!.Id); + Assert.AreNotSame(first, second); + } + } + + [Test] + public void Exists_By_Guid_Returns_Correct_Result() + { + ICoreScopeProvider provider = ScopeProvider; + using (provider.CreateCoreScope()) + { + var repository = CreateRepository(provider); + var relationType = new RelationType("Test Exists", "testExists", true, Constants.ObjectTypes.Document, Constants.ObjectTypes.Document, false); + repository.Save(relationType); + + Assert.IsTrue(repository.Exists(relationType.Key)); + Assert.IsFalse(repository.Exists(Guid.NewGuid())); + } + } + + [Test] + public void Get_By_Guid_Mutation_Does_Not_Affect_Subsequent_Get() + { + ICoreScopeProvider provider = ScopeProvider; + using (provider.CreateCoreScope()) + { + var repository = CreateRepository(provider); + var relationType = new RelationType("Test Mutation", "testMutation", true, Constants.ObjectTypes.Document, Constants.ObjectTypes.Document, false); + repository.Save(relationType); + + var first = repository.Get(relationType.Key); + Assert.IsNotNull(first); + var originalName = first!.Name; + first.Name = "MUTATED_" + Guid.NewGuid(); + + var second = repository.Get(relationType.Key); + Assert.IsNotNull(second); + Assert.AreEqual(originalName, second!.Name, "Mutation of a returned entity should not affect the cached copy"); + } + } } diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/ServerRegistrationRepositoryTest.cs b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/ServerRegistrationRepositoryTest.cs index 395a0f6703bd..024005604870 100644 --- a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/ServerRegistrationRepositoryTest.cs +++ b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/ServerRegistrationRepositoryTest.cs @@ -231,6 +231,41 @@ public void Can_Perform_Exists_On_Repository() } } + [Test] + public void Get_By_Id_Returns_Deep_Clone_Not_Cached_Instance() + { + var provider = ScopeProvider; + using (provider.CreateScope()) + { + var repository = CreateRepository(provider); + var all = repository.GetMany().ToArray(); + Assert.IsTrue(all.Length > 0); + + var first = repository.Get(all[0].Id); + var second = repository.Get(all[0].Id); + + Assert.IsNotNull(first); + Assert.IsNotNull(second); + Assert.AreEqual(first!.Id, second!.Id); + Assert.AreNotSame(first, second); + } + } + + [Test] + public void Exists_Returns_Correct_Result() + { + var provider = ScopeProvider; + using (provider.CreateScope()) + { + var repository = CreateRepository(provider); + var all = repository.GetMany().ToArray(); + Assert.IsTrue(all.Length > 0); + + Assert.IsTrue(repository.Exists(all[0].Id)); + Assert.IsFalse(repository.Exists(99999)); + } + } + public void CreateTestData() { var provider = ScopeProvider;