[Autofac 6.4.0]: RegisterGenericDecorator() resolves incorrect instance for the decorated type

[ViktorDolezel]

Describe the Bug

When I register a generic decorator for my command handlers with RegisterGenericDecorator() and then try to resolve all command handlers by container.Resolve(IEnumerable<IHandler<Command>>), the returned decorators all decorate the same instance.

Steps to Reproduce

full code here

[TestFixture]
public class ReproTest
{
   [Test]
   public void ResolveAllDecoratedCommandHandlers_IsSuccessful()
    {
        //Arrange
        var builder = new ContainerBuilder();

        builder.RegisterType(typeof(CommandHandler1))
            .As(typeof(ICommandHandler<Command>))
            .As(typeof(IHandler<Command>));
        builder.RegisterType(typeof(CommandHandler2))
            .As(typeof(ICommandHandler<Command>))
            .As(typeof(IHandler<Command>));

        builder.RegisterGenericDecorator(
            typeof(CommandHandlerDecorator<>),
            typeof(IHandler<>),
            context => context.ImplementationType.GetInterfaces().Any(t => t.IsGenericType 
                && t.GetGenericTypeDefinition() == typeof(ICommandHandler<>))
        );

        var container = builder.Build();

        //Act
        var commandHandlers = ((IEnumerable<IHandler<Command>>)container.Resolve(typeof(IEnumerable<IHandler<Command>>))).ToList();
        
        //Assert
        Assert.AreEqual(
            ((CommandHandlerDecorator<Command>)commandHandlers[0]).Decorated.GetType(), 
            typeof(CommandHandler1)); //fails, decorated is typeof(CommandHandler2)
        Assert.AreEqual(
            ((CommandHandlerDecorator<Command>)commandHandlers[1]).Decorated.GetType(), 
            typeof(CommandHandler2));

    }
}

interface IRequest { }
interface IHandler<in TRequest> where TRequest : IRequest { public void Handle(TRequest request); } 

interface ICommand: IRequest { }
interface ICommandHandler<in TCommand>: IHandler<TCommand> where TCommand: ICommand { } 

class Command : ICommand { }
class CommandHandler1 : ICommandHandler<Command> { public void Handle(Command command) => Console.WriteLine("CommandHandler1"); }
class CommandHandler2 : ICommandHandler<Command> { public void Handle(Command command) => Console.WriteLine("CommandHandler2"); }

class CommandHandlerDecorator<TCommand> : ICommandHandler<TCommand> where TCommand : ICommand
{
    public ICommandHandler<TCommand> Decorated { get; }
    public CommandHandlerDecorator(ICommandHandler<TCommand> decorated) => Decorated = decorated;
    public void Handle(TCommand request)
    {
        Console.WriteLine($"Command Decorator for {Decorated.GetType().FullName}");
        Decorated.Handle(request);
    }
}

Expected Behavior

the above unit test should pass:

Without using decorators, container.Resolve<IEnumerable<IHandler<Command>>() returns expected { CommandHandler1, CommandHandler2 }.
With decorators, I'm expecting { CommandHandlerDecorator(CommandHandler1), CommandHandlerDecorator(CommandHandler2) } but getting { CommandHandlerDecorator(CommandHandler2), CommandHandlerDecorator(CommandHandler2) }.

Dependency Versions

Autofac: 6.4.0

Additional Info

This was encountered with the MediatR library where MediatR works on its base interface IRequest.
I have control over the registration part (the "Arrange" section in the unit test) but not how MediatR resolves the command handlers.
In other words, the following solutions would not solve my issue:

• Resolve by calling Resolve(typeof(IEnumerable<ICommandHandler<Command>>) rather than Resolve(typeof(IEnumerable<IHandler<Command>>) [no control over that]
• Get rid of ICommand and IQuery [but I needz them! for example, the UnitOfWorkCommandHandlerDecorator should only apply to CommandHandlers]
• ditch MediatR
• use IPipelineBehavior of MediatR instead [the problem is present for MediatR.INofitications as well and those don't support pipelines]

(thank you for your time, you are appreciated!)

[tillig]

While I've not had the time to repro this or anything, it seems like the important aspect here is the IEnumerable<T> part. It's not that the decorator isn't decorating the right type all the time, it's that if you resolve a list of all the handlers (IEnumerable<T>), that's when you see the behavior.

It also means this line from the issue:

Without using decorators, container.Resolve<IHandler<Command>>() returns expected { CommandHandler1, CommandHandler2 }.

...is actually somewhat incorrect and should be:

Without using decorators, container.Resolve<IEnumerable<IHandler<Command>>>() returns expected { CommandHandler1, CommandHandler2 }.

Again, that IEnumerable<T> is super key.

[ViktorDolezel]

Sir, you are correct, nice catch! I fixed it in the Expected Behavior section.

Here's the corresponding passing test from my repo (without the decorator).

    [Test]
    public void ResolveAllCommandHandlers_IsSuccessful()
    {
        //Arrange
        var builder = new ContainerBuilder();

        builder.RegisterType(typeof(CommandHandler1))
            .As(typeof(ICommandHandler<Command>))
            .As(typeof(IHandler<Command>));
        builder.RegisterType(typeof(CommandHandler2))
            .As(typeof(ICommandHandler<Command>))
            .As(typeof(IHandler<Command>));

        var container = builder.Build();

        //Act
        var commandHandlers = ((IEnumerable<IHandler<Command>>)container.Resolve(typeof(IEnumerable<IHandler<Command>>))).ToList();

        //Assert
        Assert.AreEqual(commandHandlers[0].GetType(), typeof(CommandHandler1));
        Assert.AreEqual(commandHandlers[1].GetType(), typeof(CommandHandler2));

    }

And yes, IEnumerable<T> seems to be the issue.

[tillig]

I started looking at this and tried adding a failing test using our test types in the OpenGenericDecoratorTests fixture. I wasn't able to replicate the issue.

[Fact]
public void CanResolveMultipleClosedGenericDecoratedServices()
{
    var builder = new ContainerBuilder();
    builder.RegisterType<StringImplementorA>().As<IDecoratedService<string>>();
    builder.RegisterType<StringImplementorB>().As<IDecoratedService<string>>();
    builder.RegisterGenericDecorator(typeof(DecoratorA<>), typeof(IDecoratedService<>));
    var container = builder.Build();

    var services = container.Resolve<IEnumerable<IDecoratedService<string>>>();

    // This passes - the internal implementations are different types as expected.
    Assert.Collection(
        services,
        s =>
        {
            Assert.IsType<DecoratorA<string>>(s);
            Assert.IsType<StringImplementorA>(s.Decorated);
        },
        s =>
        {
            Assert.IsType<DecoratorA<string>>(s);
            Assert.IsType<StringImplementorB>(s.Decorated);
        });
}

public interface IDecoratedService<T>
{
    IDecoratedService<T> Decorated { get; }
}

public class StringImplementorA : IDecoratedService<string>
{
    public IDecoratedService<string> Decorated => this;
}

public class StringImplementorB : IDecoratedService<string>
{
    public IDecoratedService<string> Decorated => this;
}

public class DecoratorA<T> : IDecoratedService<T>
{
    public DecoratorA(IDecoratedService<T> decorated)
    {
        Decorated = decorated;
    }

    public IDecoratedService<T> Decorated { get; }
}

I started looking at your repro, but it seems like there is a lot in there. For example, I don't know that IRequest needs to be in there. I'm not sure why the repro needs to have things registered as both ICommandHandler<T> and IHandler<T> (unless that's part of what makes it fail?).

In order to really dig in here, I need the repro simplified a lot.

• Remove the "passing" tests. I'm glad they're passing, but they also confuse the issue.
• Remove any assembly scanning.
• Remove any types that are not directly involved in observing the failure. Interfaces that implement other interfaces. Base types that are used in covariant/contravarient constraints (IGeneric<in T>, IGeneric<out T>).
• Use the specific assertions with expected/actual in the right spot. For example, instead of asserting if types are equal, Assert.IsInstanceOf(Type expected, Type actual). While this one seems a little nitpicky, what I'm trying to do is make sure there's not something weird going on with equality checking and also make sure expected and actual are reported properly so we know what we're actually looking for. (Right now, the expected and actual are in the wrong spots so will report incorrectly.)

Reducing the repro to the absolute minimum helps because it points us at very specific things to look at. With the more complex repro and all the additional types, it's hard to know if it's a covariant/contravariant problem, a registration problem, a problem that happens only if you register a component As<T> two different services, or what.

Post the complete, minified repro here in this issue (not in a remote repo) and I can come back and revisit.

[ViktorDolezel]

Hi, thanks for looking into it.
Here's a failing test that can be added to the OpenGenericDecoratorTests.

    [Fact]
    public void ResolvesMultipleDecoratedServicesWhenResolvedByOtherServices()
    {
        var builder = new ContainerBuilder();
        builder.RegisterGeneric(typeof(ImplementorA<>)).As(typeof(IDecoratedService<>)).As(typeof(IService<>));
        builder.RegisterGeneric(typeof(ImplementorB<>)).As(typeof(IDecoratedService<>)).As(typeof(IService<>));
        builder.RegisterGenericDecorator(typeof(DecoratorA<>), typeof(IService<>));
        var container = builder.Build();

        var services = container.Resolve<IEnumerable<IService<int>>>();

        Assert.Collection(
            services,
            s =>
            {
                var ds = s as IDecoratedService<int>;
                Assert.IsType<DecoratorA<int>>(ds);
                Assert.IsType<ImplementorA<int>>(ds.Decorated);
            },
            s =>
            {
                var ds = s as IDecoratedService<int>;
                Assert.IsType<DecoratorA<int>>(ds);
                Assert.IsType<ImplementorB<int>>(ds.Decorated);
            });
    }

[tillig]

Awesome, thanks for that, it helps. Looks like the difference is that the components are registered as two different services rather than just one - two As clauses. It should still work, but I'm guessing that's the key here.

This is going to take some time to get to. I have a bunch of PRs I'm trying to work through and both I and the other maintainers are pretty swamped with day job stuff lately. Obviously we'll look into it, for sure, but it may not be fast. If you have time and can figure it out faster, we'd totally take a PR.

[ViktorDolezel]

I'll give it a try but it could be out of my league.

[jfbourke]

Hi, trying to familiarize myself with the code base more and this problem looked interesting. Would something like this develop...jfbourke:Autofac:issue1330 be appropriate? Or is there a better place to look?

[tillig]

@jfbourke I think the DecoratorMiddleware is a reasonable place to start looking, yes.

[jfbourke]

Found some time to look into this again. A bit of a brain dump.

In this case, we have resolved an IService and the appropriate decorator. The decorator needs an IDecoratedService instance but this cannot be supplied by the service on the context as the TypedParameter is being used and the predicate in that class performs an exact match only. This tracks to the ConstructorBinder which uses the availableParameters on the ResolveRequest to find matching items; if none match a ForBindFailure is returned which leads to the resolve of the unexpected ImplementorB.

I can see two options at the moment (maybe there are more);

1. Add all the registrations in the context to the resolveParameters array (sample code shared previously), or

2. Switch from TypedParameter to IsInstanceOfTypeParameter for the serviceParameter; this is a new class that checks if the value we have is an instance of the type we need, e.g.

    public IsInstanceOfTypeParameter(Type type, object? value)
        : base(value, pi => pi.ParameterType.IsInstanceOfType(value))
    {
        Type = type ?? throw new ArgumentNullException(nameof(type));
    }

[GianlucaLocri]

Hi! I think I'm having a very close related problem here. I'm also using MediatR.

I filed an issue on Mediatr repo, but after finding this issue, I think that the problem is related to autofac.
I am using a decorator for decorating the handlers that are implementing the INotificationHandler.
I have an example here to demonstrate the behaviour https://dotnetfiddle.net/5sIGzj

What my sample code do is

• defining the handlers to be decorated
• defining the decorator implementation
• register the container with mediator service
• call MediatR publish method in order to let him resolve and call all the handlers.

To my understanding, the problem is right here:
https://github.com/jbogard/MediatR/blob/cac76bee59a0f61c99554d76d4c39d67810fb406/src/MediatR/Wrappers/NotificationHandlerWrapper.cs#L25

MediatR is using an IServiceProvider to get all the handlers implementing INotificationHandler<TNotification>.
In my case TNotification is IEventNotification<TestEvent>>.

In fact I verified that in my case this line
serviceFactory.GetServices<INotificationHandler<IEventNotification<TestEvent>>>();
indeed returns 2 decorators with the same decorated service inside.

If instead I do not register the generic decorator, the above same line returns two distinct handlers as expected.

[GianlucaLocri]

Further testing:
I've tried with a simpler decorator:
builder.RegisterGenericDecorator(typeof(MyDecorator<>), typeof(INotificationHandler<>));

public class MyDecorator<T> : INotificationHandler<T>
    where T : INotification
{
    private readonly INotificationHandler<T> _decorated;

    public MyDecorator(INotificationHandler<T> decorated)
    {
        _decorated = decorated;
    }

    public Task Handle(T notification, CancellationToken cancellationToken)
    {
        _decorated.Handle(notification, cancellationToken);
        return Task.CompletedTask;
    }
}

The decorator is called too many times (I was expecting this) but when finally the right target is decorated, the injected decorated handler is always the same.