Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions src/Umbraco.Core/Cache/DistributedCacheExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -465,6 +465,22 @@ public static void RefreshElementCache(this DistributedCache dc, IEnumerable<Tre
UnpublishedCultures = x.UnpublishedCultures?.ToArray(),
}));

/// <summary>
/// Invalidates the element cache for the specified deleted element containers (folders).
/// </summary>
/// <param name="dc">The distributed cache.</param>
/// <param name="deletedContainers">The element containers that were deleted.</param>
/// <remarks>
/// A deleted container's node id is never reused, so its key→id mapping in
/// <see cref="Umbraco.Cms.Core.Services.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.
/// </remarks>
public static void RefreshElementCache(this DistributedCache dc, IEnumerable<EntityContainer> deletedContainers)
=> dc.RefreshByPayload(
ElementCacheRefresher.UniqueId,
deletedContainers.Select(container => new ElementCacheRefresher.JsonPayload(container.Id, container.Key, TreeChangeTypes.Remove)));
Comment thread
AndyButland marked this conversation as resolved.
Outdated

#endregion

#region Published Snapshot
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
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. Without this handler the container's stale mapping survives until the next app restart, and a
/// container recreated under the same key resolves to the old id, so the element tree's children query
/// returns nothing (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.ContainedObjectType == Constants.ObjectTypes.Element)
Comment thread
AndyButland marked this conversation as resolved.
Outdated
.ToArray();

if (elementContainers.Length == 0)
{
return;
}

_distributedCache.RefreshElementCache(elementContainers);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -466,6 +466,7 @@
.AddNotificationHandler<MemberTypeChangedNotification, MemberTypeChangedDistributedCacheNotificationHandler>()
.AddNotificationHandler<ContentTreeChangeNotification, ContentTreeChangeDistributedCacheNotificationHandler>()
.AddNotificationHandler<ElementTreeChangeNotification, ElementTreeChangeDistributedCacheNotificationHandler>()
.AddNotificationHandler<EntityContainerDeletedNotification, ElementContainerDeletedDistributedCacheNotificationHandler>()

Check warning on line 469 in src/Umbraco.Infrastructure/DependencyInjection/UmbracoBuilder.CoreServices.cs

View check run for this annotation

CodeScene Delta Analysis / CodeScene Code Health Review (release/18.0)

❌ Getting worse: Large Method

AddCoreNotifications increases from 125 to 126 lines of code, threshold = 70. Large functions with many lines of code are generally harder to understand and lower the code health. Avoid adding more lines to this function.
;

// add notification handlers for auditing
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
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 Child_Element_Is_Returned_After_Container_Recreated_Under_Same_Key()
Comment thread
AndyButland marked this conversation as resolved.
Outdated
{
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.AreEqual(firstContainer.Id, warmResolve.Result);
Comment thread
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.AreEqual(secondContainer.Id, resolvedAfter.Result, "Container key should resolve to the recreated container id.");
Comment thread
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;
}
}
Loading