From 4e97c72eeba30ae0238f05d3f1f0738f0732ba46 Mon Sep 17 00:00:00 2001 From: mnltejaswini Date: Tue, 12 Apr 2016 15:06:12 -0700 Subject: [PATCH] [Perf]:Cache TypeConverters in SimpleTypeModelBinder Fixes #4361 --- .../Binders/SimpleTypeModelBinder.cs | 33 +++- .../Binders/SimpleTypeModelBinderProvider.cs | 2 +- .../Binders/ArrayModelBinderTest.cs | 8 +- .../Binders/DictionaryModelBinderTest.cs | 42 +++-- .../Binders/KeyValuePairModelBinderTest.cs | 8 +- .../Binders/SimpleTypeModelBinderTest.cs | 174 +++++++++++++++++- 6 files changed, 239 insertions(+), 28 deletions(-) diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/SimpleTypeModelBinder.cs b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/SimpleTypeModelBinder.cs index 1d6836fdce..46a3394a14 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/SimpleTypeModelBinder.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/SimpleTypeModelBinder.cs @@ -2,6 +2,8 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; +using System.ComponentModel; +using System.Runtime.ExceptionServices; using System.Threading.Tasks; using Microsoft.AspNetCore.Mvc.Internal; @@ -12,6 +14,18 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders /// public class SimpleTypeModelBinder : IModelBinder { + private readonly TypeConverter _typeConverter; + + public SimpleTypeModelBinder(Type type) + { + if (type == null) + { + throw new ArgumentNullException(nameof(type)); + } + + _typeConverter = TypeDescriptor.GetConverter(type); + } + /// public Task BindModelAsync(ModelBindingContext bindingContext) { @@ -32,7 +46,16 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders try { - var model = valueProviderResult.ConvertTo(bindingContext.ModelType); + var value = valueProviderResult.FirstValue; + + object model = null; + if (!string.IsNullOrWhiteSpace(value)) + { + model = _typeConverter.ConvertFrom( + context: null, + culture: valueProviderResult.Culture, + value: value); + } if (bindingContext.ModelType == typeof(string)) { @@ -65,6 +88,14 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders } catch (Exception exception) { + var isFormatException = exception is FormatException; + if (!isFormatException && exception.InnerException != null) + { + // TypeConverter throws System.Exception wrapping the FormatException, + // so we capture the inner exception. + exception = ExceptionDispatchInfo.Capture(exception.InnerException).SourceException; + } + bindingContext.ModelState.TryAddModelError( bindingContext.ModelName, exception, diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/SimpleTypeModelBinderProvider.cs b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/SimpleTypeModelBinderProvider.cs index 16869d81ad..0a1bcfa7f4 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/SimpleTypeModelBinderProvider.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/SimpleTypeModelBinderProvider.cs @@ -20,7 +20,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders if (!context.Metadata.IsComplexType) { - return new SimpleTypeModelBinder(); + return new SimpleTypeModelBinder(context.Metadata.ModelType); } return null; diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/ArrayModelBinderTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/ArrayModelBinderTest.cs index b316480931..ee44b4590f 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/ArrayModelBinderTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/ArrayModelBinderTest.cs @@ -21,7 +21,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders }; var bindingContext = GetBindingContext(valueProvider); var modelState = bindingContext.ModelState; - var binder = new ArrayModelBinder(new SimpleTypeModelBinder()); + var binder = new ArrayModelBinder(new SimpleTypeModelBinder(typeof(int))); // Act var result = await binder.BindModelResultAsync(bindingContext); @@ -37,7 +37,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders public async Task ArrayModelBinder_CreatesEmptyCollection_IfIsTopLevelObject() { // Arrange - var binder = new ArrayModelBinder(new SimpleTypeModelBinder()); + var binder = new ArrayModelBinder(new SimpleTypeModelBinder(typeof(string))); var context = CreateContext(); context.IsTopLevelObject = true; @@ -67,7 +67,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders public async Task ArrayModelBinder_DoesNotCreateCollection_IfNotIsTopLevelObject(string prefix) { // Arrange - var binder = new ArrayModelBinder(new SimpleTypeModelBinder()); + var binder = new ArrayModelBinder(new SimpleTypeModelBinder(typeof(string))); var context = CreateContext(); context.ModelName = ModelNames.CreatePropertyModelName(prefix, "ArrayProperty"); @@ -116,7 +116,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders var modelState = bindingContext.ModelState; bindingContext.Model = model; - var binder = new ArrayModelBinder(new SimpleTypeModelBinder()); + var binder = new ArrayModelBinder(new SimpleTypeModelBinder(typeof(int))); // Act var result = await binder.BindModelResultAsync(bindingContext); diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/DictionaryModelBinderTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/DictionaryModelBinderTest.cs index 161e3e1236..d71c21b845 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/DictionaryModelBinderTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/DictionaryModelBinderTest.cs @@ -35,7 +35,9 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders var bindingContext = GetModelBindingContext(isReadOnly, values); bindingContext.ValueProvider = CreateEnumerableValueProvider("{0}", values); - var binder = new DictionaryModelBinder(new SimpleTypeModelBinder(), new SimpleTypeModelBinder()); + var binder = new DictionaryModelBinder( + new SimpleTypeModelBinder(typeof(int)), + new SimpleTypeModelBinder(typeof(string))); // Act var result = await binder.BindModelResultAsync(bindingContext); @@ -73,7 +75,9 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders var dictionary = new Dictionary(); bindingContext.Model = dictionary; - var binder = new DictionaryModelBinder(new SimpleTypeModelBinder(), new SimpleTypeModelBinder()); + var binder = new DictionaryModelBinder( + new SimpleTypeModelBinder(typeof(int)), + new SimpleTypeModelBinder(typeof(string))); // Act var result = await binder.BindModelResultAsync(bindingContext); @@ -127,7 +131,9 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders IDictionary dictionary) { // Arrange - var binder = new DictionaryModelBinder(new SimpleTypeModelBinder(), new SimpleTypeModelBinder()); + var binder = new DictionaryModelBinder( + new SimpleTypeModelBinder(typeof(string)), + new SimpleTypeModelBinder(typeof(string))); var context = CreateContext(); context.ModelName = modelName; @@ -163,7 +169,9 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders { "three", "three" }, }; - var binder = new DictionaryModelBinder(new SimpleTypeModelBinder(), new SimpleTypeModelBinder()); + var binder = new DictionaryModelBinder( + new SimpleTypeModelBinder(typeof(string)), + new SimpleTypeModelBinder(typeof(string))); var context = CreateContext(); context.ModelName = "prefix"; @@ -214,7 +222,9 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders // Arrange var stringDictionary = dictionary.ToDictionary(kvp => kvp.Key.ToString(), kvp => kvp.Value.ToString()); - var binder = new DictionaryModelBinder(new SimpleTypeModelBinder(), new SimpleTypeModelBinder()); + var binder = new DictionaryModelBinder( + new SimpleTypeModelBinder(typeof(long)), + new SimpleTypeModelBinder(typeof(int))); var context = CreateContext(); context.ModelName = "prefix"; @@ -271,11 +281,11 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders var valueMetadata = metadataProvider.GetMetadataForType(typeof(ModelWithProperties)); var binder = new DictionaryModelBinder( - new SimpleTypeModelBinder(), + new SimpleTypeModelBinder(typeof(int)), new ComplexTypeModelBinder(new Dictionary() { - { valueMetadata.Properties["Id"], new SimpleTypeModelBinder() }, - { valueMetadata.Properties["Name"], new SimpleTypeModelBinder() }, + { valueMetadata.Properties["Id"], new SimpleTypeModelBinder(typeof(int)) }, + { valueMetadata.Properties["Name"], new SimpleTypeModelBinder(typeof(string)) }, })); // Act @@ -311,7 +321,9 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders { // Arrange var expectedDictionary = new SortedDictionary(dictionary); - var binder = new DictionaryModelBinder(new SimpleTypeModelBinder(), new SimpleTypeModelBinder()); + var binder = new DictionaryModelBinder( + new SimpleTypeModelBinder(typeof(string)), + new SimpleTypeModelBinder(typeof(string))); var context = CreateContext(); context.ModelName = modelName; @@ -341,7 +353,9 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders public async Task DictionaryModelBinder_CreatesEmptyCollection_IfIsTopLevelObject() { // Arrange - var binder = new DictionaryModelBinder(new SimpleTypeModelBinder(), new SimpleTypeModelBinder()); + var binder = new DictionaryModelBinder( + new SimpleTypeModelBinder(typeof(string)), + new SimpleTypeModelBinder(typeof(string))); var context = CreateContext(); context.IsTopLevelObject = true; @@ -371,7 +385,9 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders public async Task DictionaryModelBinder_DoesNotCreateCollection_IfNotIsTopLevelObject(string prefix) { // Arrange - var binder = new DictionaryModelBinder(new SimpleTypeModelBinder(), new SimpleTypeModelBinder()); + var binder = new DictionaryModelBinder( + new SimpleTypeModelBinder(typeof(int)), + new SimpleTypeModelBinder(typeof(int))); var context = CreateContext(); context.ModelName = ModelNames.CreatePropertyModelName(prefix, "ListProperty"); @@ -413,7 +429,9 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders public void CanCreateInstance_ReturnsExpectedValue(Type modelType, bool expectedResult) { // Arrange - var binder = new DictionaryModelBinder(new SimpleTypeModelBinder(), new SimpleTypeModelBinder()); + var binder = new DictionaryModelBinder( + new SimpleTypeModelBinder(typeof(int)), + new SimpleTypeModelBinder(typeof(int))); // Act var result = binder.CanCreateInstance(modelType); diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/KeyValuePairModelBinderTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/KeyValuePairModelBinderTest.cs index 6f8c50e9e5..7321eb1465 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/KeyValuePairModelBinderTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/KeyValuePairModelBinderTest.cs @@ -136,7 +136,9 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders public async Task KeyValuePairModelBinder_CreatesEmptyCollection_IfIsTopLevelObject() { // Arrange - var binder = new KeyValuePairModelBinder(new SimpleTypeModelBinder(), new SimpleTypeModelBinder()); + var binder = new KeyValuePairModelBinder( + new SimpleTypeModelBinder(typeof(string)), + new SimpleTypeModelBinder(typeof(string))); var context = CreateContext(); context.IsTopLevelObject = true; @@ -167,7 +169,9 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders public async Task KeyValuePairModelBinder_DoesNotCreateCollection_IfNotIsTopLevelObject(string prefix) { // Arrange - var binder = new KeyValuePairModelBinder(new SimpleTypeModelBinder(), new SimpleTypeModelBinder()); + var binder = new KeyValuePairModelBinder( + new SimpleTypeModelBinder(typeof(string)), + new SimpleTypeModelBinder(typeof(string))); var context = CreateContext(); context.ModelName = ModelNames.CreatePropertyModelName(prefix, "KeyValuePairProperty"); diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/SimpleTypeModelBinderTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/SimpleTypeModelBinderTest.cs index eec2c8cf4a..1d1f2111cc 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/SimpleTypeModelBinderTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/SimpleTypeModelBinderTest.cs @@ -47,7 +47,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders { "theModelName", "some-value" } }; - var binder = new SimpleTypeModelBinder(); + var binder = new SimpleTypeModelBinder(destinationType); // Act var result = await binder.BindModelResultAsync(bindingContext); @@ -73,7 +73,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders { { "theModelName", string.Empty } }; - var binder = new SimpleTypeModelBinder(); + var binder = new SimpleTypeModelBinder(destinationType); // Act var result = await binder.BindModelResultAsync(bindingContext); @@ -98,7 +98,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders { "theModelName", "not an integer" } }; - var binder = new SimpleTypeModelBinder(); + var binder = new SimpleTypeModelBinder(typeof(int)); // Act var result = await binder.BindModelResultAsync(bindingContext); @@ -116,7 +116,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders { // Arrange var bindingContext = GetBindingContext(typeof(int)); - var binder = new SimpleTypeModelBinder(); + var binder = new SimpleTypeModelBinder(typeof(int)); // Act var result = await binder.BindModelResultAsync(bindingContext); @@ -136,7 +136,29 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders { "theModelName", string.Empty } }; - var binder = new SimpleTypeModelBinder(); + var binder = new SimpleTypeModelBinder(typeof(string)); + + // Act + var result = await binder.BindModelResultAsync(bindingContext); + + // Assert + Assert.Null(result.Model); + Assert.True(bindingContext.ModelState.ContainsKey("theModelName")); + } + + [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)); // Act var result = await binder.BindModelResultAsync(bindingContext); @@ -146,17 +168,59 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders 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 SimpleTypeModelBinder(typeof(int?)); + + // Act + var result = await binder.BindModelResultAsync(bindingContext); + + // Assert + Assert.True(result.IsModelSet); + Assert.Equal(12, 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 SimpleTypeModelBinder(typeof(double?)); + + // Act + var result = await binder.BindModelResultAsync(bindingContext); + + // Assert + Assert.True(result.IsModelSet); + Assert.Equal(12.5, result.Model); + Assert.True(bindingContext.ModelState.ContainsKey("theModelName")); + } + [Fact] public async Task BindModel_ValidValueProviderResult_ReturnsModel() { // Arrange var bindingContext = GetBindingContext(typeof(int)); bindingContext.ValueProvider = new SimpleValueProvider - { + { { "theModelName", "42" } }; - var binder = new SimpleTypeModelBinder(); + var binder = new SimpleTypeModelBinder(typeof(int)); // Act var result = await binder.BindModelResultAsync(bindingContext); @@ -167,6 +231,93 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders Assert.True(bindingContext.ModelState.ContainsKey("theModelName")); } + [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 SimpleTypeModelBinder(typeof(decimal)); + + // Act + var result = await binder.BindModelResultAsync(bindingContext); + + // Assert + Assert.True(result.IsModelSet); + Assert.Equal(12.5M, 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 SimpleTypeModelBinder(typeof(decimal)); + + // Act + var result = await binder.BindModelResultAsync(bindingContext); + + // Assert + Assert.False(result.IsModelSet); + Assert.Null(result.Model); + + var error = Assert.Single(bindingContext.ModelState["theModelName"].Errors); + Assert.Equal("The value '12,5' is not valid for Decimal.", 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 SimpleTypeModelBinder(typeof(IntEnum)); + + // Act + var result = await binder.BindModelResultAsync(bindingContext); + + // Assert + Assert.True(result.IsModelSet); + var boundModel = Assert.IsType(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 SimpleTypeModelBinder(typeof(IntEnum)); + + // Act + var result = await binder.BindModelResultAsync(bindingContext); + + // Assert + Assert.True(result.IsModelSet); + var boundModel = Assert.IsType(result.Model); + Assert.Equal(IntEnum.Value1, boundModel); + } + [Theory] [InlineData("0", 0)] [InlineData("1", 1)] @@ -182,7 +333,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders { "theModelName", flagsEnumValue } }; - var binder = new SimpleTypeModelBinder(); + var binder = new SimpleTypeModelBinder(typeof(FlagsEnum)); // Act var result = await binder.BindModelResultAsync(bindingContext); @@ -216,5 +367,12 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders Value4 = 4, Value8 = 8 } + + private enum IntEnum + { + Value0 = 0, + Value1 = 1, + MaxValue = int.MaxValue + } } }