From d78e5478a7b6cd853182d8b7a1f36d124ca4f42a Mon Sep 17 00:00:00 2001 From: Ryan Nowak Date: Wed, 29 Oct 2014 18:24:50 -0700 Subject: [PATCH] Fix for #65,116 - Implement 'stack of routers' This is the routing part of the fix. MVC will be updated as well (attribute routing). As the graph of routers is traversed, routers add themselves to the current 'path', which unwinds on a failed path. This mechanism is opt-in. Whoever adds something needs to remove it as part of cleanup. If a router in the tree doesn't interact with the .Routers property, then there are no consequences for those that do. Additionally, fixing #116 as part of the same change. This means that we create a nested 'RouteData' and then restore it on the way out. This is simpler than just dealing with the .Routers property in isolation. --- .../RouteCollection.cs | 23 +++- src/Microsoft.AspNet.Routing/RouteData.cs | 17 ++- .../Template/TemplateRoute.cs | 106 ++++++++--------- .../RouteCollectionTest.cs | 8 ++ .../Template/TemplateRouteTest.cs | 107 ++++++++++++++---- 5 files changed, 175 insertions(+), 86 deletions(-) diff --git a/src/Microsoft.AspNet.Routing/RouteCollection.cs b/src/Microsoft.AspNet.Routing/RouteCollection.cs index 787188ed0f..d0e34ee99c 100644 --- a/src/Microsoft.AspNet.Routing/RouteCollection.cs +++ b/src/Microsoft.AspNet.Routing/RouteCollection.cs @@ -57,10 +57,27 @@ namespace Microsoft.AspNet.Routing { var route = this[i]; - await route.RouteAsync(context); - if (context.IsHandled) + var oldRouteData = context.RouteData; + + var newRouteData = new RouteData(oldRouteData); + newRouteData.Routers.Add(route); + + try { - break; + context.RouteData = newRouteData; + + await route.RouteAsync(context); + if (context.IsHandled) + { + break; + } + } + finally + { + if (!context.IsHandled) + { + context.RouteData = oldRouteData; + } } } diff --git a/src/Microsoft.AspNet.Routing/RouteData.cs b/src/Microsoft.AspNet.Routing/RouteData.cs index 1d818aaec4..e8992b79f0 100644 --- a/src/Microsoft.AspNet.Routing/RouteData.cs +++ b/src/Microsoft.AspNet.Routing/RouteData.cs @@ -1,24 +1,31 @@ // Copyright (c) Microsoft Open Technologies, Inc. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. +using System; using System.Collections.Generic; namespace Microsoft.AspNet.Routing { - /// - /// Summary description for RouteData - /// public class RouteData { public RouteData() { + DataTokens = new Dictionary(StringComparer.OrdinalIgnoreCase); Routers = new List(); + Values = new RouteValueDictionary(); + } + + public RouteData([NotNull] RouteData other) + { + DataTokens = new Dictionary(other.DataTokens, StringComparer.OrdinalIgnoreCase); + Routers = new List(other.Routers); + Values = new RouteValueDictionary(other.Values); } public List Routers { get; private set; } - public IDictionary Values { get; set; } + public IDictionary Values { get; private set; } - public IDictionary DataTokens { get; set; } + public IDictionary DataTokens { get; private set; } } } \ No newline at end of file diff --git a/src/Microsoft.AspNet.Routing/Template/TemplateRoute.cs b/src/Microsoft.AspNet.Routing/Template/TemplateRoute.cs index 15d62adc1f..33e88554aa 100644 --- a/src/Microsoft.AspNet.Routing/Template/TemplateRoute.cs +++ b/src/Microsoft.AspNet.Routing/Template/TemplateRoute.cs @@ -120,54 +120,57 @@ namespace Microsoft.AspNet.Routing.Template // If we got back a null value set, that means the URI did not match return; } - else + + if (!RouteConstraintMatcher.Match( + Constraints, + values, + context.HttpContext, + this, + RouteDirection.IncomingRequest, + _constraintLogger)) { - var originalValues = context.RouteData.Values; - context.RouteData.Values = MergeValues(originalValues, values); - - try + if (_logger.IsEnabled(TraceType.Verbose)) { - if (RouteConstraintMatcher.Match(Constraints, - values, - context.HttpContext, - this, - RouteDirection.IncomingRequest, - _constraintLogger)) - { - context.RouteData.DataTokens = _dataTokens; - await _target.RouteAsync(context); - - if (_logger.IsEnabled(TraceType.Verbose)) - { - _logger.WriteValues(CreateRouteAsyncValues( - requestPath, - values, - matchedValues: true, - matchedConstraints: true, - handled: context.IsHandled)); - } - } - else - { - if (_logger.IsEnabled(TraceType.Verbose)) - { - _logger.WriteValues(CreateRouteAsyncValues( - requestPath, - values, - matchedValues: true, - matchedConstraints: false, - handled: context.IsHandled)); - } - } + _logger.WriteValues(CreateRouteAsyncValues( + requestPath, + values, + matchedValues: true, + matchedConstraints: false, + handled: context.IsHandled)); } - finally + + return; + } + + var oldRouteData = context.RouteData; + + var newRouteData = new RouteData(oldRouteData); + MergeValues(newRouteData.DataTokens, _dataTokens); + newRouteData.Routers.Add(_target); + MergeValues(newRouteData.Values, values); + + try + { + context.RouteData = newRouteData; + + await _target.RouteAsync(context); + + if (_logger.IsEnabled(TraceType.Verbose)) { - if (!context.IsHandled) - { - // Restore the original route data if we didn't handle the route to prevent - // poluting the original route data with the values from this route. - context.RouteData.Values = originalValues; - } + _logger.WriteValues(CreateRouteAsyncValues( + requestPath, + values, + matchedValues: true, + matchedConstraints: true, + handled: context.IsHandled)); + } + } + finally + { + // Restore the original values to prevent polluting the route data. + if (!context.IsHandled) + { + context.RouteData = oldRouteData; } } } @@ -304,26 +307,17 @@ namespace Microsoft.AspNet.Routing.Template return values; } - private static IDictionary MergeValues( - IDictionary originalValues, + private static void MergeValues( + IDictionary destination, IDictionary values) { - if (originalValues == null) - { - // There is nothing to merge - return values; - } - - var result = new RouteValueDictionary(originalValues); foreach (var kvp in values) { // This will replace the original value for the specified key. // Values from the matched route will take preference over previous // data in the route context. - result[kvp.Key] = kvp.Value; + destination[kvp.Key] = kvp.Value; } - - return result; } private void EnsureLoggers(HttpContext context) diff --git a/test/Microsoft.AspNet.Routing.Tests/RouteCollectionTest.cs b/test/Microsoft.AspNet.Routing.Tests/RouteCollectionTest.cs index b416ca4384..6179818e1f 100644 --- a/test/Microsoft.AspNet.Routing.Tests/RouteCollectionTest.cs +++ b/test/Microsoft.AspNet.Routing.Tests/RouteCollectionTest.cs @@ -114,6 +114,9 @@ namespace Microsoft.AspNet.Routing route1.Verify(e => e.RouteAsync(It.IsAny()), Times.Exactly(1)); route2.Verify(e => e.RouteAsync(It.IsAny()), Times.Exactly(0)); Assert.True(context.IsHandled); + + Assert.Equal(1, context.RouteData.Routers.Count); + Assert.Same(route1.Object, context.RouteData.Routers[0]); } [Fact] @@ -137,6 +140,9 @@ namespace Microsoft.AspNet.Routing route1.Verify(e => e.RouteAsync(It.IsAny()), Times.Exactly(1)); route2.Verify(e => e.RouteAsync(It.IsAny()), Times.Exactly(1)); Assert.True(context.IsHandled); + + Assert.Equal(1, context.RouteData.Routers.Count); + Assert.Same(route2.Object, context.RouteData.Routers[0]); } [Fact] @@ -160,6 +166,8 @@ namespace Microsoft.AspNet.Routing route1.Verify(e => e.RouteAsync(It.IsAny()), Times.Exactly(1)); route2.Verify(e => e.RouteAsync(It.IsAny()), Times.Exactly(1)); Assert.False(context.IsHandled); + + Assert.Empty(context.RouteData.Routers); } [Fact] diff --git a/test/Microsoft.AspNet.Routing.Tests/Template/TemplateRouteTest.cs b/test/Microsoft.AspNet.Routing.Tests/Template/TemplateRouteTest.cs index 49511d9a94..744471f282 100644 --- a/test/Microsoft.AspNet.Routing.Tests/Template/TemplateRouteTest.cs +++ b/test/Microsoft.AspNet.Routing.Tests/Template/TemplateRouteTest.cs @@ -175,11 +175,12 @@ namespace Microsoft.AspNet.Routing.Template var template = "{controller}/{action}/{id:int}"; var context = CreateRouteContext("/Home/Index/5"); - var originalRouteDataValues = new Dictionary - { - { "country", "USA" }, - }; - context.RouteData.Values = originalRouteDataValues; + + var originalRouteDataValues = context.RouteData.Values; + originalRouteDataValues.Add("country", "USA"); + + var originalDataTokens = context.RouteData.DataTokens; + originalDataTokens.Add("company", "Contoso"); IDictionary routeValues = null; var mockTarget = new Mock(MockBehavior.Strict); @@ -192,7 +193,13 @@ namespace Microsoft.AspNet.Routing.Template }) .Returns(Task.FromResult(true)); - var route = new TemplateRoute(mockTarget.Object, template, _inlineConstraintResolver); + var route = new TemplateRoute( + mockTarget.Object, + template, + defaults: null, + constraints: null, + dataTokens: new RouteValueDictionary(new { today = "Friday" }), + inlineConstraintResolver: _inlineConstraintResolver); // Act await route.RouteAsync(context); @@ -209,8 +216,12 @@ namespace Microsoft.AspNet.Routing.Template Assert.Equal("USA", context.RouteData.Values["country"]); Assert.True(context.RouteData.Values.ContainsKey("id")); Assert.Equal("5", context.RouteData.Values["id"]); - Assert.NotSame(originalRouteDataValues, context.RouteData.Values); + + Assert.Equal("Contoso", context.RouteData.DataTokens["company"]); + Assert.Equal("Friday", context.RouteData.DataTokens["today"]); + Assert.NotSame(originalDataTokens, context.RouteData.DataTokens); + Assert.NotSame(route.DataTokens, context.RouteData.DataTokens); } [Fact] @@ -220,11 +231,11 @@ namespace Microsoft.AspNet.Routing.Template var template = "{controller}/{action}/{id:int}"; var context = CreateRouteContext("/Home/Index/5"); - var originalRouteDataValues = new Dictionary - { - { "country", "USA" }, - }; - context.RouteData.Values = originalRouteDataValues; + var originalRouteDataValues = context.RouteData.Values; + originalRouteDataValues.Add("country", "USA"); + + var originalDataTokens = context.RouteData.DataTokens; + originalDataTokens.Add("company", "Contoso"); IDictionary routeValues = null; var mockTarget = new Mock(MockBehavior.Strict); @@ -237,7 +248,13 @@ namespace Microsoft.AspNet.Routing.Template }) .Returns(Task.FromResult(true)); - var route = new TemplateRoute(mockTarget.Object, template, _inlineConstraintResolver); + var route = new TemplateRoute( + mockTarget.Object, + template, + defaults: null, + constraints: null, + dataTokens: new RouteValueDictionary(new { today = "Friday" }), + inlineConstraintResolver: _inlineConstraintResolver); // Act await route.RouteAsync(context); @@ -252,8 +269,12 @@ namespace Microsoft.AspNet.Routing.Template Assert.True(context.RouteData.Values.ContainsKey("country")); Assert.Equal("USA", context.RouteData.Values["country"]); - + Assert.False(context.RouteData.Values.ContainsKey("id")); Assert.Same(originalRouteDataValues, context.RouteData.Values); + + Assert.Equal("Contoso", context.RouteData.DataTokens["company"]); + Assert.False(context.RouteData.DataTokens.ContainsKey("today")); + Assert.Same(originalDataTokens, context.RouteData.DataTokens); } [Fact] @@ -263,11 +284,11 @@ namespace Microsoft.AspNet.Routing.Template var template = "{controller}/{action}/{id:int}"; var context = CreateRouteContext("/Home/Index/5"); - var originalRouteDataValues = new Dictionary - { - { "country", "USA" }, - }; - context.RouteData.Values = originalRouteDataValues; + var originalRouteDataValues = context.RouteData.Values; + originalRouteDataValues.Add("country", "USA"); + + var originalDataTokens = context.RouteData.DataTokens; + originalDataTokens.Add("company", "Contoso"); IDictionary routeValues = null; var mockTarget = new Mock(MockBehavior.Strict); @@ -280,7 +301,13 @@ namespace Microsoft.AspNet.Routing.Template }) .Throws(new Exception()); - var route = new TemplateRoute(mockTarget.Object, template, _inlineConstraintResolver); + var route = new TemplateRoute( + mockTarget.Object, + template, + defaults: null, + constraints: null, + dataTokens: new RouteValueDictionary(new { today = "Friday" }), + inlineConstraintResolver: _inlineConstraintResolver); // Act var ex = await Assert.ThrowsAsync(() => route.RouteAsync(context)); @@ -295,8 +322,12 @@ namespace Microsoft.AspNet.Routing.Template Assert.True(context.RouteData.Values.ContainsKey("country")); Assert.Equal("USA", context.RouteData.Values["country"]); - + Assert.False(context.RouteData.Values.ContainsKey("id")); Assert.Same(originalRouteDataValues, context.RouteData.Values); + + Assert.Equal("Contoso", context.RouteData.DataTokens["company"]); + Assert.False(context.RouteData.DataTokens.ContainsKey("today")); + Assert.Same(originalDataTokens, context.RouteData.DataTokens); } @@ -397,7 +428,39 @@ namespace Microsoft.AspNet.Routing.Template Assert.False(context.IsHandled); // Issue #16 tracks this. - Assert.Null(context.RouteData.Values); + Assert.Empty(context.RouteData.Values); + } + + [Fact] + public async Task Match_RejectedByHandler_ClearsRouters() + { + // Arrange + var route = CreateRoute("{controller}", accept: false); + var context = CreateRouteContext("/Home"); + + // Act + await route.RouteAsync(context); + + // Assert + Assert.False(context.IsHandled); + Assert.Empty(context.RouteData.Routers); + } + + [Fact] + public async Task Match_SetsRouters() + { + // Arrange + var target = CreateTarget(accept: true); + var route = CreateRoute(target, "{controller}"); + var context = CreateRouteContext("/Home"); + + // Act + await route.RouteAsync(context); + + // Assert + Assert.True(context.IsHandled); + Assert.Equal(1, context.RouteData.Routers.Count); + Assert.Same(target, context.RouteData.Routers[0]); } [Fact]