Create model in `ComplexTypeModelBinder` if ANY property has a greedy binding source

- #7562 part 1

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.
+            // 2. Any of the model properties can be bound.
-            if (CanValueBindAnyModelProperties(bindingContext))
+            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,
+            // We want to check to see if any of the properties of the model can be bound using the value providers or
-            // because that's all that MutableObjectModelBinder can handle.
+            // a greedy binder.
             //
-            // However, because a property might specify a custom binding source ([FromForm]), it's not correct
+            // Because a property might specify a custom binding source ([FromForm]), it's not correct
-            // for us to just try bindingContext.ValueProvider.ContainsPrefixAsync(bindingContext.ModelName),
+            // 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
+            // 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
+            //      If a property has a binding source, and it's a greedy source, then it's always bound.
-            //      allowed to come from a value provider, so we skip it.
             //
             //      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
+            // 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. OR if ALL properties of the model have a greedy source,
+            // 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;
+                // If any property can be bound from a greedy binding source, then success.
-
-                // This check will skip properties which are marked explicitly using a non value binder.
                 var bindingSource = propertyMetadata.BindingSource;
-                if (bindingSource == null || !bindingSource.IsGreedy)
+                if (bindingSource != null && bindingSource.IsGreedy)
                 {
-                    isAnyPropertyEnabledForValueProviderBasedBinding = true;
+                    return true;
+                }
 
-                    var fieldName = propertyMetadata.BinderModelName ?? propertyMetadata.PropertyName;
+                // Otherwise, check whether the (perhaps filtered) value providers have a match.
-                    var modelName = ModelNames.CreatePropertyModelName(
+                var fieldName = propertyMetadata.BinderModelName ?? propertyMetadata.PropertyName;
-                        bindingContext.ModelName,
+                var modelName = ModelNames.CreatePropertyModelName(bindingContext.ModelName, fieldName);
-                        fieldName);
+                using (bindingContext.EnterNestedScope(
-
+                    modelMetadata: propertyMetadata,
-                    using (bindingContext.EnterNestedScope(
+                    fieldName: fieldName,
-                        modelMetadata: propertyMetadata,
+                    modelName: modelName,
-                        fieldName: fieldName,
+                    model: null))
-                        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.
+                        return true;
-                        if (bindingContext.ValueProvider.ContainsPrefix(bindingContext.ModelName))
-                        {
-                            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;

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<IValueProvider>();
+            valueProvider
+                .Setup(o => o.ContainsPrefix(It.IsAny<string>()))
+                .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), false, true)]
-        [InlineData(typeof(TypeWithUnmarkedAndBinderMetadataMarkedProperties), true)]
+        [InlineData(typeof(TypeWithUnmarkedAndBinderMetadataMarkedProperties), true, true)]
-        [InlineData(typeof(TypeWithNoBinderMetadata), false)]
+        [InlineData(typeof(TypeWithNoBinderMetadata), false, false)]
-        [InlineData(typeof(TypeWithNoBinderMetadata), true)]
+        [InlineData(typeof(TypeWithNoBinderMetadata), true, true)]
         public void CanCreateModel_UnmarkedProperties_UsesCurrentValueProvider(
             Type modelType,
-            bool valueProviderProvidesValue)
+            bool valueProviderProvidesValue,
+            bool expectedCanCreate)
         {
             var valueProvider = new Mock<IValueProvider>();
             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]

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<Order1>(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<Order1>(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<Order4>(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<Order4>(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