Remove related document URLs to the old key when a document is resaved with a new key.

src/Umbraco.Core/DependencyInjection/UmbracoBuilder.cs

@@ -46,6 +46,7 @@ using Umbraco.Cms.Core.Services.ImportExport;
 using Umbraco.Cms.Core.Services.Navigation;
 using Umbraco.Cms.Core.Services.Querying;
 using Umbraco.Cms.Core.Services.Querying.RecycleBin;
+using Umbraco.Cms.Core.Strings;
 using Umbraco.Cms.Core.Sync;
 using Umbraco.Cms.Core.Telemetry;
 using Umbraco.Cms.Core.Templates;
@@ -297,7 +298,25 @@ namespace Umbraco.Cms.Core.DependencyInjection
             Services.AddUnique<ITagService, TagService>();
             Services.AddUnique<IContentPermissionService, ContentPermissionService>();
             Services.AddUnique<IDictionaryPermissionService, DictionaryPermissionService>();
-            Services.AddUnique<IContentService, ContentService>();
+            Services.AddUnique<IContentService>(factory => new ContentService(
+                factory.GetRequiredService<ICoreScopeProvider>(),
+                factory.GetRequiredService<ILoggerFactory>(),
+                factory.GetRequiredService<IEventMessagesFactory>(),
+                factory.GetRequiredService<IDocumentRepository>(),
+                factory.GetRequiredService<IEntityRepository>(),
+                factory.GetRequiredService<IAuditService>(),
+                factory.GetRequiredService<IContentTypeRepository>(),
+                factory.GetRequiredService<IDocumentBlueprintRepository>(),
+                factory.GetRequiredService<ILanguageRepository>(),
+                factory.GetRequiredService<Lazy<IPropertyValidationService>>(),
+                factory.GetRequiredService<IShortStringHelper>(),
+                factory.GetRequiredService<ICultureImpactFactory>(),
+                factory.GetRequiredService<IUserIdKeyResolver>(),
+                factory.GetRequiredService<PropertyEditorCollection>(),
+                factory.GetRequiredService<IIdKeyMap>(),
+                factory.GetRequiredService<IOptionsMonitor<ContentSettings>>(),
+                factory.GetRequiredService<IRelationService>(),
+                factory.GetRequiredService<IDocumentUrlRepository>()));
             Services.AddUnique<IContentBlueprintEditingService, ContentBlueprintEditingService>();
             Services.AddUnique<IContentEditingService, ContentEditingService>();
             Services.AddUnique<IContentPublishingService, ContentPublishingService>();

src/Umbraco.Core/Services/ContentService.cs

@@ -43,10 +43,15 @@ public class ContentService : RepositoryService, IContentService
     private readonly IIdKeyMap _idKeyMap;
     private ContentSettings _contentSettings;
     private readonly IRelationService _relationService;
+    private readonly IDocumentUrlRepository _documentUrlRepository;
+
     private IQuery<IContent>? _queryNotTrashed;
 
     #region Constructors
 
+    // TODO (V19): When cleaning up the obsolete constructors, also remove the factory registration in UmbracoBuilder.cs.
+    // This has only been added to resolve an issue with ambiguous constructor resolution in this class.
+
     public ContentService(
         ICoreScopeProvider provider,
         ILoggerFactory loggerFactory,
@@ -64,7 +69,8 @@ public class ContentService : RepositoryService, IContentService
         PropertyEditorCollection propertyEditorCollection,
         IIdKeyMap idKeyMap,
         IOptionsMonitor<ContentSettings> optionsMonitor,
-        IRelationService relationService)
+        IRelationService relationService,
+        IDocumentUrlRepository documentUrlRepository)
         : base(provider, loggerFactory, eventMessagesFactory)
     {
         _documentRepository = documentRepository;
@@ -86,6 +92,48 @@ public class ContentService : RepositoryService, IContentService
         });
         _relationService = relationService;
         _logger = loggerFactory.CreateLogger<ContentService>();
+        _documentUrlRepository = documentUrlRepository;
+    }
+
+    [Obsolete("Use the non-obsolete constructor instead. Scheduled removal in v19.")]
+    public ContentService(
+        ICoreScopeProvider provider,
+        ILoggerFactory loggerFactory,
+        IEventMessagesFactory eventMessagesFactory,
+        IDocumentRepository documentRepository,
+        IEntityRepository entityRepository,
+        IAuditService auditService,
+        IContentTypeRepository contentTypeRepository,
+        IDocumentBlueprintRepository documentBlueprintRepository,
+        ILanguageRepository languageRepository,
+        Lazy<IPropertyValidationService> propertyValidationService,
+        IShortStringHelper shortStringHelper,
+        ICultureImpactFactory cultureImpactFactory,
+        IUserIdKeyResolver userIdKeyResolver,
+        PropertyEditorCollection propertyEditorCollection,
+        IIdKeyMap idKeyMap,
+        IOptionsMonitor<ContentSettings> optionsMonitor,
+        IRelationService relationService)
+        : this(
+            provider,
+            loggerFactory,
+            eventMessagesFactory,
+            documentRepository,
+            entityRepository,
+            auditService,
+            contentTypeRepository,
+            documentBlueprintRepository,
+            languageRepository,
+            propertyValidationService,
+            shortStringHelper,
+            cultureImpactFactory,
+            userIdKeyResolver,
+            propertyEditorCollection,
+            idKeyMap,
+            optionsMonitor,
+            relationService,
+            StaticServiceProvider.Instance.GetRequiredService<IDocumentUrlRepository>())
+    {
     }
 
     [Obsolete("Use the non-obsolete constructor instead. Scheduled removal in v19.")]
@@ -1111,6 +1159,16 @@ public class ContentService : RepositoryService, IContentService
                 return OperationResult.Cancel(eventMessages);
             }
 
+            // Changing key is not something we encourage, but it can be done, and there are valid use cases for it.
+            // See: https://github.com/umbraco/Umbraco-CMS/issues/21131
+            // If the key has changed for an existing document, we need to delete any existing URL records for the old key. This is
+            // because this is the only database relation where we use the key as a foreign key constraint.
+            // TODO (V18): Consider removing this and instead adding validation across all entities to prevent changing keys.
+            if (content.HasIdentity && content.IsPropertyDirty(nameof(IContent.Key)))
+            {
+                RemoveDocumentUrls(content);
+            }
+
             scope.WriteLock(Constants.Locks.ContentTree);
             userId ??= Constants.Security.SuperUserId;
 
@@ -1163,6 +1221,20 @@ public class ContentService : RepositoryService, IContentService
         return OperationResult.Succeed(eventMessages);
     }
 
+    private void RemoveDocumentUrls(IContent content)
+    {
+        IContent? existingContent = GetById(content.Id);
+        if (existingContent is null)
+        {
+            return;
+        }
+
+        using ICoreScope scope = ScopeProvider.CreateCoreScope();
+        scope.ReadLock(Constants.Locks.DocumentUrls);
+        _documentUrlRepository.DeleteByDocumentKey([existingContent.Key]);
+        scope.Complete();
+    }
+
     /// <inheritdoc />
     public OperationResult Save(IEnumerable<IContent> contents, int userId = Constants.Security.SuperUserId)
     {

tests/Umbraco.Tests.Integration/Umbraco.Core/Services/ContentServiceTests.cs

@@ -77,6 +77,8 @@ internal sealed class ContentServiceTests : UmbracoIntegrationTestWithContent
 
     private IValueEditorCache ValueEditorCache => GetRequiredService<IValueEditorCache>();
 
+    private IDocumentUrlService DocumentUrlService => GetRequiredService<IDocumentUrlService>();
+
     protected override void CustomTestSetup(IUmbracoBuilder builder) => builder
         .AddNotificationHandler<ContentPublishingNotification, ContentNotificationHandler>()
         .AddNotificationHandler<ContentCopyingNotification, ContentNotificationHandler>()
@@ -1627,6 +1629,37 @@ internal sealed class ContentServiceTests : UmbracoIntegrationTestWithContent
         Assert.That(content.Trashed, Is.True);
     }
 
+    [Test]
+    public void Can_Save_Content_And_Change_Key()
+    {
+        // Arrange
+        var content = ContentService.GetById(Textpage.Id);
+        Assert.That(content.Key, Is.EqualTo(Textpage.Key));
+
+        // - save and publish the content so it has a document URL
+        var saved = ContentService.Save(content, userId: Constants.Security.SuperUserId);
+        var published = ContentService.Publish(content, content.AvailableCultures.ToArray(), userId: Constants.Security.SuperUserId);
+        Assert.IsTrue(saved.Success);
+        Assert.IsTrue(published.Success);
+
+        // - ensure the document URL cache is populated (in production this is done via a cache refresher, but for the test
+        //   it's OK to just call the method directly).
+        DocumentUrlService.CreateOrUpdateUrlSegmentsAsync(content.Key);
+
+        // - prepare a new key
+        var newKey = Guid.NewGuid();
+        content.Key = newKey;
+
+        // Act
+        saved = ContentService.Save(content, userId: Constants.Security.SuperUserId);
+
+        // Assert
+        Assert.IsTrue(saved.Success);
+
+        content = ContentService.GetById(Textpage.Id);
+        Assert.That(content.Key, Is.EqualTo(newKey));
+    }
+
     [Test]
     public void Can_Save_And_Publish_Content()
     {