Make validation in `TryUpdateModelAsync()` consistent with model binding elsewhere

- #2941
- honor `ModelBindingResult.IsModelSet` and use `ModelBindingResult.ValidationNode`
  - enable correct validation of collections or after model binding falls back to the empty prefix
  - code previously matched `Controller.TryValidateModel()`; less context available in that case

src/Microsoft.AspNet.Mvc.Core/ModelBindingHelper.cs

@@ -3,6 +3,7 @@
 
 using System;
 using System.Collections.Generic;
+using System.Diagnostics;
 using System.Linq;
 using System.Linq.Expressions;
 using System.Reflection;
@@ -306,16 +307,18 @@ namespace Microsoft.AspNet.Mvc
             };
 
             var modelBindingResult = await modelBinder.BindModelAsync(modelBindingContext);
-            if (modelBindingResult != null)
+            if (modelBindingResult != null && modelBindingResult.IsModelSet)
             {
                 var modelExplorer = new ModelExplorer(metadataProvider, modelMetadata, modelBindingResult.Model);
                 var modelValidationContext = new ModelValidationContext(modelBindingContext, modelExplorer);
-                objectModelValidator.Validate(
+
-                    modelValidationContext,
+                var validationNode = modelBindingResult.ValidationNode;
-                    new ModelValidationNode(prefix, modelBindingContext.ModelMetadata, modelBindingResult.Model)
+                Debug.Assert(
-                    {
+                    validationNode != null,
-                        ValidateAllProperties = true
+                    "ValidationNode should never be null in a successful ModelBindingResult.");
-                    });
+
+                objectModelValidator.Validate(modelValidationContext, validationNode);
+
                 return modelState.IsValid;
             }
 

test/Microsoft.AspNet.Mvc.Core.Test/ModelBindingHelperTest.cs

