From a84d67eff8207936cd5e5df675bc6ebec503759e Mon Sep 17 00:00:00 2001 From: Laura Neto <12862535+lauraneto@users.noreply.github.com> Date: Wed, 1 Oct 2025 09:39:56 +0200 Subject: [PATCH] Migrations: Create missing tabs on content types when referenced by both composition and content type groups (closes #20058) (#20303) * Add migration to create missing tabs In v13, if a tab had groups in both a composition and the content type, the tab might not exist on the content type itself. Newer versions require such tabs to also exist directly on the content type. This migration ensures those tabs are created. Also fixes an issue in LeftJoin where nested sql arguments were being discarded. * Small fixes * WIP: Integration test. * Added asserts to show the current issue with the integration test. * Adjusted the integration test * Added logging of result. Minor re-order and extraction refactoring in integration test. --------- Co-authored-by: Andy Butland --- .../Migrations/Upgrade/UmbracoPlan.cs | 3 + .../Upgrade/V_16_4_0/CreateMissingTabs.cs | 135 ++++++++++++ .../Persistence/NPocoSqlExtensions.cs | 46 +++- .../Builders/PropertyGroupBuilder.cs | 11 +- .../Upgrade/V_16_4_0/CreateMissingTabsTest.cs | 208 ++++++++++++++++++ 5 files changed, 401 insertions(+), 2 deletions(-) create mode 100644 src/Umbraco.Infrastructure/Migrations/Upgrade/V_16_4_0/CreateMissingTabs.cs create mode 100644 tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Migrations/Upgrade/V_16_4_0/CreateMissingTabsTest.cs diff --git a/src/Umbraco.Infrastructure/Migrations/Upgrade/UmbracoPlan.cs b/src/Umbraco.Infrastructure/Migrations/Upgrade/UmbracoPlan.cs index 39917cabe5..76d6737c59 100644 --- a/src/Umbraco.Infrastructure/Migrations/Upgrade/UmbracoPlan.cs +++ b/src/Umbraco.Infrastructure/Migrations/Upgrade/UmbracoPlan.cs @@ -127,5 +127,8 @@ public class UmbracoPlan : MigrationPlan // To 16.3.0 To("{A917FCBC-C378-4A08-A36C-220C581A6581}"); To("{FB7073AF-DFAF-4AC1-800D-91F9BD5B5238}"); + + // To 16.4.0 + To("{6A7D3B80-8B64-4E41-A7C0-02EC39336E97}"); } } diff --git a/src/Umbraco.Infrastructure/Migrations/Upgrade/V_16_4_0/CreateMissingTabs.cs b/src/Umbraco.Infrastructure/Migrations/Upgrade/V_16_4_0/CreateMissingTabs.cs new file mode 100644 index 0000000000..a6ebafd075 --- /dev/null +++ b/src/Umbraco.Infrastructure/Migrations/Upgrade/V_16_4_0/CreateMissingTabs.cs @@ -0,0 +1,135 @@ +using Microsoft.Extensions.Logging; +using NPoco; +using Umbraco.Cms.Infrastructure.Persistence; +using Umbraco.Cms.Infrastructure.Persistence.Dtos; +using Umbraco.Extensions; + +namespace Umbraco.Cms.Infrastructure.Migrations.Upgrade.V_16_4_0; + +/// +/// Creates missing tabs on content types when a tab is referenced by both a composition and the content type's own groups. +/// +/// +/// In v13, if a tab had groups in both a composition and the content type, the tab might not exist on the content type itself. +/// Newer versions require such tabs to also exist directly on the content type. This migration ensures those tabs are created. +/// +[Obsolete("Remove in Umbraco 18.")] +public class CreateMissingTabs : AsyncMigrationBase +{ + private readonly ILogger _logger; + + /// + /// Initializes a new instance of the class. + /// + public CreateMissingTabs(IMigrationContext context, ILogger logger) + : base(context) => _logger = logger; + + /// + protected override async Task MigrateAsync() + { + await ExecuteMigration(Database, _logger); + Context.Complete(); + } + + /// + /// Performs the migration to create missing tabs. + /// + /// + /// Extracted into an internal static method to support integration testing. + /// + internal static async Task ExecuteMigration(IUmbracoDatabase database, ILogger logger) + { + // 1. Find all property groups (type 0) and extract their tab alias (the part before the first '/'). + // This helps identify which groups are referencing tabs. + Sql groupsSql = database.SqlContext.Sql() + .SelectDistinct("g", pt => pt.ContentTypeNodeId) + .AndSelect(GetTabAliasQuery(database.DatabaseType, "g.alias") + " AS tabAlias") + .From(alias: "p") + .InnerJoin(alias: "g").On( + (pt, ptg) => pt.PropertyTypeGroupId == ptg.Id && pt.ContentTypeId == ptg.ContentTypeNodeId, + aliasLeft: "p", + "g") + .Where(x => x.Type == 0, alias: "g") + .Where(CheckIfContainsTabAliasQuery(database.DatabaseType, "g.alias")); + + // 2. Get all existing tabs (type 1) for all content types. + Sql tabsSql = database.SqlContext.Sql() + .Select("g2", g => g.UniqueId, g => g.ContentTypeNodeId, g => g.Alias) + .From(alias: "g2") + .Where(x => x.Type == 1, alias: "g2"); + + // 3. Identify groups that reference a tab alias which does not exist as a tab for their content type. + // These are the "missing tabs" that need to be created. + Sql missingTabsSql = database.SqlContext.Sql() + .Select("groups", g => g.ContentTypeNodeId) + .AndSelect("groups.tabAlias") + .From() + .AppendSubQuery(groupsSql, "groups") + .LeftJoin(tabsSql, "tabs") + .On("groups.ContentTypeNodeId = tabs.ContentTypeNodeId AND tabs.alias = groups.tabAlias") + .WhereNull(ptg => ptg.UniqueId, "tabs"); + + // 4. For each missing tab, find the corresponding tab details (text, alias, sort order) + // from the parent content type (composition) that already has this tab. + Sql missingTabsWithDetailsSql = database.SqlContext.Sql() + .Select("missingTabs", ptg => ptg.ContentTypeNodeId) + .AndSelect("tg", ptg => ptg.Alias) + .AndSelect("MIN(text) AS text", "MIN(sortorder) AS sortOrder") + .From() + .AppendSubQuery(missingTabsSql, "missingTabs") + .InnerJoin(alias: "ct2ct") + .On( + (ptg, ct2Ct) => ptg.ContentTypeNodeId == ct2Ct.ChildId, + "missingTabs", + "ct2ct") + .InnerJoin(alias: "tg") + .On( + (ct2Ct, ptg) => ct2Ct.ParentId == ptg.ContentTypeNodeId, + "ct2ct", + "tg") + .Append("AND tg.alias = missingTabs.tabAlias") + .GroupBy("missingTabs", ptg => ptg.ContentTypeNodeId) + .AndBy("tg", ptg => ptg.Alias); + + List missingTabsWithDetails = + await database.FetchAsync(missingTabsWithDetailsSql); + + // 5. Create and insert new tab records for each missing tab, using the details from the parent/composition. + IEnumerable newTabs = missingTabsWithDetails + .Select(missingTabWithDetails => new PropertyTypeGroupDto + { + UniqueId = Guid.CreateVersion7(), + ContentTypeNodeId = missingTabWithDetails.ContentTypeNodeId, + Type = 1, + Text = missingTabWithDetails.Text, + Alias = missingTabWithDetails.Alias, + SortOrder = missingTabWithDetails.SortOrder, + }); + await database.InsertBatchAsync(newTabs); + + logger.LogInformation( + "Created {MissingTabCount} tab records to migrate property group information for content types derived from compositions.", + missingTabsWithDetails.Count); + } + + private static string GetTabAliasQuery(DatabaseType databaseType, string columnName) => + databaseType == DatabaseType.SQLite + ? $"substr({columnName}, 1, INSTR({columnName},'/') - 1)" + : $"SUBSTRING({columnName}, 1, CHARINDEX('/', {columnName}) - 1)"; + + private static string CheckIfContainsTabAliasQuery(DatabaseType databaseType, string columnName) => + databaseType == DatabaseType.SQLite + ? $"INSTR({columnName}, '/') > 0" + : $"CHARINDEX('/', {columnName}) > 0"; + + private class MissingTabWithDetails + { + public required int ContentTypeNodeId { get; set; } + + public required string Alias { get; set; } + + public required string Text { get; set; } + + public required int SortOrder { get; set; } + } +} diff --git a/src/Umbraco.Infrastructure/Persistence/NPocoSqlExtensions.cs b/src/Umbraco.Infrastructure/Persistence/NPocoSqlExtensions.cs index d2c7aa87ff..bf2e1eb723 100644 --- a/src/Umbraco.Infrastructure/Persistence/NPocoSqlExtensions.cs +++ b/src/Umbraco.Infrastructure/Persistence/NPocoSqlExtensions.cs @@ -393,6 +393,26 @@ namespace Umbraco.Extensions return sql.GroupBy(columns); } + /// + /// Appends a GROUP BY clause to the Sql statement. + /// + /// The type of the Dto. + /// The Sql statement. + /// A table alias. + /// Expression specifying the fields. + /// The Sql statement. + public static Sql GroupBy( + this Sql sql, + string tableAlias, + params Expression>[] fields) + { + ISqlSyntaxProvider sqlSyntax = sql.SqlContext.SqlSyntax; + var columns = fields.Length == 0 + ? sql.GetColumns(withAlias: false) + : fields.Select(x => sqlSyntax.GetFieldName(x, tableAlias)).ToArray(); + return sql.GroupBy(columns); + } + /// /// Appends more ORDER BY or GROUP BY fields to the Sql statement. /// @@ -548,7 +568,8 @@ namespace Umbraco.Extensions join += " " + sql.SqlContext.SqlSyntax.GetQuotedTableName(alias); } - return sql.LeftJoin(join); + sql.Append("LEFT JOIN " + join, nestedSelect.Arguments); + return new Sql.SqlJoinClause(sql); } /// @@ -801,6 +822,29 @@ namespace Umbraco.Extensions return sql; } + /// + /// Creates a SELECT DISTINCT Sql statement. + /// + /// The type of the DTO to select. + /// The origin sql. + /// A table alias. + /// Expressions indicating the columns to select. + /// The Sql statement. + /// + /// If is empty, all columns are selected. + /// + public static Sql SelectDistinct(this Sql sql, string tableAlias, params Expression>[] fields) + { + if (sql == null) + { + throw new ArgumentNullException(nameof(sql)); + } + + var columns = sql.GetColumns(tableAlias: tableAlias, columnExpressions: fields); + sql.Append("SELECT DISTINCT " + string.Join(", ", columns)); + return sql; + } + public static Sql SelectDistinct(this Sql sql, params object[] columns) { sql.Append("SELECT DISTINCT " + string.Join(", ", columns)); diff --git a/tests/Umbraco.Tests.Common/Builders/PropertyGroupBuilder.cs b/tests/Umbraco.Tests.Common/Builders/PropertyGroupBuilder.cs index 5a958f1389..39732f4fe8 100644 --- a/tests/Umbraco.Tests.Common/Builders/PropertyGroupBuilder.cs +++ b/tests/Umbraco.Tests.Common/Builders/PropertyGroupBuilder.cs @@ -45,6 +45,7 @@ public class PropertyGroupBuilder private int? _sortOrder; private bool? _supportsPublishing; private DateTime? _updateDate; + private PropertyGroupType? _type; public PropertyGroupBuilder(TParent parentBuilder) : base(parentBuilder) @@ -99,6 +100,12 @@ public class PropertyGroupBuilder set => _updateDate = value; } + public PropertyGroupBuilder WithType(PropertyGroupType type) + { + _type = type; + return this; + } + public PropertyGroupBuilder WithPropertyTypeCollection(PropertyTypeCollection propertyTypeCollection) { _propertyTypeCollection = propertyTypeCollection; @@ -122,6 +129,7 @@ public class PropertyGroupBuilder var name = _name ?? Guid.NewGuid().ToString(); var sortOrder = _sortOrder ?? 0; var supportsPublishing = _supportsPublishing ?? false; + var type = _type ?? PropertyGroupType.Group; PropertyTypeCollection propertyTypeCollection; if (_propertyTypeCollection != null) @@ -145,7 +153,8 @@ public class PropertyGroupBuilder Name = name, SortOrder = sortOrder, CreateDate = createDate, - UpdateDate = updateDate + UpdateDate = updateDate, + Type = type, }; } } diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Migrations/Upgrade/V_16_4_0/CreateMissingTabsTest.cs b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Migrations/Upgrade/V_16_4_0/CreateMissingTabsTest.cs new file mode 100644 index 0000000000..7e733eb955 --- /dev/null +++ b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Migrations/Upgrade/V_16_4_0/CreateMissingTabsTest.cs @@ -0,0 +1,208 @@ +// Copyright (c) Umbraco. +// See LICENSE for more details. + +using Microsoft.Extensions.Logging.Abstractions; +using NPoco; +using NUnit.Framework; +using Umbraco.Cms.Api.Management.ViewModels.DocumentType; +using Umbraco.Cms.Core; +using Umbraco.Cms.Core.Mapping; +using Umbraco.Cms.Core.Models; +using Umbraco.Cms.Core.Services; +using Umbraco.Cms.Infrastructure.Migrations.Upgrade.V_16_4_0; +using Umbraco.Cms.Infrastructure.Persistence; +using Umbraco.Cms.Infrastructure.Persistence.Dtos; +using Umbraco.Cms.Infrastructure.Scoping; +using Umbraco.Cms.Tests.Common.Builders; +using Umbraco.Cms.Tests.Common.Builders.Extensions; +using Umbraco.Cms.Tests.Integration.TestServerTest; + +namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Migrations.Upgrade.V16_4_0; + +[TestFixture] +internal sealed class CreateMissingTabsTest : UmbracoTestServerTestBase +{ + private IScopeProvider ScopeProvider => GetRequiredService(); + + private IContentTypeService ContentTypeService => GetRequiredService(); + + private IUmbracoMapper UmbracoMapper => GetRequiredService(); + + /// + /// A verification integration test for the solution to https://github.com/umbraco/Umbraco-CMS/issues/20058 + /// provided in https://github.com/umbraco/Umbraco-CMS/pull/20303. + /// + [Test] + public async Task Can_Create_Missing_Tabs() + { + // Prepare a base and composed content type. + (IContentType baseContentType, IContentType composedContentType) = await PrepareTestData(); + + // Assert the groups and properties are created in the database and that the content type model is as expected. + await AssertValidDbGroupsAndProperties(baseContentType.Id, composedContentType.Id); + await AssertValidContentTypeModel(composedContentType.Key); + + // Prepare the database state as it would have been in Umbraco 13. + await PreparePropertyGroupPersistedStateForUmbraco13(composedContentType); + + // Assert that the content type groups are now without a parent tab. + await AssertInvalidContentTypeModel(composedContentType.Key); + + // Run the migration to add the missing tab back. + await ExecuteMigration(); + + // Re-retrieve the content types and assert that the groups and types are as expected. + await AssertValidContentTypeModel(composedContentType.Key); + + // Verify in the database that the migration has re-added only the record we removed in the setup. + await AssertValidDbGroupsAndProperties(baseContentType.Id, composedContentType.Id); + } + + private async Task<(IContentType BaseContentType, IContentType ComposedContentType)> PrepareTestData() + { + // Prepare document types as per reproduction steps described here: https://github.com/umbraco/Umbraco-CMS/issues/20058#issuecomment-3332742559 + // - Create a new composition with a tab "Content" and inside add a group "Header" with a "Text 1" property inside. + // - Save the composition. + // - Create a new document type and inherit the composition created in step 2. + // - Add a new property "Text 2" to the Content > Header group. + // - Create a new group "Home Content", inside the "Content" tab, and add a property "Text 3". + // - Save the document type. + + // Create base content type. + var baseContentType = new ContentTypeBuilder() + .WithAlias("baseType") + .WithName("Base Type") + .AddPropertyGroup() + .WithAlias("content") + .WithName("Content") + .WithType(PropertyGroupType.Tab) + .Done() + .AddPropertyGroup() + .WithAlias("content/header") + .WithName("Header") + .WithType(PropertyGroupType.Group) + .AddPropertyType() + .WithAlias("text1") + .WithName("Text 1") + .Done() + .Done() + .Build(); + await ContentTypeService.CreateAsync(baseContentType, Constants.Security.SuperUserKey); + baseContentType = await ContentTypeService.GetAsync(baseContentType.Key); + + // Create composed content type. + var composedContentType = new ContentTypeBuilder() + .WithAlias("composedType") + .WithName("Composed Type") + .AddPropertyGroup() + .WithAlias("content") + .WithName("Content") + .WithType(PropertyGroupType.Tab) + .Done() + .AddPropertyGroup() + .WithAlias("content/header") + .WithName("Header") + .WithType(PropertyGroupType.Group) + .AddPropertyType() + .WithAlias("text2") + .WithName("Text 2") + .Done() + .Done() + .AddPropertyGroup() + .WithAlias("content/homeContent") + .WithName("Home Content") + .WithType(PropertyGroupType.Group) + .AddPropertyType() + .WithAlias("text3") + .WithName("Text 3") + .Done() + .Done() + .Build(); + composedContentType.ContentTypeComposition = [baseContentType]; + await ContentTypeService.CreateAsync(composedContentType, Constants.Security.SuperUserKey); + composedContentType = await ContentTypeService.GetAsync(composedContentType.Key); + return (baseContentType, composedContentType); + } + + private async Task AssertValidDbGroupsAndProperties(int baseContentTypeId, int composedContentTypeId) + { + using IScope scope = ScopeProvider.CreateScope(); + Sql groupsSql = scope.Database.SqlContext.Sql() + .Select() + .From() + .WhereIn(x => x.ContentTypeNodeId, new[] { baseContentTypeId, composedContentTypeId }); + var groups = await scope.Database.FetchAsync(groupsSql); + Assert.AreEqual(5, groups.Count); + + Assert.AreEqual(1, groups.Count(x => x.ContentTypeNodeId == baseContentTypeId && x.Type == (int)PropertyGroupType.Tab)); + Assert.AreEqual(1, groups.Count(x => x.ContentTypeNodeId == baseContentTypeId && x.Type == (int)PropertyGroupType.Group)); + + Assert.AreEqual(1, groups.Count(x => x.ContentTypeNodeId == composedContentTypeId && x.Type == (int)PropertyGroupType.Tab)); + Assert.AreEqual(2, groups.Count(x => x.ContentTypeNodeId == composedContentTypeId && x.Type == (int)PropertyGroupType.Group)); + + Sql propertiesSql = scope.Database.SqlContext.Sql() + .Select() + .From() + .WhereIn(x => x.ContentTypeId, new[] { baseContentTypeId, composedContentTypeId }); + var types = await scope.Database.FetchAsync(propertiesSql); + Assert.AreEqual(3, types.Count); + scope.Complete(); + } + + private async Task AssertValidContentTypeModel(Guid contentTypeKey) + { + var contentType = await ContentTypeService.GetAsync(contentTypeKey); + DocumentTypeResponseModel model = UmbracoMapper.Map(contentType)!; + Assert.AreEqual(3, model.Containers.Count()); + + var contentTab = model.Containers.FirstOrDefault(c => c.Name == "Content" && c.Type == nameof(PropertyGroupType.Tab)); + Assert.IsNotNull(contentTab); + + var headerGroup = model.Containers.FirstOrDefault(c => c.Name == "Header" && c.Type == nameof(PropertyGroupType.Group)); + Assert.IsNotNull(headerGroup); + Assert.IsNotNull(headerGroup.Parent); + Assert.AreEqual(contentTab.Id, headerGroup.Parent.Id); + + var homeContentGroup = model.Containers.FirstOrDefault(c => c.Name == "Home Content" && c.Type == nameof(PropertyGroupType.Group)); + Assert.IsNotNull(homeContentGroup); + Assert.IsNotNull(homeContentGroup.Parent); + Assert.AreEqual(contentTab.Id, homeContentGroup.Parent.Id); + } + + private async Task PreparePropertyGroupPersistedStateForUmbraco13(IContentType composedContentType) + { + // Delete one of the tab records so we get to the 13 state. + using IScope scope = ScopeProvider.CreateScope(); + Sql deleteTabSql = scope.Database.SqlContext.Sql() + .Delete() + .Where(x => x.Type == (int)PropertyGroupType.Tab && x.ContentTypeNodeId == composedContentType.Id); + var deletedCount = await scope.Database.ExecuteAsync(deleteTabSql); + scope.Complete(); + Assert.AreEqual(1, deletedCount); + } + + private async Task AssertInvalidContentTypeModel(Guid contentTypeKey) + { + var contentType = await ContentTypeService.GetAsync(contentTypeKey); + DocumentTypeResponseModel model = UmbracoMapper.Map(contentType)!; + Assert.AreEqual(2, model.Containers.Count()); + + var contentTab = model.Containers.FirstOrDefault(c => c.Name == "Content" && c.Type == nameof(PropertyGroupType.Tab)); + Assert.IsNull(contentTab); + + var headerGroup = model.Containers.FirstOrDefault(c => c.Name == "Header" && c.Type == nameof(PropertyGroupType.Group)); + Assert.IsNotNull(headerGroup); + Assert.IsNull(headerGroup.Parent); + + var homeContentGroup = model.Containers.FirstOrDefault(c => c.Name == "Home Content" && c.Type == nameof(PropertyGroupType.Group)); + Assert.IsNotNull(homeContentGroup); + Assert.IsNull(homeContentGroup.Parent); + } + + private async Task ExecuteMigration() + { + using IScope scope = ScopeProvider.CreateScope(); + await CreateMissingTabs.ExecuteMigration(scope.Database, new NullLogger()); + scope.Complete(); + } +}