ipjohnson / Grace

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

Exception with open generics #204

Closed silkfire closed 5 years ago

silkfire commented 5 years ago

I'm trying to export a class accordingly:

c.ExportFactory(() => new SyncService(new JsonShipmentService(scope.Locate(typeof(IShipmentRepository<,>), withKey: "json") as Data.Json.Repositories.ShipmentRepository),
                                      new SqlShipmentService( scope.Locate(typeof(IShipmentRepository<,>), withKey: "sql")  as ShipmentRepository)))
 .Lifestyle.Singleton();

But am getting the following exception:

image

I'm not sure how to solve this...

ipjohnson commented 5 years ago

You can’t locate an open generic, you must have a closed generic i.e ‘<T,T2>’

silkfire commented 5 years ago

But I know that Grace automatically resolves open generic in other parts of my code, how come not here?

ipjohnson commented 5 years ago

You can register open generics but when Grace goes to resolve it it’s been closed by being a dependcy.

What’s the type you expect IShipmentRepository to be closed as?

silkfire commented 5 years ago

For example, this code works just as expected (with Locate of open generic):

c.ExportFactory(() => new SqlShipmentService(scope.Locate(typeof(IShipmentRepository<,>), withKey: "sql") as ShipmentRepository)).As<IShipmentService>().Lifestyle.Singleton();

The code I showed in the original post is very similar but gives an exception.

silkfire commented 5 years ago

It seems as if the "json" keyed registration gives me the exception, but not "sql". Strange...

ipjohnson commented 5 years ago

I guess if you register a closed implementation it could work because you aren’t relying on the container to close it.

That said I’ve never used the container for that purpose.

Can you put together a small sample of what’s not working and I’ll debug it. The stack looks like the expression compiler is complaining about generic parameters so I’m guessing it has to do with the open aspect.

silkfire commented 5 years ago

I was able to reproduce it. It seems to occur when the constructor of the class to be located has parameters that are of primitive type. The "sql" registration only requires an IDbConnection which it is able to resolve with no issues, whereas the "json" one requires a string.

Try this:

App.cs

namespace Test
{
    using Grace.DependencyInjection;

    public static class App
    {
        public static void Main()
        {
            var container = new DependencyInjectionContainer();

            container.Configure(c =>
            {
                c.ExportFactory(() => new DummyService("dummy")).AsKeyed(typeof(IDummyService<>), "dummy");
            });

            var dummyService = container.Locate(typeof(IDummyService<>), withKey: "dummy");  // exception occurs here
        }
    }
}

DummyService.cs

namespace Test
{
    public class DummyService : IDummyService<Dummy>
    {
        public DummyService(string dummy)    // if we remove this parameter or replace it with a service, no exception is thrown
        {

        }
    }
}

IDummyService.cs

namespace Test
{
    public interface IDummyService<TDummy>
        where TDummy  : class
    {

    }
}

Dummy.cs

namespace Test
{
    public class Dummy
    {
    }
}

The exception throws even without a keyed registration, so that's not the issue.

silkfire commented 5 years ago

@ipjohnson Apparently it's not an issue with the arguments, I think it's actually the ExportFactory method itself. Using Export works fine but is not an option in my specific case.

By the way, when it comes to WithCtorParam(), let's say I've got two string parameters, how do I set a specific value to each of them? Right now I can't select a parameter by name.

ipjohnson commented 5 years ago

It has to do with the way ExportFactory is trying to add a check for null being returned. Essentially it takes your code and wraps it in a null check but it’s trying to use the export type as a generic parameter to the null check method.

I’m not sure I follow the question about constructor parameter. There should be a Named option, is there not one?

silkfire commented 5 years ago

Okay, is there any way this can be solved?

There is .WithNamedCtorValue() but its purpose seems a bit unclear to me. Is it to retrieve a property from the class to be resolved? Anyhow it doesn't seem to suit my needs.

ipjohnson commented 5 years ago

I’ll see if I can find some time this evening to take a look.

Does your generic interface have a base interface you could register by instead of the open generic?

silkfire commented 5 years ago

Thanks, really appreciate it. No, unfortunately that's the only interface my services inherit.

ipjohnson commented 5 years ago

What does your registration look like for Shipment? I assume you have multiple that has AsKey?

silkfire commented 5 years ago

You mean the ShipmentService?

It looks like this:

c.Export<Data.Json.Repositories.ShipmentRepository>().AsKeyed(typeof(IShipmentRepository<,>), "json").Lifestyle.SingletonPerRequest();

///

c.ExportFactory(() => new JsonShipmentService(scope.Locate(typeof(IShipmentRepository<,>), withKey: "json") as Data.Json.Repositories.ShipmentRepository)).As<IShipmentService>().Lifestyle.Singleton();

But does it really matter? The example I posted should suffice to reproduce the exception.

ipjohnson commented 5 years ago

I was curious what the difference was between the json registration and the sql registration.

I understand why it’s happening and honestly it’s working as designed. I’m looking to see if you could possibly change the way you are registering/locating to not have open generics.

silkfire commented 5 years ago

The difference is that the JSON registration uses ExportFactory because I need to provide it an argument whereas the SQL one doesn't need that.

I'd really like to use the open generics. It's as if it's working, but only half way. Is it hard to make the method support this?

silkfire commented 5 years ago

I really like Grace as it is very extensive but also very easy to use and fast. So if I could get this to work then Grace will definitely be my go-to DI framework. I recently switched from StructureMap as the author stopped developing it to its replacement Lamar, but I found that framework very slow and buggy, so I had to look for another library.

ipjohnson commented 5 years ago

One way or another I’m sure we can get this squared away. I was more just trying to get the lay of the land because usually when you register an open generic you then locate with a closed type.

Is your json service a generic implementation or closed implementation?

ipjohnson commented 5 years ago

So playing around with the example you gave me I can get the open generic locate to work if I change the way to export from ExportFactory to Export

            container.Configure(c =>
            {
                c.Export<DummyService>().AsKeyed(typeof(IDummyService<>), "dummy").
                           WithCtorParam(() => "dummy").Named("dummy");
                //c.ExportFactory(() => new DummyService("dummy")).AsKeyed(typeof(IDummyService<>), "dummy");
            });

The WithCtorParam method takes a function used to specify the value ("dummy") and you can specify the name of the parameter with Named("parameterName").

silkfire commented 5 years ago

Thanks so much for the solution. I was looking for something like that but wasn't quite able to find it. Perhaps you should add that to the documentation, about Named parameters.

Why is the ExportFactory method different from Export when it comes to open generics? Perhaps you should prevent the Fluent Interface to use Type in this case? Or give the user a more meaningful error :)

The JSON repository is a closed implementation, yes. So all the info is already there and I don't want to have to resupply it in my registrations.

public class ShipmentRepository : IShipmentRepository<Shipments.Shipment, ShipmentNumbers.ShipmentNumber>
{
    private readonly Shipments       _shipments;
    private readonly ShipmentNumbers _shipmentNumbers;

    private readonly Dictionary<string, string> _shipmentDataFilepaths;

