From a54997175e0cf647a8901448655b00f2792209c0 Mon Sep 17 00:00:00 2001 From: NillasKA Date: Fri, 7 Nov 2025 12:54:18 +0100 Subject: [PATCH 1/9] Adding fix for self-referncing redirects for 17 --- .../Services/RedirectUrlService.cs | 40 +++++++++++++++++-- 1 file changed, 37 insertions(+), 3 deletions(-) diff --git a/src/Umbraco.Core/Services/RedirectUrlService.cs b/src/Umbraco.Core/Services/RedirectUrlService.cs index c2050f7ff0d6..45d5fdda1937 100644 --- a/src/Umbraco.Core/Services/RedirectUrlService.cs +++ b/src/Umbraco.Core/Services/RedirectUrlService.cs @@ -1,8 +1,11 @@ -using System.Threading.Tasks; +using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; +using Umbraco.Cms.Core.DependencyInjection; using Umbraco.Cms.Core.Events; using Umbraco.Cms.Core.Models; +using Umbraco.Cms.Core.Models.PublishedContent; using Umbraco.Cms.Core.Persistence.Repositories; +using Umbraco.Cms.Core.Routing; using Umbraco.Cms.Core.Scoping; namespace Umbraco.Cms.Core.Services; @@ -10,19 +13,50 @@ namespace Umbraco.Cms.Core.Services; internal sealed class RedirectUrlService : RepositoryService, IRedirectUrlService { private readonly IRedirectUrlRepository _redirectUrlRepository; + private readonly IPublishedUrlProvider _publishedUrlProvider; public RedirectUrlService( ICoreScopeProvider provider, ILoggerFactory loggerFactory, IEventMessagesFactory eventMessagesFactory, - IRedirectUrlRepository redirectUrlRepository) - : base(provider, loggerFactory, eventMessagesFactory) => + IRedirectUrlRepository redirectUrlRepository, + IPublishedUrlProvider publishedUrlProvider) + : base(provider, loggerFactory, eventMessagesFactory) + { _redirectUrlRepository = redirectUrlRepository; + _publishedUrlProvider = publishedUrlProvider; + } + + [Obsolete("Use the constructor taking all parameters. Scheduled for removal in Umbraco 19")] + public RedirectUrlService( + ICoreScopeProvider provider, + ILoggerFactory loggerFactory, + IEventMessagesFactory eventMessagesFactory, + IRedirectUrlRepository redirectUrlRepository) + : this(provider, + loggerFactory, + eventMessagesFactory, + StaticServiceProvider.Instance.GetRequiredService(), + StaticServiceProvider.Instance.GetRequiredService()) + { + } public void Register(string url, Guid contentKey, string? culture = null) { using (ICoreScope scope = ScopeProvider.CreateCoreScope()) { + var newUrl = _publishedUrlProvider.GetUrl(contentKey, UrlMode.Relative, culture).TrimEnd('/'); + IEnumerable allRedirectUrls = _redirectUrlRepository.GetContentUrls(contentKey); + + // If there's a redirect URL that points to the new URL, delete it. + foreach (IRedirectUrl redirectUrl in allRedirectUrls) + { + if (redirectUrl.Url == newUrl) + { + _redirectUrlRepository.Delete(redirectUrl.Key); + } + } + IRedirectUrl? redir = _redirectUrlRepository.Get(url, contentKey, culture); if (redir != null) { From 0ecef87bc31d401c8dceb5a85a8118be5b9f733a Mon Sep 17 00:00:00 2001 From: NillasKA Date: Wed, 12 Nov 2025 09:55:33 +0100 Subject: [PATCH 2/9] Using umbraco context on failing tests --- .../Services/ContentTypeServiceVariantsTests.cs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/ContentTypeServiceVariantsTests.cs b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/ContentTypeServiceVariantsTests.cs index d1c7ef22d675..1018a616a800 100644 --- a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/ContentTypeServiceVariantsTests.cs +++ b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/ContentTypeServiceVariantsTests.cs @@ -9,6 +9,7 @@ using Umbraco.Cms.Core.Models; using Umbraco.Cms.Core.Services; using Umbraco.Cms.Core.Sync; +using Umbraco.Cms.Core.Web; using Umbraco.Cms.Infrastructure.Persistence; using Umbraco.Cms.Infrastructure.Persistence.Dtos; using Umbraco.Cms.Tests.Common.Attributes; @@ -87,6 +88,8 @@ public void Change_Content_Type_Variation_Clears_Redirects( ContentVariation changedContentTypeVariation, bool shouldUrlRedirectsBeCleared) { + var umbracoContextFactory = GetRequiredService(); + var contentType = ContentTypeBuilder.CreateBasicContentType(); contentType.Variations = startingContentTypeVariation; ContentTypeService.Save(contentType); @@ -106,8 +109,11 @@ public void Change_Content_Type_Variation_Clears_Redirects( IContent doc2 = ContentBuilder.CreateBasicContent(contentType2); ContentService.Save(doc2); - RedirectUrlService.Register("hello/world", doc.Key); - RedirectUrlService.Register("hello2/world2", doc2.Key); + using (umbracoContextFactory.EnsureUmbracoContext()) + { + RedirectUrlService.Register("hello/world", doc.Key); + RedirectUrlService.Register("hello2/world2", doc2.Key); + } // These 2 assertions should probably be moved to a test for the Register() method? Assert.AreEqual(1, RedirectUrlService.GetContentRedirectUrls(doc.Key).Count()); From 7dbec2852b9c3542b6661b9b38d964b4292c9e63 Mon Sep 17 00:00:00 2001 From: NillasKA Date: Wed, 12 Nov 2025 14:07:45 +0100 Subject: [PATCH 3/9] Tests to see if self referencing redirects gets deleted --- .../Services/RedirectUrlService.cs | 16 ---- .../Services/RedirectUrlServiceTests.cs | 84 +++++++++++++++++-- 2 files changed, 78 insertions(+), 22 deletions(-) diff --git a/src/Umbraco.Core/Services/RedirectUrlService.cs b/src/Umbraco.Core/Services/RedirectUrlService.cs index 45d5fdda1937..9de1d422a99e 100644 --- a/src/Umbraco.Core/Services/RedirectUrlService.cs +++ b/src/Umbraco.Core/Services/RedirectUrlService.cs @@ -1,6 +1,4 @@ -using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; -using Umbraco.Cms.Core.DependencyInjection; using Umbraco.Cms.Core.Events; using Umbraco.Cms.Core.Models; using Umbraco.Cms.Core.Models.PublishedContent; @@ -27,20 +25,6 @@ public RedirectUrlService( _publishedUrlProvider = publishedUrlProvider; } - [Obsolete("Use the constructor taking all parameters. Scheduled for removal in Umbraco 19")] - public RedirectUrlService( - ICoreScopeProvider provider, - ILoggerFactory loggerFactory, - IEventMessagesFactory eventMessagesFactory, - IRedirectUrlRepository redirectUrlRepository) - : this(provider, - loggerFactory, - eventMessagesFactory, - StaticServiceProvider.Instance.GetRequiredService(), - StaticServiceProvider.Instance.GetRequiredService()) - { - } - public void Register(string url, Guid contentKey, string? culture = null) { using (ICoreScope scope = ScopeProvider.CreateCoreScope()) diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/RedirectUrlServiceTests.cs b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/RedirectUrlServiceTests.cs index 2e6bb099f2d5..6226a2a9366d 100644 --- a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/RedirectUrlServiceTests.cs +++ b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/RedirectUrlServiceTests.cs @@ -7,11 +7,18 @@ using Moq; using NUnit.Framework; using Umbraco.Cms.Core.Cache; +using Umbraco.Cms.Core.Events; using Umbraco.Cms.Core.Models; +using Umbraco.Cms.Core.Models.PublishedContent; +using Umbraco.Cms.Core.Routing; +using Umbraco.Cms.Core.Scoping; using Umbraco.Cms.Core.Services; +using Umbraco.Cms.Core.Web; +using Umbraco.Cms.Infrastructure.HybridCache; using Umbraco.Cms.Infrastructure.Persistence.Repositories.Implement; using Umbraco.Cms.Infrastructure.Scoping; using Umbraco.Cms.Tests.Common.Attributes; +using Umbraco.Cms.Tests.Common.Builders; using Umbraco.Cms.Tests.Common.Testing; using Umbraco.Cms.Tests.Integration.Testing; @@ -32,18 +39,15 @@ internal sealed class RedirectUrlServiceTests : UmbracoIntegrationTestWithConten private IRedirectUrlService RedirectUrlService => GetRequiredService(); + private IPublishedUrlProvider PublishedUrlProvider => GetRequiredService(); + public override void CreateTestData() { base.CreateTestData(); using (var scope = ScopeProvider.CreateScope()) { - var repository = new RedirectUrlRepository( - (IScopeAccessor)ScopeProvider, - AppCaches.Disabled, - Mock.Of>(), - Mock.Of(), - Mock.Of()); + var repository = MockRepository(); var rootContent = ContentService.GetRootContent().First(); var subPages = ContentService.GetPagedChildren(rootContent.Id, 0, 3, out _).ToList(); _firstSubPage = subPages[0]; @@ -85,4 +89,72 @@ public void Can_Get_Most_Recent_RedirectUrl_With_Culture_When_No_CultureVariant_ var redirect = RedirectUrlService.GetMostRecentRedirectUrl(UrlAlt, UnusedCulture); Assert.AreEqual(redirect.ContentId, _thirdSubPage.Id); } + + [Test] + public void Can_Register_Redirect() + { + var umbracoContextFactory = GetRequiredService(); + + const string TestUrl = "testUrl"; + using (umbracoContextFactory.EnsureUmbracoContext()) + { + RedirectUrlService.Register(TestUrl, _firstSubPage.Key); + } + + var redirect = RedirectUrlService.GetMostRecentRedirectUrl(TestUrl, CultureEnglish); + + Assert.AreEqual(redirect.ContentId, _firstSubPage.Id); + } + + /// + /// The service if there already exists any redirect URLs that point to the new URL. If thats the case we delete it. + /// With this test, a self referencing redirect already exists through mocks, and we test to see if the Register method deletes it. + /// + [Test] + public void Deletes_Self_Referencing_Redirects() + { + var service = mockServiceSetup(); + var redirects = RedirectUrlService.GetContentRedirectUrls(_firstSubPage.Key); + var originalRedirect = RedirectUrlService.GetContentRedirectUrls(_firstSubPage.Key).First().Url; + + // Self referencing redirect exists already through mocks. + Assert.True(redirects.Any(x => x.Url == originalRedirect)); + + var umbracoContextFactory = GetRequiredService(); + string redirectUrl = "newRedirectUrl"; + using (umbracoContextFactory.EnsureUmbracoContext()) + { + service.Register(redirectUrl, _firstSubPage.Key); + } + + redirects = RedirectUrlService.GetContentRedirectUrls(_firstSubPage.Key); + + // The self referencing redirect has been successfully deleted. + Assert.False(redirects.Any(x => x.Url == originalRedirect)); + } + + private RedirectUrlService mockServiceSetup() + { + IPublishedUrlProvider publishedUrlProvider = Mock.Of(); + Mock.Get(publishedUrlProvider) + .Setup(x => x.GetUrl(_firstSubPage.Key, UrlMode.Relative, null, null)) + .Returns(Url); + + return new RedirectUrlService( + ScopeProvider, + Mock.Of(), + Mock.Of(), + MockRepository(), + publishedUrlProvider); + } + + private RedirectUrlRepository MockRepository() + { + return new RedirectUrlRepository( + (IScopeAccessor)ScopeProvider, + AppCaches.Disabled, + Mock.Of>(), + Mock.Of(), + Mock.Of()); + } } From fa523c4a39f7a8beebef3aa2c01f51518510f778 Mon Sep 17 00:00:00 2001 From: NillasKA Date: Thu, 13 Nov 2025 16:07:15 +0100 Subject: [PATCH 4/9] Refactoring and adding correct tests. --- .../Services/RedirectUrlService.cs | 21 +--- .../Routing/RedirectTracker.cs | 12 +- .../Routing/RedirectTrackerTests.cs | 115 ++++++++++++++++++ .../ContentTypeServiceVariantsTests.cs | 10 +- .../Services/RedirectUrlServiceTests.cs | 78 ++---------- 5 files changed, 138 insertions(+), 98 deletions(-) create mode 100644 tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Routing/RedirectTrackerTests.cs diff --git a/src/Umbraco.Core/Services/RedirectUrlService.cs b/src/Umbraco.Core/Services/RedirectUrlService.cs index 9de1d422a99e..9aaf6a2025d8 100644 --- a/src/Umbraco.Core/Services/RedirectUrlService.cs +++ b/src/Umbraco.Core/Services/RedirectUrlService.cs @@ -11,36 +11,19 @@ namespace Umbraco.Cms.Core.Services; internal sealed class RedirectUrlService : RepositoryService, IRedirectUrlService { private readonly IRedirectUrlRepository _redirectUrlRepository; - private readonly IPublishedUrlProvider _publishedUrlProvider; public RedirectUrlService( ICoreScopeProvider provider, ILoggerFactory loggerFactory, IEventMessagesFactory eventMessagesFactory, - IRedirectUrlRepository redirectUrlRepository, - IPublishedUrlProvider publishedUrlProvider) - : base(provider, loggerFactory, eventMessagesFactory) - { + IRedirectUrlRepository redirectUrlRepository) + : base(provider, loggerFactory, eventMessagesFactory) => _redirectUrlRepository = redirectUrlRepository; - _publishedUrlProvider = publishedUrlProvider; - } public void Register(string url, Guid contentKey, string? culture = null) { using (ICoreScope scope = ScopeProvider.CreateCoreScope()) { - var newUrl = _publishedUrlProvider.GetUrl(contentKey, UrlMode.Relative, culture).TrimEnd('/'); - IEnumerable allRedirectUrls = _redirectUrlRepository.GetContentUrls(contentKey); - - // If there's a redirect URL that points to the new URL, delete it. - foreach (IRedirectUrl redirectUrl in allRedirectUrls) - { - if (redirectUrl.Url == newUrl) - { - _redirectUrlRepository.Delete(redirectUrl.Key); - } - } - IRedirectUrl? redir = _redirectUrlRepository.Get(url, contentKey, culture); if (redir != null) { diff --git a/src/Umbraco.Infrastructure/Routing/RedirectTracker.cs b/src/Umbraco.Infrastructure/Routing/RedirectTracker.cs index 18a4d45e67d9..07df699bc5c8 100644 --- a/src/Umbraco.Infrastructure/Routing/RedirectTracker.cs +++ b/src/Umbraco.Infrastructure/Routing/RedirectTracker.cs @@ -102,12 +102,22 @@ public void CreateRedirects(IDictionary<(int ContentId, string Culture), (Guid C { try { - var newRoute = _publishedUrlProvider.GetUrl(contentKey, UrlMode.Relative, culture).TrimEnd(Constants.CharArrays.ForwardSlash); + IEnumerable allRedirectUrls = _redirectUrlService.GetContentRedirectUrls(contentKey); + var newRoute = _publishedUrlProvider.GetUrl(contentKey, UrlMode.Relative, culture).TrimEnd(Constants.CharArrays.ForwardSlash); + if (!IsValidRoute(newRoute) || oldRoute == newRoute) { continue; } + foreach (IRedirectUrl redirectUrl in allRedirectUrls) + { + if (redirectUrl.Url == newRoute) + { + _redirectUrlService.Delete(redirectUrl.Key); + } + } + _redirectUrlService.Register(oldRoute, contentKey, culture); } catch (Exception ex) diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Routing/RedirectTrackerTests.cs b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Routing/RedirectTrackerTests.cs new file mode 100644 index 000000000000..53fb83ff4ee1 --- /dev/null +++ b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Routing/RedirectTrackerTests.cs @@ -0,0 +1,115 @@ +using Microsoft.Extensions.Logging; +using Moq; +using NUnit.Framework; +using Umbraco.Cms.Core; +using Umbraco.Cms.Core.Cache; +using Umbraco.Cms.Core.Events; +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 IRedirectTracker RedirectTracker => GetRequiredService(); + + private IRedirectUrlService redirectUrlService => GetRequiredService(); + + private IContent _subPage; + + private const string Url = "RedirectUrl"; + + public override void CreateTestData() + { + base.CreateTestData(); + + using (var scope = ScopeProvider.CreateScope()) + { + var repository = MockRepository(); + var rootContent = ContentService.GetRootContent().First(); + var subPages = ContentService.GetPagedChildren(rootContent.Id, 0, 3, out _).ToList(); + _subPage = subPages[0]; + + repository.Save(new RedirectUrl { ContentKey = _subPage.Key, Url = Url, Culture = "en" }); + + scope.Complete(); + } + } + + [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)> + { + [(_subPage.Id, "en")] = (_subPage.Key, "/old-route"), + }; + var redirectTracker = mockRedirectTracker(); + + redirectTracker.CreateRedirects(dict); + + var redirects = redirectUrlService.GetContentRedirectUrls(_subPage.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(_subPage.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)> + { + [(_subPage.Id, "en")] = (_subPage.Key, newUrl), + }; + + var redirectTracker = mockRedirectTracker(); + redirectTracker.CreateRedirects(dict); + redirects = redirectUrlService.GetContentRedirectUrls(_subPage.Key); + + Assert.IsFalse(redirects.Any(x => x.Url == Url)); + Assert.IsTrue(redirects.Any(x => x.Url == newUrl)); + } + + private RedirectUrlRepository MockRepository() + { + return new RedirectUrlRepository( + (IScopeAccessor)ScopeProvider, + AppCaches.Disabled, + Mock.Of>(), + Mock.Of(), + Mock.Of()); + } + + private IRedirectTracker mockRedirectTracker() + { + IPublishedUrlProvider publishedUrlProvider = Mock.Of(); + Mock.Get(publishedUrlProvider) + .Setup(x => x.GetUrl(_subPage.Key, UrlMode.Relative, "en", null)) + .Returns(Url); + + return new RedirectTracker( + GetRequiredService(), + redirectUrlService, + GetRequiredService(), + GetRequiredService(), + GetRequiredService>(), + publishedUrlProvider, + GetRequiredService()); + } +} diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/ContentTypeServiceVariantsTests.cs b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/ContentTypeServiceVariantsTests.cs index 1018a616a800..d1c7ef22d675 100644 --- a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/ContentTypeServiceVariantsTests.cs +++ b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/ContentTypeServiceVariantsTests.cs @@ -9,7 +9,6 @@ using Umbraco.Cms.Core.Models; using Umbraco.Cms.Core.Services; using Umbraco.Cms.Core.Sync; -using Umbraco.Cms.Core.Web; using Umbraco.Cms.Infrastructure.Persistence; using Umbraco.Cms.Infrastructure.Persistence.Dtos; using Umbraco.Cms.Tests.Common.Attributes; @@ -88,8 +87,6 @@ public void Change_Content_Type_Variation_Clears_Redirects( ContentVariation changedContentTypeVariation, bool shouldUrlRedirectsBeCleared) { - var umbracoContextFactory = GetRequiredService(); - var contentType = ContentTypeBuilder.CreateBasicContentType(); contentType.Variations = startingContentTypeVariation; ContentTypeService.Save(contentType); @@ -109,11 +106,8 @@ public void Change_Content_Type_Variation_Clears_Redirects( IContent doc2 = ContentBuilder.CreateBasicContent(contentType2); ContentService.Save(doc2); - using (umbracoContextFactory.EnsureUmbracoContext()) - { - RedirectUrlService.Register("hello/world", doc.Key); - RedirectUrlService.Register("hello2/world2", doc2.Key); - } + RedirectUrlService.Register("hello/world", doc.Key); + RedirectUrlService.Register("hello2/world2", doc2.Key); // These 2 assertions should probably be moved to a test for the Register() method? Assert.AreEqual(1, RedirectUrlService.GetContentRedirectUrls(doc.Key).Count()); diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/RedirectUrlServiceTests.cs b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/RedirectUrlServiceTests.cs index 6226a2a9366d..962aa43bd9dd 100644 --- a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/RedirectUrlServiceTests.cs +++ b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/RedirectUrlServiceTests.cs @@ -1,24 +1,15 @@ // Copyright (c) Umbraco. // See LICENSE for more details. -using System.Linq; -using System.Threading; using Microsoft.Extensions.Logging; using Moq; using NUnit.Framework; using Umbraco.Cms.Core.Cache; -using Umbraco.Cms.Core.Events; using Umbraco.Cms.Core.Models; -using Umbraco.Cms.Core.Models.PublishedContent; -using Umbraco.Cms.Core.Routing; -using Umbraco.Cms.Core.Scoping; using Umbraco.Cms.Core.Services; -using Umbraco.Cms.Core.Web; -using Umbraco.Cms.Infrastructure.HybridCache; using Umbraco.Cms.Infrastructure.Persistence.Repositories.Implement; using Umbraco.Cms.Infrastructure.Scoping; using Umbraco.Cms.Tests.Common.Attributes; -using Umbraco.Cms.Tests.Common.Builders; using Umbraco.Cms.Tests.Common.Testing; using Umbraco.Cms.Tests.Integration.Testing; @@ -39,15 +30,18 @@ internal sealed class RedirectUrlServiceTests : UmbracoIntegrationTestWithConten private IRedirectUrlService RedirectUrlService => GetRequiredService(); - private IPublishedUrlProvider PublishedUrlProvider => GetRequiredService(); - public override void CreateTestData() { base.CreateTestData(); using (var scope = ScopeProvider.CreateScope()) { - var repository = MockRepository(); + var repository = new RedirectUrlRepository( + (IScopeAccessor)ScopeProvider, + AppCaches.Disabled, + Mock.Of>(), + Mock.Of(), + Mock.Of()); var rootContent = ContentService.GetRootContent().First(); var subPages = ContentService.GetPagedChildren(rootContent.Id, 0, 3, out _).ToList(); _firstSubPage = subPages[0]; @@ -93,68 +87,12 @@ public void Can_Get_Most_Recent_RedirectUrl_With_Culture_When_No_CultureVariant_ [Test] public void Can_Register_Redirect() { - var umbracoContextFactory = GetRequiredService(); - const string TestUrl = "testUrl"; - using (umbracoContextFactory.EnsureUmbracoContext()) - { - RedirectUrlService.Register(TestUrl, _firstSubPage.Key); - } + + RedirectUrlService.Register(TestUrl, _firstSubPage.Key); var redirect = RedirectUrlService.GetMostRecentRedirectUrl(TestUrl, CultureEnglish); Assert.AreEqual(redirect.ContentId, _firstSubPage.Id); } - - /// - /// The service if there already exists any redirect URLs that point to the new URL. If thats the case we delete it. - /// With this test, a self referencing redirect already exists through mocks, and we test to see if the Register method deletes it. - /// - [Test] - public void Deletes_Self_Referencing_Redirects() - { - var service = mockServiceSetup(); - var redirects = RedirectUrlService.GetContentRedirectUrls(_firstSubPage.Key); - var originalRedirect = RedirectUrlService.GetContentRedirectUrls(_firstSubPage.Key).First().Url; - - // Self referencing redirect exists already through mocks. - Assert.True(redirects.Any(x => x.Url == originalRedirect)); - - var umbracoContextFactory = GetRequiredService(); - string redirectUrl = "newRedirectUrl"; - using (umbracoContextFactory.EnsureUmbracoContext()) - { - service.Register(redirectUrl, _firstSubPage.Key); - } - - redirects = RedirectUrlService.GetContentRedirectUrls(_firstSubPage.Key); - - // The self referencing redirect has been successfully deleted. - Assert.False(redirects.Any(x => x.Url == originalRedirect)); - } - - private RedirectUrlService mockServiceSetup() - { - IPublishedUrlProvider publishedUrlProvider = Mock.Of(); - Mock.Get(publishedUrlProvider) - .Setup(x => x.GetUrl(_firstSubPage.Key, UrlMode.Relative, null, null)) - .Returns(Url); - - return new RedirectUrlService( - ScopeProvider, - Mock.Of(), - Mock.Of(), - MockRepository(), - publishedUrlProvider); - } - - private RedirectUrlRepository MockRepository() - { - return new RedirectUrlRepository( - (IScopeAccessor)ScopeProvider, - AppCaches.Disabled, - Mock.Of>(), - Mock.Of(), - Mock.Of()); - } } From 376ca4e2252b8602522e841abe2c661ec60d7a5b Mon Sep 17 00:00:00 2001 From: NillasKA Date: Fri, 14 Nov 2025 14:29:47 +0100 Subject: [PATCH 5/9] Expanding tests for RedirectTrackerTests.cs --- .../Routing/RedirectTrackerTests.cs | 47 +++++++++++++++++-- 1 file changed, 42 insertions(+), 5 deletions(-) diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Routing/RedirectTrackerTests.cs b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Routing/RedirectTrackerTests.cs index 53fb83ff4ee1..c16c83496904 100644 --- a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Routing/RedirectTrackerTests.cs +++ b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Routing/RedirectTrackerTests.cs @@ -1,9 +1,7 @@ using Microsoft.Extensions.Logging; using Moq; using NUnit.Framework; -using Umbraco.Cms.Core; using Umbraco.Cms.Core.Cache; -using Umbraco.Cms.Core.Events; using Umbraco.Cms.Core.Models; using Umbraco.Cms.Core.Models.PublishedContent; using Umbraco.Cms.Core.PublishedCache; @@ -22,8 +20,6 @@ namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Routing; [UmbracoTest(Database = UmbracoTestOptions.Database.NewSchemaPerTest)] public class RedirectTrackerTests : UmbracoIntegrationTestWithContent { - private IRedirectTracker RedirectTracker => GetRequiredService(); - private IRedirectUrlService redirectUrlService => GetRequiredService(); private IContent _subPage; @@ -47,6 +43,23 @@ public override void CreateTestData() } } + [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)> + { + [(_subPage.Id, "en")] = (_subPage.Key, "/old-route"), + }; + + var redirectTracker = mockRedirectTracker(); + + redirectTracker.StoreOldRoute(_subPage, dict); + + Assert.That(dict.Count, Is.EqualTo(1)); + Assert.AreEqual(dict.Values.First().OldRoute, Url); + } + [Test] public void Can_Create_Redirects() { @@ -98,15 +111,39 @@ private RedirectUrlRepository MockRepository() private IRedirectTracker mockRedirectTracker() { + var contentType = new Mock(); + contentType.SetupGet(c => c.Variations).Returns(ContentVariation.Nothing); + + var dict = new Dictionary + { + { "en", new PublishedCultureInfo("en", "english", "/en/", DateTime.Now) }, + }; + + var content = new Mock(); + + content.SetupGet(c => c.Key).Returns(_subPage.Key); + content.SetupGet(c => c.ContentType).Returns(contentType.Object); + content.SetupGet(c => c.Cultures).Returns(dict); + content.SetupGet(c => c.Id).Returns(_subPage.Id); + + IPublishedContentCache contentCache = Mock.Of(); + Mock.Get(contentCache) + .Setup(x => x.GetById(_subPage.Id)) + .Returns(content.Object); + IPublishedUrlProvider publishedUrlProvider = Mock.Of(); Mock.Get(publishedUrlProvider) .Setup(x => x.GetUrl(_subPage.Key, UrlMode.Relative, "en", null)) .Returns(Url); + Mock.Get(publishedUrlProvider) + .Setup(x => x.GetUrl(_subPage.Id, UrlMode.Relative, "en", null)) + .Returns(Url); + return new RedirectTracker( GetRequiredService(), redirectUrlService, - GetRequiredService(), + contentCache, GetRequiredService(), GetRequiredService>(), publishedUrlProvider, From 8791cd87a15685a3c556b7e3ac496fc3a42f08e9 Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Fri, 14 Nov 2025 16:25:54 +0100 Subject: [PATCH 6/9] Optimize by only retrieving th list of existing URLs for a content item if we have a valid route to create a redirect for. --- src/Umbraco.Infrastructure/Routing/RedirectTracker.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Umbraco.Infrastructure/Routing/RedirectTracker.cs b/src/Umbraco.Infrastructure/Routing/RedirectTracker.cs index 07df699bc5c8..291845de343f 100644 --- a/src/Umbraco.Infrastructure/Routing/RedirectTracker.cs +++ b/src/Umbraco.Infrastructure/Routing/RedirectTracker.cs @@ -102,7 +102,6 @@ public void CreateRedirects(IDictionary<(int ContentId, string Culture), (Guid C { try { - IEnumerable allRedirectUrls = _redirectUrlService.GetContentRedirectUrls(contentKey); var newRoute = _publishedUrlProvider.GetUrl(contentKey, UrlMode.Relative, culture).TrimEnd(Constants.CharArrays.ForwardSlash); if (!IsValidRoute(newRoute) || oldRoute == newRoute) @@ -110,6 +109,7 @@ public void CreateRedirects(IDictionary<(int ContentId, string Culture), (Guid C continue; } + IEnumerable allRedirectUrls = _redirectUrlService.GetContentRedirectUrls(contentKey); foreach (IRedirectUrl redirectUrl in allRedirectUrls) { if (redirectUrl.Url == newRoute) From 7abd05d6b85af5b5fa2ee1ffd0a6f309a590e6fc Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Fri, 14 Nov 2025 16:31:31 +0100 Subject: [PATCH 7/9] Extract method refactoring, added explanatory comment, fixed warnings and formatting. --- .../Routing/RedirectTracker.cs | 194 ++++++++++-------- 1 file changed, 104 insertions(+), 90 deletions(-) diff --git a/src/Umbraco.Infrastructure/Routing/RedirectTracker.cs b/src/Umbraco.Infrastructure/Routing/RedirectTracker.cs index 291845de343f..121014104a77 100644 --- a/src/Umbraco.Infrastructure/Routing/RedirectTracker.cs +++ b/src/Umbraco.Infrastructure/Routing/RedirectTracker.cs @@ -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,124 +10,137 @@ using Umbraco.Cms.Core.Services.Navigation; using Umbraco.Extensions; -namespace Umbraco.Cms.Infrastructure.Routing +namespace Umbraco.Cms.Infrastructure.Routing; + +/// +/// Tracks and manages URL redirects for content items, ensuring that old routes are stored and appropriate redirects +/// are created when content URLs change. +/// +internal sealed class RedirectTracker : IRedirectTracker { - internal sealed class RedirectTracker : IRedirectTracker + private readonly ILanguageService _languageService; + private readonly IRedirectUrlService _redirectUrlService; + private readonly IPublishedContentCache _contentCache; + private readonly IDocumentNavigationQueryService _navigationQueryService; + private readonly ILogger _logger; + private readonly IPublishedUrlProvider _publishedUrlProvider; + private readonly IPublishedContentStatusFilteringService _publishedContentStatusFilteringService; + + /// + /// Initializes a new instance of the class. + /// + public RedirectTracker( + ILanguageService languageService, + IRedirectUrlService redirectUrlService, + IPublishedContentCache contentCache, + IDocumentNavigationQueryService navigationQueryService, + ILogger logger, + IPublishedUrlProvider publishedUrlProvider, + IPublishedContentStatusFilteringService publishedContentStatusFilteringService) { - private readonly ILocalizationService _localizationService; - private readonly IRedirectUrlService _redirectUrlService; - private readonly IPublishedContentCache _contentCache; - private readonly IDocumentNavigationQueryService _navigationQueryService; - private readonly ILogger _logger; - private readonly IPublishedUrlProvider _publishedUrlProvider; - private readonly IPublishedContentStatusFilteringService _publishedContentStatusFilteringService; - - public RedirectTracker( - ILocalizationService localizationService, - IRedirectUrlService redirectUrlService, - IPublishedContentCache contentCache, - IDocumentNavigationQueryService navigationQueryService, - ILogger logger, - IPublishedUrlProvider publishedUrlProvider, - IPublishedContentStatusFilteringService publishedContentStatusFilteringService) + _languageService = languageService; + _redirectUrlService = redirectUrlService; + _contentCache = contentCache; + _navigationQueryService = navigationQueryService; + _logger = logger; + _publishedUrlProvider = publishedUrlProvider; + _publishedContentStatusFilteringService = publishedContentStatusFilteringService; + } + + /// + public void StoreOldRoute(IContent entity, Dictionary<(int ContentId, string Culture), (Guid ContentKey, string OldRoute)> oldRoutes) + { + IPublishedContent? entityContent = _contentCache.GetById(entity.Id); + if (entityContent is null) { - _localizationService = localizationService; - _redirectUrlService = redirectUrlService; - _contentCache = contentCache; - _navigationQueryService = navigationQueryService; - _logger = logger; - _publishedUrlProvider = publishedUrlProvider; - _publishedContentStatusFilteringService = publishedContentStatusFilteringService; + return; } - /// - public void StoreOldRoute(IContent entity, Dictionary<(int ContentId, string Culture), (Guid ContentKey, string OldRoute)> oldRoutes) - { - IPublishedContent? entityContent = _contentCache.GetById(entity.Id); - if (entityContent is null) - { - 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() ?? Array.Empty()); + // 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()); - // Get all language ISO codes (in case we're dealing with invariant content with variant ancestors) - var languageIsoCodes = new Lazy(() => _localizationService.GetAllLanguages().Select(x => x.IsoCode).ToArray()); + foreach (IPublishedContent publishedContent in entityContent.DescendantsOrSelf(_navigationQueryService, _publishedContentStatusFilteringService)) + { + // If this entity defines specific cultures, use those instead of the default ones + IEnumerable cultures = publishedContent.Cultures.Any() ? publishedContent.Cultures.Keys : defaultCultures.Value; - foreach (IPublishedContent publishedContent in entityContent.DescendantsOrSelf(_navigationQueryService, _publishedContentStatusFilteringService)) + foreach (var culture in cultures) { - // If this entity defines specific cultures, use those instead of the default ones - IEnumerable cultures = publishedContent.Cultures.Any() ? publishedContent.Cultures.Keys : defaultCultures.Value; - - foreach (var culture in cultures) + try { - try - { - var route = _publishedUrlProvider.GetUrl(publishedContent.Id, UrlMode.Relative, culture).TrimEnd(Constants.CharArrays.ForwardSlash); + var route = _publishedUrlProvider.GetUrl(publishedContent.Id, UrlMode.Relative, culture).TrimEnd(Constants.CharArrays.ForwardSlash); - if (IsValidRoute(route)) - { - oldRoutes[(publishedContent.Id, culture)] = (publishedContent.Key, route); - } - else if (string.IsNullOrEmpty(culture)) + if (IsValidRoute(route)) + { + oldRoutes[(publishedContent.Id, culture)] = (publishedContent.Key, route); + } + else if (string.IsNullOrEmpty(culture)) + { + // Retry using all languages, if this is invariant but has a variant ancestor. + foreach (string languageIsoCode in languageIsoCodes.Value) { - // 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); + if (IsValidRoute(route)) { - route = _publishedUrlProvider.GetUrl(publishedContent.Id, UrlMode.Relative, languageIsoCode).TrimEnd(Constants.CharArrays.ForwardSlash); - if (IsValidRoute(route)) - { - oldRoutes[(publishedContent.Id, languageIsoCode)] = (publishedContent.Key, route); - } + oldRoutes[(publishedContent.Id, languageIsoCode)] = (publishedContent.Key, route); } } } - catch (Exception ex) - { - _logger.LogWarning(ex, "Could not register redirects because the old route couldn't be retrieved for content ID {ContentId} and culture '{Culture}'.", publishedContent.Id, culture); - } + } + catch (Exception ex) + { + _logger.LogWarning(ex, "Could not register redirects because the old route couldn't be retrieved for content ID {ContentId} and culture '{Culture}'.", publishedContent.Id, culture); } } } + } - /// - public void CreateRedirects(IDictionary<(int ContentId, string Culture), (Guid ContentKey, string OldRoute)> oldRoutes) + /// + public void CreateRedirects(IDictionary<(int ContentId, string Culture), (Guid ContentKey, string OldRoute)> oldRoutes) + { + if (!oldRoutes.Any()) { - if (!oldRoutes.Any()) - { - return; - } + return; + } - foreach (((int contentId, string culture), (Guid contentKey, string oldRoute)) in oldRoutes) + foreach (((int contentId, string culture), (Guid contentKey, string oldRoute)) in oldRoutes) + { + try { - try - { - var newRoute = _publishedUrlProvider.GetUrl(contentKey, UrlMode.Relative, culture).TrimEnd(Constants.CharArrays.ForwardSlash); - - if (!IsValidRoute(newRoute) || oldRoute == newRoute) - { - continue; - } + var newRoute = _publishedUrlProvider.GetUrl(contentKey, UrlMode.Relative, culture).TrimEnd(Constants.CharArrays.ForwardSlash); - IEnumerable allRedirectUrls = _redirectUrlService.GetContentRedirectUrls(contentKey); - foreach (IRedirectUrl redirectUrl in allRedirectUrls) - { - if (redirectUrl.Url == newRoute) - { - _redirectUrlService.Delete(redirectUrl.Key); - } - } - - _redirectUrlService.Register(oldRoute, contentKey, culture); - } - catch (Exception ex) + if (!IsValidRoute(newRoute) || oldRoute == newRoute) { - _logger.LogWarning(ex, "Could not track redirects because the new route couldn't be retrieved for content ID {ContentId} and culture '{Culture}'.", contentId, culture); + 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) + { + _logger.LogWarning(ex, "Could not track redirects because the new route couldn't be retrieved for content ID {ContentId} and culture '{Culture}'.", contentId, culture); } } + } - private static bool IsValidRoute([NotNullWhen(true)] string? route) => route is not null && !route.StartsWith("err/"); + private static bool IsValidRoute([NotNullWhen(true)] string? route) => route is not null && !route.StartsWith("err/"); + + private void RemoveSelfReferencingRedirect(Guid contentKey, string route) + { + IEnumerable allRedirectUrls = _redirectUrlService.GetContentRedirectUrls(contentKey); + foreach (IRedirectUrl redirectUrl in allRedirectUrls) + { + if (redirectUrl.Url == route) + { + _redirectUrlService.Delete(redirectUrl.Key); + } + } } } From a3320b9fd6383309f199bb9d0c5abb154ca0872d Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Fri, 14 Nov 2025 16:35:02 +0100 Subject: [PATCH 8/9] Resolved warnings in RedirectService. --- .../Services/RedirectUrlService.cs | 125 ++++++++---------- 1 file changed, 58 insertions(+), 67 deletions(-) diff --git a/src/Umbraco.Core/Services/RedirectUrlService.cs b/src/Umbraco.Core/Services/RedirectUrlService.cs index 9aaf6a2025d8..e5c286caaf76 100644 --- a/src/Umbraco.Core/Services/RedirectUrlService.cs +++ b/src/Umbraco.Core/Services/RedirectUrlService.cs @@ -1,17 +1,21 @@ using Microsoft.Extensions.Logging; using Umbraco.Cms.Core.Events; using Umbraco.Cms.Core.Models; -using Umbraco.Cms.Core.Models.PublishedContent; using Umbraco.Cms.Core.Persistence.Repositories; -using Umbraco.Cms.Core.Routing; using Umbraco.Cms.Core.Scoping; namespace Umbraco.Cms.Core.Services; +/// +/// Provides services for managing redirect URLs. +/// internal sealed class RedirectUrlService : RepositoryService, IRedirectUrlService { private readonly IRedirectUrlRepository _redirectUrlRepository; + /// + /// Initializes a new instance of the class. + /// public RedirectUrlService( ICoreScopeProvider provider, ILoggerFactory loggerFactory, @@ -20,109 +24,99 @@ public RedirectUrlService( : base(provider, loggerFactory, eventMessagesFactory) => _redirectUrlRepository = redirectUrlRepository; + /// 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) { - IRedirectUrl? redir = _redirectUrlRepository.Get(url, contentKey, culture); - if (redir != null) - { - redir.CreateDateUtc = DateTime.UtcNow; - } - else - { - redir = new RedirectUrl { Key = Guid.NewGuid(), Url = url, ContentKey = contentKey, Culture = culture }; - } - - _redirectUrlRepository.Save(redir); - scope.Complete(); + redir.CreateDateUtc = DateTime.UtcNow; } + else + { + redir = new RedirectUrl { Key = Guid.NewGuid(), Url = url, ContentKey = contentKey, Culture = culture }; + } + + _redirectUrlRepository.Save(redir); + scope.Complete(); } + /// public void Delete(IRedirectUrl redirectUrl) { - using (ICoreScope scope = ScopeProvider.CreateCoreScope()) - { - _redirectUrlRepository.Delete(redirectUrl); - scope.Complete(); - } + using ICoreScope scope = ScopeProvider.CreateCoreScope(); + _redirectUrlRepository.Delete(redirectUrl); + scope.Complete(); } + /// public void Delete(Guid id) { - using (ICoreScope scope = ScopeProvider.CreateCoreScope()) - { - _redirectUrlRepository.Delete(id); - scope.Complete(); - } + using ICoreScope scope = ScopeProvider.CreateCoreScope(); + _redirectUrlRepository.Delete(id); + scope.Complete(); } + /// public void DeleteContentRedirectUrls(Guid contentKey) { - using (ICoreScope scope = ScopeProvider.CreateCoreScope()) - { - _redirectUrlRepository.DeleteContentUrls(contentKey); - scope.Complete(); - } + using ICoreScope scope = ScopeProvider.CreateCoreScope(); + _redirectUrlRepository.DeleteContentUrls(contentKey); + scope.Complete(); } + /// public void DeleteAll() { - using (ICoreScope scope = ScopeProvider.CreateCoreScope()) - { - _redirectUrlRepository.DeleteAll(); - scope.Complete(); - } + using ICoreScope scope = ScopeProvider.CreateCoreScope(); + _redirectUrlRepository.DeleteAll(); + scope.Complete(); } + /// public IRedirectUrl? GetMostRecentRedirectUrl(string url) { - using (ICoreScope scope = ScopeProvider.CreateCoreScope(autoComplete: true)) - { - return _redirectUrlRepository.GetMostRecentUrl(url); - } + using ICoreScope scope = ScopeProvider.CreateCoreScope(autoComplete: true); + return _redirectUrlRepository.GetMostRecentUrl(url); } + /// public async Task GetMostRecentRedirectUrlAsync(string url) { - using (ICoreScope scope = ScopeProvider.CreateCoreScope(autoComplete: true)) - { - return await _redirectUrlRepository.GetMostRecentUrlAsync(url); - } + using ICoreScope scope = ScopeProvider.CreateCoreScope(autoComplete: true); + return await _redirectUrlRepository.GetMostRecentUrlAsync(url); } + /// public IEnumerable GetContentRedirectUrls(Guid contentKey) { - using (ICoreScope scope = ScopeProvider.CreateCoreScope(autoComplete: true)) - { - return _redirectUrlRepository.GetContentUrls(contentKey); - } + using ICoreScope scope = ScopeProvider.CreateCoreScope(autoComplete: true); + return _redirectUrlRepository.GetContentUrls(contentKey); } + /// public IEnumerable GetAllRedirectUrls(long pageIndex, int pageSize, out long total) { - using (ICoreScope scope = ScopeProvider.CreateCoreScope(autoComplete: true)) - { - return _redirectUrlRepository.GetAllUrls(pageIndex, pageSize, out total); - } + using ICoreScope scope = ScopeProvider.CreateCoreScope(autoComplete: true); + return _redirectUrlRepository.GetAllUrls(pageIndex, pageSize, out total); } + /// public IEnumerable GetAllRedirectUrls(int rootContentId, long pageIndex, int pageSize, out long total) { - using (ICoreScope scope = ScopeProvider.CreateCoreScope(autoComplete: true)) - { - return _redirectUrlRepository.GetAllUrls(rootContentId, pageIndex, pageSize, out total); - } + using ICoreScope scope = ScopeProvider.CreateCoreScope(autoComplete: true); + return _redirectUrlRepository.GetAllUrls(rootContentId, pageIndex, pageSize, out total); } + /// public IEnumerable SearchRedirectUrls(string searchTerm, long pageIndex, int pageSize, out long total) { - using (ICoreScope scope = ScopeProvider.CreateCoreScope(autoComplete: true)) - { - return _redirectUrlRepository.SearchUrls(searchTerm, pageIndex, pageSize, out total); - } + using ICoreScope scope = ScopeProvider.CreateCoreScope(autoComplete: true); + return _redirectUrlRepository.SearchUrls(searchTerm, pageIndex, pageSize, out total); } + /// public IRedirectUrl? GetMostRecentRedirectUrl(string url, string? culture) { if (string.IsNullOrWhiteSpace(culture)) @@ -130,12 +124,11 @@ public IEnumerable SearchRedirectUrls(string searchTerm, long page return GetMostRecentRedirectUrl(url); } - using (ICoreScope scope = ScopeProvider.CreateCoreScope(autoComplete: true)) - { - return _redirectUrlRepository.GetMostRecentUrl(url, culture); - } + using ICoreScope scope = ScopeProvider.CreateCoreScope(autoComplete: true); + return _redirectUrlRepository.GetMostRecentUrl(url, culture); } + /// public async Task GetMostRecentRedirectUrlAsync(string url, string? culture) { if (string.IsNullOrWhiteSpace(culture)) @@ -143,9 +136,7 @@ public IEnumerable SearchRedirectUrls(string searchTerm, long page return await GetMostRecentRedirectUrlAsync(url); } - using (ICoreScope scope = ScopeProvider.CreateCoreScope(autoComplete: true)) - { - return await _redirectUrlRepository.GetMostRecentUrlAsync(url, culture); - } + using ICoreScope scope = ScopeProvider.CreateCoreScope(autoComplete: true); + return await _redirectUrlRepository.GetMostRecentUrlAsync(url, culture); } } From aceb8f370ac1f864fb598837ae78664623f52799 Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Fri, 14 Nov 2025 16:51:24 +0100 Subject: [PATCH 9/9] Minor naming and formatting refactor in tests. --- .../Routing/RedirectTrackerTests.cs | 73 +++++++++---------- 1 file changed, 35 insertions(+), 38 deletions(-) diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Routing/RedirectTrackerTests.cs b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Routing/RedirectTrackerTests.cs index c16c83496904..2716cee8993d 100644 --- a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Routing/RedirectTrackerTests.cs +++ b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/Routing/RedirectTrackerTests.cs @@ -1,4 +1,5 @@ -using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Logging.Abstractions; using Moq; using NUnit.Framework; using Umbraco.Cms.Core.Cache; @@ -20,9 +21,9 @@ namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Routing; [UmbracoTest(Database = UmbracoTestOptions.Database.NewSchemaPerTest)] public class RedirectTrackerTests : UmbracoIntegrationTestWithContent { - private IRedirectUrlService redirectUrlService => GetRequiredService(); + private IRedirectUrlService RedirectUrlService => GetRequiredService(); - private IContent _subPage; + private IContent _testPage; private const string Url = "RedirectUrl"; @@ -30,17 +31,15 @@ public override void CreateTestData() { base.CreateTestData(); - using (var scope = ScopeProvider.CreateScope()) - { - var repository = MockRepository(); - var rootContent = ContentService.GetRootContent().First(); - var subPages = ContentService.GetPagedChildren(rootContent.Id, 0, 3, out _).ToList(); - _subPage = subPages[0]; + 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 = _subPage.Key, Url = Url, Culture = "en" }); + repository.Save(new RedirectUrl { ContentKey = _testPage.Key, Url = Url, Culture = "en" }); - scope.Complete(); - } + scope.Complete(); } [Test] @@ -49,12 +48,12 @@ 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)> { - [(_subPage.Id, "en")] = (_subPage.Key, "/old-route"), + [(_testPage.Id, "en")] = (_testPage.Key, "/old-route"), }; - var redirectTracker = mockRedirectTracker(); + var redirectTracker = CreateRedirectTracker(); - redirectTracker.StoreOldRoute(_subPage, dict); + redirectTracker.StoreOldRoute(_testPage, dict); Assert.That(dict.Count, Is.EqualTo(1)); Assert.AreEqual(dict.Values.First().OldRoute, Url); @@ -66,13 +65,13 @@ 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)> { - [(_subPage.Id, "en")] = (_subPage.Key, "/old-route"), + [(_testPage.Id, "en")] = (_testPage.Key, "/old-route"), }; - var redirectTracker = mockRedirectTracker(); + var redirectTracker = CreateRedirectTracker(); redirectTracker.CreateRedirects(dict); - var redirects = redirectUrlService.GetContentRedirectUrls(_subPage.Key); + var redirects = RedirectUrlService.GetContentRedirectUrls(_testPage.Key); Assert.IsTrue(redirects.Any(x => x.Url == "/old-route")); } @@ -82,67 +81,65 @@ public void Removes_Self_Referncing_Redirects() { const string newUrl = "newUrl"; - var redirects = redirectUrlService.GetContentRedirectUrls(_subPage.Key); + 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)> { - [(_subPage.Id, "en")] = (_subPage.Key, newUrl), + [(_testPage.Id, "en")] = (_testPage.Key, newUrl), }; - var redirectTracker = mockRedirectTracker(); + var redirectTracker = CreateRedirectTracker(); redirectTracker.CreateRedirects(dict); - redirects = redirectUrlService.GetContentRedirectUrls(_subPage.Key); + redirects = RedirectUrlService.GetContentRedirectUrls(_testPage.Key); Assert.IsFalse(redirects.Any(x => x.Url == Url)); Assert.IsTrue(redirects.Any(x => x.Url == newUrl)); } - private RedirectUrlRepository MockRepository() - { - return new RedirectUrlRepository( + private RedirectUrlRepository CreateRedirectUrlRepository() => + new( (IScopeAccessor)ScopeProvider, AppCaches.Disabled, - Mock.Of>(), + new NullLogger(), Mock.Of(), Mock.Of()); - } - private IRedirectTracker mockRedirectTracker() + private IRedirectTracker CreateRedirectTracker() { var contentType = new Mock(); contentType.SetupGet(c => c.Variations).Returns(ContentVariation.Nothing); - var dict = new Dictionary + var cultures = new Dictionary { - { "en", new PublishedCultureInfo("en", "english", "/en/", DateTime.Now) }, + { "en", new PublishedCultureInfo("en", "english", "/en/", DateTime.UtcNow) }, }; var content = new Mock(); - content.SetupGet(c => c.Key).Returns(_subPage.Key); + content.SetupGet(c => c.Key).Returns(_testPage.Key); content.SetupGet(c => c.ContentType).Returns(contentType.Object); - content.SetupGet(c => c.Cultures).Returns(dict); - content.SetupGet(c => c.Id).Returns(_subPage.Id); + 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(_subPage.Id)) + .Setup(x => x.GetById(_testPage.Id)) .Returns(content.Object); IPublishedUrlProvider publishedUrlProvider = Mock.Of(); Mock.Get(publishedUrlProvider) - .Setup(x => x.GetUrl(_subPage.Key, UrlMode.Relative, "en", null)) + .Setup(x => x.GetUrl(_testPage.Key, UrlMode.Relative, "en", null)) .Returns(Url); Mock.Get(publishedUrlProvider) - .Setup(x => x.GetUrl(_subPage.Id, UrlMode.Relative, "en", null)) + .Setup(x => x.GetUrl(_testPage.Id, UrlMode.Relative, "en", null)) .Returns(Url); return new RedirectTracker( - GetRequiredService(), - redirectUrlService, + GetRequiredService(), + RedirectUrlService, contentCache, GetRequiredService(), GetRequiredService>(),