Use coventional routes for link generation (#9037)

Use coventional routes for link generation

This change enables using conventional routes for link generation when
using MVC conventional routes. This change makes MVC link generation
behaviour highly compatible with 2.1.

The way that this works is that we create endpoints for **MATCHING**
using the denormalized conventional route, but we tell those endpoints
to suppress link generation.

For link generation we generate a non-matching endpoints per-route with
the same order value.

I added the concept of *required value any* to link generation. This is
needed because for an endpoint to participate in link generation using
RouteValuesAddress it needs to have some required values. These details
are a little fiddly, but I think it's worth doing this feature
completely.

Author: rynowak

src/Http/Routing/ref/Microsoft.AspNetCore.Routing.netcoreapp3.0.cs

@@ -671,6 +671,7 @@ namespace Microsoft.AspNetCore.Routing.Patterns
     public sealed partial class RoutePattern
     {
         internal RoutePattern() { }
+        public static readonly object RequiredValueAny;
         public System.Collections.Generic.IReadOnlyDictionary<string, object> Defaults { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } }
         public decimal InboundPrecedence { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } }
         public decimal OutboundPrecedence { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } }

src/Http/Routing/src/Internal/LinkGenerationDecisionTree.cs

@@ -7,6 +7,7 @@
 using System.Linq;
 using System.Text;
 using Microsoft.AspNetCore.Routing.DecisionTree;
+using Microsoft.AspNetCore.Routing.Patterns;
 using Microsoft.AspNetCore.Routing.Tree;
 
 namespace Microsoft.AspNetCore.Routing.Internal
@@ -21,38 +22,57 @@ public class LinkGenerationDecisionTree
         private static readonly RouteValueDictionary EmptyAmbientValues = new RouteValueDictionary();
 
         private readonly DecisionTreeNode<OutboundMatch> _root;
-        private readonly Dictionary<string, HashSet<object>> _knownValues;
+        private readonly List<OutboundMatch> _conventionalEntries;
 
         public LinkGenerationDecisionTree(IReadOnlyList<OutboundMatch> entries)
         {
-            _root = DecisionTreeBuilder<OutboundMatch>.GenerateTree(
-                entries,
-                new OutboundMatchClassifier());
+            // We split up the entries into:
+            // 1. attribute routes - these go into the tree
+            // 2. conventional routes - these are a list
+            var attributedEntries = new List<OutboundMatch>();
+            _conventionalEntries = new List<OutboundMatch>();
 
-            _knownValues = new Dictionary<string, HashSet<object>>(StringComparer.OrdinalIgnoreCase);
+            // Anything with a RoutePattern.RequiredValueAny as a RequiredValue is a conventional route.
+            // This is because RequiredValueAny acts as a wildcard, whereas an attribute route entry
+            // is denormalized to contain an exact set of required values.
+            //
+            // We will only see conventional routes show up here for endpoint routing.
             for (var i = 0; i < entries.Count; i++)
             {
+                var isAttributeRoute = true;
                 var entry = entries[i];
                 foreach (var kvp in entry.Entry.RequiredLinkValues)
                 {
-                    if (!_knownValues.TryGetValue(kvp.Key, out var values))
+                    if (RoutePattern.IsRequiredValueAny(kvp.Value))
                     {
-                        values = new HashSet<object>(RouteValueEqualityComparer.Default);
-                        _knownValues.Add(kvp.Key, values);
+                        isAttributeRoute = false;
+                        break;
                     }
+                }
 
-                    values.Add(kvp.Value ?? string.Empty);
+                if (isAttributeRoute)
+                {
+                    attributedEntries.Add(entry);
+                }
+                else
+                {
+                    _conventionalEntries.Add(entry);
                 }
             }
+
+            _root = DecisionTreeBuilder<OutboundMatch>.GenerateTree(
+                attributedEntries,
+                new OutboundMatchClassifier());
         }
 
         public IList<OutboundMatchResult> GetMatches(RouteValueDictionary values, RouteValueDictionary ambientValues)
         {
             // Perf: Avoid allocation for List if there aren't any Matches or Criteria
-            if (_root.Matches.Count > 0 || _root.Criteria.Count > 0)
+            if (_root.Matches.Count > 0 || _root.Criteria.Count > 0 || _conventionalEntries.Count > 0)
             {
                 var results = new List<OutboundMatchResult>();
                 Walk(results, values, ambientValues ?? EmptyAmbientValues, _root, isFallbackPath: false);
+                ProcessConventionalEntries(results, values, ambientValues ?? EmptyAmbientValues);
                 results.Sort(OutboundMatchResultComparer.Instance);
                 return results;
             }
@@ -110,19 +130,6 @@ private void Walk(
                     {
                         Walk(results, values, ambientValues, branch, isFallbackPath);
                     }
-                    else
-                    {
-                        // If an explicitly specified value doesn't match any branch, then speculatively walk the
-                        // "null" path if the value doesn't match any known value.
-                        //
-                        // This can happen when linking from a page <-> action. We want to be
-                        // able to use "page" and "action" as normal route parameters.
-                        var knownValues = _knownValues[key];
-                        if (!knownValues.Contains(value ?? string.Empty) && criterion.Branches.TryGetValue(string.Empty, out branch))
-                        {
-                            Walk(results, values, ambientValues, branch, isFallbackPath: true);
-                        }
-                    }
                 }
                 else
                 {
@@ -147,6 +154,17 @@ private void Walk(
             }
         }
 
+        private void ProcessConventionalEntries(
+            List<OutboundMatchResult> results,
+            RouteValueDictionary values,
+            RouteValueDictionary ambientvalues)
+        {
+            for (var i = 0; i < _conventionalEntries.Count; i++)
+            {
+                results.Add(new OutboundMatchResult(_conventionalEntries[i], isFallbackMatch: false));
+            }
+        }
+
         private class OutboundMatchClassifier : IClassifier<OutboundMatch>
         {
             public IEqualityComparer<object> ValueComparer => RouteValueEqualityComparer.Default;

src/Http/Routing/src/Patterns/DefaultRoutePatternTransformer.cs

@@ -1,4 +1,4 @@
-// Copyright (c) .NET Foundation. All rights reserved.
+// Copyright (c) .NET Foundation. All rights reserved.
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
 using System;
@@ -43,8 +43,9 @@ private RoutePattern SubstituteRequiredValuesCore(RoutePattern original, RouteVa
             {
                 // There are three possible cases here:
                 // 1. Required value is null-ish
-                // 2. Required value corresponds to a parameter
-                // 3. Required value corresponds to a matching default value
+                // 2. Required value is *any*
+                // 3. Required value corresponds to a parameter
+                // 4. Required value corresponds to a matching default value
                 //
                 // If none of these are true then we can reject this substitution.
                 RoutePatternParameterPart parameter;
@@ -76,9 +77,28 @@ private RoutePattern SubstituteRequiredValuesCore(RoutePattern original, RouteVa
                     // Ex: {controller=Home}/{action=Index}/{id?} - with required values: { area = "", ... }
                     continue;
                 }
+                else if (RoutePattern.IsRequiredValueAny(kvp.Value))
+                {
+                    // 2. Required value is *any* - this is allowed for a parameter with a default, but not
+                    // a non-parameter default.
+                    if (original.GetParameter(kvp.Key) == null &&
+                        original.Defaults.TryGetValue(kvp.Key, out var defaultValue) &&
+                        !RouteValueEqualityComparer.Default.Equals(string.Empty, defaultValue))
+                    {
+                        // Fail: this route as a non-parameter default that is stricter than *any*.
+                        //
+                        // Ex: Admin/{controller=Home}/{action=Index}/{id?} defaults: { area = "Admin" } - with required values: { area = *any* }
+                        return null;
+                    }
+
+                    // Success: (for this parameter at least)
+                    //
+                    // Ex: {controller=Home}/{action=Index}/{id?} - with required values: { controller = *any*, ... }
+                    continue;
+                }
                 else if ((parameter = original.GetParameter(kvp.Key)) != null)
                 {
-                    // 2. Required value corresponds to a parameter - check to make sure that this value matches
+                    // 3. Required value corresponds to a parameter - check to make sure that this value matches
                     // any IRouteConstraint implementations.
                     if (!MatchesConstraints(original, parameter, kvp.Key, requiredValues))
                     {
@@ -96,7 +116,7 @@ private RoutePattern SubstituteRequiredValuesCore(RoutePattern original, RouteVa
                 else if (original.Defaults.TryGetValue(kvp.Key, out var defaultValue) &&
                     RouteValueEqualityComparer.Default.Equals(kvp.Value, defaultValue))
                 {
-                    // 3. Required value corresponds to a matching default value - check to make sure that this value matches
+                    // 4. Required value corresponds to a matching default value - check to make sure that this value matches
                     // any IRouteConstraint implementations. It's unlikely that this would happen in practice but it doesn't
                     // hurt for us to check.
                     if (!MatchesConstraints(original, parameter: null, kvp.Key, requiredValues))
@@ -142,7 +162,10 @@ private RoutePattern SubstituteRequiredValuesCore(RoutePattern original, RouteVa
                 // We only need to handle the case where the required value maps to a parameter. That's the only
                 // case where we allow a default and a required value to disagree, and we already validated the
                 // other cases.
-                if (parameter != null && 
+                //
+                // If the required value is *any* then don't remove the default.
+                if (parameter != null &&
+                    !RoutePattern.IsRequiredValueAny(kvp.Value) &&
                     original.Defaults.TryGetValue(kvp.Key, out var defaultValue) && 
                     !RouteValueEqualityComparer.Default.Equals(kvp.Value, defaultValue))
                 {

src/Http/Routing/src/Patterns/RoutePattern.cs

@@ -17,6 +17,21 @@ namespace Microsoft.AspNetCore.Routing.Patterns
     [DebuggerDisplay("{DebuggerToString()}")]
     public sealed class RoutePattern
     {
+        /// <summary>
+        /// A marker object that can be used in <see cref="RequiredValues"/> to designate that
+        /// any non-null or non-empty value is required.
+        /// </summary>
+        /// <remarks>
+        /// <see cref="RequiredValueAny"/> is only use in routing is in <see cref="RoutePattern.RequiredValues"/>.
+        /// <see cref="RequiredValueAny"/> is not valid as a route value, and will convert to the null/empty string.
+        /// </remarks>
+        public static readonly object RequiredValueAny = new RequiredValueAnySentinal();
+
+        internal static bool IsRequiredValueAny(object value)
+        {
+            return object.ReferenceEquals(RequiredValueAny, value);
+        }
+
         private const string SeparatorString = "/";
 
         internal RoutePattern(
@@ -140,5 +155,11 @@ internal string DebuggerToString()
         {
             return RawText ?? string.Join(SeparatorString, PathSegments.Select(s => s.DebuggerToString()));
         }
+
+        [DebuggerDisplay("{DebuggerToString(),nq}")]
+        private class RequiredValueAnySentinal
+        {
+            private string DebuggerToString() => "*any*";
+        }
     }
 }

src/Http/Routing/src/Template/TemplateBinder.cs

@@ -178,7 +178,8 @@ public TemplateValuesResult GetValues(RouteValueDictionary ambientValues, RouteV
                             throw new InvalidOperationException($"Unable to find required value '{key}' on route pattern.");
                         }
 
-                        if (!RoutePartsEqual(ambientValue, _pattern.RequiredValues[key]))
+                        if (!RoutePartsEqual(ambientValue, _pattern.RequiredValues[key]) &&
+                            !RoutePattern.IsRequiredValueAny(_pattern.RequiredValues[key]))
                         {
                             copyAmbientValues = false;
                             break;
@@ -261,10 +262,11 @@ public TemplateValuesResult GetValues(RouteValueDictionary ambientValues, RouteV
                 //
                 // OR in plain English... when linking from a page in an area to an action in the same area, it should
                 // be possible to use the area as an ambient value.
-                if (!copyAmbientValues && _pattern.RequiredValues.TryGetValue(key, out var requiredValue))
+                if (!copyAmbientValues && !hasExplicitValue && _pattern.RequiredValues.TryGetValue(key, out var requiredValue))
                 {
                     hasAmbientValue = ambientValues != null && ambientValues.TryGetValue(key, out ambientValue);
-                    if (hasAmbientValue && RoutePartsEqual(requiredValue, ambientValue))
+                    if (hasAmbientValue &&
+                        (RoutePartsEqual(requiredValue, ambientValue) || RoutePattern.IsRequiredValueAny(requiredValue)))
                     {
                         // Treat this an an explicit value to *force it*. 
                         slots[i] = new KeyValuePair<string, object>(key, ambientValue);

src/Http/Routing/test/UnitTests/Internal/LinkGenerationDecisionTreeTest.cs

@@ -583,9 +583,7 @@ public void GetMatches_LinkToControllerFromPage_WithPageValue()
             var matches = tree.GetMatches(context.Values, context.AmbientValues).Select(m => m.Match).ToList();
 
             // Assert
-            Assert.Collection(
-                matches,
-                m => { Assert.Same(entry1, m); });
+            Assert.Empty(matches);
         }
 
         [Fact]
@@ -689,9 +687,7 @@ public void GetMatches_LinkToPageFromController_WithActionValue()
             var matches = tree.GetMatches(context.Values, context.AmbientValues).Select(m => m.Match).ToList();
 
             // Assert
-            Assert.Collection(
-                matches,
-                m => { Assert.Same(entry2, m); });
+            Assert.Empty(matches);
         }
 
         [Fact]