    public ShipmentRepository(Dictionary<string, string> shipmentDataFilepaths) {
         ....
    }
silkfire commented 5 years ago

By the way, would you mind replying to this issue: Method not found in Grace.Net https://github.com/ipjohnson/Grace/issues/203?

ipjohnson commented 5 years ago

@silkfire there are many things that I could do or maybe should do but alas I don't have much time these days between work life and having a little one to care for.

The reason ExportFactory works differently is because it's designed to take a function that creates a known export. An open generic is as it's name implies open and can construct and infinite number of types. Technically speaking I could probably make it work but your usage of asking for an open generic through locate is very different that anyone's I've seen and I don't like adding features for niche cases when it can be solved a different way.

silkfire commented 5 years ago

Unfortunately I didn't get it to work and as of right now I'm not able to reproduce it with the dummy services.

Here's the registration:

c.Export<Data.Json.Repositories.ShipmentRepository>().WithCtorParam(() => shipmentDataFilepaths).AsKeyed(typeof(IShipmentRepository<,>), "json").Lifestyle.SingletonPerRequest();

...

c.Export<JsonShipmentService>().WithCtorParam(() => scope.Locate(typeof(IShipmentRepository<,>), withKey: "json")).As<IShipmentService>().Lifestyle.Singleton();

And the error is now (very similar to the previous one):

image

silkfire commented 5 years ago

I removed .Lifestyle.SingletonPerRequest() and now it works! How strange. But I would like to use that Lifestyle if possible. Am I missing something?

Adding .Lifestyle.SingletonPerRequest() to the dummy services reproduces the same error.

DummyService.cs

public class DummyService2
{
   public DummyService2(IDummyService<Dummy> dummyService)
   {

   }
}

App.cs

container.Configure(c =>
{
    c.Export<DummyService>().WithCtorParam(() => "dummy1").Named("dummy1").WithCtorParam(() => "dummy2").Named("dummy2").AsKeyed(typeof(IDummyService<>), "dummy").Lifestyle.SingletonPerRequest();
    c.Export<DummyService2>().WithCtorParam(() => container.Locate(typeof(IDummyService<>), withKey: "dummy"));
});

var dummyService2 = container.Locate<DummyService2>();
ipjohnson commented 5 years ago

I can kinda get it to work by changing the configuration to


            container.Configure(c =>
            {
                c.Export<DummyService>().WithCtorParam(() => "dummy1").Named("dummy").AsKeyed(typeof(IDummyService<>), "dummy").Lifestyle.SingletonPerRequest();
                c.Export<DummyService2>().WithCtorParam<object>().Named("dummyService").LocateWithKey("dummy");
            });

That said I keep coming back to the fact that registering a closed implementation by an open interface, then locating by an open interface is not the right way to do this.

Normally when you register something with an open interface the type being export is an open type definition. What you're doing is registering a closed implementation by and open interface and then locating it by hand, if you were to locate the type as a dependency it would fail because the exported type is not open.

Why not register the closed implementation by the closed interface and then locate using a closed interface? All your troubles come from the fact that you're trying to use an open interface on a closed type.

silkfire commented 5 years ago

Okay, I assume I have to try that out then. I'm just a bit surprised that it actually works with the way Export is implemented now. You don't get an exception with var dummyService = container.Locate(typeof(IDummyService<>), withKey: "dummy");, so the behaviour isn't forbidden according to Grace.

ipjohnson commented 5 years ago

This is actually something I should tighten up on the fluent registration and throw an exception on registration.

This evening I’ll open a defect and look to make it obvious upfront what the error is. The exception that’s being thrown currently is from dotnet because it’s trying to compile a function with an open interface.

I was actually surprised it worked as well but since the type isn’t trying to be closed and the container is a dictionary under the covers it looked up the type, found it and created an instance.

silkfire commented 5 years ago

If you wonder where my way of thinking comes from... I've been working a lot with StructureMap previously and in that library they have something like this:

image

http://structuremap.github.io/generics/

Basically the DI container automatically closes the open type for you. It seems I was partially able to achieve this with Grace too :) At least it's pretty clever on guessing the closing type.

When would this be okay to use:

image

?

ipjohnson commented 5 years ago

@silkfire Grace support's open generic's exactly like that example. What Grace doesn't support and StructureMap is also not going to support is exporting a closed implementation as an open interface.

For(typeof(IVisualizer<>)).Use(typeof(DefaultVisualizer<SomeClosedType>));

If you had registered your sql service as open generic and then registered your json service as a closed implementation it would override it just like the example you showed.

silkfire commented 5 years ago

@ipjohnson Thanks, I think I understand the difference now. I've updated my registrations as per your advice and things work very nicely now.

ipjohnson commented 5 years ago

Oh very good! Glad it all worked out. If you have any other questions submit an issue.

I’ll create a work item for myself to throw a better exception on registration to avoid this problem in the future.

ipjohnson commented 5 years ago

going to close this out an track the registration enhancement in issue #216

silkfire commented 5 years ago

@ipjohnson Great initiative, really appreciate it, Ian.