Add DateOnly and TimeOnly to MVC Model Binding by Nick-Stanton · Pull Request #45243 · dotnet/aspnetcore

Nick-Stanton · 2022-11-22T22:39:32Z

Add DateOnly and TimeOnly to MVC Model Binding

Adds nullable DateOnly and TimeOnly to list of model binding simple types.

Description

Part of the effort tracked in #34591 to add DateOnly and TimeOnly support to Model binding as well as Blazor/regular routing.

The logic almost exactly matches the implementation for DateTime with the exception of DateTimeStyles.AdjustToUniversal being included as a supported style as this style is not supported by the DateOnly and TimeOnly types.

On the completion and merge of this PR, I will create an additional PR to add DateOnly and TimeOnly to our list of supported types.

TanayParikh

Congrats on your first PR in aspnetcore! 🎉

Nick-Stanton · 2022-11-30T21:13:15Z

Adding the blocked label pending the approval and resolution of #45376.

halter73 · 2022-12-01T01:42:35Z

        options.ModelBinderProviders.Add(new CollectionModelBinderProvider());
        options.ModelBinderProviders.Add(new ComplexObjectModelBinderProvider());
+        options.ModelBinderProviders.Add(new DateOnlyModelBinderProvider());
+        options.ModelBinderProviders.Add(new TimeOnlyModelBinderProvider());


Can these new IModelBinderProviders ever get hit outside of unit tests? I cloned this PR and added the following lines to MvcSandbox's HomeController.

aspnetcore/src/Mvc/samples/MvcSandbox/Controllers/HomeController.cs

Lines 8 to 17 in 63c4948

public class HomeController : Controller

{

[ModelBinder]

public string Id { get; set; }

public IActionResult Index()

{

return View();

}

}

[ModelBinder] public TimeOnly Time { get; set; }

And then made a request to /?Id=3&Time=12:34:56%20PM and added breakpoints. The Time got populated by the TryParseModelBinderProvider added by @brunolins16 in #40233. Since that's added before these providers, it gets first crack at binding DateOnly and TimeOnly.

For this reason and to maintain back compat, we put TryParseModelBinderProvider after DateTimeModelBinderProvider. Even if we remove the TryParseModelBinderProvider, the SimpleTypeModelBinderProvider would handle this first as well using TypeConverter.

aspnetcore/src/Mvc/Mvc.Core/src/Infrastructure/MvcCoreMvcOptionsSetup.cs

Lines 65 to 67 in 63c4948

options.ModelBinderProviders.Add(new DateTimeModelBinderProvider());

options.ModelBinderProviders.Add(new TryParseModelBinderProvider());

options.ModelBinderProviders.Add(new SimpleTypeModelBinderProvider());

Since we don't have any back compat concerns for DateOnly and TimeOnly, I'm not sure we need these. It does allow customizing DateTimeStyles if you construct the providers yourself and add them manually, but I don't like having to maintain providers for each possible type.

@halter73 Are there any differences in behavior between the TryParse implementation and the concrete modelbinders? Or any other concern like perf? If the TryParse implementation supports all of them, should we get rid of the DateTime and DateTimeOffset modelbinders?

I would like to get rid of the DateTime and DateTimeOffset model binders, but I don't think it's worth potentially breaking working code. @brunolins16 What do you think?

@halter73 I do not think most people will change those binders directly. We can remove them from 8.0, make an announcement in preview1 and see if someone is broken by it.

