Added logging and try/catch around retrieval of references, so we don't block critical operations following an incompatible data type change (#18576)

AndyButland · Migaroez · web-flow · commit 3f10bd8c21bd · 2025-05-19T10:54:22.000+02:00
* Added logging and try/catch around retrieval of references, so we don't block critical operations following an incompatible data type change.

* Added a little more detail to the log message.

* Added a little more detail to the log message.

* Fix unittest mock dependency

---------

Co-authored-by: Migaroez &lt;geusens@gmail.com&gt;
diff --git a/src/Umbraco.Core/PropertyEditors/DataValueReferenceFactoryCollection.cs b/src/Umbraco.Core/PropertyEditors/DataValueReferenceFactoryCollection.cs
@@ -1,4 +1,7 @@
+using Microsoft.Extensions.DependencyInjection;
+using Microsoft.Extensions.Logging;
 using Umbraco.Cms.Core.Composing;
+using Umbraco.Cms.Core.DependencyInjection;
 using Umbraco.Cms.Core.Models;
 using Umbraco.Cms.Core.Models.Editors;
 
@@ -12,14 +15,27 @@ public class DataValueReferenceFactoryCollection : BuilderCollectionBase<IDataVa
     // TODO: We could further reduce circular dependencies with PropertyEditorCollection by not having IDataValueReference implemented
     // by property editors and instead just use the already built in IDataValueReferenceFactory and/or refactor that into a more normal collection
 
+    private readonly ILogger<DataValueReferenceFactoryCollection> _logger;
+
     /// <summary>
     /// Initializes a new instance of the <see cref="DataValueReferenceFactoryCollection" /> class.
     /// </summary>
     /// <param name="items">The items.</param>
+    [Obsolete("Please use the constructor taking all parameters. Scheduled for removal in Umbraco 17.")]
     public DataValueReferenceFactoryCollection(Func<IEnumerable<IDataValueReferenceFactory>> items)
-        : base(items)
+        : this(
+              items,
+              StaticServiceProvider.Instance.GetRequiredService<ILogger<DataValueReferenceFactoryCollection>>())
     { }
 
+    /// <summary>
+    /// Initializes a new instance of the <see cref="DataValueReferenceFactoryCollection" /> class.
+    /// </summary>
+    /// <param name="items">The items.</param>
+    /// <param name="logger">The logger.</param>
+    public DataValueReferenceFactoryCollection(Func<IEnumerable<IDataValueReferenceFactory>> items, ILogger<DataValueReferenceFactoryCollection> logger)
+        : base(items) => _logger = logger;
+
     /// <summary>
     /// Gets all unique references from the specified properties.
     /// </summary>
