From cb0627b28aefe94e7f06d297f73d64aa78380177 Mon Sep 17 00:00:00 2001 From: Ryan Nowak Date: Sat, 25 Aug 2018 21:25:25 -0700 Subject: [PATCH] Addressing a skipped test I think something that was meant to be revisited in a PR got left behing. I found this while fixing aspnet/Routing#772 and it seems worth addressing. This change removes the hardcoding of action/controller/area in the data source, and corrects the behavior of required route values when they aren't in that set. --- .../Internal/MvcEndpointDataSource.cs | 161 +++++++++--------- .../Internal/MvcEndpointDataSourceTests.cs | 53 ++++-- 2 files changed, 127 insertions(+), 87 deletions(-) diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Internal/MvcEndpointDataSource.cs b/src/Microsoft.AspNetCore.Mvc.Core/Internal/MvcEndpointDataSource.cs index 128e699774..2837e6ec4a 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Internal/MvcEndpointDataSource.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Internal/MvcEndpointDataSource.cs @@ -89,73 +89,88 @@ namespace Microsoft.AspNetCore.Mvc.Internal // - Home/Login foreach (var endpointInfo in ConventionalEndpointInfos) { - if (MatchRouteValue(action, endpointInfo, "Area") - && MatchRouteValue(action, endpointInfo, "Controller") - && MatchRouteValue(action, endpointInfo, "Action")) + // An 'endpointInfo' is applicable if: + // 1. it has a parameter (or default value) for 'required' non-null route value + // 2. it does not have a parameter (or default value) for 'required' null route value + var isApplicable = true; + foreach (var routeKey in action.RouteValues.Keys) { - var newPathSegments = endpointInfo.ParsedPattern.PathSegments.ToList(); - - for (var i = 0; i < newPathSegments.Count; i++) + if (!MatchRouteValue(action, endpointInfo, routeKey)) { - // 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 (UseDefaultValuePlusRemainingSegementsOptional(i, action, endpointInfo, newPathSegments)) + isApplicable = false; + break; + } + } + + if (!isApplicable) + { + 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 (UseDefaultValuePlusRemainingSegementsOptional(i, action, endpointInfo, newPathSegments)) + { + var subPathSegments = newPathSegments.Take(i); + + var subEndpoint = CreateEndpoint( + action, + endpointInfo.Name, + GetPattern(ref patternStringBuilder, subPathSegments), + subPathSegments, + endpointInfo.Defaults, + ++conventionalRouteOrder, + endpointInfo, + suppressLinkGeneration: 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)) { - var subPathSegments = newPathSegments.Take(i); - - var subEndpoint = CreateEndpoint( - action, - endpointInfo.Name, - GetPattern(ref patternStringBuilder, subPathSegments), - subPathSegments, - endpointInfo.Defaults, - ++conventionalRouteOrder, - endpointInfo, - suppressLinkGeneration: 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 && IsMvcParameter(parameterPart.Name)) + if (segmentParts == null) { - if (segmentParts == null) - { - segmentParts = segment.Parts.ToList(); - } - - // Replace parameter with literal value - segmentParts[j] = RoutePatternFactory.LiteralPart(action.RouteValues[parameterPart.Name]); + segmentParts = segment.Parts.ToList(); } - } - // A parameter part was replaced so replace segment with updated parts - if (segmentParts != null) - { - newPathSegments[i] = RoutePatternFactory.Segment(segmentParts); + // Replace parameter with literal value + segmentParts[j] = RoutePatternFactory.LiteralPart(action.RouteValues[parameterPart.Name]); } } - var endpoint = CreateEndpoint( - action, - endpointInfo.Name, - GetPattern(ref patternStringBuilder, newPathSegments), - newPathSegments, - endpointInfo.Defaults, - ++conventionalRouteOrder, - endpointInfo, - suppressLinkGeneration: false); - endpoints.Add(endpoint); + // A parameter part was replaced so replace segment with updated parts + if (segmentParts != null) + { + newPathSegments[i] = RoutePatternFactory.Segment(segmentParts); + } } + + var endpoint = CreateEndpoint( + action, + endpointInfo.Name, + GetPattern(ref patternStringBuilder, newPathSegments), + newPathSegments, + endpointInfo.Defaults, + ++conventionalRouteOrder, + endpointInfo, + suppressLinkGeneration: false); + endpoints.Add(endpoint); } } else @@ -190,18 +205,6 @@ namespace Microsoft.AspNetCore.Mvc.Internal } } - private bool IsMvcParameter(string name) - { - if (string.Equals(name, "Area", StringComparison.OrdinalIgnoreCase) - || string.Equals(name, "Controller", StringComparison.OrdinalIgnoreCase) - || string.Equals(name, "Action", StringComparison.OrdinalIgnoreCase)) - { - return true; - } - - return false; - } - private bool UseDefaultValuePlusRemainingSegementsOptional( int segmentIndex, ActionDescriptor action, @@ -225,7 +228,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal continue; } - if (IsMvcParameter(parameterPart.Name)) + if (action.RouteValues.ContainsKey(parameterPart.Name)) { if (endpointInfo.MergedDefaults[parameterPart.Name] is string defaultValue && action.RouteValues.TryGetValue(parameterPart.Name, out var routeValue) @@ -266,8 +269,10 @@ namespace Microsoft.AspNetCore.Mvc.Internal { // Action does not have a value for this routeKey, most likely because action is not in an area // Check that the pattern does not have a parameter for the routeKey - var matchingParameter = endpointInfo.ParsedPattern.Parameters.SingleOrDefault(p => string.Equals(p.Name, routeKey, StringComparison.OrdinalIgnoreCase)); - if (matchingParameter == null) + var matchingParameter = endpointInfo.ParsedPattern.GetParameter(routeKey); + if (matchingParameter == null && + (!endpointInfo.ParsedPattern.Defaults.TryGetValue(routeKey, out var value) || + !string.IsNullOrEmpty(Convert.ToString(value)))) { return true; } @@ -279,7 +284,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal return true; } - var matchingParameter = endpointInfo.ParsedPattern.Parameters.SingleOrDefault(p => string.Equals(p.Name, routeKey, StringComparison.OrdinalIgnoreCase)); + var matchingParameter = endpointInfo.ParsedPattern.GetParameter(routeKey); if (matchingParameter != null) { // Check that the value matches against constraints on that parameter @@ -358,10 +363,12 @@ namespace Microsoft.AspNetCore.Mvc.Internal object source, bool suppressLinkGeneration) { - var metadata = new List(); - // REVIEW: Used for debugging. Consider removing before release - metadata.Add(source); - metadata.Add(action); + var metadata = new List + { + // REVIEW: Used for debugging. Consider removing before release + source, + action + }; if (action.EndpointMetadata != null) { diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/MvcEndpointDataSourceTests.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/MvcEndpointDataSourceTests.cs index fbee5e6f55..e4230b1d56 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/MvcEndpointDataSourceTests.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/MvcEndpointDataSourceTests.cs @@ -511,29 +511,40 @@ namespace Microsoft.AspNetCore.Mvc.Internal Assert.Empty(endpoints); } - // Since area, controller, action and page are special, check to see if the followin test succeeds for a - // custom required value too. - [Fact(Skip = "Needs review")] + // area, controller, action and page are special, but not hardcoded. Actions can define custom required + // route values. This has been used successfully for localization, versioning and similar schemes. We should + // be able to replace custom route values too. + [Fact] public void NonReservedRequiredValue_WithNoCorresponding_TemplateParameter_DoesNotProduceEndpoint() { // Arrange - var requiredValues = new RouteValueDictionary(new { controller = "home", action = "index", foo = "bar" }); - var actionDescriptorCollection = GetActionDescriptorCollection(requiredValues); + var action1 = new RouteValueDictionary(new { controller = "home", action = "index", locale = "en-NZ" }); + var action2 = new RouteValueDictionary(new { controller = "home", action = "about", locale = "en-CA" }); + var action3 = new RouteValueDictionary(new { controller = "home", action = "index", locale = (string)null }); + + var actionDescriptorCollection = GetActionDescriptorCollection(action1, action2, action3); var dataSource = CreateMvcEndpointDataSource(actionDescriptorCollection); + + // Adding a localized route a non-localized route + dataSource.ConventionalEndpointInfos.Add(CreateEndpointInfo(string.Empty, "{locale}/{controller}/{action}")); dataSource.ConventionalEndpointInfos.Add(CreateEndpointInfo(string.Empty, "{controller}/{action}")); // Act var endpoints = dataSource.Endpoints; // Assert - Assert.Empty(endpoints); + Assert.Collection( + endpoints.Cast().OrderBy(e => e.RoutePattern.RawText), + e => Assert.Equal("en-CA/home/about", e.RoutePattern.RawText), + e => Assert.Equal("en-NZ/home/index", e.RoutePattern.RawText), + e => Assert.Equal("home/index", e.RoutePattern.RawText)); } [Fact] public void TemplateParameter_WithNoDefaultOrRequiredValue_DoesNotProduceEndpoint() { // Arrange - var requiredValues = new RouteValueDictionary(new { controller = "home", action = "index" }); + var requiredValues = new RouteValueDictionary(new { controller = "home", action = "index", area = (string)null }); var actionDescriptorCollection = GetActionDescriptorCollection(requiredValues); var dataSource = CreateMvcEndpointDataSource(actionDescriptorCollection); dataSource.ConventionalEndpointInfos.Add(CreateEndpointInfo(string.Empty, "{area}/{controller}/{action}")); @@ -606,7 +617,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal var actionDescriptorCollection = GetActionDescriptorCollection(requiredValues: requiredValues); var dataSource = CreateMvcEndpointDataSource(actionDescriptorCollection); dataSource.ConventionalEndpointInfos.Add( - CreateEndpointInfo(string.Empty, "{controller=Home}/{action=Index}")); + CreateEndpointInfo(string.Empty, "{subarea}/{controller=Home}/{action=Index}")); // Act var endpoints = dataSource.Endpoints; @@ -614,10 +625,29 @@ namespace Microsoft.AspNetCore.Mvc.Internal // Assert var endpoint = Assert.Single(endpoints); var matcherEndpoint = Assert.IsType(endpoint); - Assert.Equal("Foo/Bar", matcherEndpoint.RoutePattern.RawText); + Assert.Equal("test/Foo/Bar", matcherEndpoint.RoutePattern.RawText); AssertIsSubset(expectedDefaults, matcherEndpoint.RoutePattern.Defaults); } + [Fact] + public void RequiredValues_NotPresent_InDefaultValuesOrParameter_EndpointNotCreated() + { + // Arrange + var requiredValues = new RouteValueDictionary( + new { controller = "Foo", action = "Bar", subarea = "test" }); + var expectedDefaults = requiredValues; + var actionDescriptorCollection = GetActionDescriptorCollection(requiredValues: requiredValues); + var dataSource = CreateMvcEndpointDataSource(actionDescriptorCollection); + dataSource.ConventionalEndpointInfos.Add( + CreateEndpointInfo(string.Empty, "{controller=Home}/{action=Index}")); + + // Act + var endpoints = dataSource.Endpoints; + + // Assert + Assert.Empty(endpoints); + } + [Fact] public void RequiredValues_IsSubsetOf_DefaultValues() { @@ -629,7 +659,10 @@ namespace Microsoft.AspNetCore.Mvc.Internal var actionDescriptorCollection = GetActionDescriptorCollection(requiredValues: requiredValues); var dataSource = CreateMvcEndpointDataSource(actionDescriptorCollection); dataSource.ConventionalEndpointInfos.Add( - CreateEndpointInfo(string.Empty, "{controller=Home}/{action=Index}/{subscription=general}")); + CreateEndpointInfo( + string.Empty, + "{controller=Home}/{action=Index}/{subscription=general}", + defaults: new RouteValueDictionary(new { subarea = "test", }))); // Act var endpoints = dataSource.Endpoints;