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);