From 9ee946073a128f778cfc68fa796b95dc9c8e429a Mon Sep 17 00:00:00 2001 From: Ryan Nowak Date: Mon, 9 Feb 2015 19:21:16 -0800 Subject: [PATCH] Add support for best-effort link-generation This change adds a feature needed for aspnet/Mvc#302 There's a new option in routing that allows link-generation to proceed when the route values cannot be validated. The key scenario for this is during development of an MVC site. Routing will refuse to generate a link to actions which don't exists, this is a breaking change from the MVC5 behavior. Setting UseBestEffortLinkGeneration will allow routing to return a value even when we can't match the action. This option will remain off by default - setting this to on will impact link-generation in a bunch of scenarios involving areas where we've improved the logic for MVC6. If you're considering leaving this on outside of development scenarios, then make sure to be as explicit with route values as possible (don't rely on ambient values). Functional tests to follow up in the MVC repo. --- .../RouteCollection.cs | 102 ++++- src/Microsoft.AspNet.Routing/RouteOptions.cs | 8 + .../Template/TemplateRoute.cs | 16 +- .../RouteCollectionTest.cs | 382 +++++++++++++++++- .../Template/TemplateRouteTest.cs | 6 +- 5 files changed, 464 insertions(+), 50 deletions(-) diff --git a/src/Microsoft.AspNet.Routing/RouteCollection.cs b/src/Microsoft.AspNet.Routing/RouteCollection.cs index 06a73cd0b6..c6f3ab1dbb 100644 --- a/src/Microsoft.AspNet.Routing/RouteCollection.cs +++ b/src/Microsoft.AspNet.Routing/RouteCollection.cs @@ -3,11 +3,13 @@ using System; using System.Collections.Generic; +using System.Diagnostics; using System.Threading.Tasks; using Microsoft.AspNet.Http; using Microsoft.AspNet.Routing.Logging; using Microsoft.AspNet.Routing.Logging.Internal; using Microsoft.Framework.DependencyInjection; +using Microsoft.Framework.OptionsModel; using Microsoft.Framework.Logging; namespace Microsoft.AspNet.Routing @@ -18,7 +20,9 @@ namespace Microsoft.AspNet.Routing private readonly List _unnamedRoutes = new List(); private readonly Dictionary _namedRoutes = new Dictionary(StringComparer.OrdinalIgnoreCase); + private ILogger _logger; + private RouteOptions _options; public IRouter this[int index] { @@ -94,45 +98,103 @@ namespace Microsoft.AspNet.Routing public virtual string GetVirtualPath(VirtualPathContext context) { + EnsureOptions(context.Context); + + // If we're using Best-Effort link generation then it means that we'll first look for a route where + // the route values are validated (context.IsBound == true). If we can't find a match like that, then + // we'll return the path from the first route to return one. + var useBestEffort = _options.UseBestEffortLinkGeneration; + if (!string.IsNullOrEmpty(context.RouteName)) { + var isValidated = false; + string bestPath = null; INamedRouter matchedNamedRoute; - _namedRoutes.TryGetValue(context.RouteName, out matchedNamedRoute); + if (_namedRoutes.TryGetValue(context.RouteName, out matchedNamedRoute)) + { + bestPath = matchedNamedRoute.GetVirtualPath(context); + isValidated = context.IsBound; + } - var virtualPath = matchedNamedRoute != null ? matchedNamedRoute.GetVirtualPath(context) : null; + // If we get here and context.IsBound == true, then we know we have a match, we want to keep + // iterating to see if we have multiple matches. foreach (var unnamedRoute in _unnamedRoutes) { - var tempVirtualPath = unnamedRoute.GetVirtualPath(context); - if (tempVirtualPath != null) - { - if (virtualPath != null) - { - // There was already a previous route which matched the name. - throw new InvalidOperationException( - Resources.FormatNamedRoutes_AmbiguousRoutesFound(context.RouteName)); - } + // reset because we're sharing the context + context.IsBound = false; - virtualPath = tempVirtualPath; + var path = unnamedRoute.GetVirtualPath(context); + if (path == null) + { + continue; + } + + if (bestPath != null) + { + // There was already a previous route which matched the name. + throw new InvalidOperationException( + Resources.FormatNamedRoutes_AmbiguousRoutesFound(context.RouteName)); + } + else if (context.IsBound) + { + // This is the first 'validated' match that we've found. + bestPath = path; + isValidated = true; + } + else + { + Debug.Assert(bestPath == null); + + // This is the first 'unvalidated' match that we've found. + bestPath = path; + isValidated = false; } } - return virtualPath; + if (isValidated || useBestEffort) + { + context.IsBound = isValidated; + return bestPath; + } + else + { + return null; + } } else { + string bestPath = null; for (var i = 0; i < Count; i++) { var route = this[i]; var path = route.GetVirtualPath(context); - if (path != null) + if (path == null) { + continue; + } + + if (context.IsBound) + { + // This route has validated route values, short circuit. return path; } + else if (bestPath == null) + { + // The values aren't validated, but this is the best we've seen so far + bestPath = path; + } + } + + if (useBestEffort) + { + return bestPath; + } + else + { + return null; } } - - return null; } private void EnsureLogger(HttpContext context) @@ -143,5 +205,13 @@ namespace Microsoft.AspNet.Routing _logger = factory.Create(); } } + + private void EnsureOptions(HttpContext context) + { + if (_options == null) + { + _options = context.RequestServices.GetRequiredService>().Options; + } + } } } \ No newline at end of file diff --git a/src/Microsoft.AspNet.Routing/RouteOptions.cs b/src/Microsoft.AspNet.Routing/RouteOptions.cs index 3a572f2163..a17f762a93 100644 --- a/src/Microsoft.AspNet.Routing/RouteOptions.cs +++ b/src/Microsoft.AspNet.Routing/RouteOptions.cs @@ -61,5 +61,13 @@ namespace Microsoft.AspNet.Routing {"required", typeof(RequiredRouteConstraint) }, }; } + + /// + /// Gets or sets the value that enables best-effort link generation. + /// + /// If enabled, link generation will use allow link generation to succeed when the set of values provided + /// cannot be validated. + /// + public bool UseBestEffortLinkGeneration { get; set; } } } diff --git a/src/Microsoft.AspNet.Routing/Template/TemplateRoute.cs b/src/Microsoft.AspNet.Routing/Template/TemplateRoute.cs index 05d041e401..6d706061c6 100644 --- a/src/Microsoft.AspNet.Routing/Template/TemplateRoute.cs +++ b/src/Microsoft.AspNet.Routing/Template/TemplateRoute.cs @@ -202,24 +202,18 @@ namespace Microsoft.AspNet.Routing.Template // Validate that the target can accept these values. var childContext = CreateChildVirtualPathContext(context, values.AcceptedValues); + var path = _target.GetVirtualPath(childContext); if (path != null) { // If the target generates a value then that can short circuit. - context.IsBound = true; return path; } - else if (!childContext.IsBound) - { - // The target has rejected these values. - return null; - } + // If we can produce a value go ahead and do it, the caller can check context.IsBound + // to see if the values were validated. path = _binder.BindValues(values.AcceptedValues); - if (path != null) - { - context.IsBound = true; - } + context.IsBound = childContext.IsBound; return path; } @@ -282,7 +276,7 @@ namespace Microsoft.AspNet.Routing.Template foreach (var inlineConstraint in parameter.InlineConstraints) { constraintBuilder.AddResolvedConstraint(parameter.Name, inlineConstraint.Constraint); - } + } } return constraintBuilder.Build(); diff --git a/test/Microsoft.AspNet.Routing.Tests/RouteCollectionTest.cs b/test/Microsoft.AspNet.Routing.Tests/RouteCollectionTest.cs index 6179818e1f..081885a8f5 100644 --- a/test/Microsoft.AspNet.Routing.Tests/RouteCollectionTest.cs +++ b/test/Microsoft.AspNet.Routing.Tests/RouteCollectionTest.cs @@ -9,6 +9,7 @@ using System.Threading.Tasks; using Microsoft.AspNet.Http; using Microsoft.AspNet.Routing.Logging; using Microsoft.Framework.Logging; +using Microsoft.Framework.OptionsModel; using Moq; using Xunit; @@ -231,6 +232,300 @@ namespace Microsoft.AspNet.Routing Assert.Equal("The supplied route name 'ambiguousRoute' is ambiguous and matched more than one route.", ex.Message); } + [Fact] + public void GetVirtualPath_AmbiguousRoutes_RequiresRouteValueValidation_Error() + { + // Arrange + var namedRoute = CreateNamedRoute("Ambiguous", accept: false); + + var routeCollection = new RouteCollection(); + routeCollection.Add(namedRoute); + + var innerRouteCollection = new RouteCollection(); + innerRouteCollection.Add(namedRoute); + routeCollection.Add(innerRouteCollection); + + var options = new RouteOptions() + { + UseBestEffortLinkGeneration = true, + }; + + var virtualPathContext = CreateVirtualPathContext("Ambiguous", options: options); + + // Act & Assert + var ex = Assert.Throws(() => routeCollection.GetVirtualPath(virtualPathContext)); + Assert.Equal("The supplied route name 'Ambiguous' is ambiguous and matched more than one route.", ex.Message); + } + + [Fact] + public void GetVirtualPath_NamedRoute_BestEffort_BestInTopCollection() + { + // Arrange + var bestMatch = CreateNamedRoute("Match", accept: true, matchValue: "best"); + var noMatch = CreateNamedRoute("NoMatch", accept: true, matchValue: "bad"); + + var routeCollection = new RouteCollection(); + routeCollection.Add(bestMatch); + + var innerRouteCollection = new RouteCollection(); + innerRouteCollection.Add(noMatch); + routeCollection.Add(innerRouteCollection); + + var options = new RouteOptions() + { + UseBestEffortLinkGeneration = true, + }; + + var virtualPathContext = CreateVirtualPathContext("Match", options: options); + + // Act + var path = routeCollection.GetVirtualPath(virtualPathContext); + + Assert.Equal("best", path); + } + + [Fact] + public void GetVirtualPath_NamedRoute_BestEffort_BestMatchInNestedCollection() + { + // Arrange + var bestMatch = CreateNamedRoute("NoMatch", accept: true, matchValue: "bad"); + var noMatch = CreateNamedRoute("Match", accept: true, matchValue: "best"); + + var routeCollection = new RouteCollection(); + routeCollection.Add(noMatch); + + var innerRouteCollection = new RouteCollection(); + innerRouteCollection.Add(bestMatch); + routeCollection.Add(innerRouteCollection); + + var options = new RouteOptions() + { + UseBestEffortLinkGeneration = true, + }; + + var virtualPathContext = CreateVirtualPathContext("Match", options: options); + + // Act + var path = routeCollection.GetVirtualPath(virtualPathContext); + + Assert.Equal("best", path); + } + + [Fact] + public void GetVirtualPath_NamedRoute_BestEffort_FirstRouteWins() + { + // Arrange + var bestMatch = CreateNamedRoute("Match", accept: false, matchValue: "best"); + var noMatch = CreateNamedRoute("NoMatch", accept: false, matchValue: "bad"); + + var routeCollection = new RouteCollection(); + routeCollection.Add(noMatch); + + var innerRouteCollection = new RouteCollection(); + innerRouteCollection.Add(bestMatch); + routeCollection.Add(innerRouteCollection); + + var options = new RouteOptions() + { + UseBestEffortLinkGeneration = true, + }; + + var virtualPathContext = CreateVirtualPathContext("Match", options: options); + + // Act + var path = routeCollection.GetVirtualPath(virtualPathContext); + + Assert.Equal("best", path); + } + + [Fact] + public void GetVirtualPath_BestEffort_FirstRouteWins() + { + // Arrange + var route1 = CreateRoute(accept: false, match: true, matchValue: "best"); + var route2 = CreateRoute(accept: false, match: true, matchValue: "bad"); + var route3 = CreateRoute(accept: false, match: true, matchValue: "bad"); + + var routeCollection = new RouteCollection(); + routeCollection.Add(route1.Object); + routeCollection.Add(route2.Object); + routeCollection.Add(route3.Object); + + var options = new RouteOptions() + { + UseBestEffortLinkGeneration = true, + }; + + var virtualPathContext = CreateVirtualPathContext(options: options); + + // Act + var path = routeCollection.GetVirtualPath(virtualPathContext); + + Assert.Equal("best", path); + + // All of these should be called + route1.Verify(r => r.GetVirtualPath(It.IsAny()), Times.Once()); + route2.Verify(r => r.GetVirtualPath(It.IsAny()), Times.Once()); + route3.Verify(r => r.GetVirtualPath(It.IsAny()), Times.Once()); + } + + [Fact] + public void GetVirtualPath_NoBestEffort_NoMatch() + { + // Arrange + var route1 = CreateRoute(accept: false, match: true, matchValue: "best"); + var route2 = CreateRoute(accept: false, match: true, matchValue: "bad"); + var route3 = CreateRoute(accept: false, match: true, matchValue: "bad"); + + var routeCollection = new RouteCollection(); + routeCollection.Add(route1.Object); + routeCollection.Add(route2.Object); + routeCollection.Add(route3.Object); + + var options = new RouteOptions() + { + UseBestEffortLinkGeneration = false, + }; + + var virtualPathContext = CreateVirtualPathContext(options: options); + + // Act + var path = routeCollection.GetVirtualPath(virtualPathContext); + + Assert.Null(path); + + // All of these should be called + route1.Verify(r => r.GetVirtualPath(It.IsAny()), Times.Once()); + route2.Verify(r => r.GetVirtualPath(It.IsAny()), Times.Once()); + route3.Verify(r => r.GetVirtualPath(It.IsAny()), Times.Once()); + } + + [Fact] + public void GetVirtualPath_BestEffort_FirstRouteWins_WithNonMatchingRoutes() + { + // Arrange + var route1 = CreateRoute(accept: false, match: false, matchValue: "bad"); + var route2 = CreateRoute(accept: false, match: true, matchValue: "best"); + var route3 = CreateRoute(accept: false, match: true, matchValue: "bad"); + + var routeCollection = new RouteCollection(); + routeCollection.Add(route1.Object); + routeCollection.Add(route2.Object); + routeCollection.Add(route3.Object); + + var options = new RouteOptions() + { + UseBestEffortLinkGeneration = true, + }; + + var virtualPathContext = CreateVirtualPathContext(options: options); + + // Act + var path = routeCollection.GetVirtualPath(virtualPathContext); + + Assert.Equal("best", path); + + // All of these should be called + route1.Verify(r => r.GetVirtualPath(It.IsAny()), Times.Once()); + route2.Verify(r => r.GetVirtualPath(It.IsAny()), Times.Once()); + route3.Verify(r => r.GetVirtualPath(It.IsAny()), Times.Once()); + } + + [Fact] + public void GetVirtualPath_BestEffort_FirstValidatedValuesWins() + { + // Arrange + var route1 = CreateRoute(accept: false, match: true, matchValue: "bad"); + var route2 = CreateRoute(accept: false, match: true, matchValue: "bad"); + var route3 = CreateRoute(accept: true, match: true, matchValue: "best"); + + var routeCollection = new RouteCollection(); + routeCollection.Add(route1.Object); + routeCollection.Add(route2.Object); + routeCollection.Add(route3.Object); + + var options = new RouteOptions() + { + UseBestEffortLinkGeneration = true, + }; + + var virtualPathContext = CreateVirtualPathContext(options: options); + + // Act + var path = routeCollection.GetVirtualPath(virtualPathContext); + + Assert.Equal("best", path); + + // All of these should be called + route1.Verify(r => r.GetVirtualPath(It.IsAny()), Times.Once()); + route2.Verify(r => r.GetVirtualPath(It.IsAny()), Times.Once()); + route3.Verify(r => r.GetVirtualPath(It.IsAny()), Times.Once()); + } + + [Fact] + public void GetVirtualPath_BestEffort_FirstValidatedValuesWins_ShortCircuit() + { + // Arrange + var route1 = CreateRoute(accept: false, match: true, matchValue: "bad"); + var route2 = CreateRoute(accept: true, match: true, matchValue: "best"); + var route3 = CreateRoute(accept: true, match: true, matchValue: "bad"); + + var routeCollection = new RouteCollection(); + routeCollection.Add(route1.Object); + routeCollection.Add(route2.Object); + routeCollection.Add(route3.Object); + + var options = new RouteOptions() + { + UseBestEffortLinkGeneration = true, + }; + + var virtualPathContext = CreateVirtualPathContext(options: options); + + // Act + var path = routeCollection.GetVirtualPath(virtualPathContext); + + Assert.Equal("best", path); + + route1.Verify(r => r.GetVirtualPath(It.IsAny()), Times.Once()); + route2.Verify(r => r.GetVirtualPath(It.IsAny()), Times.Once()); + route3.Verify(r => r.GetVirtualPath(It.IsAny()), Times.Never()); + } + + [Fact] + public void GetVirtualPath_BestEffort_FirstValidatedValuesWins_Nested() + { + // Arrange + var route1 = CreateRoute(accept: false, match: true, matchValue: "bad"); + var route2 = CreateRoute(accept: false, match: true, matchValue: "bad"); + var route3 = CreateRoute(accept: true, match: true, matchValue: "best"); + + var routeCollection = new RouteCollection(); + routeCollection.Add(route1.Object); + + var innerRouteCollection = new RouteCollection(); + innerRouteCollection.Add(route2.Object); + innerRouteCollection.Add(route3.Object); + routeCollection.Add(innerRouteCollection); + + var options = new RouteOptions() + { + UseBestEffortLinkGeneration = true, + }; + + var virtualPathContext = CreateVirtualPathContext(options: options); + + // Act + var path = routeCollection.GetVirtualPath(virtualPathContext); + + Assert.Equal("best", path); + + // All of these should be called + route1.Verify(r => r.GetVirtualPath(It.IsAny()), Times.Once()); + route2.Verify(r => r.GetVirtualPath(It.IsAny()), Times.Once()); + route3.Verify(r => r.GetVirtualPath(It.IsAny()), Times.Once()); + } + private static async Task SetUp(bool enabled, bool handled) { // Arrange @@ -256,7 +551,7 @@ namespace Microsoft.AspNet.Routing var routes = new RouteCollection(); foreach (var routeName in routeNames) { - var route1 = CreateNamedRoute(routeName); + var route1 = CreateNamedRoute(routeName, accept: true); routes.Add(route1); } @@ -265,8 +560,8 @@ namespace Microsoft.AspNet.Routing private static RouteCollection GetNestedRouteCollection(string[] routeNames) { - var rnd = new Random(); - int index = rnd.Next(0, routeNames.Length - 1); + var random = new Random(); + int index = random.Next(0, routeNames.Length - 1); var first = routeNames.Take(index).ToArray(); var second = routeNames.Skip(index).ToArray(); @@ -279,12 +574,12 @@ namespace Microsoft.AspNet.Routing rc4.Add(rc2); // Add a few unnamedRoutes. - rc1.Add(CreateRoute().Object); - rc2.Add(CreateRoute().Object); - rc3.Add(CreateRoute().Object); - rc3.Add(CreateRoute().Object); - rc4.Add(CreateRoute().Object); - rc4.Add(CreateRoute().Object); + rc1.Add(CreateRoute(accept: false).Object); + rc2.Add(CreateRoute(accept: false).Object); + rc3.Add(CreateRoute(accept: false).Object); + rc3.Add(CreateRoute(accept: false).Object); + rc4.Add(CreateRoute(accept: false).Object); + rc4.Add(CreateRoute(accept: false).Object); var routeCollection = new RouteCollection(); routeCollection.Add(rc1); @@ -293,13 +588,18 @@ namespace Microsoft.AspNet.Routing return routeCollection; } - private static INamedRouter CreateNamedRoute(string name, bool accept = false) + private static INamedRouter CreateNamedRoute(string name, bool accept = false, string matchValue = null) { + if (matchValue == null) + { + matchValue = name; + } + var target = new Mock(MockBehavior.Strict); target .Setup(e => e.GetVirtualPath(It.IsAny())) - .Callback(c => c.IsBound = accept) - .Returns(c => name) + .Callback(c => c.IsBound = accept && c.RouteName == name) + .Returns(c => c.RouteName == name ? matchValue : null) .Verifiable(); target @@ -315,36 +615,78 @@ namespace Microsoft.AspNet.Routing return target.Object; } - private static VirtualPathContext CreateVirtualPathContext(string routeName) + private static VirtualPathContext CreateVirtualPathContext( + string routeName = null, + ILoggerFactory loggerFactory = null, + RouteOptions options = null) { - return new VirtualPathContext(null, null, null, routeName); + if (loggerFactory == null) + { + loggerFactory = NullLoggerFactory.Instance; + } + + if (options == null) + { + options = new RouteOptions(); + } + + + var request = new Mock(MockBehavior.Strict); + + var optionsAccessor = new Mock>(MockBehavior.Strict); + optionsAccessor.SetupGet(o => o.Options).Returns(options); + + var context = new Mock(MockBehavior.Strict); + context.Setup(m => m.RequestServices.GetService(typeof(ILoggerFactory))) + .Returns(loggerFactory); + context.Setup(m => m.RequestServices.GetService(typeof(IOptions))) + .Returns(optionsAccessor.Object); + context.SetupGet(c => c.Request).Returns(request.Object); + + return new VirtualPathContext(context.Object, null, null, routeName); } - private static RouteContext CreateRouteContext(string requestPath, ILoggerFactory factory = null) + private static RouteContext CreateRouteContext( + string requestPath, + ILoggerFactory loggerFactory = null, + RouteOptions options = null) { - if (factory == null) + if (loggerFactory == null) { - factory = NullLoggerFactory.Instance; + loggerFactory = NullLoggerFactory.Instance; + } + + if (options == null) + { + options = new RouteOptions(); } var request = new Mock(MockBehavior.Strict); request.SetupGet(r => r.Path).Returns(new PathString(requestPath)); + var optionsAccessor = new Mock>(MockBehavior.Strict); + optionsAccessor.SetupGet(o => o.Options).Returns(options); + var context = new Mock(MockBehavior.Strict); context.Setup(m => m.RequestServices.GetService(typeof(ILoggerFactory))) - .Returns(factory); + .Returns(loggerFactory); + context.Setup(m => m.RequestServices.GetService(typeof(IOptions))) + .Returns(optionsAccessor.Object); context.SetupGet(c => c.Request).Returns(request.Object); return new RouteContext(context.Object); } - private static Mock CreateRoute(bool accept = true) + private static Mock CreateRoute( + bool accept = true, + bool match = false, + string matchValue = "value") { var target = new Mock(MockBehavior.Strict); target .Setup(e => e.GetVirtualPath(It.IsAny())) .Callback(c => c.IsBound = accept) - .Returns(null) + .Returns(accept || match ? matchValue : null) .Verifiable(); target diff --git a/test/Microsoft.AspNet.Routing.Tests/Template/TemplateRouteTest.cs b/test/Microsoft.AspNet.Routing.Tests/Template/TemplateRouteTest.cs index 6309dcab39..1c5ea0db77 100644 --- a/test/Microsoft.AspNet.Routing.Tests/Template/TemplateRouteTest.cs +++ b/test/Microsoft.AspNet.Routing.Tests/Template/TemplateRouteTest.cs @@ -971,7 +971,7 @@ namespace Microsoft.AspNet.Routing.Template } [Fact] - public void GetVirtualPath_RejectedByHandler() + public void GetVirtualPath_ValuesRejectedByHandler_StillGeneratesPath() { // Arrange var route = CreateRoute("{controller}", accept: false); @@ -982,7 +982,7 @@ namespace Microsoft.AspNet.Routing.Template // Assert Assert.False(context.IsBound); - Assert.Null(path); + Assert.Equal("Home", path); } [Fact] @@ -1026,7 +1026,7 @@ namespace Microsoft.AspNet.Routing.Template // Arrange var context = CreateVirtualPathContext(new { p1 = "hello", p2 = "1234" }); - TemplateRoute r = CreateRoute( + var r = CreateRoute( "{p1}/{p2}", new { p2 = "catchall" }, true,