Using the correct (scoped) IServiceProvider for implementation factories

Author: lord-executor

src/Ninject.Web.AspNetCore.Test/Unit/ServiceProviderScopeTest.cs

@@ -0,0 +1,67 @@
+using System;
+using FluentAssertions;
+using Microsoft.Extensions.DependencyInjection;
+using Microsoft.Extensions.DependencyInjection.Extensions;
+using Ninject.Web.AspNetCore.Test.Fakes;
+using Xunit;
+
+namespace Ninject.Web.AspNetCore.Test.Unit;
+
+public class ServiceProviderScopeTest : TestKernelContext
+{
+	[Theory]
+	[MemberData(nameof(ServiceConfigurations))]
+	public void ScopedServices_ResolvedInScope_ShouldBeDifferent(ServiceCollection serviceCollection)
+	{
+		var kernel = CreateKernel(serviceCollection);
+		var provider = kernel.Get<IServiceProvider>();
+
+		var rootKnight = provider.GetRequiredService<Knight>();
+
+		using (var scope = provider.CreateScope())
+		{
+			// The IWeapon instantiated inside of this scope should also be tied to this scope when created through
+			// a ServiceDescriptor.ImplementationFactory
+			var levelOneKnight = scope.ServiceProvider.GetRequiredService<Knight>();
+			AssertNotSameKnightAndWeapon(levelOneKnight, rootKnight);
+			
+			var levelOneKnight2 = scope.ServiceProvider.GetRequiredService<Knight>();
+			AssertSameKnightAndWeapon(levelOneKnight2, levelOneKnight);
+		}
+
+		var rootKnight2 = provider.GetRequiredService<Knight>();
+		AssertSameKnightAndWeapon(rootKnight2, rootKnight);
+	}
+
+	private void AssertSameKnightAndWeapon(Knight value, Knight expected)
+	{
+		value.Should().BeSameAs(expected);
+		value.Weapon.Should().BeSameAs(expected.Weapon);
+	}
+	
+	private void AssertNotSameKnightAndWeapon(Knight value, Knight expected)
+	{
+		value.Should().NotBeSameAs(expected);
+		value.Weapon.Should().NotBeSameAs(expected.Weapon);
+	}
+	
+	public static TheoryData<ServiceCollection> ServiceConfigurations => new TheoryData<ServiceCollection>
+	{
+		{
+			new ServiceCollection() {
+				new ServiceDescriptor(typeof(Knight), typeof(Knight), ServiceLifetime.Scoped),
+				new ServiceDescriptor(typeof(IWeapon), typeof(Longsword), ServiceLifetime.Scoped)
+			}
+		},
+		{
+			new ServiceCollection() {
+				new ServiceDescriptor(typeof(Knight), typeof(Knight), ServiceLifetime.Scoped),
+				new ServiceDescriptor(typeof(Longsword), typeof(Longsword), ServiceLifetime.Scoped),
+				// See https://github.com/lord-executor/Ninject.Web.AspNetCore/issues/12
+				// The difference here is the use of a descriptor with an implementation factory where the implementation
+				// must make sure that the correct _scoped_ IServiceProvider instance is passed to the factory
+				new ServiceDescriptor(typeof(IWeapon),serviceProvider => serviceProvider.GetService<Longsword>(), ServiceLifetime.Scoped)
+			}
+		},
+	};
+}

src/Ninject.Web.AspNetCore/AspNetCoreApplicationPlugin.cs

@@ -19,7 +19,7 @@ public object GetRequestScope(IContext context)
 			// when being instantiated through the ServiceProviderScopeResolutionRoot, the parameter for explicit nested scopes
 			// created through IServiceScopeFactory.CreateScope has precedence in order to preserve the behavior that is expected
 			// from IServiceProvider with scoped services.
-			var scope = context.Parameters.OfType<ServiceProviderScopeParameter>().SingleOrDefault()?.GetValue(context, null);
+			var scope = context.GetServiceProviderScopeParameter()?.GetValue(context, null);
 			// returns the currently active request scope. Used when binding with scope InRequestScope.
 			return scope ?? RequestScope.Current ?? throw new ActivationException("Trying to activate a service InRequestScope without a request scope present");
 		}

src/Ninject.Web.AspNetCore/ContextExtensions.cs

@@ -0,0 +1,21 @@
+using System.Linq;
+using Ninject.Activation;
+
+namespace Ninject.Web.AspNetCore
+{
+	/// <summary>
+	/// Extension methods on <see cref="IContext"/> to simplify dealing with the <see cref="ServiceProviderScopeParameter" />
+	/// </summary>
+	public static class ContextExtensions
+	{
+		/// <summary>
+		/// Gets the <see cref="ServiceProviderScopeParameter"/> from the context parameters.
+		/// </summary>
+		/// <param name="context">The resolution context from which to retrieve the scope parameter</param>
+		/// <returns>The scope from which the request originated or <c>null</c> if there is none</returns>
+		public static ServiceProviderScopeParameter GetServiceProviderScopeParameter(this IContext context)
+		{
+			return context.Parameters.OfType<ServiceProviderScopeParameter>().SingleOrDefault();
+		}
+	}
+}

src/Ninject.Web.AspNetCore/NinjectServiceProviderBuilder.cs

@@ -25,7 +25,7 @@ public IServiceProvider Build()
 				// It essentially always injects the instance that is being used for the instantiation
 				// which kinda makes sense, but is not documented ANYWHERE, not tested ANYWHERE and was really quite difficult to
 				// figure out.
-				var scopeProvider = context.Parameters.OfType<ServiceProviderScopeParameter>().SingleOrDefault()?.SourceServiceProvider;
+				var scopeProvider = context.GetServiceProviderScopeParameter()?.SourceServiceProvider;
 				if (scopeProvider != null)
 				{
 					var descriptor = context.Request.ParentContext?.Binding.Metadata.Get<ServiceDescriptor>(nameof(ServiceDescriptor));

src/Ninject.Web.AspNetCore/ServiceCollectionAdapter.cs

@@ -66,8 +66,11 @@ private IBindingWithOrOnSyntax<T> ConfigureImplementationAndLifecycle<T>(
 				result = ConfigureLifecycle(bindingToSyntax.ToMethod(context
 					=>
 				{
-					var provider = context.Kernel.Get<IServiceProvider>();
-					return descriptor.ImplementationFactory(provider) as T;
+					// When resolved through the ServiceProviderScopeResolutionRoot which adds this parameter, the
+					// correct _scoped_ IServiceProvider is used. Fall back to root IServiceProvider when not created
+					// through a NinjectServiceProvider (some tests do this to prove a point)
+					var scopeProvider = context.GetServiceProviderScopeParameter()?.SourceServiceProvider ?? context.Kernel.Get<IServiceProvider>();
+					return descriptor.ImplementationFactory(scopeProvider) as T;
 				}), descriptor.Lifetime);
 			}
 			else
@@ -99,7 +102,7 @@ private IBindingNamedWithOrOnSyntax<T> ConfigureLifecycle<T>(
 					// Microsoft.Extensions.DependencyInjection expects transient services to be disposed when the IServiceScope
 					// in which they were created is disposed. See the compliance tests for more details.
 					return bindingInSyntax.InScope(context => {
-						var scope = context.Parameters.OfType<ServiceProviderScopeParameter>().SingleOrDefault();
+						var scope = context.GetServiceProviderScopeParameter();
 						return scope?.DeriveTransientScope();
 					});
 				default: