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

RegisterDecorator does not play nicely when decorating items registered with .As<IFoo>() or with .AsImplementedInterfacs() #529

Closed
JonathanMaccollum opened this Issue May 30, 2014 · 7 comments

Comments

Projects
None yet
4 participants
@JonathanMaccollum
Copy link

JonathanMaccollum commented May 30, 2014

Let’s say at container builder “build time” we wanted to use configuration to turn on or off a particular decorator.

Test structures:

    public interface IFoo
    {
    }
    public class Foo:IFoo
    {

    }
    public class FooDecorator : IFoo
    {
        private readonly IFoo _instance;

        public FooDecorator(IFoo instance)
        {
            if (instance == null) throw new ArgumentNullException("instance");
            _instance = instance;
        }
    }

Prior to using RegisterDecorator to accomplish this the code would look like this:

        bool turnDecoratorOn = true; // value from configuration
        var cb = new ContainerBuilder();
        cb.RegisterType<Foo>()
            .As<IFoo>() // this is the default implementation of IFoo to be used of the decoration is not enabled.
            .Named<IFoo>("Foo"); 
        if (turnDecoratorOn)
        {
            cb.RegisterType<FooDecorator>()
                .WithParameter(
                    (pi, ctx) => pi.ParameterType == typeof (IFoo),
                    (pi, ctx) => ctx.ResolveNamed<IFoo>("Foo")) // resolve the IFoo parameter with the one registered with name of 'Foo'
                .As<IFoo>();
        }
        using (var container = cb.Build())
        {
            var foo = container.Resolve<IFoo>();
            Assert.IsInstanceOfType(foo, typeof(FooDecorator));
        }

And if the config value had it turned off it would test successfully like this:

        bool turnDecoratorOn = false;
        var cb = new ContainerBuilder();
        cb.RegisterType<Foo>()
            .As<IFoo>() // this is the default implementation of IFoo to be used of the decoration is not enabled.
            .Named<IFoo>("Foo"); 
        if (turnDecoratorOn)
        {
            cb.RegisterType<FooDecorator>()
                .WithParameter(
                    (pi, ctx) => pi.ParameterType == typeof(IFoo),
                    (pi, ctx) => ctx.ResolveNamed<IFoo>("Foo"))
                .As<IFoo>();
        }
        using (var container = cb.Build())
        {
            var foo = container.Resolve<IFoo>();
            Assert.IsInstanceOfType(foo, typeof(Foo));
        }

The reason for this is because using the .As() or .AsImplementedInterfaces() sets up a ‘default value’ for anything that asks for IFoo unless it is over-written by additional registrations that occur on the containerbuilder afterwards.

However, in attempts to make this work with RegisterDecorator my decorated component was not resolved and instead my default component was always resolved:

                 bool turnDecoratorOn = true;
        var cb = new ContainerBuilder();
        cb.RegisterType<Foo>()
            .As<IFoo>() // this is the default implementation of IFoo to be used of the decoration is not enabled.
            .Named<IFoo>("Foo"); 
        if (turnDecoratorOn)
        {
            cb.RegisterDecorator<IFoo>(
                instance => new FooDecorator(instance), "Foo")
                .As<IFoo>();
        }
        using (var container = cb.Build())
        {
            var foo = container.Resolve<IFoo>();
            Assert.IsInstanceOfType(foo, typeof(FooDecorator));
        }

Basically RegisterDecorator is not over-writing the default registration for the un-named type ‘IFoo’ even though we are registering our decorator it with the .As() statement.

If we remove the .As from the default registration it works as expected when turnDecoratorOn is true

                 bool turnDecoratorOn = true;
        var cb = new ContainerBuilder();
        cb.RegisterType<Foo>()
            .As<IFoo>() // this is the default implementation of IFoo to be used of the decoration is not enabled.
            .Named<IFoo>("Foo"); 
        if (turnDecoratorOn)
        {
            cb.RegisterDecorator<IFoo>(
                instance => new FooDecorator(instance), "Foo")
                .As<IFoo>();
        }
        using (var container = cb.Build())
        {
            var foo = container.Resolve<IFoo>();
            Assert.IsInstanceOfType(foo, typeof(FooDecorator));
        }

…however now it will not resolve when turnDecoratorOn is false, because IFoo by itself is never registered:

        bool turnDecoratorOn = false;
        var cb = new ContainerBuilder();
        cb.RegisterType<Foo>()
            .As<IFoo>() // this is the default implementation of IFoo to be used of the decoration is not enabled.
            .Named<IFoo>("Foo"); 
        if (turnDecoratorOn)
        {
            cb.RegisterDecorator<IFoo>(
                instance => new FooDecorator(instance), "Foo")
                .As<IFoo>();
        }
        using (var container = cb.Build())
        {
            var foo = container.Resolve<IFoo>();
            Assert.IsInstanceOfType(foo, typeof(Foo));
        }
@tillig

This comment has been minimized.

Copy link
Contributor

tillig commented Sep 22, 2014

The reason this happens is the same reason as #272.

Individual component registrations are processed differently than registration source registrations. When you use RegisterDecorator what it does behind the scenes is add a registration source specifically for that one adapter. It doesn't add a direct component registration.

When you resolve types, the component registrations always get processed first, before the registration sources. This happens because in nearly every case you actually want that to happen - registration sources are what support things like the Func<T> and IEnumerable<T> relationships, and you want your individual component overrides to run before the built-in stuff.

In your particular case, what I'd say you want to do is get a little fancier with your if block.

bool turnDecoratorOn = false;
var cb = new ContainerBuilder();
var mainRegistration = cb.RegisterType<Foo>().Named<IFoo>("Foo");
if(turnDecoratorOn)
{
  cb.RegisterDecorator<IFoo>(instance => new FooDecorator(instance), "Foo")
    .As<IFoo>();
}
else
{
  mainRegistration.As<IFoo>();
}

Notice what I'm doing is conditionally adding the .As<IFoo>() in there. Instead of requiring an override, in this case you just decide whether or not to even add the service type at all.

Obviously that's not optimal, but if you follow the #272 chain, you'll see the real fix for this is pretty non-trivial.

For now I'm going to close this - there's a workaround for this specific issue, and the root cause is actually #272.

@tillig tillig closed this Sep 22, 2014

@snboisen

This comment has been minimized.

Copy link

snboisen commented Jan 11, 2017

Now that #272 has been fixed, this issue should have gone away, right?
I'm currently using Autofac 4.2.1, but I still see this exact problem.

@tillig

This comment has been minimized.

Copy link
Contributor

tillig commented Jan 11, 2017

@snboisen Good point. We ran into this peripherally in #824. I'll reopen this and we can see what we can do. At a minimum, a failing unit test to be sure we don't regress this as it gets fixed.

@tillig tillig reopened this Jan 11, 2017

@alexandrnikitin

This comment has been minimized.

Copy link
Contributor

alexandrnikitin commented Jan 12, 2017

It's sad that the #541 fix (and its test) wasn't merged 😞 It's still actual by the way.
@tillig do you want me to rebase it?

@tillig

This comment has been minimized.

Copy link
Contributor

tillig commented Jan 12, 2017

@alexandrnikitin No, but it may be a starting point for the fix here. The ComponentRegistry class really shouldn't know special cases about specific registration sources like LightweightAdapterRegistrationSource. The fix really needs to allow for Liskov's substitution principle to work - all sources treated the same.

alexandrnikitin added a commit to alexandrnikitin/Autofac that referenced this issue Jan 12, 2017

@alexandrnikitin

This comment has been minimized.

Copy link
Contributor

alexandrnikitin commented Jan 12, 2017

@tillig Yeah, that fix doesn't look nice and follows just one principle to get things done 😄
I'm sorry I don't follow Autofac development progress for a couple of years already. Some parts were changed as I see. I'll try to dive deeper on the weekend. Meanwhile a test #826

alexandrnikitin added a commit to alexandrnikitin/Autofac that referenced this issue Jan 20, 2017

tillig added a commit that referenced this issue Jan 20, 2017

@tillig

This comment has been minimized.

Copy link
Contributor

tillig commented Oct 27, 2017

We're pushing to enhance decorator syntax and features, which will hopefully resolve this issue. I've created a larger meta-issue for tracking that at #880. In the meantime, I'll close this specific issue and hopefully we can handle everything at once.

@tillig tillig closed this Oct 27, 2017

@tillig tillig reopened this Feb 6, 2018

@tillig tillig closed this Feb 6, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment