Merge branch '#49'

Author: odelvalle

Slapper.AutoMapper.Tests/CachingBehaviorTests.cs

@@ -47,6 +47,13 @@ public class OrderItem
             public int Id { get; set; }
         }
 
+
+        public class OrderWithLongId
+        {
+            public long Id { get; set; }
+            public List<OrderItem> OrderItems { get; set; }
+        }
+
         [Test]
         public void Previously_Instantiated_Objects_Will_Be_Returned_Until_The_Cache_Is_Cleared()
         {
@@ -111,6 +118,32 @@ public void Test_Nested_Duplicate_Instances()
             Assert.AreSame(employeeList[0].Department, employeeList[1].Department);           
         }
 
+        [Test]
+        public void Test_Long_Ids_With_Colliding_HashValues()
+        {
+            // This test could fail if MS GetHashCode implementation for long changed. We would then have to find new long values
+            // having the same hashcode.
+            const long longId1 = 95988224123597;
+            var item1 = new Dictionary<string, object>()
+                                               {
+                                                   { "Id", longId1 },
+                                                   { "OrderDetail_Id", 1 }
+                                               };
+
+            const long longId2 = 95983929156300;
+            var item2 = new Dictionary<string, object>()
+                                               {
+                                                   { "Id", longId2 },
+                                                   { "OrderDetail_Id", 2 }
+                                               };
+
+            var list = new List<Dictionary<string, object>>() { item1, item2 };
+            var orderList = AutoMapper.Map<OrderWithLongId>(list).ToList();
+
+            Assert.AreEqual(longId1.GetHashCode(), longId2.GetHashCode());
+            Assert.AreEqual(orderList.Count, list.Count);
+        }
+
         [Test]
         public void Cache_is_cleared_if_KeepCache_is_false()
         {

Slapper.AutoMapper.Tests/ComplexMapTests.cs

@@ -117,11 +117,16 @@ public void Can_Map_Complex_Nested_Members()
         }
 
         /// <summary>
+        /// OLD SUMMARY ===
         /// When mapping, it internally keeps a cache of instantiated objects with the key being the
         /// hash of the objects identifier hashes summed together so when another record with the exact
         /// same identifier hash is detected, it will re-use the existing instantiated object instead of
         /// creating a second one alleviating the burden of the consumer of the library to group objects
         /// by their identifier.
+        /// ===
+        /// This was flawed as SAME HASHCODE DOESN'T MEAN SAME VALUE. Hash collisions would lead to
+        /// wrongly reusing an instance instead of creating a new one (issue #48).
+        /// It's now fixed as real identifier values are compared, not their hashes anymore.
         /// </summary>
         [Test]
         public void Can_Detect_Duplicate_Parent_Members_And_Properly_Instantiate_The_Object_Only_Once()

Slapper.AutoMapper.Tests/PerformanceTests.cs

@@ -105,9 +105,9 @@ public void Complex_Performance_Test()
                     { "CustomerId", i },
                     { "FirstName", "Bob" },
                     { "LastName", "Smith" },
-                    { "Orders_Id", i },
+                    { "Orders_OrderId", i },
                     { "Orders_OrderTotal", 50.50m },
-                    { "Orders_OrderDetails_Id", i },
+                    { "Orders_OrderDetails_OrderDetailId", i },
                     { "Orders_OrderDetails_OrderDetailTotal", 50.50m },
                     { "Orders_OrderDetails_Product_Id", 546 },
                     { "Orders_OrderDetails_Product_ProductName", "Black Bookshelf" }

Slapper.AutoMapper/Slapper.AutoMapper.Cache.cs

@@ -43,7 +43,7 @@ public static partial class AutoMapper
         /// <summary>
         /// Contains the methods and members responsible for this libraries caching concerns.
         /// </summary>
-        public static class Cache
+        internal static class Cache
         {
             /// <summary>
             /// The name of the instance cache stored in the logical call context.
@@ -115,13 +115,13 @@ public static void ClearInstanceCache()
             /// unique cache.
             /// </remarks>
             /// <returns>Instance Cache</returns>
-            public static Dictionary<Tuple<int, int, object>, object> GetInstanceCache()
+            public static Dictionary<InternalHelpers.InstanceKey,object> GetInstanceCache()
             {
-                var instanceCache = InternalHelpers.ContextStorage.Get<Dictionary<Tuple<int, int, object>, object>>(InstanceCacheContextStorageKey);
+                var instanceCache = InternalHelpers.ContextStorage.Get<Dictionary<InternalHelpers.InstanceKey, object>>(InstanceCacheContextStorageKey);
 
                 if (instanceCache == null)
                 {
-                    instanceCache = new Dictionary<Tuple<int, int, object>, object>();
+                    instanceCache = new Dictionary<InternalHelpers.InstanceKey, object>();
 
                     InternalHelpers.ContextStorage.Store(InstanceCacheContextStorageKey, instanceCache);
                 }

Slapper.AutoMapper/Slapper.AutoMapper.InternalHelpers.cs

@@ -49,6 +49,73 @@ public static partial class AutoMapper
         /// </summary>
         internal static class InternalHelpers
         {
+            /// <summary>
+            /// Combine several hashcodes into a single new one. This implementation was grabbed from http://stackoverflow.com/a/34229665 where it is introduced 
+            /// as MS implementation of GetHashCode() for strings.
+            /// </summary>
+            /// <param name="hashCodes">Hascodes to be combined.</param>
+            /// <returns>A new Hascode value combining those passed as parameters.</returns>
+            private static int CombineHashCodes(params int[] hashCodes)
+            {
+                int hash1 = (5381 << 16) + 5381;
+                int hash2 = hash1;
+
+                int i = 0;
+                foreach (var hashCode in hashCodes)
+                {
+                    if (i % 2 == 0)
+                        hash1 = ((hash1 << 5) + hash1 + (hash1 >> 27)) ^ hashCode;
+                    else
+                        hash2 = ((hash2 << 5) + hash2 + (hash2 >> 27)) ^ hashCode;
+
+                    ++i;
+                }
+
+                return hash1 + (hash2 * 1566083941);
+            }
+
+            /// <summary>
+            /// Defines the key for caching instances. Overrides Equality as to get unicity for a given set of identifiers values
+            /// for a given type.
+            /// </summary>
+            public struct InstanceKey : IEquatable<InstanceKey>
+            {
+                public bool Equals(InstanceKey other) {
+                    return Equals(Type, other.Type) 
+                        && Equals(ParentInstance, other.ParentInstance) 
+                        && StructuralComparisons.StructuralEqualityComparer.Equals(IdentifierValues, other.IdentifierValues);
+                }
+
+                public override bool Equals(object obj)
+                {
+                    if (ReferenceEquals(null, obj)) return false;
+                    return obj is InstanceKey && Equals((InstanceKey) obj);
+                }
+
+                public override int GetHashCode()
+                {
+                    unchecked
+                    {
+                        return CombineHashCodes(Type?.GetHashCode() ?? 0, StructuralComparisons.StructuralEqualityComparer.GetHashCode(IdentifierValues), ParentInstance?.GetHashCode() ?? 0);
+                    }
+                }
+
+                public static bool operator ==(InstanceKey left, InstanceKey right) { return left.Equals(right); }
+
+                public static bool operator !=(InstanceKey left, InstanceKey right) { return !left.Equals(right); }
+
+                public InstanceKey(Type type, object[] identifierValues, object parentInstance)
+                {
+                    Type = type;
+                    IdentifierValues = identifierValues;
+                    ParentInstance = parentInstance;
+                }
+
+                public  Type Type { get; }
+                public object[] IdentifierValues { get; }
+                public object ParentInstance { get;  }
+            }
+
             /// <summary>
             /// Gets the identifiers for the given type. Returns NULL if not found.
             /// Results are cached for subsequent use and performance.
@@ -338,6 +405,28 @@ private static object GetMemberValue(object member, object obj)
                 return value;
             }
 
+            /// <summary>
+            /// Computes a key for storing and identifying an instance in the cache.
+            /// </summary>
+            /// <param name="type">Type of instance to get</param>
+            /// <param name="properties">List of properties and values</param>
+            /// <param name="parentInstance">Parent instance. Can be NULL if this is the root instance.</param>
+            /// <returns>
+            /// InstanceKey that will be unique for given set of identifiers values for the type. If the type isn't associated with any 
+            /// identifier, the return value is made unique by generating a Guid.
+            /// ASSUMES GetIdentifiers(type) ALWAYS RETURN IDENTIFIERS IN THE SAME ORDER FOR A GIVEN TYPE.
+            /// This is certainly the case as long as GetIdentifiers caches its result for a given type (which it does by 2016-11-25).
+            /// </returns>
+            private static InstanceKey GetCacheKey(Type type, IDictionary<string, object> properties, object parentInstance)
+            {
+                var identifierValues = GetIdentifiers(type)?.Select(id => properties[id]).DefaultIfEmpty(Guid.NewGuid()).ToArray()
+                    ?? new object[] { Guid.NewGuid() };
+
+                var key = new InstanceKey(type, identifierValues, parentInstance);
+                return key;
+            }
+
+
             /// <summary>
             /// Gets a new or existing instance depending on whether an instance with the same identifiers already existing
             /// in the instance cache.
@@ -349,7 +438,7 @@ private static object GetMemberValue(object member, object obj)
             /// 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>
-            internal static Tuple<bool, object, Tuple<int, int, object>> GetInstance(Type type, IDictionary<string, object> properties, object parentInstance = null)
+            internal static Tuple<bool, object, InstanceKey> GetInstance(Type type, IDictionary<string, object> properties, object parentInstance = null)
             {
                 var key = GetCacheKey(type, properties, parentInstance);
 
@@ -368,47 +457,6 @@ internal static Tuple<bool, object, Tuple<int, int, object>> GetInstance(Type ty
                 return Tuple.Create(isNewlyCreatedInstance, instance, key);
             }
 
-            private static Tuple<int, int, object> GetCacheKey(Type type, IDictionary<string, object> properties, object parentInstance)
-            {
-                var identifierHash = GetIdentifierHash(type, properties);
-
-                var key = Tuple.Create(identifierHash, type.GetHashCode(), parentInstance);
-                return key;
-            }
-
-            private static int GetIdentifierHash(Type type, IDictionary<string, object> properties)
-            {
-                var identifiers = GetIdentifiers(type);
-
-                var identifierHash = 0;
-
-                if (identifiers != null)
-                {
-                    foreach (var identifier in identifiers)
-                    {
-                        if (properties.ContainsKey(identifier))
-                        {
-                            var identifierValue = properties[identifier];
-                            if (identifierValue != null)
-                    {
-                                // Unchecked to avoid arithmetic overflow
-                                unchecked
-                        {
-                                    // Include identifier hashcode to avoid collisions between e.g. multiple int IDs
-                                    identifierHash += identifierValue.GetHashCode() + identifier.GetHashCode();
-                                }
-                        }
-                        }
-                    }
-                }
-                else
-                {
-                    // If the type has no identifiers we must generate a unique hash for it.
-                    identifierHash = Guid.NewGuid().GetHashCode();
-                }
-                return identifierHash;
-            }
-
             /// <summary>
             /// Populates the given instance's properties where the IDictionary key property names
             /// match the type's property names case insensitively.