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

[lauraneto]

Summary

This PR targets v17/bugfix/23103-compositions-locked-by-inheritance (#23433) directly, not v17/dev - it's an addition on top of that work, for Andy to review in isolation before it merges into his branch.

The available-compositions listing (GetAvailableCompositeContentTypes) only accounted for ancestor-lock and same-type conflicts when deciding whether a candidate composition is Allowed. It didn't check whether selecting a candidate would push a property alias down onto a descendant that already defines it - even though #23433 added exactly that check at save time (ValidateDescendantPropertyAliases). A candidate could show as allowed in the picker and then be rejected on save.

• Extracted the shared alias-resolution logic into a new ContentTypeEditingHelper so the listing and the save-time validation read from the same source of truth.
• GetAvailableCompositeContentTypes now also excludes candidates that would introduce a descendant property alias collision.
• Added coverage for the new listing behaviour and for an already-in-use composition staying locked for the parent.
• ValidateCompositions only re-validated newly-added composition keys, skipping already-persisted ones entirely whenever nothing new was being added. Tracing that back, it was a workaround (2fbdc291c9e, pre-dating Content Types: Allow editing compositions on document types that are inherited from (closes #23103) #23433) for isUsing incorrectly treating a tree-inheritance child as a composition user - a bug Content Types: Allow editing compositions on document types that are inherited from (closes #23103) #23433 already fixed at the root by excluding x.ParentId == sourceId. With that fixed, the exemption no longer protects anything reachable, so every requested composition key is now re-validated on every save, persisted or new. This is new to this PR, not a change to anything Content Types: Allow editing compositions on document types that are inherited from (closes #23103) #23433 introduced.
• ValidateDescendantPropertyAliases (added by Content Types: Allow editing compositions on document types that are inherited from (closes #23103) #23433) had the same "only check new aliases" shape, apparently to avoid retroactively rejecting configurations that pre-dated the check. Turns out that scenario was never reachable either: IContentTypeService.InternalSaveAsync calls ValidateLocked unconditionally on every save, independently walking the full transitive composition graph and throwing InvalidCompositionException on the same collision - predating this check entirely and with no persisted/new distinction of its own. Removed the exemption here too. Unlike the point above, this does adjust behaviour Content Types: Allow editing compositions on document types that are inherited from (closes #23103) #23433 introduced, flagging explicitly for Andy.
• Removed test coverage that only demonstrated the now-removed exemptions via composition/property states unreachable through the editing service (only reachable by bypassing validation directly through IContentTypeService).

Testing

Open the composition picker on a content type that has a descendant (via inheritance or composition) already defining a given property alias. Before this change, a composition that would introduce that same alias showed as selectable in the picker, then failed with a validation error on save. After this change, that composition shows as not allowed directly in the picker, consistent with what save-time validation already enforced.

The full ContentTypeEditingServiceTests, MediaTypeEditingServiceTests, and MemberTypeEditingServiceTests suites pass (220/220) on top of #23433's changes.

Possible follow-ups (not addressed here)

While testing this, ran into two pre-existing gaps, unrelated to and not fixed by this PR or #23433. Flagging for visibility, not proposing to act on them now:

• GetComposedOf is direct-only, not transitive. The composition-references endpoints backing it only look one level down. A type several inheritance hops below the direct composer - a grandchild, not just a child - is still affected by changes to that composition (it inherits the same properties transitively), but never shows up in the results. This gap can only be reached through inheritance depth, not composition depth: a type that already composes or inherits something can never itself be composed by another type, so composition chains can't form in the first place.
• Moving a content type in the tree can silently reclassify an inheritance edge into a composition edge. Composition vs. inheritance isn't a stored flag - it's derived by comparing a cmsContentType2ContentType edge's parentContentTypeId against the child's current umbracoNode.parentId (match = inheritance, mismatch = composition). The tree "Move" command changes the child's tree position but never reconciles the edge, so moving an inherited-from parent away can flip that edge's meaning with no explicit conversion step or validation. Confirmed this lets a moved type bypass the "a composition can't itself be composed" rule (Allowed count for the abandoned parent drops to 0, unearned). Verified this cannot be escalated into an actual composition cycle - closing any cycle needs a fresh edge landing on a type that already has some inheritance/composition relationship, which is independently blocked either way.

[claude]

Claude finished @lauraneto's task in 9m 51s —— View job

---

PR Review

Target: origin/v17/bugfix/23103-compositions-locked-by-inheritance · Based on commit: 5bbafd01

Extracts a shared GetAllDescendantPropertyAliases helper and wires it into both GetAvailableCompositeContentTypes (listing) and the simplified ValidateDescendantPropertyAliases (save-time), so the picker and the validator share the same source of truth. Also removes a grandfather exemption from ValidateCompositions and a "new-only" gate from ValidateDescendantPropertyAliases.

• Other changes: Error status for composition-induced alias collision changes from ContentTypeOperationStatus.DuplicatePropertyTypeAlias to ContentTypeOperationStatus.InvalidComposition; consumers checking the specific status code are affected.

---

Critical

ContentTypeEditingServiceBase.cs:449 + ContentTypeServiceExtensions.cs:96–104 — Re-saving a content type that already has composition C fails with InvalidComposition whenever it has an inheriting child.

Root cause. GetAllDescendantPropertyAliases collects each descendant's CompositionPropertyTypes, which is computed recursively (ContentTypeCompositionBase.cs:120):

return ContentTypeComposition
    .SelectMany(x => x.CompositionPropertyTypes) // recursive
    .Union(PropertyTypes);

So for child D inheriting P which composes C:

• D.CompositionPropertyTypes includes C's properties (via P's composition)
• GetAllDescendantPropertyAliases(P) therefore includes C's aliases in descendantPropertyAliases
• GetAvailableCompositeContentTypes adds those to filterPropertyTypes, then filters out any candidate whose own PropertyTypes overlap — so C is marked Allowed = false
• ValidateCompositions sees C is not in allowedCompositionKeys and returns InvalidComposition

Concrete repro:

1. Create P with composition C (property alias "shared")
2. Create D inheriting P
3. Open P in backoffice, change nothing, hit Save → InvalidComposition

Why the removed exemption was still load-bearing. The PR description argues the exemption "no longer protects anything reachable" because the isUsing bug it was originally shielding was fixed by the x.ParentId != sourceId guard. That reasoning is correct for the isUsing path. But removing the exemption is no longer safe once GetAvailableCompositeContentTypes has been extended with the descendant alias check — that new check introduces a new false-positive path that the exemption previously protected.

Suggested fix. In ValidateCompositions, apply the descendant-alias check only to newly-added compositions (those not already in contentType.ContentTypeComposition). Already-persisted compositions have already passed validation; their aliases are already propagated to descendants and don't constitute new collisions.

Guid[] compositionKeys = KeysForCompositionTypes(model, CompositionType.Composition);

// Only validate newly-added compositions against descendants; persisted ones already propagate
// their aliases onto descendants legitimately.
HashSet<Guid> alreadyPersistedKeys = contentType?.ContentTypeComposition
    .Select(c => c.Key)
    .ToHashSet() ?? [];
Guid[] keysToValidate = compositionKeys
    .Where(k => !alreadyPersistedKeys.Contains(k))
    .ToArray();

if (keysToValidate.Length == 0)
{
    return ContentTypeOperationStatus.Success;
}

Guid[] allowedCompositionKeys = _contentTypeService.GetAvailableCompositeContentTypes(
        contentType, allContentTypeCompositions, isElement: model.IsElement)
    .Results
    .Where(x => x.Allowed)
    .Select(x => x.Composition.Key)
    .ToArray();

return allowedCompositionKeys.ContainsAll(keysToValidate) is false
    ? ContentTypeOperationStatus.InvalidComposition
    : ContentTypeOperationStatus.Success;

This preserves the intent of both removals (the isUsing exemption stays gone, the new-only gate in ValidateDescendantPropertyAliases stays gone) while closing the specific re-save regression.

Missing test. There is no test for "can re-save P with existing composition C when D inherits P." Adding one would have caught this before merge.

---

Suggestions

• ContentTypeEditingServiceBase.cs:506 — descendantPropertyAliases.InvariantContains(alias) resolves to IEnumerable<string>.Contains(value, StringComparer.InvariantCultureIgnoreCase), which iterates the HashSet regardless of its internal structure. Since GetAllDescendantPropertyAliases already lowercases every alias on insertion, descendantPropertyAliases.Contains(alias.ToLowerInvariant()) would use the hash bucket (O(1) vs O(n)).

---

Request Changes

The re-save regression is a blocker: any content type that (a) has an existing composition and (b) has an inheriting child will fail on every save after this PR lands.

[AndyButland]

Thanks for the update @lauraneto. I like the idea. Much better to prevent the selection of a composition that won't be allowed than to reject it once selected on save.

I found a problem though with the logic: any document type that has at least one own property and at least one child can no longer be saved.

So that should be addressed.

I see Can_Reapply_Compositions_For_Content_Type_With_Children uses a property-less composition and property-less parent, so nothing collides, and hence this passes. So we should have a test that re-saves a parent with properties and descendants (which should fail before a fix).

Root cause looks to be that CompositionPropertyTypes is transitive, so a descendant's effective set always contains everything the source pushes down - so the source ends up colliding with itself. Subtracting source.CompositionPropertyTypes from the descendant set looks to be what's needed to fix.

The same self-collision breaks the composition path too: a type with a composition carrying properties plus any descendant now fails to re-save with InvalidComposition.

Other points:

• Consider unit tests for ContentTypeEditingHelper and ContentTypeServiceExtensions? These could be fast and in-memory, so cheap to run once written.

I've not tested the main fix yet, but will do so once this save issue is resolved.

[claude]

Claude finished @lauraneto's task in 6m 34s —— View job

---

PR Review

Target: origin/v17/bugfix/23103-compositions-locked-by-inheritance · Based on commit: e385227c

Fixes the composition picker showing candidates as selectable when selecting them would cause a descendant property alias collision. Extracts a shared GetAllDescendantPropertyAliases helper (excluding the source's own contribution to prevent self-collision) used by both GetAvailableCompositeContentTypes (picker) and ValidateDescendantPropertyAliases (save-time). Also removes the "only validate new compositions" early exit from ValidateCompositions now that the self-collision issue in the helper makes that safe.

• Other changes: Error status for composition-induced descendant alias collision changes from ContentTypeOperationStatus.DuplicatePropertyTypeAlias to ContentTypeOperationStatus.InvalidComposition; consumers (including the management API) already handle both separately with distinct 400 titles ("Invalid composition" vs "Duplicate property type alias"), but any client-side code keying on the specific status string will see a different response.

---

Suggestions

• ContentTypeEditingServiceBase.cs:506 — descendantPropertyAliases.InvariantContains(alias) dispatches through the IEnumerable<string> overload (compare.Contains(compareTo, StringComparer.InvariantCultureIgnoreCase)) which iterates the set, O(n). Since the HashSet is already constructed with StringComparer.InvariantCultureIgnoreCase, calling the instance method descendantPropertyAliases.Contains(alias) uses the hash bucket, O(1). With a small property set this won't matter, but the fix is one word:

  var collidesWithDescendants = model.Properties.Select(p => p.Alias).Any(descendantPropertyAliases.Contains);

---

Approved with Suggestions for improvement

The critical re-save regression identified in the previous round has been addressed by the ExceptWith(source.CompositionPropertyTypes) call in GetAllDescendantPropertyAliases, and the two new integration tests (Can_Reapply_Composition_With_Property_When_It_Has_An_Inheriting_Child, Can_Resave_Own_Property_When_Content_Type_Has_An_Inheriting_Child) pin the fix. The helper logic is correct: composition-pushed aliases that a descendant receives through the source are correctly excluded from the descendant collision set, preventing false positives on re-save while still catching genuinely independent descendant conflicts.

Good to go, but please carefully consider the importance of the suggestions.

[lauraneto]

@AndyButland Ups, I had totally missed the findings from Claude.
I have applied the changes and added a few more tests now.

[AndyButland]

All looks good to me now @lauraneto. Just one minor naming suggestion. Please go ahead and merge once you've considered that.

I've confirmed the bug I found in the previous review is resolved, and that the improvement provided by this PR is in place - when I try to select compositions, and types that have a property that would clash with one of the current type's descendants, are not available for selection.

[sonarqubecloud]

Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
76.7% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud