Logo Questions Linux Laravel Mysql Ubuntu Git Menu
 

Unity injection with too many constructor parameters

I have the following Unity related question. The code stub below sets up the basic scenario and the question is at the bottom.

NOTE, that [Dependency] attribute does not work for the example below and results in StackoverflowException, but the constructor injection does work.

NOTE(2) Some of the comments below started to assign "labels", like code smell, bad design, etc... So, for the avoidance of confusion here is the business setup without any design first.

The question seems to cause a severe controversy even among some of the best-known C# gurus. In fact, the question is far beyond C# and it falls more into pure computer science. The question is based on the well-known "battle" between a service locator pattern and pure dependency injection pattern: https://martinfowler.com/articles/injection.html vs http://blog.ploeh.dk/2010/02/03/ServiceLocatorisanAnti-Pattern/ and a subsequent update to remedy the situation when the dependency injection becomes too complicated: http://blog.ploeh.dk/2010/02/02/RefactoringtoAggregateServices/

Here is the situation, which does not fit nicely into what is described in the last two but seems to fit perfectly into the first one.

I have a large (50+) collection of what I called micro services. If you have a better name, please "apply" it when reading. Each of them operates on a single object, let's call it quote. However, a tuple (context + quote) seems more appropriate. Quote is a business object, which gets processed and serialized into a database and context is some supporting information, which is necessary while quote is being processed, but is not saved into the database. Some of that supporting information may actually come from database or from some third-party services. This is irrelevant. Assembly line comes to mind as a real-world example: an assembly worker (micro service) receives some input (instruction (context) + parts (quote)), processes it (does something with parts according to instruction and / or modifies instruction) and passes it further if successful OR discards it (raises exception) in case of issues. The micro services eventually get bundled up into a small number (about 5) of high-level services. This approach linearizes processing of some very complex business object and allows testing each micro service separately from all others: just give it an input state and test that it produces expected output.

Here is where it gets interesting. Because of the number of steps involved, high-level services start to depend on many micro services: 10+ and more. This dependency is natural, and it just reflects the complexity of the underlying business object. On top of that micro services can be added / removed nearly on a constant basis: basically, they are some business rules, which are almost as fluid as water.

That severely clashes with Mark's recommendation above: if I have 10+ effectively independent rules applied to a quote in some high-level service, then, according to the third blog, I should aggregate them into some logical groups of, let's say no more than 3-4 instead of injecting all 10+ via constructor. But there are no logical groups! While some of the rules are loosely dependent, most of them are not and so artificially bundling them together will do more harm than good.

Throw in that the rules change frequently, and it becomes a maintenance nightmare: all real / mocked calls must be updated every time the rules change.

And I have not even mentioned that the rules are US state dependent and so, in theory, there are about 50 collections of rules with one collection per each state and per each workflow. And while some of the rules are shared among all states (like "save quote to the database"), there are a lot of state specific rules.

Here is a very simplified example.

Quote - business object, which gets saved into database.

public class Quote
{
    public string SomeQuoteData { get; set; }
    // ...
}

Micro services. Each of them performs some small update(s) to quote. Higher level services can be also built from some lower level micro services as well.

public interface IService_1
{
    Quote DoSomething_1(Quote quote);
}
// ...

public interface IService_N
{
    Quote DoSomething_N(Quote quote);
}

All micro services inherit from this interface.

public interface IQuoteProcessor
{
    List<Func<Quote, Quote>> QuotePipeline { get; }
    Quote ProcessQuote(Quote quote = null);
}

// Low level quote processor. It does all workflow related work.
public abstract class QuoteProcessor : IQuoteProcessor
{
    public abstract List<Func<Quote, Quote>> QuotePipeline { get; }

    public Quote ProcessQuote(Quote quote = null)
    {
        // Perform Aggregate over QuotePipeline.
        // That applies each step from workflow to a quote.
        return quote;
    }
}

One of high level "workflow" services.

public interface IQuoteCreateService
{
    Quote CreateQuote(Quote quote = null);
}

and its actual implementation where we use many of low level micro services.

public class QuoteCreateService : QuoteProcessor, IQuoteCreateService
{
    protected IService_1 Service_1;
    // ...
    protected IService_N Service_N;

    public override List<Func<Quote, Quote>> QuotePipeline =>
        new List<Func<Quote, Quote>>
        {
            Service_1.DoSomething_1,
            // ...
            Service_N.DoSomething_N
        };

    public Quote CreateQuote(Quote quote = null) => 
        ProcessQuote(quote);
}

There are two main ways to achieve DI:

Standard approach is to inject all dependencies through constructor:

    public QuoteCreateService(
        IService_1 service_1,
        // ...
        IService_N service_N
        )
    {
        Service_1 = service_1;
        // ...
        Service_N = service_N;
    }

And then register all types with Unity:

public static class UnityHelper
{
    public static void RegisterTypes(this IUnityContainer container)
    {
        container.RegisterType<IService_1, Service_1>(
            new ContainerControlledLifetimeManager());
        // ...
        container.RegisterType<IService_N, Service_N>(
            new ContainerControlledLifetimeManager());

        container.RegisterType<IQuoteCreateService, QuoteCreateService>(
            new ContainerControlledLifetimeManager());
    }
}

Then Unity will do its "magic" and resolve all services at run time. The problem is that currently we have about 30 such micro services and the number is expected to increase. Subsequently some of the constructors are already getting 10+ services injected. This is inconvenient to maintain, mock, etc...

Sure, it is possible to use the idea from here: http://blog.ploeh.dk/2010/02/02/RefactoringtoAggregateServices/ However, the microservices are not really related to each other and so bundling them together is an artificial process without any justification. In addition, it will also defeat the purpose of making the whole workflow linear and independent (a micro service takes a current "state", then preforms some action with quote, and then just moves on). None of them cares about any other micro services before or after them.

An alternative idea seems to create a single "service repository":

public interface IServiceRepository
{
    IService_1 Service_1 { get; set; }
    // ...
    IService_N Service_N { get; set; }

    IQuoteCreateService QuoteCreateService { get; set; }
}

public class ServiceRepository : IServiceRepository
{
    protected IUnityContainer Container { get; }

    public ServiceRepository(IUnityContainer container)
    {
        Container = container;
    }

    private IService_1 _service_1;

    public IService_1 Service_1
    {
        get => _service_1 ?? (_service_1 = Container.Resolve<IService_1>());
        set => _service_1 = value;
    }
    // ...
}

Then register it with Unity and change the constructor of all relevant services to something like this:

    public QuoteCreateService(IServiceRepository repo)
    {
        Service_1 = repo.Service_1;
        // ...
        Service_N = repo.Service_N;
    }

The benefits of this approach (in comparison to the previous one) are as follows:

All micro services and higher-level services can be created in a unified form: new micro services can be easily added / removed without the need to fix constructor call for the services and all unit tests. Subsequently, maintenance and complexity decreases.

Due to interface IServiceRepository, it is easy to create an automated unit test, which will iterate over all properties and validate that all services can be instantiated, which means that there will be no nasty run time surprises.

The problem with this approach is that it starts looking a lot like a service locator, which some people consider as an anti-pattern: http://blog.ploeh.dk/2010/02/03/ServiceLocatorisanAnti-Pattern/ and then people start to argue that that all dependencies must be made explicit and not hidden as in ServiceRepository.

What shall I do with that?

like image 205
Konstantin Konstantinov Avatar asked Aug 21 '18 00:08

Konstantin Konstantinov


2 Answers

I would just create one interface:

public interface IDoSomethingAble
{
    Quote DoSomething(Quote quote);
}

And a Aggregate:

public interface IDoSomethingAggregate : IDoSomethingAble {}

public class DoSomethingAggregate : IDoSomethingAggregate 
{
    private IEnumerable<IDoSomethingAble> somethingAbles;

    public class DoSomethingAggregate(IEnumerable<IDoSomethingAble> somethingAbles)
    {
        _somethingAbles = somethingAbles;
    }

    public Quote DoSomething(Quote quote)
    {
        foreach(var somethingAble in _somethingAbles)
        {
            somethingAble.DoSomething(quote);
        }
        return quote;
    }
}

Note: Dependency injection doesn't mean, you need to use it everywhere.

I would go for a factory:

public class DoSomethingAggregateFactory
{
    public IDoSomethingAggregate Create()
    {
        return new DoSomethingAggregate(GetItems());
    }

    private IEnumerable<IDoSomethingAble> GetItems()
    {
        yield return new Service1();
        yield return new Service2();
        yield return new Service3();
        yield return new Service4();
        yield return new Service5();
    }
}

Everything else (which is not constructor injected) hides explicit dependencies.


As a last resort, you could also create some DTO object, inject every needed Service via the Constructor (But only one time).

This way you can request the ProcessorServiceScope and have all Service available without needing to create the ctor logic for every class:

public class ProcessorServiceScope
{
    public Service1 Service1 {get;};
    public ServiceN ServiceN {get;};

    public ProcessorServiceScope(Service1 service1, ServiceN serviceN)
    {
        Service1 = service1;
        ServiceN = serviceN;
    }
}

public class Processor1
{
    public Processor1(ProcessorServiceScope serviceScope)
    {
        //...
    }
}

public class ProcessorN
{
    public ProcessorN(ProcessorServiceScope serviceScope)
    {
        //...
    }
}

It seems like a ServiceLocator, but it does not hide the depencies, so I think this is kind of ok.

like image 100
Christian Gollhardt Avatar answered Sep 26 '22 14:09

Christian Gollhardt


Consider the various interface methods listed:

Quote DoSomething_1(Quote quote);
Quote DoSomething_N(Quote quote);
Quote ProcessQuote(Quote quote = null)
Quote CreateQuote(Quote quote = null);

Apart from the names, they're all identical. Why make things so complicated? Considering the Reused Abstractions Principle, I'd argue that it'd be better if you had fewer abstractions, and more implementations.

So instead, introduce a single abstraction:

public interface IQuoteProcessor
{
    Quote ProcessQuote(Quote quote);
}

This is a nice abstraction because it's an endomorphism over Quote, which we know is composable. You can always create a Composite of an endomorphism:

public class CompositeQuoteProcessor : IQuoteProcessor
{
    private readonly IReadOnlyCollection<IQuoteProcessor> processors;

    public CompositeQuoteProcessor(params IQuoteProcessor[] processors)
    {
        this.processors = processors ?? throw new ArgumentNullException(nameof(processors));
    }

    public Quote ProcessQuote(Quote quote)
    {
        var q = quote;
        foreach (var p in processors)
            q = p.ProcessQuote(q);
        return q;
    }
}

At this point, you're essentially done, I should think. You can now compose various services (those called microservices in the OP). Here's a simple example:

var processor = new CompositeQuoteProcessor(new Service1(), new Service2());

Such composition should go in the application's Composition Root.

The various services can have dependencies of their own:

var processor =
    new CompositeQuoteProcessor(
        new Service3(
            new Foo()),
        new Service4());

You can even nest the Composites, if that's useful:

var processor =
    new CompositeQuoteProcessor(
        new CompositeQuoteProcessor(
            new Service1(),
            new Service2()),
        new CompositeQuoteProcessor(
            new Service3(
                new Foo()),
            new Service4()));

This nicely addresses the Constructor Over-injection code smell, because the CompositeQuoteProcessor class only has a single dependency. Since that single dependency is a collection, however, you can compose arbitrarily many other processors.

In this answer, I completely ignore Unity. Dependency Injection is a question of software design. If a DI Container can't easily compose a good design, you'd be better off with Pure DI, which I've implied here.


If you must use Unity, you can always create concrete classes that derive from CompositeQuoteProcessor and take Concrete Dependencies:

public class SomeQuoteProcessor1 : CompositeQuoteProcessor
{
    public SomeQuoteProcessor1(Service1 service1, Service3 service3) :
        base(service1, service3)
    {
    }
}

Unity should be able to auto-wire that class, then...

like image 32
Mark Seemann Avatar answered Sep 24 '22 14:09

Mark Seemann