Runtime: Add decorator extension methods to IServiceCollection

[davidfowl]

Currently Scrutor has these:

https://github.com/khellang/Scrutor/blob/338a59333c7eafe25d5aefdd22434653c83eb9ab/src/Scrutor/ServiceCollectionExtensions.Decoration.cs#L10

I think we should consider adding something similar in the box for a couple of reasons:

• It helps when default implementations are internal or non virtual.
• It promotes composition over inheritance

This needs to be built in a way that doesn't require container authors to change anything (that's why extension methods are being proposed).

cc @khellang Since he may want to contribute this 😉.

Some things I am concerned about in the above implementation:

• Performance of those methods, some of them allocate (https://github.com/khellang/Scrutor/blob/338a59333c7eafe25d5aefdd22434653c83eb9ab/src/Scrutor/ServiceCollectionExtensions.Decoration.cs#L249)
• Performance of Scanning the IServiceCollection, multiple times (https://github.com/khellang/Scrutor/blob/338a59333c7eafe25d5aefdd22434653c83eb9ab/src/Scrutor/ServiceCollectionExtensions.Decoration.cs#L294)
• Performance of service resolution https://github.com/khellang/Scrutor/blob/338a59333c7eafe25d5aefdd22434653c83eb9ab/src/Scrutor/ServiceCollectionExtensions.Decoration.cs#L333-L356, ActivatorUtilities.GetServiceOrCreateInstance should be swapped with CreateFactory.
• The quirks around the fact that this mutates the service collection in place (https://github.com/khellang/Scrutor/blob/338a59333c7eafe25d5aefdd22434653c83eb9ab/src/Scrutor/ServiceCollectionExtensions.Decoration.cs#L296). It affects code like this:
  Works
  C# services.AddSingleton<IFoo, Foo>(); services.Decorate<IFoo, Foo2>();
  Does not work
  C# services.Decorate<IFoo, Foo2>(); services.AddSingleton<IFoo, Foo>();

[khellang]

Yeah... The current implementation is a giant hack around the container's current limitations and there are some confusing aspects around the API, like "what lifetime should the decorator have?". I've settled on copying the lifetime of the decoratee, but some people have argued that there are cases where this isn't the best solution.

Agreed on all the perf concerns. I've basically just kept things as simple as possible (code-wise) since it's mostly a one-time cost at startup and no one has complained (yet).

Most of them should be pretty easy to fix, i.e. remove LINQ usage, but some things are just fundamentally hard to do on top of the current feature set. I'd be really interested in hearing your take on the last bullet point, as an example.

[davidfowl]

Yeah... The current implementation is a giant hack around the container's current limitations and there are some confusing aspects around the API, like "what lifetime should the decorator have?". I've settled on copying the lifetime of the decoratee, but some people have argued that there are cases where this isn't the best solution.

I think people are wrong there, the most correct thing is to preserve the lifetime.

Agreed on all the perf concerns. I've basically just kept things as simple as possible (code-wise) since it's mostly a one-time cost at startup and no one has complained (yet).

People wouldn't complain but I would 😄. More seriously, we're been taking a look at how much time we spend re-enumerating the IServiceCollection at startup and it's kinda crazy.

Most of them should be pretty easy to fix, i.e. remove LINQ usage, but some things are just fundamentally hard to do on top of the current feature set.

Agreed but I think doing this without it being a container native feature is appealing.

I'd be really interested in hearing your take on the last bullet point, as an example.

I think that limitation is fine. The alternatives I can think of have to do with storing that list of decorators in a side object and writing a custom IServiceProviderFactory that unwraps them but that seems like overkill.

[davidfowl]

cc @aspnet/di-council

[seesharper]

@aspnet/di-council
Better decorator support would be great to have and this is something that has been supported in LightInject since the beginning.

To get the discussion going I'll give you a brief explanation of how this is implemented in LightInject.

Decorator characteristics

• Decorators MUST implement the same interface as the service being decorated.
• Decorators MUST inject the service being decorated.
• Decorators can OPTIONALLY inject other services.
• Decorators ALWAYS follow the same lifetime/lifecycle as the decorated service (decoratee).
• Decorators are ALWAYS applied in their registration order.

Example

```c#
public class FooDecorator : IFoo
{
public FooDecorator(IFoo decoratee, IBar bar)
{
}
}

### Registration

Let's take a look at the following registrations. 

```c#
container.RegisterScoped<IFoo, Foo>();
container.Decorate<IFoo, FooDecorator>();
container.Decorate<IFoo, AnotherFooDecorator>();

The decorators are applied in the registration order meaning that the decorator chain here will be

AnotherFooDecorator -> FooDecorator -> Foo

Please note that service registration is completely decoupled from the decorators meaning that the following will also work.

c# container.Decorate<IFoo, FooDecorator>(); container.Decorate<IFoo, AnotherFooDecorator>(); container.RegisterScoped<IFoo, Foo>();

Lifetime

Decorators follow the same lifetime as the service being decorated which is a very important aspect of decorators. Allowing a decorator to have a different lifetime/lifecycle than the service being decorated could lead to some serious confusion among users. If we think of decorators as being the manifestation of the Open/Closed principle, we want to be able to add new functionality/aspect without altering the behavor of the service being decorated.

If in any case we should require different lifetimes, and mind that these cases should be extremely rare, we can always do "manual" decoration in the factory delegate used during registration. So my advice here is to keep it simple and let decorators follow the lifetime of the decorated service.

Summary

This covers the very basics of decorator support in LightInject and I have intentionally not gone into more advanced scenarios like decorating open generic types which is where things really starts to get complicated from an implementation standpoint. I would also advice against such advanced features as it would make it substantially harder to confirm to the MS.DI abstraction

[davidfowl]

@seesharper See the code I linked to (@khellang's code) which implements decorators on top of the IServiceCollection primitives. I'm entertaining this approach because it doesn't require any changes to the spec or other containers. It's built on top but comes with some limitations.

[dadhi]

@davidfowl ,

If the Decorators implemented externally as extensions, does it mean that DI adapters treat them as normal services and register as usual?
As a result we are getting two types of decorators - black boxes in the ServiceCollection and the native ones in IoC libraries.

It may cause a confusion:

• Both Decorartor kinds will have a different feature set.
• Registration intent is erased in case of SC.
• Resolution Error messages may be confusing with the mixed stack of things.

[ENikS]

Could you provide a link to the relevant documentation? This term is rather ambiguous and would be nice to know exactly what is being discussed.

Is this akin to interception?

[khellang]

https://en.m.wikipedia.org/wiki/Decorator_pattern

[ENikS]

So, essentially decorators are what Unity and Castle call interception, or are these separate services?

In other words, if I resolve IFoo, will container return three services with two of them being decorators, or it will return one latest decorator with others and original service chained down the line?

[khellang]

So, essentially decorators are what Unity and Castle call interception, or are these separate services?

I'm not super familiar with it, but I always thought interception was some sort of AOP thing where proxies were generated to intercept calls to resolved instances. I guess you could look at it as interception, but with way less magic.

In other words, if I resolve IFoo, will container return three services with two of them being decorators, or it will return one latest decorator with others and original service chained down the line?

The latter. You'll get a chain of wrapped interceptors.

[ENikS]

Well, if this is the later than it would be rather challenging to implement for multiple, chained decorators. Not impossible, but very challenging. At present Unity does not implement multilevel interception.

I like the idea though

[ENikS]

@dadhi

If the Decorators implemented externally as extensions, does it mean that DI adapters treat them as normal services and register as usual?

I do not think you could register decorators as normal services. The adapter has to recognize these registrations are decorators and create relevant internal registrations (configure interception in case of Unity). The entry in a collection should unambiguously identify it as a decorator.

[khellang]

I do not think you could register decorators as normal services.

The OP shows an implementation of this on top of the existing abstractions as extensions.

[ENikS]

@khellang

I was talking about implementing decoration in the container itself. Done at the container level it would eliminate all these scans and speed things up a bit.

[seesharper]

Is this akin to interception?

You could say that decorators are a way of implementing AOP (Aspect Oriented Programming) where each decorator is responsible for an "aspect" such as logging, profiling ,caching, circuit breaking and other cross cutting concerns. The decorator pattern really starts to shine when dealing with reusable open generic types. As an example, a common way of implementing the command part of the CQRS pattern is to have an ICommandHandler<T> interface like this.

```c#
public interface ICommandHandler
{
Task HandleAsync(TCommand command, CancellationToken cancellationToken = default);
}

```c#
public class SaveCustomerCommandHandler : ICommandHandler<SaveCustomerCommand>
{
    private readonly IDbConnection dbConnection;

    public SaveCustomerCommandHandler(IDbConnection dbConnection)
    {
        this.dbConnection = dbConnection;    
    }

    public async Task HandleAsync(SaveCustomerCommand command, CancellationToken cancellationToken = default(CancellationToken))
    {
        // Save the customer to the database
    }
}

Now let's imagine that we have many of these command handlers and we want to make sure that we execute them in the context of a transaction. We could start the transaction in each command handler or we could use the decorator pattern to wrap all command handlers in a transaction like this.

```c#
public class TransactionalCommandHandler : ICommandHandler
{
private readonly IDbConnection dbConnection;
private readonly ICommandHandler commandHandler;

public TransactionalCommandHandler(IDbConnection dbConnection, ICommandHandler<TCommand> commandHandler)
{
    this.dbConnection = dbConnection;
    this.commandHandler = commandHandler;        
}

public async Task HandleAsync(TCommand command, CancellationToken cancellationToken)
{        
    using (var transaction = dbConnection.BeginTransaction())
    {
        await commandHandler.HandleAsync(command, cancellationToken);
        transaction.Commit();     
    }
}

}

To decorate each command handler with the `TransactionalCommandHandler` all we need is this line (LightInject).

```c#
container.Decorate(typeof(ICommandHandler<>), typeof(TransactionalCommandHandler<>))

And Viola, all command handlers are now executed in a transaction. We could do the same for logging, profiling and other cross cutting concerns. @khellang You can decorate open generics in Scrutor as well?

But as mentioned, most people think of AOP as magic stuff involving generating proxies and implementing interceptors. I have a fair bit of experience with how much magic it takes since I've written an interception library (LightInject.Interception). The difference is that the "decorator" is implemented at runtime (proxy) and the method calls are forwarded to the interceptor. The downside of this is that it is in many cases difficult to debug and to grasp for new developers. We will also take a performance hit since all method arguments are passed to the interceptor as object meaning there will be a lot of casting and boxing going on.

[dadhi]

The Decorators in DryIoc described here: https://github.com/dadhi/DryIoc/blob/master/docs/DryIoc.Docs/Decorators.md

In general the feature is similar to LightInject and combining with other library features may enable more things.

The __important__ point is that Decorator registrations are distinguished from the "normal" Service registrations. This info is preserved by container and used to enable nesting, special diagnostics, control the caching, etc.

For the whole picture DryIoc has a 3rd kind of registrations - Wrappers, e.g. Func, Lazy, collections go here.

[ipjohnson]

So for external containers the decorator will be created by the external container and the decorated type (original registration) will be created using ActivatorUtilities.GetServiceOrCreateInstance(provider, type); not the external container?

Can you chain together decorators? Does it support more advanced scenarios like where the decorator can have Func<T> as the constructor parameter instead of just T?

[khellang]

So for external containers the decorator will be created by the external container and the decorated type (original registration) will be created using ActivatorUtilities.GetServiceOrCreateInstance(provider, type); not the external container?

Correct. The decorated type (decoratee) won't be resolved from the external container, but all its dependencies will.

This is also problematic for disposal, since the decorated instance won't be tracked by the container for disposal 😞

[tillig]

While I like the notion that it doesn't require any container authors to change anything, it also might be nice to allow containers that already implement decorators to do so natively. Similar to the way ISupportRequiredService allowed optional support for the GetRequiredService extension.

I'm also having a tough time working out mentally: What does it mean if some decorators are registered with these extension methods while others may be registered via the underlying container? For example:

```c#
public void ConfigureServices(IServiceCollection services)
{
services.AddSingleton();
services.Decorate();
}

public void ConfigureContainer(ContainerBuilder builder)
{
builder.RegisterDecorator();
}
```

At least in Autofac, there's a whole resolution context around decoration so folks can intercept when something is getting decorated and do some complex things. I'm not clear how that would interact by some decoration being handled externally and some internally, like if there'd be some weirdness around things getting double decorated or possibly decorated in the wrong order.

I think if there was some way to map the IServiceCollection extensions to something native for those containers that support native decoration, it might be easier to address.

[ENikS]

Correct. The decorated type (decoratee) won't be resolved from the external container, but all its dependencies will.

This is a problem. My concerns are:

• How to keep track of the dependency graphs?
• Using containers configured outside of IServiceCollection
• Recurrent references?

[khellang]

Recurrent references?

The entire reason this is done is because of recurrent references (if I understand the term correctly). Decorators always get an instance of "itself" injected, i.e. you have an ICommandHandler<TCommand> that takes an ICommandHandler<TCommand> to decorate. This results in a StackOverflowException unless you do it this way.

[Dotnet-GitSync-Bot]

I couldn't figure out the best area label to add to this issue. Please help me learn by adding exactly one area label.