yv01p/Umbraco-CMS
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Redirect tracking: Ensure redirects with domains are stored with the domain node id prefix (closes #20894) (#20900)

Browse Source 
* 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>
This commit is contained in:
Andy Butland
2025-11-20 08:30:38 +01:00
committed by GitHub
parent 2f6fb7e395
commit 75dd9fab2b
2 changed files with 134 additions and 43 deletions
Download Patch File Download Diff File Expand all files Collapse all files

68
src/Umbraco.Infrastructure/Routing/RedirectTracker.cs
View File

@@ -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<RedirectTracker> _logger;
private readonly IPublishedUrlProvider _publishedUrlProvider;
private readonly IPublishedContentStatusFilteringService _publishedContentStatusFilteringService;
private readonly IDomainCache _domainCache;
/// <summary>
/// Initializes a new instance of the <see cref="RedirectTracker"/> class.
@@ -36,7 +38,8 @@ internal sealed class RedirectTracker : IRedirectTracker
IDocumentNavigationQueryService navigationQueryService,
ILogger<RedirectTracker> 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;
}
/// <inheritdoc/>
@@ -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<string[]>(() => entityContent.AncestorsOrSelf(_navigationQueryService, _publishedContentStatusFilteringService).FirstOrDefault(a => a.Cultures.Any())?.Cultures.Keys.ToArray() ?? Array.Empty<string>());
// 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().ToArray());
var languageIsoCodes = new Lazy<string[]>(() => [.. _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);
}
/// <inheritdoc/>
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)
{

109
tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Routing/RedirectTrackerTests.cs
View File

@@ -23,40 +23,47 @@ public class RedirectTrackerTests : UmbracoIntegrationTestWithContent
{
private IRedirectUrlService RedirectUrlService => GetRequiredService<IRedirectUrlService>();
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<IRepositoryCacheVersionService>(),
Mock.Of<ICacheSyncService>());
private IRedirectTracker CreateRedirectTracker()
private IRedirectTracker CreateRedirectTracker(bool assignDomain = false)
{
var contentType = new Mock<IPublishedContentType>();
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<IPublishedContent>();
var rootContent = new Mock<IPublishedContent>();
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<IPublishedContent>();
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<IPublishedContentCache>();
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<IPublishedUrlProvider>();
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<IDocumentNavigationQueryService>();
IEnumerable<Guid> ancestorKeys = [_rootPage.Key];
Mock.Get(documentNavigationQueryService)
.Setup(x => x.TryGetAncestorsKeys(_testPage.Key, out ancestorKeys))
.Returns(true);
IPublishedContentStatusFilteringService publishedContentStatusFilteringService = Mock.Of<IPublishedContentStatusFilteringService>();
Mock.Get(publishedContentStatusFilteringService)
.Setup(x => x.FilterAvailable(It.IsAny<IEnumerable<Guid>>(), It.IsAny<string?>()))
.Returns([rootContent.Object]);
IDomainCache domainCache = Mock.Of<IDomainCache>();
Mock.Get(domainCache)
.Setup(x => x.HasAssigned(_testPage.Id, It.IsAny<bool>()))
.Returns(false);
Mock.Get(domainCache)
.Setup(x => x.HasAssigned(_rootPage.Id, It.IsAny<bool>()))
.Returns(assignDomain);
return new RedirectTracker(
GetRequiredService<ILanguageService>(),
RedirectUrlService,
contentCache,
GetRequiredService<IDocumentNavigationQueryService>(),
documentNavigationQueryService,
GetRequiredService<ILogger<RedirectTracker>>(),
publishedUrlProvider,
GetRequiredService<IPublishedContentStatusFilteringService>());
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();
}
}