-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Elements: Invalidate the id/key map when an element container is deleted (closes #23072) #23074
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
AndyButland
wants to merge
4
commits into
release/18.0
Choose a base branch
from
v18/bugfix/23072-refresh-element-cache-on-container-delete
base: release/18.0
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+300
−0
Open
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
a4a53d8
Refresh the element container cache on delete to ensure the id/key ma…
AndyButland 339741e
Rename and additional asserts in test.
AndyButland 46336ef
Use a dedicated refresher for element container id/key map eviction
AndyButland a99023e
Addressed code review feedback.
AndyButland File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
43 changes: 43 additions & 0 deletions
43
...ificationHandlers/Implement/ElementContainerDeletedDistributedCacheNotificationHandler.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| using Umbraco.Cms.Core.Models; | ||
| using Umbraco.Cms.Core.Notifications; | ||
| using Umbraco.Extensions; | ||
|
|
||
| namespace Umbraco.Cms.Core.Cache; | ||
|
|
||
| /// <summary> | ||
| /// Invalidates element caches when an element container (folder) is deleted, so that its key→id mapping | ||
| /// is evicted from <see cref="Services.IIdKeyMap"/> on every server. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// Element container deletions only publish <see cref="EntityContainerDeletedNotification"/> and an | ||
| /// <see cref="ElementTreeChangeNotification"/> for the contained elements - never for the container node | ||
| /// itself, so without this handler the container's stale id/key mapping survives until the next app | ||
| /// restart (see #23072). | ||
| /// </remarks> | ||
| public sealed class ElementContainerDeletedDistributedCacheNotificationHandler | ||
| : DeletedDistributedCacheNotificationHandlerBase<EntityContainer, EntityContainerDeletedNotification> | ||
| { | ||
| private readonly DistributedCache _distributedCache; | ||
|
|
||
| /// <summary> | ||
| /// Initializes a new instance of the <see cref="ElementContainerDeletedDistributedCacheNotificationHandler"/> class. | ||
| /// </summary> | ||
| /// <param name="distributedCache">The distributed cache.</param> | ||
| public ElementContainerDeletedDistributedCacheNotificationHandler(DistributedCache distributedCache) | ||
| => _distributedCache = distributedCache; | ||
|
|
||
| /// <inheritdoc /> | ||
| protected override void Handle(IEnumerable<EntityContainer> entities, IDictionary<string, object?> state) | ||
| { | ||
| EntityContainer[] elementContainers = entities | ||
| .Where(container => container.ContainerObjectType == Constants.ObjectTypes.ElementContainer) | ||
| .ToArray(); | ||
|
|
||
| if (elementContainers.Length == 0) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| _distributedCache.RemoveElementContainerCache(elementContainers); | ||
| } | ||
| } |
109 changes: 109 additions & 0 deletions
109
src/Umbraco.Core/Cache/Refreshers/Implement/ElementContainerCacheRefresher.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,109 @@ | ||
| using Umbraco.Cms.Core.Events; | ||
| using Umbraco.Cms.Core.Notifications; | ||
| using Umbraco.Cms.Core.Serialization; | ||
| using Umbraco.Cms.Core.Services; | ||
|
|
||
| namespace Umbraco.Cms.Core.Cache; | ||
|
|
||
| /// <summary> | ||
| /// Provides cache refresh functionality for element containers (folders). | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// A deleted container's node id is never reused, so its key→id mapping in <see cref="IIdKeyMap"/> must be | ||
| /// evicted on every server. Otherwise a container recreated under the same key resolves to the stale id and | ||
| /// the element tree's children query returns nothing until the next app restart. This refresher only evicts | ||
| /// the id/key map - element data is unaffected by container changes, so it deliberately avoids the broader | ||
| /// invalidation performed by <see cref="ElementCacheRefresher"/>. | ||
| /// </remarks> | ||
| public sealed class ElementContainerCacheRefresher : PayloadCacheRefresherBase<ElementContainerCacheRefresherNotification, ElementContainerCacheRefresher.JsonPayload> | ||
| { | ||
| private readonly IIdKeyMap _idKeyMap; | ||
|
|
||
| /// <summary> | ||
| /// Initializes a new instance of the <see cref="ElementContainerCacheRefresher"/> class. | ||
| /// </summary> | ||
| public ElementContainerCacheRefresher( | ||
| AppCaches appCaches, | ||
| IJsonSerializer serializer, | ||
| IIdKeyMap idKeyMap, | ||
| IEventAggregator eventAggregator, | ||
| ICacheRefresherNotificationFactory factory) | ||
| : base(appCaches, serializer, eventAggregator, factory) | ||
| => _idKeyMap = idKeyMap; | ||
|
|
||
| #region Json | ||
|
|
||
| /// <summary> | ||
| /// Represents a JSON-serializable payload identifying an element container that changed. | ||
| /// </summary> | ||
| public class JsonPayload | ||
| { | ||
| /// <summary> | ||
| /// Initializes a new instance of the <see cref="JsonPayload"/> class. | ||
| /// </summary> | ||
| /// <param name="id">The unique integer identifier for the container.</param> | ||
| /// <param name="key">The unique GUID key associated with the container.</param> | ||
| public JsonPayload(int id, Guid key) | ||
| { | ||
| Id = id; | ||
| Key = key; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Gets the unique integer identifier for the container. | ||
| /// </summary> | ||
| public int Id { get; } | ||
|
|
||
| /// <summary> | ||
| /// Gets the unique GUID key associated with the container. | ||
| /// </summary> | ||
| public Guid Key { get; } | ||
| } | ||
|
|
||
| #endregion | ||
|
|
||
| #region Define | ||
|
|
||
| /// <summary> | ||
| /// Represents a unique identifier for the cache refresher. | ||
| /// </summary> | ||
| public static readonly Guid UniqueId = Guid.Parse("9C9D8B0E-2F1A-4D63-9C2E-7E6B5A4F3C21"); | ||
|
|
||
| /// <inheritdoc/> | ||
| public override Guid RefresherUniqueId => UniqueId; | ||
|
|
||
| /// <inheritdoc/> | ||
| public override string Name => "Element Container Cache Refresher"; | ||
|
|
||
| #endregion | ||
|
|
||
| #region Refresher | ||
|
|
||
| /// <inheritdoc/> | ||
| public override void Refresh(JsonPayload[] payloads) | ||
| { | ||
| foreach (JsonPayload payload in payloads) | ||
| { | ||
| // Clearing by id also evicts the key→id direction, as the id/key map keeps both in sync. | ||
| _idKeyMap.ClearCache(payload.Id); | ||
| } | ||
|
|
||
| base.Refresh(payloads); | ||
| } | ||
|
|
||
| // These events should never trigger. Everything should be PAYLOAD/JSON. | ||
|
|
||
| /// <inheritdoc/> | ||
| public override void RefreshAll() => throw new NotSupportedException(); | ||
|
|
||
| /// <inheritdoc/> | ||
| public override void Refresh(int id) => throw new NotSupportedException(); | ||
|
|
||
| /// <inheritdoc/> | ||
| public override void Refresh(Guid id) => throw new NotSupportedException(); | ||
|
|
||
| /// <inheritdoc/> | ||
| public override void Remove(int id) => throw new NotSupportedException(); | ||
|
|
||
| #endregion | ||
| } |
19 changes: 19 additions & 0 deletions
19
src/Umbraco.Core/Notifications/ElementContainerCacheRefresherNotification.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| using Umbraco.Cms.Core.Sync; | ||
|
|
||
| namespace Umbraco.Cms.Core.Notifications; | ||
|
|
||
| /// <summary> | ||
| /// A notification that is used to trigger the Element Container Cache Refresher. | ||
| /// </summary> | ||
| public class ElementContainerCacheRefresherNotification : CacheRefresherNotification | ||
| { | ||
| /// <summary> | ||
| /// Initializes a new instance of the <see cref="ElementContainerCacheRefresherNotification"/> class. | ||
| /// </summary> | ||
| /// <param name="messageObject">The refresher payload.</param> | ||
| /// <param name="messageType">Type of the cache refresher message, <see cref="MessageType"/>.</param> | ||
| public ElementContainerCacheRefresherNotification(object messageObject, MessageType messageType) | ||
| : base(messageObject, messageType) | ||
| { | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
114 changes: 114 additions & 0 deletions
114
...ion/Umbraco.Core/Cache/ElementContainerDeletedDistributedCacheNotificationHandlerTests.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,114 @@ | ||
| using NUnit.Framework; | ||
| using Umbraco.Cms.Core; | ||
| using Umbraco.Cms.Core.Cache; | ||
| using Umbraco.Cms.Core.Models; | ||
| using Umbraco.Cms.Core.Models.Entities; | ||
| using Umbraco.Cms.Core.Notifications; | ||
| using Umbraco.Cms.Core.Services; | ||
| using Umbraco.Cms.Core.Services.OperationStatus; | ||
| using Umbraco.Cms.Core.Sync; | ||
| using Umbraco.Cms.Tests.Common.Builders; | ||
| using Umbraco.Cms.Tests.Common.Testing; | ||
| using Umbraco.Cms.Tests.Integration.Testing; | ||
| using Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Services; | ||
|
|
||
| namespace Umbraco.Cms.Tests.Integration.Umbraco.Core.Cache; | ||
|
|
||
| /// <summary> | ||
| /// Tests for <see cref="ElementContainerDeletedDistributedCacheNotificationHandler"/>. | ||
| /// </summary> | ||
| [TestFixture] | ||
| [UmbracoTest(Database = UmbracoTestOptions.Database.NewSchemaPerTest, WithApplication = true)] | ||
| internal sealed class ElementContainerDeletedDistributedCacheNotificationHandlerTests : UmbracoIntegrationTest | ||
| { | ||
| private IElementContainerService ElementContainerService => GetRequiredService<IElementContainerService>(); | ||
|
|
||
| private IContentTypeService ContentTypeService => GetRequiredService<IContentTypeService>(); | ||
|
|
||
| private IElementService ElementService => GetRequiredService<IElementService>(); | ||
|
|
||
| private IEntityService EntityService => GetRequiredService<IEntityService>(); | ||
|
|
||
| private static readonly UmbracoObjectTypes[] _treeObjectTypes = | ||
| [UmbracoObjectTypes.ElementContainer, UmbracoObjectTypes.Element]; | ||
|
|
||
| protected override void CustomTestSetup(IUmbracoBuilder builder) | ||
| { | ||
| // Integration tests use a no-op server messenger and do not register the distributed cache | ||
| // notification handlers by default, so opt in to the element handlers under test and a messenger | ||
| // that delivers cache refreshes locally. | ||
| builder.AddNotificationHandler<ElementTreeChangeNotification, ElementTreeChangeDistributedCacheNotificationHandler>(); | ||
| builder.AddNotificationHandler<EntityContainerDeletedNotification, ElementContainerDeletedDistributedCacheNotificationHandler>(); | ||
| builder.Services.AddUnique<IServerMessenger, ContentEventsTests.LocalServerMessenger>(); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Regression test for https://github.com/umbraco/Umbraco-CMS/issues/23072: the element tree's children | ||
| /// query resolves the container key to an id via <see cref="IIdKeyMap"/>. When a container is deleted the | ||
| /// handler must evict its mapping, otherwise a container recreated under the same key resolves to the old | ||
| /// (now non-existent) id and nested elements stay invisible in the tree until the application is restarted. | ||
| /// </summary> | ||
| [Test] | ||
| public async Task Can_Resolve_Children_After_Container_Recreated_Under_Same_Key() | ||
| { | ||
| IContentType elementType = await CreateElementTypeAsync(); | ||
| var containerKey = Guid.NewGuid(); | ||
|
|
||
| // Create the container and resolve its children once, so its key->id mapping is cached in IdKeyMap. | ||
| EntityContainer firstContainer = await CreateContainerAsync(containerKey, "Container v1"); | ||
| Attempt<int> warmResolve = IdKeyMap.GetIdForKey(containerKey, UmbracoObjectTypes.ElementContainer); | ||
| Assert.IsTrue(warmResolve.Success, "Expected IdKeyMap to resolve the newly created container key."); | ||
| Assert.AreEqual(firstContainer.Id, warmResolve.Result); | ||
|
AndyButland marked this conversation as resolved.
|
||
|
|
||
| // Delete and recreate under the same key - the recreated container gets a new id. | ||
| Attempt<EntityContainer?, EntityContainerOperationStatus> deleteResult = | ||
| await ElementContainerService.DeleteAsync(containerKey, Constants.Security.SuperUserKey); | ||
| Assert.IsTrue(deleteResult.Success, $"Failed to delete container: {deleteResult.Status}"); | ||
|
|
||
| EntityContainer secondContainer = await CreateContainerAsync(containerKey, "Container v2"); | ||
| Assert.AreNotEqual(firstContainer.Id, secondContainer.Id, "Recreated container should have a new id."); | ||
|
|
||
| IElement element = CreateElementUnder(secondContainer.Id, elementType); | ||
|
|
||
| // Without the fix, the stale containerKey->firstContainer.Id mapping survives and the children query | ||
| // resolves to the old (now non-existent) parent id, returning nothing. | ||
| Attempt<int> resolvedAfter = IdKeyMap.GetIdForKey(containerKey, UmbracoObjectTypes.ElementContainer); | ||
| Assert.IsTrue(resolvedAfter.Success, "Expected IdKeyMap to resolve the recreated container key."); | ||
| Assert.AreEqual(secondContainer.Id, resolvedAfter.Result, "Container key should resolve to the recreated container id."); | ||
|
AndyButland marked this conversation as resolved.
|
||
|
|
||
| AssertChildrenContains(containerKey, element.Key); | ||
| } | ||
|
|
||
| private void AssertChildrenContains(Guid containerKey, Guid expectedElementKey) | ||
| { | ||
| IEntitySlim[] children = EntityService | ||
| .GetPagedChildren(containerKey, _treeObjectTypes, _treeObjectTypes, 0, 100, false, out var total) | ||
| .ToArray(); | ||
|
|
||
| Assert.AreEqual(1, total, "Expected the element tree children query to return the nested element."); | ||
| Assert.IsTrue(children.Any(child => child.Key == expectedElementKey), "Nested element was not returned by the children query."); | ||
| } | ||
|
|
||
| private async Task<IContentType> CreateElementTypeAsync() | ||
| { | ||
| IContentType elementType = ContentTypeBuilder.CreateSimpleElementType(); | ||
| await ContentTypeService.CreateAsync(elementType, Constants.Security.SuperUserKey); | ||
| return elementType; | ||
| } | ||
|
|
||
| private async Task<EntityContainer> CreateContainerAsync(Guid key, string name) | ||
| { | ||
| Attempt<EntityContainer?, EntityContainerOperationStatus> result = | ||
| await ElementContainerService.CreateAsync(key, name, null, Constants.Security.SuperUserKey); | ||
| Assert.IsTrue(result.Success, $"Failed to create container: {result.Status}"); | ||
| return result.Result!; | ||
| } | ||
|
|
||
| private IElement CreateElementUnder(int parentId, IContentType elementType) | ||
| { | ||
| var element = new Element($"Element {Guid.NewGuid():N}", parentId, elementType); | ||
| OperationResult saveResult = ElementService.Save(element); | ||
| Assert.IsTrue(saveResult.Success, "Failed to save element."); | ||
| return element; | ||
| } | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.