Redirects: Fixes self referencing redirects (closes #20139)

src/Umbraco.Core/Services/RedirectUrlService.cs

@@ -1,28 +1,46 @@
-using System.Threading.Tasks;
 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;
 
 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;
+    }
 
     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<IRedirectUrl> 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)
             {

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<IUmbracoContextFactory>();
+
         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());

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<IRedirectUrlService>();
 
+    private IPublishedUrlProvider PublishedUrlProvider => GetRequiredService<IPublishedUrlProvider>();
+
     public override void CreateTestData()
     {
         base.CreateTestData();
 
         using (var scope = ScopeProvider.CreateScope())
         {
-            var repository = new RedirectUrlRepository(
-                (IScopeAccessor)ScopeProvider,
-                AppCaches.Disabled,
-                Mock.Of<ILogger<RedirectUrlRepository>>(),
-                Mock.Of<IRepositoryCacheVersionService>(),
-                Mock.Of<ICacheSyncService>());
+            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<IUmbracoContextFactory>();
+
+        const string TestUrl = "testUrl";
+        using (umbracoContextFactory.EnsureUmbracoContext())
+        {
+            RedirectUrlService.Register(TestUrl, _firstSubPage.Key);
+        }
+
+        var redirect = RedirectUrlService.GetMostRecentRedirectUrl(TestUrl, CultureEnglish);
+
+        Assert.AreEqual(redirect.ContentId, _firstSubPage.Id);
+    }
+
+    /// <summary>
+    /// 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.
+    /// </summary>
+    [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<IUmbracoContextFactory>();
+        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<IPublishedUrlProvider>();
+        Mock.Get(publishedUrlProvider)
+            .Setup(x => x.GetUrl(_firstSubPage.Key, UrlMode.Relative, null, null))
+            .Returns(Url);
+
+        return new RedirectUrlService(
+            ScopeProvider,
+            Mock.Of<ILoggerFactory>(),
+            Mock.Of<IEventMessagesFactory>(),
+            MockRepository(),
+            publishedUrlProvider);
+    }
+
+    private RedirectUrlRepository MockRepository()
+    {
+        return new RedirectUrlRepository(
+            (IScopeAccessor)ScopeProvider,
+            AppCaches.Disabled,
+            Mock.Of<ILogger<RedirectUrlRepository>>(),
+            Mock.Of<IRepositoryCacheVersionService>(),
+            Mock.Of<ICacheSyncService>());
+    }
 }