diff --git a/src/Umbraco.Infrastructure/Migrations/PropertyDataCultureResolver.cs b/src/Umbraco.Infrastructure/Migrations/PropertyDataCultureResolver.cs new file mode 100644 index 000000000000..578c2a6c71ac --- /dev/null +++ b/src/Umbraco.Infrastructure/Migrations/PropertyDataCultureResolver.cs @@ -0,0 +1,117 @@ +using Umbraco.Cms.Core.Models; +using Umbraco.Extensions; + +namespace Umbraco.Cms.Infrastructure.Migrations; + +/// +/// Resolves the culture ISO code for a property data row during migrations, +/// handling the case where the property type varies by culture but the referenced +/// language no longer exists. +/// +internal static class PropertyDataCultureResolver +{ + /// + /// Log message template used when a property data row references a language that no longer exists. + /// + internal const string OrphanedLanguageWarningTemplate = + " - property data with id: {propertyDataId} references a language that does not exist - language id: {languageId} (property type: {propertyTypeName}, id: {propertyTypeId}, alias: {propertyTypeAlias})"; + + /// + /// Represents the result of resolving a culture for a property data row. + /// + internal readonly record struct CultureResolutionResult + { + /// + /// The resolved culture ISO code, or null if invariant. + /// + public string? Culture { get; init; } + + /// + /// True if the row should be skipped because it references a deleted language. + /// + public bool ShouldSkip { get; init; } + + /// + /// The language ID that was not found (only set when is true). + /// + public int? OrphanedLanguageId { get; init; } + } + + /// + /// Resolves the culture for a property data row, detecting orphaned language references. + /// + /// The property type (may vary by culture via composition). + /// The language ID from the property data row (null for invariant data). + /// Lookup of all known languages by ID. + /// + /// A result indicating the resolved culture and whether the row should be skipped. + /// When is null and the property type varies by culture, + /// this is legitimate invariant data (e.g. from a content type using a culture-varying + /// composition) and should NOT be skipped. + /// + internal static CultureResolutionResult ResolveCulture( + IPropertyType propertyType, + int? languageId, + IDictionary languagesById) + { + // NOTE: old property data rows may still have languageId populated even if the property type no longer varies + string? culture = propertyType.VariesByCulture() + && languageId.HasValue + && languagesById.TryGetValue(languageId.Value, out ILanguage? language) + ? language!.IsoCode + : null; + + // If culture is null, the property type varies by culture, AND the DTO has a non-null + // language ID, then the language has been deleted. This is an error scenario we can only + // log; in all likelihood this is an old property version and won't cause runtime issues. + // + // If languageId is NULL, this is legitimate invariant data on a content type that uses a + // culture-varying composition — we must NOT skip it. + if (culture is null && propertyType.VariesByCulture() && languageId.HasValue) + { + return new CultureResolutionResult + { + Culture = null, + ShouldSkip = true, + OrphanedLanguageId = languageId.Value, + }; + } + + return new CultureResolutionResult + { + Culture = culture, + ShouldSkip = false, + }; + } + + /// + /// Creates a suitable for migration value roundtripping. + /// + /// + /// When a property type varies by culture (e.g. inherited from a composition) but the data + /// is invariant (null culture), rejects the null culture. + /// This method handles that case by deep-cloning the property type and setting its + /// to , + /// which is safe because the data genuinely is invariant. + /// + internal static Property CreateMigrationProperty( + IPropertyType propertyType, + object? value, + string? culture, + string? segment) + { + IPropertyType effectivePropertyType = propertyType; + + if (culture is null && propertyType.VariesByCulture()) + { + // Invariant data on a culture-varying property type (composition scenario). + // Clone to avoid mutating shared state — important for parallel migration execution. + effectivePropertyType = (IPropertyType)propertyType.DeepClone(); + effectivePropertyType.Variations = ContentVariation.Nothing; + } + + var property = new Property(effectivePropertyType); + property.SetValue(value, culture, segment); + return property; + } +} diff --git a/src/Umbraco.Infrastructure/Migrations/Upgrade/V_15_0_0/ConvertBlockEditorPropertiesBase.cs b/src/Umbraco.Infrastructure/Migrations/Upgrade/V_15_0_0/ConvertBlockEditorPropertiesBase.cs index 4cb6fb1fca4a..6d3356b87b7a 100644 --- a/src/Umbraco.Infrastructure/Migrations/Upgrade/V_15_0_0/ConvertBlockEditorPropertiesBase.cs +++ b/src/Umbraco.Infrastructure/Migrations/Upgrade/V_15_0_0/ConvertBlockEditorPropertiesBase.cs @@ -184,33 +184,23 @@ void HandleUpdateBatch(UpdateBatch update) PropertyDataDto propertyDataDto = update.Poco; - // NOTE: some old property data DTOs can have variance defined, even if the property type no longer varies - var culture = propertyType.VariesByCulture() - && propertyDataDto.LanguageId.HasValue - && languagesById.TryGetValue( - propertyDataDto.LanguageId.Value, - out ILanguage? language) - ? language.IsoCode - : null; - - if (culture is null && propertyType.VariesByCulture()) + var cultureResult = PropertyDataCultureResolver.ResolveCulture(propertyType, propertyDataDto.LanguageId, languagesById); + if (cultureResult.ShouldSkip) { - // if we end up here, the property DTO is bound to a language that no longer exists. this is an error scenario, - // and we can't really handle it in any other way than logging; in all likelihood this is an old property version, - // and it won't cause any runtime issues _logger.LogWarning( - " - property data with id: {propertyDataId} references a language that does not exist - language id: {languageId} (property type: {propertyTypeName}, id: {propertyTypeId}, alias: {propertyTypeAlias})", + PropertyDataCultureResolver.OrphanedLanguageWarningTemplate, propertyDataDto.Id, - propertyDataDto.LanguageId, + cultureResult.OrphanedLanguageId, propertyType.Name, propertyType.Id, propertyType.Alias); return; } + var culture = cultureResult.Culture; + var segment = propertyType.VariesBySegment() ? propertyDataDto.Segment : null; - var property = new Property(propertyType); - property.SetValue(propertyDataDto.Value, culture, segment); + var property = PropertyDataCultureResolver.CreateMigrationProperty(propertyType, propertyDataDto.Value, culture, segment); var toEditorValue = valueEditor.ToEditor(property, culture, segment); switch (toEditorValue) { diff --git a/src/Umbraco.Infrastructure/Migrations/Upgrade/V_15_0_0/ConvertLocalLinks.cs b/src/Umbraco.Infrastructure/Migrations/Upgrade/V_15_0_0/ConvertLocalLinks.cs index e669ddc76458..4dce6fc10d30 100644 --- a/src/Umbraco.Infrastructure/Migrations/Upgrade/V_15_0_0/ConvertLocalLinks.cs +++ b/src/Umbraco.Infrastructure/Migrations/Upgrade/V_15_0_0/ConvertLocalLinks.cs @@ -332,31 +332,23 @@ private bool ProcessPropertyDataDto( IDictionary languagesById, IDataValueEditor valueEditor) { - // NOTE: some old property data DTOs can have variance defined, even if the property type no longer varies - var culture = propertyType.VariesByCulture() - && propertyDataDto.LanguageId.HasValue - && languagesById.TryGetValue(propertyDataDto.LanguageId.Value, out ILanguage? language) - ? language.IsoCode - : null; - - if (culture is null && propertyType.VariesByCulture()) + var cultureResult = PropertyDataCultureResolver.ResolveCulture(propertyType, propertyDataDto.LanguageId, languagesById); + if (cultureResult.ShouldSkip) { - // if we end up here, the property DTO is bound to a language that no longer exists. this is an error scenario, - // and we can't really handle it in any other way than logging; in all likelihood this is an old property version, - // and it won't cause any runtime issues _logger.LogWarning( - " - property data with id: {propertyDataId} references a language that does not exist - language id: {languageId} (property type: {propertyTypeName}, id: {propertyTypeId}, alias: {propertyTypeAlias})", + PropertyDataCultureResolver.OrphanedLanguageWarningTemplate, propertyDataDto.Id, - propertyDataDto.LanguageId, + cultureResult.OrphanedLanguageId, propertyType.Name, propertyType.Id, propertyType.Alias); return false; } + var culture = cultureResult.Culture; + var segment = propertyType.VariesBySegment() ? propertyDataDto.Segment : null; - var property = new Property(propertyType); - property.SetValue(propertyDataDto.Value, culture, segment); + var property = PropertyDataCultureResolver.CreateMigrationProperty(propertyType, propertyDataDto.Value, culture, segment); var toEditorValue = valueEditor.ToEditor(property, culture, segment); if (_localLinkProcessor.ProcessToEditorValue(toEditorValue) == false) diff --git a/src/Umbraco.Infrastructure/Migrations/Upgrade/V_15_1_0/FixConvertLocalLinks.cs b/src/Umbraco.Infrastructure/Migrations/Upgrade/V_15_1_0/FixConvertLocalLinks.cs index 9d5ff615922b..136089796057 100644 --- a/src/Umbraco.Infrastructure/Migrations/Upgrade/V_15_1_0/FixConvertLocalLinks.cs +++ b/src/Umbraco.Infrastructure/Migrations/Upgrade/V_15_1_0/FixConvertLocalLinks.cs @@ -252,31 +252,23 @@ private bool ProcessPropertyDataDto( IDictionary languagesById, IDataValueEditor valueEditor) { - // NOTE: some old property data DTOs can have variance defined, even if the property type no longer varies - var culture = propertyType.VariesByCulture() - && propertyDataDto.LanguageId.HasValue - && languagesById.TryGetValue(propertyDataDto.LanguageId.Value, out ILanguage? language) - ? language.IsoCode - : null; - - if (culture is null && propertyType.VariesByCulture()) + var cultureResult = PropertyDataCultureResolver.ResolveCulture(propertyType, propertyDataDto.LanguageId, languagesById); + if (cultureResult.ShouldSkip) { - // if we end up here, the property DTO is bound to a language that no longer exists. this is an error scenario, - // and we can't really handle it in any other way than logging; in all likelihood this is an old property version, - // and it won't cause any runtime issues _logger.LogWarning( - " - property data with id: {propertyDataId} references a language that does not exist - language id: {languageId} (property type: {propertyTypeName}, id: {propertyTypeId}, alias: {propertyTypeAlias})", + PropertyDataCultureResolver.OrphanedLanguageWarningTemplate, propertyDataDto.Id, - propertyDataDto.LanguageId, + cultureResult.OrphanedLanguageId, propertyType.Name, propertyType.Id, propertyType.Alias); return false; } + var culture = cultureResult.Culture; + var segment = propertyType.VariesBySegment() ? propertyDataDto.Segment : null; - var property = new Property(propertyType); - property.SetValue(propertyDataDto.Value, culture, segment); + var property = PropertyDataCultureResolver.CreateMigrationProperty(propertyType, propertyDataDto.Value, culture, segment); var toEditorValue = valueEditor.ToEditor(property, culture, segment); if (_localLinkProcessor.ProcessToEditorValue(toEditorValue) == false) diff --git a/src/Umbraco.Infrastructure/Migrations/Upgrade/V_18_0_0/MigrateSingleBlockList.cs b/src/Umbraco.Infrastructure/Migrations/Upgrade/V_18_0_0/MigrateSingleBlockList.cs index c50b03a1f964..74a05fa8c72e 100644 --- a/src/Umbraco.Infrastructure/Migrations/Upgrade/V_18_0_0/MigrateSingleBlockList.cs +++ b/src/Umbraco.Infrastructure/Migrations/Upgrade/V_18_0_0/MigrateSingleBlockList.cs @@ -350,22 +350,13 @@ private bool ProcessPropertyDataDto( IDataValueEditor valueEditor, out UpdateItem? updateItem) { - // NOTE: some old property data DTOs can have variance defined, even if the property type no longer varies - var culture = propertyType.VariesByCulture() - && propertyDataDto.LanguageId.HasValue - && languagesById.TryGetValue(propertyDataDto.LanguageId.Value, out ILanguage? language) - ? language.IsoCode - : null; - - if (culture is null && propertyType.VariesByCulture()) + var cultureResult = PropertyDataCultureResolver.ResolveCulture(propertyType, propertyDataDto.LanguageId, languagesById); + if (cultureResult.ShouldSkip) { - // if we end up here, the property DTO is bound to a language that no longer exists. this is an error scenario, - // and we can't really handle it in any other way than logging; in all likelihood this is an old property version, - // and it won't cause any runtime issues _logger.LogWarning( - " - property data with id: {propertyDataId} references a language that does not exist - language id: {languageId} (property type: {propertyTypeName}, id: {propertyTypeId}, alias: {propertyTypeAlias})", + PropertyDataCultureResolver.OrphanedLanguageWarningTemplate, propertyDataDto.Id, - propertyDataDto.LanguageId, + cultureResult.OrphanedLanguageId, propertyType.Name, propertyType.Id, propertyType.Alias); @@ -373,10 +364,11 @@ private bool ProcessPropertyDataDto( return false; } - // create a fake property to be able to get a typed value and run it trough the processors. + var culture = cultureResult.Culture; + + // create a fake property to be able to get a typed value and run it through the processors. var segment = propertyType.VariesBySegment() ? propertyDataDto.Segment : null; - var property = new Property(propertyType); - property.SetValue(propertyDataDto.Value, culture, segment); + var property = PropertyDataCultureResolver.CreateMigrationProperty(propertyType, propertyDataDto.Value, culture, segment); var toEditorValue = valueEditor.ToEditor(property, culture, segment); if (TryTransformValue(toEditorValue, property, out var updatedValue) is false) diff --git a/tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Migrations/PropertyDataCultureResolverTests.cs b/tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Migrations/PropertyDataCultureResolverTests.cs new file mode 100644 index 000000000000..9ef60ae27a2b --- /dev/null +++ b/tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Migrations/PropertyDataCultureResolverTests.cs @@ -0,0 +1,170 @@ +// Copyright (c) Umbraco. +// See LICENSE for more details. + +using Moq; +using NUnit.Framework; +using Umbraco.Cms.Core.Models; +using Umbraco.Cms.Core.Strings; +using Umbraco.Cms.Infrastructure.Migrations; + +namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Infrastructure.Migrations; + +[TestFixture] +internal sealed class PropertyDataCultureResolverTests +{ + [Test] + public void InvariantPropertyType_NullLanguageId_ReturnsNullCulture() + { + var propertyType = CreatePropertyType(ContentVariation.Nothing); + var languages = new Dictionary(); + + var result = PropertyDataCultureResolver.ResolveCulture(propertyType, languageId: null, languages); + + Assert.That(result.Culture, Is.Null); + Assert.That(result.ShouldSkip, Is.False); + } + + [Test] + public void InvariantPropertyType_WithLanguageId_ReturnsNullCulture_DoesNotSkip() + { + var propertyType = CreatePropertyType(ContentVariation.Nothing); + var languages = new Dictionary(); + + var result = PropertyDataCultureResolver.ResolveCulture(propertyType, languageId: 5, languages); + + Assert.That(result.Culture, Is.Null); + Assert.That(result.ShouldSkip, Is.False); + } + + [Test] + public void CultureVaryingPropertyType_NullLanguageId_ReturnsNullCulture_DoesNotSkip() + { + // This is the bug scenario: an invariant content type uses a composition + // with a culture-varying property. The property data has languageId = NULL + // (correct for invariant content). The migration must NOT skip this row. + var propertyType = CreatePropertyType(ContentVariation.Culture); + var languages = new Dictionary { { 1, CreateLanguage("en-US") } }; + + var result = PropertyDataCultureResolver.ResolveCulture(propertyType, languageId: null, languages); + + Assert.That(result.Culture, Is.Null); + Assert.That(result.ShouldSkip, Is.False); + } + + [Test] + public void CultureVaryingPropertyType_ValidLanguageId_ReturnsCulture() + { + var propertyType = CreatePropertyType(ContentVariation.Culture); + var languages = new Dictionary { { 1, CreateLanguage("en-US") } }; + + var result = PropertyDataCultureResolver.ResolveCulture(propertyType, languageId: 1, languages); + + Assert.That(result.Culture, Is.EqualTo("en-US")); + Assert.That(result.ShouldSkip, Is.False); + } + + [Test] + public void CultureVaryingPropertyType_OrphanedLanguageId_ShouldSkip() + { + // The language was deleted — languageId is non-null but not in the lookup. + var propertyType = CreatePropertyType(ContentVariation.Culture); + var languages = new Dictionary { { 1, CreateLanguage("en-US") } }; + + var result = PropertyDataCultureResolver.ResolveCulture(propertyType, languageId: 99, languages); + + Assert.That(result.Culture, Is.Null); + Assert.That(result.ShouldSkip, Is.True); + Assert.That(result.OrphanedLanguageId, Is.EqualTo(99)); + } + + [Test] + public void CultureVaryingPropertyType_EmptyDictionary_WithLanguageId_ShouldSkip() + { + var propertyType = CreatePropertyType(ContentVariation.Culture); + var languages = new Dictionary(); + + var result = PropertyDataCultureResolver.ResolveCulture(propertyType, languageId: 1, languages); + + Assert.That(result.Culture, Is.Null); + Assert.That(result.ShouldSkip, Is.True); + Assert.That(result.OrphanedLanguageId, Is.EqualTo(1)); + } + + [Test] + public void CultureAndSegmentVaryingPropertyType_NullLanguageId_ReturnsNullCulture_DoesNotSkip() + { + // CultureAndSegment also has VariesByCulture() == true, so the same + // composition scenario applies. + var propertyType = CreatePropertyType(ContentVariation.CultureAndSegment); + var languages = new Dictionary { { 1, CreateLanguage("en-US") } }; + + var result = PropertyDataCultureResolver.ResolveCulture(propertyType, languageId: null, languages); + + Assert.That(result.Culture, Is.Null); + Assert.That(result.ShouldSkip, Is.False); + } + + [Test] + public void CreateMigrationProperty_CultureVaryingPropertyType_NullCulture_DoesNotThrow() + { + // Without the fix, this throws NotSupportedException: + // "Variation , is not supported by the property type." + var propertyType = CreateConcretePropertyType(ContentVariation.Culture); + + var property = PropertyDataCultureResolver.CreateMigrationProperty( + propertyType, "test value", culture: null, segment: null); + + Assert.That(property, Is.Not.Null); + Assert.That(property.GetValue(null, null), Is.EqualTo("test value")); + } + + [Test] + public void CreateMigrationProperty_CultureVaryingPropertyType_WithCulture_Works() + { + var propertyType = CreateConcretePropertyType(ContentVariation.Culture); + + var property = PropertyDataCultureResolver.CreateMigrationProperty( + propertyType, "test value", culture: "en-US", segment: null); + + Assert.That(property, Is.Not.Null); + Assert.That(property.GetValue("en-US", null), Is.EqualTo("test value")); + } + + [Test] + public void CreateMigrationProperty_DoesNotMutateOriginalPropertyType() + { + var propertyType = CreateConcretePropertyType(ContentVariation.Culture); + + PropertyDataCultureResolver.CreateMigrationProperty( + propertyType, "test value", culture: null, segment: null); + + Assert.That( + propertyType.Variations, + Is.EqualTo(ContentVariation.Culture), + "Original property type Variations must not be modified"); + } + + private static IPropertyType CreatePropertyType(ContentVariation variation) + { + var mock = new Mock(); + mock.Setup(pt => pt.Variations).Returns(variation); + return mock.Object; + } + + private static PropertyType CreateConcretePropertyType(ContentVariation variation) + { + var shortStringHelper = new DefaultShortStringHelper(new DefaultShortStringHelperConfig()); + var propertyType = new PropertyType(shortStringHelper, "Umbraco.TextBox", ValueStorageType.Nvarchar, "testAlias") + { + Variations = variation, + }; + return propertyType; + } + + private static ILanguage CreateLanguage(string isoCode) + { + var mock = new Mock(); + mock.Setup(l => l.IsoCode).Returns(isoCode); + return mock.Object; + } +}