diff --git a/src/Microsoft.AspNet.Mvc.Core/ModelBindingHelper.cs b/src/Microsoft.AspNet.Mvc.Core/ModelBindingHelper.cs index 804de334f7..c80ec1e80f 100644 --- a/src/Microsoft.AspNet.Mvc.Core/ModelBindingHelper.cs +++ b/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, - new ModelValidationNode(prefix, modelBindingContext.ModelMetadata, modelBindingResult.Model) - { - ValidateAllProperties = true - }); + + var validationNode = modelBindingResult.ValidationNode; + Debug.Assert( + validationNode != null, + "ValidationNode should never be null in a successful ModelBindingResult."); + + objectModelValidator.Validate(modelValidationContext, validationNode); + return modelState.IsValid; } diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/ModelBindingHelperTest.cs b/test/Microsoft.AspNet.Mvc.Core.Test/ModelBindingHelperTest.cs index 044e8adbfa..fa9d8e0bc9 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/ModelBindingHelperTest.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/ModelBindingHelperTest.cs @@ -17,15 +17,29 @@ namespace Microsoft.AspNet.Mvc.Test { public class ModelBindingHelperTest { - [Fact] - public async Task TryUpdateModel_ReturnsFalse_IfBinderReturnsNull() + public static TheoryData UnsuccessfulModelBindingData + { + get + { + return new TheoryData + { + 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(); - binder.Setup(b => b.BindModelAsync(It.IsAny())) - .Returns(Task.FromResult(null)); + binder + .Setup(b => b.BindModelAsync(It.IsAny())) + .Returns(Task.FromResult(binderResult)); var model = new MyModel(); // Act @@ -126,18 +140,20 @@ namespace Microsoft.AspNet.Mvc.Test Assert.Equal("MyPropertyValue", model.MyProperty); } - [Fact] - public async Task TryUpdateModel_UsingIncludePredicateOverload_ReturnsFalse_IfBinderReturnsNull() + [Theory] + [MemberData(nameof(UnsuccessfulModelBindingData))] + public async Task TryUpdateModel_UsingIncludePredicateOverload_ReturnsFalse_IfBinderIsUnsuccessful( + ModelBindingResult binderResult) { // Arrange var metadataProvider = new EmptyModelMetadataProvider(); - var binder = new Mock(); - binder.Setup(b => b.BindModelAsync(It.IsAny())) - .Returns(Task.FromResult(null)); + binder + .Setup(b => b.BindModelAsync(It.IsAny())) + .Returns(Task.FromResult(binderResult)); var model = new MyModel(); - Func includePredicate = - (context, propertyName) => true; + Func includePredicate = (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] - public async Task TryUpdateModel_UsingIncludeExpressionOverload_ReturnsFalse_IfBinderReturnsNull() + [Theory] + [MemberData(nameof(UnsuccessfulModelBindingData))] + public async Task TryUpdateModel_UsingIncludeExpressionOverload_ReturnsFalse_IfBinderIsUnsuccessful( + ModelBindingResult binderResult) { // Arrange var metadataProvider = new EmptyModelMetadataProvider(); - var binder = new Mock(); - binder.Setup(b => b.BindModelAsync(It.IsAny())) - .Returns(Task.FromResult(null)); + binder + .Setup(b => b.BindModelAsync(It.IsAny())) + .Returns(Task.FromResult(binderResult)); var model = new MyModel(); // Act @@ -468,17 +486,20 @@ namespace Microsoft.AspNet.Mvc.Test ex.Message); } - [Fact] - public async Task TryUpdateModelNonGeneric_PredicateOverload_ReturnsFalse_IfBinderReturnsNull() + [Theory] + [MemberData(nameof(UnsuccessfulModelBindingData))] + public async Task TryUpdateModelNonGeneric_PredicateOverload_ReturnsFalse_IfBinderIsUnsuccessful( + ModelBindingResult binderResult) { // Arrange var metadataProvider = new EmptyModelMetadataProvider(); var binder = new Mock(); - binder.Setup(b => b.BindModelAsync(It.IsAny())) - .Returns(Task.FromResult(null)); + binder + .Setup(b => b.BindModelAsync(It.IsAny())) + .Returns(Task.FromResult(binderResult)); var model = new MyModel(); - Func includePredicate = - (context, propertyName) => true; + Func includePredicate = (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] - public async Task TryUpdateModelNonGeneric_ModelTypeOverload_ReturnsFalse_IfBinderReturnsNull() + [Theory] + [MemberData(nameof(UnsuccessfulModelBindingData))] + public async Task TryUpdateModelNonGeneric_ModelTypeOverload_ReturnsFalse_IfBinderIsUnsuccessful( + ModelBindingResult binderResult) { // Arrange var metadataProvider = new EmptyModelMetadataProvider(); - var binder = new Mock(); - binder.Setup(b => b.BindModelAsync(It.IsAny())) - .Returns(Task.FromResult(null)); + binder + .Setup(b => b.BindModelAsync(It.IsAny())) + .Returns(Task.FromResult(binderResult)); var model = new MyModel(); // Act diff --git a/test/Microsoft.AspNet.Mvc.IntegrationTests/TryUpdateModelIntegrationTest.cs b/test/Microsoft.AspNet.Mvc.IntegrationTests/TryUpdateModelIntegrationTest.cs index d7c97b7853..ff79f16f7e 100644 --- a/test/Microsoft.AspNet.Mvc.IntegrationTests/TryUpdateModelIntegrationTest.cs +++ b/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 + { + { "[0].Name", "One Name" }, + { "[1].Address.Street", "Two Street" }, + }); + }); + + var modelState = new ModelStateDictionary(); + var model = new List + { + 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 { 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