Skip to content

Commit 51ae5b6

Browse files
AndyButlandMigaroez
AndyButland
and
Migaroez
committed
Migrations: Fix property detection for invariant content types with culture-varying compositions (closes #22159) (#22167)
* Extract shared culture-resolution logic from ConvertBlockEditorPropertiesBase, ConvertLocalLinks, FixConvertLocalLinks, and MigrateSingleBlockList into PropertyDataCultureResolver, fixing a bug where NULL languageId (legitimate invariant data) was incorrectly treated as a deleted language reference. Add unit tests covering all resolution paths including the bug scenario. * Remove obsoletion on helper. * Address code review feedback. * Handle SetValue variation mismatch for invariant data on culture-varying compositions * Fixed build error in tests. --------- Co-authored-by: Sven Geusens <sge@umbraco.dk>
1 parent 49b3c24 commit 51ae5b6

6 files changed

Lines changed: 316 additions & 63 deletions

File tree

‎src/Umbraco.Infrastructure/Migrations/PropertyDataCultureResolver.cs‎

Lines changed: 117 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,117 @@
1+
using Umbraco.Cms.Core.Models;
2+
using Umbraco.Extensions;
3+
4+
namespace Umbraco.Cms.Infrastructure.Migrations;
5+
6+
/// <summary>
7+
/// Resolves the culture ISO code for a property data row during migrations,
8+
/// handling the case where the property type varies by culture but the referenced
9+
/// language no longer exists.
10+
/// </summary>
11+
internal static class PropertyDataCultureResolver
12+
{
13+
/// <summary>
14+
/// Log message template used when a property data row references a language that no longer exists.
15+
/// </summary>
16+
internal const string OrphanedLanguageWarningTemplate =
17+
" - property data with id: {propertyDataId} references a language that does not exist - language id: {languageId} (property type: {propertyTypeName}, id: {propertyTypeId}, alias: {propertyTypeAlias})";
18+
19+
/// <summary>
20+
/// Represents the result of resolving a culture for a property data row.
21+
/// </summary>
22+
internal readonly record struct CultureResolutionResult
23+
{
24+
/// <summary>
25+
/// The resolved culture ISO code, or null if invariant.
26+
/// </summary>
27+
public string? Culture { get; init; }
28+
29+
/// <summary>
30+
/// True if the row should be skipped because it references a deleted language.
31+
/// </summary>
32+
public bool ShouldSkip { get; init; }
33+
34+
/// <summary>
35+
/// The language ID that was not found (only set when <see cref="ShouldSkip"/> is true).
36+
/// </summary>
37+
public int? OrphanedLanguageId { get; init; }
38+
}
39+
40+
/// <summary>
41+
/// Resolves the culture for a property data row, detecting orphaned language references.
42+
/// </summary>
43+
/// <param name="propertyType">The property type (may vary by culture via composition).</param>
44+
/// <param name="languageId">The language ID from the property data row (null for invariant data).</param>
45+
/// <param name="languagesById">Lookup of all known languages by ID.</param>
46+
/// <returns>
47+
/// A result indicating the resolved culture and whether the row should be skipped.
48+
/// When <paramref name="languageId"/> is null and the property type varies by culture,
49+
/// this is legitimate invariant data (e.g. from a content type using a culture-varying
50+
/// composition) and should NOT be skipped.
51+
/// </returns>
52+
internal static CultureResolutionResult ResolveCulture(
53+
IPropertyType propertyType,
54+
int? languageId,
55+
IDictionary<int, ILanguage> languagesById)
56+
{
57+
// NOTE: old property data rows may still have languageId populated even if the property type no longer varies
58+
string? culture = propertyType.VariesByCulture()
59+
&& languageId.HasValue
60+
&& languagesById.TryGetValue(languageId.Value, out ILanguage? language)
61+
? language!.IsoCode
62+
: null;
63+
64+
// If culture is null, the property type varies by culture, AND the DTO has a non-null
65+
// language ID, then the language has been deleted. This is an error scenario we can only
66+
// log; in all likelihood this is an old property version and won't cause runtime issues.
67+
//
68+
// If languageId is NULL, this is legitimate invariant data on a content type that uses a
69+
// culture-varying composition — we must NOT skip it.
70+
if (culture is null && propertyType.VariesByCulture() && languageId.HasValue)
71+
{
72+
return new CultureResolutionResult
73+
{
74+
Culture = null,
75+
ShouldSkip = true,
76+
OrphanedLanguageId = languageId.Value,
77+
};
78+
}
79+
80+
return new CultureResolutionResult
81+
{
82+
Culture = culture,
83+
ShouldSkip = false,
84+
};
85+
}
86+
87+
/// <summary>
88+
/// Creates a <see cref="Property"/> suitable for migration value roundtripping.
89+
/// </summary>
90+
/// <remarks>
91+
/// When a property type varies by culture (e.g. inherited from a composition) but the data
92+
/// is invariant (null culture), <see cref="Property.SetValue"/> rejects the null culture.
93+
/// This method handles that case by deep-cloning the property type and setting its
94+
/// <see cref="IPropertyType.Variations"/> to <see cref="ContentVariation.Nothing"/>,
95+
/// which is safe because the data genuinely is invariant.
96+
/// </remarks>
97+
internal static Property CreateMigrationProperty(
98+
IPropertyType propertyType,
99+
object? value,
100+
string? culture,
101+
string? segment)
102+
{
103+
IPropertyType effectivePropertyType = propertyType;
104+
105+
if (culture is null && propertyType.VariesByCulture())
106+
{
107+
// Invariant data on a culture-varying property type (composition scenario).
108+
// Clone to avoid mutating shared state — important for parallel migration execution.
109+
effectivePropertyType = (IPropertyType)propertyType.DeepClone();
110+
effectivePropertyType.Variations = ContentVariation.Nothing;
111+
}
112+
113+
var property = new Property(effectivePropertyType);
114+
property.SetValue(value, culture, segment);
115+
return property;
116+
}
117+
}

‎src/Umbraco.Infrastructure/Migrations/Upgrade/V_15_0_0/ConvertBlockEditorPropertiesBase.cs‎

Lines changed: 7 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -169,33 +169,23 @@ void HandleUpdateBatch(UpdateBatch<PropertyDataDto> update)
169169

170170
PropertyDataDto propertyDataDto = update.Poco;
171171

172-
// NOTE: some old property data DTOs can have variance defined, even if the property type no longer varies
173-
var culture = propertyType.VariesByCulture()
174-
&& propertyDataDto.LanguageId.HasValue
175-
&& languagesById.TryGetValue(
176-
propertyDataDto.LanguageId.Value,
177-
out ILanguage? language)
178-
? language.IsoCode
179-
: null;
180-
181-
if (culture is null && propertyType.VariesByCulture())
172+
var cultureResult = PropertyDataCultureResolver.ResolveCulture(propertyType, propertyDataDto.LanguageId, languagesById);
173+
if (cultureResult.ShouldSkip)
182174
{
183-
// if we end up here, the property DTO is bound to a language that no longer exists. this is an error scenario,
184-
// and we can't really handle it in any other way than logging; in all likelihood this is an old property version,
185-
// and it won't cause any runtime issues
186175
_logger.LogWarning(
187-
" - property data with id: {propertyDataId} references a language that does not exist - language id: {languageId} (property type: {propertyTypeName}, id: {propertyTypeId}, alias: {propertyTypeAlias})",
176+
PropertyDataCultureResolver.OrphanedLanguageWarningTemplate,
188177
propertyDataDto.Id,
189-
propertyDataDto.LanguageId,
178+
cultureResult.OrphanedLanguageId,
190179
propertyType.Name,
191180
propertyType.Id,
192181
propertyType.Alias);
193182
return;
194183
}
195184

185+
var culture = cultureResult.Culture;
186+
196187
var segment = propertyType.VariesBySegment() ? propertyDataDto.Segment : null;
197-
var property = new Property(propertyType);
198-
property.SetValue(propertyDataDto.Value, culture, segment);
188+
var property = PropertyDataCultureResolver.CreateMigrationProperty(propertyType, propertyDataDto.Value, culture, segment);
199189
var toEditorValue = valueEditor.ToEditor(property, culture, segment);
200190
switch (toEditorValue)
201191
{

‎src/Umbraco.Infrastructure/Migrations/Upgrade/V_15_0_0/ConvertLocalLinks.cs‎

Lines changed: 7 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -311,31 +311,23 @@ private bool ProcessPropertyDataDto(
311311
IDictionary<int, ILanguage> languagesById,
312312
IDataValueEditor valueEditor)
313313
{
314-
// NOTE: some old property data DTOs can have variance defined, even if the property type no longer varies
315-
var culture = propertyType.VariesByCulture()
316-
&& propertyDataDto.LanguageId.HasValue
317-
&& languagesById.TryGetValue(propertyDataDto.LanguageId.Value, out ILanguage? language)
318-
? language.IsoCode
319-
: null;
320-
321-
if (culture is null && propertyType.VariesByCulture())
314+
var cultureResult = PropertyDataCultureResolver.ResolveCulture(propertyType, propertyDataDto.LanguageId, languagesById);
315+
if (cultureResult.ShouldSkip)
322316
{
323-
// if we end up here, the property DTO is bound to a language that no longer exists. this is an error scenario,
324-
// and we can't really handle it in any other way than logging; in all likelihood this is an old property version,
325-
// and it won't cause any runtime issues
326317
_logger.LogWarning(
327-
" - property data with id: {propertyDataId} references a language that does not exist - language id: {languageId} (property type: {propertyTypeName}, id: {propertyTypeId}, alias: {propertyTypeAlias})",
318+
PropertyDataCultureResolver.OrphanedLanguageWarningTemplate,
328319
propertyDataDto.Id,
329-
propertyDataDto.LanguageId,
320+
cultureResult.OrphanedLanguageId,
330321
propertyType.Name,
331322
propertyType.Id,
332323
propertyType.Alias);
333324
return false;
334325
}
335326

327+
var culture = cultureResult.Culture;
328+
336329
var segment = propertyType.VariesBySegment() ? propertyDataDto.Segment : null;
337-
var property = new Property(propertyType);
338-
property.SetValue(propertyDataDto.Value, culture, segment);
330+
var property = PropertyDataCultureResolver.CreateMigrationProperty(propertyType, propertyDataDto.Value, culture, segment);
339331
var toEditorValue = valueEditor.ToEditor(property, culture, segment);
340332

341333
if (_localLinkProcessor.ProcessToEditorValue(toEditorValue) == false)

‎src/Umbraco.Infrastructure/Migrations/Upgrade/V_15_1_0/FixConvertLocalLinks.cs‎

Lines changed: 7 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -235,31 +235,23 @@ private bool ProcessPropertyDataDto(
235235
IDictionary<int, ILanguage> languagesById,
236236
IDataValueEditor valueEditor)
237237
{
238-
// NOTE: some old property data DTOs can have variance defined, even if the property type no longer varies
239-
var culture = propertyType.VariesByCulture()
240-
&& propertyDataDto.LanguageId.HasValue
241-
&& languagesById.TryGetValue(propertyDataDto.LanguageId.Value, out ILanguage? language)
242-
? language.IsoCode
243-
: null;
244-
245-
if (culture is null && propertyType.VariesByCulture())
238+
var cultureResult = PropertyDataCultureResolver.ResolveCulture(propertyType, propertyDataDto.LanguageId, languagesById);
239+
if (cultureResult.ShouldSkip)
246240
{
247-
// if we end up here, the property DTO is bound to a language that no longer exists. this is an error scenario,
248-
// and we can't really handle it in any other way than logging; in all likelihood this is an old property version,
249-
// and it won't cause any runtime issues
250241
_logger.LogWarning(
251-
" - property data with id: {propertyDataId} references a language that does not exist - language id: {languageId} (property type: {propertyTypeName}, id: {propertyTypeId}, alias: {propertyTypeAlias})",
242+
PropertyDataCultureResolver.OrphanedLanguageWarningTemplate,
252243
propertyDataDto.Id,
253-
propertyDataDto.LanguageId,
244+
cultureResult.OrphanedLanguageId,
254245
propertyType.Name,
255246
propertyType.Id,
256247
propertyType.Alias);
257248
return false;
258249
}
259250

251+
var culture = cultureResult.Culture;
252+
260253
var segment = propertyType.VariesBySegment() ? propertyDataDto.Segment : null;
261-
var property = new Property(propertyType);
262-
property.SetValue(propertyDataDto.Value, culture, segment);
254+
var property = PropertyDataCultureResolver.CreateMigrationProperty(propertyType, propertyDataDto.Value, culture, segment);
263255
var toEditorValue = valueEditor.ToEditor(property, culture, segment);
264256

265257
if (_localLinkProcessor.ProcessToEditorValue(toEditorValue) == false)

‎src/Umbraco.Infrastructure/Migrations/Upgrade/V_18_0_0/MigrateSingleBlockList.cs‎

Lines changed: 8 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -328,33 +328,25 @@ private bool ProcessPropertyDataDto(
328328
IDataValueEditor valueEditor,
329329
out UpdateItem? updateItem)
330330
{
331-
// NOTE: some old property data DTOs can have variance defined, even if the property type no longer varies
332-
var culture = propertyType.VariesByCulture()
333-
&& propertyDataDto.LanguageId.HasValue
334-
&& languagesById.TryGetValue(propertyDataDto.LanguageId.Value, out ILanguage? language)
335-
? language.IsoCode
336-
: null;
337-
338-
if (culture is null && propertyType.VariesByCulture())
331+
var cultureResult = PropertyDataCultureResolver.ResolveCulture(propertyType, propertyDataDto.LanguageId, languagesById);
332+
if (cultureResult.ShouldSkip)
339333
{
340-
// if we end up here, the property DTO is bound to a language that no longer exists. this is an error scenario,
341-
// and we can't really handle it in any other way than logging; in all likelihood this is an old property version,
342-
// and it won't cause any runtime issues
343334
_logger.LogWarning(
344-
" - property data with id: {propertyDataId} references a language that does not exist - language id: {languageId} (property type: {propertyTypeName}, id: {propertyTypeId}, alias: {propertyTypeAlias})",
335+
PropertyDataCultureResolver.OrphanedLanguageWarningTemplate,
345336
propertyDataDto.Id,
346-
propertyDataDto.LanguageId,
337+
cultureResult.OrphanedLanguageId,
347338
propertyType.Name,
348339
propertyType.Id,
349340
propertyType.Alias);
350341
updateItem = null;
351342
return false;
352343
}
353344

354-
// create a fake property to be able to get a typed value and run it trough the processors.
345+
var culture = cultureResult.Culture;
346+
347+
// create a fake property to be able to get a typed value and run it through the processors.
355348
var segment = propertyType.VariesBySegment() ? propertyDataDto.Segment : null;
356-
var property = new Property(propertyType);
357-
property.SetValue(propertyDataDto.Value, culture, segment);
349+
var property = PropertyDataCultureResolver.CreateMigrationProperty(propertyType, propertyDataDto.Value, culture, segment);
358350
var toEditorValue = valueEditor.ToEditor(property, culture, segment);
359351

360352
if (TryTransformValue(toEditorValue, property, out var updatedValue) is false)

0 commit comments

Comments
 (0)