ipjohnson / Grace

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

Locate ignores extraData #242

Closed Forester- closed 4 years ago

Forester- commented 4 years ago

Grace 7.1.0.0 VS2019 16.3.10 Target .NET Framework 4.6.2

Locate method ignores extraData parameter, creates new instance for constructor parameter value instead of passing extraData value.

Sample:

using System;
using Grace.DependencyInjection;

namespace Grace.Err1
{
    class A
    {
        public readonly B b;
        public A(B b)
        {
            this.b = b;
        }
    }
    class B
    {
        public string c = "Default";
        public B()
        {
        }
    }
    class Program
    {
        static void Main(string[] args)
        {
            var container = new DependencyInjectionContainer();
            container.Configure(registry =>
            {
                registry.Export<A>();
            });
            var a2 = container.Locate<A>(new { b = new B { c = "New C" } });
            Console.WriteLine($"{a2.b.c}");
        }
    }
}

Expected output:

New C

Actual output:

Default

Workaround - disable auto registering types:

            var container = new DependencyInjectionContainer(config =>
            {
                config.AutoRegisterUnknown = false;
            });
ipjohnson commented 4 years ago

Hi @Forester-

That's not really a bug more of a design decision. Grace by default will only use extra data to fill in dependencies it doesn't know how to fulfill. With auto registration turned on type B will be constructed automatically.

You can blacklist types from being auto registered or you can change the way A is registered.

Is this something you're doing a lot of?

Forester- commented 4 years ago

It was the first time.

Before I used StructureMap/Lamar, MS ASP.NET Core DI-frameworks in ASP.NET MVC. There were no such problems, because there were no statefull services.

Now I'm exploring WPF and MVVM. I like DI paradigm and would like to use it with WPF and MVVM. Perhaps, I understand something wrong. In MVVM we have the View, the ModelView and the Model in the following relations: View <-> ModelView <-> Model In my mind the Model just store data, it does nothing (sometimes the Model is absent when there is no need to store data). The View just show data. The ModelView is "active" object, it interacts with different application services. Hence the ModelView has to get these services in constructor. And the ModelView itself has to be instanciated with DI-container. I don't have difficulty with the View <-> ModelView relation. But I don't have enough understanding with the ModelView <-> Model relation. In my mind the Model is part of the ModelView. There should be no way to replace the Model object of the ModelView object. So I see it as:

class ModelView
{
    private readonly Model _model;
}

Here _model has to be initialized in the constructor.

For example lets take some app with CRUD operations over some entity. These app will have store service and some editor for entity. The entity instance is the Model. The editor is View <-> ModelView <-> Model composition. For create, read and update operations the app have to show the editor with new or selected entity. How the app will pass entity instance in the editor and its ModelView? We need to get the ModelView from DI-container and pass the entity in the ModelView constructor.

I've tried to use Lamar but it doesn't provide ability to pass values to constructor (only static values). So I've found Grace with extraData.

Ofcourse, it is possible to use something like Options from ASP.NET Core. We may have PassValueService as singleton, store our entity instance in it and get PassValueService with the entity instance in the ModelView constructor. But I would like to avoid additional services.

As a summary I'm not sure that my thoughts about MVVM are right and DI-container needs the ability to pass values to constructor.

So, about this issue. In my mind the default behaviour of DI-container (and Locate() method) is to get values for constructor parameters from container. Signature of the Locate(extraData) method shows it extends this behaviour by passing extraData to constructor. And it is strange that in some configuration it passes the data, but in others doesn't. It means that the Locate(extraData) sometimes works and sometimes doesn't. Then it will be better to make something like ExtraDataContainer and to place Locate(extraData) only in it. Or rename extraData to something like fallbackData. But I don't think the Locate method needs fallbackData. Such case is the configuration error and should be corrected in the container config.

Thank you for your attention! =)

uygary commented 4 years ago

I'm aware I'm stepping outside the issue that was raised, but I think it's worth pointing a few things out here:

@Forester- in MVVM, the ViewModel would normally rely on service classes to access model instances. Ie, it would take in an IRepository in its constructor via dependency injection, and then use methods on that IRepository to retrieve the model instances that will be displayed, or save the modified instances back into the database. In a case like you mentioned, where the data will not be persisted or even retrieved from a database, I don't think there actually is data apart from the state on the ViewModel. Meaning, you don't actually have a Model object, all you need is an internal state on the ViewModel instance. That could translate into simply having the relevant properties on the viewmodel.

That being said, if you still think you need to instantiate a default model that won't be persisted for whatever reason, I'd rather inject an IDefaultModelFactory, or something like that, and have the model built using that factory service rather than inject a model instance. From my perspective, what you're exactly after is kind of outside the responsibilities of a DI library, not to mention outside the intended use of the MVVM pattern.

Forester- commented 4 years ago

@uygary thank you for your replay. I would like to discuss this case, but I don't want to litter this issue. So I invite you to continue the dicussion in one of my repos https://github.com/Forester-/EFCoreNavPropInit/issues/1

ipjohnson commented 4 years ago

@Forester- if the issue ultimately is that you can't pass in a model through extra data I think it's reasonable just to black list the namespace your models are in so they aren't auto-registered. You can black list a whole namespace using the configuration method ExcludeTypeFromAutoRegistration("Namespace.*")

Forester- commented 4 years ago

@ipjohnson Thank you for your advise. I've decided to use the two-stage initialization. Now I'm trying to completely hide the initialization details inside the factory.

Speaking in general, I don't like the two-stage initialization. It's some kind of workaround. So I'd like the DI-containers developers will think how to pass parameters values to the constructor during instance creation. ;-) In my mind it is the needful case.

Thank you for your work and your help!

ipjohnson commented 4 years ago

I think you’re misunderstanding something because Grace does support passing parameters to constructors through extra data. If you disable auto registration for the type you want to pass it will use it out of extra data.

Forester- commented 4 years ago

@ipjohnson As you have said "Grace by default will only use extra data to fill in dependencies it doesn't know how to fulfill." So, yes, it supports passing extra data, but only as fallback usecase of parameter resolution. I'd like to see it as main usecase, that is independent of configuration.

silkfire commented 4 years ago

Perhaps a LocateWithExtraData method could be added that overrides/ignores the AutoRegisterUnknown setting?

ipjohnson commented 4 years ago

@Forester- technically speaking Grace can be configured to do what I you want but I usually recommend not using it because it's much much slower and most use cases can be solved without having to do what you're looking for.

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

There is a reason why most containers don't support this behavior at all. It leads to questions about which dependency is used and ultimately make the activation delegate more complex and slower.

Forester- commented 4 years ago

@ipjohnson I took a look on the DynamicConstructorExpressionCreator. It seems it is that i was looking for.

I've implemented two-stage initialization already and will not remake it. When I will make the same thing I'll try to use ConstructorSelectionMethod.Dynamic.

Thank you!

ipjohnson commented 4 years ago

@Forester- sounds good. I'm going to close this issue out.