Repository navigation
Content Types: Allow editing compositions on document types that are inherited from (closes #23103) - #23433
Conversation
|
Claude finished @AndyButland's task in 7m 51s —— View job PR ReviewTarget: Fixes the backoffice incorrectly locking compositions on document/media types that are only inherited from (not used as a composition), by correctly distinguishing the two relationships — which share the same underlying storage — through
Suggestions
Approved with Suggestions for improvementThe fix is well-reasoned and architecturally sound. It uses the established |
There was a problem hiding this comment.
Pull request overview
Fixes an editing-time false-positive where content types that are only inherited from were treated as being used as a composition, which incorrectly locked composition editing in the backoffice. The change separates “composed-of” relationships into composition vs inheritance where needed, and adds save-time validation to prevent newly introduced descendant alias collisions.
Changes:
- Introduces
ComposedOfTypeplus aGetComposedOf(..., ComposedOfType)extension to filter “composed of” results by relationship kind, and uses it in composition-reference endpoints. - Updates
GetAvailableCompositeContentTypesto treat inheriting children (ParentId == sourceId) as inheritance, not “used in composition”, so the parent remains composable. - Adds descendant effective-alias collision validation when introducing new properties/compositions on a type that already has descendants, with comprehensive unit/integration test coverage.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/Umbraco.Tests.UnitTests/Umbraco.ModelsBuilder.Embedded/BuilderTests.cs | Adds ModelsBuilder regression test for base-type + composition + inheriting child scenario. |
| tests/Umbraco.Tests.UnitTests/Umbraco.Core/Services/ContentTypeServiceExtensionsTests.cs | Updates unit test to assert inheritance doesn’t lock parent out of compositions. |
| tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/ContentTypeServiceTests.cs | Adds integration test validating “composed of” filtering by composition vs inheritance. |
| tests/Umbraco.Tests.Integration/Umbraco.Core/Services/MediaTypeEditingServiceTests.Update.cs | Adds integration tests for adding composition to parent-with-child and descendant alias-collision guard (media). |
| tests/Umbraco.Tests.Integration/Umbraco.Core/Services/MediaTypeEditingServiceTests.GetAvailableCompositions.cs | Adds integration test asserting available compositions for parent with inheriting child (media). |
| tests/Umbraco.Tests.Integration/Umbraco.Core/Services/ContentTypeEditingServiceTests.Update.cs | Repurposes/extends update-path tests to allow compositions with inheriting children + new alias-collision validation cases (content). |
| tests/Umbraco.Tests.Integration/Umbraco.Core/Services/ContentTypeEditingServiceTests.GetAvailableCompositions.cs | Adds integration test asserting available compositions for parent with inheriting child (content). |
| tests/Umbraco.Tests.Integration/Umbraco.Core/Cache/PublishedContentTypeCacheTests.cs | Adds regression test ensuring published child exposes composition property via inherited parent. |
| src/Umbraco.Core/Services/IContentTypeBaseService.cs | Clarifies GetComposedOf(int) semantics (includes inheritance) and points to filtering extension. |
| src/Umbraco.Core/Services/ContentTypeServiceExtensions.cs | Adds filtered GetComposedOf extension + fixes “isUsing” guard to ignore inheritance children. |
| src/Umbraco.Core/Services/ContentTypeEditing/ContentTypeEditingServiceBase.cs | Adds validation preventing new aliases from colliding with descendants’ effective property aliases. |
| src/Umbraco.Core/Services/ComposedOfType.cs | Adds enum to express “composed-of” relationship axis (composition/inheritance/all). |
| src/Umbraco.Cms.Api.Management/Controllers/MemberType/CompositionReferenceMemberTypeController.cs | Uses composition-only filtering for composition-reference endpoint (member types). |
| src/Umbraco.Cms.Api.Management/Controllers/MediaType/CompositionReferenceMediaTypeController.cs | Uses composition-only filtering for composition-reference endpoint (media types). |
| src/Umbraco.Cms.Api.Management/Controllers/DocumentType/CompositionReferenceDocumentTypeController.cs | Uses composition-only filtering for composition-reference endpoint (document types). |
…added ones ValidateCompositions only checked genuinely new composition keys against GetAvailableCompositeContentTypes, letting an already-persisted composition ride along unchecked even if it had since become structurally invalid (e.g. now colliding with a descendant's property alias). We initially assumed this exemption was deliberately protecting a legitimate scenario, but tracing it back to when it was introduced (2fbdc29, before #23433) showed it was only ever working around isUsing incorrectly treating an inheritance child as a composition user. isUsing now correctly excludes inheritance children, so the exemption is no longer needed and every requested composition key - persisted or new - is validated on every save. Removed test coverage that only demonstrated the now-removed exemption via composition states unreachable through the editing service or the tree Move command (only reachable by bypassing validation directly through IContentTypeService, e.g. legacy data or raw API/import usage).
|
@AndyButland I've made some changes on top of this and created a PR: #23529. |
…hen listing available compositions (#23529) * Content Types: Also check descendants for property alias collisions when listing available compositions Available-compositions listing only checked ancestor-lock and same-type conflicts, missing the descendant-alias-collision check that save-time validation already enforced. A candidate could show as allowed in the picker and then be rejected on save. Extracted the shared alias-resolution logic (own aliases, descendant aliases) into ContentTypeEditingHelper so both the listing and the save-time validation use the same source of truth. * Content Types: Add coverage for descendant property alias collisions Covers the descendant-collision check in the available-compositions listing (available but not allowed), that an already-in-use composition stays locked for the parent, and that a pre-existing collision on one composition doesn't exempt an unrelated candidate sharing the same alias. Also updates existing status-code assertions from DuplicatePropertyTypeAlias to InvalidComposition now that composition-caused collisions are split from own-property-caused ones. * Content Types: Re-validate every composition on save, not just newly added ones ValidateCompositions only checked genuinely new composition keys against GetAvailableCompositeContentTypes, letting an already-persisted composition ride along unchecked even if it had since become structurally invalid (e.g. now colliding with a descendant's property alias). We initially assumed this exemption was deliberately protecting a legitimate scenario, but tracing it back to when it was introduced (2fbdc29, before #23433) showed it was only ever working around isUsing incorrectly treating an inheritance child as a composition user. isUsing now correctly excludes inheritance children, so the exemption is no longer needed and every requested composition key - persisted or new - is validated on every save. Removed test coverage that only demonstrated the now-removed exemption via composition states unreachable through the editing service or the tree Move command (only reachable by bypassing validation directly through IContentTypeService, e.g. legacy data or raw API/import usage). * Content Types: Drop the grandfather exemption in ValidateDescendantPropertyAliases Same reasoning as the ValidateCompositions change: this exemption assumed a content type could legitimately have an own-property alias colliding with a descendant's, from before this check existed, and that resaving it shouldn't retroactively reject that. Tracing the deeper save path shows that scenario was never actually reachable - IContentTypeService.InternalSaveAsync calls ValidateLocked on every create/update, unconditionally, which already walks the full transitive composition/inheritance graph and throws InvalidCompositionException on any such collision. It has no persisted/new distinction and predates this check entirely. Keeping the exemption only made the failure mode worse: if it ever let a collision through, the save wouldn't succeed anyway, it would just fail later as an unhandled exception from ValidateLocked instead of a clean DuplicatePropertyTypeAlias status here. Also removes ContentTypeEditingHelper.GetAllPropertyAliases, which was only used to compute the now-removed persisted/new split and has no other caller. * Content Types: Exclude source's own contribution from descendant alias collisions GetAllDescendantPropertyAliases collected a descendant's full effective aliases, including whatever it inherits from the type being validated itself. Re-validating a type's own already-effective compositions or properties against its descendants then always found its own aliases reflected back as a false collision, rejecting a plain resave of any composed-and-inherited-from content type - flagged by review on this PR. Fixed at the source: subtract the type's own current CompositionPropertyTypes from the descendant alias set before comparing. ValidateCompositions and ValidateDescendantPropertyAliases keep checking every requested key on every save with no persisted/new split, but now only flag aliases reaching a descendant through some other, genuinely separate path. * Content Types: Stop lowercasing descendant aliases Every caller of GetAllDescendantPropertyAliases already compares case-insensitively (InvariantContains, StringComparer.InvariantCultureIgnoreCase), so lowercasing aliases inside the method was redundant and just obscured the original casing. The set now uses an InvariantCultureIgnoreCase comparer and preserves original casing; comparisons stay case-insensitive everywhere they already were. * Content Types: Add unit tests for the self-collision fix Fast, in-memory unit tests for ContentTypeEditingHelper and ContentTypeServiceExtensions, per review request: GetAllDescendantPropertyAliases excluding a source's own composition/property contribution, and GetAvailableCompositeContentTypes keeping an already-selected composition Allowed when it carries properties and the source is inherited from. * Content Types: Address SonarQube findings on the new test files - ShortStringHelper: static readonly field instead of a get-only property that constructed a new DefaultShortStringHelper on every access (CA1822). - AddPropertyType: narrowed the parameter to ContentType, the concrete type actually passed everywhere (CA1859). - Removed a stray double blank line (SA1507). * Rename GetAllDescendantPropertyAliases to GetPropertyAliasesReservedByDescendants
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/Umbraco.Core/Services/ContentTypeEditing/ContentTypeEditingHelper.cs:54
descendant.CompositionPropertyTypesalready includes the descendant's ownPropertyTypes(it unions them inContentTypeCompositionBase.CompositionPropertyTypes), so concatenatingdescendant.PropertyTypeshere is redundant and causes unnecessary enumeration work when walking large type graphs.
IEnumerable<IPropertyType> allProperties = descendant.PropertyTypes
.Concat(descendant.CompositionPropertyTypes);
foreach (IPropertyType descendantPropertyType in allProperties)
|
lauraneto
left a comment
There was a problem hiding this comment.
Gave it another small test, and all good!
…e-repositories
Conflicts resolved in 8 files. Where the branch had already superseded the
conflicting code, the upstream change was re-expressed rather than dropped:
- Directory.Packages.props: took the incoming bumps; the branch-only
Microsoft.EntityFrameworkCore/.Relational entries follow to 10.0.12 so they
don't sit below the 10.0.12 Sqlite/SqlServer packages (NU1605).
- DictionaryRepository: kept the EF Core implementation and re-applied the new
DictionarySettings.KeySearchMode to its IQueryable filter.
- CompositionReference{DocumentType,MemberType}Controller: added an async
GetComposedOfAsync(int, ComposedOfType) extension so the async content type
services get the composition/inheritance split the sync ones gained.
- MsgPackContentNestedDataSerializerFactory: took the upstream removal of
member handling over the branch's async-ification of it.
- MediaCacheService, SqlServerEFCoreDistributedLockingMechanism and the two
repository test files: kept both sides, porting the incoming tests to the
async repository APIs.
Upstream fixes that landed only in a sync base class whose Async* fork is also
live on this branch, and so were re-applied by hand:
- AsyncContentTypeServiceBase: the composition-change RefreshMain propagation
and reworked raw-data rebuild condition (#23433).
- AsyncContentTypeEditingServiceBase: ValidateDescendantPropertyAliases and the
removal of the already-persisted-composition validation exemption (#23103).
- AsyncContentNavigationServiceBase: TryGetHasChildren/TryGetHasChildrenInBin,
which had silently degraded to the interface default implementation (#23685).
Incoming callers of APIs that are async-only on this branch were ported:
IIdKeyMap.GetKeyForId in DefaultUrlProvider and three test files, and
IPublicAccessService.GetAll in DocumentCollectionPresentationFactoryTests.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>



Description
When a document type has compositions applied and is also inherited from by child document types, the backoffice incorrectly locks its compositions. Opening the parent's composition picker shows:
…even though the parent is only inherited from, not used as a composition elsewhere. The compositions can no longer be added or removed. This blocks a supported modelling pattern (a base type with shared compositions plus several inheriting child types) which works correctly at runtime and in Models Builder — only the editing UI/API locks the user out.
Fixes #23103.
Root cause
Umbraco stores inheritance and composition in the same place — the
IContentTypeComposition.ContentTypeCompositioncollection and thecmsContentType2ContentTypetable — with no discriminator. When a document type is created as a child of another (tree inheritance), the parent is pushed into the child'sContentTypeComposition. The only way to tell the two apart is to compare an entry's id against the type's treeParentId(exactly whatContentTypeMapDefinition.CalculateCompositionTypealready does).Two server-side checks compared only the ids and never the
ParentId, so an inheriting child counted as "using it in a composition":GetComposedOf(id)— behind thecomposition-referencesendpoint that drives the lock message.isUsingguard inGetAvailableCompositeContentTypes— which produces the available-compositions list and backs save-time validation (ValidateCompositions).Changes
All changes are server-side; the composition picker is entirely driven by the server responses, so no client change is required.
Distinguish the two axes when resolving "composed of". Added a
ComposedOfTypeenum (Composition/Inheritance/All) and aContentTypeServiceExtensions.GetComposedOf<TItem>(this IContentTypeBaseService<TItem>, int id, ComposedOfType)extension that filters the existingGetComposedOf(id)result by treeParentId. The threeCompositionReference*Controllers (document/media/member) now requestComposedOfType.Composition, so inheriting children no longer trigger the lock.Fix the availability guard.
GetAvailableCompositeContentTypes'sisUsingcheck now also requiresx.ParentId != sourceId, so a type that is only inherited from is offered compositions and passes save validation. A genuine nested composition is still blocked.Add descendant property-alias validation. Relaxing the guard makes it possible to add a composition/property to a type that already has descendants.
ContentTypeEditingServiceBase.ValidateDescendantPropertyAliasesnow rejects (DuplicatePropertyTypeAlias) any newly-introduced alias that would collide with a property already effective on a descendant (checked against the descendant's fullCompositionPropertyTypes, so collisions via a descendant's own properties or its other compositions are both caught). Only genuinely new aliases are checked, so pre-existing configurations are never retroactively rejected.Refresh inheriting/composing descendants when a composition is added (cache-propagation fix). This is a distinct, pre-existing bug — not caused by the lock fix, but closely related and surfaced while testing it. Adding a composition to a content type did not invalidate the published content type of the types that inherit from or are composed of it:
ContentTypeServiceBase.ComposeContentTypeChangesonly treated property/alias/variation changes and composition removals as a main impact, so a composition addition producedRefreshOtherand skipped theGetComposedOfdescendant propagation. A published document of a descendant type therefore rendered as if the newly-composed property did not exist until the descendant type was itself re-saved. The composition-collection change is now included in the main-impact check, so the edited type and its descendants are refreshed. Because an addition only introduces aliases that have no stored values yet, it is flaggedRawDataUnaffected— refreshing the content-type and converted caches only, without an unnecessary content (cmsContentNu) rebuild. Alias/variation changes, composition removal, and a property removal combined with a composition change still require a full rebuild, as before.Applies to document and media types (both support inheritance); member types share the code path harmlessly.
Why this is safe
isUsingguard and its test predate 2020 and treat inheritance and composition identically ("nested comp is not allowed"). The modern editing service (Content Type inheritance #19034) later added an explicit escape hatch to preserve the "compositions configured before child content types are created" state, andCan_Reapply/Remove_Compositions_For_Content_Type_With_Childrentests exist — i.e. the codebase already treats a parent-with-compositions-and-children as valid. This configuration is already reachable today by adding the composition before creating the children, and works at runtime and in Models Builder. The bug was only that you couldn't reach that supported state by adding the composition after the children exist.ParentIdcomparison already used byCalculateCompositionType.GetComposedOf(int id)— its semantics and signature are untouched; the new filter is an additive extension.Testing
Automated
New/changed tests, each verified to fail before the corresponding fix and pass after (the production change was reverted to confirm the test genuinely catches the bug):
GetComposedOfaxis filtering —ContentTypeServiceTests.GetComposedOf_Distinguishes_Composition_From_Inheritanceasserts the composition axis excludes the inheriting child, the inheritance axis returns only the child, andAllmatches the parameterless overload.ContentTypeServiceExtensionsTests(unit) updated so an inherited-from type is offered compositions; the genuine-nested-composition case is kept as a regression guard.ContentTypeEditingServiceTests.GetAvailableCompositions+ media equivalent cover the end-to-end available list.ContentTypeEditingServiceTests.Update(+ media): a composition can now be added to a type that already has an inheriting child; the previously bug-encodingCannot_Add_Compositions_For_Content_Type_With_Childrenwas repurposed to assert success and that the child still inherits.PublishedContentTypeCacheTests.Published_Child_Type_Reflects_Composition_Added_To_Inherited_Parent_After_Cachingprimes an inheriting child's published content type, adds a composition to the parent, and asserts the child then exposes the composed property (red before the fix, green after).Can_Add_Compositionswas updated to expectRefreshMain | RawDataUnaffected(refresh without a content rebuild).Manual verification
Run each scenario on this branch, then repeat on
v17/devto confirm the old (broken) behaviour and that the change is what makes the difference.Scenario A — compositions are editable on an inherited-from type (the fix)
metaField). Save.Scenario B — descendant alias-collision is rejected (new validation)
shared. Save.shared. Save.Extrawould give the inheriting PageDefault thesharedalias twice).v17/devthis state can't be reached, because step 3's picker is locked per Scenario A.)Scenario C — genuine nested compositions are still blocked (regression check, both branches)
Scenario D — a composition added to an inherited-from type renders immediately (change 4)
This checks the cache-propagation fix end-to-end through the front-end. It needs a template that outputs the properties and a published document of the inheriting type.
Setup (continuing from Scenario A): give PageDefault a template that renders its properties, e.g.
Allow PageDefault under the site root (or Home), create a PageDefault document under it, and publish it — its page renders
Meta field/Sharedbut an emptySeo title(theseoTitleproperty does not exist yet).seoTitle.seoTitle, and publish.seoTitlevalue — no re-save of the PageDefault document type is required.seoTitlerenders empty; it only appears after the PageDefault document type is manually re-saved, because its cached published content type was left stale when the composition was added to BasePage.