From 2b0bea675ec55dc752aacd47223376045a9be1ab Mon Sep 17 00:00:00 2001 From: jacalvar Date: Fri, 15 Jan 2016 17:05:48 -0800 Subject: [PATCH] [Fixes #3752] Cleanup Controller invocation pipeline * Added a Release method to IControllerActivator * Changed Create in IControllerActivator to take in a ControllerActionContext * Move the check to determine if a controller can be instantiated into the controller activator. * Move logic for disposing controllers into the controller activator and make release on the controller factory delegate into the activator. * Changed release methods to take in a controller context. --- .../Controllers/DefaultControllerActivator.cs | 58 +++++++- .../Controllers/DefaultControllerFactory.cs | 40 +++--- .../Controllers/IControllerActivator.cs | 10 +- .../Controllers/IControllerFactory.cs | 2 +- .../ServiceBasedControllerActivator.cs | 12 +- .../Internal/ControllerActionInvoker.cs | 7 +- .../DefaultControllerActivatorTest.cs | 127 ++++++++++++++++-- .../DefaultControllerFactoryTest.cs | 77 ++--------- .../ServiceBasedControllerActivatorTest.cs | 32 +++-- .../Internal/ControllerActionInvokerTest.cs | 2 +- 10 files changed, 246 insertions(+), 121 deletions(-) diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Controllers/DefaultControllerActivator.cs b/src/Microsoft.AspNetCore.Mvc.Core/Controllers/DefaultControllerActivator.cs index 25493affb4..fc35b6286d 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Controllers/DefaultControllerActivator.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Controllers/DefaultControllerActivator.cs @@ -2,6 +2,9 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; +using System.Reflection; +using Microsoft.AspNetCore.Mvc.Core; +using Microsoft.AspNetCore.Mvc.Infrastructure; using Microsoft.AspNetCore.Mvc.Internal; namespace Microsoft.AspNetCore.Mvc.Controllers @@ -19,23 +22,64 @@ namespace Microsoft.AspNetCore.Mvc.Controllers /// The . public DefaultControllerActivator(ITypeActivatorCache typeActivatorCache) { + if (typeActivatorCache == null) + { + throw new ArgumentNullException(nameof(typeActivatorCache)); + } + _typeActivatorCache = typeActivatorCache; } + /// - public virtual object Create(ActionContext actionContext, Type controllerType) + public virtual object Create(ControllerContext controllerContext) { - if (actionContext == null) + if (controllerContext == null) { - throw new ArgumentNullException(nameof(actionContext)); + throw new ArgumentNullException(nameof(controllerContext)); } - if (controllerType == null) + if (controllerContext.ActionDescriptor == null) { - throw new ArgumentNullException(nameof(controllerType)); + throw new ArgumentException(Resources.FormatPropertyOfTypeCannotBeNull( + nameof(ControllerContext.ActionDescriptor), + nameof(ControllerContext))); } - var serviceProvider = actionContext.HttpContext.RequestServices; - return _typeActivatorCache.CreateInstance(serviceProvider, controllerType); + var controllerTypeInfo = controllerContext.ActionDescriptor.ControllerTypeInfo; + if (controllerTypeInfo.IsValueType || + controllerTypeInfo.IsInterface || + controllerTypeInfo.IsAbstract || + (controllerTypeInfo.IsGenericType && controllerTypeInfo.IsGenericTypeDefinition)) + { + var message = Resources.FormatValueInterfaceAbstractOrOpenGenericTypesCannotBeActivated( + controllerTypeInfo.FullName, + GetType().FullName); + + throw new InvalidOperationException(message); + } + + var serviceProvider = controllerContext.HttpContext.RequestServices; + return _typeActivatorCache.CreateInstance(serviceProvider, controllerTypeInfo.AsType()); + } + + /// + public virtual void Release(ControllerContext context, object controller) + { + if (context == null) + { + throw new ArgumentNullException(nameof(context)); + } + + if (controller == null) + { + throw new ArgumentNullException(nameof(controller)); + } + + var disposable = controller as IDisposable; + if (disposable != null) + { + disposable.Dispose(); + } } } } diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Controllers/DefaultControllerFactory.cs b/src/Microsoft.AspNetCore.Mvc.Core/Controllers/DefaultControllerFactory.cs index 9db1d4ad23..bc1476205e 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Controllers/DefaultControllerFactory.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Controllers/DefaultControllerFactory.cs @@ -31,6 +31,16 @@ namespace Microsoft.AspNetCore.Mvc.Controllers IControllerActivator controllerActivator, IEnumerable propertyActivators) { + if (controllerActivator == null) + { + throw new ArgumentNullException(nameof(controllerActivator)); + } + + if (propertyActivators == null) + { + throw new ArgumentNullException(nameof(propertyActivators)); + } + _controllerActivator = controllerActivator; _propertyActivators = propertyActivators.ToArray(); } @@ -61,20 +71,7 @@ namespace Microsoft.AspNetCore.Mvc.Controllers nameof(ControllerContext))); } - var controllerType = context.ActionDescriptor.ControllerTypeInfo.AsType(); - var controllerTypeInfo = controllerType.GetTypeInfo(); - if (controllerTypeInfo.IsValueType || - controllerTypeInfo.IsInterface || - controllerTypeInfo.IsAbstract || - (controllerTypeInfo.IsGenericType && controllerTypeInfo.IsGenericTypeDefinition)) - { - var message = Resources.FormatValueInterfaceAbstractOrOpenGenericTypesCannotBeActivated( - controllerType.FullName, - GetType().FullName); - throw new InvalidOperationException(message); - } - - var controller = _controllerActivator.Create(context, controllerType); + var controller = _controllerActivator.Create(context); foreach (var propertyActivator in _propertyActivators) { propertyActivator.Activate(context, controller); @@ -84,14 +81,19 @@ namespace Microsoft.AspNetCore.Mvc.Controllers } /// - public virtual void ReleaseController(object controller) + public virtual void ReleaseController(ControllerContext context, object controller) { - var disposableController = controller as IDisposable; - - if (disposableController != null) + if (context == null) { - disposableController.Dispose(); + throw new ArgumentNullException(nameof(context)); } + + if (controller == null) + { + throw new ArgumentNullException(nameof(controller)); + } + + _controllerActivator.Release(context, controller); } } } diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Controllers/IControllerActivator.cs b/src/Microsoft.AspNetCore.Mvc.Core/Controllers/IControllerActivator.cs index efa52d028b..46c7259638 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Controllers/IControllerActivator.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Controllers/IControllerActivator.cs @@ -13,7 +13,13 @@ namespace Microsoft.AspNetCore.Mvc.Controllers /// /// Creates a controller. /// - /// The for the executing action. - object Create(ActionContext context, Type controllerType); + /// The for the executing action. + object Create(ControllerContext context); + + /// + /// Releases a controller. + /// + /// The controller to release. + void Release(ControllerContext context, object controller); } } \ No newline at end of file diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Controllers/IControllerFactory.cs b/src/Microsoft.AspNetCore.Mvc.Core/Controllers/IControllerFactory.cs index e1af5c5497..e4a16086f1 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Controllers/IControllerFactory.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Controllers/IControllerFactory.cs @@ -19,6 +19,6 @@ namespace Microsoft.AspNetCore.Mvc.Controllers /// Releases a controller instance. /// /// The controller. - void ReleaseController(object controller); + void ReleaseController(ControllerContext context, object controller); } } diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Controllers/ServiceBasedControllerActivator.cs b/src/Microsoft.AspNetCore.Mvc.Core/Controllers/ServiceBasedControllerActivator.cs index d4306414b4..663a31daa0 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Controllers/ServiceBasedControllerActivator.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Controllers/ServiceBasedControllerActivator.cs @@ -13,19 +13,21 @@ namespace Microsoft.AspNetCore.Mvc.Controllers public class ServiceBasedControllerActivator : IControllerActivator { /// - public object Create(ActionContext actionContext, Type controllerType) + public object Create(ControllerContext actionContext) { if (actionContext == null) { throw new ArgumentNullException(nameof(actionContext)); } - if (controllerType == null) - { - throw new ArgumentNullException(nameof(controllerType)); - } + var controllerType = actionContext.ActionDescriptor.ControllerTypeInfo.AsType(); return actionContext.HttpContext.RequestServices.GetRequiredService(controllerType); } + + /// + public virtual void Release(ControllerContext context, object controller) + { + } } } diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Internal/ControllerActionInvoker.cs b/src/Microsoft.AspNetCore.Mvc.Core/Internal/ControllerActionInvoker.cs index bf5b418002..242ee715be 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Internal/ControllerActionInvoker.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Internal/ControllerActionInvoker.cs @@ -83,11 +83,16 @@ namespace Microsoft.AspNetCore.Mvc.Internal protected override void ReleaseInstance(object instance) { - _controllerFactory.ReleaseController(instance); + _controllerFactory.ReleaseController(Context, instance); } protected override async Task InvokeActionAsync(ActionExecutingContext actionExecutingContext) { + if (actionExecutingContext == null) + { + throw new ArgumentNullException(nameof(actionExecutingContext)); + } + var actionMethodInfo = _descriptor.MethodInfo; var arguments = ControllerActionExecutor.PrepareArguments( actionExecutingContext.ActionArguments, diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Controllers/DefaultControllerActivatorTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/Controllers/DefaultControllerActivatorTest.cs index dcca781c4a..c82769f6ba 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/Controllers/DefaultControllerActivatorTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/Controllers/DefaultControllerActivatorTest.cs @@ -2,9 +2,13 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; +using System.Reflection; using Microsoft.AspNetCore.Http.Internal; using Microsoft.AspNetCore.Mvc.Abstractions; +using Microsoft.AspNetCore.Mvc.Infrastructure; using Microsoft.AspNetCore.Mvc.Internal; +using Microsoft.AspNetCore.Mvc.ModelBinding; +using Microsoft.AspNetCore.Mvc.ModelBinding.Validation; using Microsoft.AspNetCore.Routing; using Moq; using Xunit; @@ -26,16 +30,79 @@ namespace Microsoft.AspNetCore.Mvc.Controllers { RequestServices = serviceProvider.Object }; - var actionContext = new ActionContext(httpContext, - new RouteData(), - new ActionDescriptor()); + + var context = new ControllerContext( + new ActionContext( + httpContext, + new RouteData(), + new ControllerActionDescriptor + { + ControllerTypeInfo = type.GetTypeInfo() + })); + // Act - var instance = activator.Create(actionContext, type); + var instance = activator.Create(context); // Assert Assert.IsType(type, instance); } + [Fact] + public void Release_DisposesController_IfDisposable() + { + // Arrange + var controller = new MyController(); + var activator = new DefaultControllerActivator(Mock.Of()); + + // Act + activator.Release(new ControllerContext(), controller); + + // Assert + Assert.Equal(true, controller.Disposed); + } + + [Theory] + [InlineData(typeof(int))] + [InlineData(typeof(OpenGenericType<>))] + [InlineData(typeof(AbstractType))] + [InlineData(typeof(InterfaceType))] + public void CreateController_ThrowsIfControllerCannotBeActivated(Type type) + { + // Arrange + var actionDescriptor = new ControllerActionDescriptor + { + ControllerTypeInfo = type.GetTypeInfo() + }; + + var context = new ControllerContext() + { + ActionDescriptor = actionDescriptor, + HttpContext = new DefaultHttpContext() + { + RequestServices = GetServices(), + }, + }; + var factory = new DefaultControllerActivator(new TypeActivatorCache()); + + // Act and Assert + var exception = Assert.Throws(() => factory.Create(context)); + Assert.Equal( + $"The type '{type.FullName}' cannot be activated by '{typeof(DefaultControllerActivator).FullName}' " + + "because it is either a value type, an interface, an abstract class or an open generic type.", + exception.Message); + } + + [Fact] + public void DefaultControllerActivator_ReleasesNonIDisposableController() + { + // Arrange + var activator = new DefaultControllerActivator(Mock.Of()); + var controller = new object(); + + // Act + Assert (does not throw) + activator.Release(Mock.Of(), controller); + } + [Fact] public void Create_TypeActivatesTypesWithServices() { @@ -46,16 +113,23 @@ namespace Microsoft.AspNetCore.Mvc.Controllers serviceProvider.Setup(s => s.GetService(typeof(TestService))) .Returns(testService) .Verifiable(); - + var httpContext = new DefaultHttpContext { RequestServices = serviceProvider.Object }; - var actionContext = new ActionContext(httpContext, - new RouteData(), - new ActionDescriptor()); + + var context = new ControllerContext( + new ActionContext( + httpContext, + new RouteData(), + new ControllerActionDescriptor + { + ControllerTypeInfo = typeof(TypeDerivingFromControllerWithServices).GetTypeInfo() + })); + // Act - var instance = activator.Create(actionContext, typeof(TypeDerivingFromControllerWithServices)); + var instance = activator.Create(context); // Assert var controller = Assert.IsType(instance); @@ -81,6 +155,19 @@ namespace Microsoft.AspNetCore.Mvc.Controllers public TestService TestService { get; } } + private IServiceProvider GetServices() + { + var metadataProvider = new EmptyModelMetadataProvider(); + var services = new Mock(); + services.Setup(s => s.GetService(typeof(IUrlHelper))) + .Returns(Mock.Of()); + services.Setup(s => s.GetService(typeof(IModelMetadataProvider))) + .Returns(metadataProvider); + services.Setup(s => s.GetService(typeof(IObjectModelValidator))) + .Returns(new DefaultObjectValidator(metadataProvider)); + return services.Object; + } + private class PocoType { } @@ -88,5 +175,27 @@ namespace Microsoft.AspNetCore.Mvc.Controllers private class TestService { } + + private class OpenGenericType : Controller + { + } + + private abstract class AbstractType : Controller + { + } + + private interface InterfaceType + { + } + + private class MyController : IDisposable + { + public bool Disposed { get; set; } + + public void Dispose() + { + Disposed = true; + } + } } } diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Controllers/DefaultControllerFactoryTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/Controllers/DefaultControllerFactoryTest.cs index 27698bb17f..29653dd48b 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/Controllers/DefaultControllerFactoryTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/Controllers/DefaultControllerFactoryTest.cs @@ -35,7 +35,7 @@ namespace Microsoft.AspNetCore.Mvc.Controllers }; var activator = new Mock(); - activator.Setup(a => a.Create(context, typeof(MyController))) + activator.Setup(a => a.Create(context)) .Returns(expected) .Verifiable(); @@ -186,61 +186,20 @@ namespace Microsoft.AspNetCore.Mvc.Controllers exception.Message); } - [Theory] - [InlineData(typeof(int))] - [InlineData(typeof(OpenGenericType<>))] - [InlineData(typeof(AbstractType))] - [InlineData(typeof(InterfaceType))] - public void CreateController_ThrowsIfControllerCannotBeActivated(Type type) - { - // Arrange - var actionDescriptor = new ControllerActionDescriptor - { - ControllerTypeInfo = type.GetTypeInfo() - }; - - var context = new ControllerContext() - { - ActionDescriptor = actionDescriptor, - HttpContext = new DefaultHttpContext() - { - RequestServices = GetServices(), - }, - }; - var factory = CreateControllerFactory(new DefaultControllerActivator(new TypeActivatorCache())); - - // Act and Assert - var exception = Assert.Throws(() => factory.CreateController(context)); - Assert.Equal( - $"The type '{type.FullName}' cannot be activated by '{typeof(DefaultControllerFactory).FullName}' " + - "because it is either a value type, an interface, an abstract class or an open generic type.", - exception.Message); - } - [Fact] - public void DefaultControllerFactory_DisposesIDisposableController() + public void DefaultControllerFactory_DelegatesDisposalToControllerActivator() { // Arrange - var factory = CreateControllerFactory(); + var activatorMock = new Mock(); + activatorMock.Setup(s => s.Release(It.IsAny(), It.IsAny())); + + var factory = CreateControllerFactory(activatorMock.Object); var controller = new MyController(); // Act + Assert - Assert.False(controller.Disposed); + factory.ReleaseController(new ControllerContext(), controller); - factory.ReleaseController(controller); - - Assert.True(controller.Disposed); - } - - [Fact] - public void DefaultControllerFactory_ReleasesNonIDisposableController() - { - // Arrange - var factory = CreateControllerFactory(); - var controller = new object(); - - // Act + Assert (does not throw) - factory.ReleaseController(controller); + activatorMock.Verify(); } private IServiceProvider GetServices() @@ -258,7 +217,9 @@ namespace Microsoft.AspNetCore.Mvc.Controllers private static DefaultControllerFactory CreateControllerFactory(IControllerActivator controllerActivator = null) { - controllerActivator = controllerActivator ?? Mock.Of(); + var activatorMock = new Mock(); + + controllerActivator = controllerActivator ?? activatorMock.Object; var propertyActivators = new IControllerPropertyActivator[] { new DefaultControllerPropertyActivator(), @@ -313,21 +274,5 @@ namespace Microsoft.AspNetCore.Mvc.Controllers private class TestService { } - - private class Controller - { - } - - private class OpenGenericType : Controller - { - } - - private abstract class AbstractType : Controller - { - } - - private interface InterfaceType - { - } } } diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Controllers/ServiceBasedControllerActivatorTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/Controllers/ServiceBasedControllerActivatorTest.cs index 5735afc155..3b76de3b9e 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/Controllers/ServiceBasedControllerActivatorTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/Controllers/ServiceBasedControllerActivatorTest.cs @@ -2,6 +2,7 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; +using System.Reflection; using Microsoft.AspNetCore.Http.Internal; using Microsoft.AspNetCore.Mvc.Abstractions; using Microsoft.AspNetCore.Routing; @@ -26,12 +27,16 @@ namespace Microsoft.AspNetCore.Mvc.Controllers RequestServices = serviceProvider.Object }; var activator = new ServiceBasedControllerActivator(); - var actionContext = new ActionContext(httpContext, - new RouteData(), - new ActionDescriptor()); + var context = new ControllerContext(new ActionContext( + httpContext, + new RouteData(), + new ControllerActionDescriptor + { + ControllerTypeInfo = typeof(DIController).GetTypeInfo() + })); // Act - var instance = activator.Create(actionContext, typeof(DIController)); + var instance = activator.Create(context); // Assert Assert.Same(controller, instance); @@ -44,19 +49,26 @@ namespace Microsoft.AspNetCore.Mvc.Controllers // Arrange var expected = "No service for type '" + typeof(DIController) + "' has been registered."; var controller = new DIController(); - var serviceProvider = new Mock(); + var httpContext = new DefaultHttpContext { - RequestServices = serviceProvider.Object + RequestServices = Mock.Of() }; + var activator = new ServiceBasedControllerActivator(); - var actionContext = new ActionContext(httpContext, - new RouteData(), - new ActionDescriptor()); + var context = new ControllerContext( + new ActionContext( + httpContext, + new RouteData(), + new ControllerActionDescriptor + { + ControllerTypeInfo = typeof(DIController).GetTypeInfo() + })); // Act and Assert var ex = Assert.Throws( - () => activator.Create(actionContext, typeof(DIController))); + () => activator.Create(context)); + Assert.Equal(expected, ex.Message); } diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ControllerActionInvokerTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ControllerActionInvokerTest.cs index 6074747ab8..3bd87bd680 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ControllerActionInvokerTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ControllerActionInvokerTest.cs @@ -2208,7 +2208,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal return _controller; } - public void ReleaseController(object controller) + public void ReleaseController(ControllerContext context, object controller) { Assert.NotNull(controller); Assert.Same(_controller, controller);