Validate scope into singleton sevice injection #430
Conversation
| @@ -9,7 +9,12 @@ public static class ServiceCollectionContainerBuilderExtensions | |||
| { | |||
| public static IServiceProvider BuildServiceProvider(this IServiceCollection services) | |||
| { | |||
| return new ServiceProvider(services); | |||
| return BuildServiceProvider(services, false); | |||
There was a problem hiding this comment.
nit: style validateScopes: false
|
Make that style change where the optional parameter syntax is used everywhere |
|
I think there's value in being able to catch this at startup time before the first resolve. |
|
Indeed there is but service provider is lazy for a reason and some of them could not be caught, like open generics use cases |
|
|
||
| namespace Microsoft.Extensions.DependencyInjection.ServiceLookup | ||
| { | ||
| internal struct CallSiteValidatorState |
There was a problem hiding this comment.
Make this nested inside CallSiteValidator? It's not used outside of it.
|
⌚ for the few questions. |
| return BuildServiceProvider(services, validateScopes: false); | ||
| } | ||
|
|
||
| public static IServiceProvider BuildServiceProvider(this IServiceCollection services, bool validateScopes) |
There was a problem hiding this comment.
Time for doc comments? On one hand, I kind of hope no one ever uses this (except for may us). On the other hand, without context, I would have no idea what validateScopes is even about.
There was a problem hiding this comment.
For the other method + type too since they're all public.
|
Is the issue we want to identify really a scoped service being injected into a singleton (perhaps transitively). Or is the issue even more fundamental, should we vialidate that a scoped service is never resolved from the root? We might have to validate on every call to |
|
🆙📅 |
| { | ||
| internal class CallSiteValidator: CallSiteVisitor<CallSiteValidator.CallSiteValidatorState, Type> | ||
| { | ||
| private readonly Dictionary<Type, Type> _scopedServices = new Dictionary<Type, Type>(); |
There was a problem hiding this comment.
Comment to indicate the key is the service being resolved via GetService and the value is a single scoped service that it depends on (but there may be more.
|
|
| /// </summary> | ||
| /// <param name="services">The <see cref="IServiceCollection"/> containing service descriptors.</param> | ||
| /// <param name="validateScopes"> | ||
| /// <c>true</c> to perform check verifying that scoped services never gets resolved from root provider; otherwise <c>false</c>. |
There was a problem hiding this comment.
Might as well point out false is the default behavior.
|
|
|
In relation to @halter73's comment; I've always wondered what happens if/when you resolve a scoped service from the top-level container... Does it effectively become a singleton? |
|
If that's the case, you have (at least) two ways a scoped/transient service can be held captive; either as a dependency of a singleton/scoped service or in the top-level provider. |
|
@khellang yes, this PR is targeting these two scenarios. |
|
🚢 |
|
This is a great addition! Since it is now defined that resolving scoped services from a non-scoped provider should not be possible, shouldn't this also be covered in the Specification.Tests library to make sure other containers don't allow this either? |
Fixes:#88
@davidfowl @halter73