Conversation
…s and avoiding descendant traversal when there has been no change to the node's URL segment.
There was a problem hiding this comment.
Pull request overview
This PR improves redirect tracking reliability/performance during publish operations by detecting URL-segment changes (to avoid unnecessary descendant traversal) and fixes invariant-culture URL segment lookups so redirects are correctly created when invariant content is renamed.
Changes:
- Add URL-segment change detection for publishes (with an
isMoveflag to preserve always-traverse semantics for moves) and batch de-duplication during redirect route capture. - Fix invariant-content segment lookups in
DocumentUrlServicewhen culture is""(and related invariant fallbacks). - Fix
DefaultUrlSegmentProviderto respect thepublishedparameter when falling back to the content name, plus add/extend tests.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| uui-range-slider-issue.md | Adds documentation about a uui-range-slider minGap=0 issue (appears unrelated to this PR’s redirect-tracking focus). |
| tests/Umbraco.Tests.UnitTests/Umbraco.Core/Strings/DefaultUrlSegmentProviderTests.cs | New unit coverage for draft vs published name fallback behavior. |
| tests/Umbraco.Tests.UnitTests/Umbraco.Core/Services/DocumentUrlServiceTests.cs | Adds tests for invariant segment resolution when culture is empty or when culture-specific entries are missing. |
| tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Routing/RedirectTrackerTests.cs | Extends integration coverage for publish/move traversal behavior, batch de-duplication, and provider override scenarios. |
| src/Umbraco.Infrastructure/Routing/RedirectTrackingHandler.cs | Passes isMove context into redirect tracking during move vs publish notifications. |
| src/Umbraco.Infrastructure/Routing/RedirectTracker.cs | Implements segment-change gating for publishes and batch de-duplication; wires in URL segment providers + document URL service. |
| src/Umbraco.Core/Strings/IUrlSegmentProvider.cs | Adds HasUrlSegmentChanged default interface method for consistent change detection across providers. |
| src/Umbraco.Core/Strings/DefaultUrlSegmentProvider.cs | Fixes name fallback to honor the published flag (draft vs published segment source). |
| src/Umbraco.Core/Services/DocumentUrlService.cs | Fixes invariant fallback behavior in GetUrlSegment/GetUrlSegments/TryGetPrimaryUrlSegment when culture can’t be resolved. |
| src/Umbraco.Core/Routing/IRedirectTracker.cs | Adds StoreOldRoute(..., isMove) overload and obsoletes the older overload. |
|
We've discussed an an edge case where the segment-unchanged optimization could break custom ScenarioA custom segment provider uses When Proposed SolutionAdded
Custom providers that derive descendant segments from ancestor data (or external sources) can override this, either returning true unconditionally, or using logic (e.g. checking the content type, level or property values) to limit traversal to affected subtrees. The built-in provider returns |
Migaroez
left a comment
There was a problem hiding this comment.
LGTM and covers all the edgecases i can think off.
…nt traversal (#22091) * Optimise redirect tracker by avoiding re-producing of descendant nodes and avoiding descendant traversal when there has been no change to the node's URL segment. * Delete inadvertently added file * Allow URL segment providers to ensure descendent traversal if needed. * Pushed missing files. * Refactors to reduce large method code smells.
Prerequisites
Description
This PR addresses #22082 by optimising the redirect tracker.
As part of that work, I've found and fixed a couple of bugs that would otherwise prevent URL redirects from being created when content is renamed.
Changes
1. Optimisation: Skip descendant traversal when URL segment is unchanged
The main problem I feel is that we are currently checking to create redirects for the node and it's descendants on every save and publish. In the vast majority of these cases, no redirects are needed and so this is wasteful. On the other hand, we can't just simply check if the node name or
umbracoUrlNameproperty has changed, as whilst out of the box that's all that impacts the generated URL segment. URL segment providers are extensible, so we can't know what factors might go into calculating the URL segment.To resolve this I've used the segment providers themselves to find the new segment, compared it to the old, and if they are the same, early return to not carry out the unnecessary traversal of descendent nodes. If the segment is unchanged, no descendant URLs can have changed either, so the entire traversal is skipped.
This is implemented via a new
IUrlSegmentProvider.HasUrlSegmentChangedmethod with a default interface implementation, replacing a long-standing TODO comment. The intention is that this a permanent default implementation, so only providers that need to update this behaviour need to implement it.The
IRedirectTracker.StoreOldRoutemethod has a newbool isMoveoverload so the handler can distinguish publishes from moves — moves will still always traverse descendants since the parent path changes.2. Optimisation: Batch deduplication of already-processed descendants
When multiple content items are published in a batch and a parent's traversal already processed a child, the child's subsequent
StoreOldRoutecall is skipped by checking if its ID is already in theoldRoutesdictionary.3. Bug fix:
DocumentUrlService.GetUrlSegmentreturned null for invariant contentGetUrlSegmentandGetUrlSegmentsreturned null/empty when called with an empty culture string for invariant content. Invariant content is stored with a null language ID in the cache, butTryGetLanguageIdFromCulture("")fails — the code previously returned early at that point. Now it falls through to the invariant (null language ID) cache lookup.TryGetPrimaryUrlSegmenthad the same issue and is fixed identically.4. Bug fix:
DefaultUrlSegmentProviderignoredpublishedparameter in name fallbackWhen content has no
umbracoUrlNameproperty,GetUrlSegmentSourcefalls back to the content name. The fallback had a special case: ifdocument.Editedis true and a published name exists, always return the published name — regardless of thepublishedparameter. This meantGetUrlSegment(content, published: false, culture)returned the old published name instead of the current draft name.This broke redirect tracking:
HasUrlSegmentChangedcompares the currently-published segment against what the segment will be after publishing (draft values), but both sides returned the published name, so no change was ever detected for invariant content.The fix adds
published &&to the condition so the published-name preference only applies whenpublished: trueis requested. Whenpublished: false, the current (draft) name is returned — consistent with how theumbracoUrlNameproperty path already behaved.Testing
Automated
The various unit and integration tests added in this PR, plus existing ones for these classes, should pass.
Manual
Invariant content — rename and publish
/original-name/original-name→/new-name/original-name— should redirect to/new-nameVariant content — rename and publish
Move operation — verify descendants are tracked
/parent/child)No-change publish — verify no unnecessary work
RedirectTracker(line 89) and verify that the method early returns to avoid the unnecessary work.Documentation
We should document
MayAffectDescendantSegmentsonIUrlSegmentProvider.