Filters provided by filter providers are made to never be cached

[Fixes #4504] Possible double-execution of filter providers
aspnet · Apr 21, 2016 · 47013e5 · 47013e5
1 parent 683356e
commit 47013e5
Show file tree

Hide file tree

Showing 3 changed files with 115 additions and 43 deletions.
diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Internal/ControllerActionInvokerCache.cs b/src/Microsoft.AspNetCore.Mvc.Core/Internal/ControllerActionInvokerCache.cs
@@ -56,22 +56,28 @@ public Entry GetCacheEntry(ControllerContext controllerContext)
                 return entry;
             }
 
-            var executor = ObjectMethodExecutor.Create(actionDescriptor.MethodInfo, actionDescriptor.ControllerTypeInfo);
+            var executor = ObjectMethodExecutor.Create(
+                actionDescriptor.MethodInfo,
+                actionDescriptor.ControllerTypeInfo);
 
-            var items = new List<FilterItem>(actionDescriptor.FilterDescriptors.Count);
+            var staticallyDefinedFilterItems = new List<FilterItem>(actionDescriptor.FilterDescriptors.Count);
             for (var i = 0; i < actionDescriptor.FilterDescriptors.Count; i++)
             {
-                items.Add(new FilterItem(actionDescriptor.FilterDescriptors[i]));
+                staticallyDefinedFilterItems.Add(new FilterItem(actionDescriptor.FilterDescriptors[i]));
             }
 
-            ExecuteProviders(controllerContext, items);
+            var finalFilterItems = new List<FilterItem>(staticallyDefinedFilterItems);
+            ExecuteProviders(controllerContext, finalFilterItems);
 
-            var filters = ExtractFilters(items);
+            var filters = ExtractFilters(finalFilterItems);
 
+            // 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
             var allFiltersCached = true;
-            for (var i = 0; i < items.Count; i++)
+            for (var i = 0; i < staticallyDefinedFilterItems.Count; i++)
             {
-                var item = items[i];
+                var item = staticallyDefinedFilterItems[i];
                 if (!item.IsReusable)
                 {
                     item.Filter = null;
@@ -85,7 +91,7 @@ public Entry GetCacheEntry(ControllerContext controllerContext)
             }
             else
             {
-                entry = new Entry(null, items, executor);
+                entry = new Entry(null, staticallyDefinedFilterItems, executor);
             }
 
             cache.Entries.TryAdd(actionDescriptor, entry);
@@ -186,7 +192,7 @@ public InnerCache(int version)
                 Version = version;
             }
 
-            public ConcurrentDictionary<ActionDescriptor, Entry> Entries { get; } = 
+            public ConcurrentDictionary<ActionDescriptor, Entry> Entries { get; } =
                 new ConcurrentDictionary<ActionDescriptor, Entry>();
 
             public int Version { get; }

diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Internal/FilterItemOrderComparer.cs b/src/Microsoft.AspNetCore.Mvc.Core/Internal/FilterItemOrderComparer.cs
diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ControllerActionInvokerCacheTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ControllerActionInvokerCacheTest.cs
@@ -186,8 +186,80 @@ public void GetControllerActionMethodExecutor_CachesExecutor()
             Assert.Same(executor1, executor2);
         }
 
+        [Theory]
+        [InlineData(true)]
+        [InlineData(false)]
+        public void GetFilters_FiltersFromFilterProviders_AreNeverCached(bool reusable)
+        {
+            // Arrange
+            var services = CreateServices();
+            var cache = CreateCache(
+                new DefaultFilterProvider(),
+                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 action = new ControllerActionDescriptor()
+            {
+                FilterDescriptors = 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));
+            context.HttpContext.Items.Add("name", "foo");
+
+            // Act - 1
+            var filters1 = cache.GetFilters(context);
+
+            // 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
+                f => Assert.Equal("foo", ((TestFilter)filters1[2]).Data));  // Created by provider
+
+            // Act - 2
+            context.HttpContext.Items["name"] = "bar";
+            var filters2 = cache.GetFilters(context);
+
+            Assert.NotSame(filters1, filters2);
+
+            Assert.Collection(
+                filters2,
+                f => Assert.NotSame(filters1[0], f),                        // Created by factory (again)
+                f => Assert.Same(action.FilterDescriptors[1].Filter, f),    // Cached
+                f => Assert.Equal("bar", ((TestFilter)filters2[2]).Data));  // Created by provider (again)
+        }
+
         private class TestFilter : IFilterMetadata
         {
+            public TestFilter()
+            {
+            }
+
+            public TestFilter(string data)
+            {
+                Data = data;
+            }
+
+            public string Data { get; }
         }
 
         private class TestFilterFactory : IFilterFactory
@@ -200,6 +272,34 @@ public IFilterMetadata CreateInstance(IServiceProvider serviceProvider)
             }
         }
 
+        private class TestFilterProvider : IFilterProvider
+        {
+            private readonly Action<FilterProviderContext> _providerExecuting;
+            private readonly Action<FilterProviderContext> _providerExecuted;
+
+            public TestFilterProvider(
+                Action<FilterProviderContext> providerExecuting,
+                Action<FilterProviderContext> 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 IServiceProvider CreateServices()
         {
             return new ServiceCollection().BuildServiceProvider();