[Fixes #357] Fix behavior when optional parameters are defined within a route and not at the end

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": {},

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")]

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<TemplateSegment> 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;
+        }
     }
 }

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
         /// </summary>
         public int Version { get; }
 
+        internal IEnumerable<UrlMatchingTree> MatchingTrees => _trees;
+
         /// <inheritdoc />
         public VirtualPathData GetVirtualPath(VirtualPathContext context)
         {

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-*",

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<IRouter>(),
+                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<IRouter>(),
+                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<IRouter>(),
+                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<IRouter>(),
+                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<IRouter>(),
+                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<UriBuildingContext>(objectPolicy);
+            var objectPool = objectPoolProvider.Create(objectPolicy);
 
-            var constraintResolver = Mock.Of<IInlineConstraintResolver>();
+            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<IOptions<RouteOptions>>();
+            return new DefaultInlineConstraintResolver(accessor);
+        }
     }
 }

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()
         {

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-*",