From 42df4cf2eddd92221104c55fe6bc48a42e2b3030 Mon Sep 17 00:00:00 2001 From: Ryan Nowak Date: Tue, 27 Jan 2015 14:09:39 -0800 Subject: [PATCH] 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. --- .../DefaultControllerActionArgumentBinder.cs | 17 ++- src/Microsoft.AspNet.Mvc.Core/MvcOptions.cs | 3 +- .../ModelStateDictionary.cs | 85 ++++++++++---- .../Validation/ModelValidationNode.cs | 2 +- .../ControllerActionInvokerTest.cs | 2 +- .../ControllerActionArgumentBinderTests.cs | 109 +++++++++++++----- .../Validation/ModelStateDictionaryTest.cs | 31 ++++- 7 files changed, 183 insertions(+), 66 deletions(-) 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",