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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

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.

Semantics here mostly related to fallout and discussions from #95582. Here's a summary of IEnumerable-based queries that was copied from a test that was added in this PR:

/*
 * Table for what results are included in GetServices()\GetKeyedServices():
 *
 * Query                     | Keyed? | Unkeyed? | AnyKey? | nullkey?
 * -------------------------------------------------------------------
 * GetServices(Type)         | no     | yes      | no      | yes
 * GetKeyedServices(null)    | no     | yes      | no      | yes
 * GetKeyedServices(AnyKey)  | yes    | no       | no      | no
 * GetKeyedServices(key)     | yes    | no       | no      | 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.
 * - It is not possible to query for AnyKey registrations using IEnumerable; a singleton query resolves.
 */

@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.

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.

2 participants