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.

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")
+                        // An 'endpointInfo' is applicable if:
-                            && MatchRouteValue(action, endpointInfo, "Controller")
+                        // 1. it has a parameter (or default value) for 'required' non-null route value
-                            && MatchRouteValue(action, endpointInfo, "Action"))
+                        // 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();
+                            if (!MatchRouteValue(action, endpointInfo, routeKey))
-
-                            for (var i = 0; i < newPathSegments.Count; i++)
                             {
-                                // Check if the pattern can be shortened because the remaining parameters are optional
+                                isApplicable = false;
-                                //
+                                break;
-                                // e.g. Matching pattern {controller=Home}/{action=Index}/{id?} against HomeController.Index
+                            }
-                                // can resolve to the following endpoints:
+                        }
-                                // - /Home/Index/{id?}
+
-                                // - /Home
+                        if (!isApplicable)
-                                // - /
+                        {
-                                if (UseDefaultValuePlusRemainingSegementsOptional(i, action, endpointInfo, newPathSegments))
+                            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<RoutePatternPart> 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);
+                                    if (segmentParts == null)
-
-                                    var subEndpoint = CreateEndpoint(
-                                        action,
-                                        endpointInfo.Name,
-                                        GetPattern(ref patternStringBuilder, subPathSegments),
-                                        subPathSegments,
-                                        endpointInfo.Defaults,
-                                        ++conventionalRouteOrder,
-                                        endpointInfo,
-                                        suppressLinkGeneration: false);
-                                    endpoints.Add(subEndpoint);
-                                }
-
-                                List<RoutePatternPart> 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)
+                                        segmentParts = segment.Parts.ToList();
-                                        {
-                                            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
+                                    // Replace parameter with literal value
-                                if (segmentParts != null)
+                                    segmentParts[j] = RoutePatternFactory.LiteralPart(action.RouteValues[parameterPart.Name]);
-                                {
-                                    newPathSegments[i] = RoutePatternFactory.Segment(segmentParts);
                                 }
                             }
 
-                            var endpoint = CreateEndpoint(
+                            // A parameter part was replaced so replace segment with updated parts
-                                action,
+                            if (segmentParts != null)
-                                endpointInfo.Name,
+                            {
-                                GetPattern(ref patternStringBuilder, newPathSegments),
+                                newPathSegments[i] = RoutePatternFactory.Segment(segmentParts);
-                                newPathSegments,
+                            }
-                                endpointInfo.Defaults,
-                                ++conventionalRouteOrder,
-                                endpointInfo,
-                                suppressLinkGeneration: false);
-                            endpoints.Add(endpoint);
                         }
+
+                        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));
+                var matchingParameter = endpointInfo.ParsedPattern.GetParameter(routeKey);
-                if (matchingParameter == null)
+                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<object>();
+            var metadata = new List<object>
-            // REVIEW: Used for debugging. Consider removing before release
+            {
-            metadata.Add(source);
+                // REVIEW: Used for debugging. Consider removing before release
-            metadata.Add(action);
+                source,
+                action
+            };
 
             if (action.EndpointMetadata != null)
             {

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 
+        // area, controller, action and page are special, but not hardcoded. Actions can define custom required
-        // custom required value too.
+        // route values. This has been used successfully for localization, versioning and similar schemes. We should
-        [Fact(Skip = "Needs review")]
+        // 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 action1 = new RouteValueDictionary(new { controller = "home", action = "index", locale = "en-NZ" });
-            var actionDescriptorCollection = GetActionDescriptorCollection(requiredValues);
+            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<RouteEndpoint>().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<RouteEndpoint>(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;