From 02a0a218b94dd70e398093e82c1bd3038489fd44 Mon Sep 17 00:00:00 2001 From: jacalvar Date: Mon, 20 Oct 2014 16:22:08 -0700 Subject: [PATCH] [Fixes #90] RouteTemplate does not take RouteData Changed the implementation of route template to merge the existing route data with the values obtained from parsing the request path with the given template. Restored original route data values in case the route template data does not match. --- .../Template/TemplateRoute.cs | 86 +++++++--- .../Template/TemplateRouteTest.cs | 152 ++++++++++++++++-- 2 files changed, 202 insertions(+), 36 deletions(-) diff --git a/src/Microsoft.AspNet.Routing/Template/TemplateRoute.cs b/src/Microsoft.AspNet.Routing/Template/TemplateRoute.cs index d2d742c908..9648e3199f 100644 --- a/src/Microsoft.AspNet.Routing/Template/TemplateRoute.cs +++ b/src/Microsoft.AspNet.Routing/Template/TemplateRoute.cs @@ -121,39 +121,51 @@ namespace Microsoft.AspNet.Routing.Template } else { - // Not currently doing anything to clean this up if it's not a match. Consider hardening this. - context.RouteData.Values = values; + var originalValues = context.RouteData.Values; + context.RouteData.Values = MergeValues(originalValues, values); - if (RouteConstraintMatcher.Match(Constraints, - values, - context.HttpContext, - this, - RouteDirection.IncomingRequest, - _constraintLogger)) + try { - context.RouteData.DataTokens = _dataTokens; - await _target.RouteAsync(context); - - if (_logger.IsEnabled(TraceType.Information)) + if (RouteConstraintMatcher.Match(Constraints, + values, + context.HttpContext, + this, + RouteDirection.IncomingRequest, + _constraintLogger)) { - _logger.WriteValues(CreateRouteAsyncValues( - requestPath, - values, - matchedValues: true, - matchedConstraints: true, - handled: context.IsHandled)); + context.RouteData.DataTokens = _dataTokens; + await _target.RouteAsync(context); + + if (_logger.IsEnabled(TraceType.Information)) + { + _logger.WriteValues(CreateRouteAsyncValues( + requestPath, + values, + matchedValues: true, + matchedConstraints: true, + handled: context.IsHandled)); + } + } + else + { + if (_logger.IsEnabled(TraceType.Information)) + { + _logger.WriteValues(CreateRouteAsyncValues( + requestPath, + values, + matchedValues: true, + matchedConstraints: false, + handled: context.IsHandled)); + } } } - else + finally { - if (_logger.IsEnabled(TraceType.Information)) + if (!context.IsHandled) { - _logger.WriteValues(CreateRouteAsyncValues( - requestPath, - values, - matchedValues: true, - matchedConstraints: false, - handled: 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; } } } @@ -291,6 +303,28 @@ namespace Microsoft.AspNet.Routing.Template return values; } + private static IDictionary MergeValues( + IDictionary originalValues, + 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; + } + + return result; + } + private void EnsureLoggers(HttpContext context) { if (_logger == null) diff --git a/test/Microsoft.AspNet.Routing.Tests/Template/TemplateRouteTest.cs b/test/Microsoft.AspNet.Routing.Tests/Template/TemplateRouteTest.cs index 5de78131d0..49511d9a94 100644 --- a/test/Microsoft.AspNet.Routing.Tests/Template/TemplateRouteTest.cs +++ b/test/Microsoft.AspNet.Routing.Tests/Template/TemplateRouteTest.cs @@ -168,6 +168,138 @@ namespace Microsoft.AspNet.Routing.Template Assert.Equal(context.IsHandled, values.Handled); } + [Fact] + public async Task RouteAsync_MergesExistingRouteData_IfRouteMatches() + { + // Arrange + var template = "{controller}/{action}/{id:int}"; + + var context = CreateRouteContext("/Home/Index/5"); + var originalRouteDataValues = new Dictionary + { + { "country", "USA" }, + }; + context.RouteData.Values = originalRouteDataValues; + + IDictionary routeValues = null; + var mockTarget = new Mock(MockBehavior.Strict); + mockTarget + .Setup(s => s.RouteAsync(It.IsAny())) + .Callback(ctx => + { + routeValues = ctx.RouteData.Values; + ctx.IsHandled = true; + }) + .Returns(Task.FromResult(true)); + + var route = new TemplateRoute(mockTarget.Object, template, _inlineConstraintResolver); + + // Act + await route.RouteAsync(context); + + // Assert + Assert.NotNull(routeValues); + + Assert.True(routeValues.ContainsKey("country")); + Assert.Equal("USA", routeValues["country"]); + Assert.True(routeValues.ContainsKey("id")); + Assert.Equal("5",routeValues["id"]); + + Assert.True(context.RouteData.Values.ContainsKey("country")); + 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); + } + + [Fact] + public async Task RouteAsync_CleansUpMergedRouteData_IfRouteDoesNotMatch() + { + // Arrange + var template = "{controller}/{action}/{id:int}"; + + var context = CreateRouteContext("/Home/Index/5"); + var originalRouteDataValues = new Dictionary + { + { "country", "USA" }, + }; + context.RouteData.Values = originalRouteDataValues; + + IDictionary routeValues = null; + var mockTarget = new Mock(MockBehavior.Strict); + mockTarget + .Setup(s => s.RouteAsync(It.IsAny())) + .Callback(ctx => + { + routeValues = ctx.RouteData.Values; + ctx.IsHandled = false; + }) + .Returns(Task.FromResult(true)); + + var route = new TemplateRoute(mockTarget.Object, template, _inlineConstraintResolver); + + // Act + await route.RouteAsync(context); + + // Assert + Assert.NotNull(routeValues); + + Assert.True(routeValues.ContainsKey("country")); + Assert.Equal("USA", routeValues["country"]); + Assert.True(routeValues.ContainsKey("id")); + Assert.Equal("5", routeValues["id"]); + + Assert.True(context.RouteData.Values.ContainsKey("country")); + Assert.Equal("USA", context.RouteData.Values["country"]); + + Assert.Same(originalRouteDataValues, context.RouteData.Values); + } + + [Fact] + public async Task RouteAsync_CleansUpMergedRouteData_IfHandlerThrows() + { + // Arrange + var template = "{controller}/{action}/{id:int}"; + + var context = CreateRouteContext("/Home/Index/5"); + var originalRouteDataValues = new Dictionary + { + { "country", "USA" }, + }; + context.RouteData.Values = originalRouteDataValues; + + IDictionary routeValues = null; + var mockTarget = new Mock(MockBehavior.Strict); + mockTarget + .Setup(s => s.RouteAsync(It.IsAny())) + .Callback(ctx => + { + routeValues = ctx.RouteData.Values; + ctx.IsHandled = false; + }) + .Throws(new Exception()); + + var route = new TemplateRoute(mockTarget.Object, template, _inlineConstraintResolver); + + // Act + var ex = await Assert.ThrowsAsync(() => route.RouteAsync(context)); + + // Assert + Assert.NotNull(routeValues); + + Assert.True(routeValues.ContainsKey("country")); + Assert.Equal("USA", routeValues["country"]); + Assert.True(routeValues.ContainsKey("id")); + Assert.Equal("5", routeValues["id"]); + + Assert.True(context.RouteData.Values.ContainsKey("country")); + Assert.Equal("USA", context.RouteData.Values["country"]); + + Assert.Same(originalRouteDataValues, context.RouteData.Values); + } + + [Fact] public async Task RouteAsync_MatchFailOnConstraints_DoesNotLogWhenDisabled() { @@ -265,7 +397,7 @@ namespace Microsoft.AspNet.Routing.Template Assert.False(context.IsHandled); // Issue #16 tracks this. - Assert.NotNull(context.RouteData.Values); + Assert.Null(context.RouteData.Values); } [Fact] @@ -490,9 +622,9 @@ namespace Microsoft.AspNet.Routing.Template .Returns(null); var route = CreateRoute(target.Object, "{controller}/{action}"); - var context = CreateVirtualPathContext(new { action = "Store" }, new { Controller = "Home", action = "Blog"}); + var context = CreateVirtualPathContext(new { action = "Store" }, new { Controller = "Home", action = "Blog" }); - var expectedValues = new RouteValueDictionary(new {controller = "Home", action = "Store"}); + var expectedValues = new RouteValueDictionary(new { controller = "Home", action = "Store" }); // Act var path = route.GetVirtualPath(context); @@ -513,7 +645,7 @@ namespace Microsoft.AspNet.Routing.Template .Callback(c => { childContext = c; c.IsBound = true; }) .Returns(null); - var route = CreateRoute(target.Object, "Admin/{controller}/{action}", new {area = "Admin"}); + var route = CreateRoute(target.Object, "Admin/{controller}/{action}", new { area = "Admin" }); var context = CreateVirtualPathContext(new { action = "Store" }, new { Controller = "Home", action = "Blog" }); var expectedValues = new RouteValueDictionary(new { controller = "Home", action = "Store", area = "Admin" }); @@ -564,7 +696,7 @@ namespace Microsoft.AspNet.Routing.Template constraints: new { c = constraint }); var context = CreateVirtualPathContext( - values: new { action = "Store" }, + values: new { action = "Store" }, ambientValues: new { Controller = "Home", action = "Blog", extra = "42" }); var expectedValues = new RouteValueDictionary( @@ -750,7 +882,7 @@ namespace Microsoft.AspNet.Routing.Template var routeBuilder = CreateRouteBuilder(); // Assert - ExceptionAssert.Throws(() => routeBuilder.MapRoute("mockName", + ExceptionAssert.Throws(() => routeBuilder.MapRoute("mockName", "{controller}/{action}", defaults: null, constraints: new { controller = "a.*", action = new Object() }), @@ -937,10 +1069,10 @@ namespace Microsoft.AspNet.Routing.Template public IDictionary Values { get; private set; } public bool Match( - HttpContext httpContext, - IRouter route, - string routeKey, - IDictionary values, + HttpContext httpContext, + IRouter route, + string routeKey, + IDictionary values, RouteDirection routeDirection) { Values = new RouteValueDictionary(values);