Fix using a loaded IPersistentCollection in a Query #1560
Conversation
gliljas
force-pushed
the
GH-1556
branch
2 times, most recently
from
d67255f to
f5bc175
Compare
| using Remotion.Linq.Parsing.ExpressionVisitors.TreeEvaluation; | ||
|
|
||
| namespace NHibernate.Linq.Visitors | ||
| { | ||
| //Modified version of PartialEvaluatingExpressionVisitor from Relinq (https://github.com/re-motion/Relinq) | ||
| //Copyright (c) rubicon IT GmbH, www.rubicon.eu |
There was a problem hiding this comment.
According to https://tldrlegal.com/license/apache-license-2.0-(apache-2.0) we must include the license. So, I think it shall be included on the top of the file.
There was a problem hiding this comment.
I think lines differing from original implementation should be signaled by comments, for easing seeing what is done differently in NHibernate.
| { | ||
| return EvaluateIndependentSubtrees(expression, new NhEvaluatableExpressionFilter()); | ||
| } | ||
| /// <summary> |
There was a problem hiding this comment.
Better have a blank line between methods.
| using Remotion.Linq.Parsing.ExpressionVisitors.TreeEvaluation; | ||
|
|
||
| namespace NHibernate.Linq.Visitors | ||
| { | ||
| //Modified version of PartialEvaluatingExpressionVisitor from Relinq (https://github.com/re-motion/Relinq) | ||
| //Copyright (c) rubicon IT GmbH, www.rubicon.eu |
There was a problem hiding this comment.
I think lines differing from original implementation should be signaled by comments, for easing seeing what is done differently in NHibernate.
| var constantExpression = (ConstantExpression) subtree; | ||
| var valueAsIQueryable = constantExpression.Value as IQueryable; | ||
| if (valueAsIQueryable != null && !IsEvaluatedQueryable(constantExpression.Value) && valueAsIQueryable.Expression != constantExpression) | ||
| return valueAsIQueryable.Expression; |
There was a problem hiding this comment.
I guess this is here that code differs. It seems to create a bunch of test failures, but I have not tried analyzing them yet.
| @@ -35,6 +124,8 @@ public Expression VisitPartialEvaluationException(PartialEvaluationExceptionExpr | |||
| } | |||
| } | |||
|
|
|||
|
|
|||
|
|
|||
There was a problem hiding this comment.
Undue additional blank lines.
gliljas
force-pushed
the
GH-1556
branch
2 times, most recently
from
d67b1ec to
b0d63a6
Compare
|
Interesting test errors on a couple of DB types. Investigating. |
|
The simplest fix is to extend filter to mark this type of collection as not enumerable. |
hazzik
changed the title
|
EDIT: I was wrong. |
|
@hazzik That's a bad side effect. |
|
Also, I'm all for LGTM, but I'd prefer a heads up rather than an edit. |
|
I’ll double check, but I’m pretty sure that your original changes had the same side effect. The code change is trivial, so it was easier to check-in, because to verify my assumption I had to make this change. |
|
@gliljas, actually, I was wrong (it seems that I'm blind / was dreaming). The both cases produce exactly the same query. They do not inline the collection into the query, but rather use the |
|
For proper attribution it would be better if this PR is squashed/merged by Alexander. If I squash/merge it myself only Gunnar and me would appear on the master commit. |
Apparently GitHub supports co-authors for commits. |
Fixes #1556
The problem was that the IPersistentCollection now implement IQueryable, which the PartialEvaluatingExpressionVisitor always rewrites to its inner expression.
I could find no way to intercept this mechanism, except to clone the entire class from source and rewrite it according to our needs. Relinq is licensed under Apache 2.0, so that should not be an issue.