ActivatorUtilities needs to use the longest available constructor when multiple ctors are available

[mkArtakMSFT]

From @mkArtakMSFT on February 12, 2018 18:48

From @jbagga on February 5, 2018 21:54

For cases that need a new constructor for a class (additional param), in order to avoid a breaking change we add an overload. However, ActivatorUtilities does not allow multiple applicable constructors.

Message: System.InvalidOperationException : Multiple constructors accepting all given argument types have been found in type 
'Microsoft.AspNetCore.Mvc.Razor.TagHelpers.HeadTagHelper'. There should only be one applicable constructor.

This restricts TagHelpers from changing the constructors.

cc @rynowak

Copied from original issue: aspnet/Mvc#7330

Copied from original issue: aspnet/DependencyInjection#627

[mkArtakMSFT]

From @NTaylorMullen on February 5, 2018 22:22

I'd think if we wanted to support multiple ctors we'd take an approach of decorating the constructor we'd want to use with some sort of attribute to say "Use this constructor when activating". Other DI systems do this already.

Granted I think it'd be a bigger change outside of MVC if we were going to do this (don't think it's worth doing for just TagHelpers).

[mkArtakMSFT]

@jbagga, what are the scenarios this would enable?

[mkArtakMSFT]

From @jbagga on February 6, 2018 17:59

Multiple constructors for TagHelpers (and also other types that use ActivitorUtilities in MVC to create instances). But @rynowak said it was particularly problematic for TagHelpers

[mkArtakMSFT]

From @rynowak on February 7, 2018 3:17

We have created a potential problem for our users everywhere that we use ActivatorUtilities. This includes things like tag helpers, controllers, view components, page models, filters, etc.

ActivatorUtilities only supports a single constructor (in our use cases).

This means that if you both:

1. Need to add constructor parameters
2. Care about making breaking changes to your API

Then you have a problem.

This is particularly bad for tag helpers since we intend for it to be possible for someone to ship them as a library. This is also bad for everything else, since shipping those in libraries is a thing as well.

---

property injection is the best injection IMO

[mkArtakMSFT]

We just had a meeting about this and the solution we will go forward for ActivatorUtilities will be to take the only longest available public constructor for instantiation.

[Tratcher]

This is now blocking a middleware fix. See #2915

[Eilon]

@mkArtakMSFT / @muratg - if this is blocking @Tratcher 's work can you guys figure out how to get this on the priority list for preview2?

[campersau]

Nice! As a workaround I ordered the constructors in the source code so that the first constructor is the one I want to be activated by DI.

[muratg]

@Tratcher That bug was closed. What's the exact fix that was blocked?

@pakrym @davidfowl Any thoughts on this? Are we picking up the first constructor currently? Would changing this be a breaking change?

[campersau]

Test case:

var serviceProvider = new ServiceCollection().AddSingleton<dep2>().BuildServiceProvider();

ActivatorUtilities.CreateInstance<test1>(serviceProvider, new object[] { new dep1() });
ActivatorUtilities.CreateInstance<test2>(serviceProvider, new object[] { new dep1() });


class dep1 { }
class dep2 { }

class test1
{
    public test1(dep1 a) { }  // this constructor is used
    public test1(dep1 a, dep2 b) { }
}

class test2
{
    public test2(dep1 a, dep2 b) { } // this constructor is used
    public test2(dep1 a) { }
}

[Tratcher]

@muratg aspnet/Security#1588

I'll try the ordering workaround in Security but that wasn't working consistently in some UseMiddleware tests last night.

[pakrym]

There are two separate issues here that should be treated differently.

1. The one originally reported by @jbagga, where ActivatorUtilities would throw because of multiple applicable constructors. We can enable this scenario by selecting the longest one and it won't be a breaking change because things didn't work before at all.
2. The other issue is the one that Chris is seeing, where there is, for some reason, no exception. First, we should figure out why isn't it throwing when obviously having multiple applicable constructors. Then add a test that maintains this behaviour. And try not to break it while fixing issue 1. Fixing issue 2 might be a breaking change.

[pranavkm]

The one originally reported by @jbagga, where ActivatorUtilities would throw because of multiple applicable constructors.

I think depending on the code path, CreateInstance with non-DI arguments and the one where it's basically DI anti-pattern, you may not get this error: https://github.com/aspnet/DependencyInjection/blob/dev/shared/Microsoft.Extensions.ActivatorUtilities.Sources/ActivatorUtilities.cs#L41-L74

Could we possibly model it around a contract where you specify exactly all the ctor argument types to remove all ambiguity around this?
ActivatorUtilities.CreateInstance(IServiceProvider, Type[] constructorArgs, object[] defaultValues)

[Tratcher]

Found it:
https://github.com/aspnet/DependencyInjection/blob/0c2dac8b2a0e9495cf5c3de10143ea656bfab3c1/shared/Microsoft.Extensions.ActivatorUtilities.Sources/ActivatorUtilities.cs#L48-L64
A) This code path has no provision for throwing if there are multiple constructors.
B) It's trying to do longest match but the ConstructorMatcher is only matching against the supplementary params list, it doesn't look at DI at all. Match returns 0 for most constructors, which is greater than -1 so the first constructor wins.

Re-ordering my constructors did work around the issue but is not a long term solution.

[pakrym]

Why do we have two completely different code paths for these methods?

[pranavkm]

@pakrym you make it sound like we thought this type through 😄

[Tratcher]

HttpsRedirect and IIS middlewares also have multiple constructors, but they add local params rather than DI params so it appears to work.

[muratg]

Triaging this in based on the discussions so far.