Allow services to decorate each other by preventing recursive resolution

aspnet-hello commented 6 years ago

From @tuespetre on Friday, December 18, 2015 8:00:37 AM

User Story / Use Case Scenario

As a developer, I want to extend a previously registered service by wrapping it with another service that exposes the same interface.

See: https://github.com/aspnet/Mvc/issues/3739

Example interface and implementations

public interface IMyService
{
    void DoSomething();
}

public partial class DefaultMyService
{
    public partial void DoSomething();
}

public partial class DecoratorForMyService
{
    private IMyService decorated;

    public DecoratorForMyService(IMyService decorated)
    {
        this.decorated = decorated;
    }

    public partial void DoSomething();
}

Examples of ways to set up the service collection

Using constructor resolution

var collection = new ServiceCollection();
collection.AddTransient<IMyService, DefaultMyService>();
collection.AddTransient<IMyService, DecoratorForMyService>();
var provider = collection.BuildServiceProvider();
var service = collection.GetService<IMyService>(); // Returns the constructed DecoratorForMyService

Using implementation factories

var collection = new ServiceCollection();
collection.AddTransient<IMyService, DefaultMyService>();
collection.AddTransient<IMyService>(_provider =>
{
    // Returns the previously registered IMyService.
    // Calling _provider.GetService<IEnumerable<IMyService>>() here
    // would return a collection containing all registered IMyServices
    // except for this one.
    var decorated = _provider.GetService<IMyService>(); 
    return new DecoratorForMyService(decorated);
});
var provider = collection.BuildServiceProvider();
var service = collection.GetService<IMyService>(); // Returns the constructed DecoratorForMyService

Behavioral Considerations

Currently, the above two approaches result in 'Circular Dependency' exception and a StackOverflowException, respectively. An implementation that allowed for the above scenarios would effectively prevent the StackOverflowException entirely, and ~~throw the 'Circular Dependency' exception only when there are no other previously registered services for the type that is being resolved~~ result in circular dependency scenarios throwing the "Unable to resolve service..." kind of exception, which should in itself sufficiently convey the circularity for those cases.

Copied from original issue: aspnet/DependencyInjection#340

aspnet-hello commented 6 years ago

From @davidfowl on Friday, December 18, 2015 8:17:42 AM

We're not going to support something like this. The problem is it's a very specific feature and we'd need other DI systems to work this way. Or we could just give up and force you to use ours

aspnet-hello commented 6 years ago

From @davidfowl on Friday, December 18, 2015 8:19:50 AM

For research can you take a look at some other DI systems and see how they solve those problem. If they even allow it

aspnet-hello commented 6 years ago

From @tuespetre on Friday, December 18, 2015 9:17:07 AM

NInject allows the specification of a particular implementation to be used when injecting into another particular implementation. Not sure of the 'factory' situation.
StructureMap allows similar specification, as well as the ability to specify a type with which to wrap other registered implementations upfront.
Spring.NET I am not sure of; my initial searching does not turn up an example.
Windsor appears to allow you to simply register each of the types, although it seems it might take an opposite-ordered approach
Autofac takes an approach similar to NInject but it would seem that additional specification is required using 'name' string keys
Unity appears to have done similar to Autofac, using named registrations to handle decoration, but there is also a decorator extension available that functions similar to Windsor.

These are all of the 'major' DI libraries for .NET that I could recall. I saw names of some others that are not nearly as popular and so I did not look into them much further.

What I have proposed is then very similar to Windsor and Unity.

aspnet-hello commented 6 years ago

From @tuespetre on Friday, December 18, 2015 9:20:36 AM

@davidfowl:

I know you said you're not here to discuss philosophy, but the aspnet/DependencyInjection implementation already approaches this issue inconsistently by detecting and bailing out of the recursion for one scenario but not the other. That inconsistency could be addressed in a number of ways:

Add detection to the factory scenario to prevent a StackOverflowException from occurring
Remove detection from the constructor scenario and allow a StackOverflowException to occur
Remove detection from the constructor scenario but prevent a StackOverflowException from ever occurring by assuring that a given service is unavailable for resolution while it is already in the process of resolving

Each of these ways would resolve the inconsistency, but only one would bring something useful to the plate.

In addition to that matter, we have the matter of 'scopes', which is an implementation detail not guaranteed by the IServiceProvider interface. And while other DI libraries that implement the one-method IServiceProvider interface may implement some form of scoping, some might not. Please correct me if I am mistaken about that, but considering that, where is the resistance to having any sort of 'special' implementation details coming from? Here is a full list of things that are implemented by aspnet/DependencyInjection's implementation that go outside of the extremely nondescript GetService(Type) interface exposed by IServiceProvider:

Automatically registering the IServiceProvider as a service
Implementing and automatically registering the IServiceScopeFactory
Implementing and automatically registering the openly generic IEnumerable<> service which can be used to retrieve each service registered by a certain type

Now, specifically about those other container libraries again, such as NInject, StructureMap, and Autofac, the way they allow the specification of decorators is very easily accomplished with the factory lambda method available to aspnet/DependencyInjection's implementation, if only that implementation would prevent recursion up front. :wink:

Honestly, I haven't seen too much of anything at all that other DI libraries offer that isn't possible with the aspnet/DependencyInjection implementation. I think the whole issue of 'letting other containers do this or that' is really a non-issue.

aspnet-hello commented 6 years ago

From @brockallen on Friday, December 18, 2015 2:06:05 PM

Having the ability to do a decorator pattern in the DI system is quite useful and valuable.

aspnet-hello commented 6 years ago

From @tuespetre on Friday, December 18, 2015 2:37:43 PM

I've got it working in that PR. It doesn't matter the order in which you register the services. There's a test case with 36 combinations of scopes/orders/etc. Please let me know what you think.

Edit: This also changed the circular dependency scenario's outcome. Now, instead of a Circular Dependency exception, you will get the 'failed to resolve service of type...' or whatever exception, which in itself can inform the developer of the circularity.

aspnet-hello commented 6 years ago

From @Eilon on Tuesday, December 29, 2015 2:02:48 PM

I like this feature idea but moving it to the backlog because we are closing down on new features at this time.

aspnet-hello commented 6 years ago

From @cwe1ss on Monday, May 9, 2016 9:30:43 AM

Support for decorators would be great, that's the only big feature I'm missing right now.

I know, using a different container would be easy, but I just don't like the fact that your DI and the 3rd party container has to be configured differently. It's weird/confusing to have two different methods for container configuration in an application.

aspnet-hello commented 6 years ago

From @davidfowl on Monday, May 9, 2016 9:43:58 AM

I know, using a different container would be easy, but I just don't like the fact that your DI and the 3rd party container has to be configured differently. It's weird/confusing to have two different methods for container configuration in an application.

You're only configuring a single container. You must mean turning IServiceCollection (a bunch of metadata for describing service registrations) into the specific container API. That's not configuring 2 containers (and it's not going away).

PS: Don't expect us to add features to this container just because you need to use a 3rd party container to do something that isn't supported. We'll be adding features to this container very judiciously.

aspnet-hello commented 6 years ago

From @cwe1ss on Monday, May 9, 2016 11:51:35 AM

Thank you for your answer, David! I really like the IServiceCollection abstraction because it`s simple and it allows me to use it in my own libraries as well. and my host projects can still decide to use a different container.

However in one of my libraries I would need to use decorators. Now I'm kinda stuck... I can either decide not to use IServiceCollection at all in my libraries or use a 3rd party container and therefore have multiple containers in my libraries (which then have to be configured/called differently - either through ConfigureServices or through some other method)

I know, ideally there wouldn't be any DI code in my libraries but it`s just really nice to have a default config in every library (like you do it everywhere as well).

So would you recommend not using IServiceCollection in our own libraries and instead using a 3rd party container everywhere?

aspnet-hello commented 6 years ago

From @khellang on Monday, May 9, 2016 12:06:09 PM

Why would you want to impose a specific container on consumers of your library? IMO, that's not a decision for you, the library author, to make. Wouldn't it be better to just add extensibility points, like factories, providers, activators, whatever, that can be implemented for whatever container the consumer uses? :smile:

aspnet-hello commented 6 years ago

From @cwe1ss on Monday, May 9, 2016 12:16:44 PM

In my case I'm talking about internal libraries for my company. I know that it would be better to do so, but you know... Deadlines :-)

And to add a counter argument - if we shouldn't use containers in libraries, why does this github project exist??? :-)

aspnet-hello commented 6 years ago

From @cwe1ss on Monday, May 9, 2016 12:38:40 PM

BTW: my real life scenario is a business code library that contains many command handlers. I'm using something similar to MediatR to call the handlers. I would like to add validation, authorization, ... as decorators because it's an extremely simple solution.

Since there is no decorator support in IServiceCollection, I'm already using a separate IMessageInterceptor-interface and the dispatcher calls all registered instances of it before and after calling the actual handler. this is OK but it starts to get weird for error handling, transaction handling and so on because you can't just put a try..catch or a using-statement on top of it like you could with decorators.

I could also do something similar to the new ASP.NET request/Middleware pipeline which is delegate based but this seems like an overkill to me.

Maybe you have some other ideas?! I really appreciate your feedback!

aspnet-hello commented 6 years ago

From @khellang on Monday, May 9, 2016 1:03:36 PM

And to add a counter argument - if we shouldn't use containers in libraries, why does this github project exist??? :-)

Eh, it exists to use containers in apps?

Maybe you have some other ideas?! I really appreciate your feedback!

What about just simply wiring up the decoration manually?

Some pseudo-code:

public class ErrorHandlingMediator : IMediator
{
    public ErrorHandlingMediator(IErrorHandler errorHandler, IMediator inner) { }
}

public class ValidatingMediator : IMediator
{
    public ErrorHandlingMediator(IEnumerable<IValidator> validators, IMediator inner) { }
}

public class TransactionMediator : IMediator
{
    public ErrorHandlingMediator(ITransactionManager transactionManager, IMediator inner) { }
}

// Add IErrorHandler, IValidators and ITransactionManager to the service collection.

services.AddScoped<IMediator>(provider => {
    var singleFactory = new Func<Type, object>(provider.GetService);
    var multiFactory = new Func<Type, IEnumerable<object>>(provider.GetServices);

    // Let's configure the following pipeline: Error Handling -> Validation -> Transaction -> Request Handler

    return new ErrorHandlingMediator(
        provider.GetService<IErrorHandler>(),
        new ValidatingMediator(
            provider.GetServices<IValdiator>(),
            new TransactionMediator(
                provider.GetService<ITransactionManager>(),
                new Mediator( // This is the Mediator type from MediatR...
                    singleFactory,
                    multiFactory))));
});

This can of course be done completely without any container as well, but you'd need hooks to resolve the different services, like IErrorHandler etc. :smile:

aspnet-hello commented 6 years ago

From @cwe1ss on Monday, May 9, 2016 1:33:47 PM

o_O - I'm stupid, I didn't think about that. :-) It's not that dynamic/beautiful but I guess I can definitely live with that. Thank you Kristian!! A beer is on me if we ever meet at a conference. :beer:

A lot to learn there is! :-)

aspnet-hello commented 6 years ago

From @cwe1ss on Monday, May 9, 2016 2:18:26 PM

Maybe interesting for other people, who struggle with something like this as well: I was trying to decorate the IRequestHandler<TMessage> interface instead of the mediator. This way, the mediator would have had to create/resolve this chain of classes and I wouldn't have liked this code there. But I guess even in that case I could have solved it differently by providing another hook for the handler creation. Thanks again, Kristian!

aspnet-hello commented 6 years ago

From @hennys on Monday, May 9, 2016 3:43:48 PM

We are building a framework that extends the MVC framework and as a part of this we want to be able decorate some services provided by MVC with additional functionality. As a framework we don't want to impose a specific container on the Application that will use our framework, but we also don't know what concrete service that the application has configured beforehand.

I would imagine the same problem would arise for any Framework that is trying to provide the same type of generic extensibility as ASP.NET Core does through the service configuration as well. To extend the functionality you would either have to control the container, i.e. be the application, or make a assumption on which concrete service that you will extend. Alternatively you will need to fall back to exposing specific extension points such as factories or providers in your framework. Is that a fair description?

So from this perspective it seems like this proposed feature would be of immense help.

aspnet-hello commented 6 years ago

From @khellang on Monday, May 9, 2016 4:08:08 PM

I don't think a lot of people realize this, or they simply don't care, but by piling on features in this minimal, "lowest common denominator" implementation of an IoC container, you're basically excluding other containers that can't or won't support this required set of functionality.

aspnet-hello commented 6 years ago

From @hennys on Monday, May 9, 2016 4:28:45 PM

I can't speak for others, but I am (at least) aware of the Conforming Container problem. But since the decision has been made to implement one in the framework, I guess there will be a conflict of interest between the wish to include and support as many container implementations as possible and the wish from framework developers not in control of the container to have access to as many features as possible.

Personally I don't consider myself experienced enough to suggest where that balance should lie, but I wanted to add a view point that I hadn't seen being raised in this conversation.

aspnet-hello commented 6 years ago

From @khellang on Monday, May 9, 2016 5:14:15 PM

I guess there will be a conflict of interest between the wish to include and support as many container implementations as possible and the wish from framework developers not in control of the container to have access to as many features as possible.

You mention these framework developers that want access to as many features as possible. Why can't these frameworks use any other container that already have these features? I don't see how this container is special and can/should be used in all kinds of libraries and frameworks, while other containers can't?

Also, as I mentioned above; many would consider forcing a specific container on consumers of your framework/library bad form.

Personally I don't consider myself experienced enough to suggest where that balance should lie

If I've understood the team correctly, I think the balance is something like "Whatever the framework needs to compose itself. For anything more advanced, we recommend BYOC".

If Microsoft were to pile on features for this container, they'd effectively end up killing, or at least sucking a lot of air out of the room for other, existing containers in the .NET space, like they've done before with various other community efforts. Now, there's still no guarantee that that won't happen, but limiting the container's scope, will at least limit some of that effect.

aspnet-hello commented 6 years ago

From @khellang on Monday, May 9, 2016 5:37:24 PM

I'm not sure how performant it is, but this seems to work nicely; https://gist.github.com/khellang/c9d39444f713eab04c26dc09d5687196 :smile:

aspnet-hello commented 6 years ago

From @hennys on Monday, May 9, 2016 6:31:16 PM

Why can't these frameworks use any other container that already have these features?

I believe you answered that yourself. As a framework we don't want to force a specific container on the consuming application.

I don't see how this container is special and can/should be used in all kinds of libraries and frameworks, while other containers can't?

I guess it an abstraction that is already in the .NET framework. Sure, we could choose to expose our own or some other existing abstraction of a conforming container to which we aren't restricted to the features that Microsoft required in their framework, but that would mean we need additional Container adapters which doesn't really help anyone in the long run. Alternatively we ignore the ServiceCollection altogether and expose our own extension points, but that could potentially restrict our consumers from accessing the .NET extension points they are expecting.

I think that in this case we need to insert ourselves just along the lines that you have shown in your example. We've been experimenting with something similar although I admit not as elegant as your solution. We will just have to measure the impact that creating a service provider for each decorator has. There is also a potential issue that the decorated implementation could get an incorrect service dependency injected if they in turn are replaced after the decoration.

I understand the problem for all Containers that you are describing and I have great sympathy for that side of the issue. But, I also think that Microsoft has been looking at the container implementation primarily from the .NET frameworks and from the Application side of things and missed out on some of the needs of framework developers. But maybe this group is so small that those needs should be downplayed.

aspnet-hello commented 6 years ago

From @khellang on Tuesday, May 10, 2016 12:48:50 AM

I believe you answered that yourself. As a framework we don't want to force a specific container on the consuming application.

Exactly. You're making the point for me :wink: Why is Microsoft.Extensions.DependencyInjection not a "specific container"?

I also stated that if you don't want to depend on "a specific container" in your framework/library, you need to put hooks in place to "outsource" composition.

I guess it an abstraction that is already in the .NET framework.

No, it's not. It's a NuGet package you depend on, just like any other IoC container :smile:

Sure, we could choose to expose our own or some other existing abstraction of a conforming container to which we aren't restricted to the features that Microsoft required in their framework, but that would mean we need additional Container adapters which doesn't really help anyone in the long run.

Or you could put extensibility points in place to not use any "conforming container" at all.

BTW, do you know that several containers already have these adapters for MS.Ext.DI? Read my blog post on it :wink:

