Content Types: Also check descendants for property alias collisions when listing available compositions

src/Umbraco.Core/Services/ContentTypeEditing/ContentTypeEditingHelper.cs

@@ -0,0 +1,67 @@
+using Umbraco.Cms.Core.Models;
+
+namespace Umbraco.Cms.Core.Services.ContentTypeEditing;
+
+/// <summary>
+///     Shared helper logic for content type editing.
+/// </summary>
+internal static class ContentTypeEditingHelper
+{
+    /// <summary>
+    ///     Gets the combined, full effective property alias set (own properties plus everything each one gets from
+    ///     its own compositions) of every content type descending from <paramref name="source"/>, across both the
+    ///     tree inheritance and composition axes - excluding whatever already flows into those descendants through
+    ///     <paramref name="source"/>'s own current state.
+    /// </summary>
+    /// <param name="source">The content type to find descendants of, or <c>null</c> when there are none (e.g. creating a new content type).</param>
+    /// <param name="allContentTypes">All existing content type compositions, used to resolve the descendant tree.</param>
+    /// <returns>The property aliases (compared case-insensitively) already effective on every descendant of <paramref name="source"/>, excluding aliases <paramref name="source"/> itself already contributes.</returns>
+    /// <remarks>
+    ///     Inheritance children hold their parent in their own <see cref="IContentTypeComposition.ContentTypeComposition"/>,
+    ///     so walking the composition graph downward captures inheritance and composition descendants alike.
+    /// </remarks>
+    internal static HashSet<string> GetPropertyAliasesReservedByDescendants(this IContentTypeComposition? source, IContentTypeComposition[] allContentTypes)
+    {
+        if (source is null)
+        {
+            return [];
+        }
+
+        // build a "referenced id -> types that directly reference it" lookup once, so the traversal is O(n + d)
+        // rather than rescanning every content type for each descendant
+        ILookup<int, IContentTypeComposition> directReferencingTypes = allContentTypes
+            .SelectMany(
+                contentType => contentType.ContentTypeComposition,
+                (contentType, referenced) => (ReferencedId: referenced.Id, ContentType: contentType))
+            .ToLookup(x => x.ReferencedId, x => x.ContentType);
+
+        var descendantIds = new HashSet<int>();
+        var descendantPropertyAliases = new HashSet<string>(StringComparer.InvariantCultureIgnoreCase);
+        var stack = new Stack<int>();
+        stack.Push(source.Id);
+        while (stack.Count > 0)
+        {
+            var currentId = stack.Pop();
+            foreach (IContentTypeComposition descendant in directReferencingTypes[currentId])
+            {
+                if (!descendantIds.Add(descendant.Id))
+                {
+                    continue;
+                }
+
+                IEnumerable<IPropertyType> allProperties = descendant.PropertyTypes
+                    .Concat(descendant.CompositionPropertyTypes);
+                foreach (IPropertyType descendantPropertyType in allProperties)
+                {
+                    descendantPropertyAliases.Add(descendantPropertyType.Alias);
+                }
+
+                stack.Push(descendant.Id);
+            }
+        }
+
+        descendantPropertyAliases.ExceptWith(source.CompositionPropertyTypes.Select(pt => pt.Alias));
+
+        return descendantPropertyAliases;
+    }
+}

src/Umbraco.Core/Services/ContentTypeEditing/ContentTypeEditingServiceBase.cs

@@ -286,7 +286,7 @@
         }
 
         // element types cannot vary by segment
-        if (model.IsElement && model.VariesBySegment)
+        if (model is { IsElement: true, VariesBySegment: true })
         {
             return ContentTypeOperationStatus.InvalidSegmentVariationForElementType;
         }
@@ -439,33 +439,25 @@
     /// <param name="model">The model to validate.</param>
     /// <param name="allContentTypeCompositions">All existing content type compositions.</param>
     /// <returns>The validation status.</returns>
