fixup! Recompute hashcodes on deserialization

Re-implement Printer.ToString

Author: fredericDelaporte

src/NHibernate.Test/CacheTest/FilterKeyFixture.cs

@@ -20,13 +20,13 @@ public void ToStringIncludeAll()
 			var f = new FilterImpl(Sfi.GetFilterDefinition(filterName));
 			f.SetParameter("pLike", "so%");
 			var fk = new FilterKey(f);
-			Assert.That(fk.ToString(), Is.EqualTo("FilterKey[DescriptionLike{'pLike'='so%'}]"), "Like");
+			Assert.That(fk.ToString(), Is.EqualTo("FilterKey[DescriptionLike['[pLike, so%]']]"), "Like");
 
 			filterName = "DescriptionEqualAndValueGT";
 			f = new FilterImpl(Sfi.GetFilterDefinition(filterName));
 			f.SetParameter("pDesc", "something").SetParameter("pValue", 10);
 			fk = new FilterKey(f);
-			Assert.That(fk.ToString(), Is.EqualTo("FilterKey[DescriptionEqualAndValueGT{'pDesc'='something', 'pValue'='10'}]"), "Value");
+			Assert.That(fk.ToString(), Is.EqualTo("FilterKey[DescriptionEqualAndValueGT['[pDesc, something]', '[pValue, 10]']]"), "Value");
 		}
 
 		[Test]

src/NHibernate.Test/CacheTest/QueryKeyFixture.cs

@@ -1,9 +1,9 @@
-using System.Collections;
 using System.Collections.Generic;
 using NHibernate.Cache;
 using NHibernate.Engine;
 using NHibernate.Impl;
 using NHibernate.SqlCommand;
+using NHibernate.Util;
 using NUnit.Framework;
 
 namespace NHibernate.Test.CacheTest
@@ -88,7 +88,7 @@ public void HashCodeWithFilters()
 		public void NotEqualHashCodeWithFilters()
 		{
 			// GetHashCode semantic does not guarantee no collision may ever occur, but the algorithm should
-			// generates different hashcodes for similar but inequal cases. These tests check that cache keys
+			// generates different hashcodes for similar but unequal cases. These tests check that cache keys
 			// for a query generated for different parameters values are no more equal.
 			QueryKeyFilterDescLikeToCompare(out var qk, out var qk1, false);
 			Assert.That(qk.GetHashCode(), Is.Not.EqualTo(qk1.GetHashCode()), "qk & qk1");
@@ -100,6 +100,20 @@ public void NotEqualHashCodeWithFilters()
 			Assert.That(qk1.GetHashCode(), Is.Not.EqualTo(qvk1.GetHashCode()), "qk1 & qvk1");
 		}
 
+		[Test]
+		public void HashCodeWithFiltersAndSerialization()
+		{
+			QueryKeyFilterDescLikeToCompare(out var qk, out var qk1, true);
+			var bytes = SerializationHelper.Serialize(qk);
+			qk = (QueryKey) SerializationHelper.Deserialize(bytes);
+			Assert.That(qk.GetHashCode(), Is.EqualTo(qk1.GetHashCode()), "Like");
+
+			QueryKeyFilterDescValueToCompare(out qk, out qk1, true);
+			bytes = SerializationHelper.Serialize(qk);
+			qk = (QueryKey) SerializationHelper.Deserialize(bytes);
+			Assert.That(qk.GetHashCode(), Is.EqualTo(qk1.GetHashCode()), "Value");
+		}
+
 		[Test]
 		public void ToStringWithFilters()
 		{

src/NHibernate.Test/DebugSessionFactory.cs

@@ -30,25 +30,31 @@ namespace NHibernate.Test
 	/// it is used when testing to check that tests clean up after themselves.
 	/// </summary>
 	/// <remarks>Sessions opened from other sessions are not tracked.</remarks>
+	[Serializable]
 	public partial class DebugSessionFactory : ISessionFactoryImplementor
 	{
+		[NonSerialized]
+		private DebugConnectionProvider _debugConnectionProvider;
+
 		/// <summary>
 		/// The debug connection provider if configured for using it, <see langword="null"/> otherwise.
 		/// Use <c>ActualFactory.ConnectionProvider</c> if needing unconditionally the connection provider, be
 		/// it debug or not.
 		/// </summary>
-		public DebugConnectionProvider DebugConnectionProvider { get; }
+		public DebugConnectionProvider DebugConnectionProvider
+			=> _debugConnectionProvider ??
+				(_debugConnectionProvider = ActualFactory.ConnectionProvider as DebugConnectionProvider);
 		public ISessionFactoryImplementor ActualFactory { get; }
 
 		public EventListeners EventListeners => ((SessionFactoryImpl)ActualFactory).EventListeners;
 
+		[NonSerialized]
 		private readonly ConcurrentBag<ISessionImplementor> _openedSessions = new ConcurrentBag<ISessionImplementor>();
 		private static readonly ILog _log = LogManager.GetLogger(typeof(DebugSessionFactory).Assembly, typeof(TestCase));
 
 		public DebugSessionFactory(ISessionFactory actualFactory)
 		{
 			ActualFactory = (ISessionFactoryImplementor)actualFactory;
-			DebugConnectionProvider = ActualFactory.ConnectionProvider as DebugConnectionProvider;
 		}
 
 		#region Session tracking

src/NHibernate/Cache/CacheKey.cs

@@ -1,4 +1,5 @@
 using System;
+using System.Runtime.Serialization;
 using NHibernate.Engine;
 using NHibernate.Type;
 
@@ -10,12 +11,15 @@ namespace NHibernate.Cache
 	/// keys which do not properly implement equals()/hashCode().
 	/// </summary>
 	[Serializable]
-	public class CacheKey
+	public class CacheKey : IDeserializationCallback
 	{
 		private readonly object key;
 		private readonly IType type;
 		private readonly string entityOrRoleName;
-		private readonly int hashCode;
+		// hashcode may vary among processes, they cannot be stored and have to be re-computed after deserialization
+		[NonSerialized]
+		private int? _hashCode;
+		private readonly ISessionFactoryImplementor _factory;
 
 		/// <summary> 
 		/// Construct a new key for a collection or entity instance.
@@ -31,7 +35,9 @@ public CacheKey(object id, IType type, string entityOrRoleName, ISessionFactoryI
 			key = id;
 			this.type = type;
 			this.entityOrRoleName = entityOrRoleName;
-			hashCode = type.GetHashCode(key, factory);
+			_factory = factory;
+
+			_hashCode = GenerateHashCode();
 		}
 
 		//Mainly for SysCache and Memcache
@@ -50,7 +56,22 @@ public override bool Equals(object obj)
 
 		public override int GetHashCode()
 		{
-			return hashCode;
+			// If the object is put in a set or dictionary during deserialization, the hashcode will not yet be
+			// computed. Compute the hashcode on the fly. So long as this happens only during deserialization, there
+			// will be no thread safety issues. For the hashcode to be always defined after deserialization, the
+			// deserialization callback is used.
+			return _hashCode ?? GenerateHashCode();
+		}
+
+		/// <inheritdoc />
+		public void OnDeserialization(object sender)
+		{
+			_hashCode = GenerateHashCode();
+		}
+
+		private int GenerateHashCode()
+		{
+			return type.GetHashCode(key, _factory);
 		}
 
 		public object Key
@@ -63,4 +84,4 @@ public string EntityOrRoleName
 			get { return entityOrRoleName; }
 		}
 	}
-}
+}

src/NHibernate/Cache/FilterKey.cs

@@ -1,5 +1,6 @@
 using System;
 using System.Collections.Generic;
+using System.Linq;
 using NHibernate.Engine;
 using NHibernate.Impl;
 using NHibernate.Type;
