Umbraco 10 upgrade messes up Property Type Group aliases

[arknu]

Which exact Umbraco version are you using? For example: 9.0.1 - don't just write v9

10.1.0

Bug summary

I'm upgrading a site from v8 to v10. During the upgrade, v10 will mess up the aliases for property type groups. Take this example:

The top result here is after running the v10 upgrade, the bottom one is before.

Note that, the top one (id 15) correctly has the alias "websted/websted", since it is a group called "Websted" on a tab called "Websted". However, after the upgrade, they now have an identical alias. Specifically, it is the tab name that gets stripped out. This is of course going to cause problems, and indeed you get an exception:

ArgumentException: An item with the same key has already been added. Key: indhold
System.Collections.Generic.Dictionary<TKey, TValue>.TryInsert(TKey key, TValue value, InsertionBehavior behavior)
System.Collections.ObjectModel.KeyedCollection<TKey, TItem>.InsertItem(int index, TItem item)
Umbraco.Cms.Core.Models.PropertyGroupCollection.InsertItem(int index, PropertyGroup item)
Umbraco.Cms.Core.Models.PropertyGroupCollection.Add(PropertyGroup item)
Umbraco.Cms.Infrastructure.Persistence.Repositories.Implement.ContentTypeCommonRepository.MapGroupsAndProperties(IDictionary<int, IContentTypeComposition> contentTypes)
Umbraco.Cms.Infrastructure.Persistence.Repositories.Implement.ContentTypeCommonRepository.GetAllTypesInternal()
Umbraco.Extensions.AppCacheExtensions+<>c__DisplayClass0_0<T>.<GetCacheItem>b__0()
Umbraco.Cms.Core.Cache.SafeLazy+<>c__DisplayClass1_0.<GetSafeLazy>b__0()
Umbraco.Cms.Core.Cache.ObjectCacheAppCache.Get(string key, Func<object> factory, Nullable<TimeSpan> timeout, bool isSliding, string[] dependentFiles)
Umbraco.Cms.Core.Cache.DeepCloneAppCache.Get(string key, Func<object> factory, Nullable<TimeSpan> timeout, bool isSliding, string[] dependentFiles)
Umbraco.Extensions.AppCacheExtensions.GetCacheItem<T>(IAppPolicyCache provider, string cacheKey, Func<T> getCacheItem, Nullable<TimeSpan> timeout, bool isSliding, string[] dependentFiles)
Umbraco.Cms.Infrastructure.Persistence.Repositories.Implement.ContentTypeCommonRepository.GetAllTypes()
Umbraco.Cms.Infrastructure.Persistence.Repositories.Implement.ContentTypeRepository.GetAllWithFullCachePolicy()
Umbraco.Cms.Infrastructure.Persistence.Repositories.Implement.ContentTypeRepositoryBase<TEntity>.PerformGetAll(int[] ids)
Umbraco.Cms.Core.Cache.FullDataSetRepositoryCachePolicy<TEntity, TId>.GetAllCached(Func<TId[], IEnumerable<TEntity>> performGetAll)
Umbraco.Cms.Core.Cache.FullDataSetRepositoryCachePolicy<TEntity, TId>.GetAll(TId[] ids, Func<TId[], IEnumerable<TEntity>> performGetAll)
Umbraco.Cms.Core.Services.ContentTypeServiceBase<TRepository, TItem>.GetAll(int[] ids)
Umbraco.Cms.Infrastructure.PublishedCache.PublishedSnapshotService.LoadContentFromDatabaseLocked(bool onStartup)
Umbraco.Cms.Infrastructure.PublishedCache.PublishedSnapshotService.<EnsureCaches>b__52_3()
Umbraco.Cms.Infrastructure.PublishedCache.PublishedSnapshotService.LockAndLoadContent(Func<bool> action)
Umbraco.Cms.Infrastructure.PublishedCache.PublishedSnapshotService.<EnsureCaches>b__52_0()
System.Threading.LazyInitializer.EnsureInitializedCore<T>(ref T target, ref bool initialized, ref object syncLock, Func<T> valueFactory)
System.Threading.LazyInitializer.EnsureInitialized<T>(ref T target, ref bool initialized, ref object syncLock, Func<T> valueFactory)
Umbraco.Cms.Infrastructure.PublishedCache.PublishedSnapshotService.EnsureCaches()
Umbraco.Cms.Infrastructure.PublishedCache.PublishedSnapshotService.CreatePublishedSnapshot(string previewToken)
Umbraco.Cms.Web.Common.UmbracoContext.UmbracoContext+<>c__DisplayClass11_0.<.ctor>b__0()
System.Lazy<T>.ViaFactory(LazyThreadSafetyMode mode)
System.Lazy<T>.ExecutionAndPublication(LazyHelper executionAndPublication, bool useDefaultConstructor)
System.Lazy<T>.CreateValue()
Umbraco.Cms.Web.Common.UmbracoContext.UmbracoContext.get_PublishedSnapshot()
Umbraco.Cms.Web.Common.UmbracoContext.UmbracoContext.get_Content()
Umbraco.Cms.Web.Website.Routing.UmbracoRouteValueTransformer.TransformAsync(HttpContext httpContext, RouteValueDictionary values)
System.Threading.Tasks.ValueTask<TResult>.get_Result()
Microsoft.AspNetCore.Mvc.Routing.DynamicControllerEndpointMatcherPolicy.ApplyAsync(HttpContext httpContext, CandidateSet candidates)
Microsoft.AspNetCore.Routing.Matching.DfaMatcher.SelectEndpointWithPoliciesAsync(HttpContext httpContext, IEndpointSelectorPolicy[] policies, CandidateSet candidateSet)
Microsoft.AspNetCore.Routing.EndpointRoutingMiddleware.<Invoke>g__AwaitMatch|8_1(EndpointRoutingMiddleware middleware, HttpContext httpContext, Task matchTask)
SixLabors.ImageSharp.Web.Middleware.ImageSharpMiddleware.Invoke(HttpContext httpContext, bool retry)
StackExchange.Profiling.MiniProfilerMiddleware.Invoke(HttpContext context) in MiniProfilerMiddleware.cs
Umbraco.Cms.Web.Common.Middleware.UmbracoRequestMiddleware.InvokeAsync(HttpContext context, RequestDelegate next)
Umbraco.Cms.Web.Common.Middleware.UmbracoRequestMiddleware.InvokeAsync(HttpContext context, RequestDelegate next)
Microsoft.AspNetCore.Builder.UseMiddlewareExtensions+<>c__DisplayClass6_1+<<UseMiddlewareInterface>b__1>d.MoveNext()
Umbraco.Cms.Web.Common.Middleware.PreviewAuthenticationMiddleware.InvokeAsync(HttpContext context, RequestDelegate next)
Microsoft.AspNetCore.Builder.UseMiddlewareExtensions+<>c__DisplayClass6_1+<<UseMiddlewareInterface>b__1>d.MoveNext()
Umbraco.Cms.Web.Common.Middleware.UmbracoRequestLoggingMiddleware.InvokeAsync(HttpContext context, RequestDelegate next)
Microsoft.AspNetCore.Builder.UseMiddlewareExtensions+<>c__DisplayClass6_1+<<UseMiddlewareInterface>b__1>d.MoveNext()
Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware.Invoke(HttpContext context)

The only solution is then to modify the aliases back manually, which is not exactly optimal.

Specifics

No response

Steps to reproduce

Take a v8 database with document type with a tab containing a group of the same name. Upgrade to v10. Notice the exception.

Expected result / actual result

No response

[github-actions[bot]]

Hi there @arknu!

Firstly, a big thank you for raising this issue. Every piece of feedback we receive helps us to make Umbraco better.

We really appreciate your patience while we wait for our team to have a look at this but we wanted to let you know that we see this and share with you the plan for what comes next.

• We'll assess whether this issue relates to something that has already been fixed in a later version of the release that it has been raised for.
• If it's a bug, is it related to a release that we are actively supporting or is it related to a release that's in the end-of-life or security-only phase?
• We'll replicate the issue to ensure that the problem is as described.
• We'll decide whether the behavior is an issue or if the behavior is intended.

We wish we could work with everyone directly and assess your issue immediately but we're in the fortunate position of having lots of contributions to work with and only a few humans who are able to do it. We are making progress though and in the meantime, we will keep you in the loop and let you know when we have any questions.

Thanks, from your friendly Umbraco GitHub bot :robot: :slightly_smiling_face:

[lm-chpa]

I kind of having the same problem.

Upgrading a Umbraco database from latest Umbraco version 8.18.4 to latest 10.1.0 On a document type I have a Tab and a Property Group sharing same name.

The fix for me was to change one of the names in the database so I could run the upgrade.

[nul800sebastiaan]

@arknu I feel like something was wrong in your before setup, this is what it should have looked like:

What v8 version were you coming from? When I create a Group "Websted" on tab "Websted", this is the result. Maybe we fixed a bug at some point when this didn't happen? I'm not sure.

[arknu]

@nul800sebastiaan I'm coming from 8.18. They do look like that in the v8 database:

However, the upgrade code messes them up.

I have a suspicion that this line may be causing the issue: https://github.com/umbraco/Umbraco-CMS/blob/5f4f16957fddc884bb6650cdb8bacd07e7ecb25e/src/Umbraco.Infrastructure/Migrations/Upgrade/V_8_17_0/AddPropertyTypeGroupColumns.cs#L17-L18

I my mind, this should use the existing alias (when one exists) rather than trying to create a new one from the text

This migration gets rerun in the v9 upgrade plan, as specified here: https://github.com/umbraco/Umbraco-CMS/blob/5f4f16957fddc884bb6650cdb8bacd07e7ecb25e/src/Umbraco.Infrastructure/Migrations/Upgrade/UmbracoPlan.cs#L267-L268

However, I didn't have time to investigate further before I got called away by other work.

[nul800sebastiaan]

