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

fredericDelaporte · 2018-03-13T14:11:13Z

fredericDelaporte · 2018-03-13T14:13:22Z

src/NHibernate.Test/FilterTest/DynamicFilterTest.cs

+						       .SetParameterList("regions", new HashSet<string> { "LA", "APAC" });
+
+						log.Debug("Performing query of Salespersons");
+						var salespersons = session.CreateQuery("from Salesperson").List();


Before the fix, this case does not throw, but convert the hashset to a single string it uses as single argument in the in clause, causing it to fail retrieving expected results.

fredericDelaporte · 2018-03-13T14:14:34Z

src/NHibernate.Test/FilterTest/DynamicFilterTest.cs

+						       .SetParameterList("guids", new HashSet<Guid> { testData.Product1Guid, testData.Product2Guid });
+
+						log.Debug("Performing query of Products");
+						var products = session.CreateQuery("from Product").List();


Before the fix, this case throws an invalid cast exception, which seems dependent on the dataprovider. (I have not reproduced the same one than #1585 report, but I have checked only with PostgreSQL.)

fredericDelaporte · 2018-03-13T14:18:40Z

TypedValue is not involved in the cases I have reproduced, but it likely represents an issue for FilterKey equality. I could add a commit in this PR for it.

bahusoid · 2018-03-13T15:41:01Z

src/NHibernate/Engine/Query/NativeSQLQueryPlan.cs

 						{
-							collectionSpan = collectionValue.Count;
-							bindFragment = string.Join(", ", Enumerable.Repeat(typeBindFragment, collectionValue.Count).ToArray());
+							collectionSpan = listValue.Cast<object>().Count();


Wouldn't it result in full enumeration and unnecessary boxing for all struct elements?
If we still need to deal with IEnumerable interface I think it's better to add internal Count() extension:

internal static int Count(this IEnumerable source) { if (source is ICollection col) return col.Count; int count = 0; var e = source.GetEnumerator(); //in case it's generic disposable enumerator using (e as IDisposable) { while (e.MoveNext()) { count++; } } return count; }

Yes you are right. But I was unwilling to add this to the recently obsoleted EnumerableExtensions utility.

It's obsoleted to be public. I think it's still can survive as internal helper.

Commit amended.

Wouldn't it result in full enumeration and unnecessary boxing for all struct elements?

Not in every case.

Not in every case.

Could you give an example? I'm looking at Cast and Count implementation and don't see any shortcuts.

Could you give an example?

Cast

IEnumerable<T> is co-variant. Cast has a shortcut for it:

public static IEnumerable<TResult> Cast<TResult>(this IEnumerable source) { IEnumerable<TResult> typedSource = source as IEnumerable<TResult>; if (typedSource != null) return typedSource; if (source == null) throw Error.ArgumentNull("source"); return CastIterator<TResult>(source); }

If collection is generic and T is class, cast will chose this path. CastIterator would be involved only if collection is not generic or T is struct.

In our case we first cast ICollection<T> to IEnumerable then do .Cast<T> which will do the upcast to IEnumerable<T> without involving CastIterator.

Count

Count has shortcuts also:

public static int Count<TSource>(this IEnumerable<TSource> source) { if (source == null) throw Error.ArgumentNull("source"); ICollection<TSource> collectionoft = source as ICollection<TSource>; if (collectionoft != null) return collectionoft.Count; ICollection collection = source as ICollection; if (collection != null) return collection.Count; int count = 0; using (IEnumerator<TSource> e = source.GetEnumerator()) { checked { while (e.MoveNext()) count++; } } return count; }

Because we did not perform CastIterator it will just call .Count on our collection.

UPD: Added notice about that T needs to be referenced type.
UPD2: https://dotnetfiddle.net/KHAZBA

It seems I misread your comment. Indeed for structs it will always result in a cast and full enumeration.

fredericDelaporte · 2018-03-13T18:23:26Z

I could add a commit in this PR for it.

Instead done as another PR (#1612) build over this one.

hazzik · 2018-03-13T20:36:38Z

src/NHibernate/Impl/FilterImpl.cs

@@ -16,6 +17,7 @@ public class FilterImpl : IFilter
 		private FilterDefinition definition;

 		private readonly IDictionary<string, object> parameters = new Dictionary<string, object>();
+		private readonly IDictionary<string, IEnumerable> _parameterLists = new Dictionary<string, IEnumerable>();


Why non generic IEnumerable?

Because it is not possible to put anything more specific.

System.Array is possible. But it will require to "materialize" the incoming collection.

hazzik · 2018-03-13T20:44:50Z

src/NHibernate/Util/EnumerableExt.cs

+{
+	internal static class EnumerableExt
+	{
+		internal static int Count(this IEnumerable source)


This method takes precedence over standard Enumerable.Count<T> in some cases.

Fixed (forced push).

fredericDelaporte · 2018-03-26T14:43:18Z

5.1.1 or 5.2? (I think this non-regression bug fix is simple enough for 5.1.1.)

hazzik · 2018-04-11T05:40:12Z

I've made a PR: fredericDelaporte#4

fredericDelaporte · 2018-04-11T16:09:42Z

src/NHibernate/Impl/FilterImpl.cs

-		/// Get a span of collection parameter by name. <see langword="null" /> if the parameter is not a collectino or
-		/// there is no such parameter exist.
+		/// Get the span of a value list parameter by name. <see langword="null" /> if the parameter is not a value list
+		/// or if there is no such parameter.


I have changed "collection" for "value list" for consistency with the xml comment of SetParameterList above.

fredericDelaporte · 2018-04-11T16:13:05Z

I intend to squash the last three commits in one with Alexander as co-author.

hazzik

Approved.

The question remains the same: do we want to this to be in 5.1.x or 5.2?

fredericDelaporte · 2018-04-12T08:47:41Z

Now that we have started merging 5.2 stuff in master, I am keen on putting only regressions in 5.1.x, in order to limit the likelihood of having conflicts when merging it back to master.

hazzik · 2018-04-12T12:43:46Z

Still, let's do 5.1.2.

* Add proper SetUp and Teardown * Add missing using ()

Fixes nhibernate#1585 Co-authored-by: Alexander Zaytsev <[email protected]>

fredericDelaporte · 2018-04-12T13:52:24Z

Squashed and 5.1.x rebased, waiting builds. (Well, maybe not TeamCity which has a two days long queue.)

fredericDelaporte added the t: Fix label Mar 13, 2018

fredericDelaporte commented Mar 13, 2018

View reviewed changes

bahusoid reviewed Mar 13, 2018

View reviewed changes

fredericDelaporte force-pushed the ParameterListHashset branch from 67474d3 to ec86c45 Compare March 13, 2018 15:53

fredericDelaporte mentioned this pull request Mar 13, 2018

Fix TypedValue not always using adequate comparer with SetParameterList #1612

Merged

hazzik reviewed Mar 13, 2018

View reviewed changes

fredericDelaporte force-pushed the ParameterListHashset branch 2 times, most recently from 46ec534 to 6687043 Compare March 14, 2018 01:00

fredericDelaporte commented Apr 11, 2018

View reviewed changes

hazzik approved these changes Apr 11, 2018

View reviewed changes

hazzik changed the base branch from master to 5.1.x April 12, 2018 12:46

hazzik added this to the 5.1.2 milestone Apr 12, 2018

fredericDelaporte and others added 2 commits April 12, 2018 15:47

Clean filters tests

0d1a4b7

* Add proper SetUp and Teardown * Add missing using ()

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

a6c4a5a

Fixes nhibernate#1585 Co-authored-by: Alexander Zaytsev <[email protected]>

fredericDelaporte force-pushed the ParameterListHashset branch from 6be41cf to a6c4a5a Compare April 12, 2018 13:50

hazzik merged commit 56d3f3d into nhibernate:5.1.x Apr 13, 2018

hazzik added the r: Fixed label Apr 13, 2018

fredericDelaporte deleted the ParameterListHashset branch April 13, 2018 10:10

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

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

Uh oh!

Conversation

fredericDelaporte commented Mar 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte Mar 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte commented Mar 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hazzik Mar 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bahusoid Mar 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hazzik Mar 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Cast

Count

Uh oh!

hazzik Mar 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte commented Mar 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte commented Mar 26, 2018

Uh oh!

hazzik commented Apr 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte commented Apr 11, 2018

Uh oh!

hazzik left a comment

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte commented Apr 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hazzik commented Apr 12, 2018

Uh oh!

fredericDelaporte commented Apr 12, 2018

Uh oh!

Uh oh!

fredericDelaporte Mar 13, 2018 •

edited

Loading

hazzik Mar 13, 2018 •

edited

Loading

bahusoid Mar 15, 2018 •

edited

Loading

hazzik Mar 16, 2018 •

edited

Loading

hazzik Mar 16, 2018 •

edited

Loading

fredericDelaporte commented Mar 13, 2018 •

edited

Loading

hazzik commented Apr 11, 2018 •

edited

Loading

fredericDelaporte commented Apr 12, 2018 •

edited

Loading