Fixes for strange routing behaviour with optionals.

 The routing engine incorrectly routes to longer urls in preference
 of the score, plus it falls back on routing to the last added matching routes
 instead of the first.

 I've fixed the sorting comparison method and added some test coverage to
 show the particular use cases where I found issues.

Author: PhilipSkinner

src/Nancy/Routing/Trie/MatchResult.cs

@@ -65,37 +65,40 @@ public MatchResult()
         /// <param name="other">An object to compare with this object.</param>
         public int CompareTo(MatchResult other)
         {
-            // Length of the route takes precedence over score
-            if (this.RouteLength < other.RouteLength)
+            // Score of the route takes precedence over length
+            if (this.Score < other.Score)
             {
                 return -1;
             }
 
-            if (this.RouteLength > other.RouteLength)
+            if (this.Score > other.Score)
             {
                 return 1;
             }
 
-            if (this.Score < other.Score)
+            // If the score is the same, take the shortest route that matches
+            if (this.RouteLength < other.RouteLength)
             {
-                return -1;
+                return 1;
             }
 
-            if (this.Score > other.Score)
+            if (this.RouteLength > other.RouteLength)
             {
-                return 1;
+                return -1;
             }
 
+
             if (Equals(this.ModuleType, other.ModuleType))
             {
+                //Otherwise, we need to take the route that was indexed first
                 if (this.RouteIndex < other.RouteIndex)
                 {
-                    return -1;
+                    return 1;
                 }
 
                 if (this.RouteIndex > other.RouteIndex)
                 {
-                    return 1;
+                    return -1;
                 }
             }
 

test/Nancy.Tests/Unit/Routing/DefaultRouteResolverFixture.cs

@@ -152,11 +152,11 @@ public async Task Should_resolve_optional_capture_with_default_with_optional_spe
             //Then
             if (ShouldBeFound(path, caseSensitive))
             {
-              result.Body.AsString().ShouldEqual("OptionalCaptureWithDefault " + expected);
+                result.Body.AsString().ShouldEqual("OptionalCaptureWithDefault " + expected);
             }
             else
             {
-              result.StatusCode.ShouldEqual(HttpStatusCode.NotFound);
+                result.StatusCode.ShouldEqual(HttpStatusCode.NotFound);
             }
         }
 
@@ -174,14 +174,73 @@ public async Task Should_resolve_optional_capture_with_default_with_optional_not
             //Then
             if (ShouldBeFound(path, caseSensitive))
             {
-              result.Body.AsString().ShouldEqual("OptionalCaptureWithDefault test");
+                result.Body.AsString().ShouldEqual("OptionalCaptureWithDefault test");
             }
             else
             {
-              result.StatusCode.ShouldEqual(HttpStatusCode.NotFound);
+                result.StatusCode.ShouldEqual(HttpStatusCode.NotFound);
             }
         }
 
+        [Theory]
+        [InlineData("/api", "Single optional segment")]
+        [InlineData("/api/", "Single optional segment")]
+        [InlineData("/api/arg1", "Single optional segment")]
+        [InlineData("/api/arg1/", "Single optional segment")]
+        [InlineData("/api/arg1/arg2", "Two optional segments")]
+        [InlineData("/api/arg1/arg2/", "Two optional segments")]
+        [InlineData("/api/arg1/arg2/arg3", "Three optional segments")]
+        [InlineData("/api/arg1/arg2/arg3/", "Three optional segments")]
+        public async Task Should_Resolve_Optionals_Correctly(string path, string expected)
+        {
+            var browser = InitBrowser(caseSensitive: false);
+            var result = await browser.Get(path);
+            result.Body.AsString().ShouldEqual(expected);
+        }
+
+        [Theory]
+        [InlineData("/api/greedy/arg1", "Greedy match")]
+        [InlineData("/api/greedy/arg1/arg2", "Greedy match")]
+        public async Task Should_Resolve_Greedy_Alongside_Optionals(string path, string expected)
+        {
+            var browser = InitBrowser(caseSensitive: false);
+            var result = await browser.Get(path);
+            result.Body.AsString().ShouldEqual(expected);
+        }
+
+        [Theory]
+        [InlineData("/optional/literal/bar", "Single optional segment, literal segment at end")]
+        [InlineData("/optional/literal/arg1/bar", "Single optional segment, literal segment at end")]
+        [InlineData("/optional/literal/arg1/arg2/bar", "Two optional segments, literal segment at end")]
+        public async Task Should_Resolve_Optionals_with_Literal_Ends(string path, string expected)
+        {
+            var browser = InitBrowser(caseSensitive: false);
+            var result = await browser.Get(path);
+            result.Body.AsString().ShouldEqual(expected);
+        }
+        
+        [Theory]
+        [InlineData("/optional/variable/hello", "Single  hello")]
+        [InlineData("/optional/variable/hello/there", "Single hello there")]
+        [InlineData("/optional/variable/hello/there/everybody", "Double hello there everybody")]
+        public async Task Should_Resolve_Optionals_with_Variable_Ends(string path, string expected)
+        {
+            var browser = InitBrowser(caseSensitive: false);
+            var result = await browser.Get(path);            
+            result.Body.AsString().ShouldEqual(expected);
+        }
+        
+        [Theory]        
+        [InlineData("/too/greedy/arg1", "One arg1")]
+        [InlineData("/too/greedy/arg1/hello", "Literal arg1")]
+        public async Task Should_Not_Be_Too_Greedy(string path, string expected)
+        {
+            var browser = InitBrowser(caseSensitive: false);
+            var result = await browser.Get(path);
+            var woot = result.Body.AsString();
+            result.Body.AsString().ShouldEqual(expected);
+        }
+
         [Theory]
         [InlineData("/bleh/this/is/some/stuff", true, "this/is/some/stuff")]
         [InlineData("/bleh/this/is/some/stuff", false, "this/is/some/stuff")]
@@ -442,6 +501,7 @@ public NoRootModule()
             }
         }
 
+
         private class TestModule : NancyModule
         {
             public TestModule()
@@ -475,6 +535,26 @@ public TestModule()
                 Get("/capturenodewithliteral/{file}.html", args => "CaptureNodeWithLiteral " + args.file + ".html");
 
                 Get(@"/regex/(?<foo>\d{2,4})/{bar}", args => string.Format("RegEx {0} {1}", args.foo, args.bar));
+
+                Get("/api/{arg1?}", args => "Single optional segment");
+
+                Get("/api/{arg1?}/{arg2?}", args => "Two optional segments");
+
+                Get("/api/{arg1?}/{arg2?}/{arg3?}", args => "Three optional segments");
+
+                Get("/api/greedy/{something*}", args => "Greedy match");
+
+                Get("/optional/literal/{arg1?}/bar", args => "Single optional segment, literal segment at end");
+
+                Get("/optional/literal/{arg1?}/{arg2?}/bar", args => "Two optional segments, literal segment at end");
+
+                Get("/optional/variable/{arg1?}/{variable}", args => string.Format("Single {0} {1}", args.arg1.Value, args.variable.Value));
+
+                Get("/optional/variable/{arg1?}/{arg2?}/{variable}", args => string.Format("Double {0} {1} {2}", args.arg1.Value, args.arg2.Value, args.variable.Value));
+
+                Get("/too/greedy/{arg1*}", args => string.Format("One {0}", args.arg1.Value));
+
+                Get("/too/greedy/{arg1*}/hello", args => string.Format("Literal {0}", args.arg1.Value));                
             }
         }
     }