diff --git a/src/Umbraco.Core/Persistence/Repositories/RepositoryCacheKeys.cs b/src/Umbraco.Core/Persistence/Repositories/RepositoryCacheKeys.cs index a6b6c16aa5..aca68e9762 100644 --- a/src/Umbraco.Core/Persistence/Repositories/RepositoryCacheKeys.cs +++ b/src/Umbraco.Core/Persistence/Repositories/RepositoryCacheKeys.cs @@ -1,3 +1,5 @@ +using System.Runtime.InteropServices; + namespace Umbraco.Cms.Core.Persistence.Repositories; /// @@ -5,15 +7,34 @@ namespace Umbraco.Cms.Core.Persistence.Repositories; /// public static class RepositoryCacheKeys { - // used to cache keys so we don't keep allocating strings + /// + /// A cache for the keys we don't keep allocating strings. + /// private static readonly Dictionary Keys = new(); + /// + /// Gets the repository cache key for the provided type. + /// public static string GetKey() { Type type = typeof(T); - return Keys.TryGetValue(type, out var key) ? key : Keys[type] = "uRepo_" + type.Name + "_"; + + // The following code is a micro-optimization to avoid an unnecessary lookup in the Keys dictionary, when writing the newly created key. + // Previously, the code was: + // return Keys.TryGetValue(type, out var key) + // ? key + // : Keys[type] = "uRepo_" + type.Name + "_"; + + // Look up the existing value or get a reference to the newly created default value. + ref string? key = ref CollectionsMarshal.GetValueRefOrAddDefault(Keys, type, out _); + + // As we have the reference, we can just assign it if null, without the expensive write back to the dictionary. + return key ??= "uRepo_" + type.Name + "_"; } + /// + /// Gets the repository cache key for the provided type and Id. + /// public static string GetKey(TId? id) { if (EqualityComparer.Default.Equals(id, default)) diff --git a/src/Umbraco.Core/Scoping/LockingMechanism.cs b/src/Umbraco.Core/Scoping/LockingMechanism.cs index 0cee4293f6..e078b047e6 100644 --- a/src/Umbraco.Core/Scoping/LockingMechanism.cs +++ b/src/Umbraco.Core/Scoping/LockingMechanism.cs @@ -1,3 +1,4 @@ +using System.Runtime.InteropServices; using System.Text; using Microsoft.Extensions.Logging; using Umbraco.Cms.Core.Collections; @@ -7,7 +8,7 @@ using Umbraco.Extensions; namespace Umbraco.Cms.Core.Scoping; /// -/// Mechanism for handling read and write locks +/// Mechanism for handling read and write locks. /// public class LockingMechanism : ILockingMechanism { @@ -189,24 +190,43 @@ public class LockingMechanism : ILockingMechanism /// Lock ID to increment. /// Instance ID of the scope requesting the lock. /// Reference to the dictionary to increment on - private void IncrementLock(int lockId, Guid instanceId, ref Dictionary>? locks) + /// Internal for tests. + internal static void IncrementLock(int lockId, Guid instanceId, ref Dictionary>? locks) { // Since we've already checked that we're the parent in the WriteLockInner method, we don't need to check again. - // If it's the very first time a lock has been requested the WriteLocks dict hasn't been instantiated yet. - locks ??= new Dictionary>(); + // If it's the very first time a lock has been requested the WriteLocks dictionary hasn't been instantiated yet. + locks ??= []; - // Try and get the dict associated with the scope id. - var locksDictFound = locks.TryGetValue(instanceId, out Dictionary? locksDict); + // Try and get the dictionary associated with the scope id. + + // The following code is a micro-optimization. + // GetValueRefOrAddDefault does lookup or creation with only one hash key generation, internal bucket lookup and value lookup in the bucket. + // This compares to doing it twice when initializing, one for the lookup and one for the insertion of the initial value, we had with the + // previous code: + // var locksDictFound = locks.TryGetValue(instanceId, out Dictionary? locksDict); + // if (locksDictFound) + // { + // locksDict!.TryGetValue(lockId, out var value); + // locksDict[lockId] = value + 1; + // } + // else + // { + // // The scope hasn't requested a lock yet, so we have to create a dict for it. + // locks.Add(instanceId, new Dictionary()); + // locks[instanceId][lockId] = 1; + // } + + ref Dictionary? locksDict = ref CollectionsMarshal.GetValueRefOrAddDefault(locks, instanceId, out bool locksDictFound); if (locksDictFound) { - locksDict!.TryGetValue(lockId, out var value); - locksDict[lockId] = value + 1; + // By getting a reference to any existing or default 0 value, we can increment it without the expensive write back into the dictionary. + ref int value = ref CollectionsMarshal.GetValueRefOrAddDefault(locksDict!, lockId, out _); + value++; } else { - // The scope hasn't requested a lock yet, so we have to create a dict for it. - locks.Add(instanceId, new Dictionary()); - locks[instanceId][lockId] = 1; + // The scope hasn't requested a lock yet, so we have to create a dictionary for it. + locksDict = new Dictionary { { lockId, 1 } }; } } diff --git a/src/Umbraco.Infrastructure/PropertyEditors/BlockValuePropertyValueEditorBase.cs b/src/Umbraco.Infrastructure/PropertyEditors/BlockValuePropertyValueEditorBase.cs index 00bc627d60..91d3ddf2ad 100644 --- a/src/Umbraco.Infrastructure/PropertyEditors/BlockValuePropertyValueEditorBase.cs +++ b/src/Umbraco.Infrastructure/PropertyEditors/BlockValuePropertyValueEditorBase.cs @@ -184,6 +184,12 @@ public abstract class BlockValuePropertyValueEditorBase : DataV foreach (BlockItemData item in items) { + // if changes were made to the element type variations, we need those changes reflected in the block property values. + // for regular content this happens when a content type is saved (copies of property values are created in the DB), + // but for local block level properties we don't have that kind of handling, so we to do it manually. + // to be friendly we'll map "formerly invariant properties" to the default language ISO code instead of performing a + // hard reset of the property values (which would likely be the most correct thing to do from a data point of view). + item.Values = _blockEditorVarianceHandler.AlignPropertyVarianceAsync(item.Values, culture).GetAwaiter().GetResult(); foreach (BlockPropertyValue blockPropertyValue in item.Values) { IPropertyType? propertyType = blockPropertyValue.PropertyType; @@ -199,13 +205,6 @@ public abstract class BlockValuePropertyValueEditorBase : DataV continue; } - // if changes were made to the element type variation, we need those changes reflected in the block property values. - // for regular content this happens when a content type is saved (copies of property values are created in the DB), - // but for local block level properties we don't have that kind of handling, so we to do it manually. - // to be friendly we'll map "formerly invariant properties" to the default language ISO code instead of performing a - // hard reset of the property values (which would likely be the most correct thing to do from a data point of view). - _blockEditorVarianceHandler.AlignPropertyVarianceAsync(blockPropertyValue, propertyType, culture).GetAwaiter().GetResult(); - if (!valueEditorsByKey.TryGetValue(propertyType.DataTypeKey, out IDataValueEditor? valueEditor)) { var configuration = _dataTypeConfigurationCache.GetConfiguration(propertyType.DataTypeKey); diff --git a/src/Umbraco.Infrastructure/PropertyEditors/ValueConverters/BlockEditorVarianceHandler.cs b/src/Umbraco.Infrastructure/PropertyEditors/ValueConverters/BlockEditorVarianceHandler.cs index 815bc0f1b0..042d57ff04 100644 --- a/src/Umbraco.Infrastructure/PropertyEditors/ValueConverters/BlockEditorVarianceHandler.cs +++ b/src/Umbraco.Infrastructure/PropertyEditors/ValueConverters/BlockEditorVarianceHandler.cs @@ -25,15 +25,7 @@ public sealed class BlockEditorVarianceHandler _contentTypeService = contentTypeService; } - /// - /// Aligns a block property value for variance changes. - /// - /// The block property value to align. - /// The underlying property type. - /// The culture being handled (null if invariant). - /// - /// Used for aligning variance changes when editing content. - /// + [Obsolete("Please use the method that allows alignment for a collection of values. Scheduled for removal in V17.")] public async Task AlignPropertyVarianceAsync(BlockPropertyValue blockPropertyValue, IPropertyType propertyType, string? culture) { culture ??= await _languageService.GetDefaultIsoCodeAsync(); @@ -45,6 +37,48 @@ public sealed class BlockEditorVarianceHandler } } + /// + /// Aligns a collection of block property values for variance changes. + /// + /// The block property values to align. + /// The culture being handled (null if invariant). + /// + /// Used for aligning variance changes when editing content. + /// + public async Task> AlignPropertyVarianceAsync(IList blockPropertyValues, string? culture) + { + var defaultIsoCodeAsync = await _languageService.GetDefaultIsoCodeAsync(); + culture ??= defaultIsoCodeAsync; + + var valuesToRemove = new List(); + foreach (BlockPropertyValue blockPropertyValue in blockPropertyValues) + { + IPropertyType? propertyType = blockPropertyValue.PropertyType; + if (propertyType is null) + { + throw new ArgumentException("One or more block properties did not have a resolved property type. Block editor values must be resolved before attempting to map them to editor.", nameof(blockPropertyValues)); + } + + if (propertyType.VariesByCulture() == VariesByCulture(blockPropertyValue)) + { + continue; + } + + if (propertyType.VariesByCulture() is false && blockPropertyValue.Culture.InvariantEquals(defaultIsoCodeAsync) is false) + { + valuesToRemove.Add(blockPropertyValue); + } + else + { + blockPropertyValue.Culture = propertyType.VariesByCulture() + ? culture + : null; + } + } + + return blockPropertyValues.Except(valuesToRemove).ToList(); + } + /// /// Aligns a block property value for variance changes. /// @@ -199,6 +233,8 @@ public sealed class BlockEditorVarianceHandler blockValue.Expose.Add(new BlockItemVariation(contentData.Key, value.Culture, value.Segment)); } } + + blockValue.Expose = blockValue.Expose.DistinctBy(e => $"{e.ContentKey}.{e.Culture}.{e.Segment}").ToList(); } private static bool VariesByCulture(BlockPropertyValue blockPropertyValue) diff --git a/src/Umbraco.Web.UI.Login/package-lock.json b/src/Umbraco.Web.UI.Login/package-lock.json index d2685028b3..847fd8258f 100644 --- a/src/Umbraco.Web.UI.Login/package-lock.json +++ b/src/Umbraco.Web.UI.Login/package-lock.json @@ -9,7 +9,7 @@ "@umbraco-cms/backoffice": "15.2.1", "msw": "^2.7.0", "typescript": "^5.7.3", - "vite": "^6.2.2", + "vite": "^6.2.3", "vite-tsconfig-paths": "^5.1.4" }, "engines": { @@ -3772,9 +3772,9 @@ } }, "node_modules/vite": { - "version": "6.2.2", - "resolved": "https://registry.npmjs.org/vite/-/vite-6.2.2.tgz", - "integrity": "sha512-yW7PeMM+LkDzc7CgJuRLMW2Jz0FxMOsVJ8Lv3gpgW9WLcb9cTW+121UEr1hvmfR7w3SegR5ItvYyzVz1vxNJgQ==", + "version": "6.2.3", + "resolved": "https://registry.npmjs.org/vite/-/vite-6.2.3.tgz", + "integrity": "sha512-IzwM54g4y9JA/xAeBPNaDXiBF8Jsgl3VBQ2YQ/wOY6fyW3xMdSoltIV3Bo59DErdqdE6RxUfv8W69DvUorE4Eg==", "dev": true, "license": "MIT", "dependencies": { diff --git a/src/Umbraco.Web.UI.Login/package.json b/src/Umbraco.Web.UI.Login/package.json index 449fa53bfe..02dc6a9ee9 100644 --- a/src/Umbraco.Web.UI.Login/package.json +++ b/src/Umbraco.Web.UI.Login/package.json @@ -16,7 +16,7 @@ "@umbraco-cms/backoffice": "15.2.1", "msw": "^2.7.0", "typescript": "^5.7.3", - "vite": "^6.2.2", + "vite": "^6.2.3", "vite-tsconfig-paths": "^5.1.4" }, "msw": { diff --git a/tests/Umbraco.Tests.Common/Builders/ContentBuilder.cs b/tests/Umbraco.Tests.Common/Builders/ContentBuilder.cs index 53c2f50f10..1fd66da312 100644 --- a/tests/Umbraco.Tests.Common/Builders/ContentBuilder.cs +++ b/tests/Umbraco.Tests.Common/Builders/ContentBuilder.cs @@ -185,10 +185,7 @@ public class ContentBuilder { if (string.IsNullOrWhiteSpace(name)) { - if (_cultureNames.TryGetValue(culture, out _)) - { - _cultureNames.Remove(culture); - } + _cultureNames.Remove(culture); } else { diff --git a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/PropertyEditors/BlockListElementLevelVariationTests.Editing.cs b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/PropertyEditors/BlockListElementLevelVariationTests.Editing.cs index 295b134e22..8af4cf4987 100644 --- a/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/PropertyEditors/BlockListElementLevelVariationTests.Editing.cs +++ b/tests/Umbraco.Tests.Integration/Umbraco.Infrastructure/PropertyEditors/BlockListElementLevelVariationTests.Editing.cs @@ -1,11 +1,10 @@ -using Microsoft.Extensions.DependencyInjection; -using NUnit.Framework; +using NUnit.Framework; using Umbraco.Cms.Core; -using Umbraco.Cms.Core.Configuration.Models; using Umbraco.Cms.Core.Models; using Umbraco.Cms.Core.Models.Blocks; using Umbraco.Cms.Core.Models.ContentEditing; using Umbraco.Cms.Core.Models.Membership; +using Umbraco.Cms.Core.PropertyEditors; using Umbraco.Cms.Core.Services; using Umbraco.Cms.Tests.Common.Builders; using Umbraco.Cms.Tests.Common.Builders.Extensions; @@ -742,6 +741,185 @@ internal partial class BlockListElementLevelVariationTests } } + [Test] + public async Task Can_Align_Culture_Variance_For_Variant_Element_Types() + { + var elementType = CreateElementType(ContentVariation.Culture); + var blockListDataType = await CreateBlockListDataType(elementType); + var contentType = CreateContentType(ContentVariation.Nothing, blockListDataType); + + var content = CreateContent( + contentType, + elementType, + new List + { + new() { Alias = "invariantText", Value = "The invariant content value" }, + new() { Alias = "variantText", Value = "Another invariant content value" } + }, + new List + { + new() { Alias = "invariantText", Value = "The invariant settings value" }, + new() { Alias = "variantText", Value = "Another invariant settings value" } + }, + false); + + contentType.Variations = ContentVariation.Culture; + ContentTypeService.Save(contentType); + + // re-fetch content + content = ContentService.GetById(content.Key); + + var valueEditor = (BlockListPropertyEditorBase.BlockListEditorPropertyValueEditor)blockListDataType.Editor!.GetValueEditor(); + + var blockListValue = valueEditor.ToEditor(content!.Properties["blocks"]!) as BlockListValue; + Assert.IsNotNull(blockListValue); + Assert.Multiple(() => + { + Assert.AreEqual(1, blockListValue.ContentData.Count); + Assert.AreEqual(2, blockListValue.ContentData.First().Values.Count); + var invariantValue = blockListValue.ContentData.First().Values.First(value => value.Alias == "invariantText"); + var variantValue = blockListValue.ContentData.First().Values.First(value => value.Alias == "variantText"); + Assert.IsNull(invariantValue.Culture); + Assert.AreEqual("en-US", variantValue.Culture); + }); + Assert.Multiple(() => + { + Assert.AreEqual(1, blockListValue.SettingsData.Count); + Assert.AreEqual(2, blockListValue.SettingsData.First().Values.Count); + var invariantValue = blockListValue.SettingsData.First().Values.First(value => value.Alias == "invariantText"); + var variantValue = blockListValue.SettingsData.First().Values.First(value => value.Alias == "variantText"); + Assert.IsNull(invariantValue.Culture); + Assert.AreEqual("en-US", variantValue.Culture); + }); + Assert.Multiple(() => + { + Assert.AreEqual(1, blockListValue.Expose.Count); + Assert.AreEqual("en-US", blockListValue.Expose.First().Culture); + }); + } + + [TestCase(ContentVariation.Culture)] + [TestCase(ContentVariation.Nothing)] + public async Task Can_Turn_Invariant_Element_Variant(ContentVariation contentTypeVariation) + { + var elementType = CreateElementType(ContentVariation.Nothing); + var blockListDataType = await CreateBlockListDataType(elementType); + var contentType = CreateContentType(contentTypeVariation, blockListDataType); + + var content = CreateContent( + contentType, + elementType, + new List + { + new() { Alias = "invariantText", Value = "The invariant content value" }, + new() { Alias = "variantText", Value = "Another invariant content value" } + }, + new List + { + new() { Alias = "invariantText", Value = "The invariant settings value" }, + new() { Alias = "variantText", Value = "Another invariant settings value" } + }, + false); + + elementType.Variations = ContentVariation.Culture; + elementType.PropertyTypes.First(p => p.Alias == "variantText").Variations = ContentVariation.Culture; + ContentTypeService.Save(elementType); + + // re-fetch content + content = ContentService.GetById(content.Key); + + var valueEditor = (BlockListPropertyEditorBase.BlockListEditorPropertyValueEditor)blockListDataType.Editor!.GetValueEditor(); + + var blockListValue = valueEditor.ToEditor(content!.Properties["blocks"]!) as BlockListValue; + Assert.IsNotNull(blockListValue); + Assert.Multiple(() => + { + Assert.AreEqual(1, blockListValue.ContentData.Count); + Assert.AreEqual(2, blockListValue.ContentData.First().Values.Count); + var invariantValue = blockListValue.ContentData.First().Values.First(value => value.Alias == "invariantText"); + var variantValue = blockListValue.ContentData.First().Values.First(value => value.Alias == "variantText"); + Assert.IsNull(invariantValue.Culture); + Assert.AreEqual("en-US", variantValue.Culture); + }); + Assert.Multiple(() => + { + Assert.AreEqual(1, blockListValue.SettingsData.Count); + Assert.AreEqual(2, blockListValue.SettingsData.First().Values.Count); + var invariantValue = blockListValue.SettingsData.First().Values.First(value => value.Alias == "invariantText"); + var variantValue = blockListValue.SettingsData.First().Values.First(value => value.Alias == "variantText"); + Assert.IsNull(invariantValue.Culture); + Assert.AreEqual("en-US", variantValue.Culture); + }); + Assert.Multiple(() => + { + Assert.AreEqual(1, blockListValue.Expose.Count); + Assert.AreEqual("en-US", blockListValue.Expose.First().Culture); + }); + } + + [TestCase(ContentVariation.Nothing)] + [TestCase(ContentVariation.Culture)] + public async Task Can_Turn_Variant_Element_Invariant(ContentVariation contentTypeVariation) + { + var elementType = CreateElementType(ContentVariation.Culture); + var blockListDataType = await CreateBlockListDataType(elementType); + var contentType = CreateContentType(contentTypeVariation, blockListDataType); + + var content = CreateContent( + contentType, + elementType, + new List + { + new() { Alias = "invariantText", Value = "The invariant content value" }, + new() { Alias = "variantText", Value = "Variant content in English", Culture = "en-US" }, + new() { Alias = "variantText", Value = "Variant content in Danish", Culture = "da-DK" } + }, + new List + { + new() { Alias = "invariantText", Value = "The invariant settings value" }, + new() { Alias = "variantText", Value = "Variant settings in English", Culture = "en-US" }, + new() { Alias = "variantText", Value = "Variant settings in Danish", Culture = "da-DK" } + }, + false); + + elementType.Variations = ContentVariation.Nothing; + elementType.PropertyTypes.First(p => p.Alias == "variantText").Variations = ContentVariation.Nothing; + ContentTypeService.Save(elementType); + + // re-fetch content + content = ContentService.GetById(content.Key); + + var valueEditor = (BlockListPropertyEditorBase.BlockListEditorPropertyValueEditor)blockListDataType.Editor!.GetValueEditor(); + + var blockListValue = valueEditor.ToEditor(content!.Properties["blocks"]!) as BlockListValue; + Assert.IsNotNull(blockListValue); + Assert.Multiple(() => + { + Assert.AreEqual(1, blockListValue.ContentData.Count); + Assert.AreEqual(2, blockListValue.ContentData.First().Values.Count); + var invariantValue = blockListValue.ContentData.First().Values.First(value => value.Alias == "invariantText"); + var variantValue = blockListValue.ContentData.First().Values.First(value => value.Alias == "variantText"); + Assert.IsNull(invariantValue.Culture); + Assert.IsNull(variantValue.Culture); + Assert.AreEqual("Variant content in English", variantValue.Value); + }); + Assert.Multiple(() => + { + Assert.AreEqual(1, blockListValue.SettingsData.Count); + Assert.AreEqual(2, blockListValue.SettingsData.First().Values.Count); + var invariantValue = blockListValue.SettingsData.First().Values.First(value => value.Alias == "invariantText"); + var variantValue = blockListValue.SettingsData.First().Values.First(value => value.Alias == "variantText"); + Assert.IsNull(invariantValue.Culture); + Assert.IsNull(variantValue.Culture); + Assert.AreEqual("Variant settings in English", variantValue.Value); + }); + Assert.Multiple(() => + { + Assert.AreEqual(1, blockListValue.Expose.Count); + Assert.IsNull(blockListValue.Expose.First().Culture); + }); + } + private async Task CreateLimitedUser() { var userGroupService = GetRequiredService(); diff --git a/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Persistence/Repositories/RepositoryCacheKeysTests.cs b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Persistence/Repositories/RepositoryCacheKeysTests.cs new file mode 100644 index 0000000000..fef83541b9 --- /dev/null +++ b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Persistence/Repositories/RepositoryCacheKeysTests.cs @@ -0,0 +1,26 @@ +// Copyright (c) Umbraco. +// See LICENSE for more details. + +using NUnit.Framework; +using Umbraco.Cms.Core.Models; +using Umbraco.Cms.Core.Persistence.Repositories; + +namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Core.PropertyEditors; + +[TestFixture] +public class RepositoryCacheKeysTests +{ + [Test] + public void GetKey_Returns_Expected_Key_For_Type() + { + var key = RepositoryCacheKeys.GetKey(); + Assert.AreEqual("uRepo_IContent_", key); + } + + [Test] + public void GetKey_Returns_Expected_Key_For_Type_And_Id() + { + var key = RepositoryCacheKeys.GetKey(1000); + Assert.AreEqual("uRepo_IContent_1000", key); + } +} diff --git a/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Scoping/LockingMechanismTests.cs b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Scoping/LockingMechanismTests.cs new file mode 100644 index 0000000000..eb2d6abdfb --- /dev/null +++ b/tests/Umbraco.Tests.UnitTests/Umbraco.Core/Scoping/LockingMechanismTests.cs @@ -0,0 +1,45 @@ +// Copyright (c) Umbraco. +// See LICENSE for more details. + +using NUnit.Framework; +using Umbraco.Cms.Core.Scoping; + +namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Core.Scoping; + +[TestFixture] +internal class LockingMechanismTests +{ + private const int LockId = 1000; + private const int LockId2 = 1001; + private static readonly Guid _scopeInstanceId = Guid.NewGuid(); + + [Test] + public void IncrementLock_WithoutLocksDictionary_CreatesLock() + { + var locks = new Dictionary>(); + LockingMechanism.IncrementLock(LockId, _scopeInstanceId, ref locks); + Assert.AreEqual(1, locks.Count); + Assert.AreEqual(1, locks[_scopeInstanceId][LockId]); + } + + [Test] + public void IncrementLock_WithExistingLocksDictionary_CreatesLock() + { + var locks = new Dictionary>() + { + { + _scopeInstanceId, + new Dictionary() + { + { LockId, 100 }, + { LockId2, 200 } + } + } + }; + LockingMechanism.IncrementLock(LockId, _scopeInstanceId, ref locks); + Assert.AreEqual(1, locks.Count); + Assert.AreEqual(2, locks[_scopeInstanceId].Count); + Assert.AreEqual(101, locks[_scopeInstanceId][LockId]); + Assert.AreEqual(200, locks[_scopeInstanceId][LockId2]); + } +}