Skip to content

Conversation

@tommysor
Copy link
Contributor

@tommysor tommysor commented Dec 8, 2023

Fix #95754

I assume here that the issue (currently untriaged) is confirmed as a bug.

Based on documentation: https://learn.microsoft.com/en-us/dotnet/core/extensions/dependency-injection#keyed-services

The key isn't limited to string, it can be any object you want, as long as the type correctly implements Equals.

Also based on comparison for resolving in ServiceProvider.GetRequiredKeyedService.
That is implemented in Microsoft.Extensions.DependencyInjection.ServiceLookup.ServiceIdentifier

public bool Equals(ServiceIdentifier other)
{
	if (ServiceKey == null && other.ServiceKey == null)
	{
		return ServiceType == other.ServiceType;
	}
	else if (ServiceKey != null && other.ServiceKey != null)
	{
		return ServiceType == other.ServiceType && ServiceKey.Equals(other.ServiceKey);
	}
	return false;
}

This brings RemoveAllKeyed in line with service resolution (ServiceProvider.GetRequiredKeyedService)
@ghost ghost added area-Extensions-DependencyInjection community-contribution Indicates that the PR has been added by a community member labels Dec 8, 2023
@ghost
Copy link

ghost commented Dec 8, 2023

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

Issue Details

Fix #95754

I assume here that the issue (currently untriaged) is confirmed as a bug.

Based on documentation: https://learn.microsoft.com/en-us/dotnet/core/extensions/dependency-injection#keyed-services

The key isn't limited to string, it can be any object you want, as long as the type correctly implements Equals.

Also based on comparison for resolving in ServiceProvider.GetRequiredKeyedService.
That is implemented in Microsoft.Extensions.DependencyInjection.ServiceLookup.ServiceIdentifier

public bool Equals(ServiceIdentifier other)
{
	if (ServiceKey == null && other.ServiceKey == null)
	{
		return ServiceType == other.ServiceType;
	}
	else if (ServiceKey != null && other.ServiceKey != null)
	{
		return ServiceType == other.ServiceType && ServiceKey.Equals(other.ServiceKey);
	}
	return false;
}
Author: tommysor
Assignees: -
Labels:

area-Extensions-DependencyInjection

Milestone: -

@tommysor tommysor changed the title RemoveAllKeyed use Equals for matching ServiceKey comparisons use Equals for matching Dec 9, 2023
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.

Thank you! This looks like a simple fix to the keyed TryAddEnumerable/Remove/Replace service collection extensions. Thanks for added the regression tests too.

I think we should backport this for .NET 8 patching.

@ericstj
Copy link
Member

ericstj commented Jan 8, 2024

/azp run

@azure-pipelines
Copy link

You have several pipelines (over 10) configured to build pull requests in this repository. Specify which pipelines you would like to run by using /azp run [pipelines] command. You can specify multiple pipelines using a comma separated list.

@ericstj
Copy link
Member

ericstj commented Jan 8, 2024

/azp run runtime

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@steveharter steveharter merged commit f3750be into dotnet:main Jan 11, 2024
@steveharter
Copy link
Contributor

/backport to release/8.0-staging

@github-actions
Copy link
Contributor

Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/7491216853

@tommysor tommysor deleted the RemoveAllKeyed_matching branch January 11, 2024 17:29
tommysor added a commit to tommysor/runtime that referenced this pull request Jan 11, 2024
This change avoids TryAddEnumerable_DoesNotAddDuplicate giving false negative (passing test when it should fail) due to match on ReferenceEquals.

Delete 2 tests that were added in dotnet#95807 due to this weakness in `TryAddEnumerable_DoesNotAddDuplicate`
tmds pushed a commit to tmds/runtime that referenced this pull request Jan 23, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Feb 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-Extensions-DependencyInjection community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RemoveAllKeyed Requires Equality Operator

5 participants