diff --git a/build/dependencies.props b/build/dependencies.props index c8c3ed64f1..aca99b4e4b 100644 --- a/build/dependencies.props +++ b/build/dependencies.props @@ -47,8 +47,8 @@ 2.2.0-preview3-35359 2.2.0-preview3-35359 2.2.0-preview3-35359 - 2.2.0-a-preview3-endpoint-selector-17041 - 2.2.0-a-preview3-endpoint-selector-17041 + 2.2.0-a-preview3-matcher-policy-17051 + 2.2.0-a-preview3-matcher-policy-17051 2.2.0-preview3-35359 2.2.0-preview3-35359 2.2.0-preview3-35359 diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Internal/ActionConstraintCache.cs b/src/Microsoft.AspNetCore.Mvc.Core/Internal/ActionConstraintCache.cs index e8bf938aef..384d48bffa 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Internal/ActionConstraintCache.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Internal/ActionConstraintCache.cs @@ -150,10 +150,10 @@ namespace Microsoft.AspNetCore.Mvc.Internal { return null; } - + var actionConstraints = new IActionConstraint[count]; var actionConstraintIndex = 0; - for (int i = 0; i < items.Count; i++) + for (var i = 0; i < items.Count; i++) { var actionConstraint = items[i].Constraint; if (actionConstraint != null) @@ -168,7 +168,6 @@ namespace Microsoft.AspNetCore.Mvc.Internal internal class InnerCache { private readonly ActionDescriptorCollection _actions; - private bool? _hasActionConstraints; public InnerCache(ActionDescriptorCollection actions) { @@ -179,61 +178,6 @@ namespace Microsoft.AspNetCore.Mvc.Internal new ConcurrentDictionary(); public int Version => _actions.Version; - - public bool HasActionConstraints - { - get - { - // This is a safe race-condition, since it always transitions from null to non-null. - // All writers will always get the same result. - if (_hasActionConstraints == null) - { - var found = false; - for (var i = 0; i < _actions.Items.Count; i++) - { - var action = _actions.Items[i]; - 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; - } - } } internal readonly struct CacheEntry diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Routing/ActionConstraintMatcherPolicy.cs b/src/Microsoft.AspNetCore.Mvc.Core/Routing/ActionConstraintMatcherPolicy.cs index 488b15646b..d31e06daf4 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Routing/ActionConstraintMatcherPolicy.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Routing/ActionConstraintMatcherPolicy.cs @@ -11,7 +11,6 @@ using Microsoft.AspNetCore.Mvc.ActionConstraints; using Microsoft.AspNetCore.Mvc.Internal; using Microsoft.AspNetCore.Routing; using Microsoft.AspNetCore.Routing.Matching; -using Microsoft.Extensions.Logging; namespace Microsoft.AspNetCore.Mvc.Routing { @@ -24,7 +23,7 @@ namespace Microsoft.AspNetCore.Mvc.Routing // We need to be able to run IActionConstraints on Endpoints that aren't associated // with an action. This is a sentinel value we use when the endpoint isn't from MVC. internal static readonly ActionDescriptor NonAction = new ActionDescriptor(); - + private readonly ActionConstraintCache _actionConstraintCache; public ActionConstraintMatcherPolicy(ActionConstraintCache actionConstraintCache) @@ -35,27 +34,59 @@ 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, EndpointSelectorContext context, CandidateSet candidateSet) + public bool AppliesToEndpoints(IReadOnlyList endpoints) { - // PERF: we can skip over action constraints if there aren't any app-wide. - // - // 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. - if (ShouldRunActionConstraints) + if (endpoints == null) { - ApplyActionConstraints(httpContext, candidateSet); + throw new ArgumentNullException(nameof(endpoints)); } - return Task.CompletedTask; + // We can skip over action constraints when they aren't any for this set + // of endpoints. This happens once on startup so it removes this component + // from the code path in most scenarios. + for (var i = 0; i < endpoints.Count; i++) + { + var endpoint = endpoints[i]; + var action = endpoint.Metadata.GetMetadata(); + 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. + return true; + } + } + + return false; + + bool HasSignificantActionConstraint(ActionDescriptor a) + { + for (var i = 0; i < a.ActionConstraints.Count; i++) + { + var actionConstraint = a.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; + } } - - private void ApplyActionConstraints( - HttpContext httpContext, - CandidateSet candidateSet) + + public Task ApplyAsync(HttpContext httpContext, EndpointSelectorContext context, CandidateSet candidateSet) { var finalMatches = EvaluateActionConstraints(httpContext, candidateSet); @@ -74,6 +105,8 @@ namespace Microsoft.AspNetCore.Mvc.Routing candidateSet.SetValidity(finalMatches[i].index, true); } } + + return Task.CompletedTask; } // This is almost the same as the code in ActionSelector, but we can't really share the logic @@ -171,8 +204,10 @@ namespace Microsoft.AspNetCore.Mvc.Routing var endpointsWithConstraint = new List<(int index, ActionSelectorCandidate candidate)>(); var endpointsWithoutConstraint = new List<(int index, ActionSelectorCandidate candidate)>(); - var constraintContext = new ActionConstraintContext(); - constraintContext.Candidates = items.Select(i => i.candidate).ToArray(); + var constraintContext = new ActionConstraintContext + { + Candidates = items.Select(i => i.candidate).ToArray() + }; // Perf: Avoid allocations for (var i = 0; i < items.Count; i++) diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Routing/ConsumesMatcherPolicy.cs b/src/Microsoft.AspNetCore.Mvc.Core/Routing/ConsumesMatcherPolicy.cs index 65dd1d117b..c0f0dd35bd 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Routing/ConsumesMatcherPolicy.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Routing/ConsumesMatcherPolicy.cs @@ -23,7 +23,7 @@ namespace Microsoft.AspNetCore.Mvc.Routing public IComparer Comparer { get; } = new ConsumesMetadataEndpointComparer(); - public bool AppliesToNode(IReadOnlyList endpoints) + public bool AppliesToEndpoints(IReadOnlyList endpoints) { if (endpoints == null) { @@ -173,7 +173,7 @@ namespace Microsoft.AspNetCore.Mvc.Routing return new ConsumesPolicyJumpTable(exitDestination, ordered); } - private int GetScore(MediaType mediaType) + private int GetScore(in MediaType mediaType) { // Higher score == lower priority - see comments on MediaType. if (mediaType.MatchesAllTypes) diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Routing/ConsumesMatcherPolicyTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/Routing/ConsumesMatcherPolicyTest.cs index cad3edc0b9..b0a59ce678 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/Routing/ConsumesMatcherPolicyTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/Routing/ConsumesMatcherPolicyTest.cs @@ -16,7 +16,7 @@ namespace Microsoft.AspNetCore.Mvc.Routing public class ConsumesMatcherPolicyTest { [Fact] - public void AppliesToNode_EndpointWithoutMetadata_ReturnsFalse() + public void AppliesToEndpoints_EndpointWithoutMetadata_ReturnsFalse() { // Arrange var endpoints = new[] { CreateEndpoint("/", null), }; @@ -24,14 +24,14 @@ namespace Microsoft.AspNetCore.Mvc.Routing var policy = CreatePolicy(); // Act - var result = policy.AppliesToNode(endpoints); + var result = policy.AppliesToEndpoints(endpoints); // Assert Assert.False(result); } [Fact] - public void AppliesToNode_EndpointWithoutContentTypes_ReturnsFalse() + public void AppliesToEndpoints_EndpointWithoutContentTypes_ReturnsFalse() { // Arrange var endpoints = new[] @@ -42,14 +42,14 @@ namespace Microsoft.AspNetCore.Mvc.Routing var policy = CreatePolicy(); // Act - var result = policy.AppliesToNode(endpoints); + var result = policy.AppliesToEndpoints(endpoints); // Assert Assert.False(result); } [Fact] - public void AppliesToNode_EndpointHasContentTypes_ReturnsTrue() + public void AppliesToEndpoints_EndpointHasContentTypes_ReturnsTrue() { // Arrange var endpoints = new[] @@ -61,7 +61,7 @@ namespace Microsoft.AspNetCore.Mvc.Routing var policy = CreatePolicy(); // Act - var result = policy.AppliesToNode(endpoints); + var result = policy.AppliesToEndpoints(endpoints); // Assert Assert.True(result); diff --git a/test/Microsoft.AspNetCore.Mvc.Test/Routing/ActionConstraintMatcherPolicyTest.cs b/test/Microsoft.AspNetCore.Mvc.Test/Routing/ActionConstraintMatcherPolicyTest.cs index 5210180832..b21072b853 100644 --- a/test/Microsoft.AspNetCore.Mvc.Test/Routing/ActionConstraintMatcherPolicyTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Test/Routing/ActionConstraintMatcherPolicyTest.cs @@ -338,7 +338,7 @@ namespace Microsoft.AspNetCore.Mvc.Routing } [Fact] - public void ShouldRunActionConstraints_IgnoresIgnorableConstraints() + public void AppliesToEndpoints_IgnoresIgnorableConstraints() { // Arrange var actions = new ActionDescriptor[] @@ -369,11 +369,12 @@ namespace Microsoft.AspNetCore.Mvc.Routing }, }, }; + var endpoints = actions.Select(CreateEndpoint).ToArray(); var selector = CreateSelector(actions); // Act - var result = selector.ShouldRunActionConstraints; + var result = selector.AppliesToEndpoints(endpoints); // Assert Assert.False(result); @@ -397,11 +398,12 @@ namespace Microsoft.AspNetCore.Mvc.Routing }, }, }; + var endpoints = actions.Select(CreateEndpoint).ToArray(); var selector = CreateSelector(actions); // Act - var result = selector.ShouldRunActionConstraints; + var result = selector.AppliesToEndpoints(endpoints); // Assert Assert.True(result);