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 validation on build #87354

Merged
merged 6 commits into from
Jun 28, 2023
Merged

Conversation

mapogolions
Copy link
Contributor

@mapogolions mapogolions commented Jun 9, 2023

Fixes #87429

Let's look at the following example

interface IBar {}
class Bar1 : IBar {}
class Bar2 : IBar {}

services.AddScoped<IBar, Bar1>();  // Slot: 1
services.AddTransient<IBar, Bar2>(); // Slot: 0

var sp = services.BuildServiceProvider(validateScopes: true);
Debug.Assert(sp.GetService<IBar>() is Bar2)

The last implementation will be resolved by convention. For more details see the CallSiteFactory class and the DefaultSlot property. The scope validation is fine with a service lifetime (GetService for Transient service on the Root scope).
But the behavior breaks when we set the ValidateOnBuild property to 'true'. Please see unit tests that reproduce the issue.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Jun 9, 2023
@ghost
Copy link

ghost commented Jun 9, 2023

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

Issue Details

Let's look at the following example

interface IBar {}
class Bar1 : IBar {}
class Bar2 : IBar {}

services.AddScoped<IBar, Bar1>();  // Slot: 1
services.AddTransient<IBar, Bar2>(); // Slot: 0

var sp = services.BuildServiceProvider(validateScopes: true);
Debug.Assert(sp.GetService<IBar>() is Bar2)

The last implementation will be resolved by convention. For more details see the CallSiteFactory class and the DefaultSlot property. The scope validation is fine with a service lifetime (GetService for Transient service on the Root scope).
But the behavior changes when we set the ValidateOnBuild property to 'true'. Please see unit tests that reproduce the issue.

Author: mapogolions
Assignees: -
Labels:

area-Extensions-DependencyInjection

Milestone: -

@@ -97,6 +98,12 @@ public void ValidateResolution(Type serviceType, IServiceScope scope, IServiceSc

protected override Type? VisitFactory(FactoryCallSite factoryCallSite, CallSiteValidatorState state) => null;

private static ServiceCacheKey GetCacheKey(ServiceCallSite callSite)
{
return callSite.Cache.Key.Equals(ServiceCacheKey.Empty)
Copy link
Member

Choose a reason for hiding this comment

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

The call to callSite.Cache.Key should only ever occur once for perf.

{
OnResolve(callSite, scope);
DependencyInjectionEventSource.Log.ServiceResolved(this, serviceType);
return realizedService.Invoke(scope);
Copy link
Member

Choose a reason for hiding this comment

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

.Invoke() is unnecessary.

}

[Fact]
public void ScopeValidation_ShouldBeAbleToDistingushGenericCollections_WhenGetServiceIsCalledOnRoot()
Copy link
Member

Choose a reason for hiding this comment

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

It appears this passes without the changes here. Is that intentional?

Copy link
Contributor Author

@mapogolions mapogolions Jun 12, 2023

Choose a reason for hiding this comment

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

Yes, It is intentional. It tests the GetCacheKey() private method that replaces ResultCache.None for services. (For instance, IEnumerable<Foo> and IEnumerable<Bar> have the same ResultCache that is equal to ResultCache.None if at least one of the Foo and at least one of the Bar have transient lifetime)

@steveharter
Copy link
Member

Let's look at the following example

Please add a corresponding issue and link to it from this PR. Thanks

@mapogolions
Copy link
Contributor Author

mapogolions commented Jun 12, 2023

Fix #87429

@steveharter
Copy link
Member

Failure on runtime known issue.

@steveharter steveharter merged commit c06d77a into dotnet:main Jun 28, 2023
@steveharter
Copy link
Member

Thanks @mapogolions

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.

Service Provider cannot resolve transient service from the Root scope if the ValidateOnBuild option is enabled
2 participants