@@ -11,29 +12,38 @@ namespace NHibernate.Cache
 	public class FilterKey
 	{
 		private readonly string _filterName;
-		private readonly Dictionary<string, TypedValue> _filterParameters = new Dictionary<string, TypedValue>();
+
+		// Sets and dictionaries are populated last during deserialization, causing them to be potentially empty
+		// during the deserialization callback. This causes them to be unreliable when used in hashcode or equals
+		// computations. These computations occur during the deserialization callback for example when another
+		// serialized set or dictionary contain an instance of this class. (QueryKey was having such a dictionary
+		// or set.)
+		// So better serialize them as other structures, so long for Equals implementation which actually needs a
+		// dictionary.
+		private readonly KeyValuePair<string, TypedValue>[] _filterParameters;
 
 		// Since v5.2
 		[Obsolete("Use overload taking a FilterImpl")]
 		public FilterKey(string name, IEnumerable<KeyValuePair<string, object>> @params, IDictionary<string, IType> types)
 		{
 			_filterName = name;
-			foreach (KeyValuePair<string, object> me in @params)
-			{
-				IType type = types[me.Key];
-				_filterParameters[me.Key] = new TypedValue(type, me.Value);
-			}
+			_filterParameters = @params.Select(
+				p => new KeyValuePair<string, TypedValue>(
+					p.Key,
+					new TypedValue(types[p.Key], p.Value))).ToArray();
 		}
 
 		public FilterKey(FilterImpl filter)
 		{
 			var types = filter.FilterDefinition.ParameterTypes;
 			_filterName = filter.Name;
-			foreach (var me in filter.Parameters)
-			{
-				var type = types[me.Key];
-				_filterParameters[me.Key] = new TypedValue(type, me.Value, filter.GetParameterSpan(me.Key) != null);
-			}
+			_filterParameters = filter.Parameters.Select(
+				p => new KeyValuePair<string, TypedValue>(
+					p.Key,
+					new TypedValue(
+						types[p.Key],
+						p.Value,
+						filter.GetParameterSpan(p.Key) != null))).ToArray();
 		}
 
 		public override int GetHashCode()
@@ -47,15 +57,16 @@ public override int GetHashCode()
 		public override bool Equals(object other)
 		{
 			var that = other as FilterKey;
-			if (that == null)
-			{
-				return false;
-			}
-			if (!that._filterName.Equals(_filterName))
+			if (that == null || !that._filterName.Equals(_filterName))
 				return false;
-			if (!CollectionHelper.DictionaryEquals<string, TypedValue>(that._filterParameters, _filterParameters))
-				return false;
-			return true;
+
+			// BagEquals is less efficient than a DictionaryEquals, but serializing dictionaries causes issues on
+			// deserialization if GetHashCode or Equals are called in its deserialization callback. And building
+			// dictionaries on the fly will in most cases be worst than BagEquals, unless re-coding its short-circuits.
+			return CollectionHelper.BagEquals(
+				_filterParameters,
+				that._filterParameters,
+				NamedParameterComparer.Instance);
 		}
 
 		public override string ToString()

src/NHibernate/Cache/QueryKey.cs

@@ -1,5 +1,7 @@
 using System;
 using System.Collections.Generic;
+using System.Linq;
+using System.Runtime.Serialization;
 using System.Text;
 using NHibernate.Engine;
 using NHibernate.Impl;
@@ -11,23 +13,32 @@
 namespace NHibernate.Cache
 {
 	[Serializable]
-	public class QueryKey
+	public class QueryKey : IDeserializationCallback
 	{
 		private readonly ISessionFactoryImplementor _factory;
 		private readonly SqlString _sqlQueryString;
 		private readonly IType[] _types;
 		private readonly object[] _values;
 		private readonly int _firstRow = RowSelection.NoValue;
 		private readonly int _maxRows = RowSelection.NoValue;
-		private readonly IDictionary<string, TypedValue> _namedParameters;
-		private readonly ISet<FilterKey> _filters;
+
+		// Sets and dictionaries are populated last during deserialization, causing them to be potentially empty
+		// during the deserialization callback. This causes them to be unreliable when used in hashcode or equals
+		// computations. These computations occur during the deserialization callback for example when another
+		// serialized set or dictionary contain an instance of this class.
+		// So better serialize them as other structures, so long for Equals implementation which actually needs a
+		// dictionary and set.
+		private readonly KeyValuePair<string, TypedValue>[] _namedParameters;
+		private readonly FilterKey[] _filters;
+
 		private readonly CacheableResultTransformer _customTransformer;
-		private readonly int _hashCode;
+		// hashcode may vary among processes, they cannot be stored and have to be re-computed after deserialization
+		[NonSerialized]
+		private int? _hashCode;
 
 		private int[] _multiQueriesFirstRows;
 		private int[] _multiQueriesMaxRows;
 
-
 		/// <summary>
 		/// Initializes a new instance of the <see cref="QueryKey"/> class.
 		/// </summary>
@@ -55,9 +66,11 @@ public QueryKey(ISessionFactoryImplementor factory, SqlString queryString, Query
 				_firstRow = RowSelection.NoValue;
 				_maxRows = RowSelection.NoValue;
 			}
-			_namedParameters = queryParameters.NamedParameters;
-			_filters = filters;
+
+			_namedParameters = queryParameters.NamedParameters?.ToArray();
+			_filters = filters?.ToArray();
 			_customTransformer = customTransformer;
+
 			_hashCode = ComputeHashCode();
 		}
 
@@ -127,15 +140,14 @@ public override bool Equals(object other)
 				}
 			}
 