Then it should be re-run! So I am suspecting that you went straight from 8 to 10? The recommendation is to go to 9 first, where all the migrations run and then to 10. I can see this is not documented anywhere so I think we'll need to add that to the docs.

[arknu]

@nul800sebastiaan It does run this migration during the upgrade from v8 to v10, that is the whole problem. From reading the code in the migration, it doesn't take the hierarchy of property groups into account (probably because the migration was originally written before the hierarchy existed).

I believe I tried upgrading to v9 with the same result, although I am not 100 % certain. I'll test this when I'm able to.

Upgrading to v9 first is not a solution. Having to these in-between upgrades makes the whole thing a lot more complicated, because you have to deploy in multiple steps. Given the increased frequency of major versions (a very good thing), Umbraco is going to have to support upgrading multiple major versions at a time.

[nul800sebastiaan]

For sure we understand that it's not good to have to go to intermediate major versions each time, the migration path from v8 to v10 requires this for good reasons, once you're on 10 we expect major upgrades won't be requiring intermediate major upgrades though.

[nul800sebastiaan]

I don't think the AddPropertyTypeGroupColumns does anything harmful, it even detects the same alias for multiple property groups, the problem is that your tab and your groups should already have had a different alias.

Do you have any idea what Umbraco version you had at the time those tabs / groups were created (SELECT createDate FROM umbracoNode WHERE id = 1073 to learn when the doctype was created might help)?

[arknu]

@nul800sebastiaan Just retested, upgrading from v8 to 9.5.3. Exactly the same issue:

It crashes after upgrade due to multiple identical aliases.

This migration is the only place I could find that would touch those aliases during an upgrade so it has to be that, somehow.

I'll attempt to debug a v8 to v10 upgrade. I can also get you a copy of the database, if you want.

[arknu]

@nul800sebastiaan Here's my theory: The AddPropertyTypeGroupColumns migration was originally written to do the upgrade to 8.17 which introduced tabs. Thus creating a need for a hierarchy in property type groups, since you can have groups within tabs. However, since this hierarchy didn't exist pre-8.17, this migration discards that and simply creates aliases from the Text property.

The migration doesn't take the hierarchy into account - because that hierarchy didn't exist pre-8.17. This migration was never intended to be run on a database that already has tabs and nested PropertyTypeGroups. Nowhere in the migration does it even attempt to add the parent group alias, as Umbraco normally does when generating child group aliases normally. All groups on the "Websted" tab will have an alias starting with "websted/". And it is precisely that prefix which is lost during the upgrade - a prefix that did not exist pre 8.17.

It breaks when you have a group with a tabs with the same name. I have a tab called "Websted" (alias "websted"), with a group called "Websted" (alias "websted/websted"). When running this migration, it creates new aliases from the Text of each PropertyGroup, resulting in two groups with the same aliases.

[nul800sebastiaan]

Maybe.. I am not sure what happened to get the data in an incorrect state.

I just tested in 8.17.0 and I can't get the data to be wrong, the database correctly makes a websted/websted and a websted alias. So yes, it could be that some migrations got it wrong.

Abit of SQL digging should help though.

select s.id, s.alias
from cmsPropertyTypeGroup s
join (
    select alias, contenttypeNodeId
    from cmsPropertyTypeGroup
    group by contenttypeNodeId, alias
    having count(*) > 1
) t on s.contenttypeNodeId = t.contenttypeNodeId and s.alias = t.alias
WHERE type = 0

That would give you all the rows that need to be updated, so all of these should append / plus the existing alias and then you should be good to go.

[arknu]

@nul800sebastiaan I ended up just correcting the aliases in the database manually, so I got the upgrade sorted.

But again, I am quite sure that this happens because the AddPropertyTypeGroupColumns migration does not take the hierarchy into account. It updates all aliases by running the Text value through the ToSafeAlias method - which discards hierarchy information. So "Websted" and "Websted"/"Websted" both get the alias "websted".

[arknu]

Here is a complete before and after.

Before (v8)

After (v9 in this case, v10 is the same)

Notice how every single prefix has now gone, discarded by the broken migration

[arknu]

So, I finally managed to debug this issue. And my theory is correct.

Here the existing DTOs from the database have been loaded. Note that the aliases are correctly prefixed.

It then groups by the Text property. I have taken the Websted groups here, since there are only two of those.

Now, we loop through each DTO in the grouping. Notice that the "websted/websted" alias is still intact.

And after executing this line, the alias is now incorrect, as this code does not take the hierarchy into account.

I don't think we can modify the migration plan this migration is used in, so the best thing would be to update this migration to handle both being run on a pre-8.17 database with no hierarchy and a post-8.17 database with hierarchy, making the appropriate prefixes for both.

[lm-chpa]

Side note: @arknu OMG good investigation/debugging

[arknu]

I have submitted a PR (#13109) to fix this issue. I think the simplest solution is simply to avoid running the PopulateAliases method on any PropetyTypeGroupDtos that already have an aliases (i.e. the database was already upgraded to 8.17).

[arknu]

@nul800sebastiaan Have you had a chance to look at the PR to confirm if this is the correct approach?