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

Redirects: Fixes self referencing redirects (closes #20139) (#20767)

Browse Source 
* Adding fix for self-referncing redirects for 17

* Using umbraco context on failing tests

* Tests to see if self referencing redirects gets deleted

* Refactoring and adding correct tests.

* Expanding tests for RedirectTrackerTests.cs

* Optimize by only retrieving th list of existing URLs for a content item if we have a valid route to create a redirect for.

* Extract method refactoring, added explanatory comment, fixed warnings and formatting.

* Resolved warnings in RedirectService.

* Minor naming and formatting refactor in tests.

---------

Co-authored-by: Andy Butland <abutland73@gmail.com>
This commit is contained in:
Nicklas Kramer
2025-11-14 17:37:59 +01:00
committed by GitHub
parent 01b300336b
commit 43230dfac8
4 changed files with 333 additions and 158 deletions
Download Patch File Download Diff File Expand all files Collapse all files

72
src/Umbraco.Core/Services/RedirectUrlService.cs
View File

@@ -1,4 +1,3 @@
using System.Threading.Tasks;
using Microsoft.Extensions.Logging;
using Umbraco.Cms.Core.Events;
using Umbraco.Cms.Core.Models;
@@ -7,10 +6,16 @@ using Umbraco.Cms.Core.Scoping;
namespace Umbraco.Cms.Core.Services;
/// <summary>
/// Provides services for managing redirect URLs.
/// </summary>
internal sealed class RedirectUrlService : RepositoryService, IRedirectUrlService
{
private readonly IRedirectUrlRepository _redirectUrlRepository;
/// <summary>
/// Initializes a new instance of the <see cref="RedirectUrlService"/> class.
/// </summary>
public RedirectUrlService(
ICoreScopeProvider provider,
ILoggerFactory loggerFactory,
@@ -19,10 +24,10 @@ internal sealed class RedirectUrlService : RepositoryService, IRedirectUrlServic
: base(provider, loggerFactory, eventMessagesFactory) =>
_redirectUrlRepository = redirectUrlRepository;
/// <inheritdoc/>
public void Register(string url, Guid contentKey, string? culture = null)
{
using (ICoreScope scope = ScopeProvider.CreateCoreScope())
{
using ICoreScope scope = ScopeProvider.CreateCoreScope();
IRedirectUrl? redir = _redirectUrlRepository.Get(url, contentKey, culture);
if (redir != null)
{
@@ -36,92 +41,82 @@ internal sealed class RedirectUrlService : RepositoryService, IRedirectUrlServic
_redirectUrlRepository.Save(redir);
scope.Complete();
}
}
/// <inheritdoc/>
public void Delete(IRedirectUrl redirectUrl)
{
using (ICoreScope scope = ScopeProvider.CreateCoreScope())
{
using ICoreScope scope = ScopeProvider.CreateCoreScope();
_redirectUrlRepository.Delete(redirectUrl);
scope.Complete();
}
}
/// <inheritdoc/>
public void Delete(Guid id)
{
using (ICoreScope scope = ScopeProvider.CreateCoreScope())
{
using ICoreScope scope = ScopeProvider.CreateCoreScope();
_redirectUrlRepository.Delete(id);
scope.Complete();
}
}
/// <inheritdoc/>
public void DeleteContentRedirectUrls(Guid contentKey)
{
using (ICoreScope scope = ScopeProvider.CreateCoreScope())
{
using ICoreScope scope = ScopeProvider.CreateCoreScope();
_redirectUrlRepository.DeleteContentUrls(contentKey);
scope.Complete();
}
}
/// <inheritdoc/>
public void DeleteAll()
{
using (ICoreScope scope = ScopeProvider.CreateCoreScope())
{
using ICoreScope scope = ScopeProvider.CreateCoreScope();
_redirectUrlRepository.DeleteAll();
scope.Complete();
}
}
/// <inheritdoc/>
public IRedirectUrl? GetMostRecentRedirectUrl(string url)
{
using (ICoreScope scope = ScopeProvider.CreateCoreScope(autoComplete: true))
{
using ICoreScope scope = ScopeProvider.CreateCoreScope(autoComplete: true);
return _redirectUrlRepository.GetMostRecentUrl(url);
}
}
/// <inheritdoc/>
public async Task<IRedirectUrl?> GetMostRecentRedirectUrlAsync(string url)
{
using (ICoreScope scope = ScopeProvider.CreateCoreScope(autoComplete: true))
{
using ICoreScope scope = ScopeProvider.CreateCoreScope(autoComplete: true);
return await _redirectUrlRepository.GetMostRecentUrlAsync(url);
}
}
/// <inheritdoc/>
public IEnumerable<IRedirectUrl> GetContentRedirectUrls(Guid contentKey)
{
using (ICoreScope scope = ScopeProvider.CreateCoreScope(autoComplete: true))
{
using ICoreScope scope = ScopeProvider.CreateCoreScope(autoComplete: true);
return _redirectUrlRepository.GetContentUrls(contentKey);
}
}
/// <inheritdoc/>
public IEnumerable<IRedirectUrl> GetAllRedirectUrls(long pageIndex, int pageSize, out long total)
{
using (ICoreScope scope = ScopeProvider.CreateCoreScope(autoComplete: true))
{
using ICoreScope scope = ScopeProvider.CreateCoreScope(autoComplete: true);
return _redirectUrlRepository.GetAllUrls(pageIndex, pageSize, out total);
}
}
/// <inheritdoc/>
public IEnumerable<IRedirectUrl> GetAllRedirectUrls(int rootContentId, long pageIndex, int pageSize, out long total)
{
using (ICoreScope scope = ScopeProvider.CreateCoreScope(autoComplete: true))
{
using ICoreScope scope = ScopeProvider.CreateCoreScope(autoComplete: true);
return _redirectUrlRepository.GetAllUrls(rootContentId, pageIndex, pageSize, out total);
}
}
/// <inheritdoc/>
public IEnumerable<IRedirectUrl> SearchRedirectUrls(string searchTerm, long pageIndex, int pageSize, out long total)
{
using (ICoreScope scope = ScopeProvider.CreateCoreScope(autoComplete: true))
{
using ICoreScope scope = ScopeProvider.CreateCoreScope(autoComplete: true);
return _redirectUrlRepository.SearchUrls(searchTerm, pageIndex, pageSize, out total);
}
}
/// <inheritdoc/>
public IRedirectUrl? GetMostRecentRedirectUrl(string url, string? culture)
{
if (string.IsNullOrWhiteSpace(culture))
@@ -129,12 +124,11 @@ internal sealed class RedirectUrlService : RepositoryService, IRedirectUrlServic
return GetMostRecentRedirectUrl(url);
}
using (ICoreScope scope = ScopeProvider.CreateCoreScope(autoComplete: true))
{
using ICoreScope scope = ScopeProvider.CreateCoreScope(autoComplete: true);
return _redirectUrlRepository.GetMostRecentUrl(url, culture);
}
}
/// <inheritdoc/>
public async Task<IRedirectUrl?> GetMostRecentRedirectUrlAsync(string url, string? culture)
{
if (string.IsNullOrWhiteSpace(culture))
@@ -142,9 +136,7 @@ internal sealed class RedirectUrlService : RepositoryService, IRedirectUrlServic
return await GetMostRecentRedirectUrlAsync(url);
}
using (ICoreScope scope = ScopeProvider.CreateCoreScope(autoComplete: true))
{
using ICoreScope scope = ScopeProvider.CreateCoreScope(autoComplete: true);
return await _redirectUrlRepository.GetMostRecentUrlAsync(url, culture);
}
}
}

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

@@ -1,6 +1,7 @@
using System.Diagnostics.CodeAnalysis;
using Microsoft.Extensions.Logging;
using Umbraco.Cms.Core;
using Umbraco.Cms.Core.Extensions;
using Umbraco.Cms.Core.Models;
using Umbraco.Cms.Core.Models.PublishedContent;
using Umbraco.Cms.Core.PublishedCache;
@@ -9,11 +10,15 @@ using Umbraco.Cms.Core.Services;
using Umbraco.Cms.Core.Services.Navigation;
using Umbraco.Extensions;
namespace Umbraco.Cms.Infrastructure.Routing
namespace Umbraco.Cms.Infrastructure.Routing;
/// <summary>
/// Tracks and manages URL redirects for content items, ensuring that old routes are stored and appropriate redirects
/// are created when content URLs change.
/// </summary>
internal sealed class RedirectTracker : IRedirectTracker
{
internal sealed class RedirectTracker : IRedirectTracker
{
private readonly ILocalizationService _localizationService;
private readonly ILanguageService _languageService;
private readonly IRedirectUrlService _redirectUrlService;
private readonly IPublishedContentCache _contentCache;
private readonly IDocumentNavigationQueryService _navigationQueryService;
@@ -21,8 +26,11 @@ namespace Umbraco.Cms.Infrastructure.Routing
private readonly IPublishedUrlProvider _publishedUrlProvider;
private readonly IPublishedContentStatusFilteringService _publishedContentStatusFilteringService;
/// <summary>
/// Initializes a new instance of the <see cref="RedirectTracker"/> class.
/// </summary>
public RedirectTracker(
ILocalizationService localizationService,
ILanguageService languageService,
IRedirectUrlService redirectUrlService,
IPublishedContentCache contentCache,
IDocumentNavigationQueryService navigationQueryService,
@@ -30,7 +38,7 @@ namespace Umbraco.Cms.Infrastructure.Routing
IPublishedUrlProvider publishedUrlProvider,
IPublishedContentStatusFilteringService publishedContentStatusFilteringService)
{
_localizationService = localizationService;
_languageService = languageService;
_redirectUrlService = redirectUrlService;
_contentCache = contentCache;
_navigationQueryService = navigationQueryService;
@@ -52,7 +60,7 @@ namespace Umbraco.Cms.Infrastructure.Routing
var defaultCultures = new Lazy<string[]>(() => entityContent.AncestorsOrSelf(_navigationQueryService, _publishedContentStatusFilteringService).FirstOrDefault(a => a.Cultures.Any())?.Cultures.Keys.ToArray() ?? Array.Empty<string>());
// Get all language ISO codes (in case we're dealing with invariant content with variant ancestors)
var languageIsoCodes = new Lazy<string[]>(() => _localizationService.GetAllLanguages().Select(x => x.IsoCode).ToArray());
var languageIsoCodes = new Lazy<string[]>(() => _languageService.GetAllIsoCodesAsync().GetAwaiter().GetResult().ToArray());
foreach (IPublishedContent publishedContent in entityContent.DescendantsOrSelf(_navigationQueryService, _publishedContentStatusFilteringService))
{
@@ -103,11 +111,16 @@ namespace Umbraco.Cms.Infrastructure.Routing
try
{
var newRoute = _publishedUrlProvider.GetUrl(contentKey, UrlMode.Relative, culture).TrimEnd(Constants.CharArrays.ForwardSlash);
if (!IsValidRoute(newRoute) || oldRoute == newRoute)
{
continue;
}
// Ensure we don't create a self-referencing redirect. This can occur if a document is renamed and then the name is reverted back
// to the original. We resolve this by removing any existing redirect that points to the new route.
RemoveSelfReferencingRedirect(contentKey, newRoute);
_redirectUrlService.Register(oldRoute, contentKey, culture);
}
catch (Exception ex)
@@ -118,5 +131,16 @@ namespace Umbraco.Cms.Infrastructure.Routing
}
private static bool IsValidRoute([NotNullWhen(true)] string? route) => route is not null && !route.StartsWith("err/");
private void RemoveSelfReferencingRedirect(Guid contentKey, string route)
{
IEnumerable<IRedirectUrl> allRedirectUrls = _redirectUrlService.GetContentRedirectUrls(contentKey);
foreach (IRedirectUrl redirectUrl in allRedirectUrls)
{
if (redirectUrl.Url == route)
{
_redirectUrlService.Delete(redirectUrl.Key);
}
}
}
}

149
tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Routing/RedirectTrackerTests.cs Normal file
View File

@@ -0,0 +1,149 @@
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Logging.Abstractions;
using Moq;
using NUnit.Framework;
using Umbraco.Cms.Core.Cache;
using Umbraco.Cms.Core.Models;
using Umbraco.Cms.Core.Models.PublishedContent;
using Umbraco.Cms.Core.PublishedCache;
using Umbraco.Cms.Core.Routing;
using Umbraco.Cms.Core.Services;
using Umbraco.Cms.Core.Services.Navigation;
using Umbraco.Cms.Infrastructure.Persistence.Repositories.Implement;
using Umbraco.Cms.Infrastructure.Routing;
using Umbraco.Cms.Infrastructure.Scoping;
using Umbraco.Cms.Tests.Common.Testing;
using Umbraco.Cms.Tests.Integration.Testing;
namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Routing;
[TestFixture]
[UmbracoTest(Database = UmbracoTestOptions.Database.NewSchemaPerTest)]
public class RedirectTrackerTests : UmbracoIntegrationTestWithContent
{
private IRedirectUrlService RedirectUrlService => GetRequiredService<IRedirectUrlService>();
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();
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"),
};
var redirectTracker = CreateRedirectTracker();
redirectTracker.StoreOldRoute(_testPage, dict);
Assert.That(dict.Count, Is.EqualTo(1));
Assert.AreEqual(dict.Values.First().OldRoute, Url);
}
[Test]
public void Can_Create_Redirects()
{
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, "/old-route"),
};
var redirectTracker = CreateRedirectTracker();
redirectTracker.CreateRedirects(dict);
var redirects = RedirectUrlService.GetContentRedirectUrls(_testPage.Key);
Assert.IsTrue(redirects.Any(x => x.Url == "/old-route"));
}
[Test]
public void Removes_Self_Referncing_Redirects()
{
const string newUrl = "newUrl";
var redirects = RedirectUrlService.GetContentRedirectUrls(_testPage.Key);
Assert.IsTrue(redirects.Any(x => x.Url == Url)); // 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),
};
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));
}
private RedirectUrlRepository CreateRedirectUrlRepository() =>
new(
(IScopeAccessor)ScopeProvider,
AppCaches.Disabled,
new NullLogger<RedirectUrlRepository>(),
Mock.Of<IRepositoryCacheVersionService>(),
Mock.Of<ICacheSyncService>());
private IRedirectTracker CreateRedirectTracker()
{
var contentType = new Mock<IPublishedContentType>();
contentType.SetupGet(c => c.Variations).Returns(ContentVariation.Nothing);
var cultures = new Dictionary<string, PublishedCultureInfo>
{
{ "en", new PublishedCultureInfo("en", "english", "/en/", DateTime.UtcNow) },
};
var content = new Mock<IPublishedContent>();
content.SetupGet(c => c.Key).Returns(_testPage.Key);
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);
IPublishedUrlProvider publishedUrlProvider = Mock.Of<IPublishedUrlProvider>();
Mock.Get(publishedUrlProvider)
.Setup(x => x.GetUrl(_testPage.Key, UrlMode.Relative, "en", null))
.Returns(Url);
Mock.Get(publishedUrlProvider)
.Setup(x => x.GetUrl(_testPage.Id, UrlMode.Relative, "en", null))
.Returns(Url);
return new RedirectTracker(
GetRequiredService<ILanguageService>(),
RedirectUrlService,
contentCache,
GetRequiredService<IDocumentNavigationQueryService>(),
GetRequiredService<ILogger<RedirectTracker>>(),
publishedUrlProvider,
GetRequiredService<IPublishedContentStatusFilteringService>());
}
}

14
tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/RedirectUrlServiceTests.cs
View File

@@ -1,8 +1,6 @@
// Copyright (c) Umbraco.
// See LICENSE for more details.
using System.Linq;
using System.Threading;
using Microsoft.Extensions.Logging;
using Moq;
using NUnit.Framework;
@@ -85,4 +83,16 @@ internal sealed class RedirectUrlServiceTests : UmbracoIntegrationTestWithConten
var redirect = RedirectUrlService.GetMostRecentRedirectUrl(UrlAlt, UnusedCulture);
Assert.AreEqual(redirect.ContentId, _thirdSubPage.Id);
}
[Test]
public void Can_Register_Redirect()
{
const string TestUrl = "testUrl";
RedirectUrlService.Register(TestUrl, _firstSubPage.Key);
var redirect = RedirectUrlService.GetMostRecentRedirectUrl(TestUrl, CultureEnglish);
Assert.AreEqual(redirect.ContentId, _firstSubPage.Id);
}
}