dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
14.98k stars 4.66k forks source link

DI resolve parameter using same ServiceKey #99084

Open dferretti opened 6 months ago

dferretti commented 6 months ago

I have a few classes that are set up using keyed services in DI. Currently I have to do one of the following:

// classes are nice and clean
class A() { }
class B(A a) { public A A { get; } = a; }

// but registration is less convenient
services.AddKeyedSingleton<A>("first");
services.AddKeyedSingleton<B>("first", (sp, key) => new B(sp.GetRequiredKeyedService<A>(key)));

or

// injecting IServiceProvider doesn't feel graet
class A() { }
class B([ServiceKey] string name, IServiceProvider sp) { public A A { get; } = sp.GetRequiredKeyedService<A>(name); }

// but registration is cleaner
services.AddKeyedSingleton<A>("first");
services.AddKeyedSingleton<B>("first");

I would like to be able to do something like this instead:

// classes are still fairly clean
class A() { }
class B([FromKeyedServices(something)] A a) { public A A { get; } = a; }

// registration is still clean
services.AddKeyedSingleton<A>("first");
services.AddKeyedSingleton<B>("first");

Not sure exactly what that would look like, a different attribute or something maybe. Or maybe some sentinel value to use in FromKeyedServices. But the idea is being able to say "from keyed services, but using the value that ServiceKeyAttribute would return"

ghost commented 6 months ago

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

Issue Details
I have a few classes that are set up using keyed services in DI. Currently I have to do one of the following: ```c# // classes are nice and clean class A() { } class B(A a) { public A A { get; } = a; } // but registration is less convenient services.AddKeyedSingleton("first"); services.AddKeyedSingleton("first", (sp, key) => new B(sp.GetRequiredKeyedService(key))); ``` or ```c# // injecting IServiceProvider doesn't feel graet class A() { } class B([ServiceKey] string name, IServiceProvider sp) { public A A { get; } = sp.GetRequiredKeyedService(name); } // but registration is cleaner services.AddKeyedSingleton("first"); services.AddKeyedSingleton("first"); ``` I would like to be able to do something like this instead: ```c# // classes are still fairly clean class A() { } class B([FromKeyedServices(something)] A a) { public A A { get; } = a; } // registration is still clean services.AddKeyedSingleton("first"); services.AddKeyedSingleton("first"); ``` Not sure exactly what that would look like, a different attribute or something maybe. Or maybe some sentinel value to use in FromKeyedServices. But the idea is being able to say "from keyed services, but using the value that ServiceKeyAttribute would return"
Author: dferretti
Assignees: -
Labels: `untriaged`, `area-Extensions-DependencyInjection`
Milestone: -
Oskarsson commented 6 months ago

I would see this as very helpful and I've seen that Aspire has the same "problem". They went with the first workaround

steveharter commented 6 months ago

cc @benjaminpetit

drolevar commented 4 months ago

Indeed a very useful feature.

I'd suggest adding a new attribute, something like InheritServiceKeyAttribute and modifying CallSiteFactory::CreateArgumentCallSites so that it follows the same logic as for FromKeyedServicesAttribute, only with serviceIdentifier.ServiceKey instead of keyed.Key.

drolevar commented 4 months ago

@dferretti I've provided a PR #102160 to address this issue. But apparently it has to go through API review. Could you please update the issue with a proposed API change as per referenced PR and mark it as api-suggestion? Or alternatively I can make a new issue and reference yours.

pinkfloydx33 commented 4 months ago

Just ran into needing this as well. Was able to provide a factory to workaround the need, but it gets messy with multiple levels or multiple dependencies. Would be nice to have it built in

steveharter commented 4 months ago

PTAL @benjaminpetit

steveharter commented 1 month ago

This issue needs to be updated to have a valid API proposal, meaning a list any new members.

julealgon commented 1 month ago

This looks like yet again another hack proposal for the lack of "dependent services" support in the container.

I'd rather see that implemented, than continue with proposals such as this that keep creating special cases and making the whole DI API surface more convoluted and coupled.

wvpm commented 1 month ago

@dferretti I don't understand your proposal.

    [Fact]
    public void Do() {
        ServiceCollection services = new();
        // registration is still clean
        services.AddKeyedSingleton<A>("first");
        services.AddKeyedSingleton<B>("first");
        using ServiceProvider serviceProvider = services.BuildServiceProvider(new ServiceProviderOptions() {
            ValidateOnBuild = true,
            ValidateScopes = true
        });
        serviceProvider.GetRequiredKeyedService<B>("first");
    }

    // classes are still fairly clean
    private class A() { }

    private class B([FromKeyedServices("first")] A a) { public A A { get; } = a; }

This already works fine (when running XUnit testing).

If you don't want [FromKeyedServices("first")] in your constructor, see #105913 which proposes to change the resolving behaviour. So if you call GetKeyedService, it will pass that key along when resolving dependencies (FromKeyedServices should have priority). If there is no keyed service (and no FromKeyedServices!), it could fall back to unkeyed service.