Skip to content
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

Open-generic registrations can not be decorated #39

Closed
dotnetjunkie opened this issue Oct 14, 2017 · 8 comments
Closed

Open-generic registrations can not be decorated #39

dotnetjunkie opened this issue Oct 14, 2017 · 8 comments

Comments

@dotnetjunkie
Copy link

dotnetjunkie commented Oct 14, 2017

Let me start by saying that I am quite amazed to see what you've been able to build on top of the DI abstraction. The omission of being able to apply decorators (and batch-registration) makes the built-in container IMO a worthless tool for any reasonably sized code base.

That said, I stumbled upon a limitation of your current implementation. Although you're able to apply open-generic decorators to closed-generic registrations, it seems impossible to apply open-generic decorators to open-generic registrations.

This might very well be a limitation of the API you are building upon, but I'll let you be the judge of that.

Here's a failing unit test:

[Fact]
public void CanDecorateOpenGenericTypeBasedOnOpenGenericRegistration()
{
    var provider = ConfigureProvider(services =>
    {
        services.AddTransient(typeof(IQueryHandler<,>), typeof(QueryHandler<,>));
        services.Decorate(typeof(IQueryHandler<,>), typeof(LoggingQueryHandler<,>));
        services.Decorate(typeof(IQueryHandler<,>), typeof(TelemetryQueryHandler<,>));
    });

    var instance = provider.GetRequiredService<IQueryHandler<MyQuery, MyResult>>();

    var telemetryDecorator = Assert.IsType<TelemetryQueryHandler<MyQuery, MyResult>>(instance);
    var loggingDecorator = Assert.IsType<LoggingQueryHandler<MyQuery, MyResult>>(
        telemetryDecorator.Inner);
    Assert.IsType<MyQueryHandler>(loggingDecorator.Inner);
}
@khellang
Copy link
Owner

This might very well be a limitation of the API you are building upon, but I'll let you be the judge of that.

Yes. This is definitely a limitation of the container itself.

At first, I was convinced that it was impossible to implement this (given the existing constraints), but @nicholasham came up with a way where we could leverage the existing (closed) registrations in order to create decorated registrations.

Because there's no way to get information about the requested generic type arguments, I can't see a way for us to create closed generic types to decorate 😢

@khellang
Copy link
Owner

I don't think there's anything we can do here, unfortunately 😞

@julealgon
Copy link

I don't think there's anything we can do here, unfortunately 😞

This is incredibly sad to hear. As I mentioned in #49, open decorators are an incredible feature that I'm honestly not sure if I can live without.

Are you 100% sure it is impossible to make it work now? Can we consider asking Microsoft for the necessary tooling to create this feature in Scrutor?

I understand they would probably not add decorator registrations themselves, but maybe they could add what we need in order to do that in this library here.

@dotnetjunkie
Copy link
Author

Asking Microsoft will have little effect. Any time microsft adds a feature to their DI Container, they are introducing a breaking change that might affect every adapter to their abstraction.

The solution, however, is rather simple: just use an alternative DI Container. Most libraries do support the feature you are looking for. For sure I know Autofac, StructureMap and Simple Injector support this.

@khellang
Copy link
Owner

The solution, however, is rather simple: just use an alternative DI Container.

Yes, do this 👍

@akovanev
Copy link

Any updates here?

@dotnetjunkie
Copy link
Author

Any updates here?

@akovanev, Microsoft didn't add any feature in .NET 5 or .NET 6 to MS.DI that would allow @khellang to implement this feature in Scrutor. That status of this feature request is, therefore, unchanged.

@akovanev
Copy link

I don't know if my workaround will be helpful to anyone, as it has a few significant disadvantages, but let me put the link to the project here CqsDecorator.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants