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

#98551 - Regression in DI scope validation #98661

Merged
merged 4 commits into from
Feb 27, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -41,18 +41,27 @@ public void ValidateResolution(ServiceCallSite callSite, IServiceScope scope, IS
// First, check if we have encountered this call site before to prevent visiting call site trees that have already been visited
// If firstScopedServiceInCallSiteTree is null there are no scoped dependencies in this service's call site tree
// If firstScopedServiceInCallSiteTree has a value, it contains the first scoped service in this service's call site tree
if (_scopedServices.TryGetValue(callSite.Cache.Key, out Type? firstScopedServiceInCallSiteTree))
if (!_scopedServices.TryGetValue(callSite.Cache.Key, out Type? firstScopedServiceInCallSiteTree))
{
return firstScopedServiceInCallSiteTree;
}
// This call site wasn't cached yet, walk the tree
firstScopedServiceInCallSiteTree = base.VisitCallSite(callSite, argument);

// Walk the tree
Type? scoped = base.VisitCallSite(callSite, argument);
// Cache the result
_scopedServices[callSite.Cache.Key] = firstScopedServiceInCallSiteTree;
}

// Store the result for each visited service
_scopedServices[callSite.Cache.Key] = scoped;
// If there is a scoped service in the call site tree, make sure we are not resolving it from a singleton
if (firstScopedServiceInCallSiteTree != null && argument.Singleton != null)
Copy link
Member

Choose a reason for hiding this comment

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

{
throw new InvalidOperationException(SR.Format(SR.ScopedInSingletonException,
callSite.ServiceType,
argument.Singleton.ServiceType,
nameof(ServiceLifetime.Scoped).ToLowerInvariant(),
nameof(ServiceLifetime.Singleton).ToLowerInvariant()
));
}

return scoped;
return firstScopedServiceInCallSiteTree;
}

protected override Type? VisitConstructor(ConstructorCallSite constructorCallSite, CallSiteValidatorState state)
Expand Down Expand Up @@ -91,15 +100,6 @@ public void ValidateResolution(ServiceCallSite callSite, IServiceScope scope, IS
{
return null;
}
if (state.Singleton != null)
{
throw new InvalidOperationException(SR.Format(SR.ScopedInSingletonException,
scopedCallSite.ServiceType,
state.Singleton.ServiceType,
nameof(ServiceLifetime.Scoped).ToLowerInvariant(),
nameof(ServiceLifetime.Singleton).ToLowerInvariant()
));
}

VisitCallSiteMain(scopedCallSite, state);
return scopedCallSite.ServiceType;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,84 @@ public void GetService_DoesNotThrow_WhenGetServiceForNonScopedImplementationWith
Assert.NotNull(result);
}

[Fact]
public void BuildServiceProvider_ValidateOnBuild_Throws_WhenScopedIsInjectedIntoSingleton()
{
// Arrange
var serviceCollection = new ServiceCollection();
serviceCollection.AddScoped<IBar, Bar>();
serviceCollection.AddSingleton<IFoo, Foo>();

// Act + Assert
var aggregateException = Assert.Throws<AggregateException>(() => serviceCollection.BuildServiceProvider(new ServiceProviderOptions() { ValidateOnBuild = true, ValidateScopes = true }));
Copy link
Member

Choose a reason for hiding this comment

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

I assume without the fix here, that no exception was thrown here.

Copy link
Contributor Author

@christiaanderidder christiaanderidder Feb 22, 2024

Choose a reason for hiding this comment

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

An exception was indeed not thrown here, however it was thrown when registering the dependencies in the reversed order. The aspnet core repo happens to test the this scenario, while the runtime repo only covered the latter. This happened because in the previous code, the scoped-in-singleton check was not performed if a callsite was already cached. See here as well: #98551 (comment)

Assert.StartsWith("Some services are not able to be constructed", aggregateException.Message);
Assert.Equal(1, aggregateException.InnerExceptions.Count);
Assert.Equal("Error while validating the service descriptor 'ServiceType: Microsoft.Extensions.DependencyInjection.Tests.ServiceProviderValidationTests+IFoo Lifetime: Singleton ImplementationType: Microsoft.Extensions.DependencyInjection.Tests.ServiceProviderValidationTests+Foo': " +
"Cannot consume scoped service 'Microsoft.Extensions.DependencyInjection.Tests.ServiceProviderValidationTests+IBar' from singleton 'Microsoft.Extensions.DependencyInjection.Tests.ServiceProviderValidationTests+IFoo'."
, aggregateException.InnerExceptions[0].Message);
}

[Fact]
public void BuildServiceProvider_ValidateOnBuild_Throws_WhenScopedIsInjectedIntoSingleton_ReverseRegistrationOrder()
{
// Arrange
var serviceCollection = new ServiceCollection();
serviceCollection.AddSingleton<IFoo, Foo>();
serviceCollection.AddScoped<IBar, Bar>();

// Act + Assert
var aggregateException = Assert.Throws<AggregateException>(() => serviceCollection.BuildServiceProvider(new ServiceProviderOptions() { ValidateOnBuild = true, ValidateScopes = true }));
Assert.StartsWith("Some services are not able to be constructed", aggregateException.Message);
Assert.Equal(1, aggregateException.InnerExceptions.Count);
Assert.Equal("Error while validating the service descriptor 'ServiceType: Microsoft.Extensions.DependencyInjection.Tests.ServiceProviderValidationTests+IFoo Lifetime: Singleton ImplementationType: Microsoft.Extensions.DependencyInjection.Tests.ServiceProviderValidationTests+Foo': " +
"Cannot consume scoped service 'Microsoft.Extensions.DependencyInjection.Tests.ServiceProviderValidationTests+IBar' from singleton 'Microsoft.Extensions.DependencyInjection.Tests.ServiceProviderValidationTests+IFoo'."
, aggregateException.InnerExceptions[0].Message);
}

[Fact]
public void BuildServiceProvider_ValidateOnBuild_DoesNotThrow_WhenScopeFactoryIsInjectedIntoSingleton()
{
// Arrange
var serviceCollection = new ServiceCollection();
serviceCollection.AddSingleton<IBoo, Boo>();

// Act + Assert
serviceCollection.BuildServiceProvider(new ServiceProviderOptions() { ValidateOnBuild = true, ValidateScopes = true });
}

[Fact]
public void BuildServiceProvider_ValidateOnBuild_Throws_WhenScopedIsInjectedIntoSingleton_CachedCallSites()
{
// Arrange
var serviceCollection = new ServiceCollection();
serviceCollection.AddScoped<Foo>();
serviceCollection.AddSingleton<Foo2>();
serviceCollection.AddScoped<IBar, Bar2>();
serviceCollection.AddScoped<IBaz, Baz>();

// Act + Assert
var aggregateException = Assert.Throws<AggregateException>(() => serviceCollection.BuildServiceProvider(new ServiceProviderOptions() { ValidateOnBuild = true, ValidateScopes = true }));
Assert.StartsWith("Some services are not able to be constructed", aggregateException.Message);
Assert.Equal(1, aggregateException.InnerExceptions.Count);
Assert.Equal("Error while validating the service descriptor 'ServiceType: Microsoft.Extensions.DependencyInjection.Tests.ServiceProviderValidationTests+Foo2 Lifetime: Singleton ImplementationType: Microsoft.Extensions.DependencyInjection.Tests.ServiceProviderValidationTests+Foo2': " +
"Cannot consume scoped service 'Microsoft.Extensions.DependencyInjection.Tests.ServiceProviderValidationTests+IBar' from singleton 'Microsoft.Extensions.DependencyInjection.Tests.ServiceProviderValidationTests+Foo2'."
, aggregateException.InnerExceptions[0].Message);
}

[Fact]
public void BuildServiceProvider_ValidateOnBuild_DoesNotThrow_CachedCallSites()
{
// Arrange
var serviceCollection = new ServiceCollection();
serviceCollection.AddScoped<Foo>();
serviceCollection.AddScoped<Foo2>();
serviceCollection.AddScoped<IBar, Bar2>();
serviceCollection.AddScoped<IBaz, Baz>();

// Act + Assert
serviceCollection.BuildServiceProvider(new ServiceProviderOptions() { ValidateOnBuild = true, ValidateScopes = true });
}

[Fact]
public void BuildServiceProvider_ValidateOnBuild_ThrowsForUnresolvableServices()
{
Expand Down Expand Up @@ -327,6 +405,13 @@ public Foo(IBar bar)
}
}

private class Foo2 : IFoo
{
public Foo2(IBar bar)
{
}
}

private interface IBar
{
}
Expand Down
Loading