U4-11427 - V8 Dependency Injection Implementation

Created by James South 08 Jun 2018, 16:36:42 Updated by Stephan 04 Jul 2018, 08:07:13

I've been reading through the source for V8 and there's something really worrying me about the DI implementation.

You've actually referenced specific attribute types for the concrete container implementation within the core library!

Think about it. You've put together an API of interchangeable parts and then tightly coupled that entire API to a specific container. Doesn't that strike you a odd?

If this is due to specific requirements such as property injection then the class design is simply wrong. Property injection is designed as means of last resort for classes that you do not control.

Using it in your own codebase is a very strong indicator of design smell which can cause serious issues as it leads to Temporal Coupling1 (i.e you now have a temporal dependency, one class has to be initialized before another).

I cannot recommend strongly enough that you alter your approach now before it is too late.

I recommend the following:

  • Delete all instances of property injection and refactor.
  • Use constructor injection or abstract factories throughout.
  • Ditch the {{Current}} Service Locator 2
  • Use the the abstractions available in Microsoft.Extensions.DependencyInjection 3 to create your {{IUmbracoBuilder}} or similar API.
  • Then you can use whatever container you want 4

Here's some example code 5 from David Fowler demonstrating setting up the container from MS.

2 Attachments

Comments

Lars-Erik Aabech 08 Jun 2018, 17:35:27

Hear hear!


Stephan 08 Jun 2018, 18:13:43

Yes. And Yes. And we kinda thought about it ;-)

For instance, Current is clearly a service locator, but it's the only way to start refactoring without doing everything at once. It temporarily replaces the bazillions of singletons that live in v7. Would be way nicer not to have it, but then we'd end up having to refactor everything at once. Including some very old and ugly legacy code.

Eventually, the picture is going to be way cleaner - cleaner enough to kill Current. Or abstract the container. Etc.

But, time.


Mads Krohn 08 Jun 2018, 22:15:05

Totally get both sides, just so happy we can have these kinds of conversations, keep up the awesome work everybody :)


Shannon Deminick 09 Jun 2018, 06:14:18

All valid points and most have been considered. There are definitely reasons for decisions being made ;)

First thing to remember is that v8 isn't the last version of Umbraco ever made. We will release more versions after than and a .NET Core version will be another major release too. We don't have to solve all of the worlds problems in a single release and each release will get better.

Concrete container

I don't think this is an issue, there's no reason why using a specific container for DI is a bad thing. In .NET Core you can replace the default container with your own if you require more than basic container functionality: https://docs.microsoft.com/en-us/aspnet/core/fundamentals/dependency-injection?view=aspnetcore-2.1#replacing-the-default-services-container. We are not limiting people from choosing their own container, this is just the container Umbraco bits internally will use and the plan is to allow other containers to wrap ours, just like the .NET core approach by exposing an IServiceProvider (see link). And when moving to .NET Core this approach can still be used, the ConfigureServices block will allow developers to register whatever they want in the .NET Core container, there will be a call to wrap our container and it can also potential wrap multiple contains if the developer chooses to use their own implementation.

If this is due to specific requirements such as property injection then the class design is simply wrong. Property injection is designed as means of last resort for classes that you do not control.

Umbraco itself doesn't rely on property injection. I realize people hate this concept and there's generally not a reason for it. BUT there is a reason we're using it and it's to simplify base class usages for developers. Not every Umbraco developer cares about DI and some won't even know what it is. Some developers want to create an implementation of UmbracoApiController and then just access the property Services and it will work. By allowing an empty ctor, all of the base class properties can be wired up automatically with property injection and this will simply be an easier experience for non technical devs. Developers who care and use DI can create their own constructors to inject whatever they want and use our non empty base class ctor's to wire everything up. AFAIK there's no property injection taking place that is required for Umbraco to work, it's just allowing for syntactical sugar for this scenario. If a developer chooses to use a different container like Autofac, that will still work but Autofac will be in charge of creating controllers and therefore the developer would be required to use ctor injection - which we'd assume they would use anyways since they clearly know about and care about DI.

Service locator

Yes this is a religious debate and i don't think anyone argues that it's a good thing. However, it's a necessary evil in plenty of cases:

  • Attribute classes - you cannot DI these and in fact .NET Core uses the service locator pattern in plenty of places for stuff like this, they also expose the locator pattern (container) directly on things like HttpContext and some other classes.
  • We are still in the world of webforms: aspx, ascx classes only as of a few months ago support DI but we're not going to spend our time wiring this legacy stuff up and things like httphandlers, httpmodules, asmx, etc... don't support DI at all and developers will need a way to access services
  • Extension method syntactical sugar - it's possible to have extension methods not use a service locator by passing in all required arguments but in some cases having easy to use APIs wins. Whether devs choose to use simpler methods that are not testable or more verbose methods that are testable will be up to them.

Of course making use of this should be a last resort and we'd prefer that devs don't use Current at all which is even true today, but people will continue to use it and if we don't expose one, they'll create their own which will inevitably have worse side affects.

Microsoft.Extensions.DependencyInjection

I'm all for try this out for v8 and we can even wrap the container with LightInject to keep the functionality we currently have, but time is the problem right now. If anyone wants to POC this we can go from there!


Lars-Erik Aabech 09 Jun 2018, 09:20:57

I just had a look, and TBH it's worse than I thought. In fact, it's horrible!

There isn't that much to do to make it a bit less coupled. I really don't mind using a service locator, but only to provide a parameterless constructor for those who don't care about DI. That removes the need for property injection all together. However, tightly coupling a concrete third-party service locator to everything, including implementor's assemblies is just evil. How many lines of code and how many minutes does it take to abstract away LightInject? It's a matter of one interface and one concrete, which is referenced once in the boot manager. It should even be implemented one layer down from Umbraco.Core.

I suggest rewriting anything that uses the inject attribute to provide a service locating ctor and a full injectable ctor. It is a bit boring, but should be done within hours.


Shannon Deminick 09 Jun 2018, 09:33:44

I don't know what is meant by

However, tightly coupling a concrete third-party service locator to everything

I'm referring to service locator by our 'Current' singleton, if there's some other pattern used I'm not sure about it.

Fair enough on the property injector side of things, I only know of its use for controllers that devs can inherit from, if there's other required uses we'll have to ask Stephane. But yup for the controller side of things we can just 'inject' the required services from and empty ctor to the proper one with services from 'Current'


Lars-Erik Aabech 09 Jun 2018, 09:45:40

Ref. props: Cool.

What I mean by tightly coupling a third-party locator is that the IServiceContainer and ServiceContainer types from LightInject are used all over. The whole point of dependency inversion and using a container is to invert dependencies, not add another. :D Besides, it's stupid that it's named IServiceContainer. It can inject more than services. As mentioned, it's not that much work to create an abstract IContainer in Core and then in the web assembly assign an adapter dependent on LightInject - from for instance Umbraco.LightInject.dll.


Lars-Erik Aabech 09 Jun 2018, 09:54:25

This is like 121-123 lines too many mentioning LightInject:

{cut Search results for "LightInject"}

\Umbraco-CMS V8\src\Umbraco.Web_Legacy\Actions\ActionCollectionBuilder.cs(4):using LightInject; \Umbraco-CMS V8\src\Umbraco.Web\Cache\CacheRefresherComponent.cs(23):using LightInject; \Umbraco-CMS V8\src\Umbraco.Web\Components\DatabaseServerRegistrarAndMessengerComponent.cs(6):using LightInject; \Umbraco-CMS V8\src\Umbraco.Web\Composing\CompositionRoots\InstallerCompositionRoot.cs(1):using LightInject; \Umbraco-CMS V8\src\Umbraco.Web\Composing\CompositionRoots\WebMappingProfilesCompositionRoot.cs(1):using LightInject; \Umbraco-CMS V8\src\Umbraco.Web\Composing\Current.cs(4):using LightInject; \Umbraco-CMS V8\src\Umbraco.Web\CompositionExtensions.cs(2):using LightInject; \Umbraco-CMS V8\src\Umbraco.Web\Editors\BackOfficeController.cs(12):using LightInject; \Umbraco-CMS V8\src\Umbraco.Web\Editors\DataTypeValidateAttribute.cs(8):using LightInject; \Umbraco-CMS V8\src\Umbraco.Web\Editors\DataTypeValidateAttribute.cs(24): // LightInject can inject dependencies into properties \Umbraco-CMS V8\src\Umbraco.Web\Editors\EditorValidatorCollectionBuilder.cs(1):using LightInject; \Umbraco-CMS V8\src\Umbraco.Web\Editors\TourController.cs(5):using LightInject; \Umbraco-CMS V8\src\Umbraco.Web\HealthCheck\HealthCheckNotificationMethodCollectionBuilder.cs(1):using LightInject; \Umbraco-CMS V8\src\Umbraco.Web\HealthCheck\HeathCheckCollectionBuilder.cs(1):using LightInject; \Umbraco-CMS V8\src\Umbraco.Web\Install\InstallHelper.cs(9):using LightInject; \Umbraco-CMS V8\src\Umbraco.Web\LightInjectExtensions.cs(4):using LightInject; \Umbraco-CMS V8\src\Umbraco.Web\LightInjectExtensions.cs(10): internal static class LightInjectExtensions \Umbraco-CMS V8\src\Umbraco.Web\Mvc\EnsurePublishedContentRequestAttribute.cs(6):using LightInject; \Umbraco-CMS V8\src\Umbraco.Web\Mvc\FilteredControllerFactoryCollectionBuilder.cs(1):using LightInject; \Umbraco-CMS V8\src\Umbraco.Web\Mvc\PluginController.cs(4):using LightInject; \Umbraco-CMS V8\src\Umbraco.Web\Mvc\RenderRouteHandler.cs(15):using LightInject; \Umbraco-CMS V8\src\Umbraco.Web\Mvc\UmbracoController.cs(4):using LightInject; \Umbraco-CMS V8\src\Umbraco.Web\Mvc\UmbracoViewPageOfTModel.cs(6):using LightInject; \Umbraco-CMS V8\src\Umbraco.Web\Mvc\UmbracoVirtualNodeRouteHandler.cs(7):using LightInject; \Umbraco-CMS V8\src\Umbraco.Web\PublishedCache\XmlPublishedCache\XmlCacheComponent.cs(9):using LightInject; \Umbraco-CMS V8\src\Umbraco.Web\Routing\ContentFinderCollectionBuilder.cs(1):using LightInject; \Umbraco-CMS V8\src\Umbraco.Web\Routing\UrlProviderCollectionBuilder.cs(1):using LightInject; \Umbraco-CMS V8\src\Umbraco.Web\Routing\UrlProviderExtensions.cs(7):using LightInject; \Umbraco-CMS V8\src\Umbraco.Web\Runtime\WebRuntime.cs(3):using LightInject; \Umbraco-CMS V8\src\Umbraco.Web\Runtime\WebRuntime.cs(49): if (!(container.ScopeManagerProvider is MixedLightInjectScopeManagerProvider smp)) \Umbraco-CMS V8\src\Umbraco.Web\Runtime\WebRuntime.cs(50): throw new Exception("Container.ScopeManagerProvider is not MixedLightInjectScopeManagerProvider."); \Umbraco-CMS V8\src\Umbraco.Web\Runtime\WebRuntimeComponent.cs(16):using LightInject; \Umbraco-CMS V8\src\Umbraco.Web\Runtime\WebRuntimeComponent.cs(115): // IoC setup for LightInject for MVC/WebApi \Umbraco-CMS V8\src\Umbraco.Web\Runtime\WebRuntimeComponent.cs(116): // see comments on MixedLightInjectScopeManagerProvider for explainations of what we are doing here \Umbraco-CMS V8\src\Umbraco.Web\Runtime\WebRuntimeComponent.cs(117): if (!(composition.Container.ScopeManagerProvider is MixedLightInjectScopeManagerProvider smp)) \Umbraco-CMS V8\src\Umbraco.Web\Runtime\WebRuntimeComponent.cs(118): throw new Exception("Container.ScopeManagerProvider is not MixedLightInjectScopeManagerProvider."); \Umbraco-CMS V8\src\Umbraco.Web\Scheduling\BackgroundTaskRunner.cs(9):using LightInject; \Umbraco-CMS V8\src\Umbraco.Web\Search\SearchableTreeCollectionBuilder.cs(1):using LightInject; \Umbraco-CMS V8\src\Umbraco.Web\Security\MembershipHelper.cs(8):using LightInject; \Umbraco-CMS V8\src\Umbraco.Web\SignalR\PreviewHubComponent.cs(2):using LightInject; \Umbraco-CMS V8\src\Umbraco.Web\Templates\TemplateRenderer.cs(15):using LightInject; \Umbraco-CMS V8\src\Umbraco.Web\Tour\TourFilterCollectionBuilder.cs(5):using LightInject; \Umbraco-CMS V8\src\Umbraco.Web\UmbracoModule.cs(8):using LightInject; \Umbraco-CMS V8\src\Umbraco.Web\WebApi\Filters\FeatureAuthorizeAttribute.cs(3):using LightInject; \Umbraco-CMS V8\src\Umbraco.Web\WebApi\UmbracoApiControllerBase.cs(4):using LightInject; \Umbraco-CMS V8\src\Umbraco.Core_Legacy\PackageActions\PackageActionCollectionBuilder.cs(1):using LightInject; \Umbraco-CMS V8\src\Umbraco.Core\Cache\CacheRefresherCollectionBuilder.cs(2):using LightInject; \Umbraco-CMS V8\src\Umbraco.Core\Components\BootLoader.cs(6):using LightInject; \Umbraco-CMS V8\src\Umbraco.Core\Components\Composition.cs(1):using LightInject; \Umbraco-CMS V8\src\Umbraco.Core\Components\CompositionExtensions.cs(3):using LightInject; \Umbraco-CMS V8\src\Umbraco.Core\Composing\CollectionBuilderBase.cs(5):using LightInject; \Umbraco-CMS V8\src\Umbraco.Core\Composing\CompositionRoots\ConfigurationCompositionRoot.cs(1):using LightInject; \Umbraco-CMS V8\src\Umbraco.Core\Composing\CompositionRoots\CoreMappingProfilesCompositionRoot.cs(1):using LightInject; \Umbraco-CMS V8\src\Umbraco.Core\Composing\CompositionRoots\RepositoryCompositionRoot.cs(2):using LightInject; \Umbraco-CMS V8\src\Umbraco.Core\Composing\CompositionRoots\ServicesCompositionRoot.cs(4):using LightInject; \Umbraco-CMS V8\src\Umbraco.Core\Composing\Current.cs(2):using LightInject; \Umbraco-CMS V8\src\Umbraco.Core\Composing\Current.cs(23): /// This class is initialized with the container via LightInjectExtensions.ConfigureUmbracoCore, \Umbraco-CMS V8\src\Umbraco.Core\Composing\LazyCollectionBuilderBase.cs(4):using LightInject; \Umbraco-CMS V8\src\Umbraco.Core\Composing\LightInjectExtensions.cs(5):using LightInject; \Umbraco-CMS V8\src\Umbraco.Core\Composing\LightInjectExtensions.cs(11): /// Provides extensions to LightInject. \Umbraco-CMS V8\src\Umbraco.Core\Composing\LightInjectExtensions.cs(13): public static class LightInjectExtensions \Umbraco-CMS V8\src\Umbraco.Core\Composing\LightInjectExtensions.cs(26): // from the docs: "LightInject considers all read/write properties a dependency, but implements \Umbraco-CMS V8\src\Umbraco.Core\Composing\LightInjectExtensions.cs(34): // could not find it documented, but tests & code review shows that LightInject considers a \Umbraco-CMS V8\src\Umbraco.Core\Composing\LightInjectExtensions.cs(43): // see notes in MixedLightInjectScopeManagerProvider \Umbraco-CMS V8\src\Umbraco.Core\Composing\LightInjectExtensions.cs(44): container.ScopeManagerProvider = new MixedLightInjectScopeManagerProvider(); \Umbraco-CMS V8\src\Umbraco.Core\Composing\LightInjectExtensions.cs(191): // see https://github.com/seesharper/LightInject/issues/133 \Umbraco-CMS V8\src\Umbraco.Core\Composing\LightInjectExtensions.cs(195): // optimized in LightInject) \Umbraco-CMS V8\src\Umbraco.Core\Composing\LightInjectExtensions.cs(291): LightInjectException.TryThrow(e); \Umbraco-CMS V8\src\Umbraco.Core\Composing\LightInjectExtensions.cs(310): // fixme temp - STOP doing this, it confuses LightInject and then we get ugly exception traces \Umbraco-CMS V8\src\Umbraco.Core\Composing\LightInjectExtensions.cs(311): // we HAVE to let LightInject throw - and catch at THE OUTERMOST if InvalidOperationException in LightInject.Anything! \Umbraco-CMS V8\src\Umbraco.Core\Composing\LightInjectExtensions.cs(320): // LightInjectException.TryThrow(e, implementingType); \Umbraco-CMS V8\src\Umbraco.Core\Composing\LightInjectExtensions.cs(338): //Current.Logger.Debug(typeof(LightInjectExtensions), $"Fallback for type ."); \Umbraco-CMS V8\src\Umbraco.Core\Composing\LightInjectExtensions.cs(339): // https://github.com/seesharper/LightInject/issues/173 \Umbraco-CMS V8\src\Umbraco.Core\Composing\LightInjectExtensions.cs(360): //Current.Logger.Debug(typeof(LightInjectExtensions), $"Fallback for type ."); \Umbraco-CMS V8\src\Umbraco.Core\Composing\LightInjectExtensions.cs(361): // https://github.com/seesharper/LightInject/issues/173 \Umbraco-CMS V8\src\Umbraco.Core\Composing\MixedLightInjectScopeManagerProvider.cs(1):using LightInject; \Umbraco-CMS V8\src\Umbraco.Core\Composing\MixedLightInjectScopeManagerProvider.cs(2):using LightInject.Web; \Umbraco-CMS V8\src\Umbraco.Core\Composing\MixedLightInjectScopeManagerProvider.cs(20): public class MixedLightInjectScopeManagerProvider : IScopeManagerProvider \Umbraco-CMS V8\src\Umbraco.Core\Composing\MixedLightInjectScopeManagerProvider.cs(24): public MixedLightInjectScopeManagerProvider() \Umbraco-CMS V8\src\Umbraco.Core\Composing\OrderedCollectionBuilderBase.cs(3):using LightInject; \Umbraco-CMS V8\src\Umbraco.Core\Composing\WeightedCollectionBuilderBase.cs(4):using LightInject; \Umbraco-CMS V8\src\Umbraco.Core\Exceptions\LightInjectException.cs(9): /// Represents errors that occur due to LightInject. \Umbraco-CMS V8\src\Umbraco.Core\Exceptions\LightInjectException.cs(11): public class LightInjectException : Exception \Umbraco-CMS V8\src\Umbraco.Core\Exceptions\LightInjectException.cs(13): public LightInjectException(string message) \Umbraco-CMS V8\src\Umbraco.Core\Exceptions\LightInjectException.cs(17): public LightInjectException(string message, Exception innerException) \Umbraco-CMS V8\src\Umbraco.Core\Exceptions\LightInjectException.cs(21): private const string LightInjectUnableToResolveType = "Unable to resolve type:"; \Umbraco-CMS V8\src\Umbraco.Core\Exceptions\LightInjectException.cs(22): private const string LightInjectUnresolvedDependency = "Unresolved dependency "; \Umbraco-CMS V8\src\Umbraco.Core\Exceptions\LightInjectException.cs(23): private const string LightInjectRequestedDependency = "[Requested dependency:"; \Umbraco-CMS V8\src\Umbraco.Core\Exceptions\LightInjectException.cs(28): if (ex == null || ex.Message.StartsWith(LightInjectUnableToResolveType) == false) \Umbraco-CMS V8\src\Umbraco.Core\Exceptions\LightInjectException.cs(32): sb.AppendLine("Unresolved type: " + ex.Message.Substring(LightInjectUnableToResolveType.Length)); \Umbraco-CMS V8\src\Umbraco.Core\Exceptions\LightInjectException.cs(34): throw new LightInjectException(sb.ToString(), e); \Umbraco-CMS V8\src\Umbraco.Core\Exceptions\LightInjectException.cs(40): if (ex == null || ex.Message.StartsWith(LightInjectUnableToResolveType) == false) \Umbraco-CMS V8\src\Umbraco.Core\Exceptions\LightInjectException.cs(44): sb.AppendLine("Unresolved type: " + ex.Message.Substring(LightInjectUnableToResolveType.Length)); \Umbraco-CMS V8\src\Umbraco.Core\Exceptions\LightInjectException.cs(47): throw new LightInjectException(sb.ToString(), e); \Umbraco-CMS V8\src\Umbraco.Core\Exceptions\LightInjectException.cs(57): if (message.StartsWith(LightInjectUnableToResolveType)) \Umbraco-CMS V8\src\Umbraco.Core\Exceptions\LightInjectException.cs(59): sb.AppendLine("-> Unresolved type: " + message.Substring(LightInjectUnableToResolveType.Length)); \Umbraco-CMS V8\src\Umbraco.Core\Exceptions\LightInjectException.cs(61): else if (message.StartsWith(LightInjectUnresolvedDependency)) \Umbraco-CMS V8\src\Umbraco.Core\Exceptions\LightInjectException.cs(63): var pos = message.InvariantIndexOf(LightInjectRequestedDependency); \Umbraco-CMS V8\src\Umbraco.Core\Exceptions\LightInjectException.cs(64): sb.AppendLine("-> Unresolved dependency: " + message.Substring(pos + LightInjectRequestedDependency.Length + 1).TrimEnd(']')); \Umbraco-CMS V8\src\Umbraco.Core\IO\MediaFileSystem.cs(9):using LightInject; \Umbraco-CMS V8\src\Umbraco.Core\IRuntime.cs(1):using LightInject; \Umbraco-CMS V8\src\Umbraco.Core\Migrations\MigrationBuilder.cs(2):using LightInject; \Umbraco-CMS V8\src\Umbraco.Core\Migrations\MigrationBuilder.cs(17): // see https://github.com/seesharper/LightInject/issues/294 \Umbraco-CMS V8\src\Umbraco.Core\Migrations\MigrationBuilder.cs(29): // LightInject .Create() is a shortcut for .Register() + .GetInstance() \Umbraco-CMS V8\src\Umbraco.Core\Migrations\PostMigrationCollectionBuilder.cs(1):using LightInject; \Umbraco-CMS V8\src\Umbraco.Core\Persistence\Mappers\MapperCollectionBuilder.cs(1):using LightInject; \Umbraco-CMS V8\src\Umbraco.Core\Persistence\Repositories\Implement\AuditRepository.cs(4):using LightInject; \Umbraco-CMS V8\src\Umbraco.Core\Persistence\Repositories\Implement\PartialViewMacroRepository.cs(1):using LightInject; \Umbraco-CMS V8\src\Umbraco.Core\Persistence\Repositories\Implement\PartialViewRepository.cs(5):using LightInject; \Umbraco-CMS V8\src\Umbraco.Core\Persistence\Repositories\Implement\RelationRepository.cs(4):using LightInject; \Umbraco-CMS V8\src\Umbraco.Core\Persistence\Repositories\Implement\ScriptRepository.cs(5):using LightInject; \Umbraco-CMS V8\src\Umbraco.Core\Persistence\Repositories\Implement\StylesheetRepository.cs(4):using LightInject; \Umbraco-CMS V8\src\Umbraco.Core\Persistence\Repositories\Implement\TaskRepository.cs(4):using LightInject; \Umbraco-CMS V8\src\Umbraco.Core\Persistence\Repositories\Implement\TaskTypeRepository.cs(4):using LightInject; \Umbraco-CMS V8\src\Umbraco.Core\Persistence\Repositories\Implement\TemplateRepository.cs(6):using LightInject; \Umbraco-CMS V8\src\Umbraco.Core\Persistence\UmbracoDatabaseFactory.cs(53): /// Used by LightInject. \Umbraco-CMS V8\src\Umbraco.Core\PropertyEditors\DataEditorCollectionBuilder.cs(1):using LightInject; \Umbraco-CMS V8\src\Umbraco.Core\PropertyEditors\ManifestValueValidatorCollectionBuilder.cs(1):using LightInject; \Umbraco-CMS V8\src\Umbraco.Core\PropertyEditors\PropertyValueConverterCollectionBuilder.cs(1):using LightInject; \Umbraco-CMS V8\src\Umbraco.Core\Runtime\CoreRuntime.cs(6):using LightInject; \Umbraco-CMS V8\src\Umbraco.Core\Runtime\CoreRuntimeComponent.cs(6):using LightInject; \Umbraco-CMS V8\src\Umbraco.Core\Services\ServiceContext.cs(42): /// Used by IoC. Note that LightInject will favor lazy args when picking a constructor. \Umbraco-CMS V8\src\Umbraco.Core\Strings\UrlSegmentProviderCollectionBuilder.cs(1):using LightInject; \Umbraco-CMS V8\src\Umbraco.Core\UmbracoApplicationBase.cs(7):using LightInject;


James South 09 Jun 2018, 15:21:03

Shannon, I'm not sure you're fully understanding the issues I'm raising here.

Concrete Container

I don't think this is an issue, there's no reason why using a specific container for DI is a bad thing. In .NET >Core you can replace the default container with your own if you require more than basic container functionality: >https://docs.microsoft.com/en-us/aspnet/core/fundamentals/dependency-injection?view=aspnetcore-2.1#replacing-the->default-services-container.

The behaviour you've referenced here is precisely what I'm saying you should do and what Umbraco isn't doing.

None of your classes should be using a concrete implementation, but they are. For example, {{MediaFileService}} for example uses the {{InjectAttribute}}1 for a couple of properties. This is a concrete implementation. You cannot replace this injection with another container.

The API itself should have no awareness of any container, only the builder class should have awareness of the abstraction.

I actually linked to the list of available containers that have implemented the Microsoft dependency-injection abstractions in my original issue so there's no additional work required there.

Service locator

Yes this is a religious debate and i don't think anyone argues that it's a good thing. However, it's a necessary evil in plenty of cases:

  • Attribute Classes - Do they? I'm not seeing that in the code. There's an {{FromServicesAttribute}} class that allows getting the request services but I don't think that's the same. Strictly speaking, yes, we cannot use dependency injection to inject a dependency into an attribute but attributes are for metadata not behavior. The AttributeSpecification2 encourages this by forbidding reference types as arguments. *HttpModules - These can be injected. You need to create a container HttpModule initialized via startup attributes that will dispose of them though. This would use a service locator but it's not exposed in public code. Phil Haacked wrote a blog post (some of the code is out of date but the principle is sound) years ago demonstrating how3.
  • Extension method syntactic sugar - This feels like code smell to me; extension methods should be coupled to the type only and not any external service. Any requirements should be passed as a parameter. (On that note, someone remind me to create an issue regarding the navigation methods on {{IPublishedContent}}, you'll need them to become a service at some point)

I don't think there much else that cannot be implemented via abstract factories. However, I would try to refactor your API to avoid these situations.

Not every Umbraco developer cares about DI and some won't even know what it is. Some developers want to create >an implementation of UmbracoApiController and then just access the property Services and it will work.

Not every developer might care about dependency injection but you are using it in Umbraco. They're going to simply have to accept the fact.

DI is all-in, a hybrid approach is a broken one. If you are concerned about developers consuming constructors with multiple required parameters wrap the services in some sort of {{IServiceAggregator}} (Though I would limit what's contained in this to prevent misuse) so it's a single parameter. They have to assign the instance field but that shouldn't be a concern, they are, after all, developers. If you do this, developers won't create their own version.

First thing to remember is that v8 isn't the last version of Umbraco ever made. We will release more versions >after than and a .NET Core version will be another major release too. We don't have to solve all of the worlds >problems in a single release and each release will get better.

V8 isn't the last no, but it's your last opportunity to make breaking changes before an eventual migration to Core with V9. These are fundamental architectural changes I'm talking about which you do need to get right now.

You want that migration to Core and V9 to go quickly and smoothly to make up for lost time developing V8 (2+ years by my count) doing DI right will allow that.

I know I'm sounding like the harbinger of doom but I'm raising this as I care about each and every one of the people working on and consuming this project. The Umbraco community has been a great friend to me and I want to do what I can to help out.


Lars-Erik Aabech 10 Jun 2018, 08:01:37

I'll see about trying this in V8 as well, but a quick unit test of MS' abstractions in Framework 4.6.1 shows the gist of it:

// NO LIGHTINJECT using System; using Microsoft.Extensions.DependencyInjection; using NUnit.Framework;

namespace AbstractInjectionTests
{
    [TestFixture]
    public class Can_Mention_Lightinject_Only_Once
    {
        private IServiceProvider _serviceProvider;

        [SetUp]
        public void Setup()
        {
            var services = new ServiceCollection();
            RegisterServices(services);
            _serviceProvider = CreateProvider(services);
        }

        private IServiceProvider CreateProvider(IServiceCollection services)
        {
            // THIS IS THE ONLY (!!) PLACE WE REFERENCE LIGHTINJECT, AND SHOULD BE REPLACABLE
            var container = new LightInject.ServiceContainer();
            var provider = LightInject.Microsoft.DependencyInjection.DependencyInjectionContainerExtensions.CreateServiceProvider(container, services);
            return provider;
        }

        private void RegisterServices(IServiceCollection services)
        {
            services.AddTransient<IThing, Thing>();
        }

        [Test]
        public void And_Resolve_Services()
        {
            var thing = _serviceProvider.GetService<IThing>();
            Assert.That(thing, Is.InstanceOf<Thing>());
        }
    }

    public interface IThing
    {

    }

    public class Thing : IThing
    {

    }
}

I put everything in the same assembly for the test, but of course the LightInject dependency should be plugged in.

{code}

I copied and did not modify this example implementation for LightInject: https://github.com/seesharper/LightInject.Microsoft.DependencyInjection/blob/master/src/LightInject.Microsoft.DependencyInjection/LightInject.Microsoft.DependencyInjection.cs

Will report back when I (hopefully) have had a chance to give it a go on a temp8 fork.


Kevin Giszewski 10 Jun 2018, 11:46:54

I think this is all summed up with @JimbobSquarePants => "DI is all-in, a hybrid approach is a broken one."

And I fully agree with @lars-erik with the container references. Only the composition root should know about the container. A class should be blissfully ignorant that DI is being used. Constructor injection all-the-things.

Ambient contexts are very tricky in DI. Any chance of removing .Current or simply removing the non-DI singletons?


Lars-Erik Aabech 10 Jun 2018, 16:03:48

Started a PR: https://github.com/umbraco/Umbraco-CMS/pull/2683/commits/d7ed5d775e6e6fec6534e69fc0b3226bef0b072d

After digging in I realize why you say it'll take time. It's like it's rigorously welded to everything in the core. So it's a bit more of a task than I thought, but it also looks like it'll be possible to do search/replace soon. A few things really tie hard into LI, so they might have to stay coupled for a while, but at least the main bits can go through abstractions only. One important task is to split registration from initialization. Hence both the container and services on the composition base for now.

The property injection is a small thing that could be done first, but I would be really sad to see this super tight coupling spread any more into the source.


Lars-Erik Aabech 10 Jun 2018, 19:18:17

Short status. I think we need to discuss and agree a bit before I run further.

I would've added an Umbraco specific abstraction around everything DI if I'd just jumped in. @JimbobSquarePants' suggestion to use MS' abstractions is a better idea given there's complete implementations for most containers already. It'll also be pretty future-proof.

The problem as far as I understand right now lies mostly in that registration and resolving is kind of intermingled during startup. In my opinion ALL registration should be done in order before the container is being used. I think it should be possible to extract stuff, but I don't know all usages yet.

Again - this is if we agree it's a good direction to continue in. If HQ lacks time, I'm still fairly motivated to keep hacking at it, but I need some insights into some of the more tangled parts. See comment on last commit. :) https://github.com/umbraco/Umbraco-CMS/pull/2683/commits/5461ee7ad9e006e0c3465e0ab1183b36949166bb#diff-8744d1e558c316cbd4aaac3afab40349L226


Stephan 10 Jun 2018, 23:13:15

Not going to repeat myself. So:

Abstracting the usage of LightInject. I'm all for it. Yes, it it should rely on what MS has already done. I'll review the current state of the PR later tomorrow. If you can help us get it right and simple, that would be amazing. Actually working on a PR is the best way to help.

Properties injection. If it can be done without creating a monster, I'm all for it.

Service locator. The ''content.Children()'' extension method. We have to think about the average dev creating websites. We want to be easy, simple, friendly. It should be possible to inject a service and do ''service.GetChildren(content)'' -- if you want the pure way. But if DI is not your thing, you just want to do ''content.Children()'' and that extension method is not going away. We can discuss things again and again.

One practical thing can (and should) be done: ensure that Core only uses the service locator when no other solution would work (eg, the extension methods). There are plenty of places where we're still using it because, v7.

Registration and resolving during startup. They are intermingled, as DI is used to discover and boot components, which in turn can modify registrations. Only a very small amount of things should be resolved during that phase, ie before "everything" is registered. It indeed kinda hurt when I did it, so I don't need to be convinced it's weird. But it's the best I could do / that was simple enough. Only way to help is to figure out and propose something that would be simpler, and cleaner - in that order.


Lars-Erik Aabech 11 Jun 2018, 07:25:49

Cool! I'll keep going. Hope you can forgive my ranty way to continue James' constructive approach here. Nothing personal. :)

I'd also like to ephasize that I don't really have a problem with Current.X or Container.Resolve. However, it should only be used when absolutely necessary, and pref. only in "bastard constructors". I think we can keep them for now, and they're somewhat necessary for small/noob sites. Can stop peeps from not learning DI.

So the action points for now wrt. OP would be something like

  • Delete all instances of property injection and refactor.
    • After abstracting
  • Use constructor injection or abstract factories throughout.
    • Abstract factory = Abstracted service locator, right? So... Current.
  • Ditch the Current Service Locator [2]
    • See above
  • Use the the abstractions available in Microsoft.Extensions.DependencyInjection [3] to create your IUmbracoBuilder or similar API.
    • Comes from refactoring point 1

To prevent temporal coupling from being a problem, there could be a unit test, boot step, whatever to verify that all required services are there. The challenge is figuring out the right way to mark them. Attributes from Umbraco.Core could be an alternative. Having support for unit tests with different granularities would help setting up dependencies in tests. Of course it's still an anti-pattern to mind, but we can only go so far. Then again, as long as all concretes have ctors exposing all dependencies we should be good. If peeps manage to mess up the config while only using parameterless, they would hopefully notice fairly quick.

Reg. DI in general. I'm gonna go out on a limb and pretend I know this shit. There are generally two solutions for Dependency ''Inversion'': Dependency Injection and Inversion of Control. Dependency Injection is preferably done with ctor injection. Inversion of Control is when you ask for instances, IE. using a "ServiceLocator" or a "Container". Both are proved and verified patterns. The latter comes with some luggage. IE. temporal coupling. Which means you have more shit to mind. Doesn't mean you absolutely shouldn't do it. It's like the agile manifesto, "we prefer x over y". But the bottom line is that it's all to invert dependencies. Which we'll do by abstracting away everything including the DI container.

I think to start with, it'd make sense to also abstract MS' abstractions a bit. It might be necessary to allow for the mingled registration and resolving until we're all done refactoring. The "Umbraco layer" would keep adding to ServiceContainer and ServiceCollection after the IServiceProvider has been created. Over time it could possibly be refactored into a "boot" container and a "runtime" container, or just keep old fashioned singletons until the container is about to be created. They could be created by abstract factories at bootup and then put in the container during registration.


Stephan 11 Jun 2018, 08:25:08

Cool! I'll keep going. Hope you can forgive my ranty way to continue James' constructive approach here. Nothing personal. :)

No prob. I can take some it's ''Completely Broken™'' ;-)

I'd also like to ephasize that I don't really have a problem with Current.X or Container.Resolve. However, it should only be used when absolutely necessary, and pref. only in "bastard constructors". I think we can keep them for now, and they're somewhat necessary for small/noob sites. Can stop peeps from not learning DI.

This. Totally this.

Liking the action points - no time to go into details right now.


Lars-Erik Aabech 11 Jun 2018, 08:31:11

\o/


Pete Duncanson 11 Jun 2018, 10:56:15

Wow, that was a hell of a read. Just to throw some balance into the mix here to even out all this awesome hardcore geekery.

Umrbaco is "the developers CMS" and that is a broad spectrum of skills sets. Web developer, hardcore developers, junior developers all are welcome and catered for within the walls of Umbraco. Umbraco has the following it has because it is relatively easy to pick and solve problems with quickly and go deeper if you require. DI can still be quick IF you have the many days/weeks/months of prior experience of using it day in day out then yes you can be super quick and tested and all that goodness.

However the target audience don't all have the luxury of that prior knowledge and practise to make using DI a quick and pain free experience and in those cases DI can have the reverse effect; it becomes a very slow solution to not really fix your problems which can lead to what I call "oh screw this...I'm using Word Press!".

I'm sure that on reading that there might be a response welling up inside you that "if they don't like it they can use something else" (heard that before) but why should "they"? If we are talking about DI deep in the core where only the brave will venture then do what you like but when that leaks out to the realms of mere mortals that is where you will cause issues. You'll have won the DI battle but will have lost the war so to speak. I'm not saying we should make everything suit the lowest skill level ever, just that we should be mindful of our own levels of developer comprehension before wading into this, its a multi-faceted issue that needs some thought and empathy.

When you talk about adding "a ''custom abstraction'' over the ''MS abstraction'' for the ''container framework'' which is ''abstracting'' away the need for a ''concrete singleton''"...I had to giggle.

The following line summed up what we need to do best and its not even technical:

"simpler, and cleaner - in that order"

I would love it if you clever developers taking up this challenge use this as a guiding light for all levels of developers out there.


Lars-Erik Aabech 11 Jun 2018, 11:20:20

Not to worry, @peteduncanson. What we're talking about is a simpler and cleaner API, there's just so many things to mind to get there. You will be able to not know there's DI under the hood. Probably even for "service registration". You can copy an application event handler from the docs, change your one-line registration based on some guidance and off you go.


Kevin Giszewski 11 Jun 2018, 12:06:53

@peteduncanson We'll never settle the "simplicity" perception. One developer's "simple" is another's "complex". Microsoft has made DI fundamental to .net core just like Google made it fundamental to AngularJs.

Good DI is a simple solution to a complex problem (dependency management). I don't think anyone is trying to make it more complex, in fact the opposite is true.

"Simple and cleaner" is what DI does with respect to dependencies. We all carry the baggage of our "current knowledge bias" and use that prove a point.

DI has a set of "rules" to follow however retrofitting a repo is a monumental task and I say we cheer them on.

