ipjohnson / Grace

Grace is a feature rich dependency injection container library
MIT License
336 stars 33 forks source link

Cannot register implementations with key #211

Closed davidkeaveny closed 5 years ago

davidkeaveny commented 5 years ago

I am having problems registering multiple implementations of the same interface with Grace 6.4.2 on .NET Framework 4.7.2. If I have the following initialisation:

var container = new DependencyInjectionContainer();
container.Configure(c =>
{
    c.Export<FirstRegisteredImplementation>().As<IRegisteredInterface>();
    c.Export<SecondRegisteredImplementation>().As<IRegisteredInterface>();

    c.ExportAsKeyed<FirstMultiRegisteredImplementation, IMultiRegisteredInterface>("First");
    c.ExportAsKeyed<SecondMultiRegisteredImplementation, IMultiRegisteredInterface>("Second");

    c.Export<FirstMultiRegisteredImplementation>().AsKeyed<IMultiRegisteredInterface>("First");
    c.Export<SecondMultiRegisteredImplementation>().AsKeyed<IMultiRegisteredInterface>("Second");
});

var registrations = container.WhatDoIHave(true);
Console.WriteLine(registrations);

then the first two registrations work successfully, but whether I use the longer-winded .Export<TImpl>().AsKeyed<TInt>(object key); or the helper method .ExportAsKeyed<TImpl, TInt>(object key);, I have no registrations for IMultiRegisteredInterface, and when I call .LocateAll(), I get an empty collection. Am I doing something wrong in my registration?

davidkeaveny commented 5 years ago

I noticed that if I register without using keys, then .LocateAll<TInt>() works as expected.

ipjohnson commented 5 years ago

Hi @davidkeaveny

I'll take a look this evening, I have an idea what's happening but I want to confirm my theory.

ipjohnson commented 5 years ago

So there are to different things happening here.

The second point was a design decision because my thought was that if an export was registered as keyed it would be located by a key not part of a collection.

I'm curious what the use case is where you want to use it as keyed and as part of the collection.

ipjohnson commented 5 years ago

@davidkeaveny did that answer your question? Is there anything I can do?

davidkeaveny commented 5 years ago

Hi,

What I am doing at the moment is trying to transition my application away from Unity 2.1; to that end I created an IContainer interface with basic Resolve and ResolveAll methods to support our current application, created Unity and Grace implementations, and wrote a bunch of unit tests to ensure that both container implementations produced the same results. So I was hoping that Grace's registration/resolution approach would be similar enough to Unity, which wants multiple implementations registered to a single interface to be named. The unit tests are what exposed the issue.

ipjohnson commented 5 years ago

Hmmm interesting issue, let me think about it a bit. I can probably make this work.

eglauko commented 5 years ago

There could be an optional parameter in the LocateAll method to determine the key. By default, it would be null, keeping the current behavior. To find all services, with and without a key, there could be a special key, a singleton object from Grace's library, something like: LocateAllBehavior.IncludeKeyedExports.

davidkeaveny commented 5 years ago

That would work for me 👍

ipjohnson commented 5 years ago

Hi Guys, I was thinking of having a configuration property that dictates if keyed instances are returned as part of an IEnumerable<T>. The idea being when you flip the switch the container acts more like unity?

The other option would be to add a parameter object withKey = null. Which option would make more sense?

uygary commented 5 years ago

Just putting in my two cents, feel free to completely ignore me: This is bit of an iffy change from my perspective.

On one hand, if something is registered with a key, that would mean it will be resolved with a key. Otherwise having the concept of key becomes pointless. It has to be a resolution identifier, or it would become an intrinsic property of the object that is being stored outside the object for no good reason. Having multiple types or instances registered under the same key is also not a valid case from my perspective. I believe that's the use case for child containers, not keys.

I simply don't like the idea of making design choices simply to have "behavioral compatibility" with another IOC framework. However, if the decision is made towards that goal, a configuration property seems the semantically correct choice. I suspect having more granularity by means of having a parameter on the resolution method might come in handy for some folks who deal with some edge cases, I can't deny that possibility. But I'm just wondering what actual real-world use cases actually warrant that kind of a requirement other than having separate services within the same application for the purpose of inspecting the containers.

ipjohnson commented 5 years ago

@uygary you raise some very interesting points and I think you hit the nail on the head with this statement

"On one hand, if something is registered with a key, that would mean it will be resolved with a key. Otherwise having the concept of key becomes pointless. It has to be a resolution identifier, or it would become an intrinsic property of the object that is being stored outside the object for no good reason."

That said in the past I've been open to making compatability changes if they are opt in and don't require much code change (hence the desire to make it an opt in property).

ipjohnson commented 5 years ago

@eglauko and @davidkeaveny I've pushed build 7.0.0-Beta743 to nuget with a new scope configuration property ReturnKeyedInEnumerable. Let me know how it works.

eglauko commented 5 years ago

I did some tests here and everything ran perfectly.

ipjohnson commented 5 years ago

I'm going to close this out and will release 7.0 probably in 2-3 weeks.