From 232b73a1516f7eae1d3a3697aabb2c1c7ffc530b Mon Sep 17 00:00:00 2001 From: Ryan Nowak Date: Fri, 28 Apr 2017 12:22:04 -0700 Subject: [PATCH] Fix aspnet/Mvc#6218 This fixes the case described in the comments in TemplateBinder. This case is much more common for pages which is why we're only seeing it now. We've had this issue for all of 1.0.0 in both conventional and attribute routing. --- .../Template/TemplateBinder.cs | 22 +++++ .../RouteTest.cs | 97 +++++++++++++++++++ .../Tree/TreeRouterTest.cs | 21 ++++ 3 files changed, 140 insertions(+) diff --git a/src/Microsoft.AspNetCore.Routing/Template/TemplateBinder.cs b/src/Microsoft.AspNetCore.Routing/Template/TemplateBinder.cs index 809a7a6913..68770fd2bc 100644 --- a/src/Microsoft.AspNetCore.Routing/Template/TemplateBinder.cs +++ b/src/Microsoft.AspNetCore.Routing/Template/TemplateBinder.cs @@ -64,6 +64,9 @@ namespace Microsoft.AspNetCore.Routing.Template // If the URI had ordered parameters a="1", b="2", c="3" and the new values // specified that b="9", then we need to invalidate everything after it. The new // values should then be a="1", b="9", c=. + // + // We also handle the case where a parameter is optional but has no value - we shouldn't + // accept additional parameters that appear *after* that parameter. for (var i = 0; i < _template.Parameters.Count; i++) { var parameter = _template.Parameters[i]; @@ -87,6 +90,25 @@ namespace Microsoft.AspNetCore.Routing.Template } } + if (!hasNewParameterValue && + !hasCurrentParameterValue && + _defaults?.ContainsKey(parameter.Name) != true) + { + // This is an unsatisfied parameter value and there are no defaults. We might still + // be able to generate a URL but we should stop 'accepting' ambient values. + // + // This might be a case like: + // template: a/{b?}/{c?} + // ambient: { c = 17 } + // values: { } + // + // We can still generate a URL from this ("/a") but we shouldn't accept 'c' because + // we can't use it. + // + // In the example above we should fall into this block for 'b'. + break; + } + // If the parameter is a match, add it to the list of values we will use for URI generation if (hasNewParameterValue) { diff --git a/test/Microsoft.AspNetCore.Routing.Tests/RouteTest.cs b/test/Microsoft.AspNetCore.Routing.Tests/RouteTest.cs index 1514ed4c34..e7ce265e48 100644 --- a/test/Microsoft.AspNetCore.Routing.Tests/RouteTest.cs +++ b/test/Microsoft.AspNetCore.Routing.Tests/RouteTest.cs @@ -1372,6 +1372,103 @@ namespace Microsoft.AspNetCore.Routing Assert.Empty(pathData.DataTokens); } + [Fact] + public void GetVirtualPath_TwoOptionalParameters_OneValueFromAmbientValues() + { + // Arrange + var route = CreateRoute( + template: "a/{b=15}/{c?}/{d?}", + defaults: null, + handleRequest: true, + constraints: null); + + var context = CreateVirtualPathContext( + values: new { }, + ambientValues: new { c = "17" }); + + // Act + var pathData = route.GetVirtualPath(context); + + // Assert + Assert.NotNull(pathData); + Assert.Equal("/a/15/17", pathData.VirtualPath); + Assert.Same(route, pathData.Router); + Assert.Empty(pathData.DataTokens); + } + + + [Fact] + public void GetVirtualPath_OptionalParameterAfterDefault_OneValueFromAmbientValues() + { + // Arrange + var route = CreateRoute( + template: "a/{b=15}/{c?}", + defaults: null, + handleRequest: true, + constraints: null); + + var context = CreateVirtualPathContext( + values: new { }, + ambientValues: new { c = "17" }); + + // Act + var pathData = route.GetVirtualPath(context); + + // Assert + Assert.NotNull(pathData); + Assert.Equal("/a/15/17", pathData.VirtualPath); + Assert.Same(route, pathData.Router); + Assert.Empty(pathData.DataTokens); + } + + [Fact] + public void GetVirtualPath_TwoOptionalParametersAfterDefault_OneValueFromAmbientValues() + { + // Arrange + var route = CreateRoute( + template: "a/{b=15}/{c?}/{d?}", + defaults: null, + handleRequest: true, + constraints: null); + + var context = CreateVirtualPathContext( + values: new { }, + ambientValues: new { c = "17" }); + + // Act + var pathData = route.GetVirtualPath(context); + + // Assert + Assert.NotNull(pathData); + Assert.Equal("/a/15/17", pathData.VirtualPath); + Assert.Same(route, pathData.Router); + Assert.Empty(pathData.DataTokens); + } + + [Fact] + public void GetVirtualPath_TwoOptionalParametersAfterDefault_LastValueFromAmbientValues() + { + // Arrange + var route = CreateRoute( + template: "a/{b=15}/{c?}/{d?}", + defaults: null, + handleRequest: true, + constraints: null); + + var context = CreateVirtualPathContext( + values: new { }, + ambientValues: new { d = "17" }); + + // Act + var pathData = route.GetVirtualPath(context); + + // Assert + Assert.NotNull(pathData); + Assert.Equal("/a", pathData.VirtualPath); + Assert.Same(route, pathData.Router); + Assert.Empty(pathData.DataTokens); + } + private static VirtualPathContext CreateVirtualPathContext(object values) { return CreateVirtualPathContext(new RouteValueDictionary(values), null); diff --git a/test/Microsoft.AspNetCore.Routing.Tests/Tree/TreeRouterTest.cs b/test/Microsoft.AspNetCore.Routing.Tests/Tree/TreeRouterTest.cs index 00fd208ee9..eabd339948 100644 --- a/test/Microsoft.AspNetCore.Routing.Tests/Tree/TreeRouterTest.cs +++ b/test/Microsoft.AspNetCore.Routing.Tests/Tree/TreeRouterTest.cs @@ -1317,6 +1317,27 @@ namespace Microsoft.AspNetCore.Routing.Tree Assert.Empty(pathData.DataTokens); } + [Fact] + public void TreeRouter_GenerateLink_Match_HasTwoOptionalParametersWithoutValues() + { + // Arrange + var builder = CreateBuilder(); + MapOutboundEntry(builder, "Customers/SeparatePageModels/{handler?}/{id?}", new { page = "/Customers/SeparatePageModels/Index" }); + var route = builder.Build(); + + var context = CreateVirtualPathContext(new { page = "/Customers/SeparatePageModels/Index" }, new { page = "/Customers/SeparatePageModels/Edit", id = "17" }); + + // Act + var pathData = route.GetVirtualPath(context); + + // Assert + Assert.NotNull(pathData); + Assert.Equal("/Customers/SeparatePageModels", pathData.VirtualPath); + Assert.Same(route, pathData.Router); + Assert.Empty(pathData.DataTokens); + } + + [Fact] public void TreeRouter_GenerateLink_Match_WithParameters() {