Skip to content

Commit 278c9f9

Browse files
geirsagberggeirsagberg
committed
Avoid hash collisions by including more keys
1 parent c87aaa2 commit 278c9f9

File tree

8 files changed

+169
-65
lines changed

8 files changed

+169
-65
lines changed

‎Slapper.AutoMapper.Tests/CachingBehaviorTests.cs‎

Lines changed: 36 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,17 @@ public class Department
3636
public string Name { get; set; }
3737
}
3838

39+
public class Order
40+
{
41+
public int Id { get; set; }
42+
public List<OrderItem> OrderItems { get; set; }
43+
}
44+
45+
public class OrderItem
46+
{
47+
public int Id { get; set; }
48+
}
49+
3950
[Test]
4051
public void Previously_Instantiated_Objects_Will_Be_Returned_Until_The_Cache_Is_Cleared()
4152
{
@@ -51,7 +62,7 @@ public void Previously_Instantiated_Objects_Will_Be_Returned_Until_The_Cache_Is_
5162
var customer = Slapper.AutoMapper.Map<Customer>(dictionary);
5263

5364
// Assert
54-
Assert.That(customer.FirstName == "Bob");
65+
Assert.AreEqual("Bob", customer.FirstName);
5566

5667
// Arrange
5768
var dictionary2 = new Dictionary<string, object> { { "CustomerId", 1 } };
@@ -61,7 +72,7 @@ public void Previously_Instantiated_Objects_Will_Be_Returned_Until_The_Cache_Is_
6172

6273
// Assert that this will be "Bob" because the identifier of the Customer object was the same,
6374
// so we recieved back the cached instance of the Customer object.
64-
Assert.That(customer2.FirstName == "Bob");
75+
Assert.AreEqual("Bob", customer2.FirstName);
6576

6677
// Arrange
6778
var dictionary3 = new Dictionary<string, object> { { "CustomerId", 1 } };
@@ -72,7 +83,7 @@ public void Previously_Instantiated_Objects_Will_Be_Returned_Until_The_Cache_Is_
7283
var customer3 = Slapper.AutoMapper.Map<Customer>(dictionary3);
7384

7485
// Assert
75-
Assert.That(customer3.FirstName == null);
86+
Assert.Null(customer3.FirstName);
7687
}
7788

7889
[Test]
@@ -99,5 +110,27 @@ public void Test_Nested_Duplicate_Instances()
99110

100111
Assert.AreSame(employeeList[0].Department, employeeList[1].Department);
101112
}
113+
114+
[Test]
115+
public void Cache_is_cleared_if_KeepCache_is_false()
116+
{
117+
var item1 = new Dictionary<string, object> {
118+
{ "Id", 1 },
119+
{ "OrderItems_Id", 1 }
120+
};
121+
122+
var item2 = new Dictionary<string, object> {
123+
{ "Id", 1 },
124+
{ "OrderItems_Id", 2 }
125+
};
126+
127+
var firstResult = AutoMapper.Map<Order>(item1, false);
128+
var secondResult = AutoMapper.Map<Order>(item2, false);
129+
130+
Assert.AreEqual(1, firstResult.OrderItems.Count);
131+
Assert.AreEqual(1, firstResult.OrderItems[0].Id);
132+
Assert.AreEqual(1, secondResult.OrderItems.Count);
133+
Assert.AreEqual(2, secondResult.OrderItems[0].Id);
134+
}
102135
}
103136
}

‎Slapper.AutoMapper.Tests/ComplexMapsParentsAndChlidTest.cs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ public class Booking
3535
}
3636

3737
[Test]
38-
public void Can_Make_Cache_HashTypeEquals_With_Diferents_Parents()
38+
public void Can_Make_Cache_HashTypeEquals_With_Different_Parents()
3939
{
4040
var listOfDictionaries = new List<Dictionary<string, object>>
4141
{
@@ -68,13 +68,13 @@ public void Can_Make_Cache_HashTypeEquals_With_Diferents_Parents()
6868
Assert.That(bookings[0].Services.Count() == 2);
6969

7070
Assert.NotNull(bookings[0].Services.SingleOrDefault(s => s.Id == 1));
71-
Assert.That(bookings[0].Services.SingleOrDefault(s => s.Id == 1).Hotels.Count() == 1);
72-
Assert.That(bookings[0].Services.SingleOrDefault(s => s.Id == 2).Hotels.Count() == 1);
71+
Assert.That(bookings[0].Services.Single(s => s.Id == 1).Hotels.Count() == 1);
72+
Assert.That(bookings[0].Services.Single(s => s.Id == 2).Hotels.Count() == 1);
7373

7474
Assert.That(bookings[1].Services.Count() == 1);
7575

7676
Assert.NotNull(bookings[1].Services.SingleOrDefault(s => s.Id == 1));
77-
Assert.That(bookings[1].Services.SingleOrDefault(s => s.Id == 1).Hotels.Count() == 1);
77+
Assert.That(bookings[1].Services.Single(s => s.Id == 1).Hotels.Count() == 1);
7878
}
7979
}
8080
}

‎Slapper.AutoMapper.Tests/HashCollisionTests.cs‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
using System.Collections.Generic;
2+
using System.Dynamic;
3+
using System.Linq;
4+
using NUnit.Framework;
5+
6+
namespace Slapper.Tests
7+
{
8+
public class HashCollisionTests
9+
{
10+
[Test]
11+
public void Avoids_Hash_Collisions()
12+
{
13+
// Arrange
14+
var id2 = typeof (Employee).GetHashCode() - typeof (Contract).GetHashCode();
15+
16+
var source = new List<object>();
17+
18+
dynamic obj1 = new ExpandoObject();
19+
20+
obj1.Id = 1;
21+
obj1.Contracts_Id = id2;
22+
23+
source.Add(obj1);
24+
25+
dynamic obj2 = new ExpandoObject();
26+
27+
obj2.Id = 1;
28+
obj2.Contracts_Id = id2 + 1;
29+
30+
source.Add(obj2);
31+
32+
// Act/Assert
33+
var result = AutoMapper.MapDynamic<Employee>(source).First();
34+
}
35+
36+
public class Employee
37+
{
38+
public int Id { get; set; }
39+
40+
public List<Contract> Contracts { get; set; }
41+
42+
public override int GetHashCode()
43+
{
44+
return Id;
45+
}
46+
}
47+
48+
public class Contract
49+
{
50+
public int Id { get; set; }
51+
52+
public override int GetHashCode()
53+
{
54+
return Id;
55+
}
56+
}
57+
}
58+
}

‎Slapper.AutoMapper.Tests/Slapper.Tests.csproj‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@
4646
<Compile Include="ArrayTests.cs" />
4747
<Compile Include="ComplexMapsParentsAndChlidTest.cs" />
4848
<Compile Include="EmptyList.cs" />
49+
<Compile Include="HashCollisionTests.cs" />
4950
<Compile Include="MapCollectionsTypedTest.cs" />
5051
<Compile Include="MappingToGuidTests.cs" />
5152
<Compile Include="MappingToNullableTypesTests.cs" />

‎Slapper.AutoMapper/Properties/AssemblyInfo.cs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,3 +34,4 @@
3434
// [assembly: AssemblyVersion("1.0.*")]
3535
[assembly: AssemblyVersion("1.0.0.7")]
3636
[assembly: AssemblyFileVersion("1.0.0.7")]
37+
[assembly: InternalsVisibleTo("Slapper.Tests")]

