Fail ComplexTypeModelBinder after CanCreateModel(...) in some cases (#6793)

- #4802 and #6616
- also reduces the impact incorrect metadata as in #4939
- postpone some property binding in `ComplexTypeModelBinder`

Author: dougbu

src/Mvc/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/ComplexTypeModelBinder.cs

@@ -3,6 +3,7 @@
 
 using System;
 using System.Collections.Generic;
+using System.Diagnostics;
 using System.Linq.Expressions;
 using System.Reflection;
 using System.Threading.Tasks;
@@ -17,6 +18,19 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders
     /// </summary>
     public class ComplexTypeModelBinder : IModelBinder
     {
+        // Don't want a new public enum because communication between the private and internal methods of this class
+        // should not be exposed. Can't use an internal enum because types of [TheoryData] values must be public.
+
+        // Model contains only properties that are expected to bind from value providers and no value provider has
+        // matching data.
+        internal const int NoDataAvailable = 0;
+        // If model contains properties that are expected to bind from value providers, no value provider has matching
+        // data. Remaining (greedy) properties might bind successfully.
+        internal const int GreedyPropertiesMayHaveData = 1;
+        // Model contains at least one property that is expected to bind from value providers and a value provider has
+        // matching data.
+        internal const int ValueProviderDataAvailable = 2;
+
         private readonly IDictionary<ModelMetadata, IModelBinder> _propertyBinders;
         private readonly ILogger _logger;
         private Func<object> _modelCreator;
@@ -76,18 +90,21 @@ public Task BindModelAsync(ModelBindingContext bindingContext)
 
             _logger.AttemptingToBindModel(bindingContext);
 
-            if (!CanCreateModel(bindingContext))
+            var propertyData = CanCreateModel(bindingContext);
+            if (propertyData == NoDataAvailable)
             {
                 return Task.CompletedTask;
             }
 
             // Perf: separated to avoid allocating a state machine when we don't
             // need to go async.
-            return BindModelCoreAsync(bindingContext);
+            return BindModelCoreAsync(bindingContext, propertyData);
         }
 
-        private async Task BindModelCoreAsync(ModelBindingContext bindingContext)
+        private async Task BindModelCoreAsync(ModelBindingContext bindingContext, int propertyData)
         {
+            Debug.Assert(propertyData == GreedyPropertiesMayHaveData || propertyData == ValueProviderDataAvailable);
+
             // Create model first (if necessary) to avoid reporting errors about properties when activation fails.
             if (bindingContext.Model == null)
             {
@@ -96,6 +113,8 @@ private async Task BindModelCoreAsync(ModelBindingContext bindingContext)
 
             var modelMetadata = bindingContext.ModelMetadata;
             var attemptedPropertyBinding = false;
+            var propertyBindingSucceeded = false;
+            var postponePlaceholderBinding = false;
             for (var i = 0; i < modelMetadata.Properties.Count; i++)
             {
                 var property = modelMetadata.Properties[i];
@@ -104,42 +123,62 @@ private async Task BindModelCoreAsync(ModelBindingContext bindingContext)
                     continue;
                 }
 
-                // Pass complex (including collection) values down so that binding system does not unnecessarily
-                // recreate instances or overwrite inner properties that are not bound. No need for this with simple
-                // values because they will be overwritten if binding succeeds. Arrays are never reused because they
-                // cannot be resized.
-                object propertyModel = null;
-                if (property.PropertyGetter != null &&
-                    property.IsComplexType &&
-                    !property.ModelType.IsArray)
+                if (_propertyBinders[property] is PlaceholderBinder)
                 {
-                    propertyModel = property.PropertyGetter(bindingContext.Model);
+                    if (postponePlaceholderBinding)
+                    {
+                        // Decided to postpone binding properties that complete a loop in the model types when handling
+                        // an earlier loop-completing property. Postpone binding this property too.
+                        continue;
+                    }
+                    else if (!bindingContext.IsTopLevelObject &&
+                        !propertyBindingSucceeded &&
+                        propertyData == GreedyPropertiesMayHaveData)
+                    {
+                        // Have no confirmation of data for the current instance. Postpone completing the loop until
+                        // we _know_ the current instance is useful. Recursion would otherwise occur prior to the
+                        // block with a similar condition after the loop.
+                        //
+                        // Example cases include an Employee class containing
+                        // 1. a Manager property of type Employee
+                        // 2. an Employees property of type IList<Employee>
+                        postponePlaceholderBinding = true;
+                        continue;
+                    }
                 }
 
                 var fieldName = property.BinderModelName ?? property.PropertyName;
                 var modelName = ModelNames.CreatePropertyModelName(bindingContext.ModelName, fieldName);
-
-                ModelBindingResult result;
-                using (bindingContext.EnterNestedScope(
-                    modelMetadata: property,
-                    fieldName: fieldName,
-                    modelName: modelName,
-                    model: propertyModel))
-                {
-                    await BindProperty(bindingContext);
-                    result = bindingContext.Result;
-                }
+                var result = await BindProperty(bindingContext, property, fieldName, modelName);
 
                 if (result.IsModelSet)
                 {
                     attemptedPropertyBinding = true;
-                    SetProperty(bindingContext, modelName, property, result);
+                    propertyBindingSucceeded = true;
                 }
                 else if (property.IsBindingRequired)
                 {
                     attemptedPropertyBinding = true;
-                    var message = property.ModelBindingMessageProvider.MissingBindRequiredValueAccessor(fieldName);
-                    bindingContext.ModelState.TryAddModelError(modelName, message);
+                }
+            }
+
+            if (postponePlaceholderBinding && propertyBindingSucceeded)
+            {
+                // Have some data for this instance. Continue with the model type loop.
+                for (var i = 0; i < modelMetadata.Properties.Count; i++)
+                {
+                    var property = modelMetadata.Properties[i];
+                    if (!CanBindProperty(bindingContext, property))
+                    {
+                        continue;
+                    }
+
+                    if (_propertyBinders[property] is PlaceholderBinder)
+                    {
+                        var fieldName = property.BinderModelName ?? property.PropertyName;
+                        var modelName = ModelNames.CreatePropertyModelName(bindingContext.ModelName, fieldName);
+                        await BindProperty(bindingContext, property, fieldName, modelName);
+                    }
                 }
             }
 
@@ -157,8 +196,37 @@ private async Task BindModelCoreAsync(ModelBindingContext bindingContext)
                 bindingContext.ModelState.TryAddModelError(bindingContext.ModelName, message);
             }
 
-            bindingContext.Result = ModelBindingResult.Success(bindingContext.Model);
             _logger.DoneAttemptingToBindModel(bindingContext);
+
+            // Have all binders failed because no data was available?
+            //
+            // If CanCreateModel determined a property has data, failures are likely due to conversion errors. For
+            // example, user may submit ?[0].id=twenty&[1].id=twenty-one&[2].id=22 for a collection of a complex type
+            // with an int id property. In that case, the bound model should be [ {}, {}, { id = 22 }] and
+            // ModelState should contain errors about both [0].id and [1].id. Do not inform higher-level binders of the
+            // failure in this and similar cases.
+            //
+            // If CanCreateModel could not find data for non-greedy properties, failures indicate greedy binders were
+            // unsuccessful. For example, user may submit file attachments [0].File and [1].File but not [2].File for
+            // a collection of a complex type containing an IFormFile property. In that case, we have exhausted the
+            // attached files and checking for [3].File is likely be pointless. (And, if it had a point, would we stop
+            // after 10 failures, 100, or more -- all adding redundant errors to ModelState?) Inform higher-level
+            // binders of the failure.
+            //
+            // Required properties do not change the logic below. Missed required properties cause ModelState errors
+            // but do not necessarily prevent further attempts to bind.
+            //
+            // This logic is intended to maximize correctness but does not avoid infinite loops or recursion when a
+            // greedy model binder succeeds unconditionally.
+            if (!bindingContext.IsTopLevelObject &&
+                !propertyBindingSucceeded &&
+                propertyData == GreedyPropertiesMayHaveData)
+            {
+                bindingContext.Result = ModelBindingResult.Failed();
+                return;
+            }
+
+            bindingContext.Result = ModelBindingResult.Success(bindingContext.Model);
         }
 
         /// <summary>
