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

Author: dougbu

src/Microsoft.AspNetCore.Mvc.Abstractions/ModelBinding/ModelMetadata.cs

@@ -92,8 +92,8 @@ public string PropertyName
         public abstract BindingSource BindingSource { get; }
 
         /// <summary>
-        /// Gets a value indicating whether or not to convert an empty string value to <c>null</c> 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 <c>null</c> when representing a model as text.
         /// </summary>
         public abstract bool ConvertEmptyStringToNull { get; }
 

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/SimpleTypeModelBinder.cs

@@ -47,23 +47,30 @@ public Task BindModelAsync(ModelBindingContext bindingContext)
             {
                 var value = valueProviderResult.FirstValue;
 
-                object model = null;
-                if (!string.IsNullOrWhiteSpace(value))
-                {
-                    model = _typeConverter.ConvertFrom(
-                        context: null,
-                        culture: valueProviderResult.Culture,
-                        value: value);
-                }
-
+                object model;
                 if (bindingContext.ModelType == typeof(string))
                 {
-                    var modelAsString = model as string;
-                    if (bindingContext.ModelMetadata.ConvertEmptyStringToNull &&
-                        string.IsNullOrEmpty(modelAsString))
+                    // 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,
+                        culture: valueProviderResult.Culture,
+                        value: value);
                 }
 
                 // When converting newModel a null value may indicate a failed conversion for an otherwise required
@@ -75,7 +82,7 @@ public Task BindModelAsync(ModelBindingContext bindingContext)
                         bindingContext.ModelName,
                         bindingContext.ModelMetadata.ModelBindingMessageProvider.ValueMustNotBeNullAccessor(
                             valueProviderResult.ToString()));
-                    
+
                     return TaskCache.CompletedTask;
                 }
                 else

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Metadata/DisplayMetadata.cs

@@ -17,8 +17,9 @@ public class DisplayMetadata
         public IDictionary<object, object> AdditionalValues { get; } = new Dictionary<object, object>();
 
         /// <summary>
-        /// Gets or sets a value indicating whether or not empty strings should be treated as <c>null</c>.
-        /// See <see cref="ModelMetadata.ConvertEmptyStringToNull"/>
+        /// Gets or sets a value indicating whether or not to convert an empty string value or one containing only
+        /// whitespace characters to <c>null</c> when representing a model as text. See
+        /// <see cref="ModelMetadata.ConvertEmptyStringToNull"/>
         /// </summary>
         public bool ConvertEmptyStringToNull { get; set; } = true;
 

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<Type> ConvertableTypeData
         {
             get
@@ -125,26 +177,6 @@ public async Task BindModel_EmptyValueProviderResult_ReturnsFailed()
             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 @@ public async Task BindModel_ValidValueProviderResult_ReturnsModel()
             // Arrange
             var bindingContext = GetBindingContext(typeof(int));
             bindingContext.ValueProvider = new SimpleValueProvider
-            { 
+            {
                 { "theModelName", "42" }
             };