From f278278b4f32fe86d924fc3b48165de917f73e81 Mon Sep 17 00:00:00 2001 From: Roman Artiukhin Date: Thu, 16 May 2019 15:25:35 +0300 Subject: [PATCH 1/7] Proper support for IN clause for composite properties in Criteria --- .../ClassWithCompositeIdFixture.cs | 28 +++++-- .../ClassWithCompositeIdFixture.cs | 28 +++++-- src/NHibernate/Criterion/InExpression.cs | 84 ++++++++++--------- src/NHibernate/SqlCommand/SqlString.cs | 13 +++ src/NHibernate/SqlCommand/SqlStringHelper.cs | 47 ++++++++++- 5 files changed, 148 insertions(+), 52 deletions(-) diff --git a/src/NHibernate.Test/Async/CompositeId/ClassWithCompositeIdFixture.cs b/src/NHibernate.Test/Async/CompositeId/ClassWithCompositeIdFixture.cs index e3ae7de3018..666efc3bfd8 100644 --- a/src/NHibernate.Test/Async/CompositeId/ClassWithCompositeIdFixture.cs +++ b/src/NHibernate.Test/Async/CompositeId/ClassWithCompositeIdFixture.cs @@ -38,11 +38,6 @@ protected override string[] Mappings get { return new string[] {"CompositeId.ClassWithCompositeId.hbm.xml"}; } } - protected override bool AppliesTo(Dialect.Dialect dialect) - { - return !(dialect is Dialect.FirebirdDialect); // Firebird has no CommandTimeout, and locks up during the tear-down of this fixture - } - protected override void OnSetUp() { id = new Id("stringKey", 3, firstDateTime); @@ -52,9 +47,11 @@ protected override void OnSetUp() protected override void OnTearDown() { using (ISession s = Sfi.OpenSession()) + using (var t = s.BeginTransaction()) { s.Delete("from ClassWithCompositeId"); s.Flush(); + t.Commit(); } } @@ -397,5 +394,26 @@ public async Task QueryOverOrderByAndWhereWithIdProjectionDoesntThrowAsync() Assert.That(results.Count, Is.EqualTo(1)); } } + + [Test] + public async Task QueryOverInClauseAsync() + { + // insert the new objects + using (ISession s = OpenSession()) + using (ITransaction t = s.BeginTransaction()) + { + await (s.SaveAsync(new ClassWithCompositeId(id) {OneProperty = 5})); + await (s.SaveAsync(new ClassWithCompositeId(secondId) {OneProperty = 10})); + await (s.SaveAsync(new ClassWithCompositeId(new Id(id.KeyString, id.GetKeyShort(), secondId.KeyDateTime)))); + + await (t.CommitAsync()); + } + + using (var s = OpenSession()) + { + var results = await (s.QueryOver().WhereRestrictionOn(p => p.Id).IsIn(new[] {id, secondId}).ListAsync()); + Assert.That(results.Count, Is.EqualTo(2)); + } + } } } diff --git a/src/NHibernate.Test/CompositeId/ClassWithCompositeIdFixture.cs b/src/NHibernate.Test/CompositeId/ClassWithCompositeIdFixture.cs index 5cf758b76ef..64640a65f1f 100644 --- a/src/NHibernate.Test/CompositeId/ClassWithCompositeIdFixture.cs +++ b/src/NHibernate.Test/CompositeId/ClassWithCompositeIdFixture.cs @@ -27,11 +27,6 @@ protected override string[] Mappings get { return new string[] {"CompositeId.ClassWithCompositeId.hbm.xml"}; } } - protected override bool AppliesTo(Dialect.Dialect dialect) - { - return !(dialect is Dialect.FirebirdDialect); // Firebird has no CommandTimeout, and locks up during the tear-down of this fixture - } - protected override void OnSetUp() { id = new Id("stringKey", 3, firstDateTime); @@ -41,9 +36,11 @@ protected override void OnSetUp() protected override void OnTearDown() { using (ISession s = Sfi.OpenSession()) + using (var t = s.BeginTransaction()) { s.Delete("from ClassWithCompositeId"); s.Flush(); + t.Commit(); } } @@ -386,5 +383,26 @@ public void QueryOverOrderByAndWhereWithIdProjectionDoesntThrow() Assert.That(results.Count, Is.EqualTo(1)); } } + + [Test] + public void QueryOverInClause() + { + // insert the new objects + using (ISession s = OpenSession()) + using (ITransaction t = s.BeginTransaction()) + { + s.Save(new ClassWithCompositeId(id) {OneProperty = 5}); + s.Save(new ClassWithCompositeId(secondId) {OneProperty = 10}); + s.Save(new ClassWithCompositeId(new Id(id.KeyString, id.GetKeyShort(), secondId.KeyDateTime))); + + t.Commit(); + } + + using (var s = OpenSession()) + { + var results = s.QueryOver().WhereRestrictionOn(p => p.Id).IsIn(new[] {id, secondId}).List(); + Assert.That(results.Count, Is.EqualTo(2)); + } + } } } diff --git a/src/NHibernate/Criterion/InExpression.cs b/src/NHibernate/Criterion/InExpression.cs index d2edd83eba2..0c8122b54ba 100644 --- a/src/NHibernate/Criterion/InExpression.cs +++ b/src/NHibernate/Criterion/InExpression.cs @@ -1,5 +1,4 @@ using System; -using System.Collections; using System.Collections.Generic; using System.Linq; using NHibernate.Engine; @@ -13,9 +12,6 @@ namespace NHibernate.Criterion /// An that constrains the property /// to a specified list of values. /// - /// - /// InExpression - should only be used with a Single Value column - no multicolumn properties... - /// [Serializable] public class InExpression : AbstractCriterion { @@ -62,41 +58,50 @@ public override SqlString ToSqlString(ICriteria criteria, ICriteriaQuery criteri return new SqlString("1=0"); } - //TODO: add default capacity - SqlStringBuilder result = new SqlStringBuilder(); - SqlString[] columnNames = - CriterionUtil.GetColumnNames(_propertyName, _projection, criteriaQuery, criteria); - - // Generate SqlString of the form: - // columnName1 in (values) and columnName2 in (values) and ... - Parameter[] parameters = GetParameterTypedValues(criteria, criteriaQuery).SelectMany(t => criteriaQuery.NewQueryParameter(t)).ToArray(); + SqlString[] columns = CriterionUtil.GetColumnNames(_propertyName, _projection, criteriaQuery, criteria); - for (int columnIndex = 0; columnIndex < columnNames.Length; columnIndex++) + var list = new List(columns.Length * Values.Length); + foreach (var typedValue in GetParameterTypedValues(criteria, criteriaQuery)) { - SqlString columnName = columnNames[columnIndex]; - - if (columnIndex > 0) - { - result.Add(" and "); - } + //Must be executed after CriterionUtil.GetColumnNames (as it might add _projection parameters to criteria) + list.AddRange(criteriaQuery.NewQueryParameter(typedValue)); + } - result - .Add(columnName) - .Add(" in ("); + var bogusParam = Parameter.Placeholder; - for (int i = 0; i < _values.Length; i++) - { - if (i > 0) - { - result.Add(StringHelper.CommaSpace); - } - result.Add(parameters[i]); - } + var sqlString = GetSqlString(criteriaQuery, columns, bogusParam); + sqlString.SubstituteBogusParameters(list, bogusParam); + return sqlString; + } - result.Add(")"); + private SqlString GetSqlString(ICriteriaQuery criteriaQuery, SqlString[] columns, Parameter bogusParam) + { + if (columns.Length <= 1 || criteriaQuery.Factory.Dialect.SupportsRowValueConstructorSyntaxInInList) + { + var parens = columns.Length > 1 ? new[] {new SqlString("("), new SqlString(")"),} : null; + SqlString comaSeparator = new SqlString(", "); + var singleValueParam = SqlStringHelper.Repeat(new SqlString(bogusParam), columns.Length, comaSeparator, parens); + + var parameters = SqlStringHelper.Repeat(singleValueParam, Values.Length, comaSeparator, null); + + //single column: col1 in (?, ?) + //multi column: (col1, col2) in ((?, ?), (?, ?)) + return new SqlString( + parens?[0] ?? SqlString.Empty, + SqlStringHelper.Join(comaSeparator, columns), + parens?[1] ?? SqlString.Empty, + " in (", + parameters, + ")"); } - return result.ToSqlString(); + //((col1 = ? and col2 = ?) or (col1 = ? and col2 = ?)) + var cols = new SqlString( + " ( ", + SqlStringHelper.Join(new SqlString(" = ", bogusParam, " and "), columns), + new SqlString("= ", bogusParam, " ) ")); + cols = SqlStringHelper.Repeat(cols, Values.Length, "or ", new[] {" ( ", " ) "}); + return cols; } private void AssertPropertyIsNotCollection(ICriteriaQuery criteriaQuery, ICriteria criteria) @@ -127,16 +132,13 @@ private List GetParameterTypedValues(ICriteria criteria, ICriteriaQu List list = new List(); IAbstractComponentType actype = (IAbstractComponentType) type; IType[] types = actype.Subtypes; - - for (int i = 0; i < types.Length; i++) + for (int vi = 0; vi < _values.Length; vi++) + for (int ti = 0; ti < types.Length; ti++) { - for (int j = 0; j < _values.Length; j++) - { - object subval = _values[j] == null - ? null - : actype.GetPropertyValues(_values[j])[i]; - list.Add(new TypedValue(types[i], subval, false)); - } + object subval = _values[vi] == null + ? null + : actype.GetPropertyValues(_values[vi])[ti]; + list.Add(new TypedValue(types[ti], subval, false)); } return list; diff --git a/src/NHibernate/SqlCommand/SqlString.cs b/src/NHibernate/SqlCommand/SqlString.cs index dce3f630db9..7b0ae8f13bd 100644 --- a/src/NHibernate/SqlCommand/SqlString.cs +++ b/src/NHibernate/SqlCommand/SqlString.cs @@ -1024,6 +1024,19 @@ public SqlString GetSubselectString() return new SubselectClauseExtractor(this).GetSqlString(); } + internal void SubstituteBogusParameters(IReadOnlyList actualParams, Parameter bogusParam) + { + int index = 0; + var keys = _parameters.Keys; + // ReSharper disable once ForCanBeConvertedToForeach + for (var i = 0; i < keys.Count; i++) + { + var key = keys[i]; + if (ReferenceEquals(_parameters[key], bogusParam)) + _parameters[key] = actualParams[index++]; + } + } + [Serializable] private struct Part : IEquatable { diff --git a/src/NHibernate/SqlCommand/SqlStringHelper.cs b/src/NHibernate/SqlCommand/SqlStringHelper.cs index ec111f6b1d0..dc45f4880f6 100644 --- a/src/NHibernate/SqlCommand/SqlStringHelper.cs +++ b/src/NHibernate/SqlCommand/SqlStringHelper.cs @@ -56,7 +56,6 @@ public static bool IsNotEmpty(SqlString str) return !IsEmpty(str); } - public static bool IsEmpty(SqlString str) { return str == null || str.Count == 0; @@ -90,5 +89,51 @@ internal static SqlString ParametersList(List parameters) return builder.ToSqlString(); } + + internal static SqlString Repeat(SqlString placeholder, int count, string separator, string[] wrapResult) + { + return Repeat( + placeholder, + count, + new SqlString(separator), + wrapResult == null + ? null + : new[] + { + new SqlString(wrapResult[0]), + new SqlString(wrapResult[1]), + }); + } + + internal static SqlString Repeat(SqlString placeholder, int count, SqlString separator, SqlString[] wrapResult) + { + if (wrapResult == null) + { + if (count == 0) + return SqlString.Empty; + if (count == 1) + return placeholder; + } + + var builder = new SqlStringBuilder(count * 2 + 1); + if (wrapResult != null) + { + builder.Add(wrapResult[0]); + } + + if (count > 0) + builder.Add(placeholder); + + for (int i = 1; i < count; i++) + { + builder.Add(separator).Add(placeholder); + } + + if (wrapResult != null) + { + builder.Add(wrapResult[1]); + } + return builder.ToSqlString(); + } } } From bfb96a238f8b9f47c8fd46bdeb85f21e817258b8 Mon Sep 17 00:00:00 2001 From: Alexander Zaytsev Date: Wed, 11 Sep 2019 11:07:33 +0300 Subject: [PATCH 2/7] Optimize GetParameterTypedValues for components --- src/NHibernate/Criterion/InExpression.cs | 28 +++++++++++------------- 1 file changed, 13 insertions(+), 15 deletions(-) diff --git a/src/NHibernate/Criterion/InExpression.cs b/src/NHibernate/Criterion/InExpression.cs index 0c8122b54ba..58817b2bc54 100644 --- a/src/NHibernate/Criterion/InExpression.cs +++ b/src/NHibernate/Criterion/InExpression.cs @@ -127,26 +127,24 @@ private List GetParameterTypedValues(ICriteria criteria, ICriteriaQu { IType type = GetElementType(criteria, criteriaQuery); - if (type.IsComponentType) + if (!type.IsComponentType) { - List list = new List(); - IAbstractComponentType actype = (IAbstractComponentType) type; - IType[] types = actype.Subtypes; - for (int vi = 0; vi < _values.Length; vi++) + return _values.Select(v => new TypedValue(type, v, false)).ToList(); + } + + List list = new List(); + IAbstractComponentType actype = (IAbstractComponentType) type; + var types = actype.Subtypes; + foreach (var value in _values) + { + var propertyValues = value != null ? actype.GetPropertyValues(value) : null; for (int ti = 0; ti < types.Length; ti++) { - object subval = _values[vi] == null - ? null - : actype.GetPropertyValues(_values[vi])[ti]; - list.Add(new TypedValue(types[ti], subval, false)); + list.Add(new TypedValue(types[ti], propertyValues?[ti], false)); } - - return list; - } - else - { - return _values.ToList(v => new TypedValue(type, v, false)); } + + return list; } /// From 852bbcaeca16bb9b1643a1266bef76c65ba46628 Mon Sep 17 00:00:00 2001 From: Roman Artiukhin Date: Wed, 11 Sep 2019 13:38:22 +0300 Subject: [PATCH 3/7] Simplify parens handling --- .../ClassWithCompositeIdFixture.cs | 6 ++- .../ClassWithCompositeIdFixture.cs | 6 ++- src/NHibernate/Criterion/InExpression.cs | 12 ++--- src/NHibernate/SqlCommand/SqlStringHelper.cs | 45 +++++++------------ 4 files changed, 30 insertions(+), 39 deletions(-) diff --git a/src/NHibernate.Test/Async/CompositeId/ClassWithCompositeIdFixture.cs b/src/NHibernate.Test/Async/CompositeId/ClassWithCompositeIdFixture.cs index 666efc3bfd8..37f83f76c73 100644 --- a/src/NHibernate.Test/Async/CompositeId/ClassWithCompositeIdFixture.cs +++ b/src/NHibernate.Test/Async/CompositeId/ClassWithCompositeIdFixture.cs @@ -411,8 +411,10 @@ public async Task QueryOverInClauseAsync() using (var s = OpenSession()) { - var results = await (s.QueryOver().WhereRestrictionOn(p => p.Id).IsIn(new[] {id, secondId}).ListAsync()); - Assert.That(results.Count, Is.EqualTo(2)); + var results1 = await (s.QueryOver().WhereRestrictionOn(p => p.Id).IsIn(new[] {id, secondId}).ListAsync()); + var results2 = await (s.QueryOver().WhereRestrictionOn(p => p.Id).IsIn(new[] {id}).ListAsync()); + Assert.That(results1.Count, Is.EqualTo(2)); + Assert.That(results2.Count, Is.EqualTo(1)); } } } diff --git a/src/NHibernate.Test/CompositeId/ClassWithCompositeIdFixture.cs b/src/NHibernate.Test/CompositeId/ClassWithCompositeIdFixture.cs index 64640a65f1f..276a04268b0 100644 --- a/src/NHibernate.Test/CompositeId/ClassWithCompositeIdFixture.cs +++ b/src/NHibernate.Test/CompositeId/ClassWithCompositeIdFixture.cs @@ -400,8 +400,10 @@ public void QueryOverInClause() using (var s = OpenSession()) { - var results = s.QueryOver().WhereRestrictionOn(p => p.Id).IsIn(new[] {id, secondId}).List(); - Assert.That(results.Count, Is.EqualTo(2)); + var results1 = s.QueryOver().WhereRestrictionOn(p => p.Id).IsIn(new[] {id, secondId}).List(); + var results2 = s.QueryOver().WhereRestrictionOn(p => p.Id).IsIn(new[] {id}).List(); + Assert.That(results1.Count, Is.EqualTo(2)); + Assert.That(results2.Count, Is.EqualTo(1)); } } } diff --git a/src/NHibernate/Criterion/InExpression.cs b/src/NHibernate/Criterion/InExpression.cs index 58817b2bc54..e52efd7f188 100644 --- a/src/NHibernate/Criterion/InExpression.cs +++ b/src/NHibernate/Criterion/InExpression.cs @@ -78,18 +78,18 @@ private SqlString GetSqlString(ICriteriaQuery criteriaQuery, SqlString[] columns { if (columns.Length <= 1 || criteriaQuery.Factory.Dialect.SupportsRowValueConstructorSyntaxInInList) { - var parens = columns.Length > 1 ? new[] {new SqlString("("), new SqlString(")"),} : null; + var wrapInParens = columns.Length > 1; SqlString comaSeparator = new SqlString(", "); - var singleValueParam = SqlStringHelper.Repeat(new SqlString(bogusParam), columns.Length, comaSeparator, parens); + var singleValueParam = SqlStringHelper.Repeat(new SqlString(bogusParam), columns.Length, comaSeparator, wrapInParens); - var parameters = SqlStringHelper.Repeat(singleValueParam, Values.Length, comaSeparator, null); + var parameters = SqlStringHelper.Repeat(singleValueParam, Values.Length, comaSeparator, wrapInParens: false); //single column: col1 in (?, ?) //multi column: (col1, col2) in ((?, ?), (?, ?)) return new SqlString( - parens?[0] ?? SqlString.Empty, + wrapInParens ? StringHelper.OpenParen : string.Empty, SqlStringHelper.Join(comaSeparator, columns), - parens?[1] ?? SqlString.Empty, + wrapInParens ? StringHelper.ClosedParen : string.Empty, " in (", parameters, ")"); @@ -100,7 +100,7 @@ private SqlString GetSqlString(ICriteriaQuery criteriaQuery, SqlString[] columns " ( ", SqlStringHelper.Join(new SqlString(" = ", bogusParam, " and "), columns), new SqlString("= ", bogusParam, " ) ")); - cols = SqlStringHelper.Repeat(cols, Values.Length, "or ", new[] {" ( ", " ) "}); + cols = SqlStringHelper.Repeat(cols, Values.Length, new SqlString(" or "), wrapInParens: Values.Length > 1); return cols; } diff --git a/src/NHibernate/SqlCommand/SqlStringHelper.cs b/src/NHibernate/SqlCommand/SqlStringHelper.cs index dc45f4880f6..845a7ac0257 100644 --- a/src/NHibernate/SqlCommand/SqlStringHelper.cs +++ b/src/NHibernate/SqlCommand/SqlStringHelper.cs @@ -56,6 +56,7 @@ public static bool IsNotEmpty(SqlString str) return !IsEmpty(str); } + public static bool IsEmpty(SqlString str) { return str == null || str.Count == 0; @@ -90,49 +91,35 @@ internal static SqlString ParametersList(List parameters) return builder.ToSqlString(); } - internal static SqlString Repeat(SqlString placeholder, int count, string separator, string[] wrapResult) + internal static SqlString Repeat(SqlString placeholder, int count, SqlString separator, bool wrapInParens) { - return Repeat( - placeholder, - count, - new SqlString(separator), - wrapResult == null - ? null - : new[] - { - new SqlString(wrapResult[0]), - new SqlString(wrapResult[1]), - }); - } + if (count == 0) + return SqlString.Empty; - internal static SqlString Repeat(SqlString placeholder, int count, SqlString separator, SqlString[] wrapResult) - { - if (wrapResult == null) - { - if (count == 0) - return SqlString.Empty; - if (count == 1) - return placeholder; - } + if (count == 1) + return wrapInParens + ? new SqlString("(", placeholder, ")") + : placeholder; - var builder = new SqlStringBuilder(count * 2 + 1); - if (wrapResult != null) + var builder = new SqlStringBuilder((placeholder.Count + separator.Count) * count + 1); + + if (wrapInParens) { - builder.Add(wrapResult[0]); + builder.Add("("); } - if (count > 0) - builder.Add(placeholder); + builder.Add(placeholder); for (int i = 1; i < count; i++) { builder.Add(separator).Add(placeholder); } - if (wrapResult != null) + if (wrapInParens) { - builder.Add(wrapResult[1]); + builder.Add(")"); } + return builder.ToSqlString(); } } From 6366ac6184512619b9247c6e9f49be5439aa6ed9 Mon Sep 17 00:00:00 2001 From: Roman Artiukhin Date: Fri, 13 Sep 2019 08:44:49 +0300 Subject: [PATCH 4/7] Optimize ToList conversion --- src/NHibernate/Criterion/InExpression.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/NHibernate/Criterion/InExpression.cs b/src/NHibernate/Criterion/InExpression.cs index e52efd7f188..a45508737f2 100644 --- a/src/NHibernate/Criterion/InExpression.cs +++ b/src/NHibernate/Criterion/InExpression.cs @@ -129,7 +129,7 @@ private List GetParameterTypedValues(ICriteria criteria, ICriteriaQu if (!type.IsComponentType) { - return _values.Select(v => new TypedValue(type, v, false)).ToList(); + return _values.ToList(v => new TypedValue(type, v, false)); } List list = new List(); From b7912cf516620f944f25813cbef9def6b7f25e7e Mon Sep 17 00:00:00 2001 From: Roman Artiukhin Date: Fri, 13 Sep 2019 10:01:38 +0300 Subject: [PATCH 5/7] string separator --- src/NHibernate/Criterion/InExpression.cs | 8 ++++--- src/NHibernate/SqlCommand/SqlStringHelper.cs | 22 ++++++++++++++++++-- 2 files changed, 25 insertions(+), 5 deletions(-) diff --git a/src/NHibernate/Criterion/InExpression.cs b/src/NHibernate/Criterion/InExpression.cs index a45508737f2..54409ba9f31 100644 --- a/src/NHibernate/Criterion/InExpression.cs +++ b/src/NHibernate/Criterion/InExpression.cs @@ -79,7 +79,7 @@ private SqlString GetSqlString(ICriteriaQuery criteriaQuery, SqlString[] columns if (columns.Length <= 1 || criteriaQuery.Factory.Dialect.SupportsRowValueConstructorSyntaxInInList) { var wrapInParens = columns.Length > 1; - SqlString comaSeparator = new SqlString(", "); + const string comaSeparator = ", "; var singleValueParam = SqlStringHelper.Repeat(new SqlString(bogusParam), columns.Length, comaSeparator, wrapInParens); var parameters = SqlStringHelper.Repeat(singleValueParam, Values.Length, comaSeparator, wrapInParens: false); @@ -99,8 +99,10 @@ private SqlString GetSqlString(ICriteriaQuery criteriaQuery, SqlString[] columns var cols = new SqlString( " ( ", SqlStringHelper.Join(new SqlString(" = ", bogusParam, " and "), columns), - new SqlString("= ", bogusParam, " ) ")); - cols = SqlStringHelper.Repeat(cols, Values.Length, new SqlString(" or "), wrapInParens: Values.Length > 1); + "= ", + bogusParam, + " ) "); + cols = SqlStringHelper.Repeat(cols, Values.Length, " or ", wrapInParens: Values.Length > 1); return cols; } diff --git a/src/NHibernate/SqlCommand/SqlStringHelper.cs b/src/NHibernate/SqlCommand/SqlStringHelper.cs index 845a7ac0257..ec33cc8e1f2 100644 --- a/src/NHibernate/SqlCommand/SqlStringHelper.cs +++ b/src/NHibernate/SqlCommand/SqlStringHelper.cs @@ -31,6 +31,24 @@ public static SqlString Join(SqlString separator, IEnumerable objects) return buf.ToSqlString(); } + internal static SqlString Join(string separator, IList strings) + { + if (strings.Count == 0) + return SqlString.Empty; + + if (strings.Count == 1) + return strings[0]; + + var buf = new SqlStringBuilder(); + + buf.Add(strings[0]); + for (var index = 1; index < strings.Count; index++) + { + buf.Add(separator).Add(strings[index]); + } + + return buf.ToSqlString(); + } public static SqlString[] Add(SqlString[] x, string sep, SqlString[] y) { @@ -91,7 +109,7 @@ internal static SqlString ParametersList(List parameters) return builder.ToSqlString(); } - internal static SqlString Repeat(SqlString placeholder, int count, SqlString separator, bool wrapInParens) + internal static SqlString Repeat(SqlString placeholder, int count, string separator, bool wrapInParens) { if (count == 0) return SqlString.Empty; @@ -101,7 +119,7 @@ internal static SqlString Repeat(SqlString placeholder, int count, SqlString sep ? new SqlString("(", placeholder, ")") : placeholder; - var builder = new SqlStringBuilder((placeholder.Count + separator.Count) * count + 1); + var builder = new SqlStringBuilder((placeholder.Count + 1) * count + 1); if (wrapInParens) { From 598b74d5420fae1933c6269b8b60370a04709238 Mon Sep 17 00:00:00 2001 From: Roman Artiukhin Date: Fri, 13 Sep 2019 12:47:20 +0300 Subject: [PATCH 6/7] Improve test --- .../ClassWithCompositeIdFixture.cs | 31 ++++++++++++++----- .../ClassWithCompositeIdFixture.cs | 31 ++++++++++++++----- 2 files changed, 48 insertions(+), 14 deletions(-) diff --git a/src/NHibernate.Test/Async/CompositeId/ClassWithCompositeIdFixture.cs b/src/NHibernate.Test/Async/CompositeId/ClassWithCompositeIdFixture.cs index 37f83f76c73..3b673125541 100644 --- a/src/NHibernate.Test/Async/CompositeId/ClassWithCompositeIdFixture.cs +++ b/src/NHibernate.Test/Async/CompositeId/ClassWithCompositeIdFixture.cs @@ -399,22 +399,39 @@ public async Task QueryOverOrderByAndWhereWithIdProjectionDoesntThrowAsync() public async Task QueryOverInClauseAsync() { // insert the new objects + var id1 = id; + var id2 = secondId; + var id3 = new Id(id1.KeyString, id1.GetKeyShort(), id2.KeyDateTime); + using (ISession s = OpenSession()) using (ITransaction t = s.BeginTransaction()) { - await (s.SaveAsync(new ClassWithCompositeId(id) {OneProperty = 5})); - await (s.SaveAsync(new ClassWithCompositeId(secondId) {OneProperty = 10})); - await (s.SaveAsync(new ClassWithCompositeId(new Id(id.KeyString, id.GetKeyShort(), secondId.KeyDateTime)))); + await (s.SaveAsync(new ClassWithCompositeId(id1) {OneProperty = 5})); + await (s.SaveAsync(new ClassWithCompositeId(id2) {OneProperty = 10})); + await (s.SaveAsync(new ClassWithCompositeId(id3))); await (t.CommitAsync()); } using (var s = OpenSession()) { - var results1 = await (s.QueryOver().WhereRestrictionOn(p => p.Id).IsIn(new[] {id, secondId}).ListAsync()); - var results2 = await (s.QueryOver().WhereRestrictionOn(p => p.Id).IsIn(new[] {id}).ListAsync()); - Assert.That(results1.Count, Is.EqualTo(2)); - Assert.That(results2.Count, Is.EqualTo(1)); + var results1 = await (s.QueryOver().WhereRestrictionOn(p => p.Id).IsIn(new[] {id1, id2}).ListAsync()); + var results2 = await (s.QueryOver().WhereRestrictionOn(p => p.Id).IsIn(new[] {id1}).ListAsync()); + var results3 = await (s.QueryOver().WhereRestrictionOn(p => p.Id).Not.IsIn(new[] {id1, id2}).ListAsync()); + var results4 = await (s.QueryOver().WhereRestrictionOn(p => p.Id).Not.IsIn(new[] {id1}).ListAsync()); + + Assert.Multiple( + () => + { + Assert.That(results1.Count, Is.EqualTo(2), "in multiple ids"); + Assert.That(results1.Select(r => r.Id), Is.EquivalentTo(new[] {id1, id2}), "in multiple ids"); + Assert.That(results2.Count, Is.EqualTo(1), "in single id"); + Assert.That(results2.Select(r => r.Id), Is.EquivalentTo(new[] {id1}), "in single id"); + Assert.That(results3.Count, Is.EqualTo(1), "not in multiple ids"); + Assert.That(results3.Select(r => r.Id), Is.EquivalentTo(new[] {id3}), "not in multiple ids"); + Assert.That(results4.Count, Is.EqualTo(2), "not in single id"); + Assert.That(results4.Select(r => r.Id), Is.EquivalentTo(new[] {id2, id3}), "not in single id"); + }); } } } diff --git a/src/NHibernate.Test/CompositeId/ClassWithCompositeIdFixture.cs b/src/NHibernate.Test/CompositeId/ClassWithCompositeIdFixture.cs index 276a04268b0..00ea5c3ca7a 100644 --- a/src/NHibernate.Test/CompositeId/ClassWithCompositeIdFixture.cs +++ b/src/NHibernate.Test/CompositeId/ClassWithCompositeIdFixture.cs @@ -388,22 +388,39 @@ public void QueryOverOrderByAndWhereWithIdProjectionDoesntThrow() public void QueryOverInClause() { // insert the new objects + var id1 = id; + var id2 = secondId; + var id3 = new Id(id1.KeyString, id1.GetKeyShort(), id2.KeyDateTime); + using (ISession s = OpenSession()) using (ITransaction t = s.BeginTransaction()) { - s.Save(new ClassWithCompositeId(id) {OneProperty = 5}); - s.Save(new ClassWithCompositeId(secondId) {OneProperty = 10}); - s.Save(new ClassWithCompositeId(new Id(id.KeyString, id.GetKeyShort(), secondId.KeyDateTime))); + s.Save(new ClassWithCompositeId(id1) {OneProperty = 5}); + s.Save(new ClassWithCompositeId(id2) {OneProperty = 10}); + s.Save(new ClassWithCompositeId(id3)); t.Commit(); } using (var s = OpenSession()) { - var results1 = s.QueryOver().WhereRestrictionOn(p => p.Id).IsIn(new[] {id, secondId}).List(); - var results2 = s.QueryOver().WhereRestrictionOn(p => p.Id).IsIn(new[] {id}).List(); - Assert.That(results1.Count, Is.EqualTo(2)); - Assert.That(results2.Count, Is.EqualTo(1)); + var results1 = s.QueryOver().WhereRestrictionOn(p => p.Id).IsIn(new[] {id1, id2}).List(); + var results2 = s.QueryOver().WhereRestrictionOn(p => p.Id).IsIn(new[] {id1}).List(); + var results3 = s.QueryOver().WhereRestrictionOn(p => p.Id).Not.IsIn(new[] {id1, id2}).List(); + var results4 = s.QueryOver().WhereRestrictionOn(p => p.Id).Not.IsIn(new[] {id1}).List(); + + Assert.Multiple( + () => + { + Assert.That(results1.Count, Is.EqualTo(2), "in multiple ids"); + Assert.That(results1.Select(r => r.Id), Is.EquivalentTo(new[] {id1, id2}), "in multiple ids"); + Assert.That(results2.Count, Is.EqualTo(1), "in single id"); + Assert.That(results2.Select(r => r.Id), Is.EquivalentTo(new[] {id1}), "in single id"); + Assert.That(results3.Count, Is.EqualTo(1), "not in multiple ids"); + Assert.That(results3.Select(r => r.Id), Is.EquivalentTo(new[] {id3}), "not in multiple ids"); + Assert.That(results4.Count, Is.EqualTo(2), "not in single id"); + Assert.That(results4.Select(r => r.Id), Is.EquivalentTo(new[] {id2, id3}), "not in single id"); + }); } } } From 0ea57f7b19b1171c44390e3ff655c9aea4a9ce20 Mon Sep 17 00:00:00 2001 From: Roman Artiukhin Date: Sun, 19 Apr 2020 22:34:41 +0300 Subject: [PATCH 7/7] Code review changes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Frédéric Delaporte <12201973+fredericDelaporte@users.noreply.github.com> --- src/NHibernate/SqlCommand/SqlString.cs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/NHibernate/SqlCommand/SqlString.cs b/src/NHibernate/SqlCommand/SqlString.cs index a4aec30cb20..33caf5e0096 100644 --- a/src/NHibernate/SqlCommand/SqlString.cs +++ b/src/NHibernate/SqlCommand/SqlString.cs @@ -1028,6 +1028,9 @@ internal void SubstituteBogusParameters(IReadOnlyList actualParams, P { int index = 0; var keys = _parameters.Keys; + // The loop below is technically not altering the keys collection on which we iterate, but + // the underlying implementation still throws on foreach iterations over keys even if we + // have only changed the associated value. // ReSharper disable once ForCanBeConvertedToForeach for (var i = 0; i < keys.Count; i++) {