Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Getting instance from parent scope even if replaced by Use #460

Open
phoeniks-sk opened this issue Mar 2, 2022 · 11 comments
Open

Getting instance from parent scope even if replaced by Use #460

phoeniks-sk opened this issue Mar 2, 2022 · 11 comments
Assignees
Labels
bug Something isn't working
Milestone

Comments

@phoeniks-sk
Copy link

Hello,

maybe I'm doing something wrong. I have the following code:

    public class Data
    {
        public string Text { get; set; }
    }

    public interface IDataDependent
    {
        Data Data1 { get; }
    }

    public class DataDependent : IDataDependent
    {
        public Data Data1 { get; }

        public DataDependent(IData data)
        {
            Data1 = data;
        }
    }

    public class DataDependentIndirectly
    {
        public IDataDependent Dependent { get; }

        public DataDependentIndirectly(IDataDependent dataDependent)
        {
            Dependent = dataDependent;
        }
    }

   [Fact]
   public void Should_use_data_from_the_scope_when_resolving_in_scope()
   {
        var container = new Container();
        container.RegisterInstance(new Data { Text = "parent" });
        container.Register<IDataDependent, DataDependent>();
        container.Register<DataDependentIndirectly>();
            
        // if this is removed, the test passes
        var outside = container.Resolve<DataDependentIndirectly>();

        using var scope = container.OpenScope();
        scope.Use(new Data { Text = "child" });
        var inScope = scope.Resolve<DataDependentIndirectly>();
        Assert.Equal("child", inScope.Dependent.Data1.Text);
   }

I would expect the DataDependentIndirectly object created in the scope to have the value of the Data that was added using Use injected. This happens, but only if DataDependentIndirectly is not resolved outside the scope first. Otherwise it gets the value with "parent" Text created outside the scope.

If I resolve Data manually from the scope, the value is correct.

It also works correctly if using a child container instead of a scope. Am I doing something wrong? It seems to me like this is exact use case for scopes, i.e. injecting some scope-related data into the object tree resolved in the scope.

Thanks for the response in advance.

@dadhi
Copy link
Owner

dadhi commented Mar 2, 2022

@phoeniks-sk Hi, thanks for repro tests.
What DI version are you using?

@phoeniks-sk
Copy link
Author

phoeniks-sk commented Mar 2, 2022

Ah, I should have mentioned that, I've originally tested it on 4.8.6, but 4.8.7 seems to behave the same.

@dadhi dadhi self-assigned this Mar 2, 2022
dadhi added a commit that referenced this issue Mar 2, 2022
@dadhi
Copy link
Owner

dadhi commented Mar 2, 2022

@phoeniks-sk

You need to plan for the original Data replacement making it dynamic by either registering asResolutionCall:

container.RegisterInstance(new Data { Text = "parent" }, setup: Setup.With(asResolutionCall: true));

or do original registration of Data with Use.

Check the tests in my commit 11ec0e8

@phoeniks-sk
Copy link
Author

@dadhi Thank you for a quick response. That does work.

The problem is however that my real code uses a container WithConcreteTypeDynamicRegistrations and there's no RegisterInstance(new Data{}) call. I've just added that when trying to solve the issue.

So to solve this, I came up with

var container = new Container(rules => rules
                .WithDynamicRegistrationsAsFallback(
                    (type, obj) =>
                    {
                        if (type == typeof(Data)) // some sort of whitelist here
                            return new[] { new DynamicRegistration(new ReflectionFactory(type, setup: Setup.With(asResolutionCall: true))) };

                        return null;
                    },
                    Rules.ConcreteTypeDynamicRegistrations()));

Now there are two questions:

  1. How to define the whitelist in the first dynamic registration? It seems to only be a problem for classes that don't have any dependencies (as Data). Because if I remove the DataDependentIndirectly registration (in the original test) and create another class that is dependent on it, the whole tree is resolved correctly inside the scope. Could you maybe shed some light on why these behave differently?
  2. Is there maybe a better way to solve all this?

