[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 incomming URLs in a <see cref="TreeRouter"/>.
+    /// </summary>
+    [DebuggerDisplay("{DebuggerToString()}")]
     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; }
+
+        internal string DebuggerToString()
+        {
+            return TemplateMatcher?.Template?.TemplateText;
+        }
     }
 }

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

@@ -5,10 +5,19 @@
 
 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 @@
 
 namespace Microsoft.AspNetCore.Routing.Tree
 {
+    /// <summary>
+    /// Builder for <see cref="TreeRouter"/> instances.
+    /// </summary>
     public class TreeRouteBuilder
     {
         private readonly ILogger _logger;
@@ -21,6 +24,13 @@ public class TreeRouteBuilder
         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 @@ public TreeRouteBuilder(
             _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 @@ public InboundRouteEntry MapInbound(
             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 @@ public OutboundRouteEntry MapOutbound(
             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 build 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 @@ public TreeRouter Build(int version)
                 version);
         }
 
+        /// <summary>
+        /// Removes all <see cref="InboundEntries"/> and <see cref="OutboundEntries"/> from this
+        /// <see cref="TreeRouteBuilder"/>.
+        /// </summary>
         public void Clear()
         {
             InboundEntries.Clear();
@@ -221,6 +272,36 @@ 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 matcher = new TemplateMatcher(entry.RouteTemplate, entry.Defaults);
 
             for (var i = 0; i < entry.RouteTemplate.Segments.Count; i++)

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

@@ -6,6 +6,7 @@
 using System.Collections.Generic;
 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 +175,6 @@ public async Task RouteAsync(RouteContext context)
             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 +235,11 @@ private struct TreeEnumerator : IEnumerator<UrlMatchingNode>
             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,14 +272,24 @@ public bool MoveNext()
                         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.Length == _tokenizer.Count)
                     {
-                        _segmentIndex--;
                         if (next.Matches.Count > 0)
                         {
                             Current = next;
                             return true;
                         }
+                        else
+                        {
+                            continue;
+                        }
+                    }
+                    else if (next.Length > _tokenizer.Count)
+                    {
+                        continue;
                     }
 
                     if (_tokenizer.Count == 0)
@@ -313,7 +320,7 @@ public bool MoveNext()
                     if (next.Literals.Count > 0)
                     {
                         UrlMatchingNode node;
-                        if (next.Literals.TryGetValue(_tokenizer[_segmentIndex].Value, out node))
+                        if (next.Literals.TryGetValue(_tokenizer[next.Length].Value, out node))
                         {
                             _stack.Push(node);
                         }
@@ -327,7 +334,6 @@ public void Reset()
             {
                 _stack.Clear();
                 Current = null;
-                _segmentIndex = -1;
             }
         }
 

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

@@ -3,11 +3,21 @@
 
 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()}")]
     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;
@@ -16,21 +26,56 @@ public UrlMatchingNode(int length)
             Literals = new Dictionary<string, UrlMatchingNode>(StringComparer.OrdinalIgnoreCase);
         }
 
+        /// <summary>
+        /// Gets the length of the path to this node in the <see cref="UrlMatchingTree"/>.
+        /// </summary>
         public int Length { get; }
 
+        /// <summary>
+        /// Gets or sets a value indicating wether 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 list of literal segments following this segment.
+        /// </summary>
         public Dictionary<string, UrlMatchingNode> Literals { get; }
 
+        /// <summary>
+        /// Gets or sets the list of <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 list of <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 list of <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 list of <see cref="UrlMatchingNode"/> representing
+        /// catch all parameter segments following this segment in the <see cref="TreeRouter"/>.
+        /// </summary>
         public UrlMatchingNode CatchAlls { get; set; }
+
+        internal string DebuggerToString()
+        {
+            return $"Length: {Length}, 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);
     }
 }