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

Fix issues in GetKeyedService() and GetKeyedServices() with AnyKey #113137

Merged
merged 10 commits into from
Mar 14, 2025

Conversation

steveharter
Copy link
Member

@steveharter steveharter commented Mar 4, 2025

Reviving the closed v9.0 PR #97561:

  • (in previous PR) Fix consistency issue related to a descriptor cache bug.
  • (in previous PR) Do not return AnyKey registration when enumerating services with AnyKey as the lookup key.
  • (new) Throw when attempting to resolve a single service with AnyKey.

After this PR is in, I'll update push another PR to update the docs around IEnumerable semantics.

Semantics here mostly related to fallout and discussions from #95582. Here's a summary of semantics copied from a test added in this PR:

/*
 * Table for what results are included:
 *
 * Query                     | Keyed? | Unkeyed? | AnyKey? | null key?
 * -------------------------------------------------------------------
 * GetServices(Type)         | no     | yes      | no      | yes
 * GetService(Type)          | no     | yes      | no      | yes
 *
 * GetKeyedServices(null)    | no     | yes      | no      | yes
 * GetKeyedService(null)     | no     | yes      | no      | yes
 *
 * GetKeyedServices(AnyKey)  | yes    | no       | no      | no
 * GetKeyedService(AnyKey)   | throw  | throw    | throw   | throw
 *
 * GetKeyedServices(key)     | yes    | no       | no      | no
 * GetKeyedService(key)      | yes    | no       | yes     | no
 *
 * Summary:
 * - A null key is the same as unkeyed. This allows the KeyServices APIs to support both keyed and unkeyed.
 * - AnyKey is a special case of Keyed.
 * - AnyKey registrations are not returned with GetKeyedServices(AnyKey) and GetKeyedService(AnyKey) always throws.
 * - For IEnumerable, the ordering of the results are in registration order.
 * - For a singleton resolve, the last match wins.
 */

@steveharter steveharter added breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. area-Extensions-DependencyInjection labels Mar 4, 2025
@steveharter steveharter added this to the 10.0.0 milestone Mar 4, 2025
@steveharter steveharter self-assigned this Mar 4, 2025
@Copilot Copilot bot review requested due to automatic review settings March 4, 2025 21:02
@dotnet-policy-service dotnet-policy-service bot added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Mar 4, 2025
Copy link
Contributor

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

Choose a reason for hiding this comment

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

PR Overview

This PR addresses inconsistencies in the resolution and caching behaviors for KeyedService.AnyKey. It updates the logic in service call site creation, modifies the service provider methods to validate AnyKey usage, and revises tests to ensure the correct behavior and exception throwing.

Reviewed Changes

File Description
ServiceLookup/CallSiteFactory.cs Adjusted the keys matching logic and caching behavior to exclude AnyKey registrations while preserving proper lookups.
Specification.Tests/KeyedDependencyInjectionSpecificationTests.cs Updated tests to reflect the revised expectations for service counts and exception throwing regarding AnyKey usage.
ServiceProvider.cs Added explicit checks to throw exceptions when AnyKey is used to resolve a non-collection service.
ServiceLookup/ThrowHelper.cs Introduced a helper method to throw an InvalidOperationException for invalid AnyKey resolution attempts.

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Copy link
Member

@halter73 halter73 left a comment

Choose a reason for hiding this comment

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

LGTM. I think having GetKeyedServices<T>(KeyedService.AnyKey) return all registrations for each specific key rather than the last one is fine. I was just curious if @stephentoub had any opinions since he first asked for AnyKey to even work with GetKeyedServices.

It would still be nice if we could assert the right order is maintained for the services returned by GetKeyedServices in the QueryWithIEnumerable test. I don't think the order is wrong, but it'd be nice to veriffy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Extensions-DependencyInjection breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants