From c13e2498a8061e35b23f81cab6662f80b879b591 Mon Sep 17 00:00:00 2001 From: Doug Bunting Date: Sun, 16 Sep 2018 22:39:20 -0700 Subject: [PATCH] Create model in `ComplexTypeModelBinder` if ANY property has a greedy binding source - #7562 part 1 --- .../Binders/ComplexTypeModelBinder.cs | 68 ++++++++----------- .../Binders/ComplexTypeModelBinderTest.cs | 47 ++++++++++--- .../ComplexTypeModelBinderIntegrationTest.cs | 56 +++++++++------ 3 files changed, 97 insertions(+), 74 deletions(-) diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/ComplexTypeModelBinder.cs b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/ComplexTypeModelBinder.cs index 3f15ffdf20..2ab1cdb70c 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/ComplexTypeModelBinder.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/ComplexTypeModelBinder.cs @@ -216,8 +216,8 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders return true; } - // 2. Any of the model properties can be bound using a value provider. - if (CanValueBindAnyModelProperties(bindingContext)) + // 2. Any of the model properties can be bound. + if (CanBindAnyModelProperties(bindingContext)) { return true; } @@ -225,7 +225,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders return false; } - private bool CanValueBindAnyModelProperties(ModelBindingContext bindingContext) + private bool CanBindAnyModelProperties(ModelBindingContext bindingContext) { // If there are no properties on the model, there is nothing to bind. We are here means this is not a top // level object. So we return false. @@ -235,20 +235,19 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders return false; } - // We want to check to see if any of the properties of the model can be bound using the value providers, - // because that's all that MutableObjectModelBinder can handle. + // We want to check to see if any of the properties of the model can be bound using the value providers or + // a greedy binder. // - // However, because a property might specify a custom binding source ([FromForm]), it's not correct - // for us to just try bindingContext.ValueProvider.ContainsPrefixAsync(bindingContext.ModelName), - // because that may include other value providers - that would lead us to mistakenly create the model + // Because a property might specify a custom binding source ([FromForm]), it's not correct + // for us to just try bindingContext.ValueProvider.ContainsPrefixAsync(bindingContext.ModelName); + // that may include other value providers - that would lead us to mistakenly create the model // when the data is coming from a source we should use (ex: value found in query string, but the // model has [FromForm]). // // To do this we need to enumerate the properties, and see which of them provide a binding source // through metadata, then we decide what to do. // - // If a property has a binding source, and it's a greedy source, then it's not - // allowed to come from a value provider, so we skip it. + // If a property has a binding source, and it's a greedy source, then it's always bound. // // If a property has a binding source, and it's a non-greedy source, then we'll filter the // the value providers to just that source, and see if we can find a matching prefix @@ -256,12 +255,10 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders // // If a property does not have a binding source, then it's fair game for any value provider. // - // If any property meets the above conditions and has a value from ValueProviders, then we'll - // create the model and try to bind it. OR if ALL properties of the model have a greedy source, + // Bottom line, if any property meets the above conditions and has a value from ValueProviders, then we'll + // create the model and try to bind it. Of, if ANY properties of the model have a greedy source, // then we go ahead and create it. // - var hasBindableProperty = false; - var isAnyPropertyEnabledForValueProviderBasedBinding = false; for (var i = 0; i < bindingContext.ModelMetadata.Properties.Count; i++) { var propertyMetadata = bindingContext.ModelMetadata.Properties[i]; @@ -270,41 +267,30 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders continue; } - hasBindableProperty = true; - - // This check will skip properties which are marked explicitly using a non value binder. + // If any property can be bound from a greedy binding source, then success. var bindingSource = propertyMetadata.BindingSource; - if (bindingSource == null || !bindingSource.IsGreedy) + if (bindingSource != null && bindingSource.IsGreedy) { - isAnyPropertyEnabledForValueProviderBasedBinding = true; + return true; + } - var fieldName = propertyMetadata.BinderModelName ?? propertyMetadata.PropertyName; - var modelName = ModelNames.CreatePropertyModelName( - bindingContext.ModelName, - fieldName); - - using (bindingContext.EnterNestedScope( - modelMetadata: propertyMetadata, - fieldName: fieldName, - modelName: modelName, - model: null)) + // Otherwise, check whether the (perhaps filtered) value providers have a match. + var fieldName = propertyMetadata.BinderModelName ?? propertyMetadata.PropertyName; + var modelName = ModelNames.CreatePropertyModelName(bindingContext.ModelName, fieldName); + using (bindingContext.EnterNestedScope( + modelMetadata: propertyMetadata, + fieldName: fieldName, + modelName: modelName, + model: null)) + { + // If any property can be bound from a value provider, then success. + if (bindingContext.ValueProvider.ContainsPrefix(bindingContext.ModelName)) { - // If any property can be bound from a value provider then continue. - if (bindingContext.ValueProvider.ContainsPrefix(bindingContext.ModelName)) - { - return true; - } + return true; } } } - if (hasBindableProperty && !isAnyPropertyEnabledForValueProviderBasedBinding) - { - // All the properties are marked with a non value provider based marker like [FromHeader] or - // [FromBody]. - return true; - } - _logger.CannotBindToComplexType(bindingContext); return false; diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/ComplexTypeModelBinderTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/ComplexTypeModelBinderTest.cs index c76eb27c0a..7eaedfa106 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/ComplexTypeModelBinderTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/ComplexTypeModelBinderTest.cs @@ -155,10 +155,6 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders [Theory] [InlineData(typeof(TypeWithNoBinderMetadata), false)] [InlineData(typeof(TypeWithNoBinderMetadata), true)] - [InlineData(typeof(TypeWithAtLeastOnePropertyMarkedUsingValueBinderMetadata), false)] - [InlineData(typeof(TypeWithAtLeastOnePropertyMarkedUsingValueBinderMetadata), true)] - [InlineData(typeof(TypeWithUnmarkedAndBinderMetadataMarkedProperties), false)] - [InlineData(typeof(TypeWithUnmarkedAndBinderMetadataMarkedProperties), true)] public void CanCreateModel_CreatesModelForValueProviderBasedBinderMetadatas_IfAValueProviderProvidesValue( Type modelType, bool valueProviderProvidesValue) @@ -182,6 +178,34 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders Assert.Equal(valueProviderProvidesValue, canCreate); } + [Theory] + [InlineData(typeof(TypeWithAtLeastOnePropertyMarkedUsingValueBinderMetadata), false)] + [InlineData(typeof(TypeWithAtLeastOnePropertyMarkedUsingValueBinderMetadata), true)] + [InlineData(typeof(TypeWithUnmarkedAndBinderMetadataMarkedProperties), false)] + [InlineData(typeof(TypeWithUnmarkedAndBinderMetadataMarkedProperties), true)] + public void CanCreateModel_CreatesModelForValueProviderBasedBinderMetadatas_IfPropertyHasGreedyBindingSource( + Type modelType, + bool valueProviderProvidesValue) + { + var valueProvider = new Mock(); + valueProvider + .Setup(o => o.ContainsPrefix(It.IsAny())) + .Returns(valueProviderProvidesValue); + + var bindingContext = CreateContext(GetMetadataForType(modelType)); + bindingContext.IsTopLevelObject = false; + bindingContext.ValueProvider = valueProvider.Object; + bindingContext.OriginalValueProvider = valueProvider.Object; + + var binder = CreateBinder(bindingContext.ModelMetadata); + + // Act + var canCreate = binder.CanCreateModel(bindingContext); + + // Assert + Assert.True(canCreate); + } + [Theory] [InlineData(typeof(TypeWithAtLeastOnePropertyMarkedUsingValueBinderMetadata), false)] [InlineData(typeof(TypeWithAtLeastOnePropertyMarkedUsingValueBinderMetadata), true)] @@ -214,17 +238,18 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders var canCreate = binder.CanCreateModel(bindingContext); // Assert - Assert.Equal(originalValueProviderProvidesValue, canCreate); + Assert.True(canCreate); } [Theory] - [InlineData(typeof(TypeWithUnmarkedAndBinderMetadataMarkedProperties), false)] - [InlineData(typeof(TypeWithUnmarkedAndBinderMetadataMarkedProperties), true)] - [InlineData(typeof(TypeWithNoBinderMetadata), false)] - [InlineData(typeof(TypeWithNoBinderMetadata), true)] + [InlineData(typeof(TypeWithUnmarkedAndBinderMetadataMarkedProperties), false, true)] + [InlineData(typeof(TypeWithUnmarkedAndBinderMetadataMarkedProperties), true, true)] + [InlineData(typeof(TypeWithNoBinderMetadata), false, false)] + [InlineData(typeof(TypeWithNoBinderMetadata), true, true)] public void CanCreateModel_UnmarkedProperties_UsesCurrentValueProvider( Type modelType, - bool valueProviderProvidesValue) + bool valueProviderProvidesValue, + bool expectedCanCreate) { var valueProvider = new Mock(); valueProvider @@ -247,7 +272,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders var canCreate = binder.CanCreateModel(bindingContext); // Assert - Assert.Equal(valueProviderProvidesValue, canCreate); + Assert.Equal(expectedCanCreate, canCreate); } [Fact] diff --git a/test/Microsoft.AspNetCore.Mvc.IntegrationTests/ComplexTypeModelBinderIntegrationTest.cs b/test/Microsoft.AspNetCore.Mvc.IntegrationTests/ComplexTypeModelBinderIntegrationTest.cs index 9eb3af091e..8f4fc6470f 100644 --- a/test/Microsoft.AspNetCore.Mvc.IntegrationTests/ComplexTypeModelBinderIntegrationTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.IntegrationTests/ComplexTypeModelBinderIntegrationTest.cs @@ -16,7 +16,7 @@ using Xunit; namespace Microsoft.AspNetCore.Mvc.IntegrationTests { - // Integration tests targeting the behavior of the MutableObjectModelBinder and related classes + // Integration tests targeting the behavior of the ComplexTypeModelBinder and related classes // with other model binders. public class ComplexTypeModelBinderIntegrationTest { @@ -197,10 +197,8 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests Assert.Equal("bill", entry.RawValue); } - // We don't provide enough data in this test for the 'Person' model to be created. So even though there is - // body data in the request, it won't be used. [Fact] - public async Task MutableObjectModelBinder_BindsNestedPOCO_WithBodyModelBinder_WithPrefix_PartialData() + public async Task ComplexTypeModelBinder_BindsNestedPOCO_WithBodyModelBinder_WithPrefix_PartialData() { // Arrange var parameter = new ParameterDescriptor() @@ -235,22 +233,21 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests Assert.True(modelBindingResult.IsModelSet); var model = Assert.IsType(modelBindingResult.Model); - Assert.Null(model.Customer); + Assert.NotNull(model.Customer); + Assert.Equal("1 Microsoft Way", model.Customer.Address.Street); + Assert.Equal(10, model.ProductId); - Assert.Single(modelState); Assert.Equal(0, modelState.ErrorCount); Assert.True(modelState.IsValid); - var entry = Assert.Single(modelState, e => e.Key == "parameter.ProductId").Value; + var entry = Assert.Single(modelState).Value; Assert.Equal("10", entry.AttemptedValue); Assert.Equal("10", entry.RawValue); } - // We don't provide enough data in this test for the 'Person' model to be created. So even though there is - // body data in the request, it won't be used. [Fact] - public async Task MutableObjectModelBinder_BindsNestedPOCO_WithBodyModelBinder_WithPrefix_NoData() + public async Task ComplexTypeModelBinder_BindsNestedPOCO_WithBodyModelBinder_WithPrefix_NoData() { // Arrange var parameter = new ParameterDescriptor() @@ -285,7 +282,8 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests Assert.True(modelBindingResult.IsModelSet); var model = Assert.IsType(modelBindingResult.Model); - Assert.Null(model.Customer); + Assert.NotNull(model.Customer); + Assert.Equal("1 Microsoft Way", model.Customer.Address.Street); Assert.Empty(modelState); Assert.Equal(0, modelState.ErrorCount); @@ -630,10 +628,8 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests Assert.Equal("bill", entry.RawValue); } - // We don't provide enough data in this test for the 'Person' model to be created. So even though there are - // form files in the request, it won't be used. [Fact] - public async Task MutableObjectModelBinder_BindsNestedPOCO_WithFormFileModelBinder_WithPrefix_PartialData() + public async Task ComplexTypeModelBinder_BindsNestedPOCO_WithFormFileModelBinder_WithPrefix_PartialData() { // Arrange var parameter = new ParameterDescriptor() @@ -668,22 +664,29 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests Assert.True(modelBindingResult.IsModelSet); var model = Assert.IsType(modelBindingResult.Model); - Assert.Null(model.Customer); + Assert.NotNull(model.Customer); + + var document = Assert.Single(model.Customer.Documents); + Assert.Equal("text.txt", document.FileName); + using (var reader = new StreamReader(document.OpenReadStream())) + { + Assert.Equal("Hello, World!", await reader.ReadToEndAsync()); + } + Assert.Equal(10, model.ProductId); - Assert.Single(modelState); + Assert.Equal(2, modelState.Count); Assert.Equal(0, modelState.ErrorCount); Assert.True(modelState.IsValid); + Assert.Single(modelState, e => e.Key == "parameter.Customer.Documents"); var entry = Assert.Single(modelState, e => e.Key == "parameter.ProductId").Value; Assert.Equal("10", entry.AttemptedValue); Assert.Equal("10", entry.RawValue); } - // We don't provide enough data in this test for the 'Person' model to be created. So even though there is - // body data in the request, it won't be used. [Fact] - public async Task MutableObjectModelBinder_BindsNestedPOCO_WithFormFileModelBinder_WithPrefix_NoData() + public async Task ComplexTypeModelBinder_BindsNestedPOCO_WithFormFileModelBinder_WithPrefix_NoData() { // Arrange var parameter = new ParameterDescriptor() @@ -696,7 +699,7 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests var testContext = ModelBindingTestHelper.GetTestContext(request => { request.QueryString = new QueryString("?"); - SetFormFileBodyContent(request, "Hello, World!", "parameter.Customer.Documents"); + SetFormFileBodyContent(request, "Hello, World!", "Customer.Documents"); }); var modelState = testContext.ModelState; @@ -718,11 +721,20 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests Assert.True(modelBindingResult.IsModelSet); var model = Assert.IsType(modelBindingResult.Model); - Assert.Null(model.Customer); + Assert.NotNull(model.Customer); + + var document = Assert.Single(model.Customer.Documents); + Assert.Equal("text.txt", document.FileName); + using (var reader = new StreamReader(document.OpenReadStream())) + { + Assert.Equal("Hello, World!", await reader.ReadToEndAsync()); + } - Assert.Empty(modelState); Assert.Equal(0, modelState.ErrorCount); Assert.True(modelState.IsValid); + + var entry = Assert.Single(modelState); + Assert.Equal("Customer.Documents", entry.Key); } private class Order5