Skip to content
This repository has been archived by the owner on Dec 14, 2018. It is now read-only.

Commit

Permalink
[Fixes #3752] Cleanup Controller invocation pipeline
Browse files Browse the repository at this point in the history 
* 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.
  • Loading branch information
@javiercn
javiercn committed Jan 26, 2016
1 parent e952009 commit 2b0bea6
Show file tree
Hide file tree
Showing 10 changed files with 245 additions and 120 deletions.
58 changes: 51 additions & 7 deletions src/Microsoft.AspNetCore.Mvc.Core/Controllers/DefaultControllerActivator.cs
View file
Expand Up @@ -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
Expand All @@ -19,23 +22,64 @@ public class DefaultControllerActivator : IControllerActivator
/// <param name="typeActivatorCache">The <see cref="ITypeActivatorCache"/>.</param>
public DefaultControllerActivator(ITypeActivatorCache typeActivatorCache)
{
if (typeActivatorCache == null)
{
throw new ArgumentNullException(nameof(typeActivatorCache));
}

_typeActivatorCache = typeActivatorCache;
}

/// <inheritdoc />
public virtual object Create(ControllerContext controllerContext)
{
if (controllerContext == null)
{
throw new ArgumentNullException(nameof(controllerContext));
}

if (controllerContext.ActionDescriptor == null)
{
throw new ArgumentException(Resources.FormatPropertyOfTypeCannotBeNull(
nameof(ControllerContext.ActionDescriptor),
nameof(ControllerContext)));
}

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<object>(serviceProvider, controllerTypeInfo.AsType());
}

/// <inheritdoc />
public virtual object Create(ActionContext actionContext, Type controllerType)
public virtual void Release(ControllerContext context, object controller)
{
if (actionContext == null)
if (context == null)
{
throw new ArgumentNullException(nameof(actionContext));
throw new ArgumentNullException(nameof(context));
}

if (controllerType == null)
if (controller == null)
{
throw new ArgumentNullException(nameof(controllerType));
throw new ArgumentNullException(nameof(controller));
}

var serviceProvider = actionContext.HttpContext.RequestServices;
return _typeActivatorCache.CreateInstance<object>(serviceProvider, controllerType);
var disposable = controller as IDisposable;
if (disposable != null)
{
disposable.Dispose();
}
}
}
}
38 changes: 20 additions & 18 deletions src/Microsoft.AspNetCore.Mvc.Core/Controllers/DefaultControllerFactory.cs
View file
Expand Up @@ -31,6 +31,16 @@ public class DefaultControllerFactory : IControllerFactory
IControllerActivator controllerActivator,
IEnumerable<IControllerPropertyActivator> propertyActivators)
{
if (controllerActivator == null)
{
throw new ArgumentNullException(nameof(controllerActivator));
}

if (propertyActivators == null)
{
throw new ArgumentNullException(nameof(propertyActivators));
}

_controllerActivator = controllerActivator;
_propertyActivators = propertyActivators.ToArray();
}
Expand Down Expand Up @@ -61,20 +71,7 @@ public virtual object CreateController(ControllerContext context)
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);
Expand All @@ -84,14 +81,19 @@ public virtual object CreateController(ControllerContext context)
}

/// <inheritdoc />
public virtual void ReleaseController(object controller)
public virtual void ReleaseController(ControllerContext context, object controller)
{
var disposableController = controller as IDisposable;
if (context == null)
{
throw new ArgumentNullException(nameof(context));
}

if (disposableController != null)
if (controller == null)
{
disposableController.Dispose();
throw new ArgumentNullException(nameof(controller));
}

_controllerActivator.Release(context, controller);
}
}
}
10 changes: 8 additions & 2 deletions src/Microsoft.AspNetCore.Mvc.Core/Controllers/IControllerActivator.cs
View file
Expand Up @@ -13,7 +13,13 @@ public interface IControllerActivator
/// <summary>
/// Creates a controller.
/// </summary>
/// <param name="context">The <see cref="ActionContext"/> for the executing action.</param>
object Create(ActionContext context, Type controllerType);
/// <param name="context">The <see cref="ControllerContext"/> for the executing action.</param>
object Create(ControllerContext context);

/// <summary>
/// Releases a controller.
/// </summary>
/// <param name="controller">The controller to release.</param>
void Release(ControllerContext context, object controller);
}
}
2 changes: 1 addition & 1 deletion src/Microsoft.AspNetCore.Mvc.Core/Controllers/IControllerFactory.cs
View file
Expand Up @@ -19,6 +19,6 @@ public interface IControllerFactory
/// Releases a controller instance.
/// </summary>
/// <param name="controller">The controller.</param>
void ReleaseController(object controller);
void ReleaseController(ControllerContext context, object controller);
}
}
12 changes: 7 additions & 5 deletions src/Microsoft.AspNetCore.Mvc.Core/Controllers/ServiceBasedControllerActivator.cs
View file
Expand Up @@ -13,19 +13,21 @@ namespace Microsoft.AspNetCore.Mvc.Controllers
public class ServiceBasedControllerActivator : IControllerActivator
{
/// <inheritdoc />
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);
}

/// <inheritdoc />
public virtual void Release(ControllerContext context, object controller)
{
}
}
}
7 changes: 6 additions & 1 deletion src/Microsoft.AspNetCore.Mvc.Core/Internal/ControllerActionInvoker.cs
View file
Expand Up @@ -83,11 +83,16 @@ protected override object CreateInstance()

protected override void ReleaseInstance(object instance)
{
_controllerFactory.ReleaseController(instance);
_controllerFactory.ReleaseController(Context, instance);
}

protected override async Task<IActionResult> InvokeActionAsync(ActionExecutingContext actionExecutingContext)
{
if (actionExecutingContext == null)
{
throw new ArgumentNullException(nameof(actionExecutingContext));
}

var actionMethodInfo = _descriptor.MethodInfo;
var arguments = ControllerActionExecutor.PrepareArguments(
actionExecutingContext.ActionArguments,
Expand Down
127 changes: 118 additions & 9 deletions test/Microsoft.AspNetCore.Mvc.Core.Test/Controllers/DefaultControllerActivatorTest.cs
View file
Expand Up @@ -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;
Expand All @@ -26,16 +30,79 @@ public void Create_CreatesInstancesOfTypes(Type type)
{
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<ITypeActivatorCache>());

// 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<InvalidOperationException>(() => 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<ITypeActivatorCache>());
var controller = new object();

// Act + Assert (does not throw)
activator.Release(Mock.Of<ControllerContext>(), controller);
}

[Fact]
public void Create_TypeActivatesTypesWithServices()
{
Expand All @@ -46,16 +113,23 @@ public void Create_TypeActivatesTypesWithServices()
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<TypeDerivingFromControllerWithServices>(instance);
Expand All @@ -81,12 +155,47 @@ public TypeDerivingFromControllerWithServices(TestService service)
public TestService TestService { get; }
}

private IServiceProvider GetServices()
{
var metadataProvider = new EmptyModelMetadataProvider();
var services = new Mock<IServiceProvider>();
services.Setup(s => s.GetService(typeof(IUrlHelper)))
.Returns(Mock.Of<IUrlHelper>());
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
{
}

private class TestService
{
}

private class OpenGenericType<T> : Controller
{
}

private abstract class AbstractType : Controller
{
}

private interface InterfaceType
{
}

private class MyController : IDisposable
{
public bool Disposed { get; set; }

public void Dispose()
{
Disposed = true;
}
}
}
}

0 comments on commit 2b0bea6

Please sign in to comment.