Performance: Request cache referenced entities when saving documents with block editors (#20590)

AndyButland · Copilot · kjac · AndyButland · commit 96ecef0a9267 · 2025-10-31T12:41:46.000+01:00
* Added request cache to content and media lookups in mult URL picker.

* Allow property editors to cache referenced entities from block data.

* Update src/Umbraco.Infrastructure/PropertyEditors/MultiUrlPickerValueEditor.cs

Co-authored-by: Copilot &lt;175728472+Copilot@users.noreply.github.com&gt;

* Add obsoletions.

* Minor spellcheck

* Ensure request cache is available before relying on it.

---------

Co-authored-by: Copilot &lt;175728472+Copilot@users.noreply.github.com&gt;
Co-authored-by: kjac &lt;kja@umbraco.dk&gt;
diff --git a/src/Umbraco.Core/PropertyEditors/DataValueEditor.cs b/src/Umbraco.Core/PropertyEditors/DataValueEditor.cs
@@ -3,12 +3,14 @@
 using System.Runtime.Serialization;
 using System.Xml.Linq;
 using Microsoft.Extensions.Logging;
+using Umbraco.Cms.Core.Cache;
 using Umbraco.Cms.Core.IO;
 using Umbraco.Cms.Core.Models;
 using Umbraco.Cms.Core.Models.Editors;
 using Umbraco.Cms.Core.Models.Validation;
 using Umbraco.Cms.Core.PropertyEditors.Validators;
 using Umbraco.Cms.Core.Serialization;
+using Umbraco.Cms.Core.Services;
 using Umbraco.Cms.Core.Strings;
 using Umbraco.Extensions;
 
@@ -20,6 +22,9 @@ namespace Umbraco.Cms.Core.PropertyEditors;
 [DataContract]
 public class DataValueEditor : IDataValueEditor
 {
+    private const string ContentCacheKeyFormat = nameof(DataValueEditor) + "_Content_{0}";
+    private const string MediaCacheKeyFormat = nameof(DataValueEditor) + "_Media_{0}";
+
     private readonly IJsonSerializer? _jsonSerializer;
     private readonly IShortStringHelper _shortStringHelper;
 
@@ -415,4 +420,155 @@ public virtual string ConvertDbToString(IPropertyType propertyType, object? valu
 
         return value.TryConvertTo(valueType);
     }
+
+    /// <summary>
+    /// Retrieves a <see cref="IContent"/> instance by its unique identifier, using the provided request cache to avoid redundant
+    /// lookups within the same request.
+    /// </summary>
+    /// <remarks>
+    /// This method caches content lookups for the duration of the current request to improve performance when the same content
+    /// item may be accessed multiple times. This is particularly useful in scenarios involving multiple languages or blocks.
+    /// </remarks>
+    /// <param name="key">The unique identifier of the content item to retrieve.</param>
+    /// <param name="requestCache">The request-scoped cache used to store and retrieve content items for the duration of the current request.</param>
+    /// <param name="contentService">The content service used to fetch the content item if it is not found in the cache.</param>
+    /// <returns>The <see cref="IContent"/> instance corresponding to the specified key, or null if no such content item exists.</returns>
+    [Obsolete("This method is available for support of request caching retrieved entities in derived property value editors. " +
+          "The intention is to supersede this with lazy loaded read locks, which will make this unnecessary. " +
+          "Scheduled for removal in Umbraco 19.")]
+    protected static IContent? GetAndCacheContentById(Guid key, IRequestCache requestCache, IContentService contentService)
+    {
+        if (requestCache.IsAvailable is false)
+        {
+            return contentService.GetById(key);
+        }
+
+        var cacheKey = string.Format(ContentCacheKeyFormat, key);
+        IContent? content = requestCache.GetCacheItem<IContent?>(cacheKey);
+        if (content is null)
+        {
+            content = contentService.GetById(key);
+            if (content is not null)
+            {
+                requestCache.Set(cacheKey, content);
+            }
+        }
+
+        return content;
+    }
+
+    /// <summary>
+    /// Adds the specified <see cref="IContent"/> item to the request cache using its unique key.
+    /// </summary>
+    /// <param name="content">The content item to cache.</param>
+    /// <param name="requestCache">The request cache in which to store the content item.</param>
+    [Obsolete("This method is available for support of request caching retrieved entities in derived property value editors. " +
+          "The intention is to supersede this with lazy loaded read locks, which will make this unnecessary. " +
+          "Scheduled for removal in Umbraco 19.")]
+    protected static void CacheContentById(IContent content, IRequestCache requestCache)
+    {
+        if (requestCache.IsAvailable is false)
+        {
+            return;
+        }
+
+        var cacheKey = string.Format(ContentCacheKeyFormat, content.Key);
+        requestCache.Set(cacheKey, content);
+    }
+
+    /// <summary>
+    /// Retrieves a <see cref="IMedia"/> instance by its unique identifier, using the provided request cache to avoid redundant
+    /// lookups within the same request.
+    /// </summary>
+    /// <remarks>
+    /// This method caches media lookups for the duration of the current request to improve performance when the same media
+    /// item may be accessed multiple times. This is particularly useful in scenarios involving multiple languages or blocks.
+    /// </remarks>
+    /// <param name="key">The unique identifier of the media item to retrieve.</param>
+    /// <param name="requestCache">The request-scoped cache used to store and retrieve media items for the duration of the current request.</param>
+    /// <param name="mediaService">The media service used to fetch the media item if it is not found in the cache.</param>
+    /// <returns>The <see cref="IMedia"/> instance corresponding to the specified key, or null if no such media item exists.</returns>
+    [Obsolete("This method is available for support of request caching retrieved entities in derived property value editors. " +
+          "The intention is to supersede this with lazy loaded read locks, which will make this unnecessary. " +
+          "Scheduled for removal in Umbraco 19.")]
+    protected static IMedia? GetAndCacheMediaById(Guid key, IRequestCache requestCache, IMediaService mediaService)
+    {
+        if (requestCache.IsAvailable is false)
+        {
+            return mediaService.GetById(key);
+        }
+
+        var cacheKey = string.Format(MediaCacheKeyFormat, key);
+        IMedia? media = requestCache.GetCacheItem<IMedia?>(cacheKey);
+
+        if (media is null)
+        {
+            media = mediaService.GetById(key);
+            if (media is not null)
+            {
+                requestCache.Set(cacheKey, media);
+            }
+        }
+
+        return media;
+    }
+
+    /// <summary>
+    /// Adds the specified <see cref="IMedia"/> item to the request cache using its unique key.
+    /// </summary>
+    /// <param name="media">The media item to cache.</param>
+    /// <param name="requestCache">The request cache in which to store the media item.</param>
+    [Obsolete("This method is available for support of request caching retrieved entities in derived property value editors. " +
+          "The intention is to supersede this with lazy loaded read locks, which will make this unnecessary. " +
+          "Scheduled for removal in Umbraco 19.")]
+    protected static void CacheMediaById(IMedia media, IRequestCache requestCache)
+    {
+        if (requestCache.IsAvailable is false)
+        {
+            return;
+        }
+
+        var cacheKey = string.Format(MediaCacheKeyFormat, media.Key);
+        requestCache.Set(cacheKey, media);
+    }
+
+    /// <summary>
+    /// Determines whether the content item identified by the specified key is present in the request cache.
+    /// </summary>
+    /// <param name="key">The unique identifier for the content item to check for in the cache.</param>
+    /// <param name="requestCache">The request cache in which to look for the content item.</param>
+    /// <returns>true if the content item is already cached in the request cache; otherwise, false.</returns>
+    [Obsolete("This method is available for support of request caching retrieved entities in derived property value editors. " +
+          "The intention is to supersede this with lazy loaded read locks, which will make this unnecessary. " +
+          "Scheduled for removal in Umbraco 19.")]
+    protected static bool IsContentAlreadyCached(Guid key, IRequestCache requestCache)
+    {
+        if (requestCache.IsAvailable is false)
+        {
+            return false;
+        }
+
+        var cacheKey = string.Format(ContentCacheKeyFormat, key);
+        return requestCache.GetCacheItem<IContent?>(cacheKey) is not null;
+    }
+
+    /// <summary>
+    /// Determines whether the media item identified by the specified key is present in the request cache.
+    /// </summary>
+    /// <param name="key">The unique identifier for the media item to check for in the cache.</param>
+    /// <param name="requestCache">The request cache in which to look for the media item.</param>
+    /// <returns>true if the media item is already cached in the request cache; otherwise, false.</returns>
+    [Obsolete("This method is available for support of request caching retrieved entities in derived property value editors. " +
+              "The intention is to supersede this with lazy loaded read locks, which will make this unnecessary. " +
+              "Scheduled for removal in Umbraco 19.")]
+    protected static bool IsMediaAlreadyCached(Guid key, IRequestCache requestCache)
+    {
+        if (requestCache.IsAvailable is false)
+        {
+            return false;
+        }
+
+        var cacheKey = string.Format(MediaCacheKeyFormat, key);
+        return requestCache.GetCacheItem<IMedia?>(cacheKey) is not null;
+    }
 }
diff --git a/src/Umbraco.Core/PropertyEditors/ICacheReferencedEntities.cs b/src/Umbraco.Core/PropertyEditors/ICacheReferencedEntities.cs
@@ -0,0 +1,19 @@
+namespace Umbraco.Cms.Core.PropertyEditors;
+
+/// <summary>
+/// Optionally implemented by property editors, this defines a contract for caching entities that are referenced in block values.
+/// </summary>
+[Obsolete("This interface is available for support of request caching retrieved entities in property value editors that implement it. " +
+          "The intention is to supersede this with lazy loaded read locks, which will make this unnecessary. " +
+          "Scheduled for removal in Umbraco 19.")]
+public interface ICacheReferencedEntities
+{
+    /// <summary>
+    /// Caches the entities referenced by the provided block data values.
+    /// </summary>
+    /// <param name="values">An enumerable collection of block values that may contain the entities to be cached.</param>
+    [Obsolete("This method is available for support of request caching retrieved entities in derived property value editors. " +
+              "The intention is to supersede this with lazy loaded read locks, which will make this unnecessary. " +
+              "Scheduled for removal in Umbraco 19.")]
+    void CacheReferencedEntities(IEnumerable<object> values);
+}
diff --git a/src/Umbraco.Infrastructure/PropertyEditors/BlockEditorPropertyValueEditor.cs b/src/Umbraco.Infrastructure/PropertyEditors/BlockEditorPropertyValueEditor.cs
@@ -135,6 +135,8 @@ public override object ToEditor(IProperty property, string? culture = null, stri
         BlockEditorData<TValue, TLayout>? currentBlockEditorData = SafeParseBlockEditorData(currentValue);
         BlockEditorData<TValue, TLayout>? blockEditorData = SafeParseBlockEditorData(editorValue.Value);
 
+        CacheReferencedEntities(blockEditorData);
+
         // We can skip MapBlockValueFromEditor if both editorValue and currentValue values are empty.
         if (IsBlockEditorDataEmpty(currentBlockEditorData) && IsBlockEditorDataEmpty(blockEditorData))
         {
diff --git a/src/Umbraco.Infrastructure/PropertyEditors/BlockValuePropertyValueEditorBase.cs b/src/Umbraco.Infrastructure/PropertyEditors/BlockValuePropertyValueEditorBase.cs
@@ -43,6 +43,45 @@ protected BlockValuePropertyValueEditorBase(
         _languageService = languageService;
     }
 
+    /// <summary>
+    /// Caches referenced entities for all property values with supporting property editors within the specified block editor data
+    /// optimising subsequent retrieval of entities when parsing and converting property values.
+    /// </summary>
+    /// <remarks>
+    /// This method iterates through all property values associated with data editors in the provided
+    /// block editor data and invokes caching for referenced entities where supported by the property editor.
+    /// </remarks>
+    /// <param name="blockEditorData">The block editor data containing content and settings property values to analyze for referenced entities.</param>
+    [Obsolete("This method is available for support of request caching retrieved entities in derived property value editors. " +
+              "The intention is to supersede this with lazy loaded read locks, which will make this unnecessary. " +
+              "Scheduled for removal in Umbraco 19.")]
+    protected void CacheReferencedEntities(BlockEditorData<TValue, TLayout>? blockEditorData)
+    {
+        // Group property values by their associated data editor alias.
+        IEnumerable<IGrouping<string, BlockPropertyValue>> valuesByDataEditors = (blockEditorData?.BlockValue.ContentData ?? []).Union(blockEditorData?.BlockValue.SettingsData ?? [])
+            .SelectMany(x => x.Values)
+            .Where(x => x.EditorAlias is not null && x.Value is not null)
+            .GroupBy(x => x.EditorAlias!);
+
+        // Iterate through each group and cache referenced entities if supported by the data editor.
+        foreach (IGrouping<string, BlockPropertyValue> valueByDataEditor in valuesByDataEditors)
+        {
+            IDataEditor? dataEditor = _propertyEditors[valueByDataEditor.Key];
+            if (dataEditor is null)
+            {
+                continue;
+            }
+
+            IDataValueEditor valueEditor = dataEditor.GetValueEditor();
+
+            if (valueEditor is ICacheReferencedEntities valueEditorWithPrecaching)
+            {
+                valueEditorWithPrecaching.CacheReferencedEntities(valueByDataEditor.Select(x => x.Value!));
+            }
+        }
+    }
+
+
     /// <inheritdoc />
     public abstract IEnumerable<UmbracoEntityReference> GetReferences(object? value);
 
diff --git a/src/Umbraco.Infrastructure/PropertyEditors/MediaPicker3PropertyEditor.cs b/src/Umbraco.Infrastructure/PropertyEditors/MediaPicker3PropertyEditor.cs
@@ -52,10 +52,8 @@ protected override IDataValueEditor CreateValueEditor() =>
     /// <summary>
     /// Defines the value editor for the media picker property editor.
     /// </summary>
-    internal sealed class MediaPicker3PropertyValueEditor : DataValueEditor, IDataValueReference
+    internal sealed class MediaPicker3PropertyValueEditor : DataValueEditor, IDataValueReference, ICacheReferencedEntities
     {
-        private const string MediaCacheKeyFormat = nameof(MediaPicker3PropertyValueEditor) + "_Media_{0}";
-
         private readonly IDataTypeConfigurationCache _dataTypeReadCache;
         private readonly IJsonSerializer _jsonSerializer;
         private readonly IMediaImportService _mediaImportService;
@@ -107,6 +105,27 @@ public MediaPicker3PropertyValueEditor(
             Validators.Add(validators);
         }
 
+        /// <inheritdoc/>
+        public void CacheReferencedEntities(IEnumerable<object> values)
+        {
+            var mediaKeys = values
+                .SelectMany(value => Deserialize(_jsonSerializer, value))
+                .Select(dto => dto.MediaKey)
+                .Distinct()
+                .Where(x => IsMediaAlreadyCached(x, _appCaches.RequestCache) is false)
+                .ToList();
+            if (mediaKeys.Count == 0)
+            {
+                return;
+            }
+
+            IEnumerable<IMedia> mediaItems = _mediaService.GetByIds(mediaKeys);
+            foreach (IMedia media in mediaItems)
+            {
+                CacheMediaById(media, _appCaches.RequestCache);
+            }
+        }
+
         /// <inheritdoc/>
         public IEnumerable<UmbracoEntityReference> GetReferences(object? value)
         {
@@ -208,39 +227,21 @@ private List<MediaWithCropsDto> UpdateMediaTypeAliases(List<MediaWithCropsDto> m
 
             foreach (MediaWithCropsDto mediaWithCropsDto in mediaWithCropsDtos)
             {
-                IMedia? media = GetMediaById(mediaWithCropsDto.MediaKey);
+                IMedia? media = GetAndCacheMediaById(mediaWithCropsDto.MediaKey, _appCaches.RequestCache, _mediaService);
                 mediaWithCropsDto.MediaTypeAlias = media?.ContentType.Alias ?? unknownMediaType;
             }
 
             return mediaWithCropsDtos.Where(m => m.MediaTypeAlias != unknownMediaType).ToList();
         }
 
-        private IMedia? GetMediaById(Guid key)
-        {
-            // Cache media lookups in case the same media is handled multiple times across a save operation,
-            // which is possible, particularly if we have multiple languages and blocks.
-            var cacheKey = string.Format(MediaCacheKeyFormat, key);
-            IMedia? media = _appCaches.RequestCache.GetCacheItem<IMedia?>(cacheKey);
-            if (media is null)
-            {
-                media = _mediaService.GetById(key);
-                if (media is not null)
-                {
-                    _appCaches.RequestCache.Set(cacheKey, media);
-                }
-            }
-
-            return media;
-        }
-
         private List<MediaWithCropsDto> HandleTemporaryMediaUploads(List<MediaWithCropsDto> mediaWithCropsDtos, MediaPicker3Configuration configuration)
         {
             var invalidDtos = new List<MediaWithCropsDto>();
 
             foreach (MediaWithCropsDto mediaWithCropsDto in mediaWithCropsDtos)
             {
                 // if the media already exist, don't bother with it
-                if (GetMediaById(mediaWithCropsDto.MediaKey) != null)
+                if (GetAndCacheMediaById(mediaWithCropsDto.MediaKey, _appCaches.RequestCache, _mediaService) != null)
                 {
                     continue;
                 }
@@ -480,18 +481,7 @@ public IEnumerable<ValidationResult> Validate(
 
                 foreach (var typeAlias in distinctTypeAliases)
                 {
-                    // 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<string?>(cacheKey);
-                    if (typeKey is null)
-                    {
-                        typeKey = _mediaTypeService.Get(typeAlias)?.Key.ToString();
-                        if (typeKey is not null)
-                        {
-                            _appCaches.RequestCache.Set(cacheKey, typeKey);
-                        }
-                    }
+                    string? typeKey = GetMediaTypeKey(typeAlias);
 
                     if (typeKey is null || allowedTypes.Contains(typeKey) is false)
                     {
@@ -506,6 +496,31 @@ public IEnumerable<ValidationResult> Validate(
 
                 return [];
             }
+
+            private string? GetMediaTypeKey(string 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.
+                string? GetMediaTypeKeyFromService(string typeAlias) => _mediaTypeService.Get(typeAlias)?.Key.ToString();
+
+                if (_appCaches.RequestCache.IsAvailable is false)
+                {
+                    return GetMediaTypeKeyFromService(typeAlias);
+                }
+
+                var cacheKey = string.Format(MediaTypeCacheKeyFormat, typeAlias);
+                string? typeKey = _appCaches.RequestCache.GetCacheItem<string?>(cacheKey);
+                if (typeKey is null)
+                {
+                    typeKey = GetMediaTypeKeyFromService(typeAlias);
+                    if (typeKey is not null)
+                    {
+                        _appCaches.RequestCache.Set(cacheKey, typeKey);
+                    }
+                }
+
+                return typeKey;
+            }
         }
 
         /// <summary>
diff --git a/src/Umbraco.Infrastructure/PropertyEditors/MultiUrlPickerValueEditor.cs b/src/Umbraco.Infrastructure/PropertyEditors/MultiUrlPickerValueEditor.cs
diff --git a/tests/Umbraco.Tests.UnitTests/Umbraco.Core/PropertyEditors/Validators/MultiUrlPickerValueEditorValidationTests.cs b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/PropertyEditors/Validators/MultiUrlPickerValueEditorValidationTests.cs

Original file line number	Diff line number	Diff line change
`@@ -135,6 +135,8 @@ public override object ToEditor(IProperty property, string? culture = null, stri`
`135`	`135`	`BlockEditorData<TValue, TLayout>? currentBlockEditorData = SafeParseBlockEditorData(currentValue);`
`136`	`136`	`BlockEditorData<TValue, TLayout>? blockEditorData = SafeParseBlockEditorData(editorValue.Value);`
`137`	`137`
	`138`	`+ CacheReferencedEntities(blockEditorData);`
	`139`	`+`
`138`	`140`	`// We can skip MapBlockValueFromEditor if both editorValue and currentValue values are empty.`
`139`	`141`	`if (IsBlockEditorDataEmpty(currentBlockEditorData) && IsBlockEditorDataEmpty(blockEditorData))`
`140`	`142`	`{`