unitycontainer / container

Unity.Container implementation.
Apache License 2.0
132 stars 69 forks source link

Resolving a type with container.Resolve and ResolverOverrides does not use a separate BuilderContext when resolving dependencies #230

Open RonHeck opened 4 years ago

RonHeck commented 4 years ago

Description

I have a wrapper type Foo with ctor Foo(IAbc,IXyz). I want to resolve Foo using UnityContainer.Resolve. I provide a ResolverOverride for IAbc to the call. When the container resolves the dependencies of types constructor, it uses the BuilderContext of Foo to resolve IXyz which also includes the ResolverOverride for IAbc. Regarding inversion of control this seems to be wrong as the caller of Resolve<Foo>() should not know how IXyz is resolved and therefore should not "accidentally" influence resolving IXyz (e.g. if IXyz resolves to a type that depends on IAbc.

To Reproduce

Please provide UnitTest in the form of:

[TestMethod]
public void Should_Resolve_Dependency_Without_Override()
{
    //***** arrange *****
    var unityContainer = new UnityContainer();
    unityContainer.RegisterType<IXyz, Xyz>();
    unityContainer.RegisterType<IAbc, Abc>();
    var dummyAbc = new OverrideAbc();

    //***** act *****
    var foo = unityContainer.Resolve<Foo>(new DependencyOverride<IAbc>(dummyAbc));

    //***** assert *****
    // TestContext.WriteLine("foo.Abc - " + foo.Abc.Text);
    // TestContext.WriteLine("foo.Xyz.Abc - " + foo.Xyz.Abc.Text);
    Assert.That(foo.Abc, Is.Not.SameAs(foo.Xyz.Abc));
}

private interface IAbc
{
    string Text { get; }
}

private class Abc : IAbc
{
    public string Text { get; } = "A am an registered type";
}

private class OverrideAbc : IAbc
{
    public string Text { get; } = "A am an override";
}

private interface IXyz
{
    public IAbc Abc { get; }
}

private class Xyz : IXyz
{
    public IAbc Abc { get; }

    /// <summary>
    /// Creates a new instance of <see cref="Xyz"/>.
    /// </summary>
    public Xyz(IAbc abc)
    {
         Abc = abc;
    }
}

private class Foo
{
     public IAbc Abc { get; }
     public IXyz Xyz { get; }

     /// <summary>
     /// Creates a new instance of <see cref="Foo"/>.
     /// </summary>
     public Foo(IAbc abc, IXyz xyz)
     {
          Abc = abc;
          Xyz = xyz;
     }
}

Additional context

Used Version 5.11.8 of unity container nuget to verify. Test was written with NUnit so may it has to be adjust to the used test framework.

ENikS commented 4 years ago

BuilderContext is rather expensive and I am trying to avoid allocating new one unless it is absolutely necessary. Why is it a problem for you?

RonHeck commented 4 years ago

I have multiple registrations for the same interface differ by the registered name. Most implementations are decorators. I created an extension to define how each decorator resolves the dependency on the interface. Mainly this is only a definition, which name to use for resolving. The extension then simply adds ResolveOverrides to the context. This logic is only enabled, if the context has no ResolveOverrides defined to prevent interfere of resolving with overrides. This way I try to prevent "poluting" my business code with framework specific attributes. I agree that creating a BuilderContext for resolving each dependency is a performance hit. I have to check if it is possible to merge ResolveOverrides provided from the "external" resolve call with the internally created ResolveOverrides of the extension in a propper way. Nevertheless I did not expect, that the ResolveOverrides, defined for the resolve call also effect resolving of the inner dependencies. There is no way to define, that the ResolveOverride should only take into account for a specific resolving type. Rather it will override all dependencies in the resolve hierarchy. Imagine following scenario using the types of the example above: I register type Xyz as singleton. Now resolve Foo with the override. Finally, if the singleton is not created yet, the singleton will created with the OverrideAbc. If I now resolve Foo without override, the singleton is reused. At the end this may leads to an runtime dependent behavior of Xyz if the code can not guarentee if Foo is created with or without override first. Also this behavior seems not to be transparent to the API user.

RonHeck commented 4 years ago

I also already thought about not to recreate the BuilderContext each time but using an hierachical structure to manage the overrides for each resolve call. Resolving within the same builder context is an synchrone process and mainly recursive. So before calling Resolve of the parameter type I can store the overrides localy and set the Overrides of the context to Array.Empty. In my mind this should not result in expensive heap allocating.

ENikS commented 4 years ago

A new unity is going to be using struct as a context and will not allocate anything.

RonHeck commented 4 years ago

Even better. Nevertheless could You understand my concerns? I'm not sure if I could put it into understandable terms.

ENikS commented 4 years ago

I think I do but I can’t look into this closer right this moment...

ENikS commented 4 years ago

There is a Target parameter on override type, did you try to use it?

RonHeck commented 4 years ago

I currently tried but it had no effect. I used

var foo = unityContainer.Resolve<Foo>(new DependencyOverride(typeof(Foo), typeof(IAbc), null, dummyAbc));

For my Extension this might be an option, but it also should work with the default builder strategies of UnityContainer, right?