From fc168d63f67a91b63c40745e567eda908234cdf9 Mon Sep 17 00:00:00 2001 From: Yishai Galatzer Date: Tue, 11 Mar 2014 19:28:29 -0700 Subject: [PATCH] refactor to return ordered filter list from filter provider Arrange filters to a pipeline in the action invoker Allow providing the original filter definition to a type and service filter --- .../ActionResults/EmptyResult.cs | 1 + .../Filters/ActionFilterAttribute.cs | 2 +- .../Filters/ActionResultFilterAttribute.cs | 2 +- .../Filters/AuthorizationFilterAttribute.cs | 2 +- .../Filters/DefaultFilterProvider.cs | 138 +++++++----------- .../Filters/ExceptionFilterAttribute.cs | 2 +- .../Filters/FilterDescriptor.cs | 14 +- .../Filters/FilterDescriptorOrderComparer.cs | 4 +- .../Filters/FilterProviderContext.cs | 10 +- .../Filters/IFilter.cs | 1 - .../Filters/IFilterContainer.cs | 7 + .../Filters/IOrderedFilter.cs | 7 + .../ReflectedActionInvoker.cs | 80 +++++----- .../ReflectedActionInvokerProvider.cs | 3 +- 14 files changed, 138 insertions(+), 135 deletions(-) create mode 100644 src/Microsoft.AspNet.Mvc.Core/Filters/IFilterContainer.cs create mode 100644 src/Microsoft.AspNet.Mvc.Core/Filters/IOrderedFilter.cs diff --git a/src/Microsoft.AspNet.Mvc.Core/ActionResults/EmptyResult.cs b/src/Microsoft.AspNet.Mvc.Core/ActionResults/EmptyResult.cs index 71d493c122..04913f67ee 100644 --- a/src/Microsoft.AspNet.Mvc.Core/ActionResults/EmptyResult.cs +++ b/src/Microsoft.AspNet.Mvc.Core/ActionResults/EmptyResult.cs @@ -13,6 +13,7 @@ namespace Microsoft.AspNet.Mvc public async Task ExecuteResultAsync(ActionContext context) { + context.HttpContext.Response.StatusCode = 204; } } } diff --git a/src/Microsoft.AspNet.Mvc.Core/Filters/ActionFilterAttribute.cs b/src/Microsoft.AspNet.Mvc.Core/Filters/ActionFilterAttribute.cs index b84c226fb8..f486d99da3 100644 --- a/src/Microsoft.AspNet.Mvc.Core/Filters/ActionFilterAttribute.cs +++ b/src/Microsoft.AspNet.Mvc.Core/Filters/ActionFilterAttribute.cs @@ -5,7 +5,7 @@ namespace Microsoft.AspNet.Mvc { // TODO: Consider making this user a before and after pattern instead of just Invoke, same for all other filter attributes. [AttributeUsage(AttributeTargets.Class | AttributeTargets.Method, AllowMultiple = true, Inherited = true)] - public abstract class ActionFilterAttribute : Attribute, IActionFilter, IFilter + public abstract class ActionFilterAttribute : Attribute, IActionFilter, IOrderedFilter { public abstract Task Invoke(ActionFilterContext context, Func next); diff --git a/src/Microsoft.AspNet.Mvc.Core/Filters/ActionResultFilterAttribute.cs b/src/Microsoft.AspNet.Mvc.Core/Filters/ActionResultFilterAttribute.cs index 0a745c3908..cec8cb231d 100644 --- a/src/Microsoft.AspNet.Mvc.Core/Filters/ActionResultFilterAttribute.cs +++ b/src/Microsoft.AspNet.Mvc.Core/Filters/ActionResultFilterAttribute.cs @@ -4,7 +4,7 @@ using System.Threading.Tasks; namespace Microsoft.AspNet.Mvc { [AttributeUsage(AttributeTargets.Class | AttributeTargets.Method, AllowMultiple = true, Inherited = true)] - public abstract class ActionResultFilterAttribute : Attribute, IActionResultFilter, IFilter + public abstract class ActionResultFilterAttribute : Attribute, IActionResultFilter, IOrderedFilter { public abstract Task Invoke(ActionResultFilterContext context, Func next); diff --git a/src/Microsoft.AspNet.Mvc.Core/Filters/AuthorizationFilterAttribute.cs b/src/Microsoft.AspNet.Mvc.Core/Filters/AuthorizationFilterAttribute.cs index b589972673..26f15271ce 100644 --- a/src/Microsoft.AspNet.Mvc.Core/Filters/AuthorizationFilterAttribute.cs +++ b/src/Microsoft.AspNet.Mvc.Core/Filters/AuthorizationFilterAttribute.cs @@ -4,7 +4,7 @@ using System.Threading.Tasks; namespace Microsoft.AspNet.Mvc { [AttributeUsage(AttributeTargets.Class | AttributeTargets.Method, AllowMultiple = true, Inherited = true)] - public abstract class AuthorizationFilterAttribute : Attribute, IFilter, IAuthorizationFilter + public abstract class AuthorizationFilterAttribute : Attribute, IAuthorizationFilter, IOrderedFilter { public abstract Task Invoke(AuthorizationFilterContext context, Func next); diff --git a/src/Microsoft.AspNet.Mvc.Core/Filters/DefaultFilterProvider.cs b/src/Microsoft.AspNet.Mvc.Core/Filters/DefaultFilterProvider.cs index e1908fce94..c873d8826b 100644 --- a/src/Microsoft.AspNet.Mvc.Core/Filters/DefaultFilterProvider.cs +++ b/src/Microsoft.AspNet.Mvc.Core/Filters/DefaultFilterProvider.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Generic; +using System.Diagnostics.Contracts; using Microsoft.AspNet.DependencyInjection; namespace Microsoft.AspNet.Mvc.Filters @@ -11,23 +12,23 @@ namespace Microsoft.AspNet.Mvc.Filters ServiceProvider = serviceProvider; } + public int Order + { + get { return 0; } + } + + protected IServiceProvider ServiceProvider { get; private set; } + public virtual void Invoke(FilterProviderContext context, Action callNext) { - FilterDescriptor[] filterDescriptors; - if (context.ActionDescriptor.FilterDescriptors != null) { // make a copy of the list, TODO: Make the actiondescriptor immutable - filterDescriptors = context.ActionDescriptor.FilterDescriptors.ToArray(); + var filterDescriptors = context.ActionDescriptor.FilterDescriptors.ToArray(); - //AddGlobalFilters_moveToAdPipeline(filters); - - if (filterDescriptors.Length > 0) + for (int i = 0; i < filterDescriptors.Length; i++) { - for (int i = 0; i < filterDescriptors.Length; i++) - { - GetFilter(context, filterDescriptors[i].Filter); - } + ProvideFilter(context, filterDescriptors[i].Filter); } } @@ -37,96 +38,69 @@ namespace Microsoft.AspNet.Mvc.Filters } } - public virtual void GetFilter(FilterProviderContext context, IFilter filter) + public virtual void ProvideFilter(FilterProviderContext context, IFilter filter) { - bool failIfNotFilter = true; - var serviceFilterSignature = filter as IServiceFilter; if (serviceFilterSignature != null) { - // TODO: How do we pass extra parameters - var serviceFilter = ServiceProvider.GetService(serviceFilterSignature.ServiceType); + var serviceFilter = ServiceProvider.GetService(serviceFilterSignature.ServiceType) as IFilter; - AddFilters(context, serviceFilter, true); - failIfNotFilter = false; + if (serviceFilter == null) + { + throw new InvalidOperationException("Service filter must be of type IFilter"); + } + + ApplyFilterToContainer(serviceFilter, filter); + AddFilters(context, serviceFilter); } - - var typeFilterSignature = filter as ITypeFilter; - if (typeFilterSignature != null) + else { - // TODO: How do we pass extra parameters - var typeFilter = ActivatorUtilities.CreateInstance(ServiceProvider, typeFilterSignature.ImplementationType); + var typeFilterSignature = filter as ITypeFilter; + if (typeFilterSignature != null) + { + if (typeFilterSignature.ImplementationType == null) + { + throw new InvalidOperationException("Type filter must provide a type to instantiate"); + } - AddFilters(context, typeFilter, true); - failIfNotFilter = false; + if (!typeof (IFilter).IsAssignableFrom(typeFilterSignature.ImplementationType)) + { + throw new InvalidOperationException("Type filter must implement IFilter"); + } + + // TODO: Move activatorUtilities to come from the service provider. + var typeFilter = ActivatorUtilities.CreateInstance(ServiceProvider, typeFilterSignature.ImplementationType) as IFilter; + + ApplyFilterToContainer(typeFilter, filter); + AddFilters(context, typeFilter); + } + else + { + AddFilters(context, filter); + } } - - AddFilters(context, filter, failIfNotFilter); } - protected IServiceProvider ServiceProvider { get; private set; } - - public int Order + private void AddFilters(FilterProviderContext context, IFilter filter) { - get { return 0; } + if (context.OrderedFilterList == null) + { + context.OrderedFilterList = new List(); + } + + context.OrderedFilterList.Add(filter); } - private void AddFilters(FilterProviderContext context, object filter, bool throwIfNotFilter) + private void ApplyFilterToContainer(object actualFilter, IFilter filterMetadata) { - bool shouldThrow = throwIfNotFilter; + Contract.Assert(actualFilter != null, "actualFilter should not be null"); + Contract.Assert(filterMetadata != null, "filterMetadata should not be null"); - var authFilter = filter as IAuthorizationFilter; - var actionFilter = filter as IActionFilter; - var actionResultFilter = filter as IActionResultFilter; - var exceptionFilter = filter as IExceptionFilter; + var container = actualFilter as IFilterContainer; - if (authFilter != null) + if (container != null) { - if (context.AuthorizationFilters == null) - { - context.AuthorizationFilters = new List(); - } - - context.AuthorizationFilters.Add(authFilter); - shouldThrow = false; - } - - if (actionFilter != null) - { - if (context.ActionFilters == null) - { - context.ActionFilters = new List(); - } - - context.ActionFilters.Add(actionFilter); - shouldThrow = false; - } - - if (actionResultFilter != null) - { - if (context.ActionResultFilters == null) - { - context.ActionResultFilters = new List(); - } - - context.ActionResultFilters.Add(actionResultFilter); - shouldThrow = false; - } - - if (exceptionFilter != null) - { - if (context.ExceptionFilters == null) - { - context.ExceptionFilters = new List(); - } - - context.ExceptionFilters.Add(exceptionFilter); - shouldThrow = false; - } - - if (shouldThrow) - { - throw new InvalidOperationException("Filter has to be IActionResultFilter, IActionFilter, IExceptionFilter or IAuthorizationFilter."); + container.FilterDefinition = filterMetadata; } } } diff --git a/src/Microsoft.AspNet.Mvc.Core/Filters/ExceptionFilterAttribute.cs b/src/Microsoft.AspNet.Mvc.Core/Filters/ExceptionFilterAttribute.cs index 9b1ec22493..20348788ce 100644 --- a/src/Microsoft.AspNet.Mvc.Core/Filters/ExceptionFilterAttribute.cs +++ b/src/Microsoft.AspNet.Mvc.Core/Filters/ExceptionFilterAttribute.cs @@ -4,7 +4,7 @@ using System.Threading.Tasks; namespace Microsoft.AspNet.Mvc { [AttributeUsage(AttributeTargets.Class | AttributeTargets.Method, AllowMultiple = true, Inherited = true)] - public abstract class ExceptionFilterAttribute : Attribute, IExceptionFilter, IFilter + public abstract class ExceptionFilterAttribute : Attribute, IExceptionFilter, IOrderedFilter { public abstract Task Invoke(ExceptionFilterContext context, Func next); diff --git a/src/Microsoft.AspNet.Mvc.Core/Filters/FilterDescriptor.cs b/src/Microsoft.AspNet.Mvc.Core/Filters/FilterDescriptor.cs index fab17e9d4e..edc6d518e4 100644 --- a/src/Microsoft.AspNet.Mvc.Core/Filters/FilterDescriptor.cs +++ b/src/Microsoft.AspNet.Mvc.Core/Filters/FilterDescriptor.cs @@ -1,6 +1,4 @@ -using System; - -namespace Microsoft.AspNet.Mvc +namespace Microsoft.AspNet.Mvc { public class FilterDescriptor { @@ -8,9 +6,19 @@ namespace Microsoft.AspNet.Mvc { Filter = filter; Scope = filterScope; + + var orderedFilter = Filter as IOrderedFilter; + + if (orderedFilter != null) + { + Order = orderedFilter.Order; + } } public IFilter Filter { get; private set; } + + public int Order { get; private set; } + public int Scope { get; private set; } } } diff --git a/src/Microsoft.AspNet.Mvc.Core/Filters/FilterDescriptorOrderComparer.cs b/src/Microsoft.AspNet.Mvc.Core/Filters/FilterDescriptorOrderComparer.cs index fc908b48d1..cf5edba086 100644 --- a/src/Microsoft.AspNet.Mvc.Core/Filters/FilterDescriptorOrderComparer.cs +++ b/src/Microsoft.AspNet.Mvc.Core/Filters/FilterDescriptorOrderComparer.cs @@ -10,13 +10,13 @@ namespace Microsoft.AspNet.Mvc public int Compare([NotNull]FilterDescriptor x, [NotNull]FilterDescriptor y) { - if (x.Filter.Order == y.Filter.Order) + if (x.Order == y.Order) { return x.Scope.CompareTo(y.Scope); } else { - return x.Filter.Order.CompareTo(y.Filter.Order); + return x.Order.CompareTo(y.Order); } } } diff --git a/src/Microsoft.AspNet.Mvc.Core/Filters/FilterProviderContext.cs b/src/Microsoft.AspNet.Mvc.Core/Filters/FilterProviderContext.cs index e563889810..a6b6e2b5c7 100644 --- a/src/Microsoft.AspNet.Mvc.Core/Filters/FilterProviderContext.cs +++ b/src/Microsoft.AspNet.Mvc.Core/Filters/FilterProviderContext.cs @@ -12,13 +12,7 @@ namespace Microsoft.AspNet.Mvc // Input public ActionDescriptor ActionDescriptor { get; set; } - // Results - public List AuthorizationFilters { get; set; } - - public List ActionFilters { get; set; } - - public List ActionResultFilters { get; set; } - - public List ExceptionFilters { get; set; } + // Result + public List OrderedFilterList { get; set; } } } diff --git a/src/Microsoft.AspNet.Mvc.Core/Filters/IFilter.cs b/src/Microsoft.AspNet.Mvc.Core/Filters/IFilter.cs index fbe64814fa..5491fdf902 100644 --- a/src/Microsoft.AspNet.Mvc.Core/Filters/IFilter.cs +++ b/src/Microsoft.AspNet.Mvc.Core/Filters/IFilter.cs @@ -2,6 +2,5 @@ namespace Microsoft.AspNet.Mvc { public interface IFilter { - int Order { get; } } } \ No newline at end of file diff --git a/src/Microsoft.AspNet.Mvc.Core/Filters/IFilterContainer.cs b/src/Microsoft.AspNet.Mvc.Core/Filters/IFilterContainer.cs new file mode 100644 index 0000000000..35a9720c26 --- /dev/null +++ b/src/Microsoft.AspNet.Mvc.Core/Filters/IFilterContainer.cs @@ -0,0 +1,7 @@ +namespace Microsoft.AspNet.Mvc.Filters +{ + public interface IFilterContainer + { + IFilter FilterDefinition { get; set; } + } +} diff --git a/src/Microsoft.AspNet.Mvc.Core/Filters/IOrderedFilter.cs b/src/Microsoft.AspNet.Mvc.Core/Filters/IOrderedFilter.cs new file mode 100644 index 0000000000..75491191c8 --- /dev/null +++ b/src/Microsoft.AspNet.Mvc.Core/Filters/IOrderedFilter.cs @@ -0,0 +1,7 @@ +namespace Microsoft.AspNet.Mvc +{ + public interface IOrderedFilter : IFilter + { + int Order { get; } + } +} diff --git a/src/Microsoft.AspNet.Mvc.Core/ReflectedActionInvoker.cs b/src/Microsoft.AspNet.Mvc.Core/ReflectedActionInvoker.cs index 88f21d6ba5..bda2832ad4 100644 --- a/src/Microsoft.AspNet.Mvc.Core/ReflectedActionInvoker.cs +++ b/src/Microsoft.AspNet.Mvc.Core/ReflectedActionInvoker.cs @@ -13,18 +13,20 @@ namespace Microsoft.AspNet.Mvc private readonly ActionContext _actionContext; private readonly ReflectedActionDescriptor _descriptor; private readonly IActionResultFactory _actionResultFactory; - private readonly IServiceProvider _serviceProvider; private readonly IControllerFactory _controllerFactory; private readonly IActionBindingContextProvider _bindingProvider; private readonly INestedProviderManager _filterProvider; + private readonly List _authorizationFilters = new List(); + private readonly List _actionFilters = new List(); + private readonly List _actionResultFilters = new List(); + public ReflectedActionInvoker(ActionContext actionContext, ReflectedActionDescriptor descriptor, IActionResultFactory actionResultFactory, IControllerFactory controllerFactory, IActionBindingContextProvider bindingContextProvider, - INestedProviderManager filterProvider, - IServiceProvider serviceProvider) + INestedProviderManager filterProvider) { _actionContext = actionContext; _descriptor = descriptor; @@ -32,15 +34,15 @@ namespace Microsoft.AspNet.Mvc _controllerFactory = controllerFactory; _bindingProvider = bindingContextProvider; _filterProvider = filterProvider; - - _serviceProvider = serviceProvider; } public async Task InvokeActionAsync() { IActionResult actionResult; - var context = new FilterProviderContext(_descriptor); - _filterProvider.Invoke(context); + var filterProviderContext = new FilterProviderContext(_descriptor); + _filterProvider.Invoke(filterProviderContext); + + PreArrangeFiltersInPipeline(filterProviderContext); var modelState = new ModelStateDictionary(); object controller = _controllerFactory.CreateController(_actionContext, modelState); @@ -61,15 +63,13 @@ namespace Microsoft.AspNet.Mvc { var parameterValues = await GetParameterValues(modelState); - var authZFilters = context.AuthorizationFilters; - - if (authZFilters != null && authZFilters.Count > 0) + if (_authorizationFilters.Count > 0) { var authZEndPoint = new AuthorizationFilterEndPoint(); - authZFilters.Add(authZEndPoint); + _authorizationFilters.Add(authZEndPoint); var authZContext = new AuthorizationFilterContext(_actionContext); - var authZPipeline = new FilterPipelineBuilder(authZFilters, authZContext); + var authZPipeline = new FilterPipelineBuilder(_authorizationFilters, authZContext); await authZPipeline.InvokeAsync(); @@ -92,7 +92,6 @@ namespace Microsoft.AspNet.Mvc if (actionResult == null) { - var actionFilters = context.ActionFilters ?? new List(); var actionFilterContext = new ActionFilterContext(_actionContext, parameterValues, method.ReturnType); @@ -101,9 +100,9 @@ namespace Microsoft.AspNet.Mvc var actionEndPoint = new ReflectedActionFilterEndPoint(async (inArray) => method.Invoke(controller, inArray), _actionResultFactory); - actionFilters.Add(actionEndPoint); + _actionFilters.Add(actionEndPoint); - var actionFilterPipeline = new FilterPipelineBuilder(actionFilters, + var actionFilterPipeline = new FilterPipelineBuilder(_actionFilters, actionFilterContext); await actionFilterPipeline.InvokeAsync(); @@ -113,12 +112,11 @@ namespace Microsoft.AspNet.Mvc } } - var actionResultFilters = context.ActionResultFilters ?? new List(); var actionResultFilterContext = new ActionResultFilterContext(_actionContext, actionResult); var actionResultFilterEndPoint = new ActionResultFilterEndPoint(); - actionResultFilters.Add(actionResultFilterEndPoint); + _actionResultFilters.Add(actionResultFilterEndPoint); - var actionResultPipeline = new FilterPipelineBuilder(actionResultFilters, actionResultFilterContext); + var actionResultPipeline = new FilterPipelineBuilder(_actionResultFilters, actionResultFilterContext); await actionResultPipeline.InvokeAsync(); } @@ -153,25 +151,41 @@ namespace Microsoft.AspNet.Mvc return parameterValues; } - private object[] GetArgumentValues(IDictionary parameterValues) + private void PreArrangeFiltersInPipeline(FilterProviderContext context) { - var parameters = _descriptor.MethodInfo.GetParameters(); - var arguments = new object[parameters.Length]; - - for (int i = 0; i < arguments.Length; i++) + if (context.OrderedFilterList == null || context.OrderedFilterList.Count == 0) { - object value; - if (parameterValues.TryGetValue(parameters[i].Name, out value)) - { - arguments[i] = value; - } - else - { - arguments[i] = parameters[i].DefaultValue; - } + return; } - return arguments; + foreach (var filter in context.OrderedFilterList) + { + PlaceFilter(filter); + } + } + + private void PlaceFilter(object filter) + { + var authFilter = filter as IAuthorizationFilter; + var actionFilter = filter as IActionFilter; + var actionResultFilter = filter as IActionResultFilter; + + // TODO: Exception filters + + if (authFilter != null) + { + _authorizationFilters.Add(authFilter); + } + + if (actionFilter != null) + { + _actionFilters.Add(actionFilter); + } + + if (actionResultFilter != null) + { + _actionResultFilters.Add(actionResultFilter); + } } } } diff --git a/src/Microsoft.AspNet.Mvc.Core/ReflectedActionInvokerProvider.cs b/src/Microsoft.AspNet.Mvc.Core/ReflectedActionInvokerProvider.cs index a580e2a755..798cfd52da 100644 --- a/src/Microsoft.AspNet.Mvc.Core/ReflectedActionInvokerProvider.cs +++ b/src/Microsoft.AspNet.Mvc.Core/ReflectedActionInvokerProvider.cs @@ -41,8 +41,7 @@ namespace Microsoft.AspNet.Mvc _actionResultFactory, _controllerFactory, _bindingProvider, - _filterProvider, - _serviceProvider); + _filterProvider); } callNext();