ipjohnson / Grace

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

Import Keyed injects wrong instance #181

Closed scovel closed 6 years ago

scovel commented 6 years ago

Multiple Keyed (Named?) Instances. Constructor wants a specific key. Grace injects the wrong instance. Am I doing something wrong? See //returns Mongo below...

.Net Core (NetStandar 2.0) Grace 6.4.0

using System; using Grace.DependencyInjection; using Grace.DependencyInjection.Attributes;

namespace test { class Program { static void Main(string[] args) { DatabaseFactory SqlServer = new DatabaseFactory() {name = "SqlServer"}; DatabaseFactory Oracle = new DatabaseFactory() {name = "Oracle"}; DatabaseFactory Mongo = new DatabaseFactory() {name = "Mongo"};

        var container = new DependencyInjectionContainer();
        container.Configure(c =>
        {
            c.ExportInstance(SqlServer).As<IDatabaseFactory>().AsKeyed<IDatabaseFactory>(SqlServer.name);
            c.ExportInstance(Oracle).As<IDatabaseFactory>().AsKeyed<IDatabaseFactory>(Oracle.name);
            c.ExportInstance(Mongo).As<IDatabaseFactory>().AsKeyed<IDatabaseFactory>(Mongo.name);
            c.Export<TestDal>().As<ITestDal>();
        });

        IDatabaseFactory dbFactory = container.Locate<IDatabaseFactory>(withKey: "SqlServer");
        if (dbFactory.name != "SqlServer") //works
            throw new Exception("wrong dbFactory");

        var testDal = container.Locate<ITestDal>();

        if (testDal.DatabaseFactory.name != "SqlServer") //returns Mongo
            throw new Exception("wrong database in Dal");

    }
}

public interface IDatabaseFactory
{
    string name { get; set; }
}

public class DatabaseFactory : IDatabaseFactory
{
    public string name { get; set; }
}

public interface ITestDal
{
    IDatabaseFactory DatabaseFactory { get; }
}

public class TestDal : ITestDal
{
    private IDatabaseFactory _databaseFactory;

    public TestDal([Import(Key = "SqlServer")] IDatabaseFactory techXpress)
    {
        _databaseFactory = techXpress;
    }

    public IDatabaseFactory DatabaseFactory
    {
        get { return _databaseFactory; }
    }
}

}

ipjohnson commented 6 years ago

Hi @scovel

The reason it's returning the mongo is instance is because it's not actually processing the import attribute and picking the last registered instance of IDatabaseFactory. This could be argued to be a bug because it seems like it should throw an export not found rather than picking a keyed export. I'm hesitant to change the behavior in a non major release because even though it's not technically correct it seems like a major behavior shift to introduce in a minor version release.

The assumption I made when creating the container is that people would use either the fluent interface or the attributes for configuration. I hadn't thought of the idea of using fluent for part of the registration and attributes for the other.

That said you're the second person recently that ran into problems with this exact use case. I think maybe the answer is to add a flag to force the container to process Import attributes on parameters even when doing fluent validation.

scovel commented 6 years ago

Ian,

Thanks yet again for your quick response!

This is a pattern that is used in MS Unity, “named instances.” I suspect the other guy was a Unity user like myself in a past life…

Can you give me an example of how to make it work just using attributes? I’d give it a try. I’d prefer to use the code I sent you, but if there is a better/working way, I’d give it a go. I’d definitely vote for adding that feature to Grace.

I agree, it should either fail with an error, or work properly. As it stands it does inject something, but the wrong something. Luckily I’m a suspicious guy and wrote a test app with unit tests to prove out using Grace, so the behavior showed up in a unit test.

Again, thanks for the quick response, and all your hard work on Grace.

Sean

P.S. My son’s name is Ian, which Scottish for John, and Sean is Irish for John.

