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