Consider adding support for TryParse as a way to bind primitives

[brunolins16]

Current design

Types that do not contain a Converter from string available are considered Complex Types.

aspnetcore/src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelMetadata.cs

Line 609 in 85ac505

IsComplexType = !TypeDescriptor.GetConverter(ModelType).CanConvertFrom(typeof(string));

That means they will be bind using the ComplexObjectModelBinder when they are not special types that have your own binder (Eg. CancellationToken or Dictionary) or when the [FromBody] was not inferred, in this case the BodyModelBinder will be used.

On the other hand, when it is detected as a simple type (can be converted from string) the SimpleTypeModelBinder will be used, except for the types listed below:

• DateTimeModelBinder => context.Metadata.UnderlyingOrModelType == typeof(DateTime)
• DecimalModelBinder => modelType == typeof(decimal)
• DoubleModelBinder => modelType == typeof(double)
• FloatModelBinder => modelType == typeof(float)
• EnumTypeModelBinder => context.Metadata.IsEnum

Today, this binder will basically use the actual value if the model type is string or call the converter when not.

Proposed Change

There is already the class Microsoft.AspNetCore.Http.ParameterBindingMethodCache exposing two methods HasTryParseMethod and FindTryParseMethod that will be used in this proposal, however, currently it works only with InvariantCulture instead of allowing the caller to provide a different Culture what is required in this proposal and will require a small change to this class.

With the capability to detect the TryParse method, the proposal is change ModelMetadata to include a new internal property HasTryParse, that will indicate that a TryParse method is available with the type.

namespace Microsoft.AspNetCore.Mvc.ModelBinding;

public abstract class ModelMetadata : IEquatable<ModelMetadata?>, IModelMetadataProvider
{
    internal bool HasTryParse { get; private set; }
}

An important aspect of this change, the ModelMetadata will hold a static instance of the Microsoft.AspNetCore.Http.ParameterBindingMethodCache that will be used across all calls.

With this new property available the proposal is to add a new public available ModelBinderProvider that will create a ModelBinder only when ModelMetadata.HasTryParse == true

namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders;

+ public class TryParseModelBinderProvider : IModelBinderProvider
+ {
+     public IModelBinder? GetBinder(ModelBinderProviderContext context!!)
+     { 
+         if (context.Metadata.HasTryParse)
+         {
+             var loggerFactory = context.Services.GetRequiredService<ILoggerFactory>();
+             return new TryParseModelBinder(context.Metadata.ModelType, loggerFactory);
+         }
 
+         return null;
+     }
+ }

namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders;

+ public class TryParseModelBinder : IModelBinder
+ {
+     private readonly Func<ParameterExpression, IFormatProvider, Expression> _tryParseMethodExpession;
+     private readonly ILogger _logger;

+     public TryParseModelBinder(Type type!!, ILoggerFactory loggerFactory!!)
+     {
+         _tryParseMethodExpession = ModelMetadata.FindTryParseMethod(type)!;
+         _logger = loggerFactory.CreateLogger<SimpleTypeModelBinder>();
+     }
+ }

This new provider will be added to the list of the default ModelProviders and executed before the SimpleTypeModelBinderProvider. That means that for all types that contains a TryParse and was processed previously by the SimpleTypeModelBinderProvider because it has a Converter from string will now be bound using the new TryParseModelBinder. A very common example will be int type.

Also, this proposal will not change how types that have a TryParse method but not a Converter from string have their Binding Source inferred, so, for those types of the parameter will need to be explicitly defined the binding source, eg: FromQuery.

Usage Examples

The new behavior will be enabled without any code change, however, the following piece of code will rollback to the previous behavior if the users want to.

services.Configure<MvcOptions>(options => {
    options.ModelBinderProviders.RemoveType<TryParseModelBinderProvider>();
});

[ghost]

Thanks for contacting us.

We're moving this issue to the .NET 7 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

[pranavkm]

• I wouldn't make ParameterBindingMethodCache public.
• You could introduce a TryParseModelBinder instead that is registered to run after SimpleTypeModelBinder. Users can remove it if they're don't want it (thus avoiding the need for a option type).

[brunolins16]

• I wouldn't make ParameterBindingMethodCache public.

I have the same opinion, but I prefer to capture the possibility here for discussion if we should rewrite or reuse. I saw that was decided rewrite most of the code for minimal and probably the same will happen here.

[brunolins16]

• You could introduce a TryParseModelBinder instead that is registered to run after SimpleTypeModelBinder. Users can remove it if they're don't want it (thus avoiding the need for a option type).

I think introduce the TryParseModelBinder is a good idea, and probably the user will not need to remove it most of the time. However, the point to use the option is to allow/or not the new behavior when setting the IsComplexType property. that affects how the parameter is inferred.

An alternative option is creating a new internal property to the ModelMetadata, maybe HasTryParse, that will be set when the TryPare method is detected, and we can condition the TryParseModelBinder execution to that, however, even in this situation to infer the parameter from Path or Query, instead of Body, I fell we would need to add a new option, to the APIBehaviorOptions instead, to allow users to roll back to the previous behavior.

[ghost]

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.

[pranavkm]

API Review:

We could add a new TryParseModelBinderFactory that can be removed by app code. Let's try that out and we can bring this back to API review.

[ghost]

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.

[pranavkm]

API review:

Approved with notes:

• We can start off by leaving the binder internal
• We should seal the binder provider.

+ public sealed class TryParseModelBinderProvider : IModelBinderProvider
+ {
+     public IModelBinder? GetBinder(ModelBinderProviderContext context);
+ }

[brunolins16]

The following update was needed during the implementation and need to be reviewed:

namespace Microsoft.AspNetCore.Mvc;

public class MvcOptions : IEnumerable<ICompatibilitySwitch>
{
+    /// <summary>
+    /// Gets or sets the flag that determines if binding is disabled for Parseable types (contains a TryParse method)
+    /// and will treat them as <see cref="ModelMetadata.IsComplexType"/>, in case, 
+    /// there isn't a <see cref="System.ComponentModel.TypeConverter"/> from <see cref="string"/> registered.
+    /// </summary>
+    public bool SuppressParseableTypesBinding { get; set; }
}

[ghost]

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.

[pranavkm]

API feedback: We are going to punt on the options change until we get more feedback. Users should be able to workaround this by using an explicit FromXXX attribute.