diff --git a/src/Microsoft.AspNet.Mvc.Core/DefaultControllerActionArgumentBinder.cs b/src/Microsoft.AspNet.Mvc.Core/DefaultControllerActionArgumentBinder.cs index 096cebbb27..0096ce1373 100644 --- a/src/Microsoft.AspNet.Mvc.Core/DefaultControllerActionArgumentBinder.cs +++ b/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 optionsAccessor) { _modelMetadataProvider = modelMetadataProvider; + _options = optionsAccessor.Options; } public async Task> 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, diff --git a/src/Microsoft.AspNet.Mvc.Core/MvcOptions.cs b/src/Microsoft.AspNet.Mvc.Core/MvcOptions.cs index cd5b652f87..619be72e83 100644 --- a/src/Microsoft.AspNet.Mvc.Core/MvcOptions.cs +++ b/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() { diff --git a/src/Microsoft.AspNet.Mvc.ModelBinding/ModelStateDictionary.cs b/src/Microsoft.AspNet.Mvc.ModelBinding/ModelStateDictionary.cs index 7056c95b22..07caea2a4f 100644 --- a/src/Microsoft.AspNet.Mvc.ModelBinding/ModelStateDictionary.cs +++ b/src/Microsoft.AspNet.Mvc.ModelBinding/ModelStateDictionary.cs @@ -15,13 +15,30 @@ namespace Microsoft.AspNet.Mvc.ModelBinding /// public class ModelStateDictionary : IDictionary { + // Make sure to update the doc headers if this value is changed. + /// + /// The default value for of 200. + /// + public static readonly int DefaultMaxAllowedErrors = 200; + private readonly IDictionary _innerDictionary; + private int _maxAllowedErrors; /// /// Initializes a new instance of the class. /// public ModelStateDictionary() + : this(DefaultMaxAllowedErrors) { + } + + /// + /// Initializes a new instance of the class. + /// + public ModelStateDictionary(int maxAllowedErrors) + { + MaxAllowedErrors = maxAllowedErrors; + _innerDictionary = new Dictionary(StringComparer.OrdinalIgnoreCase); } @@ -41,29 +58,53 @@ namespace Microsoft.AspNet.Mvc.ModelBinding } /// - /// Gets or sets the maximum allowed errors in this instance of . - /// Defaults to . + /// Gets or sets the maximum allowed model state errors in this instance of . + /// Defaults to 200. /// /// - /// The value of this property is used to track the total number of calls to - /// and after which - /// an error is thrown for further invocations. Errors added via modifying do not - /// count towards this limit. + /// + /// tracks the number of model errors added by calls to + /// or . + /// Once the value of MaxAllowedErrors - 1 is reached, if another attempt is made to add an error, + /// the error message will be ignored and a will be added. + /// + /// + /// Errors added via modifying directly do not count towards this limit. + /// /// - public int MaxAllowedErrors { get; set; } = int.MaxValue; + public int MaxAllowedErrors + { + get + { + return _maxAllowedErrors; + } + set + { + if (value < 0) + { + throw new ArgumentOutOfRangeException(nameof(value)); + } + + _maxAllowedErrors = value; + } + } /// - /// Gets a flag that determines if the total number of added errors (given by ) is - /// fewer than . + /// Gets a value indicating whether or not the maximum number of errors have been + /// recorded. /// - public bool CanAddErrors + /// + /// Returns true if a has been recorded; + /// otherwise false. + /// + public bool HasReachedMaxErrors { - get { return ErrorCount < MaxAllowedErrors; } + get { return ErrorCount >= MaxAllowedErrors; } } /// /// Gets the number of errors added to this instance of via - /// and . + /// or . /// public int ErrorCount { get; private set; } @@ -149,12 +190,10 @@ namespace Microsoft.AspNet.Mvc.ModelBinding /// /// The key of the to add errors to. /// The to add. - /// True if the error was added, false if the dictionary has already recorded - /// at least number of errors. - /// - /// This method only allows adding up to - 1. nt - /// invocation would result in adding a to the dictionary. - /// + /// + /// True if the given error was added, false if the error was ignored. + /// See . + /// public bool TryAddModelError([NotNull] string key, [NotNull] Exception exception) { if (ErrorCount >= MaxAllowedErrors - 1) @@ -186,12 +225,10 @@ namespace Microsoft.AspNet.Mvc.ModelBinding /// /// The key of the to add errors to. /// The error message to add. - /// True if the error was added, false if the dictionary has already recorded - /// at least number of errors. - /// - /// This method only allows adding up to - 1. nt - /// invocation would result in adding a to the dictionary. - /// + /// + /// True if the given error was added, false if the error was ignored. + /// See . + /// public bool TryAddModelError([NotNull] string key, [NotNull] string errorMessage) { if (ErrorCount >= MaxAllowedErrors - 1) diff --git a/src/Microsoft.AspNet.Mvc.ModelBinding/Validation/ModelValidationNode.cs b/src/Microsoft.AspNet.Mvc.ModelBinding/Validation/ModelValidationNode.cs index fdacf2d6d3..0dfafb3875 100644 --- a/src/Microsoft.AspNet.Mvc.ModelBinding/Validation/ModelValidationNode.cs +++ b/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. diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/ControllerActionInvokerTest.cs b/test/Microsoft.AspNet.Mvc.Core.Test/ControllerActionInvokerTest.cs index a6fdf39d44..a9f012eb15 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/ControllerActionInvokerTest.cs +++ b/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() { binder.Object } }, new MockModelValidatorProviderProvider(), new MockValueProviderFactoryProvider(), diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/ParameterBinding/ControllerActionArgumentBinderTests.cs b/test/Microsoft.AspNet.Mvc.Core.Test/ParameterBinding/ControllerActionArgumentBinderTests.cs index 342dc17525..23c83e57f9 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/ParameterBinding/ControllerActionArgumentBinderTests.cs +++ b/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)); - - var actionBindingContext = new ActionBindingContext(); + modelAccessor: null, + modelType: typeof(TypeWithIncludedPropertiesUsingBindAttribute)); // Act var context = DefaultControllerActionArgumentBinder.GetModelBindingContext( modelMetadata, - actionContext, + new ModelStateDictionary(), Mock.Of()); // 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()); - 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()); // 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()); - 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()); // 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()); - 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()); // Assert @@ -208,7 +191,9 @@ namespace Microsoft.AspNet.Mvc.Core.Test .SetupGet(o => o.InputFormatters) .Returns(new List()); - 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()); - 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 method = foo => 1; + var actionDescriptor = new ControllerActionDescriptor + { + MethodInfo = method.Method, + Parameters = new List + { + new ParameterDescriptor + { + Name = "foo", + ParameterType = typeof(object), + } + } + }; + + var binder = new Mock(); + binder + .Setup(b => b.BindModelAsync(It.IsAny())) + .Callback(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(); + inputFormattersProvider + .SetupGet(o => o.InputFormatters) + .Returns(new List()); + + 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; } diff --git a/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Validation/ModelStateDictionaryTest.cs b/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Validation/ModelStateDictionaryTest.cs index 72aca9c32c..4b16e03562 100644 --- a/test/Microsoft.AspNet.Mvc.ModelBinding.Test/Validation/ModelStateDictionaryTest.cs +++ b/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(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",