Partially decorating service implementing multiple interfaces throws

[blalonde]

Autofac 4.9.1

When a service implements 2 interfaces and only one is decorated, it fails.

public interface IDecorable
{
	
}

public interface IService
{

}

public class Service : IDecorable, IService
{

}

public class Decorator : IDecorable
{
	public Decorator(IDecorable service)
	{
	}
}

[Fact]
public void Test()
{
	var builder = new ContainerBuilder();
	builder.RegisterType<Service>().As<IDecorable>().As<IService>();
	builder.RegisterDecorator<Decorator, IDecorable>();

	var container = builder.Build();
	var decoratedService = container.Resolve<IDecorable>();
	var nondecoratedService = container.Resolve<IService>(); // fails Unable to cast object of type 'Decorator' to type 'IService'.
}

Failure : throws exception in comment

I assumed getting IDecorable would retrieve Service decorated by Decorator and retrieving IService would get same instance of Service not decorated

[tillig]

When you say "fails," is it an exception or is it failing to decorate the interface? What do you see vs. what do you expect?

[tillig]

Hmmm, this is an interesting case. I think your expectation sounds right, that you should be able to decorate a particular interface but not have the system assume every interface on the registration is decorated.

As a quick workaround, you could probably separate the registration:

builder.RegisterType<Service>().As<IDecorable>();
builder.RegisterType<Service>().As<IService>();
builder.RegisterDecorator<Decorator, IDecorable>();

But that doesn't solve the problem, it just works around it. We'll have to see what we can do about this.

[jonathansapp]

I'm also having an issue that I believe is related.

I have a class that closes an interface implementation twice. When wrapped with a generic decorator, only one of the two closed interfaces can be resolved:

public class UnitTest1
{
    [Fact]
    public void CanResolveUseCaseImplementations()
    {
        // Arrange
        var builder = new ContainerBuilder();
        builder.RegisterType<MyServiceImpl>().AsImplementedInterfaces();
        var container = builder.Build();

        // Act
        container.Resolve<IUseCase<MyServiceImpl.ConfirmRequest>>();
        container.Resolve<IUseCase<MyServiceImpl.Request>>();
    }

    [Fact]
    public void CanResolveFirstDecoratedService()
    {
        // Arrange
        var builder = new ContainerBuilder();
        builder.RegisterType<MyServiceImpl>().AsImplementedInterfaces();
        builder.RegisterGenericDecorator(typeof(ProfilerDecorator<>), typeof(IUseCase<>));
        var container = builder.Build();

        // Act
        container.Resolve<IUseCase<MyServiceImpl.ConfirmRequest>>();
    }


    [Fact]
    public void CanResolveSecondDecoratedService()
    {
        // Arrange
        var builder = new ContainerBuilder();
        builder.RegisterType<MyServiceImpl>().AsImplementedInterfaces();
        builder.RegisterGenericDecorator(typeof(ProfilerDecorator<>), typeof(IUseCase<>));
        var container = builder.Build();

        // Act
        container.Resolve<IUseCase<MyServiceImpl.Request>>();
    }

    public interface IRequest { }

    public interface IUseCase<TRequest> 
        where TRequest : class, IRequest
    {
        void Execute(TRequest request);
    }


    public class MyServiceImpl : IUseCase<MyServiceImpl.ConfirmRequest>, 
                                 IUseCase<MyServiceImpl.Request>
    {
        public void Execute(Request request) { }
        public void Execute(ConfirmRequest request) { }
        public class ConfirmRequest : IRequest { }
        public class Request : IRequest { }
    }

    public class ProfilerDecorator<TRequest> : IUseCase<TRequest> 
        where TRequest : class, IRequest
    {
        private readonly IUseCase<TRequest> inner;
        public ProfilerDecorator(IUseCase<TRequest> inner) => this.inner = inner;
        public void Execute(TRequest request) { }
    }
}