@@ -17,15 +17,29 @@ namespace Microsoft.AspNet.Mvc.Test
 {
     public class ModelBindingHelperTest
     {
-        [Fact]
+        public static TheoryData<ModelBindingResult> UnsuccessfulModelBindingData
-        public async Task TryUpdateModel_ReturnsFalse_IfBinderReturnsNull()
+        {
+            get
+            {
+                return new TheoryData<ModelBindingResult>
+                {
+                    null,
+                    new ModelBindingResult("someKey"), // IsFatalError true as well as IsModelSet false.
+                    new ModelBindingResult(model: null, key: "someKey", isModelSet: false),
+                };
+            }
+        }
+
+        [Theory]
+        [MemberData(nameof(UnsuccessfulModelBindingData))]
+        public async Task TryUpdateModel_ReturnsFalse_IfBinderIsUnsuccessful(ModelBindingResult binderResult)
         {
             // Arrange
             var metadataProvider = new EmptyModelMetadataProvider();
-
             var binder = new Mock<IModelBinder>();
-            binder.Setup(b => b.BindModelAsync(It.IsAny<ModelBindingContext>()))
+            binder
-                  .Returns(Task.FromResult<ModelBindingResult>(null));
+                .Setup(b => b.BindModelAsync(It.IsAny<ModelBindingContext>()))
+                .Returns(Task.FromResult<ModelBindingResult>(binderResult));
             var model = new MyModel();
 
             // Act
@@ -126,18 +140,20 @@ namespace Microsoft.AspNet.Mvc.Test
             Assert.Equal("MyPropertyValue", model.MyProperty);
         }
 
-        [Fact]
+        [Theory]
-        public async Task TryUpdateModel_UsingIncludePredicateOverload_ReturnsFalse_IfBinderReturnsNull()
+        [MemberData(nameof(UnsuccessfulModelBindingData))]
+        public async Task TryUpdateModel_UsingIncludePredicateOverload_ReturnsFalse_IfBinderIsUnsuccessful(
+            ModelBindingResult binderResult)
         {
             // Arrange
             var metadataProvider = new EmptyModelMetadataProvider();
-
             var binder = new Mock<IModelBinder>();
-            binder.Setup(b => b.BindModelAsync(It.IsAny<ModelBindingContext>()))
+            binder
-                  .Returns(Task.FromResult<ModelBindingResult>(null));
+                .Setup(b => b.BindModelAsync(It.IsAny<ModelBindingContext>()))
+                .Returns(Task.FromResult<ModelBindingResult>(binderResult));
             var model = new MyModel();
-            Func<ModelBindingContext, string, bool> includePredicate =
+            Func<ModelBindingContext, string, bool> includePredicate = (context, propertyName) => true;
-               (context, propertyName) => true;
+
             // Act
             var result = await ModelBindingHelper.TryUpdateModelAsync(
                 model,
@@ -214,15 +230,17 @@ namespace Microsoft.AspNet.Mvc.Test
             Assert.Equal("Old-ExcludedPropertyValue", model.ExcludedProperty);
         }
 
-        [Fact]
+        [Theory]
-        public async Task TryUpdateModel_UsingIncludeExpressionOverload_ReturnsFalse_IfBinderReturnsNull()
+        [MemberData(nameof(UnsuccessfulModelBindingData))]
+        public async Task TryUpdateModel_UsingIncludeExpressionOverload_ReturnsFalse_IfBinderIsUnsuccessful(
+            ModelBindingResult binderResult)
         {
             // Arrange
             var metadataProvider = new EmptyModelMetadataProvider();
-
             var binder = new Mock<IModelBinder>();
-            binder.Setup(b => b.BindModelAsync(It.IsAny<ModelBindingContext>()))
+            binder
-                  .Returns(Task.FromResult<ModelBindingResult>(null));
+                .Setup(b => b.BindModelAsync(It.IsAny<ModelBindingContext>()))
+                .Returns(Task.FromResult<ModelBindingResult>(binderResult));
             var model = new MyModel();
 
             // Act
@@ -468,17 +486,20 @@ namespace Microsoft.AspNet.Mvc.Test
                          ex.Message);
         }
 
-        [Fact]
+        [Theory]
-        public async Task TryUpdateModelNonGeneric_PredicateOverload_ReturnsFalse_IfBinderReturnsNull()
+        [MemberData(nameof(UnsuccessfulModelBindingData))]
+        public async Task TryUpdateModelNonGeneric_PredicateOverload_ReturnsFalse_IfBinderIsUnsuccessful(
+            ModelBindingResult binderResult)
         {
             // Arrange
             var metadataProvider = new EmptyModelMetadataProvider();
             var binder = new Mock<IModelBinder>();
-            binder.Setup(b => b.BindModelAsync(It.IsAny<ModelBindingContext>()))
+            binder
-                  .Returns(Task.FromResult<ModelBindingResult>(null));
+                .Setup(b => b.BindModelAsync(It.IsAny<ModelBindingContext>()))
+                .Returns(Task.FromResult<ModelBindingResult>(binderResult));
             var model = new MyModel();
-            Func<ModelBindingContext, string, bool> includePredicate =
+            Func<ModelBindingContext, string, bool> includePredicate = (context, propertyName) => true;
-               (context, propertyName) => true;
+
             // Act
             var result = await ModelBindingHelper.TryUpdateModelAsync(
                                                     model,
@@ -560,15 +581,17 @@ namespace Microsoft.AspNet.Mvc.Test
             Assert.Equal("Old-ExcludedPropertyValue", model.ExcludedProperty);
         }
 
-        [Fact]
+        [Theory]
-        public async Task TryUpdateModelNonGeneric_ModelTypeOverload_ReturnsFalse_IfBinderReturnsNull()
+        [MemberData(nameof(UnsuccessfulModelBindingData))]
+        public async Task TryUpdateModelNonGeneric_ModelTypeOverload_ReturnsFalse_IfBinderIsUnsuccessful(
+            ModelBindingResult binderResult)
         {
             // Arrange
             var metadataProvider = new EmptyModelMetadataProvider();
-
             var binder = new Mock<IModelBinder>();
-            binder.Setup(b => b.BindModelAsync(It.IsAny<ModelBindingContext>()))
+            binder
-                  .Returns(Task.FromResult<ModelBindingResult>(null));
+                .Setup(b => b.BindModelAsync(It.IsAny<ModelBindingContext>()))
+                .Returns(Task.FromResult<ModelBindingResult>(binderResult));
             var model = new MyModel();
 
             // Act

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

@@ -102,6 +102,63 @@ namespace Microsoft.AspNet.Mvc.IntegrationTests
             public Address Address { get; set; }
         }
 
+        [Fact]
+        public async Task TryUpdateModel_TopLevelCollection_EmptyPrefix_BindsAfterClearing()
+        {
+            // Arrange
+            var operationContext = ModelBindingTestHelper.GetOperationBindingContext(request =>
+            {
+                request.QueryString = QueryString.Create(new Dictionary<string, string>
+                {
+                    { "[0].Name", "One Name" },
+                    { "[1].Address.Street", "Two Street" },
+                });
+            });
+
+            var modelState = new ModelStateDictionary();
+            var model = new List<Person1>
+            {
+                new Person1
+                {
+                    Name = "One",
+                    Address = new Address
+                    {
+                        Street = "DefaultStreet",
+                        City = "Toronto",
+                    },
+                },
+                new Person1 { Name = "Two" },
+                new Person1 { Name = "Three" },
+            };
+
+            // Act
+            var result = await TryUpdateModel(model, string.Empty, operationContext, modelState);
+
+            // Assert
+            Assert.True(result);
+
+            // Model
+            Assert.Collection(model,
+                element =>
+                {
+                    Assert.Equal("One Name", element.Name);
+                    Assert.Null(element.Address);
+                },
+                element =>
+                {
+                    Assert.Null(element.Name);
+                    Assert.NotNull(element.Address);
+                    Assert.Equal("Two Street", element.Address.Street);
+                    Assert.Null(element.Address.City);
+                });
+
+            // ModelState
+            Assert.True(modelState.IsValid);
+            Assert.Equal(2, modelState.Count);
+            Assert.NotNull(modelState["[0].Name"]);
+            Assert.NotNull(modelState["[1].Address.Street"]);
+        }
+
         [Fact]
         public async Task TryUpdateModel_NestedPoco_EmptyPrefix_DoesNotTrounceUnboundValues()
         {
@@ -299,7 +356,6 @@ namespace Microsoft.AspNet.Mvc.IntegrationTests
             public CustomReadOnlyCollection<Address> Address { get; set; }
         }
 
-        [Fact(Skip = "Validation incorrect for collections when using TryUpdateModel, #2941")]
         public async Task TryUpdateModel_ReadOnlyCollectionModel_EmptyPrefix_DoesNotGetBound()
         {
             // Arrange
@@ -712,7 +768,6 @@ namespace Microsoft.AspNet.Mvc.IntegrationTests
             Assert.Equal(ModelValidationState.Valid, state.ValidationState);
         }
 
-        [Fact(Skip = "Validation incorrect for collections when using TryUpdateModel, #2941")]
         public async Task TryUpdateModel_ReadOnlyCollectionModel_WithPrefix_DoesNotGetBound()
         {
             // Arrange