From a0b965df0d92e1fb4f0c3cb5091ea8a32432e942 Mon Sep 17 00:00:00 2001 From: Steve Harter Date: Thu, 1 Aug 2024 15:44:53 -0500 Subject: [PATCH 1/4] Do not allow a non-keyed service to be injected into a keyed parameter --- ...edDependencyInjectionSpecificationTests.cs | 102 +++++++++++++++++- .../src/ServiceLookup/CallSiteFactory.cs | 7 +- .../DI.Tests/ServiceProviderContainerTests.cs | 21 ++++ 3 files changed, 125 insertions(+), 5 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection.Specification.Tests/src/KeyedDependencyInjectionSpecificationTests.cs b/src/libraries/Microsoft.Extensions.DependencyInjection.Specification.Tests/src/KeyedDependencyInjectionSpecificationTests.cs index a0dc73d58f820b..d3e1ee13184164 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection.Specification.Tests/src/KeyedDependencyInjectionSpecificationTests.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection.Specification.Tests/src/KeyedDependencyInjectionSpecificationTests.cs @@ -11,7 +11,7 @@ namespace Microsoft.Extensions.DependencyInjection.Specification { public abstract partial class KeyedDependencyInjectionSpecificationTests { - protected abstract IServiceProvider CreateServiceProvider(IServiceCollection collection); + protected abstract IServiceProvider CreateServiceProvider(IServiceCollection collection); [Fact] public void ResolveKeyedService() @@ -205,6 +205,70 @@ public void ResolveKeyedServiceSingletonInstanceWithKeyedParameter() Assert.Equal("service2", svc.Service2.ToString()); } + [Fact] + public void ResolveKeyedServiceWithKeyedParameter_MissingRegistration_SecondParameter() + { + var serviceCollection = new ServiceCollection(); + + serviceCollection.AddKeyedSingleton("service1"); + // We are missing the registration for "service2" here and OtherService requires it. + + serviceCollection.AddSingleton(); + + var provider = CreateServiceProvider(serviceCollection); + + Assert.Null(provider.GetService()); + Assert.Throws(() => provider.GetService()); + } + + [Fact] + public void ResolveKeyedServiceWithKeyedParameter_MissingRegistration_FirstParameter() + { + var serviceCollection = new ServiceCollection(); + + // We are not registering "service1" and "service1" keyed IService services and OtherService requires them. + + serviceCollection.AddSingleton(); + + var provider = CreateServiceProvider(serviceCollection); + + Assert.Null(provider.GetService()); + Assert.Throws(() => provider.GetService()); + } + + [Fact] + public void ResolveKeyedServiceWithKeyedParameter_MissingRegistrationButWithDefaults() + { + var serviceCollection = new ServiceCollection(); + + // We are not registering "service1" and "service1" keyed IService services and OtherServiceWithDefaultCtorArgs + // specifies them but has argument defaults if missing. + + serviceCollection.AddSingleton(); + + var provider = CreateServiceProvider(serviceCollection); + + Assert.Null(provider.GetService()); + Assert.NotNull(provider.GetService()); + } + + [Fact] + public void ResolveKeyedServiceWithKeyedParameter_MissingRegistrationButWithUnkeyedService() + { + var serviceCollection = new ServiceCollection(); + + // We are not registering "service1" and "service1" keyed IService services and OtherService requires them, + // but we are registering an unkeyed IService service which should not be injected into OtherService. + serviceCollection.AddSingleton(); + + serviceCollection.AddSingleton(); + + var provider = CreateServiceProvider(serviceCollection); + + Assert.NotNull(provider.GetService()); + Assert.Throws(() => provider.GetService()); + } + [Fact] public void CreateServiceWithKeyedParameter() { @@ -432,9 +496,9 @@ public void ResolveKeyedTransientFromScopeServiceProvider() Assert.NotSame(serviceA1, serviceB1); } - internal interface IService { } + public interface IService { } - internal class Service : IService + public class Service : IService { private readonly string _id; @@ -445,7 +509,7 @@ internal class Service : IService public override string? ToString() => _id; } - internal class OtherService + public class OtherService { public OtherService( [FromKeyedServices("service1")] IService service1, @@ -460,6 +524,36 @@ public OtherService( public IService Service2 { get; } } + internal class OtherServiceWithDefaultCtorArgs + { + public OtherServiceWithDefaultCtorArgs( + [FromKeyedServices("service1")] IService service1 = null, + [FromKeyedServices("service2")] IService service2 = null) + { + Service1 = service1; + Service2 = service2; + } + + public IService Service1 { get; } + + public IService Service2 { get; } + } + + internal class ServiceWithOtherService + { + public ServiceWithOtherService( + [FromKeyedServices("service1")] IService service1, + [FromKeyedServices("service2")] IService service2) + { + Service1 = service1; + Service2 = service2; + } + + public IService Service1 { get; } + + public IService Service2 { get; } + } + internal class ServiceWithIntKey : IService { private readonly int _id; diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteFactory.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteFactory.cs index 29f229c17fff8d..7e02dc9391743a 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteFactory.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteFactory.cs @@ -571,6 +571,7 @@ private ConstructorCallSite CreateConstructorCallSite( for (int index = 0; index < parameters.Length; index++) { ServiceCallSite? callSite = null; + bool isKeyedParameter = false; Type parameterType = parameters[index].ParameterType; foreach (var attribute in parameters[index].GetCustomAttributes(true)) { @@ -588,11 +589,15 @@ private ConstructorCallSite CreateConstructorCallSite( { var parameterSvcId = new ServiceIdentifier(keyed.Key, parameterType); callSite = GetCallSite(parameterSvcId, callSiteChain); + isKeyedParameter = true; break; } } - callSite ??= GetCallSite(ServiceIdentifier.FromServiceType(parameterType), callSiteChain); + if (!isKeyedParameter) + { + callSite ??= GetCallSite(ServiceIdentifier.FromServiceType(parameterType), callSiteChain); + } if (callSite == null && ParameterDefaultValue.TryGetDefaultValue(parameters[index], out object? defaultValue)) { diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs index 3e08a16db282e6..00becc9a282827 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs @@ -1290,6 +1290,27 @@ public void ScopedServiceResolvedFromSingletonAfterCompilation3() Assert.Same(sp.GetRequiredService>().Value.PropertyA, sp.GetRequiredService()); } + [Fact] + public void ResolveKeyedServiceWithKeyedParameter_MissingRegistrationButWithUnkeyedService() + { + var serviceCollection = new ServiceCollection(); + + // We are not registering "service1" and "service1" keyed IService services and OtherService requires them, + // but we are registering an unkeyed IService service which should not be injected into OtherService. + serviceCollection.AddSingleton(); + + serviceCollection.AddSingleton(); + + AggregateException ex = Assert.Throws(() => serviceCollection.BuildServiceProvider(new ServiceProviderOptions + { + ValidateOnBuild = true + })); + + Assert.Equal(1, ex.InnerExceptions.Count); + Assert.Contains("ServiceType: Microsoft.Extensions.DependencyInjection.Specification.KeyedDependencyInjectionSpecificationTests+OtherService", ex.Message); + Assert.Contains("Microsoft.Extensions.DependencyInjection.Specification.KeyedDependencyInjectionSpecificationTests+IService", ex.InnerExceptions[0].Message); + } + private async Task ResolveUniqueServicesConcurrently() { var types = new Type[] From 23772c6b8503920334f14e373c8e4497fee778f8 Mon Sep 17 00:00:00 2001 From: Steve Harter Date: Wed, 7 Aug 2024 09:41:23 -0500 Subject: [PATCH 2/4] Fix test issue on NetFx --- .../tests/DI.Tests/ServiceProviderContainerTests.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs index 00becc9a282827..8bcaa9240f6399 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs @@ -1307,8 +1307,9 @@ public void ResolveKeyedServiceWithKeyedParameter_MissingRegistrationButWithUnke })); Assert.Equal(1, ex.InnerExceptions.Count); - Assert.Contains("ServiceType: Microsoft.Extensions.DependencyInjection.Specification.KeyedDependencyInjectionSpecificationTests+OtherService", ex.Message); - Assert.Contains("Microsoft.Extensions.DependencyInjection.Specification.KeyedDependencyInjectionSpecificationTests+IService", ex.InnerExceptions[0].Message); + Assert.StartsWith("Some services are not able to be constructed", ex.Message); + Assert.Contains("ServiceType: Microsoft.Extensions.DependencyInjection.Specification.KeyedDependencyInjectionSpecificationTests+OtherService", ex.ToString()); + Assert.Contains("Microsoft.Extensions.DependencyInjection.Specification.KeyedDependencyInjectionSpecificationTests+IService", ex.ToString()); } private async Task ResolveUniqueServicesConcurrently() From e943973f2c2d5c899461d869864207f6f624ff5d Mon Sep 17 00:00:00 2001 From: Steve Harter Date: Fri, 23 Aug 2024 10:46:21 -0500 Subject: [PATCH 3/4] Add feature switch for compat: Microsoft.Extensions.DependencyInjection.AllowNonKeyedServiceInject --- .../src/ILLink/ILLink.Substitutions.xml | 3 +++ .../src/ServiceLookup/CallSiteFactory.cs | 2 +- .../src/ServiceProvider.cs | 5 ++++ .../DI.Tests/ServiceProviderContainerTests.cs | 24 +++++++++++++++++++ 4 files changed, 33 insertions(+), 1 deletion(-) diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ILLink/ILLink.Substitutions.xml b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ILLink/ILLink.Substitutions.xml index 6aa354ee23683c..e74fb0fb6ac7c6 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ILLink/ILLink.Substitutions.xml +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ILLink/ILLink.Substitutions.xml @@ -6,5 +6,8 @@ + + + diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteFactory.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteFactory.cs index 7e02dc9391743a..5505b3e2a6d1bb 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteFactory.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteFactory.cs @@ -594,7 +594,7 @@ private ConstructorCallSite CreateConstructorCallSite( } } - if (!isKeyedParameter) + if (!isKeyedParameter || ServiceProvider.s_allowNonKeyedServiceInject) { callSite ??= GetCallSite(ServiceIdentifier.FromServiceType(parameterType), callSiteChain); } diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceProvider.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceProvider.cs index ff5efbe98cf334..2eadf6e15a420d 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceProvider.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceProvider.cs @@ -40,6 +40,11 @@ public sealed class ServiceProvider : IServiceProvider, IKeyedServiceProvider, I internal static bool DisableDynamicEngine { get; } = AppContext.TryGetSwitch("Microsoft.Extensions.DependencyInjection.DisableDynamicEngine", out bool disableDynamicEngine) ? disableDynamicEngine : false; + internal static bool AllowNonKeyedServiceInject { get; } = + AppContext.TryGetSwitch("Microsoft.Extensions.DependencyInjection.AllowNonKeyedServiceInject", out bool allowNonKeyedServiceInject) ? allowNonKeyedServiceInject : false; + + internal static readonly bool s_allowNonKeyedServiceInject = AllowNonKeyedServiceInject; + internal static bool VerifyAotCompatibility => #if NETFRAMEWORK || NETSTANDARD2_0 false; diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs index 8bcaa9240f6399..4c88d874e7b726 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ServiceProviderContainerTests.cs @@ -7,6 +7,7 @@ using System.Text; using System.Threading; using System.Threading.Tasks; +using Microsoft.DotNet.RemoteExecutor; using Microsoft.Extensions.DependencyInjection.Extensions; using Microsoft.Extensions.DependencyInjection.Fakes; using Microsoft.Extensions.DependencyInjection.Specification; @@ -1312,6 +1313,29 @@ public void ResolveKeyedServiceWithKeyedParameter_MissingRegistrationButWithUnke Assert.Contains("Microsoft.Extensions.DependencyInjection.Specification.KeyedDependencyInjectionSpecificationTests+IService", ex.ToString()); } + [ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))] + [SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework)] // RuntimeConfigurationOptions are not supported on .NET Framework (and neither is trimming) + public void ResolveKeyedServiceWithKeyedParameter_MissingRegistrationButWithUnkeyedService_FeatureSwitch() + { + RemoteInvokeOptions options = new (); + options.RuntimeConfigurationOptions["Microsoft.Extensions.DependencyInjection.AllowNonKeyedServiceInject"] = bool.TrueString; + + using RemoteInvokeHandle remoteHandle = RemoteExecutor.Invoke(static () => + { + Assert.True(ServiceProvider.s_allowNonKeyedServiceInject); + + var serviceCollection = new ServiceCollection(); + + // Similar to the test above, but we are enabling the feature switch so we don't throw here. + serviceCollection.AddSingleton(); + serviceCollection.AddSingleton(); + serviceCollection.BuildServiceProvider(new ServiceProviderOptions + { + ValidateOnBuild = true + }); + }, options); + } + private async Task ResolveUniqueServicesConcurrently() { var types = new Type[] From ac15a6bf0ee449f978b9c95eb26e0fd00ae12f44 Mon Sep 17 00:00:00 2001 From: Eric StJohn Date: Thu, 29 Aug 2024 12:40:16 -0700 Subject: [PATCH 4/4] Enable servicing Microsoft.Extensions.DependencyInjection --- .../src/Microsoft.Extensions.DependencyInjection.csproj | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/src/Microsoft.Extensions.DependencyInjection.csproj b/src/libraries/Microsoft.Extensions.DependencyInjection/src/Microsoft.Extensions.DependencyInjection.csproj index ce7ac08140311d..cc0bebcb2c129b 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/src/Microsoft.Extensions.DependencyInjection.csproj +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/src/Microsoft.Extensions.DependencyInjection.csproj @@ -7,6 +7,8 @@ $(NoWarn);CP0001 true + true + 1 Default implementation of dependency injection for Microsoft.Extensions.DependencyInjection.