From 30be3c219e1a27f591f2411bd64114effccf5eee Mon Sep 17 00:00:00 2001 From: Steve Harter Date: Fri, 1 Mar 2024 11:04:04 -0600 Subject: [PATCH 1/3] ActivatorUtilities.CreateInstance() should respect [ActivatorUtilitiesConstructor] --- .../src/ActivatorUtilities.cs | 7 +- .../tests/DI.Tests/ActivatorUtilitiesTests.cs | 193 ++++++++++++++++++ 2 files changed, 199 insertions(+), 1 deletion(-) diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs b/src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs index e8eae2d2e9eee6..c30bbf5c4deaa6 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs @@ -107,12 +107,15 @@ public static object CreateInstance( if (isPreferred || bestLength < length) { + // When a preferred constructor is found, it is always selected however subsequent constructors + // will be selected if they have more parameters. bestLength = length; bestMatcher = matcher; multipleBestLengthFound = false; } - else if (bestLength == length) + else if (bestLength == length && bestMatcher.Constructor?.IsPreferred == false) { + // The best constructor was not the one with the attribute, so we have an ambiguous case. multipleBestLengthFound = true; } @@ -716,6 +719,8 @@ public ConstructorMatcher(ConstructorInfoEx constructor) _parameterValues = new object[constructor.Parameters.Length]; } + public ConstructorInfoEx Constructor { get { return _constructor; } } + public int Match(object[] givenParameters, IServiceProviderIsService serviceProviderIsService) { for (int givenIndex = 0; givenIndex < givenParameters.Length; givenIndex++) diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ActivatorUtilitiesTests.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ActivatorUtilitiesTests.cs index f6e7c2f3a8eb7c..cba82ddda26dee 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ActivatorUtilitiesTests.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ActivatorUtilitiesTests.cs @@ -170,6 +170,97 @@ public void CreateInstance_ClassWithABC_ConstructorWithAttribute_PicksCtorWithAt Assert.Same(a, instance.A); } + [Fact] + public void CreateInstanceFailsWithAmbiguousConstructor() + { + var serviceCollection = new ServiceCollection(); + serviceCollection.AddTransient(); + serviceCollection.AddTransient(); + serviceCollection.AddTransient(); + + var serviceProvider = serviceCollection.BuildServiceProvider(); + + // Neither ctor(A) nor ctor(B) have [ActivatorUtilitiesConstructor]. + Assert.Throws(() => ActivatorUtilities.CreateInstance(serviceProvider)); + } + + [Fact] + public void CreateInstanceFailsWithAmbiguousConstructor_ReversedOrder() + { + var serviceCollection = new ServiceCollection(); + serviceCollection.AddTransient(); + serviceCollection.AddTransient(); + serviceCollection.AddTransient(); + + var serviceProvider = serviceCollection.BuildServiceProvider(); + + // Neither ctor(A) nor ctor(B) have [ActivatorUtilitiesConstructor]. + Assert.Throws(() => ActivatorUtilities.CreateInstance(serviceProvider)); + } + + [Fact] + public void CreateInstancePassesWithAmbiguousConstructor() + { + var serviceCollection = new ServiceCollection(); + serviceCollection.AddTransient(); + serviceCollection.AddTransient(); + serviceCollection.AddTransient(); + + var serviceProvider = serviceCollection.BuildServiceProvider(); + var service = ActivatorUtilities.CreateInstance(serviceProvider); + + // Ensure ctor(A) was selected over ctor(B) since A has [ActivatorUtilitiesConstructor]. + Assert.NotNull(service.A); + } + + [Fact] + public void CreateInstancePassesWithAmbiguousConstructor_ReversedOrder() + { + var serviceCollection = new ServiceCollection(); + serviceCollection.AddTransient(); + serviceCollection.AddTransient(); + serviceCollection.AddTransient(); + + var serviceProvider = serviceCollection.BuildServiceProvider(); + var service = ActivatorUtilities.CreateInstance(serviceProvider); + + // Ensure ctor(A) was selected over ctor(B) since A has [ActivatorUtilitiesConstructor]. + Assert.NotNull(service.A); + } + + [Fact] + public void CreateInstanceIgnoresActivatorUtilitiesConstructorAttribute() + { + var serviceCollection = new ServiceCollection(); + serviceCollection.AddTransient(); + serviceCollection.AddTransient(); + serviceCollection.AddTransient(); + + var serviceProvider = serviceCollection.BuildServiceProvider(); + var service = ActivatorUtilities.CreateInstance(serviceProvider); + + // Ensure ctor(AB) was selected even though ctor(A) had [ActivatorUtilitiesConstructor]. + // Longer constructors are selected if they appear after the one with [ActivatorUtilitiesConstructor]. + Assert.NotNull(service.A); + Assert.NotNull(service.B); + } + + [Fact] + public void CreateInstanceIgnoresActivatorUtilitiesConstructorAttribute_ReversedOrder() + { + var serviceCollection = new ServiceCollection(); + serviceCollection.AddTransient(); + serviceCollection.AddTransient(); + serviceCollection.AddTransient(); + + var serviceProvider = serviceCollection.BuildServiceProvider(); + var service = ActivatorUtilities.CreateInstance(serviceProvider); + + // Ensure ctor(A) was selected since it has [ActivatorUtilitiesConstructor]. It exists after ctor(AB). + Assert.NotNull(service.A); + Assert.Null(service.B); + } + [Fact] public void CreateInstance_ClassWithABC_MultipleCtorsWithSameLength_ThrowsAmbiguous() { @@ -662,6 +753,108 @@ public ClassWithABC_LastConstructorWithAttribute(B b, C c) : this(null, b, c) { public ClassWithABC_LastConstructorWithAttribute(A a, B b, C c) : base(a, b, c) { } } + internal class ClassWithA_And_B + { + public ClassWithA_And_B(A a) + { + A = a; + } + + public ClassWithA_And_B(B b) + { + B = b; + } + + public A A { get; } + public B B { get; } + } + + internal class ClassWithB_And_A + { + public ClassWithB_And_A(A a) + { + A = a; + } + + public ClassWithB_And_A(B b) + { + B = b; + } + + public A A { get; } + public B B { get; } + } + + internal class ClassWithA_And_B_ActivatorUtilitiesConstructorAttribute + { + [ActivatorUtilitiesConstructor] + public ClassWithA_And_B_ActivatorUtilitiesConstructorAttribute(A a) + { + A = a; + } + + public ClassWithA_And_B_ActivatorUtilitiesConstructorAttribute(B b) + { + B = b; + } + + public A A { get; } + public B B { get; } + } + + internal class ClassWithB_And_A_ActivatorUtilitiesConstructorAttribute + { + public ClassWithB_And_A_ActivatorUtilitiesConstructorAttribute(B b) + { + B = b; + } + + [ActivatorUtilitiesConstructor] + public ClassWithB_And_A_ActivatorUtilitiesConstructorAttribute(A a) + { + A = a; + } + + public A A { get; } + public B B { get; } + } + + internal class ClassWithA_And_AB_ActivatorUtilitiesConstructorAttribute + { + [ActivatorUtilitiesConstructor] + public ClassWithA_And_AB_ActivatorUtilitiesConstructorAttribute(A a) + { + A = a; + } + + public ClassWithA_And_AB_ActivatorUtilitiesConstructorAttribute(A a, B b) + { + A = a; + B = b; + } + + public A A { get; } + public B B { get; } + } + + internal class ClassWithAB_And_A_ActivatorUtilitiesConstructorAttribute + { + public ClassWithAB_And_A_ActivatorUtilitiesConstructorAttribute(A a, B b) + { + A = a; + B = b; + } + + [ActivatorUtilitiesConstructor] + public ClassWithAB_And_A_ActivatorUtilitiesConstructorAttribute(A a) + { + A = a; + } + + public A A { get; } + public B B { get; } + } + internal class FakeServiceProvider : IServiceProvider { private IServiceProvider _inner; From 1d3d47e176db3abcc7392805e0cce7109bf4e068 Mon Sep 17 00:00:00 2001 From: Steve Harter Date: Tue, 5 Mar 2024 12:09:15 -0600 Subject: [PATCH 2/3] Have ctor with [ActivatorUtilitiesConstructor] always be selected --- .../src/ActivatorUtilities.cs | 26 ++++++++++--------- .../tests/DI.Tests/ActivatorUtilitiesTests.cs | 7 +++-- 2 files changed, 17 insertions(+), 16 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs b/src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs index c30bbf5c4deaa6..c484978c884dc4 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs @@ -103,23 +103,27 @@ public static object CreateInstance( { ThrowMarkedCtorDoesNotTakeAllProvidedArguments(); } - } - if (isPreferred || bestLength < length) - { - // When a preferred constructor is found, it is always selected however subsequent constructors - // will be selected if they have more parameters. + seenPreferred = true; bestLength = length; bestMatcher = matcher; multipleBestLengthFound = false; } - else if (bestLength == length && bestMatcher.Constructor?.IsPreferred == false) + else if (!seenPreferred) { - // The best constructor was not the one with the attribute, so we have an ambiguous case. - multipleBestLengthFound = true; + if (bestLength < length) + { + // When a preferred constructor is found, it is always selected however subsequent constructors + // will be selected if they have more parameters. + bestLength = length; + bestMatcher = matcher; + multipleBestLengthFound = false; + } + else if (bestLength == length) + { + multipleBestLengthFound = true; + } } - - seenPreferred |= isPreferred; } if (bestLength != -1) @@ -719,8 +723,6 @@ public ConstructorMatcher(ConstructorInfoEx constructor) _parameterValues = new object[constructor.Parameters.Length]; } - public ConstructorInfoEx Constructor { get { return _constructor; } } - public int Match(object[] givenParameters, IServiceProviderIsService serviceProviderIsService) { for (int givenIndex = 0; givenIndex < givenParameters.Length; givenIndex++) diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ActivatorUtilitiesTests.cs b/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ActivatorUtilitiesTests.cs index cba82ddda26dee..1144ff33a1c7b6 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ActivatorUtilitiesTests.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ActivatorUtilitiesTests.cs @@ -239,10 +239,9 @@ public void CreateInstanceIgnoresActivatorUtilitiesConstructorAttribute() var serviceProvider = serviceCollection.BuildServiceProvider(); var service = ActivatorUtilities.CreateInstance(serviceProvider); - // Ensure ctor(AB) was selected even though ctor(A) had [ActivatorUtilitiesConstructor]. - // Longer constructors are selected if they appear after the one with [ActivatorUtilitiesConstructor]. + // Ensure ctor(A) was selected since A has [ActivatorUtilitiesConstructor]. Assert.NotNull(service.A); - Assert.NotNull(service.B); + Assert.Null(service.B); } [Fact] @@ -256,7 +255,7 @@ public void CreateInstanceIgnoresActivatorUtilitiesConstructorAttribute_Reversed var serviceProvider = serviceCollection.BuildServiceProvider(); var service = ActivatorUtilities.CreateInstance(serviceProvider); - // Ensure ctor(A) was selected since it has [ActivatorUtilitiesConstructor]. It exists after ctor(AB). + // Ensure ctor(A) was selected since it has [ActivatorUtilitiesConstructor]. Assert.NotNull(service.A); Assert.Null(service.B); } From 5b3d4a559eec2b2b94d70dccff93dd81387c8f8e Mon Sep 17 00:00:00 2001 From: Steve Harter Date: Tue, 5 Mar 2024 14:18:51 -0600 Subject: [PATCH 3/3] Remove comment --- .../src/ActivatorUtilities.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs b/src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs index c484978c884dc4..8c8fcbd5f18b45 100644 --- a/src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs +++ b/src/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs @@ -113,8 +113,6 @@ public static object CreateInstance( { if (bestLength < length) { - // When a preferred constructor is found, it is always selected however subsequent constructors - // will be selected if they have more parameters. bestLength = length; bestMatcher = matcher; multipleBestLengthFound = false;