Skip to content

Support DateOnly and TimeOnly #450

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jan 21, 2022
Merged

Support DateOnly and TimeOnly #450

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b2b5fdb
Support DateOnly and TimeOnly
xuzhg Jan 20, 2022
85324e4
Update the nuget nuspec to include .NET 6 target framework
xuzhg Jan 20, 2022
090624a
Resolve review comment. Thanks @Juliano Leal Goncalves
xuzhg Jan 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions src/Microsoft.AspNetCore.OData/Common/TypeHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,19 @@ public static bool IsDateTime(Type clrType)
return Type.GetTypeCode(underlyingTypeOrSelf) == TypeCode.DateTime;
}

#if NET6_0
Copy link
Contributor

@julealgon julealgon Jan 20, 2022 •
edited by xuzhg
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you considered moving NET6-specific members into a partial file? That might make it easier to manage in the future. Something like TypeHelper.Net6.cs for example. #WontFix

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since there's only two new methods added, i'd like to create a new file until we have more .NET 6 specify methods added. Thanks for your suggestion.

internal static bool IsDateOnly(Type clrType)
{
Type underlyingTypeOrSelf = GetUnderlyingTypeOrSelf(clrType);
return underlyingTypeOrSelf == typeof(DateOnly);
}

internal static bool IsTimeOnly(Type clrType)
Copy link
Contributor

@julealgon julealgon Jan 20, 2022 •
edited by xuzhg
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the whole TypeHelper class is already internal, I'd recommend leaving these methods as public:
#Resolved

Copy link
Contributor

@julealgon julealgon Jan 20, 2022 •
edited by xuzhg
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize there are already similar methods here (for example, IsDateTime). However, have you considered making these extension methods instead? That would make it slightly more readable for callers to use them.

TypeHelper is already static so it would be fairly easy to convert them. #Resolved

{
Type underlyingTypeOrSelf = GetUnderlyingTypeOrSelf(clrType);
return underlyingTypeOrSelf == typeof(TimeOnly);
}
#endif
Copy link
Contributor

@julealgon julealgon Jan 20, 2022 •
edited by xuzhg
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might want to leave an extra blank line after this. #Resolved

/// <summary>
/// Determine if a type is a TimeSpan.
/// </summary>
Expand Down
7 changes: 7 additions & 0 deletions src/Microsoft.AspNetCore.OData/Edm/DefaultODataTypeMapper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,13 @@ static DefaultODataTypeMapper()
BuildTypeMapping<char>(EdmPrimitiveTypeKind.String, isStandard: false);
BuildTypeMapping<DateTime?>(EdmPrimitiveTypeKind.DateTimeOffset, isStandard: false);
BuildTypeMapping<DateTime>(EdmPrimitiveTypeKind.DateTimeOffset, isStandard: false);

#if NET6_0
BuildTypeMapping<DateOnly>(EdmPrimitiveTypeKind.Date, isStandard: false);
BuildTypeMapping<DateOnly?>(EdmPrimitiveTypeKind.Date, isStandard: false);
Copy link
Contributor

@julealgon julealgon Jan 20, 2022 •
edited by xuzhg
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume there is no behavior difference in the order we add these mappings, but I'd still suggest adding the nullable versions prior to the non-nullable ones for consistency: all other types in this method are following that pattern.

FAKE EDIT: Actually, scratch that: the order does seem to matter, as per the comment at the top of the method:

// Do not change the order for the nullable or non-nullable. Put nullable ahead of non-nullable.
// By design: non-nullable will overwrite the item1.

I'd even go as far as to suggest a small method to encapsulate this, something like:

        public void BuildStructTypeMapping<T>(EdmPrimitiveTypeKind edmPrimitiveTypeKind, bool isStandard)
            where T : struct
        {
            BuildTypeMapping<T?>(edmPrimitiveTypeKind, isStandard);
            BuildTypeMapping<T>(edmPrimitiveTypeKind, isStandard);
        }

Using this for all structs in the method would simplify it quite a bit and centralize the order concern (the comment could be even moved over into this method). #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind sharing with me a contribution for this? I'd love to take and merge it for you.

Copy link
Contributor

@julealgon julealgon Jan 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For sure. I'll wait until you merge this and then refactor the whole block so as to avoid you having to rebase it.

For now, just make sure the order is as per the comment and we should be good.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you are right for the order. I forget what i said. OMG

BuildTypeMapping<TimeOnly>(EdmPrimitiveTypeKind.TimeOfDay, isStandard: false);
BuildTypeMapping<TimeOnly?>(EdmPrimitiveTypeKind.TimeOfDay, isStandard: false);
#endif
}
#endregion

Expand Down
22 changes: 22 additions & 0 deletions src/Microsoft.AspNetCore.OData/Edm/EdmPrimitiveHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,28 @@ public static object ConvertPrimitiveValue(object value, Type type, TimeZoneInfo

throw new ValidationException(Error.Format(SRResources.PropertyMustBeBoolean));
}
#if NET6_0
else if (type == typeof(DateOnly))
Copy link
Contributor

@julealgon julealgon Jan 20, 2022 •
edited by xuzhg
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice if this could be simplified using a switch expression instead of all the else ifs.

I understand it is a bit unrelated though, so up to you. #WontFix

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion, i'd like to leave for other PR.

{
if (value is Date)
{
Date dt = (Date)value;
return new DateOnly(dt.Year, dt.Month, dt.Day);
}
Copy link
Contributor

@julealgon julealgon Jan 20, 2022 •
edited by xuzhg
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would strongly recommend using pattern matching capture instead:

if (value is Date date)
{
    return new DateOnly(date.Year, date.Month, date.Day);
}

Or a variation that includes the exception:

return value switch
{
    Date date => new DateOnly(date.Year, date.Month, date.Day).
    _ => throw new ValidationException(Error.Format(SRResources.PropertyMustBeDateTimeOffsetOrDate)),
} #Resolved


throw new ValidationException(Error.Format(SRResources.PropertyMustBeDateTimeOffsetOrDate));
}
else if (type == typeof(TimeOnly))
{
if (value is TimeOfDay)
{
TimeOfDay tod = (TimeOfDay)value;
return new TimeOnly(tod.Hours, tod.Minutes, tod.Seconds, (int)tod.Milliseconds);
Copy link
Contributor

@julealgon julealgon Jan 20, 2022 •
edited by xuzhg
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed the restriction to int and took a look just to make sure...

Man this is really wrong, the EDM type property really shouldn't have been long in the first place here:
image
#WontFix

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, It's ODL problem and now i have to cast it.

}

throw new ValidationException(Error.Format(SRResources.PropertyMustBeTimeOfDay));
}
Copy link
Contributor

@julealgon julealgon Jan 20, 2022 •
edited by xuzhg
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above. #Resolved

