-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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.
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
Fix to #5454 - Error when using Where with reference #5796
Conversation
@@ -650,9 +650,22 @@ private static Expression HandleSkip(HandlerContext handlerContext) | |||
{ | |||
var skipResultOperator = (SkipResultOperator)handlerContext.ResultOperator; | |||
|
|||
handlerContext.SelectExpression.Offset = skipResultOperator.Count; | |||
var sqlTranslatingExpressionVisitor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you move this visitor creation into a method on HandlerContext and update all of the places we do it?. I.e. handlerContext.CreateSqlTranslatingVisitor
|
Is there a test for the Skip/Take change? |
Reminder to also do a R# pass. |
tests are: 'Optional_navigation_type_compensation_works_with_skip' and 'Optional_navigation_type_compensation_works_with_take' |
Problem was for happening for queries with optional navigations like so: context.Orders.Where(o => o.Customer.IsVip) In this case Customer can be nullable, so IsVip can also be nullable. We compensate for this by introducing the following: context.Orders.Where(o => o.Customer != null ? (bool?)o.Customer.IsVip : (bool?)null) However, we didn't convert it back to the original type (users had to introduce those casts themselves). Without the cast, those queries were throwing compile-time exceptions that were not very informative. Fix is to cast back to the original type requested by user: context.Orders.Where(o => (bool)(o.Customer != null ? (bool?)o.Customer.IsVip : (bool?)null)) This still may cause runtime errors if o.Customer is actually null, but those are much more understandable now. Also fixed compilation errors for complex Skip/Take arguments. Those are evaluated on the client for the time being. CR: Andrew, Smit
Fixes #5454
Problem was for happening for queries with optional navigations like so:
context.Orders.Where(o => o.Customer.IsVip)
In this case Customer can be nullable, so IsVip can also be nullable. We compensate for this by introducing the following:
context.Orders.Where(o => o.Customer != null ? (bool?)o.Customer.IsVip : (bool?)null)
However, we didn't convert it back to the original type (users had to introduce those casts themselves). Without the cast, those queries were throwing compile-time exceptions that were not very informative.
Fix is to cast back to the original type requested by user:
context.Orders.Where(o => (bool)(o.Customer != null ? (bool?)o.Customer.IsVip : (bool?)null))
This still may cause runtime errors if o.Customer is actually null, but those are much more understandable now.
Also fixed compilation errors for complex Skip/Take arguments. Those are evaluated on the client for the time being.