-    private ContentTypeOperationStatus ValidateCompositions(TContentType? contentType, ContentTypeEditingModelBase<TPropertyTypeModel, TPropertyTypeContainer> model, IContentTypeComposition[] allContentTypeCompositions)
+    private ContentTypeOperationStatus ValidateCompositions(
+        TContentType? contentType,
+        ContentTypeEditingModelBase<TPropertyTypeModel, TPropertyTypeContainer> model,
+        IContentTypeComposition[] allContentTypeCompositions)
     {
-        // get the content type keys we want to use for compositions
         Guid[] compositionKeys = KeysForCompositionTypes(model, CompositionType.Composition);
 
-        // if the content type keys are already set as compositions, don't perform any additional validation
-        // - this covers an edge case where compositions are configured for a content type before child content types are created
-        if (contentType is not null && contentType.ContentTypeComposition
-            .Select(c => c.Key)
-            .ContainsAll(compositionKeys))
-        {
-            return ContentTypeOperationStatus.Success;
-        }
-
-        // verify that all compositions keys are allowed
-        Guid[] allowedCompositionKeys = _contentTypeService.GetAvailableCompositeContentTypes(contentType, allContentTypeCompositions, isElement: model.IsElement)
+        Guid[] allowedCompositionKeys = _contentTypeService.GetAvailableCompositeContentTypes(
+                contentType,
+                allContentTypeCompositions,
+                isElement: model.IsElement)
             .Results
             .Where(x => x.Allowed)
             .Select(x => x.Composition.Key)
             .ToArray();
 
-        if (allowedCompositionKeys.ContainsAll(compositionKeys) is false)
-        {
-            return ContentTypeOperationStatus.InvalidComposition;
-        }
-
-        return ContentTypeOperationStatus.Success;
+        return allowedCompositionKeys.ContainsAll(compositionKeys) is false
+            ? ContentTypeOperationStatus.InvalidComposition
+            : ContentTypeOperationStatus.Success;
     }
 
     /// <summary>
@@ -494,103 +486,30 @@
     }
 
     /// <summary>
-    ///     Validates that no property alias newly introduced on this content type (via its own properties or
-    ///     its compositions) collides with a property alias already effective on one of its descendants.
+    ///     Validates that no property alias on this content type's own properties collides with a property alias
+    ///     already effective on one of its descendants.
     /// </summary>
     /// <remarks>
-    ///     Descendants inherit this content type's properties (through tree inheritance and/or composition), so a
-    ///     newly introduced alias that a descendant already defines would produce a duplicate effective alias on that
-    ///     descendant. <see cref="ValidateProperties"/> only guards the edited type's own model, so this handles the
-    ///     case where the edited type is itself inherited from or composed by others. Only genuinely new aliases are
-    ///     checked so that pre-existing configurations are never retroactively rejected.
+    ///     Descendants inherit this content type's properties (through tree inheritance and/or composition), so an
+    ///     alias that a descendant already defines would produce a duplicate effective alias on that descendant.
+    ///     <see cref="ValidateProperties"/> only guards the edited type's own model, so this handles the case where
+    ///     the edited type is itself inherited from or composed by others. Collisions caused by a newly selected
+    ///     composition are caught separately by <see cref="ValidateCompositions"/>.
     /// </remarks>
     private static ContentTypeOperationStatus ValidateDescendantPropertyAliases(
         TContentType? contentType,
         ContentTypeEditingModelBase<TPropertyTypeModel, TPropertyTypeContainer> model,
         IContentTypeComposition[] allContentTypeCompositions)
     {
-        // on create the content type has no descendants yet, so there is nothing to collide with
-        if (contentType is null)
-        {
-            return ContentTypeOperationStatus.Success;
-        }
-
-        Dictionary<Guid, IContentTypeComposition> compositionsByKey = allContentTypeCompositions.ToDictionary(c => c.Key);
+        HashSet<string> descendantPropertyAliases = contentType.GetPropertyAliasesReservedByDescendants(allContentTypeCompositions);
 
-        // the aliases this content type would push down to its descendants: its own properties plus the
-        // property types of every composition/inheritance it references
-        Guid[] modelCompositionKeys = KeysForCompositionTypes(model, CompositionType.Composition, CompositionType.Inheritance);
-        IEnumerable<string> modelDownwardAliases = model.Properties
-            .Select(p => p.Alias)
-            .Concat(modelCompositionKeys
-                .Where(compositionsByKey.ContainsKey)
-                .SelectMany(k => compositionsByKey[k].CompositionPropertyTypes.Select(pt => pt.Alias)));
+        var collidesWithDescendants = model.Properties.Select(p => p.Alias).Any(descendantPropertyAliases.InvariantContains);
 
-        // the aliases already pushed down by the persisted type - anything here is not newly introduced
-        IEnumerable<string> persistedDownwardAliases = contentType.PropertyTypes
-            .Select(pt => pt.Alias)
-            .Concat(contentType.ContentTypeComposition.SelectMany(c => c.CompositionPropertyTypes.Select(pt => pt.Alias)));
-
-        var newAliases = new HashSet<string>(modelDownwardAliases.Select(a => a.ToLowerInvariant()));
-        newAliases.ExceptWith(persistedDownwardAliases.Select(a => a.ToLowerInvariant()));
-        if (newAliases.Count == 0)
-        {
-            return ContentTypeOperationStatus.Success;
-        }
-
-        HashSet<int> descendantIds = GetTransitiveDescendantIds(contentType.Id, allContentTypeCompositions);
-
-        // compare against each descendant's full effective property set (its own properties plus everything it
-        // gets from its other compositions), not just its own properties - otherwise a newly introduced alias
-        // could still collide with an alias a descendant inherits from one of its other compositions. newAliases
-        // already excludes what this content type currently pushes down, so aliases the descendant inherits from
-        // this content type cannot cause a false positive.
-        var collision = allContentTypeCompositions
-            .Where(c => descendantIds.Contains(c.Id))
-            .SelectMany(c => c.CompositionPropertyTypes.Select(pt => pt.Alias.ToLowerInvariant()))
-            .Any(newAliases.Contains);
-
-        return collision
+        return collidesWithDescendants
             ? ContentTypeOperationStatus.DuplicatePropertyTypeAlias
             : ContentTypeOperationStatus.Success;
     }
 
-    /// <summary>
-    ///     Gets the ids of all content types that descend from the specified content type, across both the tree
-    ///     inheritance and composition axes.
-    /// </summary>
-    /// <remarks>
-    ///     Inheritance children hold their parent in their own <see cref="IContentTypeComposition.ContentTypeComposition"/>,
-    ///     so walking the composition graph downward captures inheritance and composition descendants alike.
-    /// </remarks>
-    private static HashSet<int> GetTransitiveDescendantIds(int contentTypeId, IContentTypeComposition[] allContentTypeCompositions)
-    {
-        // build a "referenced id -> types that directly reference it" lookup once, so the traversal is O(n + d)
-        // rather than rescanning every content type for each descendant
-        ILookup<int, IContentTypeComposition> directReferencingTypes = allContentTypeCompositions
-            .SelectMany(
-                contentType => contentType.ContentTypeComposition,
-                (contentType, referenced) => (ReferencedId: referenced.Id, ContentType: contentType))
-            .ToLookup(x => x.ReferencedId, x => x.ContentType);
-
-        var descendantIds = new HashSet<int>();
-        var stack = new Stack<int>();
-        stack.Push(contentTypeId);
-        while (stack.Count > 0)
-        {
-            var currentId = stack.Pop();
-            foreach (IContentTypeComposition descendant in directReferencingTypes[currentId])
-            {
-                if (descendantIds.Add(descendant.Id))
-                {
-                    stack.Push(descendant.Id);
-                }
-            }
-        }
-
-        return descendantIds;
-    }
-
     /// <summary>
     ///     Validates that all containers (property groups/tabs) are properly configured.
     /// </summary>

src/Umbraco.Core/Services/ContentTypeServiceExtensions.cs

@@ -4,6 +4,7 @@
 using Umbraco.Cms.Core;
 using Umbraco.Cms.Core.Models;
 using Umbraco.Cms.Core.Services;
+using Umbraco.Cms.Core.Services.ContentTypeEditing;
 
 namespace Umbraco.Extensions;
 
@@ -53,16 +54,17 @@ public static IEnumerable<TItem> GetComposedOf<TItem>(
         };
 
     /// <summary>
-    ///     Returns the available composite content types for a given content type
+    ///     Gets the composite content types available to a given content type, indicating whether each one is
+    ///     actually allowed to be selected.
     /// </summary>
-    /// <param name="allContentTypes"></param>
+    /// <param name="ctService">The content type service.</param>
+    /// <param name="source">The content type to find available compositions for, or <c>null</c> when creating a new one.</param>
+    /// <param name="allContentTypes">All existing content types of the same kind (document, media or member type).</param>
     /// <param name="filterContentTypes">
     ///     This is normally an empty list but if additional content type aliases are passed in, any content types containing
     ///     those aliases will be filtered out
     ///     along with any content types that have matching property types that are included in the filtered content types
     /// </param>
