From 74a3063c45d24f16e2585701bfa7a7ffcbc64644 Mon Sep 17 00:00:00 2001 From: jacalvar Date: Mon, 26 Sep 2016 09:55:56 -0700 Subject: [PATCH] [Fixes #357] Fix behavior when optional parameters are defined within a route and not at the end --- samples/RoutingSample.Web/project.json | 3 +- .../Properties/AssemblyInfo.cs | 2 + .../Tree/TreeRouteBuilder.cs | 39 +++- .../Tree/TreeRouter.cs | 5 +- .../project.json | 3 +- .../Tree/TreeRouteBuilderTest.cs | 187 +++++++++++++++++- .../Tree/TreeRouterTest.cs | 87 +++++++- .../project.json | 3 +- 8 files changed, 320 insertions(+), 9 deletions(-) diff --git a/samples/RoutingSample.Web/project.json b/samples/RoutingSample.Web/project.json index 2465c4ef63..b07f4d6754 100644 --- a/samples/RoutingSample.Web/project.json +++ b/samples/RoutingSample.Web/project.json @@ -5,7 +5,8 @@ "Microsoft.AspNetCore.Routing": "1.1.0-*" }, "buildOptions": { - "emitEntryPoint": true + "emitEntryPoint": true, + "keyFile": "../../tools/Key.snk" }, "frameworks": { "net451": {}, diff --git a/src/Microsoft.AspNetCore.Routing/Properties/AssemblyInfo.cs b/src/Microsoft.AspNetCore.Routing/Properties/AssemblyInfo.cs index 76feceeff0..8dc98fff9d 100644 --- a/src/Microsoft.AspNetCore.Routing/Properties/AssemblyInfo.cs +++ b/src/Microsoft.AspNetCore.Routing/Properties/AssemblyInfo.cs @@ -3,9 +3,11 @@ using System.Reflection; using System.Resources; +using System.Runtime.CompilerServices; [assembly: AssemblyMetadata("Serviceable", "True")] [assembly: NeutralResourcesLanguage("en-us")] [assembly: AssemblyCompany("Microsoft Corporation.")] [assembly: AssemblyCopyright("© Microsoft Corporation. All rights reserved.")] [assembly: AssemblyProduct("Microsoft ASP.NET Core")] +[assembly: InternalsVisibleTo("Microsoft.AspNetCore.Routing.Tests, PublicKey=0024000004800000940000000602000000240000525341310004000001000100f33a29044fa9d740c9b3213a93e57c84b472c84e0b8a0e1ae48e67a9f8f6de9d5f7f3d52ac23e48ac51801f1dc950abe901da34d2a9e3baadb141a17c77ef3c565dd5ee5054b91cf63bb3c6ab83f72ab3aafe93d0fc3c2348b764fafb0b1c0733de51459aeab46580384bf9d74c4e28164b7cde247f891ba07891c9d872ad2bb")] diff --git a/src/Microsoft.AspNetCore.Routing/Tree/TreeRouteBuilder.cs b/src/Microsoft.AspNetCore.Routing/Tree/TreeRouteBuilder.cs index 978037c26f..e5f7d80823 100644 --- a/src/Microsoft.AspNetCore.Routing/Tree/TreeRouteBuilder.cs +++ b/src/Microsoft.AspNetCore.Routing/Tree/TreeRouteBuilder.cs @@ -333,7 +333,13 @@ namespace Microsoft.AspNetCore.Routing.Tree continue; } - if (part.IsParameter && (part.IsOptional || part.IsCatchAll || part.DefaultValue != null)) + // We accept templates that have intermediate optional values, but we ignore + // those values for route matching. For that reason, we need to add the entry + // to the list of matches, only if the remaining segments are optional. For example: + // /{controller}/{action=Index}/{id} will be equivalent to /{controller}/{action}/{id} + // for the purposes of route matching. + if (part.IsParameter && + RemainingSegmentsAreOptional(entry.RouteTemplate.Segments, i)) { current.Matches.Add(new InboundMatch() { Entry = entry, TemplateMatcher = matcher }); } @@ -392,5 +398,36 @@ namespace Microsoft.AspNetCore.Routing.Tree return result == 0 ? x.Entry.RouteTemplate.TemplateText.CompareTo(y.Entry.RouteTemplate.TemplateText) : result; }); } + + private static bool RemainingSegmentsAreOptional(IList segments, int currentParameterIndex) + { + for (var i = currentParameterIndex; i < segments.Count; i++) + { + if (!segments[i].IsSimple) + { + // /{complex}-{segment} + return false; + } + + var part = segments[i].Parts[0]; + if (!part.IsParameter) + { + // /literal + return false; + } + + var isOptionlCatchAllOrHasDefaultValue = part.IsOptional || + part.IsCatchAll || + part.DefaultValue != null; + + if (!isOptionlCatchAllOrHasDefaultValue) + { + // /{parameter} + return false; + } + } + + return true; + } } } diff --git a/src/Microsoft.AspNetCore.Routing/Tree/TreeRouter.cs b/src/Microsoft.AspNetCore.Routing/Tree/TreeRouter.cs index f14e1860d0..160b4120ae 100644 --- a/src/Microsoft.AspNetCore.Routing/Tree/TreeRouter.cs +++ b/src/Microsoft.AspNetCore.Routing/Tree/TreeRouter.cs @@ -7,7 +7,6 @@ using System.Collections.Generic; using System.Diagnostics; using System.Text.Encodings.Web; using System.Threading.Tasks; -using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Routing.Internal; using Microsoft.AspNetCore.Routing.Logging; using Microsoft.AspNetCore.Routing.Template; @@ -110,7 +109,7 @@ namespace Microsoft.AspNetCore.Routing.Tree if (_namedEntries.TryGetValue(entry.RouteName, out namedMatch) && !string.Equals( namedMatch.Entry.RouteTemplate.TemplateText, - entry.RouteTemplate.TemplateText, + entry.RouteTemplate.TemplateText, StringComparison.OrdinalIgnoreCase)) { throw new ArgumentException( @@ -134,6 +133,8 @@ namespace Microsoft.AspNetCore.Routing.Tree /// public int Version { get; } + internal IEnumerable MatchingTrees => _trees; + /// public VirtualPathData GetVirtualPath(VirtualPathContext context) { diff --git a/test/Microsoft.AspNetCore.Routing.FunctionalTests/project.json b/test/Microsoft.AspNetCore.Routing.FunctionalTests/project.json index 46d54050aa..8216ac62cf 100644 --- a/test/Microsoft.AspNetCore.Routing.FunctionalTests/project.json +++ b/test/Microsoft.AspNetCore.Routing.FunctionalTests/project.json @@ -1,6 +1,7 @@ { "buildOptions": { - "warningsAsErrors": true + "warningsAsErrors": true, + "keyFile": "../../tools/Key.snk" }, "dependencies": { "dotnet-test-xunit": "2.2.0-*", diff --git a/test/Microsoft.AspNetCore.Routing.Tests/Tree/TreeRouteBuilderTest.cs b/test/Microsoft.AspNetCore.Routing.Tests/Tree/TreeRouteBuilderTest.cs index a1ba058d17..8007a693c0 100644 --- a/test/Microsoft.AspNetCore.Routing.Tests/Tree/TreeRouteBuilderTest.cs +++ b/test/Microsoft.AspNetCore.Routing.Tests/Tree/TreeRouteBuilderTest.cs @@ -5,11 +5,14 @@ using System; using System.Collections.Generic; using System.Linq; using System.Text.Encodings.Web; +using Microsoft.AspNetCore.Routing.Constraints; using Microsoft.AspNetCore.Routing.Internal; using Microsoft.AspNetCore.Routing.Template; using Microsoft.AspNetCore.Testing; +using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging.Testing; using Microsoft.Extensions.ObjectPool; +using Microsoft.Extensions.Options; using Moq; using Xunit; @@ -70,13 +73,185 @@ namespace Microsoft.AspNetCore.Routing.Tree builder.Build(); } + [Fact] + public void TreeRouter_BuildDoesNotAddIntermediateMatchingNodes_ForRoutesWithIntermediateParametersWithDefaultValues() + { + // Arrange + var builder = CreateBuilder(); + + builder.MapInbound( + Mock.Of(), + TemplateParser.Parse("a/{b=3}/c"), + "Intermediate", + order: 0); + + // Act + var tree = builder.Build(); + + // Assert + Assert.NotNull(tree); + Assert.NotNull(tree.MatchingTrees); + var matchingTree = Assert.Single(tree.MatchingTrees); + + var firstSegment = Assert.Single(matchingTree.Root.Literals); + Assert.Equal("a", firstSegment.Key); + Assert.NotNull(firstSegment.Value.Parameters); + + var secondSegment = firstSegment.Value.Parameters; + Assert.Empty(secondSegment.Matches); + + var thirdSegment = Assert.Single(secondSegment.Literals); + Assert.Equal("c", thirdSegment.Key); + Assert.Single(thirdSegment.Value.Matches); + } + + [Fact] + public void TreeRouter_BuildDoesNotAddIntermediateMatchingNodes_ForRoutesWithMultipleIntermediateParametersWithDefaultOrOptionalValues() + { + // Arrange + var builder = CreateBuilder(); + + builder.MapInbound( + Mock.Of(), + TemplateParser.Parse("a/{b=3}/c/{d?}/e/{*f}"), + "Intermediate", + order: 0); + + // Act + var tree = builder.Build(); + + // Assert + Assert.NotNull(tree); + Assert.NotNull(tree.MatchingTrees); + var matchingTree = Assert.Single(tree.MatchingTrees); + + var firstSegment = Assert.Single(matchingTree.Root.Literals); + Assert.Equal("a", firstSegment.Key); + Assert.NotNull(firstSegment.Value.Parameters); + + var secondSegment = firstSegment.Value.Parameters; + Assert.Empty(secondSegment.Matches); + + var thirdSegment = Assert.Single(secondSegment.Literals); + Assert.Equal("c", thirdSegment.Key); + Assert.Empty(thirdSegment.Value.Matches); + + var fourthSegment = thirdSegment.Value.Parameters; + Assert.NotNull(fourthSegment); + Assert.Empty(fourthSegment.Matches); + + var fifthSegment = Assert.Single(fourthSegment.Literals); + Assert.Equal("e", fifthSegment.Key); + Assert.Single(fifthSegment.Value.Matches); + + var sixthSegment = fifthSegment.Value.CatchAlls; + Assert.NotNull(sixthSegment); + Assert.Single(sixthSegment.Matches); + } + + [Fact] + public void TreeRouter_BuildDoesNotAddIntermediateMatchingNodes_ForRoutesWithIntermediateParametersWithOptionalValues() + { + // Arrange + var builder = CreateBuilder(); + + builder.MapInbound( + Mock.Of(), + TemplateParser.Parse("a/{b?}/c"), + "Intermediate", + order: 0); + + // Act + var tree = builder.Build(); + + // Assert + Assert.NotNull(tree); + Assert.NotNull(tree.MatchingTrees); + var matchingTree = Assert.Single(tree.MatchingTrees); + + var firstSegment = Assert.Single(matchingTree.Root.Literals); + Assert.Equal("a", firstSegment.Key); + Assert.NotNull(firstSegment.Value.Parameters); + + var secondSegment = firstSegment.Value.Parameters; + Assert.Empty(secondSegment.Matches); + + var thirdSegment = Assert.Single(secondSegment.Literals); + Assert.Equal("c", thirdSegment.Key); + Assert.Single(thirdSegment.Value.Matches); + } + + [Fact] + public void TreeRouter_BuildDoesNotAddIntermediateMatchingNodes_ForRoutesWithIntermediateParametersWithConstrainedDefaultValues() + { + // Arrange + var builder = CreateBuilder(); + + builder.MapInbound( + Mock.Of(), + TemplateParser.Parse("a/{b:int=3}/c"), + "Intermediate", + order: 0); + + // Act + var tree = builder.Build(); + + // Assert + Assert.NotNull(tree); + Assert.NotNull(tree.MatchingTrees); + var matchingTree = Assert.Single(tree.MatchingTrees); + + var firstSegment = Assert.Single(matchingTree.Root.Literals); + Assert.Equal("a", firstSegment.Key); + Assert.NotNull(firstSegment.Value.ConstrainedParameters); + + var secondSegment = firstSegment.Value.ConstrainedParameters; + Assert.Empty(secondSegment.Matches); + + var thirdSegment = Assert.Single(secondSegment.Literals); + Assert.Equal("c", thirdSegment.Key); + Assert.Single(thirdSegment.Value.Matches); + } + + [Fact] + public void TreeRouter_BuildDoesNotAddIntermediateMatchingNodes_ForRoutesWithIntermediateParametersWithConstrainedOptionalValues() + { + // Arrange + var builder = CreateBuilder(); + + builder.MapInbound( + Mock.Of(), + TemplateParser.Parse("a/{b:int?}/c"), + "Intermediate", + order: 0); + + // Act + var tree = builder.Build(); + + // Assert + Assert.NotNull(tree); + Assert.NotNull(tree.MatchingTrees); + var matchingTree = Assert.Single(tree.MatchingTrees); + + var firstSegment = Assert.Single(matchingTree.Root.Literals); + Assert.Equal("a", firstSegment.Key); + Assert.NotNull(firstSegment.Value.ConstrainedParameters); + + var secondSegment = firstSegment.Value.ConstrainedParameters; + Assert.Empty(secondSegment.Matches); + + var thirdSegment = Assert.Single(secondSegment.Literals); + Assert.Equal("c", thirdSegment.Key); + Assert.Single(thirdSegment.Value.Matches); + } + private static TreeRouteBuilder CreateBuilder() { var objectPoolProvider = new DefaultObjectPoolProvider(); var objectPolicy = new UriBuilderContextPooledObjectPolicy(UrlEncoder.Default); - var objectPool = objectPoolProvider.Create(objectPolicy); + var objectPool = objectPoolProvider.Create(objectPolicy); - var constraintResolver = Mock.Of(); + var constraintResolver = GetInlineConstraintResolver(); var builder = new TreeRouteBuilder( NullLoggerFactory.Instance, UrlEncoder.Default, @@ -84,5 +259,13 @@ namespace Microsoft.AspNetCore.Routing.Tree constraintResolver); return builder; } + + private static IInlineConstraintResolver GetInlineConstraintResolver() + { + var services = new ServiceCollection().AddOptions(); + var serviceProvider = services.BuildServiceProvider(); + var accessor = serviceProvider.GetRequiredService>(); + return new DefaultInlineConstraintResolver(accessor); + } } } diff --git a/test/Microsoft.AspNetCore.Routing.Tests/Tree/TreeRouterTest.cs b/test/Microsoft.AspNetCore.Routing.Tests/Tree/TreeRouterTest.cs index 174f2bab77..9b0e065fa2 100644 --- a/test/Microsoft.AspNetCore.Routing.Tests/Tree/TreeRouterTest.cs +++ b/test/Microsoft.AspNetCore.Routing.Tests/Tree/TreeRouterTest.cs @@ -17,7 +17,6 @@ using Microsoft.Extensions.Options; using Microsoft.Extensions.WebEncoders.Testing; using Moq; using Xunit; -using Xunit.Extensions; namespace Microsoft.AspNetCore.Routing.Tree { @@ -274,6 +273,71 @@ namespace Microsoft.AspNetCore.Routing.Tree } } + [Fact] + public async Task TreeRouter_RouteAsync_DoesNotMatchRoutesWithIntermediateDefaultRouteValues() + { + // Arrange + var url = "/a/b"; + + var builder = CreateBuilder(); + + MapInboundEntry(builder, "a/b/{parameter3=3}/d"); + + var route = builder.Build(); + + var context = CreateRouteContext(url); + + // Act + await route.RouteAsync(context); + + // Assert + Assert.Null(context.Handler); + } + + [Theory] + [InlineData("a/{b=3}/c/{d?}/e/{*f}", "/a")] + [InlineData("a/{b=3}/c/{d?}/e/{*f}", "/a/b")] + [InlineData("a/{b=3}/c/{d?}/e/{*f}", "/a/b/c")] + [InlineData("a/{b=3}/c/{d?}/e/{*f}", "/a/b/c/d")] + public async Task TreeRouter_RouteAsync_DoesNotMatchRoutesWithMultipleIntermediateDefaultOrOptionalRouteValues(string template, string url) + { + // Arrange + var builder = CreateBuilder(); + + MapInboundEntry(builder, template); + + var route = builder.Build(); + + var context = CreateRouteContext(url); + + // Act + await route.RouteAsync(context); + + // Assert + Assert.Null(context.Handler); + } + + [Theory] + [InlineData("a/{b=3}/c/{d?}/e/{*f}", "/a/b/c/d/e")] + [InlineData("a/{b=3}/c/{d?}/e/{*f}", "/a/b/c/d/e/f")] + public async Task RouteAsync_MatchRoutesWithMultipleIntermediateDefaultOrOptionalRouteValues_WhenAllIntermediateValuesAreProvided(string template, string url) + { + // Arrange + var builder = CreateBuilder(); + + MapInboundEntry(builder, template); + + var route = builder.Build(); + + var context = CreateRouteContext(url); + + // Act + await route.RouteAsync(context); + + // Assert + Assert.NotNull(context.Handler); + } + [Fact] public async Task TreeRouter_RouteAsync_DoesNotMatchShorterUrl() { @@ -1032,6 +1096,27 @@ namespace Microsoft.AspNetCore.Routing.Tree Assert.Empty(result.DataTokens); } + [Fact] + public void TreeRouter_GenerateLink_CreatesLinksForRoutesWithIntermediateDefaultRouteValues() + { + // Arrange + var builder = CreateBuilder(); + + MapOutboundEntry(builder, template: "a/b/{parameter3=3}/d", requiredValues: null, order: 0); + + var route = builder.Build(); + + var context = CreateVirtualPathContext(values: null, ambientValues: null); + + // Act + var result = route.GetVirtualPath(context); + + // Assert + Assert.NotNull(result); + Assert.Equal("/a/b/3/d", result.VirtualPath); + } + + [Fact] public void TreeRouter_GeneratesLink_ForMultipleNamedEntriesWithTheSameTemplate() { diff --git a/test/Microsoft.AspNetCore.Routing.Tests/project.json b/test/Microsoft.AspNetCore.Routing.Tests/project.json index 6ded04c5f7..6f1795d8b5 100644 --- a/test/Microsoft.AspNetCore.Routing.Tests/project.json +++ b/test/Microsoft.AspNetCore.Routing.Tests/project.json @@ -1,6 +1,7 @@ { "buildOptions": { - "warningsAsErrors": true + "warningsAsErrors": true, + "keyFile": "../../tools/Key.snk" }, "dependencies": { "dotnet-test-xunit": "2.2.0-*",