From 63a0251075468508022d900eb8e1e277c84bf4f5 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Wed, 15 Feb 2023 12:23:14 -0800 Subject: [PATCH] Prefer TypeConverter over TryParse (#46577) * Prefer TypeConverter over TryParse * Rename test * PR review --- .../Infrastructure/MvcCoreMvcOptionsSetup.cs | 2 +- .../Binders/SimpleTypeModelBinderProvider.cs | 2 +- src/Mvc/Mvc/test/MvcOptionsSetupTest.cs | 2 +- .../SimpleTypeModelBinderIntegrationTest.cs | 134 ++++++++++++++++++ 4 files changed, 137 insertions(+), 3 deletions(-) diff --git a/src/Mvc/Mvc.Core/src/Infrastructure/MvcCoreMvcOptionsSetup.cs b/src/Mvc/Mvc.Core/src/Infrastructure/MvcCoreMvcOptionsSetup.cs index 7f72ccd8ed62..908a33ef721c 100644 --- a/src/Mvc/Mvc.Core/src/Infrastructure/MvcCoreMvcOptionsSetup.cs +++ b/src/Mvc/Mvc.Core/src/Infrastructure/MvcCoreMvcOptionsSetup.cs @@ -52,8 +52,8 @@ 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 TryParseModelBinderProvider()); options.ModelBinderProviders.Add(new CancellationTokenModelBinderProvider()); options.ModelBinderProviders.Add(new ByteArrayModelBinderProvider()); options.ModelBinderProviders.Add(new FormFileModelBinderProvider()); diff --git a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/SimpleTypeModelBinderProvider.cs b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/SimpleTypeModelBinderProvider.cs index 695cb25c9e14..394ba8907afb 100644 --- a/src/Mvc/Mvc.Core/src/ModelBinding/Binders/SimpleTypeModelBinderProvider.cs +++ b/src/Mvc/Mvc.Core/src/ModelBinding/Binders/SimpleTypeModelBinderProvider.cs @@ -18,7 +18,7 @@ public class SimpleTypeModelBinderProvider : IModelBinderProvider { ArgumentNullException.ThrowIfNull(context); - if (!context.Metadata.IsComplexType) + if (context.Metadata.IsConvertibleType) { var loggerFactory = context.Services.GetRequiredService(); return new SimpleTypeModelBinder(context.Metadata.ModelType, loggerFactory); diff --git a/src/Mvc/Mvc/test/MvcOptionsSetupTest.cs b/src/Mvc/Mvc/test/MvcOptionsSetupTest.cs index 79c83b8ec421..a45f1fe4ce67 100644 --- a/src/Mvc/Mvc/test/MvcOptionsSetupTest.cs +++ b/src/Mvc/Mvc/test/MvcOptionsSetupTest.cs @@ -52,8 +52,8 @@ 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), binder => Assert.IsType(binder), binder => Assert.IsType(binder), diff --git a/src/Mvc/test/Mvc.IntegrationTests/SimpleTypeModelBinderIntegrationTest.cs b/src/Mvc/test/Mvc.IntegrationTests/SimpleTypeModelBinderIntegrationTest.cs index 731d5130b39f..5796cd1898c8 100644 --- a/src/Mvc/test/Mvc.IntegrationTests/SimpleTypeModelBinderIntegrationTest.cs +++ b/src/Mvc/test/Mvc.IntegrationTests/SimpleTypeModelBinderIntegrationTest.cs @@ -1,6 +1,9 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System.ComponentModel; +using System.Diagnostics.CodeAnalysis; +using System.Globalization; using System.Text; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc.Abstractions; @@ -699,6 +702,94 @@ public async Task BindParameter_FromFormData_BindsCorrectly(Dictionary + { + request.QueryString = QueryString.Create("Parameter1", "someValue"); + }); + + var modelState = testContext.ModelState; + + // Act + var modelBindingResult = await parameterBinder.BindModelAsync(parameter, testContext); + + // Assert + + // ModelBindingResult + Assert.True(modelBindingResult.IsModelSet); + + // Model + var model = Assert.IsType(modelBindingResult.Model); + Assert.Equal("someValue", model.Value); + Assert.Equal("Converter", model.Source); + + // ModelState + Assert.True(modelState.IsValid); + + Assert.Single(modelState.Keys); + var key = Assert.Single(modelState.Keys); + Assert.Equal("Parameter1", key); + Assert.Equal("someValue", modelState[key].AttemptedValue); + Assert.Equal("someValue", modelState[key].RawValue); + Assert.Empty(modelState[key].Errors); + Assert.Equal(ModelValidationState.Valid, modelState[key].ValidationState); + } + + [Fact] + public async Task BindParameter_BindsUsingTryParse() + { + // Arrange + var parameterBinder = ModelBindingTestHelper.GetParameterBinder(); + var parameter = new ParameterDescriptor() + { + Name = "Parameter1", + BindingInfo = new BindingInfo(), + ParameterType = typeof(SampleTryParsableModel) + }; + + var testContext = ModelBindingTestHelper.GetTestContext(request => + { + request.QueryString = QueryString.Create("Parameter1", "someValue"); + }); + + var modelState = testContext.ModelState; + + // Act + var modelBindingResult = await parameterBinder.BindModelAsync(parameter, testContext); + + // Assert + + // ModelBindingResult + Assert.True(modelBindingResult.IsModelSet); + + // Model + var model = Assert.IsType(modelBindingResult.Model); + Assert.Equal("someValue", model.Value); + Assert.Equal("TryParse", model.Source); + + // ModelState + Assert.True(modelState.IsValid); + + Assert.Single(modelState.Keys); + var key = Assert.Single(modelState.Keys); + Assert.Equal("Parameter1", key); + Assert.Equal("someValue", modelState[key].AttemptedValue); + Assert.Equal("someValue", modelState[key].RawValue); + Assert.Empty(modelState[key].Errors); + Assert.Equal(ModelValidationState.Valid, modelState[key].ValidationState); + } + private class Person { public Address Address { get; set; } @@ -712,4 +803,47 @@ private class Address public int Zip { get; set; } } + + [TypeConverter(typeof(SampleModelTypeConverter))] + private class SampleModel + { + public string Value { get; set; } + public string Source { get; set; } + + public static bool TryParse([NotNullWhen(true)] string s, [MaybeNullWhen(false)] out SampleModel result) + { + result = new SampleModel() { Value = s, Source = "TryParse" }; + return true; + } + } + + private class SampleTryParsableModel + { + public string Value { get; set; } + public string Source { get; set; } + + public static bool TryParse([NotNullWhen(true)] string s, [MaybeNullWhen(false)] out SampleTryParsableModel result) + { + result = new SampleTryParsableModel() { Value = s, Source = "TryParse" }; + return true; + } + } + + private class SampleModelTypeConverter : TypeConverter + { + public override bool CanConvertFrom(ITypeDescriptorContext context, Type sourceType) + { + return sourceType == typeof(string) || base.CanConvertFrom(context, sourceType); + } + + public override object ConvertFrom(ITypeDescriptorContext context, CultureInfo culture, object value) + { + if (value is string s) + { + return new SampleModel() { Value = s, Source = "Converter" }; + } + + return base.ConvertFrom(context, culture, value); + } + } }