fixup! Recompute hashcodes on deserialization

Simplify it

Author: fredericDelaporte

src/NHibernate/Cache/FilterKey.cs

@@ -1,7 +1,6 @@
 using System;
 using System.Collections.Generic;
 using System.Linq;
-using System.Runtime.Serialization;
 using NHibernate.Engine;
 using NHibernate.Impl;
 using NHibernate.Type;
@@ -10,81 +9,65 @@
 namespace NHibernate.Cache
 {
 	[Serializable]
-	public class FilterKey : IDeserializationCallback
+	public class FilterKey
 	{
 		private readonly string _filterName;
 
 		// 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 computations.
-		// So better serialize them as other structures and use those other structures in hashcode computation.
-		[NonSerialized]
-		private Dictionary<string, TypedValue> _filterParameters = new Dictionary<string, TypedValue>();
-		private IEnumerable<KeyValuePair<string, TypedValue>> _serializedFilterParameters;
+		// 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()
 		{
 			var result = 13;
 			result = 37 * result + _filterName.GetHashCode();
 
-			// On deserialization, _filterParameters may be not yet populated. Use _serializedFilterParameters
-			// if it is defined.
-			var filterParameters = _serializedFilterParameters ?? _filterParameters;
-			result = 37 * result + CollectionHelper.GetHashCode(filterParameters, NamedParameterComparer.Instance);
+			result = 37 * result + CollectionHelper.GetHashCode(_filterParameters, NamedParameterComparer.Instance);
 			return result;
 		}
 
 		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;
 
-			// On deserialization, _filterParameters may be not yet populated. Use _serializedFilterParameters
-			// if it is defined.
-			var filterParameters = _filterParameters;
-			if (_serializedFilterParameters != null)
-			{
-				filterParameters = new Dictionary<string, TypedValue>(_serializedFilterParameters.Count());
-				foreach (var param in _serializedFilterParameters)
-					filterParameters.Add(param.Key, param.Value);
-			}
-			var thatFilterParameters = that._filterParameters;
-			if (that._serializedFilterParameters != null)
-			{
-				thatFilterParameters = new Dictionary<string, TypedValue>(that._serializedFilterParameters.Count());
-				foreach (var param in that._serializedFilterParameters)
-					thatFilterParameters.Add(param.Key, param.Value);
-			}
-			if (!CollectionHelper.DictionaryEquals<string, TypedValue>(thatFilterParameters, 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()
@@ -105,21 +88,5 @@ public static ISet<FilterKey> CreateFilterKeys(IDictionary<string, IFilter> enab
 			}
 			return result;
 		}
-
-		[OnSerializing]
-		private void OnSerializing(StreamingContext context)
-		{
-			_serializedFilterParameters = _filterParameters.ToArray();
-		}
-
-		/// <inheritdoc />
-		public void OnDeserialization(object sender)
-		{
-			_filterParameters = new Dictionary<string, TypedValue>(_serializedFilterParameters.Count());
-			foreach (var param in _serializedFilterParameters)
-				_filterParameters.Add(param.Key, param.Value);
-
-			_serializedFilterParameters = null;
-		}
 	}
 }

src/NHibernate/Cache/QueryKey.cs

@@ -23,14 +23,13 @@ public class QueryKey : IDeserializationCallback
 		private readonly int _maxRows = RowSelection.NoValue;
 
 		// 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 computations.
-		// So better also serialize them as other structures and use those other structures in hashcode computation.
-		// We nonetheless need to also serialize them, because they are externally supplied and may use a custom
-		// comparer, implementation, ...
-		private readonly IDictionary<string, TypedValue> _namedParameters;
-		private readonly ISet<FilterKey> _filters;
-		private IEnumerable<KeyValuePair<string, TypedValue>> _serializedNamedParameters;
-		private IEnumerable<FilterKey> _serializedFilters;
+		// 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;
 		// hashcode may vary among processes, they cannot be stored and have to be re-computed after deserialization
@@ -67,8 +66,9 @@ 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();
@@ -140,35 +140,14 @@ public override bool Equals(object other)
 				}
 			}
 
-			// On deserialization, _namedParameters and _filters may be not yet populated. Use their _serialization
-			// counterparts if they are defined.
-			if (_serializedFilters != null || that._serializedFilters != null)
-			{
-				var filters = _serializedFilters ?? _filters;
-				var thatFilters = that._serializedFilters ?? that._filters;
-				// BagEquals is less efficient than a SetEquals, but if the sets are using a custom comparer, it
-				// will cause less issues than rebuilding sets without the custom comparer.
-				if (!CollectionHelper.BagEquals(filters, thatFilters))
-					return false;
-			}
-			else 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 (_serializedNamedParameters != null || that._serializedNamedParameters != null)
-			{
-				var namedParameters = _serializedNamedParameters ?? _namedParameters;
-				var thatNamedParameters = that._serializedNamedParameters ?? that._namedParameters;
-				// BagEquals is less efficient than a DictionaryEquals, but if the dictionaries are using a custom
-				// comparer, it will cause less issues than rebuilding the dictionaries without the custom comparer.
-				if (!CollectionHelper.BagEquals(namedParameters, thatNamedParameters, NamedParameterComparer.Instance))
-					return false;
-			}
-			else if (!CollectionHelper.DictionaryEquals(_namedParameters, that._namedParameters))
-			{
+			if (!CollectionHelper.BagEquals(_namedParameters, that._namedParameters, NamedParameterComparer.Instance))
 				return false;
-			}
 
 			if (!CollectionHelper.SequenceEquals<int>(_multiQueriesFirstRows, that._multiQueriesFirstRows))
 			{
@@ -194,16 +173,6 @@ public override int GetHashCode()
 		public void OnDeserialization(object sender)
 		{
 			_hashCode = ComputeHashCode();
-
-			_serializedNamedParameters = null;
-			_serializedFilters = null;
-		}
-
-		[OnSerializing]
-		private void OnSerializing(StreamingContext context)
-		{
-			_serializedNamedParameters = _namedParameters?.ToArray();
-			_serializedFilters = _filters?.ToArray();
 		}
 
 		public int ComputeHashCode()
@@ -214,10 +183,9 @@ public int ComputeHashCode()
 				result = 37 * result + _firstRow.GetHashCode();
 				result = 37 * result + _maxRows.GetHashCode();
 
-				// On deserialization, _namedParameters may be not yet populated. Use _serializedNamedParameters
-				// if it is defined.
-				var namedParameters = _serializedNamedParameters ?? _namedParameters;
-				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++)
 				{
@@ -244,12 +212,9 @@ public int ComputeHashCode()
 					}
 				}
 
-				// On deserialization, _filters may be not yet populated. Use _serializedFilters
-				// if it is defined.
-				var filters = _serializedFilters ?? _filters;
-				if (filters != null)
+				if (_filters != null)
 				{
-					result = 37 * result + CollectionHelper.GetHashCode(filters);
+					result = 37 * result + CollectionHelper.GetHashCode(_filters);
 				}
 
 				result = 37 * result + (_customTransformer == null ? 0 : _customTransformer.GetHashCode());

src/NHibernate/Engine/AssociationKey.cs

@@ -1,5 +1,4 @@
 using System;
-using System.Runtime.Serialization;
 
 namespace NHibernate.Engine
 {
@@ -9,20 +8,15 @@ namespace NHibernate.Engine
 	/// is null during loading. 
 	/// </summary>
 	[Serializable]
-	internal sealed class AssociationKey : IDeserializationCallback
+	internal sealed class AssociationKey
 	{
 		private readonly EntityKey ownerKey;
 		private readonly string propertyName;
-		// hashcode may vary among processes, they cannot be stored and have to be re-computed after deserialization
-		[NonSerialized]
-		private int? _hashCode;
 
 		public AssociationKey(EntityKey ownerKey, string propertyName)
 		{
 			this.ownerKey = ownerKey;
 			this.propertyName = propertyName;
-
-			_hashCode = GenerateHashCode();
 		}
 
 		public override bool Equals(object that)
@@ -43,21 +37,6 @@ public override bool Equals(object that)
 		}
 
 		public override int GetHashCode()
-		{
-			// 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 ownerKey.GetHashCode() ^ propertyName.GetHashCode() ^ ownerKey.EntityName.GetHashCode();
 		}