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

Is variance supported when registering collection with registrations? #626

Open
voroninp opened this Issue Oct 30, 2018 · 21 comments

Comments

2 participants
@voroninp

voroninp commented Oct 30, 2018

I am trying to register multiple implementations of a contravarinat interface IBeforeCommitDomainEventHandler<in TEvent>, but get exception:

Test method threw exception:
System.ArgumentException: The supplied type IBeforeCommitDomainEventHandler<DomainEvent> does not implement IBeforeCommitDomainEventHandler<BeforeCommitEventsDispatcherTests.ChildDomainEvent>.
Parameter name: registrations
at SimpleInjector.Requires.ServiceIsAssignableFromImplementations(Type serviceType, IEnumerable<Type> typesToRegister, String paramName, Boolean typeCanBeServiceType)
at SimpleInjector.ContainerCollectionRegistrator.Register(Type serviceType, IEnumerable<Registration> registrations)

However, ChildDomainEvent is actually a subclass of DomainEvent.

Here's the code which fails:

var container = new Container();
container.Options.DefaultScopedLifestyle = new AsyncScopedLifestyle();

var handler1Events = new List<DomainEvent>();
var handler2Events = new List<DomainEvent>();

var handler1Mock = new Mock<IBeforeCommitDomainEventHandler<DomainEvent>>();
handler1Mock.Setup(h => h.HandleAsync(It.IsAny<object>()))
    .Callback((object e) => handler1Events.Add((DomainEvent)e))
    .Returns(Task.CompletedTask);

var handler2Mock = new Mock<IBeforeCommitDomainEventHandler<ChildDomainEvent>>();
handler1Mock.Setup(h => h.HandleAsync(It.IsAny<object>()))
    .Callback((object e) => handler2Events.Add((DomainEvent)e))
    .Returns(Task.CompletedTask);

var reg1 = Lifestyle.Scoped.CreateRegistration(() => handler1Mock.Object, container);
var reg2 = Lifestyle.Scoped.CreateRegistration(() => handler2Mock.Object, container);

container.Collection.Register<IBeforeCommitDomainEventHandler<ChildDomainEvent>>(new[]{reg1, reg2});
            
@dotnetjunkie

This comment has been minimized.

Collaborator

dotnetjunkie commented Oct 30, 2018

Variant typing certainly is supported. However, you unfortunately ran into a bug.

This bug only manifests itself in case you make the registration using Registration types, where the implementation is unknown to Simple Injector.

You make the following call:

Lifestyle.Scoped.CreateRegistration<IBeforeCommitDomainEventHandler<DomainEvent>>(() => ...,
    container)

Note how I removed type inference so you can now see what call is made.

Because of this, the Registration.ImplementationType property will become typeof(IBeforeCommitDomainEventHandler<DomainEvent>).

Unfortunately, due to this bug, Simple Injector does not match the types correctly. I will try to fix this bug in the next minor release v4.4.

However, the registration you are making is quite unusual, which is why nobody has noticed this before. Typically, Simple Injector is in control over the registered implementations. For instance, the following code doesn't have this problem:

var container = new Container();

container.Collection.Register(typeof(IBeforeCommitDomainEventHandler<>),
    typeof(IBeforeCommitDomainEventHandler<>).Assembly);

In this case, a set of implementation types is registered, which allows Simple Injector to correctly do the type check. Not only would this work around your problem, it:

  • Leads to simpler code
  • allows the IBeforeCommitDomainEventHandler<DomainEvent> to be part of an IEnumerable<IBeforeCommitDomainEventHandler<DomainEvent>> without having to explicitly register it.
@voroninp

This comment has been minimized.

voroninp commented Oct 30, 2018

I have a question about your example.

If my service is already registered as implementation of ISomeService and it happens to implement multiple IBeforeCommitDomainEventHandler<> (closed) interfaces, how many different instances will SimpleInjector create?

And in this case I need to regsiter collection against instances dynamically provided by Moq.

@dotnetjunkie

This comment has been minimized.

Collaborator

dotnetjunkie commented Oct 30, 2018

I don't understand your question. Can you provide a small example?

The other thing that confuses me is that you use a container in a unit test? What is the function of using the container in a test? What are you testing here?

@voroninp

This comment has been minimized.

voroninp commented Oct 30, 2018

=D

My intention is to decouple domain events generation and handling.

Just before transaction is commited we need to dispatch all events to the handlers.
I'd like to avoid explicit subscription (and related lifetime headache), therefore I decided to use marker interface IBeforeCommitDomainEventHandler<>. I resolve all closed instances with the help of container. (Yes, here it is a Service Locator).

Every time Dispatcher.DispatchAsync(TEventBase @event) is called, container is asked for all instances of type:

var handlerType = typeof(IBeforeCommitDomainEventHandler<>).MakeGenericType(@event.GetType())

I want some domain services to be handlers as well:

public class DeviceService: IDeviceService, IBeforeCommitDomainEventHandler<DeviceRenamed>
{
   ...
}

DeviceService is explicitly registered as implementation of IDeviceService.
Now let's do a batch registration of handlers:

container.Collection.Register(typeof(IBeforeCommitDomainEventHandler<>),
    typeof(IBeforeCommitDomainEventHandler<>).Assembly);

I want to know whether container returns same instance of DeviceService.

I use container in unit tests of dispatcher.

@dotnetjunkie

This comment has been minimized.

Collaborator

dotnetjunkie commented Oct 30, 2018

Yes, here it is a Service Locator

As long as you have the logic that calls into the container inside your Composition Root, rest asure that you are not applying the Service Locator anti-pattern. In other words, place your Dispatcher implementation inside the Composition Root and you're fine.

I want to know whether container returns same instance of DeviceService.

You can very easily test this by calling Container.Verify(). In case the collection registration for DeviceService gets a different lifestyle (the default is Transient for collections), Verify will fail.

Here's a simple test to check this:

var container = new Container();

container.Register<IDeviceService, DeviceService>(Lifestyle.Singleton);

container.Collection.Register(typeof(IBeforeCommitDomainEventHandler<>), this.GetType().Assembly);

container.Verify();

Running this example produces the following exception:

The configuration is invalid. The following diagnostic warnings were reported:
-[Ambiguous Lifestyles] The registration for IDeviceService (Singleton) maps to the same implementation (DeviceService) as the registration for IBeforeCommitDomainEventHandler<DeviceRenamed> (Transient) does, but the registration maps to a different lifestyle. This will cause each registration to resolve to a different instance.

In other words, no this does not work.

Reason for this is that Register.Collection will look up a registration for given concrete type to use its lifestyle, but falls back to the Options.DefaultLifestyle (which defaults to Transient). The Register.Collection call, however, can't find that registration, because there is only a registration for IDeviceService—not for DeviceService.

So to fix this, you can add the following registration:

container.Register<DeviceService>(Lifestyle.Singleton);

Here's a simple test to verify this:

var container = new Container();

container.Register<DeviceService>(Lifestyle.Singleton);
container.Register<IDeviceService, DeviceService>(Lifestyle.Singleton);

container.Collection.Register(typeof(IBeforeCommitDomainEventHandler<>), this.GetType().Assembly);

container.Verify();

Now verify succeeds.

Do do a double check, (in case you don't trust Simple Injector), you can add the following code:

Assert.AreSame(
    container.GetInstance<IDeviceService>(),
    container.GetAllInstances<IBeforeCommitDomainEventHandler<DeviceRenamed>>().Single());

This Assert statement will succeed.

@dotnetjunkie

This comment has been minimized.

Collaborator

dotnetjunkie commented Oct 30, 2018

I use container in unit tests of dispatcher.

Ahh, I see. You are testing your dispatcher. A simpler registration would be the following:

var container = new Container();
container.Options.DefaultScopedLifestyle = new AsyncScopedLifestyle();

var handler1Events = new List<DomainEvent>();
var handler2Events = new List<DomainEvent>();

var handler1Mock = new Mock<IBeforeCommitDomainEventHandler<DomainEvent>>();
handler1Mock.Setup(h => h.HandleAsync(It.IsAny<object>()))
    .Callback((object e) => handler1Events.Add((DomainEvent)e))
    .Returns(Task.CompletedTask);

var handler2Mock = new Mock<IBeforeCommitDomainEventHandler<ChildDomainEvent>>();
handler1Mock.Setup(h => h.HandleAsync(It.IsAny<object>()))
    .Callback((object e) => handler2Events.Add((DomainEvent)e))
    .Returns(Task.CompletedTask);

container.Collection.Register<IBeforeCommitDomainEventHandler<ChildDomainEvent>>(
    new[] { handler1Mock.Object, handler2Mock.Object });

But do note that I would not advice any of those two options. Problem with both approaches is that you are not testing anything useful at all. Your dispatches will most likely be a few lines of code, and the thing that you want to test is whether the application's real configuration is dispatched correctly by the dispatcher.

So instead, prefer creating an integration test that pulls the dispatcher from the application's real container, and use that to verify whether one or two handlers are handled.

@voroninp

This comment has been minimized.

voroninp commented Oct 30, 2018

Integration testing is the next step. This unit test is mostly to check my assumptions about SimpleInjector.

I'll try scoped festyle tomorrow, hope it works:

container.Register<DeviceService>(Lifestyle.Scoped);
container.Register<IDeviceService, DeviceService>(Lifestyle.Scoped);

Frankly speaking, I think this "double" registration may be somewhat confusing to the maintainers: Why would anyone need to addiitonaly register concrete type, if second line is usually sufficient?

So my plan is to scan for all concrete types which implement IBeforeCommitDomainEventHandler<> interface, then to create registrations for not yet registered types and to reuse registrations of already registered types. That's why I tried this overload:

container.Collection.Register<IBeforeCommitDomainEventHandler<ChildDomainEvent>>(new[]{reg1, reg2})
@dotnetjunkie

This comment has been minimized.

Collaborator

dotnetjunkie commented Oct 31, 2018

I think this "double" registration may be somewhat confusing

I can agree, so if you choose this approach, it might be good to add a code comment to explain this. Independently of which container you pick, however, you will always have these kinds of 'quirks'. Every design choice that simplifies one thing, will influence another.

The reason that the Register.Collection method tries to find an explicit concrete registration is to be able to handle the following use case (from the docs):

container.Register<ILogger, FileLogger>();
container.Register<MailLogger>(Lifestyle.Singleton);

container.Collection.Register<ILogger>(
    typeof(MailLogger), // Becomes singleton because of Register<MailLogger>
    typeof(SqlLogger),
    typeof(ILogger)); // Maps to FileLogger because of Register<ILogger, FileLogger>

In this example there are practically two registrations:

  • A registration for ILogger that will return a Transient instance of FileLogger
  • A registration for an IEnumerable<ILogger> (a collection of loggers) that returns a Singleton MailLogger, a Transient SqlLogger, and a Transient FileLogger.

Technically it would be possible for Simple Injector to go even one step further and go look for the matching concrete type, even if it is registered through one of its abstractions, as is the case with your DeviceService, that would be tricky without causing a breaking change. And the question becomes: how to handle ambuigity? For instance, what if there is an Register<IA, DeviceService>(Lifestyle.Scoped) and Register<IB, DeviceService>(Lifestyle.Singleton) registration. I know, this would be really ugly, and verification would throw an exception on that. Verification, however, can be deliberately skipped, or this particular error can be suppressed. Now the question becomes, which lifestyle should Register.Collection pick? Whatever comes first? Should it fall back on the default lifestyle? That would violate this rule. Should we throw? Better, perhaps. But how to prevent Simple Injector from throwing?

All in all not an easy fix.

But there are alternative registration approaches you might want to consider.

For instance, if all registrations are by default Scoped, you can just make Scoped the default lifestyle:

container.Options.DefaultLifestyle = new AsyncScopedLifestyle();

This allows you to simplify things as follows:

container.Register<IDeviceService, DeviceService>();
container.Collection.Register(typeof(IBeforeCommitDomainEventHandler<>), assemblies);

Or you can keep transient the default but override Simple Injector's lifestyle selection behavior:

container.Options.LifestyleSelectionBehavior = new MyOwnLifestyleSelectionBehavior(
    type =>type.Name.EndsWith("Service") ? Lifestyle.Scoped : Lifestyle.Transient);

With MyOwnLifestyleSelectionBehavior being implemented as:

public class MyOwnLifestyleSelectionBehavior : ILifestyleSelectionBehavior {
    private readonly Predicate<Type> selector;
    public MyOwnLifestyleSelectionBehavior(Predicate<Type> selector) => this.selector = selector;
    public Lifestyle SelectLifestyle(Type implementationType) => selector(implementationType);
}
@voroninp

This comment has been minimized.

voroninp commented Oct 31, 2018

Technically it would be possible for Simple Injector to go even one step further and go look for the matching concrete type, even if it is registered through one of its abstractions

But isn't it achieved via explicitly creating registrations of a concrete type and then telling the container that this registration implements ISomeInterface1, ISomeInterface2 and so on?


Let me describe my mental model of DI container.

  • We have Interfaces (in general sense)/Services and Implementors.
  • Implementor can implement multiple Interfaces.
  • Interface/Service can be implemented by multiple Implementors.

What are the use cases?

  • Define the lifetime for the concrete instance being resolved for particular Interface.
  • Define how many instances of a concrete Implementor can exist in the context of a given lifestyle.

Example:

public class Foo: IBar1, IBar2 {}

I want both IBar1 and IBar2 be singletons, so every class which depends on IBar1 always gets the same instance A. And class dependent on IBar2 gets insnatnce B.
I still want to be able to decide, where A and B refer to the same instance of class Foo or there are two separate instances.
But what if we add IBar3 to the equation, should instance of C equal to A or to B?
If we put it to extremes we may need to say:

// no IBar3 here, separate instance is created for IBar3
container.ShareImplementorBetweenServices(typeof(IBar1), typeof(IBar2)); 

Then comes the question of sane defaults:

Let's consider APIs:

Container.RegisterImplementorAndAllItsServices<TImplementor>(Lifestyle servicesLifestyle);
Container.RegisterSharedImplementorAndAllItsServices<TImplementor>(Lifestyle servicesLifestyle);

In case of transient lifestyle these calls are equivalent, but I would prohibit the second call for transient lifestyle, because semantics of sharing in not applicable.
In case of scoped and singleton lifestyle calls differ: Shared means A and B and C are same instances.


Couple of words about collections.

For me collections are just the natural result of registering multiple implementors of the interface

public class Foo1 : IBar {}
public class Foo2: IBar {}

container.RegisterImplementorAndAllItsServices<Foo1>(Lifestyle.Scoped);
container.RegisterImplementorAndAllItsServices<Foo2>(Lifestyle.Scoped);
container.Verify();

IBar has multiple registered implementors. So we already have a collection. We would even have a collection, if only Foo1 was regsitered. This would be a collection containing one item.

(Moreover, if different lifestiles were used, we'd have IBar mapped to Foo1 for one lifestyle and to
'Foo2' for the other one.)

public class GiveMeBar {
    public GiveMeBar(IEnumerable<IBar> bars) {}
}

If we register GiveMeBar as transient it gets all IBars: transient, scoped, and singletons.
If we register it as scoped - only scoped and singletons.

The real problem is ambiguity!
Wich instance to create when class depends on single instance of IBar?
I think this can be defined in settings: first wins, last wins, it is an ambiguity and container throws on Verfiy() unless ambiguity is resolved explicitly:

container.PreferImplmentor<IBar, Foo1>(LifeStyle.Scoped);
container.PreferImplementor<IBar, Foo3>(LifeStyle.Singleton);

I agree, abmiguity resolution is hard.

Final core API:

public class Container
{
    public Implementor Implementor(Type implementorType) {...}
    // Real regsitration of (concrete type, service it implements, lifestyle)
    public RegisterService(Service service) {...}
    public void ShareImplementorInstanceBetweenServices(IEnumerable<ScopedService> services) {...}
    public void ShareImplementorInstanceBetweenServices(IEnumerable<SingletonService> services) {...}
    public void PreferredImplementations(IEnumerable<Service> services) {..}
}

public class Implementor 
{
     public SingletonService AsSingletonService(Type serviceType);
     public TransientService AsTransientService(Type serviceType);
     public ScopedService AsScopedService(Type serviceType);
}

public class Service {}
public class SingletonService : Service {}
public class TransientService : Service {}
public class ScopedService : Service {}

Sure, this is a naive view which lacks consideration of thousand other caseses, but the API (I hope) does not leave much space for alternative and contradictory interpretations.
I find it confusing that most of DI frameworks have some implicit defaults and behaviours which are not clear from API calls.

@voroninp

This comment has been minimized.

voroninp commented Oct 31, 2018

This test works fine, but I am confused even more =)

