From 7cb6c1065cce41b517a3e39459572bcf2bf63a99 Mon Sep 17 00:00:00 2001 From: Ryan Nowak Date: Tue, 10 Feb 2015 15:55:54 -0800 Subject: [PATCH] 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. --- .../Routing/LinkGenerationDecisionTree.cs | 49 +++++++++++-------- .../Internal/Routing/LinkGenerationMatch.cs | 23 +++++++++ .../Routing/AttributeRoute.cs | 6 +-- .../Routing/InnerAttributeRoute.cs | 4 +- .../Routing/LinkGenerationDecisionTreeTest.cs | 29 ++++++----- .../Routing/AttributeRouteTest.cs | 2 +- 6 files changed, 76 insertions(+), 37 deletions(-) create mode 100644 src/Microsoft.AspNet.Mvc.Core/Internal/Routing/LinkGenerationMatch.cs diff --git a/src/Microsoft.AspNet.Mvc.Core/Internal/Routing/LinkGenerationDecisionTree.cs b/src/Microsoft.AspNet.Mvc.Core/Internal/Routing/LinkGenerationDecisionTree.cs index 90a2390a44..22d9b20281 100644 --- a/src/Microsoft.AspNet.Mvc.Core/Internal/Routing/LinkGenerationDecisionTree.cs +++ b/src/Microsoft.AspNet.Mvc.Core/Internal/Routing/LinkGenerationDecisionTree.cs @@ -21,11 +21,11 @@ namespace Microsoft.AspNet.Mvc.Internal.Routing new AttributeRouteLinkGenerationEntryClassifier()); } - public List GetMatches(VirtualPathContext context) + public List GetMatches(VirtualPathContext context) { - var results = new List(); - Walk(results, context, _root); - results.Sort(AttributeRouteLinkGenerationEntryComparer.Instance); + var results = new List(); + Walk(results, context, _root, isFallbackPath: false); + results.Sort(LinkGenerationMatchComparer.Instance); return results; } @@ -55,15 +55,16 @@ namespace Microsoft.AspNet.Mvc.Internal.Routing // // The decision tree uses a tree data structure to execute these rules across all candidates at once. private void Walk( - List results, + List results, VirtualPathContext context, - DecisionTreeNode node) + DecisionTreeNode 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 @@ namespace Microsoft.AspNet.Mvc.Internal.Routing DecisionTreeNode 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 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 @@ namespace Microsoft.AspNet.Mvc.Internal.Routing } } - private class AttributeRouteLinkGenerationEntryComparer : IComparer + private class LinkGenerationMatchComparer : IComparer { - 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.Order.CompareTo(y.Order); + return x.Entry.Order.CompareTo(y.Entry.Order); } - if (x.Precedence != y.Precedence) + if (x.IsFallbackMatch != y.IsFallbackMatch) { - return x.Precedence.CompareTo(y.Precedence); + // A fallback match is worse than a non-fallback + return x.IsFallbackMatch.CompareTo(y.IsFallbackMatch); } - return StringComparer.Ordinal.Compare(x.TemplateText, y.TemplateText); + if (x.Entry.Precedence != y.Entry.Precedence) + { + return x.Entry.Precedence.CompareTo(y.Entry.Precedence); + } + + 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 new file mode 100644 index 0000000000..24ad4f1fd7 --- /dev/null +++ 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; } } + } +} \ No newline at end of file diff --git a/src/Microsoft.AspNet.Mvc.Core/Routing/AttributeRoute.cs b/src/Microsoft.AspNet.Mvc.Core/Routing/AttributeRoute.cs index e5cf507313..a5e9418e1d 100644 --- a/src/Microsoft.AspNet.Mvc.Core/Routing/AttributeRoute.cs +++ b/src/Microsoft.AspNet.Mvc.Core/Routing/AttributeRoute.cs @@ -38,8 +38,8 @@ namespace Microsoft.AspNet.Mvc.Routing _routeLogger = loggerFactory.Create(); _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(); } /// @@ -305,4 +305,4 @@ namespace Microsoft.AspNet.Mvc.Routing public string Name { get; set; } } } -} \ No newline at end of file +} diff --git a/src/Microsoft.AspNet.Mvc.Core/Routing/InnerAttributeRoute.cs b/src/Microsoft.AspNet.Mvc.Core/Routing/InnerAttributeRoute.cs index 676bbf67a8..0e6e0492f7 100644 --- a/src/Microsoft.AspNet.Mvc.Core/Routing/InnerAttributeRoute.cs +++ b/src/Microsoft.AspNet.Mvc.Core/Routing/InnerAttributeRoute.cs @@ -154,9 +154,9 @@ namespace Microsoft.AspNet.Mvc.Routing // 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 index cf689641c3..ead897ba78 100644 --- 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 @@ namespace Microsoft.AspNet.Mvc.Internal.Routing var matches = tree.GetMatches(context); // Assert - Assert.Same(entry, Assert.Single(matches)); + Assert.Same(entry, Assert.Single(matches).Entry); } [Fact] @@ -48,7 +49,7 @@ namespace Microsoft.AspNet.Mvc.Internal.Routing var matches = tree.GetMatches(context); // Assert - Assert.Same(entry, Assert.Single(matches)); + Assert.Same(entry, Assert.Single(matches).Entry); } [Fact] @@ -68,7 +69,9 @@ namespace Microsoft.AspNet.Mvc.Internal.Routing 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 @@ namespace Microsoft.AspNet.Mvc.Internal.Routing 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 @@ namespace Microsoft.AspNet.Mvc.Internal.Routing 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 @@ namespace Microsoft.AspNet.Mvc.Internal.Routing var matches = tree.GetMatches(context); // Assert - Assert.Same(entry1, Assert.Single(matches)); + Assert.Same(entry1, Assert.Single(matches).Entry); } [Fact] @@ -202,7 +209,7 @@ namespace Microsoft.AspNet.Mvc.Internal.Routing 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 @@ namespace Microsoft.AspNet.Mvc.Internal.Routing 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 @@ namespace Microsoft.AspNet.Mvc.Internal.Routing 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 @@ namespace Microsoft.AspNet.Mvc.Internal.Routing 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 @@ namespace Microsoft.AspNet.Mvc.Internal.Routing 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 index ec0e7cfd22..6501db9369 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/Routing/AttributeRouteTest.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/Routing/AttributeRouteTest.cs @@ -107,4 +107,4 @@ namespace Microsoft.AspNet.Mvc.Routing } } } -#endif \ No newline at end of file +#endif