You could also just use IServiceProvider in your library. That interface has been in the good old BCL (mscorlib under the System namespace) since .NET 1.1. Implementing this interface for a container is literally 4 lines of code if you don't count the boilerplate of the class definition and implementing the interface itself.

Alternatively we ignore the ServiceCollection altogether and expose our own extension points, but that could potentially restrict our consumers from accessing the .NET extension points they are expecting.

What are these .NET extension points you're talking about?

We will just have to measure the impact that creating a service provider for each decorator has.

I don't think you need to do that?

There is also a potential issue that the decorated implementation could get an incorrect service dependency injected if they in turn are replaced after the decoration.

How would you ever get away from this? Hint: you can't, IServiceCollection is mutable :stuck_out_tongue_winking_eye:

I also think that Microsoft has been looking at the container implementation primarily from the .NET frameworks and from the Application side of things and missed out on some of the needs of framework developers.

Microsoft has already built a pretty decent framework on top of the IServiceProvider abstraction, namely ASP.NET :smile: This container is meant to be a bare-bones implementation to accomodate that frameworks' needs, nothing more, nothing less.

aspnet-hello commented 6 years ago

From @khellang on Thursday, September 8, 2016 4:29:56 AM

Hey folks!

I just polished up and included the Decorate<TService> APIs from my gist above into Scrutor (v1.9).

Here's an example on how to use it (from the README.md):

var collection = new ServiceCollection();

// First, add our service to the collection.
collection.AddSingleton<IDecoratedService, Decorated>();

// Then, decorate Decorated with the Decorator type.
collection.Decorate<IDecoratedService, Decorator>();

// Finally, decorate Decorator with the OtherDecorator type.
// As you can see, OtherDecorator requires a separate service, IService. We can get that from the provider argument.
collection.Decorate<IDecoratedService>((inner, provider) => new OtherDecorator(inner, provider.GetRequiredService<IService>()));

var serviceProvider = collection.BuildServiceProvider();

// When we resolve the IDecoratedService service, we'll get the following structure:
// OtherDecorator -> Decorator -> Decorated
var instance = serviceProvider.GetRequiredService<IDecoratedService>();

aspnet-hello commented 6 years ago

From @brockallen on Thursday, September 8, 2016 5:34:04 AM

@khellang yea, that's pretty much what we had to add to IdentityServer...

aspnet-hello commented 6 years ago

From @MatthewLymer on Friday, March 3, 2017 12:02:25 PM

@khellang Thank you for providing your project, it helped me out.

It would be nice if the decorator functionality was added to the default offering.

aspnet-hello commented 6 years ago

From @infinnie on Sunday, July 2, 2017 8:56:14 PM

@brockallen How were decorators used in the internal implementation of IdentityServer4?

aspnet-hello commented 6 years ago

From @brockallen on Monday, July 3, 2017 4:51:11 AM

@brockallen How were decorators used in the internal implementation of IdentityServer4?

Mainly caching around services that load data from a store, but we also need to do this to wrap some of the built-in authentication services to enhance their functionality.

aspnet-hello commented 6 years ago

From @andersborum on Wednesday, July 12, 2017 5:47:53 AM

@davidfowl Using decorators with DI isn't a very specific feature (in my oppinion); on the contrary, it's a fundamental pattern in the SOLID principles that easily facilitates the SRP principle.

However, it's quite easy (although not as elegant) to get around this by using the factory lamda method provided by the container. Would like to see generic support for this.

var collection = new ServiceCollection() .AddSingleton<IPackageService>(p => new FaultHandlingPackageService(p.GetService<PackageService>()));

aspnet-hello commented 6 years ago

From @khellang on Wednesday, July 12, 2017 5:49:54 AM

@andersborum This is precisely what I'm doing with Scrutor. Look at the code. It's nothing.

davidfowl commented 5 years ago

We'd recommend using Scrutor for this feature.

andriysavin commented 5 years ago

Hello, in my blogpost I described how a relatively simple extension method can solve this problem easily. Here is an example from that post which shows how decorator configuration may look like:

  services.AddDecorator<IEmailMessageSender, EmailMessageSenderWithRetryDecorator>(
    decorateeServices =>
    {
        decorateeServices.AddScoped<IEmailMessageSender, SmtpEmailMessageSender>();
    });

dotnet / aspnetcore