4

I would like to modify my current LetterFactory implementation and remove the call to Activator.CreateInstance with a call to the container to resolve the current Letter fully initialized with constructor injection. I have read the docs here and here, and even this SO Post while penning this post, but nothing seems to click.

Notes:

1) IDocumentServicesCore is an Aggregate.

2) All Letters are decorated with the LetterTypeAttribute (hundreds of them)

3) This LetterFactory itself is registered in the container.

public class LetterFactory : ILetterFactory
{
    private readonly IDocumentServicesCore _documentServicesCore;

    public LetterFactory(IDocumentServicesCore documentServicesCore)
    {
        _documentServicesCore = documentServicesCore;
    }

    public LetterBase Create(int letterId)
    {
        if (letterId <= 0)
        {
            throw new ArgumentOutOfRangeException(nameof(letterId));
        }

        List<Type> types = typeof(LetterBase).Assembly.GetTypes()
            .Where(t => !t.IsAbstract && t.IsSubclassOf(typeof(LetterBase)))
            .ToList();

        LetterBase letter = null;
        foreach(Type type in types)
        {
            LetterTypeAttribute attribute = type.GetCustomAttributes<LetterTypeAttribute>().First();

            if (!attribute.LetterId.Contains(letterId))
            {
                continue;
            }

            letter = Activator.CreateInstance(type, _documentServicesCore) as LetterBase;
            break;
        }

        if (letter != null)
        {
            return letter;
        }

        string message = $"Could not find a LetterBase to create for id {letterId}.";
        throw new NotSupportedException(message);
    }
}

Update1

The problems seems to start with the fact that the letters themselves aren't registered, how to I take the LINQ code that collects the Letters from the assembly and register those enmass?

Thank you, Stephen

Stephen Patten
  • 6,333
  • 10
  • 50
  • 84
  • 1
    `Activator.CreateInstance` is useful in certain situations, and this one looks like a good fit to me. I would change a few things round though. For example, but the `types` list into a dictionary indexed by letter ID and make it static so you don't have to keep recreating it. – DavidG May 14 '18 at 12:19
  • How do you use this factory? Do you create a lot of different `LetterBase` instances per class or only one per class? – arekzyla May 14 '18 at 12:28
  • @arekzyla The factory will produce a single letter per request keyed by LetterId (ASP.NET) – Stephen Patten May 14 '18 at 12:40
  • I don't see a point in the LetterFactory if you're using Autofac. Autofac can just create the letter you need based on the Id without an intermediate (and duplicate) class instantiator. You pretty much just need to scan your instances and give them a [named or keyed service](http://autofaccn.readthedocs.io/en/latest/advanced/keyed-services.html) and then use the Id to resolved the named instance via autofac. If you need an example of your code using this let me know, but it should be straight forward. – Erik Philips May 14 '18 at 17:01
  • 1
    Hi @ErikPhilips I agree with you, this code is one step away from being completely hosted in the container, but finding out how to solve the scanning was one issue, the other was the delegate signature. By all means more sample code would be beneficial if you don't mind. Thank you -Stephen – Stephen Patten May 14 '18 at 18:15

2 Answers2

4

You are looking for IIndex<TKey, TValue> which is a kind of dictionary and it can be composed so IIndex<Int32, Func<LetterBase>> is the type you want.

With such a type your LetterFactory will look like this :

public class LetterFactory : ILetterFactory
{
    private readonly IIndex<Int32, Func<LetterBase>> _lettersFactory; 
    public LetterFactory(IIndex<Int32, Func<LetterBase>> lettersFactory)
    {
        _lettersFactory = lettersFactory;
    }

    public LetterBase Create(int letterId)
    {
        if (letterId <= 0)
        {
            throw new ArgumentOutOfRangeException(nameof(letterId));
        }

        Func<LetterBase> letterFactory = null; 
        if(!this._lettersFactory.tryGetValue(letterId, out letterFactory))
        {
            string message = $"Could not find a LetterBase to create for id {letterId}.";
            throw new NotSupportedException(message);        
        }

        Letter letter = letterFactory(); 
        return letter; 
    }
}

And then you have to register your types like this :

List<Type> letterTypes = typeof(LetterBase).Assembly.GetTypes()
    .Where(t => !t.IsAbstract && t.IsSubclassOf(typeof(LetterBase)))
    .ToList();

foreach(Type letterType in letterTypes)
{
    LetterTypeAttribute attribute = type.GetCustomAttributes<LetterTypeAttribute>()
                                        .First();

    builder.RegisterType(letterType)
           .Keyed<LetterBase>(attribute.LetterId);
}

You will also improve performance with this code : the heavy assembly scanning will only happen once at startup and not for each call.

By the way, be aware of assembly scanning limitation in IIS hosted application : http://autofaccn.readthedocs.io/en/latest/register/scanning.html#iis-hosted-web-applications

You can also rely directly on IIndex<Int32, LetterBase> instead of IIndex<Int32, Func<LetterBase>> it depends on your scope strategy.

Cyril Durand
  • 15,834
  • 5
  • 54
  • 62
  • I don't see the point to have an intermediate factory class. If the class is registered, whatever class (constructor) requires the `LetterBase` could simply depend on the class via constructor with either the KeyFilter attribute or WithAttributeFiltering attribute and let Autofac resolve it. – Erik Philips May 14 '18 at 17:08
  • 1
    `KeyFilter` or `WithAttributeFiltering` works only if the `letterId` value is known at build time and doesn't work when the value is known at runtime which seems to be the case. – Cyril Durand May 14 '18 at 17:13
  • You're correct. I do like your code though, it's very nice and clean and well read. – Erik Philips May 14 '18 at 18:27
  • @CyrilDurand The code works correctly in a unit test, but when hosted in ASP.MVC5 AND running the multi-tenat container, it fails to find any of the letters from the Index. This is somewhat harder to debug, so I'm going to try to write a test around it. Have you ever heard of any issues with using the Index with the MT container? Thank you Cyril – Stephen Patten May 16 '18 at 16:23
  • 1
    I don't know how you configure your multi tenant container. Could you add a breakpoint and check if your components are registered? Maybe the assembly are not loaded yet and types can't be found. Have a look at the IOS link I mentioned. It may help finding the problem. – Cyril Durand May 16 '18 at 20:01
2

You made me do real work, good job :) The following is my solution.

Autofac - Named and Keyed Services - Resolving with Index

using System;
using System.Collections.Generic;
using System.Linq;
using Autofac;
using Autofac.Features.Indexed;


public class Program
{
    private static IContainer _Container;

    public static void Main()
    {
        InitDependencyInjection();  

        var rd1 = _Container.Resolve<RequiresDependency>(new NamedParameter("letterId", 1));
        rd1.PrintType();

        var rd2 = _Container.Resolve<RequiresDependency>(new NamedParameter("letterId", 2));
        rd2.PrintType();
    }

    private static void InitDependencyInjection()
    {
        var builder = new ContainerBuilder();

        var letterTypes = typeof(LetterBase).Assembly.GetTypes()
            // Find all types that derice from LetterBase
            .Where(t => !t.IsAbstract && t.IsSubclassOf(typeof(LetterBase)))
            // Make sure they are decorated by attribute
            .Where(t => 
              t.GetCustomAttributes(typeof(LetterTypeAttribute), false).Length == 1)
            .ToList();

        //Register with Autofac, Keyed by LetterId
        //This should throw an exception if any are duplicated
        //You may want to consider using an enum instead
        //It's not hard to convert an Int to Enum
        foreach(Type letterType in letterTypes)
        {
            // we already tested the type has the attribute above
            var attribute = letterType
              .GetCustomAttributes(typeof(LetterTypeAttribute)
                , false)[0] as LetterTypeAttribute;

            builder.RegisterType(letterType)
                .Keyed<LetterBase>(attribute.LetterId);
        }

        builder.RegisterType<RequiresDependency>();

        _Container = builder.Build();
    }

}

public class RequiresDependency
{
    private readonly LetterBase _letter;

    //Autofac automagically provides a factory that returns type
    //type you need via indexer
    public RequiresDependency(int letterId, IIndex<int, LetterBase> letterFactory)
    {
        //resolve the needed type based on the index value passed in
        _letter = letterFactory[letterId];
    }

    public void PrintType()
    {
        Console.WriteLine(_letter.GetType().Name);
    }
}

public abstract class LetterBase
{
}

[LetterType(1)]
public class LetterA : LetterBase
{}

[LetterType(2)]
public class LetterB : LetterBase
{}

// make sure the classes using this attribute has only a single attribute
[AttributeUsage(AttributeTargets.Class, AllowMultiple = false)]
public class LetterTypeAttribute : Attribute
{
    public LetterTypeAttribute(int letterId)
    {
        LetterId = letterId;
    }

    public int LetterId { get; private set; }
}

DotNetFiddle Example

Result:

LetterA

LetterB

Erik Philips
  • 53,428
  • 11
  • 128
  • 150
  • Erik, thank you for taking the time to demonstrate a second "working' version. I'm going to switch my vote to Cyril's answer since he was actually the first one with the correct concept IIndex, and his code was a straight drop-in functionally. Don't get me wrong, I still had to make changes to his code to suit my needs, but conceptually it is complete. Cheers Stephen – Stephen Patten May 15 '18 at 12:33
  • I'm coming around to your way of thinking re: the removal of my Factory.. either way I'm stuck now since the IINdex doesn't seem to work in a multi-tenant container – Stephen Patten May 17 '18 at 18:49
  • You could always `IIndex>`. – Erik Philips May 18 '18 at 04:07