5

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?

Improve this question
4
  • 1
    My opinion is that this has code smell all over it. It also looks like an XY problem. The current design decisions should be reviewed. Commented Aug 21, 2018 at 2:15
  • Since you are referring to Mark Seemann's blog, I would advise you to read chapter 6.1 of Dependency Injection in .NET, second edition that talks extensively about fixing the constructor injection code smell. Commented Aug 21, 2018 at 10:13
  • 1
    And please don't 'abuse' the term micro service. A micro service is autonomous application that runs in isolation. Your micro services both run in the same process and mutate the same entity, both characteristics that are very different from a micro service. Commented Aug 21, 2018 at 10:16
  • The fact that you are getting a stack overflow exception is an indication that you have a cyclic dependency. Most DI Containers have checks for this and give a meaningful exception instead of throwing a stack trace. Commented Aug 21, 2018 at 10:20

4 Answers 4

Reset to default
6

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.

Improve this answer
Sign up to request clarification or add additional context in comments.

13 Comments

We mock all the services during unit tests, which means that I need to be able to mock GetItems . That would require writing another "mocked" version of it and then keep them in sync. This may lead to errors if they are out of sync. The original implementation above does not have that flaw.
Couldn't your unit test look at the GetItemsDefault and GetItemsMocked method, and do some checks based on the unique quantity of object types returned? Alternativly you could have a Service1Factory which can create a mocked and a default version. @KonstantinKonstantinov
I feel that there is substantial design issue with using IEnumerable. Mocked code does not get mixed with production code at our place, so I can’t have GetItemsMocked implemented by a production level factory. Writing unit / integration tests when some or even all objects are substituted by mocks becomes a nightmare: what if I want to mock 5 out of 10+ objects in some tests and some other 3 out of these 10+ in some other tests? That means that I would have to create different factories for each test setup and then update all of them when the rules change. @ChristianGollhardt
If they are different Tests, they should be different created. Either inline or through a different Factory. @KonstantinKonstantinov
I have about 100 tests only for this "quote" object (and we have about 10K tests for the whole system). The "quote" test are scattered among 15-20 test classes. IEnumerable factory approach means that there will be at lest these 15-20 factories, which must be kept in sync when the rules change. And they do. This will be a maintenance nightmare. And the compiler won't help to figure out what should be changed :(
|
3

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...

Improve this answer

10 Comments

Dear @MarkSeemann Thanks, a lot for your answer. It is a great idea, but, as you wrote it completely ignores Unity. Let's see what happens if we bring it in: There will be real usages of CompositeQuoteProcessor, unit test(s), integration test(s), etc... All of them must have identically constructed calls with some or all classes mocked. That becomes a maintenance nightmare because the compiler is not there to help us figure out where to make changes if, for example, we add an extra service. Therefore, I am afraid that a param array in the constructor will do more harm than good :(
@KonstantinKonstantinov See edit. It seems to me, however, like Unity is preventing you from making progress, so I'd recommend that you remove it in favour of Pure DI.
That's what I already have, except the "top" level processors are declared with interfaces, so that I can mock anything I need during tests: public SomeQuoteProcessor1(IService1 service1, …, IServiceN serviceN). Everything works, but the number of “rules”, or services is large. This is a natural business requirement and most of the rules are independent. Sure, I can apply aggregate services. However, because the rules are independent bundling them together is not justified. What I want is a linearized version of Unity controlled, compiler validated, and mockable structure.
@KonstantinKonstantinov Considering something like SomeQuoteProcessor1 above, why do you feel that you need to unit test it? It has no behaviour that CompositeQuoteProcessor doesn't have. You can easily unit test CompositeQuoteProcessor, if you feel that you need to. That's only a few test cases that you need to write once.
@KonstantinKonstantinov It's possible that there's something that I'm missing here, so I apologise if my advice isn't useful. My advice, based on what I understand, is what's in my current answer. When ignoring Unity, you can address the Constructor Over-injection smell. If you add Unity to the mix, then yes, I can see that what I suggest will require you to add dozens of Concrete Dependencies to such classes. It's a workaround that only exists to satiate Unity. It's entirely declarative; it's configuration as code. Is it important, then, how many dependencies it has?
|
0

Unity supports property injection. Instead of passing all those values in to the constructor just have public setters available with the [Dependency] attribute. This allows you to add as many injections as you need without having to update the constructor.

public class QuoteCreateService : QuoteProcessor, IQuoteCreateService
{
    [Dependency]
    protected IService_1 Service_1 { get; public set; }
    // ...
    [Dependency]
    protected IService_N Service_N; { get; public set; }

    public override QuoteUpdaterList QuotePipeline =>
        new QuoteUpdaterList
        {
            Service_1.DoSomething_1,
            // ...
            Service_N.DoSomething_N
        };

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

7 Comments

Sorry, I should've mentioned that in the question. Unity crashes with StackoverflowException if [Dependency] attribute is used for that structure.
@KonstantinKonstantinov Could you include a copy of the exception with a stack trace in your original question?
There is no stack trace for StackoverflowException because it cannot be caught anymore :(
You should get a stack trace in the debugger when it is thrown. Make sure you have break when exceptions thrown turned on and "Just My Code" turned off.
I downvoted this answer because the use of property injection does not solve the underlying design problem, which is that QuoteCreateService violates the Single Responsibility Principle.
|
0

I never thought that I would answer my own question, though a substantial part of the credit should go to https://softwareengineering.stackexchange.com/users/115084/john-wu - he was the one who had my mind set in a proper direction.

Nevertheless, nearly two year have passed since the time when I asked the question and while I built the solution to the question slightly after asking it (and thanks to everyone who replied), it took more than a year for most of the developers in the company that I work for to actually understand how does it work and what does it do (and yes, they all are well above average developers and yes, the code is written in pure C# with no external libraries). So, I think that it could be important for others who might have similar business scenarios.

As mentioned in the question, the root of our problem is that the parameter space that we are dealing with is too large. We have about 6-8 values of what we call workflow (call it W), about 30-40 values of what we call a state config (call it S) – this is a combination of US state code and two other parameters, though not all triples are possible (the actual content of what is that state config is irrelevant), and about 30-50 values of what we call a risk rule (call it R) - that value depends on the product but this is also irrelevant as different products are treated differently.

So, the total dimension of parameter space is N = W * S * R and it is around 10K (and I am not much concerned about a precise value). Which means that when the code runs, we need approximately the following: for each workflow (obviously only one is running at a time but all of them do run at some time) and each state config (again only one is running at a time but any of them could run at some time) we need to evaluate all risk rules, which are relevant for that workflow and that state config.

Well, if the dimension of parameter space is around some N, then the number of tests needed to cover the whole space is at least on the order of that N. And this is exactly what the legacy code and tests were trying to do and what resulted in the question. The answer turned out to be in a pure math, rather than in a pure computer science and it is based on what is called separable spaces: https://en.wikipedia.org/wiki/Separable_space and what in the group theory terms is called irreducible representation: https://en.wikipedia.org/wiki/Irreducible_representation . Though I have to admit that the latter one was more like an inspiration rather than the actual application of the group theory.

If you already lost me, that’s fine. Just, please, read the math mentioned above before proceeding further.

The space separability here means that we can choose such a space N so that subspaces W, S, and R become independent (or separable). To the best of my understanding, this can always be done for finite spaces that we are dealing with in CS.

This means that we can describe N space as e.g. S lists (or sets) of some rules whereas each rule is applicable in some of W workflows by assigning a set of applicable workflows to each rule. And yes, if we have some bad rules that originally should be applied in some weird combinations of workflows and state configs then we can split them into more than one rule, which would then allow maintaining separability.

This, of course, can be generalized, but I will skip the details as they are irrelevant.

At this point, someone may wonder, what’s the point. Well, if we can split N dimensional space (and N is about 10K in our case) into independent subspaces, then the magic happens and instead of writing on the order of N = W *S * R tests to cover the whole parameter space we only need to write on the order of W + S + R tests to cover the whole parameter space. In our case the difference is about 100X.

But that’s still not all. As we can describe the subspaces in the notions of sets or lists (depending on the needs) that naturally brings us to the notion of useless tests.

Wait, did I just say useless tests? Yes, I did. Let me explain. A typical TDD paradigm is that if the code failed, then the first thing that we need to do is to create a test, which would’ve caught that bug. Well, if the code is described by a static list or set (== list or set that was hard coded in the code) and the test would be described by an identity transformation from that list/set, then this makes such a test useless as it would have to repeat the original list/set…

The state configs form a historical pattern, e.g., let say, that we had some set of rules for the state of CA some time in 2018. That set of rules might be slightly changed to some other set of rules in 2019 and into some set of rules in 2020. These changes are small: a set of rule might pick up or lose a few rules and/or the rule might be tweaked a little bit, e.g. if we are comparing some value to be above some threshold, then the value of that threshold might be changed at some point for some state config. And once the rule or collection of rules is changed, then it should stay as it is until it changed again. Meanwhile some other rules could be changed, and every such change requires introduction of what we call state config. So, for each US state we have ordered collection (list) of these state configs and for each state config we have a collection of rules. Most of the rules don’t change but some of them do sporadically change as described. A natural IOC approach is to register each rule collection and each rule for each state config with IOC container, e.g. Unity using a combination of unique “name” of the state config and name of rule / collection (we actually run more than one collection of rules during workflow), whereas each rule already has a collection of workflows where it should be applicable. Then when the code runs for a given state config and a given workflow we can pull the collection out of Unity. A collection then contains the names of the rules that should be run. Then combining the name of the rule with the name of state config we can pull the actual rule out of Unity, filter the collection to leave only the rules that are applicable for a given workflow and then apply all the rules. What happens here is that rule names / collection names form some closed sets and they benefit greatly by describing them that way. We obviously don’t want to register each rule / collection for each state config by hands as that would be tedious and error prone. So we use what we call “normalizers”. Let’s say that we have a general rule – that’s a rule that is the same for all state config. Then we register it by name only and the normalizer will “automatically” register it for all state configs. The same goes with the historic versioning. Once we register a rule / collection with Unity by rule / collection name + state config, then the normalizer will fill in the gap until we change the rule at some later state config.

As a result, each rule becomes extremely simple. Most of them have either zero or one injected constructor parameter, a few of them have two, and I know only one rule that has three injected parameters. As rules are independent and very simple, the tests for rules become very simple as well.

We do have some ideas to make the core of whatever I wrote above open source, provided that it could bring some value to the community...

Improve this answer

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.