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

Dependency Injection - Behaviour on constructor overload selection needs update. #6331

Closed
tiakun opened this issue May 10, 2018 · 12 comments
Closed
Assignees
Labels
Source - Docs.ms Docs Customer feedback via GitHub Issue

Comments

@tiakun
Copy link

tiakun commented May 10, 2018

The document said that constructor overloads are supported only when no more than one overloaded constructor can be fulfilled, but this is not exactly correct as of .NET Core 2.0 (I am not really sure when it started).

Here is the current text:

Constructor injection requires that only one applicable constructor exist. Constructor overloads are supported, but only one overload can exist whose arguments can all be fulfilled by dependency injection. If more than one exists, your app will throw an InvalidOperationException

https://github.com/aspnet/Docs/blob/master/aspnetcore/fundamentals/dependency-injection.md

However, the latest version of DI allow us to have multiple constructor overloads which their set of parameters are subset/superset of each other and fulfil the largest parameter set possible. For example, we can have

class Foo {
Foo(IBar bar) { }
Foo(IBar bar, IOptional optionalService) { }
}

If IOptional is registered, then both overloads can be fulfilled hence it should throw an exception if the document were accurate. However, the current version of DI will not throw exception but will choose second overload because its parameter set is the superset of all other applicable overloads.

The reason other than making this accurate is that it can be used to inject optional service, which I believe is useful for many developers.

@Rick-Anderson
Copy link
Contributor

@dougbu or @ardalis can you comment?

@Rick-Anderson Rick-Anderson added the Source - Docs.ms Docs Customer feedback via GitHub Issue label May 11, 2018
@Rick-Anderson
Copy link
Contributor

@stevejgordon how does this fit with #6312

@stevejgordon
Copy link
Contributor

@Rick-Anderson It feels like it could be worthy of some expansion if we make a more advanced section.

If the current published advice is no longer valid it might be relevant to fix that more immediately. I guess a tabbed section can be added to show relevant advice for the different versions. We need to confirm when/if the behaviour changed.

I don't think I've registered services with multiple ctors to see this in action (but I will be trying this later now I've read this)!

It feels like the existing doc should state the high level rule regarding how a ctor is matched / when exceptions will be thrown. If we feel there's more nuance and detail, perhaps link to an advanced section?

@tiakun
Copy link
Author

tiakun commented May 11, 2018

Additionally, making last parameters as null default also working, e.g.

class Foo {
    Foo(IBar bar, IOptional optionalService = null) { }
}

From my test, it seems to cover the same as as in the original post. And if there are more than one default arguments, all default arguments would be treated as optional by DI framework.

PS: My test didn't work in 1.0, so I believe these features are added in 2.0

@dougbu
Copy link
Member

dougbu commented May 11, 2018


One caveat: As @tiakun mentioned later in their "used to inject optional service" comment, the following is not always true.

the current version of DI will not throw exception but will choose second overload because its parameter set is the superset of all other applicable overloads

DI will choose the second overload only if an IOptional service is registered. DI will not throw an Exception when only an IBar service is registered.

@pakrym
Copy link
Contributor

pakrym commented May 11, 2018

Unfortunately, we have two resolution mechanisms in DependencyInjection that have different rules:

  1. Resolving services from IServiceProvider directly
  2. Using ActivatorUtilities to create instances

First one happens most of the time for services, but for some user-facing abstractions like TagHelpers/Controllers/SignaR Hubs/ModelBinders/etc. when they were not registered in DI we use ActivatorUtilities class that allows creating objects without registering them beforehand.

Rules described in the Constructor injection behaviour section are applicable only to ActivatorUtilities based resolution. IServiceProvider based resolution allows constructor overloads and private constructors.

@stevejgordon
Copy link
Contributor

@pakrym I managed to put together a quick test project yesterday which matches with your comments. The only one where I want to confirm is around private ctors in IServiceProvider based resolution. I found that DI ignored private ctors and if all where private it threw an exception. Is that as expected?

This was a quick test on 2.1. I may do a little more digging.

@pakrym
Copy link
Contributor

pakrym commented May 14, 2018

My bad, constructor has to be public. Mixed it up with support for internal types.

@stevejgordon
Copy link
Contributor

@Rick-Anderson If you want I can include any clarifications in the main doc along with the work in #6312? I could potentially include a section there that explains the two resolution mechanisms and the rules which apply to both?

@Rick-Anderson
Copy link
Contributor

OK by me unless @pakrym objects. He'll be reviewing the PR, not me.

@pakrym
Copy link
Contributor

pakrym commented May 18, 2018

I'm fine with that, we just need a good list of when it happens. I don't want people to be overly consfused.

@perlun
Copy link

perlun commented Feb 3, 2025

Sorry to post on an age-old topic, but here goes...

What's the current "correct" way to mark a constructor as "this is the primary constructor that should be used with DI"? I found out about [ActivatorUtilitiesConstructor](https://learn.microsoft.com/dotnet/api/microsoft.extensions.dependencyinjection.activatorutilitiesconstructorattribute), but I believe the service is being resolved by IServiceProvider in my case - since adding this attribute seems to "change nothing" when running my program.

Just to make things clear: the dependency resolution works in my case, the correct constructor is selected. However, coming from a Java/Spring background recently, I feel like I'm lacking the attribute for saying "this is the primary constructor, always choose this when possible". The https://github.com/dotnet/AspNetCore.Docs/blob/main/aspnetcore/fundamentals/dependency-injection.md document doesn't seem to be mention overloads at all nowadays.

It would feel "safer" to be able to mark a constructor like this, or at least be able to read the canonical "this is how it works" guide in ASP.NET Core. The behavior mentioned in this issue - that the DI subsystem/IOC container tries to "fulfil the largest parameter set possible" (@tiakun), I guess this is documented somewhere? If so, where?


My use case more specifically: I recently added a second constructor to one of our classes (SomeManager) and the DI subsystem seems to select the first overload below. This is interesting and "correct" but I would understand more about why it does this. I guess it's because there's no string service registered? Regardless, some kind of [PrimaryConstructor] approach would feel safe/nice. 😛

public SomeManager(ISomeService someService) {
    // ...
}

public SomeManager(string url) {
    // ...
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Source - Docs.ms Docs Customer feedback via GitHub Issue
Projects
None yet
Development

No branches or pull requests

7 participants