From: Ian Johnson notifications@github.com Reply-To: ipjohnson/Grace reply@reply.github.com Date: Wednesday, August 1, 2018 at 8:16 AM To: ipjohnson/Grace Grace@noreply.github.com Cc: Sean Covel SCOVEL@travelers.com, Mention mention@noreply.github.com Subject: Re: [ipjohnson/Grace] Import Keyed injects wrong instance (#181)

Hi @scovelhttps://github.com/scovel

The reason it's returning the mongo is instance is because it's not actually processing the import attribute and picking the last registered instance of IDatabaseFactory. This could be argued to be a bug because it seems like it should throw an export not found rather than picking a keyed export. I'm hesitant to change the behavior in a non major release because even though it's not technically correct it seems like a major behavior shift to introduce in a minor version release.

The assumption I made when creating the container is that people would use either the fluent interface or the attributes for configuration. I hadn't thought of the idea of using fluent for part of the registration and attributes for the other.

That said you're the second person recently that ran into problems with this exact use case. I think maybe the answer is to add a flag to force the container to process Import attributes on parameters even when doing fluent validation.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/ipjohnson/Grace/issues/181#issuecomment-409554654, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AYMQXo2H6QRpEujLmcwSL7yjkxPQ3VHpks5uMZwWgaJpZM4Voi_a.


This message (including any attachments) may contain confidential, proprietary, privileged and/or private information. The information is intended to be for the use of the individual or entity designated above. If you are not the intended recipient of this message, please notify the sender immediately, and delete the message and any attachments. Any disclosure, reproduction, distribution or other use of this message or any attachments by an individual or entity other than the intended recipient is prohibited.

TRVDiscDefault::1201

ipjohnson commented 6 years ago

@scovel for the moment I think it's much easier to make a slight change to the fluent registration to specify the key than set up registration using attributes.

c.Export<TestDal>().As<ITestDal>().WithCtorParam<IDatabaseFactory>().LocateWithKey("SqlServer");

I'll see if I can find some time this weekend to add the flag as it's shouldn't much work and I'll release a beta for you to try.

That's very cool about your son, very good name :)

scovel commented 6 years ago

That kinda works…

If I add just the one registration, it works.

c.Export().As().WithCtorParam().LocateWithKey("SqlServer");

If I add all 3, it goes back to picking Mongo.

c.Export().As().WithCtorParam().LocateWithKey("Oracle"); c.Export().As().WithCtorParam().LocateWithKey("Mongo");

Let me describe the use-case so we’re on the same page.

We have a “framework” that is used by our developers to handle most cross-cutting concerns. (we have a lot of less experienced developers, in a different time-zone…). One thing the framework does is read a config file for database connection information and add them to the container.

DATASOURCE_CONFIG=[\ {"ConnectionType":"MsSqlServer","ConnectionName":"SqlServer","Properties":[{"key":"ProviderName","value":"System.Data.SqlClient"},{"key":"ConnectionString","value":"xxxx"}]},\ {"ConnectionType":"Oracle","ConnectionName":"Oracle","Properties":[{"key":"ProviderName","value":"Oracle.ManagedDataAccess.Client"},{"key":"ConnectionString","value":"xxxx", "value": “CONFIG_DV"}]},\ {"ConnectionType":"MongoDb","ConnectionName":"Mongo","Properties":[{"key": "server","value": "xxx"}]}]

We then inject named instances of a database factory into the container.

When the developers create their business Dal, they specify which database factory they need in their constructor, and it gets injected automagically.

Mjultiple connections will be registered. Multiple business Dals will get registered (again, automatically by the framework). Actually we scan their components and add them to the container, they don’t even specify the registration themselves.

What this syntax meant to me:

public TestDal([Import(Key = "SqlServer")] IDatabaseFactory databaseFactory)

Is the container should find the named instance of the IDatabaseFactory called “Oracle” and inject it. If there were multiple Dals…

public TestDal2([Import(Key = "Oracle")] IDatabaseFactory databaseFactory)

the container should be able to find the correct named instance and inject it at run-time.

Does that make sense?

Let me know if you need any more details.

Sean

From: Ian Johnson notifications@github.com Reply-To: ipjohnson/Grace reply@reply.github.com Date: Wednesday, August 1, 2018 at 8:44 AM To: ipjohnson/Grace Grace@noreply.github.com Cc: Sean Covel SCOVEL@travelers.com, Mention mention@noreply.github.com Subject: Re: [ipjohnson/Grace] Import Keyed injects wrong instance (#181)

@scovelhttps://github.com/scovel for the moment I think it's much easier to make a slight change to the fluent registration to specify the key than set up registration using attributes.

c.Export().As().WithCtorParam().LocateWithKey("SqlServer");

I'll see if I can find some time this weekend to add the flag as it's shouldn't much work and I'll release a beta for you to try.

That's very cool about your son, very good name :)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/ipjohnson/Grace/issues/181#issuecomment-409561595, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AYMQXswRjGDC_BK0kDqBqtzApVn2QEDyks5uMaKUgaJpZM4Voi_a.


This message (including any attachments) may contain confidential, proprietary, privileged and/or private information. The information is intended to be for the use of the individual or entity designated above. If you are not the intended recipient of this message, please notify the sender immediately, and delete the message and any attachments. Any disclosure, reproduction, distribution or other use of this message or any attachments by an individual or entity other than the intended recipient is prohibited.

TRVDiscDefault::1201

ipjohnson commented 6 years ago

That makes complete sense. I’ll write a more in depth reply this evening but the short version is I’ll add a flag to process the import attribute automatically and it should solve your issue.

scovel commented 6 years ago

Awesome! Thanks

From: Ian Johnson notifications@github.com Reply-To: ipjohnson/Grace reply@reply.github.com Date: Wednesday, August 1, 2018 at 10:47 AM To: ipjohnson/Grace Grace@noreply.github.com Cc: Sean Covel SCOVEL@travelers.com, Mention mention@noreply.github.com Subject: Re: [ipjohnson/Grace] Import Keyed injects wrong instance (#181)

That makes complete sense. I’ll write a more in depth reply this evening but the short version is I’ll add a flag to process the import attribute automatically and it should solve your issue.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/ipjohnson/Grace/issues/181#issuecomment-409601039, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AYMQXtFCGjJzNMFpXvb-D3mtWwwzUKeYks5uMb9bgaJpZM4Voi_a.


This message (including any attachments) may contain confidential, proprietary, privileged and/or private information. The information is intended to be for the use of the individual or entity designated above. If you are not the intended recipient of this message, please notify the sender immediately, and delete the message and any attachments. Any disclosure, reproduction, distribution or other use of this message or any attachments by an individual or entity other than the intended recipient is prohibited.

TRVDiscDefault::1201

ipjohnson commented 6 years ago

@scovel I've added a flag to process import attributes on constructor parameters. I set it to true by default because it seems like if you went the length to attach an attribute you want it processed.

Would it be possible for you to test it off the nightly nuget feed? I'd prefer to test it before I release an RC to nuget.org.

https://ci.appveyor.com/nuget/grace-master

scovel commented 6 years ago

I was just looking through your dev branch when this email came in. Sure, I can test it. I’ll get back to you today.

Sean

From: Ian Johnson notifications@github.com Reply-To: ipjohnson/Grace reply@reply.github.com Date: Friday, August 3, 2018 at 8:22 AM To: ipjohnson/Grace Grace@noreply.github.com Cc: Sean Covel SCOVEL@travelers.com, Mention mention@noreply.github.com Subject: Re: [ipjohnson/Grace] Import Keyed injects wrong instance (#181)

@scovelhttps://github.com/scovel I've added a flag to process import attributes on constructor parameters. I set it to true by default because it seems like if you went the length to attach an attribute you want it processed.

Would it be possible for you to test it off the nightly nuget feed? I'd prefer to test it before I release an RC to nuget.org.

https://ci.appveyor.com/nuget/grace-master

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/ipjohnson/Grace/issues/181#issuecomment-410237047, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AYMQXlGlJM2kJQZu-nll5_0RWycthlFKks5uNEB4gaJpZM4Voi_a.


This message (including any attachments) may contain confidential, proprietary, privileged and/or private information. The information is intended to be for the use of the individual or entity designated above. If you are not the intended recipient of this message, please notify the sender immediately, and delete the message and any attachments. Any disclosure, reproduction, distribution or other use of this message or any attachments by an individual or entity other than the intended recipient is prohibited.

TRVDiscDefault::1201

ipjohnson commented 6 years ago

@scovel one thing I did realize when I was working this is that your registration should be tweaked a little.

I think you want

c.ExportInstance(SqlServer).AsKeyed<IDatabaseFactory>(SqlServer.name);

I don't think you want the extra As<IDatabaseFactory> , I think you want only the keyed export.

scovel commented 6 years ago

That would explain why I had each item in the container 2X…

Where do I get the nightly build?

From: Ian Johnson notifications@github.com Reply-To: ipjohnson/Grace reply@reply.github.com Date: Friday, August 3, 2018 at 8:40 AM To: ipjohnson/Grace Grace@noreply.github.com Cc: Sean Covel SCOVEL@travelers.com, Mention mention@noreply.github.com Subject: Re: [ipjohnson/Grace] Import Keyed injects wrong instance (#181)

@scovelhttps://github.com/scovel one thing I did realize when I was working this is that your registration should be tweaked a little.

I think you want

c.ExportInstance(SqlServer).AsKeyed(SqlServer.name);

I don't think you want the extra As , I think you want only the keyed export.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/ipjohnson/Grace/issues/181#issuecomment-410241136, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AYMQXp5QdbCbkqOzkYpmQC_zlry_TZbiks5uNESqgaJpZM4Voi_a.


This message (including any attachments) may contain confidential, proprietary, privileged and/or private information. The information is intended to be for the use of the individual or entity designated above. If you are not the intended recipient of this message, please notify the sender immediately, and delete the message and any attachments. Any disclosure, reproduction, distribution or other use of this message or any attachments by an individual or entity other than the intended recipient is prohibited.

TRVDiscDefault::1201

scovel commented 6 years ago

I pulled the dev branch and built it…

It works as expected in my test project. AWESOME!

Thanks,

Sean

From: Sean Covel SCOVEL@travelers.com Date: Friday, August 3, 2018 at 8:41 AM To: ipjohnson/Grace reply@reply.github.com, ipjohnson/Grace Grace@noreply.github.com Cc: Mention mention@noreply.github.com Subject: Re: [ipjohnson/Grace] Import Keyed injects wrong instance (#181)

That would explain why I had each item in the container 2X…

Where do I get the nightly build?

From: Ian Johnson notifications@github.com Reply-To: ipjohnson/Grace reply@reply.github.com Date: Friday, August 3, 2018 at 8:40 AM To: ipjohnson/Grace Grace@noreply.github.com Cc: Sean Covel SCOVEL@travelers.com, Mention mention@noreply.github.com Subject: Re: [ipjohnson/Grace] Import Keyed injects wrong instance (#181)

@scovelhttps://github.com/scovel one thing I did realize when I was working this is that your registration should be tweaked a little.

I think you want

c.ExportInstance(SqlServer).AsKeyed(SqlServer.name);

I don't think you want the extra As , I think you want only the keyed export.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/ipjohnson/Grace/issues/181#issuecomment-410241136, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AYMQXp5QdbCbkqOzkYpmQC_zlry_TZbiks5uNESqgaJpZM4Voi_a.


This message (including any attachments) may contain confidential, proprietary, privileged and/or private information. The information is intended to be for the use of the individual or entity designated above. If you are not the intended recipient of this message, please notify the sender immediately, and delete the message and any attachments. Any disclosure, reproduction, distribution or other use of this message or any attachments by an individual or entity other than the intended recipient is prohibited.

TRVDiscDefault::1201

ipjohnson commented 6 years ago

@scovel I'll push an RC to nuget this weekend and plan to do an official release later this month

ipjohnson commented 6 years ago

I released an RC version today

scovel commented 6 years ago

The RC is working perfectly. Looking forward to the official release. Thanks for your hard work and quick response.

Sean

From: Ian Johnson notifications@github.com Reply-To: ipjohnson/Grace reply@reply.github.com Date: Monday, August 6, 2018 at 11:55 AM To: ipjohnson/Grace Grace@noreply.github.com Cc: Sean Covel SCOVEL@travelers.com, Mention mention@noreply.github.com Subject: Re: [ipjohnson/Grace] Import Keyed injects wrong instance (#181)

I released an RC version today

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/ipjohnson/Grace/issues/181#issuecomment-410757121, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AYMQXhU9eGwTiue_vYpEKYE6YM3Hg9eKks5uOGbrgaJpZM4Voi_a.


This message (including any attachments) may contain confidential, proprietary, privileged and/or private information. The information is intended to be for the use of the individual or entity designated above. If you are not the intended recipient of this message, please notify the sender immediately, and delete the message and any attachments. Any disclosure, reproduction, distribution or other use of this message or any attachments by an individual or entity other than the intended recipient is prohibited.

TRVDiscDefault::1201

ipjohnson commented 6 years ago

@scovel sorry had to publish another preview for another issue. I'm shooting for middle of next week for an official release.