From d9a3c265eaa13012c5f4d0f68bfe6d8b8fe6e948 Mon Sep 17 00:00:00 2001 From: Ryan Nowak Date: Wed, 12 Nov 2014 13:21:17 -0800 Subject: [PATCH] Fix for MVC #1539 - Deal with the case where request services are not set Rather than throwing here, this does what routing does. If request services aren't set, we just create our own scope. This will NOT create an extra scope if request services are already set. --- .../MvcRouteHandler.cs | 89 +++++++++-------- .../MvcRouteHandlerTests.cs | 97 ++++++++++++++++++- 2 files changed, 142 insertions(+), 44 deletions(-) diff --git a/src/Microsoft.AspNet.Mvc.Core/MvcRouteHandler.cs b/src/Microsoft.AspNet.Mvc.Core/MvcRouteHandler.cs index e432d4736d..2b8e20e7d4 100644 --- a/src/Microsoft.AspNet.Mvc.Core/MvcRouteHandler.cs +++ b/src/Microsoft.AspNet.Mvc.Core/MvcRouteHandler.cs @@ -8,6 +8,7 @@ using Microsoft.AspNet.Http; using Microsoft.AspNet.Mvc.Core; using Microsoft.AspNet.Mvc.Internal; using Microsoft.AspNet.Mvc.Logging; +using Microsoft.AspNet.RequestContainer; using Microsoft.AspNet.Routing; using Microsoft.Framework.DependencyInjection; using Microsoft.Framework.Logging; @@ -32,60 +33,62 @@ 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); - - EnsureLogger(context.HttpContext); - using (_logger.BeginScope("MvcRouteHandler.RouteAsync")) + // This will create a scope if needed. This was likely already been done by routing, but + // we can enforce it just in case we're used in some other scenario. + using (RequestServicesContainer.EnsureRequestServices(context.HttpContext, services: null)) { - var actionSelector = services.GetRequiredService(); - var actionDescriptor = await actionSelector.SelectAsync(context); + var services = context.HttpContext.RequestServices; + Contract.Assert(services != null); - if (actionDescriptor == 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); + + EnsureLogger(context.HttpContext); + using (_logger.BeginScope("MvcRouteHandler.RouteAsync")) { - LogActionSelection(actionSelected: false, actionInvoked: false, handled: context.IsHandled); - return; - } + var actionSelector = services.GetRequiredService(); + var actionDescriptor = await actionSelector.SelectAsync(context); - // 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 (actionDescriptor == null) { - if (!newRouteData.Values.ContainsKey(kvp.Key)) + 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) { - newRouteData.Values.Add(kvp.Key, kvp.Value); + if (!newRouteData.Values.ContainsKey(kvp.Key)) + { + newRouteData.Values.Add(kvp.Key, kvp.Value); + } } } - } - try - { - context.RouteData = newRouteData; - - await InvokeActionAsync(context, actionDescriptor); - context.IsHandled = true; - } - finally - { - if (!context.IsHandled) + try { - context.RouteData = oldRouteData; - } - } + context.RouteData = newRouteData; - LogActionSelection(actionSelected: true, actionInvoked: true, handled: context.IsHandled); + await InvokeActionAsync(context, actionDescriptor); + context.IsHandled = true; + } + finally + { + if (!context.IsHandled) + { + context.RouteData = oldRouteData; + } + } + + LogActionSelection(actionSelected: true, actionInvoked: true, handled: context.IsHandled); + } } } diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/MvcRouteHandlerTests.cs b/test/Microsoft.AspNet.Mvc.Core.Test/MvcRouteHandlerTests.cs index f3be3f42ea..2dfe574d19 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/MvcRouteHandlerTests.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/MvcRouteHandlerTests.cs @@ -2,11 +2,11 @@ // 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.Threading.Tasks; using Microsoft.AspNet.Http; using Microsoft.AspNet.Mvc.Internal; using Microsoft.AspNet.Mvc.Logging; +using Microsoft.AspNet.PipelineCore; using Microsoft.AspNet.Routing; using Microsoft.Framework.DependencyInjection; using Microsoft.Framework.Logging; @@ -248,6 +248,101 @@ namespace Microsoft.AspNet.Mvc Assert.Equal(initialRouter, Assert.Single(actionRouteData.Routers)); } + [Fact] + public async Task RouteAsync_CreatesNewServiceScope() + { + // Arrange + var httpContext = new DefaultHttpContext(); + + var services = new Mock(MockBehavior.Strict); + httpContext.ApplicationServices = services.Object; + + var requestServices = new Mock(MockBehavior.Strict); + requestServices + .Setup(s => s.GetService(It.IsAny())) + .Returns(t => services.Object.GetService(t)); + + var scope = new Mock(MockBehavior.Strict); + scope + .SetupGet(s => s.ServiceProvider) + .Returns(requestServices.Object); + scope + .Setup(s => s.Dispose()) + .Verifiable(); + + var scopeFactory = new Mock(MockBehavior.Strict); + scopeFactory + .Setup(f => f.CreateScope()) + .Returns(scope.Object); + + // Needed by RequestContainer to create a new scope + services + .Setup(s => s.GetService(typeof(IServiceProvider))) + .Returns(services.Object); + services + .Setup(s => s.GetService(typeof(IContextAccessor))) + .Returns(new ContextAccessor()); + services + .Setup(s => s.GetService(typeof(IServiceScopeFactory))) + .Returns(scopeFactory.Object); + + // These services will be resolved from the request container + var action = new Mock(); + + var actionSelector = new Mock(); + actionSelector + .Setup(a => a.SelectAsync(It.IsAny())) + .Returns(Task.FromResult(action.Object)); + requestServices + .Setup(s => s.GetService(typeof(IActionSelector))) + .Returns(actionSelector.Object); + + var invoker = new Mock(); + invoker + .Setup(i => i.InvokeAsync()) + .Returns(Task.FromResult(true)) + .Verifiable(); + + var invokerFactory = new Mock(); + invokerFactory + .Setup(f => f.CreateInvoker(It.IsAny())) + .Returns(invoker.Object); + requestServices + .Setup(s => s.GetService(typeof(IActionInvokerFactory))) + .Returns(invokerFactory.Object); + + requestServices + .Setup(s => s.GetService(typeof(IContextAccessor))) + .Returns(new ContextAccessor()); + + requestServices + .Setup(s => s.GetService(typeof(MvcMarkerService))) + .Returns(new MvcMarkerService()); + + requestServices + .Setup(s => s.GetService(typeof(ILoggerFactory))) + .Returns(NullLoggerFactory.Instance); + + requestServices + .Setup(s => s.GetService(typeof(IOptions))) + .Returns(new MockMvcOptionsAccessor()); + + var context = new RouteContext(httpContext); + var handler = new MvcRouteHandler(); + + // Act + await handler.RouteAsync(context); + + // Assert + // + // If we created the scope it should be disposed + scope.Verify(s => s.Dispose()); + + // We can't directly verify that the httpContext.RequestServices property was set, + // but we can verify that our invoker (only reachable through request services) was used. + invoker.Verify(); + } + private RouteContext CreateRouteContext( IActionSelector actionSelector = null, IActionInvokerFactory invokerFactory = null,