Skip to content

Fix constant value in function cached in LINQ query #1540

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

Fix constant value in function cached in LINQ query #1540

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
163007a
Fix constant value in function cached in LINQ query
fredericDelaporte Jan 18, 2018
ee6e31a
Add a test for generated keys with constants only in where.
fredericDelaporte Jan 19, 2018
3526ada
Remove ToString workaround
hazzik Jan 20, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions src/NHibernate.Test/Async/Linq/ConstantTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
using System.Collections.Generic;
using System.Linq;
using NHibernate.DomainModel.Northwind.Entities;
using NHibernate.Linq.Visitors;
using NUnit.Framework;
using NHibernate.Linq;

Expand Down Expand Up @@ -179,7 +180,6 @@ public int GetItemValue(Product p)

// Adapted from NH-2500 first test case by Andrey Titov (file NHTest3.zip)
[Test]
[Ignore("Not fixed yet")]
public async Task ObjectConstantsAsync()
{
var builder = new InfoBuilder(1);
Expand All @@ -200,7 +200,6 @@ private int TestFunc(Product item, int closureValue)

// Adapted from NH-3673
[Test]
[Ignore("Not fixed yet")]
public async Task ConstantsInFuncCallAsync()
{
var closureVariable = 1;
Expand Down
22 changes: 20 additions & 2 deletions src/NHibernate.Test/Linq/ConstantTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
using System.Collections.Generic;
using System.Linq;
using NHibernate.DomainModel.Northwind.Entities;
using NHibernate.Linq.Visitors;
using NUnit.Framework;

namespace NHibernate.Test.Linq
Expand Down Expand Up @@ -167,7 +168,6 @@ public int GetItemValue(Product p)

// Adapted from NH-2500 first test case by Andrey Titov (file NHTest3.zip)
[Test]
[Ignore("Not fixed yet")]
public void ObjectConstants()
{
var builder = new InfoBuilder(1);
Expand All @@ -188,7 +188,6 @@ private int TestFunc(Product item, int closureValue)

// Adapted from NH-3673
[Test]
[Ignore("Not fixed yet")]
public void ConstantsInFuncCall()
{
var closureVariable = 1;
Expand All @@ -201,5 +200,24 @@ public void ConstantsInFuncCall()
Assert.That(v1, Is.EqualTo(1), "v1");
Assert.That(v2, Is.EqualTo(2), "v2");
}

[Test]
public void ConstantInWhereDoesNotCauseManyKeys()
{
var q1 = (from c in db.Customers
where c.CustomerId == "ALFKI"
select c);
var q2 = (from c in db.Customers
where c.CustomerId == "ANATR"
select c);
var parameters1 = ExpressionParameterVisitor.Visit(q1.Expression, Sfi);
var k1 = ExpressionKeyVisitor.Visit(q1.Expression, parameters1);
var parameters2 = ExpressionParameterVisitor.Visit(q2.Expression, Sfi);
var k2 = ExpressionKeyVisitor.Visit(q2.Expression, parameters2);

Assert.That(parameters1, Has.Count.GreaterThan(0), "parameters1");
Assert.That(parameters2, Has.Count.GreaterThan(0), "parameters2");
Assert.That(k2, Is.EqualTo(k1));
}
}
}
53 changes: 29 additions & 24 deletions src/NHibernate/Linq/Visitors/ExpressionKeyVisitor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,19 +109,23 @@ protected override Expression VisitConstant(ConstantExpression expression)
{
_string.Append("NULL");
}
else
else if (expression.Value is IQueryable)
{
var value = expression.Value as IEnumerable;
if (value != null && !(value is string) && !(value is IQueryable))
{
_string.Append("{");
_string.Append(String.Join(",", value.Cast<object>()));
_string.Append("}");
}
else
_string.Append(expression.Value);
}
else if (expression.Value is IEnumerable enumerable && !(enumerable is string))
{
_string.Append("{");
foreach (var value in enumerable)
{
_string.Append(expression.Value);
_string.Append(value).Append("#").Append(value.GetHashCode()).Append(",");
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a hard time conceiving a valid query having objects instances instead of primitive types in a list/array. Are you sure this is useful to change the enumerable case too?

}

_string.Append("}");
}
else
{
_string.Append(expression.Value).Append("#").Append(expression.Value.GetHashCode());
}
}

Expand Down Expand Up @@ -163,20 +167,21 @@ protected override Expression VisitMethodCall(MethodCallExpression expression)
{
var old = insideSelectClause;

switch (expression.Method.Name)
{
case "First":
case "FirstOrDefault":
case "Single":
case "SingleOrDefault":
case "Select":
case "GroupBy":
insideSelectClause = true;
break;
default:
insideSelectClause = false;
break;
}
if (expression.Method.DeclaringType?.Namespace == "System.Linq")
Copy link
Member

@hazzik hazzik Jan 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should have else branch.

Copy link
Member Author

@fredericDelaporte fredericDelaporte Jan 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For putting what in it? All the point of that if is to not alter insideSelectClause, considering that only System.Linq calls may represent an exit out of/entry in select clause.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To put insideSelectClause = false; as it is in the default branch of switch statement.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I've checked that my suggestion does not work.

switch (expression.Method.Name)
{
case "First":
case "FirstOrDefault":
case "Single":
case "SingleOrDefault":
case "Select":
case "GroupBy":
insideSelectClause = true;
break;
default:
insideSelectClause = false;
break;
}

Visit(expression.Object);
_string.Append('.');
Expand Down