-			if (!CollectionHelper.SetEquals(_filters, that._filters))
-			{
+			// BagEquals is less efficient than a SetEquals or DictionaryEquals, but serializing dictionaries causes
+			// issues on deserialization if GetHashCode or Equals are called in its deserialization callback. And
+			// building sets or dictionaries on the fly will in most cases be worst than BagEquals, unless re-coding
+			// its short-circuits.
+			if (!CollectionHelper.BagEquals(_filters, that._filters))
 				return false;
-			}
-
-			if (!CollectionHelper.DictionaryEquals(_namedParameters, that._namedParameters))
-			{
+			if (!CollectionHelper.BagEquals(_namedParameters, that._namedParameters, NamedParameterComparer.Instance))
 				return false;
-			}
 
 			if (!CollectionHelper.SequenceEquals<int>(_multiQueriesFirstRows, that._multiQueriesFirstRows))
 			{
@@ -150,7 +162,17 @@ public override bool Equals(object other)
 
 		public override int GetHashCode()
 		{
-			return _hashCode;
+			// If the object is put in a set or dictionary during deserialization, the hashcode will not yet be
+			// computed. Compute the hashcode on the fly. So long as this happens only during deserialization, there
+			// will be no thread safety issues. For the hashcode to be always defined after deserialization, the
+			// deserialization callback is used.
+			return _hashCode ?? ComputeHashCode();
+		}
+
+		/// <inheritdoc />
+		public void OnDeserialization(object sender)
+		{
+			_hashCode = ComputeHashCode();
 		}
 
 		public int ComputeHashCode()
@@ -161,7 +183,9 @@ public int ComputeHashCode()
 				result = 37 * result + _firstRow.GetHashCode();
 				result = 37 * result + _maxRows.GetHashCode();
 
-				result = 37 * result + (_namedParameters == null ? 0 : CollectionHelper.GetHashCode(_namedParameters, NamedParameterComparer.Instance));
+				result = 37 * result + (_namedParameters == null
+					? 0
+					: CollectionHelper.GetHashCode(_namedParameters, NamedParameterComparer.Instance));
 
 				for (int i = 0; i < _types.Length; i++)
 				{
@@ -190,10 +214,7 @@ public int ComputeHashCode()
 
 				if (_filters != null)
 				{
-					foreach (object filter in _filters)
-					{
-						result = 37 * result + filter.GetHashCode();
-					}
+					result = 37 * result + CollectionHelper.GetHashCode(_filters);
 				}
 
 				result = 37 * result + (_customTransformer == null ? 0 : _customTransformer.GetHashCode());

src/NHibernate/Engine/AssociationKey.cs

@@ -12,13 +12,11 @@ internal sealed class AssociationKey
 	{
 		private readonly EntityKey ownerKey;
 		private readonly string propertyName;
-		private readonly int hashCode;
 
 		public AssociationKey(EntityKey ownerKey, string propertyName)
 		{
 			this.ownerKey = ownerKey;
 			this.propertyName = propertyName;
-			hashCode = ownerKey.GetHashCode() ^ propertyName.GetHashCode() ^ ownerKey.EntityName.GetHashCode();
 		}
 
 		public override bool Equals(object that)
@@ -40,7 +38,7 @@ public override bool Equals(object that)
 
 		public override int GetHashCode()
 		{
-			return hashCode;
+			return ownerKey.GetHashCode() ^ propertyName.GetHashCode() ^ ownerKey.EntityName.GetHashCode();
 		}
 	}
-}
+}