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

Decorator is not applied when component is registered by interface #999

Closed
horse315 opened this issue Jun 27, 2019 · 6 comments
Closed

Decorator is not applied when component is registered by interface #999

horse315 opened this issue Jun 27, 2019 · 6 comments

Comments

@horse315
Copy link

Hi!

I'm trying to decorate a delegate component, created with a factory method, which returns an interface. I've discovered that in this case delegate is not applied.
Is it a bug or a feature?

Autofac 4.9.2

    /// <summary>
    /// package add package Autofac --version 4.9.2
    /// </summary>
    public class DecoratorTest
    {
        public interface IWork { }

        public class Work : IWork { }

        public class WorkDecorator : IWork { }

        [Fact] // fails
        public void RegisterInterface()
        {
            var containerBuilder = new ContainerBuilder();

            // interface
            containerBuilder.Register(c => (IWork)new Work()) // <== or Register<IWork>(c => new Work())
                .As<IWork>()
                .InstancePerDependency();

            // decorator
            containerBuilder.RegisterDecorator<IWork>((c, p, d) => new WorkDecorator());

            var container = containerBuilder.Build();

            var work = container.Resolve<IWork>();

            Assert.IsType<WorkDecorator>(work);
        }

        [Fact] // passes
        public void RegisterClass()
        {
            var containerBuilder = new ContainerBuilder();

            // instance
            containerBuilder.Register(c => new Work()).As<IWork>()
                .InstancePerDependency();

            // decorator
            containerBuilder.RegisterDecorator<IWork>((c, p, d) => new WorkDecorator());

            var container = containerBuilder.Build();

            var work = container.Resolve<IWork>();

            Assert.IsType<WorkDecorator>(work);
        }
    }
@tillig
Copy link
Member

tillig commented Jun 27, 2019

Before we take the time to try and set this up, repro it, debug it, etc... the only difference I see in these two things is in the first one, you're registering a lambda and casting it inside the lambda...

Register(c => (IWork)new Work())

...while the second one there's no cast.

Register(c => new Work())

In both cases they're registered As<IWork> and are InstancePerDependency. The decorators are also registered the same.

Am I reading that right?

@horse315
Copy link
Author

@tillig , yes

@tillig
Copy link
Member

tillig commented Jun 28, 2019

Looks like we have a bug! Thanks for the report. I'm able to reproduce this. Not sure how to fix it right now, but baby steps.

@tillig
Copy link
Member

tillig commented Jun 28, 2019

Added failing (but skipped, for now) unit test here.

@RaymondHuy
Copy link
Member

@tillig I think that the reason is inside this condition:
https://github.com/autofac/Autofac/blob/develop/src/Autofac/Features/Decorators/InstanceDecorator.cs#L44
This condition "registration.Activator.LimitType != instanceType" may cause bug if we have multiple types returned inside lamda expression when we register component. For example:

builder.Register(ctx => {
if (condition)
return (IDecoratedService)new ImplementorA();
else return new ImplementorA();
})

The LimitType always be the most specific type returned in the expression (in this case is IDecoratedService) but the instance type can only be determined at runtime. Therefore it results to that all registered decorators will be ignored. My solution is to remove this condition. I have re-run all test cases after removing it and they all passed. What do you think about it or do you have any better solutions :) ?

@alexmg alexmg closed this as completed in 41d3503 Jul 18, 2019
@alexmg
Copy link
Member

alexmg commented Jul 18, 2019

The IsAdapting check seems to now be covering all the required cases. I have removed this and can committed the change. It will be included in the next release.

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

No branches or pull requests

4 participants