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.

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

Fix consistency issue in GetKeyedServices with AnyKey #97561

Closed
wants to merge 3 commits into from
Closed

Fix consistency issue in GetKeyedServices with AnyKey #97561

Show file tree
Hide file tree
Changes from all commits
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
47 changes: 43 additions & 4 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 @@ -152,10 +153,48 @@ public void ResolveKeyedServicesAnyKeyWithAnyKeyRegistration()
_ = provider.GetKeyedService<IService>("something-else");
_ = provider.GetKeyedService<IService>("something-else-again");

// Return all services registered with a non null key, but not the one "created" with KeyedService.AnyKey
// Return all services registered with a non null key, but not the one "created" with KeyedService.AnyKey,
// nor the KeyedService.AnyKey registration
var allServices = provider.GetKeyedServices<IService>(KeyedService.AnyKey).ToList();
Assert.Equal(5, allServices.Count);
Assert.Equal(new[] { service1, service2, service3, service4 }, allServices.Skip(1));
Assert.Equal(4, allServices.Count);
Assert.Equal(new[] { service1, service2, service3, service4 }, allServices);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Assert.Equal(new[] { service1, service2, service3, service4 }, allServices);
Assert.Equal(new[] { service1, service2, service3, service4 }, allServices);
var someKeyedServices = provider.GetKeyedServices<IService>("service").ToList();
Assert.Equal(new[] { service2, service3, service4 }, someKeyedServices);
var unkeyedServices = provider.GetServices<IService>().ToList();
Assert.Equal(new[] { service5, service6 }, unkeyedServices);

Nit: I'm assuming this all works, and some of this might even be covered partially by other tests. But we don't have many tests with the combination of AnyKey registrations, unkeyed registrations, and enumerable resolution of both keyed and unkeyed services. And I don't see the harm in extra verification.

}

[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));
// We don't return KeyedService.AnyKey registration when listing services
Assert.Equal(new[] { service }, provider1.GetKeyedServices<IService>(KeyedService.AnyKey));

var provider2 = CreateServiceProvider(serviceCollection);
Assert.Equal(new[] { service }, provider2.GetKeyedServices<IService>(KeyedService.AnyKey));
// But we should be able to directly do a lookup on it
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 }, provider1.GetKeyedServices<IService>(KeyedService.AnyKey));

// Check twice in different order to check caching
var provider2 = CreateServiceProvider(serviceCollection);
Assert.Equal(new[] { service }, provider2.GetKeyedServices<IService>(KeyedService.AnyKey));
Assert.Same(any, provider2.GetKeyedService<IService>(KeyedService.AnyKey));
Copy link
Member

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
Member

@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]
Expand Down Expand Up @@ -243,7 +282,7 @@ public void ResolveKeyedServicesSingletonInstanceWithAnyKey()
var provider = CreateServiceProvider(serviceCollection);

var services = provider.GetKeyedServices<IFakeOpenGenericService<PocoClass>>("some-key").ToList();
Assert.Equal(new[] { service1, service2 }, services);
Assert.Equal(new[] { service2 }, services);
}

[Fact]
Expand Down
2 changes: 1 addition & 1 deletion ...ies/Microsoft.Extensions.DependencyInjection/Microsoft.Extensions.DependencyInjection.sln
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,4 +159,4 @@ Global
GlobalSection(ExtensibilityGlobals) = postSolution
SolutionGuid = {68A7BDA7-8093-433C-BF7A-8A6A7560BD02}
EndGlobalSection
EndGlobal
EndGlobal
62 changes: 39 additions & 23 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 Expand Up @@ -360,6 +368,32 @@ void AddCallSite(ServiceCallSite callSite, int index)
{
callSiteChain.Remove(serviceIdentifier);
}

static bool KeysMatch(object? lookupKey, object? descriptorKey)
{
if (lookupKey == null && descriptorKey == null)
{
// Both are non keyed services
return true;
}

if (lookupKey != null && descriptorKey != null)
{
// Both are keyed services

// We don't want to return AnyKey registration, so ignore it
if (descriptorKey.Equals(KeyedService.AnyKey))
return false;

// Check if both keys are equal, or if the lookup key
// should matches all keys (except AnyKey)
return lookupKey.Equals(descriptorKey)
|| lookupKey.Equals(KeyedService.AnyKey);
}

// One is a keyed service, one is not
return false;
}
}

private static CallSiteResultCacheLocation GetCommonCacheLocation(CallSiteResultCacheLocation locationA, CallSiteResultCacheLocation locationB)
Expand Down Expand Up @@ -688,24 +722,6 @@ internal bool IsService(ServiceIdentifier serviceIdentifier)
serviceType == typeof(IServiceProviderIsKeyedService);
}

/// <summary>
/// Returns true if both keys are null or equals, or if key1 is KeyedService.AnyKey and key2 is not null
/// </summary>
private static bool KeysMatch(object? key1, object? key2)
{
if (key1 == null && key2 == null)
return true;

if (key1 != null && key2 != null)
{
return key1.Equals(key2)
|| key1.Equals(KeyedService.AnyKey)
|| key2.Equals(KeyedService.AnyKey);
}

return false;
}

private struct ServiceDescriptorCacheItem
{
[DisallowNull]
Expand Down
Loading