Fix for #1538 and #1891

Changes here are all focused around MaxModelErrors on
ModelStateDictionary.

MaxAllowedErrors now defaults to 200 (same as options). This means that
constructing a new ModelStateDictionary with the default constructor will
use this default. There's a new constructor for creating a
MaxAllowedErrors with a non-default value.

The ControllerActionArgumentBinder is now responsible for setting the
value from options onto ActionContext.ModelState. This results in better
layering and guarantees the option is respected if someone uses
extensibility to call model binding.

ModelStateDictionary.CanAddErrors is renamed to MaxErrorsReached. We
wanted to change the behavior of this property, but realized that it's
very useful inside the model validation code, so opted to renamed.

There's also a bunch of doc cleanup inside ModelStateDictionary to
simplify things and improve clarity.

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

@@ -6,6 +6,7 @@ using System.Collections.Generic;
 using System.Threading.Tasks;
 using Microsoft.AspNet.Mvc.Core;
 using Microsoft.AspNet.Mvc.ModelBinding;
+using Microsoft.Framework.OptionsModel;
 
 namespace Microsoft.AspNet.Mvc
 {
@@ -16,10 +17,14 @@ namespace Microsoft.AspNet.Mvc
     public class DefaultControllerActionArgumentBinder : IControllerActionArgumentBinder
     {
         private readonly IModelMetadataProvider _modelMetadataProvider;
+        private readonly MvcOptions _options;
 
-        public DefaultControllerActionArgumentBinder(IModelMetadataProvider modelMetadataProvider)
+        public DefaultControllerActionArgumentBinder(
+            IModelMetadataProvider modelMetadataProvider,
+            IOptions<MvcOptions> optionsAccessor)
         {
             _modelMetadataProvider = modelMetadataProvider;
+            _options = optionsAccessor.Options;
         }
 
         public async Task<IDictionary<string, object>> GetActionArgumentsAsync(
@@ -84,10 +89,13 @@ namespace Microsoft.AspNet.Mvc
                 ValueProvider = bindingContext.ValueProvider,
             };
 
+            var modelState = actionContext.ModelState;
+            modelState.MaxAllowedErrors = _options.MaxModelValidationErrors;
+
             foreach (var parameter in parameterMetadata)
             {
                 var parameterType = parameter.ModelType;
-                var modelBindingContext = GetModelBindingContext(parameter, actionContext, operationBindingContext);
+                var modelBindingContext = GetModelBindingContext(parameter, modelState, operationBindingContext);
                 if (await bindingContext.ModelBinder.BindModelAsync(modelBindingContext) &&
                     modelBindingContext.IsModelSet)
                 {
@@ -96,16 +104,17 @@ namespace Microsoft.AspNet.Mvc
             }
         }
 
+        // Internal for tests
         internal static ModelBindingContext GetModelBindingContext(
             ModelMetadata modelMetadata,
-            ActionContext actionContext,
+            ModelStateDictionary modelState,
             OperationBindingContext operationBindingContext)
         {
             var modelBindingContext = new ModelBindingContext
             {
                 ModelName = modelMetadata.BinderModelName ?? modelMetadata.PropertyName,
                 ModelMetadata = modelMetadata,
-                ModelState = actionContext.ModelState,
+                ModelState = modelState,
 
                 // Fallback only if there is no explicit model name set.
                 FallbackToEmptyPrefix = modelMetadata.BinderModelName == null,

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

@@ -6,6 +6,7 @@ using System.Collections.Generic;
 using Microsoft.AspNet.Mvc.ApplicationModels;
 using Microsoft.AspNet.Mvc.Core;
 using Microsoft.AspNet.Mvc.OptionDescriptors;
+using Microsoft.AspNet.Mvc.ModelBinding;
 
 namespace Microsoft.AspNet.Mvc
 {
@@ -15,7 +16,7 @@ namespace Microsoft.AspNet.Mvc
     public class MvcOptions
     {
         private AntiForgeryOptions _antiForgeryOptions = new AntiForgeryOptions();
-        private int _maxModelStateErrors = 200;
+        private int _maxModelStateErrors = ModelStateDictionary.DefaultMaxAllowedErrors;
 
         public MvcOptions()
         {

src/Microsoft.AspNet.Mvc.ModelBinding/ModelStateDictionary.cs

@@ -15,13 +15,30 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
     /// </summary>
     public class ModelStateDictionary : IDictionary<string, ModelState>
     {
+        // Make sure to update the doc headers if this value is changed.
+        /// <summary>
+        /// The default value for <see cref="MaxAllowedErrors"/> of <c>200</c>.
+        /// </summary>
+        public static readonly int DefaultMaxAllowedErrors = 200;
+
         private readonly IDictionary<string, ModelState> _innerDictionary;
+        private int _maxAllowedErrors;
 
         /// <summary>
         /// Initializes a new instance of the <see cref="ModelStateDictionary"/> class.
         /// </summary>
         public ModelStateDictionary()
+            : this(DefaultMaxAllowedErrors)
         {
+        }
+
+        /// <summary>
+        /// Initializes a new instance of the <see cref="ModelStateDictionary"/> class.
+        /// </summary>
+        public ModelStateDictionary(int maxAllowedErrors)
+        {
+            MaxAllowedErrors = maxAllowedErrors;
+
             _innerDictionary = new Dictionary<string, ModelState>(StringComparer.OrdinalIgnoreCase);
         }
 
@@ -41,29 +58,53 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
         }
 
         /// <summary>
-        /// Gets or sets the maximum allowed errors in this instance of <see cref="ModelStateDictionary"/>.
+        /// Gets or sets the maximum allowed model state errors in this instance of <see cref="ModelStateDictionary"/>.
-        /// Defaults to <see cref="int.MaxValue"/>.
+        /// Defaults to <c>200</c>.
         /// </summary>
         /// <remarks>
-        /// The value of this property is used to track the total number of calls to
+        /// <para>
-        /// <see cref="AddModelError(string, Exception)"/> and <see cref="AddModelError(string, string)"/> after which
+        /// <see cref="ModelStateDictionary"/> tracks the number of model errors added by calls to
-        /// an error is thrown for further invocations. Errors added via modifying <see cref="ModelState"/> do not
+        /// <see cref="AddModelError(string, Exception)"/> or <see cref="TryAddModelError(string, Exception)"/>.
-        /// count towards this limit.
+        /// Once the value of <code>MaxAllowedErrors - 1</code> is reached, if another attempt is made to add an error,
+        /// the error message will be ignored and a <see cref="TooManyModelErrorsException"/> will be added.
+        /// </para>
+        /// <para>
+        /// Errors added via modifying <see cref="ModelState"/> directly do not count towards this limit.
+        /// </para>
         /// </remarks>
-        public int MaxAllowedErrors { get; set; } = int.MaxValue;
+        public int MaxAllowedErrors
+        {
+            get
+            {
+                return _maxAllowedErrors;
+            }
+            set
+            {
+                if (value < 0)
+                {
+                    throw new ArgumentOutOfRangeException(nameof(value));
+                }
+
+                _maxAllowedErrors = value;
+            }
+        }
 
         /// <summary>
-        /// Gets a flag that determines if the total number of added errors (given by <see cref="ErrorCount"/>) is
+        /// Gets a value indicating whether or not the maximum number of errors have been
-        /// fewer than <see cref="MaxAllowedErrors"/>.
+        /// recorded.
         /// </summary>
-        public bool CanAddErrors
+        /// <remarks>
+        /// Returns <c>true</c> if a <see cref="TooManyModelErrorsException"/> has been recorded;
+        /// otherwise <c>false</c>.
+        /// </remarks>
+        public bool HasReachedMaxErrors
         {
-            get { return ErrorCount < MaxAllowedErrors; }
+            get { return ErrorCount >= MaxAllowedErrors; }
         }
 
         /// <summary>
         /// Gets the number of errors added to this instance of <see cref="ModelStateDictionary"/> via
-        /// <see cref="AddModelError(string, Exception)"/> and <see cref="AddModelError(string, string)"/>.
+        /// <see cref="AddModelError"/> or <see cref="TryAddModelError"/>.
         /// </summary>
         public int ErrorCount { get; private set; }
 
@@ -149,12 +190,10 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
         /// </summary>
         /// <param name="key">The key of the <see cref="ModelState"/> to add errors to.</param>
         /// <param name="exception">The <see cref="Exception"/> to add.</param>
-        /// <returns>True if the error was added, false if the dictionary has already recorded
+        /// <returns>
-        /// at least <see cref="MaxAllowedErrors"/> number of errors.</returns>
+        /// <c>True</c> if the given error was added, <c>false</c> if the error was ignored.
-        /// <remarks>
+        /// See <see cref="MaxAllowedErrors"/>.
-        /// This method only allows adding up to <see cref="MaxAllowedErrors"/> - 1. <see cref="MaxAllowedErrors"/>nt
+        /// </returns>
-        /// invocation would result in adding a <see cref="TooManyModelErrorsException"/> to the dictionary.
-        /// </remarks>
         public bool TryAddModelError([NotNull] string key, [NotNull] Exception exception)
         {
             if (ErrorCount >= MaxAllowedErrors - 1)
@@ -186,12 +225,10 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
         /// </summary>
         /// <param name="key">The key of the <see cref="ModelState"/> to add errors to.</param>
         /// <param name="errorMessage">The error message to add.</param>
-        /// <returns>True if the error was added, false if the dictionary has already recorded
+        /// <returns>
-        /// at least <see cref="MaxAllowedErrors"/> number of errors.</returns>
+        /// <c>True</c> if the given error was added, <c>false</c> if the error was ignored.
-        /// <remarks>
+        /// See <see cref="MaxAllowedErrors"/>.
-        /// This method only allows adding up to <see cref="MaxAllowedErrors"/> - 1. <see cref="MaxAllowedErrors"/>nt
+        /// </returns>
-        /// invocation would result in adding a <see cref="TooManyModelErrorsException"/> to the dictionary.
-        /// </remarks>
         public bool TryAddModelError([NotNull] string key, [NotNull] string errorMessage)
         {
             if (ErrorCount >= MaxAllowedErrors - 1)

src/Microsoft.AspNet.Mvc.ModelBinding/Validation/ModelValidationNode.cs

@@ -102,7 +102,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
 
         public void Validate([NotNull] ModelValidationContext validationContext, ModelValidationNode parentNode)
         {
-            if (SuppressValidation || !validationContext.ModelState.CanAddErrors)
+            if (SuppressValidation || validationContext.ModelState.HasReachedMaxErrors)
             {
                 // Short circuit if validation does not need to be applied or if we've reached the max number of
                 // validation errors.

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

@@ -2043,7 +2043,7 @@ namespace Microsoft.AspNet.Mvc
                 controllerFactory.Object,
                 actionDescriptor,
                 inputFormattersProvider.Object,
-                new DefaultControllerActionArgumentBinder(new EmptyModelMetadataProvider()),
+                new DefaultControllerActionArgumentBinder(new EmptyModelMetadataProvider(), new MockMvcOptionsAccessor()),
                 new MockModelBinderProvider() { ModelBinders = new List<IModelBinder>() { binder.Object } },
                 new MockModelValidatorProviderProvider(),
                 new MockValueProviderFactoryProvider(),

test/Microsoft.AspNet.Mvc.Core.Test/ParameterBinding/ControllerActionArgumentBinderTests.cs

@@ -51,21 +51,15 @@ namespace Microsoft.AspNet.Mvc.Core.Test
         public void GetModelBindingContext_ReturnsOnlyIncludedProperties_UsingBindAttributeInclude()
         {
             // Arrange
-            var actionContext = new ActionContext(
-                new DefaultHttpContext(),
-                new RouteData(),
-                new ControllerActionDescriptor());
-
             var metadataProvider = new DataAnnotationsModelMetadataProvider();
             var modelMetadata = metadataProvider.GetMetadataForType(
-                modelAccessor: null, modelType: typeof(TypeWithIncludedPropertiesUsingBindAttribute));
+                modelAccessor: null, 
-
+                modelType: typeof(TypeWithIncludedPropertiesUsingBindAttribute));
-            var actionBindingContext = new ActionBindingContext();
 
             // Act
             var context = DefaultControllerActionArgumentBinder.GetModelBindingContext(
                 modelMetadata,
-                actionContext,
+                new ModelStateDictionary(),
                 Mock.Of<OperationBindingContext>());
 
             // Assert
@@ -80,11 +74,6 @@ namespace Microsoft.AspNet.Mvc.Core.Test
             var type = typeof(ControllerActionArgumentBinderTests);
             var methodInfo = type.GetMethod("ParameterWithNoBindAttribute");
 
-            var actionContext = new ActionContext(
-                new DefaultHttpContext(),
-                new RouteData(),
-                Mock.Of<ControllerActionDescriptor>());
-
             var metadataProvider = new DataAnnotationsModelMetadataProvider();
             var modelMetadata = metadataProvider.GetMetadataForParameter(modelAccessor: null,
                                                                          methodInfo: methodInfo,
@@ -93,7 +82,7 @@ namespace Microsoft.AspNet.Mvc.Core.Test
             // Act
             var context = DefaultControllerActionArgumentBinder.GetModelBindingContext(
                 modelMetadata,
-                actionContext,
+                new ModelStateDictionary(),
                 Mock.Of<OperationBindingContext>());
 
             // Assert
@@ -106,17 +95,14 @@ namespace Microsoft.AspNet.Mvc.Core.Test
         [InlineData("ParameterHasPrefixAndComplexType", false, "simpleModelPrefix")]
         [InlineData("ParameterHasEmptyBindAttribute", true, "parameter")]
         public void GetModelBindingContext_ModelBindingContextIsSetWithModelName_ForParameters(
-            string actionMethodName, bool expectedFallToEmptyPrefix, string expectedModelName)
+            string actionMethodName,
+            bool expectedFallToEmptyPrefix,
+            string expectedModelName)
         {
             // Arrange
             var type = typeof(ControllerActionArgumentBinderTests);
             var methodInfo = type.GetMethod(actionMethodName);
 
-            var actionContext = new ActionContext(
-                new DefaultHttpContext(),
-                new RouteData(),
-                Mock.Of<ControllerActionDescriptor>());
-
             var metadataProvider = new DataAnnotationsModelMetadataProvider();
             var modelMetadata = metadataProvider.GetMetadataForParameter(modelAccessor: null,
                                                                          methodInfo: methodInfo,
@@ -125,7 +111,7 @@ namespace Microsoft.AspNet.Mvc.Core.Test
             // Act
             var context = DefaultControllerActionArgumentBinder.GetModelBindingContext(
                 modelMetadata, 
-                actionContext,
+                new ModelStateDictionary(),
                 Mock.Of<OperationBindingContext>());
 
             // Assert
@@ -138,17 +124,14 @@ namespace Microsoft.AspNet.Mvc.Core.Test
         [InlineData("ParameterHasPrefixAndComplexType", false, "simpleModelPrefix")]
         [InlineData("ParameterHasEmptyBindAttribute", true, "parameter1")]
         public void GetModelBindingContext_ModelBindingContextIsNotSet_ForTypes(
-            string actionMethodName, bool expectedFallToEmptyPrefix, string expectedModelName)
+            string actionMethodName,
+            bool expectedFallToEmptyPrefix,
+            string expectedModelName)
         {
             // Arrange
             var type = typeof(ControllerActionArgumentBinderTests);
             var methodInfo = type.GetMethod(actionMethodName);
 
-            var actionContext = new ActionContext(
-                new DefaultHttpContext(),
-                new RouteData(),
-                Mock.Of<ControllerActionDescriptor>());
-
             var metadataProvider = new DataAnnotationsModelMetadataProvider();
             var modelMetadata = metadataProvider.GetMetadataForParameter(modelAccessor: null,
                                                                          methodInfo: methodInfo,
@@ -157,7 +140,7 @@ namespace Microsoft.AspNet.Mvc.Core.Test
             // Act
             var context = DefaultControllerActionArgumentBinder.GetModelBindingContext(
                 modelMetadata,
-                actionContext,
+                new ModelStateDictionary(),
                 Mock.Of<OperationBindingContext>());
 
             // Assert
@@ -208,7 +191,9 @@ namespace Microsoft.AspNet.Mvc.Core.Test
                 .SetupGet(o => o.InputFormatters)
                 .Returns(new List<IInputFormatter>());
 
-            var invoker = new DefaultControllerActionArgumentBinder(new DataAnnotationsModelMetadataProvider());
+            var invoker = new DefaultControllerActionArgumentBinder(
+                new DataAnnotationsModelMetadataProvider(),
+                new MockMvcOptionsAccessor());
 
             // Act
             var result = await invoker.GetActionArgumentsAsync(actionContext, actionBindingContext);
@@ -259,7 +244,9 @@ namespace Microsoft.AspNet.Mvc.Core.Test
                 .SetupGet(o => o.InputFormatters)
                 .Returns(new List<IInputFormatter>());
 
-            var invoker = new DefaultControllerActionArgumentBinder(new DataAnnotationsModelMetadataProvider());
+            var invoker = new DefaultControllerActionArgumentBinder(
+                new DataAnnotationsModelMetadataProvider(),
+                new MockMvcOptionsAccessor());
 
             // Act
             var result = await invoker.GetActionArgumentsAsync(actionContext, actionBindingContext);
@@ -312,7 +299,9 @@ namespace Microsoft.AspNet.Mvc.Core.Test
                 ModelBinder = binder.Object,
             };
 
-            var invoker = new DefaultControllerActionArgumentBinder(metadataProvider);
+            var invoker = new DefaultControllerActionArgumentBinder(
+                metadataProvider,
+                new MockMvcOptionsAccessor());
 
             // Act
             var result = await invoker.GetActionArgumentsAsync(actionContext, actionBindingContext);
@@ -322,6 +311,62 @@ namespace Microsoft.AspNet.Mvc.Core.Test
             Assert.Equal(value, result["foo"]);
         }
 
+        [Fact]
+        public async Task GetActionArgumentsAsync_SetsMaxModelErrors()
+        {
+            // Arrange
+            Func<object, int> method = foo => 1;
+            var actionDescriptor = new ControllerActionDescriptor
+            {
+                MethodInfo = method.Method,
+                Parameters = new List<ParameterDescriptor>
+                {
+                    new ParameterDescriptor
+                    {
+                        Name = "foo",
+                        ParameterType = typeof(object),
+                    }
+                }
+            };
+
+            var binder = new Mock<IModelBinder>();
+            binder
+                .Setup(b => b.BindModelAsync(It.IsAny<ModelBindingContext>()))
+                .Callback<ModelBindingContext>(c =>
+                {
+                    c.Model = "Hello";
+                })
+                .Returns(Task.FromResult(result: true));
+
+            var actionContext = new ActionContext(
+                new DefaultHttpContext(),
+                new RouteData(),
+                actionDescriptor);
+
+            var actionBindingContext = new ActionBindingContext()
+            {
+                ModelBinder = binder.Object,
+            };
+
+            var inputFormattersProvider = new Mock<IInputFormattersProvider>();
+            inputFormattersProvider
+                .SetupGet(o => o.InputFormatters)
+                .Returns(new List<IInputFormatter>());
+
+            var options = new MockMvcOptionsAccessor();
+            options.Options.MaxModelValidationErrors = 5;
+
+            var invoker = new DefaultControllerActionArgumentBinder(
+                new DataAnnotationsModelMetadataProvider(),
+                options);
+
+            // Act
+            var result = await invoker.GetActionArgumentsAsync(actionContext, actionBindingContext);
+
+            // Assert
+            Assert.Equal(5, actionContext.ModelState.MaxAllowedErrors);
+        }
+
         private class TestController
         {
             public string UnmarkedProperty { get; set; }

test/Microsoft.AspNet.Mvc.ModelBinding.Test/Validation/ModelStateDictionaryTest.cs

@@ -385,7 +385,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
             dictionary.AddModelError("key6", "error6");
 
             // Act and Assert
-            Assert.False(dictionary.CanAddErrors);
+            Assert.True(dictionary.HasReachedMaxErrors);
             Assert.Equal(5, dictionary.ErrorCount);
             var error = Assert.Single(dictionary[""].Errors);
             Assert.IsType<TooManyModelErrorsException>(error.Exception);
@@ -415,7 +415,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
             result = dictionary.TryAddModelError("key4", "error4");
             Assert.False(result);
 
-            Assert.False(dictionary.CanAddErrors);
+            Assert.True(dictionary.HasReachedMaxErrors);
             Assert.Equal(3, dictionary.ErrorCount);
             Assert.Equal(3, dictionary.Count);
 
@@ -441,7 +441,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
             dictionary.AddModelError("key5", new FormatException());
 
             // Act and Assert
-            Assert.False(dictionary.CanAddErrors);
+            Assert.True(dictionary.HasReachedMaxErrors);
             Assert.Equal(4, dictionary.ErrorCount);
             Assert.Equal(4, dictionary.Count);
             var error = Assert.Single(dictionary[""].Errors);
@@ -499,6 +499,31 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
             Assert.Equal(expected, error.Exception.Message);
         }
 
+        [Theory]
+        [InlineData(2, false)]
+        [InlineData(3, true)]
+        [InlineData(4, true)]
+        public void ModelStateDictionary_HasReachedMaxErrors(int errorCount, bool expected)
+        {
+            // Arrange
+            var dictionary = new ModelStateDictionary()
+            {
+                MaxAllowedErrors = 3
+            };
+
+            for (var i = 0; i < errorCount; i++)
+            {
+                dictionary.AddModelError("key" + i, "error");
+            }
+
+            // Act
+            var canAdd = dictionary.HasReachedMaxErrors;
+
+            // Assert
+            Assert.Equal(expected, canAdd);
+        }
+
+
         private static ValueProviderResult GetValueProviderResult(object rawValue = null, string attemptedValue = null)
         {
             return new ValueProviderResult(rawValue ?? "some value",