From fc2019c973e84c8b7faa99793cba56412e032c70 Mon Sep 17 00:00:00 2001
From: "N. Taylor Mullen" <nimullen@microsoft.com>
Date: Thu, 2 Jul 2015 12:22:33 -0700
Subject: [PATCH] Modify `TypeConverterModelBinder`'s
 `ModelBindingResult.IsModelSet` to be false when model value is `null` for
 non-null accepting types.

- Previously `ModelBindingResult.IsModelSet` would be set to true if type conversions resulted in empty => `null` values for ValueTypes. This resulted in improper usage of `ModelBindingResult`s creating null ref exceptions in certain cases.
- Updated existing functional test to account for new behavior. Previously it was handling the null ref exception by stating that errors were simply invalid; now we can provide a more distinct error.
- Added unit test to validate `TypeConverterModelBinder` does what it's supposed to when conversions result in null values.
- Added additional integration tests for `TypeConverterModelBinder`.

#2720
---
 .../ModelBinding/TypeConverterModelBinder.cs  | 26 ++++--
 .../TypeConverterModelBinderTest.cs           | 33 ++++++-
 ...WebSite.HtmlGeneration_Customer.Index.html |  3 +-
 ...TypeConverterModelBinderIntegrationTest.cs | 88 +++++++++++++++++++
 4 files changed, 142 insertions(+), 8 deletions(-)

diff --git a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/TypeConverterModelBinder.cs b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/TypeConverterModelBinder.cs
index c0d5a59f96..af07c39073 100644
--- a/src/Microsoft.AspNet.Mvc.Core/ModelBinding/TypeConverterModelBinder.cs
+++ b/src/Microsoft.AspNet.Mvc.Core/ModelBinding/TypeConverterModelBinder.cs
@@ -2,7 +2,9 @@
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
 using System;
+using System.Reflection;
 using System.Threading.Tasks;
+using Microsoft.AspNet.Mvc.Core;
 
 namespace Microsoft.AspNet.Mvc.ModelBinding
 {
@@ -34,12 +36,21 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
                    bindingContext.ModelName,
                    bindingContext.ModelMetadata,
                    newModel);
+                var isModelSet = true;
 
-                return new ModelBindingResult(
-                    newModel,
-                    bindingContext.ModelName,
-                    isModelSet: true,
-                    validationNode: validationNode);
+                // 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.
+                if (newModel == null && !AllowsNullValue(bindingContext.ModelType))
+                {
+                    bindingContext.ModelState.TryAddModelError(
+                        bindingContext.ModelName,
+                        Resources.FormatCommon_ValueNotValidForProperty(newModel));
+
+                    isModelSet = false;
+                }
+
+                return new ModelBindingResult(newModel, bindingContext.ModelName, isModelSet, validationNode);
             }
             catch (Exception ex)
             {
@@ -50,5 +61,10 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
             // Tell the model binding system to skip other model binders i.e. return non-null.
             return new ModelBindingResult(model: null, key: bindingContext.ModelName, isModelSet: false);
         }
+
+        private static bool AllowsNullValue(Type type)
+        {
+            return !type.GetTypeInfo().IsValueType || Nullable.GetUnderlyingType(type) != null;
+        }
     }
 }
diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/TypeConverterModelBinderTest.cs b/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/TypeConverterModelBinderTest.cs
index 3c6e139c41..2a3206b938 100644
--- a/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/TypeConverterModelBinderTest.cs
+++ b/test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/TypeConverterModelBinderTest.cs
@@ -67,6 +67,35 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test
             Assert.NotNull(retVal);
         }
 
+        [Theory]
+        [InlineData(typeof(byte))]
+        [InlineData(typeof(short))]
+        [InlineData(typeof(int))]
+        [InlineData(typeof(long))]
+        [InlineData(typeof(Guid))]
+        [InlineData(typeof(double))]
+        [InlineData(typeof(DayOfWeek))]
+        public async Task BindModel_CreatesError_WhenTypeConversionIsNull(Type destinationType)
+        {
+            // Arrange
+            var bindingContext = GetBindingContext(destinationType);
+            bindingContext.ValueProvider = new SimpleHttpValueProvider
+            {
+                { "theModelName", string.Empty }
+            };
+            var binder = new TypeConverterModelBinder();
+
+            // Act
+            var result = await binder.BindModelAsync(bindingContext);
+
+            // Assert
+            Assert.False(result.IsModelSet);
+            Assert.NotNull(result.ValidationNode);
+            var error = Assert.Single(bindingContext.ModelState["theModelName"].Errors);
+            Assert.Equal(error.ErrorMessage, "The value '' is invalid.", StringComparer.Ordinal);
+            Assert.Null(error.Exception);
+        }
+
         [Fact]
         public async Task BindModel_Error_FormatExceptionsTurnedIntoStringsInModelState()
         {
@@ -113,7 +142,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test
             var bindingContext = GetBindingContext(typeof(string));
             bindingContext.ValueProvider = new SimpleHttpValueProvider
             {
-                { "theModelName", "" }
+                { "theModelName", string.Empty }
             };
 
             var binder = new TypeConverterModelBinder();
@@ -144,7 +173,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test
 
             // Assert
             Assert.NotNull(retVal);
-            Assert.Equal(42, retVal.Model);;
+            Assert.Equal(42, retVal.Model);
             Assert.True(bindingContext.ModelState.ContainsKey("theModelName"));
         }
 
diff --git a/test/Microsoft.AspNet.Mvc.FunctionalTests/compiler/resources/HtmlGenerationWebSite.HtmlGeneration_Customer.Index.html b/test/Microsoft.AspNet.Mvc.FunctionalTests/compiler/resources/HtmlGenerationWebSite.HtmlGeneration_Customer.Index.html
index 631e57ecad..7d6cf27c87 100644
--- a/test/Microsoft.AspNet.Mvc.FunctionalTests/compiler/resources/HtmlGenerationWebSite.HtmlGeneration_Customer.Index.html
+++ b/test/Microsoft.AspNet.Mvc.FunctionalTests/compiler/resources/HtmlGenerationWebSite.HtmlGeneration_Customer.Index.html
@@ -30,7 +30,8 @@
             <input type="radio" value="Female" id="Gender" name="Gender" /> Female
             <span class="field-validation-valid" data-valmsg-for="Gender" data-valmsg-replace="true"></span>
         </div>
-        <div class="order validation-summary-errors" data-valmsg-summary="true"><ul><li>The Password field is required.</li>
+        <div class="order validation-summary-errors" data-valmsg-summary="true"><ul><li>The value &#x27;&#x27; is invalid.</li>
+<li>The Password field is required.</li>
 </ul></div>
         <div class="order validation-summary-errors"><ul><li style="display:none"></li>
 </ul></div>
diff --git a/test/Microsoft.AspNet.Mvc.IntegrationTests/TypeConverterModelBinderIntegrationTest.cs b/test/Microsoft.AspNet.Mvc.IntegrationTests/TypeConverterModelBinderIntegrationTest.cs
index 74ef9990c3..ecca5e01ba 100644
--- a/test/Microsoft.AspNet.Mvc.IntegrationTests/TypeConverterModelBinderIntegrationTest.cs
+++ b/test/Microsoft.AspNet.Mvc.IntegrationTests/TypeConverterModelBinderIntegrationTest.cs
@@ -1,6 +1,8 @@
 // Copyright (c) .NET Foundation. All rights reserved.
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
+using System;
+using System.Collections;
 using System.Threading.Tasks;
 using Microsoft.AspNet.Http;
 using Microsoft.AspNet.Mvc.ModelBinding;
@@ -164,6 +166,92 @@ namespace Microsoft.AspNet.Mvc.IntegrationTests
             Assert.Equal(ModelValidationState.Valid, modelState[key].ValidationState);
         }
 
+        [Theory]
+        [InlineData(typeof(int))]
+        [InlineData(typeof(bool))]
+        public async Task BindParameter_WithEmptyData_DoesNotBind(Type parameterType)
+        {
+            // Arrange
+            var argumentBinder = ModelBindingTestHelper.GetArgumentBinder();
+            var parameter = new ParameterDescriptor
+            {
+                Name = "Parameter1",
+                BindingInfo = new BindingInfo(),
+
+                ParameterType = parameterType
+            };
+            var operationContext = ModelBindingTestHelper.GetOperationBindingContext(request =>
+            {
+                request.QueryString = QueryString.Create("Parameter1", string.Empty);
+            });
+            var modelState = new ModelStateDictionary();
+
+            // Act
+            var modelBindingResult = await argumentBinder.BindModelAsync(parameter, modelState, operationContext);
+
+            // Assert
+
+            // ModelBindingResult
+            Assert.NotNull(modelBindingResult);
+            Assert.False(modelBindingResult.IsModelSet);
+
+            // Model
+            Assert.Null(modelBindingResult.Model);
+
+            // ModelState
+            Assert.False(modelState.IsValid);
+            var key = Assert.Single(modelState.Keys);
+            Assert.Equal("Parameter1", key);
+            Assert.Equal(string.Empty, modelState[key].Value.AttemptedValue);
+            Assert.Equal(string.Empty, modelState[key].Value.RawValue);
+            var error = Assert.Single(modelState[key].Errors);
+            Assert.Equal(error.ErrorMessage, "The value '' is invalid.", StringComparer.Ordinal);
+            Assert.Null(error.Exception);
+        }
+
+        [InlineData(typeof(int?))]
+        [InlineData(typeof(bool?))]
+        [InlineData(typeof(string))]
+        [InlineData(typeof(object))]
+        [InlineData(typeof(IEnumerable))]
+        public async Task BindParameter_WithEmptyData_BindsMutableAndNullableObjects(Type parameterType)
+        {
+            // Arrange
+            var argumentBinder = ModelBindingTestHelper.GetArgumentBinder();
+            var parameter = new ParameterDescriptor
+            {
+                Name = "Parameter1",
+                BindingInfo = new BindingInfo(),
+
+                ParameterType = parameterType
+            };
+            var operationContext = ModelBindingTestHelper.GetOperationBindingContext(request =>
+            {
+                request.QueryString = QueryString.Create("Parameter1", string.Empty);
+            });
+            var modelState = new ModelStateDictionary();
+
+            // Act
+            var modelBindingResult = await argumentBinder.BindModelAsync(parameter, modelState, operationContext);
+
+            // Assert
+
+            // ModelBindingResult
+            Assert.NotNull(modelBindingResult);
+            Assert.True(modelBindingResult.IsModelSet);
+
+            // Model
+            Assert.Null(modelBindingResult.Model);
+
+            // ModelState
+            Assert.True(modelState.IsValid);
+            var key = Assert.Single(modelState.Keys);
+            Assert.Equal("Parameter1", key);
+            Assert.Equal(string.Empty, modelState[key].Value.AttemptedValue);
+            Assert.Equal(string.Empty, modelState[key].Value.RawValue);
+            Assert.Empty(modelState[key].Errors);
+        }
+
         [Fact]
         public async Task BindParameter_NoData_DoesNotGetBound()
         {