-
-
Notifications
You must be signed in to change notification settings - Fork 832
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Autofac 6.4.0]: RegisterGenericDecorator() resolves incorrect instance for the decorated type #1330
Comments
While I've not had the time to repro this or anything, it seems like the important aspect here is the It also means this line from the issue:
...is actually somewhat incorrect and should be:
Again, that |
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, |
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 In order to really dig in here, I need the repro simplified a lot.
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 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.
|
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 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 develop...jfbourke:Autofac:issue1330 be appropriate? Or is there a better place to look? |
@jfbourke I think the |
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!)
The text was updated successfully, but these errors were encountered: