From 75dd9fab2b0c1c921bec5b3971d313c4f32a8489 Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Thu, 20 Nov 2025 08:30:38 +0100 Subject: [PATCH] Redirect tracking: Ensure redirects with domains are stored with the domain node id prefix (closes #20894) (#20900) * Ensure redirects with domains are stored with the domain node id prefix. * Handle removal of self-referencing redirect when domains are used. * Use entity path to save further queries for retrieving ancestor IDs. * Apply suggestions from code review Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Applied refactoring suggested in code review. --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- .../Routing/RedirectTracker.cs | 68 +++++++++-- .../Routing/RedirectTrackerTests.cs | 109 ++++++++++++------ 2 files changed, 134 insertions(+), 43 deletions(-) diff --git a/src/Umbraco.Infrastructure/Routing/RedirectTracker.cs b/src/Umbraco.Infrastructure/Routing/RedirectTracker.cs index 121014104a..c05a9b2b00 100644 --- a/src/Umbraco.Infrastructure/Routing/RedirectTracker.cs +++ b/src/Umbraco.Infrastructure/Routing/RedirectTracker.cs @@ -3,6 +3,7 @@ using Microsoft.Extensions.Logging; using Umbraco.Cms.Core; using Umbraco.Cms.Core.Extensions; using Umbraco.Cms.Core.Models; +using Umbraco.Cms.Core.Models.Entities; using Umbraco.Cms.Core.Models.PublishedContent; using Umbraco.Cms.Core.PublishedCache; using Umbraco.Cms.Core.Routing; @@ -25,6 +26,7 @@ internal sealed class RedirectTracker : IRedirectTracker private readonly ILogger _logger; private readonly IPublishedUrlProvider _publishedUrlProvider; private readonly IPublishedContentStatusFilteringService _publishedContentStatusFilteringService; + private readonly IDomainCache _domainCache; /// /// Initializes a new instance of the class. @@ -36,7 +38,8 @@ internal sealed class RedirectTracker : IRedirectTracker IDocumentNavigationQueryService navigationQueryService, ILogger logger, IPublishedUrlProvider publishedUrlProvider, - IPublishedContentStatusFilteringService publishedContentStatusFilteringService) + IPublishedContentStatusFilteringService publishedContentStatusFilteringService, + IDomainCache domainCache) { _languageService = languageService; _redirectUrlService = redirectUrlService; @@ -45,6 +48,7 @@ internal sealed class RedirectTracker : IRedirectTracker _logger = logger; _publishedUrlProvider = publishedUrlProvider; _publishedContentStatusFilteringService = publishedContentStatusFilteringService; + _domainCache = domainCache; } /// @@ -56,11 +60,15 @@ internal sealed class RedirectTracker : IRedirectTracker 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(() => entityContent.AncestorsOrSelf(_navigationQueryService, _publishedContentStatusFilteringService).FirstOrDefault(a => a.Cultures.Any())?.Cultures.Keys.ToArray() ?? Array.Empty()); + // 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(() => 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(() => GetNodeIdWithAssignedDomain(entityContent)); // Get all language ISO codes (in case we're dealing with invariant content with variant ancestors) - var languageIsoCodes = new Lazy(() => _languageService.GetAllIsoCodesAsync().GetAwaiter().GetResult().ToArray()); + var languageIsoCodes = new Lazy(() => [.. _languageService.GetAllIsoCodesAsync().GetAwaiter().GetResult()]); foreach (IPublishedContent publishedContent in entityContent.DescendantsOrSelf(_navigationQueryService, _publishedContentStatusFilteringService)) { @@ -71,21 +79,20 @@ internal sealed class RedirectTracker : IRedirectTracker { try { - var route = _publishedUrlProvider.GetUrl(publishedContent.Id, UrlMode.Relative, culture).TrimEnd(Constants.CharArrays.ForwardSlash); - + var route = _publishedUrlProvider.GetUrl(publishedContent.Key, UrlMode.Relative, culture).TrimEnd(Constants.CharArrays.ForwardSlash); if (IsValidRoute(route)) { - oldRoutes[(publishedContent.Id, culture)] = (publishedContent.Key, route); + StoreRoute(oldRoutes, publishedContent, culture, route, domainRootId.Value); } else if (string.IsNullOrEmpty(culture)) { // Retry using all languages, if this is invariant but has a variant ancestor. foreach (string languageIsoCode in languageIsoCodes.Value) { - route = _publishedUrlProvider.GetUrl(publishedContent.Id, UrlMode.Relative, languageIsoCode).TrimEnd(Constants.CharArrays.ForwardSlash); + route = GetUrl(publishedContent.Key, languageIsoCode); if (IsValidRoute(route)) { - oldRoutes[(publishedContent.Id, languageIsoCode)] = (publishedContent.Key, route); + StoreRoute(oldRoutes, publishedContent, languageIsoCode, route, domainRootId.Value); } } } @@ -98,6 +105,35 @@ internal sealed class RedirectTracker : IRedirectTracker } } + private bool TryGetNodeIdWithAssignedDomain(IPublishedContent entityContent, out int domainRootId) + { + domainRootId = GetNodeIdWithAssignedDomain(entityContent); + return domainRootId > 0; + } + + private int GetNodeIdWithAssignedDomain(IPublishedContent entityContent) => + entityContent.Path.Split(',').Select(int.Parse).Reverse() + .FirstOrDefault(x => _domainCache.HasAssigned(x, includeWildcards: true)); + + private string GetUrl(Guid contentKey, string languageIsoCode) => + _publishedUrlProvider.GetUrl(contentKey, UrlMode.Relative, languageIsoCode).TrimEnd(Constants.CharArrays.ForwardSlash); + + private static void StoreRoute( + Dictionary<(int ContentId, string Culture), (Guid ContentKey, string OldRoute)> oldRoutes, + IPublishedContent publishedContent, + string culture, + string route, + int domainRootId) + { + // Prepend the Id of the node with the associated domain to the route if there is one assigned. + if (domainRootId > 0) + { + route = domainRootId + route; + } + + oldRoutes[(publishedContent.Id, culture)] = (publishedContent.Key, route); + } + /// public void CreateRedirects(IDictionary<(int ContentId, string Culture), (Guid ContentKey, string OldRoute)> oldRoutes) { @@ -108,9 +144,21 @@ internal sealed class RedirectTracker : IRedirectTracker foreach (((int contentId, string culture), (Guid contentKey, string oldRoute)) in oldRoutes) { + IPublishedContent? entityContent = _contentCache.GetById(contentKey); + if (entityContent is null) + { + continue; + } + try { - var newRoute = _publishedUrlProvider.GetUrl(contentKey, UrlMode.Relative, culture).TrimEnd(Constants.CharArrays.ForwardSlash); + var newRoute = GetUrl(contentKey, culture); + + // Prepend the Id of the node with the associated domain to the route if there is one assigned. + if (TryGetNodeIdWithAssignedDomain(entityContent, out int domainRootId)) + { + newRoute = domainRootId + newRoute; + } if (!IsValidRoute(newRoute) || oldRoute == newRoute) { diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Routing/RedirectTrackerTests.cs b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Routing/RedirectTrackerTests.cs index 2716cee899..a92f64d637 100644 --- a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Routing/RedirectTrackerTests.cs +++ b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Routing/RedirectTrackerTests.cs @@ -23,40 +23,47 @@ public class RedirectTrackerTests : UmbracoIntegrationTestWithContent { private IRedirectUrlService RedirectUrlService => GetRequiredService(); + private IContent _rootPage; private IContent _testPage; - private const string Url = "RedirectUrl"; - public override void CreateTestData() { base.CreateTestData(); - using var scope = ScopeProvider.CreateScope(); - var repository = CreateRedirectUrlRepository(); var rootContent = ContentService.GetRootContent().First(); + _rootPage = rootContent; var subPages = ContentService.GetPagedChildren(rootContent.Id, 0, 3, out _).ToList(); _testPage = subPages[0]; - - repository.Save(new RedirectUrl { ContentKey = _testPage.Key, Url = Url, Culture = "en" }); - - scope.Complete(); } [Test] public void Can_Store_Old_Route() { - Dictionary<(int ContentId, string Culture), (Guid ContentKey, string OldRoute)> dict = - new Dictionary<(int ContentId, string Culture), (Guid ContentKey, string OldRoute)> - { - [(_testPage.Id, "en")] = (_testPage.Key, "/old-route"), - }; + Dictionary<(int ContentId, string Culture), (Guid ContentKey, string OldRoute)> dict = []; var redirectTracker = CreateRedirectTracker(); redirectTracker.StoreOldRoute(_testPage, dict); - Assert.That(dict.Count, Is.EqualTo(1)); - Assert.AreEqual(dict.Values.First().OldRoute, Url); + Assert.AreEqual(1, dict.Count); + var key = dict.Keys.First(); + Assert.AreEqual(_testPage.Key, dict[key].ContentKey); + Assert.AreEqual("/new-route", dict[key].OldRoute); + } + + [Test] + public void Can_Store_Old_Route_With_Domain_Root_Prefix() + { + Dictionary<(int ContentId, string Culture), (Guid ContentKey, string OldRoute)> dict = []; + + var redirectTracker = CreateRedirectTracker(assignDomain: true); + + redirectTracker.StoreOldRoute(_testPage, dict); + + Assert.AreEqual(1, dict.Count); + var key = dict.Keys.First(); + Assert.AreEqual(_testPage.Key, dict[key].ContentKey); + Assert.AreEqual($"{_rootPage.Id}/new-route", dict[key].OldRoute); } [Test] @@ -72,30 +79,32 @@ public class RedirectTrackerTests : UmbracoIntegrationTestWithContent redirectTracker.CreateRedirects(dict); var redirects = RedirectUrlService.GetContentRedirectUrls(_testPage.Key); - - Assert.IsTrue(redirects.Any(x => x.Url == "/old-route")); + Assert.AreEqual(1, redirects.Count()); + var redirect = redirects.First(); + Assert.AreEqual("/old-route", redirect.Url); } [Test] - public void Removes_Self_Referncing_Redirects() + public void Will_Remove_Self_Referencing_Redirects() { - const string newUrl = "newUrl"; + CreateExistingRedirect(); var redirects = RedirectUrlService.GetContentRedirectUrls(_testPage.Key); - Assert.IsTrue(redirects.Any(x => x.Url == Url)); // Ensure self referencing redirect exists. + Assert.IsTrue(redirects.Any(x => x.Url == "/new-route")); // Ensure self referencing redirect exists. IDictionary<(int ContentId, string Culture), (Guid ContentKey, string OldRoute)> dict = new Dictionary<(int ContentId, string Culture), (Guid ContentKey, string OldRoute)> { - [(_testPage.Id, "en")] = (_testPage.Key, newUrl), + [(_testPage.Id, "en")] = (_testPage.Key, "/old-route"), }; var redirectTracker = CreateRedirectTracker(); redirectTracker.CreateRedirects(dict); - redirects = RedirectUrlService.GetContentRedirectUrls(_testPage.Key); - Assert.IsFalse(redirects.Any(x => x.Url == Url)); - Assert.IsTrue(redirects.Any(x => x.Url == newUrl)); + redirects = RedirectUrlService.GetContentRedirectUrls(_testPage.Key); + Assert.AreEqual(1, redirects.Count()); + var redirect = redirects.First(); + Assert.AreEqual("/old-route", redirect.Url); } private RedirectUrlRepository CreateRedirectUrlRepository() => @@ -106,7 +115,7 @@ public class RedirectTrackerTests : UmbracoIntegrationTestWithContent Mock.Of(), Mock.Of()); - private IRedirectTracker CreateRedirectTracker() + private IRedirectTracker CreateRedirectTracker(bool assignDomain = false) { var contentType = new Mock(); contentType.SetupGet(c => c.Variations).Returns(ContentVariation.Nothing); @@ -116,34 +125,68 @@ public class RedirectTrackerTests : UmbracoIntegrationTestWithContent { "en", new PublishedCultureInfo("en", "english", "/en/", DateTime.UtcNow) }, }; - var content = new Mock(); + var rootContent = new Mock(); + rootContent.SetupGet(c => c.Id).Returns(_rootPage.Id); + rootContent.SetupGet(c => c.Key).Returns(_rootPage.Key); + rootContent.SetupGet(c => c.Name).Returns(_rootPage.Name); + rootContent.SetupGet(c => c.Path).Returns(_rootPage.Path); + var content = new Mock(); + content.SetupGet(c => c.Id).Returns(_testPage.Id); content.SetupGet(c => c.Key).Returns(_testPage.Key); + content.SetupGet(c => c.Name).Returns(_testPage.Name); + content.SetupGet(c => c.Path).Returns(_testPage.Path); content.SetupGet(c => c.ContentType).Returns(contentType.Object); content.SetupGet(c => c.Cultures).Returns(cultures); - content.SetupGet(c => c.Id).Returns(_testPage.Id); IPublishedContentCache contentCache = Mock.Of(); Mock.Get(contentCache) .Setup(x => x.GetById(_testPage.Id)) .Returns(content.Object); + Mock.Get(contentCache) + .Setup(x => x.GetById(_testPage.Key)) + .Returns(content.Object); IPublishedUrlProvider publishedUrlProvider = Mock.Of(); Mock.Get(publishedUrlProvider) .Setup(x => x.GetUrl(_testPage.Key, UrlMode.Relative, "en", null)) - .Returns(Url); + .Returns("/new-route"); - Mock.Get(publishedUrlProvider) - .Setup(x => x.GetUrl(_testPage.Id, UrlMode.Relative, "en", null)) - .Returns(Url); + IDocumentNavigationQueryService documentNavigationQueryService = Mock.Of(); + IEnumerable ancestorKeys = [_rootPage.Key]; + Mock.Get(documentNavigationQueryService) + .Setup(x => x.TryGetAncestorsKeys(_testPage.Key, out ancestorKeys)) + .Returns(true); + + IPublishedContentStatusFilteringService publishedContentStatusFilteringService = Mock.Of(); + Mock.Get(publishedContentStatusFilteringService) + .Setup(x => x.FilterAvailable(It.IsAny>(), It.IsAny())) + .Returns([rootContent.Object]); + + IDomainCache domainCache = Mock.Of(); + Mock.Get(domainCache) + .Setup(x => x.HasAssigned(_testPage.Id, It.IsAny())) + .Returns(false); + Mock.Get(domainCache) + .Setup(x => x.HasAssigned(_rootPage.Id, It.IsAny())) + .Returns(assignDomain); return new RedirectTracker( GetRequiredService(), RedirectUrlService, contentCache, - GetRequiredService(), + documentNavigationQueryService, GetRequiredService>(), publishedUrlProvider, - GetRequiredService()); + publishedContentStatusFilteringService, + domainCache); + } + + private void CreateExistingRedirect() + { + using var scope = ScopeProvider.CreateScope(); + var repository = CreateRedirectUrlRepository(); + repository.Save(new RedirectUrl { ContentKey = _testPage.Key, Url = "/new-route", Culture = "en" }); + scope.Complete(); } }