Return null instead of throwing in ServiceDescriptor.ImplementationInstance\Type\Factory if a keyed service#105776
Conversation
…stance\Type\Factory if a keyed service
ghost
added
the
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
|
||
| private static void ThrowKeyedDescriptor() => throw new InvalidOperationException(SR.KeyedDescriptorMisuse); | ||
|
|
||
| private static void ThrowNonKeyedDescriptor() => throw new InvalidOperationException(SR.NonKeyedDescriptorMisuse); |
There was a problem hiding this comment.
If we don't throw when accessing on non-leyed property when the descriptor is kedyed, shoud we also remove this, to be consistent?
There was a problem hiding this comment.
I was wondering about that as well. The original issue was a backwards compat issue with existing APIs so not throwing there makes sense. Throwing for the new APIs I think is OK and desired to help catch issues (as originally intended).
Other than consistency, can you think of a scenario where we would want to return null for the new APIs?
There was a problem hiding this comment.
Nope it's only for consistency's sake. It's fine to me to throw here otherwise
src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ServiceDescriptor.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ServiceDescriptor.cs
Show resolved
Hide resolved
buyaa-n
left a comment
buyaa-n
left a comment
There was a problem hiding this comment.
Nits on docs, otherwise LGTM
|
/backport to release/8.0-staging |
|
Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/10512853943 |
|
@steveharter backporting to release/8.0-staging failed, the patch most likely resulted in conflicts: $ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Return null instead of throwing in ServiceDescriptor.ImplementationInstance\Type\Factory if a keyed service
Using index info to reconstruct a base tree...
M src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ServiceDescriptor.cs
M src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceCollectionKeyedServiceExtensionsTest.cs
Falling back to patching base and 3-way merge...
Auto-merging src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceCollectionKeyedServiceExtensionsTest.cs
Auto-merging src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ServiceDescriptor.cs
CONFLICT (content): Merge conflict in src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ServiceDescriptor.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0001 Return null instead of throwing in ServiceDescriptor.ImplementationInstance\Type\Factory if a keyed service
Error: The process '/usr/bin/git' failed with exit code 128Please backport manually! |
|
@steveharter an error occurred while backporting to release/8.0-staging, please check the run log for details! Error: git am failed, most likely due to a merge conflict. |
3 participants
Fixes #95789
This fixes a regression for cases where
ServiceDescriptor.ImplementationInstance\Type\Factoryis called for code that is not servicekey-aware. Instead of throwing,nullis returned.This will be backported to 8.0.