From 609e9f43e1b55f0987ecf982ed9dadc0c73ec607 Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Wed, 26 Mar 2025 07:37:04 +0100 Subject: [PATCH 1/6] Fixed issue where siblings of type at route are omitted from the result. (#18796) --- .../Extensions/PublishedContentExtensions.cs | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-) diff --git a/src/Umbraco.Core/Extensions/PublishedContentExtensions.cs b/src/Umbraco.Core/Extensions/PublishedContentExtensions.cs index 6b9b0359d7..f8ef83c350 100644 --- a/src/Umbraco.Core/Extensions/PublishedContentExtensions.cs +++ b/src/Umbraco.Core/Extensions/PublishedContentExtensions.cs @@ -3850,9 +3850,8 @@ public static class PublishedContentExtensions if (parentKey.HasValue) { - IEnumerable childrenKeys; var foundChildrenKeys = contentTypeAlias is null - ? navigationQueryService.TryGetChildrenKeys(parentKey.Value, out childrenKeys) + ? navigationQueryService.TryGetChildrenKeys(parentKey.Value, out IEnumerable childrenKeys) : navigationQueryService.TryGetChildrenKeysOfType(parentKey.Value, contentTypeAlias, out childrenKeys); return foundChildrenKeys @@ -3860,19 +3859,12 @@ public static class PublishedContentExtensions : []; } - IEnumerable rootKeys; var foundRootKeys = contentTypeAlias is null - ? navigationQueryService.TryGetRootKeys(out rootKeys) + ? navigationQueryService.TryGetRootKeys(out IEnumerable rootKeys) : navigationQueryService.TryGetRootKeysOfType(contentTypeAlias, out rootKeys); - if (foundRootKeys) - { - IEnumerable rootKeysArray = rootKeys as Guid[] ?? rootKeys.ToArray(); - return rootKeysArray.Contains(content.Key) - ? publishedStatusFilteringService.FilterAvailable(rootKeysArray, culture) - : []; - } - - return []; + return foundRootKeys + ? publishedStatusFilteringService.FilterAvailable(rootKeys, culture) + : []; } } From 54601d42e25b771993d5d7b0d32d41bda646946f Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Tue, 25 Mar 2025 15:26:37 +0000 Subject: [PATCH 2/6] Bump vite from 6.2.2 to 6.2.3 in /src/Umbraco.Web.UI.Login Bumps [vite](https://github.com/vitejs/vite/tree/HEAD/packages/vite) from 6.2.2 to 6.2.3. - [Release notes](https://github.com/vitejs/vite/releases) - [Changelog](https://github.com/vitejs/vite/blob/v6.2.3/packages/vite/CHANGELOG.md) - [Commits](https://github.com/vitejs/vite/commits/v6.2.3/packages/vite) --- updated-dependencies: - dependency-name: vite dependency-type: direct:development ... Signed-off-by: dependabot[bot] --- src/Umbraco.Web.UI.Login/package-lock.json | 8 ++++---- src/Umbraco.Web.UI.Login/package.json | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) 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": { From 37035e6e7fb25177182d13b7c5dcec55a65b22c8 Mon Sep 17 00:00:00 2001 From: Kenn Jacobsen Date: Wed, 26 Mar 2025 11:27:23 +0100 Subject: [PATCH 3/6] Clean up leftover block item data when changing element variance (#18804) --- .../BlockValuePropertyValueEditorBase.cs | 13 +- .../BlockEditorVarianceHandler.cs | 54 ++++- ...kListElementLevelVariationTests.Editing.cs | 184 +++++++++++++++++- 3 files changed, 232 insertions(+), 19 deletions(-) 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/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(); From 028da4545ebe23df44ed03f2cbc7df21d41775c3 Mon Sep 17 00:00:00 2001 From: Henrik Date: Wed, 26 Mar 2025 16:01:53 +0100 Subject: [PATCH 4/6] Reduce CPU time when initiating RepositoryCacheKeys (#18267) * Avoid an unneeded lookups in the Keys dictionary when initiating key cache * Add further comments and unit tests around updated code. --------- Co-authored-by: Andy Butland --- .../Repositories/RepositoryCacheKeys.cs | 25 ++++++++++++++++-- .../Repositories/RepositoryCacheKeysTests.cs | 26 +++++++++++++++++++ 2 files changed, 49 insertions(+), 2 deletions(-) create mode 100644 tests/Umbraco.Tests.UnitTests/Umbraco.Core/Persistence/Repositories/RepositoryCacheKeysTests.cs 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/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); + } +} From 95e89f84816dd0ca9738f5cd82930656a58e7f68 Mon Sep 17 00:00:00 2001 From: Henrik Date: Wed, 26 Mar 2025 16:55:03 +0100 Subject: [PATCH 5/6] Avoid a hash key generation and lookup when inserting in the LockingMechanism (#18243) * Avoid a hash key generation and lookup when inserting in the LockingMechanism * Added comments for CollectionsMarshal.GetValueRefOrAddDefault * Added further comments and tests. --------- Co-authored-by: Andy Butland --- src/Umbraco.Core/Scoping/LockingMechanism.cs | 42 ++++++++++++----- .../Builders/ContentBuilder.cs | 5 +-- .../Scoping/LockingMechanismTests.cs | 45 +++++++++++++++++++ 3 files changed, 77 insertions(+), 15 deletions(-) create mode 100644 tests/Umbraco.Tests.UnitTests/Umbraco.Core/Scoping/LockingMechanismTests.cs 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/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.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]); + } +} From 685a05827e797c22d2daff00fb5a6b8a944b1b8b Mon Sep 17 00:00:00 2001 From: Andy Butland Date: Fri, 28 Mar 2025 11:16:03 +0100 Subject: [PATCH 6/6] Adds MemberTwoFactorLoginService. (#18810) --- .../DependencyInjection/UmbracoBuilder.cs | 1 + .../Services/IMemberTwoFactorLoginService.cs | 37 ++++++++++ .../Services/ITwoFactorLoginService.cs | 6 +- .../Services/MemberTwoFactorLoginService.cs | 72 +++++++++++++++++++ .../Services/UserTwoFactorLoginService.cs | 6 +- 5 files changed, 116 insertions(+), 6 deletions(-) create mode 100644 src/Umbraco.Core/Services/IMemberTwoFactorLoginService.cs create mode 100644 src/Umbraco.Core/Services/MemberTwoFactorLoginService.cs diff --git a/src/Umbraco.Core/DependencyInjection/UmbracoBuilder.cs b/src/Umbraco.Core/DependencyInjection/UmbracoBuilder.cs index 3d36c67d3a..24cc907339 100644 --- a/src/Umbraco.Core/DependencyInjection/UmbracoBuilder.cs +++ b/src/Umbraco.Core/DependencyInjection/UmbracoBuilder.cs @@ -422,6 +422,7 @@ namespace Umbraco.Cms.Core.DependencyInjection // Two factor providers Services.AddUnique(); Services.AddUnique(); + Services.AddUnique(); // Add Query services Services.AddUnique(); diff --git a/src/Umbraco.Core/Services/IMemberTwoFactorLoginService.cs b/src/Umbraco.Core/Services/IMemberTwoFactorLoginService.cs new file mode 100644 index 0000000000..3a7ec99c8e --- /dev/null +++ b/src/Umbraco.Core/Services/IMemberTwoFactorLoginService.cs @@ -0,0 +1,37 @@ +using Umbraco.Cms.Core.Models; +using Umbraco.Cms.Core.Security; +using Umbraco.Cms.Core.Services.OperationStatus; + +namespace Umbraco.Cms.Core.Services; + +/// +/// A member specific Two factor service, that ensures the member exists before doing the job. +/// +public interface IMemberTwoFactorLoginService +{ + /// + /// Disables a specific two factor provider on a specific member. + /// + Task> DisableAsync(Guid memberKey, string providerName); + + /// + /// Gets the two factor providers on a specific member. + /// + Task, TwoFactorOperationStatus>> GetProviderNamesAsync(Guid memberKey); + + /// + /// The returned type can be anything depending on the setup providers. You will need to cast it to the type handled by + /// the provider. + /// + Task> GetSetupInfoAsync(Guid memberKey, string providerName); + + /// + /// Validates and Saves. + /// + Task> ValidateAndSaveAsync(string providerName, Guid memberKey, string modelSecret, string modelCode); + + /// + /// Disables 2FA with Code. + /// + Task> DisableByCodeAsync(string providerName, Guid memberKey, string code); +} diff --git a/src/Umbraco.Core/Services/ITwoFactorLoginService.cs b/src/Umbraco.Core/Services/ITwoFactorLoginService.cs index a5ad0d84e8..01558376e3 100644 --- a/src/Umbraco.Core/Services/ITwoFactorLoginService.cs +++ b/src/Umbraco.Core/Services/ITwoFactorLoginService.cs @@ -29,7 +29,7 @@ public interface ITwoFactorLoginService : IService /// The returned type can be anything depending on the setup providers. You will need to cast it to the type handled by /// the provider. /// - [Obsolete("Use IUserTwoFactorLoginService.GetSetupInfoAsync. This will be removed in Umbraco 15.")] + [Obsolete("Use IUserTwoFactorLoginService.GetSetupInfoAsync or IMemberTwoFactorLoginService.GetSetupInfoAsync. Scheduled for removal in Umbraco 16.")] Task GetSetupInfoAsync(Guid userOrMemberKey, string providerName); /// @@ -60,13 +60,13 @@ public interface ITwoFactorLoginService : IService /// /// Disables 2FA with Code. /// - [Obsolete("Use IUserTwoFactorLoginService.DisableByCodeAsync. This will be removed in Umbraco 15.")] + [Obsolete("Use IUserTwoFactorLoginService.DisableByCodeAsync or IMemberTwoFactorLoginService.DisableByCodeAsync. Scheduled for removal in Umbraco 16.")] Task DisableWithCodeAsync(string providerName, Guid userOrMemberKey, string code); /// /// Validates and Saves. /// - [Obsolete("Use IUserTwoFactorLoginService.ValidateAndSaveAsync. This will be removed in Umbraco 15.")] + [Obsolete("Use IUserTwoFactorLoginService.ValidateAndSaveAsync or IMemberTwoFactorLoginService.ValidateAndSaveAsync. Scheduled for removal in Umbraco 16.")] Task ValidateAndSaveAsync(string providerName, Guid userKey, string secret, string code); } diff --git a/src/Umbraco.Core/Services/MemberTwoFactorLoginService.cs b/src/Umbraco.Core/Services/MemberTwoFactorLoginService.cs new file mode 100644 index 0000000000..b21558b834 --- /dev/null +++ b/src/Umbraco.Core/Services/MemberTwoFactorLoginService.cs @@ -0,0 +1,72 @@ +using Umbraco.Cms.Core.Models; +using Umbraco.Cms.Core.Scoping; +using Umbraco.Cms.Core.Security; +using Umbraco.Cms.Core.Services.OperationStatus; + +namespace Umbraco.Cms.Core.Services; + +/// +internal class MemberTwoFactorLoginService : TwoFactorLoginServiceBase, IMemberTwoFactorLoginService +{ + private readonly IMemberService _memberService; + + public MemberTwoFactorLoginService( + ITwoFactorLoginService twoFactorLoginService, + IEnumerable twoFactorSetupGenerators, + IMemberService memberService, + ICoreScopeProvider scopeProvider) + : base(twoFactorLoginService, twoFactorSetupGenerators, scopeProvider) => + _memberService = memberService; + + /// + public override async Task> DisableAsync(Guid memberKey, string providerName) + { + IMember? member = _memberService.GetByKey(memberKey); + + if (member is null) + { + return Attempt.Fail(TwoFactorOperationStatus.UserNotFound); + } + + return await base.DisableAsync(memberKey, providerName); + } + + /// + public override async Task, TwoFactorOperationStatus>> GetProviderNamesAsync(Guid memberKey) + { + IMember? member = _memberService.GetByKey(memberKey); + + if (member is null) + { + return Attempt.FailWithStatus(TwoFactorOperationStatus.UserNotFound, Enumerable.Empty()); + } + + return await base.GetProviderNamesAsync(memberKey); + } + + /// + public override async Task> GetSetupInfoAsync(Guid memberKey, string providerName) + { + IMember? member = _memberService.GetByKey(memberKey); + + if (member is null) + { + return Attempt.FailWithStatus(TwoFactorOperationStatus.UserNotFound, new NoopSetupTwoFactorModel()); + } + + return await base.GetSetupInfoAsync(memberKey, providerName); + } + + /// + public override async Task> ValidateAndSaveAsync(string providerName, Guid memberKey, string secret, string code) + { + IMember? member = _memberService.GetByKey(memberKey); + + if (member is null) + { + return Attempt.Fail(TwoFactorOperationStatus.UserNotFound); + } + + return await base.ValidateAndSaveAsync(providerName, memberKey, secret, code); + } +} diff --git a/src/Umbraco.Core/Services/UserTwoFactorLoginService.cs b/src/Umbraco.Core/Services/UserTwoFactorLoginService.cs index 89241d31f2..10a9de140f 100644 --- a/src/Umbraco.Core/Services/UserTwoFactorLoginService.cs +++ b/src/Umbraco.Core/Services/UserTwoFactorLoginService.cs @@ -32,7 +32,7 @@ internal class UserTwoFactorLoginService : TwoFactorLoginServiceBase, IUserTwoFa return await base.DisableAsync(userKey, providerName); } - /// + /// public override async Task, TwoFactorOperationStatus>> GetProviderNamesAsync(Guid userKey) { IUser? user = await _userService.GetAsync(userKey); @@ -45,7 +45,7 @@ internal class UserTwoFactorLoginService : TwoFactorLoginServiceBase, IUserTwoFa return await base.GetProviderNamesAsync(userKey); } - /// + /// public override async Task> GetSetupInfoAsync(Guid userKey, string providerName) { IUser? user = await _userService.GetAsync(userKey); @@ -58,7 +58,7 @@ internal class UserTwoFactorLoginService : TwoFactorLoginServiceBase, IUserTwoFa return await base.GetSetupInfoAsync(userKey, providerName); } - /// + /// public override async Task> ValidateAndSaveAsync(string providerName, Guid userKey, string secret, string code) { IUser? user = await _userService.GetAsync(userKey);