From f573b8840a0473ff95cd69894ea1f0a6cf1daaa9 Mon Sep 17 00:00:00 2001 From: Ryan Nowak Date: Thu, 6 Sep 2018 17:05:11 -0700 Subject: [PATCH] Fix aspnet/Routing#782 Currently MVC is still running the IActionConstraint implementations for features that we've already moved into the routing layer. This has a significant perf cost associated with, and so we want to skip it because it's redundant. However if anyone has implemented their own `IActionConstraint`-based features, they still need to just work. This change takes the approach of skipping the action constraint phase at runtime unless we see something 'unknown'. This is an all or nothing choice, and will run action constraints if **any** action constraint we don't special case exists. This is the most compatible behavior (running redundant constraints) when the application is using constraints that the developer implemented. Another approach I considered was to eliminate these constraints as part of the process of building ADs. I don't think that's ideal because people have written code that introspects action constraints. We should consider something like this in 3.0. --- .../Internal/ActionConstraintCache.cs | 31 +++++++- .../Routing/ActionConstraintMatcherPolicy.cs | 6 +- .../CorsHttpMethodActionConstraint.cs | 1 + .../ActionConstraintMatcherPolicyTest.cs | 75 ++++++++++++++++++- 4 files changed, 108 insertions(+), 5 deletions(-) rename test/{Microsoft.AspNetCore.Mvc.Core.Test => Microsoft.AspNetCore.Mvc.Test}/Routing/ActionConstraintMatcherPolicyTest.cs (86%) diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Internal/ActionConstraintCache.cs b/src/Microsoft.AspNetCore.Mvc.Core/Internal/ActionConstraintCache.cs index ff50a463b0..e8bf938aef 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Internal/ActionConstraintCache.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Internal/ActionConstraintCache.cs @@ -192,14 +192,43 @@ namespace Microsoft.AspNetCore.Mvc.Internal for (var i = 0; i < _actions.Items.Count; i++) { var action = _actions.Items[i]; - if (action.ActionConstraints?.Count > 0) + if (action.ActionConstraints?.Count > 0 && HasSignificantActionConstraint(action)) { + // We need to check for some specific action constraint implementations. + // We've implemented consumes, and HTTP method support inside endpoint routing, so + // we don't need to run an 'action constraint phase' if those are the only constraints. found = true; break; } } _hasActionConstraints = found; + + bool HasSignificantActionConstraint(ActionDescriptor action) + { + for (var i = 0; i < action.ActionConstraints.Count; i++) + { + var actionConstraint = action.ActionConstraints[i]; + if (actionConstraint.GetType() == typeof(HttpMethodActionConstraint)) + { + // This one is OK, we implement this in endpoint routing. + } + else if (actionConstraint.GetType().FullName == "Microsoft.AspNetCore.Mvc.Cors.Internal.CorsHttpMethodActionConstraint") + { + // This one is OK, we implement this in endpoint routing. + } + else if (actionConstraint.GetType() == typeof(ConsumesAttribute)) + { + // This one is OK, we implement this in endpoint routing. + } + else + { + return true; + } + } + + return false; + } } return _hasActionConstraints.Value; diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Routing/ActionConstraintMatcherPolicy.cs b/src/Microsoft.AspNetCore.Mvc.Core/Routing/ActionConstraintMatcherPolicy.cs index f858f4fe1e..e03640b59c 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Routing/ActionConstraintMatcherPolicy.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Routing/ActionConstraintMatcherPolicy.cs @@ -35,6 +35,9 @@ namespace Microsoft.AspNetCore.Mvc.Routing // Run really late. public override int Order => 100000; + // Internal for testing + internal bool ShouldRunActionConstraints => _actionConstraintCache.CurrentCache.HasActionConstraints; + public Task ApplyAsync(HttpContext httpContext, EndpointFeature endpointFeature, CandidateSet candidateSet) { // PERF: we can skip over action constraints if there aren't any app-wide. @@ -42,8 +45,7 @@ namespace Microsoft.AspNetCore.Mvc.Routing // Running action constraints (or just checking for them) in a candidate set // is somewhat expensive compared to other routing operations. This should only // happen if user-code adds action constraints. - var actions = _actionConstraintCache.CurrentCache; - if (actions.HasActionConstraints) + if (ShouldRunActionConstraints) { ApplyActionConstraints(httpContext, candidateSet); } diff --git a/src/Microsoft.AspNetCore.Mvc.Cors/Internal/CorsHttpMethodActionConstraint.cs b/src/Microsoft.AspNetCore.Mvc.Cors/Internal/CorsHttpMethodActionConstraint.cs index 71ba2d696c..de3ff06468 100644 --- a/src/Microsoft.AspNetCore.Mvc.Cors/Internal/CorsHttpMethodActionConstraint.cs +++ b/src/Microsoft.AspNetCore.Mvc.Cors/Internal/CorsHttpMethodActionConstraint.cs @@ -10,6 +10,7 @@ using Microsoft.AspNetCore.Mvc.Internal; namespace Microsoft.AspNetCore.Mvc.Cors.Internal { + // Don't casually change the name of this. We reference the full type name in ActionConstraintCache. public class CorsHttpMethodActionConstraint : HttpMethodActionConstraint { private readonly string OriginHeader = "Origin"; diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Routing/ActionConstraintMatcherPolicyTest.cs b/test/Microsoft.AspNetCore.Mvc.Test/Routing/ActionConstraintMatcherPolicyTest.cs similarity index 86% rename from test/Microsoft.AspNetCore.Mvc.Core.Test/Routing/ActionConstraintMatcherPolicyTest.cs rename to test/Microsoft.AspNetCore.Mvc.Test/Routing/ActionConstraintMatcherPolicyTest.cs index 5936d34ef6..9889b12e1b 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/Routing/ActionConstraintMatcherPolicyTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Test/Routing/ActionConstraintMatcherPolicyTest.cs @@ -7,16 +7,17 @@ using System.Threading.Tasks; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc.Abstractions; using Microsoft.AspNetCore.Mvc.ActionConstraints; +using Microsoft.AspNetCore.Mvc.Cors.Internal; using Microsoft.AspNetCore.Mvc.Infrastructure; using Microsoft.AspNetCore.Mvc.Internal; using Microsoft.AspNetCore.Routing; using Microsoft.AspNetCore.Routing.Matching; -using Microsoft.AspNetCore.Routing.Patterns; using Moq; using Xunit; namespace Microsoft.AspNetCore.Mvc.Routing { + // These tests are intentionally in Mvc.Test so we can also test the CORS action constraint. public class ActionConstraintMatcherPolicyTest { [Fact] @@ -49,7 +50,7 @@ namespace Microsoft.AspNetCore.Mvc.Routing { ActionConstraints = new List() { - new HttpMethodActionConstraint(new string[] { "POST" }), + new BooleanConstraint() { Pass = true, }, }, Parameters = new List(), }; @@ -336,6 +337,76 @@ namespace Microsoft.AspNetCore.Mvc.Routing Assert.False(candidateSet[2].IsValidCandidate); } + [Fact] + public void ShouldRunActionConstraints_IgnoresIgnorableConstraints() + { + // Arrange + var actions = new ActionDescriptor[] + { + new ActionDescriptor() + { + + }, + new ActionDescriptor() + { + ActionConstraints = new List() + { + new HttpMethodActionConstraint(new[]{ "GET", }), + }, + }, + new ActionDescriptor() + { + ActionConstraints = new List() + { + new ConsumesAttribute("text/json"), + }, + }, + new ActionDescriptor() + { + ActionConstraints = new List() + { + new CorsHttpMethodActionConstraint(new HttpMethodActionConstraint(new[]{ "GET", })), + }, + }, + }; + + var selector = CreateSelector(actions); + + // Act + var result = selector.ShouldRunActionConstraints; + + // Assert + Assert.False(result); + } + + [Fact] + public void ShouldRunActionConstraints_RunsForArbitraryActionConstraint() + { + // Arrange + var actions = new ActionDescriptor[] + { + new ActionDescriptor() + { + + }, + new ActionDescriptor() + { + ActionConstraints = new List() + { + new BooleanConstraint(), + }, + }, + }; + + var selector = CreateSelector(actions); + + // Act + var result = selector.ShouldRunActionConstraints; + + // Assert + Assert.True(result); + } + private ActionConstraintMatcherPolicy CreateSelector(ActionDescriptor[] actions) { // We need to actually provide some actions with some action constraints metadata