Fix TypedValue not always using adequate comparer with SetParameterList #1612

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Merged

fredericDelaporte merged 5 commits into nhibernate:master from fredericDelaporte:TypedValue

May 28, 2018

Member

fredericDelaporte commented Mar 13, 2018 •

edited

Loading

TypedValue may use the default comparer instead of the list comparer when a list parameter is supplied with a HashSet. This will then result in undue cache miss.

~~This PR is build over #1611 (two first commit currently appearing here).~~

fredericDelaporte added t: Bug c: Core p: Minor labels

fredericDelaporte force-pushed the TypedValue branch from 5b46348 to da7b6d4 Compare

March 13, 2018 17:54

fredericDelaporte commented

View reviewed changes

src/NHibernate.Test/CacheTest/FilterKeyFixture.cs

               		}
               		[Test]
               		public void Equality()
               		{
-              			// Equality is aware only by parameters names not values

Member Author

fredericDelaporte Mar 13, 2018

Wrong, but not contradicted by the test because the test was bugged.

src/NHibernate.Test/CacheTest/FilterKeyFixture.cs

               			var f1 = new FilterImpl(Sfi.GetFilterDefinition(filterName));
-              			f1.SetParameter("pLike", "%ing");
-              			fk1 = new FilterKey(filterName, f.Parameters, f.FilterDefinition.ParameterTypes);

Member Author

fredericDelaporte Mar 13, 2018

Here lies the test bug, affecting all tests: the second key was construct with the first filter values...

src/NHibernate.Test/CacheTest/FilterKeyFixture.cs

               			var f1 = new FilterImpl(Sfi.GetFilterDefinition(filterName));
-              			f1.SetParameter("pDesc", "something").SetParameter("pValue", 11);
-              			fk1 = new FilterKey(filterName, f.Parameters, f.FilterDefinition.ParameterTypes);

Member Author

fredericDelaporte Mar 13, 2018

Same test bug than in FilterDescLikeToCompare, fixed too.

src/NHibernate.Test/CacheTest/FilterKeyFixture.cs

-              			FilterKey fk, fk1;
-              			FilterDescLikeToCompare(out fk, out fk1);
-              			Assert.That(fk, Is.EqualTo(fk1));
+              			FilterDescLikeToCompare(out var fk, out var fk1, true);

Member Author

fredericDelaporte Mar 13, 2018

They can be equal only with same value. The NotEquality test is completed with an assert showcasing that a different value causes inequality.

src/NHibernate.Test/CacheTest/FilterKeyFixture.cs

               		}
               		[Test]
               		public void HashCode()
               		{
-              			// HashCode is aware only by parameters names not values (should work as Equal)

Member Author

fredericDelaporte Mar 13, 2018

Unfortunately true for the current hashcode implementation, which is bad for performances. But in fact it should ideally not be equal, since actually filter with different parameters values are not equals.

src/NHibernate.Test/CacheTest/QueryKeyFixture.cs

    
              			f1.SetParameter("pLike", "%ing");

              			var fk1 = new FilterKey(filterName, f.Parameters, f.FilterDefinition.ParameterTypes);

              			f1.SetParameter("pLike", sameValue ? "so%" : "%ing");

              			var fk1 = new FilterKey(f1);

Member Author

fredericDelaporte Mar 13, 2018

Same test bug as in FilterKeyFixture.

This was referenced Mar 13, 2018

Fix CollectionHelper.GetHashCode ignoring values of dictionaries #1613

Closed

Fix IFilter.SetParameterList not supporting HashSet<T> arguments #1611

Merged

fredericDelaporte force-pushed the TypedValue branch 2 times, most recently from 1644337 to c7894b0 Compare

March 14, 2018 01:00

fredericDelaporte added this to the 5.2 milestone

Member

hazzik commented May 10, 2018

Needs to be rewritten as 5.1.2 is merged to master now.

fredericDelaporte force-pushed the TypedValue branch 2 times, most recently from 608a925 to abed562 Compare

May 10, 2018 12:10

Member Author

fredericDelaporte commented May 10, 2018

Done.

Member Author

fredericDelaporte commented May 10, 2018

If this PR is merged after #1613, it should then further adjusts query and filter key hashcode tests.

fredericDelaporte force-pushed the TypedValue branch from abed562 to cd0214e Compare

May 10, 2018 13:02

hazzik reviewed

View reviewed changes

src/NHibernate/Cache/FilterKey.cs Outdated

@@ @@ -13,6 +13,8 @@ public class FilterKey @@
               		private readonly string _filterName;
               		private readonly Dictionary<string, TypedValue> _filterParameters = new Dictionary<string, TypedValue>();
+              		// Since v5.1

Member

hazzik May 10, 2018

5.2

hazzik reviewed

View reviewed changes

src/NHibernate/Engine/TypedValue.cs

    
              			{

              				if (x == y)

              					return true;

              				if (x == null || y == null)

              					return false;

              				if (x.Count != y.Count)

              				if (x is ICollection xCol && y is ICollection yCol && xCol.Count != yCol.Count)

              					return false;

              				var ye = y.GetEnumerator();

Member

hazzik May 10, 2018 •

edited

Loading

Here are a few bugs:

[Test]
public void PaddedShouldNotEqualShort()
{
	var v1 = new TypedValue(NHibernateUtil.Int32, ToLazyEnumerable(3, 2, 1, 0, 0, 0), true);
	var v2 = new TypedValue(NHibernateUtil.Int32, ToLazyEnumerable(3, 2, 1, 0), true);

	Warn.If(v1.GetHashCode() == v2.GetHashCode(), "Equal hash codes");
	Assert.AreNotEqual(v1, v2);
}

[Test]
public void ShortShouldNotEqualLonger()
{
	var v1 = new TypedValue(NHibernateUtil.Int32, ToLazyEnumerable(1, 2, 3), true);
	var v2 = new TypedValue(NHibernateUtil.Int32, ToLazyEnumerable(1, 2, 3, 1, -1), true);

	Warn.If(v1.GetHashCode() == v2.GetHashCode(), "Equal hash codes");
	Assert.AreNotEqual(v1, v2);
}

static IEnumerable<T> ToLazyEnumerable<T>(params T[] array)
{
	// We want to get an IEnumerable which is lazy (eg does not implement ICollection interface)
	
	// ReSharper disable once LoopCanBeConvertedToQuery
	foreach (var item in array) yield return item;
}

Member Author

fredericDelaporte May 11, 2018 •

edited

Loading

Yes, my bad. (Now fixed.)

fredericDelaporte mentioned this pull request

Implement CollectionHelper.GetHashCode that accepts IEqualityComparer<T> #1694

Merged

fredericDelaporte force-pushed the TypedValue branch from f9b2257 to 315c79c Compare

May 15, 2018 22:31

Member Author

fredericDelaporte commented May 15, 2018

Rebased and hashcode tests enabled.

(All commits are to be squashed in one.)

hazzik reviewed

View reviewed changes

src/NHibernate/Criterion/InExpression.cs

               					}
               				}
               				return list;
               			}
               			else
               			{
-              				return _values.Select(v => new TypedValue(type, v)).ToList();
+              				return _values.Select(v => new TypedValue(type, v, false)).ToList();

Member

hazzik May 23, 2018

Not sure about this...

Member Author

fredericDelaporte May 24, 2018

_values is the collection of values to be put in the in clause. They must be elements, not themselves lists.

hazzik reviewed

View reviewed changes

src/NHibernate/Engine/TypedValue.cs Outdated

		@@ -66,50 +81,60 @@ public bool Equals(TypedValue x, TypedValue y)
		if (y == null) return false;

Member

hazzik May 27, 2018

Can we add reference equals short-cut?

Member

hazzik May 27, 2018

And for DefaultComparer as well

fredericDelaporte added 4 commits

May 28, 2018 10:31


          Fix TypedValue not using adequate comparer with SetParameterList

45aa0fa


          Specify to TypeValue whether the value is a list or not

cacde04


          Fix TypedValue hash-code colliding too easily

e91f9e6


          Enable query cache key hashcode tests

bfafb14

fredericDelaporte force-pushed the TypedValue branch from 660a35c to bfafb14 Compare

May 28, 2018 08:31


          Add shortcuts in TypedValue comparators

02cd325

And add an argument pre-condition check.

To be squashed.

fredericDelaporte commented

View reviewed changes

src/NHibernate/Engine/TypedValue.cs

+              		public TypedValue(IType type, object value, bool isList)
+              		{
+              			if (isList && value != null && !(value is IEnumerable))
+              				throw new ArgumentException($"{nameof(value)} must be an {nameof(IEnumerable)} when {nameof(isList)} is true", nameof(value));

Member Author

fredericDelaporte May 28, 2018

In case some mistake is done later, better check value is at least IEnumerable when isList is true. (The list comparator will consider as always equals values which are both not IEnumerable.)

src/NHibernate/Engine/TypedValue.cs

               		{
               			public bool Equals(TypedValue x, TypedValue y)
               			{
-              				if (y == null) return false;
+              				if (ReferenceEquals(x, y))
+              					return true;

Member Author

fredericDelaporte May 28, 2018

Shortcut added, although I do not expect it to be much useful, since its main usage is in query cache key comparison. Unless a not originally cached query instance is re-executed a second time (which is likely a code smell), typed values will be different instances.

src/NHibernate/Engine/TypedValue.cs

+              				if (ReferenceEquals(x, y))
+              					return true;
+              				if (x == null || y == null)
+              					return false;

Member Author

fredericDelaporte May 28, 2018

With its current usage, x is never supposed to be null. But as the comparer class is publicly accessible, better still check this.

hazzik approved these changes

View reviewed changes

fredericDelaporte merged commit 0d61f46 into nhibernate:master

fredericDelaporte deleted the TypedValue branch

May 28, 2018 15:15

fredericDelaporte added the r: Fixed label

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c: Core p: Minor r: Fixed t: Bug