dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.47k stars 4.76k forks source link

[API Proposal]: Allow Dependency Injection registration of classes that implent more than one interface #109480

Open danzuep opened 4 weeks ago

danzuep commented 4 weeks ago

Background and motivation

There are two features I would like to see added to Microsoft.Extensions.DependencyInjection.ServiceCollectionServiceExtensions, both that can be added with a couple of extension methods.

The first is to be able to specify the lifetime when adding services to the collection. As a library maintainer, people have asked me to add a one line "services.AddLibrary()" extension method, but currently the easiest way to implement Transient, Singleton, or Scoped service lifetimes is to copy and paste registrations then rename. The service collection has a way to do this, but it is not exposed for external usage.

The other feature I'd like to use is to be able to register classes that implent more than one interface. These limitations have been highlighted multiple times before, e.g. https://github.com/aspnet/DependencyInjection/issues/360.

Both of these limitations can be solved relatively easily, so why not make it accessible to everyone else? The usage is: services.Add<I1, I2, Concrete>(serviceLifetime);. I've put a more comprehensive solution on GitHub.

API Proposal

namespace Microsoft.Extensions.DependencyInjection.Extensions;

public static class ServiceCollectionExtensions
{
    public static IServiceCollection Add<TService, [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] TImplementation>(
        this IServiceCollection services,
        ServiceLifetime serviceLifetime)
        where TService : class
        where TImplementation : class, TService =>
        services.Add<TImplementation>(serviceLifetime, typeof(TService));

    public static IServiceCollection Add<T1, T2, [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] TImplementation>(
        this IServiceCollection services,
        ServiceLifetime serviceLifetime)
        where T1 : class
        where T2 : class
        where TImplementation : class, T1, T2 =>
        services.Add<TImplementation>(serviceLifetime, typeof(T1), typeof(T2));

    private static IServiceCollection AddInheritedTypes(
        IServiceCollection collection,
        ServiceLifetime lifetime,
        Type implementationType,
        params Type[] inheritedTypes)
    {
        // Add a ServiceDescriptors for each service type the implemented type inherits from
        if (inheritedTypes?.Length > 0)
        {
            foreach (var serviceType in inheritedTypes)
            {
                // Ensure each implementationType is assignable to the serviceType
                if (!serviceType.IsAssignableFrom(implementationType))
                {
                    throw new ArgumentException($"{serviceType.Name} must be assignable from {implementationType.Name}.");
                }

                // Add a new ServiceDescriptor for each interface of the service type using the previously registered concrete service
                var serviceDescriptor = new ServiceDescriptor(serviceType, provider => provider.GetRequiredService(implementationType), lifetime);
                collection.Add(serviceDescriptor);
            }
        }
        return collection;
    }

    public static IServiceCollection Add<TImplementation>(
        this IServiceCollection collection,
        ServiceLifetime serviceLifetime,
        Func<IServiceProvider, TImplementation> implementationFactory,
        params Type[] inheritedTypes)
        where TImplementation : class
    {
        ArgumentNullException.ThrowIfNull(implementationFactory);
        var implementationType = typeof(TImplementation);
        // Transient lifestyle breaks this pattern by definition, so use scoped instead
        var concreteLifetime = serviceLifetime == ServiceLifetime.Transient ? ServiceLifetime.Scoped : serviceLifetime;
        // Add the implemented service type ServiceDescriptor
        collection.Add(new ServiceDescriptor(implementationType, implementationFactory, concreteLifetime));
        // Add a ServiceDescriptor for each inherited service type
        AddInheritedTypes(collection, serviceLifetime, implementationType, inheritedTypes);
        return collection;
    }

    public static IServiceCollection Add<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] TImplementation>(
        this IServiceCollection serviceCollection,
        ServiceLifetime serviceLifetime,
        params Type[] inheritedTypes)
    {
        var implementationType = typeof(TImplementation);
        // Transient lifestyle breaks this pattern by definition, so use scoped instead
        var concreteLifetime = serviceLifetime == ServiceLifetime.Transient ? ServiceLifetime.Scoped : serviceLifetime;
        // Add the implemented service type ServiceDescriptor
        serviceCollection.Add(new ServiceDescriptor(implementationType, implementationType, concreteLifetime));
        // Add a ServiceDescriptor for each inherited service type
        AddInheritedTypes(serviceCollection, serviceLifetime, implementationType, inheritedTypes);
        return serviceCollection;
    }
}

API Usage

public static class Example
{
    public interface I1 { }
    public interface I2 : I1 { }

    public class Concrete : I2 { }

    public static void RegisterConcrete(this IServiceCollection services, ServiceLifetime serviceLifetime) =>
        services.Add<I1, I2, Concrete>(serviceLifetime);
}

Alternative Designs

Alternative options could include using a different method name, not including the overloads, not including ServiceLifetime etc.

One alternative could be to using a type of builder for fluent composability:

services.Add<Concrete>().As<I1>().As<I2>().As<I3>().Singleton();
services.Add<Service>().As<I4>().As<I5>().Scoped();
services.Add<Api>().As<I6>().As<I7>().Scoped();

At that point you start to question why you wouldn't just use Autofac though, so let's just keep it simple.

Risks

No breaking changes, slight performance hit for people who use it but most people would be happy to at least have this as an option.

wvpm commented 4 weeks ago

Add<TService, TImplementation> already exists in the form of Add(IServiceCollection, ServiceDescriptor) . Note it does not have a default ServiceLifetime and it should not have one. Registering services Scopes by default is a very opinionated implementation.

As for registering one implementation type (or one instance) as multiple service types, in my personal experience this signals an implementation type that does too much and breaks the single responsibility principle. I doubt it's wise to encourage that by adding additional extension methods.

If you do find yourself in a situation where you often use this, you can already include said extension methods in your project. To me, it doesn't feel as something the dotnet framework should include.

colejohnson66 commented 3 weeks ago

Is your proposal that a class with n interface implementations be registered all at once on one line instead of n lines?

Concrete impl = new();
// before
builder.RegisterX<IFace1>(impl);
builder.RegisterX<IFace2>(impl);
builder.RegisterX<IFace3>(impl);
// after
builder.RegisterX<IFace1, IFace2, IFace3>(impl);
julealgon commented 3 weeks ago

I just don't see how such a proposal can scale using the current API of MEDI.

For something like this to be feasible, I think the entire API surface would have to be reconsidered to be more composable. The existing patterns are extremely unflexible and these types of additions only add confusion to how it should be used.

It would be a nice addition though, conceptually. I've needed it multiple times myself and it is a PITA to register it when using lambdas.


@colejohnson66

Is your proposal that a class with n interface implementations be registered all at once on one line instead of n lines?

Concrete impl = new(); // before builder.RegisterX(impl); builder.RegisterX(impl); builder.RegisterX(impl); // after builder.RegisterX<IFace1, IFace2, IFace3>(impl);

That looks simple enough because you are providing an instance (which is usually to be avoided). If you don't, this starts to become a mess and you are basically forced to register the concrete type itself:

// before
builder.RegisterX<Concrete>();
builder.RegisterX<IFace1>(p => p.GetRequiredService<Concrete>());
builder.RegisterX<IFace2>(p => p.GetRequiredService<Concrete>());
builder.RegisterX<IFace3>(p => p.GetRequiredService<Concrete>());

// after
builder.RegisterX<IFace1, IFace2, IFace3, Concrete>();

This is "delegation" is especially important when using scoped and singleton objects to ensure the lifetimes are honored.

danzuep commented 3 weeks ago

Registering services Scopes by default is a very opinionated implementation.

You're right, I've removed the default as that was uncalled for.

As for registering one implementation type (or one instance) as multiple service types, in my personal experience this signals an implementation type that does too much and breaks the single responsibility principle.

In my own software I agree, but recently I've wanted this functionality when migrating client software and that part of the cleanup would come later. Also, it's not that rare for I2 to implement I1, then some services depend on I1 and others on I2.

danzuep commented 3 weeks ago

Is your proposal that a class with n interface implementations be registered all at once on one line instead of n lines?

At the essense of the proposal, yes. The example you gave and the others in the sample code are trivial, but you can imagine with that if the Concrete implementation had ten dependencies which each had ten of their own dependencies etc. you end up with a lot of dependencies. Registering it this way for that scenario could save a lot of hassle (this is the scenario I faced in someone's code last week - bad I know).

danzuep commented 3 weeks ago

I think the entire API surface would have to be reconsidered to be more composable. The existing patterns are extremely unflexible and these types of additions only add confusion to how it should be used.

I think that's the bigger picture here, composablility. A builder pattern would do wonders.

It would be a nice addition though, conceptually. I've needed it multiple times myself and it is a PITA to register it when using lambdas.

This is the type of use case I'm thinking of:

services.AddTransient<I1, I2, I3, Concrete>((_) => new Concrete());
services.AddTransient<I4, I5, Service>(provider => new Service(provider.GetRequiredService<I2>()));
services.AddTransient<I6, I7, Api>(provider => new Api(provider.GetRequiredService<I5>()));

Even better if we could do this:

services.AddTransient<I1, I2, I3, Concrete>();
services.AddTransient<I4, I5, Service>();
services.AddTransient<I6, I7, Api>();

Or this (cudos to Autofac for 'As'):

services.Add<Concrete>().As<I1>().As<I2>().As<I3>().Transient();
services.Add<Service>().As<I4>().As<I5>().Transient();
services.Add<Api>().As<I6>().As<I7>().Transient();

All of them are much simpler than the current ways!

danzuep commented 3 weeks ago

I actually really like that last one, but composability is a separate topic. "Add" would return a service builder object, then "Singleton", "Scoped" etc. would build and add the service to the collection.

services.Add<Concrete>().As<I1>().As<I2>().As<I3>().Singleton();
services.Add<Service>().As<I4>().As<I5>().Scoped();
services.Add<Api>().As<I6>().As<I7>().Scoped();
steveharter commented 1 week ago

danzuep thanks for the issue; please update it to what you think is the preferred API - e.g. adding As<T>.

danzuep commented 1 week ago

@steveharter I've updated the proposal to be just two new public methods added to the existing extensions library. More could be added on top of that that aid with usage (e.g. services.AddSingleton<I1, I2, Concrete>();), but I'll leave that up to the team to decide.