From 105f8b47a15299de690dcced925ea0134888fab1 Mon Sep 17 00:00:00 2001 From: James Newton-King Date: Wed, 12 Sep 2018 21:16:50 +1200 Subject: [PATCH] Fix endpoint support for area/controller/action in attribute route (#8447) --- .../Builder/MvcEndpointInfo.cs | 27 ++- .../Internal/MvcEndpointDataSource.cs | 197 ++++++++++-------- .../Internal/MvcEndpointDataSourceTests.cs | 91 ++++++-- 3 files changed, 200 insertions(+), 115 deletions(-) diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Builder/MvcEndpointInfo.cs b/src/Microsoft.AspNetCore.Mvc.Core/Builder/MvcEndpointInfo.cs index 8b1eba5fbf..161189b871 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Builder/MvcEndpointInfo.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Builder/MvcEndpointInfo.cs @@ -28,7 +28,7 @@ namespace Microsoft.AspNetCore.Builder // Data we parse from the pattern will be used to fill in the rest of the constraints or // defaults. The parser will throw for invalid routes. ParsedPattern = RoutePatternFactory.Parse(pattern, defaults, constraints); - Constraints = BuildConstraints(parameterPolicyFactory); + ParameterPolicies = BuildParameterPolicies(ParsedPattern.Parameters, parameterPolicyFactory); Defaults = defaults; // Merge defaults outside of RoutePattern because the defaults will already have values from pattern @@ -50,33 +50,30 @@ namespace Microsoft.AspNetCore.Builder // Inline and non-inline defaults merged into one public RouteValueDictionary MergedDefaults { get; } - public IDictionary> Constraints { get; } + public IDictionary> ParameterPolicies { get; } public RouteValueDictionary DataTokens { get; } public RoutePattern ParsedPattern { get; private set; } - private Dictionary> BuildConstraints(ParameterPolicyFactory parameterPolicyFactory) + internal static Dictionary> BuildParameterPolicies(IReadOnlyList parameters, ParameterPolicyFactory parameterPolicyFactory) { - var constraints = new Dictionary>(StringComparer.OrdinalIgnoreCase); + var policies = new Dictionary>(StringComparer.OrdinalIgnoreCase); - foreach (var parameter in ParsedPattern.Parameters) + foreach (var parameter in parameters) { foreach (var parameterPolicy in parameter.ParameterPolicies) { var createdPolicy = parameterPolicyFactory.Create(parameter, parameterPolicy); - if (createdPolicy is IRouteConstraint routeConstraint) + if (!policies.TryGetValue(parameter.Name, out var policyList)) { - if (!constraints.TryGetValue(parameter.Name, out var paramConstraints)) - { - paramConstraints = new List(); - constraints.Add(parameter.Name, paramConstraints); - } - - paramConstraints.Add(routeConstraint); + policyList = new List(); + policies.Add(parameter.Name, policyList); } + + policyList.Add(createdPolicy); } } - return constraints; + return policies; } } -} +} \ No newline at end of file diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Internal/MvcEndpointDataSource.cs b/src/Microsoft.AspNetCore.Mvc.Core/Internal/MvcEndpointDataSource.cs index 9c1e7dd801..82026635ca 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Internal/MvcEndpointDataSource.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Internal/MvcEndpointDataSource.cs @@ -9,7 +9,6 @@ using System.Text; using System.Threading; using Microsoft.AspNetCore.Builder; using Microsoft.AspNetCore.Http; -using Microsoft.AspNetCore.Http.Features; using Microsoft.AspNetCore.Mvc.Abstractions; using Microsoft.AspNetCore.Mvc.Infrastructure; using Microsoft.AspNetCore.Mvc.Routing; @@ -113,7 +112,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal // // Start with an order of '1' for conventional routes as attribute routes have a default order of '0'. // This is for scenarios dealing with migrating existing Router based code to Endpoint Routing world. - var conventionalRouteOrder = 0; + var conventionalRouteOrder = 1; // Check each of the conventional patterns to see if the action would be reachable // If the action and pattern are compatible then create an endpoint with the @@ -143,90 +142,38 @@ namespace Microsoft.AspNetCore.Mvc.Internal continue; } - var newPathSegments = endpointInfo.ParsedPattern.PathSegments.ToList(); - - for (var i = 0; i < newPathSegments.Count; i++) - { - // Check if the pattern can be shortened because the remaining parameters are optional - // - // e.g. Matching pattern {controller=Home}/{action=Index}/{id?} against HomeController.Index - // can resolve to the following endpoints: - // - /Home/Index/{id?} - // - /Home - // - / - if (UseDefaultValuePlusRemainingSegmentsOptional(i, action, endpointInfo, newPathSegments)) - { - var subPathSegments = newPathSegments.Take(i); - - var subEndpoint = CreateEndpoint( - action, - endpointInfo.Name, - GetPattern(ref patternStringBuilder, subPathSegments), - subPathSegments, - endpointInfo.Defaults, - ++conventionalRouteOrder, - endpointInfo, - endpointInfo.DataTokens, - suppressLinkGeneration: false, - suppressPathMatching: false); - endpoints.Add(subEndpoint); - } - - List segmentParts = null; // Initialize only as needed - var segment = newPathSegments[i]; - for (var j = 0; j < segment.Parts.Count; j++) - { - var part = segment.Parts[j]; - - if (part.IsParameter && - part is RoutePatternParameterPart parameterPart && - action.RouteValues.ContainsKey(parameterPart.Name)) - { - if (segmentParts == null) - { - segmentParts = segment.Parts.ToList(); - } - - // Replace parameter with literal value - segmentParts[j] = RoutePatternFactory.LiteralPart(action.RouteValues[parameterPart.Name]); - } - } - - // A parameter part was replaced so replace segment with updated parts - if (segmentParts != null) - { - newPathSegments[i] = RoutePatternFactory.Segment(segmentParts); - } - } - - var endpoint = CreateEndpoint( + conventionalRouteOrder = CreateEndpoints( + endpoints, + ref patternStringBuilder, action, - endpointInfo.Name, - GetPattern(ref patternStringBuilder, newPathSegments), - newPathSegments, + conventionalRouteOrder, + endpointInfo.ParsedPattern, + endpointInfo.MergedDefaults, endpointInfo.Defaults, - ++conventionalRouteOrder, - endpointInfo, + endpointInfo.Name, endpointInfo.DataTokens, + endpointInfo.ParameterPolicies, suppressLinkGeneration: false, suppressPathMatching: false); - endpoints.Add(endpoint); } } else { - var endpoint = CreateEndpoint( + var attributeRoutePattern = RoutePatternFactory.Parse(action.AttributeRouteInfo.Template); + + CreateEndpoints( + endpoints, + ref patternStringBuilder, action, - action.AttributeRouteInfo.Name, - action.AttributeRouteInfo.Template, - RoutePatternFactory.Parse(action.AttributeRouteInfo.Template).PathSegments, - nonInlineDefaults: null, action.AttributeRouteInfo.Order, - action.AttributeRouteInfo, + attributeRoutePattern, + attributeRoutePattern.Defaults, + nonInlineDefaults: null, + action.AttributeRouteInfo.Name, dataTokens: null, - suppressLinkGeneration: action.AttributeRouteInfo.SuppressLinkGeneration, - suppressPathMatching: action.AttributeRouteInfo.SuppressPathMatching); - endpoints.Add(endpoint); + allParameterPolicies: null, + action.AttributeRouteInfo.SuppressLinkGeneration, + action.AttributeRouteInfo.SuppressPathMatching); } } @@ -246,6 +193,93 @@ namespace Microsoft.AspNetCore.Mvc.Internal // Step 4 - trigger old token oldCancellationTokenSource?.Cancel(); } + } + + // CreateEndpoints processes the route pattern, replacing area/controller/action parameters with endpoint values + // Because of default values it is possible for a route pattern to resolve to multiple endpoints + private int CreateEndpoints( + List endpoints, + ref StringBuilder patternStringBuilder, + ActionDescriptor action, + int routeOrder, + RoutePattern routePattern, + IReadOnlyDictionary allDefaults, + IReadOnlyDictionary nonInlineDefaults, + string name, + RouteValueDictionary dataTokens, + IDictionary> allParameterPolicies, + bool suppressLinkGeneration, + bool suppressPathMatching) + { + var newPathSegments = routePattern.PathSegments.ToList(); + + for (var i = 0; i < newPathSegments.Count; i++) + { + // Check if the pattern can be shortened because the remaining parameters are optional + // + // e.g. Matching pattern {controller=Home}/{action=Index}/{id?} against HomeController.Index + // can resolve to the following endpoints: + // - /Home/Index/{id?} + // - /Home + // - / + if (UseDefaultValuePlusRemainingSegmentsOptional(i, action, allDefaults, newPathSegments)) + { + var subPathSegments = newPathSegments.Take(i); + + var subEndpoint = CreateEndpoint( + action, + name, + GetPattern(ref patternStringBuilder, subPathSegments), + subPathSegments, + nonInlineDefaults, + routeOrder++, + dataTokens, + suppressLinkGeneration, + suppressPathMatching); + endpoints.Add(subEndpoint); + } + + List segmentParts = null; // Initialize only as needed + var segment = newPathSegments[i]; + for (var j = 0; j < segment.Parts.Count; j++) + { + var part = segment.Parts[j]; + + if (part.IsParameter && + part is RoutePatternParameterPart parameterPart && + action.RouteValues.ContainsKey(parameterPart.Name)) + { + if (segmentParts == null) + { + segmentParts = segment.Parts.ToList(); + } + + var parameterRouteValue = action.RouteValues[parameterPart.Name]; + + segmentParts[j] = RoutePatternFactory.LiteralPart(parameterRouteValue); + } + } + + // A parameter part was replaced so replace segment with updated parts + if (segmentParts != null) + { + newPathSegments[i] = RoutePatternFactory.Segment(segmentParts); + } + } + + var endpoint = CreateEndpoint( + action, + name, + GetPattern(ref patternStringBuilder, newPathSegments), + newPathSegments, + nonInlineDefaults, + routeOrder++, + dataTokens, + suppressLinkGeneration, + suppressPathMatching); + endpoints.Add(endpoint); + + return routeOrder; string GetPattern(ref StringBuilder sb, IEnumerable segments) { @@ -265,7 +299,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal private bool UseDefaultValuePlusRemainingSegmentsOptional( int segmentIndex, ActionDescriptor action, - MvcEndpointInfo endpointInfo, + IReadOnlyDictionary allDefaults, List pathSegments) { // Check whether the remaining segments are all optional and one or more of them is @@ -287,7 +321,8 @@ namespace Microsoft.AspNetCore.Mvc.Internal if (action.RouteValues.ContainsKey(parameterPart.Name)) { - if (endpointInfo.MergedDefaults[parameterPart.Name] is string defaultValue + if (allDefaults.TryGetValue(parameterPart.Name, out var v) + && v is string defaultValue && action.RouteValues.TryGetValue(parameterPart.Name, out var routeValue) && string.Equals(defaultValue, routeValue, StringComparison.OrdinalIgnoreCase)) { @@ -346,11 +381,12 @@ namespace Microsoft.AspNetCore.Mvc.Internal { // Check that the value matches against constraints on that parameter // e.g. For {controller:regex((Home|Login))} the controller value must match the regex - if (endpointInfo.Constraints.TryGetValue(routeKey, out var constraints)) + if (endpointInfo.ParameterPolicies.TryGetValue(routeKey, out var parameterPolicies)) { - foreach (var constraint in constraints) + foreach (var policy in parameterPolicies) { - if (!constraint.Match(httpContext: null, NullRouter.Instance, routeKey, new RouteValueDictionary(action.RouteValues), RouteDirection.IncomingRequest)) + if (policy is IRouteConstraint constraint + && !constraint.Match(httpContext: null, NullRouter.Instance, routeKey, new RouteValueDictionary(action.RouteValues), RouteDirection.IncomingRequest)) { // Did not match constraint return false; @@ -372,7 +408,6 @@ namespace Microsoft.AspNetCore.Mvc.Internal IEnumerable segments, object nonInlineDefaults, int order, - object source, RouteValueDictionary dataTokens, bool suppressLinkGeneration, bool suppressPathMatching) @@ -394,7 +429,6 @@ namespace Microsoft.AspNetCore.Mvc.Internal action, routeName, new RouteValueDictionary(action.RouteValues), - source, dataTokens, suppressLinkGeneration, suppressPathMatching); @@ -413,7 +447,6 @@ namespace Microsoft.AspNetCore.Mvc.Internal ActionDescriptor action, string routeName, RouteValueDictionary requiredValues, - object source, RouteValueDictionary dataTokens, bool suppressLinkGeneration, bool suppressPathMatching) diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/MvcEndpointDataSourceTests.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/MvcEndpointDataSourceTests.cs index 17f88cea1a..03d280fdef 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/MvcEndpointDataSourceTests.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/MvcEndpointDataSourceTests.cs @@ -74,7 +74,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal Assert.Equal(displayName, matcherEndpoint.DisplayName); Assert.Equal(order, matcherEndpoint.Order); - Assert.Equal(template, matcherEndpoint.RoutePattern.RawText); + Assert.Equal("Template!", matcherEndpoint.RoutePattern.RawText); } [Fact] @@ -134,21 +134,41 @@ namespace Microsoft.AspNetCore.Mvc.Internal Assert.True(actionInvokerCalled); } + public static TheoryData GetSingleActionData_Conventional + { + get => GetSingleActionData(true); + } + + public static TheoryData GetSingleActionData_Attribute + { + get => GetSingleActionData(false); + } + + private static TheoryData GetSingleActionData(bool isConventionalRouting) + { + var data = new TheoryData + { + {"{controller}/{action}/{id?}", new[] { "TestController/TestAction/{id?}" }}, + {"{controller}/{id?}", isConventionalRouting ? new string[] { } : new[] { "TestController/{id?}" }}, + {"{action}/{id?}", isConventionalRouting ? new string[] { } : new[] { "TestAction/{id?}" }}, + {"{Controller}/{Action}/{id?}", new[] { "TestController/TestAction/{id?}" }}, + {"{CONTROLLER}/{ACTION}/{id?}", new[] { "TestController/TestAction/{id?}" }}, + {"{controller}/{action=TestAction}", new[] { "TestController", "TestController/TestAction" }}, + {"{controller}/{action=TestAction}/{id?}", new[] { "TestController", "TestController/TestAction/{id?}" }}, + {"{controller=TestController}/{action=TestAction}/{id?}", new[] { "", "TestController", "TestController/TestAction/{id?}" }}, + {"{controller}/{action}/{*catchAll}", new[] { "TestController/TestAction/{*catchAll}" }}, + {"{controller}/{action=TestAction}/{*catchAll}", new[] { "TestController", "TestController/TestAction/{*catchAll}" }}, + {"{controller}/{action=TestAction}/{id?}/{*catchAll}", new[] { "TestController", "TestController/TestAction/{id?}/{*catchAll}" }}, + {"{controller}/{action}.{ext?}", new[] { "TestController/TestAction.{ext?}" }}, + {"{controller}/{action=TestAction}.{ext?}", new[] { "TestController", "TestController/TestAction.{ext?}" }}, + }; + + return data; + } + [Theory] - [InlineData("{controller}/{action}/{id?}", new[] { "TestController/TestAction/{id?}" })] - [InlineData("{controller}/{id?}", new string[] { })] - [InlineData("{action}/{id?}", new string[] { })] - [InlineData("{Controller}/{Action}/{id?}", new[] { "TestController/TestAction/{id?}" })] - [InlineData("{CONTROLLER}/{ACTION}/{id?}", new[] { "TestController/TestAction/{id?}" })] - [InlineData("{controller}/{action=TestAction}", new[] { "TestController", "TestController/TestAction" })] - [InlineData("{controller}/{action=TestAction}/{id?}", new[] { "TestController", "TestController/TestAction/{id?}" })] - [InlineData("{controller=TestController}/{action=TestAction}/{id?}", new[] { "", "TestController", "TestController/TestAction/{id?}" })] - [InlineData("{controller}/{action}/{*catchAll}", new[] { "TestController/TestAction/{*catchAll}" })] - [InlineData("{controller}/{action=TestAction}/{*catchAll}", new[] { "TestController", "TestController/TestAction/{*catchAll}" })] - [InlineData("{controller}/{action=TestAction}/{id?}/{*catchAll}", new[] { "TestController", "TestController/TestAction/{id?}/{*catchAll}" })] - [InlineData("{controller}/{action}.{ext?}", new[] { "TestController/TestAction.{ext?}" })] - [InlineData("{controller}/{action=TestAction}.{ext?}", new[] { "TestController", "TestController/TestAction.{ext?}" })] - public void Endpoints_SingleAction(string endpointInfoRoute, string[] finalEndpointPatterns) + [MemberData(nameof(GetSingleActionData_Conventional))] + public void Endpoints_Conventional_SingleAction(string endpointInfoRoute, string[] finalEndpointPatterns) { // Arrange var actionDescriptorCollection = GetActionDescriptorCollection( @@ -168,6 +188,28 @@ namespace Microsoft.AspNetCore.Mvc.Internal Assert.Collection(endpoints, inspectors); } + [Theory] + [MemberData(nameof(GetSingleActionData_Attribute))] + public void Endpoints_AttributeRouting_SingleAction(string endpointInfoRoute, string[] finalEndpointPatterns) + { + // Arrange + var actionDescriptorCollection = GetActionDescriptorCollection( + attributeRouteTemplate: endpointInfoRoute, + new { controller = "TestController", action = "TestAction" }); + var dataSource = CreateMvcEndpointDataSource(actionDescriptorCollection); + + // Act + var endpoints = dataSource.Endpoints; + + // Assert + var inspectors = finalEndpointPatterns + .Select(t => new Action(e => Assert.Equal(t, Assert.IsType(e).RoutePattern.RawText))) + .ToArray(); + + // Assert + Assert.Collection(endpoints, inspectors); + } + [Theory] [InlineData("{area}/{controller}/{action}/{id?}", new[] { "TestArea/TestController/TestAction/{id?}" })] [InlineData("{controller}/{action}/{id?}", new string[] { })] @@ -740,11 +782,16 @@ namespace Microsoft.AspNetCore.Mvc.Internal } private IActionDescriptorCollectionProvider GetActionDescriptorCollection(params object[] requiredValues) + { + return GetActionDescriptorCollection(attributeRouteTemplate: null, requiredValues); + } + + private IActionDescriptorCollectionProvider GetActionDescriptorCollection(string attributeRouteTemplate, params object[] requiredValues) { var actionDescriptors = new List(); foreach (var requiredValue in requiredValues) { - actionDescriptors.Add(CreateActionDescriptor(requiredValue)); + actionDescriptors.Add(CreateActionDescriptor(requiredValue, attributeRouteTemplate)); } var actionDescriptorCollectionProviderMock = new Mock(); @@ -756,10 +803,10 @@ namespace Microsoft.AspNetCore.Mvc.Internal private ActionDescriptor CreateActionDescriptor(string controller, string action, string area = null) { - return CreateActionDescriptor(new { controller = controller, action = action, area = area }); + return CreateActionDescriptor(new { controller = controller, action = action, area = area }, attributeRouteTemplate: null); } - private ActionDescriptor CreateActionDescriptor(object requiredValues) + private ActionDescriptor CreateActionDescriptor(object requiredValues, string attributeRouteTemplate = null) { var actionDescriptor = new ActionDescriptor(); var routeValues = new RouteValueDictionary(requiredValues); @@ -767,6 +814,14 @@ namespace Microsoft.AspNetCore.Mvc.Internal { actionDescriptor.RouteValues[kvp.Key] = kvp.Value?.ToString(); } + if (!string.IsNullOrEmpty(attributeRouteTemplate)) + { + actionDescriptor.AttributeRouteInfo = new AttributeRouteInfo + { + Name = attributeRouteTemplate, + Template = attributeRouteTemplate + }; + } return actionDescriptor; }