Improve support passing for constructor arguments to Resolve()

[darkcamper]

Hi! I was looking for a replacement for Ninject compatible with ASP Net Core and came across Grace.

I found it really appealing because it has nice features from Autofac such as lightweight lifetime scopes and the contextual injection I like so much from Ninject (and it's alive!). While I was writing some tests to learn how to do the same things I do with other DI containers I reached a dead end when trying to simulate Ninject's ConstructorArgument (and the other IConstructorArgument implementations).

Initially I was more or less able to replace them by using the extraData parameter of the DependencyInjectionContainer.Locate method but soon I found some cases I couldn't find a solution for this case:

    public class SomeClass
    {
        private readonly A _someA;
        private readonly int _someInteger;
        private readonly B _someB;

        public SomeClass(B someB)   
        {
            _someB = someB;
        }

        public SomeClass(B someB,int someInt) : this(someB)
        {
            _someInteger = someInt;
        }

        public SomeClass(B someB, int someInt, A someA) : this(someB,someInt)
        {
            _someA = someA;
        }

    }

    static void Main(string[] args)
    {

        var container = new DependencyInjectionContainer();
        container.Configure(c =>
        {
            c.Export<SomeClass>();
            c.Export<B>();
        });

        var instance = container.Locate<SomeClass>(new { someInt = 10 });
    }

I expected that SomeClass would be instantiated using the second constructor as the container has an export for "B" and knows a value for the "someInt" parameter, but the chosen constructor was the first one. In this scenario using "ConstructorSelectionMethod.MostParameters" wouldn't do the trick because it would use the last constructor and fail since I don't have an export for "A" in my container.

Looking at the sources it seems that the constructor selector doesn't use contextual info (extradata) when deciding the best constructor, and you couldn't easily change this because "IWrapperOrExportActivationStrategy.GetActivationStrategyDelegate" doesn't receive an "IInjectionContext".

What do you think on expanding the delegate generation and constructor selection by allowing usage of injection context? And maybe also adding support for additional constructor arguments like Ninject's "TypeMatchingConstructorArgument". By the way if I'm mistaken and all I'm saying is already possible please forgive my ignorance 😄

Whew! Sorry for the wall of text.

Best regards and thank you for this cool project!

[ipjohnson]

Hi darkcamper,

Glad you like Grace. Part of what makes Grace so performant is that it picks one constructor upfront and then uses it. So in the case where you have multiple constructors there isn't currently a way to construct a type many different ways based on what's in the constructor.

I'd be open to offer a new ConstructorSelectionMethod that is based on what's available in the context in the case where there are multiple constructors. It also would be good to make it configurable per export.

There is the option of specifying which constructor you want if you know it ahead of time, but it doesn't solve your dynamic case

var container = new DependencyInjectionContainer();
container.Configure(c =>
{
     // the expression isn't executed it's just inspected to figure out what constructor to use
     c.Export<SomeClass>().ImportConstructor(() => new SomeClass(null,0));
     c.Export<B>();
});

// will call second constructor
var instance = container.Locate<SomeClass>(new { someInt = 10 });

[ipjohnson]

After thinking about it some more I think I can support both the dynamic constructor scenario and the NInject IConstructorArgument idea without having to change any of the fundamentals of the containers.

The idea will be to add the selection method ConstructorSelectionMethod.Dynamic then the linq code generation will look something similar to

private SomeClass CreateMethod(IExportLocatorScope scope, IDisposalScope disposalScope, IInjectionContext context)
{
  var canResolveB = DynamicCanLocate<B>(scope, context, "someB");
  var canResolveSomeInt = DynamicCanLocate<int>(scope, context, "someInt");
  var canResolveA = DynamicCanLocate<A>(scope, context, "someA");

  if(canResolveB && canResolveSomeInt && canResolveA)
  {
    return new SomeClass(DynamicLocate<B>(scope, diposalScope, context,"someB"), 
                         DynamicLocate<int>(scope, diposalScope, context, "someInt"), 
                         DynamicLocate<A>(scope, diposalScope, context, "someA"));
  }

  if(canResolveB && canResolveSomeInt)
  {
    return new SomeClass(DynamicLocate<B>(scope, diposalScope, context,"someB"), 
                         DynamicLocate<int>(scope, diposalScope, context, "someInt"));
  }

  if(canResolveB )
  {
    return new SomeClass(DynamicLocate<B>(scope, diposalScope, context,"someB"));
  }

  throw new LocateException(staticContext, "Couldn't find B");
}

public static bool DynamicCanLocate<T>(IExportLocateScope scope, IInjectionContext context, string parameterName)
{
  /// look in injection context and scope for type with parameter name
}

public static T DynamicLocate<T>(IExportLocatorScope scope, IDisposalScope disposalScope, context, string parameterName)
{
   // look in context, then scope for T 
}

The idea of IConstructorArgument becomes easy as the original raw object that's passed in as extra data is available on the IInjectionContext.ExtraData property.

You will be able to configure the whole container that way, type sets, or at the individual export level.

I'm going to begin playing with the idea but it may take me a week or two to get exactly the way I want. Would you be able to test a beta at some point before I release?

[darkcamper]

Sounds like a nice solution. In this way you have fine-grained control between the performant old constructor selection and the new dynamic selector at all levels (container, exports...).

As for the beta testing, count me in! I'd be really happy to help you improve Grace.

Btw, as you'll be adding another member to ConstructorSelectionMethod enum, maybe it could be a good time to extract constructor selectors into their on interface/implementations and enabling support for ConstructorSelectionMethod.Other, thus giving the users support for more 'exotic' constructor selection requirements.

[ipjohnson]

I was thinking the same thing on making an interface to abstract it out, the place where I'm a little stuck is that having the enum ConstructorSelectionMethod is a little constricting. I'm thinking ultimately it should be an int that way you could have more than one 'exotic' constructor methods and I can get rid of Other.

This is what it would look like registering a custom constructor selection method

var container = new DependencyInjectionContainer(c => 
{
   c.Behaviors.ConstructorSelection = SomeConstant;
   c.Implementation.Locate<IInstantiationExpressionCreator>()
      .RegisterConstructorExpressionCreator(SomeConstant, new ExoticConstructorExpressionCreator());
});

The default implementations for MostParameters, LeastParameters, BestMatach, and Dynamic they will be separated out into their own classes and InstantiationExpressionCreator will essentially just lookup the correct method to execute.

What's your opinion on enum vs int vs something else?

[darkcamper]

I think I'd go for a direct usage of the ConstructorExpressionCreator instances instead of continuing using the enum or including any kind of registration. You can always provide a "DefaultConstructorExpressionCreators" class with public properties containing the implementations of the provided constructor selector strategies (less parameters, most parameters, best fit dynamic). You could then use it this way:

var container = new DependencyInjectionContainer(c => 
{
   c.Behaviors.ConstructorSelection = DefaultConstructorExpressionCreators.Bestfit
});

or

var container = new DependencyInjectionContainer(c => 
{
   c.Behaviors.ConstructorSelection = new MyIncrediblyExoticConstructorExpressionCreator();
});

I'm not really sold on having to associate an int to an ConstructorExpressionCreator because I think that the different constructor selection strategies are already well represented by their own classes, and the registration mechanism would be a bit redundant.

In any case I completely agree with you in that continuing using the enum it's restrictive and could lead to some big and ugly switch statements if, in the future, you decide to add more predefined strategies to Grace.

[ipjohnson]

That seems reasonable I think I'll go that way and convert the current Enum to a static class with properties that match the enum names. That way when existing codebases are upgraded there is no change needed.

[ipjohnson]

@darkcamper I'm thinking of breaking this work up into two sections

• Add Dynamic constructor selection method
• Add 'TypeMatchingConstructorArgument'

I've got the implementation for the first part nearly complete and I'm looking to put out a beta this weekend with the support added.

The second part I'd like to hold off on for a bit to see if extra data + Dynamic constructor selection suites your needs or if 'TypeMatchingConstructorArgument' is a must. The implementation would probably need to be applied to not only constructors but property injection, method parameter injection, and that's more than I'd like to roll into this release.

That work for you?

[darkcamper]

@ipjohnson Sure. Besides it will be easier to test each functionallity individually.

[ipjohnson]

@darkcamper I've pushed a Beta version that contains the new Dynamic constructor selection.

var container = new DependencyInjectionContainer(c => c.Behaviors.ConstructorSelection = ConstructorSelectionMethod.Dynamic);

I need to add some more tests but these are the tests I've got so far.

[darkcamper]

@ipjohnson I've run some tests (not as many as I'd like to do) and it seems to work well!

