diff --git a/src/Microsoft.AspNet.Mvc.Core/MvcRouteHandler.cs b/src/Microsoft.AspNet.Mvc.Core/MvcRouteHandler.cs index b5d28e6631..e432d4736d 100644 --- a/src/Microsoft.AspNet.Mvc.Core/MvcRouteHandler.cs +++ b/src/Microsoft.AspNet.Mvc.Core/MvcRouteHandler.cs @@ -32,18 +32,16 @@ namespace Microsoft.AspNet.Mvc public async Task RouteAsync([NotNull] RouteContext context) { + // TODO: Throw an error here that's descriptive enough so that + // users understand they should call the per request scoped middleware + // or set HttpContext.Services manually var services = context.HttpContext.RequestServices; + Contract.Assert(services != null); // Verify if AddMvc was done before calling UseMvc // We use the MvcMarkerService to make sure if all the services were added. MvcServicesHelper.ThrowIfMvcNotRegistered(services); - Contract.Assert(services != null); - - // TODO: Throw an error here that's descriptive enough so that - // users understand they should call the per request scoped middleware - // or set HttpContext.Services manually - EnsureLogger(context.HttpContext); using (_logger.BeginScope("MvcRouteHandler.RouteAsync")) { @@ -52,76 +50,75 @@ namespace Microsoft.AspNet.Mvc if (actionDescriptor == null) { - if (_logger.IsEnabled(LogLevel.Verbose)) - { - _logger.WriteValues(new MvcRouteHandlerRouteAsyncValues() - { - ActionSelected = false, - ActionInvoked = false, - Handled = context.IsHandled - }); - } - + LogActionSelection(actionSelected: false, actionInvoked: false, handled: context.IsHandled); return; } + // Replacing the route data allows any code running here to dirty the route values or data-tokens + // without affecting something upstream. + var oldRouteData = context.RouteData; + var newRouteData = new RouteData(oldRouteData); + if (actionDescriptor.RouteValueDefaults != null) { foreach (var kvp in actionDescriptor.RouteValueDefaults) { - if (!context.RouteData.Values.ContainsKey(kvp.Key)) + if (!newRouteData.Values.ContainsKey(kvp.Key)) { - context.RouteData.Values.Add(kvp.Key, kvp.Value); + newRouteData.Values.Add(kvp.Key, kvp.Value); } } } - var actionContext = new ActionContext(context.HttpContext, context.RouteData, actionDescriptor); - - var optionsAccessor = services.GetRequiredService>(); - actionContext.ModelState.MaxAllowedErrors = optionsAccessor.Options.MaxModelValidationErrors; - - var contextAccessor = services.GetRequiredService>(); - using (contextAccessor.SetContextSource(() => actionContext, PreventExchange)) + try { - var invokerFactory = services.GetRequiredService(); - var invoker = invokerFactory.CreateInvoker(actionContext); - if (invoker == null) - { - var ex = new InvalidOperationException( - Resources.FormatActionInvokerFactory_CouldNotCreateInvoker( - actionDescriptor.DisplayName)); + context.RouteData = newRouteData; - // Add tracing/logging (what do we think of this pattern of - // tacking on extra data on the exception?) - ex.Data.Add("AD", actionDescriptor); - - if (_logger.IsEnabled(LogLevel.Verbose)) - { - _logger.WriteValues(new MvcRouteHandlerRouteAsyncValues() - { - ActionSelected = true, - ActionInvoked = false, - Handled = context.IsHandled - }); - } - - throw ex; - } - - await invoker.InvokeAsync(); + await InvokeActionAsync(context, actionDescriptor); context.IsHandled = true; - - if (_logger.IsEnabled(LogLevel.Verbose)) + } + finally + { + if (!context.IsHandled) { - _logger.WriteValues(new MvcRouteHandlerRouteAsyncValues() - { - ActionSelected = true, - ActionInvoked = true, - Handled = context.IsHandled - }); + context.RouteData = oldRouteData; } } + + LogActionSelection(actionSelected: true, actionInvoked: true, handled: context.IsHandled); + } + } + + private async Task InvokeActionAsync(RouteContext context, ActionDescriptor actionDescriptor) + { + var services = context.HttpContext.RequestServices; + Contract.Assert(services != null); + + var actionContext = new ActionContext(context.HttpContext, context.RouteData, actionDescriptor); + + var optionsAccessor = services.GetRequiredService>(); + actionContext.ModelState.MaxAllowedErrors = optionsAccessor.Options.MaxModelValidationErrors; + + var contextAccessor = services.GetRequiredService>(); + using (contextAccessor.SetContextSource(() => actionContext, PreventExchange)) + { + var invokerFactory = services.GetRequiredService(); + var invoker = invokerFactory.CreateInvoker(actionContext); + if (invoker == null) + { + LogActionSelection(actionSelected: true, actionInvoked: false, handled: context.IsHandled); + + var ex = new InvalidOperationException( + Resources.FormatActionInvokerFactory_CouldNotCreateInvoker( + actionDescriptor.DisplayName)); + + // Add tracing/logging (what do we think of this pattern of + // tacking on extra data on the exception?) + ex.Data.Add("AD", actionDescriptor); + throw ex; + } + + await invoker.InvokeAsync(); } } @@ -138,5 +135,18 @@ namespace Microsoft.AspNet.Mvc _logger = factory.Create(); } } + + private void LogActionSelection(bool actionSelected, bool actionInvoked, bool handled) + { + if (_logger.IsEnabled(LogLevel.Verbose)) + { + _logger.WriteValues(new MvcRouteHandlerRouteAsyncValues() + { + ActionSelected = actionSelected, + ActionInvoked = actionInvoked, + Handled = handled, + }); + } + } } } diff --git a/src/Microsoft.AspNet.Mvc.Core/Routing/AttributeRoute.cs b/src/Microsoft.AspNet.Mvc.Core/Routing/AttributeRoute.cs index 6769ea67a5..9332116b89 100644 --- a/src/Microsoft.AspNet.Mvc.Core/Routing/AttributeRoute.cs +++ b/src/Microsoft.AspNet.Mvc.Core/Routing/AttributeRoute.cs @@ -96,22 +96,38 @@ namespace Microsoft.AspNet.Mvc.Routing { foreach (var route in _matchingRoutes) { - await route.RouteAsync(context); + var oldRouteData = context.RouteData; + + var newRouteData = new RouteData(oldRouteData); + newRouteData.Routers.Add(route); + + try + { + context.RouteData = newRouteData; + await route.RouteAsync(context); + } + finally + { + if (!context.IsHandled) + { + context.RouteData = oldRouteData; + } + } if (context.IsHandled) { break; } } + } if (_logger.IsEnabled(LogLevel.Verbose)) + { + _logger.WriteValues(new AttributeRouteRouteAsyncValues() { - _logger.WriteValues(new AttributeRouteRouteAsyncValues() - { - MatchingRoutes = _matchingRoutes, - Handled = context.IsHandled - }); - } + MatchingRoutes = _matchingRoutes, + Handled = context.IsHandled + }); } } diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/MvcRouteHandlerTests.cs b/test/Microsoft.AspNet.Mvc.Core.Test/MvcRouteHandlerTests.cs index 95a001d74a..f3be3f42ea 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/MvcRouteHandlerTests.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/MvcRouteHandlerTests.cs @@ -163,6 +163,91 @@ namespace Microsoft.AspNet.Mvc Assert.True(invoked); } + [Fact] + public async Task RouteAsync_CreatesNewRouteData() + { + // Arrange + RouteData actionRouteData = null; + var invoker = new Mock(); + invoker + .Setup(i => i.InvokeAsync()) + .Returns(Task.FromResult(true)); + + var invokerFactory = new Mock(); + invokerFactory + .Setup(f => f.CreateInvoker(It.IsAny())) + .Returns((c) => + { + actionRouteData = c.RouteData; + return invoker.Object; + }); + + var initialRouter = Mock.Of(); + + var context = CreateRouteContext(invokerFactory: invokerFactory.Object); + var handler = new MvcRouteHandler(); + + var originalRouteData = context.RouteData; + originalRouteData.Routers.Add(initialRouter); + originalRouteData.Values.Add("action", "Index"); + + // Act + await handler.RouteAsync(context); + + // Assert + Assert.NotSame(originalRouteData, context.RouteData); + Assert.NotSame(originalRouteData, actionRouteData); + Assert.Same(actionRouteData, context.RouteData); + + // The new routedata is a copy + Assert.Equal("Index", context.RouteData.Values["action"]); + + Assert.Equal(initialRouter, Assert.Single(context.RouteData.Routers)); + } + + [Fact] + public async Task RouteAsync_ResetsRouteDataOnException() + { + // Arrange + RouteData actionRouteData = null; + var invoker = new Mock(); + invoker + .Setup(i => i.InvokeAsync()) + .Throws(new Exception()); + + var invokerFactory = new Mock(); + invokerFactory + .Setup(f => f.CreateInvoker(It.IsAny())) + .Returns((c) => + { + actionRouteData = c.RouteData; + c.RouteData.Values.Add("action", "Index"); + return invoker.Object; + }); + + var context = CreateRouteContext(invokerFactory: invokerFactory.Object); + var handler = new MvcRouteHandler(); + + var initialRouter = Mock.Of(); + + var originalRouteData = context.RouteData; + originalRouteData.Routers.Add(initialRouter); + + // Act + await Assert.ThrowsAsync(() => handler.RouteAsync(context)); + + // Assert + Assert.Same(originalRouteData, context.RouteData); + Assert.NotSame(originalRouteData, actionRouteData); + Assert.NotSame(actionRouteData, context.RouteData); + + // The new routedata is a copy + Assert.Null(context.RouteData.Values["action"]); + Assert.Equal("Index", actionRouteData.Values["action"]); + + Assert.Equal(initialRouter, Assert.Single(actionRouteData.Routers)); + } + private RouteContext CreateRouteContext( IActionSelector actionSelector = null, IActionInvokerFactory invokerFactory = null, diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/Routing/AttributeRouteTests.cs b/test/Microsoft.AspNet.Mvc.Core.Test/Routing/AttributeRouteTests.cs index ea4f024c44..f9935549e0 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/Routing/AttributeRouteTests.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/Routing/AttributeRouteTests.cs @@ -1099,6 +1099,131 @@ namespace Microsoft.AspNet.Mvc.Routing Assert.Equal("Store", path); } + [Fact] + public async Task AttributeRoute_CreatesNewRouteData() + { + // Arrange + RouteData nestedRouteData = null; + var next = new Mock(); + next + .Setup(r => r.RouteAsync(It.IsAny())) + .Callback((c) => + { + nestedRouteData = c.RouteData; + c.IsHandled = true; + }) + .Returns(Task.FromResult(true)); + + var entry = CreateMatchingEntry(next.Object, "api/Store", order: 0); + var route = CreateAttributeRoute(next.Object, entry); + + var context = CreateRouteContext("/api/Store"); + + var originalRouteData = context.RouteData; + originalRouteData.Values.Add("action", "Index"); + + // Act + await route.RouteAsync(context); + + // Assert + Assert.NotSame(originalRouteData, context.RouteData); + Assert.NotSame(originalRouteData, nestedRouteData); + Assert.Same(nestedRouteData, context.RouteData); + + // The new routedata is a copy + Assert.Equal("Index", context.RouteData.Values["action"]); + Assert.Single(context.RouteData.Values, kvp => kvp.Key == "test_route_group"); + + Assert.IsType(context.RouteData.Routers[0]); + Assert.Same(next.Object, context.RouteData.Routers[1]); + } + + [Fact] + public async Task AttributeRoute_CreatesNewRouteData_ResetsWhenNotMatched() + { + // Arrange + RouteData nestedRouteData = null; + var next = new Mock(); + next + .Setup(r => r.RouteAsync(It.IsAny())) + .Callback((c) => + { + nestedRouteData = c.RouteData; + c.IsHandled = false; + }) + .Returns(Task.FromResult(true)); + + var entry = CreateMatchingEntry(next.Object, "api/Store", order: 0); + var route = CreateAttributeRoute(next.Object, entry); + + var context = CreateRouteContext("/api/Store"); + + var originalRouteData = context.RouteData; + originalRouteData.Values.Add("action", "Index"); + + // Act + await route.RouteAsync(context); + + // Assert + Assert.Same(originalRouteData, context.RouteData); + Assert.NotSame(originalRouteData, nestedRouteData); + Assert.NotSame(nestedRouteData, context.RouteData); + + // The new routedata is a copy + Assert.Equal("Index", context.RouteData.Values["action"]); + Assert.Equal("Index", nestedRouteData.Values["action"]); + Assert.None(context.RouteData.Values, kvp => kvp.Key == "test_route_group"); + Assert.Single(nestedRouteData.Values, kvp => kvp.Key == "test_route_group"); + + Assert.Empty(context.RouteData.Routers); + + Assert.IsType(nestedRouteData.Routers[0]); + Assert.Same(next.Object, nestedRouteData.Routers[1]); + } + + [Fact] + public async Task AttributeRoute_CreatesNewRouteData_ResetsWhenThrows() + { + // Arrange + RouteData nestedRouteData = null; + var next = new Mock(); + next + .Setup(r => r.RouteAsync(It.IsAny())) + .Callback((c) => + { + nestedRouteData = c.RouteData; + c.IsHandled = false; + }) + .Throws(new Exception()); + + var entry = CreateMatchingEntry(next.Object, "api/Store", order: 0); + var route = CreateAttributeRoute(next.Object, entry); + + var context = CreateRouteContext("/api/Store"); + + var originalRouteData = context.RouteData; + originalRouteData.Values.Add("action", "Index"); + + // Act + await Assert.ThrowsAsync(() => route.RouteAsync(context)); + + // Assert + Assert.Same(originalRouteData, context.RouteData); + Assert.NotSame(originalRouteData, nestedRouteData); + Assert.NotSame(nestedRouteData, context.RouteData); + + // The new routedata is a copy + Assert.Equal("Index", context.RouteData.Values["action"]); + Assert.Equal("Index", nestedRouteData.Values["action"]); + Assert.None(context.RouteData.Values, kvp => kvp.Key == "test_route_group"); + Assert.Single(nestedRouteData.Values, kvp => kvp.Key == "test_route_group"); + + Assert.Empty(context.RouteData.Routers); + + Assert.IsType(nestedRouteData.Routers[0]); + Assert.Same(next.Object, nestedRouteData.Routers[1]); + } + private static RouteContext CreateRouteContext(string requestPath) { var request = new Mock(MockBehavior.Strict); @@ -1241,6 +1366,15 @@ namespace Microsoft.AspNet.Mvc.Routing NullLoggerFactory.Instance); } + private static AttributeRoute CreateAttributeRoute(IRouter next, params AttributeRouteMatchingEntry[] entries) + { + return new AttributeRoute( + next, + entries, + Enumerable.Empty(), + NullLoggerFactory.Instance); + } + private static AttributeRoute CreateRoutingAttributeRoute(ILoggerFactory loggerFactory = null, params AttributeRouteMatchingEntry[] entries) { loggerFactory = loggerFactory ?? NullLoggerFactory.Instance; diff --git a/test/Microsoft.AspNet.Mvc.FunctionalTests/RouteDataTest.cs b/test/Microsoft.AspNet.Mvc.FunctionalTests/RouteDataTest.cs new file mode 100644 index 0000000000..065555ed6d --- /dev/null +++ b/test/Microsoft.AspNet.Mvc.FunctionalTests/RouteDataTest.cs @@ -0,0 +1,79 @@ +// 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.Net; +using System.Threading.Tasks; +using Microsoft.AspNet.Builder; +using Microsoft.AspNet.Mvc.Routing; +using Microsoft.AspNet.Routing; +using Microsoft.AspNet.Routing.Template; +using Microsoft.AspNet.TestHost; +using Newtonsoft.Json; +using Xunit; + +namespace Microsoft.AspNet.Mvc.FunctionalTests +{ + public class RouteDataTest + { + private readonly IServiceProvider _services = TestHelper.CreateServices(nameof(BasicWebSite)); + private readonly Action _app = new BasicWebSite.Startup().Configure; + + [Fact] + public async Task RouteData_Routers_ConventionalRoute() + { + // Arrange + var server = TestServer.Create(_services, _app); + var client = server.CreateClient(); + + // Act + var response = await client.GetAsync("http://localhost/Routing/Conventional"); + + // Assert + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + + var body = await response.Content.ReadAsStringAsync(); + var result = JsonConvert.DeserializeObject(body); + + Assert.Equal(new string[] + { + typeof(RouteCollection).FullName, + typeof(TemplateRoute).FullName, + typeof(MvcRouteHandler).FullName, + }, + result.Routers); + } + + [Fact] + public async Task RouteData_Routers_AttributeRoute() + { + // Arrange + var server = TestServer.Create(_services, _app); + var client = server.CreateClient(); + + // Act + var response = await client.GetAsync("http://localhost/Routing/Attribute"); + + // Assert + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + + var body = await response.Content.ReadAsStringAsync(); + var result = JsonConvert.DeserializeObject(body); + + Assert.Equal(new string[] + { + typeof(RouteCollection).FullName, + typeof(AttributeRoute).FullName, + typeof(TemplateRoute).FullName, + typeof(MvcRouteHandler).FullName, + }, + result.Routers); + } + + + private class ResultData + { + public string[] Routers { get; set; } + } + } +} \ No newline at end of file diff --git a/test/WebSites/BasicWebSite/Controllers/RoutingController.cs b/test/WebSites/BasicWebSite/Controllers/RoutingController.cs new file mode 100644 index 0000000000..e54736185f --- /dev/null +++ b/test/WebSites/BasicWebSite/Controllers/RoutingController.cs @@ -0,0 +1,28 @@ +// 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.Linq; +using Microsoft.AspNet.Mvc; + +namespace BasicWebSite +{ + public class RoutingController : Controller + { + public object Conventional() + { + return GetData(); + } + + [Route("Routing/Attribute")] + public object Attribute() + { + return GetData(); + } + + private object GetData() + { + var routers = ActionContext.RouteData.Routers.Select(r => r.GetType().FullName).ToArray(); + return new { Routers = routers }; + } + } +} \ No newline at end of file