@@ -194,6 +262,48 @@ protected virtual bool CanBindProperty(ModelBindingContext bindingContext, Model
             return true;
         }
 
+        private async Task<ModelBindingResult> BindProperty(
+            ModelBindingContext bindingContext,
+            ModelMetadata property,
+            string fieldName,
+            string modelName)
+        {
+            // Pass complex (including collection) values down so that binding system does not unnecessarily
+            // recreate instances or overwrite inner properties that are not bound. No need for this with simple
+            // values because they will be overwritten if binding succeeds. Arrays are never reused because they
+            // cannot be resized.
+            object propertyModel = null;
+            if (property.PropertyGetter != null &&
+                property.IsComplexType &&
+                !property.ModelType.IsArray)
+            {
+                propertyModel = property.PropertyGetter(bindingContext.Model);
+            }
+
+            ModelBindingResult result;
+            using (bindingContext.EnterNestedScope(
+                modelMetadata: property,
+                fieldName: fieldName,
+                modelName: modelName,
+                model: propertyModel))
+            {
+                await BindProperty(bindingContext);
+                result = bindingContext.Result;
+            }
+
+            if (result.IsModelSet)
+            {
+                SetProperty(bindingContext, modelName, property, result);
+            }
+            else if (property.IsBindingRequired)
+            {
+                var message = property.ModelBindingMessageProvider.MissingBindRequiredValueAccessor(fieldName);
+                bindingContext.ModelState.TryAddModelError(modelName, message);
+            }
+
+            return result;
+        }
+
         /// <summary>
         /// Attempts to bind a property of the model.
         /// </summary>
@@ -208,7 +318,7 @@ protected virtual Task BindProperty(ModelBindingContext bindingContext)
             return binder.BindModelAsync(bindingContext);
         }
 
-        internal bool CanCreateModel(ModelBindingContext bindingContext)
+        internal int CanCreateModel(ModelBindingContext bindingContext)
         {
             var isTopLevelObject = bindingContext.IsTopLevelObject;
 
@@ -227,33 +337,28 @@ internal bool CanCreateModel(ModelBindingContext bindingContext)
             var bindingSource = bindingContext.BindingSource;
             if (!isTopLevelObject && bindingSource != null && bindingSource.IsGreedy)
             {
-                return false;
+                return NoDataAvailable;
             }
 
             // Create the object if:
             // 1. It is a top level model.
             if (isTopLevelObject)
             {
-                return true;
+                return ValueProviderDataAvailable;
             }
 
             // 2. Any of the model properties can be bound.
-            if (CanBindAnyModelProperties(bindingContext))
-            {
-                return true;
-            }
-
-            return false;
+            return CanBindAnyModelProperties(bindingContext);
         }
 
-        private bool CanBindAnyModelProperties(ModelBindingContext bindingContext)
+        private int 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.
             if (bindingContext.ModelMetadata.Properties.Count == 0)
             {
                 _logger.NoPublicSettableProperties(bindingContext);
-                return false;
+                return NoDataAvailable;
             }
 
             // We want to check to see if any of the properties of the model can be bound using the value providers or
@@ -279,7 +384,7 @@ private bool CanBindAnyModelProperties(ModelBindingContext bindingContext)
             // 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 hasGreedyBinders = false;
             for (var i = 0; i < bindingContext.ModelMetadata.Properties.Count; i++)
             {
                 var propertyMetadata = bindingContext.ModelMetadata.Properties[i];
@@ -292,7 +397,8 @@ private bool CanBindAnyModelProperties(ModelBindingContext bindingContext)
                 var bindingSource = propertyMetadata.BindingSource;
                 if (bindingSource != null && bindingSource.IsGreedy)
                 {
-                    return true;
+                    hasGreedyBinders = true;
+                    continue;
                 }
 
                 // Otherwise, check whether the (perhaps filtered) value providers have a match.
@@ -307,14 +413,19 @@ private bool CanBindAnyModelProperties(ModelBindingContext bindingContext)
                     // If any property can be bound from a value provider, then success.
                     if (bindingContext.ValueProvider.ContainsPrefix(bindingContext.ModelName))
                     {
-                        return true;
+                        return ValueProviderDataAvailable;
                     }
                 }
             }
 
+            if (hasGreedyBinders)
+            {
+                return GreedyPropertiesMayHaveData;
+            }
+
             _logger.CannotBindToComplexType(bindingContext);
 
-            return false;
+            return NoDataAvailable;
         }
 
         // Internal for tests