Clear roots before rebuilding navigation dictionary (#18766)

AndyButland · web-flow · commit 5f3ce280321b · 2025-04-04T09:14:48.000+02:00
* Clear roots before rebuilding navigation dictionary.

* Added tests to verify fix.

* Correct test implementation.

* Convert integration tests with method overloads into test cases.

* Integration test compatibility supressions.
diff --git a/src/Umbraco.Core/Services/Navigation/ContentNavigationServiceBase.cs b/src/Umbraco.Core/Services/Navigation/ContentNavigationServiceBase.cs
@@ -293,12 +293,12 @@ public bool RestoreFromBin(Guid key, Guid? targetParentKey = null)
     /// <param name="readLock">The read lock value, should be -333 or -334 for content and media trees.</param>
     /// <param name="objectTypeKey">The key of the object type to rebuild.</param>
     /// <param name="trashed">Indicates whether the items are in the recycle bin.</param>
-    protected async Task HandleRebuildAsync(int readLock, Guid objectTypeKey, bool trashed)
+    protected Task HandleRebuildAsync(int readLock, Guid objectTypeKey, bool trashed)
     {
         // This is only relevant for items in the content and media trees
         if (readLock != Constants.Locks.ContentTree && readLock != Constants.Locks.MediaTree)
         {
-            return;
+            return Task.CompletedTask;
         }
 
         using ICoreScope scope = _coreScopeProvider.CreateCoreScope(autoComplete: true);
@@ -307,14 +307,18 @@ protected async Task HandleRebuildAsync(int readLock, Guid objectTypeKey, bool t
         // Build the corresponding navigation structure
         if (trashed)
         {
+            _recycleBinRoots.Clear();
             IEnumerable<INavigationModel> navigationModels = _navigationRepository.GetTrashedContentNodesByObjectType(objectTypeKey);
             BuildNavigationDictionary(_recycleBinNavigationStructure, _recycleBinRoots,  navigationModels);
         }
         else
         {
+            _roots.Clear();
             IEnumerable<INavigationModel> navigationModels = _navigationRepository.GetContentNodesByObjectType(objectTypeKey);
             BuildNavigationDictionary(_navigationStructure, _roots, navigationModels);
         }
+
+        return Task.CompletedTask;
     }
 
     private bool TryGetParentKeyFromStructure(ConcurrentDictionary<Guid, NavigationNode> structure, Guid childKey, out Guid? parentKey)
diff --git a/tests/Umbraco.Tests.Integration/CompatibilitySuppressions.xml b/tests/Umbraco.Tests.Integration/CompatibilitySuppressions.xml
@@ -78,6 +78,20 @@
     <Right>lib/net9.0/Umbraco.Tests.Integration.dll</Right>
     <IsBaselineSuppression>true</IsBaselineSuppression>
   </Suppression>
+  <Suppression>
+    <DiagnosticId>CP0002</DiagnosticId>
+    <Target>M:Umbraco.Cms.Tests.Integration.Umbraco.Core.Services.DocumentNavigationServiceTests.Bin_Structure_Can_Rebuild</Target>
+    <Left>lib/net9.0/Umbraco.Tests.Integration.dll</Left>
+    <Right>lib/net9.0/Umbraco.Tests.Integration.dll</Right>
+    <IsBaselineSuppression>true</IsBaselineSuppression>
+  </Suppression>
+  <Suppression>
+    <DiagnosticId>CP0002</DiagnosticId>
+    <Target>M:Umbraco.Cms.Tests.Integration.Umbraco.Core.Services.DocumentNavigationServiceTests.Structure_Can_Rebuild</Target>
+    <Left>lib/net9.0/Umbraco.Tests.Integration.dll</Left>
+    <Right>lib/net9.0/Umbraco.Tests.Integration.dll</Right>
+    <IsBaselineSuppression>true</IsBaselineSuppression>
+  </Suppression>
   <Suppression>
     <DiagnosticId>CP0002</DiagnosticId>
     <Target>M:Umbraco.Cms.Tests.Integration.Umbraco.Core.Services.UserServiceCrudTests.Cannot_Request_Disabled_If_Hidden(Umbraco.Cms.Core.Models.Membership.UserState)</Target>
@@ -94,21 +108,21 @@
   </Suppression>
   <Suppression>
     <DiagnosticId>CP0002</DiagnosticId>
-    <Target>M:Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Services.MemberEditingServiceTests.Cannot_Change_IsApproved_Without_Access</Target>
+    <Target>M:Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Services.EntityServiceTests.CreateTestData</Target>
     <Left>lib/net9.0/Umbraco.Tests.Integration.dll</Left>
     <Right>lib/net9.0/Umbraco.Tests.Integration.dll</Right>
     <IsBaselineSuppression>true</IsBaselineSuppression>
   </Suppression>
   <Suppression>
     <DiagnosticId>CP0002</DiagnosticId>
-    <Target>M:Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Services.MemberEditingServiceTests.Cannot_Change_IsLockedOut_Without_Access</Target>
+    <Target>M:Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Services.MemberEditingServiceTests.Cannot_Change_IsApproved_Without_Access</Target>
     <Left>lib/net9.0/Umbraco.Tests.Integration.dll</Left>
     <Right>lib/net9.0/Umbraco.Tests.Integration.dll</Right>
     <IsBaselineSuppression>true</IsBaselineSuppression>
   </Suppression>
   <Suppression>
     <DiagnosticId>CP0002</DiagnosticId>
-    <Target>M:Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Services.EntityServiceTests.CreateTestData</Target>
+    <Target>M:Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Services.MemberEditingServiceTests.Cannot_Change_IsLockedOut_Without_Access</Target>
     <Left>lib/net9.0/Umbraco.Tests.Integration.dll</Left>
     <Right>lib/net9.0/Umbraco.Tests.Integration.dll</Right>
     <IsBaselineSuppression>true</IsBaselineSuppression>
diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Core/Services/DocumentNavigationServiceTests.Rebuild.cs b/tests/Umbraco.Tests.Integration/Umbraco.Core/Services/DocumentNavigationServiceTests.Rebuild.cs
@@ -9,8 +9,9 @@ namespace Umbraco.Cms.Tests.Integration.Umbraco.Core.Services;
 
 public partial class DocumentNavigationServiceTests
 {
-    [Test]
-    public async Task Structure_Can_Rebuild()
+    [TestCase(1, TestName = "Structure_Can_Rebuild")]
+    [TestCase(2, TestName = "Structure_Can_Rebuild_MultipleTimes")]
+    public async Task Structure_Can_Rebuild(int numberOfRebuilds)
     {
         // Arrange
         Guid nodeKey = Root.Key;
@@ -21,6 +22,7 @@ public async Task Structure_Can_Rebuild()
         DocumentNavigationQueryService.TryGetDescendantsKeys(nodeKey, out IEnumerable<Guid> originalDescendantsKeys);
         DocumentNavigationQueryService.TryGetAncestorsKeys(nodeKey, out IEnumerable<Guid> originalAncestorsKeys);
         DocumentNavigationQueryService.TryGetSiblingsKeys(nodeKey, out IEnumerable<Guid> originalSiblingsKeys);
+        DocumentNavigationQueryService.TryGetRootKeys(out IEnumerable<Guid> originalRouteKeys);
 
         // In-memory navigation structure is empty here
         var newDocumentNavigationService = new DocumentNavigationService(
@@ -30,14 +32,18 @@ public async Task Structure_Can_Rebuild()
         var initialNodeExists = newDocumentNavigationService.TryGetParentKey(nodeKey, out _);
 
         // Act
-        await newDocumentNavigationService.RebuildAsync();
+        for (int i = 0; i < numberOfRebuilds; i++)
+        {
+            await newDocumentNavigationService.RebuildAsync();
+        }
 
         // Capture rebuilt state
         var nodeExists = newDocumentNavigationService.TryGetParentKey(nodeKey, out Guid? parentKeyFromRebuild);
         newDocumentNavigationService.TryGetChildrenKeys(nodeKey, out IEnumerable<Guid> childrenKeysFromRebuild);
         newDocumentNavigationService.TryGetDescendantsKeys(nodeKey, out IEnumerable<Guid> descendantsKeysFromRebuild);
         newDocumentNavigationService.TryGetAncestorsKeys(nodeKey, out IEnumerable<Guid> ancestorsKeysFromRebuild);
         newDocumentNavigationService.TryGetSiblingsKeys(nodeKey, out IEnumerable<Guid> siblingsKeysFromRebuild);
+        newDocumentNavigationService.TryGetRootKeys(out IEnumerable<Guid> routeKeysFromRebuild);
 
         // Assert
         Assert.Multiple(() =>
@@ -53,11 +59,13 @@ public async Task Structure_Can_Rebuild()
             CollectionAssert.AreEquivalent(originalDescendantsKeys, descendantsKeysFromRebuild);
             CollectionAssert.AreEquivalent(originalAncestorsKeys, ancestorsKeysFromRebuild);
             CollectionAssert.AreEquivalent(originalSiblingsKeys, siblingsKeysFromRebuild);
+            CollectionAssert.AreEquivalent(originalRouteKeys, routeKeysFromRebuild);
         });
     }
 
-    [Test]
-    public async Task Bin_Structure_Can_Rebuild()
+    [TestCase(1, TestName = "Bin_Structure_Can_Rebuild")]
+    [TestCase(2, TestName = "Bin_Structure_Can_Rebuild_MultipleTimes")]
+    public async Task Bin_Structure_Can_Rebuild(int numberOfRebuilds)
     {
         // Arrange
         Guid nodeKey = Root.Key;
@@ -69,6 +77,7 @@ public async Task Bin_Structure_Can_Rebuild()
         DocumentNavigationQueryService.TryGetDescendantsKeysInBin(nodeKey, out IEnumerable<Guid> originalDescendantsKeys);
         DocumentNavigationQueryService.TryGetAncestorsKeysInBin(nodeKey, out IEnumerable<Guid> originalAncestorsKeys);
         DocumentNavigationQueryService.TryGetSiblingsKeysInBin(nodeKey, out IEnumerable<Guid> originalSiblingsKeys);
+        DocumentNavigationQueryService.TryGetRootKeys(out IEnumerable<Guid> originalRouteKeys);
 
         // In-memory navigation structure is empty here
         var newDocumentNavigationService = new DocumentNavigationService(
@@ -78,14 +87,18 @@ public async Task Bin_Structure_Can_Rebuild()
         var initialNodeExists = newDocumentNavigationService.TryGetParentKeyInBin(nodeKey, out _);
 
         // Act
-        await newDocumentNavigationService.RebuildBinAsync();
+        for (int i = 0; i < numberOfRebuilds; i++)
+        {
+            await newDocumentNavigationService.RebuildBinAsync();
+        }
 
         // Capture rebuilt state
         var nodeExists = newDocumentNavigationService.TryGetParentKeyInBin(nodeKey, out Guid? parentKeyFromRebuild);
         newDocumentNavigationService.TryGetChildrenKeysInBin(nodeKey, out IEnumerable<Guid> childrenKeysFromRebuild);
         newDocumentNavigationService.TryGetDescendantsKeysInBin(nodeKey, out IEnumerable<Guid> descendantsKeysFromRebuild);
         newDocumentNavigationService.TryGetAncestorsKeysInBin(nodeKey, out IEnumerable<Guid> ancestorsKeysFromRebuild);
         newDocumentNavigationService.TryGetSiblingsKeysInBin(nodeKey, out IEnumerable<Guid> siblingsKeysFromRebuild);
+        newDocumentNavigationService.TryGetRootKeys(out IEnumerable<Guid> routeKeysFromRebuild);
 
         // Assert
         Assert.Multiple(() =>
@@ -101,6 +114,7 @@ public async Task Bin_Structure_Can_Rebuild()
             CollectionAssert.AreEquivalent(originalDescendantsKeys, descendantsKeysFromRebuild);
             CollectionAssert.AreEquivalent(originalAncestorsKeys, ancestorsKeysFromRebuild);
             CollectionAssert.AreEquivalent(originalSiblingsKeys, siblingsKeysFromRebuild);
+            CollectionAssert.AreEquivalent(originalRouteKeys, routeKeysFromRebuild);
         });
     }
 }

Original file line number	Diff line number	Diff line change
`@@ -293,12 +293,12 @@ public bool RestoreFromBin(Guid key, Guid? targetParentKey = null)`
`293`	`293`	`/// <param name="readLock">The read lock value, should be -333 or -334 for content and media trees.</param>`
`294`	`294`	`/// <param name="objectTypeKey">The key of the object type to rebuild.</param>`
`295`	`295`	`/// <param name="trashed">Indicates whether the items are in the recycle bin.</param>`
`296`		`- protected async Task HandleRebuildAsync(int readLock, Guid objectTypeKey, bool trashed)`
	`296`	`+ protected Task HandleRebuildAsync(int readLock, Guid objectTypeKey, bool trashed)`
`297`	`297`	`{`
`298`	`298`	`// This is only relevant for items in the content and media trees`
`299`	`299`	`if (readLock != Constants.Locks.ContentTree && readLock != Constants.Locks.MediaTree)`
`300`	`300`	`{`
`301`		`- return;`
	`301`	`+ return Task.CompletedTask;`
`302`	`302`	`}`
`303`	`303`
`304`	`304`	`using ICoreScope scope = _coreScopeProvider.CreateCoreScope(autoComplete: true);`
`@@ -307,14 +307,18 @@ protected async Task HandleRebuildAsync(int readLock, Guid objectTypeKey, bool t`
`307`	`307`	`// Build the corresponding navigation structure`
`308`	`308`	`if (trashed)`
`309`	`309`	`{`
	`310`	`+ _recycleBinRoots.Clear();`
`310`	`311`	`IEnumerable<INavigationModel> navigationModels = _navigationRepository.GetTrashedContentNodesByObjectType(objectTypeKey);`
`311`	`312`	`BuildNavigationDictionary(_recycleBinNavigationStructure, _recycleBinRoots, navigationModels);`
`312`	`313`	`}`
`313`	`314`	`else`
`314`	`315`	`{`
	`316`	`+ _roots.Clear();`
`315`	`317`	`IEnumerable<INavigationModel> navigationModels = _navigationRepository.GetContentNodesByObjectType(objectTypeKey);`
`316`	`318`	`BuildNavigationDictionary(_navigationStructure, _roots, navigationModels);`
`317`	`319`	`}`
	`320`	`+`
	`321`	`+ return Task.CompletedTask;`
`318`	`322`	`}`
`319`	`323`
`320`	`324`	`private bool TryGetParentKeyFromStructure(ConcurrentDictionary<Guid, NavigationNode> structure, Guid childKey, out Guid? parentKey)`