Add support for model binding DateTime as UTC #24893

pranavkm · 2020-08-13T23:26:02Z

This change adds a model binder that produces UTC DateTime values by default. Prior to this change, model bound DateTime values used local times. However, this was inconsistent with JSON formatted values which produces DateTime of UTC kinds.

I'll add a migration note as a follow up with a code sample to revert to the previous behavior.

Fixes #11584

dougbu

/fyi tests still don't work in other time zones

src/Mvc/Mvc.Core/src/ModelBinding/Binders/DateTimeModelBinder.cs

src/Mvc/Mvc.Core/src/ModelBinding/Binders/DateTimeModelBinderProvider.cs

src/Mvc/Mvc.Core/test/ModelBinding/Binders/DateTimeModelBinderTest.cs

dougbu · 2020-08-14T02:18:41Z

Missed your latest commit but my other comments stand

* Cleanup unused exception code path, fix doc comments * Clean up usage of variables * Adjust logging to be consistent

src/Mvc/Mvc.Core/src/ModelBinding/Binders/DateTimeModelBinder.cs

pranavkm · 2020-08-14T16:14:45Z

🆙 📅

pranavkm · 2020-08-14T16:14:51Z

🆙 📅

ghost · 2020-08-14T16:17:11Z

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

dougbu

LGTM

src/Mvc/Mvc.Core/src/ModelBinding/Binders/DateTimeModelBinder.cs

rynowak · 2020-08-16T23:02:36Z

src/Mvc/Mvc.Core/src/ModelBinding/Binders/DateTimeModelBinder.cs

+            var type = metadata.UnderlyingOrModelType;
+            try
+            {
+


rynowak · 2020-08-16T23:03:49Z

src/Mvc/Mvc.Core/src/ModelBinding/Binders/DateTimeModelBinder.cs

+                else
+                {
+                    // unreachable
+                    throw new NotSupportedException();


This type is constructable right? So it isn't really unreachable.

Maybe because this uncommon? IDK, removed it because it didn't really add much value.

captainsafia · 2020-08-17T18:00:22Z

src/Mvc/Mvc.Core/src/ModelBinding/Binders/SimpleTypeModelBinder.cs

@@ -46,6 +46,8 @@ public Task BindModelAsync(ModelBindingContext bindingContext)
                throw new ArgumentNullException(nameof(bindingContext));
            }

+            _logger.AttemptingToBindModel(bindingContext);


Do we need move the logging above the work in other binders?

Of the ones that relied on the value provider, Simple type was the only outlier.

captainsafia · 2020-08-17T18:00:44Z

src/Mvc/Mvc.Core/src/ModelBinding/Binders/FloatModelBinder.cs

@@ -73,7 +72,7 @@ public Task BindModelAsync(ModelBindingContext bindingContext)
                }
                else if (type == typeof(float))
                {
-                    model = float.Parse(value, _supportedStyles, culture);
+                    model = float.Parse(value, _supportedStyles, valueProviderResult.Culture);


Any functional reason for inlining this or just code clarity?

See #24893 (comment)

src/Mvc/Mvc.Core/src/ModelBinding/Binders/DateTimeModelBinder.cs

pranavkm · 2020-08-17T18:16:29Z

src/Mvc/Mvc.Core/src/ModelBinding/Binders/DateTimeModelBinder.cs

+                else
+                {
+                    // unreachable
+                    throw new NotSupportedException();


Maybe because this uncommon? IDK, removed it because it didn't really add much value.

ghost · 2020-08-17T18:18:38Z

Hello @pranavkm!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

davidfowl · 2020-08-17T18:43:38Z

src/Mvc/Mvc.Core/src/ModelBinding/Binders/DateTimeModelBinderProvider.cs

+    /// </summary>
+    public class DateTimeModelBinderProvider : IModelBinderProvider
+    {
+        internal static readonly DateTimeStyles SupportedStyles = DateTimeStyles.AdjustToUniversal | DateTimeStyles.AllowWhiteSpaces;


Should this be configurable?

You can create a binder provider that configures the model binder (it's constructable). It's the pattern we use for other types.

augustoproiete · 2020-11-02T00:38:29Z

I was bitten by this bug today on .NET Core 3.1.403. Is this going to be back-ported to a future 3.1.x release or .NET 5 only?

pranavkm · 2020-11-02T16:59:10Z

@augustoproiete we generally do not backport new features or non-critical bug fixes to earlier releases. You could consider using this PR as a template to copy to your application if you require this to work in your application.

On a side note, it's incredibly easy for us to miss comments on closed issues or PR. For the next time, please consider filing a new issue so there's clear visibility.

Add support for model binding DateTime as UTC

fd50e5d

Fixes #11584

ghost added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Aug 13, 2020

pranavkm requested review from dougbu, javiercn and DamianEdwards August 13, 2020 23:26

pranavkm added this to the 5.0.0-rc1 milestone Aug 13, 2020

Make test work in other TZs

559ad78

dougbu requested changes Aug 14, 2020

View reviewed changes

Changes per PR comments

efe27b0

* Cleanup unused exception code path, fix doc comments * Clean up usage of variables * Adjust logging to be consistent

pranavkm force-pushed the prkrishn/utc-datetime branch from 2e97cb8 to efe27b0 Compare August 14, 2020 14:43

pranavkm commented Aug 14, 2020

View reviewed changes

src/Mvc/Mvc.Core/src/ModelBinding/Binders/DateTimeModelBinder.cs Show resolved Hide resolved

src/Mvc/Mvc.Core/src/ModelBinding/Binders/DateTimeModelBinder.cs Show resolved Hide resolved

pranavkm added the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Aug 14, 2020

dougbu approved these changes Aug 14, 2020

View reviewed changes

src/Mvc/Mvc.Core/src/ModelBinding/Binders/DateTimeModelBinder.cs Show resolved Hide resolved

rynowak reviewed Aug 16, 2020

View reviewed changes

src/Mvc/Mvc.Core/src/ModelBinding/Binders/DateTimeModelBinder.cs Outdated

var type = metadata.UnderlyingOrModelType;

try

{

Copy link

Member

rynowak Aug 16, 2020

Choose a reason for hiding this comment

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

@JamesNK

rynowak reviewed Aug 16, 2020

View reviewed changes

captainsafia reviewed Aug 17, 2020

View reviewed changes

pranavkm added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Aug 17, 2020

pranavkm commented Aug 17, 2020

View reviewed changes

Apply suggestions from code review

70bb448

pranavkm added the auto-merge label Aug 17, 2020

davidfowl reviewed Aug 17, 2020

View reviewed changes

pranavkm changed the base branch from master to release/5.0 August 17, 2020 21:25

ghost merged commit 512a49c into release/5.0 Aug 18, 2020

ghost deleted the prkrishn/utc-datetime branch August 18, 2020 04:04

dotnet locked as resolved and limited conversation to collaborators Nov 2, 2020

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for model binding DateTime as UTC #24893

Add support for model binding DateTime as UTC #24893

pranavkm commented Aug 13, 2020

dougbu left a comment

dougbu commented Aug 14, 2020

pranavkm commented Aug 14, 2020

pranavkm commented Aug 14, 2020

ghost commented Aug 14, 2020

dougbu left a comment

rynowak Aug 16, 2020

rynowak Aug 16, 2020

pranavkm Aug 17, 2020

captainsafia Aug 17, 2020

pranavkm Aug 17, 2020

captainsafia Aug 17, 2020

pranavkm Aug 17, 2020

pranavkm Aug 17, 2020

ghost commented Aug 17, 2020

davidfowl Aug 17, 2020

pranavkm Aug 17, 2020

augustoproiete commented Nov 2, 2020

pranavkm commented Nov 2, 2020

Add support for model binding DateTime as UTC #24893

Add support for model binding DateTime as UTC #24893

Conversation

pranavkm commented Aug 13, 2020

dougbu left a comment

Choose a reason for hiding this comment

dougbu commented Aug 14, 2020

pranavkm commented Aug 14, 2020

pranavkm commented Aug 14, 2020

ghost commented Aug 14, 2020

dougbu left a comment

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

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

Choose a reason for hiding this comment

ghost commented Aug 17, 2020

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

augustoproiete commented Nov 2, 2020

pranavkm commented Nov 2, 2020

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.