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,11 +38,12 @@
     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.
     /// </summary>
-    [Obsolete("Please use the constructor taking all parameters. Scheduled for removal in Umbraco 19.")]
+    [Obsolete("Please use the constructor also taking IPublishedContentTypeCache. Scheduled for removal in Umbraco 19.")]
     public ContentCacheRefresher(
         AppCaches appCaches,
         IJsonSerializer serializer,
@@ -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 also taking IPublishedContentTypeCache. 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

@@ -1,4 +1,4 @@
 using System.Globalization;
 using System.Runtime.InteropServices;
 using Microsoft.Extensions.DependencyInjection;
 using Microsoft.Extensions.Logging;
@@ -344,129 +344,154 @@
     {
         // 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>>();
 
+        // Shared by every content type in the batch, so a deriving type reached from more than one of them is
+        // the same instance each time, and AddChange merges its changes into a single entry.
+        ILookup<int, TItem>? directReferencingTypes = null;
+
         // 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
+        // type is both saved directly and returned by the composition traversal 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)
+            // A content type that derives from this one - through composition or inheritance, at any depth -
+            // resolves its published content type from this one's alias and property types, so any change to
+            // either has to refresh the whole closure beneath it. Left stale, a deriving type keeps projecting
+            // content through a property set the content type no longer has, and keeps reporting the old alias
+            // from its composition aliases (which IPublishedContent.IsComposedOf reads).
+            if (hasPropertyMainImpact || hasAnyPropertyBeenAdded || hasAliasChanged)
+            {
+                directReferencingTypes ??= GetDirectReferencingTypes();
+                foreach (TItem c in GetComposedOfTransitive(contentType.Id, directReferencingTypes))
                 {
-                    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
+                    {
+                        // Neither a property addition nor a content type alias change re-keys anything the
+                        // deriving types have stored - the added alias has no value yet, and the stored blob is
+                        // keyed by property alias, not content type alias. They 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 +730,65 @@
         return GetComposedOf(id, allContentTypes);
     }
 
+    /// <summary>
+    /// Builds a lookup from each content type id to the content types that directly reference it as a composition
+    /// (or as the content type they inherit from).
+    /// </summary>
+    /// <returns>The lookup of directly referencing content types, keyed by referenced content type id.</returns>
+    /// <remarks>
+    /// Every call takes a fresh snapshot of all content types, and the full dataset cache policy deep clones each
+    /// of them, so build this once per unit of work and share it.
+    /// </remarks>
+    private ILookup<int, TItem> GetDirectReferencingTypes() =>
+        GetAll()
+            .SelectMany(
+                contentType => contentType.ContentTypeComposition,
+                (contentType, referenced) => (ReferencedId: referenced.Id, ContentType: contentType))
+            .ToLookup(x => x.ReferencedId, x => x.ContentType);
+
+    /// <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>
+    /// <param name="directReferencingTypes">
+    /// The content types that directly reference each content type, as built by
+    /// <see cref="GetDirectReferencingTypes" />.
+    /// </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 resolves both its property types and its composition
+    /// aliases recursively through its compositions (see
+    /// <see cref="IContentTypeComposition.CompositionPropertyTypes" /> and
+    /// <see cref="IContentTypeComposition.CompositionAliases()" />), so a change to one content type is a change
+    /// to everything deriving from it, not just to its direct consumers.
+    /// </remarks>
+    private static TItem[] GetComposedOfTransitive(int id, ILookup<int, TItem> directReferencingTypes)
+    {
+        var composedOf = new Dictionary<int, TItem>();
+        var remaining = new Stack<int>();
+        remaining.Push(id);
+
+        while (remaining.Count > 0)
+        {
+            foreach (TItem contentType in directReferencingTypes[remaining.Pop()])
+            {
+                // TryAdd doubles as the guard against cycles in the composition graph
+                if (composedOf.TryAdd(contentType.Id, contentType) is false)
+                {
+                    continue;
+                }
+
+                remaining.Push(contentType.Id);
+            }
+        }
+
+        return [.. composedOf.Values];
+    }
+
     /// <inheritdoc />
     public int Count()
     {
         using ICoreScope scope = ScopeProvider.CreateCoreScope(autoComplete: true);
         scope.ReadLock(ReadLockIds);