Prefer TypeConverter over TryParse (dotnet#46577)

* Prefer TypeConverter over TryParse * Rename test * PR review
v-chayan · Feb 15, 2023 · 63a0251 · 63a0251
1 parent 37a3eb9
commit 63a0251
Show file tree

Hide file tree

Showing 4 changed files with 137 additions and 3 deletions.
diff --git 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
@@ -18,7 +18,7 @@ public class SimpleTypeModelBinderProvider : IModelBinderProvider
     {
         ArgumentNullException.ThrowIfNull(context);
 
-        if (!context.Metadata.IsComplexType)
+        if (context.Metadata.IsConvertibleType)
         {
             var loggerFactory = context.Services.GetRequiredService<ILoggerFactory>();
             return new SimpleTypeModelBinder(context.Metadata.ModelType, loggerFactory);

diff --git 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<FloatingPointTypeModelBinderProvider>(binder),
             binder => Assert.IsType<EnumTypeModelBinderProvider>(binder),
             binder => Assert.IsType<DateTimeModelBinderProvider>(binder),
-            binder => Assert.IsType<TryParseModelBinderProvider>(binder),
             binder => Assert.IsType<SimpleTypeModelBinderProvider>(binder),
+            binder => Assert.IsType<TryParseModelBinderProvider>(binder),
             binder => Assert.IsType<CancellationTokenModelBinderProvider>(binder),
             binder => Assert.IsType<ByteArrayModelBinderProvider>(binder),
             binder => Assert.IsType<FormFileModelBinderProvider>(binder),

diff --git 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<string, S
         Assert.Equal(new[] { "line 1", "line 2" }, entry.RawValue);
     }
 
+    [Fact]
+    public async Task BindParameter_PrefersTypeConverter_OverTryParse()
+    {
+        // Arrange
+        var parameterBinder = ModelBindingTestHelper.GetParameterBinder();
+        var parameter = new ParameterDescriptor()
+        {
+            Name = "Parameter1",
+            BindingInfo = new BindingInfo(),
+            ParameterType = typeof(SampleModel)
+        };
+
+        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<SampleModel>(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<SampleTryParsableModel>(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);
+        }
+    }
 }