-
Notifications
You must be signed in to change notification settings - Fork 178
Fix date() and time() function translation (#792) #1407
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -84,6 +84,11 @@ internal ExpressionBinderBase(IEdmModel model, ODataQuerySettings querySettings) | |
QuerySettings = querySettings; | ||
Model = model; | ||
} | ||
|
||
/// <summary> | ||
/// If queryable the query is being applied to is EF6 | ||
/// </summary> | ||
protected bool ClassicEF { get; private set; } | ||
#endregion | ||
|
||
#region Abstract properties and methods | ||
|
@@ -390,10 +395,18 @@ private Expression BindDate(SingleValueFunctionCallNode node) | |
|
||
// We should support DateTime & DateTimeOffset even though DateTime is not part of OData v4 Spec. | ||
Contract.Assert(arguments.Length == 1 && ExpressionBinderHelper.IsDateOrOffset(arguments[0].Type)); | ||
|
||
// EF doesn't support new Date(int, int, int), also doesn't support other property access, for example DateTime.Date. | ||
// Therefore, we just return the source (DateTime or DateTimeOffset). | ||
return arguments[0]; | ||
|
||
// EF6 and earlier don't support translating the Date property, so just return the original value | ||
if (ClassicEF) | ||
{ | ||
return arguments[0]; | ||
} | ||
|
||
Type type = Nullable.GetUnderlyingType(arguments[0].Type) ?? arguments[0].Type; | ||
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 commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for this fix. Just to confirm.. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @WanjohiSammy Added handling for EF6 (just falls back to the previous behaviour of returning the source). This required moving the IsClassicEF method and ClassicEF properties from TransformationBinderBase up to ExpressionBinderBase. |
||
} | ||
|
||
private Expression BindNow(SingleValueFunctionCallNode node) | ||
|
@@ -403,7 +416,7 @@ private Expression BindNow(SingleValueFunctionCallNode node) | |
// Function Now() does not take any arguments. | ||
Expression[] arguments = BindArguments(node.Parameters); | ||
Contract.Assert(arguments.Length == 0); | ||
|
||
return Expression.Property(null, typeof(DateTimeOffset), "UtcNow"); | ||
} | ||
|
||
|
@@ -415,10 +428,18 @@ private Expression BindTime(SingleValueFunctionCallNode node) | |
|
||
// We should support DateTime & DateTimeOffset even though DateTime is not part of OData v4 Spec. | ||
Contract.Assert(arguments.Length == 1 && ExpressionBinderHelper.IsDateOrOffset(arguments[0].Type)); | ||
|
||
// EF6 and earlier don't support translating the TimeOfDay property, so just return the original value | ||
if (ClassicEF) | ||
{ | ||
return arguments[0]; | ||
} | ||
|
||
// EF doesn't support new TimeOfDay(int, int, int, int), also doesn't support other property access, for example DateTimeOffset.DateTime. | ||
// Therefore, we just return the source (DateTime or DateTimeOffset). | ||
return arguments[0]; | ||
Type type = Nullable.GetUnderlyingType(arguments[0].Type) ?? arguments[0].Type; | ||
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 commentThe reason will be displayed to describe this comment to others. Learn more. Same here: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Resolved same as in https://github.com/OData/AspNetCoreOData/pull/1407/files#r1944232224 |
||
} | ||
|
||
private Expression BindFractionalSeconds(SingleValueFunctionCallNode node) | ||
|
@@ -981,6 +1002,15 @@ protected Expression GetFlattenedPropertyExpression(string propertyPath) | |
|
||
throw new ODataException(Error.Format(SRResources.PropertyOrPathWasRemovedFromContext, propertyPath)); | ||
} | ||
|
||
/// <summary> | ||
/// Initialize the state of the binder using the queryable to apply to | ||
/// </summary> | ||
/// <param name="query">Queryable the query is being applied to</param> | ||
protected void InitializeQuery(IQueryable query) | ||
{ | ||
ClassicEF = IsClassicEF(query); | ||
} | ||
#endregion | ||
|
||
#region Internal methods | ||
|
@@ -1336,5 +1366,15 @@ internal Expression BindCastToEnumType(Type sourceType, Type targetClrType, Quer | |
} | ||
} | ||
} | ||
|
||
/// <summary> | ||
/// Checks IQueryable provider for need of EF6 optimization | ||
/// </summary> | ||
/// <param name="query"></param> | ||
/// <returns>True if EF6 optimization are needed.</returns> | ||
internal virtual bool IsClassicEF(IQueryable query) | ||
{ | ||
return TypeHelper.IsClassicEFQueryProvider(query.Provider.GetType()); | ||
} | ||
#endregion | ||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -699,9 +699,17 @@ protected virtual Expression BindDate(SingleValueFunctionCallNode node, QueryBin | |||||||||||||
// We should support DateTime & DateTimeOffset even though DateTime is not part of OData v4 Spec. | ||||||||||||||
Contract.Assert(arguments.Length == 1 && ExpressionBinderHelper.IsDateOrOffset(arguments[0].Type)); | ||||||||||||||
|
||||||||||||||
// EF doesn't support new Date(int, int, int), also doesn't support other property access, for example DateTime.Date. | ||||||||||||||
// Therefore, we just return the source (DateTime or DateTimeOffset). | ||||||||||||||
return arguments[0]; | ||||||||||||||
// EF6 and earlier don't support translating the Date property, so just return the original value | ||||||||||||||
if (context.IsClassicEF) | ||||||||||||||
{ | ||||||||||||||
return arguments[0]; | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
Type type = Nullable.GetUnderlyingType(arguments[0].Type) ?? arguments[0].Type; | ||||||||||||||
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 commentThe 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 commentThe 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 That would probably work for the translation in AspNetCoreOData/src/Microsoft.AspNetCore.OData/Query/Expressions/TransformationBinderBase.cs Lines 49 to 54 in 40caec2
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @WanjohiSammy Added handling for EF6 here too, same as Resolved as in https://github.com/OData/AspNetCoreOData/pull/1407/files#r1944232224. As previously mentioned QueryBinder wasn't exposed to the queryable at any point, to get around this an IsClassicEF property has been added to QueryBinderContext and a new constructor for it accepting IQueryProvider added. This has been added as an additional constructor to preserve the public API, the new one could be changed to be internal if wanted but leaving it as public probably makes the most sense. The new constructor should now be in use everywhere that requires it. |
||||||||||||||
} | ||||||||||||||
|
||||||||||||||
/// <summary> | ||||||||||||||
|
@@ -718,10 +726,18 @@ protected virtual Expression BindTime(SingleValueFunctionCallNode node, QueryBin | |||||||||||||
|
||||||||||||||
// We should support DateTime & DateTimeOffset even though DateTime is not part of OData v4 Spec. | ||||||||||||||
Contract.Assert(arguments.Length == 1 && ExpressionBinderHelper.IsDateOrOffset(arguments[0].Type)); | ||||||||||||||
|
||||||||||||||
// EF6 and earlier don't support translating the TimeOfDay property, so just return the original value | ||||||||||||||
if (context.IsClassicEF) | ||||||||||||||
{ | ||||||||||||||
return arguments[0]; | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
Type type = Nullable.GetUnderlyingType(arguments[0].Type) ?? arguments[0].Type; | ||||||||||||||
PropertyInfo property = type.GetProperty(nameof(DateTime.TimeOfDay)); | ||||||||||||||
|
||||||||||||||
// EF doesn't support new TimeOfDay(int, int, int, int), also doesn't support other property access, for example DateTimeOffset.DateTime. | ||||||||||||||
// Therefore, we just return the source (DateTime or DateTimeOffset). | ||||||||||||||
return arguments[0]; | ||||||||||||||
Expression propertyAccessExpr = ExpressionBinderHelper.MakePropertyAccess(property, arguments[0], context.QuerySettings); | ||||||||||||||
return ExpressionBinderHelper.CreateFunctionCallWithNullPropagation(propertyAccessExpr, arguments, context.QuerySettings); | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
/// <summary> | ||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. Are you using EF here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These tests are for the in-memory queryable provider, there are a couple of tests for EF too below |
||
|
||
new[] {"$orderby=date(SameDayDateTimeOffset), Id desc", "5 > 4 > 3 > 2 > 1"}, | ||
new[] {"$orderby=date(SameDayNullableDateTimeOffset), Id desc", "3 > 5 > 4 > 2 > 1"}, | ||
|
||
new[] {"$orderby=time(SameDayDateTime)", "1 > 2 > 3 > 4 > 5"}, | ||
new[] {"$orderby=time(SameDayNullableDateTime)", "2 > 4 > 1 > 3 > 5"}, | ||
|
||
new[] {"$orderby=time(SameDayDateTimeOffset)", "5 > 4 > 3 > 2 > 1"}, | ||
new[] {"$orderby=time(SameDayNullableDateTimeOffset)", "3 > 1 > 2 > 4 > 5"}, | ||
}; | ||
TheoryDataSet<string, string, string> data = new TheoryDataSet<string, string, string>(); | ||
foreach (string mode in modes) | ||
|
@@ -559,6 +571,10 @@ public static TheoryDataSet<string, string> OrderByDataEf | |
|
||
new[] {"$orderby=NullableOffset", "3 > 1 > 2 > 4 > 5"}, | ||
new[] {"$orderby=NullableOffset desc", "5 > 4 > 2 > 1 > 3"}, | ||
|
||
new[] {"$orderby=date(NullableDateTime), Id desc", "4 > 2 > 3 > 1 > 5"}, | ||
|
||
new[] {"$orderby=time(DateTime)", "1 > 2 > 3 > 4 > 5"}, | ||
}; | ||
TheoryDataSet<string, string> data = new TheoryDataSet<string, string>(); | ||
foreach (string[] order in orders) | ||
|
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.
The new
QueryBinder
andQueryBinderContext
were designed to remove the tight coupling withIQueryable
that was present inExpressionBinderBase
and the older Binders.Can you add a Custom
FilterBinder
and override this method with your custom implementation? That's a cleaner solution.Reference https://devblogs.microsoft.com/odata/customizing-filter-for-spatial-data-in-asp-net-core-odata-8/#customizing-filterbinder
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.
That would be the ideal solution, however that doesn't work for all operators. Aggregation and compute operations still use
ExpressionBinderBase
so can't be customised (unless that has changed since this PR was opened?)