Skip to content

Fix consistency issue in GetKeyedServices with AnyKey #97561

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.

Sign up for GitHub

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

Closed
benjaminpetit wants to merge 3 commits into from
Closed

Fix consistency issue in GetKeyedServices with AnyKey #97561

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e96f6f7
Fix consistency issue in GetKeyedServices with AnyKey
benjaminpetit Jan 26, 2024
d6bacae
Do not return AnyKey registration when enumerating services with AnyK…
benjaminpetit Feb 2, 2024
7d22771
Revert solution change
benjaminpetit Feb 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Fix consistency issue in GetKeyedServices with AnyKey
  • Loading branch information
@benjaminpetit
benjaminpetit committed Jan 26, 2024
commit e96f6f7d30392f8aab6633f8bc655ee19df16b85
35 changes: 35 additions & 0 deletions ...DependencyInjection.Specification.Tests/src/KeyedDependencyInjectionSpecificationTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using System.Security.Cryptography;
using Microsoft.Extensions.DependencyInjection.Specification.Fakes;
using Xunit;
using static Microsoft.Extensions.DependencyInjection.Specification.KeyedDependencyInjectionSpecificationTests;

namespace Microsoft.Extensions.DependencyInjection.Specification
{
Expand Down Expand Up @@ -158,6 +159,40 @@ public void ResolveKeyedServicesAnyKeyWithAnyKeyRegistration()
Assert.Equal(new[] { service1, service2, service3, service4 }, allServices.Skip(1));
}

[Fact]
public void ResolveKeyedServicesAnyKeyConsistency()
{
var serviceCollection = new ServiceCollection();
var service = new Service("first-service");
serviceCollection.AddKeyedSingleton<IService>("first-service", service);

var provider1 = CreateServiceProvider(serviceCollection);
Assert.Null(provider1.GetKeyedService<IService>(KeyedService.AnyKey));
Assert.Equal(new[] { service }, provider1.GetKeyedServices<IService>(KeyedService.AnyKey));

var provider2 = CreateServiceProvider(serviceCollection);
Assert.Equal(new[] { service }, provider2.GetKeyedServices<IService>(KeyedService.AnyKey));
Assert.Null(provider2.GetKeyedService<IService>(KeyedService.AnyKey));
}

[Fact]
public void ResolveKeyedServicesAnyKeyConsistencyWithAnyKeyRegistration()
{
var serviceCollection = new ServiceCollection();
var service = new Service("first-service");
var any = new Service("any");
serviceCollection.AddKeyedSingleton<IService>("first-service", service);
serviceCollection.AddKeyedSingleton<IService>(KeyedService.AnyKey, (sp, key) => any);

var provider1 = CreateServiceProvider(serviceCollection);
Assert.Same(any, provider1.GetKeyedService<IService>(KeyedService.AnyKey));
Assert.Equal(new[] { service, any }, provider1.GetKeyedServices<IService>(KeyedService.AnyKey));

var provider2 = CreateServiceProvider(serviceCollection);
Assert.Equal(new[] { service, any }, provider2.GetKeyedServices<IService>(KeyedService.AnyKey));
Assert.Same(any, provider2.GetKeyedService<IService>(KeyedService.AnyKey));
Copy link
Member

@halter73 halter73 Feb 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that the AnyKey has special status for enumerable lookups, I think we should be careful not to imply it's doing anything special for single service resolution. The simplest thing is to simply not allow it, and only allow it for service registration and enumerable lookups.

If you have [ServiceKey] string key in the constructor of your AnyKey service, the above line will already throw, so it's not something a library or framework developer can rely on always working anyway.

System.InvalidOperationException: The type of the key used for lookup doesn't match the type in the constructor parameter with the ServiceKey attribute.

Given this, and that you can just use new object() instead if you really want to get the AnyKey registration for a service and you know it will not blow up on plain object instance, I see no reason not to always throw an InvalidOperationException when someone attempts to resolve a single service using the AnyKey. Then we also don't have to explain why the service returned when using AnyKey for single service resolution isn't included in the enumerable lookup.

Technically this is breaking like any behavior change is, but I would be surprised if anything other than our tests specifically try to resolve a single service with the AnyKey. And if anyone is doing that, they should probably stop and just use something else like new object() or "" which is easier to reason about.

Suggested change
Assert.Same(any, provider2.GetKeyedService<IService>(KeyedService.AnyKey));
Assert.ThrowsAny<InvalidOperationException>(() => provider2.GetKeyedService<IService>(KeyedService.AnyKey));
Assert.Same(any, provider2.GetKeyedService<IService>(new object()));

Less importantly, it also leaves the door open for us retconning what the AnyKey in single service resolution means. Maybe in .NET 20, we decide that it should return a non-keyed service if there are no keyed service registrations for a given type,

Copy link
Contributor

@steveharter steveharter Mar 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that we should throw on GetKeyedService(AnyKey) and also agree on the previously discussed hiding of the AnyKey registration when querying with IEnumerable<>.

}

[Fact]
public void ResolveKeyedGenericServices()
{
Expand Down
30 changes: 19 additions & 11 deletions ...ies/Microsoft.Extensions.DependencyInjection/Microsoft.Extensions.DependencyInjection.sln
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
Microsoft Visual Studio Solution File, Format Version 12.00

Microsoft Visual Studio Solution File, Format Version 12.00
# Visual Studio Version 17
VisualStudioVersion = 17.9.34414.90
MinimumVisualStudioVersion = 10.0.40219.1
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "TestUtilities", "..\Common\tests\TestUtilities\TestUtilities.csproj", "{30201F60-A891-4C3F-A1A6-DDDD1C8525E3}"
EndProject
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Microsoft.Bcl.AsyncInterfaces", "..\Microsoft.Bcl.AsyncInterfaces\ref\Microsoft.Bcl.AsyncInterfaces.csproj", "{047FD3F2-B3A0-4639-B4F0-40D29E61725D}"
Expand Down Expand Up @@ -43,11 +47,11 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "src", "src", "{74C4FAFF-491
EndProject
Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "gen", "gen", "{28065F40-B930-4A5D-95D8-A3BD5F86CE11}"
EndProject
Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "gen", "tools\gen", "{0B56ECAF-7B4A-4135-A343-1577ACE09920}"
Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "gen", "gen", "{0B56ECAF-7B4A-4135-A343-1577ACE09920}"
EndProject
Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "src", "tools\src", "{5DD887A4-ED78-4782-B372-34176C27F0C1}"
Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "src", "src", "{5DD887A4-ED78-4782-B372-34176C27F0C1}"
EndProject
Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "ref", "tools\ref", "{6FA0C03B-0901-4FD8-AF44-D4C9E5516C2F}"
Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "ref", "ref", "{6FA0C03B-0901-4FD8-AF44-D4C9E5516C2F}"
EndProject
Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "tools", "tools", "{D6F10CF9-982E-4D09-A112-0CBFD2D46AFF}"
EndProject
Expand Down Expand Up @@ -135,28 +139,32 @@ Global
EndGlobalSection
GlobalSection(NestedProjects) = preSolution
{30201F60-A891-4C3F-A1A6-DDDD1C8525E3} = {5F2D3A6A-76D3-4C22-A7F7-8D73D67A98F8}
{2C6983BB-3CB4-4EB7-9AF1-2F24FE1ECEAB} = {5F2D3A6A-76D3-4C22-A7F7-8D73D67A98F8}
{F11DD75C-122C-4B98-9EED-F71551F9562A} = {5F2D3A6A-76D3-4C22-A7F7-8D73D67A98F8}
{1EE6CA66-6585-459D-8889-666D4C2D4C27} = {5F2D3A6A-76D3-4C22-A7F7-8D73D67A98F8}
{047FD3F2-B3A0-4639-B4F0-40D29E61725D} = {E168C5B8-F2EB-4BDE-942A-59C1EB130D59}
{6D90C067-5CCD-4443-81A5-B9C385011F68} = {E168C5B8-F2EB-4BDE-942A-59C1EB130D59}
{66E6ADF5-200F-41F3-9CA4-858EF69D2A61} = {E168C5B8-F2EB-4BDE-942A-59C1EB130D59}
{3068B34E-D975-4C11-B2F2-F10790051F2E} = {74C4FAFF-491D-448C-8CA0-F8E5FC838CC5}
{6D90C067-5CCD-4443-81A5-B9C385011F68} = {E168C5B8-F2EB-4BDE-942A-59C1EB130D59}
{9CD9F9EB-379C-44C1-9016-33DFEC821C76} = {74C4FAFF-491D-448C-8CA0-F8E5FC838CC5}
{4532D9F9-1E0D-4A62-8038-D3454B255E86} = {74C4FAFF-491D-448C-8CA0-F8E5FC838CC5}
{66E6ADF5-200F-41F3-9CA4-858EF69D2A61} = {E168C5B8-F2EB-4BDE-942A-59C1EB130D59}
{C5ECD02C-FA5A-4B56-9CA2-47AD8989714A} = {74C4FAFF-491D-448C-8CA0-F8E5FC838CC5}
{2C6983BB-3CB4-4EB7-9AF1-2F24FE1ECEAB} = {5F2D3A6A-76D3-4C22-A7F7-8D73D67A98F8}
{F11DD75C-122C-4B98-9EED-F71551F9562A} = {5F2D3A6A-76D3-4C22-A7F7-8D73D67A98F8}
{1EE6CA66-6585-459D-8889-666D4C2D4C27} = {5F2D3A6A-76D3-4C22-A7F7-8D73D67A98F8}
{FD3C8D4F-0EAA-4575-A685-F77C6D340E60} = {28065F40-B930-4A5D-95D8-A3BD5F86CE11}
{9E5124E4-BEDA-4B2D-9699-60E2A7B1881D} = {28065F40-B930-4A5D-95D8-A3BD5F86CE11}
{C85FD264-C77B-44F3-926C-D61C5DAD369E} = {0B56ECAF-7B4A-4135-A343-1577ACE09920}
{A2CD66D3-74F2-4608-A56E-F866CC494620} = {0B56ECAF-7B4A-4135-A343-1577ACE09920}
{0B56ECAF-7B4A-4135-A343-1577ACE09920} = {D6F10CF9-982E-4D09-A112-0CBFD2D46AFF}
{45502850-3207-49B0-9F5D-5DE95091DB5A} = {5DD887A4-ED78-4782-B372-34176C27F0C1}
{B1E4A89A-6451-4484-B42B-4D1DF21B6961} = {5DD887A4-ED78-4782-B372-34176C27F0C1}
{5DD887A4-ED78-4782-B372-34176C27F0C1} = {D6F10CF9-982E-4D09-A112-0CBFD2D46AFF}
{7E8189DE-6BBA-4CD5-9BA6-DF5ADBD027D3} = {6FA0C03B-0901-4FD8-AF44-D4C9E5516C2F}
{0B56ECAF-7B4A-4135-A343-1577ACE09920} = {D6F10CF9-982E-4D09-A112-0CBFD2D46AFF}
{5DD887A4-ED78-4782-B372-34176C27F0C1} = {D6F10CF9-982E-4D09-A112-0CBFD2D46AFF}
{6FA0C03B-0901-4FD8-AF44-D4C9E5516C2F} = {D6F10CF9-982E-4D09-A112-0CBFD2D46AFF}
EndGlobalSection
GlobalSection(ExtensibilityGlobals) = postSolution
SolutionGuid = {68A7BDA7-8093-433C-BF7A-8A6A7560BD02}
EndGlobalSection
GlobalSection(SharedMSBuildProjectFiles) = preSolution
..\..\tools\illink\src\ILLink.Shared\ILLink.Shared.projitems*{a2cd66d3-74f2-4608-a56e-f866cc494620}*SharedItemsImports = 5
..\..\tools\illink\src\ILLink.Shared\ILLink.Shared.projitems*{b1e4a89a-6451-4484-b42b-4d1df21b6961}*SharedItemsImports = 5
EndGlobalSection
EndGlobal
18 changes: 13 additions & 5 deletions src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,11 +282,13 @@ private static bool AreCompatible(DynamicallyAccessedMemberTypes serviceDynamica
CallSiteResultCacheLocation cacheLocation = CallSiteResultCacheLocation.Root;
ServiceCallSite[] callSites;

var isAnyKeyLookup = serviceIdentifier.ServiceKey == KeyedService.AnyKey;

// If item type is not generic we can safely use descriptor cache
// Special case for KeyedService.AnyKey, we don't want to check the cache because a KeyedService.AnyKey registration
// will "hide" all the other service registration
if (!itemType.IsConstructedGenericType &&
!KeyedService.AnyKey.Equals(cacheKey.ServiceKey) &&
!isAnyKeyLookup &&
_descriptorLookup.TryGetValue(cacheKey, out ServiceDescriptorCacheItem descriptors))
{
callSites = new ServiceCallSite[descriptors.Count];
Expand Down Expand Up @@ -317,19 +319,25 @@ private static bool AreCompatible(DynamicallyAccessedMemberTypes serviceDynamica
int slot = 0;
for (int i = _descriptors.Length - 1; i >= 0; i--)
{
if (KeysMatch(_descriptors[i].ServiceKey, cacheKey.ServiceKey))
if (KeysMatch(cacheKey.ServiceKey, _descriptors[i].ServiceKey))
{
if (TryCreateExact(_descriptors[i], cacheKey, callSiteChain, slot) is { } callSite)
// Special case for AnyKey: we don't want to add in cache a mapping AnyKey -> specific type,
// so we need to ask creation with the original identity of the descriptor
var registrationKey = isAnyKeyLookup ? ServiceIdentifier.FromDescriptor(_descriptors[i]) : cacheKey;
if (TryCreateExact(_descriptors[i], registrationKey, callSiteChain, slot) is { } callSite)
{
AddCallSite(callSite, i);
}
}
}
for (int i = _descriptors.Length - 1; i >= 0; i--)
{
if (KeysMatch(_descriptors[i].ServiceKey, cacheKey.ServiceKey))
if (KeysMatch(cacheKey.ServiceKey, _descriptors[i].ServiceKey))
{
if (TryCreateOpenGeneric(_descriptors[i], cacheKey, callSiteChain, slot, throwOnConstraintViolation: false) is { } callSite)
// Special case for AnyKey: we don't want to add in cache a mapping AnyKey -> specific type,
// so we need to ask creation with the original identity of the descriptor
var registrationKey = isAnyKeyLookup ? ServiceIdentifier.FromDescriptor(_descriptors[i]) : cacheKey;
if (TryCreateOpenGeneric(_descriptors[i], registrationKey, callSiteChain, slot, throwOnConstraintViolation: false) is { } callSite)
{
AddCallSite(callSite, i);
}
Expand Down