We all can look at Umbraco and say it has pushed our own skills up. DI shouldn't be regarded as a superfluous skill. Perhaps we should rejoice that it will be a great place to learn it.


Jason Prothero 11 Jun 2018, 15:45:58

I love the discussion!

I've been around long enough that I've seen too many well architected APIs/implementations/products come out as slow, hard to use, and overly complicated. Hell, I've built plenty myself along the way.

I second Pete's message for simple and easy to learn and add another:

  • Make it fast and scale well

I'm all for using DI so that it could actually allow for easier unit testing (we are doing this right?). All I'm asking is to keep those goals in mind (performance and simple API) so that we don't end up somewhere we didn't mean to get. Also, please keep an open mind when people who aren't up on all the DI patterns express that "it's hard". Remember that you won't get to sit in the room with the next person trying to learn Umbraco and explain why it actually makes total sense. Yes, we are trying to make a popular CMS, not a niche CMS so there may be some compromises to make things easy to learn and a joy to work with for the most people possible.

#h5yr all for taking the time to review the v8 code, not be afraid to ask questions, and for actually proposing some real potential solutions so we all know what you're talking about!


Kevin Giszewski 11 Jun 2018, 16:22:42

I think I'll be in the minority regarding DI. Unit testing and DI are strong bedfellows in my humble opinion. I also know that the average dev does not care about DI or unit testing (sadly). I suspect the resistance to DI will be lack of use or bad prior experiences -- the "it works now, so why change it?" mentality. I also fear dev's will blame "DI complexity" if v8 has issues or that DI supporters are 'idealists'.

I like James' courage to open this issue as the DI topic is loaded with opinion -- both implementation style and whether or not it should be even used.

I also think we should guard against "well that's hard for me" as we would have never adopted MVVM on the frontend thinking that way. Angular isn't exactly easy but brings huge benefits and learning materials are available.

I simply would like DI so that unit testing is not such a bear in Umbraco.

Change is uncomfortable. My 2 cents from the other side.


Lars-Erik Aabech 11 Jun 2018, 16:28:42

The thing is that todays implementation in V8 is so intermingled with other concerns that I'm guessing not even the author(s) know everything that's been done. It is also tightly coupled to LightInject for more or less all complex scenarios one can use it for. Both these concerns mean that it's super hard to learn and use when contributing to Umbraco.

By doing what's proposed (Bear with me: by abstracting MS' abstractions and refactoring all the LightInject couplings…) we will pave the way for a more Umbraco-domain-specific API. Over time we will also get rid of anti-patterns in use at the moment. Some will stay in, but will hopefully be better managed and documented.

At least by baking an Umbraco-specific API around it is should be easier to arrive at "fast, scalable, maintainable and explainable" over time.

All of the above is for HQ, contributors and people who want to extend what's handled by DI.

For "laymen", as discussed, the current options to not provide a constructor in your SurfaceController or WhatNotController and accessing all services through properties or a singleton accessor will stay in.

Tech gibberish warning: Beneath the surface though, there will be DI ctors you ''could'' override and parameterless ctors using IoC to call the other one.


Ronald Barendse 11 Jun 2018, 17:23:01

Maybe check the DI implementation of Orchard Core: https://orchardcore.readthedocs.io/en/latest/OrchardCore/OrchardCore.Modules/README/? It seems they already faced (and fixed) this problem, including how it can work together with ASP.NET Core...

Regarding the services in controllers: have a single interface (IUmbracoServicesAccessor) and implementation that has all services as properties (injected via ctor DI). Inheriting the controller would than only require a ctor with a single parameter and call the base class ctor (that assigns it to the Services property). Something like https://docs.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.http.ihttpcontextaccessor.


Lars-Erik Aabech 11 Jun 2018, 19:56:38

The extensions they made for ServiceCollection in Orchard looks nice, @rgmbarendse. I think it's even better to remove that dependency as well, though. For now I think we'll need to keep the registration available for some time after initialization of the container, and that would probably work better with another layer. The mingled registration/resolving will possibly disqualify half of the containers. If we figure out a way to avoid that we might get off without another abstraction. But there's still a matter of whether MS' abstractions will live forever and whether we can make an even more durable solution.

What I'm not a fan of is the use of markers in Orchard. I like having all dependencies referenced and copied to my output folder on build. I also like the auto discovery we have today with ApplicationEventHandler although it could possibly be named better. There should be some kind of fluent builder API when we go to core. While still on Framework we could use Owin, but I think that's a case for a different issue. The boot sequence today is (AFAIK) still pretty tight coupled to the traditional IIS / ASP.NET pipeline. Hopefully we can make it as similar as possible.

Reg. services, they already have a parameter object / grouping / accessor abstraction called IServices. It's defo. better to inject that than individual services when you need several. What is it? Three parameters max? :)


Shannon Deminick 12 Jun 2018, 03:05:10

So the action points for now Delete all instances of property injection and refactor. After abstracting. Use constructor injection or abstract factories throughout. Abstract factory = Abstracted service locator, right? So... Current.

Makes sense and if that's all possible and doesn't require huge amounts of refactoring, then go for it :)

Ditch the Current Service Locator [2]

For now Current will need to remain and like i said there will be necessary evils when using this service locator pattern. We are not in ASP.Net Core land yet, we still have to contend with webforms bits, old school config provider based objects, httpmodules, httphandlers, asmx files and Attributes which in ASP.Net MVC land are not just metadata. Validation, model binding, authorization, etc... in most cases are all based on Attributes and even in .NET Core the service locator pattern is used for this.

Use the the abstractions available in Microsoft.Extensions.DependencyInjection [3] to create your IUmbracoBuilder or similar API.

So this is where I will have to partially disagree (sorry for the novel but some important notes here ;)

