diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Internal/ControllerActionInvokerCache.cs b/src/Microsoft.AspNetCore.Mvc.Core/Internal/ControllerActionInvokerCache.cs index 7ac32a063f..402149b61a 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Internal/ControllerActionInvokerCache.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Internal/ControllerActionInvokerCache.cs @@ -45,96 +45,59 @@ namespace Microsoft.AspNetCore.Mvc.Internal } } - public Entry GetCacheEntry(ControllerContext controllerContext) + public ControllerActionInvokerState GetState(ControllerContext controllerContext) { + // Filter instances from statically defined filter descriptors + from filter providers + IFilterMetadata[] filters; + var cache = CurrentCache; var actionDescriptor = controllerContext.ActionDescriptor; - Entry entry; - if (cache.Entries.TryGetValue(actionDescriptor, out entry)) + Entry cacheEntry; + if (cache.Entries.TryGetValue(actionDescriptor, out cacheEntry)) { - return entry; + filters = GetFilters(controllerContext, cacheEntry.FilterItems); + + return new ControllerActionInvokerState(filters, cacheEntry.ActionMethodExecutor); } - var executor = ObjectMethodExecutor.Create(actionDescriptor.MethodInfo, actionDescriptor.ControllerTypeInfo); + var executor = ObjectMethodExecutor.Create( + actionDescriptor.MethodInfo, + actionDescriptor.ControllerTypeInfo); - var items = new List(actionDescriptor.FilterDescriptors.Count); + var staticFilterItems = new List(actionDescriptor.FilterDescriptors.Count); for (var i = 0; i < actionDescriptor.FilterDescriptors.Count; i++) { - items.Add(new FilterItem(actionDescriptor.FilterDescriptors[i])); + staticFilterItems.Add(new FilterItem(actionDescriptor.FilterDescriptors[i])); } - ExecuteProviders(controllerContext, items); + filters = GetFilters(controllerContext, staticFilterItems); - var filters = ExtractFilters(items); - - var allFiltersCached = true; - for (var i = 0; i < items.Count; i++) + // Cache the filter items based on the following criteria + // 1. Are created statically (ex: via filter attributes, added to global filter list etc.) + // 2. Are re-usable + for (var i = 0; i < staticFilterItems.Count; i++) { - var item = items[i]; + var item = staticFilterItems[i]; if (!item.IsReusable) { item.Filter = null; - allFiltersCached = false; } } + cacheEntry = new Entry(staticFilterItems, executor); + cache.Entries.TryAdd(actionDescriptor, cacheEntry); - if (allFiltersCached) - { - entry = new Entry(filters, null, executor); - } - else - { - entry = new Entry(null, items, executor); - } - - cache.Entries.TryAdd(actionDescriptor, entry); - return entry; + return new ControllerActionInvokerState(filters, cacheEntry.ActionMethodExecutor); } - public IFilterMetadata[] GetFilters(ControllerContext controllerContext) + private IFilterMetadata[] GetFilters(ActionContext actionContext, List staticFilterItems) { - var entry = GetCacheEntry(controllerContext); - return GetFiltersFromEntry(entry, controllerContext); - } + // Create a separate collection as we want to hold onto the statically defined filter items + // in order to cache them + var filterItems = new List(staticFilterItems); - public ObjectMethodExecutor GetControllerActionMethodExecutor(ControllerContext controllerContext) - { - var entry = GetCacheEntry(controllerContext); - return entry.ActionMethodExecutor; - } - - public IFilterMetadata[] GetFiltersFromEntry(Entry entry, ActionContext actionContext) - { - Debug.Assert(entry.Filters != null || entry.FilterItems != null); - - if (entry.Filters != null) - { - return entry.Filters; - } - - var items = new List(entry.FilterItems.Count); - for (var i = 0; i < entry.FilterItems.Count; i++) - { - var item = entry.FilterItems[i]; - if (item.IsReusable) - { - items.Add(item); - } - else - { - items.Add(new FilterItem(item.Descriptor)); - } - } - - ExecuteProviders(actionContext, items); - - return ExtractFilters(items); - } - - private void ExecuteProviders(ActionContext actionContext, List items) - { - var context = new FilterProviderContext(actionContext, items); + // Execute providers + var context = new FilterProviderContext(actionContext, filterItems); for (var i = 0; i < _filterProviders.Length; i++) { @@ -145,14 +108,12 @@ namespace Microsoft.AspNetCore.Mvc.Internal { _filterProviders[i].OnProvidersExecuted(context); } - } - private IFilterMetadata[] ExtractFilters(List items) - { + // Extract filter instances from statically defined filters and filter providers var count = 0; - for (var i = 0; i < items.Count; i++) + for (var i = 0; i < filterItems.Count; i++) { - if (items[i].Filter != null) + if (filterItems[i].Filter != null) { count++; } @@ -166,9 +127,9 @@ namespace Microsoft.AspNetCore.Mvc.Internal { var filters = new IFilterMetadata[count]; var filterIndex = 0; - for (int i = 0; i < items.Count; i++) + for (int i = 0; i < filterItems.Count; i++) { - var filter = items[i].Filter; + var filter = filterItems[i].Filter; if (filter != null) { filters[filterIndex++] = filter; @@ -186,25 +147,38 @@ namespace Microsoft.AspNetCore.Mvc.Internal Version = version; } - public ConcurrentDictionary Entries { get; } = + public ConcurrentDictionary Entries { get; } = new ConcurrentDictionary(); public int Version { get; } } - public struct Entry + private struct Entry { - public Entry(IFilterMetadata[] filters, List items, ObjectMethodExecutor executor) + public Entry(List items, ObjectMethodExecutor executor) { FilterItems = items; - Filters = filters; ActionMethodExecutor = executor; } - public IFilterMetadata[] Filters { get; } public List FilterItems { get; } public ObjectMethodExecutor ActionMethodExecutor { get; } } + + public struct ControllerActionInvokerState + { + public ControllerActionInvokerState( + IFilterMetadata[] filters, + ObjectMethodExecutor actionMethodExecutor) + { + Filters = filters; + ActionMethodExecutor = actionMethodExecutor; + } + + public IFilterMetadata[] Filters { get; } + + public ObjectMethodExecutor ActionMethodExecutor { get; set; } + } } } diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Internal/FilterActionInvoker.cs b/src/Microsoft.AspNetCore.Mvc.Core/Internal/FilterActionInvoker.cs index af924afce9..49ec7321ba 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Internal/FilterActionInvoker.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Internal/FilterActionInvoker.cs @@ -123,9 +123,9 @@ namespace Microsoft.AspNetCore.Mvc.Internal public virtual async Task InvokeAsync() { - var cacheEntry = _controllerActionInvokerCache.GetCacheEntry(Context); - _filters = _controllerActionInvokerCache.GetFiltersFromEntry(cacheEntry, Context); - _controllerActionMethodExecutor = cacheEntry.ActionMethodExecutor; + var controllerActionInvokerState = _controllerActionInvokerCache.GetState(Context); + _filters = controllerActionInvokerState.Filters; + _controllerActionMethodExecutor = controllerActionInvokerState.ActionMethodExecutor; _cursor = new FilterCursor(_filters); Context.ModelState.MaxAllowedErrors = _maxModelValidationErrors; diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Internal/FilterItemOrderComparer.cs b/src/Microsoft.AspNetCore.Mvc.Core/Internal/FilterItemOrderComparer.cs deleted file mode 100644 index 00f3e86316..0000000000 --- a/src/Microsoft.AspNetCore.Mvc.Core/Internal/FilterItemOrderComparer.cs +++ /dev/null @@ -1,34 +0,0 @@ -// Copyright (c) .NET Foundation. All rights reserved. -// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. - -using System; -using System.Collections.Generic; -using Microsoft.AspNetCore.Mvc.Filters; - -namespace Microsoft.AspNetCore.Mvc.Internal -{ - public class FilterItemOrderComparer : IComparer - { - private static readonly FilterItemOrderComparer _comparer = new FilterItemOrderComparer(); - - public static FilterItemOrderComparer Comparer - { - get { return _comparer; } - } - - public int Compare(FilterItem x, FilterItem y) - { - if (x == null) - { - throw new ArgumentNullException(nameof(x)); - } - - if (y == null) - { - throw new ArgumentNullException(nameof(y)); - } - - return FilterDescriptorOrderComparer.Comparer.Compare(x.Descriptor, y.Descriptor); - } - } -} diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ControllerActionInvokerCacheTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ControllerActionInvokerCacheTest.cs index 3aea1d4371..329d56e101 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ControllerActionInvokerCacheTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ControllerActionInvokerCacheTest.cs @@ -6,8 +6,8 @@ using System.Reflection; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc.Controllers; using Microsoft.AspNetCore.Mvc.Filters; +using Microsoft.AspNetCore.Mvc.Infrastructure; using Microsoft.AspNetCore.Routing; -using Microsoft.Extensions.DependencyInjection; using Xunit; namespace Microsoft.AspNetCore.Mvc.Internal @@ -18,198 +18,278 @@ namespace Microsoft.AspNetCore.Mvc.Internal public void GetFilters_CachesAllFilters() { // Arrange - var services = CreateServices(); - var cache = CreateCache(new DefaultFilterProvider()); - - var action = new ControllerActionDescriptor() - { - FilterDescriptors = new[] + var staticFilter1 = new TestFilter(); + var staticFilter2 = new TestFilter(); + var controllerContext = CreateControllerContext(new[] { - new FilterDescriptor(new TestFilter(), FilterScope.Action), - new FilterDescriptor(new TestFilter(), FilterScope.Action), - }, - MethodInfo = typeof(ControllerActionInvokerCache).GetMethod( - nameof(ControllerActionInvokerCache.GetControllerActionMethodExecutor)), - ControllerTypeInfo = typeof(ControllerActionInvokerCache).GetTypeInfo() - }; - - var context = new ControllerContext(new ActionContext( - new DefaultHttpContext(), - new RouteData(), - action)); + new FilterDescriptor(staticFilter1, FilterScope.Action), + new FilterDescriptor(staticFilter2, FilterScope.Action), + }); + var controllerActionInvokerCache = CreateControllerActionInvokerCache( + controllerContext, + new[] { new DefaultFilterProvider() }); // Act - 1 - var filters1 = cache.GetFilters(context); + var request1Filters = controllerActionInvokerCache.GetState(controllerContext).Filters; // Assert - 1 Assert.Collection( - filters1, - f => Assert.Same(action.FilterDescriptors[0].Filter, f), // Copied by provider - f => Assert.Same(action.FilterDescriptors[1].Filter, f)); // Copied by provider + request1Filters, + f => Assert.Same(staticFilter1, f), + f => Assert.Same(staticFilter2, f)); // Act - 2 - var filters2 = cache.GetFilters(context); - - Assert.Same(filters1, filters2); + var request2Filters = controllerActionInvokerCache.GetState(controllerContext).Filters; + // Assert - 2 Assert.Collection( - filters2, - f => Assert.Same(action.FilterDescriptors[0].Filter, f), // Cached - f => Assert.Same(action.FilterDescriptors[1].Filter, f)); // Cached + request2Filters, + f => Assert.Same(staticFilter1, f), + f => Assert.Same(staticFilter2, f)); } [Fact] public void GetFilters_CachesFilterFromFactory() { // Arrange - var services = CreateServices(); - var cache = CreateCache(new DefaultFilterProvider()); - - var action = new ControllerActionDescriptor() - { - FilterDescriptors = new[] + var staticFilter = new TestFilter(); + var controllerContext = CreateControllerContext(new[] { new FilterDescriptor(new TestFilterFactory() { IsReusable = true }, FilterScope.Action), - new FilterDescriptor(new TestFilter(), FilterScope.Action), - }, - MethodInfo = typeof(ControllerActionInvokerCache).GetMethod( - nameof(ControllerActionInvokerCache.GetControllerActionMethodExecutor)), - ControllerTypeInfo = typeof(ControllerActionInvokerCache).GetTypeInfo() - }; - - var context = new ControllerContext(new ActionContext( - new DefaultHttpContext(), - new RouteData(), - action)); + new FilterDescriptor(staticFilter, FilterScope.Action), + }); + var controllerActionInvokerCache = CreateControllerActionInvokerCache( + controllerContext, + new[] { new DefaultFilterProvider() }); + var filterDescriptors = controllerContext.ActionDescriptor.FilterDescriptors; // Act - 1 - var filters1 = cache.GetFilters(context); + var filters = controllerActionInvokerCache.GetState(controllerContext).Filters; // Assert - 1 - Assert.Collection( - filters1, - f => Assert.NotSame(action.FilterDescriptors[0].Filter, f), // Created by factory - f => Assert.Same(action.FilterDescriptors[1].Filter, f)); // Copied by provider + Assert.Equal(2, filters.Length); + var request1Filter1 = Assert.IsType(filters[0]); // Created by factory + Assert.Same(staticFilter, filters[1]); // Cached and the same statically created filter instance // Act - 2 - var filters2 = cache.GetFilters(context); - - Assert.Same(filters1, filters2); + filters = controllerActionInvokerCache.GetState(controllerContext).Filters; + // Assert - 2 Assert.Collection( - filters2, - f => Assert.Same(filters1[0], f), // Cached - f => Assert.Same(filters1[1], f)); // Cached + filters, + f => Assert.Same(request1Filter1, f), // Cached + f => Assert.Same(staticFilter, f)); // Cached and the same statically created filter instance } [Fact] public void GetFilters_DoesNotCacheFiltersWithIsReusableFalse() { // Arrange - var services = CreateServices(); - var cache = CreateCache(new DefaultFilterProvider()); - - var action = new ControllerActionDescriptor() - { - FilterDescriptors = new[] + var staticFilter = new TestFilter(); + var controllerContext = CreateControllerContext(new[] { new FilterDescriptor(new TestFilterFactory() { IsReusable = false }, FilterScope.Action), - new FilterDescriptor(new TestFilter(), FilterScope.Action), - }, - MethodInfo = typeof(ControllerActionInvokerCache).GetMethod( - nameof(ControllerActionInvokerCache.GetControllerActionMethodExecutor)), - ControllerTypeInfo = typeof(ControllerActionInvokerCache).GetTypeInfo() - }; - - var context = new ControllerContext(new ActionContext( - new DefaultHttpContext(), - new RouteData(), - action)); + new FilterDescriptor(staticFilter, FilterScope.Action), + }); + var controllerActionInvokerCache = CreateControllerActionInvokerCache( + controllerContext, + new[] { new DefaultFilterProvider() }); + var filterDescriptors = controllerContext.ActionDescriptor.FilterDescriptors; // Act - 1 - var filters1 = cache.GetFilters(context); + var filters = controllerActionInvokerCache.GetState(controllerContext).Filters; // Assert - 1 - Assert.Collection( - filters1, - f => Assert.NotSame(action.FilterDescriptors[0].Filter, f), // Created by factory - f => Assert.Same(action.FilterDescriptors[1].Filter, f)); // Copied by provider + Assert.Equal(2, filters.Length); + var request1Filter1 = Assert.IsType(filters[0]); // Created by factory + Assert.Same(staticFilter, filters[1]); // Cached and the same statically created filter instance // Act - 2 - var filters2 = cache.GetFilters(context); - - Assert.NotSame(filters1, filters2); + filters = controllerActionInvokerCache.GetState(controllerContext).Filters; + // Assert - 2 Assert.Collection( - filters2, - f => Assert.NotSame(filters1[0], f), // Created by factory (again) - f => Assert.Same(filters1[1], f)); // Cached + filters, + f => Assert.NotSame(request1Filter1, f), // Created by factory again + f => Assert.Same(staticFilter, f)); // Cached and the same statically created filter instance } [Fact] - public void GetControllerActionMethodExecutor_CachesExecutor() + public void GetControllerActionMethodExecutor_CachesActionMethodExecutor() { // Arrange - var services = CreateServices(); - var cache = CreateCache(new DefaultFilterProvider()); + var filter = new TestFilter(); + var controllerContext = CreateControllerContext(new[] + { + new FilterDescriptor(filter, FilterScope.Action) + }); + var controllerActionInvokerCache = CreateControllerActionInvokerCache( + controllerContext, + new[] { new DefaultFilterProvider() }); - var action = new ControllerActionDescriptor() - { - FilterDescriptors = new[] + // Act + var cacheEntry1 = controllerActionInvokerCache.GetState(controllerContext); + var cacheEntry2 = controllerActionInvokerCache.GetState(controllerContext); + + // Assert + Assert.Same(cacheEntry1.ActionMethodExecutor, cacheEntry2.ActionMethodExecutor); + } + + [Theory] + [InlineData(true)] + [InlineData(false)] + public void GetFilters_FiltersAddedByFilterProviders_AreNeverCached(bool reusable) + { + // Arrange + var customFilterProvider = new TestFilterProvider( + providerExecuting: (providerContext) => + { + var filter = new TestFilter(providerContext.ActionContext.HttpContext.Items["name"] as string); + providerContext.Results.Add( + new FilterItem(new FilterDescriptor(filter, FilterScope.Global), filter) + { + IsReusable = reusable + }); + }, + providerExecuted: null); + var staticFilter = new TestFilter(); + var controllerContext = CreateControllerContext(new[] { new FilterDescriptor(new TestFilterFactory() { IsReusable = false }, FilterScope.Action), - new FilterDescriptor(new TestFilter(), FilterScope.Action), - }, - MethodInfo = typeof(ControllerActionInvokerCache).GetMethod( - nameof(ControllerActionInvokerCache.GetControllerActionMethodExecutor)), - ControllerTypeInfo = typeof(ControllerActionInvokerCache).GetTypeInfo() - - }; - - var context = new ControllerContext( - new ActionContext(new DefaultHttpContext(), - new RouteData(), - action)); + new FilterDescriptor(staticFilter, FilterScope.Action), + }); + var controllerActionInvokerCache = CreateControllerActionInvokerCache( + controllerContext, + new IFilterProvider[] { new DefaultFilterProvider(), customFilterProvider }); + var filterDescriptors = controllerContext.ActionDescriptor.FilterDescriptors; // Act - 1 - var executor1 = cache.GetControllerActionMethodExecutor(context); + controllerContext.HttpContext.Items["name"] = "foo"; + var filters = controllerActionInvokerCache.GetState(controllerContext).Filters; - Assert.NotNull(executor1); - - var filters1 = cache.GetFilters(context); - - Assert.NotNull(filters1); + // Assert - 1 + Assert.Equal(3, filters.Length); + var request1Filter1 = Assert.IsType(filters[0]); // Created by factory + Assert.Same(staticFilter, filters[1]); // Cached and the same statically created filter instance + var request1Filter3 = Assert.IsType(filters[2]); // Created by custom filter provider + Assert.Equal("foo", request1Filter3.Data); // Act - 2 - var executor2 = cache.GetControllerActionMethodExecutor(context); + controllerContext.HttpContext.Items["name"] = "bar"; + filters = controllerActionInvokerCache.GetState(controllerContext).Filters; - Assert.Same(executor1, executor2); + // Assert -2 + Assert.Equal(3, filters.Length); + var request2Filter1 = Assert.IsType(filters[0]); + Assert.NotSame(request1Filter1, request2Filter1); // Created by factory + Assert.Same(staticFilter, filters[1]); // Cached and the same statically created filter instance + var request2Filter3 = Assert.IsType(filters[2]); + Assert.NotSame(request1Filter3, request2Filter3); // Created by custom filter provider again + Assert.Equal("bar", request2Filter3.Data); } private class TestFilter : IFilterMetadata { + public TestFilter() + { + } + + public TestFilter(string data) + { + Data = data; + } + + public string Data { get; } } private class TestFilterFactory : IFilterFactory { + private TestFilter testFilter; + public bool IsReusable { get; set; } public IFilterMetadata CreateInstance(IServiceProvider serviceProvider) { - return new TestFilter(); + if (IsReusable) + { + if (testFilter == null) + { + testFilter = new TestFilter(); + } + return testFilter; + } + else + { + return new TestFilter(); + } } } - private static IServiceProvider CreateServices() + private class TestFilterProvider : IFilterProvider { - return new ServiceCollection().BuildServiceProvider(); + private readonly Action _providerExecuting; + private readonly Action _providerExecuted; + + public TestFilterProvider( + Action providerExecuting, + Action providerExecuted, + int order = 0) + { + _providerExecuting = providerExecuting; + _providerExecuted = providerExecuted; + Order = order; + } + + public int Order { get; } + + public void OnProvidersExecuting(FilterProviderContext context) + { + _providerExecuting?.Invoke(context); + } + + public void OnProvidersExecuted(FilterProviderContext context) + { + _providerExecuted?.Invoke(context); + } } - private static ControllerActionInvokerCache CreateCache(params IFilterProvider[] providers) + private class TestController { - var services = CreateServices(); - var descriptorProvider = new ActionDescriptorCollectionProvider(services); - return new ControllerActionInvokerCache(descriptorProvider, providers); + public void Index() + { + } + } + + private class CustomActionDescriptorCollectionProvider : IActionDescriptorCollectionProvider + { + public CustomActionDescriptorCollectionProvider(ControllerActionDescriptor[] actionDescriptors) + { + ActionDescriptors = new ActionDescriptorCollection(actionDescriptors, version: 1); + } + + public ActionDescriptorCollection ActionDescriptors { get; } + } + + private static ControllerActionInvokerCache CreateControllerActionInvokerCache( + ControllerContext controllerContext, + IFilterProvider[] filterProviders) + { + var descriptorProvider = new CustomActionDescriptorCollectionProvider( + new[] { controllerContext.ActionDescriptor }); + return new ControllerActionInvokerCache(descriptorProvider, filterProviders); + } + + private static ControllerContext CreateControllerContext(FilterDescriptor[] filterDescriptors) + { + var actionDescriptor = new ControllerActionDescriptor() + { + FilterDescriptors = filterDescriptors, + MethodInfo = typeof(TestController).GetMethod(nameof(TestController.Index)), + ControllerTypeInfo = typeof(TestController).GetTypeInfo() + }; + + var actionContext = new ActionContext(new DefaultHttpContext(), new RouteData(), actionDescriptor); + return new ControllerContext(actionContext); } } }