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

ActivatorUtilities.CreateInstance is creating non-empty generic collections #109034

Closed
miguelnunes94 opened this issue Oct 18, 2024 · 6 comments
Closed

Comments

@miguelnunes94
Copy link

Description

Generic collections that have copy constructors (with IEnumerable<T> as parameter) are initialized with elements, when using ActivatorUtilities.CreateInstance/ActivatorUtilities.GetServiceOrCreateInstance.

Reproduction Steps

    public interface ITest { }

    public class Test : ITest { }

    public class Test2 : ITest { }

    public static void Main(string[] args)
    {

        var serviceCollection = new ServiceCollection();

        serviceCollection.AddTransient<ITest, Test>();
        serviceCollection.AddTransient<ITest, Test2>();

        var services = serviceCollection.BuildServiceProvider();

        var collection = ActivatorUtilities.CreateInstance<List<ITest>>(services);
    }

Expected behavior

The collection object is an empty instance of List, because ActivatorUtilities.CreateInstance uses the constructor public List()

Actual behavior

The collection object is an instance of List containing two elements: one instance of Test and one of Test2. Currently, ActivatorUtilities.CreateInstance is selecting the constructor public List(IEnumerable<T> collection).

Regression?

We found this issue while upgrading from .Net 6 to .Net 8

Known Workarounds

No response

Configuration

.Net SDK 8.0.403

Other information

In such cases, the parameterless constructor may be preferable - particularly for generic collections with copy constructors. Consider annotating the expected constructors with the ActivatorUtilitiesConstructorAttribute to ensure the correct constructor is chosen.

It should be related with:

@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Oct 18, 2024
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-extensions-dependencyinjection
See info in area-owners.md if you want to be subscribed.

@julealgon
Copy link

If you want an empty list... why go through ActivatorUtilities for that?

@miguelnunes94
Copy link
Author

The ActivatorUtilities is being used in a serialization & deserialization context where many different types are involved, including system generic collections. We used to have a clean solution by using the it.

@julealgon
Copy link

@miguelnunes94 I believe the behavior difference you are seeing is not related to the target framework itself (.NET6 vs .NET8), but to the version of Microsoft.Extensions.DependencyInjection packages.

There has been a breaking change specifically around ActivatorUtilities and how it selects constructors in MEDI v8:

Regardless, I think your proposal is unreasonable however... if the team decided to pursue it, they would have to annotate all sorts of other constructors for various other collection and non-collection types to avoid picking copy constructors: it's just not feasible IMHO.

To me, you are misusing this class if you are running into this problem, and I would suggest reconsidering your design instead.

Perhaps if you wanted to expose a little bit more how/why you are using ActivatorUtilities in a deserialization context, someone could offer tips or alternatives?

For instance, if you want to force MEDI to use a specific constructor when dealing with the raw List<T> type, I believe you can just override the constructor for that in the container itself using a lambda factory:

services.AddTransient<List<int>>(_ => new List<int>());

Of course... the problem with that is that you would be forced to specify such an override for each specific T, as generic registrations don't have access to the underlying generic type for you to use in a generic factory.

@steveharter
Copy link
Member

steveharter commented Oct 21, 2024

@miguelnunes94 I believe the behavior difference you are seeing is not related to the target framework itself (.NET6 vs .NET8), but to the version of Microsoft.Extensions.DependencyInjection packages.

Correct. Related:

In v9, the ctor selected depends on the arguments but always respects the attribute [ActivatorUtilitiesConstructor]. In the case here, however, that attribute can't be used since it would need to be on List<T>. Somewhat unrelated, but there is an open issue for v10 of having DI's GetService() have the same semantics as ActivatorUtilities.CreateInstance() with respect to [ActivatorUtilitiesConstructor], but that wouldn't apply here due to it being List<T>.

Is there a reason why GetService() can't be used instead? i.e.

var collection = services.GetService<List<ITest>>();

that would just call the List<T> default constructor.

@steveharter steveharter closed this as not planned Won't fix, can't repro, duplicate, stale Oct 21, 2024
@dotnet-policy-service dotnet-policy-service bot removed the untriaged New issue has not been triaged by the area owner label Oct 21, 2024
@miguelnunes94
Copy link
Author

miguelnunes94 commented Oct 26, 2024

Thank you @julealgon and @steveharter for your comments!

I understand that the difference in behavior is related to the breaking change mentioned (as described in the issue). However, it feels strange to see collections initialized with elements, and I'd like to get the team's opinion on this.

I can provide more details on our use case, but I’d like to focus the discussion on the fact that, while ActivatorUtilities can be used with any type, we may encounter unexpected results with .NET collections due to this breaking change.

After those changes, does it feel appropriate for a .NET initialization utility to return .NET collections pre-populated with elements?

Details about our use case

In brief, we have a large framework, widely used across numerous projects and clients, where domain entities and DTOs are represented by the same classes. The entities, which follow the active record pattern, access services through dependency injection.

Since DTOs and entities share the same classes, and the entities require services, deserialization is handled by a contract resolver that supports dependency injection. However, not every type used in the DTOs is registered in the dependency injection container, which is where ActivatorUtilities comes into play.

We often question this architecture, and a major refactoring is planned, but it will take several years to complete. In the meantime, business needs persist, and we continue to keep the framework updated to the latest .NET version.

We can't use services.GetService<List<ITest>>(); since not all types are registered and maintaining registrations like services.AddTransient<List<int>>(_ => new List<int>()); would be impractical, as the framework or clients could use any type.

As a workaround during our upgrade to .NET 8, we added extra logic to the contract resolver. Based on certain rules, it can now selectively use either ActivatorUtilities or Activator to preserve the previous behavior of ActivatorUtilities.

@github-actions github-actions bot locked and limited conversation to collaborators Nov 26, 2024
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

3 participants