From 130ae833d4d393b1fe5a3a02adfdf1f8c02dab36 Mon Sep 17 00:00:00 2001 From: Duncan M Date: Tue, 8 Nov 2016 10:22:37 -0700 Subject: [PATCH] NH-3918 - Nominate equality expressions in SELECT clauses with constants in them * Allow for entity equality to be correctly translated to HQL * Prevents unneeded joins and caching of entities in the query plan --- src/NHibernate.Test/Linq/SelectionTests.cs | 29 ++++ .../NHSpecificTest/NH3918/FixtureByCode.cs | 127 ++++++++++++++++++ .../NHSpecificTest/NH3918/Model.cs | 25 ++++ src/NHibernate.Test/NHibernate.Test.csproj | 2 + .../Linq/Visitors/SelectClauseNominator.cs | 15 ++- 5 files changed, 195 insertions(+), 3 deletions(-) create mode 100644 src/NHibernate.Test/NHSpecificTest/NH3918/FixtureByCode.cs create mode 100644 src/NHibernate.Test/NHSpecificTest/NH3918/Model.cs diff --git a/src/NHibernate.Test/Linq/SelectionTests.cs b/src/NHibernate.Test/Linq/SelectionTests.cs index 0ab83139438..1332ba8709b 100644 --- a/src/NHibernate.Test/Linq/SelectionTests.cs +++ b/src/NHibernate.Test/Linq/SelectionTests.cs @@ -450,6 +450,35 @@ public void CanSelectConditionalEntity() Assert.That(fatherInsteadOfChild, Has.Exactly(2).With.Property("SerialNumber").EqualTo("5678")); } + [Test] + public void CanSelectConditionalEntityWithCast() + { + var fatherInsteadOfChild = db.Mammals.Select(a => a.Father.SerialNumber == "5678" ? (object)a.Father : (object)a).ToList(); + Assert.That(fatherInsteadOfChild, Has.Exactly(2).With.Property("SerialNumber").EqualTo("5678")); + } + + [Test] + public void CanSelectConditionalEntityValue() + { + var fatherInsteadOfChild = db.Animals.Select(a => a.Father.SerialNumber == "5678" ? a.Father.SerialNumber : a.SerialNumber).ToList(); + Assert.That(fatherInsteadOfChild, Has.Exactly(2).EqualTo("5678")); + } + + [Test] + public void CanSelectConditionalEntityValueWithEntityComparison() + { + var father = db.Animals.Single(a => a.SerialNumber == "5678"); + var fatherInsteadOfChild = db.Animals.Select(a => a.Father == father ? a.Father.SerialNumber : a.SerialNumber).ToList(); + Assert.That(fatherInsteadOfChild, Has.Exactly(2).EqualTo("5678")); + } + + [Test] + public void CanSelectConditionalEntityValueWithEntityComparisonRepeat() + { + // Check again in the same ISessionFactory to ensure caching doesn't cause failures + CanSelectConditionalEntityValueWithEntityComparison(); + } + [Test] public void CanSelectConditionalObject() { diff --git a/src/NHibernate.Test/NHSpecificTest/NH3918/FixtureByCode.cs b/src/NHibernate.Test/NHSpecificTest/NH3918/FixtureByCode.cs new file mode 100644 index 00000000000..af6b7ab3b0b --- /dev/null +++ b/src/NHibernate.Test/NHSpecificTest/NH3918/FixtureByCode.cs @@ -0,0 +1,127 @@ +using System; +using System.Linq; +using System.Linq.Expressions; +using NHibernate.Cfg.MappingSchema; +using NHibernate.Linq; +using NHibernate.Mapping.ByCode; +using NUnit.Framework; + +namespace NHibernate.Test.NHSpecificTest.NH3918 +{ + public class ByCodeFixture : TestCaseMappingByCode + { + protected override HbmMapping GetMappings() + { + var mapper = new ModelMapper(); + mapper.Class(rc => + { + rc.Id(x => x.Id, m => m.Generator(Generators.GuidComb)); + rc.Property(x => x.Name); + }); + mapper.Class(rc => + { + rc.Id(x => x.Id, m => m.Generator(Generators.GuidComb)); + rc.Property(x => x.Name); + rc.ManyToOne(m => m.Owner, m => + { + m.Column("OwnerId"); + }); + }); + + return mapper.CompileMappingForAllExplicitlyAddedEntities(); + } + + protected override void OnSetUp() + { + using (ISession session = OpenSession()) + using (ITransaction transaction = session.BeginTransaction()) + { + var bob = CreateOwner(session, "Bob"); + var carl = CreateOwner(session, "Carl"); + var doug = CreateOwner(session, "Doug"); + + CreateEntity(session, "Test 1", bob); + CreateEntity(session, "Test 2", carl); + CreateEntity(session, "Test 3", doug); + CreateEntity(session, "Test 4", bob); + CreateEntity(session, "Test 5", carl); + CreateEntity(session, "Test 6", doug); + + session.Flush(); + transaction.Commit(); + } + } + + protected Owner CreateOwner(ISession session, string name) + { + var t = new Owner { Name = name }; + session.Save(t); + return t; + } + + protected void CreateEntity(ISession session, string name, Owner owner) + { + var t = new Entity + { + Name = name, + Owner = owner, + }; + session.Save(t); + } + + protected override void OnTearDown() + { + using (ISession session = OpenSession()) + using (ITransaction transaction = session.BeginTransaction()) + { + session.Delete("from Entity"); + session.Delete("from Owner"); + + session.Flush(); + transaction.Commit(); + } + } + + [Test] + public void EntityComparisonTest() + { + using (ISession session = OpenSession()) + using (session.BeginTransaction()) + { + var bob = session.Query().Single(o => o.Name == "Bob"); + + var queryWithWhere = session.Query() + .Where(WhereExpression(bob)) + .Select(e => e.Name); + var queryWithSelect = session.Query() + .Select(SelectExpression(bob)); + + var resultsFromWhere = queryWithWhere.ToList(); + var resultsFromSelect = queryWithSelect.ToList(); + + Assert.That(resultsFromSelect.Where(x => (bool)x[1]).Select(x => (string)x[0]), Is.EquivalentTo(resultsFromWhere)); + } + } + + [Test] + public void EntityComparisonTestAgain() + { + // When the entire fixture is run this will execute the test again within the same ISessionFactory which will test caching + EntityComparisonTest(); + } + + protected Expression> WhereExpression(Owner owner) + { + return e => e.Owner == owner; + } + + protected Expression> SelectExpression(Owner owner) + { + return e => new object[] + { + e.Name, + e.Owner == owner + }; + } + } +} diff --git a/src/NHibernate.Test/NHSpecificTest/NH3918/Model.cs b/src/NHibernate.Test/NHSpecificTest/NH3918/Model.cs new file mode 100644 index 00000000000..68e7ec5aa05 --- /dev/null +++ b/src/NHibernate.Test/NHSpecificTest/NH3918/Model.cs @@ -0,0 +1,25 @@ +using System; +using System.Collections.Generic; + +namespace NHibernate.Test.NHSpecificTest.NH3918 +{ + public interface IModelObject + { + Guid Id { get; set; } + string Name { get; set; } + } + + public class Entity : IModelObject + { + public virtual Guid Id { get; set; } + public virtual string Name { get; set; } + + public virtual Owner Owner { get; set; } + } + + public class Owner : IModelObject + { + public virtual Guid Id { get; set; } + public virtual string Name { get; set; } + } +} diff --git a/src/NHibernate.Test/NHibernate.Test.csproj b/src/NHibernate.Test/NHibernate.Test.csproj index 0d9c94be0ce..357d455111f 100644 --- a/src/NHibernate.Test/NHibernate.Test.csproj +++ b/src/NHibernate.Test/NHibernate.Test.csproj @@ -735,6 +735,8 @@ + + diff --git a/src/NHibernate/Linq/Visitors/SelectClauseNominator.cs b/src/NHibernate/Linq/Visitors/SelectClauseNominator.cs index 4c67403932b..4dae4f2aa9e 100644 --- a/src/NHibernate/Linq/Visitors/SelectClauseNominator.cs +++ b/src/NHibernate/Linq/Visitors/SelectClauseNominator.cs @@ -61,7 +61,7 @@ public override Expression VisitExpression(Expression expression) return innerExpression; } - var projectConstantsInHql = _stateStack.Peek() || IsRegisteredFunction(expression); + var projectConstantsInHql = _stateStack.Peek() || expression.NodeType == ExpressionType.Equal || IsRegisteredFunction(expression); // Set some flags, unless we already have proper values for them: // projectConstantsInHql if they are inside a method call executed server side. @@ -153,8 +153,17 @@ private bool CanBeEvaluatedInHqlSelectStatement(Expression expression, bool proj if (expression.NodeType == ExpressionType.Call) { // Depends if it's in the function registry - if (!IsRegisteredFunction(expression)) - return false; + return IsRegisteredFunction(expression); + } + + if (expression.NodeType == ExpressionType.Conditional) + { + // Theoretically, any conditional that returns a CAST-able primitive should be constructable in HQL. + // The type needs to be CAST-able because HQL wraps the CASE clause in a CAST and only supports + // certain types (as defined by the HqlIdent constructor that takes a System.Type as the second argument). + // However, this may still not cover all cases, so to limit the nomination of conditional expressions, + // we will only consider those which are already getting constants projected into them. + return projectConstantsInHql; } // Assume all is good