dadhi / DryIoc

DryIoc is fast, small, full-featured IoC Container for .NET
MIT License
988 stars 122 forks source link

WithDependencyInjectionAdapter / Populate do not work with Default IfAlreadyRegistered.Replace #520

Closed kirides closed 1 year ago

kirides commented 1 year ago

IOptions-Pattern does not work properly with the DependencyInjectionAdapter/IContainer.Populate as "multiple registrations but different implementations" are not appended.

I propose to update RegisterDescriptor(this IContainer container, ServiceDescriptor descriptor) to set ifAlreadyRegistered to IfAlreadyRegistered.AppendNotKeyed to make it work despite having a weird combination of IContainer and IServiceCollection

Code to reproduce the error:

using DryIoc;
using DryIoc.Microsoft.DependencyInjection;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Options;
using System.Net.Http;

void Main()
{
    var services = new ServiceCollection();

    services.AddHttpClient(Options.DefaultName)
    .ConfigurePrimaryHttpMessageHandler(c =>
    {
        Console.WriteLine("Never called, as it was replaced");
        return new HttpClientHandler();
    });

    services.AddHttpClient<TypedClient>()
    .ConfigurePrimaryHttpMessageHandler(() =>
    {
        Console.WriteLine("TypedClient called");
        return new HttpClientHandler();
    });

    var container = new Container(r =>
        r.WithDefaultIfAlreadyRegistered(IfAlreadyRegistered.Replace))
    .WithDependencyInjectionAdapter(services);

    var factory = container.Resolve<IHttpClientFactory>();

    using var client = factory.CreateClient();
}

public class TypedClient { }
dadhi commented 1 year ago

@kirides Taking a note. Will look on the weekend.

dadhi commented 1 year ago

@kirides Hi.

As I understood, you want the ability to pass IfAlreadyRegistered into Populate, so you may control it in RegisterDescriptor?

If so, did you try to specify the optional registerDescriptor in WithDependencyInjectionAdapter, e.g. like this:

    var container = new Container(r =>
        r.WithDefaultIfAlreadyRegistered(IfAlreadyRegistered.Replace))
    .WithDependencyInjectionAdapter(services, registerDescriptor: RegisterTuned);

   bool RegisterTuned(IRegistrator r, ServiceDecriptor s) 
   {
       // - check descriptor and return false if its not the desired service -> fallback to the normal registration
       // - register service manually with registrator and IfAlreadyRegistered flag and return true to indicate the custom registration
   }
kirides commented 1 year ago

As I understood, you want the ability to pass IfAlreadyRegistered into Populate

it's more that the current "adapter" doesn't properly "map" ServiceCollection-Implementation into the DryIoc container, by not setting "AppendNotKeyed" and instead using the containers default.

I see why this might be the case, but Microsoft.*.Extensions depend on the mentioned behaviour and break if we use "Replace" and not manually add the descriptors

What you mentioned is our current workaround, but we have to add it to every of our services for them to properly work, as we set our container to default to Throw when multiple registrations occur

dadhi commented 1 year ago

@kirides Ok, let's assume that WithDependencyInjectionAdapter will have the optional parameter IfAlreadyRegistered ifAlreadyRegistered = IfAlreadyRegistered.AppendNotKeyed and the register descriptor will use it inside Would it be sufficient?

kirides commented 1 year ago

Probably yes. But then, why doesn't it add the services in the "compatible" way anyways? I mean, sure that would be a breaking change, but current code is broken anyways when using any WithDefaultIfAlreadyRegistered other than AppendNotKeyed

I just looked up the code for UnityContainer extensions and it does something akin to AppentNotKeyed

internal static IUnityContainer AddServices(this IUnityContainer container, IServiceCollection services)
{
    ILifetimeContainer lifetime = ((UnityContainer)container).Configure<MdiExtension>().Lifetime;
    Func<Type, string, InternalRegistration, IPolicySet> registerFunc = ((UnityContainer)container).Register;

        // ----- THIS LINE HERE
    ((UnityContainer)container).Register = ((UnityContainer)container).AppendNew;
    foreach (ServiceDescriptor descriptor in services)
    {
        container.Register(descriptor, lifetime);
    }
    ((UnityContainer)container).Register = registerFunc;
    return container;
}
dadhi commented 1 year ago

@kirides So be it, I will set the service collection registrations explicitly with AppendNoKeyed. I may even simplify and speed up some things.

dadhi commented 1 year ago

@kirides Could you look at https://github.com/dadhi/DryIoc/commit/72bcf2065754b09629872754028e791f075fcd9d and verify the output in comment is wgat you'd expect?

kirides commented 1 year ago

@dadhi Works just as intended.

Our custom http client handlers are now properly resolved.

dadhi commented 1 year ago

DI.MS.DI v6.1.0 is out on NuGet