U4-9562 - HealthCheckController doesn't work when using IoC with Unity and WebAPI

Created by simone chiaretta 23 Feb 2017, 16:31:18 Updated by andrei 06 Nov 2017, 21:46:06

Tags: Unscheduled PR

Relates to: U4-10625

Relates to: U4-10642

I installed in my application Unity for WebAPI and registered the controller as recommended on the [https://our.umbraco.org/forum/getting-started/installing-umbraco/53839-Unable-To-Upgrade-When-Using-Unity-IOC-The-type-UmbracoContext-does-not-have-an-accessible-constructor forums]

            container.RegisterType<UmbracoContext>(
                new ContainerControlledLifetimeManager(),
                new InjectionFactory(c => UmbracoContext.Current));

But when then I get an error when I try to load the developer section as the HealthCheckController raises an error.


An error occurred when trying to create a controller of type 'HealthCheckController'. Make sure that the controller has a parameterless public constructor.

EXCEPTION DETAILS

System.InvalidOperationException: An error occurred when trying to create a controller of type 'HealthCheckController'. Make sure that the controller has a parameterless public constructor.

INNER EXCEPTION

Microsoft.Practices.Unity.ResolutionFailedException: Resolution of the dependency failed, type = "Umbraco.Web.HealthCheck.HealthCheckController", name = "(none)". Exception occurred while: while resolving. Exception is: InvalidOperationException - The current type, Umbraco.Web.HealthCheck.IHealthCheckResolver, is an interface and cannot be constructed. Are you missing a type mapping?

At the time of the exception, the container was:

Resolving Umbraco.Web.HealthCheck.HealthCheckController,(none)
Resolving parameter "healthCheckResolver" of constructor
Umbraco.Web.HealthCheck.HealthCheckController(Umbraco.Web.HealthCheck.IHealthCheckResolver healthCheckResolver)
Resolving Umbraco.Web.HealthCheck.IHealthCheckResolver,(none)

Only way to make it work is to force Unity to construct the controller using the parameterless constructor.

container.RegisterType<HealthCheckController>(new InjectionConstructor());

This is kind of weird as the HealthCheck controller is the only one that gives this problem: all others work fine.

I think the difference is that all other controllers use the Services class that is part of the UmbracoApiControllerBase while the HealthCheckController manages dependencies in its own way.

1 Attachments

Comments

Gunnar Már Óttarsson 19 Jul 2017, 21:39:32

I also had to register ContentTypeController MediaController MediaTypeController to get the media section working

but everything seems to be 100% after that


Gunnar Már Óttarsson 29 Jul 2017, 16:17:39

According to the docs here https://our.umbraco.org/documentation/Reference/Common-Pitfalls/ UmbracoContext is request scoped and depends directly on HttpContext.


Shannon Deminick 12 Sep 2017, 00:18:45

If you look into the code you can see the parameter that is required for the HealthCheckController https://github.com/umbraco/Umbraco-CMS/blob/dev-v7/src/Umbraco.Web/HealthCheck/HealthCheckController.cs#L21

So you probably aren't registering a IHealthCheckResolver in your container.

Using the parameterless constructor sort of voids the whole point of IoC ;)


Gunnar Már Óttarsson 12 Sep 2017, 00:24:42

@Shandem I believe it's not registering due to the fact that the HealthCheckResolver class is marked internal.


Shannon Deminick 12 Sep 2017, 01:10:35

Yes that is definitely part of the problem. Not sure how Unity works but seeing as though the error says:

The current type, Umbraco.Web.HealthCheck.IHealthCheckResolver, is an interface and cannot be constructed. Are you missing a type mapping?

It sounds like the container hasn't registered an instance of IHealthCheckResolver, in most containers you need to specify the instance like: Container.Register<IHealthCheckResolver, HealthCheckResolver>(); which if that had been done we probably would have already known that HealthCheckResolver being internal was a problem.

Would be happy to accept a PR to make HealthCheckResolver public which will solve this problem.


Gunnar Már Óttarsson 12 Sep 2017, 08:56:30

I'll take a look this week :)


Shannon Deminick 14 Sep 2017, 05:36:50

Saw your PR and have merged it :) so that will be available in 7.6.7 which (i think) should be released on Tuesday


Shannon Deminick 14 Sep 2017, 05:38:07

I've set to 'fixed' since ultimately that should fix the issue but please test and report back. Cheers!


simone chiaretta 19 Sep 2017, 12:22:50

I think the point here is:

  • as user of a framework, I shouldn't need to wire up dependencies for internals of the framework myself. I should only bother about my own dependencies. Maybe Umbraco should ship a small extension method for each of the main IoC container out there which wires up all the internals. Or come with a IoC container out of the box and then everyone using umbraco have to use that one.


andrei 29 Oct 2017, 17:05:44

Just got into this issue myself, using SimpleInjector IOC. Is the temporary fix registering IHealthCheckResolver, like: Container.Register<IHealthCheckResolver, HealthCheckResolver>()?

[Update] - no, that doesn't work: An error occurred when trying to create a controller of type 'HealthCheckController'. Make sure that the controller has a parameterless public constructor.

The constructor of type HealthCheckResolver contains the parameter with name 'logger' and type ILogger that is not registered. Please ensure ILogger is registered, or change the constructor of HealthCheckResolver.

I'm in version 7.7.1, any ideas?


Shannon Deminick 30 Oct 2017, 03:54:53

@amv Your error message says you need register an instance of ILogger in your container, so you should register one.


andrei 30 Oct 2017, 09:23:24

Yes, I would have solved by registering ILogger, but there is a second parameter to HealthCheckResolver constructor which I didn't know how to provide.


Shannon Deminick 30 Oct 2017, 09:38:07

@amv right, you can see the construction of the HealthCheckResolver in the codebase here: https://github.com/umbraco/Umbraco-CMS/blob/e0025db56d52b770d2b3aedbd48a3b804fd15ef0/src/Umbraco.Web/WebBootManager.cs so you could use that same code as a delegate to register the instance.


Shannon Deminick 30 Oct 2017, 09:39:07

@simonech yes i agree it's something that has been discussed for a long time and some community members had planned on making different Nuget packages for just that purpose but nothing has transpired yet.


andrei 30 Oct 2017, 20:43:38

So, just gave it another shot. Looking at source code, but also with reflector, trying to do the same, I end up trying something like this: container.Register(()=> new HealthCheckResolver(LoggerResolver.Current.Logger, () => PluginManager.Current.ResolveTypes(true, null)), Lifestyle.Scoped); Here I try to use PluginManager.Current, because I don't have the property PluginManager at hand as WebBootManager has. Unfortunately PluginManager.Current is null, so I guess I need way to get a the PluginManager. Following the source, I can see PluginManager being initialized in CoreBootManager (base of WebBootManager) like: PluginManager = new PluginManager(ServiceProvider, ApplicationCache.RuntimeCache, ProfilingLogger); PluginManager.Current = PluginManager; So, I will need now parameters for PluginManager constructor. However the first param ServiceProvider is of type ActivatorServiceProvider which is an internal class. I guess the rabbit hole ends here :).. I could try to make my own ActivatorServiceProvider from IServiceProvider, but the smell is killing me already..


Shannon Deminick 31 Oct 2017, 00:31:11

@aamv where/when are you creating your container? You should be creating your container on ApplicationStarted, see https://our.umbraco.org/documentation/reference/using-ioc . At this stage, PluginManager.Current will exist. Also note that when registering IHealthCheckResolver as a delegate it won't be resolved until it's needed so it seems mighty strange that your container is trying to resolve an instance before PlugingManager.Current is inititalized. You should definitely not try re-creating a new instance of PluginManager.

What you've done here is on the right path but this shouldn't be scoped, this should be a singleton and you don't need the optional parameters for ResolveTypes.

container.Register<IHealthCheckResolver>(()=> new HealthCheckResolver(LoggerResolver.Current.Logger, () => PluginManager.Current.ResolveTypes<HealthCheck>(true, null)), Lifestyle.Scoped);


andrei 31 Oct 2017, 18:58:43

Hi @Shandem, you're right, I was creating my container in ApplicationStarting. So the good news is that after moving it to ApplicationStarted, the IOC was able to initialize a HealthCheckRezolver and subsequently a HealthCheckController instance. The bad news is that HealthCheckController only seems to work when initialized with the parameter-less constructor. The second constructor that takes a IHealthCheckResolver parameter, will not initialize some private variables (healthCheckConfig and disableCheckIds) and will throw a null reference when GetAllHealthChecks() gets called in Developer section. I guess this is a bug?

Anyway, I fixed it by making my IOC to use the parameter-less constructor for HealthCheckController type. That also makes the IOC registration for IHealthCheckResolver no longer necessary.


Shannon Deminick 31 Oct 2017, 23:37:49

@amv So i realized that i was wrong for registering a IHealthCheckResolver in the container, all you need to do for that is :

container.Register<IHealthCheckResolver>(()=> HealthCheckResolver.Current);


andrei 01 Nov 2017, 21:37:46

@Shandem that looks much cleaner. However, no reason to register it with this version (7.7.1) - as described in my previous comment, HealthCheckController only works if created with default constructor, otherwise it will throw null reference when GetAllHealthChecks() gets hit. I think this is a bug - HealthCheckController second constructor 'forgets' to initialize same variables.


Shannon Deminick 02 Nov 2017, 03:28:08

@amv ouch! yes it does, i will fix up asap.


andrei 05 Nov 2017, 20:58:35

Just hit another problematic controller - LegacyTreeController. It happens under members section when trying to expand Member groups or trying to create a new member group:

SimpleInjector.ActivationException: The constructor of type LegacyTreeController contains the parameter with name >'xmlTreeNode' and type XmlTreeNode that is not registered. Please ensure XmlTreeNode is registered, or change the constructor of LegacyTreeController. (screenshot attached)

I believe the controller was intended to be used with empty constructor when automatically activated. However, the IOC will complain as it doesn't know that.

Perhaps second constructor should be internal, not public? It is used only from another internal method anyway.


Priority: Normal

Type: Bug

State: Fixed

Assignee:

Difficulty: Normal

Category:

Backwards Compatible: True

Fix Submitted:

Affected versions:

Due in version: 7.6.7

Sprint: Sprint 67

Story Points:

Cycle: