Closed ViktorDolezel closed 1 week ago
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.
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.
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.
IGeneric<in T>
, IGeneric<out T>
). 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.
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);
});
}
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.
I'll give it a try but it could be out of my league.
Hi, trying to familiarize myself with the code base more and this problem looked interesting. Would something like this https://github.com/autofac/Autofac/compare/develop...jfbourke:Autofac:issue1330 be appropriate? Or is there a better place to look?
@jfbourke I think the DecoratorMiddleware
is a reasonable place to start looking, yes.
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);
Add all the registrations in the context to the resolveParameters array (sample code shared previously), or
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));
}
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
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.
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.
Describe the Bug
When I register a generic decorator for my command handlers with
RegisterGenericDecorator()
and then try to resolve all command handlers bycontainer.Resolve(IEnumerable<IHandler<Command>>)
, the returned decorators all decorate the same instance.Steps to Reproduce
full code here
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(typeof(IEnumerable<ICommandHandler<Command>>)
rather thanResolve(typeof(IEnumerable<IHandler<Command>>)
[no control over that]ICommand
andIQuery
[but I needz them! for example, theUnitOfWorkCommandHandlerDecorator
should only apply to CommandHandlers]IPipelineBehavior
of MediatR instead [the problem is present forMediatR.INofitication
s as well and those don't support pipelines](thank you for your time, you are appreciated!)