Skip to content

Commit 23ddb4d

Browse files
fredericDelaportefredericDelaporte
committed
Avoid re-using query plan embedding different constant values.
 * Fixes nhibernate#1330 * Fixes nhibernate#866 * Fixes most of nhibernate#1363 (a workaround is still needed for one case)
1 parent 7a01d97 commit 23ddb4d

File tree

10 files changed

+373
-41
lines changed

10 files changed

+373
-41
lines changed

‎src/NHibernate.Test/Async/Linq/ConstantTest.cs

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
using System.Collections.Generic;
1212
using System.Linq;
1313
using NHibernate.DomainModel.Northwind.Entities;
14+
using NHibernate.Linq.Visitors;
1415
using NUnit.Framework;
1516
using NHibernate.Linq;
1617

@@ -175,13 +176,19 @@ public int GetItemValue(Product p)
175176
{
176177
return _value;
177178
}
179+
180+
// Workaround for having a different key per different instances.
181+
public override string ToString()
182+
{
183+
return base.ToString() + _value;
184+
}
178185
}
179186

180187
// Adapted from NH-2500 first test case by Andrey Titov (file NHTest3.zip)
181188
[Test]
182-
[Ignore("Not fixed yet")]
183189
public async Task ObjectConstantsAsync()
184190
{
191+
// Fixed with a workaround, see InfoBuilder above.
185192
var builder = new InfoBuilder(1);
186193
var v1 = await ((from p in db.Products
187194
select builder.GetItemValue(p)).FirstAsync());
@@ -200,7 +207,6 @@ private int TestFunc(Product item, int closureValue)
200207

201208
// Adapted from NH-3673
202209
[Test]
203-
[Ignore("Not fixed yet")]
204210
public async Task ConstantsInFuncCallAsync()
205211
{
206212
var closureVariable = 1;

‎src/NHibernate.Test/Async/NHSpecificTest/NH2658/Fixture.cs

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,98 @@
1+
//------------------------------------------------------------------------------
2+
// <auto-generated>
3+
// This code was generated by AsyncGenerator.
4+
//
5+
// Changes to this file may cause incorrect behavior and will be lost if
6+
// the code is regenerated.
7+
// </auto-generated>
8+
//------------------------------------------------------------------------------
9+
10+
11+
using System;
12+
using System.Collections;
13+
using System.Collections.ObjectModel;
14+
using System.Linq;
15+
using System.Linq.Expressions;
16+
using System.Reflection;
17+
using NHibernate.Engine;
18+
using NHibernate.Hql.Ast;
19+
using NHibernate.Linq.Functions;
20+
using NHibernate.Linq.Visitors;
21+
using NHibernate.Util;
22+
using NUnit.Framework;
23+
using NHibernate.Linq;
24+
25+
namespace NHibernate.Test.NHSpecificTest.NH2658
26+
{
27+
using System.Threading.Tasks;
28+
29+
[TestFixture]
30+
public class FixtureAsync : TestCase
31+
{
32+
public class DynamicPropertyGenerator : BaseHqlGeneratorForMethod
33+
{
34+
public DynamicPropertyGenerator()
35+
{
36+
//just registering for string here, but in a real implementation we'd be doing a runtime generator
37+
SupportedMethods = new[]
38+
{
39+
ReflectHelper.GetMethodDefinition(() => ObjectExtensions.GetProperty<string>(null, null))
40+
};
41+
}
42+
43+
public override HqlTreeNode BuildHql(
44+
MethodInfo method,
45+
Expression targetObject,
46+
ReadOnlyCollection<Expression> arguments,
47+
HqlTreeBuilder treeBuilder,
48+
IHqlExpressionVisitor visitor)
49+
{
50+
var propertyName = (string) ((ConstantExpression) arguments[1]).Value;
51+
52+
return treeBuilder.Dot(
53+
visitor.Visit(arguments[0]).AsExpression(),
54+
treeBuilder.Ident(propertyName)).AsExpression();
55+
}
56+
}
57+
58+
protected override string MappingsAssembly => "NHibernate.Test";
59+
60+
protected override IList Mappings => new[] { "NHSpecificTest.NH2658.Mappings.hbm.xml" };
61+
62+
protected override DebugSessionFactory BuildSessionFactory()
63+
{
64+
var sfi = base.BuildSessionFactory();
65+
66+
//add our linq extension
67+
((ISessionFactoryImplementor)sfi).Settings.LinqToHqlGeneratorsRegistry.Merge(new DynamicPropertyGenerator());
68+
return sfi;
69+
}
70+
71+
[Test]
72+
public async Task Does_Not_Cache_NonParametersAsync()
73+
{
74+
using (var session = OpenSession())
75+
{
76+
//PASSES
77+
using (var spy = new SqlLogSpy())
78+
{
79+
//Query by name
80+
await ((from p in session.Query<Product>() where p.GetProperty<string>("Name") == "Value" select p).ToListAsync());
81+
82+
Assert.That(spy.GetWholeLog(), Does.Contain("Name=@p0"));
83+
}
84+
85+
//FAILS
86+
//Because this query is considered the same as the top query the hql will be reused from the top statement
87+
//Even though GetProperty has a parameter that never get passed to sql or hql
88+
using (var spy = new SqlLogSpy())
89+
{
90+
//Query by description
91+
await ((from p in session.Query<Product>() where p.GetProperty<string>("Description") == "Value" select p).ToListAsync());
92+
93+
Assert.That(spy.GetWholeLog(), Does.Contain("Description=@p0"));
94+
}
95+
}
96+
}
97+
}
98+
}

‎src/NHibernate.Test/Linq/ConstantTest.cs

Lines changed: 27 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
using System.Collections.Generic;
22
using System.Linq;
33
using NHibernate.DomainModel.Northwind.Entities;
4+
using NHibernate.Linq.Visitors;
45
using NUnit.Framework;
56

67
namespace NHibernate.Test.Linq
@@ -163,13 +164,19 @@ public int GetItemValue(Product p)
163164
{
164165
return _value;
165166
}
167+
168+
// Workaround for having a different key per different instances.
169+
public override string ToString()
170+
{
171+
return base.ToString() + _value;
172+
}
166173
}
167174

168175
// Adapted from NH-2500 first test case by Andrey Titov (file NHTest3.zip)
169176
[Test]
170-
[Ignore("Not fixed yet")]
171177
public void ObjectConstants()
172178
{
179+
// Fixed with a workaround, see InfoBuilder above.
173180
var builder = new InfoBuilder(1);
174181
var v1 = (from p in db.Products
175182
select builder.GetItemValue(p)).First();
@@ -188,7 +195,6 @@ private int TestFunc(Product item, int closureValue)
188195

189196
// Adapted from NH-3673
190197
[Test]
191-
[Ignore("Not fixed yet")]
192198
public void ConstantsInFuncCall()
193199
{
194200
var closureVariable = 1;
@@ -201,5 +207,24 @@ public void ConstantsInFuncCall()
201207
Assert.That(v1, Is.EqualTo(1), "v1");
202208
Assert.That(v2, Is.EqualTo(2), "v2");
203209
}
210+
211+
[Test]
212+
public void ConstantInWhereDoesNotCauseManyKeys()
213+
{
214+
var q1 = (from c in db.Customers
215+
where c.CustomerId == "ALFKI"
216+
select c);
217+
var q2 = (from c in db.Customers
218+
where c.CustomerId == "ANATR"
219+
select c);
220+
var parameters1 = ExpressionParameterVisitor.Visit(q1.Expression, Sfi);
221+
var k1 = ExpressionKeyVisitor.Visit(q1.Expression, parameters1);
222+
var parameters2 = ExpressionParameterVisitor.Visit(q2.Expression, Sfi);
223+
var k2 = ExpressionKeyVisitor.Visit(q2.Expression, parameters2);
224+
225+
Assert.That(parameters1, Has.Count.GreaterThan(0), "parameters1");
226+
Assert.That(parameters2, Has.Count.GreaterThan(0), "parameters2");
227+
Assert.That(k2, Is.EqualTo(k1));
228+
}
204229
}
205230
}

‎src/NHibernate.Test/NHSpecificTest/NH2658/Entity.cs

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
namespace NHibernate.Test.NHSpecificTest.NH2658
2+
{
3+
public class Product
4+
{
5+
public virtual string ProductId { get; set; }
6+
7+
public virtual string Name { get; set; }
8+
9+
public virtual string Description { get; set; }
10+
}
11+
}

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

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,94 @@
1+
using System;
2+
using System.Collections;
3+
using System.Collections.ObjectModel;
4+
using System.Linq;
5+
using System.Linq.Expressions;
6+
using System.Reflection;
7+
using NHibernate.Engine;
8+
using NHibernate.Hql.Ast;
9+
using NHibernate.Linq.Functions;
10+
using NHibernate.Linq.Visitors;
11+
using NHibernate.Util;
12+
using NUnit.Framework;
13+
14+
namespace NHibernate.Test.NHSpecificTest.NH2658
15+
{
16+
public static class ObjectExtensions
17+
{
18+
public static T GetProperty<T>(this object o, string propertyName)
19+
{
20+
//no implementation for this test
21+
throw new NotImplementedException();
22+
}
23+
}
24+
25+
[TestFixture]
26+
public class Fixture : TestCase
27+
{
28+
public class DynamicPropertyGenerator : BaseHqlGeneratorForMethod
29+
{
30+
public DynamicPropertyGenerator()
31+
{
32+
//just registering for string here, but in a real implementation we'd be doing a runtime generator
33+
SupportedMethods = new[]
34+
{
35+
ReflectHelper.GetMethodDefinition(() => ObjectExtensions.GetProperty<string>(null, null))
36+
};
37+
}
38+
39+
public override HqlTreeNode BuildHql(
40+
MethodInfo method,
41+
Expression targetObject,
42+
ReadOnlyCollection<Expression> arguments,
43+
HqlTreeBuilder treeBuilder,
44+
IHqlExpressionVisitor visitor)
45+
{
46+
var propertyName = (string) ((ConstantExpression) arguments[1]).Value;
47+
48+
return treeBuilder.Dot(
49+
visitor.Visit(arguments[0]).AsExpression(),
50+
treeBuilder.Ident(propertyName)).AsExpression();
51+
}
52+
}
53+
54+
protected override string MappingsAssembly => "NHibernate.Test";
55+
56+
protected override IList Mappings => new[] { "NHSpecificTest.NH2658.Mappings.hbm.xml" };
57+
58+
protected override DebugSessionFactory BuildSessionFactory()
59+
{
60+
var sfi = base.BuildSessionFactory();
61+
62+
//add our linq extension
63+
((ISessionFactoryImplementor)sfi).Settings.LinqToHqlGeneratorsRegistry.Merge(new DynamicPropertyGenerator());
64+
return sfi;
65+
}
66+
67+
[Test]
68+
public void Does_Not_Cache_NonParameters()
69+
{
70+
using (var session = OpenSession())
71+
{
72+
//PASSES
73+
using (var spy = new SqlLogSpy())
74+
{
75+
//Query by name
76+
(from p in session.Query<Product>() where p.GetProperty<string>("Name") == "Value" select p).ToList();
77+
78+
Assert.That(spy.GetWholeLog(), Does.Contain("Name=@p0"));
79+
}
80+
81+
//FAILS
82+
//Because this query is considered the same as the top query the hql will be reused from the top statement
83+
//Even though GetProperty has a parameter that never get passed to sql or hql
84+
using (var spy = new SqlLogSpy())
85+
{
86+
//Query by description
87+
(from p in session.Query<Product>() where p.GetProperty<string>("Description") == "Value" select p).ToList();
88+
89+
Assert.That(spy.GetWholeLog(), Does.Contain("Description=@p0"));
90+
}
91+
}
92+
}
93+
}
94+
}

‎src/NHibernate.Test/NHSpecificTest/NH2658/Mappings.hbm.xml

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
<?xml version="1.0" encoding="utf-8" ?>
2+
<hibernate-mapping xmlns="urn:nhibernate-mapping-2.2" assembly="NHibernate.Test"
3+
namespace="NHibernate.Test.NHSpecificTest.NH2658">
4+
<class name="Product" table="Products">
5+
<id name="ProductId" column="ProductId" type="String">
6+
<generator class="assigned"/>
7+
</id>
8+
<property name="Name" column="Name"/>
9+
<property name="Description" column="Description"/>
10+
</class>
11+
</hibernate-mapping>

‎src/NHibernate/Engine/Query/QueryPlanCache.cs

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,15 +58,38 @@ public IQueryExpressionPlan GetHQLQueryPlan(IQueryExpression queryExpression, bo
5858
{
5959
log.Debug("unable to locate HQL query plan in cache; generating ({0})", queryExpression.Key);
6060
}
61+
62+
var refinableQuery = queryExpression as IRefinableKeyQueryExpression;
63+
var wasAlreadyRefined = refinableQuery?.RefinedKey;
64+
6165
plan = new QueryExpressionPlan(queryExpression, shallow, enabledFilters, factory);
6266
planCache.Put(key, plan);
67+
68+
if (wasAlreadyRefined == false && refinableQuery.RefinedKey)
69+
{
70+
// Additionally cache with the refined key. The cached entry with the non refined key is still
71+
// needed for allowing cache hit with non-refined key, which then refines the key.
72+
planCache.Put(new HQLQueryPlanKey(queryExpression, shallow, enabledFilters), plan);
73+
}
6374
}
6475
else
6576
{
6677
if (log.IsDebugEnabled())
6778
{
6879
log.Debug("located HQL query plan in cache ({0})", queryExpression.Key);
6980
}
81+
82+
if (plan.QueryExpression is IRefinableKeyQueryExpression cachedRefinableQuery && cachedRefinableQuery.RefinedKey &&
83+
queryExpression is IRefinableKeyQueryExpression refinableQuery && !refinableQuery.RefinedKey)
84+
{
85+
refinableQuery.RefineKey(cachedRefinableQuery.ParametersRefiningKey);
86+
if (refinableQuery.Key != cachedRefinableQuery.Key)
87+
{
88+
log.Debug("Key was refined and is no more matching, querying cache again.");
89+
return GetHQLQueryPlan(queryExpression, shallow, enabledFilters);
90+
}
91+
}
92+
7093
plan = CopyIfRequired(plan, queryExpression);
7194
}
7295

