-
-
Notifications
You must be signed in to change notification settings - Fork 839
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
Enhance decorator support and syntax #880
Comments
@tillig Any movement on this? This is still quite a pain-point that I have had for a few years. |
Any "movement" will be updated in this issue. No need to ping; the point of the issue here is to ensure visibility into any progress. It is recognized as a pain point; it's also recognized as not a trivial thing to fix that will likely create breaking changes to the API. As such... well, there are only two project owners and there are a lot of issues. Huge speculative issues that will bring breaking changes are difficult to get momentum on. |
Understood - I thought movement was happening. This way I will not have huge possibilities to deal with and can get the lambda based conditional decorator support I need so bad! Sorry for any inconvenience @tillig |
I had a chance on the weekend to play around with some ideas in a branch and have pushed the branch up to GitHub. The unit tests linked to below probably provide the best description of where the journey has taken me so far. For those who aren't up for reading through some unit tests I'll try to summarise the key points here. The You can resolve this service as though it was a regular builder.RegisterDecorated<ImplementorA, IDecoratedService>(); You can configure additional attributes of the registration such as lifetime scope, metadata, parameters and such. builder.RegisterDecorated<ImplementorA, IDecoratedService>().SingleInstance(); The builder.RegisterDecorated<ImplementorA, IDecoratedService>(); // May be decorated, but doesn't have to be.
builder.RegisterDecorator<DecoratorA, IDecoratedService>(); // Will be applied first
builder.RegisterDecorator<DecoratorB, IDecoratedService>(); // Will be applied last You cannot configure additional attributes of the registration such as lifetime scope on the decorated registration. It will inherit the lifetime scope and other attributes such as metadata of the service it is decorating. builder.RegisterDecorated<ImplementorA, IDecoratedService>(); // This method returns void. The generic type constraints ensure that builder.RegisterDecorated(typeof(ImplementorA), typeof(IDecoratedService));
builder.RegisterDecorator(typeof(DecoratorA), typeof(IDecoratedService));
builder.RegisterDecorator(typeof(DecoratorB), typeof(IDecoratedService)); It is possible to conditionally register decorators. In the example below, builder.RegisterDecorated<ImplementorA, IDecoratedService>();
builder.RegisterDecorator<DecoratorA, IDecoratedService>(context => context.AppliedDecorators.Any());
builder.RegisterDecorator<DecoratorB, IDecoratedService>(); The context also provides access to the last instance created in the decorator chain which is initially the inner implementation and then updated to decorators as they are applied. This could open up some interesting runtime decorator chain construction scenarios. I would be interested to know if people think this would be useful. builder.RegisterDecorated<ImplementorA, IDecoratedService>();
builder.RegisterDecorator<DecoratorA, IDecoratedService>(ctx => ((IDecoratedService)ctx.CurrentInstance).Foo == "A");
builder.RegisterDecorator<DecoratorB, IDecoratedService>(ctx => ((IDecoratedService)ctx.CurrentInstance).Foo == "B");
builder.RegisterDecorator<DecoratorC, IDecoratedService>(ctx => ((IDecoratedService)ctx.CurrentInstance).Foo == "C"); The full list of applied decorator instances is available on the context. Maybe having a dedicated property for the implementation instance, and leaving accessing the last applied decorator as public interface IDecoratorContext
{
Type ImplementationType { get; }
Type ServiceType { get; }
IEnumerable<Type> AppliedDecoratorTypes { get; }
IEnumerable<object> AppliedDecorators { get; }
object CurrentInstance { get; }
} Any parameters that are configured on the var parameter = new NamedParameter("parameter", "ABC");
var instance = container.Resolve<IDecoratedService>(parameter); // Parameter will be passed to all decorators and implementation type. As I mentioned in #824 (comment) I'm not sure this is actually a good idea but wanted to see if it was possible. I'll continue to post updates here and welcome all feedback. It's early stages and there is no support for open generic decorators but it should work the same way except with trickier runtime type binding validation. |
Hi! Great to hear that there is some action going on regarding decorator registrations 👍 Nice work! Two feedback points from my side:
?
Best regards, |
Looks like you made some good progress! Have a look at my comment #727 (comment) So, at registration time, I will look at some attribute on the Query class - maybe look for [Permission] attribute or something. If it is there, I will decorate the handler with the permission decorator. If not, no need for it, etc. I currently have this all working with open generics. I do not select decorators at resolution based on some condition - but rather at registration time. Here is an example usage:
|
Thanks for the feedback everyone. I have taken it on board and made some changes. I managed to remove the requirement for using builder.RegisterType<ImplementorA>().As<IDecoratedService>();
builder.RegisterDecorator<DecoratorA, IDecoratedService>(); When adding a registration in a child lifetime scope, if there is a decorator already registered in a parent scope, then it will be applied to the new registration. var builder = new ContainerBuilder();
builder.RegisterDecorator<DecoratorA, IDecoratedService>();
var container = builder.Build();
var scope = container.BeginLifetimeScope(b => b.RegisterType<ImplementorA>().As<IDecoratedService>());
var instance = scope.Resolve<IDecoratedService>();
Assert.IsType<DecoratorA>(instance); A decorator can be registered in a child lifetime scope and will only be applied to resolutions within that scope. var builder = new ContainerBuilder();
builder.RegisterType<ImplementorA>().As<IDecoratedService>();
var container = builder.Build();
var scope = container.BeginLifetimeScope(b => b.RegisterDecorator<DecoratorA, IDecoratedService>());
var scopedInstance = scope.Resolve<IDecoratedService>();
Assert.IsType<DecoratorA>(scopedInstance);
var rootInstance = container.Resolve<IDecoratedService>();
Assert.IsType<ImplementorA>(rootInstance); You can now register decorators using a lambda syntax as shown below, where builder.RegisterType<ImplementorA>().As<IDecoratedService>();
builder.RegisterDecorator<IDecoratedService>((c, p, i) => new DecoratorA(i)); I also introduced the same functionality for open generic decorators (with the exception of lambda based registrations). There is no need to call a var builder = new ContainerBuilder();
builder.RegisterGeneric(typeof(ImplementorA<>)).As(typeof(IDecoratedService<>);
builder.RegisterGenericDecorator(typeof(DecoratorA<>), typeof(IDecoratedService<>));
var container = builder.Build();
Assert.IsType<DecoratorA<int>>(container.Resolve<IDecoratedService<int>>()); The decorators (regular and open generic) work in conjunction with the existing relationship types like var builder = new ContainerBuilder();
builder.RegisterGeneric(typeof(ImplementorA<>)).As(typeof(IDecoratedService<>));
builder.RegisterGeneric(typeof(ImplementorB<>)).As(typeof(IDecoratedService<>));
builder.RegisterGenericDecorator(typeof(DecoratorA<>), typeof(IDecoratedService<>));
var container = builder.Build();
var services = container.Resolve<IEnumerable<IDecoratedService<int>>>();
Assert.Collection(
services,
s =>
{
Assert.IsType<DecoratorA<int>>(s);
Assert.IsType<ImplementorA<int>>(s.Decorated);
},
s =>
{
Assert.IsType<DecoratorA<int>>(s);
Assert.IsType<ImplementorB<int>>(s.Decorated);
}); Decorator services are tracked when added to the component registry so that when resolving a service decoration is only attempted if the service is known to be decorated. The new decorator support lives alongside the existing decorator implementation and there are no breaking changes in the API. You can test out the updates by grabbing the https://www.myget.org/F/autofac/api/v3/index.json I would love for people to take this for a test and drive and help lookout for edge cases. Depending on feedback I will merge the changes from the The https://github.com/autofac/Autofac/tree/decorators/test/Autofac.Test/Features/Decorators |
Why is this the case. Nearly 100% of my use of decorators are around open generics. |
That was a typo on my part @waynebrantley. It was meant to say there is no As you can see in the later examples open generic decorators are most certainly included and work in exactly the same way, with the exception of not having a registration method that uses a lambda to return the decorator. You use the existing |
What you have done here looks like it will make conditional registration much easier - now that we don't have to know the 'last one' in advance. Why no lambda for open generics? That is pretty much what I need for completely not needing additional code. For example, if the class that is the type for the open generic has an attribute of some type on it - I want to decorate it..otherwise I do not...etc. Is there a reason for not having the lambda? |
@waynebrantley I think you are referring to the lambda used to define the decorator condition and the open generic decorators definitely have that. This is where you would put your attribute based condition. builder.RegisterGenericDecorator(typeof(DecoratorA<>), typeof(IDecoratedService<>), context => context.ImplementationType.GetTypeInfo().GetCustomAttribute<Attribute>() != null); What they don't have is a lambda for declaring the actual decoration process, as the type of the instance that would be passed into the lambda is not know at the time of registration. In the example below for non open generic decorators, the instance parameter builder.RegisterDecorator<IDecoratedService>((c, p, i) => new DecoratorA(i)); You can't do that with open generic registrations because they are declared with |
What is needed is to send the types of the things that make up the openGeneric into the lambda. So a real example is this:
If the Am I making any sense? |
@waynebrantley You have access to the service type and implementation type via the decorator context and can extract the generic type arguments, look for attributes, or anything else that you like. |
I have pushed a pre-release |
I only had time to read the blog post, but I like it. Maybe always provide the initial object in the |
I, too, have read the blog post, and I like the feature! It implements exactly what we need. |
I'm not currently working on a project using Autofac, but from the blog post it sounds exactly like what I wanted back when I set up a couple of projects to use Autofac. Great job! |
@drauch I assume you mean the implementation instance that is initially available in the |
@alexmg this looks great. I will hook this into my CQRS project and see how it all ends up looking. I also have unit tests for the decorations - so I will exercise this against this new build. Thanks and I will be back in touch. |
Thanks @waynebrantley. That would be awesome. |
@alexmg Sorry for the delay. I have not 'tested' functionality - a bit stuck on the api. Here are the extensions I currently have to make this happen (based on your original code)
Here is example code that uses those extensions
Using your new code it would look like this:
This brings up a few issues
|
@alexmg Anything else you need? |
I've tested the Great work. |
@dotnetjunkie All this is of course around the CQRS pattern and I used your blog for inspiration several years ago. I would like to see the book and have some potentially really cool things I could offer for it. |
@dotnetjunkie Thank you. Knowing you have tested this for the book gives me confidence that the changes are working as expected. |
This is now available in the |
@alexmg Well, glad it is released - however, as you can see I had a few issues on how we could use your new API to accomplish what is necessary. Any comments? (Thanks for getting this great feature out) |
@waynebrantley You mentioned earlier you haven't actually tried the new API - have you tried it and still found problems? You posted some fairly complex scenarios with no repro, it might be worth trying the new system to see how it works. Worst case, you can still use the old syntax for the time being. If you find a specific shortcoming that we can repro and address, maybe file an issue with that? |
@tillig - I have not "run tests" or "run code" against the new API - because I cannot see a way to use it! My questions were about how to use the API - more about how does the current API you added help achieve the original goals. Once I could actually code the registrations - I can definitely run my existing test suite on it. The two examples I gave were
|
The registration for a decorator doesn't search assemblies. It decorates things you have registered in the container. What you put in the container is up to you. RegisterAssemblyTypes will scan the assemblies you pass it, but that is a separate thing from registering the decorators. Here's what I'd recommend: Look at the unit tests. The are some for decorating open generics and some without open generics. These show a pretty good range of usage of the new syntax. Register the base open generics services with assembly scanning as usual. Use the new syntax to attach the decorators using examples from the unit tests. Part of the help we need here might actually require a bit of spelunking on your part. We're doing our best to make things better, but it'd help if you could dive in the deep end, use intellisense and the available source, and hammer on it a bit. It may well turn out you can't do what you want or that it's hard; that's valuable feedback, too. If that doesn't work, you're welcome to leave that code up there and we can get to it in time, but it likely won't be soon, and I apologize up front for that. |
Ok, that makes sense...I was just coming at it from the "way I had to do it before". I will dig and report back. |
Still no compositor pattern support? |
Hi @ArtemBan. Unfortunately, composite pattern support did not make it across the line. I am still interested in supporting it though and will open a separate issue for that feature. |
Consider multiple service implementations. Since the decorator has to inject the decoratee itself (Meta won't work), how is it possible to get inherited metadata? Also, would be reasonable to inject the decoratee with Implicit Relationship Type? I often want to add cross-cutting concern for lazy, func or even child scoped support. In such case, the previous problem with Meta drops off. |
Another observation. Generic IDecoratorContext support. Something like public interface IDecoratorContext<TImplementation, TService> {
IReadOnlyList<TService> AppliedDecorators { get; }
IReadOnlyList<Type> AppliedDecoratorTypes { get; } // redundant?
TService CurrentInstance { get; }
} Sorry for flooding closed issue. |
If there's an enhancement you'd like I'd recommend opening a new issue. Include a description of the enhancement and some examples of how you imagine it'd be used. We can start the discussion on it from there. |
@khamza85 The It simply allows for this:
Instead of this:
It seemed worthwhile to make a common scenario a little easier to code. |
We have a failing unit test, currently skipped that points to #529 and #880. It uses the older keyed-decorator syntax, like this: public class DecoratingServiceThatHasDefaultImplementation
{
private readonly IContainer _container;
public DecoratingServiceThatHasDefaultImplementation()
{
const string from = "from";
var builder = new ContainerBuilder();
builder.RegisterType<Implementer1>().As<IService>().Named<IService>(from);
builder.RegisterDecorator<IService>(s => new Decorator(s), from);
_container = builder.Build();
}
[Fact(Skip = "Issue #529 => #880")]
public void InstanceWithDefaultImplementationIsDecorated()
{
var decorator = _container.Resolve<IService>();
Assert.IsType<Decorator>(decorator);
Assert.IsType<Implementer1>(((Decorator)decorator).Decorated);
}
} This still doesn't pass in the v6 codeline, but... I'm wondering if this is interesting at all to solve. If we change it to use the new syntax, it passes: public class DecoratingServiceThatHasDefaultImplementation
{
private readonly IContainer _container;
public DecoratingServiceThatHasDefaultImplementation()
{
const string from = "from";
var builder = new ContainerBuilder();
builder.RegisterType<Implementer1>().As<IService>().Named<IService>(from);
// This is the change:
builder.RegisterDecorator<Decorator, IService>();
_container = builder.Build();
}
[Fact]
public void InstanceWithDefaultImplementationIsDecorated()
{
var decorator = _container.Resolve<IService>();
Assert.IsType<Decorator>(decorator);
Assert.IsType<Implementer1>(((Decorator)decorator).Decorated);
}
} I'm thinking the best course would be to just drop this test. It isn't a problem with the new decorator syntax and, if we don't intend on fixing the old syntax, there's no value in trucking the test around being skipped forever. |
Agreed that dropping the test for the old syntax (but perhaps leaving in a test using the new syntax?). The old syntax doesn't use the improved decorator pipeline behaviour anyway, so should be strongly discouraged. |
I'll commit a test for this using the new syntax and remove the old test. I would wager we already have a test like this for the new stuff but... I'll look. |
There are several issues around how we handle decorators and enhancements that are desired in that space. Updating the syntax and adding features sort of go hand-in-hand since, for the most part, it'll all require some changes to the internals and will likely introduce some overall breaking changes.
This issue is to round up discussions about what we want decorator syntax to look like and what additional features we want to support. After that's determined, let's get it done. Fixing up decorator syntax has been a long time coming.
Issues:
.As<T>
and.AsImplementedInterfaces
correctly for decoration targetsI think @alexmg had a spike going with some ideas on an updated decorator syntax. I'll let him chime in as able. In the meantime, I'll close these other issues and we can handle it here.
The text was updated successfully, but these errors were encountered: