YairHalberstadt / stronginject

compile time dependency injection for .NET
MIT License
845 stars 24 forks source link

Provide the consumer type in factories #109

Open cabralRodrigo opened 3 years ago

cabralRodrigo commented 3 years ago

First of all, love the project. I've been really excited for source generators and this project just uses then so well.

Now, I'm trying to inject a logging interface in my application. The interface is:

public interface ILogger
{
    void Info(...);
    void Error(...);
}

And my implementation is:

public class Logger<T> : ILogger
{
    public Logger()
    {
        //uses T to get the actual logger from some library
    }

    public void Info(...);
    public void Error(...);
}

As you can see, the consumer type is used in the implementation but not in the interface. I know that default (at least for microsoft) is to use something like ILogger<T>, but all my projects uses ILogger (and I personally thinks that is better).

I don't know if currently is possible to use StrongInject to resolve this. One possible solution is to provide the consumer type (the type begin resolved) in the factory methods and in the IFactory interface.

I'm thinking something like this:

public interface ILogger { }

public class Logger<T> : ILogger { }

public class A { public A(ILogger logger) { } }

public class B { public B(C c) { } }

public class C { public C(ILogger logger) { } }

public partial class Container : IContainer<A>, IContainer<B>
{
    [Factory]
    private static ILogger Create<TConsumer>() => new Logger<TConsumer>();
}

var container = new Container();
container.Resolve<A>(); // Here TConsumer is 'A'
container.Resolve<B>() // Here TConsumer is 'C'

Or maybe something like this:

public interface ILogger { }

public class Logger : ILogger { public Logger(Type type) { } }

public class A { public A(ILogger logger) { } }

public class B { public B(C c) { } }

public class C { public C(ILogger logger) { } }

public partial class Container : IContainer<A>, IContainer<B>
{
    [Factory]
    private static ILogger Create(Type constumerType) => new Logger(constumerType);
}

var container = new Container();
container.Resolve<A>(); // Here the parameter 'constumerType' is 'A'
container.Resolve<B>() // Here is 'C'

I understand that this is probably a breaking change and if it's not something that you're not willing to do.

Thanks for the wonderful project 😄

YairHalberstadt commented 3 years ago

Hi

I can definitely see the use case here.

Currently in StrongInject there's no way to see how a type is going to be used when you resolve it in order to modify how it's resolved.

I don't know the best way to support this functionality. I think what I'll do is I'll mull over this for a few days, and see if I can think of anything elegant, and not overly complex. If I do, I'll post the design here for further discussion.

YairHalberstadt commented 3 years ago

Sorry it took me so long to get back to you - I've been distracted by other things and then forgot about this.

What I'm thinking of doing is adding a type:

namespace StrongInject
{
    public class ResolutionInfo
    {
        public Type ResolvedAsDependencyOf { get; }
    }
}

Then ONLY for InstancePerDependency dependencies you can have a parameter of type ResolutionInfo. If no registrations exist for that type, then StrongInject will create it with the correct type.

So you could do:

[Factory(Scope.InstancePerDependency)] ILogger CreateLogger(ResolutionInfo rInfo) => new Logger(rInfo.ResolvedAsDependencyOf);

WDYT?

trampster commented 3 years ago

For my usecase with Serilog I need to create a new logger from an existing one.

ILogger logger = _generalLogger.ForContext(typeof(parent));

So would look something like:

[Factory(Scope.InstancePerDependency)] ILogger CreateLogger(ResolutionInfo rInfo, ILogger logger) => logger.ForContext(typeof(rInfo.ResolvedAsDependencyOf));

But I worry that this would be circular.

My generic logger is currently created as follows

      [Factory(Scope.SingleInstance)]
      public static ILogger CreateLogger(ILogService logService)
      {
         return new LoggerConfiguration()
            .Enrich.FromLogContext()
            .WriteTo.File(Path.Combine(logService.GetLogFolder(), "log.txt"), fileSizeLimitBytes: 1024000, rollingInterval: RollingInterval.Day,
               outputTemplate: "{Timestamp:yyyy-MM-dd HH:mm:ss.fff zzz} [{Level}] ({SourceContext}) {Message}{NewLine}{Exception}", rollOnFileSizeLimit: true, hooks: new LoggingFileLifecycleCycleHooks())
            .WriteTo.AndroidLog()
            .MinimumLevel.Debug()
            .CreateLogger();
      }

I think I can solve this by creating a ILoggerResolver, which holds the generic ILogger, and then make a factory that looks like this:

[Factory(Scope.InstancePerDependency)] ILogger CreateLogger(ResolutionInfo rInfo, ILoggerResolver resolver) => resolver.GenericLogger.ForContext(typeof(rInfo.ResolvedAsDependencyOf));

So I think your design is good :+1:

YairHalberstadt commented 3 years ago

@trampster you could do that by making it a decorator instead.

[Decorator] ILogger CreateLogger(ResolutionInfo rInfo, ILogger logger) => logger.ForContext(typeof(rInfo.ResolvedAsDependencyOf));
AtomicBlom commented 3 years ago

This is exactly my use case as well, using a decorator seems perfectly reasonable.

YairHalberstadt commented 3 years ago

I think there's some further design questions that need to be answered.

If I have the following code:

public class A{ public (IB b){} }
public class B : IB { public B(ILog log){} }
public class Log : ILog { public Log(ResolutionInfo info){} }

What would you expect ResolutionInfo to contain? I presume typeof(B) or typeof(IB) (and not typeof(ILog)) but which one?

AtomicBlom commented 3 years ago

I would expect it to be typeof(B). In the case of logging with Serilog, you'd want the logger to be the concrete implementation, because when you're looking for the SourceContext to hunt down a bug you're going to want to know which concrete implementation exactly it was.

YairHalberstadt commented 3 years ago

Some further cases which I don't think are obvious what to do.

If I have:

public record A(ResolutionInfo info);
public record B(A[] as);

Should ResolutionInfo contain typeof(A[]) ortypeof(B). If we saytypeof(B)`, what about the following:

public record A(ResolutionInfo info);
public record B(List<A> as);

[Register(typeof(A)]
[Register(typeof(B))]
public class Container : IContainer<B>
{
    [Factory] List<T> CreateList<T>(T[] ts) => ts.ToArray();
}

By the same logic, ResolutionInfo ought to contain typeof(B) not typeof(List<A>), but there's no way for StrongInject to know that List<A> can be ignored as just a container type rather than the actual consumer.

YairHalberstadt commented 3 years ago

Note that you can work around this issue by using Microsoft's ILogger<T> interface as suggested at https://github.com/YairHalberstadt/stronginject/issues/117#issuecomment-854337288, but I understand that can be a hurdle to change to.

AtomicBlom commented 3 years ago

Sorry, I saw this and got stuck on an answer for it and forgot to come back to it.

It's a really good question that I don't have an answer to because I've never actually encountered a situation where it was a problem.

There are a couple of things that I would note that may make this harder or simpler, I'm not sure, and that's the case that I've never needed the concept that ResolutionInfo provides directly inside a dependancy. In general, I've always tried to keep my code unaware of the container as possible unless it's doing nested resolution, and as such I personally would only need it to be available for [Factory] methods (and probably decorators)

I honestly wish I had a better answer, but I'm afraid I don't.