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

ActivatorUtilitiesConstructorAttribute Does Not Work in Some Cases #42339

Closed
lawrence-laz opened this issue Jul 24, 2020 · 4 comments
Closed

Comments

@lawrence-laz
Copy link

Describe the bug

ActivatorUtilities.CreateInstance<T>(IServiceProvider provider, params object[] parameters) does not respect ActivatorUtilitiesConstructorAttribute in a case when constructor ordering is changed.

To Reproduce

Steps to reproduce the behavior:

  1. Using version '3.1.6' of package 'Microsoft.Extensions.DependencyInjection'
  2. Run this test code:
    using Microsoft.Extensions.DependencyInjection;
    using System;
    using Xunit;
    
    namespace UnitTests
    {
        public class ActivatorUtilitiesConstructorTests
        {
            public class Dependency1 { }
            public class Dependency2 { }
            public class Dependency3 { }
    
            public class Service1
            {
                public Service1(Dependency1 dependency1, Dependency3 dependency3)
                {
                    throw new Exception("This should not be called.");
                }
    
                [ActivatorUtilitiesConstructor]
                public Service1(Dependency1 dependency1, Dependency2 dependency2, Dependency3 dependency3)
                {
                }
            }
    
            public class Service2
            {
                [ActivatorUtilitiesConstructor]
                public Service2(Dependency1 dependency1, Dependency2 dependency2, Dependency3 dependency3)
                {
                }
    
                public Service2(Dependency1 dependency1, Dependency3 dependency3)
                {
                    throw new Exception("This should not be called.");
                }
            }
    
            [Fact]
            public void CreateInstance_WithActivatorUtilitiesConstructorBeingSecond_ShouldChooseMarkedCtor()
            {
                // Arrange
                var dependency1 = new Dependency1();
                var dependency3 = new Dependency3();
    
                var provider = new ServiceCollection()
                    .AddScoped<Dependency2>()
                    .BuildServiceProvider();
    
                // Act & Assert
                ActivatorUtilities.CreateInstance<Service1>(provider, dependency1, dependency3); // This works.
            }
    
            [Fact]
            public void CreateInstance_WithActivatorUtilitiesConstructorBeingFirst_ShouldChooseMarkedCtor()
            {
                // Arrange
                var dependency1 = new Dependency1();
                var dependency3 = new Dependency3();
    
                var provider = new ServiceCollection()
                    .AddScoped<Dependency2>()
                    .BuildServiceProvider();
    
                // Act & Assert
                ActivatorUtilities.CreateInstance<Service2>(provider, dependency1, dependency3); // This fails.
            }
        }
    }
  3. See error This should not be called. when constructor marked with ActivatorUtilitiesConstructorAttribute is defined first, but no error when it is defined second.

Expected behavior

I expect ActivatorUtilitiesConstructorAttribute to be respected regardless of the order in which constructors were defined.

Additional context

The given example is reproduced using 3 dependencies. Removing one dependency makes both tests pass. Maybe this will help to get on a right path.

@BrennanConroy BrennanConroy transferred this issue from dotnet/extensions Sep 16, 2020
@Dotnet-GitSync-Bot
Copy link
Collaborator

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Sep 16, 2020
@ghost
Copy link

ghost commented Sep 16, 2020

Tagging subscribers to this area: @eerhardt, @maryamariyan
See info in area-owners.md if you want to be subscribed.

@ericstj ericstj added bug and removed untriaged New issue has not been triaged by the area owner labels Sep 18, 2020
@ericstj ericstj added this to the 6.0.0 milestone Sep 18, 2020
@maryamariyan
Copy link
Member

Closing as dupe of #46132 since there is more discussion there

lawrence-laz added a commit to lawrence-laz/runtime that referenced this issue Apr 3, 2021
Using [ActivatorUtilitiesconstructor] attribute on a constructor should
force `ActivatorUtilities.CreateInstance` to use that constructor
regardless of the constructor definition order. This test shows that it
is not the case in current implementaion.
lawrence-laz added a commit to lawrence-laz/runtime that referenced this issue Apr 3, 2021
Previously `ActivatorUtilities.CreateInstance` and
`ActivatorUtilities.CreateFactory` behaved differently: former depended
on ctor definition order to disambiguate ctors, while latter failed
altogether.

The behavior is now unified and addresses concerns raised in dotnet#45119
 dotnet#42339 and dotnet#46132. Constructors are chosen based on the following
factors in a given order:
1) Preferred ctor (having [ActivatorUtilitiesConstructor] attribute)
2) Ctor with the most surjective parameters based on the given arguments.
   In other words constructor that has the least unresolved parameters
   is chosen.
3) Ctor with more resolved parameters is prefered. If two ctors have all
   of the supplied arguments, but one of them has additional parameters
   with default values, then the one with more parameters is chosen.
@ghost ghost locked as resolved and limited conversation to collaborators Apr 29, 2021
@steveharter
Copy link
Member

Note that this issue was closed, but there are still issues with the attribute not working. For example adding this to the original repro fails:

        public class Service3
        {
            [ActivatorUtilitiesConstructor]
            public Service3(Dependency1 dependency1, Dependency3 dependency3)
            {
            }

            public Service3(Dependency1 dependency1, Dependency2 dependency2, object? o)
            {
                throw new Exception("This should not be called.");
            }
        }

        [Fact]
        public void Test()
        {
            var dependency1 = new Dependency1();
            var dependency3 = new Dependency3();

            var provider = new ServiceCollection()
                .AddScoped<Dependency2>()
                .BuildServiceProvider();

            // This should call (d1, d3) but instead calls (d1, d2, null).
            ActivatorUtilities.CreateInstance<Service3>(provider, dependency1, dependency3); 

        }

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants