NH-3944 - Relinq v2 migration #599
Conversation
| /// Indicates to the Linq-to-NHibernate provider a method that must not be evaluated. If supported, | ||
| /// it will always be converted to the corresponding SQL statement. | ||
| /// </summary> | ||
| public class DBOnlyAttribute: Attribute |
There was a problem hiding this comment.
Better name welcome. Thanks to this attribute, we no more need to swallow silently exception raised from a non evaluatable method. Now, if the method is properly marked, no exception will be raised.
| { | ||
| public abstract class NhExpression : Expression | ||
| { | ||
| public override ExpressionType NodeType => ExpressionType.Extension; |
There was a problem hiding this comment.
Expression Trees v2 Spec mandates all extensions to declare their NodeType as ExpressionType.Extension.
Thus I have moved NhExpressionType to a new property.
| return base.Accept(visitor); | ||
| } | ||
|
|
||
| protected abstract Expression Accept(NhExpressionVisitor visitor); |
There was a problem hiding this comment.
With ET v2, that is the expression which trigger the adequate call on the visitor. This method will be responsible to do that for visitor supporting it. If we want it to be not restricted to sub-classes of NhExpressionVisitor, we should declare some more interfaces and use them instead.
| return newExpression != Expression | ||
| ? CreateNew(newExpression) | ||
| : this; | ||
| } |
There was a problem hiding this comment.
Implementation mandated by Expression Trees v2 Spec. This method is the default call performed by base Expression.Accept method. When overriding it, the Accept either call a specialized method on the Visitor which has to do appropriate work for it, or call its base implementation which will in turn call VisitChildren.
| } | ||
|
|
||
| public virtual Expression VisitNhStar(NhStarExpression expression) | ||
| => VisitExtension(expression); |
There was a problem hiding this comment.
With Expression Trees v2 Spec, those methods are now called by Expression.Accept. The old default implementation (calling Accept on the expression) would cause an infinite calls loop. Instead, the default implementation should be now to call VisitExtension.
|
|
||
| namespace NHibernate.Linq.Visitors | ||
| { | ||
| internal class NhPartialEvaluatingExpressionVisitor : RelinqExpressionVisitor |
There was a problem hiding this comment.
Does no more implements IPartialEvaluationExceptionExpressionVisitor. It was needed for supporting failed attempts at partially evaluating expressions on methods to be translated by the Linq provider. With the NhEvaluatableExpressionFilter, we can now avoid those failures by marking methods which should not be evaluated.
| } | ||
| else if (expression.NodeType == ExpressionType.LessThanOrEqual) | ||
| { | ||
| HandleBinaryOperation((a, b) => a.LessThanOrEqual(b)); |
There was a problem hiding this comment.
Huge pile of else if instead of a switch case. Not really related to the migration, but well, with the amount of other changes on those classes, I have decide to refactor such things by the way.
|
For reference, here is the page linking to ET v2 reference documentation. |
|
Now I have done this migration myself, I am no more convinced NHibernate implementation was really deviating from "Relinq good practices" as I have stated here. It seems to me extending Have a look at Relinq |
fredericDelaporte
force-pushed
the
NH-3944-Mig
branch
from
55e5758 to
beee716
Compare
|
So, what implementation shall we choose? |
|
Well, this one match a bit closer ET v2 spec guidelines, but this is not related to the having/with/join clauses removal. We could do the same with #568 (and probably should if you want to choose it). The removal of having/with/join clauses seems to simplify a bit the implementation of Relinq migration, but I find the result to be maybe less robust. (Due to having to lookup expressions in an additional data map for finding out they are a bit more than what they are by themselves.) I think I need to get a closer look to #568 to see if we can simplify it or if we would have better to use #599 instead. |
|
I have looked again to all that, and I have still mixed feelings about both PR. Extending expressions is well supported by Relinq, but extending clauses is a bit less. With Relinq v2 migration, as soon as NH own clauses from #568 are added in the model, only visitors aware of them could be used. Currently that does not cause issues, but maybe later could it turn out problematic. On the other hand, #599 flagging clauses as having additional meaning and matching them later will breaks in case of cloning operations. I think #599 relative simplicity will allow it to better follow future Relinq changes, compared to #568. So I vote for choosing #599 rather than #568. |
fredericDelaporte
force-pushed
the
NH-3944-Mig
branch
from
beee716 to
831f12c
Compare
|
Rebased for resolving conflicts. (x2) |
fredericDelaporte
force-pushed
the
NH-3944-Mig
branch
2 times, most recently
from
48b414c to
b18b12f
Compare
fredericDelaporte
force-pushed
the
NH-3944-Mig
branch
from
b18b12f to
f7b7601
Compare
|
Choosing #568 instead. |
Based on #594, here is the corresponding migration to Relinq v2.
It differs on some more points with #568, I am going to comment them with a review.