#endif
else
{
if (TypeHelper.TryGetInstance(type, value, out var result))
Expand Down
3 changes: 3 additions & 0 deletions src/Microsoft.AspNetCore.OData/Extensions/SerializableErrorExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Diagnostics.CodeAnalysis;
using System.Globalization;
using System.Linq;
using System.Text;
using Microsoft.AspNetCore.Mvc;
Expand Down Expand Up @@ -102,6 +104,7 @@ private static ODataInnerError ToODataInnerError(this Dictionary<string, object>

// Convert the model state errors in to a string (for debugging only).
// This should be improved once ODataError allows more details.
[SuppressMessage("Globalization", "CA1305:Specify IFormatProvider", Justification = "<Pending>")]
Copy link
Contributor

@julealgon julealgon Jan 20, 2022 •
edited by xuzhg
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume you forgot to write the justification there. Please double check.

Also, can you elaborate why we are touching this file as part of this PR? It looks unrelated to me. #Resolved

private static string ConvertModelStateErrors(this IReadOnlyDictionary<string, object> errors)
{
StringBuilder builder = new StringBuilder();
Expand Down
17 changes: 17 additions & 0 deletions src/Microsoft.AspNetCore.OData/Formatter/Serialization/ODataPrimitiveSerializer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,23 @@ internal static object ConvertPrimitiveValue(object value, IEdmPrimitiveTypeRefe
return tod;
}

#if NET6_0
// Since ODL doesn't support "DateOnly" and "TimeOnly", we have to use Date and TimeOfDay defined in ODL as a bridge.
Copy link
Contributor

@julealgon julealgon Jan 20, 2022 •
edited by xuzhg
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the comment is currently sitting on top of an if statement, and the other if statement below also refers to it, I`d suggest splitting the comment in 2 and adding one for each type on their respective blocks.

Otherwise this creates a sequential coupling between the blocks that is very hard to see (i.e., if code is added in-between them, it will suddenly stop making sense). #Resolved

if (primitiveType != null && primitiveType.IsDate() && TypeHelper.IsDateOnly(type))
{
DateOnly dateOnly = (DateOnly)value;
Date dt = new Date(dateOnly.Year, dateOnly.Month, dateOnly.Day);
return dt;
Copy link
Contributor

@julealgon julealgon Jan 20, 2022 •
edited by xuzhg
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just inline the dt variable here? #Resolved

}

if (primitiveType != null && primitiveType.IsTimeOfDay() && TypeHelper.IsTimeOnly(type))
{
TimeOnly timeOnly = (TimeOnly)value;
TimeOfDay tod = new TimeOfDay(timeOnly.Hour, timeOnly.Minute, timeOnly.Second, timeOnly.Millisecond);
return tod;
Copy link
Contributor

@julealgon julealgon Jan 20, 2022 •
edited by xuzhg
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above on inlining unnecessary temporary variable. #Resolved

}
#endif

return ConvertUnsupportedPrimitives(value, timeZoneInfo);
}

Expand Down
2 changes: 1 addition & 1 deletion src/Microsoft.AspNetCore.OData/Microsoft.AspNetCore.OData.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
<Project Sdk="Microsoft.NET.Sdk.Web">

<PropertyGroup>
<TargetFrameworks>netcoreapp3.1;net5.0</TargetFrameworks>
<TargetFrameworks>netcoreapp3.1;net5.0;net6.0</TargetFrameworks>
<RootNamespace>Microsoft.AspNetCore.OData</RootNamespace>
<DocumentationFile>$(OutputPath)$(AssemblyName).xml</DocumentationFile>
<NoDefaultLaunchSettingsFile>true</NoDefaultLaunchSettingsFile>
Expand Down
19 changes: 19 additions & 0 deletions src/Microsoft.AspNetCore.OData/Query/ClrCanonicalFunctions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,25 @@ internal class ClrCanonicalFunctions
new KeyValuePair<string, PropertyInfo>(MillisecondFunctionName, typeof(TimeOfDay).GetProperty("Milliseconds")),
}.ToDictionary(kvp => kvp.Key, kvp => kvp.Value);

#if NET6_0
// DateOnly properties
public static readonly Dictionary<string, PropertyInfo> DateOnlyProperties = new[]
{
new KeyValuePair<string, PropertyInfo>(YearFunctionName, typeof(DateOnly).GetProperty("Year")),
Copy link
Contributor

@julealgon julealgon Jan 20, 2022 •
edited by xuzhg
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of using hardcoded strings, can we use nameof for these properties?

For example:
typeof(DateOnly).GetProperty(nameof(DateOnly.Year)) #Resolved

new KeyValuePair<string, PropertyInfo>(MonthFunctionName, typeof(DateOnly).GetProperty("Month")),
new KeyValuePair<string, PropertyInfo>(DayFunctionName, typeof(DateOnly).GetProperty("Day")),
}.ToDictionary(kvp => kvp.Key, kvp => kvp.Value);
Copy link
Contributor

@julealgon julealgon Jan 20, 2022 •
edited by xuzhg
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a very convoluted and expensive way to initialize the dictionary. I'd recommend the following instead:

        public static readonly Dictionary<string, PropertyInfo> DateOnlyProperties = new()
        {
            [YearFunctionName] = typeof(DateOnly).GetProperty("Year"),
            [MonthFunctionName] = typeof(DateOnly).GetProperty("Month"),
            [DayFunctionName] = typeof(DateOnly).GetProperty("Day"),
        };
``` #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed to use {}.


// TimeOnly
Copy link
Contributor

@julealgon julealgon Jan 20, 2022 •
edited by xuzhg
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: would suggest adding the " properties" at the end for consistency #Resolved

public static readonly Dictionary<string, PropertyInfo> TimeOnlyProperties = new[]
{
new KeyValuePair<string, PropertyInfo>(HourFunctionName, typeof(TimeOnly).GetProperty("Hour")),
new KeyValuePair<string, PropertyInfo>(MinuteFunctionName, typeof(TimeOnly).GetProperty("Minute")),
new KeyValuePair<string, PropertyInfo>(SecondFunctionName, typeof(TimeOnly).GetProperty("Second")),
new KeyValuePair<string, PropertyInfo>(MillisecondFunctionName, typeof(TimeOnly).GetProperty("Millisecond")),
}.ToDictionary(kvp => kvp.Key, kvp => kvp.Value);
Copy link
Contributor

@julealgon julealgon Jan 20, 2022 •
edited by xuzhg
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comments as above apply here. #Resolved

#endif

// TimeSpan properties
public static readonly Dictionary<string, PropertyInfo> TimeSpanProperties = new[]
{
Expand Down
49 changes: 46 additions & 3 deletions src/Microsoft.AspNetCore.OData/Query/Expressions/ExpressionBinderHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,21 @@ public static Expression CreateBinaryExpression(BinaryOperatorKind binaryOperato
right = CreateTimeBinaryExpression(right, querySettings);
}

#if NET6_0
if ((IsType<DateOnly>(leftUnderlyingType) && IsDate(rightUnderlyingType)) ||
(IsDate(leftUnderlyingType) && IsType<DateOnly>(rightUnderlyingType)))
{
left = CreateDateBinaryExpression(left, querySettings);
right = CreateDateBinaryExpression(right, querySettings);
}
else if((IsType<TimeOnly>(leftUnderlyingType) && IsTimeOfDay(rightUnderlyingType)) ||
(IsTimeOfDay(leftUnderlyingType) && IsType<TimeOnly>(rightUnderlyingType)))
{
left = CreateTimeBinaryExpression(left, querySettings);
right = CreateTimeBinaryExpression(right, querySettings);
}
#endif

if (left.Type != right.Type)
{
// one of them must be nullable and the other is not.
Expand Down Expand Up @@ -381,6 +396,16 @@ private static Expression GetProperty(Expression source, string propertyName, OD
{
return MakePropertyAccess(ClrCanonicalFunctions.TimeSpanProperties[propertyName], source, querySettings);
}
#if NET6_0
else if (IsType<DateOnly>(source.Type))
{
return MakePropertyAccess(ClrCanonicalFunctions.DateOnlyProperties[propertyName], source, querySettings);
}
else if (IsType<TimeOnly>(source.Type))
{
return MakePropertyAccess(ClrCanonicalFunctions.TimeOnlyProperties[propertyName], source, querySettings);
}
#endif

return source;
}
Expand All @@ -407,15 +432,19 @@ private static Expression CreateTimeBinaryExpression(Expression source, ODataQue
{
source = ConvertToDateTimeRelatedConstExpression(source);

long ticksPerHour = 36000000000L;
long ticksPerMinute = 600000000L;
long ticksPerSecond = 10000000L;
Copy link
Contributor

@julealgon julealgon Jan 20, 2022 •
edited by xuzhg
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These values should be const.

Having said that, I'd strongly recommend just using the constants exposed by the TimeSpan type itself, namely TimeSpan.TicksPerHour, TimeSpan.TicksPerMinute and TimeSpan.TicksPerSecond, which have these exact same values. You should be able to just use them directly in the expression below. #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's just what i am looking for.


// Hour, Minute, Second, Millisecond
Expression hour = GetProperty(source, ClrCanonicalFunctions.HourFunctionName, querySettings);
Expression minute = GetProperty(source, ClrCanonicalFunctions.MinuteFunctionName, querySettings);
Expression second = GetProperty(source, ClrCanonicalFunctions.SecondFunctionName, querySettings);
Expression milliSecond = GetProperty(source, ClrCanonicalFunctions.MillisecondFunctionName, querySettings);

Expression hourTicks = Expression.Multiply(Expression.Convert(hour, typeof(long)), Expression.Constant(TimeOfDay.TicksPerHour));
Expression minuteTicks = Expression.Multiply(Expression.Convert(minute, typeof(long)), Expression.Constant(TimeOfDay.TicksPerMinute));
Expression secondTicks = Expression.Multiply(Expression.Convert(second, typeof(long)), Expression.Constant(TimeOfDay.TicksPerSecond));
Expression hourTicks = Expression.Multiply(Expression.Convert(hour, typeof(long)), Expression.Constant(ticksPerHour, typeof(long)));
Expression minuteTicks = Expression.Multiply(Expression.Convert(minute, typeof(long)), Expression.Constant(ticksPerMinute, typeof(long)));
Expression secondTicks = Expression.Multiply(Expression.Convert(second, typeof(long)), Expression.Constant(ticksPerSecond, typeof(long)));

// return (hour * TicksPerHour + minute * TicksPerMinute + second * TicksPerSecond + millisecond)
Expression result = Expression.Add(hourTicks, Expression.Add(minuteTicks, Expression.Add(secondTicks, Expression.Convert(milliSecond, typeof(long)))));
Expand Down Expand Up @@ -451,6 +480,20 @@ private static Expression ConvertToDateTimeRelatedConstExpression(Expression sou
{
return Expression.Constant(timeOfDay.Value, typeof(TimeOfDay));
}

#if NET6_0
var dateOnly = parameterizedConstantValue as DateOnly?;
if (dateOnly != null)
{
return Expression.Constant(dateOnly.Value, typeof(DateOnly));
}

var timeOnly = parameterizedConstantValue as TimeOnly?;
if (timeOnly != null)
{
return Expression.Constant(timeOnly.Value, typeof(TimeOnly));
}
Copy link
Contributor

@julealgon julealgon Jan 20, 2022 •
edited by xuzhg
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These checks are fairly convoluted as well. We can use a simple pattern match capture here:

                if (parameterizedConstantValue is DateOnly dateOnly)
                {
                    return Expression.Constant(dateOnly);
                }

                if (parameterizedConstantValue is TimeOnly timeOnly)
                {
                    return Expression.Constant(timeOnly);
                }

The type parameter is then probably not necessary as well since they are directly typed to the non-null type.

Again... this entire method should be simplified with a big switch expression return but I understand it might be unrelated here. #Resolved

#endif
}

return source;
Expand Down
27 changes: 25 additions & 2 deletions src/Microsoft.AspNetCore.OData/Query/Expressions/QueryBinder.SingleValueFunctionCall.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,7 +348,11 @@ protected virtual Expression BindDateRelatedProperty(SingleValueFunctionCallNode
CheckArgumentNull(node, context);

Expression[] arguments = BindArguments(node.Parameters, context);
Contract.Assert(arguments.Length == 1 && ExpressionBinderHelper.IsDateRelated(arguments[0].Type));
Contract.Assert(arguments.Length == 1 && (ExpressionBinderHelper.IsDateRelated(arguments[0].Type)
#if NET6_0
|| ExpressionBinderHelper.IsType<DateOnly>(arguments[0].Type)
Copy link
Contributor

@julealgon julealgon Jan 20, 2022 •
edited by xuzhg
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of adding a conditional at this point, I'd strongly suggest adding the same check inside the IsDateRelated method itself. Otherwise, this will decentralize the behavior and be a potential source of bugs. #Resolved

#endif
));

// We should support DateTime & DateTimeOffset even though DateTime is not part of OData v4 Spec.
Expression parameter = arguments[0];
Expand All @@ -359,6 +363,13 @@ protected virtual Expression BindDateRelatedProperty(SingleValueFunctionCallNode
Contract.Assert(ClrCanonicalFunctions.DateProperties.ContainsKey(node.Name));
property = ClrCanonicalFunctions.DateProperties[node.Name];
}
#if NET6_0
else if (ExpressionBinderHelper.IsType<DateOnly>(parameter.Type))
Copy link
Contributor

@julealgon julealgon Jan 20, 2022 •
edited by xuzhg
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you considered adding a IsDateOnly and IsTimeOnly methods for consistency with the way other checks are made?

I understand they are trivial 1 liners, but it makes the code more consistent when the checks use the same approaches.

Otherwise, when reading this code, it immediately raises questions to me like "wait, but why are we not using IsType<DateTime> as well?", you know? #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:) I have the same concern.

{
Contract.Assert(ClrCanonicalFunctions.DateOnlyProperties.ContainsKey(node.Name));
property = ClrCanonicalFunctions.DateOnlyProperties[node.Name];
}
#endif
else if (ExpressionBinderHelper.IsDateTime(parameter.Type))
{
Contract.Assert(ClrCanonicalFunctions.DateTimeProperties.ContainsKey(node.Name));
Expand All @@ -384,7 +395,12 @@ protected virtual Expression BindTimeRelatedProperty(SingleValueFunctionCallNode
CheckArgumentNull(node, context);

Expression[] arguments = BindArguments(node.Parameters, context);
Contract.Assert(arguments.Length == 1 && (ExpressionBinderHelper.IsTimeRelated(arguments[0].Type)));

Copy link
Contributor

@julealgon julealgon Jan 20, 2022 •
edited by xuzhg
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unintended line break? #Resolved

Contract.Assert(arguments.Length == 1 && (ExpressionBinderHelper.IsTimeRelated(arguments[0].Type)
#if NET6_0
|| ExpressionBinderHelper.IsType<TimeOnly>(arguments[0].Type)
#endif
Copy link
Contributor

@julealgon julealgon Jan 20, 2022 •
edited by xuzhg
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above. Please move the check to inside the IsTimeRelated function itself. #Resolved

));

// We should support DateTime & DateTimeOffset even though DateTime is not part of OData v4 Spec.
Expression parameter = arguments[0];
Expand All @@ -395,6 +411,13 @@ protected virtual Expression BindTimeRelatedProperty(SingleValueFunctionCallNode
Contract.Assert(ClrCanonicalFunctions.TimeOfDayProperties.ContainsKey(node.Name));
property = ClrCanonicalFunctions.TimeOfDayProperties[node.Name];
}
#if NET6_0
else if (ExpressionBinderHelper.IsType<TimeOnly>(parameter.Type))
Copy link
Contributor

@julealgon julealgon Jan 20, 2022 •
edited by xuzhg
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above regarding IsType<T> vs IsTimeOnly. #Resolved

{
Contract.Assert(ClrCanonicalFunctions.TimeOnlyProperties.ContainsKey(node.Name));
property = ClrCanonicalFunctions.TimeOnlyProperties[node.Name];
}
#endif
else if (ExpressionBinderHelper.IsDateTime(parameter.Type))
{
Contract.Assert(ClrCanonicalFunctions.DateTimeProperties.ContainsKey(node.Name));
Expand Down
6 changes: 6 additions & 0 deletions src/Microsoft.AspNetCore.OData/Query/Expressions/QueryBinder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1216,6 +1216,12 @@ internal static Expression ConvertNonStandardPrimitives(Expression source, Query
// we handle enum conversions ourselves
convertedExpression = source;
}
#if NET6_0
else if (TypeHelper.IsDateOnly(sourceType) || TypeHelper.IsTimeOnly(sourceType))
{
convertedExpression = source;
}
#endif
else
{
switch (Type.GetTypeCode(sourceType))
Expand Down
2 changes: 2 additions & 0 deletions src/Microsoft.AspNetCore.OData/Routing/ODataRouteDebugMiddleware.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
using System;
using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
using System.Globalization;
using System.Linq;
using System.Net.Mime;
using System.Text;
Expand Down Expand Up @@ -163,6 +164,7 @@ internal static bool AcceptsJson(IHeaderDictionary headers)
return result;
}

[SuppressMessage("Globalization", "CA1305:Specify IFormatProvider", Justification = "<Pending>")]
Copy link
Contributor

@julealgon julealgon Jan 20, 2022 •
edited by xuzhg
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also missing justification (and also apparently unrelated to the changes). #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.NET 6 introduces a new overload for Append and raises a compile waring to use the old overload.

So, before .NET 6, it's ok to build without a warning. however, there's a build warning after add .NET 6 target framework.

Since I have to add .NET 6 target framework to introduce DateOnly and TimeOnly type, so, a build warning raises.

Originally, i use
#if NET6_0
// call new overload
#else
// call old overload
#endif

But, i think it's easy to suppress the message since that's only for debugging purposes. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.NET 6 introduces a new overload for Append and raises a compile waring to use the old overload.

Ah I see, so that's why it is only raised now. Gotcha.

Originally, i use
#if NET6_0
// call new overload
#else
// call old overload
#endif

But, i think it's easy to suppress the message since that's only for debugging purposes. Thoughts?

I'd agree. The docs for this rule state:

It is safe to suppress a warning from this rule when it is certain that the default format is the correct choice, and where code maintainability is not an important development priority.

I LOL'd at the second part ("nah, this doesn't need to be maintainable" 🤣 ) but the first part says that if you are ok with the defaults it is a safe suppression.

Just make sure to replace the "<Pending>" there with a simple explanation, maybe like "The default format provider is fine here." or something.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took your suggestion. Thanks.

private static void AppendRoute(StringBuilder builder, EndpointRouteInfo routeInfo)
{
builder.Append("<tr>");
Expand Down
Loading