Avoid hash collisions by including more keys

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<OrderItem> 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<Customer>(dictionary);
 
             // Assert
-            Assert.That(customer.FirstName == "Bob");
+            Assert.AreEqual("Bob", customer.FirstName);
 
             // Arrange
             var dictionary2 = new Dictionary<string, object> { { "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<string, object> { { "CustomerId", 1 } };
@@ -72,7 +83,7 @@ public void Previously_Instantiated_Objects_Will_Be_Returned_Until_The_Cache_Is_
             var customer3 = Slapper.AutoMapper.Map<Customer>(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<string, object> {
+                { "Id", 1 },
+                { "OrderItems_Id", 1 }
+            };
+
+            var item2 = new Dictionary<string, object> {
+                { "Id", 1 },
+                { "OrderItems_Id", 2 }
+            };
+
+            var firstResult = AutoMapper.Map<Order>(item1, false);
+            var secondResult = AutoMapper.Map<Order>(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);
+        }
     }
 }

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<Dictionary<string, object>>
             {
@@ -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);
         }
     }
 }

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<object>();
+
+            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<Employee>(source).First();
+        }
+
+        public class Employee
+        {
+            public int Id { get; set; }
+
+            public List<Contract> Contracts { get; set; }
+
+            public override int GetHashCode()
+            {
+                return Id;
+            }
+        }
+
+        public class Contract
+        {
+            public int Id { get; set; }
+
+            public override int GetHashCode()
+            {
+                return Id;
+            }
+        }
+    }
+}

Slapper.AutoMapper.Tests/Slapper.Tests.csproj

@@ -46,6 +46,7 @@
     <Compile Include="ArrayTests.cs" />
     <Compile Include="ComplexMapsParentsAndChlidTest.cs" />
     <Compile Include="EmptyList.cs" />
+    <Compile Include="HashCollisionTests.cs" />
     <Compile Include="MapCollectionsTypedTest.cs" />
     <Compile Include="MappingToGuidTests.cs" />
     <Compile Include="MappingToNullableTypesTests.cs" />

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")]

Slapper.AutoMapper/Slapper.AutoMapper.Cache.cs

@@ -48,17 +48,17 @@ public static class Cache
             /// <summary>
             /// The name of the instance cache stored in the logical call context.
             /// </summary>
-            public const string InstanceCacheContextStorageKey = "Slapper.AutoMapper.InstanceCache";
+            internal const string InstanceCacheContextStorageKey = "Slapper.AutoMapper.InstanceCache";
 
             /// <summary>
             /// Cache of TypeMaps containing the types identifiers and PropertyInfo/FieldInfo objects.
             /// </summary>
-            public static readonly ConcurrentDictionary<Type, TypeMap> TypeMapCache = new ConcurrentDictionary<Type, TypeMap>();
+            internal static readonly ConcurrentDictionary<Type, TypeMap> TypeMapCache = new ConcurrentDictionary<Type, TypeMap>();
 
             /// <summary>
             /// A TypeMap holds data relevant for a particular Type.
             /// </summary>
-            public class TypeMap
+            internal class TypeMap
             {
                 /// <summary>
                 /// Creates a new <see cref="TypeMap"/>.
@@ -115,13 +115,13 @@ public static void ClearInstanceCache()
             /// unique cache.
             /// </remarks>
             /// <returns>Instance Cache</returns>
-            public static Dictionary<object, object> GetInstanceCache()
+            public static Dictionary<Tuple<int, int, object>, object> GetInstanceCache()
             {
-                var instanceCache = InternalHelpers.ContextStorage.Get<Dictionary<object, object>>(InstanceCacheContextStorageKey);
+                var instanceCache = InternalHelpers.ContextStorage.Get<Dictionary<Tuple<int, int, object>, object>>(InstanceCacheContextStorageKey);
 
                 if (instanceCache == null)
                 {
-                    instanceCache = new Dictionary<object, object>();
+                    instanceCache = new Dictionary<Tuple<int, int, object>, object>();
 
                     InternalHelpers.ContextStorage.Store(InstanceCacheContextStorageKey, instanceCache);
                 }

Slapper.AutoMapper/Slapper.AutoMapper.InternalHelpers.cs

@@ -46,7 +46,7 @@ public static partial class AutoMapper
         /// <summary>
         /// Contains the methods and members responsible for this libraries internal concerns.
         /// </summary>
-        public static class InternalHelpers
+        internal static class InternalHelpers
         {
             /// <summary>
             /// Gets the identifiers for the given type. Returns NULL if not found.
@@ -298,7 +298,7 @@ private static object ConvertValuesTypeToMembersType(object value, string member
             /// <param name="member">FieldInfo or PropertyInfo object</param>
             /// <param name="obj">Object to get the value from</param>
             /// <returns>Value of the member</returns>
-            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)
             /// </summary>
             /// <param name="type">Type of instance to get</param>
             /// <param name="properties">List of properties and values</param>
-            /// <param name="parentHash">Hash from parent object</param>
+            /// <param name="parentInstance">Parent instance. Can be NULL if this is the root instance.</param>
             /// <returns>
             /// 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.
             /// </returns>
-            public static Tuple<bool, object, int> GetInstance(Type type, IDictionary<string, object> properties, int parentHash)
+            internal static Tuple<bool, object, Tuple<int, int, object>> GetInstance(Type type, IDictionary<string, object> 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<int, int, object> GetCacheKey(Type type, IDictionary<string, object> 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<string, object> properties)
+            {
+                var identifiers = GetIdentifiers(type);
+
+                var identifierHash = 0;
 
                 if (identifiers != null)
                 {
@@ -352,40 +373,23 @@ public static Tuple<bool, object, int> GetInstance(Type type, IDictionary<string
                         {
                             var identifierValue = properties[identifier];
                             if (identifierValue != null)
-                                identifierHash += identifierValue.GetHashCode() + type.GetHashCode() + parentHash;
-                        }
-                    }
-
-                    if (identifierHash != 0)
-                    {
-                        if (instanceCache.ContainsKey(identifierHash))
-                        {
-                            instance = instanceCache[identifierHash];
-                        }
-                        else
-                        {
-                            instance = CreateInstance(type);
-
-                            instanceCache.Add(identifierHash, instance);
-
-                            isNewlyCreatedInstance = true;
+                            {
+                                // Unchecked to avoid arithmetic overflow
+                                unchecked
+                                {
+                                    // Include identifier hashcode to avoid collisions between e.g. multiple int IDs
+                                    identifierHash += identifierValue.GetHashCode() + identifier.GetHashCode();
+                                }
+                            }
                         }
                     }
                 }
-
-                // An identifier hash with a value of zero means the type does not have any identifiers.
-                // To make this instance unique generate a unique hash for it.
-                if (identifierHash == 0 && identifiers != null) identifierHash = type.GetHashCode() + parentHash;
-
-                if (instance == null)
+                else
                 {
-                    instance = CreateInstance(type);
+                    // If the type has no identifiers we must generate a unique hash for it.
                     identifierHash = Guid.NewGuid().GetHashCode();
-
-                    isNewlyCreatedInstance = true;
                 }
-
-                return new Tuple<bool, object, int>(isNewlyCreatedInstance, instance, identifierHash);
+                return identifierHash;
             }
 
             /// <summary>
@@ -399,7 +403,7 @@ public static Tuple<bool, object, int> GetInstance(Type type, IDictionary<string
             /// <param name="instance">Instance to populate</param>
             /// <param name="parentInstance">Optional parent instance of the instance being populated</param>
             /// <returns>Populated instance</returns>
-            public static object Map(IDictionary<string, object> dictionary, object instance, object parentInstance = null)
+            internal static object Map(IDictionary<string, object> dictionary, object instance, object parentInstance = null)
             {
                 if (instance.GetType().IsPrimitive || instance is string)
                 {
@@ -471,7 +475,7 @@ public static object Map(IDictionary<string, object> 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<string, object> dictionary, object instance
             /// <param name="instance">Instance to populate</param>
             /// <param name="parentInstance">Optional parent instance of the instance being populated</param>
             /// <returns>Populated instance</returns>
-            public static object MapCollection(Type type, IDictionary<string, object> dictionary, object instance, object parentInstance = null)
+            internal static object MapCollection(Type type, IDictionary<string, object> dictionary, object instance, object parentInstance = null)
             {
                 Type baseListType = typeof(List<>);
 
@@ -526,7 +530,7 @@ public static object MapCollection(Type type, IDictionary<string, object> 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<string, object> dictio
             /// Provides a means of getting/storing data in the host application's
             /// appropriate context.
             /// </summary>
-            public interface IContextStorage
+            internal interface IContextStorage
             {
                 /// <summary>
                 /// 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.
             /// </remarks>
-            public static class ContextStorage
+            internal static class ContextStorage
             {
                 /// <summary>
                 /// Provides a means of getting/storing data in the host application's
@@ -730,7 +734,7 @@ public static void Remove(string key)
             /// <summary>
             /// Contains the methods and members responsible for this libraries reflection concerns.
             /// </summary>
-            public static class ReflectionHelper
+            private static class ReflectionHelper
             {
                 /// <summary>
                 /// Provides access to System.Web.HttpContext.Current.Items via reflection.