Skip to content

Commit ee42371

Browse files
fredericDelaportefredericDelaporte
committed
NH-3956 - native SQL query plan cache fix: avoiding hashcode collision.
1 parent a5723bc commit ee42371

File tree

4 files changed

+241
-2
lines changed

4 files changed

+241
-2
lines changed

‎src/NHibernate.Test/NHSpecificTest/NH3956/Fixture.cs

Lines changed: 221 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,221 @@
1+
using System.Reflection;
2+
using NHibernate.Engine.Query.Sql;
3+
using NUnit.Framework;
4+
5+
namespace NHibernate.Test.NHSpecificTest.NH3956
6+
{
7+
[TestFixture]
8+
public class Fixture
9+
{
10+
private static readonly FieldInfo HashCodeField =
11+
typeof(NativeSQLQuerySpecification).GetField("hashCode", BindingFlags.Instance | BindingFlags.NonPublic);
12+
13+
/// <summary>
14+
/// Allows to simulate a hashcode collision. Issue would be unpractical to test otherwise.
15+
/// Hashcode collision must be supported for avoiding unexpected and hard to reproduce failures.
16+
/// </summary>
17+
private void TweakHashcode(NativeSQLQuerySpecification specToTweak, int hashCode)
18+
{
19+
// Though hashCode is a readonly field, this works at the time of this writing. If it starts breaking and cannot be fixed,
20+
// ignore those tests or throw them away.
21+
HashCodeField.SetValue(specToTweak, hashCode);
22+
}
23+
24+
[Test]
25+
public void NativeSQLQuerySpecificationEqualityOnQuery()
26+
{
27+
var spec1 = new NativeSQLQuerySpecification("select blah", null, null);
28+
// Empty spaces array should be equivalent to null. Maybe results too but current implementation does not handle this.
29+
var spec2 = new NativeSQLQuerySpecification("select blah", null, new string[0]);
30+
31+
Assert.IsTrue(spec1.Equals(spec2));
32+
Assert.IsTrue(spec2.Equals(spec1));
33+
}
34+
35+
[Test]
36+
public void NativeSQLQuerySpecificationInequalityOnQuery()
37+
{
38+
var spec1 = new NativeSQLQuerySpecification("select blah", null, null);
39+
var spec2 = new NativeSQLQuerySpecification("select blargh", null, null);
40+
41+
TweakHashcode(spec2, spec1.GetHashCode());
42+
Assert.IsFalse(spec1.Equals(spec2));
43+
Assert.IsFalse(spec2.Equals(spec1));
44+
}
45+
46+
[Test]
47+
public void NativeSQLQuerySpecificationEqualityOnReturns()
48+
{
49+
var spec1 = new NativeSQLQuerySpecification("select blah",
50+
new[]
51+
{
52+
new NativeSQLQueryScalarReturn("alias1", NHibernateUtil.Character),
53+
new NativeSQLQueryScalarReturn("alias2", NHibernateUtil.Decimal)
54+
},
55+
null);
56+
var spec2 = new NativeSQLQuerySpecification("select blah",
57+
new[]
58+
{
59+
// Aliases ordering matters, do not get them mixed. (But type does not matter, I guess this means
60+
// a case with a same query having sames aliases but different types is never supposed to happen
61+
// Note that Hibernate does test other properties as of this writing:
62+
// https://github.com/hibernate/hibernate-orm/blob/master/hibernate-core/src/main/java/org/hibernate/engine/query/spi/sql/NativeSQLQueryScalarReturn.java
63+
// And same on other INativeSQLQueryReturn implementations.
64+
new NativeSQLQueryScalarReturn("alias1", NHibernateUtil.Character),
65+
new NativeSQLQueryScalarReturn("alias2", NHibernateUtil.Decimal)
66+
},
67+
// Empty spaces array should be equivalent to null.
68+
new string[0]);
69+
70+
Assert.IsTrue(spec1.Equals(spec2));
71+
Assert.IsTrue(spec2.Equals(spec1));
72+
}
73+
74+
[Test]
75+
public void NativeSQLQuerySpecificationInequalityOnNullReturn()
76+
{
77+
var spec1 = new NativeSQLQuerySpecification("select blah", null, null);
78+
var spec2 = new NativeSQLQuerySpecification("select blah",
79+
new[]
80+
{
81+
new NativeSQLQueryScalarReturn("alias1", NHibernateUtil.Character)
82+
},
83+
null);
84+
85+
TweakHashcode(spec2, spec1.GetHashCode());
86+
Assert.IsFalse(spec1.Equals(spec2));
87+
Assert.IsFalse(spec2.Equals(spec1));
88+
}
89+
90+
[Test]
91+
public void NativeSQLQuerySpecificationInequalityOnReturnContents()
92+
{
93+
var spec1 = new NativeSQLQuerySpecification("select blah",
94+
new[]
95+
{
96+
new NativeSQLQueryScalarReturn("alias1", NHibernateUtil.Character),
97+
new NativeSQLQueryScalarReturn("alias2", NHibernateUtil.Decimal)
98+
},
99+
null);
100+
var spec2 = new NativeSQLQuerySpecification("select blah",
101+
new[]
102+
{
103+
new NativeSQLQueryScalarReturn("alias1", NHibernateUtil.Character),
104+
new NativeSQLQueryScalarReturn("alias3", NHibernateUtil.Decimal)
105+
},
106+
null);
107+
108+
TweakHashcode(spec2, spec1.GetHashCode());
109+
Assert.IsFalse(spec1.Equals(spec2));
110+
Assert.IsFalse(spec2.Equals(spec1));
111+
}
112+
113+
[Test]
114+
public void NativeSQLQuerySpecificationInequalityOnReturnLengthes()
115+
{
116+
var spec1 = new NativeSQLQuerySpecification("select blah",
117+
new[]
118+
{
119+
new NativeSQLQueryScalarReturn("alias1", NHibernateUtil.Character),
120+
},
121+
null);
122+
var spec2 = new NativeSQLQuerySpecification("select blah",
123+
new[]
124+
{
125+
new NativeSQLQueryScalarReturn("alias1", NHibernateUtil.Character),
126+
new NativeSQLQueryScalarReturn("alias2", NHibernateUtil.Decimal)
127+
},
128+
null);
129+
130+
TweakHashcode(spec2, spec1.GetHashCode());
131+
Assert.IsFalse(spec1.Equals(spec2));
132+
Assert.IsFalse(spec2.Equals(spec1));
133+
}
134+
135+
[Test]
136+
public void NativeSQLQuerySpecificationInequalityOnReturnOrderings()
137+
{
138+
var spec1 = new NativeSQLQuerySpecification("select blah",
139+
new[]
140+
{
141+
new NativeSQLQueryScalarReturn("alias1", NHibernateUtil.Character),
142+
new NativeSQLQueryScalarReturn("alias2", NHibernateUtil.Decimal)
143+
},
144+
null);
145+
var spec2 = new NativeSQLQuerySpecification("select blah",
146+
new[]
147+
{
148+
new NativeSQLQueryScalarReturn("alias2", NHibernateUtil.Decimal),
149+
new NativeSQLQueryScalarReturn("alias1", NHibernateUtil.Character)
150+
},
151+
null);
152+
153+
TweakHashcode(spec2, spec1.GetHashCode());
154+
Assert.IsFalse(spec1.Equals(spec2));
155+
Assert.IsFalse(spec2.Equals(spec1));
156+
}
157+
158+
[Test]
159+
public void NativeSQLQuerySpecificationEqualityOnSpaces()
160+
{
161+
var spec1 = new NativeSQLQuerySpecification("select blah", null,
162+
new[] { "space1", "space2" });
163+
var spec2 = new NativeSQLQuerySpecification("select blah", null,
164+
new[] { "space1", "space2" });
165+
166+
Assert.IsTrue(spec1.Equals(spec2));
167+
Assert.IsTrue(spec2.Equals(spec1));
168+
}
169+
170+
[Test]
171+
public void NativeSQLQuerySpecificationInequalityOnNullSpace()
172+
{
173+
var spec1 = new NativeSQLQuerySpecification("select blah", null, null);
174+
var spec2 = new NativeSQLQuerySpecification("select blah", null,
175+
new[] { "space1", "space2" });
176+
177+
TweakHashcode(spec2, spec1.GetHashCode());
178+
Assert.IsFalse(spec1.Equals(spec2));
179+
Assert.IsFalse(spec2.Equals(spec1));
180+
}
181+
182+
[Test]
183+
public void NativeSQLQuerySpecificationInequalityOnSpaceContents()
184+
{
185+
var spec1 = new NativeSQLQuerySpecification("select blah", null,
186+
new[] { "space1", "space2" });
187+
var spec2 = new NativeSQLQuerySpecification("select blah", null,
188+
new[] { "space1", "space3" });
189+
190+
TweakHashcode(spec2, spec1.GetHashCode());
191+
Assert.IsFalse(spec1.Equals(spec2));
192+
Assert.IsFalse(spec2.Equals(spec1));
193+
}
194+
195+
[Test]
196+
public void NativeSQLQuerySpecificationInequalityOnSpaceLengthes()
197+
{
198+
var spec1 = new NativeSQLQuerySpecification("select blah", null,
199+
new[] { "space1", "space2" });
200+
var spec2 = new NativeSQLQuerySpecification("select blah", null,
201+
new[] { "space1", "space2", "space3" });
202+
203+
TweakHashcode(spec2, spec1.GetHashCode());
204+
Assert.IsFalse(spec1.Equals(spec2));
205+
Assert.IsFalse(spec2.Equals(spec1));
206+
}
207+
208+
[Test]
209+
public void NativeSQLQuerySpecificationInequalityOnSpaceOrderings()
210+
{
211+
var spec1 = new NativeSQLQuerySpecification("select blah", null,
212+
new[] { "space1", "space2" });
213+
var spec2 = new NativeSQLQuerySpecification("select blah", null,
214+
new[] { "space2", "space1" });
215+
216+
TweakHashcode(spec2, spec1.GetHashCode());
217+
Assert.IsFalse(spec1.Equals(spec2));
218+
Assert.IsFalse(spec2.Equals(spec1));
219+
}
220+
}
221+
}

‎src/NHibernate.Test/NHibernate.Test.csproj

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -943,6 +943,7 @@
943943
<Compile Include="NHSpecificTest\NH3909\Entity.cs" />
944944
<Compile Include="NHSpecificTest\NH3909\FixtureByCode.cs" />
945945
<Compile Include="NHSpecificTest\NH3957\ResultTransformerEqualityFixture.cs" />
946+
<Compile Include="NHSpecificTest\NH3956\Fixture.cs" />
946947
<Compile Include="NHSpecificTest\NH646\Domain.cs" />
947948
<Compile Include="NHSpecificTest\NH646\Fixture.cs" />
948949
<Compile Include="NHSpecificTest\NH3234\Fixture.cs" />

‎src/NHibernate/Engine/Query/Sql/NativeSQLQuerySpecification.cs

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -61,8 +61,12 @@ public override bool Equals(object obj)
6161
if (that == null)
6262
return false;
6363

64-
// NHibernate different impl.: NativeSQLQuerySpecification is immutable and the hash is calculated at Ctor
65-
return hashCode == that.hashCode;
64+
// NH-3956: hashcode inequality rules out equality, but hashcode equality is not enough.
65+
// Code taken back from 8e92af3f and amended according to NH-1931.
66+
return hashCode == that.hashCode &&
67+
queryString.Equals(that.queryString) &&
68+
CollectionHelper.CollectionEquals(querySpaces, that.querySpaces) &&
69+
CollectionHelper.CollectionEquals<INativeSQLQueryReturn>(sqlQueryReturns, that.sqlQueryReturns);
6670
}
6771

6872
public override int GetHashCode()

‎src/NHibernate/Util/CollectionHelper.cs

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -216,6 +216,12 @@ public IEnumerator GetEnumerator()
216216
public static readonly ICollection EmptyCollection = EmptyMap;
217217
public static readonly IList EmptyList = new EmptyListClass();
218218

219+
/// <summary>
220+
/// Determines if two collections have equals elements, with the same ordering.
221+
/// </summary>
222+
/// <param name="c1">The first collection.</param>
223+
/// <param name="c2">The second collection.</param>
224+
/// <returns><c>true</c> if collection are equals, <c>false</c> otherwise.</returns>
219225
public static bool CollectionEquals(ICollection c1, ICollection c2)
220226
{
221227
if (c1 == c2)
@@ -620,6 +626,13 @@ public static bool SetEquals<T>(ISet<T> a, ISet<T> b)
620626
return true;
621627
}
622628

629+
/// <summary>
630+
/// Determines if two collections have equals elements, with the same ordering.
631+
/// </summary>
632+
/// <typeparam name="T">The type of the elements.</typeparam>
633+
/// <param name="c1">The first collection.</param>
634+
/// <param name="c2">The second collection.</param>
635+
/// <returns><c>true</c> if collection are equals, <c>false</c> otherwise.</returns>
623636
public static bool CollectionEquals<T>(ICollection<T> c1, ICollection<T> c2)
624637
{
625638
if (c1 == c2)

0 commit comments

Comments
 (0)