From fae0e9a66ead378dbbda85fab2925fc299ce2713 Mon Sep 17 00:00:00 2001 From: Doug Bunting Date: Fri, 2 Sep 2016 11:15:57 -0700 Subject: [PATCH] Handle `!ConvertEmptyStringToNull` cases correctly in `SimpleTypeModelBinder` - #4988 - preserve whitespace as the setting demands - correct previous `string.IsNullOrEmpty()` call to match previous `ValueProviderResultExtensions.ConvertTo()` use - short-circuit other `string`-to-`string` conversions (as `ValueProviderResultExtensions.ConvertTo()` does) - correct documentation of `ConvertEmptyStringToNull` properties - add more tests of these scenarios and remove duplicate `BindModel_ValidValueProviderResult_ConvertEmptyStringsToNull()` test --- .../ModelBinding/ModelMetadata.cs | 4 +- .../Binders/SimpleTypeModelBinder.cs | 33 +++++---- .../ModelBinding/Metadata/DisplayMetadata.cs | 5 +- .../Binders/SimpleTypeModelBinderTest.cs | 74 +++++++++++++------ 4 files changed, 78 insertions(+), 38 deletions(-) diff --git a/src/Microsoft.AspNetCore.Mvc.Abstractions/ModelBinding/ModelMetadata.cs b/src/Microsoft.AspNetCore.Mvc.Abstractions/ModelBinding/ModelMetadata.cs index be2d6cebc8..7de72be66b 100644 --- a/src/Microsoft.AspNetCore.Mvc.Abstractions/ModelBinding/ModelMetadata.cs +++ b/src/Microsoft.AspNetCore.Mvc.Abstractions/ModelBinding/ModelMetadata.cs @@ -92,8 +92,8 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding public abstract BindingSource BindingSource { get; } /// - /// Gets a value indicating whether or not to convert an empty string value to null when - /// representing a model as text. + /// Gets a value indicating whether or not to convert an empty string value or one containing only whitespace + /// characters to null when representing a model as text. /// public abstract bool ConvertEmptyStringToNull { get; } diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/SimpleTypeModelBinder.cs b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/SimpleTypeModelBinder.cs index ad70420966..92ab83ad2f 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/SimpleTypeModelBinder.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/SimpleTypeModelBinder.cs @@ -47,8 +47,25 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders { var value = valueProviderResult.FirstValue; - object model = null; - if (!string.IsNullOrWhiteSpace(value)) + object model; + if (bindingContext.ModelType == typeof(string)) + { + // Already have a string. No further conversion required but handle ConvertEmptyStringToNull. + if (bindingContext.ModelMetadata.ConvertEmptyStringToNull && string.IsNullOrWhiteSpace(value)) + { + model = null; + } + else + { + model = value; + } + } + else if (string.IsNullOrWhiteSpace(value)) + { + // Other than the StringConverter, converters Trim() the value then throw if the result is empty. + model = null; + } + else { model = _typeConverter.ConvertFrom( context: null, @@ -56,16 +73,6 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders value: value); } - if (bindingContext.ModelType == typeof(string)) - { - var modelAsString = model as string; - if (bindingContext.ModelMetadata.ConvertEmptyStringToNull && - string.IsNullOrEmpty(modelAsString)) - { - model = null; - } - } - // 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. @@ -75,7 +82,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders bindingContext.ModelName, bindingContext.ModelMetadata.ModelBindingMessageProvider.ValueMustNotBeNullAccessor( valueProviderResult.ToString())); - + return TaskCache.CompletedTask; } else diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Metadata/DisplayMetadata.cs b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Metadata/DisplayMetadata.cs index 4d02a152e8..f1fd3fef08 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Metadata/DisplayMetadata.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Metadata/DisplayMetadata.cs @@ -17,8 +17,9 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Metadata public IDictionary AdditionalValues { get; } = new Dictionary(); /// - /// Gets or sets a value indicating whether or not empty strings should be treated as null. - /// See + /// Gets or sets a value indicating whether or not to convert an empty string value or one containing only + /// whitespace characters to null when representing a model as text. See + /// /// public bool ConvertEmptyStringToNull { get; set; } = true; 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 9941b572ec..7ab5c007ef 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/SimpleTypeModelBinderTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/SimpleTypeModelBinderTest.cs @@ -11,6 +11,58 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders { public class SimpleTypeModelBinderTest { + [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)); + + // 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)); + + // Act + await binder.BindModelAsync(bindingContext); + + // Assert + Assert.Same(value, bindingContext.Result.Model); + Assert.True(bindingContext.ModelState.ContainsKey("theModelName")); + } + public static TheoryData ConvertableTypeData { get @@ -125,26 +177,6 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders Assert.Empty(bindingContext.ModelState); } - [Fact] - public async Task BindModel_ValidValueProviderResult_ConvertEmptyStringsToNull() - { - // Arrange - var bindingContext = GetBindingContext(typeof(string)); - bindingContext.ValueProvider = new SimpleValueProvider - { - { "theModelName", string.Empty } - }; - - var binder = new SimpleTypeModelBinder(typeof(string)); - - // Act - await binder.BindModelAsync(bindingContext); - - // Assert - Assert.Null(bindingContext.Result.Model); - Assert.True(bindingContext.ModelState.ContainsKey("theModelName")); - } - [Theory] [InlineData("")] [InlineData(" \t \r\n ")] @@ -215,7 +247,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders // Arrange var bindingContext = GetBindingContext(typeof(int)); bindingContext.ValueProvider = new SimpleValueProvider - { + { { "theModelName", "42" } };