CanResolveSecondDecoratedService is failing with:

Message: System.InvalidCastException : Unable to cast object of type 
'ProfilerDecorator`1[AutofacBugTest.UnitTest1+MyServiceImpl+ConfirmRequest]' to type 
'IUseCase`1[AutofacBugTest.UnitTest1+MyServiceImpl+Request]'.

[tillig]

Yeah, that sounds like the same thing - the decorator is trying to decorate the whole registration rather than just the appropriate service.

[tillig]

I've definitely been able to repro this and noticed in our unit tests that the decorated interface was inheriting from the base service interface like this:

public interface IService { }
public interface IDecoratedService : IService { }

What that means is that if you have a decorator that implements IDecoratedService it automatically already had all the IService stuff in it, so there was no casting problem.

Removing that relationship revealed the problem noted above. Now to see what to do about it.

[tillig]

Hooooo boy.

I figured out why this isn't working but I'm not sure how to fix it without some non-trivial internal changes. Granted, some of those changes may be well worthwhile to do, but it could involve breaking interfaces.

The short version: By the time the decorator is getting chosen, we know which registration is being resolved, but not which service on that registration. That means a single registration exposing multiple services will try to decorate all of them.

Longer explanation with details (in which I won't blame you if you fall asleep 😴 ):

When you resolve a component from an IComponentContext (like a container or a lifetime scope) two things happen:

1. The service (e.g., the interface type you want to resolve) gets looked up in the component registry. That should get a registration which is basically the mapping of underlying concrete type (MyServicImpl) to the set of interfaces you registered (IService, IOtherService).
2. The component context is told to resolve that registration. The information, at that point, about the exact service you're resolving is dropped.

Later, deep in that chain of resolution, is basically where Autofac constructs (activates) all the concrete objects and handles decoration.

Again, by the time we get way, way down into the stack there, we've long since lost the information about exactly which service (interface) the caller was trying to resolve so the approach is just to decorate whatever comes out of the whole registration. If that registration has multiple services exposed but only one of them is decorated, you'll get the exception because the decorator is trying to decorate anything coming out there, not just the one service.

Hmmm.

I think the solution here is to actually create a sort of ResolveContext object that gets passed around during resolution that has all the information we need, sort of like how HttpContext has all the request/response info during a web request.

The challenge with switching to that is it'll affect a lot of public interfaces. For example, IComponentContext would change from

public interface IComponentContext
{
  IComponentRegistry ComponentRegistry { get; }
  object ResolveComponent(IComponentRegistration registration, IEnumerable<Parameter> parameters);
}

to

public interface IComponentContext
{
  IComponentRegistry ComponentRegistry { get; }
  object ResolveComponent(ResolveContext context);
}

That's a pretty big breaking change. It'd affect resolution extension methods and likely some integration packages that we both do and don't own. IComponentContext is one of the most prominent interfaces we have. (ILifetimeScope and IContainer both inherit IComponentContext and any resolution operations run through that ResolveComponent method... so this would break ILifetimeScope and IContainer, too.)

It may also affect memory usage and perf, but we'd have to benchmark it to see.

I think that'd be a positive change because it would allow us to add data to the resolve pipeline that we didn't previously have; and adding/changing properties in that context object would be possible without breaking interfaces. I think there are quite a few things we could make better if we had all the information at all levels of the operation.

But... it's not a five minute fix, so I won't be able to drop a quick 0.0.1 bug fix on this one. It's bigger than that.

Again, the workaround appears to be separating the registrations:

builder.RegisterType<Service>().As<IDecorable>();
builder.RegisterType<Service>().As<IService>();
builder.RegisterDecorator<Decorator, IDecorable>();

Which sucks, and isn't as pretty, and it makes some things harder, and I'm truly sorry about that. I think this would be a great thing to target for a 5.0 release where we can do some breaking changes.

[blalonde]

I managed to read through... I don't quite know the inner workings of autofac, but maybe another approach would be to decorate the registration in step 1, where you resolve it from the component registry and you know which service is being resolved.

Simple suggestion, don't know if that makes any sense to you.

[tillig]

I slept on this and realized something at like 3AM when all the important thoughts hit: This may not be fixable at all.

It has to do with component lifetimes.

Just for future readers of this chain...

• A component is the underlying implementation of the thing. In a RegisterType<T>, this is the T.
• A service is the interface the component is exposing. When you register As<X>, this is the X.
• A component can expose one or more services.
• The lifetime is associated with the component, not the service.

In all the examples we've talked about so far, every component registration has been instance-per-dependency: Ask for an instance, get a new one.

var builder = new ContainerBuilder();
builder.RegisterType<Component>();
var container = builder.Build();
var a = container.Resolve<Component>();
var b = container.Resolve<Component>();
Assert.NotSame(a, b);

In that case, it makes total sense that something like this should work:

var builder = new ContainerBuilder();
builder.RegisterType<Component>()
       .As<IService1>()
       .As<IService2>();
builder.RegisterDecorator<Decorator, IService1>();
var container = builder.Build();
var a = container.Resolve<IService1>();
var b = container.Resolve<IService2>();
Assert.IsInstanceOf<Decorator>(a);
Assert.IsInstanceOf<Component>(b);

That is, when you ask for it as an IService1 it gets decorated because you registered a decorator for IService1, but when you ask for it as an IService2 it's not decorated because you didn't register a decorator for that.

However, let's say Component is a singleton.

var builder = new ContainerBuilder();
builder.RegisterType<Component>()
       .SingleInstance();
var container = builder.Build();
var a = container.Resolve<Component>();
var b = container.Resolve<Component>();
Assert.Same(a, b);

Now you want to do the decoration again.

var builder = new ContainerBuilder();
builder.RegisterType<Component>()
       .As<IService1>()
       .As<IService2>()
       .SingleInstance();
builder.RegisterDecorator<Decorator, IService1>();
var container = builder.Build();
var a = container.Resolve<IService1>();
var b = container.Resolve<IService2>();

// This should still pass! It's a singleton!
Assert.Same(a, b);

// What type T in here would make this true?
Assert.IsInstanceOf<T>(a);
Assert.IsInstanceOf<T>(b);

For the singleton case, the same component instance needs to fulfill all of the services. Which means if you want to decorate a singleton... the decorator also needs to fulfill all the services.

In the above unit test illustration, the T either has to be Component or Decorator since it's the same object instance... however, a was resolved as IService1 and b is IService2, so whatever T is, it also needs to be both IService1 and IService2.

This same logic holds for instance-per-lifetime-scope (instance-per-request), ExternallyOwned, and other situations where an instance of a component may need to be shared across multiple resolve calls.

I can imagine a really, really complex system of dynamic proxies and/or interceptors where we basically generate a hybrid wrapper for objects that uses the decorator for methods in the decorated service but passes through to the undecorated methods for things that aren't decorated, but that could get really complicated really quick, especially when you start layering decorators together.

To be honest, I'm not really interested in building or maintaining that level of complexity and potential unreliability for this case.

Basically, the only option I see here is to make the behavior and reasoning behind it more clear and make it easier to troubleshoot.

• Clarify the exception message if possible to explain why it came up so it's not just an invalid cast.
• Document this on the decorator page to ensure people know about the gotcha and how to work around it (separating registrations).

I still think the ResolveContext idea is solid, but I don't think it'll help in this case. We'd still have the issue where a component with a shared lifetime can't be both decorated and not decorated at the same time.

Sort of... Schrodinger's Decorator, if you will.

[jonathansapp]

With generics, it appears as if the issue is predicated upon the registration order of the component's services and/or the order of interface declaration in the service implementation body.

[Fact]
public void ResolveSecondDecoratedService_Fails1()
{
    // Arrange
    var builder = new ContainerBuilder();
    builder.RegisterType<MyServiceImpl>().AsImplementedInterfaces().InstancePerLifetimeScope();
    builder.RegisterGenericDecorator(typeof(ProfilerDecorator<>), typeof(IUseCase<>));
    var container = builder.Build();

    // Act
    container.Resolve<IUseCase<MyServiceImpl.Request>>();
}

[Fact]
public void ResolveSecondDecoratedService_Fails2()
{
    // Arrange
    var builder = new ContainerBuilder();
    builder.RegisterType<MyServiceImpl>().As<IUseCase<MyServiceImpl.ConfirmRequest>>()
        .As<IUseCase<MyServiceImpl.Request>>()
        .InstancePerLifetimeScope();

    builder.RegisterGenericDecorator(typeof(ProfilerDecorator<>), typeof(IUseCase<>));
    var container = builder.Build();

    // Act
    container.Resolve<IUseCase<MyServiceImpl.Request>>();
}

[Fact]
public void ResolveSecondDecoratedService_Succeeds()
{
    // Arrange
    var builder = new ContainerBuilder();
    builder.RegisterType<MyServiceImpl>().As<IUseCase<MyServiceImpl.ConfirmRequest>>().InstancePerLifetimeScope();
    builder.RegisterType<MyServiceImpl>().As<IUseCase<MyServiceImpl.Request>>().InstancePerLifetimeScope();

    builder.RegisterGenericDecorator(typeof(ProfilerDecorator<>), typeof(IUseCase<>));
    var container = builder.Build();

    // Act
    container.Resolve<IUseCase<MyServiceImpl.Request>>();
}

If I switch the implementation order of MyServiceImpl such that instead of:

public class MyServiceImpl : IUseCase<MyServiceImpl.ConfirmRequest>, 
                             IUseCase<MyServiceImpl.Request>
{
    public void Execute(Request request) { }
    public void Execute(ConfirmRequest request) { }
    public class ConfirmRequest : IRequest { }
    public class Request : IRequest { }
}

we have

public class MyServiceImpl : IUseCase<MyServiceImpl.Request>, 
                             IUseCase<MyServiceImpl.ConfirmRequest>
{
    public void Execute(Request request) { }
    public void Execute(ConfirmRequest request) { }
    public class ConfirmRequest : IRequest { }
    public class Request : IRequest { }
}

Then ResolveSecondDecoratedService_Fails1 now succeeds and CanResolveFirstDecoratedService starts to fail. Interestingly, ResolveSecondDecoratedService_Fails2 continues to fail until I change the service registration order to:

builder.RegisterType<MyServiceImpl>().As<IUseCase<MyServiceImpl.Request>>()
  .As<IUseCase<MyServiceImpl.ConfirmRequest>>()
  .InstancePerLifetimeScope();

I'm finding it hard to correlate the reasoning behind the issues of the concrete decorator with that of the generic one. Thank you for taking time to address these issues.

[alexmg]

Just catching up on the action here. Early on when developing the feature I created a test that was trying to resolve the implementation type directly and checking that it wasn't decorated.

[Fact(Skip ="Cannot currently determine requested resolve service type")]
public void DecoratedRegistrationCanIncludeImplementationType()
{
    var builder = new ContainerBuilder();
    builder.RegisterType<ImplementorA>().As<IDecoratedService>().AsSelf();
    builder.RegisterDecorator<DecoratorA, IDecoratedService>();
    var container = builder.Build();
    Assert.IsType<ImplementorA>(container.Resolve<ImplementorA>());
}

It never sat right that the one decorated component registration could exhibit different runtime behaviour depending on how you asked it to be resolved. The singleton instance is the perfect example, because if the logic in the decorated component assumed singleton behaviour, having one decorated instance and one non-decorated instance breaks that assumption. Unfortunately, I completely forgot about this while getting all the other features across the line and never came back to revist what the right behaviour should be.

I think it would be possible for InstancePerDependency services to conditionally apply the decorator and that was something that I actually looked at. I got as far as making all the required changes to get the resolved service type down to the resolve operation but it was a breaking change and there was one place where this was especially obvious. That spot was the ResolveComponent method on the base IComponentContext interface that accepts the IComponentRegistration directly. It assumes that some other mechanism has taken the requested service and figured out what IComponentRegistration needs to be resolved.

public object ResolveComponent(IComponentRegistration registration, IEnumerable<Parameter> parameters)

I was torn between updating that method to get the extra information further down into the resolution process and possibly causing a massive breaking change. It doesn't seem like a common method to use directly, but it has been around since the very early days, so it's inevitable that some people will be using it one way or another. I'm sure I stashed those changes and might take another look at them to see if anything else comes back. If a breaking change is required then going down the context route might be nice as it should be easier to add things to it without introducing breaking changes.

I'm still not sure that having a component that can be decorated or not depending on the service it is resolved by is a good idea. There is support in the new decorators to conditionally apply decorators that handles scenarios where the decorator chain might need to be adjusted based on runtime information.

[alexmg]

You are right about registration order playing a role in all this @jonathansapp.

If the service order is swapped around it changes the behaviour. The example below no longer throws an exception because the decorator is not applied. Resolving IService will return Service, and resolving IDecorable will also return Service.

var builder = new ContainerBuilder();
builder.RegisterType<Service>().As<IService>().As<IDecorable>();
builder.RegisterDecorator<Decorator, IDecorable>();
var container = builder.Build();

var foo = container.Resolve<IService>();
var bar = container.Resolve<IDecorable>();

It seems that not all services are considered when it comes to looking for decorators and that is indeed a bug. I found the problem and with the fix in place the order does not matter. You will receive the InvalidCastException when resolving IService regardless of the order. I'll get that issue sorted out first so that it is easier to reason about what is going on.

[blalonde]

For the singleton case, I tend to disagree with tillig's reasoning. I feel it would be natural as such :

var builder = new ContainerBuilder();
builder.RegisterType<Component>()
       .As<IService1>()
       .As<IService2>()
       .SingleInstance();
builder.RegisterDecorator<Decorator, IService1>();
var container = builder.Build();
var a = container.Resolve<IService1>();
var b = container.Resolve<IService2>();

// This should still pass! It's a singleton!
Assert.Same(a.**Inner**, b);

// What type T in here would make this true?
Assert.IsInstanceOf<**Decorator**>(a);
Assert.IsInstanceOf<**Component**>(b);

In essence,
resolving IService1 would generate Decorator decorating Component
resolving IService2 would generate Component

Both Component would be the same single instance, but Decorator would only wrap Component when resolving IService1.

[alexmg]

The fix for the decorator only being applied if the decorator interface was the first service assigned to the registration is included in 4.9.2 on NuGet.

[multiarc]

@tillig I think it's reasonable to break perfect theoretical model of Component and Services to support Service level decoration in order to give real flexibility. It just needs to be obvious and documented (which it is by the nature of decoration).
It's not that bad that we would have two different instances (Component + Decorator), since we still have a link to original instance of Component along with it's original unchanged lifetime which would be the same as Decorator object lifetime.
Existence of multiple instances expected behavior for the decorator anyways.

[alexmg]

I have refactored the decorator implementation to use Service on the new ResolveRequest and this is partially addressed. When resolving for InstancePerDependency you will receive two different instances. As it stands now, for the InstancePerLifetimeScope and SingleInstance scenarios, the first service to be resolved wins due to the caching mechanism that is in place for these lifetime scopes. I'm not sure with the refactoring if it will be easier to treat these as separate components and if that is even an appropriate thing to do.

[alexmg]

This has been fixed in 5.1.0 which is now on NuGet.

[blalonde]

Greate job guys ! thanks.