From dd79d0b88ccca57a45452c6630f8ed1056829227 Mon Sep 17 00:00:00 2001 From: jacalvar Date: Wed, 1 Jun 2016 10:28:06 -0700 Subject: [PATCH] [Fixes #316] TreeRouter does not match a route with the correct length --- .../Tree/InboundMatch.cs | 16 +++ .../Tree/OutboundMatch.cs | 9 ++ .../Tree/TreeRouteBuilder.cs | 82 +++++++++++++- .../Tree/TreeRouter.cs | 28 ++--- .../Tree/UrlMatchingNode.cs | 51 ++++++++- .../Tree/UrlMatchingTree.cs | 13 +++ .../Tree/TreeRouterTest.cs | 105 ++++++++++++++++-- 7 files changed, 279 insertions(+), 25 deletions(-) diff --git a/src/Microsoft.AspNetCore.Routing/Tree/InboundMatch.cs b/src/Microsoft.AspNetCore.Routing/Tree/InboundMatch.cs index 5c731286a6..57f1b6db7b 100644 --- a/src/Microsoft.AspNetCore.Routing/Tree/InboundMatch.cs +++ b/src/Microsoft.AspNetCore.Routing/Tree/InboundMatch.cs @@ -1,14 +1,30 @@ // 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 System.Diagnostics; using Microsoft.AspNetCore.Routing.Template; namespace Microsoft.AspNetCore.Routing.Tree { + /// + /// A candidate route to match incoming URLs in a . + /// + [DebuggerDisplay("{DebuggerToString(),nq}")] public class InboundMatch { + /// + /// Gets or sets the . + /// public InboundRouteEntry Entry { get; set; } + /// + /// Gets or sets the . + /// public TemplateMatcher TemplateMatcher { get; set; } + + private string DebuggerToString() + { + return TemplateMatcher?.Template?.TemplateText; + } } } diff --git a/src/Microsoft.AspNetCore.Routing/Tree/OutboundMatch.cs b/src/Microsoft.AspNetCore.Routing/Tree/OutboundMatch.cs index a00212fd8f..49980b9912 100644 --- a/src/Microsoft.AspNetCore.Routing/Tree/OutboundMatch.cs +++ b/src/Microsoft.AspNetCore.Routing/Tree/OutboundMatch.cs @@ -5,10 +5,19 @@ using Microsoft.AspNetCore.Routing.Template; namespace Microsoft.AspNetCore.Routing.Tree { + /// + /// A candidate match for link generation in a . + /// public class OutboundMatch { + /// + /// Gets or sets the . + /// public OutboundRouteEntry Entry { get; set; } + /// + /// Gets or sets the . + /// public TemplateBinder TemplateBinder { get; set; } } } diff --git a/src/Microsoft.AspNetCore.Routing/Tree/TreeRouteBuilder.cs b/src/Microsoft.AspNetCore.Routing/Tree/TreeRouteBuilder.cs index d6e8d58e26..7ab234b6cd 100644 --- a/src/Microsoft.AspNetCore.Routing/Tree/TreeRouteBuilder.cs +++ b/src/Microsoft.AspNetCore.Routing/Tree/TreeRouteBuilder.cs @@ -13,6 +13,9 @@ using Microsoft.Extensions.ObjectPool; namespace Microsoft.AspNetCore.Routing.Tree { + /// + /// Builder for instances. + /// public class TreeRouteBuilder { private readonly ILogger _logger; @@ -21,6 +24,13 @@ namespace Microsoft.AspNetCore.Routing.Tree private readonly ObjectPool _objectPool; private readonly IInlineConstraintResolver _constraintResolver; + /// + /// Initializes a new instance of . + /// + /// The . + /// The . + /// The . + /// The . public TreeRouteBuilder( ILoggerFactory loggerFactory, UrlEncoder urlEncoder, @@ -55,6 +65,14 @@ namespace Microsoft.AspNetCore.Routing.Tree _constraintLogger = loggerFactory.CreateLogger(typeof(RouteConstraintMatcher).FullName); } + /// + /// Adds a new inbound route to the . + /// + /// The for handling the route. + /// The of the route. + /// The route name. + /// The route order. + /// The . public InboundRouteEntry MapInbound( IRouter handler, RouteTemplate routeTemplate, @@ -112,6 +130,15 @@ namespace Microsoft.AspNetCore.Routing.Tree return entry; } + /// + /// Adds a new outbound route to the . + /// + /// The for handling the link generation. + /// The of the route. + /// The containing the route values. + /// The route name. + /// The route order. + /// The . public OutboundRouteEntry MapOutbound( IRouter handler, RouteTemplate routeTemplate, @@ -176,17 +203,37 @@ namespace Microsoft.AspNetCore.Routing.Tree return entry; } + /// + /// Gets the list of . + /// public IList InboundEntries { get; } = new List(); + /// + /// Gets the list of . + /// public IList OutboundEntries { get; } = new List(); + /// + /// Builds a with the + /// and defined in this . + /// + /// The . public TreeRouter Build() { return Build(version: 0); } + /// + /// Builds a with the + /// and defined in this . + /// + /// The version of the . + /// The . public TreeRouter Build(int version) { + // Tree route builder builds a tree for each of the different route orders defined by + // the user. When a route needs to be matched, the matching algorithm in tree router + // just iterates over the trees in ascending order when it tries to match the route. var trees = new Dictionary(); foreach (var entry in InboundEntries) @@ -211,6 +258,10 @@ namespace Microsoft.AspNetCore.Routing.Tree version); } + /// + /// Removes all and from this + /// . + /// public void Clear() { InboundEntries.Clear(); @@ -219,8 +270,37 @@ namespace Microsoft.AspNetCore.Routing.Tree private void AddEntryToTree(UrlMatchingTree tree, InboundRouteEntry entry) { - var current = tree.Root; + // The url matching tree represents all the routes asociated with a given + // order. Each node in the tree represents all the different categories + // a segment can have for which there is a defined inbound route entry. + // Each node contains a set of Matches that indicate all the routes for which + // a URL is a potential match. This list contains the routes with the same + // number of segments and the routes with the same number of segments plus an + // additional catch all parameter (as it can be empty). + // For example, for a set of routes like: + // 'Customer/Index/{id}' + // '{Controller}/{Action}/{*parameters}' + // + // The route tree will look like: + // Root -> + // Literals: Customer -> + // Literals: Index -> + // Parameters: {id} + // Matches: 'Customer/Index/{id}' + // Parameters: {Controller} -> + // Parameters: {Action} -> + // Matches: '{Controller}/{Action}/{*parameters}' + // CatchAlls: {*parameters} + // Matches: '{Controller}/{Action}/{*parameters}' + // + // When the tree router tries to match a route, it iterates the list of url matching trees + // in ascending order. For each tree it traverses each node starting from the root in the + // following order: Literals, constrained parameters, parameters, constrained catch all routes, catch alls. + // When it gets to a node of the same length as the route its trying to match, it simply looks at the list of + // candidates (which is in precence order) and tries to match the url against it. + // + var current = tree.Root; var matcher = new TemplateMatcher(entry.RouteTemplate, entry.Defaults); for (var i = 0; i < entry.RouteTemplate.Segments.Count; i++) diff --git a/src/Microsoft.AspNetCore.Routing/Tree/TreeRouter.cs b/src/Microsoft.AspNetCore.Routing/Tree/TreeRouter.cs index 310161e3ce..f14e1860d0 100644 --- a/src/Microsoft.AspNetCore.Routing/Tree/TreeRouter.cs +++ b/src/Microsoft.AspNetCore.Routing/Tree/TreeRouter.cs @@ -4,8 +4,10 @@ using System; using System.Collections; 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; @@ -174,7 +176,6 @@ namespace Microsoft.AspNetCore.Routing.Tree foreach (var tree in _trees) { var tokenizer = new PathTokenizer(context.HttpContext.Request.Path); - var enumerator = tokenizer.GetEnumerator(); var root = tree.Root; var treeEnumerator = new TreeEnumerator(root, tokenizer); @@ -235,14 +236,11 @@ namespace Microsoft.AspNetCore.Routing.Tree private readonly Stack _stack; private readonly PathTokenizer _tokenizer; - private int _segmentIndex; - public TreeEnumerator(UrlMatchingNode root, PathTokenizer tokenizer) { _stack = new Stack(); _tokenizer = tokenizer; Current = null; - _segmentIndex = -1; _stack.Push(root); } @@ -275,19 +273,23 @@ namespace Microsoft.AspNetCore.Routing.Tree Current = next; return true; } - else if (++_segmentIndex >= _tokenizer.Count) + // Next template has the same length as the url we are trying to match + // The only possible matching segments are either our current matches or + // any catch-all segment after this segment in which the catch all is empty. + else if (next.Depth == _tokenizer.Count) { - _segmentIndex--; if (next.Matches.Count > 0) { Current = next; return true; } - } - - if (_tokenizer.Count == 0) - { - continue; + else + { + // We can stop looking as any other child node from this node will be + // either a literal, a constrained parameter or a parameter. + // (Catch alls and constrained catch alls will show up as candidate matches). + continue; + } } if (next.CatchAlls != null) @@ -313,7 +315,8 @@ namespace Microsoft.AspNetCore.Routing.Tree if (next.Literals.Count > 0) { UrlMatchingNode node; - if (next.Literals.TryGetValue(_tokenizer[_segmentIndex].Value, out node)) + Debug.Assert(next.Depth < _tokenizer.Count); + if (next.Literals.TryGetValue(_tokenizer[next.Depth].Value, out node)) { _stack.Push(node); } @@ -327,7 +330,6 @@ namespace Microsoft.AspNetCore.Routing.Tree { _stack.Clear(); Current = null; - _segmentIndex = -1; } } diff --git a/src/Microsoft.AspNetCore.Routing/Tree/UrlMatchingNode.cs b/src/Microsoft.AspNetCore.Routing/Tree/UrlMatchingNode.cs index 99b4b6e90d..ffc387efe9 100644 --- a/src/Microsoft.AspNetCore.Routing/Tree/UrlMatchingNode.cs +++ b/src/Microsoft.AspNetCore.Routing/Tree/UrlMatchingNode.cs @@ -3,34 +3,79 @@ using System; using System.Collections.Generic; +using System.Diagnostics; +using System.Linq; namespace Microsoft.AspNetCore.Routing.Tree { + /// + /// A node in a . + /// + [DebuggerDisplay("{DebuggerToString(),nq}")] public class UrlMatchingNode { + /// + /// Initializes a new instance of . + /// + /// The length of the path to this node in the . public UrlMatchingNode(int length) { - Length = length; + Depth = length; Matches = new List(); Literals = new Dictionary(StringComparer.OrdinalIgnoreCase); } - public int Length { get; } + /// + /// Gets the length of the path to this node in the . + /// + public int Depth { get; } + /// + /// Gets or sets a value indicating whether this node represents a catch all segment. + /// public bool IsCatchAll { get; set; } - // These entries are sorted by precedence then template + /// + /// Gets the list of matching route entries associated with this node. + /// + /// + /// These entries are sorted by precedence then template. + /// public List Matches { get; } + /// + /// Gets the literal segments following this segment. + /// public Dictionary Literals { get; } + /// + /// Gets or sets the representing + /// parameter segments with constraints following this segment in the . + /// public UrlMatchingNode ConstrainedParameters { get; set; } + /// + /// Gets or sets the representing + /// parameter segments following this segment in the . + /// public UrlMatchingNode Parameters { get; set; } + /// + /// Gets or sets the representing + /// catch all parameter segments with constraints following this segment in the . + /// public UrlMatchingNode ConstrainedCatchAlls { get; set; } + /// + /// Gets or sets the representing + /// catch all parameter segments following this segment in the . + /// public UrlMatchingNode CatchAlls { get; set; } + + private string DebuggerToString() + { + return $"Length: {Depth}, Matches: {string.Join(" | ", Matches?.Select(m => $"({m.TemplateMatcher.Template.TemplateText})"))}"; + } } } diff --git a/src/Microsoft.AspNetCore.Routing/Tree/UrlMatchingTree.cs b/src/Microsoft.AspNetCore.Routing/Tree/UrlMatchingTree.cs index c7459fc0b6..90528d75b9 100644 --- a/src/Microsoft.AspNetCore.Routing/Tree/UrlMatchingTree.cs +++ b/src/Microsoft.AspNetCore.Routing/Tree/UrlMatchingTree.cs @@ -3,15 +3,28 @@ namespace Microsoft.AspNetCore.Routing.Tree { + /// + /// A tree part of a . + /// public class UrlMatchingTree { + /// + /// Initializes a new instance of . + /// + /// The order associated with routes in this . public UrlMatchingTree(int order) { Order = order; } + /// + /// Gets the order of the routes associated with this . + /// public int Order { get; } + /// + /// Gets the root of the . + /// public UrlMatchingNode Root { get; } = new UrlMatchingNode(length: 0); } } diff --git a/test/Microsoft.AspNetCore.Routing.Tests/Tree/TreeRouterTest.cs b/test/Microsoft.AspNetCore.Routing.Tests/Tree/TreeRouterTest.cs index e519ab5c04..5cd8562391 100644 --- a/test/Microsoft.AspNetCore.Routing.Tests/Tree/TreeRouterTest.cs +++ b/test/Microsoft.AspNetCore.Routing.Tests/Tree/TreeRouterTest.cs @@ -64,6 +64,95 @@ namespace Microsoft.AspNetCore.Routing.Tree Assert.Equal(expectedRouteGroup, context.RouteData.Values["test_route_group"]); } + [Theory] + [InlineData("/", "")] + [InlineData("/Literal1", "Literal1")] + [InlineData("/Literal1/Literal2", "Literal1/Literal2")] + [InlineData("/Literal1/Literal2/Literal3", "Literal1/Literal2/Literal3")] + [InlineData("/Literal1/Literal2/Literal3/4", "Literal1/Literal2/Literal3/{*constrainedCatchAll:int}")] + [InlineData("/Literal1/Literal2/Literal3/Literal4", "Literal1/Literal2/Literal3/{*catchAll}")] + [InlineData("/1", "{constrained1:int}")] + [InlineData("/1/2", "{constrained1:int}/{constrained2:int}")] + [InlineData("/1/2/3", "{constrained1:int}/{constrained2:int}/{constrained3:int}")] + [InlineData("/1/2/3/4", "{constrained1:int}/{constrained2:int}/{constrained3:int}/{*constrainedCatchAll:int}")] + [InlineData("/1/2/3/CatchAll4", "{constrained1:int}/{constrained2:int}/{constrained3:int}/{*catchAll}")] + [InlineData("/parameter1", "{parameter1}")] + [InlineData("/parameter1/parameter2", "{parameter1}/{parameter2}")] + [InlineData("/parameter1/parameter2/parameter3", "{parameter1}/{parameter2}/{parameter3}")] + [InlineData("/parameter1/parameter2/parameter3/4", "{parameter1}/{parameter2}/{parameter3}/{*constrainedCatchAll:int}")] + [InlineData("/parameter1/parameter2/parameter3/CatchAll4", "{parameter1}/{parameter2}/{parameter3}/{*catchAll}")] + public async Task TreeRouter_RouteAsync_MatchesRouteWithTheRightLength(string url, string expected) + { + // Arrange + var routes = new[] { + "", + "Literal1", + "Literal1/Literal2", + "Literal1/Literal2/Literal3", + "Literal1/Literal2/Literal3/{*constrainedCatchAll:int}", + "Literal1/Literal2/Literal3/{*catchAll}", + "{constrained1:int}", + "{constrained1:int}/{constrained2:int}", + "{constrained1:int}/{constrained2:int}/{constrained3:int}", + "{constrained1:int}/{constrained2:int}/{constrained3:int}/{*constrainedCatchAll:int}", + "{constrained1:int}/{constrained2:int}/{constrained3:int}/{*catchAll}", + "{parameter1}", + "{parameter1}/{parameter2}", + "{parameter1}/{parameter2}/{parameter3}", + "{parameter1}/{parameter2}/{parameter3}/{*constrainedCatchAll:int}", + "{parameter1}/{parameter2}/{parameter3}/{*catchAll}", + }; + + var expectedRouteGroup = CreateRouteGroup(0, expected); + + var builder = CreateBuilder(); + + // We setup the route entries in reverse order of precedence to ensure that when we + // try to route the request, the route with a higher precedence gets tried first. + foreach (var template in routes.Reverse()) + { + MapInboundEntry(builder, template); + } + + var route = builder.Build(); + + var context = CreateRouteContext(url); + + // Act + await route.RouteAsync(context); + + // Assert + Assert.Equal(expectedRouteGroup, context.RouteData.Values["test_route_group"]); + } + + [Fact] + public async Task TreeRouter_RouteAsync_DoesNotMatchShorterUrl() + { + // Arrange + var routes = new[] { + "Literal1/Literal2/Literal3", + }; + + var builder = CreateBuilder(); + + // We setup the route entries in reverse order of precedence to ensure that when we + // try to route the request, the route with a higher precedence gets tried first. + foreach (var template in routes.Reverse()) + { + MapInboundEntry(builder, template); + } + + var route = builder.Build(); + + var context = CreateRouteContext("/Literal1"); + + // Act + await route.RouteAsync(context); + + // Assert + Assert.Null(context.Handler); + } + [Theory] [InlineData("template/5", "template/{parameter:int}")] [InlineData("template/5", "template/{parameter}")] @@ -623,7 +712,7 @@ namespace Microsoft.AspNetCore.Routing.Tree { // Arrange var builder = CreateBuilder(); - + // We setup the route entries with a lower relative order first to ensure that when // we try to generate a link, the route with the higher relative order gets tried first. MapOutboundEntry(builder, firstTemplate, requiredValues: null, order: 1); @@ -653,7 +742,7 @@ namespace Microsoft.AspNetCore.Routing.Tree { // Arrange var builder = CreateBuilder(); - + // We setup the route entries with a lower relative template order first to ensure that when // we try to generate a link, the route with the higher template order gets tried first. MapOutboundEntry(builder, secondTemplate, requiredValues: null, order: 0); @@ -716,7 +805,7 @@ namespace Microsoft.AspNetCore.Routing.Tree { // Arrange var builder = CreateBuilder(); - + // The named route has a lower order which will ensure that we aren't trying the route as // if it were an unnamed route. MapOutboundEntry(builder, "named", requiredValues: null, order: 1, name: "NamedRoute"); @@ -774,7 +863,7 @@ namespace Microsoft.AspNetCore.Routing.Tree { // Arrange var builder = CreateBuilder(); - + // The named route has a lower order which will ensure that we aren't trying the route as // if it were an unnamed route. MapOutboundEntry(builder, template, requiredValues: null, order: 1, name: "NamedRoute"); @@ -899,7 +988,7 @@ namespace Microsoft.AspNetCore.Routing.Tree { // Arrange var builder = CreateBuilder(); - MapOutboundEntry(builder, + MapOutboundEntry(builder, "api/{area}/dosomething/{controller}/{action}", new { action = "Index", controller = "Store", area = "AwesomeCo" }); @@ -1438,9 +1527,9 @@ namespace Microsoft.AspNetCore.Routing.Tree IRouter handler = null) { var entry = builder.MapInbound( - handler ?? new StubRouter(), - TemplateParser.Parse(template), - routeName: null, + handler ?? new StubRouter(), + TemplateParser.Parse(template), + routeName: null, order: order); // Add a generated 'route group' so we can identify later which entry matched.