Support DateOnly and TimeOnly #450

xuzhg · 2022-01-20T03:18:56Z

Support DateOnly & TimeOnly new types defined in .NET 6.

Add the .NET 6.0 target framework to introduce new types (DateOnly and TimeOnly)
Support serialize of DateOnly & TimeOnly as "Date" and "TimeOfDay"
Support deserialize of DateOnly & TimeOnly from "Date" and "TimeOfDay".
Support the query option on DateOnly & TimeOnly, for example $filter.

To be clear: EF core 6.0 doesn't support DateOnly and TimeOnly. See dotnet/efcore#24507

julealgon · 2022-01-20T17:48:18Z

src/Microsoft.AspNetCore.OData/Common/TypeHelper.cs

@@ -90,6 +90,19 @@ public static bool IsDateTime(Type clrType)
            return Type.GetTypeCode(underlyingTypeOrSelf) == TypeCode.DateTime;
        }

+#if NET6_0


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

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.

julealgon · 2022-01-20T17:51:47Z

src/Microsoft.AspNetCore.OData/Common/TypeHelper.cs

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


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

julealgon · 2022-01-20T17:54:34Z

src/Microsoft.AspNetCore.OData/Common/TypeHelper.cs

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


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

julealgon · 2022-01-20T17:56:01Z

src/Microsoft.AspNetCore.OData/Common/TypeHelper.cs

+            Type underlyingTypeOrSelf = GetUnderlyingTypeOrSelf(clrType);
+            return underlyingTypeOrSelf == typeof(TimeOnly);
+        }
+#endif


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

julealgon · 2022-01-20T18:03:38Z

src/Microsoft.AspNetCore.OData/Edm/DefaultODataTypeMapper.cs

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


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

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

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.

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

julealgon · 2022-01-20T18:55:36Z

src/Microsoft.AspNetCore.OData/Query/Expressions/QueryBinder.SingleValueFunctionCall.cs

@@ -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))


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

:) I have the same concern.

julealgon · 2022-01-20T18:55:54Z

src/Microsoft.AspNetCore.OData/Query/Expressions/QueryBinder.SingleValueFunctionCall.cs

@@ -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)));
+


Unintended line break? #Resolved

julealgon · 2022-01-20T18:56:51Z

src/Microsoft.AspNetCore.OData/Query/Expressions/QueryBinder.SingleValueFunctionCall.cs

@@ -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))


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

julealgon · 2022-01-20T19:01:21Z

src/Microsoft.AspNetCore.OData/Routing/ODataRouteDebugMiddleware.cs

@@ -163,6 +164,7 @@ internal static bool AcceptsJson(IHeaderDictionary headers)
            return result;
        }

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


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

.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?

.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.

Took your suggestion. Thanks.

julealgon · 2022-01-20T19:02:46Z

test/Microsoft.AspNetCore.OData.E2E.Tests/Microsoft.AspNetCore.OData.E2E.Tests.csproj

+  <ItemGroup Condition="'$(TargetFramework)' == 'net6.0' ">
+    <PackageReference Include="Microsoft.EntityFrameworkCore" Version="6.0.0" />
+    <PackageReference Include="Microsoft.EntityFrameworkCore.InMemory" Version="6.0.0" />
+    <PackageReference Include="Microsoft.EntityFrameworkCore.SqlServer" Version="6.0.0" />
+    <PackageReference Include="Microsoft.EntityFrameworkCore.Proxies" Version="6.0.0" />
+  </ItemGroup>


Considering none of the changes related to TimeOnly and DateOnly have any relationship to EF (I'm assuming this), do we really need to do this? #WontFix

Oh, I originally thought There were many requirements for DateOnly and TimeOnly because EF Core supports them.

So, when I first draft the e2e, I assume to use EF Core 6.0. However, it seems EF Core doesn't support them until EF Core 7.

So, I forgot to remove them. Thanks.

Of course, there's a workaround to map the DateOnly and TimeOnly to "date" and "time". I'd like to try that.

Also, i think it's ok to add them since I added .NET 5 target framework, and make sure the test running under .NET 6 runtime using the 6.0 EF core nuget.

julealgon · 2022-01-20T19:06:03Z

You might also want to add a comment to the description to hard link this PR with #413.

xuzhg · 2022-01-20T19:52:02Z

I already added the link between issue and pr. did i miss something?

In reply to: 1017831541

@juliano

* Support DateOnly and TimeOnly * Update the nuget nuspec to include .NET 6 target framework * Resolve review comment. Thanks @juliano Leal Goncalves

Support DateOnly and TimeOnly

b2b5fdb

xuzhg mentioned this pull request Jan 20, 2022

Support for DateOnly and TimeOnly types #413

Closed

Update the nuget nuspec to include .NET 6 target framework

85324e4

julealgon suggested changes Jan 20, 2022

View reviewed changes

xuzhg linked an issue Jan 20, 2022 that may be closed by this pull request

Support for DateOnly and TimeOnly types #413

Closed

xuzhg requested review from corranrogue9, gathogojr and mikepizzo January 20, 2022 19:32

Resolve review comment. Thanks @juliano Leal Goncalves

090624a

xuzhg merged commit dad7c4c into main Jan 21, 2022

xuzhg deleted the DateTimeOnlyNet6 branch January 21, 2022 07:37

renovate bot mentioned this pull request Feb 1, 2022

chore(*): Update dependency Microsoft.AspNetCore.OData to v8.0.7 Liu233w/acm-statistics#2430

Merged

1 task

habbes mentioned this pull request May 7, 2025

Configuring CreateDateBinaryExpression Behavior #1473

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support DateOnly and TimeOnly #450

Support DateOnly and TimeOnly #450

xuzhg commented Jan 20, 2022 •

edited

Loading

julealgon Jan 20, 2022 •

edited by xuzhg

Loading

xuzhg Jan 20, 2022

julealgon Jan 20, 2022 •

edited by xuzhg

Loading

julealgon Jan 20, 2022 •

edited by xuzhg

Loading

julealgon Jan 20, 2022 •

edited by xuzhg

Loading

julealgon Jan 20, 2022 •

edited by xuzhg

Loading

xuzhg Jan 20, 2022

julealgon Jan 20, 2022 •

edited

Loading

xuzhg Jan 20, 2022

julealgon Jan 20, 2022 •

edited by xuzhg

Loading

xuzhg Jan 20, 2022

julealgon Jan 20, 2022 •

edited by xuzhg

Loading

julealgon Jan 20, 2022 •

edited by xuzhg

Loading

julealgon Jan 20, 2022 •

edited by xuzhg

Loading

xuzhg Jan 20, 2022

julealgon Jan 20, 2022

xuzhg Jan 20, 2022

julealgon Jan 20, 2022 •

edited by xuzhg

Loading

xuzhg Jan 20, 2022

xuzhg Jan 21, 2022

julealgon commented Jan 20, 2022

xuzhg commented Jan 20, 2022

Support DateOnly and TimeOnly #450

Support DateOnly and TimeOnly #450

Conversation

xuzhg commented Jan 20, 2022 • edited Loading

julealgon Jan 20, 2022 • edited by xuzhg Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julealgon Jan 20, 2022 • edited by xuzhg Loading

Choose a reason for hiding this comment

julealgon Jan 20, 2022 • edited by xuzhg Loading

Choose a reason for hiding this comment

julealgon Jan 20, 2022 • edited by xuzhg Loading

Choose a reason for hiding this comment

julealgon Jan 20, 2022 • edited by xuzhg Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julealgon Jan 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julealgon Jan 20, 2022 • edited by xuzhg Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julealgon Jan 20, 2022 • edited by xuzhg Loading

Choose a reason for hiding this comment

julealgon Jan 20, 2022 • edited by xuzhg Loading

Choose a reason for hiding this comment

julealgon Jan 20, 2022 • edited by xuzhg Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julealgon Jan 20, 2022 • edited by xuzhg Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julealgon commented Jan 20, 2022

xuzhg commented Jan 20, 2022

xuzhg commented Jan 20, 2022 •

edited

Loading

julealgon Jan 20, 2022 •

edited by xuzhg

Loading

julealgon Jan 20, 2022 •

edited by xuzhg

Loading

julealgon Jan 20, 2022 •

edited by xuzhg

Loading

julealgon Jan 20, 2022 •

edited by xuzhg

Loading

julealgon Jan 20, 2022 •

edited by xuzhg

Loading

julealgon Jan 20, 2022 •

edited

Loading

julealgon Jan 20, 2022 •

edited by xuzhg

Loading

julealgon Jan 20, 2022 •

edited by xuzhg

Loading

julealgon Jan 20, 2022 •

edited by xuzhg

Loading

julealgon Jan 20, 2022 •

edited by xuzhg

Loading

julealgon Jan 20, 2022 •

edited by xuzhg

Loading