[TestMethod]
public async Task Test()
{
    var container = new Container();

    var handler1Events = new List<DomainEvent>();
    var handler2Events = new List<DomainEvent>();

    var handler1Mock = new Mock<IBeforeCommitDomainEventHandler<DomainEvent>>();
    handler1Mock.Setup(h => h.HandleAsync(It.IsAny<object>()))
        .Callback((object e) => handler1Events.Add((DomainEvent)e))
        .Returns(Task.CompletedTask);

    var handler2Mock = new Mock<IBeforeCommitDomainEventHandler<ChildDomainEvent>>();
    handler2Mock.Setup(h => h.HandleAsync(It.IsAny<object>()))
        .Callback((object e) => handler2Events.Add((DomainEvent)e))
        .Returns(Task.CompletedTask);

    container.Collection.Register(handler1Mock.Object);
    container.Collection.Register(handler2Mock.Object);

    var dispatcher = new BeforeCommitEventsDispatcher(container);

    var event1 = new DomainEvent();
    var event2 = new ChildDomainEvent();

    await dispatcher.DispatchAsync(event1);
    await dispatcher.DispatchAsync(event2);

    handler1Events.Should().BeEquivalentTo(event1, event2);
    handler2Events.Should().BeEquivalentTo(event2);
}

Injector resolves both handlers for ChildDomainEvent. ...

@dotnetjunkie

This comment has been minimized.

Collaborator

dotnetjunkie commented Oct 31, 2018

Injector resolves both handlers for ChildDomainEvent. ...

Please show the code of BeforeCommitEventsDispatcher.

@voroninp

This comment has been minimized.

voroninp commented Oct 31, 2018

public interface IBeforeCommitDomainEventsDispatcher<in TEventBase>
{
    Task DispatchAsync(TEventBase @event);
}

public interface IBeforeCommitDomainEventHandler
{
    Task HandleAsync(object @event);
}

public interface IBeforeCommitDomainEventHandler<in TEvent> : IBeforeCommitDomainEventHandler
{

}

public abstract class BeforeCommitDomainEventsDispatcher<TEventBase> : IBeforeCommitDomainEventsDispatcher<TEventBase>
{
    protected abstract IReadOnlyCollection<IBeforeCommitDomainEventHandler> GetHandlers(TEventBase @event);

    public async Task DispatchAsync(TEventBase @event)
    {
        Guard.ArgumentNotNull(@event, nameof(@event));

        var handlers = GetHandlers(@event) ?? Array.Empty<IBeforeCommitDomainEventHandler>();

        foreach (var handler in handlers.Where(h => !(h is null)))
        {
            var task = handler.HandleAsync(@event) ?? Task.CompletedTask;
            await task;
        }
    }
}

public class BeforeCommitEventsDispatcher: BeforeCommitDomainEventsDispatcher<DomainEvent>
{
    private readonly Container _container;

    public BeforeCommitEventsDispatcher(Container container)
    {
        Guard.ArgumentNotNull(container, nameof(container));

        _container = container;
    }

    private static readonly ConcurrentDictionary<Type, Type> HandlerTypes = new ConcurrentDictionary<Type, Type>();


    protected override IReadOnlyCollection<IBeforeCommitDomainEventHandler> GetHandlers(object @event)
    {
        Guard.ArgumentNotNull(@event, nameof(@event));

        var eventType = @event.GetType();
        var handlersType = HandlerTypes.GetOrAdd(
            eventType,
            t => typeof(IBeforeCommitDomainEventHandler<>).MakeGenericType(t));

        var handlers = _container.GetAllInstances(handlersType).Cast<IBeforeCommitDomainEventHandler>().ToList();

        return handlers;
    }
}
@dotnetjunkie

This comment has been minimized.

Collaborator

dotnetjunkie commented Oct 31, 2018

Injector resolves both handlers for ChildDomainEvent. ...

What is the behavior you are expecting here?

@voroninp

This comment has been minimized.

voroninp commented Oct 31, 2018

Exactly this behavior.

But I got from your explanation, that I have to explicitly map interfaces and concrete types. In other words I had to explcictly register handler1Mock.Object as IBeforeComminEventHandler<DomainEvent> and 'IBeforeComminEventHandler<ChildDomainEvent>.

@dotnetjunkie

This comment has been minimized.

Collaborator

dotnetjunkie commented Oct 31, 2018

No, that's not the case. Even when registering a closed type directly, Simple Injector will place all registrations of its open-generic type in the same bucket, and will later make sure all compatible registrations are put in a collection.

@voroninp

This comment has been minimized.

voroninp commented Nov 2, 2018

It would be nice, by the way, to have an option to specify collection of services as optional, that is it will be empty, if no service is registered.

Now injector throws on GetAllInstances, if no registrations were made for collection.

@dotnetjunkie

This comment has been minimized.

Collaborator

dotnetjunkie commented Nov 2, 2018

You can just call:

container.Options.ResolveUnregisteredCollections = true;

But TBH, we decided to block resolving unregistered collections by default since the introduction of v3, because we saw that this behavior let to confusion.

Out of curiosity, why do you think auto-resolve unregistered collections is "nice"? Is that just for testing purposes, or do you have a specific use case (for your actual production code) where this is beneficial?

@voroninp

This comment has been minimized.

voroninp commented Nov 2, 2018

It's same task, imagine:

public class Notifier
{
    public Notifier([Optional] IEnumerable<INotfificationHandler>> handlers)
    {
    }
}

Why should I require here the existence of registered handlers?

@dotnetjunkie

This comment has been minimized.

Collaborator

dotnetjunkie commented Nov 2, 2018

Let me turn it around: why shouldn't you require a registration? What's tge problem of explicitly stating there are 0 handlers?

@voroninp

This comment has been minimized.

voroninp commented Nov 2, 2018

Because I want implcit autowiring.
Suppose, I defined this class in a separate assembly and provided continer integration package.

Notifier.RegisterHandlersWith(continer, assembliesToScan);

In this method I search for types implementing interface INotificationHandler and register collection.
In this approach developer does not need to register type explcicitly, implementing interface is sufficient.
Delepoer does not have to go into specifics of particular container.
It would just work out of the box.

update
Ok, I can register 0 handlers explicitly in this method, but I did not expect this is gona work. It's not evident behaviour: registration of zero handlers != no registration at all.

@dotnetjunkie

This comment has been minimized.

Collaborator

dotnetjunkie commented Nov 2, 2018

The whole idea is that you explicitly make a collection registration, even if there are 0. This typically works when doing auto-registration, where you're not sure there there are any implementation. Especially when dealing with generic interfaces you'll see that some closed versions have no implementations.

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