A long time ago we created a fully abstracted DI container in a version called v5. It was an ugly mess because we were trying to abstract out all functionality from all DI containers which is just not possible since all DI containers have some features not available in others. Microsoft have built their own slim container and have put a small abstraction on it which is great because it's simple and it doesn't try to abstract all DI container functionality away but instead, when you bring your own container type to .NET Core like (https://docs.microsoft.com/en-us/aspnet/core/fundamentals/dependency-injection?view=aspnetcore-2.1#replacing-the-default-services-container) you are 'wrapping' their container which is a far more clever solution because the abstraction doesn't have to take into account container specific features and still allows the end user to use container specific features. When we move to .NET Core, with the current LightInject coupling we still wouldn't have to change anything because .NET Core allows us to bring our own container with it's own feature set and wrap the built in MS one.

Similarly for v8 we wanted to do the same thing... though the functionality isn't implemented yet. We use LightInject as our container of choice and yes we are tightly coupled to it and it's features/functionality, but we want to allow the end user to bring their own container and be able to wrap our container. The point here is that our internal usage of a container we've chosen shouldn't affect the end user. At this time we also don't plan to be able to just swap out the container type we've chosen with another one. The amount of extra work involved to abstract out LightInject internally seems to have very little immediate benefit. In the grand scheme of things, there's few places where LightInject is used referenced and by fixing up things you'll see in the codebase (i.e. // fixme inject) will do which will reduce the direct uses of LightInject.

We have a new ApplicationEventHandler in v8 which is called IUmbracoComponent and currently in here you can modify the LightInject container. This is where I will agree with you that we should use an abstraction since we would not necessarily like to expose LightInject directly to the end user. This is also where the functionality could exist for a user to bring their own container and wrap ours. Since we are not in the .NET Core world, this would also be the place where ASP.NET MVC and WebApi service locators would be modified if the end user wanted to replace things like DependencyResolver.SetResolver (we would have already set this to the LightInject container by default)

Action points?

Based on the above, IMO these are the things that should be prioritized

#Fix all places where things should be injected, most are already marked with //fixme inject (or similar) #Reduce the property injection where possible #Update the implementation of IUmbracoComponent to allow for developers to modify our container or bring their own container while not directly referencing LightInject

This will get us to the minimum place where we want to go in v8 and will reduce the amount of coupling to Light Inject. After that if there's time:

#decouple LightInject entirely internally in Umbraco if you think that's important #tweak the DI implementation to not resolve anything before all registrations are done, but that might be more painful than it seems for this version

Remember v8 isn't complete, there's still functionality to implement, there's still a lot of cleanup to do, we still have legacy things to contend, and it isn't going to be the final version we'll release. We can incrementally work towards making this better and better.

I do enjoy that we've gone from "There should be DI/IoC in Umbraco" to "The DI/IoC in Umbraco should be abstracted", that is actually pretty good progress ;)


James South 12 Jun 2018, 07:12:33

I kinda feel like I'm banging my head against a brick wall a little here. :(

The issue I originally raised. (I strongly encourage everyone to read it again) was that you have tightly coupled your DI API to a container as a way to manage the use of various anti-patterns throughout the API and that you should be using abstractions to wire up your container. Nothing has changed there.

Microsoft and the community at large have made a great solution for flexible DI with very little work for the end user (As demonstrated by the linked examples). It's the product of years of work, is a great standardization and simply works.

Wanna switch out a container with that API? Reference the Nuget package, add the appropriate (very simple) code and you're done.

Wanna with the current and planned codebase in v8? Largely ignore an entire industry of expertise and do something entirely different.

Similarly for v8 we wanted to do the same thing... though the functionality isn't implemented yet. We use LightInject as our container of choice and yes we are tightly coupled to it and it's features/functionality, but we want to allow the end user to bring their own container and be able to wrap our container. The point here is that our internal usage of a container we've chosen shouldn't affect the end user. At this time we also don't plan to be able to just swap out the container type we've chosen with another one. The amount of extra work involved to abstract out LightInject internally seems to have very little immediate benefit. In the grand scheme of things, there's few places where LightInject is used referenced and by fixing up things you'll see in the codebase (i.e. // fixme inject ) will do which will reduce the direct uses of LightInject.

Absolutely it affects the end user. The container shouldn't be an implementation detail in your general classes and you have property resolution that will simply, silently fail if they switch out containers(Unless of course you are planning on running multiple containers side-by-side?). And of course it's of great benefit! You end up with a system that is API compatible with ASP.NET Core and you have to do very little when it (finally) comes time to migrate over. Why would you reinvent the wheel?

No-one is asking you to switch out LightInject, we are asking you decouple your API and to use the abstractions provided by MS so that you can, if you then want to continue to use LightInject and also allow others the power to easily use the container of their choice.

Regarding {{Current}}. I still regard this as a horrible implementation adding no more benefit to the end developer than an injected service. Using a ServiceLocator in environments where you have no choice is of course perfectly acceptable (as long as you are only resolving only what you need, when you need it) but that simply isn't one of them. With {{Current}} you will be resolving a number of unnecessary services.

Regarding developer ease-of-use. Are we really saying that using a field is going to make development so much more difficult? The code sample below is super simple and is backed by compiler rules. The IDE will even generate the constructor for you (without Resharper).

// Your base public class RenderMvcController {

public RenderMvcController (IServices services) {
    this.Services = services;
}

public IServices Services {get;}

}

// Their consumer public class HomeController : RenderMvcController {

public HomeController (IServices services)
    base:(services)
{
}

public override ActionResult Index (RenderModel model) {
    var x = this.Services....
}

}

I appreciate that you have already invested time in your current API and you loath to make the required changes but I would strongly advise that you do. You'll end up with a better, simpler, much more testable API that should also bring about performance improvements and future time saved.

Making these changes will also make to codebase more familiar to any developers who have invested any time in ASP.NET Core which will increase the probability of contribution.

Now I have to be painfully honest here, I often struggle to follow the V7 codebase when I read it due to some of the more interesting patterns I see in there (plus of course the total lack of any meaningful intellisense docs) and I'd like to think I have a fairly high level of technical expertise. V8 in some areas is more so now as a result of these decisions. It puts me off contributing, and it will others also. (Whether you guys care about that I don't know).


Lars-Erik Aabech 12 Jun 2018, 08:03:14

Last two are tl;dr. for me atm. Full follow-up tonight. ;)

