From 3d448f71976d094b8faac027776cf75b63f21637 Mon Sep 17 00:00:00 2001 From: Ryan Nowak Date: Fri, 22 Feb 2019 11:44:43 -0800 Subject: [PATCH] Fix #5055 use of page parameter (#7441) This change introduces some new heuristics to make it easier to use 'page' or other resevered parameter names as parameters in URL generation. -- The main change here is to allow the link generation tree to *ignore* a value passed in to URL generation when it conflicts with an endpoint's required values. The main concern of this feature area is "how do we tell whether you are linking to an action or a page?". Routing attempts to do the right thing will requiring very little from the user in terms of expressing intent. In this case, we try to tell the difference between an attempt to generate a link to an action due to the presence of the 'action' parameter and absence of the 'page' parameter. This obviously doesn't work when you want to use 'page' as a non-reserved parameter in an action. The same case occurs for pages, but users are already used to the idea that 'action' is a reserved word in MVC. We can loosen this restriction when the value that's supplied for 'page' is known not to be any existing value of the 'page' route value. This approach seems somewhat reasonable but has many of the problems inherent to this area. When it fails (the value you want to use for 'page' causes a conflict) - it's going to be esoteric and hard to understand. --- .../Internal/LinkGenerationDecisionTree.cs | 32 +- .../LinkGenerationDecisionTreeTest.cs | 381 +++++++++++++++++- src/Mvc/Mvc.sln | 15 + .../Mvc.FunctionalTests/RoutingTestsBase.cs | 11 + .../Controllers/PageParameterController.cs | 23 ++ 5 files changed, 460 insertions(+), 2 deletions(-) create mode 100644 src/Mvc/test/WebSites/RoutingWebSite/Controllers/PageParameterController.cs diff --git a/src/Http/Routing/src/Internal/LinkGenerationDecisionTree.cs b/src/Http/Routing/src/Internal/LinkGenerationDecisionTree.cs index 1561ed868c..a1aee949c9 100644 --- a/src/Http/Routing/src/Internal/LinkGenerationDecisionTree.cs +++ b/src/Http/Routing/src/Internal/LinkGenerationDecisionTree.cs @@ -21,12 +21,29 @@ namespace Microsoft.AspNetCore.Routing.Internal private static readonly RouteValueDictionary EmptyAmbientValues = new RouteValueDictionary(); private readonly DecisionTreeNode _root; + private readonly Dictionary> _knownValues; public LinkGenerationDecisionTree(IReadOnlyList entries) { _root = DecisionTreeBuilder.GenerateTree( entries, new OutboundMatchClassifier()); + + _knownValues = new Dictionary>(StringComparer.OrdinalIgnoreCase); + for (var i = 0; i < entries.Count; i++) + { + var entry = entries[i]; + foreach (var kvp in entry.Entry.RequiredLinkValues) + { + if (!_knownValues.TryGetValue(kvp.Key, out var values)) + { + values = new HashSet(RouteValueEqualityComparer.Default); + _knownValues.Add(kvp.Key, values); + } + + values.Add(kvp.Value ?? string.Empty); + } + } } public IList GetMatches(RouteValueDictionary values, RouteValueDictionary ambientValues) @@ -93,6 +110,19 @@ namespace Microsoft.AspNetCore.Routing.Internal { 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 { @@ -210,4 +240,4 @@ namespace Microsoft.AspNetCore.Routing.Internal } } } -} \ No newline at end of file +} diff --git a/src/Http/Routing/test/UnitTests/Internal/LinkGenerationDecisionTreeTest.cs b/src/Http/Routing/test/UnitTests/Internal/LinkGenerationDecisionTreeTest.cs index 94568c2cd8..effcda3bd8 100644 --- a/src/Http/Routing/test/UnitTests/Internal/LinkGenerationDecisionTreeTest.cs +++ b/src/Http/Routing/test/UnitTests/Internal/LinkGenerationDecisionTreeTest.cs @@ -340,6 +340,385 @@ namespace Microsoft.AspNetCore.Routing.Internal.Routing Assert.Equal(entries, matches); } + [Fact] + public void GetMatches_ControllersWithArea_AllValuesExplicit() + { + // Arrange + var entries = new List(); + + var entry1 = CreateMatch(new { controller = "Store", action = "Buy", area = (string)null, }); + entry1.Entry.RouteTemplate = TemplateParser.Parse("a"); + entries.Add(entry1); + + var entry2 = CreateMatch(new { controller = "Store", action = "Buy", area = "Admin" }); + entry2.Entry.RouteTemplate = TemplateParser.Parse("b"); + entries.Add(entry2); + + var tree = new LinkGenerationDecisionTree(entries); + + var context = CreateContext(new { controller = "Store", action = "Buy", area = "Admin" }); + + // Act + var matches = tree.GetMatches(context.Values, context.AmbientValues).Select(m => m.Match).ToList(); + + // Assert + // Assert + Assert.Collection( + matches, + m => { Assert.Same(entry2, m); }); + } + + [Fact] + public void GetMatches_ControllersWithArea_SomeValuesAmbient() + { + // Arrange + var entries = new List(); + + var entry1 = CreateMatch(new { controller = "Store", action = "Buy", area = (string)null, }); + entry1.Entry.RouteTemplate = TemplateParser.Parse("a"); + entries.Add(entry1); + + var entry2 = CreateMatch(new { controller = "Store", action = "Buy", area = "Admin" }); + entry2.Entry.RouteTemplate = TemplateParser.Parse("b"); + entries.Add(entry2); + + var tree = new LinkGenerationDecisionTree(entries); + + var context = CreateContext(new { controller = "Store", }, new { action = "Buy", area = "Admin", }); + + // Act + var matches = tree.GetMatches(context.Values, context.AmbientValues).Select(m => m.Match).ToList(); + + // Assert + Assert.Collection( + matches, + m => { Assert.Same(entry2, m); }, + m => { Assert.Same(entry1, m); }); + } + + [Fact] + public void GetMatches_ControllersWithArea_AllValuesAmbient() + { + // Arrange + var entries = new List(); + + var entry1 = CreateMatch(new { controller = "Store", action = "Buy", area = (string)null, }); + entry1.Entry.RouteTemplate = TemplateParser.Parse("a"); + entries.Add(entry1); + + var entry2 = CreateMatch(new { controller = "Store", action = "Buy", area = "Admin" }); + entry2.Entry.RouteTemplate = TemplateParser.Parse("b"); + entries.Add(entry2); + + var tree = new LinkGenerationDecisionTree(entries); + + var context = CreateContext(new { }, new { controller = "Store", action = "Buy", area = "Admin", }); + + // Act + var matches = tree.GetMatches(context.Values, context.AmbientValues).Select(m => m.Match).ToList(); + + // Assert + Assert.Collection( + matches, + m => { Assert.Same(entry2, m); }, + m => { Assert.Same(entry1, m); }); + } + + [Fact] + public void GetMatches_PagesWithArea_AllValuesExplicit() + { + // Arrange + var entries = new List(); + + var entry1 = CreateMatch(new { page = "/Store/Buy", area = (string)null, }); + entry1.Entry.RouteTemplate = TemplateParser.Parse("a"); + entries.Add(entry1); + + var entry2 = CreateMatch(new { page = "/Store/Buy", area = "Admin" }); + entry2.Entry.RouteTemplate = TemplateParser.Parse("b"); + entries.Add(entry2); + + var tree = new LinkGenerationDecisionTree(entries); + + var context = CreateContext(new { page = "/Store/Buy", area = "Admin" }); + + // Act + var matches = tree.GetMatches(context.Values, context.AmbientValues).Select(m => m.Match).ToList(); + + // Assert + Assert.Collection( + matches, + m => { Assert.Same(entry2, m); }); + } + + [Fact] + public void GetMatches_PagesWithArea_SomeValuesAmbient() + { + // Arrange + var entries = new List(); + + var entry1 = CreateMatch(new { page = "/Store/Buy", area = (string)null, }); + entry1.Entry.RouteTemplate = TemplateParser.Parse("a"); + entries.Add(entry1); + + var entry2 = CreateMatch(new { page = "/Store/Buy", area = "Admin" }); + entry2.Entry.RouteTemplate = TemplateParser.Parse("b"); + entries.Add(entry2); + + var tree = new LinkGenerationDecisionTree(entries); + + var context = CreateContext(new { page = "/Store/Buy", }, new { area = "Admin", }); + + // Act + var matches = tree.GetMatches(context.Values, context.AmbientValues).Select(m => m.Match).ToList(); + + // Assert + Assert.Collection( + matches, + m => { Assert.Same(entry2, m); }, + m => { Assert.Same(entry1, m); }); + } + + [Fact] + public void GetMatches_PagesWithArea_AllValuesAmbient() + { + // Arrange + var entries = new List(); + + var entry1 = CreateMatch(new { page = "/Store/Buy", area = (string)null, }); + entry1.Entry.RouteTemplate = TemplateParser.Parse("a"); + entries.Add(entry1); + + var entry2 = CreateMatch(new { page = "/Store/Buy", area = "Admin" }); + entry2.Entry.RouteTemplate = TemplateParser.Parse("b"); + entries.Add(entry2); + + var tree = new LinkGenerationDecisionTree(entries); + + var context = CreateContext(new { }, new { page = "/Store/Buy", area = "Admin", }); + + // Act + var matches = tree.GetMatches(context.Values, context.AmbientValues).Select(m => m.Match).ToList(); + + // Assert + Assert.Collection( + matches, + m => { Assert.Same(entry2, m); }, + m => { Assert.Same(entry1, m); }); + } + + [Fact] + public void GetMatches_LinkToControllerFromPage() + { + // Arrange + var entries = new List(); + + var entry1 = CreateMatch(new { controller = "Home", action = "Index", area = (string)null, page = (string)null, }); + entry1.Entry.RouteTemplate = TemplateParser.Parse("a"); + entries.Add(entry1); + + var entry2 = CreateMatch(new { page = "/Store/Buy", area = (string)null, controller = (string)null, action = (string)null, }); + entry2.Entry.RouteTemplate = TemplateParser.Parse("b"); + entries.Add(entry2); + + var tree = new LinkGenerationDecisionTree(entries); + + var context = CreateContext(new { controller = "Home", action = "Index", }, new { page = "/Store/Buy", }); + + // Act + var matches = tree.GetMatches(context.Values, context.AmbientValues).Select(m => m.Match).ToList(); + + // Assert + Assert.Collection( + matches, + m => { Assert.Same(entry1, m); }); + } + + [Fact] + public void GetMatches_LinkToControllerFromPage_WithArea() + { + // Arrange + var entries = new List(); + + var entry1 = CreateMatch(new { controller = "Home", action = "Index", area = "Admin", page = (string)null, }); + entry1.Entry.RouteTemplate = TemplateParser.Parse("a"); + entries.Add(entry1); + + var entry2 = CreateMatch(new { page = "/Store/Buy", area = "Admin", controller = (string)null, action = (string)null, }); + entry2.Entry.RouteTemplate = TemplateParser.Parse("b"); + entries.Add(entry2); + + var tree = new LinkGenerationDecisionTree(entries); + + var context = CreateContext(new { controller = "Home", action = "Index", }, new { page = "/Store/Buy", area = "Admin", }); + + // Act + var matches = tree.GetMatches(context.Values, context.AmbientValues).Select(m => m.Match).ToList(); + + // Assert + Assert.Collection( + matches, + m => { Assert.Same(entry1, m); }); + } + + [Fact] + public void GetMatches_LinkToControllerFromPage_WithPageValue() + { + // Arrange + var entries = new List(); + + var entry1 = CreateMatch(new { controller = "Home", action = "Index", area = (string)null, page = (string)null, }); + entry1.Entry.RouteTemplate = TemplateParser.Parse("a"); + entries.Add(entry1); + + var entry2 = CreateMatch(new { page = "/Store/Buy", area = (string)null, controller = (string)null, action = (string)null, }); + entry2.Entry.RouteTemplate = TemplateParser.Parse("b"); + entries.Add(entry2); + + var tree = new LinkGenerationDecisionTree(entries); + + var context = CreateContext(new { controller = "Home", action = "Index", page = "16", }, new { page = "/Store/Buy", }); + + // Act + var matches = tree.GetMatches(context.Values, context.AmbientValues).Select(m => m.Match).ToList(); + + // Assert + Assert.Collection( + matches, + m => { Assert.Same(entry1, m); }); + } + + [Fact] + public void GetMatches_LinkToControllerFromPage_WithPageValueAmbiguous() + { + // Arrange + var entries = new List(); + + var entry1 = CreateMatch(new { controller = "Home", action = "Index", area = (string)null, page = (string)null, }); + entry1.Entry.RouteTemplate = TemplateParser.Parse("a"); + entries.Add(entry1); + + var entry2 = CreateMatch(new { page = "/Store/Buy", area = (string)null, controller = (string)null, action = (string)null, }); + entry2.Entry.RouteTemplate = TemplateParser.Parse("b"); + entries.Add(entry2); + + var tree = new LinkGenerationDecisionTree(entries); + + var context = CreateContext(new { controller = "Home", action = "Index", page = "/Store/Buy", }, new { page = "/Store/Buy", }); + + // Act + var matches = tree.GetMatches(context.Values, context.AmbientValues).Select(m => m.Match).ToList(); + + // Assert + Assert.Empty(matches); + } + + [Fact] + public void GetMatches_LinkToPageFromController() + { + // Arrange + var entries = new List(); + + var entry1 = CreateMatch(new { controller = "Home", action = "Index", area = (string)null, page = (string)null, }); + entry1.Entry.RouteTemplate = TemplateParser.Parse("a"); + entries.Add(entry1); + + var entry2 = CreateMatch(new { page = "/Store/Buy", area = (string)null, controller = (string)null, action = (string)null, }); + entry2.Entry.RouteTemplate = TemplateParser.Parse("b"); + entries.Add(entry2); + + var tree = new LinkGenerationDecisionTree(entries); + + var context = CreateContext(new { page = "/Store/Buy", }, new { controller = "Home", action = "Index", }); + + // Act + var matches = tree.GetMatches(context.Values, context.AmbientValues).Select(m => m.Match).ToList(); + + // Assert + Assert.Collection( + matches, + m => { Assert.Same(entry2, m); }); + } + + [Fact] + public void GetMatches_LinkToPageFromController_WithArea() + { + // Arrange + var entries = new List(); + + var entry1 = CreateMatch(new { controller = "Home", action = "Index", area = "Admin", page = (string)null, }); + entry1.Entry.RouteTemplate = TemplateParser.Parse("a"); + entries.Add(entry1); + + var entry2 = CreateMatch(new { page = "/Store/Buy", area = "Admin", controller = (string)null, action = (string)null, }); + entry2.Entry.RouteTemplate = TemplateParser.Parse("b"); + entries.Add(entry2); + + var tree = new LinkGenerationDecisionTree(entries); + + var context = CreateContext(new { page = "/Store/Buy", }, new { controller = "Home", action = "Index", area = "Admin", }); + + // Act + var matches = tree.GetMatches(context.Values, context.AmbientValues).Select(m => m.Match).ToList(); + + // Assert + Assert.Collection( + matches, + m => { Assert.Same(entry2, m); }); + } + + [Fact] + public void GetMatches_LinkToPageFromController_WithActionValue() + { + // Arrange + var entries = new List(); + + var entry1 = CreateMatch(new { controller = "Home", action = "Index", area = (string)null, page = (string)null, }); + entry1.Entry.RouteTemplate = TemplateParser.Parse("a"); + entries.Add(entry1); + + var entry2 = CreateMatch(new { page = "/Store/Buy", area = (string)null, controller = (string)null, action = (string)null, }); + entry2.Entry.RouteTemplate = TemplateParser.Parse("b"); + entries.Add(entry2); + + var tree = new LinkGenerationDecisionTree(entries); + + var context = CreateContext(new { page = "/Store/Buy", action = "buy", }, new { controller = "Home", action = "Index", page = "16", }); + + // Act + var matches = tree.GetMatches(context.Values, context.AmbientValues).Select(m => m.Match).ToList(); + + // Assert + Assert.Collection( + matches, + m => { Assert.Same(entry2, m); }); + } + + [Fact] + public void GetMatches_LinkToPageFromController_WithActionValueAmbiguous() + { + // Arrange + var entries = new List(); + + var entry1 = CreateMatch(new { controller = "Home", action = "Index", area = (string)null, page = (string)null, }); + entry1.Entry.RouteTemplate = TemplateParser.Parse("a"); + entries.Add(entry1); + + var entry2 = CreateMatch(new { page = "/Store/Buy", area = (string)null, controller = (string)null, action = (string)null, }); + entry2.Entry.RouteTemplate = TemplateParser.Parse("b"); + entries.Add(entry2); + + var tree = new LinkGenerationDecisionTree(entries); + + var context = CreateContext(new { page = "/Store/Buy", action = "Index", }, new { controller = "Home", action = "Index", page = "16", }); + + // Act + var matches = tree.GetMatches(context.Values, context.AmbientValues).Select(m => m.Match).ToList(); + + // Assert + Assert.Empty(matches); + } + [Fact] public void ToDebuggerDisplayString_GivesAFlattenedTree() { @@ -392,4 +771,4 @@ namespace Microsoft.AspNetCore.Routing.Internal.Routing return context; } } -} \ No newline at end of file +} diff --git a/src/Mvc/Mvc.sln b/src/Mvc/Mvc.sln index f8fd01d2dd..640bcddd59 100644 --- a/src/Mvc/Mvc.sln +++ b/src/Mvc/Mvc.sln @@ -293,6 +293,8 @@ Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Microsoft.AspNetCore.Mvc.Ra EndProject Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Microsoft.AspNetCore.Mvc.Razor.RuntimeCompilation.Test", "Mvc.Razor.RuntimeCompilation\test\Microsoft.AspNetCore.Mvc.Razor.RuntimeCompilation.Test.csproj", "{2FFB927A-C039-4A1F-83A5-CBBB664A0E81}" EndProject +Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Microsoft.AspNetCore.Components.Server", "..\Components\Server\src\Microsoft.AspNetCore.Components.Server.csproj", "{916BF32D-6896-4D02-BBD1-A72878FDBDFF}" +EndProject Global GlobalSection(SolutionConfigurationPlatforms) = preSolution Debug|Any CPU = Debug|Any CPU @@ -1641,6 +1643,18 @@ Global {2FFB927A-C039-4A1F-83A5-CBBB664A0E81}.Release|Mixed Platforms.Build.0 = Release|Any CPU {2FFB927A-C039-4A1F-83A5-CBBB664A0E81}.Release|x86.ActiveCfg = Release|Any CPU {2FFB927A-C039-4A1F-83A5-CBBB664A0E81}.Release|x86.Build.0 = Release|Any CPU + {916BF32D-6896-4D02-BBD1-A72878FDBDFF}.Debug|Any CPU.ActiveCfg = Debug|Any CPU + {916BF32D-6896-4D02-BBD1-A72878FDBDFF}.Debug|Any CPU.Build.0 = Debug|Any CPU + {916BF32D-6896-4D02-BBD1-A72878FDBDFF}.Debug|Mixed Platforms.ActiveCfg = Debug|Any CPU + {916BF32D-6896-4D02-BBD1-A72878FDBDFF}.Debug|Mixed Platforms.Build.0 = Debug|Any CPU + {916BF32D-6896-4D02-BBD1-A72878FDBDFF}.Debug|x86.ActiveCfg = Debug|Any CPU + {916BF32D-6896-4D02-BBD1-A72878FDBDFF}.Debug|x86.Build.0 = Debug|Any CPU + {916BF32D-6896-4D02-BBD1-A72878FDBDFF}.Release|Any CPU.ActiveCfg = Release|Any CPU + {916BF32D-6896-4D02-BBD1-A72878FDBDFF}.Release|Any CPU.Build.0 = Release|Any CPU + {916BF32D-6896-4D02-BBD1-A72878FDBDFF}.Release|Mixed Platforms.ActiveCfg = Release|Any CPU + {916BF32D-6896-4D02-BBD1-A72878FDBDFF}.Release|Mixed Platforms.Build.0 = Release|Any CPU + {916BF32D-6896-4D02-BBD1-A72878FDBDFF}.Release|x86.ActiveCfg = Release|Any CPU + {916BF32D-6896-4D02-BBD1-A72878FDBDFF}.Release|x86.Build.0 = Release|Any CPU EndGlobalSection GlobalSection(SolutionProperties) = preSolution HideSolutionNode = FALSE @@ -1758,6 +1772,7 @@ Global {F49A6E01-BFC5-4CEB-9A2C-19DDD3539510} = {25C08DED-1C7D-4C6D-B1CC-F340C1B21DE7} {0CE75D4A-4EFD-434A-99CD-7776AE2BFD39} = {1261FF02-C7F8-4395-AA8A-29F69FC9870B} {2FFB927A-C039-4A1F-83A5-CBBB664A0E81} = {1261FF02-C7F8-4395-AA8A-29F69FC9870B} + {916BF32D-6896-4D02-BBD1-A72878FDBDFF} = {5FE3048A-E96B-44F8-A7C4-FC590D7E04B4} EndGlobalSection GlobalSection(ExtensibilityGlobals) = postSolution SolutionGuid = {63D344F6-F86D-40E6-85B9-0AABBE338C4A} diff --git a/src/Mvc/test/Mvc.FunctionalTests/RoutingTestsBase.cs b/src/Mvc/test/Mvc.FunctionalTests/RoutingTestsBase.cs index d7d51e19c3..3ff0dd20fd 100644 --- a/src/Mvc/test/Mvc.FunctionalTests/RoutingTestsBase.cs +++ b/src/Mvc/test/Mvc.FunctionalTests/RoutingTestsBase.cs @@ -1533,6 +1533,17 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests Assert.Equal("Hello from middleware after routing", content); } + [Fact] + public async Task CanUseLinkGeneration_To_ConventionalActionWithPageParameter() + { + // Act + var response = await Client.GetAsync("/PageParameter/LinkToPageParameter"); + + // Assert + await response.AssertStatusCodeAsync(HttpStatusCode.OK); + var content = await response.Content.ReadAsStringAsync(); + Assert.Equal("/PageParameter/PageParameter?page=17", content); + } protected static LinkBuilder LinkFrom(string url) { diff --git a/src/Mvc/test/WebSites/RoutingWebSite/Controllers/PageParameterController.cs b/src/Mvc/test/WebSites/RoutingWebSite/Controllers/PageParameterController.cs new file mode 100644 index 0000000000..3174963111 --- /dev/null +++ b/src/Mvc/test/WebSites/RoutingWebSite/Controllers/PageParameterController.cs @@ -0,0 +1,23 @@ +// 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 Microsoft.AspNetCore.Mvc; + +namespace RoutingWebSite.Controllers +{ + public class PageParameterController : Controller + { + // We've had issues with using 'page' as a parameter in tandem with conventional + // routing + razor pages. + public ActionResult PageParameter(string page) + { + return Content($"page={page}"); + } + + [HttpGet("/PageParameter/LinkToPageParameter")] + public ActionResult LinkToPageParameter() + { + return Content(Url.Action(nameof(PageParameter), new { page = "17", })); + } + } +}