Register decorator with another dependency of same generic type

Question

I think I have stumbled upon a quirk in Simple Injector's RegisterDecorator(). It occurs even in the most recent 2.5.0. I have a situation where I want to decorate a closed generic type, for example ICommandHandler<MessageCommand>, with a decorator that takes (via constructor injection) an inner handler of type ICommandHandler<MessageCommand>, but also another type of handler, say ICommandHandler<LogCommand>. Even though these command handler types are distinct, SimpleInjector seems to get confused and throws an exception when I call RegisterDecorator on such a decorator type:

ArgumentException: For the container to be able to use MessageLogger as a decorator, its constructor must include a single parameter of type ICommandHandler<MessageCommand> (or Func<ICommandHandler<MessageCommand>>) - i.e. the type of the instance that is being decorated. The parameter type ICommandHandler<MessageCommand> is defined multiple times in the constructor of class MessageLogger.

... even though the decorator clearly only has one ICommandHandler<MessageCommand> parameter.

Here is the full working example that throws the exception:

public interface ICommandHandler<T>
{
    void Execute(T command);
}

public class LogCommand
{
    public string LogMessage { get; set; }
    public DateTime Time { get; set; }
}

public class Logger : ICommandHandler<LogCommand>
{
    public void Execute(LogCommand command)
    {
        Debug.WriteLine(string.Format("Message \"{0}\" sent at {1}",
            command.LogMessage, command.Time));
    }
}


public class MessageCommand
{
    public string Message { get; set; }
}

public class MessageSender : ICommandHandler<MessageCommand>
{
    public void Execute(MessageCommand command)
    {
        Debug.WriteLine(command.Message);
    }
}

// message command handler decorator that logs about messages being sent
public class MessageLogger : ICommandHandler<MessageCommand>
{
    private ICommandHandler<MessageCommand> innerHandler;
    private ICommandHandler<LogCommand> logger;

    // notice these dependencies are two distinct closed generic types
    public MessageLogger(ICommandHandler<MessageCommand> innerHandler,
        ICommandHandler<LogCommand> logger)
    {
        this.innerHandler = innerHandler;
        this.logger = logger;
    }

    public void Execute(MessageCommand command)
    {
        innerHandler.Execute(command);

        var logCommand = new LogCommand
            {
                LogMessage = command.Message,
                Time = DateTime.Now
            };
        logger.Execute(logCommand);
    }
}

// this works as intended, but is tedious in a real-world app
ICommandHandler<MessageCommand> ResolveManually()
{
    ICommandHandler<MessageCommand> sender = new MessageSender();
    ICommandHandler<LogCommand> logger = new Logger();

    ICommandHandler<MessageCommand> loggerSender =
        new MessageLogger(sender, logger);

    return loggerSender;
}

// this is what I want to work - seems simple?
ICommandHandler<MessageCommand> ResolveWithSimpleInjector()
{
    var container = new Container();

    container.Register<ICommandHandler<LogCommand>, Logger>();

    container.Register<ICommandHandler<MessageCommand>, MessageSender>();

    // this next line throws the exception
    container.RegisterDecorator(typeof(ICommandHandler<MessageCommand>),
        typeof(MessageLogger));

    return container.GetInstance<ICommandHandler<MessageCommand>>();
}

void Main()
{
    //ICommandHandler<MessageCommand> sender = ResolveManually();
    ICommandHandler<MessageCommand> sender = ResolveWithSimpleInjector();

    var command = new MessageCommand { Message = "Hello World!" };
    sender.Execute(command);
}

I could not find any information about this situation. Is this a bug, or am I missing something?

Edit

I am fishing for feedback from a dev on SimpleInjector to find out if there is a technical reason for this limitation, or it is something that was overlooked... Unless someone can convince me there is a logical flaw in this design and there is a good reason I shouldn't do things this way, which none of the answers thus far have been able to do. I do appreciate the feedback.

It seems to me that the core issue is that RegisterDecorator() is treating two different closed generic types as the same type. There may be a technical reason for this based on its inner workings, but maybe not?

Answer 1

I had to do some investigation in the code base to see what was going one. You might call this a glitch in Simple Injector's implementation, but it's IMO a fair trade off. Simple Injector's decorator sub system is based around the idea of working with open generic types and open generic decorators. The check it does upon decorator registration is to see if a decorator's constructor has only one decoratee. This check is done using the open generic abstraction to which the decorator must be applied; in your case ICommandHandler<T>. Since at that point only the generic ICommandHandler<T> is available, two constructor parameters match this type.

It is possible to improve these pre-condition checks, but this is actually quite nasty, while the usefulness of this feature is very limited. It's limited because it's only useful for non-generic decorators. Take a look at the following decorator for instance:

public class GenericDecorator<TCommand> : ICommandHandler<TCommand> {
    public GenericDecorator(
        ICommandHandler<TCommand> decoratee,
        ICommandHandler<LoggingCommand> dependency)
    {
    }
}

This decorator is generic and allows you to apply it to any decorator, which is much more useful. But what happens when you resolve an ICommandHandler<LoggingCommand>? That would cause a cyclic dependency graph and Simple Injector will (obviously) not be able to create that graph and will throw an exception. It must throw, since the decorator will in that case have two ICommandHandler<LoggingCommand> arguments. The first will be the decoratee and will be injected with your Logger, and the second will be a normal dependency and will be injected with a GenericDecorator<LoggingCommand>, which is recursive of course.

So I would argue that the problem is in your design. In general I advice against composing command handlers out of other command handlers. The ICommandHandler<T> should be the abstraction that lies on top of your business layer that defines how the presentation layer communicates with the business layer. It's not a mechanism for the business layer to use internally. If you start doing this, your dependency configuration becomes very complicated. Here's an example of a graph that uses DeadlockRetryCommandHandlerDecorator<T> and a TransactionCommandHandlerDecorator<T>:

new DeadlockRetryCommandHandlerDecorator<MessageCommand>(
    new TransactionCommandHandlerDecorator<MessageCommand>(
        new MessageSender()))

In this case the DeadlockRetryCommandHandlerDecorator<T> and a TransactionCommandHandlerDecorator<T> are applied to the MessageSender command handler. But look what happens we apply your MessageLogger decorator as well:

new DeadlockRetryCommandHandlerDecorator<MessageCommand>(
    new TransactionCommandHandlerDecorator<MessageCommand>(
        new MessageLogger(
            new MessageSender(),
            new DeadlockRetryCommandHandlerDecorator<MessageLogger>(
                new TransactionCommandHandlerDecorator<MessageLogger>(
                    new Logger())))))

Notice how there's a second DeadlockRetryCommandHandlerDecorator<T> and a second TransactionCommandHandlerDecorator<T> in the object graph. What does it mean to have a transaction in a transaction and have a nested deadlock retry (within a transaction). This can cause serious reliability problems in your application (since a database deadlock will cause your operation to continue in a transaction-less connection).

Although it is possible to create your decorators in such way that they are able to detect that they are nested to make them work correctly in case they're nested, this makes implementing them much harder and much more fragile. IMO that's a waste of your time.

So instead of allowing command handlers to be nested, let command handlers and command handler decorators depend upon other abstractions. In your case, the problem can be easily fixed by changing your decorator by letting it use an ILogger interface of some sort:

public class MessageLogger : ICommandHandler<MessageCommand> {
    private ICommandHandler<MessageCommand> innerHandler;
    private ILogger logger;

    public MessageLogger(
        ICommandHandler<MessageCommand> innerHandler, ILogger logger) {
        this.innerHandler = innerHandler;
        this.logger = logger;
    }

    public void Execute(MessageCommand command) {
        innerHandler.Execute(command);

        logger.Log(command.Message);
    }
}

You can still have an ICommandHandler<LogCommand> implementation in case the presentation layer needs to log directly, but in that case that implementation can simply depend on that ILogger as well:

public class LogCommandHandler : ICommandHandler<LogCommand> {
    private ILogger logger;

    public LogCommandHandler(ILogger logger) {
        this.logger = logger;
    }

    public void Execute(LogCommand command) {
        logger(string.Format("Message \"{0}\" sent at {1}",
            command.LogMessage, DateTime.Now));
    }
}

Answer 2

This is an edge case that you could possibly argue either way but the fact is that Simple Injector explicitly does not support what you are trying to do.

A decorator would normally be required to apply common logic across all (or some) of a particular abstraction, which in your example is ICommandHandler. In other words MessageLogger is designed to decorate ICommandHandler's and as it is a decorator of ICommandHandler's it can only take one ICommandHandler in it's constructor. In addition, allowing something like this would require reams of horrible circular checks that are best avoided with a cleaner design!

As such you would normally define a decorator with the same interface (and generic parameters) as the type it is decorating

public class MessageLogger<TCommand> : ICommandHandler<TCommand>
    where TCommand : <some criteria e.g. MessageCommand>
{
    //....
}

The first solution I can think of to mitigate your problem is create a mediator to remove the direct dependency:

public class LoggerMediator
{
    private readonly ICommandHandler<LogCommand> logger;

    public LoggerMediator(ICommandHandler<LogCommand> logger)
    {
        this.logger = logger;
    }

    public void Execute(LogCommand command)
    {
        this.logger.Execute(command);
    }
}

And change your MessageLogger to use the mediator.

public class MessageLogger<TCommand> : ICommandHandler<TCommand>
    where TCommand : MessageCommand
{
    private ICommandHandler<TCommand> innerHandler;
    private LoggerMediator logger;

    public MessageLogger(
        ICommandHandler<TCommand> innerHandler,
        LoggerMediator logger)
    {
        this.innerHandler = innerHandler;
        this.logger = logger;
    }

    public void Execute(TCommand command)
    {
        innerHandler.Execute(command);

        var logCommand = new LogCommand
        {
            LogMessage = command.Message,
            Time = DateTime.Now
        };
        logger.Execute(logCommand);
    }
}

BTW you can simplify your registrations like this

var container = new Container();
container.RegisterManyForOpenGeneric(
    typeof(ICommandHandler<>), 
    typeof(ICommandHandler<>).Assembly);
container.Register<LoggerMediator>();
container.RegisterDecorator(typeof(ICommandHandler<>), typeof(MessageLogger<>));
container.Verify();

---

UPDATE

Looking through my code base here I have found that I have had a similar requirement and I resolved it with one extra class - a generic command mediator:

public class CommandHandlerMediator<TCommand>
{
    private readonly ICommandHandler<TCommand> handler;

    public CommandHandlerMediator(ICommandHandler<TCommand> handler)
    {
        this.handler = handler;
    }

    public void Execute(TCommand command)
    {
        this.handler.Execute(command);
    }
}

registered like this:

container.RegisterOpenGeneric(
    typeof(CommandHandlerMediator<>), 
    typeof(CommandHandlerMediator<>));

and referenced like this:

public class MessageLogger<TCommand> : ICommandHandler<TCommand>
    where TCommand : <some criteria e.g. MessageCommand>
{
    private ICommandHandler<TCommand> decorated;
    private CommandHandlerMediator<LogCommand> logger;

    public MessageLogger(
        ICommandHandler<TCommand> decorated,
        CommandHandlerMediator<LogCommand> logger)
    {
        this.innerHandler = innerHandler;
        this.logger = logger;
    }

    //....

}

One new class and you're sorted for all of your handlers.

Answer 3

You can change your Decorator ctor as

public MessageLogger(ICommandHandler<MessageCommand> innerHandler)
{
    this.innerHandler = innerHandler;
}

to match the neccessary ctor signature with "a single parameter of type ICommandHandler (or Func>)". And inject logger as a property and not the ctor argument. I didn't work with simple-injector but looking at your exception message it's the most obvious solution, because of decorator constructor signature limitations.

Answer 4

Your solution seems a bit awkward as it is a combination of a decorator and constructor injection/composition (something). While it's not exactly an asnwer to your question, it'll probably solve your problem (in a nicer way I'd say):

public class LoggingHandler : ICommandHandler<MessageCommand>
{
    private ICommandHandler<MessageCommand> innerHandler;

    public LoggingHandler(ICommandHandler<MessageCommand> innerHandler)
    {
        this.innerHandler = innerHandler;
    }

    public void Execute(MessageCommand command)
    {
        innerHandler.Execute(command);

        Debug.WriteLine(string.Format("Message \"{0}\" sent at {1}",
        command.Message, DateTime.Now));
    }
}

I don't see the need for a separate CommandHandler for a LogMessage. You could just log inside the object that decorates your actual commandhandler. What is its purpose otherwise?

With this approach you have a pure decorator, which is a better solution IMO as it saves you two extra classes.