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

Make GetKeyedServices(KeyedService.AnyKey) return all services #95308

Closed
wants to merge 1 commit into from

Conversation

stephentoub
Copy link
Member

I expected GetKeyedServices(KeyedService.AnyKey) to return all services, since AnyKey is documented as "Gets a key that matches any key." This changes the behavior of KeysMatch accordingly.

Feel free to say "this isn't the right answer" if there's a better answer.

Changes the behavior of CallSiteFactory.KeysMatch to allow for either key1 or key2 to be AnyKey and for that to mean match anything, regardless of whether the other key is non-null.
@ghost
Copy link

ghost commented Nov 28, 2023

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

Issue Details

I expected GetKeyedServices(KeyedService.AnyKey) to return all services, since AnyKey is documented as "Gets a key that matches any key." This changes the behavior of KeysMatch accordingly.

Feel free to say "this isn't the right answer" if there's a better answer.

Author: stephentoub
Assignees: -
Labels:

area-Extensions-DependencyInjection

Milestone: -

@skyoxZ
Copy link
Contributor

skyoxZ commented Nov 28, 2023

Is there any docs/comments that let users learn that a magic value KeyedService.AnyKey can be used on relative Add/Get methods?

@benjaminpetit
Copy link
Member

We need first to decide what to do when there is a KeyedService.AnyKey service registration. Currently with your PR, it does this:

var service1 = new Service();
var service2 = new Service();
var service3 = new Service();
var service4 = new Service();
var service5 = new Service();
var service6 = new Service();
var anyService = new Service();
var serviceCollection = new ServiceCollection();
serviceCollection.AddKeyedSingleton<IService>(KeyedService.AnyKey, anyService);
serviceCollection.AddKeyedSingleton<IService>("first-service", service1);
serviceCollection.AddKeyedSingleton<IService>("service", service2);
serviceCollection.AddKeyedSingleton<IService>("service", service3);
serviceCollection.AddKeyedSingleton<IService>("service", service4);
serviceCollection.AddKeyedSingleton<IService>(null, service5);
serviceCollection.AddSingleton<IService>(service6);

var provider = CreateServiceProvider(serviceCollection);

var allServices = provider.GetKeyedServices<IService>(KeyedService.AnyKey).ToList(); // returns only anyService

I don't think that's what we want?

@stephentoub
Copy link
Member Author

We need first to decide what to do when there is a KeyedService.AnyKey service registration

I still don't fully understand when I would want one of those and so don't have a good answer for what it should mean when there is one.

@benjaminpetit
Copy link
Member

benjaminpetit commented Nov 29, 2023

Pinging @davidfowl

For example, you could do something like this:

serviceCollection.AddKeyedTransient<ILogger>(KeyedService.AnyKey, (sp, key) =>
{
    var factory = sp.GetRequiredService<ILoggerFactory>();
    return factory.CreateLogger(key.ToString());
});

And then in your class:

public class Service()
{
    public Service([FromKeyedServices("SomeCategory") ILogger logger]) { }
}

EDIT: plus, I don't think GetKeyedServices<IService>(KeyedService.AnyKey) should return non-keyed services too.

Also, right now if you have:

serviceCollection.AddKeyedSingleton<IService>("a", serviceA);
serviceCollection.AddKeyedSingleton<IService>(KeyedService.AnyKey, anyService);

then calling GetKeyedService<IService>("a") will return anyService, I also don't think it's correct.

@davidfowl
Copy link
Member

then calling GetKeyedService("a") will return anyService, I also don't think it's correct.

That is fine if AnyKey means any key including the anykey itself no?

@stephentoub
Copy link
Member Author

stephentoub commented Nov 30, 2023

For example

Thanks, I understand it better now.

plus, I don't think GetKeyedServices(KeyedService.AnyKey) should return non-keyed services too.

Why? It returns them today if you pass null as the key, e.g. this prints 3:

using Microsoft.Extensions.DependencyInjection;

var c = new ServiceCollection();
c.AddSingleton<IService>(new Service("1"));
c.AddSingleton<IService>(new Service("2"));
c.AddKeyedSingleton<IService>(null, new Service("3"));
var p = c.BuildServiceProvider();

Console.WriteLine(p.GetKeyedServices<IService>(null).Count());

interface IService { }
record Service(string Name) : IService { }

null is valid in the domain of keys, so I don't know why it would be considered special here for the purposes of AnyKey.

Taking a step back, @benjaminpetit, we need a backportable solution to enumerating all of the instances. I don't particularly care whether it's GetServices<T>() or GetKeyedServices<T>(AnyKey), and I don't have strong views on what the behavior should be when AnyKey is used during registration. Can you help drive to a solution? If this PR isn't the answer, happy to close it and you can submit a replacement, or if there's a small change you'd like me to make, happy to do that, too. We just need a solution soon.

@benjaminpetit
Copy link
Member

That is fine if AnyKey means any key including the anykey itself no?

Maybe? After some thoughts, ss long as we document it, I think both solutions are fine.

Why? It returns them today if you pass null as the key, e.g. this prints 3:

Because collection.AddKeyedSingleton<IService, Service>(null) and collection.AddSingleton<IService, Service>() are considered equivalent, same thing for collection.GetKeyedService<IMyService>(null) and collection.GetService<IMyService>().

A service with a null key is equivalent as a non-keyed service.

IMO the correct behavior would be: GetKeyedServices<T>(KeyedService.AnyKey) shoud return all the explicitely registered services, not the one that were "created" with AnyKey:

serviceCollection.AddKeyedSingleton<IMyService, MyService>(KeyedService.AnyKey);
serviceCollection.AddKeyedSingleton<IMyService, MyOtherService>("another_key");
 
var serviceProvider = serviceCollection.BuildServiceProvider();

// Will return two services, one with key "KeyedService.AnyKey", the other with "another_key"?
var services1 = serviceProvider.GetKeyedServices<IMyService>(KeyedService.AnyKey);
 
// Returns "MyService" -- thanks to the KeyedService.AnyKey registration
_ = serviceProvider.GetKeyedService<IMyService>("something_else");
 
// Returns two services, same as service1
var services2 = serviceProvider.GetKeyedServices<IMyService>(KeyedService.AnyKey);
 
// What should it return? To be consistent with what was returned in services2, nothing?
var services3 = serviceProvider.GetKeyedServices<IMyService>("plop");

What we choose to do for services2 and services3 will really impact this PR.

And maybe we should add another method like this (probaby for 9?):

IEnumerable<(object key, T service)> GetKeyedServices<T>();

I imagine that some people would be interested to get the key too.

@stephentoub
Copy link
Member Author

Because collection.AddKeyedSingleton<IService, Service>(null) and collection.AddSingleton<IService, Service>() are considered equivalent, same thing for collection.GetKeyedService(null) and collection.GetService().

That's my point. GetKeyedServices(null) returns services registered without a key. And null is a valid value for a key. So I expect GetKeyedServices(AnyKey) to include null keys and therefor things registered without keys. It would otherwise be inconsistent.

@stephentoub
Copy link
Member Author

Closing in favor of #95582

@stephentoub stephentoub closed this Dec 4, 2023
@stephentoub stephentoub deleted the getallservices branch December 4, 2023 14:35
@github-actions github-actions bot locked and limited conversation to collaborators Jan 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants