Reconsider property injection with required properties

[AntMaster7]

Feature requests for property injection have been made several times in the past and they were always closed. However, now that we have support for required properties in C# I think it would make perfect sense to support them through the default DI container in dotnet core.

If thats definitely not gonna be supported it would be nice to have the default DI container designed such that it could be extended by inheriting from it to add this functionailty in customer projects. The current implementation of the DI container registers itself as the implementation for the IServiceProvider making it impossible to extended the default service provider.

[ghost]

Tagging subscribers to this area: @dotnet/area-extensions-dependencyinjection See info in area-owners.md if you want to be subscribed.

Issue Details

---

Feature requests for property injection have been made several times in the past and they were always closed. However, now that we have support for required properties in C# I think it would make perfect sense to support them through the default DI container in dotnet core. If thats definitely not gonna be supported it would be nice to have the default DI container designed such that it could be extended by inheriting from it to add this functionailty in customer projects. The current implementation of the DI container registers itself as the implementation for the IServiceProvider making it impossible to extended the default service provider.

Author:	AntMaster7
Assignees:	-
Labels:	`untriaged`, `area-Extensions-DependencyInjection`
Milestone:	-

[steveharter]

Can you provide a proposed API?

I assume by adding protected members to DefaultServiceProviderFactory.

[ghost]

This issue has been marked needs-author-action and may be missing some important information.

[AntMaster7]

@steveharter My idea would be to have a protected virtual OnRealize method on the ServiceProvider (removing the sealed modifier) that gets called whenever a service is actually "realized" but not when its loaded from the cache (scope). This would then allow one to create a decorated service provider like so:

public class DecoratedServiceProvider : ServiceProvider
{
    public DecoratedServiceProvider(ICollection<ServiceDescriptor> serviceDescriptors, ServiceProviderOptions options) : base(serviceDescriptors, options)
    {
    }

    protected override void OnRealize(Type serviceType, object service)
    {
        // Decorate with for example property injection
    }
}

A simple implementation of the IServiceProviderFactory can then return this custom service provider instance.

To make this work in a local demo I have changed the constructor of the ServiceProvider such that it uses the actual instance type as implementation for the internal IServiceProvider registration:

[RequiresDynamicCode(RequiresDynamicCodeMessage)]
protected internal ServiceProvider(ICollection<ServiceDescriptor> serviceDescriptors, ServiceProviderOptions options)
{
    // ...
    CallSiteFactory = new CallSiteFactory(serviceDescriptors);
    // The list of built in services that aren't part of the list of service descriptors
    // keep this in sync with CallSiteFactory.IsService
    CallSiteFactory.Add(typeof(IServiceProvider), new ServiceProviderCallSite(GetType())); // CHANGE HERE
    CallSiteFactory.Add(typeof(IServiceScopeFactory), new ConstantCallSite(typeof(IServiceScopeFactory), Root));
    CallSiteFactory.Add(typeof(IServiceProviderIsService), new ConstantCallSite(typeof(IServiceProviderIsService), CallSiteFactory));
    // ...
}

I have the modified the internal CreateServiceAccessor method like this:

protected virtual void OnRealize(Type serviceType, object service) { }

// ....

[RequiresDynamicCode(RequiresDynamicCodeMessage)]
private Func<ServiceProviderEngineScope, object?> CreateServiceAccessor(Type serviceType)
{
    ServiceCallSite? callSite = CallSiteFactory.GetCallSite(serviceType, new CallSiteChain());
    if (callSite != null)
    {
        DependencyInjectionEventSource.Log.CallSiteBuilt(this, serviceType, callSite);
        OnCreate(callSite);

        // Optimize singleton case
        if (callSite.Cache.Location == CallSiteResultCacheLocation.Root)
        {
            object? value = CallSiteRuntimeResolver.Instance.Resolve(callSite, Root);

            if(value is not null)
            {
                OnRealize(serviceType, value);
            }

            return scope => value;
        }

        return (ServiceProviderEngineScope engineScope) =>
        {
            var previousValue = callSite.Value;
            var service = _engine.RealizeService(callSite)(engineScope);

            if (service is not null && service != callSite.Value)
            {
                OnRealize(serviceType, service);
            }

            return service;
        };
    }

    return _ => null;
}      

The ReplaceServiceAccessor method would have to be changed accordingly.

The only use case I see for this proposed change is to allow users to implement property injection on top of the existing infrastructure with minimal effort. This could be documented and would in a way add indirect support for property injection. However, most people will probably not use this "extended api" and this begs the question whether wrapping the RealizeService call into a closure is a performance issue. Although it could probably be made conditional by having an additional constructor thats protected instead of the "main" constructor and setting some flag when the class is inherited and only then wrap the RealizeService call into a closure.

[davidfowl]

I don’t think this should be an extensibility point. We either do this feature or we don’t. I’d like to understand if other container authors are headed down this route before we do anything.

[AntMaster7]

@dotnetjunkie @tillig What do you think about whether dotnet should support property injection or not with required properties? Both of your containers support this and you're experts on the area of DI containers.

[davidfowl]

@z4kn4fein @dadhi @ipjohnson @alexmg @tillig @pakrym @ENikS @seesharper @jeremydmiller @alistairjevans @halter73 @steveharter

[adaris-it]

Supporting property injection is already a common use case in Blazor. I don't see a single reason why this shouldn't be supported as a general feature of the default DI.

[dadhi]

@davidfowl In the current DryIoc version (v5) it is the opt-in via the configuration Rule.

[davidfowl]

@dadhi required properties? or properties in general?

[alistairjevans]

As @AntMaster7 has noted, Autofac has supported required properties since .NET7 (I implemented the support, see https://autofac.readthedocs.io/en/latest/register/prop-method-injection.html#required-properties).

Required properties are treated as "another mandatory constructor parameter" from the perspective of instance creation. Effectively guaranteeing Autofac won't return the new object without successfully resolving the object.

I broadly like the feature for .NET DI providers, as to whether the default MSDI container should support required properties...I'd rather keep the conforming container simple and not add more requirements to it; if users want to mandate required properties, third party options exist, i.e. Autofac, DryIoc, and others.

[AntMaster7]

@alistairjevans I think with keyed dependecies added in .NET 8 the default DI container is no longer just a simple container.

[alistairjevans]

@AntMaster7, I agree with that statement, and that has downsides for conforming containers. AnyKey, for example, is a challenge for a variety of reasons due to the behaviour it implies.

[AntMaster7]

PropertyInjection comes especially in handy when implementing / using application frameworks. They usually have some sort of a service base class like an ApplicationService for example. As a user of the framework, I want to inherit from these base services / components without having to forward all sorts of constructor parameters to the parent.

[alistairjevans]

@AntMaster7, out of curiosity, if you want property injection, why not use one of the third party containers that support it?

[AntMaster7]

@alistairjevans Right now we use AutoFac. But I of course prefer to have as little dependencies as possible and stick with the basic tools. This also adds the requirement for any user of our framework to also hook up AutoFac in any "customer project". And as far as I know performance of the default container is amazing. ABP (popular asp.net core framework) seems to have a similar requirement. They also use AutoFac. I don't know what other DI features they use. But property injection is one of them. This allows to have clean service implementations like this. The service only has to care about its own dependencies (the actual repository). The ApplicationService class has lots of dependencies and it would be horrible to forward all them in every service implementation.

@hikalkan

[dadhi]

@davidfowl Required and the rest via the different opt-in rules for the DryIoc

[pakrym]

In my opinion, adding requirements for conforming containers should be done based on the ASP.NET Core/Extension features they unlock. For example, IsService unlocked fast controller activation; if we had named services at the time of Options design we would've done those differently, etc.

Pure DI features that don't have cross-cutting framework impact should be left to third-party containers.

Unfortunately, this makes MEDI feel limited and outdated because MEDI == conforming container and must have the minimal required features set.

MEDI, being the minimal container, solves two goals:

1. Ensures the framework doesn't use features that are not part of the conforming container.
2. Ensures easy migration from MEDI to 3rd party containers.

It's feasible to make MEDI the superset of the conforming container (similar to other 3rd party DI implementations) and add features that only users can access (and do additional testing to ensure that the framework doesn't rely on them).

This makes it harder for established users of MEDI to migrate to 3rd party containers but allows MEDI to feel modern and add pure DI features without expanding conforming container rules.

[robertcoltheart]

I'd love to see this added. A great use case would be for a required ILogger property, which allows the class to access logging and simplifies unit testing, meaning unit tests don't have to pass in the null logger in constructors everywhere.

[TonyValenti]

Just posting here to say I'd love to see this added. I've been using this in AutoFac ever since it was added and I can definitely say this is a huge benefit that massively reduces a large amount of boilerplate constructors.

[steveharter]

I do think the property injection is more palatable now with required properties. However, it is unlikely to be implemented in the 9.0 timeframe so moving to future.

[AntMaster7]

I came up with the idea of doing the property injection manually as a workaround by using the service locator pattern inside the constructor of the class and resolve the "dependent properties". In order to do that I need to reference the root service provider in a static variable somewhere. But I'm not sure if thats a really bad idea or if thats fine?

[julealgon]

@robertcoltheart

I'd love to see this added. A great use case would be for a required ILogger property, which allows the class to access logging and simplifies unit testing, meaning unit tests don't have to pass in the null logger in constructors everywhere.

Can you elaborate how this would simplify unit tests? Don't you still need to provide the same null logger during initialization, but as a property instead of a ctor parameter?

[julealgon]

@TonyValenti

Just posting here to say I'd love to see this added. I've been using this in AutoFac ever since it was added and I can definitely say this is a huge benefit that massively reduces a large amount of boilerplate constructors.

Would you mind sharing an example of such massive reduction in boilerplate code?

[TonyValenti]

Yes. In the "traditional" method, you create properties, then a constructor that accepts values and assigns them to the properties. If you have inheritance, then you have to duplicate tons of constructor code.

With required properties in Autofac, it is awesome because I just put "required" in front of my properties and never have to deal with duplicating all the boilerplate constructor code.

[julealgon]

Ok @TonyValenti so you are talking exclusively about boilerplate code in an inheritance scenario where the base class requires dependencies, is that it?

That dramatically dimishes the usefulness of this feature IMHO as that's a very specific use case.

I also wanna say one should actively try to avoid that scenario in the first place and prefer decorators and composition in general. If you have deep inheritance hierarchies with dependencies that usually signals a problem with the design IMHO.

[robertcoltheart]

@robertcoltheart

I'd love to see this added. A great use case would be for a required ILogger property, which allows the class to access logging and simplifies unit testing, meaning unit tests don't have to pass in the null logger in constructors everywhere.

Can you elaborate how this would simplify unit tests? Don't you still need to provide the same null logger during initialization, but as a property instead of a ctor parameter?

This is how I would structure a service that requires logging and also simplifies unit testing. It means that I don't need to pass in a null logger via the constructor for every unit test.

public class Service
{
    // Defaults to the null logger (for unit tests) but injects a real logger when using ME.DI
    public required ILogger<Service> Logger { get; set; } = NullLogger<Service>.Instance;

    public void DoSomething()
    {
        Logger.LogInformation("my code");
    }
}

public class Test
{
    [Fact]
    public void TestThatItWorks()
    {
        var service = new Service();
        service.DoSomething();
    }
}

[TonyValenti]

To piggy back on the above example from @julealgon - Imagine if you had a "ServiceBase" class that required only a logger and db context. Every derived class would need to recapture those and pass them to a base constructor. Not so with required properties.

Five minutes of using required properties via AutoFac is enough to make just about anyone a supporter of this feature.

[julealgon]

@robertcoltheart

public class Service
{
    // Defaults to the null logger (for unit tests) but injects a real logger when using ME.DI
    public required ILogger<Service> Logger { get; set; } = NullLogger<Service>.Instance;

    public void DoSomething()
    {
        Logger.LogInformation("my code");
    }
}

public class Test
{
    [Fact]
    public void TestThatItWorks()
    {
        var service = new Service();
        service.DoSomething();
    }
}

Oh I see... you are defaulting it to the null logger in the implementation, that makes sense.

Now... personally, I think this is really bad. First, because I absolutely hate seeing "for unit test" things in real code. And second, because it shouldn't be up to the class to define a default for the logger (IMHO).

This approach you are using also treats the logger as a "second class citizen", "optional dependency", which I'm usually very opposed to: to me, logging calls are behavior that should be validated/mocked/etc, not ignored like what you are doing. If the logging behavior is "secondary" on your implementation, then it should probably be moved into a decorator implementation where it becomes primary behavior.

Besides, you are not saving that much code by just not passing the null logger (or a mocked logger) in the new Service() call there... it is just a few extra characters.

---

@TonyValenti

To piggy back on the above example from @julealgon - Imagine if you had a "ServiceBase" class that required only a logger and db context. Every derived class would need to recapture those and pass them to a base constructor. Not so with required properties.

Five minutes of using required properties via AutoFac is enough to make just about anyone a supporter of this feature.

Why do you have a design where all services inherit from a ServiceBase though? That is usually really nasty design IMHO. Perhaps you are using inheritance as a means to share helper code of some sort, or using a base class to implement a template pattern? I'd again suggest reconsidering your design if you rely on this so much: switch to some form of composition (like decorators) and you completely eliminate the issue.

[AntMaster7]

@julealgon

Ok @TonyValenti so you are talking exclusively about boilerplate code in an inheritance scenario where the base class requires dependencies, is that it?

That dramatically dimishes the usefulness of this feature IMHO as that's a very specific use case.

I also wanna say one should actively try to avoid that scenario in the first place and prefer decorators and composition in general. If you have deep inheritance hierarchies with dependencies that usually signals a problem with the design IMHO.

For me thats a major reason for property injection as well. Often you have something like a ServiceBase class or something similar thats specific to your application framework (see abp.io for example). We use a common base class for request handlers in our own application framework. This doesn't mean a big inheritance hierarchy. Each controller in ASP.NET Core has to inherit from ControllerBase. Thats perfectly fine. Anyway, there are some services that almost every service or request handler needs like dto mappers, logger, unit of work, permission service, user service and whatever you can think of. Its a burden if you always have to forward all these dependencies to the base constructor. And this is composition =).

C# or ASP.NET Core contain a lot of features with limited use. But these features are still important. Keyed services for example. I personally have no use for them but that doesn't mean they are useless because their use is limited to a special case.

Another aspect is that it would simply be incorrect if MEDI ignores required properties because required means the instance can be sure that these properties are not null. But if MEDI ignores required properties we basically have an instance in an invalid state. But thats more of an academic reasoning.

[julealgon]

@AntMaster7

@julealgon

Ok @TonyValenti so you are talking exclusively about boilerplate code in an inheritance scenario where the base class requires dependencies, is that it? That dramatically dimishes the usefulness of this feature IMHO as that's a very specific use case. I also wanna say one should actively try to avoid that scenario in the first place and prefer decorators and composition in general. If you have deep inheritance hierarchies with dependencies that usually signals a problem with the design IMHO.

For me thats a major reason for property injection as well. Often you have something like a ServiceBase class or something similar thats specific to your application framework (see abp.io for example). We use a common base class for request handlers in our own application framework. This doesn't mean a big inheritance hierarchy. Each controller in ASP.NET Core has to inherit from ControllerBase. Thats perfectly fine. Anyway, there are some services that almost every service or request handler needs like dto mappers, logger, unit of work, permission service, user service and whatever you can think of. Its a burden if you always have to forward all these dependencies to the base constructor. And this is composition =).

Well AspNetCore has controllers, but most of the capabilities that require special services and hooks rely on external pipelines with the filters mechanism, so not everything is built into the base. If you rely on so many different services in your base class, it doesn't really matter whether or not it is a "deep" inheritance chain, it's still bad IMHO. I'd suggest reconsidering your design to avoid this strong of a dependency in a base class doing all the work, but alas.

C# or ASP.NET Core contain a lot of features with limited use. But these features are still important. Keyed services for example. I personally have no use for them but that doesn't mean they are useless because their use is limited to a special case.

Keyed services have much broader applicability, and they don't push you into bad practices like "property injection" does. While required properties do alleviate the "bad practice" aspect of property injection, I still don't think it is worth pursuing just for corner cases related to (IMHO poorly-designed) base classes.

Another aspect is that it would simply be incorrect if MEDI ignores required properties because required means the instance can be sure that these properties are not null. But if MEDI ignores required properties we basically have an instance in an invalid state. But thats more of an academic reasoning.

I'd say MEDI should just throw something like InvalidOperationException if a required property is detected in an injected service. As you well put, if it just ignores it, it can lead to an invalid object; invariants are broken and consuming code can start to misbehave because of uninitialized members.

[AntMaster7]

@julealgon I wouldn't call the dependence on multiple dependencies bad in general. There are various approaches to build applications. We took the approach that best serves our need. You might have different needs and requirements. Many of the IoC containers out there support property injection out of the box.

Let me give you a concrete example and let me know how you would solve this. I'm always open to learn better ways to do something:

We use the mediator pattern after we realised that the service approach get messy real quick. That is, we have a handler for each business case. The goal is to have as little boilerplate code as possible because we need to create many of these handlers. A handler then might look like the following:

public class UpdateContactHandler : RequestHandlerBase<CreateUpdateContact, ContactDto>
{
    public override async Task<ContactDto> Handle(CreateUpdateContact request, CancellationToken cancellationToken)
    {
        // common dependencies: permission service, repository, mapper

        ArgumentNullException.ThrowIfNull(request.Contact.Id);

        Contact contact = Repo.GetById<Contact>(request.Contact.Id.Value);

        PermissionService.RequirePermission(contact, DomainOperation.Update);

        // ... property mapping

        await Repo.CommitAsync(false);

        return await DtoMapper.ToDynamicDto<ContactDto>(contact, ContactDto.FieldSet);
    }
}

Its a simplification of things. But the actual handlers are pretty much like this. In this simple example it would be possible to put the permission check into a middleware. But there are more complex cases that would require the duplication of a lot of business logic in the middleware and thats not something we want. We decided that it's the handlers responsibility to only execute requests its allowed to.

[julealgon]

@AntMaster7

@julealgon I wouldn't call the dependence on multiple dependencies bad in general. There are various approaches to build applications. We took the approach that best serves our need. You might have different needs and requirements.

Fair enough, this is fairly subjective. I will say that relying on a base class with dependencies as a central mechanism is not something I personally like, I'll put it that way. I try to reduce the need for base classes in my applications to the bare minimum, particularly when they need injected services.

Many of the IoC containers out there support property injection out of the box.

This is true, and unfortunate in my opinion. If I had to guess why this ended up being the case, I'd assume it is because of older frameworks such as AspNet WebForms which had no way to perform constructor-level injection as no abstraction was exposed to intercept the object construction step, so containers at the time were basically forced to support property injection workarounds to be able to inject services after pages/controls were created.

This was also the case with WCF classes I'm pretty sure and other similarly old frameworks.

I'd hope if the ecosystem was like it is today, where mostly everything is created in a DI-friendly manner, that most if not all containers did not have any need to support property injection.

Let me give you a concrete example and let me know how you would solve this. I'm always open to learn better ways to do something:

We use the mediator pattern after we realised that the service approach get messy real quick. That is, we have a handler for each business case. The goal is to have as little boilerplate code as possible because we need to create many of these handlers. A handler then might look like the following:

public class UpdateContactHandler : RequestHandlerBase<CreateUpdateContact, ContactDto>
{
    public override async Task<ContactDto> Handle(CreateUpdateContact request, CancellationToken cancellationToken)
    {
        // common dependencies: permission service, repository, mapper

        ArgumentNullException.ThrowIfNull(request.Contact.Id);

        Contact contact = Repo.GetById<Contact>(request.Contact.Id.Value);

        PermissionService.RequirePermission(contact, DomainOperation.Update);

        // ... property mapping

        await Repo.CommitAsync(false);

        return await DtoMapper.ToDynamicDto<ContactDto>(contact, ContactDto.FieldSet);
    }
}

Its a simplification of things. But the actual handlers are pretty much like this. In this simple example it would be possible to put the permission check into a middleware. But there are more complex cases that would require the duplication of a lot of business logic in the middleware and thats not something we want. We decided that it's the handlers responsibility to only execute requests its allowed to.

So immediately when looking at the code, I don't see a particular reason why Repo and DtoMapper are base class members instead of just being injected abstractions. That's the type of "code sharing" that I really dislike when used with inheritance that can be easily refactored into composition. It also appears like you are doing "resource-based authorization", which has a native abstraction you can also use. If this was me, the code would look maybe something like this:

public class UpdateContactHandler : RequestHandler<CreateUpdateContact, ContactDto>
{
    private readonly IRepository<Contact> contactRepository;
    private readonly IMapper<Contact, ContactDto> contactMapper;
    private readonly IAuthorizationService authorizationService;

    public UpdateContactHandler(
        IRepository<Contact> contactRepository,
        IMapper<Contact, ContactDto> contactMapper,
        IAuthorizationService authorizationService)
    {
        this.contactRepository = contactRepository;
        this.contactMapper = contactMapper;
        this.authorizationService = authorizationService;
    }

    public override async Task<ContactDto> Handle(CreateUpdateContact request, CancellationToken cancellationToken)
    {
        ArgumentNullException.ThrowIfNull(request.Contact.Id);

        Contact contact = await this.contactRepository.GetByIdAsync(request.Contact.Id.Value);

        var authorizationResult = await _authorizationService.AuthorizeAsync(request.User, contact, Operations.Update);

        // Handle authorization result here

        await this.contactRepository.CommitAsync(false);

        return await this.contactMapper.ToDynamicDto(contact, ContactDto.FieldSet);
    }
}

No base classes needed at all.

[tillig]

I don't have a horse in this race, but I thought I'd respond to this:

If I had to guess why this ended up being the case, I'd assume it is because of older frameworks such as AspNet WebForms which had no way to perform constructor-level injection

There were largely three use cases I recall:

1. What you mentioned - that some frameworks didn't support constructor injection.
2. Required vs. optional dependencies - constructor dependencies would be considered required, while properties may or may not be available. (This is less interesting once the concept of "required properties" comes about, but from a historical perspective it's interesting to know.)
3. Circular dependencies - for cases where a dependency gets created and, say, needs to link to its owner, you can break the circle a little by having the owner be injected as a property rather than as a constructor dependency.

Again, no horse in this race - I'm not saying these are good use cases, just saying these are things that historically needed to be addressed.

[AntMaster7]

@julealgon Thank you for the detailed answer. The updated example does have the benefit of really only requiring the dependencies that are needed. But this comes with the trade-off of more boilerplate code. And its the latter that I try to avoid. I believe it comes down to personal preference like iOS vs. Android. Thats an endless discussion.