I'm inclined to agree with @Shandem about the priorities, but I'm totally not in on the "wrapping of tightly coupled containers" (ref. @JimbobSquarePants's last post) . ;)


Shannon Deminick 12 Jun 2018, 08:49:34

@JimbobSquarePants I'm happy to be a very small brick wall for you to bang your head against :P

For the record, I'm not arguing that any points raised here shouldn't be done. I'm trying to prioritise the work involved and explain some choices being made, discussion is healthy lets continue.

The container shouldn't be an implementation detail in your general classes and you have property resolution that will simply, silently fail if they switch out containers (Unless of course you are planning on running multiple containers side-by-side?)

Yes that is what we originally were proposing. The container in Umbraco would not be replaced, it would exist internally and people wouldn't need to know its ever being used but people could of course use it if they wanted. Or if they wanted to bring their own container they would configure it as they felt necessary and we'd expose an IServiceProvider to wrap our container so they wouldn't have to re-register everything.

So if there's enough time and enthusiasm to get to the stage of fully decoupling LightInject and get the .NET Core DI implementation ready for v8 than I'm all for it, but we'd need community help to achieve that for a v8 release.


Lars-Erik Aabech 12 Jun 2018, 09:20:44

Or if they wanted to bring their own container they would configure it as they felt necessary and we'd expose an IServiceProvider to wrap our container so they wouldn't have to re-register everything.

Unless I'm completely missing something, this will fail massively. Say I have the following:

public class MyFancyThing { public MyFancyThing(Umbraco.Core.IServices services, My.Core.IRepository repo) }

Which of the containers will create this artifact? Would I have to register the repo implementation in both? Will I have to register IServices in both? Which comes first? I just can't wrap my head around it. Either you go with LightInject, or you replace it entirely.

IF you want to swap it out, there will be core registrations you need to replace because you're using special features from LightInject. Yours can still be tightly coupled in your Umbraco.LightInject.dll, but activated as abstract somthings. But the core logic must use only abstractions. When I say abstract the container, I don't mean wrap all LightInject features, but rather expose the container to those concrete implementations through some Object property on the main container abstraction.

Also, I'm pretty confident the "use your own container with MS' abstractions" method does not wrap the concrete ServiceContainer from MS. You're implementing a new concrete for the IServiceProvider contract. I used ServiceCollection in the PR and it comes from MS' concrete implementation. But we don't use ServiceProvider at all. The best approach would be to create an Umbraco-specific IServiceCollection and drop the concrete MS DI reference as well. I should actually have minded not doing the same mistake as we're trying to avoid. :)


Stephan 12 Jun 2018, 09:43:55

The issue I originally raised. (I strongly encourage everyone to read it again) was that you have tightly coupled your DI API to a container as a way to manage the use of various anti-patterns throughout the API and that you should be using abstractions to wire up your container. Nothing has changed there.

Yes, totally.

Putting that container in place while dealing with the Umbraco subtleties (such as using the container to instanciate components that ppl can use to tweak composition of other things, because that's a simple enough way to do it) and not rewriting Umbraco entirely, was a crazy enough task.

But it smells. There is not much of a wall. I am convinced.

And then, the plan is to ship v8 at some point, and there are only a finite amount of hours per day, and Shan and I currently are working on other parts of v8. What we have today is the best we can have today.

Of course it can change, only someone has to change it. The right to call something CompletelyBroken™ comes with the responsibility to fix it. I'll review @lars-erik PR sometime this week and see how I can help you help us :-)


Lars-Erik Aabech 12 Jun 2018, 10:16:45

@zpqrtbnk Do look at the PR, but in its current state it's just about getting a feel for where MS' abstractions aren't enough. IE. what you guys use specific LI features for, and how that will have to be abstracted. Dunno if it's better to keep concrete implementation discussions there or if that should continue here.


Lars-Erik Aabech 12 Jun 2018, 21:14:11

Look ma'! No property injection on the IHttpModule! :) (Even abusing the tightly coupled container)

Imitated this: https://haacked.com/archive/2011/06/03/dependency-injection-with-asp-net-httpmodules.aspx/

Only registered UmbracoModule for now, but should also make it possible to register custom modules during bootup like in Haack's article.

Code in separate branch for now: https://github.com/lars-erik/Umbraco-CMS/blob/temp8-u4-11427-remove-propertyinjection/src/Umbraco.Web/UmbracoModule.cs

(Oh, and don't tell Stephane, but the PublishedRouter is now public)


Shannon Deminick 13 Jun 2018, 01:16:46

@lars-erik Ideally any work done for this could be submitted as smaller PRs since we can pull them in much faster and make comments there, so if you wanted to have a PR just for removing property injection that would be ace! (we should also remove the public setters from properties when that is fixed since they shouldn't be mutable)

Which of the containers will create this artifact? Would I have to register the repo implementation in both? Will I have to register IServices in both? Which comes first? I just can't wrap my head around it. Either you go with LightInject, or you replace it entirely.

The idea is that Umbraco has a container, let's call it "A". And if the user wants to have their own container, let's call that "B". Since this user isn't using the Umbraco container, they will not modify the Umbraco container. Since Umbraco only needs to resolve things from container "A", internally any Umbraco service is created with container "A". In your example MyFancyThing is only used by the user (not Umbraco) and would only be registered by the user in container "B", so container "B" is the thing always creating this object. For MVC and WebApi controllers, this is done by the user by setting the MVC/WebApi DependencyResolvers with their own container. Since it would be annoying to have to re-register all Umbraco services in container "B", there's 2 things that could be done which depend on container "B"s features: 1) If the container supports an event handler for when a service cannot be found, it can try to lookup that service in container "A" 2) Else, when the container "B" is being built it can iterate the services registered in container "A" and add them all in container "B" as proxies/callbacks to container "A".

And you are correct, the aspnet core stuff isn't 'wrapping' the concrete container. I guess it's more that it seems that way and that similar code could be used to wrap a container (i.e. in option #2 above). This was an idea for v8 to allow our container to be isolated from users while allowing users to be able to use the container or bring their own.

If we can get enough help and instead go with all things suggested here instead then I'm all for it.

@lars-erik I'm not sure if this helps at all but I created a DI library for v7 to allow devs to get DI into Umbraco in an easier way without having to worry about manually registering all Umbraco dependencies. This comes with it's own abstractions and then each container reads from these collections to build their container... very similar to .net core stuff. https://github.com/Shazwazza/Our.Umbraco.IoC

@zpqrtbnk One thing that could be of interest from the MS DependencyInjection lib is this: https://github.com/aspnet/DependencyInjection/blob/dev/shared/Microsoft.Extensions.ActivatorUtilities.Sources/ActivatorUtilities.cs#L103 (and https://github.com/aspnet/Mvc/blob/dev/src/Microsoft.AspNetCore.Mvc.Core/Internal/TypeActivatorCache.cs#L36) which is how MVC creates controller instances, by default it doesn't use the container to create controller instances, instead it uses this method to resolve all dependencies and manually construct the object. I know we do some interesting things for the IUmbracoComponent initialization so might useful or at least insightful :)


Lars-Erik Aabech 14 Jun 2018, 17:20:50

I've started a second PR for removing property injection. It's here: https://github.com/umbraco/Umbraco-CMS/pull/2690.

For the attributes I'll go with inglorious bastard constructors calling Current.Container.GetService. This also goes for dual constructors on controllers and the like (explained below). This is not as bad as tightly coupled bastard constructors since the only added dependency is the container abstraction. Testers and implementors will quickly spot the "right" injectable ctor alternative, so it's not really temporal coupling either.

Right now the container is a concrete LightInject container which means we won't get rid of the LightInject dependency, so I'm going to wrap the container as the next step.

As cheered about above, the UmbracoModule now gets stuff injected, and I did the DataTypeValidateAttribute just now. The latter still mentions LightInject due to the tightly bound GetService extension.

Reg. why I want to give the base controllers bastard constructors, it's to take care of the kids who don't even know what an interface is. Keep the barrier low and then make great guides on how to build more complex systems. (Got that, @peteduncanson?)

I had a good discussion with @zpqrtbnk yesterday and I think we are more or less on the same page. Think he'll add some notes suddenly.

@Shandem: As much as your solution for two containers would work, I do hope we can avoid it. It has a very interesting odour to it. ;)

I really hope that we'll be able to auto-generate a list of registered components, lifetime and possible oddities they might require. It would make a nice checklist for container compatibility. Hopefully if we manage to make the container replacable we'd soon see community-driven Umbraco.DI.Autofac, Umbraco.DI.Ninject, Umbraco.DI.Castle on nuget. No need for everyone to roll their own. If the (generated) docs and abstraction is just right, it would be possible to fairly easily replicate all registration in those packages.


Lars-Erik Aabech 14 Jun 2018, 17:24:52

Oh, and I'll keep a keen eye comparing to the project you linked, @shandem. Looks good.


Stephan 14 Jun 2018, 17:26:16

Think he'll add some notes suddenly.

Yup I'm putting them together, and all of a sudden, they'll be posted! ;-)


Stephan 14 Jun 2018, 17:28:09

@lars-erik re. attributes, do you know when they are instanciated? Are we positive they will be instanciated ''after'' the container is ready? I would have to check the C# specs to make sure that they cannot be, for instance, instanciated when the code is loaded in memory. In which case the inglorious ctor is not a solution.


Lars-Erik Aabech 14 Jun 2018, 17:32:37

@zpqrtbnk The DataTypeValidate one I did now is instantiated when the request is made. I debugged and checked the ctor & members while saving a type in the backoffice. I guess we'll have to test attribute by attribute. Worst case we swap to lazies calling the container. First goal is to get rid of the tight coupling, then remove as many anti-patterns as possible.


Stephan 14 Jun 2018, 17:36:04

@lars-erik sure. also found this quote: ''An Instance of the attribute class is only instantiated when the reflection mechanism accesses one of its representatives, Depending on its use, an attribute class in not necessarily instantiated (as with the System.ObsoleteAttribute class which does not need to be used by the reflection mechanism).'' so we are probably safe


Lars-Erik Aabech 14 Jun 2018, 17:39:58

thumbs up


Lars-Erik Aabech 19 Jun 2018, 18:19:08

So I've got some distance with this. Several property injections are now gone and all tests still pass. I've also started to ease in an abstraction for the container. Mainly for the bastard constructors still lying around in there. I'm thinking quite a few can just go since they probably won't be inherited or messed with by implementors.

However, when I look at the three "public" base controllers, I can't shake the feeling that several of the properties instantiated are just there for convenience for the non-DI users. UmbracoContext, Cache, Services, DatabaseFactory (!), Logger, ProfilingLogger (!) and more. Is it really necessary? Could those be accessed from Current.X instead? It makes for quite big ctors that everyone will need to relate to.

Could we have more concrete base classes like SurfaceControllerWithTrainingWheels etc? J/K, but there has to be a limit to how much padding you should get if you don't learn the "main" architecture.

Any other good way to bundle all the dependencies you might want for non-DI users?


Lars-Erik Aabech 28 Jun 2018, 21:31:34

Turned out to be a mouthful, this. But I can happily report I just pushed the last commit to abolish property injection to the PR. We're now 10% cleaner. :)


Stephan 29 Jun 2018, 05:56:40

going to review then ;-) then maybe don't add more code to that PR, we'll have another one for the remaining 90% ;-)


Priority: Normal

Type: Bug

State: In Progress

Assignee:

Difficulty: Normal

Category:

Backwards Compatible: True

Fix Submitted:

Affected versions: 8.0.0

Due in version: 8.0.0

Sprint:

Story Points:

Cycle: