Redirect Tracking: Fix segment change detection and optimise descendant traversal (closes #22082)

src/Umbraco.Core/Routing/IRedirectTracker.cs

@@ -12,8 +12,24 @@ public interface IRedirectTracker
         /// </summary>
         /// <param name="entity">The content entity updated.</param>
         /// <param name="oldRoutes">The dictionary of routes for population.</param>
+        [Obsolete("Use the overload accepting all parameters. Scheduled for removal in Umbraco 19.")]
         void StoreOldRoute(IContent entity, Dictionary<(int ContentId, string Culture), (Guid ContentKey, string OldRoute)> oldRoutes);
 
+        /// <summary>
+        /// Stores the existing routes for a content item before update, with context about whether this is a move operation.
+        /// </summary>
+        /// <param name="entity">The content entity updated.</param>
+        /// <param name="oldRoutes">The dictionary of routes for population.</param>
+        /// <param name="isMove">Whether this is a move operation (always traverses descendants) or a publish (skips if URL segment unchanged).</param>
+        // TODO (V19): Remove the default implementation when the obsolete overload is removed.
+        void StoreOldRoute(
+            IContent entity,
+            Dictionary<(int ContentId, string Culture), (Guid ContentKey, string OldRoute)> oldRoutes,
+            bool isMove)
+#pragma warning disable CS0618 // Type or member is obsolete
+            => StoreOldRoute(entity, oldRoutes);
+#pragma warning restore CS0618 // Type or member is obsolete
+
         /// <summary>
         /// Creates appropriate redirects for the content item following an update.
         /// </summary>

src/Umbraco.Core/Services/DocumentUrlService.cs

@@ -442,21 +442,20 @@ private long CalculateCacheMemoryUsage()
     public string? GetUrlSegment(Guid documentKey, string culture, bool isDraft)
     {
         ThrowIfNotInitialized();
-        if (TryGetLanguageIdFromCulture(culture, out int languageId) is false)
-        {
-            return null;
-        }
-
-        // Try culture-specific lookup first
-        UrlCacheKey cacheKey = CreateCacheKey(documentKey, languageId, isDraft);
-        if (_documentUrlCache.TryGetValue(cacheKey, out UrlSegmentCache? cache))
+        if (TryGetLanguageIdFromCulture(culture, out int languageId))
         {
-            return cache.PrimarySegment;
+            // Try culture-specific lookup first.
+            UrlCacheKey cacheKey = CreateCacheKey(documentKey, languageId, isDraft);
+            if (_documentUrlCache.TryGetValue(cacheKey, out UrlSegmentCache? cache))
+            {
+                return cache.PrimarySegment;
+            }
         }
 
-        // Try invariant lookup (NULL languageId) - for invariant content that stores with NULL.
+        // Try invariant lookup (NULL languageId) - for invariant content that stores with NULL,
+        // or when the culture couldn't be resolved to a language ID (e.g. empty string for invariant content).
         UrlCacheKey invariantKey = CreateCacheKey(documentKey, null, isDraft);
-        return _documentUrlCache.TryGetValue(invariantKey, out cache) ? cache.PrimarySegment : null;
+        return _documentUrlCache.TryGetValue(invariantKey, out UrlSegmentCache? invariantCache) ? invariantCache.PrimarySegment : null;
     }
 
     private bool TryGetLanguageIdFromCulture(string culture, out int languageId)
@@ -483,22 +482,21 @@ private bool TryGetLanguageIdFromCulture(string culture, out int languageId)
     public IEnumerable<string> GetUrlSegments(Guid documentKey, string culture, bool isDraft)
     {
         ThrowIfNotInitialized();
-        if (TryGetLanguageIdFromCulture(culture, out int languageId) is false)
-        {
-            return Enumerable.Empty<string>();
-        }
-
-        // Try culture-specific lookup first.
-        UrlCacheKey cacheKey = CreateCacheKey(documentKey, languageId, isDraft);
-        if (_documentUrlCache.TryGetValue(cacheKey, out UrlSegmentCache? cache))
+        if (TryGetLanguageIdFromCulture(culture, out int languageId))
         {
-            return cache.GetAllSegments();
+            // Try culture-specific lookup first.
+            UrlCacheKey cacheKey = CreateCacheKey(documentKey, languageId, isDraft);
+            if (_documentUrlCache.TryGetValue(cacheKey, out UrlSegmentCache? cache))
+            {
+                return cache.GetAllSegments();
+            }
         }
 
-        // Try invariant lookup (NULL languageId) - for invariant content that stores with NULL.
+        // Try invariant lookup (NULL languageId) - for invariant content that stores with NULL,
+        // or when the culture couldn't be resolved to a language ID (e.g. empty string for invariant content).
         UrlCacheKey invariantKey = CreateCacheKey(documentKey, null, isDraft);
-        return _documentUrlCache.TryGetValue(invariantKey, out cache)
-            ? cache.GetAllSegments()
+        return _documentUrlCache.TryGetValue(invariantKey, out UrlSegmentCache? invariantCache)
+            ? invariantCache.GetAllSegments()
             : Enumerable.Empty<string>();
     }
 
@@ -1218,13 +1216,14 @@ private bool TryGetPrimaryUrlSegment(Guid documentKey, string culture, bool isDr
                 segment = cache.PrimarySegment;
                 return true;
             }
+        }
 
-            // Try invariant lookup (NULL languageId) - for invariant content that stores with NULL.
-            if (_documentUrlCache.TryGetValue(CreateCacheKey(documentKey, null, isDraft), out cache))
-            {
-                segment = cache.PrimarySegment;
-                return true;
-            }
+        // Try invariant lookup (NULL languageId) - for invariant content that stores with NULL,
+        // or when the culture couldn't be resolved to a language ID (e.g. empty string for invariant content).
+        if (_documentUrlCache.TryGetValue(CreateCacheKey(documentKey, null, isDraft), out UrlSegmentCache? invariantCache))
+        {
+            segment = invariantCache.PrimarySegment;
+            return true;
         }
 
         segment = null;

src/Umbraco.Core/Strings/DefaultUrlSegmentProvider.cs

@@ -57,9 +57,12 @@ public class DefaultUrlSegmentProvider : IUrlSegmentProvider
 
         if (string.IsNullOrWhiteSpace(source))
         {
-            // If the name of a node has been updated, but it has not been published, the url should use the published name, not the current node name
-            // If this node has never been published (GetPublishName is null), use the unpublished name
-            source = content is IContent document && document.Edited && document.GetPublishName(culture) != null
+            // When the published segment is requested and the name has been updated but not yet published,
+            // use the published name so that the current live URL is returned (not the pending draft name).
+            // When the draft segment is requested (published: false), use the current name so callers
+            // (e.g. redirect tracking) can determine what the segment *will* be after publishing.
+            // If this node has never been published (GetPublishName is null), use the unpublished name.
+            source = content is IContent document && published && document.Edited && document.GetPublishName(culture) != null
                 ? document.GetPublishName(culture)
                 : content.GetCultureName(culture);
         }

src/Umbraco.Core/Strings/IUrlSegmentProvider.cs

@@ -45,7 +45,46 @@ public interface IUrlSegmentProvider
     /// </remarks>
     string? GetUrlSegment(IContentBase content, bool published, string? culture = null) => GetUrlSegment(content, culture);
 
-    // TODO: For the 301 tracking, we need to add another extended interface to this so that
-    // the RedirectTrackingEventHandler can ask the IUrlSegmentProvider if the URL is changing.
-    // Currently the way it works is very hacky, see notes in: RedirectTrackingEventHandler.ContentService_Publishing
+    /// <summary>
+    /// Determines whether the URL segment for the given content has changed compared to the
+    /// currently published segment. Used by redirect tracking to avoid unnecessary descendant
+    /// traversal when URL segments haven't changed.
+    /// </summary>
+    /// <param name="content">The content being published (carries new property values).</param>
+    /// <param name="currentPublishedSegment">The currently published URL segment (from IDocumentUrlService).</param>
+    /// <param name="culture">The culture.</param>
+    /// <returns>True if the segment has changed, false otherwise.</returns>
+    /// <remarks>
+    /// The default implementation computes the new URL segment via <see cref="GetUrlSegment(IContentBase, bool, string?)"/>
+    /// using draft values (<c>published: false</c>) and compares it to <paramref name="currentPublishedSegment"/>.
+    /// Draft values are used because this method is called during publishing, before the new values are committed
+    /// as published — so the draft values represent what the segment <em>will</em> be after publishing.
+    /// This is intentionally a permanent default so that custom providers automatically get correct change detection
+    /// without additional implementation.
+    /// Override only if you need custom change detection logic (e.g., URL segments derived from external state).
+    /// </remarks>
+    bool HasUrlSegmentChanged(IContentBase content, string? currentPublishedSegment, string? culture)
+        => !string.Equals(
+            GetUrlSegment(content, published: false, culture),
+            currentPublishedSegment,
+            StringComparison.OrdinalIgnoreCase);
+
+    /// <summary>
+    /// Determines whether changes to the given content item may affect URL segments of its
+    /// descendant content items. Used by redirect tracking to decide whether descendant
+    /// traversal can be skipped when the content item's own URL segment is unchanged.
+    /// </summary>
+    /// <param name="content">The content item being published.</param>
+    /// <returns>
+    /// <c>true</c> if this provider may compute descendant segments based on data from this
+    /// content item; <c>false</c> if this provider only uses each content item's own data.
+    /// </returns>
+    /// <remarks>
+    /// The default is <c>false</c>, meaning this provider derives segments solely from the
+    /// content item itself (e.g. its Name or properties). Custom providers that read ancestor
+    /// properties to compute descendant segments should override this — either returning
+    /// <c>true</c> unconditionally, or using logic (e.g. checking the content type or whether
+    /// relevant properties have changed) to limit the impact to affected subtrees.
+    /// </remarks>
+    bool MayAffectDescendantSegments(IContentBase content) => false;
 }

src/Umbraco.Infrastructure/Routing/RedirectTracker.cs

@@ -1,4 +1,4 @@
 using System.Diagnostics.CodeAnalysis;
 using Microsoft.Extensions.Logging;
 using Umbraco.Cms.Core;
 using Umbraco.Cms.Core.Extensions;
@@ -8,6 +8,7 @@
 using Umbraco.Cms.Core.Routing;
 using Umbraco.Cms.Core.Services;
 using Umbraco.Cms.Core.Services.Navigation;
+using Umbraco.Cms.Core.Strings;
 using Umbraco.Extensions;
 
 namespace Umbraco.Cms.Infrastructure.Routing;
@@ -28,6 +29,8 @@
     private readonly IPublishedUrlProvider _publishedUrlProvider;
     private readonly IPublishedContentStatusFilteringService _publishedContentStatusFilteringService;
     private readonly IDomainCache _domainCache;
+    private readonly UrlSegmentProviderCollection _urlSegmentProviders;
+    private readonly IDocumentUrlService _documentUrlService;
 
     /// <summary>
     /// Initializes a new instance of the <see cref="RedirectTracker"/> class.
@@ -40,7 +43,9 @@
         ILogger<RedirectTracker> logger,
         IPublishedUrlProvider publishedUrlProvider,
         IPublishedContentStatusFilteringService publishedContentStatusFilteringService,
-        IDomainCache domainCache)
+        IDomainCache domainCache,
+        UrlSegmentProviderCollection urlSegmentProviders,
+        IDocumentUrlService documentUrlService)
     {
         _languageService = languageService;
         _redirectUrlService = redirectUrlService;
@@ -50,29 +55,58 @@
         _publishedUrlProvider = publishedUrlProvider;
         _publishedContentStatusFilteringService = publishedContentStatusFilteringService;
         _domainCache = domainCache;
+        _urlSegmentProviders = urlSegmentProviders;
+        _documentUrlService = documentUrlService;
     }
 
     /// <inheritdoc/>
+#pragma warning disable CS0618 // Type or member is obsolete
     public void StoreOldRoute(IContent entity, Dictionary<(int ContentId, string Culture), (Guid ContentKey, string OldRoute)> oldRoutes)
+#pragma warning restore CS0618 // Type or member is obsolete
+        => StoreOldRoute(entity, oldRoutes, isMove: true);
+
+    /// <inheritdoc/>
+    public void StoreOldRoute(
+        IContent entity,
+        Dictionary<(int ContentId, string Culture), (Guid ContentKey, string OldRoute)> oldRoutes,
+        bool isMove)
     {
         IPublishedContent? entityContent = _contentCache.GetById(entity.Id);
         if (entityContent is null)
         {
             return;
         }
 
+        // If this entity was already processed by an ancestor's traversal in this batch,
+        // all its descendants will also have been processed — skip entirely to avoid redundant
+        // cache lookups, segment checks, and navigation queries.
+        if (oldRoutes.Keys.Any(k => k.ContentId == entityContent.Id))
+        {
+            return;
+        }
+
+        // For publishes (not moves), check if URL segment actually changed and whether any provider
+        // derives descendant segments from this content's data.
+        // If the segment is unchanged and no provider affects descendants, we don't need to traverse.
+        // For moves, we have to assume all descendant URLs may have changed since the parent path is part of the URL.
+        if (ShouldIgnoreForOldRouteStorage(entity, isMove, entityContent))
+        {
+            return;
+        }
+
         // Get the default affected cultures by going up the tree until we find the first culture variant entity (default to no cultures).
         var defaultCultures = new Lazy<string[]>(() => entityContent.AncestorsOrSelf(_navigationQueryService, _publishedContentStatusFilteringService)
             .FirstOrDefault(a => a.Cultures.Any())?.Cultures.Keys.ToArray() ?? []);
 
         // Get the domains assigned to the content item again by looking up the tree (default to 0).
         var domainRootId = new Lazy<int>(() => GetNodeIdWithAssignedDomain(entityContent));
 
         // Get all language ISO codes (in case we're dealing with invariant content with variant ancestors)
         var languageIsoCodes = new Lazy<string[]>(() => [.. _languageService.GetAllIsoCodesAsync().GetAwaiter().GetResult()]);
 
         foreach (IPublishedContent publishedContent in entityContent.DescendantsOrSelf(_navigationQueryService, _publishedContentStatusFilteringService))
         {
+
             // If this entity defines specific cultures, use those instead of the default ones
             IEnumerable<string> cultures = publishedContent.Cultures.Any() ? publishedContent.Cultures.Keys : defaultCultures.Value;
 
@@ -106,6 +140,82 @@
         }
     }
 
+    private bool ShouldIgnoreForOldRouteStorage(IContent entity, bool isMove, IPublishedContent entityContent) =>
+        isMove is false &&
+            HasUrlSegmentChanged(entity, entityContent) is false &&
+            HasProviderAffectingDescendantSegments(entity) is false;
+
+    private bool HasUrlSegmentChanged(IContent entity, IPublishedContent publishedContent)
+    {
+        // During upgrades, the document URL service is not initialized (see DocumentUrlServiceInitializerNotificationHandler).
+        // If a migration triggers content publishing before initialization, fall back to full traversal.
+        if (_documentUrlService.IsInitialized is false)
+        {
+            return true;
+        }
+
+        foreach (var culture in GetCultures(publishedContent))
+        {
+            var currentPublishedSegment = _documentUrlService.GetUrlSegment(entity.Key, culture, isDraft: false);
+
+            // In the unexpected case that the current published segment couldn't be retrieved (e.g. cache inconsistency),
+            // we can't confirm the segment is unchanged — fall back to full traversal.
+            // Otherwise, if the provider(s) that contribute to the segment detect a change, we need to traverse since the
+            // URL of the current node and all descendents has changed.
+            if (currentPublishedSegment is null || HasProviderDetectedSegmentChange(entity, currentPublishedSegment, culture))
+            {
+                return true;
+            }
+        }
+
+        return false;
+    }
+
+    private static IEnumerable<string> GetCultures(IPublishedContent publishedContent) =>
+        publishedContent.Cultures.Any()
+            ? publishedContent.Cultures.Keys
+            : [string.Empty];
+
+    private bool HasProviderDetectedSegmentChange(IContent entity, string currentPublishedSegment, string culture)
+    {
+        // Check each provider to see if any detect a change in the URL segment for this content and culture.
+        foreach (IUrlSegmentProvider provider in _urlSegmentProviders)
+        {
+            // Skip providers that don't produce a segment for this content/culture.
+            if (string.IsNullOrEmpty(provider.GetUrlSegment(entity, published: false, culture)))
+            {
+                continue;
+            }
+
+            if (provider.HasUrlSegmentChanged(entity, currentPublishedSegment, culture))
+            {
+                return true;
+            }
+
+            // This provider handled the segment — don't check further providers unless it allows additional segments.
+            if (provider.AllowAdditionalSegments is false)
+            {
+                return false;
+            }
+        }
+
+        // No provider produced a segment, so none would have at publish time either — no change.
+        return false;
+    }
+
+    private bool HasProviderAffectingDescendantSegments(IContent entity)
+    {
+        foreach (IUrlSegmentProvider provider in _urlSegmentProviders)
+        {
+            if (provider.MayAffectDescendantSegments(entity))
+            {
+                return true;
+            }
+        }
+
+        return false;
+    }
+
     private bool TryGetNodeIdWithAssignedDomain(IPublishedContent entityContent, out int domainRootId)
     {
         domainRootId = GetNodeIdWithAssignedDomain(entityContent);

src/Umbraco.Infrastructure/Routing/RedirectTrackingHandler.cs

@@ -54,7 +54,7 @@ public RedirectTrackingHandler(
     /// </summary>
     /// <param name="notification">The notification containing information about the moved content.</param>
     public void Handle(ContentMovingNotification notification) =>
-        StoreOldRoutes(notification.MoveInfoCollection.Select(m => m.Entity), notification);
+        StoreOldRoutes(notification.MoveInfoCollection.Select(m => m.Entity), notification, isMove: true);
 
     /// <summary>
     /// Handles a <see cref="ContentPublishedNotification"/> to track and manage redirects when content is published.
@@ -67,9 +67,9 @@ public void Handle(ContentMovingNotification notification) =>
     /// </summary>
     /// <param name="notification">The content moved notification.</param>
     public void Handle(ContentPublishingNotification notification) =>
-        StoreOldRoutes(notification.PublishedEntities, notification);
+        StoreOldRoutes(notification.PublishedEntities, notification, isMove: false);
 
-    private void StoreOldRoutes(IEnumerable<IContent> entities, IStatefulNotification notification)
+    private void StoreOldRoutes(IEnumerable<IContent> entities, IStatefulNotification notification, bool isMove)
     {
         // Don't let the notification handlers kick in if redirect tracking is turned off in the config.
         if (_webRoutingSettings.CurrentValue.DisableRedirectUrlTracking)
@@ -80,7 +80,7 @@ private void StoreOldRoutes(IEnumerable<IContent> entities, IStatefulNotificatio
         Dictionary<(int ContentId, string Culture), (Guid ContentKey, string OldRoute)> oldRoutes = GetOldRoutes(notification);
         foreach (IContent entity in entities)
         {
-            _redirectTracker.StoreOldRoute(entity, oldRoutes);
+            _redirectTracker.StoreOldRoute(entity, oldRoutes, isMove);
         }
     }