-    /// <param name="ctService"></param>
-    /// <param name="source"></param>
     /// <param name="filterPropertyTypes">
     ///     This is normally an empty list but if additional property type aliases are passed in, any content types that have
     ///     these aliases will be filtered out.
@@ -71,7 +73,12 @@ public static IEnumerable<TItem> GetComposedOf<TItem>(
     ///     be looked up via the db, they need to be passed in.
     /// </param>
     /// <param name="isElement">Whether the composite content types should be applicable for an element type</param>
-    /// <returns></returns>
+    /// <returns>Every candidate composition (excluding <paramref name="source"/> itself), each flagged as allowed or not.</returns>
+    /// <remarks>
+    ///     A composition is marked not allowed when it is <paramref name="source"/>'s own ancestor, when it is filtered
+    ///     out via <paramref name="filterContentTypes"/> or <paramref name="filterPropertyTypes"/>, or when selecting it
+    ///     would push a property alias down onto one of <paramref name="source"/>'s descendants that already defines it.
+    /// </remarks>
     public static ContentTypeAvailableCompositionsResults GetAvailableCompositeContentTypes(
         this IContentTypeService ctService,
         IContentTypeComposition? source,
@@ -80,23 +87,21 @@ public static ContentTypeAvailableCompositionsResults GetAvailableCompositeConte
         string[]? filterPropertyTypes = null,
         bool isElement = false)
     {
-        filterContentTypes = filterContentTypes == null
-            ? Array.Empty<string>()
-            : filterContentTypes.Where(x => !x.IsNullOrWhiteSpace()).ToArray();
-
-        filterPropertyTypes = filterPropertyTypes == null
-            ? Array.Empty<string>()
-            : filterPropertyTypes.Where(x => !x.IsNullOrWhiteSpace()).ToArray();
-
-        // create the full list of property types to use as the filter
-        // this is the combination of all property type aliases found in the content types passed in for the filter
-        // as well as the specific property types passed in for the filter
-        filterPropertyTypes = allContentTypes
-            .Where(c => filterContentTypes.InvariantContains(c.Alias))
-            .SelectMany(c => c.PropertyTypes)
-            .Select(c => c.Alias)
-            .Union(filterPropertyTypes)
-            .ToArray();
+        filterContentTypes = filterContentTypes?.Where(x => !x.IsNullOrWhiteSpace()).ToArray() ?? [];
+        filterPropertyTypes = filterPropertyTypes?.Where(x => !x.IsNullOrWhiteSpace()).ToArray() ?? [];
+
+        // aliases to exclude candidates on: the ones passed in, the properties of any filtered content types, and
+        // every alias already effective on a descendant. Always reports the truth, even for already-selected
+        // compositions - callers that want to ignore unchanged state must filter that out themselves.
+        filterPropertyTypes =
+        [
+            .. filterPropertyTypes,
+            .. allContentTypes
+                .Where(c => filterContentTypes.InvariantContains(c.Alias))
+                .SelectMany(c => c.PropertyTypes)
+                .Select(c => c.Alias),
+            .. source.GetPropertyAliasesReservedByDescendants(allContentTypes)
+        ];
 
         var sourceId = source?.Id ?? 0;
 
@@ -116,9 +121,10 @@ public static ContentTypeAvailableCompositionsResults GetAvailableCompositeConte
 
         // if it is not used then composition is possible
         // hashset guarantees uniqueness on Id
-        var list = new HashSet<IContentTypeComposition>(new DelegateEqualityComparer<IContentTypeComposition>(
-            (x, y) => x?.Id == y?.Id,
-            x => x.Id));
+        var list = new HashSet<IContentTypeComposition>(
+            new DelegateEqualityComparer<IContentTypeComposition>(
+                (x, y) => x?.Id == y?.Id,
+                x => x.Id));
 
         // usable types are those that are top-level
         // do not allow element types to be composed by non-element types as this will break the model generation in ModelsBuilder
@@ -162,9 +168,7 @@ public static ContentTypeAvailableCompositionsResults GetAvailableCompositeConte
 
         // now we can create our result based on what is still available and the ancestors
         var result = list
-
-            // not itself
-            .Where(x => x.Id != sourceId)
+            .Where(x => x.Id != sourceId) // not itself
             .OrderBy(x => x.Name)
             .Select(composition => filtered.Contains(composition)
                 ? new ContentTypeAvailableCompositionsResult(composition, ancestorIds.Contains(composition.Id) == false)