Conversation
…tiesBase, 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.
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes an upgrade-migration edge case where property data rows for invariant content types were incorrectly skipped when the property type varies by culture via compositions, and centralizes culture-resolution logic into a shared helper with unit test coverage.
Changes:
- Fix culture-varying skip logic so rows are only skipped when
languageIdis non-null but missing from the language lookup (orphaned/deleted language scenario). - Refactor duplicated culture-resolution + warning-template logic into
PropertyDataCultureResolver. - Add unit tests covering invariant/culture-varying and orphaned-language scenarios (including the reported bug case).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Migrations/PropertyDataCultureResolverTests.cs | Adds unit tests for all ResolveCulture decision paths, including the invariant-content / culture-varying-composition bug scenario. |
| src/Umbraco.Infrastructure/Migrations/Upgrade/V_18_0_0/MigrateSingleBlockList.cs | Switches migration logic to the shared resolver and centralized warning template when encountering orphaned languages. |
| src/Umbraco.Infrastructure/Migrations/Upgrade/V_15_1_0/FixConvertLocalLinks.cs | Replaces duplicated culture-resolution logic with PropertyDataCultureResolver. |
| src/Umbraco.Infrastructure/Migrations/Upgrade/V_15_0_0/ConvertLocalLinks.cs | Replaces duplicated culture-resolution logic with PropertyDataCultureResolver. |
| src/Umbraco.Infrastructure/Migrations/Upgrade/V_15_0_0/ConvertBlockEditorPropertiesBase.cs | Replaces duplicated culture-resolution logic with PropertyDataCultureResolver inside batch processing. |
| src/Umbraco.Infrastructure/Migrations/PropertyDataCultureResolver.cs | Introduces the shared resolver + structured result and a shared warning template constant. |
You can also share your feedback on Copilot code review. Take the survey.
src/Umbraco.Infrastructure/Migrations/PropertyDataCultureResolver.cs
Outdated
Show resolved
Hide resolved
src/Umbraco.Infrastructure/Migrations/Upgrade/V_18_0_0/MigrateSingleBlockList.cs
Outdated
Show resolved
Hide resolved
Contributor
|
Code LGTM |
…c-in-convert-migrations
AndyButland
added a commit
that referenced
this pull request
Mar 19, 2026
…ulture-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>
Contributor
Author
|
Cherry-picked into |
This was referenced Apr 2, 2026
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR addresses the issue raised in #22159, where where V15+ upgrade migrations that convert property data would skip on invariant content types that use culture-varying compositions.
We do this in four migrations, so I've extracted it into a shared, testable helper (
PropertyDataCultureResolver).Then I could add unit tests covering all resolution paths, including the bug scenario.
Problem
When upgrading to V15+, four migrations (
ConvertBlockEditorProperties,ConvertLocalLinks,FixConvertLocalLinks,MigrateSingleBlockList) resolve the culture for each property data row before processing it. The resolution logic had a bug in its skip condition:This skipped all rows where culture resolved to null and the property type varied by culture. But there is a legitimate case where both conditions are true yet the row should be processed: when an invariant content type composes a culture-varying content type. In that scenario:
VariesByCulture() == true(inherited from the composition).languageId = NULL(correct — the content type is invariant).nullbecauselanguageIdis null.The intended skip case is when
languageIdis non-null but references a language that no longer exists in the database.Fix
Add
&& propertyDataDto.LanguageId.HasValueto the skip condition, distinguishing:languageIdVariesByCultureNULLtruetruetrueAdditional Fix
When manually testing, I found that when
PropertyDataCultureResolvercorrectly stops skipping invariant rows,Property.SetValuewould throwNotSupportedExceptionbecause the property type reportsVariesByCulturebut culture isnull. To resolve I've added aCreateMigrationPropertyhelper that, instead of creating a newPropertydeep-clones the property type withVariations=Nothingfor this case, keeping the original unmodified for thread safety during parallel migration execution.Refactoring
The identical culture-resolution + skip logic was duplicated across 4 migration files. This PR extracts it into
PropertyDataCultureResolver.ResolveCulture(), a pure static method that returns a result struct. This:IPropertyType,int?, andIDictionary<int, ILanguage>, making it trivially testable with mocks and no infrastructure dependenciesconstreplaces 4 copies of the same stringTesting
Automated
Unit tests have been added in
PropertyDataCultureResolverTestswhich tests the code around the bug. The expected ones failed before fix.An integration test for the migration didn't prove feasible to setup.
Manual testing steps
main.