Do not cache ExpandedQueryExpression query plan if it's based on not cacheable query expression

Author: bahusoid

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

@@ -11,6 +11,7 @@
 using System.Collections.Generic;
 using System.Linq;
 using System.Reflection;
+using NHibernate.Criterion;
 using NHibernate.DomainModel.Northwind.Entities;
 using NHibernate.Engine.Query;
 using NHibernate.Linq.Visitors;
@@ -269,5 +270,62 @@ public async Task PlansWithNonParameterizedConstantsAreNotCachedAsync()
 				Has.Count.EqualTo(0),
 				"Query plan should not be cached.");
 		}
+
+		[Test]
+		public async Task PlansWithNonParameterizedConstantsAreNotCachedForExpandedQueryAsync()
+		{
+			var queryPlanCacheType = typeof(QueryPlanCache);
+
+			var cache = (SoftLimitMRUCache)
+				queryPlanCacheType
+					.GetField("planCache", BindingFlags.Instance | BindingFlags.NonPublic)
+					.GetValue(Sfi.QueryPlanCache);
+			cache.Clear();
+
+			var ids = new[] {"ANATR", "UNKNOWN"}.ToList();
+			await (db.Customers.Where(x => ids.Contains(x.CustomerId)).Select(
+				c => new {c.CustomerId, c.ContactName, Constant = 1}).FirstAsync());
+
+			Assert.That(
+				cache,
+				Has.Count.EqualTo(0),
+				"Query plan should not be cached.");
+		}
+
+		//GH-2298 - Different Update queries - same query cache plan
+		[Test]
+		public async Task DmlPlansForExpandedQueryAsync()
+		{
+			var queryPlanCacheType = typeof(QueryPlanCache);
+
+			var cache = (SoftLimitMRUCache)
+				queryPlanCacheType
+					.GetField("planCache", BindingFlags.Instance | BindingFlags.NonPublic)
+					.GetValue(Sfi.QueryPlanCache);
+			cache.Clear();
+
+			using (session.BeginTransaction())
+			{
+				var list = new[] {"UNKNOWN", "UNKNOWN2"}.ToList();
+				await (db.Customers.Where(x => list.Contains(x.CustomerId)).UpdateAsync(
+					x => new Customer
+					{
+						CompanyName = "Constant1"
+					}));
+
+				await (db.Customers.Where(x => list.Contains(x.CustomerId))
+				.UpdateAsync(
+					x => new Customer
+					{
+						ContactName = "Constant1"
+					}));
+
+				Assert.That(
+					cache.Count,
+					//2 original queries + 2 expanded queries are expected in cache
+					Is.EqualTo(0).Or.EqualTo(4),
+					"Query plans should either be cached separately or not cached at all.");
+			}
+		}
 	}
 }

src/NHibernate.Test/Linq/ConstantTest.cs

@@ -1,6 +1,7 @@
 using System.Collections.Generic;
 using System.Linq;
 using System.Reflection;
+using NHibernate.Criterion;
 using NHibernate.DomainModel.Northwind.Entities;
 using NHibernate.Engine.Query;
 using NHibernate.Linq.Visitors;
@@ -276,5 +277,62 @@ public void PlansWithNonParameterizedConstantsAreNotCached()
 				Has.Count.EqualTo(0),
 				"Query plan should not be cached.");
 		}
+
+		[Test]
+		public void PlansWithNonParameterizedConstantsAreNotCachedForExpandedQuery()
+		{
+			var queryPlanCacheType = typeof(QueryPlanCache);
+
+			var cache = (SoftLimitMRUCache)
+				queryPlanCacheType
+					.GetField("planCache", BindingFlags.Instance | BindingFlags.NonPublic)
+					.GetValue(Sfi.QueryPlanCache);
+			cache.Clear();
+
+			var ids = new[] {"ANATR", "UNKNOWN"}.ToList();
+			db.Customers.Where(x => ids.Contains(x.CustomerId)).Select(
+				c => new {c.CustomerId, c.ContactName, Constant = 1}).First();
+
+			Assert.That(
+				cache,
+				Has.Count.EqualTo(0),
+				"Query plan should not be cached.");
+		}
+
+		//GH-2298 - Different Update queries - same query cache plan
+		[Test]
+		public void DmlPlansForExpandedQuery()
+		{
+			var queryPlanCacheType = typeof(QueryPlanCache);
+
+			var cache = (SoftLimitMRUCache)
+				queryPlanCacheType
+					.GetField("planCache", BindingFlags.Instance | BindingFlags.NonPublic)
+					.GetValue(Sfi.QueryPlanCache);
+			cache.Clear();
+
+			using (session.BeginTransaction())
+			{
+				var list = new[] {"UNKNOWN", "UNKNOWN2"}.ToList();
+				db.Customers.Where(x => list.Contains(x.CustomerId)).Update(
+					x => new Customer
+					{
+						CompanyName = "Constant1"
+					});
+
+				db.Customers.Where(x => list.Contains(x.CustomerId))
+				.Update(
+					x => new Customer
+					{
+						ContactName = "Constant1"
+					});
+
+				Assert.That(
+					cache.Count,
+					//2 original queries + 2 expanded queries are expected in cache
+					Is.EqualTo(0).Or.EqualTo(4),
+					"Query plans should either be cached separately or not cached at all.");
+			}
+		}
 	}
 }

src/NHibernate/Engine/Query/QueryPlanCache.cs

@@ -62,7 +62,7 @@ public IQueryExpressionPlan GetHQLQueryPlan(IQueryExpression queryExpression, bo
 				}
 				plan = new QueryExpressionPlan(queryExpression, shallow, enabledFilters, factory);
 				// 6.0 TODO: add "CanCachePlan { get; }" to IQueryExpression interface
-				if (!(queryExpression is NhLinqExpression linqExpression) || linqExpression.CanCachePlan)
+				if (!(queryExpression is ICacheableQueryExpression linqExpression) || linqExpression.CanCachePlan)
 					planCache.Put(key, plan);
 				else
 					log.Debug("Query plan not cacheable");
@@ -117,7 +117,7 @@ public IQueryExpressionPlan GetFilterQueryPlan(IQueryExpression queryExpression,
 				log.Debug("unable to locate collection-filter query plan in cache; generating ({0} : {1})", collectionRole, queryExpression.Key);
 				plan = new FilterQueryPlan(queryExpression, collectionRole, shallow, enabledFilters, factory);
 				// 6.0 TODO: add "CanCachePlan { get; }" to IQueryExpression interface
-				if (!(queryExpression is NhLinqExpression linqExpression) || linqExpression.CanCachePlan)
+				if (!(queryExpression is ICacheableQueryExpression linqExpression) || linqExpression.CanCachePlan)
 					planCache.Put(key, plan);
 				else
 					log.Debug("Query plan not cacheable");

src/NHibernate/IQueryExpression.cs

@@ -5,11 +5,17 @@
 
 namespace NHibernate
 {
+	//TODO 6.0: Merge into IQueryExpression
+	internal interface ICacheableQueryExpression
+	{
+		bool CanCachePlan { get; }
+	}
+
 	public interface IQueryExpression
 	{
 		IASTNode Translate(ISessionFactoryImplementor sessionFactory, bool filter);
 		string Key { get; }
 		System.Type Type { get; }
 		IList<NamedParameterDescriptor> ParameterDescriptors { get; }
 	}
-}
+}

src/NHibernate/Impl/ExpressionQueryImpl.cs

@@ -150,16 +150,18 @@ public override object[] ValueArray()
 		}
 	}
 
-	internal class ExpandedQueryExpression : IQueryExpression
+	internal class ExpandedQueryExpression : IQueryExpression, ICacheableQueryExpression
 	{
 		private readonly IASTNode _tree;
+		private ICacheableQueryExpression _cacheableExpression;
 
 		public ExpandedQueryExpression(IQueryExpression queryExpression, IASTNode tree, string key)
 		{
 			_tree = tree;
 			Key = key;
 			Type = queryExpression.Type;
 			ParameterDescriptors = queryExpression.ParameterDescriptors;
+			 _cacheableExpression = queryExpression as ICacheableQueryExpression;
 		}
 
 		#region IQueryExpression Members
@@ -176,6 +178,8 @@ public IASTNode Translate(ISessionFactoryImplementor sessionFactory, bool filter
 		public IList<NamedParameterDescriptor> ParameterDescriptors { get; private set; }
 
 		#endregion
+
+		public bool CanCachePlan => _cacheableExpression?.CanCachePlan ?? true;
 	}
 
 	internal class ParameterExpander

src/NHibernate/Linq/NhLinqExpression.cs

@@ -11,7 +11,7 @@
 
 namespace NHibernate.Linq
 {
-	public class NhLinqExpression : IQueryExpression
+	public class NhLinqExpression : IQueryExpression, ICacheableQueryExpression
 	{
 		public string Key { get; protected set; }