Skip to content
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

Add DateOnly and TimeOnly to MVC Model Binding #45243

Closed
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,8 @@ public void Configure(MvcOptions options)
options.ModelBinderProviders.Add(new ArrayModelBinderProvider());
options.ModelBinderProviders.Add(new CollectionModelBinderProvider());
options.ModelBinderProviders.Add(new ComplexObjectModelBinderProvider());
options.ModelBinderProviders.Add(new DateOnlyModelBinderProvider());
options.ModelBinderProviders.Add(new TimeOnlyModelBinderProvider());
Copy link
Member

Choose a reason for hiding this comment

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

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

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.

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

@brunolins16 brunolins16 Dec 2, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

These are the DatetimeStyles when using the TryParse binder:

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
};

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


// Set up filters
options.Filters.Add(new UnsupportedContentTypeFilter());
Expand Down
104 changes: 104 additions & 0 deletions src/Mvc/Mvc.Core/src/ModelBinding/Binders/DateOnlyModelBinder.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

#nullable enable

using System.Globalization;
using Microsoft.Extensions.Logging;

namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders;

/// <summary>
/// An <see cref="IModelBinder"/> for <see cref="DateOnly"/> and nullable <see cref="DateOnly"/> models.
/// </summary>
public class DateOnlyModelBinder : IModelBinder
{
private readonly DateTimeStyles _supportedStyles;
private readonly ILogger _logger;

/// <summary>
/// Initializes a new instance of <see cref="DateOnlyModelBinder"/>.
/// </summary>
/// <param name="supportedStyles">The <see cref="DateTimeStyles"/>.</param>
/// <param name="loggerFactory">The <see cref="ILoggerFactory"/>.</param>
public DateOnlyModelBinder(DateTimeStyles supportedStyles, ILoggerFactory loggerFactory)
{
if (loggerFactory == null)
{
throw new ArgumentNullException(nameof(loggerFactory));
}

_supportedStyles = supportedStyles;
_logger = loggerFactory.CreateLogger<DateOnlyModelBinder>();
}

/// <inheritdoc />
public Task BindModelAsync(ModelBindingContext bindingContext)
{
if (bindingContext == null)
{
throw new ArgumentNullException(nameof(bindingContext));
}

_logger.AttemptingToBindModel(bindingContext);

var modelName = bindingContext.ModelName;
var valueProviderResult = bindingContext.ValueProvider.GetValue(modelName);
if (valueProviderResult == ValueProviderResult.None)
{
_logger.FoundNoValueInRequest(bindingContext);

// no entry
_logger.DoneAttemptingToBindModel(bindingContext);
return Task.CompletedTask;
}

var modelState = bindingContext.ModelState;
modelState.SetModelValue(modelName, valueProviderResult);

var metadata = bindingContext.ModelMetadata;
var type = metadata.UnderlyingOrModelType;
try
{
var value = valueProviderResult.FirstValue;

object? model;
if (string.IsNullOrWhiteSpace(value))
{
// Parse() method trims the value (with common DateTimeSyles) then throws if the result is empty.
model = null;
}
else if (type == typeof(DateOnly))
{
model = DateOnly.Parse(value, valueProviderResult.Culture, _supportedStyles);
Copy link
Member

Choose a reason for hiding this comment

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

I recommend the TryParse here to avoid the exception

}
else
{
throw new NotSupportedException();
Nick-Stanton marked this conversation as resolved.
Show resolved Hide resolved
}

// When converting value, a null model may indicate a failed conversion for an otherwise required
// model (can't set a ValueType to null). This detects if a null model value is acceptable given the
// current bindingContext. If not, an error is logged.
if (model == null && !metadata.IsReferenceOrNullableType)
{
modelState.TryAddModelError(
modelName,
metadata.ModelBindingMessageProvider.ValueMustNotBeNullAccessor(
valueProviderResult.ToString()));
}
else
{
bindingContext.Result = ModelBindingResult.Success(model);
}
}
catch (Exception exception)
{
// Conversion failed.
modelState.TryAddModelError(modelName, exception, metadata);
}

_logger.DoneAttemptingToBindModel(bindingContext);
return Task.CompletedTask;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

#nullable enable

using System.Globalization;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Logging;

namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders;

/// <summary>
/// An <see cref="IModelBinderProvider"/> for binding <see cref="DateOnly" /> and nullable <see cref="DateOnly"/> models.
/// </summary>
public class DateOnlyModelBinderProvider : IModelBinderProvider
{
internal const DateTimeStyles SupportedStyles = DateTimeStyles.AllowWhiteSpaces;

/// <inheritdoc />
public IModelBinder? GetBinder(ModelBinderProviderContext context)
{
if (context == null)
{
throw new ArgumentNullException(nameof(context));
}
Nick-Stanton marked this conversation as resolved.
Show resolved Hide resolved

var modelType = context.Metadata.UnderlyingOrModelType;
if (modelType == typeof(DateOnly))
{
var loggerFactory = context.Services.GetRequiredService<ILoggerFactory>();
return new DateOnlyModelBinder(SupportedStyles, loggerFactory);
}

return null;
}
}
104 changes: 104 additions & 0 deletions src/Mvc/Mvc.Core/src/ModelBinding/Binders/TimeOnlyModelBinder.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

#nullable enable

using System.Globalization;
using Microsoft.Extensions.Logging;

namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders;

/// <summary>
/// An <see cref="IModelBinder"/> for <see cref="TimeOnly"/> and nullable <see cref="TimeOnly"/> models.
/// </summary>
public class TimeOnlyModelBinder : IModelBinder
{
private readonly DateTimeStyles _supportedStyles;
private readonly ILogger _logger;

/// <summary>
/// Initializes a new instance of <see cref="TimeOnlyModelBinder"/>.
/// </summary>
/// <param name="supportedStyles">The <see cref="DateTimeStyles"/>.</param>
/// <param name="loggerFactory">The <see cref="ILoggerFactory"/>.</param>
public TimeOnlyModelBinder(DateTimeStyles supportedStyles, ILoggerFactory loggerFactory)
TanayParikh marked this conversation as resolved.
Show resolved Hide resolved
{
if (loggerFactory == null)
{
throw new ArgumentNullException(nameof(loggerFactory));
}

_supportedStyles = supportedStyles;
_logger = loggerFactory.CreateLogger<TimeOnlyModelBinder>();
}

/// <inheritdoc />
public Task BindModelAsync(ModelBindingContext bindingContext)
{
if (bindingContext == null)
{
throw new ArgumentNullException(nameof(bindingContext));
}

_logger.AttemptingToBindModel(bindingContext);

var modelName = bindingContext.ModelName;
var valueProviderResult = bindingContext.ValueProvider.GetValue(modelName);
if (valueProviderResult == ValueProviderResult.None)
{
_logger.FoundNoValueInRequest(bindingContext);

// no entry
_logger.DoneAttemptingToBindModel(bindingContext);
return Task.CompletedTask;
}

var modelState = bindingContext.ModelState;
modelState.SetModelValue(modelName, valueProviderResult);

var metadata = bindingContext.ModelMetadata;
var type = metadata.UnderlyingOrModelType;
try
{
var value = valueProviderResult.FirstValue;

object? model;
if (string.IsNullOrWhiteSpace(value))
{
// Parse() method trims the value (with common DateTimeSyles) then throws if the result is empty.
model = null;
}
else if (type == typeof(TimeOnly))
{
model = TimeOnly.Parse(value, valueProviderResult.Culture, _supportedStyles);
Copy link
Member

Choose a reason for hiding this comment

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

I recommend the TryParse here to avoid the exception

}
else
{
throw new NotSupportedException();
}

// When converting value, a null model may indicate a failed conversion for an otherwise required
// model (can't set a ValueType to null). This detects if a null model value is acceptable given the
// current bindingContext. If not, an error is logged.
if (model == null && !metadata.IsReferenceOrNullableType)
{
modelState.TryAddModelError(
modelName,
metadata.ModelBindingMessageProvider.ValueMustNotBeNullAccessor(
valueProviderResult.ToString()));
}
else
{
bindingContext.Result = ModelBindingResult.Success(model);
}
}
catch (Exception exception)
{
// Conversion failed.
modelState.TryAddModelError(modelName, exception, metadata);
}

_logger.DoneAttemptingToBindModel(bindingContext);
return Task.CompletedTask;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

#nullable enable

using System.Globalization;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Logging;

namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders;

/// <summary>
/// An <see cref="IModelBinderProvider"/> for binding <see cref="TimeOnly"/> and nullable <see cref="TimeOnly"/> models.
/// </summary>
public class TimeOnlyModelBinderProvider : IModelBinderProvider
{
internal const DateTimeStyles SupportedStyles = DateTimeStyles.AllowWhiteSpaces;

/// <inheritdoc />
public IModelBinder? GetBinder(ModelBinderProviderContext context)
{
if (context == null)
{
throw new ArgumentNullException(nameof(context));
}

var modelType = context.Metadata.UnderlyingOrModelType;
if (modelType == typeof(TimeOnly))
{
var loggerFactory = context.Services.GetRequiredService<ILoggerFactory>();
return new TimeOnlyModelBinder(SupportedStyles, loggerFactory);
}

return null;
}
}
12 changes: 12 additions & 0 deletions src/Mvc/Mvc.Core/src/PublicAPI.Shipped.txt
Original file line number Diff line number Diff line change
Expand Up @@ -1139,6 +1139,12 @@ Microsoft.AspNetCore.Mvc.ModelBinding.Binders.ComplexObjectModelBinderProvider.G
Microsoft.AspNetCore.Mvc.ModelBinding.Binders.ComplexTypeModelBinder
Microsoft.AspNetCore.Mvc.ModelBinding.Binders.ComplexTypeModelBinderProvider
Microsoft.AspNetCore.Mvc.ModelBinding.Binders.ComplexTypeModelBinderProvider.ComplexTypeModelBinderProvider() -> void
Microsoft.AspNetCore.Mvc.ModelBinding.Binders.DateOnlyModelBinder
Nick-Stanton marked this conversation as resolved.
Show resolved Hide resolved
Microsoft.AspNetCore.Mvc.ModelBinding.Binders.DateOnlyModelBinder.BindModelAsync(Microsoft.AspNetCore.Mvc.ModelBinding.ModelBindingContext! bindingContext) -> System.Threading.Tasks.Task!
Microsoft.AspNetCore.Mvc.ModelBinding.Binders.DateOnlyModelBinder.DateOnlyModelBinder(System.Globalization.DateTimeStyles supportedStyles, Microsoft.Extensions.Logging.ILoggerFactory! loggerFactory) -> void
Microsoft.AspNetCore.Mvc.ModelBinding.Binders.DateOnlyModelBinderProvider
Microsoft.AspNetCore.Mvc.ModelBinding.Binders.DateOnlyModelBinderProvider.DateOnlyModelBinderProvider() -> void
Microsoft.AspNetCore.Mvc.ModelBinding.Binders.DateOnlyModelBinderProvider.GetBinder(Microsoft.AspNetCore.Mvc.ModelBinding.ModelBinderProviderContext! context) -> Microsoft.AspNetCore.Mvc.ModelBinding.IModelBinder?
Microsoft.AspNetCore.Mvc.ModelBinding.Binders.DateTimeModelBinder
Microsoft.AspNetCore.Mvc.ModelBinding.Binders.DateTimeModelBinder.BindModelAsync(Microsoft.AspNetCore.Mvc.ModelBinding.ModelBindingContext! bindingContext) -> System.Threading.Tasks.Task!
Microsoft.AspNetCore.Mvc.ModelBinding.Binders.DateTimeModelBinder.DateTimeModelBinder(System.Globalization.DateTimeStyles supportedStyles, Microsoft.Extensions.Logging.ILoggerFactory! loggerFactory) -> void
Expand Down Expand Up @@ -1206,6 +1212,12 @@ Microsoft.AspNetCore.Mvc.ModelBinding.Binders.SimpleTypeModelBinder.SimpleTypeMo
Microsoft.AspNetCore.Mvc.ModelBinding.Binders.SimpleTypeModelBinderProvider
Microsoft.AspNetCore.Mvc.ModelBinding.Binders.SimpleTypeModelBinderProvider.GetBinder(Microsoft.AspNetCore.Mvc.ModelBinding.ModelBinderProviderContext! context) -> Microsoft.AspNetCore.Mvc.ModelBinding.IModelBinder?
Microsoft.AspNetCore.Mvc.ModelBinding.Binders.SimpleTypeModelBinderProvider.SimpleTypeModelBinderProvider() -> void
Microsoft.AspNetCore.Mvc.ModelBinding.Binders.TimeOnlyModelBinder
Microsoft.AspNetCore.Mvc.ModelBinding.Binders.TimeOnlyModelBinder.BindModelAsync(Microsoft.AspNetCore.Mvc.ModelBinding.ModelBindingContext! bindingContext) -> System.Threading.Tasks.Task!
Microsoft.AspNetCore.Mvc.ModelBinding.Binders.TimeOnlyModelBinder.TimeOnlyModelBinder(System.Globalization.DateTimeStyles supportedStyles, Microsoft.Extensions.Logging.ILoggerFactory! loggerFactory) -> void
Microsoft.AspNetCore.Mvc.ModelBinding.Binders.TimeOnlyModelBinderProvider
Microsoft.AspNetCore.Mvc.ModelBinding.Binders.TimeOnlyModelBinderProvider.TimeOnlyModelBinderProvider() -> void
Microsoft.AspNetCore.Mvc.ModelBinding.Binders.TimeOnlyModelBinderProvider.GetBinder(Microsoft.AspNetCore.Mvc.ModelBinding.ModelBinderProviderContext! context) -> Microsoft.AspNetCore.Mvc.ModelBinding.IModelBinder?
Microsoft.AspNetCore.Mvc.ModelBinding.BindingBehavior
Microsoft.AspNetCore.Mvc.ModelBinding.BindingBehavior.Never = 1 -> Microsoft.AspNetCore.Mvc.ModelBinding.BindingBehavior
Microsoft.AspNetCore.Mvc.ModelBinding.BindingBehavior.Optional = 0 -> Microsoft.AspNetCore.Mvc.ModelBinding.BindingBehavior
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders;

public class DateOnlyModelBinderProviderTest
{
private readonly DateOnlyModelBinderProvider _provider = new DateOnlyModelBinderProvider();

[Theory]
[InlineData(typeof(string))]
[InlineData(typeof(DateTimeOffset))]
[InlineData(typeof(DateTimeOffset?))]
[InlineData(typeof(DateTime))]
[InlineData(typeof(DateTime?))]
[InlineData(typeof(TimeSpan))]
public void Create_ForNonDateOnly_ReturnsNull(Type modelType)
{
// Arrange
var context = new TestModelBinderProviderContext(modelType);

// Act
var result = _provider.GetBinder(context);

// Assert
Assert.Null(result);
}

[Fact]
public void Create_ForDateOnly_ReturnsBinder()
{
// Arrange
var context = new TestModelBinderProviderContext(typeof(DateOnly));

// Act
var result = _provider.GetBinder(context);

// Assert
Assert.IsType<DateOnlyModelBinder>(result);
}

[Fact]
public void Create_ForNullableDateOnly_ReturnsBinder()
{
// Arrange
var context = new TestModelBinderProviderContext(typeof(DateOnly?));

// Act
var result = _provider.GetBinder(context);

// Assert
Assert.IsType<DateOnlyModelBinder>(result);
}
}
Loading