@@ -33,7 +49,7 @@ public ISet<UmbracoEntityReference> GetAllReferences(IPropertyCollection propert
         var references = new HashSet<UmbracoEntityReference>();
 
         // Group by property editor alias to avoid duplicate lookups and optimize value parsing
-        foreach (var propertyValuesByPropertyEditorAlias in properties.GroupBy(x => x.PropertyType.PropertyEditorAlias, x => x.Values))
+        foreach (IGrouping<string, IReadOnlyCollection<IPropertyValue>> propertyValuesByPropertyEditorAlias in properties.GroupBy(x => x.PropertyType.PropertyEditorAlias, x => x.Values))
         {
             if (!propertyEditors.TryGet(propertyValuesByPropertyEditorAlias.Key, out IDataEditor? dataEditor))
             {
@@ -48,7 +64,7 @@ public ISet<UmbracoEntityReference> GetAllReferences(IPropertyCollection propert
                 values.Add(propertyValue.PublishedValue);
             }
 
-            references.UnionWith(GetReferences(dataEditor, values));
+            references.UnionWith(GetReferences(dataEditor, values, propertyValuesByPropertyEditorAlias.Key));
         }
 
         return references;
@@ -74,14 +90,18 @@ public IEnumerable<UmbracoEntityReference> GetReferences(IDataEditor dataEditor,
     /// The references.
     /// </returns>
     public ISet<UmbracoEntityReference> GetReferences(IDataEditor dataEditor, IEnumerable<object?> values) =>
-        GetReferencesEnumerable(dataEditor, values).ToHashSet();
-    private IEnumerable<UmbracoEntityReference> GetReferencesEnumerable(IDataEditor dataEditor, IEnumerable<object?> values)
+        GetReferencesEnumerable(dataEditor, values, null).ToHashSet();
+
+    private ISet<UmbracoEntityReference> GetReferences(IDataEditor dataEditor, IEnumerable<object?> values, string propertyEditorAlias) =>
+        GetReferencesEnumerable(dataEditor, values, propertyEditorAlias).ToHashSet();
+
+    private IEnumerable<UmbracoEntityReference> GetReferencesEnumerable(IDataEditor dataEditor, IEnumerable<object?> values, string? propertyEditorAlias)
     {
         // TODO: We will need to change this once we support tracking via variants/segments
         // for now, we are tracking values from ALL variants
         if (dataEditor.GetValueEditor() is IDataValueReference dataValueReference)
         {
-            foreach (UmbracoEntityReference reference in values.SelectMany(dataValueReference.GetReferences))
+            foreach (UmbracoEntityReference reference in GetReferencesFromPropertyValues(values, dataValueReference, propertyEditorAlias))
             {
                 yield return reference;
             }
@@ -107,6 +127,38 @@ private IEnumerable<UmbracoEntityReference> GetReferencesEnumerable(IDataEditor
         }
     }
 
+    private IEnumerable<UmbracoEntityReference> GetReferencesFromPropertyValues(IEnumerable<object?> values, IDataValueReference dataValueReference, string? propertyEditorAlias)
+    {
+        var result = new List<UmbracoEntityReference>();
+        foreach (var value in values)
+        {
+            // When property editors on data types are changed, we could have values that are incompatible with the new editor.
+            // Leading to issues such as:
+            // - https://github.com/umbraco/Umbraco-CMS/issues/17628
+            // - https://github.com/umbraco/Umbraco-CMS/issues/17725
+            // Although some changes like this are not intended to be compatible, we should handle them gracefully and not
+            // error in retrieving references, which would prevent manipulating or deleting the content that uses the data type.
+            try
+            {
+                IEnumerable<UmbracoEntityReference> references = dataValueReference.GetReferences(value);
+                result.AddRange(references);
+            }
+            catch (Exception ex)
+            {
+                // Log the exception but don't throw, continue with the next value.
+                _logger.LogError(
+                    ex,
+                    "Error getting references from value {Value} with data editor {DataEditor} and property editor alias {PropertyEditorAlias}.",
+                    value,
+                    dataValueReference.GetType().FullName,
+                    propertyEditorAlias ?? "n/a");
+                throw;
+            }
+        }
+
+        return result;
+    }
+
     /// <summary>
     /// Gets all relation type aliases that are automatically tracked.
     /// </summary>
@@ -117,6 +169,7 @@ private IEnumerable<UmbracoEntityReference> GetReferencesEnumerable(IDataEditor
     [Obsolete("Use GetAllAutomaticRelationTypesAliases. This will be removed in Umbraco 15.")]
     public ISet<string> GetAutomaticRelationTypesAliases(PropertyEditorCollection propertyEditors) =>
         GetAllAutomaticRelationTypesAliases(propertyEditors);
+
     public ISet<string> GetAllAutomaticRelationTypesAliases(PropertyEditorCollection propertyEditors)
     {
         // Always add default automatic relation types
diff --git a/src/Umbraco.Infrastructure/PropertyEditors/BlockValuePropertyValueEditorBase.cs b/src/Umbraco.Infrastructure/PropertyEditors/BlockValuePropertyValueEditorBase.cs
@@ -99,17 +99,17 @@ protected IEnumerable<UmbracoEntityReference> GetBlockValueReferences(TValue blo
                 continue;
             }
 
-            var districtValues = valuesByPropertyEditorAlias.Distinct().ToArray();
+            var distinctValues = valuesByPropertyEditorAlias.Distinct().ToArray();
 
             if (dataEditor.GetValueEditor() is IDataValueReference reference)
             {
-                foreach (UmbracoEntityReference value in districtValues.SelectMany(reference.GetReferences))
+                foreach (UmbracoEntityReference value in distinctValues.SelectMany(reference.GetReferences))
                 {
                     result.Add(value);
                 }
             }
 
-            IEnumerable<UmbracoEntityReference> references = _dataValueReferenceFactoryCollection.GetReferences(dataEditor, districtValues);
+            IEnumerable<UmbracoEntityReference> references = _dataValueReferenceFactoryCollection.GetReferences(dataEditor, distinctValues);
 
             foreach (UmbracoEntityReference value in references)
             {
diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/DocumentRepositoryTest.cs b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/DocumentRepositoryTest.cs
@@ -4,6 +4,7 @@
 using System.Collections.Generic;
 using System.Linq;
 using Microsoft.Extensions.Logging;
+using Microsoft.Extensions.Logging.Abstractions;
 using Microsoft.Extensions.Options;
 using Moq;
 using NUnit.Framework;
@@ -129,7 +130,7 @@ private DocumentRepository CreateRepository(IScopeAccessor scopeAccessor, out Co
         var propertyEditors =
             new PropertyEditorCollection(new DataEditorCollection(() => Enumerable.Empty<IDataEditor>()));
         var dataValueReferences =
-            new DataValueReferenceFactoryCollection(() => Enumerable.Empty<IDataValueReferenceFactory>());
+            new DataValueReferenceFactoryCollection(() => Enumerable.Empty<IDataValueReferenceFactory>(), new NullLogger<DataValueReferenceFactoryCollection>());
         var repository = new DocumentRepository(
             scopeAccessor,
             appCaches,
diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/MediaRepositoryTest.cs b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/MediaRepositoryTest.cs
@@ -3,6 +3,7 @@
 
 using System.Linq;
 using Microsoft.Extensions.Logging;
+using Microsoft.Extensions.Logging.Abstractions;
 using Moq;
 using NUnit.Framework;
 using Umbraco.Cms.Core;
@@ -64,7 +65,7 @@ private MediaRepository CreateRepository(IScopeProvider provider, out MediaTypeR
             new PropertyEditorCollection(new DataEditorCollection(() => Enumerable.Empty<IDataEditor>()));
         var mediaUrlGenerators = new MediaUrlGeneratorCollection(() => Enumerable.Empty<IMediaUrlGenerator>());
         var dataValueReferences =
-            new DataValueReferenceFactoryCollection(() => Enumerable.Empty<IDataValueReferenceFactory>());
+            new DataValueReferenceFactoryCollection(() => Enumerable.Empty<IDataValueReferenceFactory>(), new NullLogger<DataValueReferenceFactoryCollection>());
         var repository = new MediaRepository(
             scopeAccessor,
             appCaches,
diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/MemberRepositoryTest.cs b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/MemberRepositoryTest.cs
@@ -4,6 +4,7 @@
 using System.Diagnostics;
 using System.Linq;
 using Microsoft.Extensions.Logging;
+using Microsoft.Extensions.Logging.Abstractions;
 using Microsoft.Extensions.Options;
 using Moq;
 using NPoco;
@@ -53,7 +54,7 @@ private MemberRepository CreateRepository(IScopeProvider provider)
         var propertyEditors =
             new PropertyEditorCollection(new DataEditorCollection(() => Enumerable.Empty<IDataEditor>()));
         var dataValueReferences =
-            new DataValueReferenceFactoryCollection(() => Enumerable.Empty<IDataValueReferenceFactory>());
+            new DataValueReferenceFactoryCollection(() => Enumerable.Empty<IDataValueReferenceFactory>(), new NullLogger<DataValueReferenceFactoryCollection>());
         return new MemberRepository(
             accessor,
             AppCaches.Disabled,
diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/TemplateRepositoryTest.cs b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/TemplateRepositoryTest.cs
@@ -6,6 +6,7 @@
 using System.Linq;
 using System.Text;
 using Microsoft.Extensions.Logging;
+using Microsoft.Extensions.Logging.Abstractions;
 using Microsoft.Extensions.Options;
 using Moq;
 using NUnit.Framework;
@@ -272,7 +273,7 @@ public void Can_Perform_Delete_When_Assigned_To_Doc()
             var propertyEditors =
                 new PropertyEditorCollection(new DataEditorCollection(() => Enumerable.Empty<IDataEditor>()));
             var dataValueReferences =
-                new DataValueReferenceFactoryCollection(() => Enumerable.Empty<IDataValueReferenceFactory>());
+                new DataValueReferenceFactoryCollection(() => Enumerable.Empty<IDataValueReferenceFactory>(), new NullLogger<DataValueReferenceFactoryCollection>());
             var contentRepo = new DocumentRepository(
                 scopeAccessor,
                 AppCaches.Disabled,
diff --git a/tests/Umbraco.Tests.UnitTests/Umbraco.Core/PropertyEditors/BlockListEditorPropertyValueEditorTests.cs b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/PropertyEditors/BlockListEditorPropertyValueEditorTests.cs
@@ -1,5 +1,6 @@
 using System.Globalization;
 using System.Text.Json.Nodes;
+using Microsoft.Extensions.Logging;
 using Microsoft.Extensions.Logging.Abstractions;
 using Moq;
 using NUnit.Framework;
@@ -148,7 +149,7 @@ private static BlockListEditorPropertyValueEditor CreateValueEditor()
             new DataEditorAttribute("alias"),
             new BlockListEditorDataConverter(jsonSerializer),
             new(new DataEditorCollection(() => [])),
-            new DataValueReferenceFactoryCollection(Enumerable.Empty<IDataValueReferenceFactory>),
+            new DataValueReferenceFactoryCollection(Enumerable.Empty<IDataValueReferenceFactory>, Mock.Of<ILogger<DataValueReferenceFactoryCollection>>()),
             Mock.Of<IDataTypeConfigurationCache>(),
             Mock.Of<IBlockEditorElementTypeCache>(),
             localizedTextServiceMock.Object,
diff --git a/tests/Umbraco.Tests.UnitTests/Umbraco.Core/PropertyEditors/DataValueEditorReuseTests.cs b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/PropertyEditors/DataValueEditorReuseTests.cs
@@ -1,4 +1,5 @@
-﻿using Microsoft.Extensions.Logging;
+using Microsoft.Extensions.Logging;
+using Microsoft.Extensions.Logging.Abstractions;
 using Moq;
 using NUnit.Framework;
 using Umbraco.Cms.Core.Cache;
@@ -35,7 +36,7 @@ public void SetUp()
                 Mock.Of<IIOHelper>()));
 
         _propertyEditorCollection = new PropertyEditorCollection(new DataEditorCollection(Enumerable.Empty<IDataEditor>));
-        _dataValueReferenceFactories = new DataValueReferenceFactoryCollection(Enumerable.Empty<IDataValueReferenceFactory>);
+        _dataValueReferenceFactories = new DataValueReferenceFactoryCollection(Enumerable.Empty<IDataValueReferenceFactory>, new NullLogger<DataValueReferenceFactoryCollection>());
 
         var blockVarianceHandler = new BlockEditorVarianceHandler(Mock.Of<ILanguageService>(), Mock.Of<IContentTypeService>());
         _dataValueEditorFactoryMock
diff --git a/tests/Umbraco.Tests.UnitTests/Umbraco.Core/PropertyEditors/DataValueReferenceFactoryCollectionTests.cs b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/PropertyEditors/DataValueReferenceFactoryCollectionTests.cs
@@ -1,6 +1,7 @@
 // Copyright (c) Umbraco.
 // See LICENSE for more details.
 
+using Microsoft.Extensions.Logging.Abstractions;
 using Moq;
 using NUnit.Framework;
 using Umbraco.Cms.Core;
@@ -49,7 +50,7 @@ public class DataValueReferenceFactoryCollectionTests
     [Test]
     public void GetAllReferences_All_Variants_With_IDataValueReferenceFactory()
     {
-        var collection = new DataValueReferenceFactoryCollection(() => new TestDataValueReferenceFactory().Yield());
+        var collection = new DataValueReferenceFactoryCollection(() => new TestDataValueReferenceFactory().Yield(), new NullLogger<DataValueReferenceFactoryCollection>());
 
         // label does not implement IDataValueReference
         var labelEditor = new LabelPropertyEditor(
@@ -93,7 +94,7 @@ public void GetAllReferences_All_Variants_With_IDataValueReferenceFactory()
     [Test]
     public void GetAllReferences_All_Variants_With_IDataValueReference_Editor()
     {
-        var collection = new DataValueReferenceFactoryCollection(() => Enumerable.Empty<IDataValueReferenceFactory>());
+        var collection = new DataValueReferenceFactoryCollection(() => Enumerable.Empty<IDataValueReferenceFactory>(), new NullLogger<DataValueReferenceFactoryCollection>());
 
         // mediaPicker does implement IDataValueReference
         var mediaPicker = new MediaPicker3PropertyEditor(
@@ -137,7 +138,7 @@ public void GetAllReferences_All_Variants_With_IDataValueReference_Editor()
     [Test]
     public void GetAllReferences_Invariant_With_IDataValueReference_Editor()
     {
-        var collection = new DataValueReferenceFactoryCollection(() => Enumerable.Empty<IDataValueReferenceFactory>());
+        var collection = new DataValueReferenceFactoryCollection(() => Enumerable.Empty<IDataValueReferenceFactory>(), new NullLogger<DataValueReferenceFactoryCollection>());
 
         // mediaPicker does implement IDataValueReference
         var mediaPicker = new MediaPicker3PropertyEditor(
@@ -181,7 +182,7 @@ public void GetAllReferences_Invariant_With_IDataValueReference_Editor()
     [Test]
     public void GetAutomaticRelationTypesAliases_ContainsDefault()
     {
-        var collection = new DataValueReferenceFactoryCollection(Enumerable.Empty<IDataValueReferenceFactory>);
+        var collection = new DataValueReferenceFactoryCollection(Enumerable.Empty<IDataValueReferenceFactory>, new NullLogger<DataValueReferenceFactoryCollection>());
         var propertyEditors = new PropertyEditorCollection(new DataEditorCollection(Enumerable.Empty<IDataEditor>));
 
         var result = collection.GetAllAutomaticRelationTypesAliases(propertyEditors).ToArray();
@@ -193,7 +194,7 @@ public void GetAutomaticRelationTypesAliases_ContainsDefault()
     [Test]
     public void GetAutomaticRelationTypesAliases_ContainsCustom()
     {
-        var collection = new DataValueReferenceFactoryCollection(() => new TestDataValueReferenceFactory().Yield());
+        var collection = new DataValueReferenceFactoryCollection(() => new TestDataValueReferenceFactory().Yield(), new NullLogger<DataValueReferenceFactoryCollection>());
 
         var labelPropertyEditor = new LabelPropertyEditor(DataValueEditorFactory, IOHelper);
         var propertyEditors = new PropertyEditorCollection(new DataEditorCollection(() => labelPropertyEditor.Yield()));

Original file line number	Diff line number	Diff line change
`@@ -99,17 +99,17 @@ protected IEnumerable<UmbracoEntityReference> GetBlockValueReferences(TValue blo`
`99`	`99`	`continue;`
`100`	`100`	`}`
`101`	`101`
`102`		`- var districtValues = valuesByPropertyEditorAlias.Distinct().ToArray();`
	`102`	`+ var distinctValues = valuesByPropertyEditorAlias.Distinct().ToArray();`
`103`	`103`
`104`	`104`	`if (dataEditor.GetValueEditor() is IDataValueReference reference)`
`105`	`105`	`{`
`106`		`- foreach (UmbracoEntityReference value in districtValues.SelectMany(reference.GetReferences))`
	`106`	`+ foreach (UmbracoEntityReference value in distinctValues.SelectMany(reference.GetReferences))`
`107`	`107`	`{`
`108`	`108`	`result.Add(value);`
`109`	`109`	`}`
`110`	`110`	`}`
`111`	`111`
`112`		`- IEnumerable<UmbracoEntityReference> references = _dataValueReferenceFactoryCollection.GetReferences(dataEditor, districtValues);`
	`112`	`+ IEnumerable<UmbracoEntityReference> references = _dataValueReferenceFactoryCollection.GetReferences(dataEditor, distinctValues);`
`113`	`113`
`114`	`114`	`foreach (UmbracoEntityReference value in references)`
`115`	`115`	`{`