-
Notifications
You must be signed in to change notification settings - Fork 164
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 date() and time() function translation (#792) #1407
base: main
Are you sure you want to change the base?
Fix date() and time() function translation (#792) #1407
Conversation
@microsoft-github-policy-service agree |
PropertyInfo property = type.GetProperty(nameof(DateTime.Date)); | ||
|
||
Expression propertyAccessExpr = ExpressionBinderHelper.MakePropertyAccess(property, arguments[0], QuerySettings); | ||
return ExpressionBinderHelper.CreateFunctionCallWithNullPropagation(propertyAccessExpr, arguments, QuerySettings); |
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.
Thanks for this fix.
Just to confirm..
Will this change break for users still using older versions of Entity Framework other than EF6? Why not add a condition to ensure it supports older versions of EF.
PropertyInfo property = type.GetProperty(nameof(DateTime.TimeOfDay)); | ||
|
||
Expression propertyAccessExpr = ExpressionBinderHelper.MakePropertyAccess(property, arguments[0], QuerySettings); | ||
return ExpressionBinderHelper.CreateFunctionCallWithNullPropagation(propertyAccessExpr, arguments, QuerySettings); |
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.
Same here:
Older versions might not support new features from EF6. Adding a condition ensures compatibility.
PropertyInfo property = type.GetProperty(nameof(DateTime.Date)); | ||
|
||
Expression propertyAccessExpr = ExpressionBinderHelper.MakePropertyAccess(property, arguments[0], context.QuerySettings); | ||
return ExpressionBinderHelper.CreateFunctionCallWithNullPropagation(propertyAccessExpr, arguments, context.QuerySettings); |
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.
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.
Is it possible to tell if the query provider is EF6 from here?
The only place I can find that checks if it is EF6 is in TransformationBinderBase
, and it uses the type of IQueryable.Provider
to do so.
That would probably work for the translation in ExpressionBinderBase
since TransformationBinderBase
derives from it, but QueryBinder
isn't exposed to the queryable at any point?
AspNetCoreOData/src/Microsoft.AspNetCore.OData/Query/Expressions/TransformationBinderBase.cs
Lines 49 to 54 in 40caec2
internal virtual bool IsClassicEF(IQueryable query) | |
{ | |
var providerNS = query.Provider.GetType().Namespace; | |
return (providerNS == HandleNullPropagationOptionHelper.ObjectContextQueryProviderNamespaceEF6 | |
|| providerNS == HandleNullPropagationOptionHelper.EntityFrameworkQueryProviderNamespace); | |
} |
@@ -273,6 +273,18 @@ public static TheoryDataSet<string, string, string> OrderByData | |||
|
|||
new[] {"$orderby=NullableTimeOfDay", "3 > 1 > 2 > 4 > 5"}, | |||
new[] {"$orderby=NullableTimeOfDay desc", "5 > 4 > 2 > 1 > 3"}, | |||
|
|||
new[] {"$orderby=date(SameDayDateTime), Id desc", "5 > 4 > 3 > 2 > 1"}, | |||
new[] {"$orderby=date(SameDayNullableDateTime), Id desc", "4 > 2 > 5 > 3 > 1"}, |
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.
Are you using EF here?
Fix #792 -
date()
function not being translated. Also fixes similar issue oftime()
function not being translated.The comments previously implied that the
Date
andTimeOfDay
properties ofDateTime
andDateTimeOffset
were not supported by EF, but this is not the case with EF Core (see function mappings documentation).The translation has been updated to use these properties for the functions so that they work correctly. Test cases have been added for when they are used in $orderby specifically.
Note that this issue was not present in most circumstances when the
date()
function was used in $filter due to the type conversion used when dealing with Date types in binary expressions resulting in dates being compared via an integer representation instead:AspNetCoreOData/src/Microsoft.AspNetCore.OData/Query/Expressions/ExpressionBinderHelper.cs
Lines 425 to 441 in c860cdc
This won't work with EF6 as it doesn't support translating the
Date
property. Not sure if this is a problem, does this library even officially support EF6? Attempting to work around it feels somewhat out of scope.