@@ -109,12 +132,35 @@ public IQueryExpressionPlan GetFilterQueryPlan(IQueryExpression queryExpression,
109132
if (plan == null)
110133
{
111134
log.Debug("unable to locate collection-filter query plan in cache; generating ({0} : {1})", collectionRole, queryExpression.Key);
135+
136+
var refinableQuery = queryExpression as IRefinableKeyQueryExpression;
137+
var wasAlreadyRefined = refinableQuery?.RefinedKey;
138+
112139
plan = new FilterQueryPlan(queryExpression, collectionRole, shallow, enabledFilters, factory);
113140
planCache.Put(key, plan);
141+
142+
if (wasAlreadyRefined == false && refinableQuery.RefinedKey)
143+
{
144+
// Additionally cache with the refined key. The cached entry with the non refined key is still
145+
// needed for allowing cache hit with non-refined key, which then refines the key.
146+
planCache.Put(new FilterQueryPlanKey(queryExpression.Key, collectionRole, shallow, enabledFilters), plan);
147+
}
114148
}
115149
else
116150
{
117151
log.Debug("located collection-filter query plan in cache ({0} : {1})", collectionRole, queryExpression.Key);
152+
153+
if (plan.QueryExpression is IRefinableKeyQueryExpression cachedRefinableQuery && cachedRefinableQuery.RefinedKey &&
154+
queryExpression is IRefinableKeyQueryExpression refinableQuery && !refinableQuery.RefinedKey)
155+
{
156+
refinableQuery.RefineKey(cachedRefinableQuery.ParametersRefiningKey);
157+
if (refinableQuery.Key != cachedRefinableQuery.Key)
158+
{
159+
log.Debug("Key was refined and is no more matching, querying cache again.");
160+
return GetFilterQueryPlan(queryExpression, collectionRole, shallow, enabledFilters);
161+
}
162+
}
163+
118164
plan = CopyIfRequired(plan, queryExpression);
119165
}
120166

0 commit comments

Comments
 (0)