From 260dfced43592a4a1ae6399f95329282334f170f Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Wed, 5 Mar 2025 10:25:29 +0100 Subject: [PATCH 1/4] Added logging and try/catch around retrieval of references, so we don't block critical operations following an incompatible data type change. --- .../DataValueReferenceFactoryCollection.cs | 50 +++++++++++++++++-- .../Repositories/DocumentRepositoryTest.cs | 3 +- .../Repositories/MediaRepositoryTest.cs | 3 +- .../Repositories/MemberRepositoryTest.cs | 3 +- .../Repositories/TemplateRepositoryTest.cs | 3 +- .../DataValueEditorReuseTests.cs | 5 +- ...ataValueReferenceFactoryCollectionTests.cs | 11 ++-- 7 files changed, 64 insertions(+), 14 deletions(-) diff --git a/src/Umbraco.Core/PropertyEditors/DataValueReferenceFactoryCollection.cs b/src/Umbraco.Core/PropertyEditors/DataValueReferenceFactoryCollection.cs index 86c9c48fc0a7..19159355b43a 100644 --- 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 _logger; + /// /// Initializes a new instance of the class. /// /// The items. + [Obsolete("Please use the constructor taking all parameters. Scheduled for removal in Umbraco 17.")] public DataValueReferenceFactoryCollection(Func> items) - : base(items) + : this( + items, + StaticServiceProvider.Instance.GetRequiredService>()) { } + /// + /// Initializes a new instance of the class. + /// + /// The items. + /// The logger. + public DataValueReferenceFactoryCollection(Func> items, ILogger logger) + : base(items) => _logger = logger; + /// /// Gets all unique references from the specified properties. /// @@ -75,13 +91,14 @@ public IEnumerable GetReferences(IDataEditor dataEditor, /// public ISet GetReferences(IDataEditor dataEditor, IEnumerable values) => GetReferencesEnumerable(dataEditor, values).ToHashSet(); + private IEnumerable GetReferencesEnumerable(IDataEditor dataEditor, IEnumerable values) { // 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 GetReferences(values, dataValueReference)) { yield return reference; } @@ -91,7 +108,7 @@ private IEnumerable GetReferencesEnumerable(IDataEditor // implementation of GetReferences in IDataValueReference. // Allows developers to add support for references by a // package /property editor that did not implement IDataValueReference themselves - foreach (IDataValueReferenceFactory dataValueReferenceFactory in this) + foreach (IDataValueReferenceFactory dataValueReferenceFactory in this) { // Check if this value reference is for this datatype/editor // Then call it's GetReferences method - to see if the value stored @@ -107,6 +124,32 @@ private IEnumerable GetReferencesEnumerable(IDataEditor } } + private IEnumerable GetReferences(IEnumerable values, IDataValueReference dataValueReference) + { + var result = new List(); + 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 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 for value {Value} with data editor {DataEditor}.", value, dataValueReference.GetType().FullName); + } + } + + return result; + } + /// /// Gets all relation type aliases that are automatically tracked. /// @@ -117,6 +160,7 @@ private IEnumerable GetReferencesEnumerable(IDataEditor [Obsolete("Use GetAllAutomaticRelationTypesAliases. This will be removed in Umbraco 15.")] public ISet GetAutomaticRelationTypesAliases(PropertyEditorCollection propertyEditors) => GetAllAutomaticRelationTypesAliases(propertyEditors); + public ISet GetAllAutomaticRelationTypesAliases(PropertyEditorCollection propertyEditors) { // Always add default automatic relation types diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/DocumentRepositoryTest.cs b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/Repositories/DocumentRepositoryTest.cs index e6ce8d467ece..37ae7fb0710c 100644 --- 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())); var dataValueReferences = - new DataValueReferenceFactoryCollection(() => Enumerable.Empty()); + new DataValueReferenceFactoryCollection(() => Enumerable.Empty(), new NullLogger()); 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 index 932be80a0176..c01d84de7e45 100644 --- 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())); var mediaUrlGenerators = new MediaUrlGeneratorCollection(() => Enumerable.Empty()); var dataValueReferences = - new DataValueReferenceFactoryCollection(() => Enumerable.Empty()); + new DataValueReferenceFactoryCollection(() => Enumerable.Empty(), new NullLogger()); 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 index 58dc4bb4b038..a9350877019b 100644 --- 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())); var dataValueReferences = - new DataValueReferenceFactoryCollection(() => Enumerable.Empty()); + new DataValueReferenceFactoryCollection(() => Enumerable.Empty(), new NullLogger()); 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 index 8e4a68af709a..02e5bac7c57b 100644 --- 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())); var dataValueReferences = - new DataValueReferenceFactoryCollection(() => Enumerable.Empty()); + new DataValueReferenceFactoryCollection(() => Enumerable.Empty(), new NullLogger()); var contentRepo = new DocumentRepository( scopeAccessor, AppCaches.Disabled, diff --git a/tests/Umbraco.Tests.UnitTests/Umbraco.Core/PropertyEditors/DataValueEditorReuseTests.cs b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/PropertyEditors/DataValueEditorReuseTests.cs index 217b820c78aa..2919783865ea 100644 --- 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; @@ -34,7 +35,7 @@ public void SetUp() Mock.Of())); _propertyEditorCollection = new PropertyEditorCollection(new DataEditorCollection(Enumerable.Empty)); - _dataValueReferenceFactories = new DataValueReferenceFactoryCollection(Enumerable.Empty); + _dataValueReferenceFactories = new DataValueReferenceFactoryCollection(Enumerable.Empty, new NullLogger()); var blockVarianceHandler = new BlockEditorVarianceHandler(Mock.Of(), Mock.Of()); _dataValueEditorFactoryMock diff --git a/tests/Umbraco.Tests.UnitTests/Umbraco.Core/PropertyEditors/DataValueReferenceFactoryCollectionTests.cs b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/PropertyEditors/DataValueReferenceFactoryCollectionTests.cs index 33f4f307f8bf..43757f064c76 100644 --- 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()); // 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()); + var collection = new DataValueReferenceFactoryCollection(() => Enumerable.Empty(), new NullLogger()); // 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()); + var collection = new DataValueReferenceFactoryCollection(() => Enumerable.Empty(), new NullLogger()); // 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); + var collection = new DataValueReferenceFactoryCollection(Enumerable.Empty, new NullLogger()); var propertyEditors = new PropertyEditorCollection(new DataEditorCollection(Enumerable.Empty)); 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()); var labelPropertyEditor = new LabelPropertyEditor(DataValueEditorFactory, IOHelper); var propertyEditors = new PropertyEditorCollection(new DataEditorCollection(() => labelPropertyEditor.Yield())); From d42eb2cce5f4fe5c7d463021141a1de84a71efdc Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Thu, 6 Mar 2025 11:56:54 +0100 Subject: [PATCH 2/4] Added a little more detail to the log message. --- .../PropertyEditors/BlockValuePropertyValueEditorBase.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Umbraco.Infrastructure/PropertyEditors/BlockValuePropertyValueEditorBase.cs b/src/Umbraco.Infrastructure/PropertyEditors/BlockValuePropertyValueEditorBase.cs index d66e2b2caa32..24aa4cc3a77a 100644 --- a/src/Umbraco.Infrastructure/PropertyEditors/BlockValuePropertyValueEditorBase.cs +++ b/src/Umbraco.Infrastructure/PropertyEditors/BlockValuePropertyValueEditorBase.cs @@ -98,17 +98,17 @@ protected IEnumerable 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 references = _dataValueReferenceFactoryCollection.GetReferences(dataEditor, districtValues); + IEnumerable references = _dataValueReferenceFactoryCollection.GetReferences(dataEditor, distinctValues); foreach (UmbracoEntityReference value in references) { From 5c856a1e1ec726ea092131d0cc0edd4ddd8eca5f Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Thu, 6 Mar 2025 12:29:31 +0100 Subject: [PATCH 3/4] Added a little more detail to the log message. --- .../DataValueReferenceFactoryCollection.cs | 23 +++++++++++++------ 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/src/Umbraco.Core/PropertyEditors/DataValueReferenceFactoryCollection.cs b/src/Umbraco.Core/PropertyEditors/DataValueReferenceFactoryCollection.cs index 19159355b43a..5d137d04c60f 100644 --- a/src/Umbraco.Core/PropertyEditors/DataValueReferenceFactoryCollection.cs +++ b/src/Umbraco.Core/PropertyEditors/DataValueReferenceFactoryCollection.cs @@ -49,7 +49,7 @@ public ISet GetAllReferences(IPropertyCollection propert var references = new HashSet(); // 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> propertyValuesByPropertyEditorAlias in properties.GroupBy(x => x.PropertyType.PropertyEditorAlias, x => x.Values)) { if (!propertyEditors.TryGet(propertyValuesByPropertyEditorAlias.Key, out IDataEditor? dataEditor)) { @@ -64,7 +64,7 @@ public ISet GetAllReferences(IPropertyCollection propert values.Add(propertyValue.PublishedValue); } - references.UnionWith(GetReferences(dataEditor, values)); + references.UnionWith(GetReferences(dataEditor, values, propertyValuesByPropertyEditorAlias.Key)); } return references; @@ -90,15 +90,18 @@ public IEnumerable GetReferences(IDataEditor dataEditor, /// The references. /// public ISet GetReferences(IDataEditor dataEditor, IEnumerable values) => - GetReferencesEnumerable(dataEditor, values).ToHashSet(); + GetReferencesEnumerable(dataEditor, values, null).ToHashSet(); - private IEnumerable GetReferencesEnumerable(IDataEditor dataEditor, IEnumerable values) + private ISet GetReferences(IDataEditor dataEditor, IEnumerable values, string propertyEditorAlias) => + GetReferencesEnumerable(dataEditor, values, propertyEditorAlias).ToHashSet(); + + private IEnumerable GetReferencesEnumerable(IDataEditor dataEditor, IEnumerable 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 GetReferences(values, dataValueReference)) + foreach (UmbracoEntityReference reference in GetReferencesFromPropertyValues(values, dataValueReference, propertyEditorAlias)) { yield return reference; } @@ -124,7 +127,7 @@ private IEnumerable GetReferencesEnumerable(IDataEditor } } - private IEnumerable GetReferences(IEnumerable values, IDataValueReference dataValueReference) + private IEnumerable GetReferencesFromPropertyValues(IEnumerable values, IDataValueReference dataValueReference, string? propertyEditorAlias) { var result = new List(); foreach (var value in values) @@ -143,7 +146,13 @@ private IEnumerable GetReferences(IEnumerable v catch (Exception ex) { // Log the exception but don't throw, continue with the next value. - _logger.LogError(ex, "Error getting references for value {Value} with data editor {DataEditor}.", value, dataValueReference.GetType().FullName); + _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; } } From eed95a5eb26b5a7407e00ee85f5cbaeb67d787b8 Mon Sep 17 00:00:00 2001 From: Migaroez Date: Mon, 19 May 2025 10:03:56 +0200 Subject: [PATCH 4/4] Fix unittest mock dependency --- .../PropertyEditors/BlockListEditorPropertyValueEditorTests.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/Umbraco.Tests.UnitTests/Umbraco.Core/PropertyEditors/BlockListEditorPropertyValueEditorTests.cs b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/PropertyEditors/BlockListEditorPropertyValueEditorTests.cs index 879400f79aab..fc5653520002 100644 --- 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), + new DataValueReferenceFactoryCollection(Enumerable.Empty, Mock.Of>()), Mock.Of(), Mock.Of(), localizedTextServiceMock.Object,