From 064c01cf2b022e365f82e5729a616e17b7f54d9a Mon Sep 17 00:00:00 2001 From: sornaks Date: Thu, 9 Apr 2015 09:57:30 -0700 Subject: [PATCH] #2267 - Moving responsibility for setting ModelState.MaxAllowedErrors into a more appropriate location. --- .../ControllerActionInvoker.cs | 6 +- .../ControllerActionInvokerProvider.cs | 5 +- .../DefaultControllerActionArgumentBinder.cs | 6 +- .../FilterActionInvoker.cs | 8 ++- .../MvcRouteHandler.cs | 3 - .../ControllerActionInvokerTest.cs | 57 ++++++++++++++++--- .../MvcRouteHandlerTests.cs | 36 ------------ .../ControllerActionArgumentBinderTests.cs | 37 +----------- 8 files changed, 64 insertions(+), 94 deletions(-) diff --git a/src/Microsoft.AspNet.Mvc.Core/ControllerActionInvoker.cs b/src/Microsoft.AspNet.Mvc.Core/ControllerActionInvoker.cs index 1031623b04..c2555b1c5d 100644 --- a/src/Microsoft.AspNet.Mvc.Core/ControllerActionInvoker.cs +++ b/src/Microsoft.AspNet.Mvc.Core/ControllerActionInvoker.cs @@ -30,7 +30,8 @@ namespace Microsoft.AspNet.Mvc.Core [NotNull] IReadOnlyList modelValidatorProviders, [NotNull] IReadOnlyList valueProviderFactories, [NotNull] IScopedInstance actionBindingContextAccessor, - [NotNull] ITempDataDictionary tempData) + [NotNull] ITempDataDictionary tempData, + int maxModelValidationErrors) : base( actionContext, filterProviders, @@ -39,7 +40,8 @@ namespace Microsoft.AspNet.Mvc.Core modelBinders, modelValidatorProviders, valueProviderFactories, - actionBindingContextAccessor) + actionBindingContextAccessor, + maxModelValidationErrors) { _descriptor = descriptor; _controllerFactory = controllerFactory; diff --git a/src/Microsoft.AspNet.Mvc.Core/ControllerActionInvokerProvider.cs b/src/Microsoft.AspNet.Mvc.Core/ControllerActionInvokerProvider.cs index 598acc1972..d183ab3c2c 100644 --- a/src/Microsoft.AspNet.Mvc.Core/ControllerActionInvokerProvider.cs +++ b/src/Microsoft.AspNet.Mvc.Core/ControllerActionInvokerProvider.cs @@ -22,6 +22,7 @@ namespace Microsoft.AspNet.Mvc.Core private readonly IReadOnlyList _valueProviderFactories; private readonly IScopedInstance _actionBindingContextAccessor; private readonly ITempDataDictionary _tempData; + private readonly int _maxModelValidationErrors; public ControllerActionInvokerProvider( IControllerFactory controllerFactory, @@ -41,6 +42,7 @@ namespace Microsoft.AspNet.Mvc.Core _valueProviderFactories = optionsAccessor.Options.ValueProviderFactories.ToArray(); _actionBindingContextAccessor = actionBindingContextAccessor; _tempData = tempData; + _maxModelValidationErrors = optionsAccessor.Options.MaxModelValidationErrors; } public int Order @@ -67,7 +69,8 @@ namespace Microsoft.AspNet.Mvc.Core _modelValidatorProviders, _valueProviderFactories, _actionBindingContextAccessor, - _tempData); + _tempData, + _maxModelValidationErrors); } } diff --git a/src/Microsoft.AspNet.Mvc.Core/DefaultControllerActionArgumentBinder.cs b/src/Microsoft.AspNet.Mvc.Core/DefaultControllerActionArgumentBinder.cs index ac98375c8b..d272796a91 100644 --- a/src/Microsoft.AspNet.Mvc.Core/DefaultControllerActionArgumentBinder.cs +++ b/src/Microsoft.AspNet.Mvc.Core/DefaultControllerActionArgumentBinder.cs @@ -20,16 +20,13 @@ namespace Microsoft.AspNet.Mvc public class DefaultControllerActionArgumentBinder : IControllerActionArgumentBinder { private readonly IModelMetadataProvider _modelMetadataProvider; - private readonly MvcOptions _options; private readonly IObjectModelValidator _validator; public DefaultControllerActionArgumentBinder( IModelMetadataProvider modelMetadataProvider, - IObjectModelValidator validator, - IOptions optionsAccessor) + IObjectModelValidator validator) { _modelMetadataProvider = modelMetadataProvider; - _options = optionsAccessor.Options; _validator = validator; } @@ -89,7 +86,6 @@ namespace Microsoft.AspNet.Mvc IDictionary arguments, IEnumerable parameterMetadata) { - modelState.MaxAllowedErrors = _options.MaxModelValidationErrors; foreach (var parameter in parameterMetadata) { var metadata = _modelMetadataProvider.GetMetadataForType(parameter.ParameterType); diff --git a/src/Microsoft.AspNet.Mvc.Core/FilterActionInvoker.cs b/src/Microsoft.AspNet.Mvc.Core/FilterActionInvoker.cs index ec65bd2f57..4b3882a10d 100644 --- a/src/Microsoft.AspNet.Mvc.Core/FilterActionInvoker.cs +++ b/src/Microsoft.AspNet.Mvc.Core/FilterActionInvoker.cs @@ -21,8 +21,8 @@ namespace Microsoft.AspNet.Mvc.Core private readonly IReadOnlyList _outputFormatters; private readonly IReadOnlyList _modelValidatorProviders; private readonly IReadOnlyList _valueProviderFactories; - private readonly IScopedInstance _actionBindingContextAccessor; + private readonly int _maxModelValidationErrors; private IFilter[] _filters; private FilterCursor _cursor; @@ -48,7 +48,8 @@ namespace Microsoft.AspNet.Mvc.Core [NotNull] IReadOnlyList modelBinders, [NotNull] IReadOnlyList modelValidatorProviders, [NotNull] IReadOnlyList valueProviderFactories, - [NotNull] IScopedInstance actionBindingContextAccessor) + [NotNull] IScopedInstance actionBindingContextAccessor, + int maxModelValidationErrors) { ActionContext = actionContext; @@ -59,6 +60,7 @@ namespace Microsoft.AspNet.Mvc.Core _modelValidatorProviders = modelValidatorProviders; _valueProviderFactories = valueProviderFactories; _actionBindingContextAccessor = actionBindingContextAccessor; + _maxModelValidationErrors = maxModelValidationErrors; } protected ActionContext ActionContext { get; private set; } @@ -101,6 +103,8 @@ namespace Microsoft.AspNet.Mvc.Core _filters = GetFilters(); _cursor = new FilterCursor(_filters); + ActionContext.ModelState.MaxAllowedErrors = _maxModelValidationErrors; + await InvokeAllAuthorizationFiltersAsync(); // If Authorization Filters return a result, it's a short circuit because diff --git a/src/Microsoft.AspNet.Mvc.Core/MvcRouteHandler.cs b/src/Microsoft.AspNet.Mvc.Core/MvcRouteHandler.cs index 522d5fa94d..7dfeaed280 100644 --- a/src/Microsoft.AspNet.Mvc.Core/MvcRouteHandler.cs +++ b/src/Microsoft.AspNet.Mvc.Core/MvcRouteHandler.cs @@ -93,9 +93,6 @@ namespace Microsoft.AspNet.Mvc var actionContext = new ActionContext(context.HttpContext, context.RouteData, actionDescriptor); - var optionsAccessor = services.GetRequiredService>(); - actionContext.ModelState.MaxAllowedErrors = optionsAccessor.Options.MaxModelValidationErrors; - var contextAccessor = services.GetRequiredService>(); contextAccessor.Value = actionContext; var invokerFactory = services.GetRequiredService(); diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/ControllerActionInvokerTest.cs b/test/Microsoft.AspNet.Mvc.Core.Test/ControllerActionInvokerTest.cs index cc20c2adbd..8ed3773e0b 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/ControllerActionInvokerTest.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/ControllerActionInvokerTest.cs @@ -13,6 +13,7 @@ using Microsoft.AspNet.Mvc.ModelBinding; using Microsoft.AspNet.Mvc.ModelBinding.Validation; using Microsoft.AspNet.Routing; using Microsoft.AspNet.Testing; +using Microsoft.Framework.Internal; using Microsoft.Framework.Logging; using Microsoft.Framework.Logging.Testing; using Microsoft.Framework.OptionsModel; @@ -1959,18 +1960,37 @@ namespace Microsoft.AspNet.Mvc Assert.Same(input, contentResult.Value); } + [Fact] + public async Task MaxAllowedErrorsIsSet_BeforeCallingAuthorizationFilter() + { + // Arrange + var expected = 147; + var filter = new MockAuthorizationFilter(expected); + var invoker = CreateInvoker( + filter, + actionThrows: false, + tempData: null, + maxAllowedErrorsInModelState: expected); + + // Act & Assert + // The authorization filter asserts if MaxAllowedErrors was set to the right value. + await invoker.InvokeAsync(); + } + private TestControllerActionInvoker CreateInvoker( IFilter filter, bool actionThrows = false, - ITempDataDictionary tempData = null) + ITempDataDictionary tempData = null, + int maxAllowedErrorsInModelState = 200) { - return CreateInvoker(new[] { filter }, actionThrows, tempData); + return CreateInvoker(new[] { filter }, actionThrows, tempData, maxAllowedErrorsInModelState); } private TestControllerActionInvoker CreateInvoker( IFilter[] filters, bool actionThrows = false, - ITempDataDictionary tempData = null) + ITempDataDictionary tempData = null, + int maxAllowedErrorsInModelState = 200) { var actionDescriptor = new ControllerActionDescriptor() { @@ -2044,7 +2064,8 @@ namespace Microsoft.AspNet.Mvc new IModelValidatorProvider[0], new IValueProviderFactory[0], new MockScopedInstance(), - tempData); + tempData, + maxAllowedErrorsInModelState); return invoker; } @@ -2100,13 +2121,13 @@ namespace Microsoft.AspNet.Mvc new IOutputFormatter[0], new DefaultControllerActionArgumentBinder( metadataProvider, - new DefaultObjectValidator(new IExcludeTypeValidationFilter[0], metadataProvider), - new MockMvcOptionsAccessor()), + new DefaultObjectValidator(new IExcludeTypeValidationFilter[0], metadataProvider)), new IModelBinder[] { binder.Object }, new IModelValidatorProvider[0], new IValueProviderFactory[0], new MockScopedInstance(), - Mock.Of()); + Mock.Of(), + 200); // Act await invoker.InvokeAsync(); @@ -2205,7 +2226,8 @@ namespace Microsoft.AspNet.Mvc IReadOnlyList modelValidatorProviders, IReadOnlyList valueProviderFactories, IScopedInstance actionBindingContext, - ITempDataDictionary tempData) + ITempDataDictionary tempData, + int maxAllowedErrorsInModelState) : base( actionContext, filterProvider, @@ -2218,7 +2240,8 @@ namespace Microsoft.AspNet.Mvc modelValidatorProviders, valueProviderFactories, actionBindingContext, - tempData) + tempData, + maxAllowedErrorsInModelState) { ControllerFactory = controllerFactory; } @@ -2233,5 +2256,21 @@ namespace Microsoft.AspNet.Mvc ControllerFactory.Verify(); } } + + private class MockAuthorizationFilter : IAuthorizationFilter + { + int _expectedMaxAllowedErrors; + + public MockAuthorizationFilter(int maxAllowedErrors) + { + _expectedMaxAllowedErrors = maxAllowedErrors; + } + + public void OnAuthorization([NotNull]AuthorizationContext context) + { + Assert.NotNull(context.ModelState.MaxAllowedErrors); + Assert.Equal(_expectedMaxAllowedErrors, context.ModelState.MaxAllowedErrors); + } + } } } diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/MvcRouteHandlerTests.cs b/test/Microsoft.AspNet.Mvc.Core.Test/MvcRouteHandlerTests.cs index e1cbff1005..ceb2856940 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/MvcRouteHandlerTests.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/MvcRouteHandlerTests.cs @@ -67,42 +67,6 @@ namespace Microsoft.AspNet.Mvc Assert.Equal(expectedMessage, sink.Writes[0].State?.ToString()); } - [Fact] - public async Task RouteAsync_SetsMaxErrorCountOnModelStateDictionary() - { - // Arrange - var expected = 199; - var optionsAccessor = new Mock>(); - var options = new MvcOptions - { - MaxModelValidationErrors = expected - }; - optionsAccessor.SetupGet(o => o.Options) - .Returns(options); - - var invoked = false; - var mockInvokerFactory = new Mock(); - mockInvokerFactory.Setup(f => f.CreateInvoker(It.IsAny())) - .Callback(c => - { - Assert.Equal(expected, c.ModelState.MaxAllowedErrors); - invoked = true; - }) - .Returns(Mock.Of()); - - var context = CreateRouteContext( - invokerFactory: mockInvokerFactory.Object, - optionsAccessor: optionsAccessor.Object); - - var handler = new MvcRouteHandler(); - - // Act - await handler.RouteAsync(context); - - // Assert - Assert.True(invoked); - } - [Fact] public async Task RouteAsync_CreatesNewRouteData() { diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/ParameterBinding/ControllerActionArgumentBinderTests.cs b/test/Microsoft.AspNet.Mvc.Core.Test/ParameterBinding/ControllerActionArgumentBinderTests.cs index c725f743d3..c53a3d16b2 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/ParameterBinding/ControllerActionArgumentBinderTests.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/ParameterBinding/ControllerActionArgumentBinderTests.cs @@ -245,37 +245,6 @@ namespace Microsoft.AspNet.Mvc.Core.Test mockValidatorProvider.Verify(o => o.Validate(It.IsAny()), Times.Never()); } - [Fact] - public async Task GetActionArgumentsAsync_SetsMaxModelErrors() - { - // Arrange - Func method = foo => 1; - var actionDescriptor = GetActionDescriptor(); - actionDescriptor.Parameters.Add( - new ParameterDescriptor - { - Name = "foo", - ParameterType = typeof(object), - BindingInfo = new BindingInfo(), - }); - - var actionContext = new ActionContext( - new DefaultHttpContext(), - new RouteData(), - actionDescriptor); - - var actionBindingContext = GetActionBindingContext(); - - var argumentBinder = GetArgumentBinder(); - - // Act - var result = await argumentBinder - .BindActionArgumentsAsync(actionContext, actionBindingContext, new TestController()); - - // Assert - Assert.Equal(5, actionContext.ModelState.MaxAllowedErrors); - } - [Fact] public async Task GetActionArgumentsAsync_CallsValidator_ForControllerProperties_IfModelBinderSucceeds() { @@ -409,9 +378,6 @@ namespace Microsoft.AspNet.Mvc.Core.Test private static DefaultControllerActionArgumentBinder GetArgumentBinder(IObjectModelValidator validator = null) { - var options = new MockMvcOptionsAccessor(); - options.Options.MaxModelValidationErrors = 5; - if (validator == null) { var mockValidator = new Mock(MockBehavior.Strict); @@ -421,8 +387,7 @@ namespace Microsoft.AspNet.Mvc.Core.Test return new DefaultControllerActionArgumentBinder( TestModelMetadataProvider.CreateDefaultProvider(), - validator, - options); + validator); } private class TestController