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

Different behavior of FromKeyedServices attribute in Minimal APIs handler vs class library #102204

Closed
1 task done
marcominerva opened this issue May 6, 2024 · 9 comments · Fixed by #105839
Closed
1 task done
Assignees
Labels
area-Extensions-DependencyInjection blocking-release in-pr There is an active PR which will close this issue when it is merged
Milestone

Comments

@marcominerva
Copy link

marcominerva commented May 6, 2024

Is there an existing issue for this?

  • I have searched the existing issues

Describe the bug

If we use the FromKeyedServices attribute in a class library and the keyed registration is missing, it falls back to the default dependency injection behavior and gets the last registered implementation (even with no key). However, If we use FromKeyedServices directly in a Minimal API endpoint handler and the key does not exist, it throws an exception.

Expected Behavior

The FromKeyedServices attribute should behave in the same way in every context.

Steps To Reproduce

Minimal repro here: https://github.com/marcominerva/KeyedServicesIssue

Exceptions (if any)

When using FromKeyedServices directly in a Minimal API endpoint handler:

System.InvalidOperationException: No service for type 'KeyedServicesIssue.Library.IDependency' has been registered.
at Microsoft.Extensions.DependencyInjection.ServiceProvider.GetRequiredKeyedService(Type serviceType, Object serviceKey, ServiceProviderEngineScope serviceProviderEngineScope)
at lambda_method2(Closure, Object, HttpContext)

When using FromKeyedServices in a class library, no exception at all is thrown, even if the key does not exist.

.NET Version

8.0.204

@captainsafia
Copy link
Member

@marcominerva Thanks for reporting this!

The behavior that you're seeing is a consistent and intentional choice with all the implementations for keyed services in ASP.NET Core (SignalR, MVC, minimal APIs).

The expected pattern for resolving a service instance with any key using keyed DI is to take advantage of the KeyedService.AnyKey definition which is designed to match the behavior that you see here.

cc: @benjaminpetit For insights onto the behavior in the runtime's DI resolution strategy

@marcominerva
Copy link
Author

So, to be sure to have understood the context, in the following case:

builder.Services.AddSingleton<SampleConfig>();

app.MapGet("api/sample-config", ([FromKeyedServices("x")] SampleConfig config) =>
{
    return TypedResults.Ok(config.Value);
});

This code throws an exception, because there isn't a SampleConifg instance that has been registered with the key x.

On the other hand, in the following scenario:

builder.Services.AddSingleton<SampleConfig>();
builder.Services.AddScoped<OtherSampleService>();

app.MapGet("api/external-sample-config", (OtherSampleService otherSampleService) =>
{
    return TypedResults.Ok(otherSampleService.ConfigValue);
})

public class OtherSampleService
{
    private readonly SampleConfig config;

    public OtherSampleService([FromKeyedServices("x")] SampleConfig config, IServiceProvider serviceProvider)
    {
        this.config = config;
    }

    public string ConfigValue => config.Value.ToString();
}

The code doesn't raise any exception and, even if OtherSampleService requires a SampleConfig with key x, because it doesn't exist, it gets the "default" registration.

@captainsafia
Copy link
Member

So, to be sure to have understood the context, in the following case:

Yep, your understanding here is correct. I agree with you that the difference in behavior is a little strange so I've pinged @benjaminpetit above (who worked on the keyed services implementation) to get a sense of why the implementation behaves this way in constructor binding.

@adityamandaleeka adityamandaleeka transferred this issue from dotnet/aspnetcore May 14, 2024
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label May 14, 2024
Copy link
Contributor

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

@steveharter steveharter added needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration and removed untriaged New issue has not been triaged by the area owner labels May 29, 2024
@steveharter steveharter added this to the 9.0.0 milestone May 29, 2024
@steveharter
Copy link
Member

This should be fixed for v9.

@captainsafia
Copy link
Member

@steveharter Thanks! Do you happen to know what preview the fix shipped in for verification?

@dotnet-policy-service dotnet-policy-service bot added the in-pr There is an active PR which will close this issue when it is merged label Aug 1, 2024
@steveharter
Copy link
Member

I think the correct behavior is to throw for the case where a keyed parameter is unresolved - i.e. don't inject an unkeyed service. I put up a PR for that.

@steveharter
Copy link
Member

@steveharter Thanks! Do you happen to know what preview the fix shipped in for verification?

@captainsafia the fix has not been shipped yet. v9.0 will be released later this year. Whether we want to backport this to 8.0 is an open topic.

@steveharter
Copy link
Member

steveharter commented Aug 19, 2024

This will ship in v9 RC1. Once validated, we will backport to v8.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Extensions-DependencyInjection blocking-release in-pr There is an active PR which will close this issue when it is merged
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants