Add `ModelValidationNode`s consistently

- #2633
- do not leave `ModelBindingResult.ValidationNode` as `null` when we hit the `null` `RawValue` special case
 - move two bits of code together to make the special case more obvious
- add `ModelValidationNode` (that suppresses validation) when `HttpRequestMessageModelBinder` is successful
 - also suppress validation of `HttpRequestMEssage` properties
- suppress validation in `CancellationTokenModelBinder`, `FormCollectionModelBinder`, `FormCollectionModelBinder`
- do not create a `ModelValidationNode` when validation fails in `TypeConverterModelBinder`

nits:
- improve some doc comments
- add a quick `HttpRequestMessageModelBinderTest`

src/Microsoft.AspNet.Mvc.Core/ModelBinding/CancellationTokenModelBinder.cs

@@ -7,7 +7,7 @@ using System.Threading.Tasks;
 namespace Microsoft.AspNet.Mvc.ModelBinding
 {
     /// <summary>
-    /// Represents a model binder which can bind models of type <see cref="CancellationToken"/>.
+    /// <see cref="IModelBinder"/> implementation to bind models of type <see cref="CancellationToken"/>.
     /// </summary>
     public class CancellationTokenModelBinder : IModelBinder
     {
@@ -18,7 +18,11 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
             {
                 var model = bindingContext.OperationBindingContext.HttpContext.RequestAborted;
                 var validationNode =
-                    new ModelValidationNode(bindingContext.ModelName, bindingContext.ModelMetadata, model);
+                    new ModelValidationNode(bindingContext.ModelName, bindingContext.ModelMetadata, model)
+                    {
+                        SuppressValidation = true,
+                    };
+
                 return Task.FromResult(new ModelBindingResult(
                     model,
                     bindingContext.ModelName,

src/Microsoft.AspNet.Mvc.Core/ModelBinding/CollectionModelBinder.cs

@@ -62,6 +62,21 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
             }
             else
             {
+                if (valueProviderResult.RawValue == null)
+                {
+                    // Value exists but is null. Handle similarly to fallback case above. This avoids a
+                    // ModelBindingResult with IsModelSet = true but ValidationNode = null.
+                    model = bindingContext.Model ?? CreateEmptyCollection();
+                    var validationNode =
+                        new ModelValidationNode(bindingContext.ModelName, bindingContext.ModelMetadata, model);
+
+                    return new ModelBindingResult(
+                        model,
+                        bindingContext.ModelName,
+                        isModelSet: true,
+                        validationNode: validationNode);
+                }
+
                 result = await BindSimpleCollection(
                     bindingContext,
                     valueProviderResult.RawValue,
@@ -83,7 +98,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
                 model,
                 bindingContext.ModelName,
                 isModelSet: true,
-                validationNode: result?.ValidationNode);
+                validationNode: result.ValidationNode);
         }
 
         /// <inheritdoc />
@@ -135,11 +150,6 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
             object rawValue,
             CultureInfo culture)
         {
-            if (rawValue == null)
-            {
-                return null; // nothing to do
-            }
-
             var boundCollection = new List<TElement>();
 
             var metadataProvider = bindingContext.OperationBindingContext.MetadataProvider;

src/Microsoft.AspNet.Mvc.Core/ModelBinding/FormCollectionModelBinder.cs

@@ -12,7 +12,7 @@ using Microsoft.Framework.Internal;
 namespace Microsoft.AspNet.Mvc.ModelBinding
 {
     /// <summary>
-    /// Modelbinder to bind form values to <see cref="IFormCollection"/>.
+    /// <see cref="IModelBinder"/> implementation to bind form values to <see cref="IFormCollection"/>.
     /// </summary>
     public class FormCollectionModelBinder : IModelBinder
     {
@@ -46,7 +46,11 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
             }
 
             var validationNode =
-                 new ModelValidationNode(bindingContext.ModelName, bindingContext.ModelMetadata, model);
+                 new ModelValidationNode(bindingContext.ModelName, bindingContext.ModelMetadata, model)
+                 {
+                     SuppressValidation = true,
+                 };
+
             return new ModelBindingResult(
                 model,
                 bindingContext.ModelName,

src/Microsoft.AspNet.Mvc.Core/ModelBinding/FormFileModelBinder.cs

@@ -15,7 +15,7 @@ using Microsoft.Net.Http.Headers;
 namespace Microsoft.AspNet.Mvc.ModelBinding
 {
     /// <summary>
-    /// Modelbinder to bind posted files to <see cref="IFormFile"/>.
+    /// <see cref="IModelBinder"/> implementation to bind posted files to <see cref="IFormFile"/>.
     /// </summary>
     public class FormFileModelBinder : IModelBinder
     {
@@ -43,7 +43,10 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
             if (value != null)
             {
                 validationNode =
-                    new ModelValidationNode(bindingContext.ModelName, bindingContext.ModelMetadata, value);
+                    new ModelValidationNode(bindingContext.ModelName, bindingContext.ModelMetadata, value)
+                    {
+                        SuppressValidation = true,
+                    };
 
                 var valueProviderResult = new ValueProviderResult(rawValue: value);
                 bindingContext.ModelState.SetModelValue(bindingContext.ModelName, valueProviderResult);

src/Microsoft.AspNet.Mvc.Core/ModelBinding/TypeConverterModelBinder.cs

@@ -23,7 +23,8 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
             var valueProviderResult = await bindingContext.ValueProvider.GetValueAsync(bindingContext.ModelName);
             if (valueProviderResult == null)
             {
-                return null; // no entry
+                // no entry
+                return null;
             }
 
             object newModel;
@@ -32,13 +33,9 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
             {
                 newModel = valueProviderResult.ConvertTo(bindingContext.ModelType);
                 ModelBindingHelper.ReplaceEmptyStringWithNull(bindingContext.ModelMetadata, ref newModel);
-                var validationNode = new ModelValidationNode(
-                   bindingContext.ModelName,
-                   bindingContext.ModelMetadata,
-                   newModel);
                 var isModelSet = true;
 
-                // When converting newModel a null value may indicate a failed conversion for an otherwise required 
+                // 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))
@@ -50,6 +47,11 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
                     isModelSet = false;
                 }
 
+                // Include a ModelValidationNode if binding succeeded.
+                var validationNode = isModelSet ?
+                    new ModelValidationNode(bindingContext.ModelName, bindingContext.ModelMetadata, newModel) :
+                    null;
+
                 return new ModelBindingResult(newModel, bindingContext.ModelName, isModelSet, validationNode);
             }
             catch (Exception ex)

src/Microsoft.AspNet.Mvc.WebApiCompatShim/HttpRequestMessage/HttpRequestMessageModelBinder.cs

@@ -7,14 +7,28 @@ using Microsoft.AspNet.Mvc.ModelBinding;
 
 namespace Microsoft.AspNet.Mvc.WebApiCompatShim
 {
+    /// <summary>
+    /// <see cref="IModelBinder"/> implementation to bind models of type <see cref="HttpRequestMessage"/>.
+    /// </summary>
     public class HttpRequestMessageModelBinder : IModelBinder
     {
+        /// <inheritdoc />
         public Task<ModelBindingResult> BindModelAsync(ModelBindingContext bindingContext)
         {
             if (bindingContext.ModelType == typeof(HttpRequestMessage))
             {
                 var model = bindingContext.OperationBindingContext.HttpContext.GetHttpRequestMessage();
-                return Task.FromResult(new ModelBindingResult(model, bindingContext.ModelName, isModelSet: true));
+                var validationNode =
+                    new ModelValidationNode(bindingContext.ModelName, bindingContext.ModelMetadata, model)
+                    {
+                        SuppressValidation = true,
+                    };
+
+                return Task.FromResult(new ModelBindingResult(
+                    model,
+                    bindingContext.ModelName,
+                    isModelSet: true,
+                    validationNode: validationNode));
             }
 
             return Task.FromResult<ModelBindingResult>(null);

src/Microsoft.AspNet.Mvc.WebApiCompatShim/WebApiCompatShimOptionsSetup.cs

@@ -37,6 +37,7 @@ namespace Microsoft.AspNet.Mvc.WebApiCompatShim
             // Add a formatter to write out an HttpResponseMessage to the response
             options.OutputFormatters.Insert(0, new HttpResponseMessageOutputFormatter());
 
+            options.ValidationExcludeFilters.Add(typeof(HttpRequestMessage));
             options.ValidationExcludeFilters.Add(typeof(HttpResponseMessage));
         }
 

test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/CancellationTokenModelBinderTests.cs

@@ -19,11 +19,14 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test
             var binder = new CancellationTokenModelBinder();
 
             // Act
-            var bound = await binder.BindModelAsync(bindingContext);
+            var result = await binder.BindModelAsync(bindingContext);
 
             // Assert
-            Assert.NotNull(bound);
-            Assert.Equal(bindingContext.OperationBindingContext.HttpContext.RequestAborted, bound.Model);
+            Assert.NotNull(result);
+            Assert.True(result.IsModelSet);
+            Assert.Equal(bindingContext.OperationBindingContext.HttpContext.RequestAborted, result.Model);
+            Assert.NotNull(result.ValidationNode);
+            Assert.True(result.ValidationNode.SuppressValidation);
         }
 
         [Theory]
@@ -37,10 +40,10 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test
             var binder = new CancellationTokenModelBinder();
 
             // Act
-            var bound = await binder.BindModelAsync(bindingContext);
+            var result = await binder.BindModelAsync(bindingContext);
 
             // Assert
-            Assert.Null(bound);
+            Assert.Null(result);
         }
 
         private static ModelBindingContext GetBindingContext(Type modelType)

test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/CollectionModelBinderTest.cs

@@ -181,6 +181,33 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test
 
             Assert.True(modelState.IsValid);
         }
+
+        [Fact]
+        public async Task BindModelAsync_SimpleCollectionWithNullValue_Succeeds()
+        {
+            // Arrange
+            var binder = new CollectionModelBinder<int>();
+            var valueProvider = new SimpleHttpValueProvider
+            {
+                { "someName", null },
+            };
+            var bindingContext = GetModelBindingContext(valueProvider, isReadOnly: false);
+            var modelState = bindingContext.ModelState;
+
+            // Act
+            var result = await binder.BindModelAsync(bindingContext);
+
+            // Assert
+            Assert.NotNull(result);
+            Assert.True(result.IsModelSet);
+            Assert.NotNull(result.Model);
+            Assert.NotNull(result.ValidationNode);
+
+            var model = Assert.IsType<List<int>>(result.Model);
+            Assert.Empty(model);
+
+            Assert.True(modelState.IsValid);
+        }
 #endif
 
         [Fact]
@@ -205,19 +232,6 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test
             Assert.Empty(boundCollection.Model);
         }
 
-        [Fact]
-        public async Task BindSimpleCollection_RawValueIsNull_ReturnsNull()
-        {
-            // Arrange
-            var binder = new CollectionModelBinder<int>();
-
-            // Act
-            var boundCollection = await binder.BindSimpleCollection(bindingContext: null, rawValue: null, culture: null);
-
-            // Assert
-            Assert.Null(boundCollection);
-        }
-
         [Fact]
         public async Task CollectionModelBinder_DoesNotCreateCollection_IfIsTopLevelObjectAndIsFirstChanceBinding()
         {

test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/FormCollectionModelBinderTest.cs

@@ -34,6 +34,10 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
 
             // Assert
             Assert.NotNull(result);
+            Assert.True(result.IsModelSet);
+            Assert.NotNull(result.ValidationNode);
+            Assert.True(result.ValidationNode.SuppressValidation);
+
             var form = Assert.IsAssignableFrom<IFormCollection>(result.Model);
             Assert.Equal(2, form.Count);
             Assert.Equal("value1", form["field1"]);

test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/FormFileModelBinderTest.cs

@@ -32,6 +32,10 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
 
             // Assert
             Assert.NotNull(result);
+            Assert.True(result.IsModelSet);
+            Assert.NotNull(result.ValidationNode);
+            Assert.True(result.ValidationNode.SuppressValidation);
+
             var files = Assert.IsAssignableFrom<IList<IFormFile>>(result.Model);
             Assert.Equal(2, files.Count);
         }

test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/TypeConverterModelBinderTest.cs

@@ -90,7 +90,9 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Test
 
             // Assert
             Assert.False(result.IsModelSet);
-            Assert.NotNull(result.ValidationNode);
+            Assert.Null(result.Model);
+            Assert.Null(result.ValidationNode);
+
             var error = Assert.Single(bindingContext.ModelState["theModelName"].Errors);
             Assert.Equal(error.ErrorMessage, "The value '' is invalid.", StringComparer.Ordinal);
             Assert.Null(error.Exception);

test/Microsoft.AspNet.Mvc.IntegrationTests/FormFileModelBindingIntegrationTest.cs

@@ -73,7 +73,7 @@ namespace Microsoft.AspNet.Mvc.IntegrationTests
             var key = Assert.Single(modelState.Keys, k => k == "Address.File");
             Assert.NotNull(modelState[key].Value);
             Assert.Empty(modelState[key].Errors);
-            Assert.Equal(ModelValidationState.Valid, modelState[key].ValidationState);
+            Assert.Equal(ModelValidationState.Skipped, modelState[key].ValidationState);
         }
 
         [Fact]
@@ -121,7 +121,7 @@ namespace Microsoft.AspNet.Mvc.IntegrationTests
             var entry = Assert.Single(modelState);
             Assert.Equal("CustomParameter", entry.Key);
             Assert.Empty(entry.Value.Errors);
-            Assert.Equal(ModelValidationState.Valid, entry.Value.ValidationState);
+            Assert.Equal(ModelValidationState.Skipped, entry.Value.ValidationState);
             Assert.Null(entry.Value.Value.AttemptedValue);
             Assert.Equal(file, entry.Value.Value.RawValue);
         }

test/Microsoft.AspNet.Mvc.WebApiCompatShimTest/HttpRequestMessage/HttpRequestMessageModelBinderTest.cs

@@ -0,0 +1,70 @@
+// 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.Net.Http;
+using System.Threading.Tasks;
+using Microsoft.AspNet.Http.Internal;
+using Microsoft.AspNet.Mvc.ModelBinding;
+using Xunit;
+
+namespace Microsoft.AspNet.Mvc.WebApiCompatShim
+{
+    public class HttpRequestMessageModelBinderTest
+    {
+        [Fact]
+        public async Task BindModelAsync_ReturnsNotNull_ForHttpRequestMessageType()
+        {
+            // Arrange
+            var binder = new HttpRequestMessageModelBinder();
+            var bindingContext = GetBindingContext(typeof(HttpRequestMessage));
+            var expectedModel = bindingContext.OperationBindingContext.HttpContext.GetHttpRequestMessage();
+
+            // Act
+            var result = await binder.BindModelAsync(bindingContext);
+
+            // Assert
+            Assert.NotNull(result);
+            Assert.True(result.IsModelSet);
+            Assert.Same(expectedModel, result.Model);
+            Assert.NotNull(result.ValidationNode);
+            Assert.True(result.ValidationNode.SuppressValidation);
+        }
+
+        [Theory]
+        [InlineData(typeof(int))]
+        [InlineData(typeof(object))]
+        [InlineData(typeof(HttpRequestMessageModelBinderTest))]
+        public async Task BindModelAsync_ReturnsNull_ForNonHttpRequestMessageType(Type type)
+        {
+            // Arrange
+            var binder = new HttpRequestMessageModelBinder();
+            var bindingContext = GetBindingContext(type);
+
+            // Act
+            var result = await binder.BindModelAsync(bindingContext);
+
+            // Assert
+            Assert.Null(result);
+        }
+
+        private static ModelBindingContext GetBindingContext(Type modelType)
+        {
+            var metadataProvider = new EmptyModelMetadataProvider();
+            ModelBindingContext bindingContext = new ModelBindingContext
+            {
+                ModelMetadata = metadataProvider.GetMetadataForType(modelType),
+                ModelName = "someName",
+                OperationBindingContext = new OperationBindingContext
+                {
+                    HttpContext = new DefaultHttpContext(),
+                    MetadataProvider = metadataProvider,
+                }
+            };
+
+            bindingContext.OperationBindingContext.HttpContext.Request.Method = "GET";
+
+            return bindingContext;
+        }
+    }
+}