From 278c9f9be5ec97c58e3b2d3e15f5fbd20df62751 Mon Sep 17 00:00:00 2001 From: Geir Sagberg Date: Mon, 22 Feb 2016 14:56:12 +0100 Subject: [PATCH 1/2] Avoid hash collisions by including more keys --- .../CachingBehaviorTests.cs | 39 +++++++- .../ComplexMapsParentsAndChlidTest.cs | 8 +- .../HashCollisionTests.cs | 58 ++++++++++++ Slapper.AutoMapper.Tests/Slapper.Tests.csproj | 1 + Slapper.AutoMapper/Properties/AssemblyInfo.cs | 1 + .../Slapper.AutoMapper.Cache.cs | 8 +- .../Slapper.AutoMapper.InternalHelpers.cs | 90 ++++++++++--------- Slapper.AutoMapper/Slapper.AutoMapper.cs | 29 +++--- 8 files changed, 169 insertions(+), 65 deletions(-) create mode 100644 Slapper.AutoMapper.Tests/HashCollisionTests.cs diff --git a/Slapper.AutoMapper.Tests/CachingBehaviorTests.cs b/Slapper.AutoMapper.Tests/CachingBehaviorTests.cs index 3ac29a8..1710000 100644 --- a/Slapper.AutoMapper.Tests/CachingBehaviorTests.cs +++ b/Slapper.AutoMapper.Tests/CachingBehaviorTests.cs @@ -36,6 +36,17 @@ public class Department public string Name { get; set; } } + public class Order + { + public int Id { get; set; } + public List OrderItems { get; set; } + } + + public class OrderItem + { + public int Id { get; set; } + } + [Test] public void Previously_Instantiated_Objects_Will_Be_Returned_Until_The_Cache_Is_Cleared() { @@ -51,7 +62,7 @@ public void Previously_Instantiated_Objects_Will_Be_Returned_Until_The_Cache_Is_ var customer = Slapper.AutoMapper.Map(dictionary); // Assert - Assert.That(customer.FirstName == "Bob"); + Assert.AreEqual("Bob", customer.FirstName); // Arrange var dictionary2 = new Dictionary { { "CustomerId", 1 } }; @@ -61,7 +72,7 @@ public void Previously_Instantiated_Objects_Will_Be_Returned_Until_The_Cache_Is_ // Assert that this will be "Bob" because the identifier of the Customer object was the same, // so we recieved back the cached instance of the Customer object. - Assert.That(customer2.FirstName == "Bob"); + Assert.AreEqual("Bob", customer2.FirstName); // Arrange var dictionary3 = new Dictionary { { "CustomerId", 1 } }; @@ -72,7 +83,7 @@ public void Previously_Instantiated_Objects_Will_Be_Returned_Until_The_Cache_Is_ var customer3 = Slapper.AutoMapper.Map(dictionary3); // Assert - Assert.That(customer3.FirstName == null); + Assert.Null(customer3.FirstName); } [Test] @@ -99,5 +110,27 @@ public void Test_Nested_Duplicate_Instances() Assert.AreSame(employeeList[0].Department, employeeList[1].Department); } + + [Test] + public void Cache_is_cleared_if_KeepCache_is_false() + { + var item1 = new Dictionary { + { "Id", 1 }, + { "OrderItems_Id", 1 } + }; + + var item2 = new Dictionary { + { "Id", 1 }, + { "OrderItems_Id", 2 } + }; + + var firstResult = AutoMapper.Map(item1, false); + var secondResult = AutoMapper.Map(item2, false); + + Assert.AreEqual(1, firstResult.OrderItems.Count); + Assert.AreEqual(1, firstResult.OrderItems[0].Id); + Assert.AreEqual(1, secondResult.OrderItems.Count); + Assert.AreEqual(2, secondResult.OrderItems[0].Id); + } } } \ No newline at end of file diff --git a/Slapper.AutoMapper.Tests/ComplexMapsParentsAndChlidTest.cs b/Slapper.AutoMapper.Tests/ComplexMapsParentsAndChlidTest.cs index 2e66876..ecca398 100644 --- a/Slapper.AutoMapper.Tests/ComplexMapsParentsAndChlidTest.cs +++ b/Slapper.AutoMapper.Tests/ComplexMapsParentsAndChlidTest.cs @@ -35,7 +35,7 @@ public class Booking } [Test] - public void Can_Make_Cache_HashTypeEquals_With_Diferents_Parents() + public void Can_Make_Cache_HashTypeEquals_With_Different_Parents() { var listOfDictionaries = new List> { @@ -68,13 +68,13 @@ public void Can_Make_Cache_HashTypeEquals_With_Diferents_Parents() Assert.That(bookings[0].Services.Count() == 2); Assert.NotNull(bookings[0].Services.SingleOrDefault(s => s.Id == 1)); - Assert.That(bookings[0].Services.SingleOrDefault(s => s.Id == 1).Hotels.Count() == 1); - Assert.That(bookings[0].Services.SingleOrDefault(s => s.Id == 2).Hotels.Count() == 1); + Assert.That(bookings[0].Services.Single(s => s.Id == 1).Hotels.Count() == 1); + Assert.That(bookings[0].Services.Single(s => s.Id == 2).Hotels.Count() == 1); Assert.That(bookings[1].Services.Count() == 1); Assert.NotNull(bookings[1].Services.SingleOrDefault(s => s.Id == 1)); - Assert.That(bookings[1].Services.SingleOrDefault(s => s.Id == 1).Hotels.Count() == 1); + Assert.That(bookings[1].Services.Single(s => s.Id == 1).Hotels.Count() == 1); } } } diff --git a/Slapper.AutoMapper.Tests/HashCollisionTests.cs b/Slapper.AutoMapper.Tests/HashCollisionTests.cs new file mode 100644 index 0000000..2c3344d --- /dev/null +++ b/Slapper.AutoMapper.Tests/HashCollisionTests.cs @@ -0,0 +1,58 @@ +using System.Collections.Generic; +using System.Dynamic; +using System.Linq; +using NUnit.Framework; + +namespace Slapper.Tests +{ + public class HashCollisionTests + { + [Test] + public void Avoids_Hash_Collisions() + { + // Arrange + var id2 = typeof (Employee).GetHashCode() - typeof (Contract).GetHashCode(); + + var source = new List(); + + dynamic obj1 = new ExpandoObject(); + + obj1.Id = 1; + obj1.Contracts_Id = id2; + + source.Add(obj1); + + dynamic obj2 = new ExpandoObject(); + + obj2.Id = 1; + obj2.Contracts_Id = id2 + 1; + + source.Add(obj2); + + // Act/Assert + var result = AutoMapper.MapDynamic(source).First(); + } + + public class Employee + { + public int Id { get; set; } + + public List Contracts { get; set; } + + public override int GetHashCode() + { + return Id; + } + } + + public class Contract + { + public int Id { get; set; } + + public override int GetHashCode() + { + return Id; + } + } + } +} \ No newline at end of file diff --git a/Slapper.AutoMapper.Tests/Slapper.Tests.csproj b/Slapper.AutoMapper.Tests/Slapper.Tests.csproj index 68ef60b..b8c1172 100644 --- a/Slapper.AutoMapper.Tests/Slapper.Tests.csproj +++ b/Slapper.AutoMapper.Tests/Slapper.Tests.csproj @@ -46,6 +46,7 @@ + diff --git a/Slapper.AutoMapper/Properties/AssemblyInfo.cs b/Slapper.AutoMapper/Properties/AssemblyInfo.cs index b23abc9..1e1c1aa 100644 --- a/Slapper.AutoMapper/Properties/AssemblyInfo.cs +++ b/Slapper.AutoMapper/Properties/AssemblyInfo.cs @@ -34,3 +34,4 @@ // [assembly: AssemblyVersion("1.0.*")] [assembly: AssemblyVersion("1.0.0.7")] [assembly: AssemblyFileVersion("1.0.0.7")] +[assembly: InternalsVisibleTo("Slapper.Tests")] \ No newline at end of file diff --git a/Slapper.AutoMapper/Slapper.AutoMapper.Cache.cs b/Slapper.AutoMapper/Slapper.AutoMapper.Cache.cs index 17b8e71..1ff193d 100644 --- a/Slapper.AutoMapper/Slapper.AutoMapper.Cache.cs +++ b/Slapper.AutoMapper/Slapper.AutoMapper.Cache.cs @@ -43,7 +43,7 @@ public static partial class AutoMapper /// /// Contains the methods and members responsible for this libraries caching concerns. /// - public static class Cache + internal static class Cache { /// /// The name of the instance cache stored in the logical call context. @@ -115,13 +115,13 @@ public static void ClearInstanceCache() /// unique cache. /// /// Instance Cache - public static Dictionary GetInstanceCache() + public static Dictionary, object> GetInstanceCache() { - var instanceCache = InternalHelpers.ContextStorage.Get>(InstanceCacheContextStorageKey); + var instanceCache = InternalHelpers.ContextStorage.Get, object>>(InstanceCacheContextStorageKey); if (instanceCache == null) { - instanceCache = new Dictionary(); + instanceCache = new Dictionary, object>(); InternalHelpers.ContextStorage.Store(InstanceCacheContextStorageKey, instanceCache); } diff --git a/Slapper.AutoMapper/Slapper.AutoMapper.InternalHelpers.cs b/Slapper.AutoMapper/Slapper.AutoMapper.InternalHelpers.cs index 9dac659..5d9e567 100644 --- a/Slapper.AutoMapper/Slapper.AutoMapper.InternalHelpers.cs +++ b/Slapper.AutoMapper/Slapper.AutoMapper.InternalHelpers.cs @@ -46,7 +46,7 @@ public static partial class AutoMapper /// /// Contains the methods and members responsible for this libraries internal concerns. /// - public static class InternalHelpers + internal static class InternalHelpers { /// /// Gets the identifiers for the given type. Returns NULL if not found. @@ -298,7 +298,7 @@ private static object ConvertValuesTypeToMembersType(object value, string member /// FieldInfo or PropertyInfo object /// Object to get the value from /// Value of the member - public static object GetMemberValue(object member, object obj) + private static object GetMemberValue(object member, object obj) { object value = null; @@ -327,22 +327,43 @@ public static object GetMemberValue(object member, object obj) /// /// Type of instance to get /// List of properties and values - /// Hash from parent object + /// Parent instance. Can be NULL if this is the root instance. /// /// Tuple of bool, object, int where bool represents whether this is a newly created instance, /// object being an instance of the requested type and int being the instance's identifier hash. /// - public static Tuple GetInstance(Type type, IDictionary properties, int parentHash) + internal static Tuple> GetInstance(Type type, IDictionary properties, object parentInstance = null) { + var key = GetCacheKey(type, properties, parentInstance); + var instanceCache = Cache.GetInstanceCache(); - var identifiers = GetIdentifiers(type); + object instance; + + var isNewlyCreatedInstance = !instanceCache.TryGetValue(key, out instance); + + if (isNewlyCreatedInstance) + { + instance = CreateInstance(type); + instanceCache[key] = instance; + } + + return Tuple.Create(isNewlyCreatedInstance, instance, key); + } - object instance = null; + private static Tuple GetCacheKey(Type type, IDictionary properties, object parentInstance) + { + var identifierHash = GetIdentifierHash(type, properties); - bool isNewlyCreatedInstance = false; + var key = Tuple.Create(identifierHash, type.GetHashCode(), parentInstance); + return key; + } - int identifierHash = 0; + private static int GetIdentifierHash(Type type, IDictionary properties) + { + var identifiers = GetIdentifiers(type); + + var identifierHash = 0; if (identifiers != null) { @@ -352,40 +373,23 @@ public static Tuple GetInstance(Type type, IDictionary(isNewlyCreatedInstance, instance, identifierHash); + return identifierHash; } /// @@ -399,7 +403,7 @@ public static Tuple GetInstance(Type type, IDictionaryInstance to populate /// Optional parent instance of the instance being populated /// Populated instance - public static object Map(IDictionary dictionary, object instance, object parentInstance = null) + internal static object Map(IDictionary dictionary, object instance, object parentInstance = null) { if (instance.GetType().IsPrimitive || instance is string) { @@ -471,7 +475,7 @@ public static object Map(IDictionary dictionary, object instance } else { - var result = GetInstance(memberType, newDictionary, parentInstance == null ? 0 : parentInstance.GetHashCode()); + var result = GetInstance(memberType, newDictionary, parentInstance); nestedInstance = result.Item2; } } @@ -509,7 +513,7 @@ public static object Map(IDictionary dictionary, object instance /// Instance to populate /// Optional parent instance of the instance being populated /// Populated instance - public static object MapCollection(Type type, IDictionary dictionary, object instance, object parentInstance = null) + internal static object MapCollection(Type type, IDictionary dictionary, object instance, object parentInstance = null) { Type baseListType = typeof(List<>); @@ -526,7 +530,7 @@ public static object MapCollection(Type type, IDictionary dictio return instance; } - var getInstanceResult = GetInstance(type, dictionary, parentInstance == null ? 0 : parentInstance.GetHashCode()); + var getInstanceResult = GetInstance(type, dictionary, parentInstance); // Is this a newly created instance? If false, then this item was retrieved from the instance cache. bool isNewlyCreatedInstance = getInstanceResult.Item1; @@ -582,7 +586,7 @@ public static object MapCollection(Type type, IDictionary dictio /// Provides a means of getting/storing data in the host application's /// appropriate context. /// - public interface IContextStorage + internal interface IContextStorage { /// /// Get a stored item. @@ -683,7 +687,7 @@ public void Remove(string key) /// For ASP.NET applications, it will store in the data in the current HTTPContext. /// For all other applications, it will store the data in the logical call context. /// - public static class ContextStorage + internal static class ContextStorage { /// /// Provides a means of getting/storing data in the host application's @@ -730,7 +734,7 @@ public static void Remove(string key) /// /// Contains the methods and members responsible for this libraries reflection concerns. /// - public static class ReflectionHelper + private static class ReflectionHelper { /// /// Provides access to System.Web.HttpContext.Current.Items via reflection. diff --git a/Slapper.AutoMapper/Slapper.AutoMapper.cs b/Slapper.AutoMapper/Slapper.AutoMapper.cs index b45809b..cbf1f71 100644 --- a/Slapper.AutoMapper/Slapper.AutoMapper.cs +++ b/Slapper.AutoMapper/Slapper.AutoMapper.cs @@ -72,9 +72,10 @@ public class Id : Attribute /// /// Type to instantiate and automap to /// Dynamic list of property names and values + /// If false, clears instance cache after mapping is completed. Defaults to true, meaning instances are kept between calls. /// List of type /// Exception that is thrown when the cannot be converted to an IDictionary of type string and object. - public static T MapDynamic( object dynamicObject ) + public static T MapDynamic( object dynamicObject, bool keepCache = true) { if ( dynamicObject == null ) return default( T ); @@ -86,7 +87,7 @@ public static T MapDynamic( object dynamicObject ) var propertiesList = new List> { dictionary }; - return Map( propertiesList ).FirstOrDefault(); + return Map( propertiesList, keepCache ).FirstOrDefault(); } /// @@ -97,9 +98,10 @@ public static T MapDynamic( object dynamicObject ) /// /// Type to instantiate and automap to /// Dynamic list of property names and values + /// If false, clears instance cache after mapping is completed. Defaults to true, meaning instances are kept between calls. /// List of type /// Exception that is thrown when the cannot be converted to an IDictionary of type string and object. - public static IEnumerable MapDynamic( IEnumerable dynamicListOfProperties ) + public static IEnumerable MapDynamic( IEnumerable dynamicListOfProperties, bool keepCache = true) { if ( dynamicListOfProperties == null ) return new List(); @@ -112,7 +114,7 @@ public static IEnumerable MapDynamic( IEnumerable dynamicListOfPro if ( dictionary.Count == 0 || dictionary[ 0 ] == null ) return new List(); - return Map( dictionary ); + return Map( dictionary, keepCache ); } /// @@ -123,12 +125,13 @@ public static IEnumerable MapDynamic( IEnumerable dynamicListOfPro /// /// Type to instantiate and automap to /// List of property names and values + /// If false, clears instance cache after mapping is completed. Defaults to true, meaning instances are kept between calls. /// List of type - public static T Map( IDictionary listOfProperties ) + public static T Map( IDictionary listOfProperties, bool keepCache = true) { var propertiesList = new List> { listOfProperties }; - return Map( propertiesList ).FirstOrDefault(); + return Map( propertiesList, keepCache ).FirstOrDefault(); } /// @@ -139,22 +142,23 @@ public static T Map( IDictionary listOfProperties ) /// /// Type to instantiate and automap to /// List of property names and values + /// If false, clears instance cache after mapping is completed. Defaults to true, meaning instances are kept between calls. /// List of type - public static IEnumerable Map( IEnumerable> listOfProperties ) + public static IEnumerable Map( IEnumerable> listOfProperties, bool keepCache = true) { var instanceCache = new Dictionary(); foreach ( var properties in listOfProperties ) { - var getInstanceResult = InternalHelpers.GetInstance( typeof ( T ), properties, 0); + var getInstanceResult = InternalHelpers.GetInstance( typeof ( T ), properties); object instance = getInstanceResult.Item2; - int instanceIdentifierHash = getInstanceResult.Item3; + var key = getInstanceResult.Item3; - if ( instanceCache.ContainsKey( instanceIdentifierHash ) == false ) + if ( instanceCache.ContainsKey(key) == false ) { - instanceCache.Add( instanceIdentifierHash, instance ); + instanceCache.Add(key, instance ); } var caseInsensitiveDictionary = new Dictionary( properties, StringComparer.OrdinalIgnoreCase ); @@ -162,6 +166,9 @@ public static IEnumerable Map( IEnumerable> li InternalHelpers.Map(caseInsensitiveDictionary, instance); } + if (!keepCache) + Cache.ClearInstanceCache(); + return instanceCache.Select(pair => ( T ) pair.Value); } From c7eacd33a07a45b472241a1722224dccbf51d2ec Mon Sep 17 00:00:00 2001 From: Geir Sagberg Date: Mon, 22 Feb 2016 15:12:24 +0100 Subject: [PATCH 2/2] Make Cache public again but internal members internal --- Slapper.AutoMapper/Slapper.AutoMapper.Cache.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Slapper.AutoMapper/Slapper.AutoMapper.Cache.cs b/Slapper.AutoMapper/Slapper.AutoMapper.Cache.cs index 1ff193d..43c48ec 100644 --- a/Slapper.AutoMapper/Slapper.AutoMapper.Cache.cs +++ b/Slapper.AutoMapper/Slapper.AutoMapper.Cache.cs @@ -43,22 +43,22 @@ public static partial class AutoMapper /// /// Contains the methods and members responsible for this libraries caching concerns. /// - internal static class Cache + public static class Cache { /// /// The name of the instance cache stored in the logical call context. /// - public const string InstanceCacheContextStorageKey = "Slapper.AutoMapper.InstanceCache"; + internal const string InstanceCacheContextStorageKey = "Slapper.AutoMapper.InstanceCache"; /// /// Cache of TypeMaps containing the types identifiers and PropertyInfo/FieldInfo objects. /// - public static readonly ConcurrentDictionary TypeMapCache = new ConcurrentDictionary(); + internal static readonly ConcurrentDictionary TypeMapCache = new ConcurrentDictionary(); /// /// A TypeMap holds data relevant for a particular Type. /// - public class TypeMap + internal class TypeMap { /// /// Creates a new .