Fix for #1913 - Improve attribute route link generation for areas

Attribute route link generation will now have a slight preference for entries that can use ambient values (vs ignoring an ambient value). This means that areas will be more 'sticky' with regard to link generation without the need to specify a better Order.
aspnet · Feb 11, 2015 · 7cb6c10 · 7cb6c10
1 parent 1721d90
commit 7cb6c10
Show file tree

Hide file tree

Showing 6 changed files with 76 additions and 37 deletions.
diff --git a/src/Microsoft.AspNet.Mvc.Core/Internal/Routing/LinkGenerationDecisionTree.cs b/src/Microsoft.AspNet.Mvc.Core/Internal/Routing/LinkGenerationDecisionTree.cs
@@ -21,11 +21,11 @@ public LinkGenerationDecisionTree(IReadOnlyList<AttributeRouteLinkGenerationEntr
                 new AttributeRouteLinkGenerationEntryClassifier());
         }
 
-        public List<AttributeRouteLinkGenerationEntry> GetMatches(VirtualPathContext context)
+        public List<LinkGenerationMatch> GetMatches(VirtualPathContext context)
         {
-            var results = new List<AttributeRouteLinkGenerationEntry>();
-            Walk(results, context, _root);
-            results.Sort(AttributeRouteLinkGenerationEntryComparer.Instance);
+            var results = new List<LinkGenerationMatch>();
+            Walk(results, context, _root, isFallbackPath: false);
+            results.Sort(LinkGenerationMatchComparer.Instance);
             return results;
         }
 
@@ -55,15 +55,16 @@ public List<AttributeRouteLinkGenerationEntry> GetMatches(VirtualPathContext con
         //
         // The decision tree uses a tree data structure to execute these rules across all candidates at once.
         private void Walk(
-            List<AttributeRouteLinkGenerationEntry> results,
+            List<LinkGenerationMatch> results,
             VirtualPathContext context,
-            DecisionTreeNode<AttributeRouteLinkGenerationEntry> node)
+            DecisionTreeNode<AttributeRouteLinkGenerationEntry> node,
+            bool isFallbackPath)
         {
             // Any entries in node.Matches have had all their required values satisfied, so add them
             // to the results.
             for (var i = 0; i < node.Matches.Count; i++)
             {
-                results.Add(node.Matches[i]);
+                results.Add(new LinkGenerationMatch(node.Matches[i], isFallbackPath));
             }
 
             for (var i = 0; i < node.Criteria.Count; i++)
@@ -77,26 +78,27 @@ public List<AttributeRouteLinkGenerationEntry> GetMatches(VirtualPathContext con
                     DecisionTreeNode<AttributeRouteLinkGenerationEntry> branch;
                     if (criterion.Branches.TryGetValue(value ?? string.Empty, out branch))
                     {
-                        Walk(results, context, branch);
+                        Walk(results, context, branch, isFallbackPath);
                     }
                 }
                 else
                 {
                     // If a value wasn't explicitly supplied, match BOTH the ambient value and the empty value
-                    // if an ambient value was supplied.
+                    // if an ambient value was supplied. The path explored with the empty value is considered
+                    // the fallback path.
                     DecisionTreeNode<AttributeRouteLinkGenerationEntry> branch;
                     if (context.AmbientValues.TryGetValue(key, out value) &&
                         !criterion.Branches.Comparer.Equals(value, string.Empty))
                     {
                         if (criterion.Branches.TryGetValue(value, out branch))
                         {
-                            Walk(results, context, branch);
+                            Walk(results, context, branch, isFallbackPath);
                         }
                     }
 
                     if (criterion.Branches.TryGetValue(string.Empty, out branch))
                     {
-                        Walk(results, context, branch);
+                        Walk(results, context, branch, isFallbackPath: true);
                     }
                 }
             }
@@ -123,24 +125,31 @@ public AttributeRouteLinkGenerationEntryClassifier()
             }
         }
 
-        private class AttributeRouteLinkGenerationEntryComparer : IComparer<AttributeRouteLinkGenerationEntry>
+        private class LinkGenerationMatchComparer : IComparer<LinkGenerationMatch>
         {
-            public static readonly AttributeRouteLinkGenerationEntryComparer Instance =
-                new AttributeRouteLinkGenerationEntryComparer();
+            public static readonly LinkGenerationMatchComparer Instance = new LinkGenerationMatchComparer();
 
-            public int Compare(AttributeRouteLinkGenerationEntry x, AttributeRouteLinkGenerationEntry y)
+            public int Compare(LinkGenerationMatch x, LinkGenerationMatch y)
             {
-                if (x.Order != y.Order)
+                // For these comparisons lower is better.
+
+                if (x.Entry.Order != y.Entry.Order)
+                {
+                    return x.Entry.Order.CompareTo(y.Entry.Order);
+                }
+
+                if (x.IsFallbackMatch != y.IsFallbackMatch)
                 {
-                    return x.Order.CompareTo(y.Order);
+                    // A fallback match is worse than a non-fallback
+                    return x.IsFallbackMatch.CompareTo(y.IsFallbackMatch);
                 }
 
-                if (x.Precedence != y.Precedence)
+                if (x.Entry.Precedence != y.Entry.Precedence)
                 {
-                    return x.Precedence.CompareTo(y.Precedence);
+                    return x.Entry.Precedence.CompareTo(y.Entry.Precedence);
                 }
 
-                return StringComparer.Ordinal.Compare(x.TemplateText, y.TemplateText);
+                return StringComparer.Ordinal.Compare(x.Entry.TemplateText, y.Entry.TemplateText);
             }
         }
     }

diff --git a/src/Microsoft.AspNet.Mvc.Core/Internal/Routing/LinkGenerationMatch.cs b/src/Microsoft.AspNet.Mvc.Core/Internal/Routing/LinkGenerationMatch.cs
@@ -0,0 +1,23 @@
+// Copyright (c) Microsoft Open Technologies, Inc. All rights reserved.
+// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
+
+using Microsoft.AspNet.Mvc.Routing;
+
+namespace Microsoft.AspNet.Mvc.Internal.Routing
+{
+    public struct LinkGenerationMatch
+    {
+        private readonly bool _isFallbackMatch;
+        private readonly AttributeRouteLinkGenerationEntry _entry;
+
+        public LinkGenerationMatch(AttributeRouteLinkGenerationEntry entry, bool isFallbackMatch)
+        {
+            _entry = entry;
+            _isFallbackMatch = isFallbackMatch;
+        }
+
+        public AttributeRouteLinkGenerationEntry Entry { get { return _entry; } }
+
+        public bool IsFallbackMatch { get { return _isFallbackMatch; } }
+    }
+}
diff --git a/src/Microsoft.AspNet.Mvc.Core/Routing/AttributeRoute.cs b/src/Microsoft.AspNet.Mvc.Core/Routing/AttributeRoute.cs
@@ -38,8 +38,8 @@ public class AttributeRoute : IRouter
             _routeLogger = loggerFactory.Create<InnerAttributeRoute>();
             _constraintLogger = loggerFactory.Create(typeof(RouteConstraintMatcher).FullName);
 
-			// Force creation of the route to report issues on startup.
-			GetInnerRoute();
+             // Force creation of the route to report issues on startup.
+            GetInnerRoute();
         }
 
         /// <inheritdoc />
@@ -305,4 +305,4 @@ private class RouteInfo
             public string Name { get; set; }
         }
     }
-}
+}
diff --git a/src/Microsoft.AspNet.Mvc.Core/Routing/InnerAttributeRoute.cs b/src/Microsoft.AspNet.Mvc.Core/Routing/InnerAttributeRoute.cs
@@ -154,9 +154,9 @@ public string GetVirtualPath([NotNull] VirtualPathContext context)
             // order. We just need to iterate them and use the first one that can generate a link.
             var matches = _linkGenerationTree.GetMatches(context);
 
-            foreach (var entry in matches)
+            foreach (var match in matches)
             {
-                var path = GenerateLink(context, entry);
+                var path = GenerateLink(context, match.Entry);
                 if (path != null)
                 {
                     context.IsBound = true;

diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/Internal/Routing/LinkGenerationDecisionTreeTest.cs b/test/Microsoft.AspNet.Mvc.Core.Test/Internal/Routing/LinkGenerationDecisionTreeTest.cs
@@ -2,6 +2,7 @@
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
 using System.Collections.Generic;
+using System.Linq;
 using Microsoft.AspNet.Http.Core;
 using Microsoft.AspNet.Mvc.Routing;
 using Microsoft.AspNet.Routing;
@@ -28,7 +29,7 @@ public void SelectSingleEntry_NoCriteria()
             var matches = tree.GetMatches(context);
 
             // Assert
-            Assert.Same(entry, Assert.Single(matches));
+            Assert.Same(entry, Assert.Single(matches).Entry);
         }
 
         [Fact]
@@ -48,7 +49,7 @@ public void SelectSingleEntry_MultipleCriteria()
             var matches = tree.GetMatches(context);
 
             // Assert
-            Assert.Same(entry, Assert.Single(matches));
+            Assert.Same(entry, Assert.Single(matches).Entry);
         }
 
         [Fact]
@@ -68,7 +69,9 @@ public void SelectSingleEntry_MultipleCriteria_AmbientValues()
             var matches = tree.GetMatches(context);
 
             // Assert
-            Assert.Same(entry, Assert.Single(matches));
+            var match = Assert.Single(matches);
+            Assert.Same(entry, match.Entry);
+            Assert.False(match.IsFallbackMatch);
         }
 
         [Fact]
@@ -90,7 +93,9 @@ public void SelectSingleEntry_MultipleCriteria_Replaced()
             var matches = tree.GetMatches(context);
 
             // Assert
-            Assert.Same(entry, Assert.Single(matches));
+            var match = Assert.Single(matches);
+            Assert.Same(entry, match.Entry);
+            Assert.False(match.IsFallbackMatch);
         }
 
         [Fact]
@@ -112,7 +117,9 @@ public void SelectSingleEntry_MultipleCriteria_AmbientValue_Ignored()
             var matches = tree.GetMatches(context);
 
             // Assert
-            Assert.Same(entry, Assert.Single(matches));
+            var match = Assert.Single(matches);
+            Assert.Same(entry, match.Entry);
+            Assert.True(match.IsFallbackMatch);
         }
 
         [Fact]
@@ -179,7 +186,7 @@ public void SelectMultipleEntries_OneDoesntMatch()
             var matches = tree.GetMatches(context);
 
             // Assert
-            Assert.Same(entry1, Assert.Single(matches));
+            Assert.Same(entry1, Assert.Single(matches).Entry);
         }
 
         [Fact]
@@ -202,7 +209,7 @@ public void SelectMultipleEntries_BothMatch_CriteriaSubset()
                 ambientValues: new { controller = "Store", action = "Buy" });
 
             // Act
-            var matches = tree.GetMatches(context);
+            var matches = tree.GetMatches(context).Select(m => m.Entry).ToList();
 
             // Assert
             Assert.Equal(entries, matches);
@@ -226,7 +233,7 @@ public void SelectMultipleEntries_BothMatch_NonOverlappingCriteria()
             var context = CreateContext(new { controller = "Store", action = "Buy", slug = "1234" });
 
             // Act
-            var matches = tree.GetMatches(context);
+            var matches = tree.GetMatches(context).Select(m => m.Entry).ToList();
 
             // Assert
             Assert.Equal(entries, matches);
@@ -253,7 +260,7 @@ public void SelectMultipleEntries_BothMatch_OrderedByOrder()
             var context = CreateContext(new { controller = "Store", action = "Buy" });
 
             // Act
-            var matches = tree.GetMatches(context);
+            var matches = tree.GetMatches(context).Select(m => m.Entry).ToList();
 
             // Assert
             Assert.Equal(entries, matches);
@@ -279,7 +286,7 @@ public void SelectMultipleEntries_BothMatch_OrderedByPrecedence()
             var context = CreateContext(new { controller = "Store", action = "Buy" });
 
             // Act
-            var matches = tree.GetMatches(context);
+            var matches = tree.GetMatches(context).Select(m => m.Entry).ToList();
 
             // Assert
             Assert.Equal(entries, matches);
@@ -305,7 +312,7 @@ public void SelectMultipleEntries_BothMatch_OrderedByTemplate()
             var context = CreateContext(new { controller = "Store", action = "Buy" });
 
             // Act
-            var matches = tree.GetMatches(context);
+            var matches = tree.GetMatches(context).Select(m => m.Entry).ToList();
 
             // Assert
             Assert.Equal(entries, matches);

diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/Routing/AttributeRouteTest.cs b/test/Microsoft.AspNet.Mvc.Core.Test/Routing/AttributeRouteTest.cs
@@ -107,4 +107,4 @@ public async Task AttributeRoute_UsesUpdatedActionDescriptors()
         }
     }
 }
-#endif
+#endif