From ed93a6c812a905cb6312f5ccaee5c33b878e44d9 Mon Sep 17 00:00:00 2001 From: Ryan Nowak Date: Fri, 11 Dec 2015 09:29:54 -0800 Subject: [PATCH] React to routing breaking changes --- .../Infrastructure/MvcRouteHandler.cs | 66 ++++---- .../Routing/AttributeRoute.cs | 15 +- .../MvcAreaRouteBuilderExtensionsTest.cs | 12 +- .../Infrastructure/MvcRouteHandlerTests.cs | 150 +++++------------- .../Routing/AttributeRouteTest.cs | 8 +- .../Routing/UrlHelperTest.cs | 2 +- .../RouteDataTest.cs | 2 +- .../CustomRouteWebSite/LocalizedRoute.cs | 26 +-- 8 files changed, 94 insertions(+), 187 deletions(-) diff --git a/src/Microsoft.AspNet.Mvc.Core/Infrastructure/MvcRouteHandler.cs b/src/Microsoft.AspNet.Mvc.Core/Infrastructure/MvcRouteHandler.cs index 12027f4336..b685d54325 100644 --- a/src/Microsoft.AspNet.Mvc.Core/Infrastructure/MvcRouteHandler.cs +++ b/src/Microsoft.AspNet.Mvc.Core/Infrastructure/MvcRouteHandler.cs @@ -59,73 +59,61 @@ namespace Microsoft.AspNet.Mvc.Infrastructure 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 (!newRouteData.Values.ContainsKey(kvp.Key)) + if (!context.RouteData.Values.ContainsKey(kvp.Key)) { - newRouteData.Values.Add(kvp.Key, kvp.Value); + context.RouteData.Values.Add(kvp.Key, kvp.Value); } } } // Removing RouteGroup from RouteValues to simulate the result of conventional routing - newRouteData.Values.Remove(TreeRouter.RouteGroupKey); + context.RouteData.Values.Remove(TreeRouter.RouteGroupKey); + context.Handler = (c) => InvokeActionAsync(c, actionDescriptor); + } + + private async Task InvokeActionAsync(HttpContext httpContext, ActionDescriptor actionDescriptor) + { + var routeData = httpContext.GetRouteData(); try { - context.RouteData = newRouteData; - - _diagnosticSource.BeforeAction(actionDescriptor, context.HttpContext, context.RouteData); + _diagnosticSource.BeforeAction(actionDescriptor, httpContext, routeData); using (_logger.ActionScope(actionDescriptor)) { _logger.ExecutingAction(actionDescriptor); var startTime = Environment.TickCount; - await InvokeActionAsync(context, actionDescriptor); - context.IsHandled = true; + + var actionContext = new ActionContext(httpContext, routeData, actionDescriptor); + if (_actionContextAccessor != null) + { + _actionContextAccessor.ActionContext = actionContext; + } + + var invoker = _actionInvokerFactory.CreateInvoker(actionContext); + if (invoker == null) + { + throw new InvalidOperationException( + Resources.FormatActionInvokerFactory_CouldNotCreateInvoker( + actionDescriptor.DisplayName)); + } + + await invoker.InvokeAsync(); _logger.ExecutedAction(actionDescriptor, startTime); } } finally { - _diagnosticSource.AfterAction(actionDescriptor, context.HttpContext, context.RouteData); - - if (!context.IsHandled) - { - context.RouteData = oldRouteData; - } + _diagnosticSource.AfterAction(actionDescriptor, httpContext, routeData); } } - private Task InvokeActionAsync(RouteContext context, ActionDescriptor actionDescriptor) - { - var actionContext = new ActionContext(context.HttpContext, context.RouteData, actionDescriptor); - - if (_actionContextAccessor != null) - { - _actionContextAccessor.ActionContext = actionContext; - } - - var invoker = _actionInvokerFactory.CreateInvoker(actionContext); - if (invoker == null) - { - throw new InvalidOperationException( - Resources.FormatActionInvokerFactory_CouldNotCreateInvoker( - actionDescriptor.DisplayName)); - } - - return invoker.InvokeAsync(); - } - private void EnsureServices(HttpContext context) { if (_servicesRetrieved) diff --git a/src/Microsoft.AspNet.Mvc.Core/Routing/AttributeRoute.cs b/src/Microsoft.AspNet.Mvc.Core/Routing/AttributeRoute.cs index 7b9c7119f6..45eda4a834 100644 --- a/src/Microsoft.AspNet.Mvc.Core/Routing/AttributeRoute.cs +++ b/src/Microsoft.AspNet.Mvc.Core/Routing/AttributeRoute.cs @@ -282,9 +282,14 @@ namespace Microsoft.AspNet.Mvc.Routing routeInfo.Constraints = constraintBuilder.Build(); - routeInfo.Defaults = routeInfo.ParsedTemplate.Parameters - .Where(p => p.DefaultValue != null) - .ToDictionary(p => p.Name, p => p.DefaultValue, StringComparer.OrdinalIgnoreCase); + routeInfo.Defaults = new RouteValueDictionary(); + foreach (var parameter in routeInfo.ParsedTemplate.Parameters) + { + if (parameter.DefaultValue != null) + { + routeInfo.Defaults.Add(parameter.Name, parameter.DefaultValue); + } + } return routeInfo; } @@ -293,9 +298,9 @@ namespace Microsoft.AspNet.Mvc.Routing { public ActionDescriptor ActionDescriptor { get; set; } - public IReadOnlyDictionary Constraints { get; set; } + public IDictionary Constraints { get; set; } - public IReadOnlyDictionary Defaults { get; set; } + public RouteValueDictionary Defaults { get; set; } public string ErrorMessage { get; set; } diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/Builder/MvcAreaRouteBuilderExtensionsTest.cs b/test/Microsoft.AspNet.Mvc.Core.Test/Builder/MvcAreaRouteBuilderExtensionsTest.cs index 62d181fcfd..153d320488 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/Builder/MvcAreaRouteBuilderExtensionsTest.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/Builder/MvcAreaRouteBuilderExtensionsTest.cs @@ -2,11 +2,9 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; -using System.Collections.Generic; using System.Linq; using Microsoft.AspNet.Routing; using Microsoft.AspNet.Routing.Constraints; -using Microsoft.AspNet.Routing.Template; using Microsoft.Extensions.DependencyInjection; using Moq; using Xunit; @@ -29,7 +27,7 @@ namespace Microsoft.AspNet.Builder builder.MapAreaRoute(name: null, areaName: "admin", template: "site/Admin/"); // Assert - var route = Assert.IsType((Assert.Single(builder.Routes))); + var route = Assert.IsType((Assert.Single(builder.Routes))); Assert.Null(route.Name); Assert.Equal("site/Admin/", route.RouteTemplate); @@ -68,7 +66,7 @@ namespace Microsoft.AspNet.Builder defaults: new { action = "Home" }); // Assert - var route = Assert.IsType((Assert.Single(builder.Routes))); + var route = Assert.IsType((Assert.Single(builder.Routes))); Assert.Equal("admin_area", route.Name); Assert.Equal("site/Admin/", route.RouteTemplate); @@ -113,7 +111,7 @@ namespace Microsoft.AspNet.Builder constraints: new { id = new IntRouteConstraint() }); // Assert - var route = Assert.IsType((Assert.Single(builder.Routes))); + var route = Assert.IsType((Assert.Single(builder.Routes))); Assert.Equal("admin_area", route.Name); Assert.Equal("site/Admin/", route.RouteTemplate); @@ -164,7 +162,7 @@ namespace Microsoft.AspNet.Builder dataTokens: new { some_token = "hello" }); // Assert - var route = Assert.IsType((Assert.Single(builder.Routes))); + var route = Assert.IsType((Assert.Single(builder.Routes))); Assert.Equal("admin_area", route.Name); Assert.Equal("site/Admin/", route.RouteTemplate); @@ -221,7 +219,7 @@ namespace Microsoft.AspNet.Builder dataTokens: new { some_token = "hello" }); // Assert - var route = Assert.IsType((Assert.Single(builder.Routes))); + var route = Assert.IsType((Assert.Single(builder.Routes))); Assert.Equal("admin_area", route.Name); Assert.Equal("site/Admin/", route.RouteTemplate); diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/Infrastructure/MvcRouteHandlerTests.cs b/test/Microsoft.AspNet.Mvc.Core.Test/Infrastructure/MvcRouteHandlerTests.cs index 647dd020e6..41fbc5d41b 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/Infrastructure/MvcRouteHandlerTests.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/Infrastructure/MvcRouteHandlerTests.cs @@ -20,7 +20,7 @@ namespace Microsoft.AspNet.Mvc.Infrastructure public class MvcRouteHandlerTests { [Fact] - public async Task RouteAsync_Success_LogsCorrectValues() + public async Task RouteHandler_Success_LogsCorrectValues() { // Arrange var sink = new TestSink(); @@ -35,9 +35,10 @@ namespace Microsoft.AspNet.Mvc.Infrastructure var context = CreateRouteContext(actionDescriptor: actionDescriptor.Object, loggerFactory: loggerFactory); var handler = new MvcRouteHandler(); + await handler.RouteAsync(context); // Act - await handler.RouteAsync(context); + await context.Handler(context.HttpContext); // Assert Assert.Single(sink.Scopes); @@ -77,10 +78,9 @@ namespace Microsoft.AspNet.Mvc.Infrastructure } [Fact] - public async Task RouteAsync_CreatesNewRouteData() + public async Task RouteHandler_RemovesRouteGroupFromRouteValues() { // Arrange - RouteData actionRouteData = null; var invoker = new Mock(); invoker .Setup(i => i.InvokeAsync()) @@ -91,49 +91,6 @@ namespace Microsoft.AspNet.Mvc.Infrastructure .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_RemovesRouteGroupFromRouteValues() - { - // 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; }); @@ -147,59 +104,14 @@ namespace Microsoft.AspNet.Mvc.Infrastructure await handler.RouteAsync(context); // Assert - Assert.NotSame(originalRouteData, context.RouteData); - Assert.NotSame(originalRouteData, actionRouteData); - Assert.Same(actionRouteData, context.RouteData); + Assert.Same(originalRouteData, context.RouteData); + - // The new routedata is a copy Assert.False(context.RouteData.Values.ContainsKey(TreeRouter.RouteGroupKey)); } [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)); - } - - [Fact] - public async Task RouteAsync_WritesDiagnostic_ActionSelected() + public async Task RouteHandler_WritesDiagnostic_ActionSelected() { // Arrange var listener = new TestDiagnosticListener(); @@ -208,9 +120,10 @@ namespace Microsoft.AspNet.Mvc.Infrastructure context.RouteData.Values.Add("tag", "value"); var handler = new MvcRouteHandler(); + await handler.RouteAsync(context); // Act - await handler.RouteAsync(context); + await context.Handler(context.HttpContext); // Assert Assert.NotNull(listener.BeforeAction?.ActionDescriptor); @@ -224,7 +137,7 @@ namespace Microsoft.AspNet.Mvc.Infrastructure } [Fact] - public async Task RouteAsync_WritesDiagnostic_ActionInvoked() + public async Task RouteHandler_WritesDiagnostic_ActionInvoked() { // Arrange var listener = new TestDiagnosticListener(); @@ -232,9 +145,10 @@ namespace Microsoft.AspNet.Mvc.Infrastructure var context = CreateRouteContext(diagnosticListener: listener); var handler = new MvcRouteHandler(); + await handler.RouteAsync(context); // Act - await handler.RouteAsync(context); + await context.Handler(context.HttpContext); // Assert Assert.NotNull(listener.AfterAction?.ActionDescriptor); @@ -293,22 +207,42 @@ namespace Microsoft.AspNet.Mvc.Infrastructure diagnosticSource.SubscribeWithAdapter(diagnosticListener); } + var routingFeature = new RoutingFeature(); + var httpContext = new Mock(); - httpContext.Setup(h => h.RequestServices.GetService(typeof(IActionContextAccessor))) + httpContext + .Setup(h => h.RequestServices.GetService(typeof(IActionContextAccessor))) .Returns(new ActionContextAccessor()); - httpContext.Setup(h => h.RequestServices.GetService(typeof(IActionSelector))) + httpContext + .Setup(h => h.RequestServices.GetService(typeof(IActionSelector))) .Returns(actionSelector); - httpContext.Setup(h => h.RequestServices.GetService(typeof(IActionInvokerFactory))) + httpContext + .Setup(h => h.RequestServices.GetService(typeof(IActionInvokerFactory))) .Returns(invokerFactory); - httpContext.Setup(h => h.RequestServices.GetService(typeof(ILoggerFactory))) + httpContext + .Setup(h => h.RequestServices.GetService(typeof(ILoggerFactory))) .Returns(loggerFactory); - httpContext.Setup(h => h.RequestServices.GetService(typeof(MvcMarkerService))) - .Returns(new MvcMarkerService()); - httpContext.Setup(h => h.RequestServices.GetService(typeof(IOptions))) - .Returns(optionsAccessor); - httpContext.Setup(h => h.RequestServices.GetService(typeof(DiagnosticSource))) + httpContext + .Setup(h => h.RequestServices.GetService(typeof(MvcMarkerService))) + .Returns(new MvcMarkerService()); + httpContext + .Setup(h => h.RequestServices.GetService(typeof(IOptions))) + .Returns(optionsAccessor); + httpContext + .Setup(h => h.RequestServices.GetService(typeof(DiagnosticSource))) .Returns(diagnosticSource); - return new RouteContext(httpContext.Object); + httpContext + .Setup(h => h.Features[typeof(IRoutingFeature)]) + .Returns(routingFeature); + + var routeContext = new RouteContext(httpContext.Object); + routingFeature.RouteData = routeContext.RouteData; + return routeContext; + } + + private class RoutingFeature : IRoutingFeature + { + public RouteData RouteData { get; set; } } } } \ No newline at end of file diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/Routing/AttributeRouteTest.cs b/test/Microsoft.AspNet.Mvc.Core.Test/Routing/AttributeRouteTest.cs index b128423581..a3f6f0d968 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/Routing/AttributeRouteTest.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/Routing/AttributeRouteTest.cs @@ -19,6 +19,8 @@ namespace Microsoft.AspNet.Mvc.Routing { public class AttributeRouteTest { + private static readonly RequestDelegate NullHandler = (c) => Task.FromResult(0); + // This test verifies that AttributeRoute can respond to changes in the AD collection. It does this // by running a successful request, then removing that action and verifying the next route isn't // successful. @@ -29,7 +31,7 @@ namespace Microsoft.AspNet.Mvc.Routing var handler = new Mock(MockBehavior.Strict); handler .Setup(h => h.RouteAsync(It.IsAny())) - .Callback(c => c.IsHandled = true) + .Callback(c => c.Handler = NullHandler) .Returns(Task.FromResult(true)) .Verifiable(); @@ -85,7 +87,7 @@ namespace Microsoft.AspNet.Mvc.Routing await route.RouteAsync(context); // Assert 1 - Assert.True(context.IsHandled); + Assert.NotNull(context.Handler); Assert.Equal("5", context.RouteData.Values["id"]); Assert.Equal("2", context.RouteData.Values[TreeRouter.RouteGroupKey]); @@ -103,7 +105,7 @@ namespace Microsoft.AspNet.Mvc.Routing await route.RouteAsync(context); // Assert 2 - Assert.False(context.IsHandled); + Assert.Null(context.Handler); Assert.Empty(context.RouteData.Values); handler.Verify(h => h.RouteAsync(It.IsAny()), Times.Once()); diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/Routing/UrlHelperTest.cs b/test/Microsoft.AspNet.Mvc.Core.Test/Routing/UrlHelperTest.cs index adf6ebb1d6..389b22d912 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/Routing/UrlHelperTest.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/Routing/UrlHelperTest.cs @@ -1020,7 +1020,7 @@ namespace Microsoft.AspNet.Mvc.Routing public Task RouteAsync(RouteContext context) { - context.IsHandled = true; + context.Handler = (c) => Task.FromResult(0); return Task.FromResult(false); } } diff --git a/test/Microsoft.AspNet.Mvc.FunctionalTests/RouteDataTest.cs b/test/Microsoft.AspNet.Mvc.FunctionalTests/RouteDataTest.cs index aeda74cdf2..e5354bf9fc 100644 --- a/test/Microsoft.AspNet.Mvc.FunctionalTests/RouteDataTest.cs +++ b/test/Microsoft.AspNet.Mvc.FunctionalTests/RouteDataTest.cs @@ -39,7 +39,7 @@ namespace Microsoft.AspNet.Mvc.FunctionalTests new string[] { typeof(RouteCollection).FullName, - typeof(TemplateRoute).FullName, + typeof(Route).FullName, typeof(MvcRouteHandler).FullName, }, result.Routers); diff --git a/test/WebSites/CustomRouteWebSite/LocalizedRoute.cs b/test/WebSites/CustomRouteWebSite/LocalizedRoute.cs index 82eba00ccc..a85da8b8aa 100644 --- a/test/WebSites/CustomRouteWebSite/LocalizedRoute.cs +++ b/test/WebSites/CustomRouteWebSite/LocalizedRoute.cs @@ -32,32 +32,12 @@ namespace CustomRouteWebSite public async Task RouteAsync(RouteContext context) { - // Saving and restoring the original route data ensures that any values we - // add won't 'leak' if action selection doesn't match. - var oldRouteData = context.RouteData; - - // For diagnostics and link-generation purposes, routing should include - // a list of IRoute instances that lead to the ultimate destination. - // It's the responsibility of each IRouter to add the 'next' before - // calling it. - var newRouteData = new RouteData(oldRouteData); - newRouteData.Routers.Add(_next); + context.RouteData.Routers.Add(_next); var locale = GetLocale(context.HttpContext) ?? "en-US"; - newRouteData.Values.Add("locale", locale); + context.RouteData.Values.Add("locale", locale); - try - { - context.RouteData = newRouteData; - await _next.RouteAsync(context); - } - finally - { - if (!context.IsHandled) - { - context.RouteData = oldRouteData; - } - } + await _next.RouteAsync(context); } private string GetLocale(HttpContext context)