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

Parameters don't propagate to the target of a decorator/adapter #824

Closed
brunoklein99 opened this issue Jan 5, 2017 · 6 comments
Closed

Comments

@brunoklein99
Copy link

The following snippet outputs ServiceCImpl instead of the expected ServiceCDecorator. Is this a bug? If not, how could I add a decorator around ServiceCImpl in this context (factory delegate)?

class Program
{
    static void Main(string[] args)
    {
        var builder = new ContainerBuilder();

        builder.RegisterType<ServiceCImpl>().Named<IServiceC>("service").As<IServiceC>();

        builder.RegisterDecorator<IServiceC>(inner => new ServiceCDecorator(inner), fromKey: "service");

        var container = builder.Build();

        var factory = container.Resolve<Func<IServiceA, IServiceB, IServiceC>>();

        var service = factory(new ServiceAImpl(), new ServiceBImpl());
        
        Console.WriteLine(service.GetType().Name);

        Console.ReadLine();
    }

    private interface IServiceA
    {
    }

    private class ServiceAImpl : IServiceA
    {
    }

    private interface IServiceB
    {
    }

    private class ServiceBImpl : IServiceB
    {
    }

    private interface IServiceC
    {
    }

    private class ServiceCImpl : IServiceC
    {
        private readonly IServiceA _serviceA;
        private readonly IServiceB _serviceB;

        public ServiceCImpl(IServiceA serviceA, IServiceB serviceB)
        {
            if (serviceA == null) throw new ArgumentNullException(nameof(serviceA));
            if (serviceB == null) throw new ArgumentNullException(nameof(serviceB));
            _serviceA = serviceA;
            _serviceB = serviceB;
        }
    }

    private class ServiceCDecorator : IServiceC
    {
        private readonly IServiceC _inner;

        public ServiceCDecorator(IServiceC inner)
        {
            if (inner == null) throw new ArgumentNullException(nameof(inner));
            _inner = inner;
        }
    }
}

@tillig
Copy link
Member

tillig commented Jan 5, 2017

Try changing this line...

builder.RegisterType<ServiceCImpl>().Named<IServiceC>("service").As<IServiceC>();

...to this:

builder.RegisterType<ServiceCImpl>().Named<IServiceC>("service");

Which is to say, only make the named service registration. (Note that's how it's shown in the documentation, as well.)

@brunoklein99
Copy link
Author

brunoklein99 commented Jan 5, 2017

I get an exception then.

"None of the constructors found with 'Autofac.Core.Activators.Reflection.DefaultConstructorFinder' on type 'AutofacDelegateTest.Program+ServiceCImpl' can be invoked with the available services and parameters:\r\nCannot resolve parameter 'IServiceA serviceA' of constructor 'Void .ctor(IServiceA, IServiceB)'."

Looks like it is not using the delegate parameters to compose the instance.

@tillig
Copy link
Member

tillig commented Jan 5, 2017

The problem appears to be that the parameters being passed by the Func<X, Y, B> are handed to the decorator rather than the thing being decorated.

More specifically, the order of operations is:

  • You resolve a factory delegate Func<IServiceA, IServiceB, IServiceC>
  • Autofac creates a delegate that does this:
    • See the IServiceC is a decorator.
      • Resolve the inner IServiceC which is the ServiceCImpl.
      • Hand the ServiceCImpl and the set of parameters back to the decorator creation.
    • Execute a delegate that looks like (ctx, parm, inner) => new ServiceCDecorator(inner) where the parm enumerable is the set of parameters you passed to the Func<IServiceA, IServiceB, IServiceC>.

The challenge here is that you're trying to pass parameters to something in the middle of the resolve chain - which sort of implies that you know the backing type of IServiceC to begin with rather than letting the DI do the work. If you switched ServiceCImpl to be something else, the parameters you'd be passing would have no effect (as you're seeing now) or would cause a hard to debug situation.

Here's a more complete example that shows a bit more tracing and a way you can see the parameters as they get passed. I also show how, if you don't mind resolving the inner object twice, you can get the parameters to pass down the stack.

using System;
using System.Linq;
using Autofac;

namespace ConsoleApplication1
{
  class Program
  {
    static void Main(string[] args)
    {
      var builder = new ContainerBuilder();

      builder.RegisterType<ServiceAImpl>().As<IServiceA>();
      builder.RegisterType<ServiceBImpl>().As<IServiceB>();
      builder.RegisterType<ServiceCImpl>().Named<IServiceC>("service");

      builder.RegisterDecorator<IServiceC>((ctx, parm, inner) =>
      {
        foreach (var p in parm.Cast<TypedParameter>())
        {
          Console.WriteLine("Parameter: {0}", p.Type);
        }

        var i = ctx.ResolveNamed<IServiceC>("service", parm.ToArray());
        return new ServiceCDecorator(i);
      }, fromKey: "service");

      var container = builder.Build();

      var factory = container.Resolve<Func<IServiceA, IServiceB, IServiceC>>();

      var a = new ServiceAImpl();
      Console.WriteLine("Parameter A: {0}", a.Id);
      var b = new ServiceBImpl();
      Console.WriteLine("Parameter B: {0}", b.Id);

      var service = factory(a, b);

      Console.WriteLine("Decorator Type: {0}", service.GetType().Name);
      Console.WriteLine("Service A: {0}", service.ServiceA.Id);
      Console.WriteLine("Service B: {0}", service.ServiceB.Id);

      Console.ReadLine();
    }

    private interface IServiceA
    {
      Guid Id { get; }
    }

    private class ServiceAImpl : IServiceA
    {
      public ServiceAImpl()
      {
        this.Id = Guid.NewGuid();
      }

      public Guid Id { get; set; }
    }

    private interface IServiceB
    {
      Guid Id { get; }
    }

    private class ServiceBImpl : IServiceB
    {
      public ServiceBImpl()
      {
        this.Id = Guid.NewGuid();
      }

      public Guid Id { get; set; }
    }

    private interface IServiceC
    {
      IServiceA ServiceA { get; }
      IServiceB ServiceB { get; }
    }

    private class ServiceCImpl : IServiceC
    {
      public ServiceCImpl(IServiceA serviceA, IServiceB serviceB)
      {
        Console.WriteLine("Creating ServiceCImpl");
        if (serviceA == null) throw new ArgumentNullException(nameof(serviceA));
        if (serviceB == null) throw new ArgumentNullException(nameof(serviceB));
        this.ServiceA = serviceA;
        this.ServiceB = serviceB;
      }

      public IServiceA ServiceA { get; set; }
      public IServiceB ServiceB { get; set; }
    }

    private class ServiceCDecorator : IServiceC
    {
      private readonly IServiceC _inner;

      public ServiceCDecorator(IServiceC inner)
      {
        if (inner == null) throw new ArgumentNullException(nameof(inner));
        _inner = inner;
      }
      public IServiceA ServiceA { get { return this._inner.ServiceA; } }
      public IServiceB ServiceB { get { return this._inner.ServiceB; } }
    }
  }
}

The thing is... I'm not entirely sure if this is a bug or not. I can see both sides:

  • When you're passing the parameters to a resolve operation those shouldn't go to every resolve in the whole chain. That doesn't make sense and can create a lot of trouble.
  • Adapters and decorators feel like they should allow the parameters to pass through because they, themselves, aren't necessarily the base target of the resolution.

I do see that we don't pass parameters to decorators or to adapters.

IIRC, @alexmg was noodling with some potential changes to make adapters and decorators easier to use. I'm curious if he has any thoughts on what should happen. I sort of lean toward the parameters getting passed down to the child resolve operation.

In the meantime, I'm going to update the title of the issue to be more about parameters making it through decorator/adapter operations.

@tillig tillig changed the title Possible bug with decorators? Parameters don't propagate to the target of a decorator/adapter Jan 5, 2017
@alexmg
Copy link
Member

alexmg commented Jan 9, 2017

I agree that passing the parameters to anything other than the inner most component is dangerous. The decorators tend to represent cross-cutting concerns and should be transparent to the caller. As @tillig mentioned, even the parameters required by the inner most implementation type really shouldn't be known to the caller when working against an abstraction. Having the parameters passed to components all the way through the decorator chain would make the abstraction leak further and could result in hard to find bugs. If parameters are to be supported they should be simply forwarded through to the inner most component.

@tillig
Copy link
Member

tillig commented Jan 11, 2017

Note the issue with As<IServiceC>() appears to be #529, which I thought was resolved but appears is not. I'm not sure if we need to reopen that potentially.

@tillig
Copy link
Member

tillig commented Apr 10, 2018

Fix released in v4.7.0.

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

3 participants