As reported here , it could potentially be break and was decided (I don't have the historical reason) to create specific binders for them even though the simpletypebinder supported them a long ago. That said I believe this is a worthy investigation and we don't need to delete them and instead just remove the default registration and let the binder available for manual registration.

Here is a potential reason why the specific binders were created #34591 (comment)

Great point, this was an essential part of getting DateTime binding correct. That being said, DateTimeStyles.AdjustToUniversal is not supported for DateOnly and TimeOnly so the same requirement is not there.

Yeah, the only benefit I see in have those two new binders, is allow users to customize the DateTimeStyles creating their own ModelBinderProviders.

Suggestion 💡: What if we add those two new Binder to allow users customization, without the Providers (no registration), and let the TryParse/Converter binder do the work by default.

These are the DatetimeStyles when using the TryParse binder:

aspnetcore/src/Shared/ParameterBindingMethodCache.cs

Lines 130 to 135 in 5c62d0f

var dateTimeStyles = type switch

{

Type t when t == typeof(DateTime) => DateTimeStyles.AdjustToUniversal | DateTimeStyles.AllowWhiteSpaces,

Type t when t == typeof(DateTimeOffset) => DateTimeStyles.AssumeUniversal | DateTimeStyles.AllowWhiteSpaces,

_ => DateTimeStyles.AllowWhiteSpaces

};

Yep, DateTimeStyles.AllowWhiteSpaces is what I currently have for these proposed providers. While writing them I found that the only supported styles for TimeOnly and DateOnly were the ones pertaining to white space, but I can't seem to find a doc page to back that up.

Also, do you think there's any gain/loss when TryParse is used over any of the other providers/binders? I see that TryParse goes before SimpleType.

javiercn

Overall looks great, I think we are missing some E2E test coverage, as @halter73 pointed out.

brunolins16 · 2022-12-02T14:29:45Z

On the completion and merge of this PR, I will create an additional PR to add DateOnly and TimeOnly to our list of supported types.

Thanks for that but those types are already simple types since this definition is not based on the Converter or TryParse methods .

ghost · 2022-12-02T14:29:50Z

Hi @brunolins16. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

brunolins16 · 2022-12-02T14:30:09Z

Sorry closed by mistake.

Nick-Stanton · 2022-12-02T15:48:09Z

On the completion and merge of this PR, I will create an additional PR to add DateOnly and TimeOnly to our list of supported types.

Thanks for that but those types are already simple types since this definition is not based on the Converter or TryParse methods .

@brunolins16 Yep, sounds like I need to make that docs PR regardless of the outcome of this one.

brunolins16 · 2022-12-02T18:09:26Z

+            }
+            else if (type == typeof(DateOnly))
+            {
+                model = DateOnly.Parse(value, valueProviderResult.Culture, _supportedStyles);


I recommend the TryParse here to avoid the exception

brunolins16 · 2022-12-02T18:09:56Z

+            }
+            else if (type == typeof(TimeOnly))
+            {
+                model = TimeOnly.Parse(value, valueProviderResult.Culture, _supportedStyles);


I recommend the TryParse here to avoid the exception

Nick-Stanton · 2023-01-09T22:39:45Z

Closing this PR since TryParseModelBinder already covers this use case with no additional API required

Nick-Stanton added 3 commits November 21, 2022 15:27

Add TimeOnly and DateOnly bind availability

5474e5d

Merge branch 'dotnet:main' into DateOnlyTimeOnlyBinding

5534faf

Add test coverage for binders and binderproviders

cff11c5

Nick-Stanton requested review from a team as code owners November 22, 2022 22:39

ghost added the old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels label Nov 22, 2022

danmoseley reviewed Nov 22, 2022

View reviewed changes

Comment thread src/Mvc/Mvc.Core/src/ModelBinding/Binders/DateOnlyModelBinderProvider.cs Outdated

TanayParikh reviewed Nov 23, 2022

View reviewed changes

Comment thread src/Mvc/Mvc.Core/src/ModelBinding/Binders/TimeOnlyModelBinder.cs

Eilon reviewed Nov 23, 2022

View reviewed changes

Comment thread src/Mvc/Mvc.Core/src/ModelBinding/Binders/DateOnlyModelBinder.cs

BrennanConroy reviewed Nov 28, 2022

View reviewed changes

Comment thread src/Mvc/Mvc.Core/src/PublicAPI.Shipped.txt Outdated

Nick-Stanton added 2 commits November 28, 2022 11:48

Move new API to unshipped file

50beacf

Replace null exception throwing

ed90b05

Nick-Stanton mentioned this pull request Nov 30, 2022

Model Binding Support for TimeOnly and DateOnly #45376

Closed

Nick-Stanton linked an issue Nov 30, 2022 that may be closed by this pull request

Model Binding Support for TimeOnly and DateOnly #45376

Closed

Nick-Stanton added the blocked The work on this issue is blocked due to some dependency label Nov 30, 2022

halter73 reviewed Dec 1, 2022

View reviewed changes

javiercn reviewed Dec 1, 2022

View reviewed changes

brunolins16 closed this Dec 2, 2022

brunolins16 reopened this Dec 2, 2022

brunolins16 reviewed Dec 2, 2022

View reviewed changes

Nick-Stanton mentioned this pull request Dec 7, 2022

Add DateOnly and TimeOnly support to model binding & routing #34591

Closed

3 tasks

Nick-Stanton closed this Jan 9, 2023

Nick-Stanton deleted the DateOnlyTimeOnlyBinding branch January 9, 2023 22:39

	public class HomeController : Controller
	{
	[ModelBinder]
	public string Id { get; set; }

	public IActionResult Index()
	{
	return View();
	}
	}

	options.ModelBinderProviders.Add(new DateTimeModelBinderProvider());
	options.ModelBinderProviders.Add(new TryParseModelBinderProvider());
	options.ModelBinderProviders.Add(new SimpleTypeModelBinderProvider());

	var dateTimeStyles = type switch
	{
	Type t when t == typeof(DateTime) => DateTimeStyles.AdjustToUniversal \| DateTimeStyles.AllowWhiteSpaces,
	Type t when t == typeof(DateTimeOffset) => DateTimeStyles.AssumeUniversal \| DateTimeStyles.AllowWhiteSpaces,
	_ => DateTimeStyles.AllowWhiteSpaces
	};

Conversation

Nick-Stanton commented Nov 22, 2022

Add DateOnly and TimeOnly to MVC Model Binding

Description

Uh oh!

Uh oh!

TanayParikh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Nick-Stanton commented Nov 30, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brunolins16 Dec 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javiercn left a comment

Choose a reason for hiding this comment

Uh oh!

brunolins16 commented Dec 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Dec 2, 2022

Uh oh!

brunolins16 commented Dec 2, 2022

Uh oh!

Nick-Stanton commented Dec 2, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nick-Stanton commented Jan 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

brunolins16 Dec 2, 2022 •

edited

Loading

brunolins16 commented Dec 2, 2022 •

edited

Loading