From df4b6c835c56b3d661c96043b00f1df3ee96d414 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Fri, 11 Feb 2022 13:15:37 -0800 Subject: [PATCH 01/31] New tryparse modelbinder --- ...crosoft.AspNetCore.Mvc.Abstractions.csproj | 5 + .../src/ModelBinding/ModelMetadata.cs | 12 ++ .../Infrastructure/MvcCoreMvcOptionsSetup.cs | 1 + .../src/Microsoft.AspNetCore.Mvc.Core.csproj | 8 +- .../Binders/TryParseModelBinder.cs | 116 ++++++++++++++++++ .../Binders/TryParseModelBinderProvider.cs | 32 +++++ src/Mvc/Mvc.Core/src/PublicAPI.Unshipped.txt | 6 + 7 files changed, 178 insertions(+), 2 deletions(-) create mode 100644 src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinder.cs create mode 100644 src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinderProvider.cs diff --git a/src/Mvc/Mvc.Abstractions/src/Microsoft.AspNetCore.Mvc.Abstractions.csproj b/src/Mvc/Mvc.Abstractions/src/Microsoft.AspNetCore.Mvc.Abstractions.csproj index b277931317a3..eaa569a06e3c 100644 --- a/src/Mvc/Mvc.Abstractions/src/Microsoft.AspNetCore.Mvc.Abstractions.csproj +++ b/src/Mvc/Mvc.Abstractions/src/Microsoft.AspNetCore.Mvc.Abstractions.csproj @@ -15,6 +15,11 @@ Microsoft.AspNetCore.Mvc.IActionResult + + + + + diff --git a/src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelMetadata.cs b/src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelMetadata.cs index bbcf91a97eda..db58e37e28ae 100644 --- a/src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelMetadata.cs +++ b/src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelMetadata.cs @@ -6,7 +6,9 @@ using System.Diagnostics; using System.Diagnostics.CodeAnalysis; using System.Linq; +using System.Linq.Expressions; using System.Reflection; +using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc.Abstractions; using Microsoft.AspNetCore.Mvc.ModelBinding.Metadata; using Microsoft.AspNetCore.Mvc.ModelBinding.Validation; @@ -25,6 +27,8 @@ public abstract class ModelMetadata : IEquatable, IModelMetadata /// public static readonly int DefaultOrder = 10000; + private static readonly ParameterBindingMethodCache ParameterBindingMethodCache = new(); + private int? _hashCode; private IReadOnlyList? _boundProperties; private IReadOnlyDictionary? _parameterMapping; @@ -475,6 +479,11 @@ internal IReadOnlyDictionary BoundConstructorPrope /// public Type UnderlyingOrModelType { get; private set; } = default!; + /// + /// + /// + internal bool HasTryParse { get; private set; } = default!; + /// /// Gets a value indicating the NullabilityState of the value or reference type. /// @@ -520,6 +529,8 @@ internal void ThrowIfRecordTypeHasValidationOnProperties() throw _recordTypeValidatorsOnPropertiesError; } } + internal static Func? FindTryParseMethod(Type modelType) + => ParameterBindingMethodCache.FindTryParseMethod(modelType); [MemberNotNull(nameof(_parameterMapping), nameof(_boundConstructorPropertyMapping))] private void CalculateRecordTypeConstructorDetails() @@ -620,6 +631,7 @@ private void InitializeTypeInformation() IsNullableValueType = Nullable.GetUnderlyingType(ModelType) != null; IsReferenceOrNullableType = !ModelType.IsValueType || IsNullableValueType; UnderlyingOrModelType = Nullable.GetUnderlyingType(ModelType) ?? ModelType; + HasTryParse = ParameterBindingMethodCache.HasTryParseMethod(ModelType); var collectionType = ClosedGenericMatcher.ExtractGenericInterface(ModelType, typeof(ICollection<>)); IsCollectionType = collectionType != null; diff --git a/src/Mvc/Mvc.Core/src/Infrastructure/MvcCoreMvcOptionsSetup.cs b/src/Mvc/Mvc.Core/src/Infrastructure/MvcCoreMvcOptionsSetup.cs index 652af4ed3507..449597cbf000 100644 --- a/src/Mvc/Mvc.Core/src/Infrastructure/MvcCoreMvcOptionsSetup.cs +++ b/src/Mvc/Mvc.Core/src/Infrastructure/MvcCoreMvcOptionsSetup.cs @@ -63,6 +63,7 @@ public void Configure(MvcOptions options) options.ModelBinderProviders.Add(new FloatingPointTypeModelBinderProvider()); options.ModelBinderProviders.Add(new EnumTypeModelBinderProvider(options)); options.ModelBinderProviders.Add(new DateTimeModelBinderProvider()); + options.ModelBinderProviders.Add(new TryParseModelBinderProvider()); options.ModelBinderProviders.Add(new SimpleTypeModelBinderProvider()); options.ModelBinderProviders.Add(new CancellationTokenModelBinderProvider()); options.ModelBinderProviders.Add(new ByteArrayModelBinderProvider()); diff --git a/src/Mvc/Mvc.Core/src/Microsoft.AspNetCore.Mvc.Core.csproj b/src/Mvc/Mvc.Core/src/Microsoft.AspNetCore.Mvc.Core.csproj index a0b45ab9d83a..0989c99b8956 100644 --- a/src/Mvc/Mvc.Core/src/Microsoft.AspNetCore.Mvc.Core.csproj +++ b/src/Mvc/Mvc.Core/src/Microsoft.AspNetCore.Mvc.Core.csproj @@ -28,8 +28,8 @@ Microsoft.AspNetCore.Mvc.RouteAttribute - - + + @@ -61,4 +61,8 @@ Microsoft.AspNetCore.Mvc.RouteAttribute + + + + diff --git a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinder.cs b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinder.cs new file mode 100644 index 000000000000..b7e9351df5e9 --- /dev/null +++ b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinder.cs @@ -0,0 +1,116 @@ +// 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.Linq.Expressions; +using Microsoft.AspNetCore.Http; +using Microsoft.Extensions.Logging; + +namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders; + +/// +/// An for simple types. +/// +public class TryParseModelBinder : IModelBinder +{ + private readonly Func _tryParseMethodExpession; + private readonly ILogger _logger; + + /// + /// Initializes a new instance of . + /// + /// The type to create binder for. + /// The . + public TryParseModelBinder(Type type, ILoggerFactory loggerFactory) + { + if (type == null) + { + throw new ArgumentNullException(nameof(type)); + } + + if (loggerFactory == null) + { + throw new ArgumentNullException(nameof(loggerFactory)); + } + + _tryParseMethodExpession = ModelMetadata.FindTryParseMethod(type)!; + _logger = loggerFactory.CreateLogger(); + } + + /// + public Task BindModelAsync(ModelBindingContext bindingContext) + { + if (bindingContext == null) + { + throw new ArgumentNullException(nameof(bindingContext)); + } + + _logger.AttemptingToBindModel(bindingContext); + + var valueProviderResult = bindingContext.ValueProvider.GetValue(bindingContext.ModelName); + if (valueProviderResult == ValueProviderResult.None) + { + _logger.FoundNoValueInRequest(bindingContext); + + // no entry + _logger.DoneAttemptingToBindModel(bindingContext); + return Task.CompletedTask; + } + + bindingContext.ModelState.SetModelValue(bindingContext.ModelName, valueProviderResult); + + try + { + var value = valueProviderResult.FirstValue; + + object? model = null; + if (string.IsNullOrWhiteSpace(value)) + { + // Other than the StringConverter, converters Trim() the value then throw if the result is empty. + model = null; + } + else + { + var parsedValue = Expression.Variable(bindingContext.ModelType, "parsedValue"); + var modelValue = Expression.Variable(typeof(object), "model"); + + var expression = Expression.Block( + new[] { parsedValue, modelValue, ParameterBindingMethodCache.TempSourceStringExpr }, + Expression.Assign(ParameterBindingMethodCache.TempSourceStringExpr, Expression.Constant(value!)), + Expression.IfThenElse(_tryParseMethodExpession(parsedValue, valueProviderResult.Culture), + Expression.Assign(modelValue, Expression.Convert(parsedValue, modelValue.Type)), + Expression.Throw(Expression.Constant(new FormatException()))), + modelValue); + + model = Expression.Lambda>(expression).Compile()(); + } + + // When converting newModel a null value 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 && !bindingContext.ModelMetadata.IsReferenceOrNullableType) + { + bindingContext.ModelState.TryAddModelError( + bindingContext.ModelName, + bindingContext.ModelMetadata.ModelBindingMessageProvider.ValueMustNotBeNullAccessor( + valueProviderResult.ToString())); + } + else + { + bindingContext.Result = ModelBindingResult.Success(model); + } + } + catch (Exception exception) + { + // Conversion failed. + bindingContext.ModelState.TryAddModelError( + bindingContext.ModelName, + exception, + bindingContext.ModelMetadata); + } + + _logger.DoneAttemptingToBindModel(bindingContext); + return Task.CompletedTask; + } +} diff --git a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinderProvider.cs b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinderProvider.cs new file mode 100644 index 000000000000..8a05614988b4 --- /dev/null +++ b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinderProvider.cs @@ -0,0 +1,32 @@ +// 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 Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging; + +namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders; + +/// +/// An for binding types that have a TryParse method. +/// +public class TryParseModelBinderProvider : IModelBinderProvider +{ + /// + public IModelBinder? GetBinder(ModelBinderProviderContext context) + { + if (context == null) + { + throw new ArgumentNullException(nameof(context)); + } + + if (context.Metadata.HasTryParse) + { + var loggerFactory = context.Services.GetRequiredService(); + return new TryParseModelBinder(context.Metadata.ModelType, loggerFactory); + } + + return null; + } +} diff --git a/src/Mvc/Mvc.Core/src/PublicAPI.Unshipped.txt b/src/Mvc/Mvc.Core/src/PublicAPI.Unshipped.txt index d326535ca4d1..0c68147a9f09 100644 --- a/src/Mvc/Mvc.Core/src/PublicAPI.Unshipped.txt +++ b/src/Mvc/Mvc.Core/src/PublicAPI.Unshipped.txt @@ -1,4 +1,10 @@ #nullable enable +Microsoft.AspNetCore.Mvc.ModelBinding.Binders.TryParseModelBinder +Microsoft.AspNetCore.Mvc.ModelBinding.Binders.TryParseModelBinder.BindModelAsync(Microsoft.AspNetCore.Mvc.ModelBinding.ModelBindingContext! bindingContext) -> System.Threading.Tasks.Task! +Microsoft.AspNetCore.Mvc.ModelBinding.Binders.TryParseModelBinder.TryParseModelBinder(System.Type! type, Microsoft.Extensions.Logging.ILoggerFactory! loggerFactory) -> void +Microsoft.AspNetCore.Mvc.ModelBinding.Binders.TryParseModelBinderProvider +Microsoft.AspNetCore.Mvc.ModelBinding.Binders.TryParseModelBinderProvider.GetBinder(Microsoft.AspNetCore.Mvc.ModelBinding.ModelBinderProviderContext! context) -> Microsoft.AspNetCore.Mvc.ModelBinding.IModelBinder? +Microsoft.AspNetCore.Mvc.ModelBinding.Binders.TryParseModelBinderProvider.TryParseModelBinderProvider() -> void Microsoft.AspNetCore.Mvc.ModelBinding.Metadata.SystemTextJsonValidationMetadataProvider Microsoft.AspNetCore.Mvc.ModelBinding.Metadata.SystemTextJsonValidationMetadataProvider.CreateDisplayMetadata(Microsoft.AspNetCore.Mvc.ModelBinding.Metadata.DisplayMetadataProviderContext! context) -> void Microsoft.AspNetCore.Mvc.ModelBinding.Metadata.SystemTextJsonValidationMetadataProvider.CreateValidationMetadata(Microsoft.AspNetCore.Mvc.ModelBinding.Metadata.ValidationMetadataProviderContext! context) -> void From a695fec1beba3512969b49cac046c4eb98f5893f Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Fri, 11 Feb 2022 13:15:51 -0800 Subject: [PATCH 02/31] Adding IformatProvider parameter --- .../src/RequestDelegateFactory.cs | 2 +- src/Shared/ParameterBindingMethodCache.cs | 24 +++++++++---------- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index 954e8c94a082..a4e559018256 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -992,7 +992,7 @@ private static Expression BindParameterFromValue(ParameterInfo parameter, Expres HttpContextExpr, parameterTypeNameConstant, parameterNameConstant, TempSourceStringExpr, Expression.Constant(factoryContext.ThrowOnBadRequest))); - var tryParseCall = tryParseMethodCall(parsedValue); + var tryParseCall = tryParseMethodCall(parsedValue, CultureInfo.InvariantCulture); // The following code is generated if the parameter is required and // the method should not be matched. diff --git a/src/Shared/ParameterBindingMethodCache.cs b/src/Shared/ParameterBindingMethodCache.cs index 810dd7923618..77b2e3c8cbf2 100644 --- a/src/Shared/ParameterBindingMethodCache.cs +++ b/src/Shared/ParameterBindingMethodCache.cs @@ -28,7 +28,7 @@ internal sealed class ParameterBindingMethodCache private readonly MethodInfo _enumTryParseMethod; // Since this is shared source, the cache won't be shared between RequestDelegateFactory and the ApiDescriptionProvider sadly :( - private readonly ConcurrentDictionary?> _stringMethodCallCache = new(); + private readonly ConcurrentDictionary?> _stringMethodCallCache = new(); private readonly ConcurrentDictionary?, int)> _bindAsyncMethodCallCache = new(); // If IsDynamicCodeSupported is false, we can't use the static Enum.TryParse since there's no easy way for @@ -52,9 +52,9 @@ public bool HasTryParseMethod(Type type) public bool HasBindAsyncMethod(ParameterInfo parameter) => FindBindAsyncMethod(parameter).Expression is not null; - public Func? FindTryParseMethod(Type type) + public Func? FindTryParseMethod(Type type) { - Func? Finder(Type type) + Func? Finder(Type type) { MethodInfo? methodInfo; @@ -64,10 +64,10 @@ public bool HasBindAsyncMethod(ParameterInfo parameter) => { methodInfo = _enumTryParseMethod.MakeGenericMethod(type); - return (expression) => Expression.Call(methodInfo!, TempSourceStringExpr, expression); + return (expression, formatProvider) => Expression.Call(methodInfo!, TempSourceStringExpr, expression); } - return (expression) => + return (expression, formatProvider) => { var enumAsObject = Expression.Variable(typeof(object), "enumAsObject"); var success = Expression.Variable(typeof(bool), "success"); @@ -108,21 +108,21 @@ public bool HasBindAsyncMethod(ParameterInfo parameter) => _ => DateTimeStyles.AllowWhiteSpaces }; - return (expression) => Expression.Call( + return (expression, formatProvider) => Expression.Call( methodInfo!, TempSourceStringExpr, - Expression.Constant(CultureInfo.InvariantCulture), + Expression.Constant(formatProvider), Expression.Constant(dateTimeStyles), expression); } if (TryGetNumberStylesTryGetMethod(type, out methodInfo, out var numberStyle)) { - return (expression) => Expression.Call( + return (expression, formatProvider) => Expression.Call( methodInfo!, TempSourceStringExpr, Expression.Constant(numberStyle), - Expression.Constant(CultureInfo.InvariantCulture), + Expression.Constant(formatProvider), expression); } @@ -130,10 +130,10 @@ public bool HasBindAsyncMethod(ParameterInfo parameter) => if (methodInfo is not null) { - return (expression) => Expression.Call( + return (expression, formatProvider) => Expression.Call( methodInfo, TempSourceStringExpr, - Expression.Constant(CultureInfo.InvariantCulture), + Expression.Constant(formatProvider), expression); } @@ -141,7 +141,7 @@ public bool HasBindAsyncMethod(ParameterInfo parameter) => if (methodInfo is not null) { - return (expression) => Expression.Call(methodInfo, TempSourceStringExpr, expression); + return (expression, formatProvider) => Expression.Call(methodInfo, TempSourceStringExpr, expression); } if (GetAnyMethodFromHierarchy(type, "TryParse") is MethodInfo invalidMethod) From f1712a27e1f9322e1a4635629c8183a59aef4997 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Mon, 14 Feb 2022 17:02:36 -0800 Subject: [PATCH 03/31] Adding tryparse binder --- .../src/ModelBinding/ModelMetadata.cs | 8 +- .../Binders/TryParseModelBinder.cs | 99 +-- .../Binders/TryParseModelBinderProvider.cs | 2 +- .../Binders/TryParseTypeModelBinderTest.cs | 622 ++++++++++++++++++ 4 files changed, 688 insertions(+), 43 deletions(-) create mode 100644 src/Mvc/Mvc.Core/test/ModelBinding/Binders/TryParseTypeModelBinderTest.cs diff --git a/src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelMetadata.cs b/src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelMetadata.cs index db58e37e28ae..386d660664a0 100644 --- a/src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelMetadata.cs +++ b/src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelMetadata.cs @@ -529,8 +529,8 @@ internal void ThrowIfRecordTypeHasValidationOnProperties() throw _recordTypeValidatorsOnPropertiesError; } } - internal static Func? FindTryParseMethod(Type modelType) - => ParameterBindingMethodCache.FindTryParseMethod(modelType); + internal static Func? FindTryParseMethod(Type modelType) + => ParameterBindingMethodCache.FindTryParseMethod(Nullable.GetUnderlyingType(modelType) ?? modelType); [MemberNotNull(nameof(_parameterMapping), nameof(_boundConstructorPropertyMapping))] private void CalculateRecordTypeConstructorDetails() @@ -627,11 +627,11 @@ private void InitializeTypeInformation() { Debug.Assert(ModelType != null); - IsComplexType = !TypeDescriptor.GetConverter(ModelType).CanConvertFrom(typeof(string)); + HasTryParse = ParameterBindingMethodCache.HasTryParseMethod(ModelType); + IsComplexType = !HasTryParse && !TypeDescriptor.GetConverter(ModelType).CanConvertFrom(typeof(string)); IsNullableValueType = Nullable.GetUnderlyingType(ModelType) != null; IsReferenceOrNullableType = !ModelType.IsValueType || IsNullableValueType; UnderlyingOrModelType = Nullable.GetUnderlyingType(ModelType) ?? ModelType; - HasTryParse = ParameterBindingMethodCache.HasTryParseMethod(ModelType); var collectionType = ClosedGenericMatcher.ExtractGenericInterface(ModelType, typeof(ICollection<>)); IsCollectionType = collectionType != null; diff --git a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinder.cs b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinder.cs index b7e9351df5e9..da4ea38da986 100644 --- a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinder.cs +++ b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinder.cs @@ -4,6 +4,7 @@ #nullable enable using System.Linq.Expressions; +using System.Reflection; using Microsoft.AspNetCore.Http; using Microsoft.Extensions.Logging; @@ -12,9 +13,17 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders; /// /// An for simple types. /// -public class TryParseModelBinder : IModelBinder +internal sealed class TryParseModelBinder : IModelBinder { - private readonly Func _tryParseMethodExpession; + private static readonly MethodInfo AddModelErrorMethod = typeof(TryParseModelBinder).GetMethod(nameof(AddModelError), BindingFlags.NonPublic | BindingFlags.Static)!; + private static readonly MethodInfo SuccessBindingResultMethod = typeof(ModelBindingResult).GetMethod(nameof(ModelBindingResult.Success), BindingFlags.Public | BindingFlags.Static)!; + private static readonly ParameterExpression BindingContextExpression = Expression.Parameter(typeof(ModelBindingContext), "bindingContext"); + private static readonly ParameterExpression ValueProviderResultExpression = Expression.Parameter(typeof(ValueProviderResult), "valueProviderResult"); + private static readonly MemberExpression BindingResultExpression = Expression.Property(BindingContextExpression, nameof(ModelBindingContext.Result)); + private static readonly MemberExpression ValueExpression = Expression.Property(ValueProviderResultExpression, nameof(ValueProviderResult.FirstValue)); + private static readonly MemberExpression CultureExpression = Expression.Property(ValueProviderResultExpression, nameof(ValueProviderResult.Culture)); + + private readonly Func _tryParseOperation; private readonly ILogger _logger; /// @@ -34,7 +43,7 @@ public TryParseModelBinder(Type type, ILoggerFactory loggerFactory) throw new ArgumentNullException(nameof(loggerFactory)); } - _tryParseMethodExpession = ModelMetadata.FindTryParseMethod(type)!; + _tryParseOperation = CreateTryParseOperation(type); _logger = loggerFactory.CreateLogger(); } @@ -63,54 +72,68 @@ public Task BindModelAsync(ModelBindingContext bindingContext) try { var value = valueProviderResult.FirstValue; - - object? model = null; if (string.IsNullOrWhiteSpace(value)) { - // Other than the StringConverter, converters Trim() the value then throw if the result is empty. - model = null; - } - else - { - var parsedValue = Expression.Variable(bindingContext.ModelType, "parsedValue"); - var modelValue = Expression.Variable(typeof(object), "model"); - - var expression = Expression.Block( - new[] { parsedValue, modelValue, ParameterBindingMethodCache.TempSourceStringExpr }, - Expression.Assign(ParameterBindingMethodCache.TempSourceStringExpr, Expression.Constant(value!)), - Expression.IfThenElse(_tryParseMethodExpession(parsedValue, valueProviderResult.Culture), - Expression.Assign(modelValue, Expression.Convert(parsedValue, modelValue.Type)), - Expression.Throw(Expression.Constant(new FormatException()))), - modelValue); - - model = Expression.Lambda>(expression).Compile()(); - } - - // When converting newModel a null value 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 && !bindingContext.ModelMetadata.IsReferenceOrNullableType) - { - bindingContext.ModelState.TryAddModelError( - bindingContext.ModelName, - bindingContext.ModelMetadata.ModelBindingMessageProvider.ValueMustNotBeNullAccessor( - valueProviderResult.ToString())); + // Is expected to the TryParse() method trims the value then fail if the result is empty. + + // When converting newModel a null value 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 (!bindingContext.ModelMetadata.IsReferenceOrNullableType) + { + bindingContext.ModelState.TryAddModelError( + bindingContext.ModelName, + bindingContext.ModelMetadata.ModelBindingMessageProvider.ValueMustNotBeNullAccessor( + valueProviderResult.ToString())); + } + else + { + bindingContext.Result = ModelBindingResult.Success(null); + } } else { - bindingContext.Result = ModelBindingResult.Success(model); + _tryParseOperation(valueProviderResult, bindingContext); } } catch (Exception exception) { // Conversion failed. - bindingContext.ModelState.TryAddModelError( - bindingContext.ModelName, - exception, - bindingContext.ModelMetadata); + AddModelError(bindingContext, exception); } _logger.DoneAttemptingToBindModel(bindingContext); return Task.CompletedTask; } + + private static void AddModelError(ModelBindingContext bindingContext, Exception exception) + { + // Conversion failed. + bindingContext.ModelState.TryAddModelError( + bindingContext.ModelName, + exception, + bindingContext.ModelMetadata); + } + + private Func CreateTryParseOperation(Type modelType) + { + modelType = Nullable.GetUnderlyingType(modelType) ?? modelType; + var tryParseMethodExpession = ModelMetadata.FindTryParseMethod(modelType) + ?? throw new InvalidOperationException($"The binder { nameof(TryParseModelBinder) } must be only used for types that contains a TryParse method"); + + var parsedValue = Expression.Variable(modelType, "parsedValue"); + var modelValue = Expression.Variable(typeof(object), "model"); + + var expression = Expression.Block( + new[] { parsedValue, modelValue, ParameterBindingMethodCache.TempSourceStringExpr }, + Expression.Assign(ParameterBindingMethodCache.TempSourceStringExpr, ValueExpression), + Expression.IfThenElse(tryParseMethodExpession(parsedValue, CultureExpression), + Expression.Block( + Expression.Assign(modelValue, Expression.Convert(parsedValue, modelValue.Type)), + Expression.Assign(BindingResultExpression, Expression.Call(SuccessBindingResultMethod, modelValue))), + Expression.Call(AddModelErrorMethod, BindingContextExpression, Expression.Constant(new FormatException()))), + modelValue); + + return Expression.Lambda>(expression, new[] { ValueProviderResultExpression, BindingContextExpression }).Compile(); + } } diff --git a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinderProvider.cs b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinderProvider.cs index 8a05614988b4..76b2f78d9608 100644 --- a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinderProvider.cs +++ b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinderProvider.cs @@ -11,7 +11,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders; /// /// An for binding types that have a TryParse method. /// -public class TryParseModelBinderProvider : IModelBinderProvider +public sealed class TryParseModelBinderProvider : IModelBinderProvider { /// public IModelBinder? GetBinder(ModelBinderProviderContext context) diff --git a/src/Mvc/Mvc.Core/test/ModelBinding/Binders/TryParseTypeModelBinderTest.cs b/src/Mvc/Mvc.Core/test/ModelBinding/Binders/TryParseTypeModelBinderTest.cs new file mode 100644 index 000000000000..41cade6eb2b2 --- /dev/null +++ b/src/Mvc/Mvc.Core/test/ModelBinding/Binders/TryParseTypeModelBinderTest.cs @@ -0,0 +1,622 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Globalization; +using Microsoft.AspNetCore.Testing; +using Microsoft.Extensions.Logging.Abstractions; +using Microsoft.Extensions.Logging.Testing; + +namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders; + +public class TryParseTypeModelBinderTest +{ + //[Theory] + //[InlineData(null)] + //[InlineData("value")] + //[InlineData("intermediate whitespace")] + //[InlineData("\t untrimmed whitespace \t\r\n")] + //public async Task BindModelAsync_ReturnsProvidedString(string value) + //{ + // // Arrange + // var bindingContext = GetBindingContext(typeof(string)); + // bindingContext.ValueProvider = new SimpleValueProvider + // { + // { "theModelName", value } + // }; + + // var binder = new SimpleTypeModelBinder(typeof(string), NullLoggerFactory.Instance); + + // // Act + // await binder.BindModelAsync(bindingContext); + + // // Assert + // Assert.Same(value, bindingContext.Result.Model); + // Assert.True(bindingContext.ModelState.ContainsKey("theModelName")); + //} + + //[Theory] + //[InlineData("")] + //[InlineData(" \t \r\n ")] + //public async Task BindModel_ReturnsProvidedWhitespaceString_WhenNotConvertEmptyStringToNull(string value) + //{ + // // Arrange + // var bindingContext = GetBindingContext(typeof(string)); + // bindingContext.ValueProvider = new SimpleValueProvider + // { + // { "theModelName", value } + // }; + + // var metadataProvider = new TestModelMetadataProvider(); + // metadataProvider + // .ForType(typeof(string)) + // .DisplayDetails(d => d.ConvertEmptyStringToNull = false); + // bindingContext.ModelMetadata = metadataProvider.GetMetadataForType(typeof(string)); + + // var binder = new SimpleTypeModelBinder(typeof(string), NullLoggerFactory.Instance); + + // // Act + // await binder.BindModelAsync(bindingContext); + + // // Assert + // Assert.Same(value, bindingContext.Result.Model); + // Assert.True(bindingContext.ModelState.ContainsKey("theModelName")); + //} + + public static TheoryData ConvertibleTypeData + { + get + { + var data = new TheoryData + { + typeof(byte), + typeof(short), + typeof(int), + typeof(long), + typeof(Guid), + typeof(double), + typeof(DayOfWeek), + }; + + // DateTimeOffset doesn't have a TypeConverter in Mono. + if (!TestPlatformHelper.IsMono) + { + data.Add(typeof(DateTimeOffset)); + } + + return data; + } + } + + [Theory] + [MemberData(nameof(ConvertibleTypeData))] + public async Task BindModel_ReturnsFailure_IfTypeCanBeConverted_AndConversionFails(Type destinationType) + { + // Arrange + var bindingContext = GetBindingContext(destinationType); + bindingContext.ValueProvider = new SimpleValueProvider + { + { "theModelName", "some-value" } + }; + + var binder = new TryParseModelBinder(destinationType, NullLoggerFactory.Instance); + + // Act + await binder.BindModelAsync(bindingContext); + + // Assert + Assert.False(bindingContext.Result.IsModelSet); + } + + [Theory] + [MemberData(nameof(ConvertibleTypeData))] + public async Task BindModel_CreatesError_WhenTypeConversionIsNull(Type destinationType) + { + // Arrange + var bindingContext = GetBindingContext(destinationType); + bindingContext.ValueProvider = new SimpleValueProvider + { + { "theModelName", string.Empty } + }; + var binder = new TryParseModelBinder(destinationType, NullLoggerFactory.Instance); + + // Act + await binder.BindModelAsync(bindingContext); + + // Assert + Assert.False(bindingContext.Result.IsModelSet); + Assert.Null(bindingContext.Result.Model); + + var error = Assert.Single(bindingContext.ModelState["theModelName"].Errors); + Assert.Equal("The value '' is invalid.", error.ErrorMessage, StringComparer.Ordinal); + Assert.Null(error.Exception); + } + + [Fact] + public async Task BindModel_Error_FormatExceptionsTurnedIntoStringsInModelState() + { + // Arrange + var message = "The value 'not an integer' is not valid."; + var bindingContext = GetBindingContext(typeof(int)); + bindingContext.ValueProvider = new SimpleValueProvider + { + { "theModelName", "not an integer" } + }; + + var binder = new TryParseModelBinder(typeof(int), NullLoggerFactory.Instance); + + // Act + await binder.BindModelAsync(bindingContext); + + // Assert + Assert.False(bindingContext.Result.IsModelSet); + Assert.Null(bindingContext.Result.Model); + Assert.False(bindingContext.ModelState.IsValid); + var error = Assert.Single(bindingContext.ModelState["theModelName"].Errors); + Assert.Equal(message, error.ErrorMessage); + } + + public static TheoryData IntegerModelMetadataDataSet + { + get + { + var metadataProvider = new EmptyModelMetadataProvider(); + var method = typeof(MetadataClass).GetMethod(nameof(MetadataClass.IsLovely)); + var parameter = method.GetParameters()[0]; // IsLovely(int parameter) + + return new TheoryData + { + metadataProvider.GetMetadataForParameter(parameter), + metadataProvider.GetMetadataForProperty(typeof(MetadataClass), nameof(MetadataClass.Property)), + metadataProvider.GetMetadataForType(typeof(int)), + }; + } + } + + [Theory] + [MemberData(nameof(IntegerModelMetadataDataSet))] + public async Task BindModel_EmptyValueProviderResult_ReturnsFailedAndLogsSuccessfully(ModelMetadata metadata) + { + // Arrange + var bindingContext = GetBindingContext(typeof(int)); + bindingContext.ModelMetadata = metadata; + + var sink = new TestSink(); + var loggerFactory = new TestLoggerFactory(sink, enabled: true); + var binder = new TryParseModelBinder(typeof(int), loggerFactory); + + // Act + await binder.BindModelAsync(bindingContext); + + // Assert + Assert.Equal(ModelBindingResult.Failed(), bindingContext.Result); + Assert.Empty(bindingContext.ModelState); + Assert.Equal(3, sink.Writes.Count()); + } + + //[Theory] + //[InlineData("")] + //[InlineData(" \t \r\n ")] + //public async Task BindModel_ReturnsNull_IfTrimmedValueIsEmptyString(object value) + //{ + // // Arrange + // var bindingContext = GetBindingContext(typeof(string)); + // bindingContext.ValueProvider = new SimpleValueProvider + // { + // { "theModelName", value } + // }; + + // var binder = new SimpleTypeModelBinder(typeof(string), NullLoggerFactory.Instance); + + // // Act + // await binder.BindModelAsync(bindingContext); + + // // Assert + // Assert.Null(bindingContext.Result.Model); + // Assert.True(bindingContext.ModelState.ContainsKey("theModelName")); + //} + + [Fact] + public async Task BindModel_NullableIntegerValueProviderResult_ReturnsModel() + { + // Arrange + var bindingContext = GetBindingContext(typeof(int?)); + bindingContext.ValueProvider = new SimpleValueProvider + { + { "theModelName", "12" } + }; + + var binder = new TryParseModelBinder(typeof(int?), NullLoggerFactory.Instance); + + // Act + await binder.BindModelAsync(bindingContext); + + // Assert + Assert.True(bindingContext.Result.IsModelSet); + Assert.Equal(12, bindingContext.Result.Model); + Assert.True(bindingContext.ModelState.ContainsKey("theModelName")); + } + + [Fact] + public async Task BindModel_NullableDoubleValueProviderResult_ReturnsModel() + { + // Arrange + var bindingContext = GetBindingContext(typeof(double?)); + bindingContext.ValueProvider = new SimpleValueProvider + { + { "theModelName", "12.5" } + }; + + var binder = new TryParseModelBinder(typeof(double?), NullLoggerFactory.Instance); + + // Act + await binder.BindModelAsync(bindingContext); + + // Assert + Assert.True(bindingContext.Result.IsModelSet); + Assert.Equal(12.5, bindingContext.Result.Model); + Assert.True(bindingContext.ModelState.ContainsKey("theModelName")); + } + + [Theory] + [MemberData(nameof(IntegerModelMetadataDataSet))] + public async Task BindModel_ValidValueProviderResult_ReturnsModelAndLogsSuccessfully(ModelMetadata metadata) + { + // Arrange + var bindingContext = GetBindingContext(typeof(int)); + bindingContext.ModelMetadata = metadata; + bindingContext.ValueProvider = new SimpleValueProvider + { + { "theModelName", "42" } + }; + + var sink = new TestSink(); + var loggerFactory = new TestLoggerFactory(sink, enabled: true); + var binder = new TryParseModelBinder(typeof(int), loggerFactory); + + // Act + await binder.BindModelAsync(bindingContext); + + // Assert + Assert.True(bindingContext.Result.IsModelSet); + Assert.Equal(42, bindingContext.Result.Model); + Assert.True(bindingContext.ModelState.ContainsKey("theModelName")); + Assert.Equal(2, sink.Writes.Count()); + } + + public static TheoryData BiggerNumericTypes + { + get + { + // Data set does not include bool, byte, sbyte, or char because they do not need thousands separators. + // Also does not include float point types (eg.: decimal, double, float) because for those we will use + // NumberStyles.Number that allow thousands operator + return new TheoryData + { + typeof(int), + typeof(long), + typeof(short), + typeof(uint), + typeof(ulong), + typeof(ushort), + }; + } + } + + [Theory] + [MemberData(nameof(BiggerNumericTypes))] + public async Task BindModel_ThousandsSeparators_LeadToErrors(Type type) + { + // Arrange + var bindingContext = GetBindingContext(type); + bindingContext.ValueProvider = new SimpleValueProvider(new CultureInfo("en-GB")) + { + { "theModelName", "32,000" } + }; + + var binder = new TryParseModelBinder(type, NullLoggerFactory.Instance); + + // Act + await binder.BindModelAsync(bindingContext); + + // Assert + Assert.False(bindingContext.Result.IsModelSet); + + var entry = Assert.Single(bindingContext.ModelState); + Assert.Equal("theModelName", entry.Key); + Assert.Equal("32,000", entry.Value.AttemptedValue); + Assert.Equal(ModelValidationState.Invalid, entry.Value.ValidationState); + + var error = Assert.Single(entry.Value.Errors); + Assert.Equal("The value '32,000' is not valid.", error.ErrorMessage); + Assert.Null(error.Exception); + } + + [Fact] + public async Task BindModel_ValidValueProviderResultWithProvidedCulture_ReturnsModel() + { + // Arrange + var bindingContext = GetBindingContext(typeof(decimal)); + bindingContext.ValueProvider = new SimpleValueProvider(new CultureInfo("fr-FR")) + { + { "theModelName", "12,5" } + }; + + var binder = new TryParseModelBinder(typeof(decimal), NullLoggerFactory.Instance); + + // Act + await binder.BindModelAsync(bindingContext); + + // Assert + Assert.True(bindingContext.Result.IsModelSet); + Assert.Equal(12.5M, bindingContext.Result.Model); + Assert.True(bindingContext.ModelState.ContainsKey("theModelName")); + } + + [Fact] + public async Task BindModel_CreatesErrorForFormatException_ValueProviderResultWithInvalidCulture() + { + // Arrange + var bindingContext = GetBindingContext(typeof(decimal)); + bindingContext.ValueProvider = new SimpleValueProvider(new CultureInfo("en-GB")) + { + { "theModelName", "12-5" } + }; + + var binder = new TryParseModelBinder(typeof(decimal), NullLoggerFactory.Instance); + + // Act + await binder.BindModelAsync(bindingContext); + + // Assert + Assert.False(bindingContext.Result.IsModelSet); + Assert.Null(bindingContext.Result.Model); + + var error = Assert.Single(bindingContext.ModelState["theModelName"].Errors); + Assert.Equal("The value '12-5' is not valid.", error.ErrorMessage, StringComparer.Ordinal); + Assert.Null(error.Exception); + } + + [Fact] + public async Task BindModel_BindsEnumModels_IfArrayElementIsStringKey() + { + // Arrange + var bindingContext = GetBindingContext(typeof(IntEnum)); + bindingContext.ValueProvider = new SimpleValueProvider + { + { "theModelName", new object[] { "Value1" } } + }; + + var binder = new TryParseModelBinder(typeof(IntEnum), NullLoggerFactory.Instance); + + // Act + await binder.BindModelAsync(bindingContext); + + // Assert + Assert.True(bindingContext.Result.IsModelSet); + var boundModel = Assert.IsType(bindingContext.Result.Model); + Assert.Equal(IntEnum.Value1, boundModel); + } + + [Fact] + public async Task BindModel_BindsEnumModels_IfArrayElementIsStringValue() + { + // Arrange + var bindingContext = GetBindingContext(typeof(IntEnum)); + bindingContext.ValueProvider = new SimpleValueProvider + { + { "theModelName", new object[] { "1" } } + }; + + var binder = new TryParseModelBinder(typeof(IntEnum), NullLoggerFactory.Instance); + + // Act + await binder.BindModelAsync(bindingContext); + + // Assert + Assert.True(bindingContext.Result.IsModelSet); + var boundModel = Assert.IsType(bindingContext.Result.Model); + Assert.Equal(IntEnum.Value1, boundModel); + } + + public static TheoryData EnumValues + { + get + { + return new TheoryData + { + { "0", 0 }, + { "1", 1 }, + { "13", 13 }, + { "Value1", 1 }, + { "Value1, Value2", 3 }, + }; + } + } + + [Theory] + [MemberData(nameof(EnumValues))] + public async Task BindModel_BindsIntEnumModels(string flagsEnumValue, int expected) + { + // Arrange + var bindingContext = GetBindingContext(typeof(IntEnum)); + bindingContext.ValueProvider = new SimpleValueProvider + { + { "theModelName", flagsEnumValue } + }; + + var binder = new TryParseModelBinder(typeof(IntEnum), NullLoggerFactory.Instance); + + // Act + await binder.BindModelAsync(bindingContext); + + // Assert + Assert.True(bindingContext.Result.IsModelSet); + var boundModel = Assert.IsType(bindingContext.Result.Model); + Assert.Equal((IntEnum)expected, boundModel); + } + + [Theory] + [InlineData("32,015")] + [InlineData("32,128")] + public async Task BindModel_CreatesErrorForFormatException_BindsIntEnumModels(string flagsEnumValue) + { + // Arrange + var bindingContext = GetBindingContext(typeof(FlagsEnum)); + bindingContext.ValueProvider = new SimpleValueProvider + { + { "theModelName", flagsEnumValue } + }; + + var binder = new TryParseModelBinder(typeof(FlagsEnum), NullLoggerFactory.Instance); + + // Act + await binder.BindModelAsync(bindingContext); + + // Different than the Converter when calling the TryParse the values will + // NOT be treat as two separate enum values that are or'd together + // Assert + Assert.False(bindingContext.Result.IsModelSet); + var error = Assert.Single(bindingContext.ModelState["theModelName"].Errors); + Assert.Equal($"The value '{flagsEnumValue}' is not valid.", error.ErrorMessage, StringComparer.Ordinal); + Assert.Null(error.Exception); + } + + [Theory] + [InlineData("~10~", 10)] + [InlineData("~5", 5)] + [InlineData("aaaa~1~aaaa", 1)] + public async Task BindModel_BindClassWithTryParseMethod(string value, int expected) + { + // Arrange + var bindingContext = GetBindingContext(typeof(TestTryParseClass)); + bindingContext.ValueProvider = new SimpleValueProvider + { + { "theModelName", value } + }; + + var binder = new TryParseModelBinder(typeof(TestTryParseClass), NullLoggerFactory.Instance); + + // Act + await binder.BindModelAsync(bindingContext); + + // Assert + Assert.True(bindingContext.Result.IsModelSet); + var boundModel = Assert.IsType(bindingContext.Result.Model); + Assert.Equal(expected, boundModel.Id); + } + + [Theory] + [InlineData("10")] + [InlineData("~~0")] + public async Task BindModel_CreatesErrorForFormatException_BindClassWithTryParseMethod(string value) + { + // Arrange + var bindingContext = GetBindingContext(typeof(TestTryParseClass)); + bindingContext.ValueProvider = new SimpleValueProvider + { + { "theModelName", value } + }; + + var binder = new TryParseModelBinder(typeof(TestTryParseClass), NullLoggerFactory.Instance); + + // Act + await binder.BindModelAsync(bindingContext); + + // Different than the Converter when calling the TryParse the values will + // NOT be treat as two separate enum values that are or'd together + // Assert + Assert.False(bindingContext.Result.IsModelSet); + var error = Assert.Single(bindingContext.ModelState["theModelName"].Errors); + Assert.Equal($"The value '{value}' is not valid.", error.ErrorMessage, StringComparer.Ordinal); + Assert.Null(error.Exception); + } + + [Theory] + [MemberData(nameof(EnumValues))] + [InlineData("Value8, Value4", 12)] + public async Task BindModel_BindsFlagsEnumModels(string flagsEnumValue, int expected) + { + // Arrange + var bindingContext = GetBindingContext(typeof(FlagsEnum)); + bindingContext.ValueProvider = new SimpleValueProvider + { + { "theModelName", flagsEnumValue } + }; + + var binder = new TryParseModelBinder(typeof(FlagsEnum), NullLoggerFactory.Instance); + + // Act + await binder.BindModelAsync(bindingContext); + + // Assert + Assert.True(bindingContext.Result.IsModelSet); + var boundModel = Assert.IsType(bindingContext.Result.Model); + Assert.Equal((FlagsEnum)expected, boundModel); + } + + private static DefaultModelBindingContext GetBindingContext(Type modelType) + { + return new DefaultModelBindingContext + { + ModelMetadata = new EmptyModelMetadataProvider().GetMetadataForType(modelType), + ModelName = "theModelName", + ModelState = new ModelStateDictionary(), + ValueProvider = new SimpleValueProvider() // empty + }; + } + + private sealed class TestClass + { + } + + private sealed class TestTryParseClass + { + public int? Id { get; set; } + + public static bool TryParse(string s, out TestTryParseClass? result) + { + result = new TestTryParseClass(); + + if (!string.IsNullOrWhiteSpace(s)) + { + var tokens = s.Split('~'); + + if (tokens.Length >= 2) + { + result.Id = int.Parse(tokens[1]); + return true; + } + } + + return false; + + } + } + + [Flags] + private enum FlagsEnum + { + Value1 = 1, + Value2 = 2, + Value4 = 4, + Value8 = 8, + } + + private enum IntEnum + { + Value0 = 0, + Value1 = 1, + Value2 = 2, + MaxValue = int.MaxValue + } + + private class MetadataClass + { + public int Property { get; set; } + + public bool IsLovely(int parameter) + { + return true; + } + } +} From d0883797d7320931872e7b5a3ec603fd07194e2f Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Mon, 14 Feb 2022 17:02:56 -0800 Subject: [PATCH 04/31] Adding IFormatProvider parameter --- .../Http.Extensions/src/RequestDelegateFactory.cs | 2 +- src/Shared/ParameterBindingMethodCache.cs | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index a4e559018256..ca91d74468f2 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -992,7 +992,7 @@ private static Expression BindParameterFromValue(ParameterInfo parameter, Expres HttpContextExpr, parameterTypeNameConstant, parameterNameConstant, TempSourceStringExpr, Expression.Constant(factoryContext.ThrowOnBadRequest))); - var tryParseCall = tryParseMethodCall(parsedValue, CultureInfo.InvariantCulture); + var tryParseCall = tryParseMethodCall(parsedValue, Expression.Constant(CultureInfo.InvariantCulture)); // The following code is generated if the parameter is required and // the method should not be matched. diff --git a/src/Shared/ParameterBindingMethodCache.cs b/src/Shared/ParameterBindingMethodCache.cs index 77b2e3c8cbf2..2129988bb2d7 100644 --- a/src/Shared/ParameterBindingMethodCache.cs +++ b/src/Shared/ParameterBindingMethodCache.cs @@ -28,7 +28,7 @@ internal sealed class ParameterBindingMethodCache private readonly MethodInfo _enumTryParseMethod; // Since this is shared source, the cache won't be shared between RequestDelegateFactory and the ApiDescriptionProvider sadly :( - private readonly ConcurrentDictionary?> _stringMethodCallCache = new(); + private readonly ConcurrentDictionary?> _stringMethodCallCache = new(); private readonly ConcurrentDictionary?, int)> _bindAsyncMethodCallCache = new(); // If IsDynamicCodeSupported is false, we can't use the static Enum.TryParse since there's no easy way for @@ -52,9 +52,9 @@ public bool HasTryParseMethod(Type type) public bool HasBindAsyncMethod(ParameterInfo parameter) => FindBindAsyncMethod(parameter).Expression is not null; - public Func? FindTryParseMethod(Type type) + public Func? FindTryParseMethod(Type type) { - Func? Finder(Type type) + Func? Finder(Type type) { MethodInfo? methodInfo; @@ -111,7 +111,7 @@ public bool HasBindAsyncMethod(ParameterInfo parameter) => return (expression, formatProvider) => Expression.Call( methodInfo!, TempSourceStringExpr, - Expression.Constant(formatProvider), + formatProvider, Expression.Constant(dateTimeStyles), expression); } @@ -122,7 +122,7 @@ public bool HasBindAsyncMethod(ParameterInfo parameter) => methodInfo!, TempSourceStringExpr, Expression.Constant(numberStyle), - Expression.Constant(formatProvider), + formatProvider, expression); } @@ -133,7 +133,7 @@ public bool HasBindAsyncMethod(ParameterInfo parameter) => return (expression, formatProvider) => Expression.Call( methodInfo, TempSourceStringExpr, - Expression.Constant(formatProvider), + formatProvider, expression); } From df3a6a4e4559b9b3399e2757bfbac40c610e0a77 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Mon, 14 Feb 2022 17:11:16 -0800 Subject: [PATCH 05/31] Updating unittests and PublicAPI.txt --- src/Mvc/Mvc.Core/src/PublicAPI.Unshipped.txt | 3 - .../TryParseTypeModelBinderProviderTest.cs | 61 +++++++++++++++ .../Binders/TryParseTypeModelBinderTest.cs | 78 +------------------ 3 files changed, 63 insertions(+), 79 deletions(-) create mode 100644 src/Mvc/Mvc.Core/test/ModelBinding/Binders/TryParseTypeModelBinderProviderTest.cs diff --git a/src/Mvc/Mvc.Core/src/PublicAPI.Unshipped.txt b/src/Mvc/Mvc.Core/src/PublicAPI.Unshipped.txt index 0c68147a9f09..0b1efb61ebc0 100644 --- a/src/Mvc/Mvc.Core/src/PublicAPI.Unshipped.txt +++ b/src/Mvc/Mvc.Core/src/PublicAPI.Unshipped.txt @@ -1,7 +1,4 @@ #nullable enable -Microsoft.AspNetCore.Mvc.ModelBinding.Binders.TryParseModelBinder -Microsoft.AspNetCore.Mvc.ModelBinding.Binders.TryParseModelBinder.BindModelAsync(Microsoft.AspNetCore.Mvc.ModelBinding.ModelBindingContext! bindingContext) -> System.Threading.Tasks.Task! -Microsoft.AspNetCore.Mvc.ModelBinding.Binders.TryParseModelBinder.TryParseModelBinder(System.Type! type, Microsoft.Extensions.Logging.ILoggerFactory! loggerFactory) -> void Microsoft.AspNetCore.Mvc.ModelBinding.Binders.TryParseModelBinderProvider Microsoft.AspNetCore.Mvc.ModelBinding.Binders.TryParseModelBinderProvider.GetBinder(Microsoft.AspNetCore.Mvc.ModelBinding.ModelBinderProviderContext! context) -> Microsoft.AspNetCore.Mvc.ModelBinding.IModelBinder? Microsoft.AspNetCore.Mvc.ModelBinding.Binders.TryParseModelBinderProvider.TryParseModelBinderProvider() -> void diff --git a/src/Mvc/Mvc.Core/test/ModelBinding/Binders/TryParseTypeModelBinderProviderTest.cs b/src/Mvc/Mvc.Core/test/ModelBinding/Binders/TryParseTypeModelBinderProviderTest.cs new file mode 100644 index 000000000000..01dc99e6c33f --- /dev/null +++ b/src/Mvc/Mvc.Core/test/ModelBinding/Binders/TryParseTypeModelBinderProviderTest.cs @@ -0,0 +1,61 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Globalization; +using System.Net; + +namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders; + +public class TryParseModelBinderProviderTest +{ + [Theory] + [InlineData(typeof(string))] + [InlineData(typeof(object))] + [InlineData(typeof(Calendar))] + [InlineData(typeof(TestClass))] + [InlineData(typeof(List))] + public void Create_ForTypesWithoutTryParse_ReturnsNull(Type modelType) + { + // Arrange + var provider = new TryParseModelBinderProvider(); + var context = new TestModelBinderProviderContext(modelType); + + // Act + var result = provider.GetBinder(context); + + // Assert + Assert.Null(result); + } + + [Theory] + [InlineData(typeof(int))] + [InlineData(typeof(DateTime))] + [InlineData(typeof(DateTime?))] + [InlineData(typeof(IPEndPoint))] + [InlineData(typeof(TestClassWithTryParse))] + public void Create_ForTypesWithTryParse_ReturnsBinder(Type modelType) + { + // Arrange + var provider = new TryParseModelBinderProvider(); + var context = new TestModelBinderProviderContext(modelType); + + // Act + var result = provider.GetBinder(context); + + // Assert + Assert.IsType(result); + } + + private class TestClass + { + } + + private class TestClassWithTryParse + { + public static bool TryParse(string s, out TestClassWithTryParse result) + { + result = new TestClassWithTryParse(); + return true; + } + } +} diff --git a/src/Mvc/Mvc.Core/test/ModelBinding/Binders/TryParseTypeModelBinderTest.cs b/src/Mvc/Mvc.Core/test/ModelBinding/Binders/TryParseTypeModelBinderTest.cs index 41cade6eb2b2..ce8f194bba72 100644 --- a/src/Mvc/Mvc.Core/test/ModelBinding/Binders/TryParseTypeModelBinderTest.cs +++ b/src/Mvc/Mvc.Core/test/ModelBinding/Binders/TryParseTypeModelBinderTest.cs @@ -10,58 +10,6 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders; public class TryParseTypeModelBinderTest { - //[Theory] - //[InlineData(null)] - //[InlineData("value")] - //[InlineData("intermediate whitespace")] - //[InlineData("\t untrimmed whitespace \t\r\n")] - //public async Task BindModelAsync_ReturnsProvidedString(string value) - //{ - // // Arrange - // var bindingContext = GetBindingContext(typeof(string)); - // bindingContext.ValueProvider = new SimpleValueProvider - // { - // { "theModelName", value } - // }; - - // var binder = new SimpleTypeModelBinder(typeof(string), NullLoggerFactory.Instance); - - // // Act - // await binder.BindModelAsync(bindingContext); - - // // Assert - // Assert.Same(value, bindingContext.Result.Model); - // Assert.True(bindingContext.ModelState.ContainsKey("theModelName")); - //} - - //[Theory] - //[InlineData("")] - //[InlineData(" \t \r\n ")] - //public async Task BindModel_ReturnsProvidedWhitespaceString_WhenNotConvertEmptyStringToNull(string value) - //{ - // // Arrange - // var bindingContext = GetBindingContext(typeof(string)); - // bindingContext.ValueProvider = new SimpleValueProvider - // { - // { "theModelName", value } - // }; - - // var metadataProvider = new TestModelMetadataProvider(); - // metadataProvider - // .ForType(typeof(string)) - // .DisplayDetails(d => d.ConvertEmptyStringToNull = false); - // bindingContext.ModelMetadata = metadataProvider.GetMetadataForType(typeof(string)); - - // var binder = new SimpleTypeModelBinder(typeof(string), NullLoggerFactory.Instance); - - // // Act - // await binder.BindModelAsync(bindingContext); - - // // Assert - // Assert.Same(value, bindingContext.Result.Model); - // Assert.True(bindingContext.ModelState.ContainsKey("theModelName")); - //} - public static TheoryData ConvertibleTypeData { get @@ -193,28 +141,6 @@ public async Task BindModel_EmptyValueProviderResult_ReturnsFailedAndLogsSuccess Assert.Equal(3, sink.Writes.Count()); } - //[Theory] - //[InlineData("")] - //[InlineData(" \t \r\n ")] - //public async Task BindModel_ReturnsNull_IfTrimmedValueIsEmptyString(object value) - //{ - // // Arrange - // var bindingContext = GetBindingContext(typeof(string)); - // bindingContext.ValueProvider = new SimpleValueProvider - // { - // { "theModelName", value } - // }; - - // var binder = new SimpleTypeModelBinder(typeof(string), NullLoggerFactory.Instance); - - // // Act - // await binder.BindModelAsync(bindingContext); - - // // Assert - // Assert.Null(bindingContext.Result.Model); - // Assert.True(bindingContext.ModelState.ContainsKey("theModelName")); - //} - [Fact] public async Task BindModel_NullableIntegerValueProviderResult_ReturnsModel() { @@ -573,7 +499,7 @@ private sealed class TestTryParseClass { public int? Id { get; set; } - public static bool TryParse(string s, out TestTryParseClass? result) + public static bool TryParse(string s, out TestTryParseClass result) { result = new TestTryParseClass(); @@ -583,7 +509,7 @@ public static bool TryParse(string s, out TestTryParseClass? result) if (tokens.Length >= 2) { - result.Id = int.Parse(tokens[1]); + result.Id = int.Parse(tokens[1], CultureInfo.CurrentCulture); return true; } } From 8c54730502116f5f3ba6236b35407ca082c5773f Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Tue, 15 Feb 2022 11:18:42 -0800 Subject: [PATCH 06/31] Changing modelbinder to generics --- .../Binders/TryParseModelBinder.cs | 16 ++---- .../Binders/TryParseModelBinderProvider.cs | 4 +- .../TryParseTypeModelBinderProviderTest.cs | 3 +- .../Binders/TryParseTypeModelBinderTest.cs | 54 +++++++++++++------ 4 files changed, 47 insertions(+), 30 deletions(-) diff --git a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinder.cs b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinder.cs index da4ea38da986..a34af6982568 100644 --- a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinder.cs +++ b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinder.cs @@ -13,9 +13,9 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders; /// /// An for simple types. /// -internal sealed class TryParseModelBinder : IModelBinder +internal sealed class TryParseModelBinder : IModelBinder { - private static readonly MethodInfo AddModelErrorMethod = typeof(TryParseModelBinder).GetMethod(nameof(AddModelError), BindingFlags.NonPublic | BindingFlags.Static)!; + private static readonly MethodInfo AddModelErrorMethod = typeof(TryParseModelBinder).GetMethod(nameof(AddModelError), BindingFlags.NonPublic | BindingFlags.Static)!; private static readonly MethodInfo SuccessBindingResultMethod = typeof(ModelBindingResult).GetMethod(nameof(ModelBindingResult.Success), BindingFlags.Public | BindingFlags.Static)!; private static readonly ParameterExpression BindingContextExpression = Expression.Parameter(typeof(ModelBindingContext), "bindingContext"); private static readonly ParameterExpression ValueProviderResultExpression = Expression.Parameter(typeof(ValueProviderResult), "valueProviderResult"); @@ -29,21 +29,15 @@ internal sealed class TryParseModelBinder : IModelBinder /// /// Initializes a new instance of . /// - /// The type to create binder for. /// The . - public TryParseModelBinder(Type type, ILoggerFactory loggerFactory) + public TryParseModelBinder(ILoggerFactory loggerFactory) { - if (type == null) - { - throw new ArgumentNullException(nameof(type)); - } - if (loggerFactory == null) { throw new ArgumentNullException(nameof(loggerFactory)); } - _tryParseOperation = CreateTryParseOperation(type); + _tryParseOperation = CreateTryParseOperation(typeof(T)); _logger = loggerFactory.CreateLogger(); } @@ -119,7 +113,7 @@ private static void AddModelError(ModelBindingContext bindingContext, Exception { modelType = Nullable.GetUnderlyingType(modelType) ?? modelType; var tryParseMethodExpession = ModelMetadata.FindTryParseMethod(modelType) - ?? throw new InvalidOperationException($"The binder { nameof(TryParseModelBinder) } must be only used for types that contains a TryParse method"); + ?? throw new InvalidOperationException($"The type '{ modelType }' does not contains a TryParse method and the binder '{ nameof(TryParseModelBinder) }' cannot be used."); var parsedValue = Expression.Variable(modelType, "parsedValue"); var modelValue = Expression.Variable(typeof(object), "model"); diff --git a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinderProvider.cs b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinderProvider.cs index 76b2f78d9608..f03203c6b28a 100644 --- a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinderProvider.cs +++ b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinderProvider.cs @@ -24,7 +24,9 @@ public sealed class TryParseModelBinderProvider : IModelBinderProvider if (context.Metadata.HasTryParse) { var loggerFactory = context.Services.GetRequiredService(); - return new TryParseModelBinder(context.Metadata.ModelType, loggerFactory); + + var binderType = typeof(TryParseModelBinder<>).MakeGenericType(context.Metadata.ModelType); + return (IModelBinder)Activator.CreateInstance(binderType, loggerFactory)!; } return null; diff --git a/src/Mvc/Mvc.Core/test/ModelBinding/Binders/TryParseTypeModelBinderProviderTest.cs b/src/Mvc/Mvc.Core/test/ModelBinding/Binders/TryParseTypeModelBinderProviderTest.cs index 01dc99e6c33f..48c04a0a33e7 100644 --- a/src/Mvc/Mvc.Core/test/ModelBinding/Binders/TryParseTypeModelBinderProviderTest.cs +++ b/src/Mvc/Mvc.Core/test/ModelBinding/Binders/TryParseTypeModelBinderProviderTest.cs @@ -43,7 +43,8 @@ public void Create_ForTypesWithTryParse_ReturnsBinder(Type modelType) var result = provider.GetBinder(context); // Assert - Assert.IsType(result); + var binderType = typeof(TryParseModelBinder<>).MakeGenericType(modelType); + Assert.IsType(binderType, result); } private class TestClass diff --git a/src/Mvc/Mvc.Core/test/ModelBinding/Binders/TryParseTypeModelBinderTest.cs b/src/Mvc/Mvc.Core/test/ModelBinding/Binders/TryParseTypeModelBinderTest.cs index ce8f194bba72..70fd3cdd5c16 100644 --- a/src/Mvc/Mvc.Core/test/ModelBinding/Binders/TryParseTypeModelBinderTest.cs +++ b/src/Mvc/Mvc.Core/test/ModelBinding/Binders/TryParseTypeModelBinderTest.cs @@ -3,6 +3,7 @@ using System.Globalization; using Microsoft.AspNetCore.Testing; +using Microsoft.Extensions.Logging; using Microsoft.Extensions.Logging.Abstractions; using Microsoft.Extensions.Logging.Testing; @@ -46,7 +47,7 @@ public async Task BindModel_ReturnsFailure_IfTypeCanBeConverted_AndConversionFai { "theModelName", "some-value" } }; - var binder = new TryParseModelBinder(destinationType, NullLoggerFactory.Instance); + var binder = CreateBinder(destinationType); // Act await binder.BindModelAsync(bindingContext); @@ -65,7 +66,7 @@ public async Task BindModel_CreatesError_WhenTypeConversionIsNull(Type destinati { { "theModelName", string.Empty } }; - var binder = new TryParseModelBinder(destinationType, NullLoggerFactory.Instance); + var binder = CreateBinder(destinationType); // Act await binder.BindModelAsync(bindingContext); @@ -90,7 +91,7 @@ public async Task BindModel_Error_FormatExceptionsTurnedIntoStringsInModelState( { "theModelName", "not an integer" } }; - var binder = new TryParseModelBinder(typeof(int), NullLoggerFactory.Instance); + var binder = CreateBinder(typeof(int)); // Act await binder.BindModelAsync(bindingContext); @@ -130,7 +131,7 @@ public async Task BindModel_EmptyValueProviderResult_ReturnsFailedAndLogsSuccess var sink = new TestSink(); var loggerFactory = new TestLoggerFactory(sink, enabled: true); - var binder = new TryParseModelBinder(typeof(int), loggerFactory); + var binder = CreateBinder(typeof(int), loggerFactory); // Act await binder.BindModelAsync(bindingContext); @@ -151,7 +152,7 @@ public async Task BindModel_NullableIntegerValueProviderResult_ReturnsModel() { "theModelName", "12" } }; - var binder = new TryParseModelBinder(typeof(int?), NullLoggerFactory.Instance); + var binder = CreateBinder(typeof(int?)); // Act await binder.BindModelAsync(bindingContext); @@ -172,7 +173,7 @@ public async Task BindModel_NullableDoubleValueProviderResult_ReturnsModel() { "theModelName", "12.5" } }; - var binder = new TryParseModelBinder(typeof(double?), NullLoggerFactory.Instance); + var binder = CreateBinder(typeof(double?)); // Act await binder.BindModelAsync(bindingContext); @@ -197,7 +198,7 @@ public async Task BindModel_ValidValueProviderResult_ReturnsModelAndLogsSuccessf var sink = new TestSink(); var loggerFactory = new TestLoggerFactory(sink, enabled: true); - var binder = new TryParseModelBinder(typeof(int), loggerFactory); + var binder = CreateBinder(typeof(int), loggerFactory); // Act await binder.BindModelAsync(bindingContext); @@ -239,7 +240,7 @@ public async Task BindModel_ThousandsSeparators_LeadToErrors(Type type) { "theModelName", "32,000" } }; - var binder = new TryParseModelBinder(type, NullLoggerFactory.Instance); + var binder = CreateBinder(type); // Act await binder.BindModelAsync(bindingContext); @@ -267,7 +268,7 @@ public async Task BindModel_ValidValueProviderResultWithProvidedCulture_ReturnsM { "theModelName", "12,5" } }; - var binder = new TryParseModelBinder(typeof(decimal), NullLoggerFactory.Instance); + var binder = CreateBinder(typeof(decimal)); // Act await binder.BindModelAsync(bindingContext); @@ -288,7 +289,7 @@ public async Task BindModel_CreatesErrorForFormatException_ValueProviderResultWi { "theModelName", "12-5" } }; - var binder = new TryParseModelBinder(typeof(decimal), NullLoggerFactory.Instance); + var binder = CreateBinder(typeof(decimal)); // Act await binder.BindModelAsync(bindingContext); @@ -312,7 +313,7 @@ public async Task BindModel_BindsEnumModels_IfArrayElementIsStringKey() { "theModelName", new object[] { "Value1" } } }; - var binder = new TryParseModelBinder(typeof(IntEnum), NullLoggerFactory.Instance); + var binder = CreateBinder(typeof(IntEnum)); // Act await binder.BindModelAsync(bindingContext); @@ -333,7 +334,7 @@ public async Task BindModel_BindsEnumModels_IfArrayElementIsStringValue() { "theModelName", new object[] { "1" } } }; - var binder = new TryParseModelBinder(typeof(IntEnum), NullLoggerFactory.Instance); + var binder = CreateBinder(typeof(IntEnum)); // Act await binder.BindModelAsync(bindingContext); @@ -370,7 +371,7 @@ public async Task BindModel_BindsIntEnumModels(string flagsEnumValue, int expect { "theModelName", flagsEnumValue } }; - var binder = new TryParseModelBinder(typeof(IntEnum), NullLoggerFactory.Instance); + var binder = CreateBinder(typeof(IntEnum)); // Act await binder.BindModelAsync(bindingContext); @@ -393,7 +394,7 @@ public async Task BindModel_CreatesErrorForFormatException_BindsIntEnumModels(st { "theModelName", flagsEnumValue } }; - var binder = new TryParseModelBinder(typeof(FlagsEnum), NullLoggerFactory.Instance); + var binder = CreateBinder(typeof(FlagsEnum)); // Act await binder.BindModelAsync(bindingContext); @@ -420,7 +421,7 @@ public async Task BindModel_BindClassWithTryParseMethod(string value, int expect { "theModelName", value } }; - var binder = new TryParseModelBinder(typeof(TestTryParseClass), NullLoggerFactory.Instance); + var binder = CreateBinder(typeof(TestTryParseClass)); // Act await binder.BindModelAsync(bindingContext); @@ -443,7 +444,7 @@ public async Task BindModel_CreatesErrorForFormatException_BindClassWithTryParse { "theModelName", value } }; - var binder = new TryParseModelBinder(typeof(TestTryParseClass), NullLoggerFactory.Instance); + var binder = CreateBinder(typeof(TestTryParseClass)); // Act await binder.BindModelAsync(bindingContext); @@ -469,7 +470,7 @@ public async Task BindModel_BindsFlagsEnumModels(string flagsEnumValue, int expe { "theModelName", flagsEnumValue } }; - var binder = new TryParseModelBinder(typeof(FlagsEnum), NullLoggerFactory.Instance); + var binder = CreateBinder(typeof(FlagsEnum)); // Act await binder.BindModelAsync(bindingContext); @@ -480,6 +481,20 @@ public async Task BindModel_BindsFlagsEnumModels(string flagsEnumValue, int expe Assert.Equal((FlagsEnum)expected, boundModel); } + [Fact] + public void BindModel_ThrowsInvalidOperationException_WhenTryParseNotFound() + { + // Arrange + var bindingContext = GetBindingContext(typeof(TestClass)); + bindingContext.ValueProvider = new SimpleValueProvider + { + { "theModelName", string.Empty } + }; + + // Act & assert + Assert.Throws(() => new TryParseModelBinder(NullLoggerFactory.Instance)); + } + private static DefaultModelBindingContext GetBindingContext(Type modelType) { return new DefaultModelBindingContext @@ -491,6 +506,11 @@ private static DefaultModelBindingContext GetBindingContext(Type modelType) }; } + private static IModelBinder CreateBinder(Type modelType, ILoggerFactory loggerFactory = null) => + (IModelBinder)Activator.CreateInstance( + typeof(TryParseModelBinder<>).MakeGenericType(modelType), + loggerFactory ?? NullLoggerFactory.Instance)!; + private sealed class TestClass { } From 4cc18a7e4d9b308585c57242885afd1a99dbd346 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Tue, 15 Feb 2022 11:27:28 -0800 Subject: [PATCH 07/31] Updating comment --- .../src/ModelBinding/Binders/TryParseModelBinder.cs | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinder.cs b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinder.cs index a34af6982568..cf01eb032b54 100644 --- a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinder.cs +++ b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinder.cs @@ -115,6 +115,19 @@ private static void AddModelError(ModelBindingContext bindingContext, Exception var tryParseMethodExpession = ModelMetadata.FindTryParseMethod(modelType) ?? throw new InvalidOperationException($"The type '{ modelType }' does not contains a TryParse method and the binder '{ nameof(TryParseModelBinder) }' cannot be used."); + // var tempSourceString = valueProviderResult.FirstValue; + // object model = null; + // if ([modeltype].TryParse(tempSourceString, [valueProviderResult.Culture,] out [modelType] parsedValue)) + // { + // model = (object)parsedValue; + // bindingContext.Result = ModelBindingResult.Success(model); + // } + // else + // { + // AddModelError(bindingContext, new FormatException()); + // } + // return model; + var parsedValue = Expression.Variable(modelType, "parsedValue"); var modelValue = Expression.Variable(typeof(object), "model"); From 5bff26857b907181dd3d799389beccb5c158e240 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Tue, 15 Feb 2022 11:30:33 -0800 Subject: [PATCH 08/31] Adding empty line --- src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelMetadata.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelMetadata.cs b/src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelMetadata.cs index 386d660664a0..95b7b94d1876 100644 --- a/src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelMetadata.cs +++ b/src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelMetadata.cs @@ -529,6 +529,7 @@ internal void ThrowIfRecordTypeHasValidationOnProperties() throw _recordTypeValidatorsOnPropertiesError; } } + internal static Func? FindTryParseMethod(Type modelType) => ParameterBindingMethodCache.FindTryParseMethod(Nullable.GetUnderlyingType(modelType) ?? modelType); From 81a2408eb16811a4feecb2e3ed0da1b79d0e4ffd Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Tue, 15 Feb 2022 12:53:58 -0800 Subject: [PATCH 09/31] removing unexpected VS change --- src/Mvc/Mvc.Core/src/Microsoft.AspNetCore.Mvc.Core.csproj | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/Mvc/Mvc.Core/src/Microsoft.AspNetCore.Mvc.Core.csproj b/src/Mvc/Mvc.Core/src/Microsoft.AspNetCore.Mvc.Core.csproj index 0989c99b8956..a0b45ab9d83a 100644 --- a/src/Mvc/Mvc.Core/src/Microsoft.AspNetCore.Mvc.Core.csproj +++ b/src/Mvc/Mvc.Core/src/Microsoft.AspNetCore.Mvc.Core.csproj @@ -28,8 +28,8 @@ Microsoft.AspNetCore.Mvc.RouteAttribute - - + + @@ -61,8 +61,4 @@ Microsoft.AspNetCore.Mvc.RouteAttribute - - - - From 17866fd7312cb9645648104a6aa8ba8c2f196986 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Tue, 15 Feb 2022 13:04:23 -0800 Subject: [PATCH 10/31] Updating csprojs --- .../src/Microsoft.AspNetCore.Mvc.Abstractions.csproj | 7 ++----- .../src/Microsoft.AspNetCore.Mvc.ApiExplorer.csproj | 1 - src/Mvc/Mvc.Core/src/Microsoft.AspNetCore.Mvc.Core.csproj | 1 - 3 files changed, 2 insertions(+), 7 deletions(-) diff --git a/src/Mvc/Mvc.Abstractions/src/Microsoft.AspNetCore.Mvc.Abstractions.csproj b/src/Mvc/Mvc.Abstractions/src/Microsoft.AspNetCore.Mvc.Abstractions.csproj index eaa569a06e3c..b97d265a0fa3 100644 --- a/src/Mvc/Mvc.Abstractions/src/Microsoft.AspNetCore.Mvc.Abstractions.csproj +++ b/src/Mvc/Mvc.Abstractions/src/Microsoft.AspNetCore.Mvc.Abstractions.csproj @@ -13,11 +13,8 @@ Microsoft.AspNetCore.Mvc.IActionResult - - - - - + + diff --git a/src/Mvc/Mvc.ApiExplorer/src/Microsoft.AspNetCore.Mvc.ApiExplorer.csproj b/src/Mvc/Mvc.ApiExplorer/src/Microsoft.AspNetCore.Mvc.ApiExplorer.csproj index b55223a8cef1..4b0491eb95ef 100644 --- a/src/Mvc/Mvc.ApiExplorer/src/Microsoft.AspNetCore.Mvc.ApiExplorer.csproj +++ b/src/Mvc/Mvc.ApiExplorer/src/Microsoft.AspNetCore.Mvc.ApiExplorer.csproj @@ -10,7 +10,6 @@ - diff --git a/src/Mvc/Mvc.Core/src/Microsoft.AspNetCore.Mvc.Core.csproj b/src/Mvc/Mvc.Core/src/Microsoft.AspNetCore.Mvc.Core.csproj index a0b45ab9d83a..9d9b2ac58e91 100644 --- a/src/Mvc/Mvc.Core/src/Microsoft.AspNetCore.Mvc.Core.csproj +++ b/src/Mvc/Mvc.Core/src/Microsoft.AspNetCore.Mvc.Core.csproj @@ -58,7 +58,6 @@ Microsoft.AspNetCore.Mvc.RouteAttribute - From fe3d122d3bed2564362c135840b1977c2f14ab5b Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Wed, 16 Feb 2022 09:36:37 -0800 Subject: [PATCH 11/31] Changing property name to IsParseableType --- src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelMetadata.cs | 6 +++--- .../src/ModelBinding/Binders/TryParseModelBinderProvider.cs | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelMetadata.cs b/src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelMetadata.cs index 95b7b94d1876..0402cbfab3bd 100644 --- a/src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelMetadata.cs +++ b/src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelMetadata.cs @@ -482,7 +482,7 @@ internal IReadOnlyDictionary BoundConstructorPrope /// /// /// - internal bool HasTryParse { get; private set; } = default!; + internal bool IsParseableType { get; private set; } = default!; /// /// Gets a value indicating the NullabilityState of the value or reference type. @@ -628,8 +628,8 @@ private void InitializeTypeInformation() { Debug.Assert(ModelType != null); - HasTryParse = ParameterBindingMethodCache.HasTryParseMethod(ModelType); - IsComplexType = !HasTryParse && !TypeDescriptor.GetConverter(ModelType).CanConvertFrom(typeof(string)); + IsParseableType = ParameterBindingMethodCache.HasTryParseMethod(ModelType); + IsComplexType = !IsParseableType && !TypeDescriptor.GetConverter(ModelType).CanConvertFrom(typeof(string)); IsNullableValueType = Nullable.GetUnderlyingType(ModelType) != null; IsReferenceOrNullableType = !ModelType.IsValueType || IsNullableValueType; UnderlyingOrModelType = Nullable.GetUnderlyingType(ModelType) ?? ModelType; diff --git a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinderProvider.cs b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinderProvider.cs index f03203c6b28a..487af7aa0d5b 100644 --- a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinderProvider.cs +++ b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinderProvider.cs @@ -21,7 +21,7 @@ public sealed class TryParseModelBinderProvider : IModelBinderProvider throw new ArgumentNullException(nameof(context)); } - if (context.Metadata.HasTryParse) + if (context.Metadata.IsParseableType) { var loggerFactory = context.Services.GetRequiredService(); From b3cff44e7eb86c7241fbf863bb9bed9421a56f4b Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Wed, 16 Feb 2022 09:52:53 -0800 Subject: [PATCH 12/31] Changing property name to IsParseableType --- src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelMetadata.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelMetadata.cs b/src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelMetadata.cs index 0402cbfab3bd..84f06ce20d1b 100644 --- a/src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelMetadata.cs +++ b/src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelMetadata.cs @@ -482,7 +482,7 @@ internal IReadOnlyDictionary BoundConstructorPrope /// /// /// - internal bool IsParseableType { get; private set; } = default!; + internal bool IsParseableType { get; private set; }; /// /// Gets a value indicating the NullabilityState of the value or reference type. From e73543db77b8424597b63d46853201cdd6058028 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Wed, 16 Feb 2022 10:13:20 -0800 Subject: [PATCH 13/31] fix typo --- src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelMetadata.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelMetadata.cs b/src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelMetadata.cs index 84f06ce20d1b..d5bea554a37e 100644 --- a/src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelMetadata.cs +++ b/src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelMetadata.cs @@ -482,7 +482,7 @@ internal IReadOnlyDictionary BoundConstructorPrope /// /// /// - internal bool IsParseableType { get; private set; }; + internal bool IsParseableType { get; private set; } /// /// Gets a value indicating the NullabilityState of the value or reference type. From 248dc11d9506b05ce1f5206ad8953c88e139aa9d Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Wed, 16 Feb 2022 10:24:58 -0800 Subject: [PATCH 14/31] PR Feeback --- .../src/ModelBinding/Binders/TryParseModelBinder.cs | 6 ++---- .../src/ModelBinding/Binders/TryParseModelBinderProvider.cs | 2 -- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinder.cs b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinder.cs index cf01eb032b54..140a6bafe4d0 100644 --- a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinder.cs +++ b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinder.cs @@ -1,8 +1,6 @@ // 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.Linq.Expressions; using System.Reflection; using Microsoft.AspNetCore.Http; @@ -38,7 +36,7 @@ public TryParseModelBinder(ILoggerFactory loggerFactory) } _tryParseOperation = CreateTryParseOperation(typeof(T)); - _logger = loggerFactory.CreateLogger(); + _logger = loggerFactory.CreateLogger>(); } /// @@ -127,7 +125,7 @@ private static void AddModelError(ModelBindingContext bindingContext, Exception // AddModelError(bindingContext, new FormatException()); // } // return model; - + var parsedValue = Expression.Variable(modelType, "parsedValue"); var modelValue = Expression.Variable(typeof(object), "model"); diff --git a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinderProvider.cs b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinderProvider.cs index 487af7aa0d5b..c04ba33ae9af 100644 --- a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinderProvider.cs +++ b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinderProvider.cs @@ -1,8 +1,6 @@ // 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 Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; From dcc5fae1cbfc2069392cb1c3e7f9743bbe8acc54 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Thu, 17 Feb 2022 09:52:18 -0800 Subject: [PATCH 15/31] Rolling back to non-generic class --- .../Binders/TryParseModelBinder.cs | 18 ++++++++++++------ .../Binders/TryParseModelBinderProvider.cs | 4 +--- 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinder.cs b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinder.cs index 140a6bafe4d0..f38d9af783f4 100644 --- a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinder.cs +++ b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinder.cs @@ -11,9 +11,9 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders; /// /// An for simple types. /// -internal sealed class TryParseModelBinder : IModelBinder +internal sealed class TryParseModelBinder : IModelBinder { - private static readonly MethodInfo AddModelErrorMethod = typeof(TryParseModelBinder).GetMethod(nameof(AddModelError), BindingFlags.NonPublic | BindingFlags.Static)!; + private static readonly MethodInfo AddModelErrorMethod = typeof(TryParseModelBinder).GetMethod(nameof(AddModelError), BindingFlags.NonPublic | BindingFlags.Static)!; private static readonly MethodInfo SuccessBindingResultMethod = typeof(ModelBindingResult).GetMethod(nameof(ModelBindingResult.Success), BindingFlags.Public | BindingFlags.Static)!; private static readonly ParameterExpression BindingContextExpression = Expression.Parameter(typeof(ModelBindingContext), "bindingContext"); private static readonly ParameterExpression ValueProviderResultExpression = Expression.Parameter(typeof(ValueProviderResult), "valueProviderResult"); @@ -27,16 +27,22 @@ internal sealed class TryParseModelBinder : IModelBinder /// /// Initializes a new instance of . /// + /// The model type. /// The . - public TryParseModelBinder(ILoggerFactory loggerFactory) + public TryParseModelBinder(Type modelType, ILoggerFactory loggerFactory) { + if (modelType == null) + { + throw new ArgumentNullException(nameof(modelType)); + } + if (loggerFactory == null) { throw new ArgumentNullException(nameof(loggerFactory)); } - _tryParseOperation = CreateTryParseOperation(typeof(T)); - _logger = loggerFactory.CreateLogger>(); + _tryParseOperation = CreateTryParseOperation(modelType); + _logger = loggerFactory.CreateLogger(); } /// @@ -111,7 +117,7 @@ private static void AddModelError(ModelBindingContext bindingContext, Exception { modelType = Nullable.GetUnderlyingType(modelType) ?? modelType; var tryParseMethodExpession = ModelMetadata.FindTryParseMethod(modelType) - ?? throw new InvalidOperationException($"The type '{ modelType }' does not contains a TryParse method and the binder '{ nameof(TryParseModelBinder) }' cannot be used."); + ?? throw new InvalidOperationException($"The type '{ modelType }' does not contains a TryParse method and the binder '{ nameof(TryParseModelBinder) }' cannot be used."); // var tempSourceString = valueProviderResult.FirstValue; // object model = null; diff --git a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinderProvider.cs b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinderProvider.cs index c04ba33ae9af..5236500569f0 100644 --- a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinderProvider.cs +++ b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinderProvider.cs @@ -22,9 +22,7 @@ public sealed class TryParseModelBinderProvider : IModelBinderProvider if (context.Metadata.IsParseableType) { var loggerFactory = context.Services.GetRequiredService(); - - var binderType = typeof(TryParseModelBinder<>).MakeGenericType(context.Metadata.ModelType); - return (IModelBinder)Activator.CreateInstance(binderType, loggerFactory)!; + return new TryParseModelBinder(context.Metadata.ModelType, loggerFactory); } return null; From d2ef7b8e665ddb802f83a9d2b875e126a56df04e Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Thu, 17 Feb 2022 10:10:32 -0800 Subject: [PATCH 16/31] Adding SuppressParseableTypesBinding option --- .../src/ModelBinding/ModelMetadata.cs | 19 ++++++++++++------- .../Infrastructure/MvcCoreMvcOptionsSetup.cs | 6 +++--- .../ModelBinding/Metadata/BindingMetadata.cs | 10 ++++++++++ .../DefaultBindingMetadataProvider.cs | 12 ++++++++++++ .../Metadata/DefaultModelMetadata.cs | 2 ++ src/Mvc/Mvc.Core/src/MvcOptions.cs | 6 ++++++ src/Mvc/Mvc.Core/src/PublicAPI.Unshipped.txt | 2 ++ 7 files changed, 47 insertions(+), 10 deletions(-) diff --git a/src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelMetadata.cs b/src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelMetadata.cs index d5bea554a37e..ab544d74f113 100644 --- a/src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelMetadata.cs +++ b/src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelMetadata.cs @@ -437,11 +437,11 @@ internal IReadOnlyDictionary BoundConstructorPrope /// Gets a value indicating whether is a complex type. /// /// - /// A complex type is defined as a without a that can convert - /// from . Most POCO and types are therefore complex. Most, if + /// A complex type is defined as a that is NOT and NOT . + /// Most POCO and types are therefore complex. Most, if /// not all, BCL value types are simple types. /// - public bool IsComplexType { get; private set; } + public bool IsComplexType => !IsConvertibleType && !IsParseableType; /// /// Gets a value indicating whether or not is a . @@ -480,9 +480,15 @@ internal IReadOnlyDictionary BoundConstructorPrope public Type UnderlyingOrModelType { get; private set; } = default!; /// - /// + /// Gets a value indicating whether or not has a TryParse method. /// - internal bool IsParseableType { get; private set; } + internal virtual bool IsParseableType { get; } + + /// + /// Gets a value indicating whether or not has a + /// from . + /// + internal bool IsConvertibleType { get; private set; } /// /// Gets a value indicating the NullabilityState of the value or reference type. @@ -628,8 +634,7 @@ private void InitializeTypeInformation() { Debug.Assert(ModelType != null); - IsParseableType = ParameterBindingMethodCache.HasTryParseMethod(ModelType); - IsComplexType = !IsParseableType && !TypeDescriptor.GetConverter(ModelType).CanConvertFrom(typeof(string)); + IsConvertibleType = TypeDescriptor.GetConverter(ModelType).CanConvertFrom(typeof(string)); IsNullableValueType = Nullable.GetUnderlyingType(ModelType) != null; IsReferenceOrNullableType = !ModelType.IsValueType || IsNullableValueType; UnderlyingOrModelType = Nullable.GetUnderlyingType(ModelType) ?? ModelType; diff --git a/src/Mvc/Mvc.Core/src/Infrastructure/MvcCoreMvcOptionsSetup.cs b/src/Mvc/Mvc.Core/src/Infrastructure/MvcCoreMvcOptionsSetup.cs index 82affa44c7b9..b011b3fda119 100644 --- a/src/Mvc/Mvc.Core/src/Infrastructure/MvcCoreMvcOptionsSetup.cs +++ b/src/Mvc/Mvc.Core/src/Infrastructure/MvcCoreMvcOptionsSetup.cs @@ -100,7 +100,7 @@ public void Configure(MvcOptions options) options.ValueProviderFactories.Add(new FormFileValueProviderFactory()); // Set up metadata providers - ConfigureAdditionalModelMetadataDetailsProviders(options.ModelMetadataDetailsProviders); + ConfigureAdditionalModelMetadataDetailsProviders(options.ModelMetadataDetailsProviders, options); // Set up validators options.ModelValidatorProviders.Add(new DefaultModelValidatorProvider()); @@ -115,13 +115,13 @@ public void PostConfigure(string? name, MvcOptions options) options.ModelMetadataDetailsProviders.Add(new HasValidatorsValidationMetadataProvider(options.ModelValidatorProviders)); } - internal static void ConfigureAdditionalModelMetadataDetailsProviders(IList modelMetadataDetailsProviders) + internal static void ConfigureAdditionalModelMetadataDetailsProviders(IList modelMetadataDetailsProviders, MvcOptions options) { // Don't bind the Type class by default as it's expensive. A user can override this behavior // by altering the collection of providers. modelMetadataDetailsProviders.Add(new ExcludeBindingMetadataProvider(typeof(Type))); - modelMetadataDetailsProviders.Add(new DefaultBindingMetadataProvider()); + modelMetadataDetailsProviders.Add(new DefaultBindingMetadataProvider(options)); modelMetadataDetailsProviders.Add(new DefaultValidationMetadataProvider()); modelMetadataDetailsProviders.Add(new BindingSourceMetadataProvider(typeof(CancellationToken), BindingSource.Special)); diff --git a/src/Mvc/Mvc.Core/src/ModelBinding/Metadata/BindingMetadata.cs b/src/Mvc/Mvc.Core/src/ModelBinding/Metadata/BindingMetadata.cs index e3b1fab3f53a..9e44b2ce55de 100644 --- a/src/Mvc/Mvc.Core/src/ModelBinding/Metadata/BindingMetadata.cs +++ b/src/Mvc/Mvc.Core/src/ModelBinding/Metadata/BindingMetadata.cs @@ -62,6 +62,16 @@ public Type? BinderType /// public bool IsBindingAllowed { get; set; } = true; + /// + /// Gets or sets a value indicating whether or not the property can be model bound + /// from a TryParse method + /// + /// + /// + /// Always false when is set to true + /// + internal bool IsBindingFromTryParseAllowed { get; set; } + /// /// Gets or sets a value indicating whether or not the request must contain a value for the model. /// Will be ignored if the model metadata being created does not represent a property. diff --git a/src/Mvc/Mvc.Core/src/ModelBinding/Metadata/DefaultBindingMetadataProvider.cs b/src/Mvc/Mvc.Core/src/ModelBinding/Metadata/DefaultBindingMetadataProvider.cs index 9caa19b7d570..95c933930afa 100644 --- a/src/Mvc/Mvc.Core/src/ModelBinding/Metadata/DefaultBindingMetadataProvider.cs +++ b/src/Mvc/Mvc.Core/src/ModelBinding/Metadata/DefaultBindingMetadataProvider.cs @@ -14,6 +14,13 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Metadata; /// internal class DefaultBindingMetadataProvider : IBindingMetadataProvider { + private readonly MvcOptions? _options; + + public DefaultBindingMetadataProvider(MvcOptions? options = null) + { + _options = options; + } + public void CreateBindingMetadata(BindingMetadataProviderContext context) { if (context == null) @@ -78,6 +85,11 @@ public void CreateBindingMetadata(BindingMetadataProviderContext context) { context.BindingMetadata.BoundConstructor = constructorInfo; } + + if (_options?.SuppressParseableTypesBinding is false && ModelMetadata.FindTryParseMethod(context.Key.ModelType) is not null) + { + context.BindingMetadata.IsBindingFromTryParseAllowed = true; + } } internal static ConstructorInfo? GetBoundConstructor(Type type) diff --git a/src/Mvc/Mvc.Core/src/ModelBinding/Metadata/DefaultModelMetadata.cs b/src/Mvc/Mvc.Core/src/ModelBinding/Metadata/DefaultModelMetadata.cs index 429c18fef155..5083fad9ea11 100644 --- a/src/Mvc/Mvc.Core/src/ModelBinding/Metadata/DefaultModelMetadata.cs +++ b/src/Mvc/Mvc.Core/src/ModelBinding/Metadata/DefaultModelMetadata.cs @@ -347,6 +347,8 @@ public override bool IsRequired } } + internal override bool IsParseableType => BindingMetadata.IsBindingFromTryParseAllowed; + /// public override ModelBindingMessageProvider ModelBindingMessageProvider => BindingMetadata.ModelBindingMessageProvider!; diff --git a/src/Mvc/Mvc.Core/src/MvcOptions.cs b/src/Mvc/Mvc.Core/src/MvcOptions.cs index 7a59a9834738..61fdbb28b9d6 100644 --- a/src/Mvc/Mvc.Core/src/MvcOptions.cs +++ b/src/Mvc/Mvc.Core/src/MvcOptions.cs @@ -136,6 +136,12 @@ public MvcOptions() /// public bool SuppressOutputFormatterBuffering { get; set; } + /// + /// Gets or sets the flag that determines if binding is disabled for Parseable types (contains a TryParse method) + /// and will treat them as , in case, there isn't a from registered. + /// + public bool SuppressParseableTypesBinding { get; set; } + /// /// Gets or sets the flag that determines if MVC should use action invoker extensibility. This will allow /// custom and execute during the request pipeline. diff --git a/src/Mvc/Mvc.Core/src/PublicAPI.Unshipped.txt b/src/Mvc/Mvc.Core/src/PublicAPI.Unshipped.txt index e72c9d39ff92..b6bb947bb5f4 100644 --- a/src/Mvc/Mvc.Core/src/PublicAPI.Unshipped.txt +++ b/src/Mvc/Mvc.Core/src/PublicAPI.Unshipped.txt @@ -13,6 +13,8 @@ Microsoft.AspNetCore.Mvc.ModelBinding.Metadata.SystemTextJsonValidationMetadataP Microsoft.AspNetCore.Mvc.ModelBinding.Metadata.SystemTextJsonValidationMetadataProvider.SystemTextJsonValidationMetadataProvider(System.Text.Json.JsonNamingPolicy! namingPolicy) -> void Microsoft.AspNetCore.Mvc.ModelBinding.Metadata.ValidationMetadata.ValidationModelName.get -> string? Microsoft.AspNetCore.Mvc.ModelBinding.Metadata.ValidationMetadata.ValidationModelName.set -> void +Microsoft.AspNetCore.Mvc.MvcOptions.SuppressParseableTypesBinding.get -> bool +Microsoft.AspNetCore.Mvc.MvcOptions.SuppressParseableTypesBinding.set -> void virtual Microsoft.AspNetCore.Mvc.Infrastructure.ConfigureCompatibilityOptions.PostConfigure(string? name, TOptions! options) -> void *REMOVED*~Microsoft.AspNetCore.Mvc.Formatters.FormatFilter.FormatFilter(Microsoft.Extensions.Options.IOptions! options, Microsoft.Extensions.Logging.ILoggerFactory! loggerFactory) -> void Microsoft.AspNetCore.Mvc.Formatters.FormatFilter.FormatFilter(Microsoft.Extensions.Options.IOptions! options, Microsoft.Extensions.Logging.ILoggerFactory! loggerFactory) -> void From d9ff2d0962f9c93ebbe16f16231b74406851efde Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Thu, 17 Feb 2022 10:13:11 -0800 Subject: [PATCH 17/31] Updating tests --- .../TryParseTypeModelBinderProviderTest.cs | 34 +++--- .../Binders/TryParseTypeModelBinderTest.cs | 13 +-- .../DefaultBindingMetadataProviderTest.cs | 106 ++++++++++++++++++ .../Metadata/DefaultModelMetadataTest.cs | 50 +++++++++ src/Mvc/Mvc/test/MvcOptionsSetupTest.cs | 1 + 5 files changed, 175 insertions(+), 29 deletions(-) diff --git a/src/Mvc/Mvc.Core/test/ModelBinding/Binders/TryParseTypeModelBinderProviderTest.cs b/src/Mvc/Mvc.Core/test/ModelBinding/Binders/TryParseTypeModelBinderProviderTest.cs index 48c04a0a33e7..bd08c44c9ecb 100644 --- a/src/Mvc/Mvc.Core/test/ModelBinding/Binders/TryParseTypeModelBinderProviderTest.cs +++ b/src/Mvc/Mvc.Core/test/ModelBinding/Binders/TryParseTypeModelBinderProviderTest.cs @@ -8,17 +8,12 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders; public class TryParseModelBinderProviderTest { - [Theory] - [InlineData(typeof(string))] - [InlineData(typeof(object))] - [InlineData(typeof(Calendar))] - [InlineData(typeof(TestClass))] - [InlineData(typeof(List))] - public void Create_ForTypesWithoutTryParse_ReturnsNull(Type modelType) + [Fact] + public void Create_ForTypesWithoutTryParse_ReturnsNull() { // Arrange var provider = new TryParseModelBinderProvider(); - var context = new TestModelBinderProviderContext(modelType); + var context = CreateContext(modelType: typeof(TestClass), false); // Act var result = provider.GetBinder(context); @@ -27,24 +22,27 @@ public void Create_ForTypesWithoutTryParse_ReturnsNull(Type modelType) Assert.Null(result); } - [Theory] - [InlineData(typeof(int))] - [InlineData(typeof(DateTime))] - [InlineData(typeof(DateTime?))] - [InlineData(typeof(IPEndPoint))] - [InlineData(typeof(TestClassWithTryParse))] - public void Create_ForTypesWithTryParse_ReturnsBinder(Type modelType) + [Fact] + public void Create_ForTypesWithTryParse_ReturnsBinder() { // Arrange var provider = new TryParseModelBinderProvider(); - var context = new TestModelBinderProviderContext(modelType); + var context = CreateContext(typeof(TestClassWithTryParse), true); // Act var result = provider.GetBinder(context); // Assert - var binderType = typeof(TryParseModelBinder<>).MakeGenericType(modelType); - Assert.IsType(binderType, result); + Assert.IsType(result); + } + + private static TestModelBinderProviderContext CreateContext(Type modelType, bool allowTryParse) + { + TestModelBinderProviderContext.CachedMetadataProvider + .ForType(modelType) + .BindingDetails(b => b.IsBindingFromTryParseAllowed = allowTryParse); + + return new TestModelBinderProviderContext(modelType); } private class TestClass diff --git a/src/Mvc/Mvc.Core/test/ModelBinding/Binders/TryParseTypeModelBinderTest.cs b/src/Mvc/Mvc.Core/test/ModelBinding/Binders/TryParseTypeModelBinderTest.cs index 70fd3cdd5c16..73cb7bc55acd 100644 --- a/src/Mvc/Mvc.Core/test/ModelBinding/Binders/TryParseTypeModelBinderTest.cs +++ b/src/Mvc/Mvc.Core/test/ModelBinding/Binders/TryParseTypeModelBinderTest.cs @@ -484,15 +484,8 @@ public async Task BindModel_BindsFlagsEnumModels(string flagsEnumValue, int expe [Fact] public void BindModel_ThrowsInvalidOperationException_WhenTryParseNotFound() { - // Arrange - var bindingContext = GetBindingContext(typeof(TestClass)); - bindingContext.ValueProvider = new SimpleValueProvider - { - { "theModelName", string.Empty } - }; - // Act & assert - Assert.Throws(() => new TryParseModelBinder(NullLoggerFactory.Instance)); + Assert.Throws(() => new TryParseModelBinder(typeof(TestClass), NullLoggerFactory.Instance)); } private static DefaultModelBindingContext GetBindingContext(Type modelType) @@ -507,9 +500,7 @@ private static DefaultModelBindingContext GetBindingContext(Type modelType) } private static IModelBinder CreateBinder(Type modelType, ILoggerFactory loggerFactory = null) => - (IModelBinder)Activator.CreateInstance( - typeof(TryParseModelBinder<>).MakeGenericType(modelType), - loggerFactory ?? NullLoggerFactory.Instance)!; + new TryParseModelBinder(modelType, loggerFactory ?? NullLoggerFactory.Instance); private sealed class TestClass { diff --git a/src/Mvc/Mvc.Core/test/ModelBinding/Metadata/DefaultBindingMetadataProviderTest.cs b/src/Mvc/Mvc.Core/test/ModelBinding/Metadata/DefaultBindingMetadataProviderTest.cs index ac8cba12279d..1792765112cf 100644 --- a/src/Mvc/Mvc.Core/test/ModelBinding/Metadata/DefaultBindingMetadataProviderTest.cs +++ b/src/Mvc/Mvc.Core/test/ModelBinding/Metadata/DefaultBindingMetadataProviderTest.cs @@ -656,6 +656,112 @@ public void CreateBindingDetails_BindingBehaviorLeftAlone_ForAttributeOnProperty Assert.Equal(initialValue, context.BindingMetadata.IsBindingRequired); } + private class NonParseableClass + { + } + + private class CustomParseableClass + { + public static bool TryParse(string s, out CustomParseableClass value) + { + value = new CustomParseableClass(); + return true; + } + } + + [Theory] + [InlineData(typeof(int))] + [InlineData(typeof(int?))] + [InlineData(typeof(Guid))] + [InlineData(typeof(DateTime))] + [InlineData(typeof(DateTime?))] + [InlineData(typeof(System.Net.IPEndPoint))] + [InlineData(typeof(CustomParseableClass))] + public void CreateBindingDetails_DetectsParseableType_OnTypes(Type modelType) + { + // Arrange + var context = new BindingMetadataProviderContext( + ModelMetadataIdentity.ForType(modelType), + new ModelAttributes(new object[0], null, null)); + + var provider = new DefaultBindingMetadataProvider(new MvcOptions()); + + // Act + provider.CreateBindingMetadata(context); + + // Assert + Assert.True(context.BindingMetadata.IsBindingFromTryParseAllowed); + } + + [Theory] + [InlineData(typeof(string))] + [InlineData(typeof(object))] + [InlineData(typeof(NonParseableClass))] + [InlineData(typeof(RecordTypeWithPrimaryConstructor))] + public void CreateBindingDetails_SkipParseableType_OnTypes(Type modelType) + { + // Arrange + var context = new BindingMetadataProviderContext( + ModelMetadataIdentity.ForType(modelType), + new ModelAttributes(new object[0], null, null)); + + var provider = new DefaultBindingMetadataProvider(new MvcOptions()); + + // Act + provider.CreateBindingMetadata(context); + + // Assert + Assert.False(context.BindingMetadata.IsBindingFromTryParseAllowed); + } + + [Theory] + [InlineData(typeof(int))] + [InlineData(typeof(int?))] + [InlineData(typeof(Guid))] + [InlineData(typeof(DateTime))] + [InlineData(typeof(DateTime?))] + [InlineData(typeof(System.Net.IPEndPoint))] + [InlineData(typeof(CustomParseableClass))] + public void CreateBindingDetails_SkipParseableType_OnTypes_WhenSuppressParseableTypesBindingIsTrue(Type modelType) + { + // Arrange + var context = new BindingMetadataProviderContext( + ModelMetadataIdentity.ForType(modelType), + new ModelAttributes(new object[0], null, null)); + + var provider = new DefaultBindingMetadataProvider(new MvcOptions() { SuppressParseableTypesBinding = true }); + + // Act + provider.CreateBindingMetadata(context); + + // Assert + Assert.False(context.BindingMetadata.IsBindingFromTryParseAllowed); + } + + [Theory] + [InlineData(typeof(int))] + [InlineData(typeof(int?))] + [InlineData(typeof(Guid))] + [InlineData(typeof(DateTime))] + [InlineData(typeof(DateTime?))] + [InlineData(typeof(System.Net.IPEndPoint))] + [InlineData(typeof(CustomParseableClass))] + public void CreateBindingDetails_SkipParseableType_OnTypes_WhenOptionsIsNull(Type modelType) + { + // Arrange + var context = new BindingMetadataProviderContext( + ModelMetadataIdentity.ForType(modelType), + new ModelAttributes(new object[0], null, null)); + + var provider = new DefaultBindingMetadataProvider(); + + // Act + provider.CreateBindingMetadata(context); + + // Assert + Assert.False(context.BindingMetadata.IsBindingFromTryParseAllowed); + } + private class DefaultConstructorType { } [Fact] diff --git a/src/Mvc/Mvc.Core/test/ModelBinding/Metadata/DefaultModelMetadataTest.cs b/src/Mvc/Mvc.Core/test/ModelBinding/Metadata/DefaultModelMetadataTest.cs index 2d7329995f31..306dfbaf6dc2 100644 --- a/src/Mvc/Mvc.Core/test/ModelBinding/Metadata/DefaultModelMetadataTest.cs +++ b/src/Mvc/Mvc.Core/test/ModelBinding/Metadata/DefaultModelMetadataTest.cs @@ -339,6 +339,56 @@ public void IsBindingRequired_ReturnsFalse_ForTypes(Type modelType) Assert.False(isBindingRequired); } + [Fact] + public void IsParseableType_ReturnsTrue_ForType() + { + // Arrange + var provider = new EmptyModelMetadataProvider(); + var detailsProvider = new EmptyCompositeMetadataDetailsProvider(); + + var key = ModelMetadataIdentity.ForType(typeof(int)); + var cache = new DefaultMetadataDetails(key, new ModelAttributes(new object[0], null, null)) + { + BindingMetadata = new BindingMetadata() + { + IsBindingFromTryParseAllowed = true, + }, + }; + + var metadata = new DefaultModelMetadata(provider, detailsProvider, cache); + + // Act + var isParseableType = metadata.IsParseableType; + + // Assert + Assert.True(isParseableType); + } + + [Fact] + public void IsParseableType_ReturnsFalse_ForType() + { + // Arrange + var provider = new EmptyModelMetadataProvider(); + var detailsProvider = new EmptyCompositeMetadataDetailsProvider(); + + var key = ModelMetadataIdentity.ForType(typeof(string)); + var cache = new DefaultMetadataDetails(key, new ModelAttributes(new object[0], null, null)) + { + BindingMetadata = new BindingMetadata() + { + IsBindingFromTryParseAllowed = false, + }, + }; + + var metadata = new DefaultModelMetadata(provider, detailsProvider, cache); + + // Act + var isParseableType = metadata.IsParseableType; + + // Assert + Assert.False(isParseableType); + } + [Theory] [InlineData(typeof(string))] [InlineData(typeof(IDisposable))] diff --git a/src/Mvc/Mvc/test/MvcOptionsSetupTest.cs b/src/Mvc/Mvc/test/MvcOptionsSetupTest.cs index de091e37a918..79c83b8ec421 100644 --- a/src/Mvc/Mvc/test/MvcOptionsSetupTest.cs +++ b/src/Mvc/Mvc/test/MvcOptionsSetupTest.cs @@ -52,6 +52,7 @@ public void Setup_SetsUpModelBinderProviders() binder => Assert.IsType(binder), binder => Assert.IsType(binder), binder => Assert.IsType(binder), + binder => Assert.IsType(binder), binder => Assert.IsType(binder), binder => Assert.IsType(binder), binder => Assert.IsType(binder), From 50dfb9d622eadec5b6751ba4bb928116a8ca1f3a Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Thu, 17 Feb 2022 12:55:46 -0800 Subject: [PATCH 18/31] Fix typo --- src/Mvc/Mvc.Core/src/MvcOptions.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Mvc/Mvc.Core/src/MvcOptions.cs b/src/Mvc/Mvc.Core/src/MvcOptions.cs index 61fdbb28b9d6..787fb9a1494b 100644 --- a/src/Mvc/Mvc.Core/src/MvcOptions.cs +++ b/src/Mvc/Mvc.Core/src/MvcOptions.cs @@ -138,7 +138,7 @@ public MvcOptions() /// /// Gets or sets the flag that determines if binding is disabled for Parseable types (contains a TryParse method) - /// and will treat them as , in case, there isn't a from registered. + /// and will treat them as , in case, there isn't a from registered. /// public bool SuppressParseableTypesBinding { get; set; } From 748b0954fc6c7f83d449045d164817b3843a22e9 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Thu, 17 Feb 2022 14:07:31 -0800 Subject: [PATCH 19/31] fix commit style --- src/Mvc/Mvc.Core/src/MvcOptions.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Mvc/Mvc.Core/src/MvcOptions.cs b/src/Mvc/Mvc.Core/src/MvcOptions.cs index 787fb9a1494b..2d6014a5cc4b 100644 --- a/src/Mvc/Mvc.Core/src/MvcOptions.cs +++ b/src/Mvc/Mvc.Core/src/MvcOptions.cs @@ -138,7 +138,8 @@ public MvcOptions() /// /// Gets or sets the flag that determines if binding is disabled for Parseable types (contains a TryParse method) - /// and will treat them as , in case, there isn't a from registered. + /// and will treat them as , in case, + /// there isn't a from registered. /// public bool SuppressParseableTypesBinding { get; set; } From 83a28d23182ea0ceadb77073e972822e6ff77229 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Thu, 17 Feb 2022 14:49:25 -0800 Subject: [PATCH 20/31] Adding missed file --- .../test/ParameterBindingMethodCacheTests.cs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/Http/Http.Extensions/test/ParameterBindingMethodCacheTests.cs b/src/Http/Http.Extensions/test/ParameterBindingMethodCacheTests.cs index 0bbd48d1f9f3..7b56ccf6a70b 100644 --- a/src/Http/Http.Extensions/test/ParameterBindingMethodCacheTests.cs +++ b/src/Http/Http.Extensions/test/ParameterBindingMethodCacheTests.cs @@ -30,7 +30,7 @@ public void FindTryParseStringMethod_ReturnsTheExpectedTryParseMethodWithInvaria Assert.NotNull(methodFound); - var call = methodFound!(Expression.Variable(type, "parsedValue")) as MethodCallExpression; + var call = methodFound!(Expression.Variable(type, "parsedValue"), Expression.Constant(CultureInfo.InvariantCulture)) as MethodCallExpression; Assert.NotNull(call); var parameters = call!.Method.GetParameters(); @@ -53,7 +53,7 @@ public void FindTryParseStringMethod_ReturnsTheExpectedTryParseMethodWithInvaria Assert.NotNull(methodFound); - var call = methodFound!(Expression.Variable(type, "parsedValue")) as MethodCallExpression; + var call = methodFound!(Expression.Variable(type, "parsedValue"), Expression.Constant(CultureInfo.InvariantCulture)) as MethodCallExpression; Assert.NotNull(call); var parameters = call!.Method.GetParameters(); @@ -85,7 +85,7 @@ public void FindTryParseStringMethod_ReturnsTheExpectedTryParseMethodWithInvaria Assert.NotNull(methodFound); - var call = methodFound!(Expression.Variable(type, "parsedValue")) as MethodCallExpression; + var call = methodFound!(Expression.Variable(type, "parsedValue"), Expression.Constant(CultureInfo.InvariantCulture)) as MethodCallExpression; Assert.NotNull(call); var parameters = call!.Method.GetParameters(); @@ -109,7 +109,7 @@ public void FindTryParseMethod_WithNoFormatProvider(Type type) var methodFound = new ParameterBindingMethodCache().FindTryParseMethod(@type); Assert.NotNull(methodFound); - var call = methodFound!(Expression.Variable(type, "parsedValue")) as MethodCallExpression; + var call = methodFound!(Expression.Variable(type, "parsedValue"), Expression.Constant(CultureInfo.InvariantCulture)) as MethodCallExpression; Assert.NotNull(call); var parameters = call!.Method.GetParameters(); @@ -155,7 +155,7 @@ public void FindTryParseStringMethod_WorksForEnums() Assert.NotNull(methodFound); - var call = methodFound!(Expression.Variable(type, "parsedValue")) as MethodCallExpression; + var call = methodFound!(Expression.Variable(type, "parsedValue"), Expression.Constant(CultureInfo.InvariantCulture)) as MethodCallExpression; Assert.NotNull(call); var method = call!.Method; var parameters = method.GetParameters(); @@ -177,7 +177,7 @@ public void FindTryParseStringMethod_WorksForEnumsWhenNonGenericEnumParseIsUsed( Assert.NotNull(methodFound); var parsedValue = Expression.Variable(type, "parsedValue"); - var block = methodFound!(parsedValue) as BlockExpression; + var block = methodFound!(parsedValue, Expression.Constant(CultureInfo.InvariantCulture)) as BlockExpression; Assert.NotNull(block); Assert.Equal(typeof(bool), block!.Type); From a6e3939cac474e38055f6618b8fd7df021509f39 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Thu, 17 Feb 2022 16:18:54 -0800 Subject: [PATCH 21/31] Adding one more missed file --- .../shared/Mvc.Core.TestCommon/TestModelMetadataProvider.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Mvc/shared/Mvc.Core.TestCommon/TestModelMetadataProvider.cs b/src/Mvc/shared/Mvc.Core.TestCommon/TestModelMetadataProvider.cs index 4f3ba4ef774b..1ccf2279632d 100644 --- a/src/Mvc/shared/Mvc.Core.TestCommon/TestModelMetadataProvider.cs +++ b/src/Mvc/shared/Mvc.Core.TestCommon/TestModelMetadataProvider.cs @@ -32,7 +32,7 @@ public static ModelMetadataProvider CreateDefaultProvider(IStringLocalizerFactor new DataMemberRequiredBindingMetadataProvider(), }; - MvcCoreMvcOptionsSetup.ConfigureAdditionalModelMetadataDetailsProviders(detailsProviders); + MvcCoreMvcOptionsSetup.ConfigureAdditionalModelMetadataDetailsProviders(detailsProviders, new MvcOptions()); var validationProviders = TestModelValidatorProvider.CreateDefaultProvider(); detailsProviders.Add(new HasValidatorsValidationMetadataProvider(validationProviders.ValidatorProviders)); @@ -54,7 +54,7 @@ public static IModelMetadataProvider CreateDefaultProvider(IList Date: Tue, 22 Feb 2022 11:23:48 -0800 Subject: [PATCH 22/31] PR Feeback --- .../src/ModelBinding/ModelMetadata.cs | 6 +- .../ModelBinding/ModelStateDictionaryTest.cs | 15 ++-- .../Binders/TryParseModelBinder.cs | 3 +- .../Binders/TryParseModelBinderProvider.cs | 2 +- .../DefaultBindingMetadataProvider.cs | 6 +- src/Mvc/Mvc.Core/src/Resources.resx | 59 ++++++------ .../DefaultBindingMetadataProviderTest.cs | 89 +++++++------------ .../test/ModelMetadataProviderTest.cs | 2 +- .../TestModelMetadataProvider.cs | 6 +- 9 files changed, 87 insertions(+), 101 deletions(-) diff --git a/src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelMetadata.cs b/src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelMetadata.cs index ab544d74f113..3ff3a8f143a6 100644 --- a/src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelMetadata.cs +++ b/src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelMetadata.cs @@ -437,9 +437,9 @@ internal IReadOnlyDictionary BoundConstructorPrope /// Gets a value indicating whether is a complex type. /// /// - /// A complex type is defined as a that is NOT and NOT . - /// Most POCO and types are therefore complex. Most, if - /// not all, BCL value types are simple types. + /// A complex type is defined as a without a that can convert + /// from and without a TryParse method. Most POCO and types are therefore complex. + /// Most, if not all, BCL value types are simple types. /// public bool IsComplexType => !IsConvertibleType && !IsParseableType; diff --git a/src/Mvc/Mvc.Abstractions/test/ModelBinding/ModelStateDictionaryTest.cs b/src/Mvc/Mvc.Abstractions/test/ModelBinding/ModelStateDictionaryTest.cs index 70af69ea5a1d..48096dfe5bd0 100644 --- a/src/Mvc/Mvc.Abstractions/test/ModelBinding/ModelStateDictionaryTest.cs +++ b/src/Mvc/Mvc.Abstractions/test/ModelBinding/ModelStateDictionaryTest.cs @@ -994,7 +994,7 @@ public void ModelStateDictionary_AddsCustomErrorMessage_WhenModelStateNotSet() var expected = "Hmm, the supplied value is not valid for Length."; var dictionary = new ModelStateDictionary(); - var bindingMetadataProvider = new DefaultBindingMetadataProvider(); + var bindingMetadataProvider = CreateBindingMetadataProvider(); var compositeProvider = new DefaultCompositeMetadataDetailsProvider(new[] { bindingMetadataProvider }); var optionsAccessor = new OptionsAccessor(); optionsAccessor.Value.ModelBindingMessageProvider.SetUnknownValueIsInvalidAccessor( @@ -1020,7 +1020,7 @@ public void ModelStateDictionary_AddsCustomErrorMessage_WhenModelStateNotSet_Wit var expected = "Hmm, the supplied value is not valid."; var dictionary = new ModelStateDictionary(); - var bindingMetadataProvider = new DefaultBindingMetadataProvider(); + var bindingMetadataProvider = CreateBindingMetadataProvider(); var compositeProvider = new DefaultCompositeMetadataDetailsProvider(new[] { bindingMetadataProvider }); var optionsAccessor = new OptionsAccessor(); optionsAccessor.Value.ModelBindingMessageProvider.SetNonPropertyUnknownValueIsInvalidAccessor( @@ -1048,7 +1048,7 @@ public void ModelStateDictionary_AddsCustomErrorMessage_WhenModelStateNotSet_Wit var expected = "Hmm, the supplied value is not valid."; var dictionary = new ModelStateDictionary(); - var bindingMetadataProvider = new DefaultBindingMetadataProvider(); + var bindingMetadataProvider = CreateBindingMetadataProvider(); var compositeProvider = new DefaultCompositeMetadataDetailsProvider(new[] { bindingMetadataProvider }); var optionsAccessor = new OptionsAccessor(); optionsAccessor.Value.ModelBindingMessageProvider.SetNonPropertyUnknownValueIsInvalidAccessor( @@ -1109,7 +1109,7 @@ public void ModelStateDictionary_AddsCustomErrorMessage_WhenModelStateSet() var dictionary = new ModelStateDictionary(); dictionary.SetModelValue("key", new string[] { "some value" }, "some value"); - var bindingMetadataProvider = new DefaultBindingMetadataProvider(); + var bindingMetadataProvider = CreateBindingMetadataProvider(); var compositeProvider = new DefaultCompositeMetadataDetailsProvider(new[] { bindingMetadataProvider }); var optionsAccessor = new OptionsAccessor(); optionsAccessor.Value.ModelBindingMessageProvider.SetAttemptedValueIsInvalidAccessor( @@ -1136,7 +1136,7 @@ public void ModelStateDictionary_AddsCustomErrorMessage_WhenModelStateSet_WithPa var dictionary = new ModelStateDictionary(); dictionary.SetModelValue("key", new string[] { "some value" }, "some value"); - var bindingMetadataProvider = new DefaultBindingMetadataProvider(); + var bindingMetadataProvider = CreateBindingMetadataProvider(); var compositeProvider = new DefaultCompositeMetadataDetailsProvider(new[] { bindingMetadataProvider }); var optionsAccessor = new OptionsAccessor(); optionsAccessor.Value.ModelBindingMessageProvider.SetNonPropertyAttemptedValueIsInvalidAccessor( @@ -1165,7 +1165,7 @@ public void ModelStateDictionary_AddsCustomErrorMessage_WhenModelStateSet_WithTy var dictionary = new ModelStateDictionary(); dictionary.SetModelValue("key", new string[] { "some value" }, "some value"); - var bindingMetadataProvider = new DefaultBindingMetadataProvider(); + var bindingMetadataProvider = CreateBindingMetadataProvider(); var compositeProvider = new DefaultCompositeMetadataDetailsProvider(new[] { bindingMetadataProvider }); var optionsAccessor = new OptionsAccessor(); optionsAccessor.Value.ModelBindingMessageProvider.SetNonPropertyAttemptedValueIsInvalidAccessor( @@ -1599,6 +1599,9 @@ public void GetModelStateForProperty_ReturnsModelStateForIndexedChildren() Assert.Equal("value1", property.RawValue); } + private DefaultBindingMetadataProvider CreateBindingMetadataProvider(MvcOptions options = null) + => CreateBindingMetadataProvider(options ?? new MvcOptions()); + private class OptionsAccessor : IOptions { public MvcOptions Value { get; } = new MvcOptions(); diff --git a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinder.cs b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinder.cs index f38d9af783f4..a273b2d9d178 100644 --- a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinder.cs +++ b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinder.cs @@ -4,6 +4,7 @@ using System.Linq.Expressions; using System.Reflection; using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Mvc.Core; using Microsoft.Extensions.Logging; namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders; @@ -117,7 +118,7 @@ private static void AddModelError(ModelBindingContext bindingContext, Exception { modelType = Nullable.GetUnderlyingType(modelType) ?? modelType; var tryParseMethodExpession = ModelMetadata.FindTryParseMethod(modelType) - ?? throw new InvalidOperationException($"The type '{ modelType }' does not contains a TryParse method and the binder '{ nameof(TryParseModelBinder) }' cannot be used."); + ?? throw new InvalidOperationException(Resources.FormatTryParseModelBinder_InvalidType(modelType, nameof(TryParseModelBinder))); // var tempSourceString = valueProviderResult.FirstValue; // object model = null; diff --git a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinderProvider.cs b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinderProvider.cs index 5236500569f0..e41c21640f5c 100644 --- a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinderProvider.cs +++ b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinderProvider.cs @@ -14,7 +14,7 @@ public sealed class TryParseModelBinderProvider : IModelBinderProvider /// public IModelBinder? GetBinder(ModelBinderProviderContext context) { - if (context == null) + if (context is null) { throw new ArgumentNullException(nameof(context)); } diff --git a/src/Mvc/Mvc.Core/src/ModelBinding/Metadata/DefaultBindingMetadataProvider.cs b/src/Mvc/Mvc.Core/src/ModelBinding/Metadata/DefaultBindingMetadataProvider.cs index 95c933930afa..2bf531c97fe7 100644 --- a/src/Mvc/Mvc.Core/src/ModelBinding/Metadata/DefaultBindingMetadataProvider.cs +++ b/src/Mvc/Mvc.Core/src/ModelBinding/Metadata/DefaultBindingMetadataProvider.cs @@ -14,9 +14,9 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Metadata; /// internal class DefaultBindingMetadataProvider : IBindingMetadataProvider { - private readonly MvcOptions? _options; + private readonly MvcOptions _options; - public DefaultBindingMetadataProvider(MvcOptions? options = null) + public DefaultBindingMetadataProvider(MvcOptions options) { _options = options; } @@ -86,7 +86,7 @@ public void CreateBindingMetadata(BindingMetadataProviderContext context) context.BindingMetadata.BoundConstructor = constructorInfo; } - if (_options?.SuppressParseableTypesBinding is false && ModelMetadata.FindTryParseMethod(context.Key.ModelType) is not null) + if (!_options.SuppressParseableTypesBinding && ModelMetadata.FindTryParseMethod(context.Key.ModelType) is not null) { context.BindingMetadata.IsBindingFromTryParseAllowed = true; } diff --git a/src/Mvc/Mvc.Core/src/Resources.resx b/src/Mvc/Mvc.Core/src/Resources.resx index d3dc8246fc20..22ffe43067ad 100644 --- a/src/Mvc/Mvc.Core/src/Resources.resx +++ b/src/Mvc/Mvc.Core/src/Resources.resx @@ -1,17 +1,17 @@ - @@ -510,4 +510,7 @@ Could not parse '{0}'. Content types with wildcards are not supported. - + + The type '{0}' does not contains a TryParse method and the binder '{1}' cannot be used. + + \ No newline at end of file diff --git a/src/Mvc/Mvc.Core/test/ModelBinding/Metadata/DefaultBindingMetadataProviderTest.cs b/src/Mvc/Mvc.Core/test/ModelBinding/Metadata/DefaultBindingMetadataProviderTest.cs index 1792765112cf..0e0c364ed2be 100644 --- a/src/Mvc/Mvc.Core/test/ModelBinding/Metadata/DefaultBindingMetadataProviderTest.cs +++ b/src/Mvc/Mvc.Core/test/ModelBinding/Metadata/DefaultBindingMetadataProviderTest.cs @@ -22,7 +22,7 @@ public void CreateBindingDetails_FindsBinderTypeProvider() ModelMetadataIdentity.ForType(typeof(string)), new ModelAttributes(attributes, null, null)); - var provider = new DefaultBindingMetadataProvider(); + var provider = CreateBindingMetadataProvider(); // Act provider.CreateBindingMetadata(context); @@ -46,7 +46,7 @@ public void CreateBindingDetails_FindsBinderTypeProvider_IfNullFallsBack() ModelMetadataIdentity.ForType(typeof(string)), new ModelAttributes(attributes, null, null)); - var provider = new DefaultBindingMetadataProvider(); + var provider = CreateBindingMetadataProvider(); // Act provider.CreateBindingMetadata(context); @@ -69,7 +69,7 @@ public void CreateBindingDetails_FindsModelName() ModelMetadataIdentity.ForType(typeof(string)), new ModelAttributes(attributes, null, null)); - var provider = new DefaultBindingMetadataProvider(); + var provider = CreateBindingMetadataProvider(); // Act provider.CreateBindingMetadata(context); @@ -93,7 +93,7 @@ public void CreateBindingDetails_FindsModelName_IfNullFallsBack() ModelMetadataIdentity.ForType(typeof(string)), new ModelAttributes(attributes, null, null)); - var provider = new DefaultBindingMetadataProvider(); + var provider = CreateBindingMetadataProvider(); // Act provider.CreateBindingMetadata(context); @@ -116,7 +116,7 @@ public void CreateBindingDetails_FindsBindingSource() ModelMetadataIdentity.ForType(typeof(string)), new ModelAttributes(attributes, null, null)); - var provider = new DefaultBindingMetadataProvider(); + var provider = CreateBindingMetadataProvider(); // Act provider.CreateBindingMetadata(context); @@ -140,7 +140,7 @@ public void CreateBindingDetails_FindsBindingSource_IfNullFallsBack() ModelMetadataIdentity.ForType(typeof(string)), new ModelAttributes(attributes, null, null)); - var provider = new DefaultBindingMetadataProvider(); + var provider = CreateBindingMetadataProvider(); // Act provider.CreateBindingMetadata(context); @@ -162,7 +162,7 @@ public void CreateBindingDetails_FindsBindingBehaviorNever_OnProperty() ModelMetadataIdentity.ForProperty(typeof(string).GetProperty(nameof(string.Length)), typeof(int), typeof(string)), new ModelAttributes(new object[0], propertyAttributes, null)); - var provider = new DefaultBindingMetadataProvider(); + var provider = CreateBindingMetadataProvider(); // Act provider.CreateBindingMetadata(context); @@ -185,7 +185,7 @@ public void CreateBindingDetails_FindsBindNever_OnProperty() ModelMetadataIdentity.ForProperty(typeof(string).GetProperty(nameof(string.Length)), typeof(int), typeof(string)), new ModelAttributes(new object[0], propertyAttributes, null)); - var provider = new DefaultBindingMetadataProvider(); + var provider = CreateBindingMetadataProvider(); // Act provider.CreateBindingMetadata(context); @@ -208,7 +208,7 @@ public void CreateBindingDetails_FindsBindingBehaviorOptional_OnProperty() ModelMetadataIdentity.ForProperty(typeof(string).GetProperty(nameof(string.Length)), typeof(int), typeof(string)), new ModelAttributes(new object[0], propertyAttributes, null)); - var provider = new DefaultBindingMetadataProvider(); + var provider = CreateBindingMetadataProvider(); // Act provider.CreateBindingMetadata(context); @@ -231,7 +231,7 @@ public void CreateBindingDetails_FindsBindingBehaviorRequired_OnProperty() ModelMetadataIdentity.ForProperty(typeof(string).GetProperty(nameof(string.Length)), typeof(int), typeof(string)), new ModelAttributes(new object[0], propertyAttributes, null)); - var provider = new DefaultBindingMetadataProvider(); + var provider = CreateBindingMetadataProvider(); // Act provider.CreateBindingMetadata(context); @@ -254,7 +254,7 @@ public void CreateBindingDetails_FindsBindRequired_OnProperty() ModelMetadataIdentity.ForProperty(typeof(string).GetProperty(nameof(string.Length)), typeof(int), typeof(string)), new ModelAttributes(new object[0], propertyAttributes, null)); - var provider = new DefaultBindingMetadataProvider(); + var provider = CreateBindingMetadataProvider(); // Act provider.CreateBindingMetadata(context); @@ -277,7 +277,7 @@ public void CreateBindingDetails_FindsBindingBehaviorNever_OnParameter() ModelMetadataIdentity.ForParameter(ParameterInfos.SampleParameterInfo), new ModelAttributes(Array.Empty(), null, parameterAttributes)); - var provider = new DefaultBindingMetadataProvider(); + var provider = CreateBindingMetadataProvider(); // Act provider.CreateBindingMetadata(context); @@ -300,7 +300,7 @@ public void CreateBindingDetails_FindsBindNever_OnParameter() ModelMetadataIdentity.ForParameter(ParameterInfos.SampleParameterInfo), new ModelAttributes(Array.Empty(), null, parameterAttributes)); - var provider = new DefaultBindingMetadataProvider(); + var provider = CreateBindingMetadataProvider(); // Act provider.CreateBindingMetadata(context); @@ -323,7 +323,7 @@ public void CreateBindingDetails_FindsBindingBehaviorOptional_OnParameter() ModelMetadataIdentity.ForParameter(ParameterInfos.SampleParameterInfo), new ModelAttributes(Array.Empty(), null, parameterAttributes)); - var provider = new DefaultBindingMetadataProvider(); + var provider = CreateBindingMetadataProvider(); // Act provider.CreateBindingMetadata(context); @@ -346,7 +346,7 @@ public void CreateBindingDetails_FindsBindingBehaviorRequired_OnParameter() ModelMetadataIdentity.ForParameter(ParameterInfos.SampleParameterInfo), new ModelAttributes(Array.Empty(), null, parameterAttributes)); - var provider = new DefaultBindingMetadataProvider(); + var provider = CreateBindingMetadataProvider(); // Act provider.CreateBindingMetadata(context); @@ -369,7 +369,7 @@ public void CreateBindingDetails_FindsBindRequired_OnParameter() ModelMetadataIdentity.ForParameter(ParameterInfos.SampleParameterInfo), new ModelAttributes(Array.Empty(), null, parameterAttributes)); - var provider = new DefaultBindingMetadataProvider(); + var provider = CreateBindingMetadataProvider(); // Act provider.CreateBindingMetadata(context); @@ -393,7 +393,7 @@ public void CreateBindingDetails_FindsCustomAttributes_OnParameter() ModelMetadataIdentity.ForParameter(ParameterInfos.SampleParameterInfo), new ModelAttributes(Array.Empty(), null, parameterAttributes)); - var provider = new DefaultBindingMetadataProvider(); + var provider = CreateBindingMetadataProvider(); // Act provider.CreateBindingMetadata(context); @@ -421,7 +421,7 @@ public void CreateBindingDetails_UsesFirstAttribute() ModelMetadataIdentity.ForProperty(typeof(string).GetProperty(nameof(string.Length)), typeof(int), typeof(string)), new ModelAttributes(new object[0], propertyAttributes, null)); - var provider = new DefaultBindingMetadataProvider(); + var provider = CreateBindingMetadataProvider(); // Act provider.CreateBindingMetadata(context); @@ -439,7 +439,7 @@ public void CreateBindingDetails_FindsBindRequired_OnContainerClass() ModelMetadataIdentity.ForProperty(typeof(BindRequiredOnClass).GetProperty(nameof(BindRequiredOnClass.Property)), typeof(int), typeof(BindRequiredOnClass)), new ModelAttributes(new object[0], new object[0], null)); - var provider = new DefaultBindingMetadataProvider(); + var provider = CreateBindingMetadataProvider(); // Act provider.CreateBindingMetadata(context); @@ -457,7 +457,7 @@ public void CreateBindingDetails_FindsBindNever_OnContainerClass() ModelMetadataIdentity.ForProperty(typeof(BindNeverOnClass).GetProperty(nameof(BindNeverOnClass.Property)), typeof(int), typeof(BindNeverOnClass)), new ModelAttributes(new object[0], new object[0], null)); - var provider = new DefaultBindingMetadataProvider(); + var provider = CreateBindingMetadataProvider(); // Act provider.CreateBindingMetadata(context); @@ -475,7 +475,7 @@ public void CreateBindingDetails_FindsBindNever_OnBaseClass() ModelMetadataIdentity.ForProperty(typeof(BindNeverOnClass).GetProperty(nameof(BindNeverOnClass.Property)), typeof(int), typeof(BindNeverOnClass)), new ModelAttributes(new object[0], new object[0], null)); - var provider = new DefaultBindingMetadataProvider(); + var provider = CreateBindingMetadataProvider(); // Act provider.CreateBindingMetadata(context); @@ -498,7 +498,7 @@ public void CreateBindingDetails_OverrideBehaviorOnClass_OverrideWithOptional() ModelMetadataIdentity.ForProperty(typeof(BindNeverOnClass).GetProperty(nameof(BindNeverOnClass.Property)), typeof(int), typeof(BindNeverOnClass)), new ModelAttributes(new object[0], propertyAttributes, null)); - var provider = new DefaultBindingMetadataProvider(); + var provider = CreateBindingMetadataProvider(); // Act provider.CreateBindingMetadata(context); @@ -521,7 +521,7 @@ public void CreateBindingDetails_OverrideBehaviorOnClass_OverrideWithRequired() ModelMetadataIdentity.ForProperty(typeof(BindNeverOnClass).GetProperty(nameof(BindNeverOnClass.Property)), typeof(int), typeof(BindNeverOnClass)), new ModelAttributes(new object[0], propertyAttributes, null)); - var provider = new DefaultBindingMetadataProvider(); + var provider = CreateBindingMetadataProvider(); // Act provider.CreateBindingMetadata(context); @@ -544,7 +544,7 @@ public void CreateBindingDetails_OverrideInheritedBehaviorOnClass_OverrideWithRe ModelMetadataIdentity.ForProperty(typeof(InheritedBindNeverOnClass).GetProperty(nameof(InheritedBindNeverOnClass.Property)), typeof(int), typeof(InheritedBindNeverOnClass)), new ModelAttributes(new object[0], propertyAttributes, null)); - var provider = new DefaultBindingMetadataProvider(); + var provider = CreateBindingMetadataProvider(); // Act provider.CreateBindingMetadata(context); @@ -567,7 +567,7 @@ public void CreateBindingDetails_OverrideBehaviorOnClass_OverrideWithNever() ModelMetadataIdentity.ForProperty(typeof(BindRequiredOnClass).GetProperty(nameof(BindRequiredOnClass.Property)), typeof(int), typeof(BindRequiredOnClass)), new ModelAttributes(new object[0], propertyAttributes, null)); - var provider = new DefaultBindingMetadataProvider(); + var provider = CreateBindingMetadataProvider(); // Act provider.CreateBindingMetadata(context); @@ -586,7 +586,7 @@ public void CreateBindingDetails_OverrideBehaviorOnBaseClass_OverrideWithRequire ModelMetadataIdentity.ForProperty(typeof(BindRequiredOverridesInheritedBindNever).GetProperty(nameof(BindRequiredOverridesInheritedBindNever.Property)), typeof(int), typeof(BindRequiredOverridesInheritedBindNever)), new ModelAttributes(new object[0], new object[0], null)); - var provider = new DefaultBindingMetadataProvider(); + var provider = CreateBindingMetadataProvider(); // Act provider.CreateBindingMetadata(context); @@ -615,7 +615,7 @@ public void CreateBindingDetails_BindingBehaviorLeftAlone_ForTypeMetadata(bool i context.BindingMetadata.IsBindingAllowed = initialValue; context.BindingMetadata.IsBindingRequired = initialValue; - var provider = new DefaultBindingMetadataProvider(); + var provider = CreateBindingMetadataProvider(); // Act provider.CreateBindingMetadata(context); @@ -646,7 +646,7 @@ public void CreateBindingDetails_BindingBehaviorLeftAlone_ForAttributeOnProperty context.BindingMetadata.IsBindingAllowed = initialValue; context.BindingMetadata.IsBindingRequired = initialValue; - var provider = new DefaultBindingMetadataProvider(); + var provider = CreateBindingMetadataProvider(); // Act provider.CreateBindingMetadata(context); @@ -684,7 +684,7 @@ public void CreateBindingDetails_DetectsParseableType_OnTypes(Type modelType) ModelMetadataIdentity.ForType(modelType), new ModelAttributes(new object[0], null, null)); - var provider = new DefaultBindingMetadataProvider(new MvcOptions()); + var provider = CreateBindingMetadataProvider(); // Act provider.CreateBindingMetadata(context); @@ -705,7 +705,7 @@ public void CreateBindingDetails_SkipParseableType_OnTypes(Type modelType) ModelMetadataIdentity.ForType(modelType), new ModelAttributes(new object[0], null, null)); - var provider = new DefaultBindingMetadataProvider(new MvcOptions()); + var provider = CreateBindingMetadataProvider(); // Act provider.CreateBindingMetadata(context); @@ -729,31 +729,7 @@ public void CreateBindingDetails_SkipParseableType_OnTypes_WhenSuppressParseable ModelMetadataIdentity.ForType(modelType), new ModelAttributes(new object[0], null, null)); - var provider = new DefaultBindingMetadataProvider(new MvcOptions() { SuppressParseableTypesBinding = true }); - - // Act - provider.CreateBindingMetadata(context); - - // Assert - Assert.False(context.BindingMetadata.IsBindingFromTryParseAllowed); - } - - [Theory] - [InlineData(typeof(int))] - [InlineData(typeof(int?))] - [InlineData(typeof(Guid))] - [InlineData(typeof(DateTime))] - [InlineData(typeof(DateTime?))] - [InlineData(typeof(System.Net.IPEndPoint))] - [InlineData(typeof(CustomParseableClass))] - public void CreateBindingDetails_SkipParseableType_OnTypes_WhenOptionsIsNull(Type modelType) - { - // Arrange - var context = new BindingMetadataProviderContext( - ModelMetadataIdentity.ForType(modelType), - new ModelAttributes(new object[0], null, null)); - - var provider = new DefaultBindingMetadataProvider(); + var provider = CreateBindingMetadataProvider(new MvcOptions() { SuppressParseableTypesBinding = true }); // Act provider.CreateBindingMetadata(context); @@ -1019,4 +995,7 @@ private class CustomAttribute : Attribute { public string Identifier { get; set; } } + + private DefaultBindingMetadataProvider CreateBindingMetadataProvider(MvcOptions options = null) + => CreateBindingMetadataProvider(options ?? new MvcOptions()); } diff --git a/src/Mvc/Mvc.DataAnnotations/test/ModelMetadataProviderTest.cs b/src/Mvc/Mvc.DataAnnotations/test/ModelMetadataProviderTest.cs index 6fd128a4a232..e494446a5d1b 100644 --- a/src/Mvc/Mvc.DataAnnotations/test/ModelMetadataProviderTest.cs +++ b/src/Mvc/Mvc.DataAnnotations/test/ModelMetadataProviderTest.cs @@ -1045,7 +1045,7 @@ public AttributeInjectModelMetadataProvider(object[] attributes) : base( new DefaultCompositeMetadataDetailsProvider(new IMetadataDetailsProvider[] { - new DefaultBindingMetadataProvider(), + new DefaultBindingMetadataProvider(new MvcOptions()), new DataAnnotationsMetadataProvider( new MvcOptions(), Options.Create(new MvcDataAnnotationsLocalizationOptions()), diff --git a/src/Mvc/shared/Mvc.Core.TestCommon/TestModelMetadataProvider.cs b/src/Mvc/shared/Mvc.Core.TestCommon/TestModelMetadataProvider.cs index 1ccf2279632d..572dfd8c254d 100644 --- a/src/Mvc/shared/Mvc.Core.TestCommon/TestModelMetadataProvider.cs +++ b/src/Mvc/shared/Mvc.Core.TestCommon/TestModelMetadataProvider.cs @@ -26,7 +26,7 @@ public static ModelMetadataProvider CreateDefaultProvider(IStringLocalizerFactor { var detailsProviders = new List { - new DefaultBindingMetadataProvider(), + new DefaultBindingMetadataProvider(new MvcOptions()), new DefaultValidationMetadataProvider(), CreateDefaultDataAnnotationsProvider(stringLocalizerFactory), new DataMemberRequiredBindingMetadataProvider(), @@ -45,7 +45,7 @@ public static IModelMetadataProvider CreateDefaultProvider(IList() { - new DefaultBindingMetadataProvider(), + new DefaultBindingMetadataProvider(new MvcOptions()), new DefaultValidationMetadataProvider(), new DataAnnotationsMetadataProvider( new MvcOptions(), @@ -88,7 +88,7 @@ private TestModelMetadataProvider(TestModelMetadataDetailsProvider detailsProvid : base( new DefaultCompositeMetadataDetailsProvider(new IMetadataDetailsProvider[] { - new DefaultBindingMetadataProvider(), + new DefaultBindingMetadataProvider(new MvcOptions()), new DefaultValidationMetadataProvider(), new DataAnnotationsMetadataProvider( new MvcOptions(), From d6a8c52f86e6c8c2102c4d65db714ef7df8980ff Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Wed, 23 Feb 2022 09:28:25 -0800 Subject: [PATCH 23/31] Fixing unit test --- .../test/ModelBinding/ModelStateDictionaryTest.cs | 2 +- .../ModelBinding/Metadata/DefaultBindingMetadataProviderTest.cs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Mvc/Mvc.Abstractions/test/ModelBinding/ModelStateDictionaryTest.cs b/src/Mvc/Mvc.Abstractions/test/ModelBinding/ModelStateDictionaryTest.cs index 48096dfe5bd0..98f0fc03ae4a 100644 --- a/src/Mvc/Mvc.Abstractions/test/ModelBinding/ModelStateDictionaryTest.cs +++ b/src/Mvc/Mvc.Abstractions/test/ModelBinding/ModelStateDictionaryTest.cs @@ -1600,7 +1600,7 @@ public void GetModelStateForProperty_ReturnsModelStateForIndexedChildren() } private DefaultBindingMetadataProvider CreateBindingMetadataProvider(MvcOptions options = null) - => CreateBindingMetadataProvider(options ?? new MvcOptions()); + => new DefaultBindingMetadataProvider(options ?? new MvcOptions()); private class OptionsAccessor : IOptions { diff --git a/src/Mvc/Mvc.Core/test/ModelBinding/Metadata/DefaultBindingMetadataProviderTest.cs b/src/Mvc/Mvc.Core/test/ModelBinding/Metadata/DefaultBindingMetadataProviderTest.cs index 0e0c364ed2be..abd033818fd0 100644 --- a/src/Mvc/Mvc.Core/test/ModelBinding/Metadata/DefaultBindingMetadataProviderTest.cs +++ b/src/Mvc/Mvc.Core/test/ModelBinding/Metadata/DefaultBindingMetadataProviderTest.cs @@ -997,5 +997,5 @@ private class CustomAttribute : Attribute } private DefaultBindingMetadataProvider CreateBindingMetadataProvider(MvcOptions options = null) - => CreateBindingMetadataProvider(options ?? new MvcOptions()); + => new DefaultBindingMetadataProvider(options ?? new MvcOptions()); } From 01c0dd6deac58fdc66f2b06c708c89f332b4ef4b Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Wed, 23 Feb 2022 12:37:40 -0800 Subject: [PATCH 24/31] Update src/Mvc/Mvc.Core/src/Resources.resx Co-authored-by: Brennan --- src/Mvc/Mvc.Core/src/Resources.resx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Mvc/Mvc.Core/src/Resources.resx b/src/Mvc/Mvc.Core/src/Resources.resx index 22ffe43067ad..206a2f420b5c 100644 --- a/src/Mvc/Mvc.Core/src/Resources.resx +++ b/src/Mvc/Mvc.Core/src/Resources.resx @@ -511,6 +511,6 @@ Could not parse '{0}'. Content types with wildcards are not supported. - The type '{0}' does not contains a TryParse method and the binder '{1}' cannot be used. + The type '{0}' does not contain a TryParse method and the binder '{1}' cannot be used. \ No newline at end of file From b730ad427367e3687d85bff78de3aebf780aece2 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Wed, 23 Feb 2022 15:48:46 -0800 Subject: [PATCH 25/31] Updating test --- .../test/ModelBinding/Metadata/DefaultModelMetadataTest.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Mvc/Mvc.Core/test/ModelBinding/Metadata/DefaultModelMetadataTest.cs b/src/Mvc/Mvc.Core/test/ModelBinding/Metadata/DefaultModelMetadataTest.cs index 306dfbaf6dc2..e2b0639c4449 100644 --- a/src/Mvc/Mvc.Core/test/ModelBinding/Metadata/DefaultModelMetadataTest.cs +++ b/src/Mvc/Mvc.Core/test/ModelBinding/Metadata/DefaultModelMetadataTest.cs @@ -340,7 +340,7 @@ public void IsBindingRequired_ReturnsFalse_ForTypes(Type modelType) } [Fact] - public void IsParseableType_ReturnsTrue_ForType() + public void IsParseableType_ReturnsTrue_WhenTryParseAllowed() { // Arrange var provider = new EmptyModelMetadataProvider(); @@ -365,7 +365,7 @@ public void IsParseableType_ReturnsTrue_ForType() } [Fact] - public void IsParseableType_ReturnsFalse_ForType() + public void IsParseableType_ReturnsFalse_WhenTryParseNotAllowed() { // Arrange var provider = new EmptyModelMetadataProvider(); From d1203e6342c76a61af1d0e4096e4bea1158937da Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Thu, 24 Feb 2022 10:08:45 -0800 Subject: [PATCH 26/31] Moving TryParse method check to DefaultModelMetadata --- .../src/ModelBinding/ModelMetadata.cs | 15 +++- .../DefaultBindingMetadataProvider.cs | 5 +- .../Metadata/DefaultModelMetadata.cs | 3 +- .../TryParseTypeModelBinderProviderTest.cs | 13 +--- .../DefaultBindingMetadataProviderTest.cs | 70 +++-------------- .../Metadata/DefaultModelMetadataTest.cs | 75 +++++++++++++++++-- 6 files changed, 99 insertions(+), 82 deletions(-) diff --git a/src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelMetadata.cs b/src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelMetadata.cs index 3ff3a8f143a6..1acc93db2eeb 100644 --- a/src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelMetadata.cs +++ b/src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelMetadata.cs @@ -537,7 +537,20 @@ internal void ThrowIfRecordTypeHasValidationOnProperties() } internal static Func? FindTryParseMethod(Type modelType) - => ParameterBindingMethodCache.FindTryParseMethod(Nullable.GetUnderlyingType(modelType) ?? modelType); + { + try + { + modelType = Nullable.GetUnderlyingType(modelType) ?? modelType; + return ParameterBindingMethodCache.FindTryParseMethod(modelType); + } + catch (InvalidOperationException) + { + // The ParameterBindingMethodCache.FindTryParseMethod throws an exception + // when an wrong try/parse method is detected + // but we don't need this behavior here and return null is enough + return null; + } + } [MemberNotNull(nameof(_parameterMapping), nameof(_boundConstructorPropertyMapping))] private void CalculateRecordTypeConstructorDetails() diff --git a/src/Mvc/Mvc.Core/src/ModelBinding/Metadata/DefaultBindingMetadataProvider.cs b/src/Mvc/Mvc.Core/src/ModelBinding/Metadata/DefaultBindingMetadataProvider.cs index 2bf531c97fe7..81b5ecd2b9c6 100644 --- a/src/Mvc/Mvc.Core/src/ModelBinding/Metadata/DefaultBindingMetadataProvider.cs +++ b/src/Mvc/Mvc.Core/src/ModelBinding/Metadata/DefaultBindingMetadataProvider.cs @@ -86,10 +86,7 @@ public void CreateBindingMetadata(BindingMetadataProviderContext context) context.BindingMetadata.BoundConstructor = constructorInfo; } - if (!_options.SuppressParseableTypesBinding && ModelMetadata.FindTryParseMethod(context.Key.ModelType) is not null) - { - context.BindingMetadata.IsBindingFromTryParseAllowed = true; - } + context.BindingMetadata.IsBindingFromTryParseAllowed = !_options.SuppressParseableTypesBinding; } internal static ConstructorInfo? GetBoundConstructor(Type type) diff --git a/src/Mvc/Mvc.Core/src/ModelBinding/Metadata/DefaultModelMetadata.cs b/src/Mvc/Mvc.Core/src/ModelBinding/Metadata/DefaultModelMetadata.cs index 5083fad9ea11..d887dcbc86df 100644 --- a/src/Mvc/Mvc.Core/src/ModelBinding/Metadata/DefaultModelMetadata.cs +++ b/src/Mvc/Mvc.Core/src/ModelBinding/Metadata/DefaultModelMetadata.cs @@ -347,7 +347,8 @@ public override bool IsRequired } } - internal override bool IsParseableType => BindingMetadata.IsBindingFromTryParseAllowed; + internal override bool IsParseableType + => BindingMetadata.IsBindingFromTryParseAllowed && ModelMetadata.FindTryParseMethod(ModelType) is not null; /// public override ModelBindingMessageProvider ModelBindingMessageProvider => diff --git a/src/Mvc/Mvc.Core/test/ModelBinding/Binders/TryParseTypeModelBinderProviderTest.cs b/src/Mvc/Mvc.Core/test/ModelBinding/Binders/TryParseTypeModelBinderProviderTest.cs index bd08c44c9ecb..1cda72ab5fcd 100644 --- a/src/Mvc/Mvc.Core/test/ModelBinding/Binders/TryParseTypeModelBinderProviderTest.cs +++ b/src/Mvc/Mvc.Core/test/ModelBinding/Binders/TryParseTypeModelBinderProviderTest.cs @@ -13,7 +13,7 @@ public void Create_ForTypesWithoutTryParse_ReturnsNull() { // Arrange var provider = new TryParseModelBinderProvider(); - var context = CreateContext(modelType: typeof(TestClass), false); + var context = new TestModelBinderProviderContext(typeof(TestClass)); // Act var result = provider.GetBinder(context); @@ -27,7 +27,7 @@ public void Create_ForTypesWithTryParse_ReturnsBinder() { // Arrange var provider = new TryParseModelBinderProvider(); - var context = CreateContext(typeof(TestClassWithTryParse), true); + var context = new TestModelBinderProviderContext(typeof(TestClassWithTryParse)); // Act var result = provider.GetBinder(context); @@ -36,15 +36,6 @@ public void Create_ForTypesWithTryParse_ReturnsBinder() Assert.IsType(result); } - private static TestModelBinderProviderContext CreateContext(Type modelType, bool allowTryParse) - { - TestModelBinderProviderContext.CachedMetadataProvider - .ForType(modelType) - .BindingDetails(b => b.IsBindingFromTryParseAllowed = allowTryParse); - - return new TestModelBinderProviderContext(modelType); - } - private class TestClass { } diff --git a/src/Mvc/Mvc.Core/test/ModelBinding/Metadata/DefaultBindingMetadataProviderTest.cs b/src/Mvc/Mvc.Core/test/ModelBinding/Metadata/DefaultBindingMetadataProviderTest.cs index abd033818fd0..fce72fc925a1 100644 --- a/src/Mvc/Mvc.Core/test/ModelBinding/Metadata/DefaultBindingMetadataProviderTest.cs +++ b/src/Mvc/Mvc.Core/test/ModelBinding/Metadata/DefaultBindingMetadataProviderTest.cs @@ -656,56 +656,15 @@ public void CreateBindingDetails_BindingBehaviorLeftAlone_ForAttributeOnProperty Assert.Equal(initialValue, context.BindingMetadata.IsBindingRequired); } - private class NonParseableClass - { - } - - private class CustomParseableClass - { - public static bool TryParse(string s, out CustomParseableClass value) - { - value = new CustomParseableClass(); - return true; - } - } - - [Theory] - [InlineData(typeof(int))] - [InlineData(typeof(int?))] - [InlineData(typeof(Guid))] - [InlineData(typeof(DateTime))] - [InlineData(typeof(DateTime?))] - [InlineData(typeof(System.Net.IPEndPoint))] - [InlineData(typeof(CustomParseableClass))] - public void CreateBindingDetails_DetectsParseableType_OnTypes(Type modelType) - { - // Arrange - var context = new BindingMetadataProviderContext( - ModelMetadataIdentity.ForType(modelType), - new ModelAttributes(new object[0], null, null)); - - var provider = CreateBindingMetadataProvider(); - - // Act - provider.CreateBindingMetadata(context); - - // Assert - Assert.True(context.BindingMetadata.IsBindingFromTryParseAllowed); - } - - [Theory] - [InlineData(typeof(string))] - [InlineData(typeof(object))] - [InlineData(typeof(NonParseableClass))] - [InlineData(typeof(RecordTypeWithPrimaryConstructor))] - public void CreateBindingDetails_SkipParseableType_OnTypes(Type modelType) + [Fact] + public void CreateBindingDetails_IsBindingFromTryParseNotAllowed_WhenSuppressParseableTypesBindingIsFalse() { // Arrange var context = new BindingMetadataProviderContext( - ModelMetadataIdentity.ForType(modelType), - new ModelAttributes(new object[0], null, null)); + ModelMetadataIdentity.ForType(typeof(int)), + new ModelAttributes(Array.Empty(), null, null)); - var provider = CreateBindingMetadataProvider(); + var provider = CreateBindingMetadataProvider(new MvcOptions() { SuppressParseableTypesBinding = false }); // Act provider.CreateBindingMetadata(context); @@ -714,28 +673,21 @@ public void CreateBindingDetails_SkipParseableType_OnTypes(Type modelType) Assert.False(context.BindingMetadata.IsBindingFromTryParseAllowed); } - [Theory] - [InlineData(typeof(int))] - [InlineData(typeof(int?))] - [InlineData(typeof(Guid))] - [InlineData(typeof(DateTime))] - [InlineData(typeof(DateTime?))] - [InlineData(typeof(System.Net.IPEndPoint))] - [InlineData(typeof(CustomParseableClass))] - public void CreateBindingDetails_SkipParseableType_OnTypes_WhenSuppressParseableTypesBindingIsTrue(Type modelType) + [Fact] + public void CreateBindingDetails_IsBindingFromTryParseAllowed_WhenSuppressParseableTypesBindingIsTrue() { // Arrange var context = new BindingMetadataProviderContext( - ModelMetadataIdentity.ForType(modelType), - new ModelAttributes(new object[0], null, null)); + ModelMetadataIdentity.ForType(typeof(int)), + new ModelAttributes(Array.Empty(), null, null)); - var provider = CreateBindingMetadataProvider(new MvcOptions() { SuppressParseableTypesBinding = true }); + var provider = CreateBindingMetadataProvider(new MvcOptions() { SuppressParseableTypesBinding = true }); // Act provider.CreateBindingMetadata(context); // Assert - Assert.False(context.BindingMetadata.IsBindingFromTryParseAllowed); + Assert.True(context.BindingMetadata.IsBindingFromTryParseAllowed); } private class DefaultConstructorType { } diff --git a/src/Mvc/Mvc.Core/test/ModelBinding/Metadata/DefaultModelMetadataTest.cs b/src/Mvc/Mvc.Core/test/ModelBinding/Metadata/DefaultModelMetadataTest.cs index e2b0639c4449..bff8ae2a4ebe 100644 --- a/src/Mvc/Mvc.Core/test/ModelBinding/Metadata/DefaultModelMetadataTest.cs +++ b/src/Mvc/Mvc.Core/test/ModelBinding/Metadata/DefaultModelMetadataTest.cs @@ -339,15 +339,23 @@ public void IsBindingRequired_ReturnsFalse_ForTypes(Type modelType) Assert.False(isBindingRequired); } - [Fact] - public void IsParseableType_ReturnsTrue_WhenTryParseAllowed() + [Theory] + [InlineData(typeof(int))] + [InlineData(typeof(int?))] + [InlineData(typeof(Guid))] + [InlineData(typeof(DateTime))] + [InlineData(typeof(DateTime?))] + [InlineData(typeof(System.Net.IPEndPoint))] + [InlineData(typeof(TypeWithTryParse))] + [InlineData(typeof(TypeWithTryParseAndIFormatProvider))] + public void IsParseableType_ReturnsTrue_ForParseableTypes(Type modelType) { // Arrange var provider = new EmptyModelMetadataProvider(); var detailsProvider = new EmptyCompositeMetadataDetailsProvider(); - var key = ModelMetadataIdentity.ForType(typeof(int)); - var cache = new DefaultMetadataDetails(key, new ModelAttributes(new object[0], null, null)) + var key = ModelMetadataIdentity.ForType(modelType); + var cache = new DefaultMetadataDetails(key, new ModelAttributes(Array.Empty(), null, null)) { BindingMetadata = new BindingMetadata() { @@ -364,6 +372,35 @@ public void IsParseableType_ReturnsTrue_WhenTryParseAllowed() Assert.True(isParseableType); } + [Theory] + [InlineData(typeof(string))] + [InlineData(typeof(object))] + [InlineData(typeof(TypeWithInvalidTryParse))] + [InlineData(typeof(TypeWithProperties))] + public void IsParseableType_ReturnsFalse_ForNonParseableTypes(Type modelType) + { + // Arrange + var provider = new EmptyModelMetadataProvider(); + var detailsProvider = new EmptyCompositeMetadataDetailsProvider(); + + var key = ModelMetadataIdentity.ForType(modelType); + var cache = new DefaultMetadataDetails(key, new ModelAttributes(Array.Empty(), null, null)) + { + BindingMetadata = new BindingMetadata() + { + IsBindingFromTryParseAllowed = true, + }, + }; + + var metadata = new DefaultModelMetadata(provider, detailsProvider, cache); + + // Act + var isParseableType = metadata.IsParseableType; + + // Assert + Assert.False(isParseableType); + } + [Fact] public void IsParseableType_ReturnsFalse_WhenTryParseNotAllowed() { @@ -371,8 +408,8 @@ public void IsParseableType_ReturnsFalse_WhenTryParseNotAllowed() var provider = new EmptyModelMetadataProvider(); var detailsProvider = new EmptyCompositeMetadataDetailsProvider(); - var key = ModelMetadataIdentity.ForType(typeof(string)); - var cache = new DefaultMetadataDetails(key, new ModelAttributes(new object[0], null, null)) + var key = ModelMetadataIdentity.ForType(typeof(int)); + var cache = new DefaultMetadataDetails(key, new ModelAttributes(Array.Empty(), null, null)) { BindingMetadata = new BindingMetadata() { @@ -1593,6 +1630,32 @@ private class TypeWithProperties public int PublicGetPublicSetProperty { get; set; } } + private class TypeWithInvalidTryParse + { + public static bool TryParse(string s) + { + return true; + } + } + + private class TypeWithTryParse + { + public static bool TryParse(string s, out TypeWithTryParse result) + { + result = new TypeWithTryParse(); + return true; + } + } + + private class TypeWithTryParseAndIFormatProvider + { + public static bool TryParse(string s, IFormatProvider provider, out TypeWithTryParseAndIFormatProvider result) + { + result = new TypeWithTryParseAndIFormatProvider(); + return true; + } + } + public class Employee { public int Id { get; set; } From 47708fee752fbefd821650213d99f0c3c89db34b Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Thu, 24 Feb 2022 10:25:20 -0800 Subject: [PATCH 27/31] Fixing unittest --- .../Metadata/DefaultBindingMetadataProviderTest.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Mvc/Mvc.Core/test/ModelBinding/Metadata/DefaultBindingMetadataProviderTest.cs b/src/Mvc/Mvc.Core/test/ModelBinding/Metadata/DefaultBindingMetadataProviderTest.cs index fce72fc925a1..52ca7bcd4b15 100644 --- a/src/Mvc/Mvc.Core/test/ModelBinding/Metadata/DefaultBindingMetadataProviderTest.cs +++ b/src/Mvc/Mvc.Core/test/ModelBinding/Metadata/DefaultBindingMetadataProviderTest.cs @@ -657,14 +657,14 @@ public void CreateBindingDetails_BindingBehaviorLeftAlone_ForAttributeOnProperty } [Fact] - public void CreateBindingDetails_IsBindingFromTryParseNotAllowed_WhenSuppressParseableTypesBindingIsFalse() + public void CreateBindingDetails_IsBindingFromTryParseNotAllowed_WhenSuppressParseableTypesBindingIsTrue() { // Arrange var context = new BindingMetadataProviderContext( ModelMetadataIdentity.ForType(typeof(int)), new ModelAttributes(Array.Empty(), null, null)); - var provider = CreateBindingMetadataProvider(new MvcOptions() { SuppressParseableTypesBinding = false }); + var provider = CreateBindingMetadataProvider(new MvcOptions() { SuppressParseableTypesBinding = true }); // Act provider.CreateBindingMetadata(context); @@ -674,14 +674,14 @@ public void CreateBindingDetails_IsBindingFromTryParseNotAllowed_WhenSuppressPar } [Fact] - public void CreateBindingDetails_IsBindingFromTryParseAllowed_WhenSuppressParseableTypesBindingIsTrue() + public void CreateBindingDetails_IsBindingFromTryParseAllowed_WhenSuppressParseableTypesBindingIsFalse() { // Arrange var context = new BindingMetadataProviderContext( ModelMetadataIdentity.ForType(typeof(int)), new ModelAttributes(Array.Empty(), null, null)); - var provider = CreateBindingMetadataProvider(new MvcOptions() { SuppressParseableTypesBinding = true }); + var provider = CreateBindingMetadataProvider(new MvcOptions() { SuppressParseableTypesBinding = false }); // Act provider.CreateBindingMetadata(context); From 5d3c7b100058913c8489d35cb8c81bc6a652baa9 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Mon, 28 Feb 2022 13:27:19 -0800 Subject: [PATCH 28/31] API Review: Removing SuppressParseableTypeBinding option --- .../src/ModelBinding/ModelMetadata.cs | 3 +- .../ModelBinding/Metadata/BindingMetadata.cs | 10 ----- .../DefaultBindingMetadataProvider.cs | 2 - .../Metadata/DefaultModelMetadata.cs | 3 -- src/Mvc/Mvc.Core/src/MvcOptions.cs | 7 ---- src/Mvc/Mvc.Core/src/PublicAPI.Unshipped.txt | 2 - .../DefaultBindingMetadataProviderTest.cs | 34 --------------- .../Metadata/DefaultModelMetadataTest.cs | 41 +------------------ 8 files changed, 4 insertions(+), 98 deletions(-) diff --git a/src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelMetadata.cs b/src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelMetadata.cs index 1acc93db2eeb..f38550133f64 100644 --- a/src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelMetadata.cs +++ b/src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelMetadata.cs @@ -482,7 +482,7 @@ internal IReadOnlyDictionary BoundConstructorPrope /// /// Gets a value indicating whether or not has a TryParse method. /// - internal virtual bool IsParseableType { get; } + internal virtual bool IsParseableType { get; private set; } /// /// Gets a value indicating whether or not has a @@ -648,6 +648,7 @@ private void InitializeTypeInformation() Debug.Assert(ModelType != null); IsConvertibleType = TypeDescriptor.GetConverter(ModelType).CanConvertFrom(typeof(string)); + IsParseableType = ModelMetadata.FindTryParseMethod(ModelType) is not null; IsNullableValueType = Nullable.GetUnderlyingType(ModelType) != null; IsReferenceOrNullableType = !ModelType.IsValueType || IsNullableValueType; UnderlyingOrModelType = Nullable.GetUnderlyingType(ModelType) ?? ModelType; diff --git a/src/Mvc/Mvc.Core/src/ModelBinding/Metadata/BindingMetadata.cs b/src/Mvc/Mvc.Core/src/ModelBinding/Metadata/BindingMetadata.cs index 9e44b2ce55de..e3b1fab3f53a 100644 --- a/src/Mvc/Mvc.Core/src/ModelBinding/Metadata/BindingMetadata.cs +++ b/src/Mvc/Mvc.Core/src/ModelBinding/Metadata/BindingMetadata.cs @@ -62,16 +62,6 @@ public Type? BinderType /// public bool IsBindingAllowed { get; set; } = true; - /// - /// Gets or sets a value indicating whether or not the property can be model bound - /// from a TryParse method - /// - /// - /// - /// Always false when is set to true - /// - internal bool IsBindingFromTryParseAllowed { get; set; } - /// /// Gets or sets a value indicating whether or not the request must contain a value for the model. /// Will be ignored if the model metadata being created does not represent a property. diff --git a/src/Mvc/Mvc.Core/src/ModelBinding/Metadata/DefaultBindingMetadataProvider.cs b/src/Mvc/Mvc.Core/src/ModelBinding/Metadata/DefaultBindingMetadataProvider.cs index 81b5ecd2b9c6..cd7fca0dca87 100644 --- a/src/Mvc/Mvc.Core/src/ModelBinding/Metadata/DefaultBindingMetadataProvider.cs +++ b/src/Mvc/Mvc.Core/src/ModelBinding/Metadata/DefaultBindingMetadataProvider.cs @@ -85,8 +85,6 @@ public void CreateBindingMetadata(BindingMetadataProviderContext context) { context.BindingMetadata.BoundConstructor = constructorInfo; } - - context.BindingMetadata.IsBindingFromTryParseAllowed = !_options.SuppressParseableTypesBinding; } internal static ConstructorInfo? GetBoundConstructor(Type type) diff --git a/src/Mvc/Mvc.Core/src/ModelBinding/Metadata/DefaultModelMetadata.cs b/src/Mvc/Mvc.Core/src/ModelBinding/Metadata/DefaultModelMetadata.cs index d887dcbc86df..429c18fef155 100644 --- a/src/Mvc/Mvc.Core/src/ModelBinding/Metadata/DefaultModelMetadata.cs +++ b/src/Mvc/Mvc.Core/src/ModelBinding/Metadata/DefaultModelMetadata.cs @@ -347,9 +347,6 @@ public override bool IsRequired } } - internal override bool IsParseableType - => BindingMetadata.IsBindingFromTryParseAllowed && ModelMetadata.FindTryParseMethod(ModelType) is not null; - /// public override ModelBindingMessageProvider ModelBindingMessageProvider => BindingMetadata.ModelBindingMessageProvider!; diff --git a/src/Mvc/Mvc.Core/src/MvcOptions.cs b/src/Mvc/Mvc.Core/src/MvcOptions.cs index 2d6014a5cc4b..7a59a9834738 100644 --- a/src/Mvc/Mvc.Core/src/MvcOptions.cs +++ b/src/Mvc/Mvc.Core/src/MvcOptions.cs @@ -136,13 +136,6 @@ public MvcOptions() /// public bool SuppressOutputFormatterBuffering { get; set; } - /// - /// Gets or sets the flag that determines if binding is disabled for Parseable types (contains a TryParse method) - /// and will treat them as , in case, - /// there isn't a from registered. - /// - public bool SuppressParseableTypesBinding { get; set; } - /// /// Gets or sets the flag that determines if MVC should use action invoker extensibility. This will allow /// custom and execute during the request pipeline. diff --git a/src/Mvc/Mvc.Core/src/PublicAPI.Unshipped.txt b/src/Mvc/Mvc.Core/src/PublicAPI.Unshipped.txt index b6bb947bb5f4..e72c9d39ff92 100644 --- a/src/Mvc/Mvc.Core/src/PublicAPI.Unshipped.txt +++ b/src/Mvc/Mvc.Core/src/PublicAPI.Unshipped.txt @@ -13,8 +13,6 @@ Microsoft.AspNetCore.Mvc.ModelBinding.Metadata.SystemTextJsonValidationMetadataP Microsoft.AspNetCore.Mvc.ModelBinding.Metadata.SystemTextJsonValidationMetadataProvider.SystemTextJsonValidationMetadataProvider(System.Text.Json.JsonNamingPolicy! namingPolicy) -> void Microsoft.AspNetCore.Mvc.ModelBinding.Metadata.ValidationMetadata.ValidationModelName.get -> string? Microsoft.AspNetCore.Mvc.ModelBinding.Metadata.ValidationMetadata.ValidationModelName.set -> void -Microsoft.AspNetCore.Mvc.MvcOptions.SuppressParseableTypesBinding.get -> bool -Microsoft.AspNetCore.Mvc.MvcOptions.SuppressParseableTypesBinding.set -> void virtual Microsoft.AspNetCore.Mvc.Infrastructure.ConfigureCompatibilityOptions.PostConfigure(string? name, TOptions! options) -> void *REMOVED*~Microsoft.AspNetCore.Mvc.Formatters.FormatFilter.FormatFilter(Microsoft.Extensions.Options.IOptions! options, Microsoft.Extensions.Logging.ILoggerFactory! loggerFactory) -> void Microsoft.AspNetCore.Mvc.Formatters.FormatFilter.FormatFilter(Microsoft.Extensions.Options.IOptions! options, Microsoft.Extensions.Logging.ILoggerFactory! loggerFactory) -> void diff --git a/src/Mvc/Mvc.Core/test/ModelBinding/Metadata/DefaultBindingMetadataProviderTest.cs b/src/Mvc/Mvc.Core/test/ModelBinding/Metadata/DefaultBindingMetadataProviderTest.cs index 52ca7bcd4b15..627657da4d33 100644 --- a/src/Mvc/Mvc.Core/test/ModelBinding/Metadata/DefaultBindingMetadataProviderTest.cs +++ b/src/Mvc/Mvc.Core/test/ModelBinding/Metadata/DefaultBindingMetadataProviderTest.cs @@ -656,40 +656,6 @@ public void CreateBindingDetails_BindingBehaviorLeftAlone_ForAttributeOnProperty Assert.Equal(initialValue, context.BindingMetadata.IsBindingRequired); } - [Fact] - public void CreateBindingDetails_IsBindingFromTryParseNotAllowed_WhenSuppressParseableTypesBindingIsTrue() - { - // Arrange - var context = new BindingMetadataProviderContext( - ModelMetadataIdentity.ForType(typeof(int)), - new ModelAttributes(Array.Empty(), null, null)); - - var provider = CreateBindingMetadataProvider(new MvcOptions() { SuppressParseableTypesBinding = true }); - - // Act - provider.CreateBindingMetadata(context); - - // Assert - Assert.False(context.BindingMetadata.IsBindingFromTryParseAllowed); - } - - [Fact] - public void CreateBindingDetails_IsBindingFromTryParseAllowed_WhenSuppressParseableTypesBindingIsFalse() - { - // Arrange - var context = new BindingMetadataProviderContext( - ModelMetadataIdentity.ForType(typeof(int)), - new ModelAttributes(Array.Empty(), null, null)); - - var provider = CreateBindingMetadataProvider(new MvcOptions() { SuppressParseableTypesBinding = false }); - - // Act - provider.CreateBindingMetadata(context); - - // Assert - Assert.True(context.BindingMetadata.IsBindingFromTryParseAllowed); - } - private class DefaultConstructorType { } [Fact] diff --git a/src/Mvc/Mvc.Core/test/ModelBinding/Metadata/DefaultModelMetadataTest.cs b/src/Mvc/Mvc.Core/test/ModelBinding/Metadata/DefaultModelMetadataTest.cs index bff8ae2a4ebe..9eb9682e6b96 100644 --- a/src/Mvc/Mvc.Core/test/ModelBinding/Metadata/DefaultModelMetadataTest.cs +++ b/src/Mvc/Mvc.Core/test/ModelBinding/Metadata/DefaultModelMetadataTest.cs @@ -355,13 +355,7 @@ public void IsParseableType_ReturnsTrue_ForParseableTypes(Type modelType) var detailsProvider = new EmptyCompositeMetadataDetailsProvider(); var key = ModelMetadataIdentity.ForType(modelType); - var cache = new DefaultMetadataDetails(key, new ModelAttributes(Array.Empty(), null, null)) - { - BindingMetadata = new BindingMetadata() - { - IsBindingFromTryParseAllowed = true, - }, - }; + var cache = new DefaultMetadataDetails(key, new ModelAttributes(Array.Empty(), null, null)); var metadata = new DefaultModelMetadata(provider, detailsProvider, cache); @@ -384,38 +378,7 @@ public void IsParseableType_ReturnsFalse_ForNonParseableTypes(Type modelType) var detailsProvider = new EmptyCompositeMetadataDetailsProvider(); var key = ModelMetadataIdentity.ForType(modelType); - var cache = new DefaultMetadataDetails(key, new ModelAttributes(Array.Empty(), null, null)) - { - BindingMetadata = new BindingMetadata() - { - IsBindingFromTryParseAllowed = true, - }, - }; - - var metadata = new DefaultModelMetadata(provider, detailsProvider, cache); - - // Act - var isParseableType = metadata.IsParseableType; - - // Assert - Assert.False(isParseableType); - } - - [Fact] - public void IsParseableType_ReturnsFalse_WhenTryParseNotAllowed() - { - // Arrange - var provider = new EmptyModelMetadataProvider(); - var detailsProvider = new EmptyCompositeMetadataDetailsProvider(); - - var key = ModelMetadataIdentity.ForType(typeof(int)); - var cache = new DefaultMetadataDetails(key, new ModelAttributes(Array.Empty(), null, null)) - { - BindingMetadata = new BindingMetadata() - { - IsBindingFromTryParseAllowed = false, - }, - }; + var cache = new DefaultMetadataDetails(key, new ModelAttributes(Array.Empty(), null, null)); var metadata = new DefaultModelMetadata(provider, detailsProvider, cache); From 49fdd216f88914c96b2a3a04d80a85619a229cff Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Mon, 28 Feb 2022 14:34:56 -0800 Subject: [PATCH 29/31] PR feeback --- .../Mvc.Core/src/ModelBinding/Binders/TryParseModelBinder.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinder.cs b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinder.cs index a273b2d9d178..af90be7bfd0f 100644 --- a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinder.cs +++ b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinder.cs @@ -114,7 +114,7 @@ private static void AddModelError(ModelBindingContext bindingContext, Exception bindingContext.ModelMetadata); } - private Func CreateTryParseOperation(Type modelType) + private static Func CreateTryParseOperation(Type modelType) { modelType = Nullable.GetUnderlyingType(modelType) ?? modelType; var tryParseMethodExpession = ModelMetadata.FindTryParseMethod(modelType) From 92dd9c596bb49bde92a026d25aafec2b06d751ef Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Thu, 3 Mar 2022 09:53:26 -0800 Subject: [PATCH 30/31] Update src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinder.cs Co-authored-by: Pranav K --- .../Mvc.Core/src/ModelBinding/Binders/TryParseModelBinder.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinder.cs b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinder.cs index af90be7bfd0f..8cf435339efe 100644 --- a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinder.cs +++ b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinder.cs @@ -73,7 +73,7 @@ public Task BindModelAsync(ModelBindingContext bindingContext) var value = valueProviderResult.FirstValue; if (string.IsNullOrWhiteSpace(value)) { - // Is expected to the TryParse() method trims the value then fail if the result is empty. + // Most TryParse() methods trim the value and fail if the result is empty. // When converting newModel a null value 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 From 7a07a9a3f526d008bd49832d135a40c589cb26a0 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Thu, 3 Mar 2022 10:20:04 -0800 Subject: [PATCH 31/31] PR Feeback --- .../test/ModelBinding/ModelStateDictionaryTest.cs | 4 ++-- .../src/Infrastructure/MvcCoreMvcOptionsSetup.cs | 6 +++--- .../src/ModelBinding/Binders/TryParseModelBinder.cs | 2 +- .../Metadata/DefaultBindingMetadataProvider.cs | 7 ------- .../Metadata/DefaultBindingMetadataProviderTest.cs | 4 ++-- .../test/ModelMetadataProviderTest.cs | 2 +- .../Mvc.Core.TestCommon/TestModelMetadataProvider.cs | 10 +++++----- 7 files changed, 14 insertions(+), 21 deletions(-) diff --git a/src/Mvc/Mvc.Abstractions/test/ModelBinding/ModelStateDictionaryTest.cs b/src/Mvc/Mvc.Abstractions/test/ModelBinding/ModelStateDictionaryTest.cs index 98f0fc03ae4a..6b8e91955734 100644 --- a/src/Mvc/Mvc.Abstractions/test/ModelBinding/ModelStateDictionaryTest.cs +++ b/src/Mvc/Mvc.Abstractions/test/ModelBinding/ModelStateDictionaryTest.cs @@ -1599,8 +1599,8 @@ public void GetModelStateForProperty_ReturnsModelStateForIndexedChildren() Assert.Equal("value1", property.RawValue); } - private DefaultBindingMetadataProvider CreateBindingMetadataProvider(MvcOptions options = null) - => new DefaultBindingMetadataProvider(options ?? new MvcOptions()); + private DefaultBindingMetadataProvider CreateBindingMetadataProvider() + => new DefaultBindingMetadataProvider(); private class OptionsAccessor : IOptions { diff --git a/src/Mvc/Mvc.Core/src/Infrastructure/MvcCoreMvcOptionsSetup.cs b/src/Mvc/Mvc.Core/src/Infrastructure/MvcCoreMvcOptionsSetup.cs index b011b3fda119..82affa44c7b9 100644 --- a/src/Mvc/Mvc.Core/src/Infrastructure/MvcCoreMvcOptionsSetup.cs +++ b/src/Mvc/Mvc.Core/src/Infrastructure/MvcCoreMvcOptionsSetup.cs @@ -100,7 +100,7 @@ public void Configure(MvcOptions options) options.ValueProviderFactories.Add(new FormFileValueProviderFactory()); // Set up metadata providers - ConfigureAdditionalModelMetadataDetailsProviders(options.ModelMetadataDetailsProviders, options); + ConfigureAdditionalModelMetadataDetailsProviders(options.ModelMetadataDetailsProviders); // Set up validators options.ModelValidatorProviders.Add(new DefaultModelValidatorProvider()); @@ -115,13 +115,13 @@ public void PostConfigure(string? name, MvcOptions options) options.ModelMetadataDetailsProviders.Add(new HasValidatorsValidationMetadataProvider(options.ModelValidatorProviders)); } - internal static void ConfigureAdditionalModelMetadataDetailsProviders(IList modelMetadataDetailsProviders, MvcOptions options) + internal static void ConfigureAdditionalModelMetadataDetailsProviders(IList modelMetadataDetailsProviders) { // Don't bind the Type class by default as it's expensive. A user can override this behavior // by altering the collection of providers. modelMetadataDetailsProviders.Add(new ExcludeBindingMetadataProvider(typeof(Type))); - modelMetadataDetailsProviders.Add(new DefaultBindingMetadataProvider(options)); + modelMetadataDetailsProviders.Add(new DefaultBindingMetadataProvider()); modelMetadataDetailsProviders.Add(new DefaultValidationMetadataProvider()); modelMetadataDetailsProviders.Add(new BindingSourceMetadataProvider(typeof(CancellationToken), BindingSource.Special)); diff --git a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinder.cs b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinder.cs index af90be7bfd0f..7ee25202367c 100644 --- a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinder.cs +++ b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/TryParseModelBinder.cs @@ -75,7 +75,7 @@ public Task BindModelAsync(ModelBindingContext bindingContext) { // Is expected to the TryParse() method trims the value then fail if the result is empty. - // When converting newModel a null value may indicate a failed conversion for an otherwise required + // When converting a null value 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 (!bindingContext.ModelMetadata.IsReferenceOrNullableType) diff --git a/src/Mvc/Mvc.Core/src/ModelBinding/Metadata/DefaultBindingMetadataProvider.cs b/src/Mvc/Mvc.Core/src/ModelBinding/Metadata/DefaultBindingMetadataProvider.cs index cd7fca0dca87..9caa19b7d570 100644 --- a/src/Mvc/Mvc.Core/src/ModelBinding/Metadata/DefaultBindingMetadataProvider.cs +++ b/src/Mvc/Mvc.Core/src/ModelBinding/Metadata/DefaultBindingMetadataProvider.cs @@ -14,13 +14,6 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Metadata; /// internal class DefaultBindingMetadataProvider : IBindingMetadataProvider { - private readonly MvcOptions _options; - - public DefaultBindingMetadataProvider(MvcOptions options) - { - _options = options; - } - public void CreateBindingMetadata(BindingMetadataProviderContext context) { if (context == null) diff --git a/src/Mvc/Mvc.Core/test/ModelBinding/Metadata/DefaultBindingMetadataProviderTest.cs b/src/Mvc/Mvc.Core/test/ModelBinding/Metadata/DefaultBindingMetadataProviderTest.cs index 627657da4d33..ab2feac0f41c 100644 --- a/src/Mvc/Mvc.Core/test/ModelBinding/Metadata/DefaultBindingMetadataProviderTest.cs +++ b/src/Mvc/Mvc.Core/test/ModelBinding/Metadata/DefaultBindingMetadataProviderTest.cs @@ -914,6 +914,6 @@ private class CustomAttribute : Attribute public string Identifier { get; set; } } - private DefaultBindingMetadataProvider CreateBindingMetadataProvider(MvcOptions options = null) - => new DefaultBindingMetadataProvider(options ?? new MvcOptions()); + private DefaultBindingMetadataProvider CreateBindingMetadataProvider() + => new DefaultBindingMetadataProvider(); } diff --git a/src/Mvc/Mvc.DataAnnotations/test/ModelMetadataProviderTest.cs b/src/Mvc/Mvc.DataAnnotations/test/ModelMetadataProviderTest.cs index e494446a5d1b..6fd128a4a232 100644 --- a/src/Mvc/Mvc.DataAnnotations/test/ModelMetadataProviderTest.cs +++ b/src/Mvc/Mvc.DataAnnotations/test/ModelMetadataProviderTest.cs @@ -1045,7 +1045,7 @@ public AttributeInjectModelMetadataProvider(object[] attributes) : base( new DefaultCompositeMetadataDetailsProvider(new IMetadataDetailsProvider[] { - new DefaultBindingMetadataProvider(new MvcOptions()), + new DefaultBindingMetadataProvider(), new DataAnnotationsMetadataProvider( new MvcOptions(), Options.Create(new MvcDataAnnotationsLocalizationOptions()), diff --git a/src/Mvc/shared/Mvc.Core.TestCommon/TestModelMetadataProvider.cs b/src/Mvc/shared/Mvc.Core.TestCommon/TestModelMetadataProvider.cs index 572dfd8c254d..4f3ba4ef774b 100644 --- a/src/Mvc/shared/Mvc.Core.TestCommon/TestModelMetadataProvider.cs +++ b/src/Mvc/shared/Mvc.Core.TestCommon/TestModelMetadataProvider.cs @@ -26,13 +26,13 @@ public static ModelMetadataProvider CreateDefaultProvider(IStringLocalizerFactor { var detailsProviders = new List { - new DefaultBindingMetadataProvider(new MvcOptions()), + new DefaultBindingMetadataProvider(), new DefaultValidationMetadataProvider(), CreateDefaultDataAnnotationsProvider(stringLocalizerFactory), new DataMemberRequiredBindingMetadataProvider(), }; - MvcCoreMvcOptionsSetup.ConfigureAdditionalModelMetadataDetailsProviders(detailsProviders, new MvcOptions()); + MvcCoreMvcOptionsSetup.ConfigureAdditionalModelMetadataDetailsProviders(detailsProviders); var validationProviders = TestModelValidatorProvider.CreateDefaultProvider(); detailsProviders.Add(new HasValidatorsValidationMetadataProvider(validationProviders.ValidatorProviders)); @@ -45,7 +45,7 @@ public static IModelMetadataProvider CreateDefaultProvider(IList() { - new DefaultBindingMetadataProvider(new MvcOptions()), + new DefaultBindingMetadataProvider(), new DefaultValidationMetadataProvider(), new DataAnnotationsMetadataProvider( new MvcOptions(), @@ -54,7 +54,7 @@ public static IModelMetadataProvider CreateDefaultProvider(IList