There's only one thing that doesn't feel alright to me, altough I'm not sure if it's a bug or a design decision. Suppose we have this constructor

public SomeClass(B someB, int someInt, A someA, int someOtherInt)

When I resolve a Func<int,SomeClass> and call it, the integer argument is passed to both int arguments of the constructor (and that's ok, it matches the type of the parameters as it has no other way of doing the match).

But when passing extraData like this:

var res = container.Locate<SomeClass>(new {someInt = 1, someOtherInt = 20 });

I would expect that each of the int parameters received a different value, but they bot receive "1". It's consistent with the Func case but it's strange (for me at least). I suppose that, if you end up adding the TypeMatchingConstructor equivalent, the syntax will become clearer.

Anyway, this is a nice improvement. Thanks for implementing it so quickly!

[ipjohnson]

@darkcamper the second case should work as you think it should, let me do a little research to see why it's not working as anticipated.

The problem looks to be a string casing bug. I've checked in a fix and the build has a nuget feed if you'd like to try it out https://ci.appveyor.com/nuget/grace-master

[darkcamper]

@ipjohnson I've just tried your last fix and it works perfectly :smile:

[ipjohnson]

@darkcamper, very good. I think what I'm going to do is take another week or two and write some more tests and then do a release. This will also give you a little more time to work with it and see if there are any other issues you run into.

[ipjohnson]

I've released 6.1.0, if you have any issues let me know