‎Slapper.AutoMapper/Slapper.AutoMapper.Cache.cs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ public static partial class AutoMapper
4343
/// <summary>
4444
/// Contains the methods and members responsible for this libraries caching concerns.
4545
/// </summary>
46-
public static class Cache
46+
internal static class Cache
4747
{
4848
/// <summary>
4949
/// The name of the instance cache stored in the logical call context.
@@ -115,13 +115,13 @@ public static void ClearInstanceCache()
115115
/// unique cache.
116116
/// </remarks>
117117
/// <returns>Instance Cache</returns>
118-
public static Dictionary<object, object> GetInstanceCache()
118+
public static Dictionary<Tuple<int, int, object>, object> GetInstanceCache()
119119
{
120-
var instanceCache = InternalHelpers.ContextStorage.Get<Dictionary<object, object>>(InstanceCacheContextStorageKey);
120+
var instanceCache = InternalHelpers.ContextStorage.Get<Dictionary<Tuple<int, int, object>, object>>(InstanceCacheContextStorageKey);
121121

122122
if (instanceCache == null)
123123
{
124-
instanceCache = new Dictionary<object, object>();
124+
instanceCache = new Dictionary<Tuple<int, int, object>, object>();
125125

126126
InternalHelpers.ContextStorage.Store(InstanceCacheContextStorageKey, instanceCache);
127127
}

‎Slapper.AutoMapper/Slapper.AutoMapper.InternalHelpers.cs‎

Lines changed: 47 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ public static partial class AutoMapper
4646
/// <summary>
4747
/// Contains the methods and members responsible for this libraries internal concerns.
4848
/// </summary>
49-
public static class InternalHelpers
49+
internal static class InternalHelpers
5050
{
5151
/// <summary>
5252
/// Gets the identifiers for the given type. Returns NULL if not found.
@@ -298,7 +298,7 @@ private static object ConvertValuesTypeToMembersType(object value, string member
298298
/// <param name="member">FieldInfo or PropertyInfo object</param>
299299
/// <param name="obj">Object to get the value from</param>
300300
/// <returns>Value of the member</returns>
301-
public static object GetMemberValue(object member, object obj)
301+
private static object GetMemberValue(object member, object obj)
302302
{
303303
object value = null;
304304

@@ -327,22 +327,43 @@ public static object GetMemberValue(object member, object obj)
327327
/// </summary>
328328
/// <param name="type">Type of instance to get</param>
329329
/// <param name="properties">List of properties and values</param>
330-
/// <param name="parentHash">Hash from parent object</param>
330+
/// <param name="parentInstance">Parent instance. Can be NULL if this is the root instance.</param>
331331
/// <returns>
332332
/// Tuple of bool, object, int where bool represents whether this is a newly created instance,
333333
/// object being an instance of the requested type and int being the instance's identifier hash.
334334
/// </returns>
335-
public static Tuple<bool, object, int> GetInstance(Type type, IDictionary<string, object> properties, int parentHash)
335+
internal static Tuple<bool, object, Tuple<int, int, object>> GetInstance(Type type, IDictionary<string, object> properties, object parentInstance = null)
336336
{
337+
var key = GetCacheKey(type, properties, parentInstance);
338+
337339
var instanceCache = Cache.GetInstanceCache();
338340

339-
var identifiers = GetIdentifiers(type);
341+
object instance;
342+
343+
var isNewlyCreatedInstance = !instanceCache.TryGetValue(key, out instance);
344+
345+
if (isNewlyCreatedInstance)
346+
{
347+
instance = CreateInstance(type);
348+
instanceCache[key] = instance;
349+
}
350+
351+
return Tuple.Create(isNewlyCreatedInstance, instance, key);
352+
}
340353

341-
object instance = null;
354+
private static Tuple<int, int, object> GetCacheKey(Type type, IDictionary<string, object> properties, object parentInstance)
355+
{
356+
var identifierHash = GetIdentifierHash(type, properties);
342357

343-
bool isNewlyCreatedInstance = false;
358+
var key = Tuple.Create(identifierHash, type.GetHashCode(), parentInstance);
359+
return key;
360+
}
344361

345-
int identifierHash = 0;
362+
private static int GetIdentifierHash(Type type, IDictionary<string, object> properties)
363+
{
364+
var identifiers = GetIdentifiers(type);
365+
366+
var identifierHash = 0;
346367

347368
if (identifiers != null)
348369
{
@@ -352,40 +373,23 @@ public static Tuple<bool, object, int> GetInstance(Type type, IDictionary<string
352373
{
353374
var identifierValue = properties[identifier];
354375
if (identifierValue != null)
355-
identifierHash += identifierValue.GetHashCode() + type.GetHashCode() + parentHash;
356-
}
357-
}
358-
359-
if (identifierHash != 0)
360-
{
361-
if (instanceCache.ContainsKey(identifierHash))
362-
{
363-
instance = instanceCache[identifierHash];
364-
}
365-
else
366-
{
367-
instance = CreateInstance(type);
368-
369-
instanceCache.Add(identifierHash, instance);
370-
371-
isNewlyCreatedInstance = true;
376+
{
377+
// Unchecked to avoid arithmetic overflow
378+
unchecked
379+
{
380+
// Include identifier hashcode to avoid collisions between e.g. multiple int IDs
381+
identifierHash += identifierValue.GetHashCode() + identifier.GetHashCode();
382+
}
383+
}
372384
}
373385
}
374386
}
375-
376-
// An identifier hash with a value of zero means the type does not have any identifiers.
377-
// To make this instance unique generate a unique hash for it.
378-
if (identifierHash == 0 && identifiers != null) identifierHash = type.GetHashCode() + parentHash;
379-
380-
if (instance == null)
387+
else
381388
{
382-
instance = CreateInstance(type);
389+
// If the type has no identifiers we must generate a unique hash for it.
383390
identifierHash = Guid.NewGuid().GetHashCode();
384-
385-
isNewlyCreatedInstance = true;
386391
}
387-
388-
return new Tuple<bool, object, int>(isNewlyCreatedInstance, instance, identifierHash);
392+
return identifierHash;
389393
}
390394

391395
/// <summary>
@@ -399,7 +403,7 @@ public static Tuple<bool, object, int> GetInstance(Type type, IDictionary<string
399403
/// <param name="instance">Instance to populate</param>
400404
/// <param name="parentInstance">Optional parent instance of the instance being populated</param>
401405
/// <returns>Populated instance</returns>
402-
public static object Map(IDictionary<string, object> dictionary, object instance, object parentInstance = null)
406+
internal static object Map(IDictionary<string, object> dictionary, object instance, object parentInstance = null)
403407
{
404408
if (instance.GetType().IsPrimitive || instance is string)
405409
{
@@ -471,7 +475,7 @@ public static object Map(IDictionary<string, object> dictionary, object instance
471475
}
472476
else
473477
{
474-
var result = GetInstance(memberType, newDictionary, parentInstance == null ? 0 : parentInstance.GetHashCode());
478+
var result = GetInstance(memberType, newDictionary, parentInstance);
475479
nestedInstance = result.Item2;
476480
}
477481
}
@@ -509,7 +513,7 @@ public static object Map(IDictionary<string, object> dictionary, object instance
509513
/// <param name="instance">Instance to populate</param>
510514
/// <param name="parentInstance">Optional parent instance of the instance being populated</param>
511515
/// <returns>Populated instance</returns>
512-
public static object MapCollection(Type type, IDictionary<string, object> dictionary, object instance, object parentInstance = null)
516+
internal static object MapCollection(Type type, IDictionary<string, object> dictionary, object instance, object parentInstance = null)
513517
{
514518
Type baseListType = typeof(List<>);
515519

@@ -526,7 +530,7 @@ public static object MapCollection(Type type, IDictionary<string, object> dictio
526530
return instance;
527531
}
528532

529-
var getInstanceResult = GetInstance(type, dictionary, parentInstance == null ? 0 : parentInstance.GetHashCode());
533+
var getInstanceResult = GetInstance(type, dictionary, parentInstance);
530534

531535
// Is this a newly created instance? If false, then this item was retrieved from the instance cache.
532536
bool isNewlyCreatedInstance = getInstanceResult.Item1;
@@ -582,7 +586,7 @@ public static object MapCollection(Type type, IDictionary<string, object> dictio
582586
/// Provides a means of getting/storing data in the host application's
583587
/// appropriate context.
584588
/// </summary>
585-
public interface IContextStorage
589+
internal interface IContextStorage
586590
{
587591
/// <summary>
588592
/// Get a stored item.
@@ -683,7 +687,7 @@ public void Remove(string key)
683687
/// For ASP.NET applications, it will store in the data in the current HTTPContext.
684688
/// For all other applications, it will store the data in the logical call context.
685689
/// </remarks>
686-
public static class ContextStorage
690+
internal static class ContextStorage
687691
{
688692
/// <summary>
689693
/// Provides a means of getting/storing data in the host application's
@@ -730,7 +734,7 @@ public static void Remove(string key)
730734
/// <summary>
731735
/// Contains the methods and members responsible for this libraries reflection concerns.
732736
/// </summary>
733-
public static class ReflectionHelper
737+
private static class ReflectionHelper
734738
{
735739
/// <summary>
736740
/// Provides access to System.Web.HttpContext.Current.Items via reflection.

0 commit comments

Comments
 (0)