Thank you.

@dadhi
Copy link
Owner

dadhi commented Mar 3, 2022

How to define the whitelist in the first dynamic registration? It seems to only be a problem for classes that don't have any dependencies (as Data). Because if I remove the DataDependentIndirectly registration (in the original test) and create another class that is dependent on it, the whole tree is resolved correctly inside the scope. Could you maybe shed some light on why these behave different

Sorry, I did not fully get it. What do you mean by whitelist?

If you mean to skip the Data from the concrete types, then you can use optional condition in ConcreteTypeDynamicRegistrations(condition), see added test.

dadhi added a commit that referenced this issue Mar 3, 2022
@phoeniks-sk
Copy link
Author

phoeniks-sk commented Mar 15, 2022

By whitelist I meant the way to determine which classes manifest the behavior described above. There's a comment in the code where the whitelist would go.

But for now I just used your first suggestion and registered the "data" classes with Setup.With(asResolutionCall: true).

However today I ran into another issue. We are using a container WithFuncAndLazyWithoutRegistration rule because of circular dependencies. If somewhere in the object graph there's a Lazy<>, then the issue as above appears again.

The changed test would be:

    public class Data
    {
        public string Text { get; set; }
    }

    public interface IDataDependent
    {
        Data Data1 { get; }
    }

    public class DataDependent : IDataDependent
    {
        public Data Data1 { get; }

        public DataDependent(IData data)
        {
            Data1 = data;
        }
    }

    public class DataDependentIndirectly
    {
        private Lazy<IDataDependent> _lazyDependent;

        public IDataDependent Dependent => _lazyDependent.Value;

        public DataDependentIndirectly(Lazy<IDataDependent> dataDependent)
        {
            _lazyDependent = dataDependent;
        }
    }

   [Fact]
   public void Should_use_data_from_the_scope_when_resolving_in_scope()
   {
        var container = new Container(rules => rules.WithFuncAndLazyWithoutRegistration()));
        container.RegisterInstance(new Data { Text = "parent" }, setup: Setup.With(asResolutionCall: true));
        container.Register<IDataDependent, DataDependent>();
        container.Register<DataDependentIndirectly>();
            
        // now it fails even if this is removed
        var outside = container.Resolve<DataDependentIndirectly>();

        using var scope = container.OpenScope();
        scope.Use(new Data { Text = "child" });
        var inScope = scope.Resolve<DataDependentIndirectly>();
        Assert.Equal("child", inScope.Dependent.Data1.Text); // fails, the value is "parent"
   }

Note the Lazy<IDataDependent> dependency in DataDependentIndirectly. The resolved DataDependent will have the Data value from the parent scope injected.

The only way how to solve this I found for now is to manually register Lazy:
container.RegisterDelegate(r => new Lazy<IDataDependent>(() => r.Resolve<IDataDependent>()));

But I cannot possibly register Lazy of all the classes that might have a Data as dependency somewhere down the tree.

Am I doing something wrong? From the documentation I would expect that Lazy is in fact just wrapping the resolve call.

NB: This fails both with and without FuncAndLazyWithoutRegistration. If I register the IDataDependent with Reuse.ScopedOrSingleton and remove the WithFuncAndLazyWithoutRegistration then it works, but as said earlier, we need FuncAndLazyWithoutRegistration (and some of the registrations might be transient anyway).

NB2: The same problem appears to be with Func<>.

@dadhi
Copy link
Owner

dadhi commented Mar 15, 2022

@phoeniks-sk Saw this, will check

@dadhi dadhi added the bug Something isn't working label Mar 16, 2022
@dadhi dadhi added this to the v4.8.8 milestone Mar 16, 2022
@dadhi
Copy link
Owner

dadhi commented Mar 16, 2022

