Content Types: Refresh composing types when a property is added to a composition (closes #24117)

src/Umbraco.Core/Cache/Refreshers/Implement/ContentCacheRefresher.cs

@@ -1,4 +1,4 @@
 using Microsoft.Extensions.DependencyInjection;
 using Umbraco.Cms.Core.DependencyInjection;
 using Umbraco.Cms.Core.Events;
 using Umbraco.Cms.Core.Models;
@@ -38,6 +38,7 @@
     private readonly ICacheManager _cacheManager;
     private readonly IPublishStatusManagementService _publishStatusManagementService;
     private readonly IIdKeyMap _idKeyMap;
+    private readonly IPublishedContentTypeCache _publishedContentTypeCache;
 
     /// <summary>
     /// Initializes a new instance of the <see cref="ContentCacheRefresher"/> class.
@@ -73,29 +74,71 @@
             contentService,
             publishStatusManagementService,
             documentCacheService,
-            cacheManager)
+            cacheManager,
+            StaticServiceProvider.Instance.GetRequiredService<IPublishedContentTypeCache>())
     {
     }
 
     /// <summary>
     /// Initializes a new instance of the <see cref="ContentCacheRefresher"/> class.
     /// </summary>
+    [Obsolete("Please use the constructor taking all parameters. Scheduled for removal in Umbraco 19.")]
     public ContentCacheRefresher(
         AppCaches appCaches,
         IJsonSerializer serializer,
         IIdKeyMap idKeyMap,
         IDomainService domainService,
         IEventAggregator eventAggregator,
         ICacheRefresherNotificationFactory factory,
         IDocumentUrlService documentUrlService,
         IDocumentUrlAliasService documentUrlAliasService,
         IDomainCacheService domainCacheService,
         IDocumentNavigationQueryService documentNavigationQueryService,
         IDocumentNavigationManagementService documentNavigationManagementService,
         IContentService contentService,
         IPublishStatusManagementService publishStatusManagementService,
         IDocumentCacheService documentCacheService,
         ICacheManager cacheManager)
+        : this(
+            appCaches,
+            serializer,
+            idKeyMap,
+            domainService,
+            eventAggregator,
+            factory,
+            documentUrlService,
+            documentUrlAliasService,
+            domainCacheService,
+            documentNavigationQueryService,
+            documentNavigationManagementService,
+            contentService,
+            publishStatusManagementService,
+            documentCacheService,
+            cacheManager,
+            StaticServiceProvider.Instance.GetRequiredService<IPublishedContentTypeCache>())
+    {
+    }
+
+    /// <summary>
+    /// Initializes a new instance of the <see cref="ContentCacheRefresher"/> class.
+    /// </summary>
+    public ContentCacheRefresher(
+        AppCaches appCaches,
+        IJsonSerializer serializer,
+        IIdKeyMap idKeyMap,
+        IDomainService domainService,
+        IEventAggregator eventAggregator,
+        ICacheRefresherNotificationFactory factory,
+        IDocumentUrlService documentUrlService,
+        IDocumentUrlAliasService documentUrlAliasService,
+        IDomainCacheService domainCacheService,
+        IDocumentNavigationQueryService documentNavigationQueryService,
+        IDocumentNavigationManagementService documentNavigationManagementService,
+        IContentService contentService,
+        IPublishStatusManagementService publishStatusManagementService,
+        IDocumentCacheService documentCacheService,
+        ICacheManager cacheManager,
+        IPublishedContentTypeCache publishedContentTypeCache)
         : base(appCaches, serializer, eventAggregator, factory)
     {
         _idKeyMap = idKeyMap;
@@ -108,6 +151,7 @@
         _contentService = contentService;
         _documentCacheService = documentCacheService;
         _publishStatusManagementService = publishStatusManagementService;
+        _publishedContentTypeCache = publishedContentTypeCache;
 
         // TODO: Ideally we should inject IElementsCache
         // this interface is in infrastructure, and changing this is very breaking
@@ -193,6 +237,15 @@
     /// <inheritdoc/>
     public override void Refresh(JsonPayload[] payloads)
     {
+        // A "refresh all" is the signal that the in-memory published caches are being rebuilt from the database
+        // cache. Published content types are cached independently of the content they describe, so they have to
+        // be dropped here too - otherwise the rebuilt content is projected through the same definitions as
+        // before and the reload has no effect on anything the content type governs.
+        if (payloads.Any(x => x.ChangeTypes.HasType(TreeChangeTypes.RefreshAll)))
+        {
+            _publishedContentTypeCache.ClearAll();
+        }
+
         var idsRemoved = new HashSet<int>();
 
         foreach (JsonPayload payload in payloads)

src/Umbraco.Core/Services/ContentTypeServiceBase{TRepository,TItem}.cs

@@ -344,129 +344,146 @@
     {
         // find all content types impacted by the changes,
         // - content type alias changed
-        // - content type property removed, or alias changed
-        // - content type composition removed (not testing if composition had properties...)
+        // - content type property removed, added, or alias changed
+        // - content type composition removed or added (not testing if composition had properties...)
         // - content type variation changed
         // - property type variation changed
         //
-        // because these are the changes that would impact the raw content data
+        // because these are the changes that alter the set of property types a content type exposes,
+        // and so the set exposed by every content type that derives from it
 
         // note
         // this is meant to run *after* uow.Commit() so must use WasPropertyDirty() everywhere
         // instead of IsPropertyDirty() since dirty properties have been reset already
 
         var changes = new List<ContentTypeChange<TItem>>();
 
         // Track which types genuinely need a raw cmsContentNu rebuild vs. which only had a property removed.
         // A type can appear via more than one path (e.g. a batch save touching a composition, where the same
         // type is both saved directly and returned by GetComposedOf as a different instance), so we key these
         // by Id — not entity reference — and resolve the RawDataUnaffected flag once at the end: it is only
         // safe when *nothing* required a rebuild for that Id.
         var rebuildRequiredIds = new HashSet<int>();
         var rawDataUnaffectedCandidateIds = new HashSet<int>();
 
         foreach (TItem contentType in contentTypes)
         {
             var dirty = (IRememberBeingDirty)contentType;
 
             // skip new content types
             // TODO: This used to be WasPropertyDirty("HasIdentity") but i don't think that actually worked for detecting new entities this does seem to work properly
             var isNewContentType = dirty.WasPropertyDirty("Id");
             if (isNewContentType)
             {
                 AddChange(changes, contentType, ContentTypeChangeTypes.Create);
                 continue;
             }
 
             // alias change?
             var hasAliasChanged = dirty.WasPropertyDirty("Alias");
 
             // existing property alias change?
             var hasAnyPropertyChangedAlias = contentType.PropertyTypes.Any(propertyType =>
             {
                 // skip new properties
                 // TODO: This used to be WasPropertyDirty("HasIdentity") but i don't think that actually worked for detecting new entities this does seem to work properly
                 var isNewProperty = propertyType.WasPropertyDirty("Id");
                 if (isNewProperty)
                 {
                     return false;
                 }
 
                 // alias change?
                 return propertyType.WasPropertyDirty("Alias");
             });
 
             // removed properties?
             var hasAnyPropertyBeenRemoved = dirty.WasPropertyDirty("HasPropertyTypeBeenRemoved");
 
             // removed compositions?
             var hasAnyCompositionBeenRemoved = dirty.WasPropertyDirty("HasCompositionTypeBeenRemoved");
 
             // variation changed?
             var hasContentTypeVariationChanged = dirty.WasPropertyDirty("Variations");
 
             // property variation change?
             var hasAnyPropertyVariationChanged = contentType.WasPropertyTypeVariationChanged();
 
             // A composition change dirties the composition collection (add or remove). Both directions change the
             // set of properties this content type - and every type composed of or inheriting from it - exposes, so
             // it is a main-impact change that must refresh this type and its descendants. Whether it also needs a
             // raw cmsContentNu rebuild is decided separately below: an addition on its own does not (its aliases
             // have no stored values yet), but a removal - or an addition that reintroduces a just-removed alias -
             // does. See the rawDataAffected calculation.
             var hasCompositionChanged = dirty.WasPropertyDirty("ContentTypeComposition");
 
+            // added properties?
+            var hasAnyPropertyBeenAdded = contentType.PropertyTypes.Any(propertyType => propertyType.WasPropertyDirty("Id"));
+
             // main impact on properties?
             var hasPropertyMainImpact = hasContentTypeVariationChanged || hasAnyPropertyVariationChanged
                                                                        || hasAnyCompositionBeenRemoved || hasAnyPropertyBeenRemoved || hasAnyPropertyChangedAlias
                                                                        || hasCompositionChanged;
 
+            // The raw cmsContentNu blob is keyed by property alias, so a structural change only requires a
+            // rebuild when it re-keys existing stored values or lets a stale value resolve to a different
+            // property. It does NOT when the only structural change is:
+            //  - a property removal - the orphaned value simply stops resolving and is never read again, or
+            //  - a composition being added - its new aliases have no stored value yet.
+            // In those cases clearing the converted content cache is enough (the content type cache is
+            // refreshed regardless). An alias or variation change, a composition removal, or a property
+            // removal combined with a composition change (which can reintroduce the removed alias behind a
+            // different property type) all re-key or revive stored values and therefore need a rebuild.
+            var rawDataAffected =
+                hasAliasChanged ||
+                hasAnyPropertyChangedAlias ||
+                hasContentTypeVariationChanged ||
+                hasAnyPropertyVariationChanged ||
+                hasAnyCompositionBeenRemoved ||
+                (hasAnyPropertyBeenRemoved && hasCompositionChanged);
+            var rawDataUnaffected = rawDataAffected is false;
+
             if (hasAliasChanged || hasPropertyMainImpact)
             {
-                // The raw cmsContentNu blob is keyed by property alias, so a structural change only requires a
-                // rebuild when it re-keys existing stored values or lets a stale value resolve to a different
-                // property. It does NOT when the only structural change is:
-                //  - a property removal - the orphaned value simply stops resolving and is never read again, or
-                //  - a composition being added - its new aliases have no stored value yet.
-                // In those cases clearing the converted content cache is enough (the content type cache is
-                // refreshed regardless). An alias or variation change, a composition removal, or a property
-                // removal combined with a composition change (which can reintroduce the removed alias behind a
-                // different property type) all re-key or revive stored values and therefore need a rebuild.
-                var rawDataAffected =
-                    hasAliasChanged ||
-                    hasAnyPropertyChangedAlias ||
-                    hasContentTypeVariationChanged ||
-                    hasAnyPropertyVariationChanged ||
-                    hasAnyCompositionBeenRemoved ||
-                    (hasAnyPropertyBeenRemoved && hasCompositionChanged);
-                var rawDataUnaffected = rawDataAffected is false;
-
                 // add that one, as a main change
                 AddChange(changes, contentType, ContentTypeChangeTypes.RefreshMain);
                 (rawDataUnaffected ? rawDataUnaffectedCandidateIds : rebuildRequiredIds).Add(contentType.Id);
 
                 // Add VariationChanged flag if content type variation changed.
                 // This is used by DocumentUrlService to rebuild URL cache with correct languageId.
                 if (hasContentTypeVariationChanged)
                 {
                     AddChange(changes, contentType, ContentTypeChangeTypes.VariationChanged);
                 }
+            }
+            else
+            {
+                // add that one, as an other change
+                AddChange(changes, contentType, ContentTypeChangeTypes.RefreshOther);
+            }
 
-                if (hasPropertyMainImpact)
+            // Any change to the set of property types a content type exposes changes it for every content type
+            // that derives from it, through composition or inheritance, at any depth. Those types resolve their
+            // published content type from the full composed property set, so they have to be refreshed too - or
+            // they keep projecting content through a property set the content type no longer has.
+            if (hasPropertyMainImpact || hasAnyPropertyBeenAdded)
+            {
+                foreach (TItem c in GetComposedOfTransitive(contentType.Id))
                 {
-                    foreach (TItem c in GetComposedOf(contentType.Id))
+                    if (hasPropertyMainImpact)
                     {
-                        // Composing types inherit the same property change, so they share its rebuild requirement.
+                        // Deriving types inherit the same property change, so they share its rebuild requirement.
                         AddChange(changes, c, ContentTypeChangeTypes.RefreshMain);
                         (rawDataUnaffected ? rawDataUnaffectedCandidateIds : rebuildRequiredIds).Add(c.Id);
                     }
+                    else
+                    {
+                        // A pure addition introduces an alias that has no stored value, so the deriving types
+                        // only need their content type and converted content caches cleared, not a rebuild.
+                        AddChange(changes, c, ContentTypeChangeTypes.RefreshOther);
+                    }
                 }
             }
-            else
-            {
-                // add that one, as an other change
-                AddChange(changes, contentType, ContentTypeChangeTypes.RefreshOther);
-            }
         }
 
         // Flag the raw data as unaffected only for types that were never independently marked as needing a
@@ -705,8 +722,44 @@
         return GetComposedOf(id, allContentTypes);
     }
 
+    /// <summary>
+    /// Gets every content type whose effective property set derives from the specified content type, directly
+    /// or indirectly.
+    /// </summary>
+    /// <param name="id">The identifier of the content type.</param>
+    /// <returns>The content types deriving from the specified content type.</returns>
+    /// <remarks>
+    /// Inheritance is stored as composition, so a single traversal of the composition graph covers both axes.
+    /// It has to be transitive because a content type also exposes the property types of its compositions'
+    /// compositions (see <see cref="IContentTypeComposition.CompositionPropertyTypes" />), so a change to one
+    /// content type's property set changes it for everything deriving from it, not just its direct consumers.
+    /// </remarks>
+    private IEnumerable<TItem> GetComposedOfTransitive(int id)
+    {
+        // GetAll is cheap, repository has a full dataset cache policy
+        TItem[] allContentTypes = GetAll().ToArray();
+
+        var composedOf = new Dictionary<int, TItem>();
+        var remaining = new Queue<int>();
+        remaining.Enqueue(id);
+
+        while (remaining.Count > 0)
+        {
+            foreach (TItem contentType in GetComposedOf(remaining.Dequeue(), allContentTypes))
+            {
+                // TryAdd doubles as the guard against cycles in the composition graph
+                if (composedOf.TryAdd(contentType.Id, contentType))
+                {
+                    remaining.Enqueue(contentType.Id);
+                }
+            }
+        }
+
+        return composedOf.Values;
+    }
+
     /// <inheritdoc />
     public int Count()
     {
         using ICoreScope scope = ScopeProvider.CreateCoreScope(autoComplete: true);
         scope.ReadLock(ReadLockIds);

tests/Umbraco.Tests.Integration/Umbraco.Core/Cache/PublishedContentTypeCacheTests.cs

@@ -194,6 +194,63 @@ public async Task Published_Child_Type_Reflects_Composition_Added_To_Inherited_P
             "child published content type should pick up the property composed onto its inherited-from parent");
     }
 
+    [Test]
+    public async Task Published_Composing_Type_Reflects_Property_Added_To_Composition_After_Caching()
+    {
+        // A composition, applied to a composing content type.
+        var composition = (await ContentTypeEditingService.CreateAsync(
+            ContentTypeEditingBuilder.CreateSimpleContentType("composition", "Composition"),
+            Constants.Security.SuperUserKey)).Result!;
+
+        var composingModel = ContentTypeEditingBuilder.CreateBasicContentType("composing", "Composing");
+        composingModel.Compositions = [new Composition { CompositionType = CompositionType.Composition, Key = composition.Key }];
+        var composing = (await ContentTypeEditingService.CreateAsync(composingModel, Constants.Security.SuperUserKey)).Result!;
+
+        // Prime the published content type cache for the composing type; it does not expose "subtitle" yet.
+        var before = PublishedContentTypeCache.Get(PublishedItemType.Content, composing.Key);
+        Assert.IsFalse(before.PropertyTypes.Any(p => p.Alias == "subtitle"), "precondition: composing type should not yet expose 'subtitle'");
+
+        // Add a "subtitle" property to the composition.
+        await AddPropertyAndSaveAsync(composition, "subtitle");
+
+        // Adding the property must invalidate the cached published content type of the composing type, so it
+        // now exposes the property it gains through the composition.
+        var after = PublishedContentTypeCache.Get(PublishedItemType.Content, composing.Key);
+        Assert.IsTrue(
+            after.PropertyTypes.Any(p => p.Alias == "subtitle"),
+            "composing published content type should pick up the property added to its composition");
+    }
+
+    [Test]
+    public async Task Published_Descendant_Type_Reflects_Property_Added_Further_Up_The_Inheritance_Chain_After_Caching()
+    {
+        // Root <- Middle <- Leaf, each inheriting from the one before.
+        var root = (await ContentTypeEditingService.CreateAsync(
+            ContentTypeEditingBuilder.CreateSimpleContentType("root", "Root"),
+            Constants.Security.SuperUserKey)).Result!;
+
+        var middleModel = ContentTypeEditingBuilder.CreateBasicContentType("middle", "Middle");
+        middleModel.Compositions = [new Composition { CompositionType = CompositionType.Inheritance, Key = root.Key }];
+        var middle = (await ContentTypeEditingService.CreateAsync(middleModel, Constants.Security.SuperUserKey)).Result!;
+
+        var leafModel = ContentTypeEditingBuilder.CreateBasicContentType("leaf", "Leaf");
+        leafModel.Compositions = [new Composition { CompositionType = CompositionType.Inheritance, Key = middle.Key }];
+        var leaf = (await ContentTypeEditingService.CreateAsync(leafModel, Constants.Security.SuperUserKey)).Result!;
+
+        // Prime the published content type cache for the leaf; it does not expose "subtitle" yet.
+        var before = PublishedContentTypeCache.Get(PublishedItemType.Content, leaf.Key);
+        Assert.IsFalse(before.PropertyTypes.Any(p => p.Alias == "subtitle"), "precondition: leaf should not yet expose 'subtitle'");
+
+        // Add a "subtitle" property two levels up.
+        await AddPropertyAndSaveAsync(root, "subtitle");
+
+        // The refresh has to reach the whole closure of deriving types, not just the direct one.
+        var after = PublishedContentTypeCache.Get(PublishedItemType.Content, leaf.Key);
+        Assert.IsTrue(
+            after.PropertyTypes.Any(p => p.Alias == "subtitle"),
+            "leaf published content type should pick up the property added to the root of its inheritance chain");
+    }
+
     private async Task<IContentType> CreateElementTypeAsync(string alias)
     {
         ContentTypeCreateModel createModel = ContentTypeEditingBuilder.CreateElementType(alias, alias);

tests/Umbraco.Tests.Integration/Umbraco.Core/Cache/ReloadPublishedCacheTests.cs

@@ -0,0 +1,45 @@
+using NUnit.Framework;
+using Umbraco.Cms.Core.Cache;
+using Umbraco.Cms.Core.DependencyInjection;
+using Umbraco.Cms.Core.Models.PublishedContent;
+using Umbraco.Cms.Core.PublishedCache;
+using Umbraco.Cms.Core.Sync;
+using Umbraco.Cms.Tests.Common.Testing;
+using Umbraco.Cms.Tests.Integration.Testing;
+using Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Services;
+using Umbraco.Extensions;
+
+namespace Umbraco.Cms.Tests.Integration.Umbraco.Core.Cache;
+
+[TestFixture]
+[UmbracoTest(Database = UmbracoTestOptions.Database.NewSchemaPerTest)]
+internal sealed class ReloadPublishedCacheTests : UmbracoIntegrationTestWithContentEditing
+{
+    private IPublishedContentTypeCache PublishedContentTypeCache => GetRequiredService<IPublishedContentTypeCache>();
+
+    private DistributedCache DistributedCache => GetRequiredService<DistributedCache>();
+
+    protected override void CustomTestSetup(IUmbracoBuilder builder)
+    {
+        builder.AddUmbracoHybridCache();
+
+        // The harness registers a no-op server messenger, which would swallow the refresh and let this test
+        // pass without the cache refresher ever running.
+        builder.Services.AddUnique<IServerMessenger, ContentEventsTests.LocalServerMessenger>();
+    }
+
+    [Test]
+    public void Reloading_The_Published_Cache_Clears_The_Published_Content_Type_Cache()
+    {
+        IPublishedContentType primed = PublishedContentTypeCache.Get(PublishedItemType.Content, ContentType.Key);
+        Assert.IsNotNull(primed);
+
+        // This is what the "Reload Memory Cache" backoffice action triggers. Reloading the published caches from
+        // the database cache is only meaningful if the content types the content is projected through are
+        // reloaded too, so the cached published content types have to go as well.
+        DistributedCache.RefreshAllPublishedSnapshot();
+
+        IPublishedContentType after = PublishedContentTypeCache.Get(PublishedItemType.Content, ContentType.Key);
+        Assert.IsFalse(ReferenceEquals(primed, after), "the published content type should have been rebuilt, not re-served from the cache");
+    }
+}