[Fixes #316] TreeRouter does not match a route with the correct length

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
 {
+    /// <summary>
+    /// A candidate route to match incoming URLs in a <see cref="TreeRouter"/>.
+    /// </summary>
+    [DebuggerDisplay("{DebuggerToString(),nq}")]
     public class InboundMatch
     {
+        /// <summary>
+        /// Gets or sets the <see cref="InboundRouteEntry"/>.
+        /// </summary>
         public InboundRouteEntry Entry { get; set; }
 
+        /// <summary>
+        /// Gets or sets the <see cref="TemplateMatcher"/>.
+        /// </summary>
         public TemplateMatcher TemplateMatcher { get; set; }
+
+        private string DebuggerToString()
+        {
+            return TemplateMatcher?.Template?.TemplateText;
+        }
     }
 }

src/Microsoft.AspNetCore.Routing/Tree/OutboundMatch.cs

@@ -5,10 +5,19 @@ using Microsoft.AspNetCore.Routing.Template;
 
 namespace Microsoft.AspNetCore.Routing.Tree
 {
+    /// <summary>
+    /// A candidate match for link generation in a <see cref="TreeRouter"/>.
+    /// </summary>
     public class OutboundMatch
     {
+        /// <summary>
+        /// Gets or sets the <see cref="OutboundRouteEntry"/>.
+        /// </summary>
         public OutboundRouteEntry Entry { get; set; }
 
+        /// <summary>
+        /// Gets or sets the <see cref="TemplateBinder"/>.
+        /// </summary>
         public TemplateBinder TemplateBinder { get; set; }
     }
 }

src/Microsoft.AspNetCore.Routing/Tree/TreeRouteBuilder.cs

@@ -13,6 +13,9 @@ using Microsoft.Extensions.ObjectPool;
 
 namespace Microsoft.AspNetCore.Routing.Tree
 {
+    /// <summary>
+    /// Builder for <see cref="TreeRouter"/> instances.
+    /// </summary>
     public class TreeRouteBuilder
     {
         private readonly ILogger _logger;
@@ -21,6 +24,13 @@ namespace Microsoft.AspNetCore.Routing.Tree
         private readonly ObjectPool<UriBuildingContext> _objectPool;
         private readonly IInlineConstraintResolver _constraintResolver;
 
+        /// <summary>
+        /// Initializes a new instance of <see cref="TreeRouteBuilder"/>.
+        /// </summary>
+        /// <param name="loggerFactory">The <see cref="ILoggerFactory"/>.</param>
+        /// <param name="urlEncoder">The <see cref="UrlEncoder"/>.</param>
+        /// <param name="objectPool">The <see cref="ObjectPool{UrlBuildingContext}"/>.</param>
+        /// <param name="constraintResolver">The <see cref="IInlineConstraintResolver"/>.</param>
         public TreeRouteBuilder(
             ILoggerFactory loggerFactory,
             UrlEncoder urlEncoder,
@@ -55,6 +65,14 @@ namespace Microsoft.AspNetCore.Routing.Tree
             _constraintLogger = loggerFactory.CreateLogger(typeof(RouteConstraintMatcher).FullName);
         }
 
+        /// <summary>
+        /// Adds a new inbound route to the <see cref="TreeRouter"/>.
+        /// </summary>
+        /// <param name="handler">The <see cref="IRouter"/> for handling the route.</param>
+        /// <param name="routeTemplate">The <see cref="RouteTemplate"/> of the route.</param>
+        /// <param name="routeName">The route name.</param>
+        /// <param name="order">The route order.</param>
+        /// <returns>The <see cref="InboundRouteEntry"/>.</returns>
         public InboundRouteEntry MapInbound(
             IRouter handler,
             RouteTemplate routeTemplate,
@@ -112,6 +130,15 @@ namespace Microsoft.AspNetCore.Routing.Tree
             return entry;
         }
 
+        /// <summary>
+        /// Adds a new outbound route to the <see cref="TreeRouter"/>.
+        /// </summary>
+        /// <param name="handler">The <see cref="IRouter"/> for handling the link generation.</param>
+        /// <param name="routeTemplate">The <see cref="RouteTemplate"/> of the route.</param>
+        /// <param name="requiredLinkValues">The <see cref="RouteValueDictionary"/> containing the route values.</param>
+        /// <param name="routeName">The route name.</param>
+        /// <param name="order">The route order.</param>
+        /// <returns>The <see cref="OutboundRouteEntry"/>.</returns>
         public OutboundRouteEntry MapOutbound(
             IRouter handler,
             RouteTemplate routeTemplate,
@@ -176,17 +203,37 @@ namespace Microsoft.AspNetCore.Routing.Tree
             return entry;
         }
 
+        /// <summary>
+        /// Gets the list of <see cref="InboundRouteEntry"/>.
+        /// </summary>
         public IList<InboundRouteEntry> InboundEntries { get; } = new List<InboundRouteEntry>();
 
+        /// <summary>
+        /// Gets the list of <see cref="OutboundRouteEntry"/>.
+        /// </summary>
         public IList<OutboundRouteEntry> OutboundEntries { get; } = new List<OutboundRouteEntry>();
 
+        /// <summary>
+        /// Builds a <see cref="TreeRouter"/> with the <see cref="InboundEntries"/>
+        /// and <see cref="OutboundEntries"/> defined in this <see cref="TreeRouteBuilder"/>.
+        /// </summary>
+        /// <returns>The <see cref="TreeRouter"/>.</returns>
         public TreeRouter Build()
         {
             return Build(version: 0);
         }
 
+        /// <summary>
+        /// Builds a <see cref="TreeRouter"/> with the <see cref="InboundEntries"/>
+        /// and <see cref="OutboundEntries"/> defined in this <see cref="TreeRouteBuilder"/>.
+        /// </summary>
+        /// <param name="version">The version of the <see cref="TreeRouter"/>.</param>
+        /// <returns>The <see cref="TreeRouter"/>.</returns>
         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<int, UrlMatchingTree>();
 
             foreach (var entry in InboundEntries)
@@ -211,6 +258,10 @@ namespace Microsoft.AspNetCore.Routing.Tree
                 version);
         }
 
+        /// <summary>
+        /// Removes all <see cref="InboundEntries"/> and <see cref="OutboundEntries"/> from this
+        /// <see cref="TreeRouteBuilder"/>.
+        /// </summary>
         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++)

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<UrlMatchingNode> _stack;
             private readonly PathTokenizer _tokenizer;
 
-            private int _segmentIndex;
-
             public TreeEnumerator(UrlMatchingNode root, PathTokenizer tokenizer)
             {
                 _stack = new Stack<UrlMatchingNode>();
                 _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;
                         }
-                    }
+                        else
-
+                        {
-                    if (_tokenizer.Count == 0)
+                            // We can stop looking as any other child node from this node will be
-                    {
+                            // either a literal, a constrained parameter or a parameter.
-                        continue;
+                            // (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;
             }
         }
 

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
 {
+    /// <summary>
+    /// A node in a <see cref="UrlMatchingTree"/>.
+    /// </summary>
+    [DebuggerDisplay("{DebuggerToString(),nq}")]
     public class UrlMatchingNode
     {
+        /// <summary>
+        /// Initializes a new instance of <see cref="UrlMatchingNode"/>.
+        /// </summary>
+        /// <param name="length">The length of the path to this node in the <see cref="UrlMatchingTree"/>.</param>
         public UrlMatchingNode(int length)
         {
-            Length = length;
+            Depth = length;
 
             Matches = new List<InboundMatch>();
             Literals = new Dictionary<string, UrlMatchingNode>(StringComparer.OrdinalIgnoreCase);
         }
 
-        public int Length { get; }
+        /// <summary>
+        /// Gets the length of the path to this node in the <see cref="UrlMatchingTree"/>.
+        /// </summary>
+        public int Depth { get; }
 
+        /// <summary>
+        /// Gets or sets a value indicating whether this node represents a catch all segment.
+        /// </summary>
         public bool IsCatchAll { get; set; }
 
-        // These entries are sorted by precedence then template
+        /// <summary>
+        /// Gets the list of matching route entries associated with this node.
+        /// </summary>
+        /// <remarks>
+        /// These entries are sorted by precedence then template.
+        /// </remarks>
         public List<InboundMatch> Matches { get; }
 
+        /// <summary>
+        /// Gets the literal segments following this segment.
+        /// </summary>
         public Dictionary<string, UrlMatchingNode> Literals { get; }
 
+        /// <summary>
+        /// Gets or sets the <see cref="UrlMatchingNode"/> representing
+        /// parameter segments with constraints following this segment in the <see cref="TreeRouter"/>.
+        /// </summary>
         public UrlMatchingNode ConstrainedParameters { get; set; }
 
+        /// <summary>
+        /// Gets or sets the <see cref="UrlMatchingNode"/> representing
+        /// parameter segments following this segment in the <see cref="TreeRouter"/>.
+        /// </summary>
         public UrlMatchingNode Parameters { get; set; }
 
+        /// <summary>
+        /// Gets or sets the <see cref="UrlMatchingNode"/> representing
+        /// catch all parameter segments with constraints following this segment in the <see cref="TreeRouter"/>.
+        /// </summary>
         public UrlMatchingNode ConstrainedCatchAlls { get; set; }
 
+        /// <summary>
+        /// Gets or sets the <see cref="UrlMatchingNode"/> representing
+        /// catch all parameter segments following this segment in the <see cref="TreeRouter"/>.
+        /// </summary>
         public UrlMatchingNode CatchAlls { get; set; }
+
+        private string DebuggerToString()
+        {
+            return $"Length: {Depth}, Matches: {string.Join(" | ", Matches?.Select(m => $"({m.TemplateMatcher.Template.TemplateText})"))}";
+        }
     }
 }

src/Microsoft.AspNetCore.Routing/Tree/UrlMatchingTree.cs

@@ -3,15 +3,28 @@
 
 namespace Microsoft.AspNetCore.Routing.Tree
 {
+    /// <summary>
+    /// A tree part of a <see cref="TreeRouter"/>.
+    /// </summary>
     public class UrlMatchingTree
     {
+        /// <summary>
+        /// Initializes a new instance of <see cref="UrlMatchingTree"/>.
+        /// </summary>
+        /// <param name="order">The order associated with routes in this <see cref="UrlMatchingTree"/>.</param>
         public UrlMatchingTree(int order)
         {
             Order = order;
         }
 
+        /// <summary>
+        /// Gets the order of the routes associated with this <see cref="UrlMatchingTree"/>.
+        /// </summary>
         public int Order { get; }
 
+        /// <summary>
+        /// Gets the root of the <see cref="UrlMatchingTree"/>.
+        /// </summary>
         public UrlMatchingNode Root { get; } = new UrlMatchingNode(length: 0);
     }
 }

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(), 
+                handler ?? new StubRouter(),
-                TemplateParser.Parse(template), 
+                TemplateParser.Parse(template),
-                routeName: null, 
+                routeName: null,
                 order: order);
 
             // Add a generated 'route group' so we can identify later which entry matched.