Create a new routedata for each 'router' for MVC

This is the MVC companion to https://github.com/aspnet/Routing/pull/122

As routing flows, routes replace the route data and mutate a copy. This
allows users to make changes that dirty the data without affecting
undesired state changes.

We also add the 'next' router for diagnostic purposes.

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))
+                    LogActionSelection(actionSelected: false, actionInvoked: false, handled: context.IsHandled);
-                    {
-                        _logger.WriteValues(new MvcRouteHandlerRouteAsyncValues()
-                        {
-                            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);
+                try
-
-                var optionsAccessor = services.GetRequiredService<IOptions<MvcOptions>>();
-                actionContext.ModelState.MaxAllowedErrors = optionsAccessor.Options.MaxModelValidationErrors;
-
-                var contextAccessor = services.GetRequiredService<IContextAccessor<ActionContext>>();
-                using (contextAccessor.SetContextSource(() => actionContext, PreventExchange))
                 {
-                    var invokerFactory = services.GetRequiredService<IActionInvokerFactory>();
+                    context.RouteData = newRouteData;
-                    var invoker = invokerFactory.CreateInvoker(actionContext);
-                    if (invoker == null)
-                    {
-                        var ex = new InvalidOperationException(
-                            Resources.FormatActionInvokerFactory_CouldNotCreateInvoker(
-                                actionDescriptor.DisplayName));
 
-                        // Add tracing/logging (what do we think of this pattern of 
+                    await InvokeActionAsync(context, actionDescriptor);
-                        // 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();
                     context.IsHandled = true;
-
+                }
-                    if (_logger.IsEnabled(LogLevel.Verbose))
+                finally
+                {
+                    if (!context.IsHandled)
                     {
-                        _logger.WriteValues(new MvcRouteHandlerRouteAsyncValues()
+                        context.RouteData = oldRouteData;
-                        {
-                            ActionSelected = true,
-                            ActionInvoked = true,
-                            Handled = context.IsHandled
-                        });
                     }
                 }
+
+                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<IOptions<MvcOptions>>();
+            actionContext.ModelState.MaxAllowedErrors = optionsAccessor.Options.MaxModelValidationErrors;
+
+            var contextAccessor = services.GetRequiredService<IContextAccessor<ActionContext>>();
+            using (contextAccessor.SetContextSource(() => actionContext, PreventExchange))
+            {
+                var invokerFactory = services.GetRequiredService<IActionInvokerFactory>();
+                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<MvcRouteHandler>();
             }
         }
+
+        private void LogActionSelection(bool actionSelected, bool actionInvoked, bool handled)
+        {
+            if (_logger.IsEnabled(LogLevel.Verbose))
+            {
+                _logger.WriteValues(new MvcRouteHandlerRouteAsyncValues()
+                {
+                    ActionSelected = actionSelected,
+                    ActionInvoked = actionInvoked,
+                    Handled = handled,
+                });
+            }
+        }
     }
 }

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

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<IActionInvoker>();
+            invoker
+                .Setup(i => i.InvokeAsync())
+                .Returns(Task.FromResult(true));
+
+            var invokerFactory = new Mock<IActionInvokerFactory>();
+            invokerFactory
+                .Setup(f => f.CreateInvoker(It.IsAny<ActionContext>()))
+                .Returns<ActionContext>((c) =>
+                {
+                    actionRouteData = c.RouteData;
+                    return invoker.Object;
+                });
+
+            var initialRouter = Mock.Of<IRouter>();
+
+            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<IActionInvoker>();
+            invoker
+                .Setup(i => i.InvokeAsync())
+                .Throws(new Exception());
+
+            var invokerFactory = new Mock<IActionInvokerFactory>();
+            invokerFactory
+                .Setup(f => f.CreateInvoker(It.IsAny<ActionContext>()))
+                .Returns<ActionContext>((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<IRouter>();
+
+            var originalRouteData = context.RouteData;
+            originalRouteData.Routers.Add(initialRouter);
+
+            // Act
+            await Assert.ThrowsAsync<Exception>(() => 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,

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<IRouter>();
+            next
+                .Setup(r => r.RouteAsync(It.IsAny<RouteContext>()))
+                .Callback<RouteContext>((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<TemplateRoute>(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<IRouter>();
+            next
+                .Setup(r => r.RouteAsync(It.IsAny<RouteContext>()))
+                .Callback<RouteContext>((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<TemplateRoute>(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<IRouter>();
+            next
+                .Setup(r => r.RouteAsync(It.IsAny<RouteContext>()))
+                .Callback<RouteContext>((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<Exception>(() => 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<TemplateRoute>(nestedRouteData.Routers[0]);
+            Assert.Same(next.Object, nestedRouteData.Routers[1]);
+        }
+
         private static RouteContext CreateRouteContext(string requestPath)
         {
             var request = new Mock<HttpRequest>(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<AttributeRouteLinkGenerationEntry>(),
+                NullLoggerFactory.Instance);
+        }
+
         private static AttributeRoute CreateRoutingAttributeRoute(ILoggerFactory loggerFactory = null, params AttributeRouteMatchingEntry[] entries)
         {
             loggerFactory = loggerFactory ?? NullLoggerFactory.Instance;

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<IApplicationBuilder> _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<ResultData>(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<ResultData>(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; }
+        }
+    }
+}

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 };
+        }
+    }
+}