From e992da71590d85066a6da4e7d0802c63805c7e85 Mon Sep 17 00:00:00 2001 From: Nikolaj Geisle <70372949+Zeegaan@users.noreply.github.com> Date: Mon, 5 Dec 2022 12:25:55 +0100 Subject: [PATCH] Fix concurrency issue in UmbracoMapper (#13524) * Add logging to UmbracoMapper * Add NullLogger to tests Co-authored-by: Zeegaan --- .../Mapping/UmbracoMapper.cs | 34 ++++++++++++++----- .../Mapping/MappingTests.cs | 15 ++++---- .../Security/MemberManagerTests.cs | 3 +- .../Security/MemberUserStoreTests.cs | 3 +- .../Factories/DatabaseSettingsFactoryTests.cs | 4 ++- .../Controllers/MemberControllerUnitTests.cs | 3 +- 6 files changed, 42 insertions(+), 20 deletions(-) diff --git a/src/Umbraco.Infrastructure/Mapping/UmbracoMapper.cs b/src/Umbraco.Infrastructure/Mapping/UmbracoMapper.cs index 09cfcf5aaf..8ebd803aac 100644 --- a/src/Umbraco.Infrastructure/Mapping/UmbracoMapper.cs +++ b/src/Umbraco.Infrastructure/Mapping/UmbracoMapper.cs @@ -1,7 +1,10 @@ using System.Collections; using System.Collections.Concurrent; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging; using Umbraco.Cms.Core.Exceptions; using Umbraco.Cms.Core.Scoping; +using Umbraco.Cms.Web.Common.DependencyInjection; namespace Umbraco.Cms.Core.Mapping; @@ -44,19 +47,32 @@ public class UmbracoMapper : IUmbracoMapper private readonly ConcurrentDictionary>> _ctors = new(); - private readonly ConcurrentDictionary>> _maps = + private readonly ConcurrentDictionary>> _maps = new(); private readonly ICoreScopeProvider _scopeProvider; + private readonly ILogger _logger; /// /// Initializes a new instance of the class. /// /// /// - public UmbracoMapper(MapDefinitionCollection profiles, ICoreScopeProvider scopeProvider) + [Obsolete("Please use ctor that takes an ILogger")] + public UmbracoMapper(MapDefinitionCollection profiles, ICoreScopeProvider scopeProvider) : this(profiles, scopeProvider, StaticServiceProvider.Instance.GetRequiredService>()) + { + } + + /// + /// Initializes a new instance of the class. + /// + /// The MapDefinitionCollection + /// The scope provider + /// The logger + public UmbracoMapper(MapDefinitionCollection profiles, ICoreScopeProvider scopeProvider, ILogger logger) { _scopeProvider = scopeProvider; + _logger = logger; foreach (IMapDefinition profile in profiles) { @@ -119,15 +135,15 @@ public class UmbracoMapper : IUmbracoMapper sourceCtors[targetType] = (source, context) => ctor((TSource)source, context)!; } - Dictionary> sourceMaps = DefineMaps(sourceType); + ConcurrentDictionary> sourceMaps = DefineMaps(sourceType); sourceMaps[targetType] = (source, target, context) => map((TSource)source, (TTarget)target, context); } private Dictionary> DefineCtors(Type sourceType) => _ctors.GetOrAdd(sourceType, _ => new Dictionary>()); - private Dictionary> DefineMaps(Type sourceType) => - _maps.GetOrAdd(sourceType, _ => new Dictionary>()); + private ConcurrentDictionary> DefineMaps(Type sourceType) => + _maps.GetOrAdd(sourceType, _ => new ConcurrentDictionary>()); #endregion @@ -428,7 +444,7 @@ public class UmbracoMapper : IUmbracoMapper return null; } - if (_maps.TryGetValue(sourceType, out Dictionary>? sourceMap) && + if (_maps.TryGetValue(sourceType, out ConcurrentDictionary>? sourceMap) && sourceMap.TryGetValue(targetType, out Action? map)) { return map; @@ -436,7 +452,7 @@ public class UmbracoMapper : IUmbracoMapper // we *may* run this more than once but it does not matter map = null; - foreach ((Type stype, Dictionary> smap) in _maps) + foreach ((Type stype, ConcurrentDictionary> smap) in _maps) { if (!stype.IsAssignableFrom(sourceType)) { @@ -462,9 +478,9 @@ public class UmbracoMapper : IUmbracoMapper { foreach (KeyValuePair> m in sourceMap) { - if (!_maps[sourceType].TryGetValue(m.Key, out _)) + if (!_maps[sourceType].TryAdd(m.Key, m.Value)) { - _maps[sourceType].Add(m.Key, m.Value); + _logger.LogDebug("Duplicate key was found, don't add to dictionary"); } } } diff --git a/tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Mapping/MappingTests.cs b/tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Mapping/MappingTests.cs index 03da5482c1..c02fa62b55 100644 --- a/tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Mapping/MappingTests.cs +++ b/tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Mapping/MappingTests.cs @@ -6,6 +6,7 @@ using System.Collections.Generic; using System.Data; using System.Linq; using System.Threading; +using Microsoft.Extensions.Logging.Abstractions; using Moq; using NUnit.Framework; using Umbraco.Cms.Core.Events; @@ -44,7 +45,7 @@ public class MappingTests public void SimpleMap() { var definitions = new MapDefinitionCollection(() => new IMapDefinition[] { new MapperDefinition1() }); - var mapper = new UmbracoMapper(definitions, _scopeProvider); + var mapper = new UmbracoMapper(definitions, _scopeProvider, NullLogger.Instance); var thing1 = new Thing1 { Value = "value" }; var thing2 = mapper.Map(thing1); @@ -66,7 +67,7 @@ public class MappingTests public void EnumerableMap() { var definitions = new MapDefinitionCollection(() => new IMapDefinition[] { new MapperDefinition1() }); - var mapper = new UmbracoMapper(definitions, _scopeProvider); + var mapper = new UmbracoMapper(definitions, _scopeProvider, NullLogger.Instance); var thing1A = new Thing1 { Value = "valueA" }; var thing1B = new Thing1 { Value = "valueB" }; @@ -97,7 +98,7 @@ public class MappingTests public void InheritedMap() { var definitions = new MapDefinitionCollection(() => new IMapDefinition[] { new MapperDefinition1() }); - var mapper = new UmbracoMapper(definitions, _scopeProvider); + var mapper = new UmbracoMapper(definitions, _scopeProvider, NullLogger.Instance); var thing3 = new Thing3 { Value = "value" }; var thing2 = mapper.Map(thing3); @@ -119,7 +120,7 @@ public class MappingTests public void CollectionsMap() { var definitions = new MapDefinitionCollection(() => new IMapDefinition[] { new MapperDefinition2() }); - var mapper = new UmbracoMapper(definitions, _scopeProvider); + var mapper = new UmbracoMapper(definitions, _scopeProvider, NullLogger.Instance); // can map a PropertyCollection var source = new PropertyCollection(); @@ -135,7 +136,7 @@ public class MappingTests new MapperDefinition1(), new MapperDefinition3(), }); - var mapper = new UmbracoMapper(definitions, _scopeProvider); + var mapper = new UmbracoMapper(definitions, _scopeProvider, NullLogger.Instance); // the mapper currently has a map from Thing1 to Thing2 // because Thing3 inherits from Thing1, it will map a Thing3 instance, @@ -191,7 +192,7 @@ public class MappingTests public void EnumMap() { var definitions = new MapDefinitionCollection(() => new IMapDefinition[] { new MapperDefinition4() }); - var mapper = new UmbracoMapper(definitions, _scopeProvider); + var mapper = new UmbracoMapper(definitions, _scopeProvider, NullLogger.Instance); var thing5 = new Thing5 { Fruit1 = Thing5Enum.Apple, Fruit2 = Thing5Enum.Banana, Fruit3 = Thing5Enum.Cherry }; @@ -207,7 +208,7 @@ public class MappingTests public void NullPropertyMap() { var definitions = new MapDefinitionCollection(() => new IMapDefinition[] { new MapperDefinition5() }); - var mapper = new UmbracoMapper(definitions, _scopeProvider); + var mapper = new UmbracoMapper(definitions, _scopeProvider, NullLogger.Instance); var thing7 = new Thing7(); diff --git a/tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberManagerTests.cs b/tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberManagerTests.cs index 12b4984bec..0026240e8b 100644 --- a/tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberManagerTests.cs +++ b/tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberManagerTests.cs @@ -5,6 +5,7 @@ using System.Threading.Tasks; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Identity; using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Logging.Abstractions; using Microsoft.Extensions.Options; using Moq; using NUnit.Framework; @@ -50,7 +51,7 @@ public class MemberManagerTests _fakeMemberStore = new MemberUserStore( _mockMemberService.Object, - new UmbracoMapper(new MapDefinitionCollection(() => mapDefinitions), scopeProvider), + new UmbracoMapper(new MapDefinitionCollection(() => mapDefinitions), scopeProvider, NullLogger.Instance), scopeProvider, new IdentityErrorDescriber(), Mock.Of(), diff --git a/tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberUserStoreTests.cs b/tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberUserStoreTests.cs index 083ed00bae..9e2a769e74 100644 --- a/tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberUserStoreTests.cs +++ b/tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberUserStoreTests.cs @@ -5,6 +5,7 @@ using System.Linq; using System.Threading; using System.Threading.Tasks; using Microsoft.AspNetCore.Identity; +using Microsoft.Extensions.Logging.Abstractions; using Moq; using NUnit.Framework; using Umbraco.Cms.Core.Events; @@ -42,7 +43,7 @@ public class MemberUserStoreTests return new MemberUserStore( _mockMemberService.Object, - new UmbracoMapper(new MapDefinitionCollection(() => new List()), mockScopeProvider.Object), + new UmbracoMapper(new MapDefinitionCollection(() => new List()), mockScopeProvider.Object, NullLogger.Instance), mockScopeProvider.Object, new IdentityErrorDescriber(), Mock.Of(), diff --git a/tests/Umbraco.Tests.UnitTests/Umbraco.New.Cms.Infrastructure/Factories/DatabaseSettingsFactoryTests.cs b/tests/Umbraco.Tests.UnitTests/Umbraco.New.Cms.Infrastructure/Factories/DatabaseSettingsFactoryTests.cs index d718daba50..fd2c7315b0 100644 --- a/tests/Umbraco.Tests.UnitTests/Umbraco.New.Cms.Infrastructure/Factories/DatabaseSettingsFactoryTests.cs +++ b/tests/Umbraco.Tests.UnitTests/Umbraco.New.Cms.Infrastructure/Factories/DatabaseSettingsFactoryTests.cs @@ -1,6 +1,7 @@ using System; using System.Collections.Generic; using System.Linq; +using Microsoft.Extensions.Logging.Abstractions; using Moq; using NUnit.Framework; using Umbraco.Cms.Core.Configuration.Models; @@ -101,7 +102,8 @@ public class DatabaseSettingsFactoryTests { var mapper = new UmbracoMapper( new MapDefinitionCollection(Enumerable.Empty), - Mock.Of()); + Mock.Of(), + NullLogger.Instance); var definition = new InstallerViewModelsMapDefinition(); definition.DefineMaps(mapper); diff --git a/tests/Umbraco.Tests.UnitTests/Umbraco.Web.BackOffice/Controllers/MemberControllerUnitTests.cs b/tests/Umbraco.Tests.UnitTests/Umbraco.Web.BackOffice/Controllers/MemberControllerUnitTests.cs index c3fc359cf5..cc620940cf 100644 --- a/tests/Umbraco.Tests.UnitTests/Umbraco.Web.BackOffice/Controllers/MemberControllerUnitTests.cs +++ b/tests/Umbraco.Tests.UnitTests/Umbraco.Web.BackOffice/Controllers/MemberControllerUnitTests.cs @@ -9,6 +9,7 @@ using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Identity; using Microsoft.AspNetCore.Routing; using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Logging.Abstractions; using Microsoft.Extensions.Options; using Moq; using NUnit.Framework; @@ -587,7 +588,7 @@ public class MemberControllerUnitTests It.IsAny(), It.IsAny()) == Mock.Of()); - _mapper = new UmbracoMapper(map, scopeProvider); + _mapper = new UmbracoMapper(map, scopeProvider, NullLogger.Instance); return new MemberController( new DefaultCultureDictionary(