Fix concurrency issue in UmbracoMapper

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<Type, Dictionary<Type, Func<object, MapperContext, object>>> _ctors =
         new();
 
-    private readonly ConcurrentDictionary<Type, Dictionary<Type, Action<object, object, MapperContext>>> _maps =
+    private readonly ConcurrentDictionary<Type, ConcurrentDictionary<Type, Action<object, object, MapperContext>>> _maps =
         new();
 
     private readonly ICoreScopeProvider _scopeProvider;
+    private readonly ILogger<UmbracoMapper> _logger;
 
     /// <summary>
     ///     Initializes a new instance of the <see cref="UmbracoMapper" /> class.
     /// </summary>
     /// <param name="profiles"></param>
     /// <param name="scopeProvider"></param>
-    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<ILogger<UmbracoMapper>>())
+    {
+    }
+
+    /// <summary>
+    ///     Initializes a new instance of the <see cref="UmbracoMapper" /> class.
+    /// </summary>
+    /// <param name="profiles">The MapDefinitionCollection</param>
+    /// <param name="scopeProvider">The scope provider</param>
+    /// <param name="logger">The logger</param>
+    public UmbracoMapper(MapDefinitionCollection profiles, ICoreScopeProvider scopeProvider, ILogger<UmbracoMapper> logger)
     {
         _scopeProvider = scopeProvider;
+        _logger = logger;
 
         foreach (IMapDefinition profile in profiles)
         {
@@ -119,15 +135,15 @@ public void Define<TSource, TTarget>(
             sourceCtors[targetType] = (source, context) => ctor((TSource)source, context)!;
         }
 
-        Dictionary<Type, Action<object, object, MapperContext>> sourceMaps = DefineMaps(sourceType);
+        ConcurrentDictionary<Type, Action<object, object, MapperContext>> sourceMaps = DefineMaps(sourceType);
         sourceMaps[targetType] = (source, target, context) => map((TSource)source, (TTarget)target, context);
     }
 
     private Dictionary<Type, Func<object, MapperContext, object>> DefineCtors(Type sourceType) =>
         _ctors.GetOrAdd(sourceType, _ => new Dictionary<Type, Func<object, MapperContext, object>>());
 
-    private Dictionary<Type, Action<object, object, MapperContext>> DefineMaps(Type sourceType) =>
-        _maps.GetOrAdd(sourceType, _ => new Dictionary<Type, Action<object, object, MapperContext>>());
+    private ConcurrentDictionary<Type, Action<object, object, MapperContext>> DefineMaps(Type sourceType) =>
+        _maps.GetOrAdd(sourceType, _ => new ConcurrentDictionary<Type, Action<object, object, MapperContext>>());
 
     #endregion
 
@@ -428,15 +444,15 @@ public TTarget Map<TSource, TTarget>(TSource source, TTarget target, MapperConte
             return null;
         }
 
-        if (_maps.TryGetValue(sourceType, out Dictionary<Type, Action<object, object, MapperContext>>? sourceMap) &&
+        if (_maps.TryGetValue(sourceType, out ConcurrentDictionary<Type, Action<object, object, MapperContext>>? sourceMap) &&
             sourceMap.TryGetValue(targetType, out Action<object, object, MapperContext>? map))
         {
             return map;
         }
 
         // we *may* run this more than once but it does not matter
         map = null;
-        foreach ((Type stype, Dictionary<Type, Action<object, object, MapperContext>> smap) in _maps)
+        foreach ((Type stype, ConcurrentDictionary<Type, Action<object, object, MapperContext>> smap) in _maps)
         {
             if (!stype.IsAssignableFrom(sourceType))
             {
@@ -462,9 +478,9 @@ public TTarget Map<TSource, TTarget>(TSource source, TTarget target, MapperConte
         {
             foreach (KeyValuePair<Type, Action<object, object, MapperContext>> 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");
                 }
             }
         }

tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Mapping/MappingTests.cs

@@ -6,6 +6,7 @@
 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 void MockScopeProvider()
     public void SimpleMap()
     {
         var definitions = new MapDefinitionCollection(() => new IMapDefinition[] { new MapperDefinition1() });
-        var mapper = new UmbracoMapper(definitions, _scopeProvider);
+        var mapper = new UmbracoMapper(definitions, _scopeProvider, NullLogger<UmbracoMapper>.Instance);
 
         var thing1 = new Thing1 { Value = "value" };
         var thing2 = mapper.Map<Thing1, Thing2>(thing1);
@@ -66,7 +67,7 @@ public void SimpleMap()
     public void EnumerableMap()
     {
         var definitions = new MapDefinitionCollection(() => new IMapDefinition[] { new MapperDefinition1() });
-        var mapper = new UmbracoMapper(definitions, _scopeProvider);
+        var mapper = new UmbracoMapper(definitions, _scopeProvider, NullLogger<UmbracoMapper>.Instance);
 
         var thing1A = new Thing1 { Value = "valueA" };
         var thing1B = new Thing1 { Value = "valueB" };
@@ -97,7 +98,7 @@ public void EnumerableMap()
     public void InheritedMap()
     {
         var definitions = new MapDefinitionCollection(() => new IMapDefinition[] { new MapperDefinition1() });
-        var mapper = new UmbracoMapper(definitions, _scopeProvider);
+        var mapper = new UmbracoMapper(definitions, _scopeProvider, NullLogger<UmbracoMapper>.Instance);
 
         var thing3 = new Thing3 { Value = "value" };
         var thing2 = mapper.Map<Thing3, Thing2>(thing3);
@@ -119,7 +120,7 @@ public void InheritedMap()
     public void CollectionsMap()
     {
         var definitions = new MapDefinitionCollection(() => new IMapDefinition[] { new MapperDefinition2() });
-        var mapper = new UmbracoMapper(definitions, _scopeProvider);
+        var mapper = new UmbracoMapper(definitions, _scopeProvider, NullLogger<UmbracoMapper>.Instance);
 
         // can map a PropertyCollection
         var source = new PropertyCollection();
@@ -135,7 +136,7 @@ public void ConcurrentMap()
             new MapperDefinition1(),
             new MapperDefinition3(),
         });
-        var mapper = new UmbracoMapper(definitions, _scopeProvider);
+        var mapper = new UmbracoMapper(definitions, _scopeProvider, NullLogger<UmbracoMapper>.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 @@ void ThreadLoop()
     public void EnumMap()
     {
         var definitions = new MapDefinitionCollection(() => new IMapDefinition[] { new MapperDefinition4() });
-        var mapper = new UmbracoMapper(definitions, _scopeProvider);
+        var mapper = new UmbracoMapper(definitions, _scopeProvider, NullLogger<UmbracoMapper>.Instance);
 
         var thing5 = new Thing5 { Fruit1 = Thing5Enum.Apple, Fruit2 = Thing5Enum.Banana, Fruit3 = Thing5Enum.Cherry };
 
@@ -207,7 +208,7 @@ public void EnumMap()
     public void NullPropertyMap()
     {
         var definitions = new MapDefinitionCollection(() => new IMapDefinition[] { new MapperDefinition5() });
-        var mapper = new UmbracoMapper(definitions, _scopeProvider);
+        var mapper = new UmbracoMapper(definitions, _scopeProvider, NullLogger<UmbracoMapper>.Instance);
 
         var thing7 = new Thing7();
 

tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberManagerTests.cs

@@ -5,6 +5,7 @@
 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 MemberManager CreateSut()
 
         _fakeMemberStore = new MemberUserStore(
             _mockMemberService.Object,
-            new UmbracoMapper(new MapDefinitionCollection(() => mapDefinitions), scopeProvider),
+            new UmbracoMapper(new MapDefinitionCollection(() => mapDefinitions), scopeProvider, NullLogger<UmbracoMapper>.Instance),
             scopeProvider,
             new IdentityErrorDescriber(),
             Mock.Of<IPublishedSnapshotAccessor>(),

tests/Umbraco.Tests.UnitTests/Umbraco.Infrastructure/Security/MemberUserStoreTests.cs

@@ -5,6 +5,7 @@
 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 MemberUserStore CreateSut()
 
         return new MemberUserStore(
             _mockMemberService.Object,
-            new UmbracoMapper(new MapDefinitionCollection(() => new List<IMapDefinition>()), mockScopeProvider.Object),
+            new UmbracoMapper(new MapDefinitionCollection(() => new List<IMapDefinition>()), mockScopeProvider.Object, NullLogger<UmbracoMapper>.Instance),
             mockScopeProvider.Object,
             new IdentityErrorDescriber(),
             Mock.Of<IPublishedSnapshotAccessor>(),

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 @@ private IUmbracoMapper CreateMapper()
     {
         var mapper = new UmbracoMapper(
             new MapDefinitionCollection(Enumerable.Empty<IMapDefinition>),
-            Mock.Of<ICoreScopeProvider>());
+            Mock.Of<ICoreScopeProvider>(),
+            NullLogger<UmbracoMapper>.Instance);
 
         var definition = new InstallerViewModelsMapDefinition();
         definition.DefineMaps(mapper);

tests/Umbraco.Tests.UnitTests/Umbraco.Web.BackOffice/Controllers/MemberControllerUnitTests.cs

@@ -9,6 +9,7 @@
 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 @@ private MemberController CreateSut(
             It.IsAny<bool>(),
             It.IsAny<bool>()) == Mock.Of<ICoreScope>());
 
-        _mapper = new UmbracoMapper(map, scopeProvider);
+        _mapper = new UmbracoMapper(map, scopeProvider, NullLogger<UmbracoMapper>.Instance);
 
         return new MemberController(
             new DefaultCultureDictionary(