@phoeniks-sk Hi, the problem is again with the capturing the resolution context for the dynamic resolution. In this particular case, it is captured to be a root container instead of scope for the Lazy<IDataDependent> (because it is not scope and there are other rules in place as well). That's why scope.Use(...) does not have an effect - the resolution still looks into the root container scope, which is singletons.
I need to play with it more to see how to solve it consistently.

dadhi added a commit that referenced this issue Mar 16, 2022
@dadhi dadhi closed this as completed in c134ce1 Mar 16, 2022
@dadhi
Copy link
Owner

dadhi commented Mar 16, 2022

The v4.8.8 with the fix is out.

@phoeniks-sk
Copy link
Author

phoeniks-sk commented Mar 24, 2022

Thank you very much for the quick turnaround, that solved the issues with Lazy<> and Func<> (even though I still need to register the "data" classes both with RegisterInstance and Use in the root container, but that's okay for now).

But the original problem still prevails, here's a simplified example:

        public class A : IA { }

        public class DifferentA : IA { }

        public interface IA { }

        public class B
        {
            public IA A { get; }

            public B(IA a) { A = a; }
        }

        [Fact]
        public void Should_use_correct_dependency_in_scope()
        {
            var container = new Container();
            container.Register<IA, A>(Reuse.ScopedOrSingleton);
            container.Register<B>(Reuse.ScopedOrSingleton);

            // test passes if B is not resolved outside the scope
            var bSingleton = container.Resolve<B>();

            using var scope = container.OpenScope();

            var differentA = new DifferentA();
            scope.Use<IA>(differentA);

            var bScope = scope.Resolve<B>();
            Assert.Equal(bScope.A, differentA); // fails, uses the A from the singleton scope
        }

It works if the <IA, A> registration is done with asResolutionCall: true. However in (probably not just) our scenario, we use the same registrations for both production code and testing. In testing we sometimes want to override some classes with mocks. This would require us to either identify all the classes that could ever be overridden or define all the registrations with asResolutionCall: true.

Is there something that can be done about that?

Edit: Because of the fix in v4.8.8, this problem also doesn't happen if B has dependency on Lazy<IA> instead of IA, which seems a bit inconsistent to me.

@dadhi
Copy link
Owner

dadhi commented Mar 25, 2022

@phoeniks-sk

v4.8.8, this problem also doesn't happen if B has dependency on Lazy instead of IA, which seems a bit inconsistent to me.

The Lazy<IA> and the plain IA are different cases, because the former may assume some late (lazy) binding, so the value for the binding may even no exist until it is requested via Lazy.Value. So I would not compare those.

In testing we sometimes want to override some classes with mocks. This would require us to either identify all the classes that could ever be overridden or define all the registrations with asResolutionCall: true.

Yeah, I share your concern. It is hard to solve for the general use-case, preserving the performance. You are either going for the rigid but fast object graph where everything is known beforehand, or you are going with a completely dynamic graph where you can decide in runtime what is the next dependency. I have decided to optimize for the well-known graph with ability to have a dynamic cuts in it via asResolutionCall.

In your situation, it is even more complicated with the dual reuse of ScopedOrSingleton. So we have two kinds of dynamism interleaved.

Anyway, thanks for the test. I will play with it.

I have some broader ideas on how to hijack the graph and substitute the pieces of it for testing. It may be no via Use but rather more low-level via Interpretation stage overload rules.
DryIoc interprets the created object-graph expression by traversing it and basically calling TryInterpret recursively on the next new or the method call statement. Imagine that you may say:

var testA = new DifferentA();
var testContainer = container.WithInterpretationOverload(implType => implType switch {
  typeof(A) => testA,
  _ => null // go the normal route
});

Cons:

  • This may be not working for all cases

Pros:

  • It has a good quality to be orthogonal to the normal resolution rules. The interpretation happens after the resolution.
  • No need to worry about the cache, because you overwrite after you get the expression from the cache.
  • You don't complicate and pay the performance price in the prod configuration by inserting the special setup just for tests, e.g. asResolutionCall.

@dadhi dadhi reopened this Mar 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants