-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Add Keyed Services Support to Dependency Injection #87183
Conversation
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
Tagging subscribers to this area: @dotnet/area-extensions-dependencyinjection Issue DetailsRelated issue: #64427 Since my last proposal (https://gist.github.com/benjaminpetit/49a6b01692d0089b1d0d14558017efbc) we made some changes and took some decisions. This PR is still in draft, but everything is working and can be reviewed. Naming is not definitive, and some exception needs to be replaced. Changes since the proposal:
|
src/libraries/Microsoft.Bcl.AsyncInterfaces/src/CompatibilitySuppressions.xml
Outdated
Show resolved
Hide resolved
Shouldn't there be some specification tests to ensure containers that implement this are doing it right? The spec tests also indicate how the .NET team intends to use the feature (to "document" both explicit and implicit behavior requirements) so having that would be helpful. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, I really like the approach. I'm a big fan of wrapping the key and type in ServiceIdentifier
. I do think we should use string
instead of object
for the name. I also think we should do more to fail loudly when used with incompatible containers.
Stuff like IServiceProviderIsService support can probably come later. We should definitely fix the _realizedServices
caching bug though.
...ependencyInjection.Abstractions/ref/Microsoft.Extensions.DependencyInjection.Abstractions.cs
Outdated
Show resolved
Hide resolved
...ependencyInjection.Abstractions/ref/Microsoft.Extensions.DependencyInjection.Abstractions.cs
Show resolved
Hide resolved
|
||
public Service() => _id = Guid.NewGuid().ToString(); | ||
|
||
public Service([ServiceKey] string id) => _id = id; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be invalid for a [ServiceKey]
parameter to be a string
and not an object? People will want it to alwas be a string so they can store it in a string field like you do here. I think this proves my earlier point about how it's more convenient to work with this parameter when you know it will be a string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree, people will not always want to use a string
. And here it just works fine, not sure why we should force it to be an object
, that's the responsibility of the developer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if someone tries to register this service using the AnyKey
? Does it validate when we add the service or build the ServiceProvider? Or does it just throw an InvalidCastException when you try to resolve it?
I think we should probably at least be validating the runtime service key type is assignable to the [ServiceKey]
parameter type in CallSiteFactory. This validation would make sense even if the service type had to be string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will throw InvalidCastException
, that's correct. I guess we could validate the parameter type.
...icrosoft.Extensions.DependencyInjection/tests/DI.Tests/KeyedServiceProviderContainerTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ServiceDescriptor.cs
Show resolved
Hide resolved
...ependencyInjection.Abstractions/ref/Microsoft.Extensions.DependencyInjection.Abstractions.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteFactory.cs
Outdated
Show resolved
Hide resolved
...ries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceDescriptorCacheKey.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceProvider.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceProvider.cs
Outdated
Show resolved
Hide resolved
4b6ab20
to
4e5c7eb
Compare
src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ISupportKeyedService.cs
Outdated
Show resolved
Hide resolved
I moved tests to the spec tests, I think they were all relevant. |
...ependencyInjection.Abstractions/ref/Microsoft.Extensions.DependencyInjection.Abstractions.cs
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteFactory.cs
Outdated
Show resolved
Hide resolved
...ependencyInjection.Abstractions/ref/Microsoft.Extensions.DependencyInjection.Abstractions.cs
Outdated
Show resolved
Hide resolved
...ries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceDescriptorCacheKey.cs
Outdated
Show resolved
Hide resolved
...ependencyInjection.Abstractions/ref/Microsoft.Extensions.DependencyInjection.Abstractions.cs
Outdated
Show resolved
Hide resolved
...icrosoft.Extensions.DependencyInjection/tests/DI.Tests/KeyedServiceProviderContainerTests.cs
Outdated
Show resolved
Hide resolved
...ries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceDescriptorCacheKey.cs
Outdated
Show resolved
Hide resolved
...ns.DependencyInjection.Specification.Tests/src/KeyedDependencyInjectionSpecificationTests.cs
Show resolved
Hide resolved
...crosoft.Extensions.DependencyInjection.Specification.Tests/src/CompatibilitySuppressions.xml
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ISupportKeyedService.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ServiceKeyAttribute.cs
Show resolved
Hide resolved
...ries/Microsoft.Extensions.DependencyInjection.Abstractions/src/FromKeyedServicesAttribute.cs
Show resolved
Hide resolved
src/libraries/System.Diagnostics.EventLog/src/CompatibilitySuppressions.xml
Outdated
Show resolved
Hide resolved
...ries/Microsoft.Extensions.DependencyInjection.Abstractions/src/CompatibilitySuppressions.xml
Outdated
Show resolved
Hide resolved
namespace Microsoft.Extensions.DependencyInjection | ||
{ | ||
[AttributeUsage(AttributeTargets.Parameter)] | ||
public class FromKeyedServicesAttribute : Attribute |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sealed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure, the developer can inherit from this attribute to use custom types for the key
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that a good idea though? Could that interfere with future source generators? We could always shoot of a quick email to get API approval for sealing it. It seems safer since we could always unseal later.
…ce and rename its method IsService to IsKeyedService
…ns to ServiceCollectionDescriptorExtensions
…to ServiceCollectionServiceExtensions
aac1dec
to
17b9312
Compare
...ependencyInjection.Abstractions/ref/Microsoft.Extensions.DependencyInjection.Abstractions.cs
Outdated
Show resolved
Hide resolved
...ries/Microsoft.Extensions.DependencyInjection.Abstractions/src/CompatibilitySuppressions.xml
Outdated
Show resolved
Hide resolved
...ries/Microsoft.Extensions.DependencyInjection.Abstractions/src/CompatibilitySuppressions.xml
Outdated
Show resolved
Hide resolved
...pendencyInjection.Abstractions/src/Extensions/ServiceCollectionDescriptorExtensions.Keyed.cs
Outdated
Show resolved
Hide resolved
...pendencyInjection.Abstractions/src/Extensions/ServiceCollectionDescriptorExtensions.Keyed.cs
Outdated
Show resolved
Hide resolved
...pendencyInjection.Abstractions/src/Extensions/ServiceCollectionDescriptorExtensions.Keyed.cs
Outdated
Show resolved
Hide resolved
...pendencyInjection.Abstractions/src/Extensions/ServiceCollectionDescriptorExtensions.Keyed.cs
Outdated
Show resolved
Hide resolved
...pendencyInjection.Abstractions/src/Extensions/ServiceCollectionDescriptorExtensions.Keyed.cs
Outdated
Show resolved
Hide resolved
...pendencyInjection.Abstractions/src/Extensions/ServiceCollectionDescriptorExtensions.Keyed.cs
Outdated
Show resolved
Hide resolved
...pendencyInjection.Abstractions/src/Extensions/ServiceCollectionDescriptorExtensions.Keyed.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.DependencyInjection/src/CompatibilitySuppressions.xml
Outdated
Show resolved
Hide resolved
...pendencyInjection.Abstractions/src/Extensions/ServiceCollectionDescriptorExtensions.Keyed.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ServiceDescriptor.cs
Outdated
Show resolved
Hide resolved
|
||
namespace Microsoft.Extensions.DependencyInjection | ||
{ | ||
public interface IServiceProviderIsKeyedService : IServiceProviderIsService |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making the API changes I suggested. As much as I like them, we should at least send an API review email to make sure any API adjustments are approved before merging. Or better yet discuss them in tomorrow's meeting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM pending some minor API approvals.
Related issue: #64427
Since my last proposal (https://gist.github.com/benjaminpetit/49a6b01692d0089b1d0d14558017efbc) we made some changes and took some decisions.
This PR is still in draft, but everything is working and can be reviewed. Naming is not definitive, and some exception needs to be replaced.
Changes since the proposal:
ServiceKeyAttribute
: used to inject the key used to resolve the service in the constructor (see testResolveKeyedServiceSingletonInstanceWithKeyInjection
FromKeyedServices
: used to get a keyed service from DI in the constructor (see testResolveKeyedServiceSingletonInstanceWithKeyedParameter
)