unitycontainer / microsoft-dependency-injection

Unity.Microsoft.DependencyInjection package
Apache License 2.0
63 stars 36 forks source link

Resolution of IdentityServerOptions fails with latest version #15

Closed pksorensen closed 5 years ago

pksorensen commented 6 years ago

The following test fails with latest version

       [TestMethod]
        public void Test13()
        {
            var c = new UnityContainer();

            var sc = new ServiceCollection();
            sc.AddIdentityServer(o =>
            {
                o.Events.RaiseErrorEvents = true;
            });

            var sp = sc.BuildServiceProvider();

            var test =  sp.GetService<IdentityServerOptions>();

            Assert.IsNotNull(test); //Passes

            var sp1 = sc.BuildServiceProvider(c);

            var test1 = sp1.GetService<IdentityServerOptions>();

            Assert.IsNotNull(test1); //fails

        }

the debugger spits this out:

Exception thrown: 'System.InvalidOperationException' in Unity.Container.dll
Exception thrown: 'System.InvalidOperationException' in Unity.Container.dll
Exception thrown: 'System.Reflection.TargetInvocationException' in mscorlib.dll
Exception thrown: 'System.Reflection.TargetInvocationException' in Unity.Container.dll
Exception thrown: 'System.InvalidOperationException' in Unity.Container.dll
Exception thrown: 'System.InvalidOperationException' in Unity.Container.dll
Exception thrown: 'Unity.Exceptions.ResolutionFailedException' in Unity.Container.dll
Exception thrown: 'System.InvalidOperationException' in Microsoft.Extensions.DependencyInjection.Abstractions.dll
Exception thrown: 'Unity.Exceptions.ResolutionFailedException' in Unity.Container.dll

Is it possible to add extension/logging or something that would allow me to inspect those resolution failed exceptions with unity ? trying to figure out what changed atm.

pksorensen commented 6 years ago

Here is a test that can be added to this project.

        [Fact]
        public void Test13()
        {
            var c = new UnityContainer();

            var sc = new ServiceCollection();
            sc.AddIdentityServer(o =>
            {
                o.Events.RaiseErrorEvents = true;
            });

            var sp = ServiceCollectionContainerBuilderExtensions.BuildServiceProvider(sc);

            var test = sp.GetService<IdentityServerOptions>();

            Assert.NotNull(test); //Passes

            var sp1 = sc.BuildServiceProvider(c);

            var test1 = sp1.GetService<IdentityServerOptions>();

            Assert.NotNull(test1);

        }

Maybe its worth adding logging output to

 public object GetService(Type serviceType)
        {
            try
            {
                return _container.Resolve(serviceType);
            }
            catch(Exception ex)  {

                /* Ignore */
            }

            return null;
        }

to output when ex is thrown as it would allow not to set up this project local to debugg.

Trying to find more information but looks like something broke with IOptions

Resolution of the dependency failed, type = 'Microsoft.Extensions.Options.IOptions`1[IdentityServer4.Configuration.IdentityServerOptions]', name = '(none)'.
Exception occurred while: while resolving.
Exception is: InvalidOperationException - The current type, Microsoft.Extensions.Options.IPostConfigureOptions`1[IdentityServer4.Configuration.IdentityServerOptions], is an interface and cannot be constructed. Are you missing a type mapping?
-----------------------------------------------
At the time of the exception, the container was: 
  Resolving Microsoft.Extensions.Options.OptionsManager`1[IdentityServer4.Configuration.IdentityServerOptions],(none) (mapped from Microsoft.Extensions.Options.IOptions`1[IdentityServer4.Configuration.IdentityServerOptions], (none))
  Resolving parameter 'factory' of constructor Microsoft.Extensions.Options.OptionsManager`1[[IdentityServer4.Configuration.IdentityServerOptions, IdentityServer4, Version=2.1.3.0, Culture=neutral, PublicKeyToken=null]](Microsoft.Extensions.Options.IOptionsFactory`1[[IdentityServer4.Configuration.IdentityServerOptions, IdentityServer4, Version=2.1.3.0, Culture=neutral, PublicKeyToken=null]] factory)
    Resolving Microsoft.Extensions.Options.OptionsFactory`1[IdentityServer4.Configuration.IdentityServerOptions],(none) (mapped from Microsoft.Extensions.Options.IOptionsFactory`1[IdentityServer4.Configuration.IdentityServerOptions], (none))
    Resolving parameter 'postConfigures' of constructor Microsoft.Extensions.Options.OptionsFactory`1[[IdentityServer4.Configuration.IdentityServerOptions, IdentityServer4, Version=2.1.3.0, Culture=neutral, PublicKeyToken=null]](System.Collections.Generic.IEnumerable`1[[Microsoft.Extensions.Options.IConfigureOptions`1[[IdentityServer4.Configuration.IdentityServerOptions, IdentityServer4, Version=2.1.3.0, Culture=neutral, PublicKeyToken=null]], Microsoft.Extensions.Options, Version=2.0.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60]] setups, System.Collections.Generic.IEnumerable`1[[Microsoft.Extensions.Options.IPostConfigureOptions`1[[IdentityServer4.Configuration.IdentityServerOptions, IdentityServer4, Version=2.1.3.0, Culture=neutral, PublicKeyToken=null]], Microsoft.Extensions.Options, Version=2.0.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60]] postConfigures)
      Resolving System.Collections.Generic.IEnumerable`1[Microsoft.Extensions.Options.IPostConfigureOptions`1[IdentityServer4.Configuration.IdentityServerOptions]],(none)
        Resolving Microsoft.Extensions.Options.IPostConfigureOptions`1[IdentityServer4.Configuration.IdentityServerOptions],(none)

Looking for a solution

pksorensen commented 6 years ago

@ENikS I think that some of the enumerable changes you recent commmits could be the issue

pksorensen commented 6 years ago

I think that if a IEnumeral is requested and no registrations of T is made, then it should return an empty Enumerable ?

pksorensen commented 6 years ago

I am at looking into:

internal static void ResolveGenericEnumerable<T>(IBuilderContext context, Type type)
        {
            var set = new MiniHashSet<InternalRegistration>();
            var container = (UnityContainer)context.Container;
            GetExplicitRegistrations(container, typeof(T), set);
            GetExplicitRegistrations(container, type, set);

            try
            {
                context.Existing = set.Select(registration =>
                {
                    return (T)((BuilderContext)context).NewBuildUp(typeof(T), registration.Name);
                }
              )
                                      .ToList();
            }catch(Exception ex)
            {

                throw;
            }

            context.BuildComplete = true;
        }

(added a few lines to debug)

but i dont understand why

    GetExplicitRegistrations(container, type, set);

has to be called.

In this example.

typeof(T).Name
"IPostConfigureOptions`1"

Its resolving a generic interface IPostConfigureOptions<IdentityServerOptions> where type then is IdentityServerOptions

So basically to return a enumerable of IPostConfigureOptions it is also building up all registrations of IdentityServerOptions ? is that the intended behavior

pksorensen commented 6 years ago

btw to run the test, the following dependency is also needed.

<PackageReference Include="IdentityServer4" Version="2.1.3" />
pksorensen commented 6 years ago

So, think i found the problem.

image

In the image the debugger shows that the GetFinalType is returning the argument IdentityServerOptions because its registered which is wrong.

Trying to figure out if GetFinalType should be rewritten, but I am coming to the conclussion that _isTypeExplicitlyRegistered should have returned true since i would expect it return on this instead if (_isTypeExplicitlyRegistered(definition)) return definition;

pksorensen commented 6 years ago

I hacky fix that solves my test and also keeps existing tests to work would be to add to EnumerableResolveStrategy such it ignores the final type .

public override void PreBuildUp(IBuilderContext context) { var plan = context.Registration.Get(); if (plan == null) { var typeArgument = context.BuildKey.Type.GetTypeInfo().GenericTypeArguments.First();

            var type = ((UnityContainer) context.Container).GetFinalType(typeArgument);
            if (typeArgument.GetTypeInfo().IsGenericType)
            {
                type= typeArgument.GetTypeInfo().GetGenericTypeDefinition();
            }

            if (type != typeArgument)
            {
                var method = _resolveGenericMethod.MakeGenericMethod(typeArgument)
                                                  .CreateDelegate(typeof(ResolveGenericEnumerable));

                plan = new DynamicMethodBuildPlan(c => method.DynamicInvoke(c, type));
            }
            else
            {
                plan = new DynamicMethodBuildPlan((DynamicBuildPlanMethod) 
                    _resolveMethod.MakeGenericMethod(typeArgument)
                                  .CreateDelegate(typeof(DynamicBuildPlanMethod)));
            }

            context.Registration.Set(typeof(IBuildPlanPolicy), plan);
        }

        plan.BuildUp(context);
        context.BuildComplete = true;
    }

I am in over my head finding a good solution for this, hoping that the stuff i found do help to get this resolved.

pksorensen commented 6 years ago

Lets continue here @ENikS

But I think I know what needs to be done now.

Cool thing is that we could add it as a extension here when we know the full list of interfaces from aspnet core that needs to be build in.

First item is: Microsoft.Extensions.Options.IPostConfigureOptions<>

ENikS commented 6 years ago

We could create a factory for the Options as part of extension. Looks like you know what to do now :)

pksorensen commented 6 years ago

Atleast I know the idea, i will go over your examples and see if I can figure it out.

Question is if we want to pull in all the additional dependencies or we want to have

Unity.Microsoft.DependencyInjection.Configuration Unity.Microsoft.DependencyInjection.Package1 Unity.Microsoft.DependencyInjection.Package2

that matches future packages in aspnet core

I am in favor of adding a extension point for developers to hook in the types that should be build in because when new packages comes they might also have types that needs to be build in.

I will try come up with a prototype and get back with the results here.

ENikS commented 6 years ago

Sounds good

pksorensen commented 6 years ago

@ENikS I think we need to take a step back and look at the original problem. Please take a moment to reflect over the following before you dismiss it :)

First - i did have a go at what we discussed above - but realized that its now the users of 3th party stuff that needs to know what to tell unity that it should use as build in types. This causes alot of trial and error and not just a builder.UseUnityContainer() that one would expect.

I am hoping we can take a look at the original problem and see if we can change something here that works for both worlds.

Here is an example of a GetFinalType that dont use a hashset and makes all tests passes:

[SuppressMessage("ReSharper", "InconsistentlySynchronizedField")]
        internal Type GetFinalType(Type argType)
        {
            Type next;
            for (var type = argType; null != type; type = next)
            {
                var info = type.GetTypeInfo();

                if (type.IsArray)
                {
                    next = type.GetElementType();
                    if (_isTypeExplicitlyRegistered(next)) return next;
                }
                else if (info.IsGenericType)
                {
                    var definition = info.GetGenericTypeDefinition();

                    if (definition == typeof(Lazy<>) ||
                         definition == typeof(IEnumerable<>) ||
                         definition == typeof(Func<>))
                    {
                        if (_isTypeExplicitlyRegistered(type)) return type;

                        if (_isTypeExplicitlyRegistered(definition)) return definition;

                        next = info.GenericTypeArguments[0];
                        if (_isTypeExplicitlyRegistered(next)) return next;

                    }
                    else
                    {
                        return definition;
                    }
                }

                else
                {
                    return type;
                }
            }

            return argType;
        }

The reason why I keep coming back to this method is that I do think the current implementation is faulty or builds on a bad assumption.

I agree that nested IEnumerable<Lazy<Fun<>>> should work - but I dont know if this should be for every generic interface like the current implementation.

Would we consider changing the spec to not have tests for Func? Or should it then have tests for Func<>, Func<,>, Func<,,>, Func<,,,> and so on?

Maybe custom build plans should be used to bring in the additional generics that should be included in this nested resolution.

Moving on, GetFinalType proposed above works with current tests.

I will try to add a small example to the web directory that illustrates the problem and hope we might be able to continue a dialog about this maybe needs fixing in Unity.Container rather then the in the consuming project.

pksorensen commented 6 years ago

I created the minimal example of issue to conclude on above:

https://github.com/pksorensen/examples/tree/issues/nestedEnumerable

Notice the service registrations

            services.Configure<MyCoolOptions>(o =>
            {
                o.Value = "This should be displayed";
            });

            services.AddSingleton<MyCoolOptions>(
                resolver => resolver.GetRequiredService<IOptions<MyCoolOptions>>().Value);

when navigating to /api/values you will get a

InvalidOperationException: No service for type 'WebApplication1.MyCoolOptions' has been registered. error.

Thats because the exception is absorbed here

  public object GetService(Type serviceType)
        {
            try
            {
                return _container.Resolve(serviceType);
            }
            catch  { /* Ignore */}

            return null;
        }

in the serviceprovider.

Which will be identical to the problem described above.

It will all be fixed if Enumerable returned an empty enumerable when T is not registered.

Problem is that the new behavior for IEnumerable<Microsoft.Extensions.Options.IPostConfigureOptions> will be faulty due to ResolveGenericEnumerable will try to resolve IEnumerable (this cant even be cast to IEnumerable<Microsoft.Extensions.Options.IPostConfigureOptions<MyCoolOptions>>) since GetFinalType will return a typeof MyCoolOptions since its registered in the service types.

So i honestly believe this is faulty/bug and not something we should resolve in the web application itself.

pksorensen commented 6 years ago

@ENikS Here is a minimal branch with updated unit test to show the issue aswell

https://github.com/pksorensen/container/tree/issues/nestedenumerables

And sorry if I am doing something stupid with the branches, but I am not that skilled with the submodule in git and how to do pull requests to those.

ENikS commented 6 years ago

You trying to convince me to change GetFinalType and for the most part I agree with you. The problem is, this behavior is carried over from version 2.+ and if we change it A LOT of code will stop working. I know it for a fact, I deal with it at work. What I released is a compromise between ability to chain generics and overriding how it is enumerated when you need to. Currently user has complete control over how types are enumerated.

Lets try to formulate the issue here. What is it exactly you don't like in present solution?

ENikS commented 6 years ago

I am not that familiar with asp.net Reading about Options pattern and I think unity extension has to be fixed to include these...

pksorensen commented 6 years ago

Would it be possible for you to look at the unit test that I just did her https://github.com/pksorensen/container/tree/issues/nestedenumerables so we both understand the issue. I think it shows the problem very well atleast.

The problem at its core is something to do with it trying to resolve IEnumerable<CustomGeneric> where the current implementation will blow up if T is also registered with the container.

Resolve IEnumerable<CustomGeneric> should return an empty enumerable when no registrations of CustomGeneric is made.

ENikS commented 6 years ago

This test?

        [Fact]
        public void Test1()
        {
            var services = new ServiceCollection();

            services.AddOptions();

            services.Configure<MyCoolOptions>(o =>
            {
                o.Value = "This should be displayed";
            });

            services.AddSingleton<MyCoolOptions>(
                resolver => resolver.GetRequiredService<IOptions<MyCoolOptions>>().Value);

            var sp = CreateServiceProvider(services);
            var a = sp.GetRequiredService<MyCoolOptions>();
        }
ENikS commented 6 years ago

Let me take a closer look at what is going on.

pksorensen commented 6 years ago

yes,

and also change the service provider to throw on exceptions to get it out.

 public object GetService(Type serviceType)
        {
            try
            {
                return _container.Resolve(serviceType);
            }
            catch  { throw; }

            return null;
        }

and you need this dependency also <PackageReference Include="Microsoft.Extensions.DependencyInjection" Version="2.0.0" />

ENikS commented 6 years ago

I usually run it with local build so I could step into Unity code.

pksorensen commented 6 years ago

I do the same now that you showed me to clone the submodules properly. But you should be able to step into the ResolveGenericEnumerable and see how it get a bad type argument

pksorensen commented 6 years ago

If we cant find out that there is something wrong and dont want to change, then the following extension point can be used

using Microsoft.Extensions.Options;
using System;
using System.Collections.Generic;
using System.Linq;
using System.Reflection;
using System.Text;
using System.Threading.Tasks;
using Unity.Builder;
using Unity.Builder.Strategy;
using Unity.Extension;
using Unity.ObjectBuilder.BuildPlan.DynamicMethod;
using Unity.Policy;
using Unity.Registration;

namespace Unity.Microsoft.DependencyInjection.Options
{
    public class IOptionsExtension<T> : UnityContainerExtension where T : class
    {
        protected override void Initialize()
        {
            // Register policy

            // Note name of the registration! It tells Unity that this policy 
            // applies to ALL resolutions of the type regardless of requested name.
            // In other words it creates 'Built-In' registration similar to Lazy or IEnumerable.

            //This did not work.
            Context.Policies.Set(typeof(IEnumerable<IPostConfigureOptions<T>>), string.Empty, typeof(IBuildPlanCreatorPolicy), new IOptionsBuildPlanCreatorPolicy(Context.Policies));

            //Lets use a stategy instead
            Context.Strategies.Add(new AspNetCoreBuilderStrategy<T>(),
             UnityBuildStage.Setup);

        }
    }
    //
    public class AspNetCoreBuilderStrategy<T> : BuilderStrategy where T : class
    {
        delegate void ResolveGenericEnumerable(IBuilderContext context, Type type);
        private MethodInfo _resolveGenericMethod = typeof(UnityContainer).GetTypeInfo().GetDeclaredMethod("ResolveGenericEnumerable");
        private MethodInfo _resolveMethod = typeof(UnityContainer).GetTypeInfo().GetDeclaredMethod("ResolveEnumerable");

        public override void PreBuildUp(IBuilderContext context)
        {
            //  Guard.ArgumentNotNull(context, "context");

            var plan = context.Registration.Get<IBuildPlanPolicy>();
            if (plan == null)
            {
                var typeArgument = context.BuildKey.Type.GetTypeInfo().GenericTypeArguments.First();

                plan = new DynamicMethodBuildPlan((DynamicBuildPlanMethod)
                         _resolveMethod.MakeGenericMethod(typeArgument)
                                       .CreateDelegate(typeof(DynamicBuildPlanMethod)));

                context.Registration.Set(typeof(IBuildPlanPolicy), plan);
            }

            plan.BuildUp(context);
            context.BuildComplete = true;

        }

        public override bool RequiredToBuildType(IUnityContainer container, INamedType namedType, params InjectionMember[] injectionMembers)
        {
            return namedType is InternalRegistration registration &&
                   registration.Type.GetTypeInfo().IsGenericType &&
                   typeof(IEnumerable<>) == registration.Type.GetGenericTypeDefinition()
                   && registration.Type.GetGenericArguments()[0] == typeof(IPostConfigureOptions<T>);
        }

    }
    public class IOptionsBuildPlanCreatorPolicy : IBuildPlanCreatorPolicy
    {
        delegate void ResolveGenericEnumerable(IBuilderContext context, Type type);

        private IPolicyList policies;

        private MethodInfo _resolveGenericMethod = typeof(UnityContainer).GetTypeInfo().GetDeclaredMethod("ResolveGenericEnumerable");

        public IOptionsBuildPlanCreatorPolicy(IPolicyList policies)
        {
            this.policies = policies;
        }

        public IBuildPlanPolicy CreatePlan(IBuilderContext context, INamedType buildKey)
        {
            var typeArgument = context.BuildKey.Type.GetTypeInfo().GenericTypeArguments.First();

            var method = _resolveGenericMethod.MakeGenericMethod(typeArgument)
                                              .CreateDelegate(typeof(ResolveGenericEnumerable));

            return new DynamicMethodBuildPlan(c => method.DynamicInvoke(c, typeArgument));

        }

    }
}

Then in startup of aspnet core apps the extension can be added:

        public void ConfigureContainer(IUnityContainer container)
        {
            container.AddNewExtension<IOptionsExtension<MyCoolOptions>>();

            container.RegisterInstance("This string is displayed if container configured correctly", 
                                       "This string is displayed if container configured correctly");
        }

or the unittest

[Fact]
        public void Test1()
        {
            var container = new UnityContainer();
            container.AddNewExtension<IOptionsExtension<MyCoolOptions>>();

            var services = new ServiceCollection();
            services.AddOptions();

            services.Configure<MyCoolOptions>(o =>
            {
                o.Value = "This should be displayed";
            });

            services.AddSingleton<MyCoolOptions>(
                resolver => resolver.GetRequiredService<IOptions<MyCoolOptions>>().Value);

            var sp = services.BuildServiceProvider(container);

            var a = sp.GetRequiredService<MyCoolOptions>();

        }

We could add the IOptionsExtension to Unity.Microsoft.DependencyInjection.

But this requires the developers to know which options to add extensions for. This can be hard because you as a developer might not know if a dependency uses something that needs to be registered.

I would consider this a last resort if you end up at a point where you dont think there is anything wrong with the current implementation - its really hard for me to judge if my purposal of GetFinalType will break stuff for a lot of other people.

ENikS commented 6 years ago

Yes, you are right. It looks like a new NuGet package Unity.Microsoft.Options. Since it is independent extension, I need to think about how to integrate it info DependencyInjection as well as standalone Unity.

pksorensen commented 6 years ago

At the same time we dont want to end up having to create packages Unity.. for everything. That puts alot of extra work on this project.

Would be a cleaner approach if unity was able to work in pair with aspnet core dependency injection.

Were you able to step though ResolveGenericEnumerable and see how GetFinalType returns a bad type of the unit test i provided?

The part that ends up returning a bad type is this:

                        next = info.GenericTypeArguments[0];
                        if (_isTypeExplicitlyRegistered(next)) return next;

so I would look at if GetFinalType could be altered before going about creating extra packages.

ENikS commented 6 years ago

This one is worth supporting

ENikS commented 6 years ago

I am working on v6 engine and all this magic will be lost there. It is better to create supporting package.

pksorensen commented 6 years ago

Super, I will go with my own enumerable resolution strategy for now to unblock myself and try to contribute in what way I can for getting unity and aspnet core to play nice together.

pksorensen commented 6 years ago

I think I made something thats usefull here:

public class IOptionsExtension : UnityContainerExtension
    {
        protected override void Initialize()
        { 

            Context.Policies.Set(typeof(OptionsFactory<>), string.Empty, typeof(IBuildPlanCreatorPolicy), new OptionsFactoryBuilderPlanCreatorPolicy(Context.Policies));

        }

        public class OptionsFactoryBuilderPlanCreatorPolicy : IBuildPlanCreatorPolicy
        {
            private readonly IPolicyList _policies;

            private readonly MethodInfo _factoryMethod =
                typeof(OptionsFactoryBuilderPlanCreatorPolicy).GetTypeInfo().GetDeclaredMethod(nameof(FactoryMethod));

            /// <summary>
            /// Factory plan to build [I]Foo type
            /// </summary>
            /// <param name="policies">Container policy list to store created plans</param>
            public OptionsFactoryBuilderPlanCreatorPolicy(IPolicyList policies)
            {
                _policies = policies;
            }

            public IBuildPlanPolicy CreatePlan(IBuilderContext context, INamedType buildKey)
            {
                // Make generic factory method for the type
                var typeToBuild = buildKey.Type.GetTypeInfo().GenericTypeArguments;
                var factoryMethod =
                    _factoryMethod.MakeGenericMethod(typeToBuild)
                                  .CreateDelegate(typeof(DynamicBuildPlanMethod));
                // Create policy
                var creatorPlan = new DynamicMethodBuildPlan((DynamicBuildPlanMethod)factoryMethod);

                // Register BuildPlan policy with the container to optimize performance
                _policies.Set(buildKey.Type, string.Empty, typeof(IBuildPlanPolicy), creatorPlan);

                return creatorPlan;
            }

            private static void FactoryMethod<TResult>(IBuilderContext context) where TResult : class, new()
            {

                var p1 = new DynamicMethodBuildPlan((DynamicBuildPlanMethod)
                             _resolveMethod.MakeGenericMethod(typeof(IConfigureOptions<TResult>))
                                           .CreateDelegate(typeof(DynamicBuildPlanMethod)));

                var a1 = p1.ExecuteBuildUp(context);

                var p2 = new DynamicMethodBuildPlan((DynamicBuildPlanMethod)
                           _resolveMethod.MakeGenericMethod(typeof(IPostConfigureOptions<TResult>))
                                         .CreateDelegate(typeof(DynamicBuildPlanMethod)));

                var a2 = p2.ExecuteBuildUp(context);

                context.Existing = new OptionsFactory<TResult>(a1 as IEnumerable<IConfigureOptions<TResult>>, a2 as IEnumerable<IPostConfigureOptions<TResult>>);
            }

            private static MethodInfo _resolveMethod = typeof(UnityContainer).GetTypeInfo().GetDeclaredMethod("ResolveEnumerable");
        }
    }

It creats a plan for the OptionsFactory<> in aspnet core options, which is the proplametic type.

Are there any way to cache or make this more efficient in the factory metory. caching p1/p2 ect?

ENikS commented 6 years ago

This line caches it so next enumerated item will be resolved by the method you've created

// Register BuildPlan policy with the container to optimize performance
 _policies.Set(buildKey.Type, string.Empty, typeof(IBuildPlanPolicy), creatorPlan);
pksorensen commented 6 years ago

ye, but the two dynamic child plans, p1 and p2- used to create the actual new OptionsFactory.

You said hashsets was slow, so was just wondering. Normally i would have put the two plans p1 and p2 into a dictioanry based on their typeof TResult to get them next time instead of creating them again

ENikS commented 6 years ago

Your factory method seems to be too complicated. Why don't you do your p1 and p2 in the CreatePlan?

ENikS commented 6 years ago

Do you understand what it does?

pksorensen commented 6 years ago

Not exactly.

I got stuck here:

public class OptionsFactoryBuilderPlanCreatorPolicy : IBuildPlanCreatorPolicy
        {
            private readonly IPolicyList _policies;

            private readonly MethodInfo _factoryMethod =
                typeof(OptionsFactoryBuilderPlanCreatorPolicy).GetTypeInfo().GetDeclaredMethod(nameof(FactoryMethod));

            /// <summary>
            /// Factory plan to build [I]Foo type
            /// </summary>
            /// <param name="policies">Container policy list to store created plans</param>
            public OptionsFactoryBuilderPlanCreatorPolicy(IPolicyList policies)
            {
                _policies = policies;
            }

            public IBuildPlanPolicy CreatePlan(IBuilderContext context, INamedType buildKey)
            {
                // Make generic factory method for the type
                var typeToBuild = buildKey.Type.GetTypeInfo().GenericTypeArguments;
                var factoryMethod =
                    _factoryMethod.MakeGenericMethod(typeToBuild)
                                  .CreateDelegate(typeof(DynamicBuildPlanMethod));
                // Create policy
                var creatorPlan = new DynamicMethodBuildPlan((DynamicBuildPlanMethod)factoryMethod);

                // Register BuildPlan policy with the container to optimize performance
                _policies.Set(buildKey.Type, string.Empty, typeof(IBuildPlanPolicy), creatorPlan);

                var t1 = typeof(IConfigureOptions<>).MakeGenericType(typeToBuild);
                CreateChildPlan(t1);
                var t2 = typeof(IPostConfigureOptions<>).MakeGenericType(typeToBuild);
                CreateChildPlan(t2);

                return creatorPlan;
            }

            private void CreateChildPlan(Type t1)
            {
                var et1 = typeof(IEnumerable<>).MakeGenericType(t1);
                var p1 = new DynamicMethodBuildPlan((DynamicBuildPlanMethod)
                           _resolveMethod.MakeGenericMethod(t1)
                                         .CreateDelegate(typeof(DynamicBuildPlanMethod)));

                _policies.Set(et1, string.Empty, typeof(IBuildPlanPolicy), p1);
            }

            private static void FactoryMethod<TResult>(IBuilderContext context) where TResult : class, new()
            {

                var p1 = new DynamicMethodBuildPlan((DynamicBuildPlanMethod)
                             _resolveMethod.MakeGenericMethod(typeof(IConfigureOptions<TResult>))
                                           .CreateDelegate(typeof(DynamicBuildPlanMethod)));

                var a1 = p1.ExecuteBuildUp(context);

                var p2 = new DynamicMethodBuildPlan((DynamicBuildPlanMethod)
                           _resolveMethod.MakeGenericMethod(typeof(IPostConfigureOptions<TResult>))
                                         .CreateDelegate(typeof(DynamicBuildPlanMethod)));

                var a2 = p2.ExecuteBuildUp(context);

                context.Existing = new OptionsFactory<TResult>(a1 as IEnumerable<IConfigureOptions<TResult>>, a2 as IEnumerable<IPostConfigureOptions<TResult>>);
            }

            private static MethodInfo _resolveMethod = typeof(UnityContainer).GetTypeInfo().GetDeclaredMethod("ResolveEnumerable");
        }

Could not figure out how to wrap it all up

ENikS commented 6 years ago

Got to run now, will continue tomorrow...

pksorensen commented 6 years ago

:) okay thanks.

rlmartins76 commented 6 years ago

I am having this same issue. Any progress on a resolution?

ENikS commented 6 years ago

Been busy lately. Perhaps @pksorensen could give you temporary workaround?

pksorensen commented 6 years ago

Let me try backtrack to what i ended up using. Will update by tomorrow with a answer of my work around.

pksorensen commented 6 years ago

I am using this currently:

using Microsoft.Extensions.Options;
using System;
using System.Collections.Generic;
using System.Reflection;
using Unity;
using Unity.Builder;
using Unity.Extension;
using Unity.Lifetime;
using Unity.ObjectBuilder.BuildPlan.DynamicMethod;
using Unity.Policy;

namespace SInnovations.Unity.AspNetCore
{
    public class IOptionsExtension : UnityContainerExtension
    {
        protected override void Initialize()
        {

            Context.Policies.Set(typeof(OptionsFactory<>), string.Empty, typeof(IBuildPlanCreatorPolicy), new OptionsFactoryBuilderPlanCreatorPolicy(Context.Policies));

        }
        public ILifetimeContainer Lifetime => Context.Lifetime;

        public class OptionsFactoryBuilderPlanCreatorPolicy : IBuildPlanCreatorPolicy
        {
            private readonly IPolicyList _policies;

            private readonly MethodInfo _factoryMethod =
                typeof(OptionsFactoryBuilderPlanCreatorPolicy).GetTypeInfo().GetDeclaredMethod(nameof(FactoryMethod));

            /// <summary>
            /// Factory plan to build [I]Foo type
            /// </summary>
            /// <param name="policies">Container policy list to store created plans</param>
            public OptionsFactoryBuilderPlanCreatorPolicy(IPolicyList policies)
            {
                _policies = policies;
            }

            public IBuildPlanPolicy CreatePlan(IBuilderContext context, INamedType buildKey)
            {
                // Make generic factory method for the type
                var typeToBuild = buildKey.Type.GetTypeInfo().GenericTypeArguments;
                var factoryMethod =
                    _factoryMethod.MakeGenericMethod(typeToBuild)
                                  .CreateDelegate(typeof(DynamicBuildPlanMethod));
                // Create policy
                var creatorPlan = new DynamicMethodBuildPlan((DynamicBuildPlanMethod)factoryMethod);

                // Register BuildPlan policy with the container to optimize performance
                _policies.Set(buildKey.Type, string.Empty, typeof(IBuildPlanPolicy), creatorPlan);

                //var t1 = typeof(IConfigureOptions<>).MakeGenericType(typeToBuild);
                //CreateChildPlan(t1);
                //var t2 = typeof(IPostConfigureOptions<>).MakeGenericType(typeToBuild);
                //CreateChildPlan(t2);

                return creatorPlan;
            }

            private void CreateChildPlan(Type t1)
            {
                var et1 = typeof(IEnumerable<>).MakeGenericType(t1);
                var p1 = new DynamicMethodBuildPlan((DynamicBuildPlanMethod)
                           _resolveMethod.MakeGenericMethod(t1)
                                         .CreateDelegate(typeof(DynamicBuildPlanMethod)));

                _policies.Set(et1, string.Empty, typeof(IBuildPlanPolicy), p1);
            }

            private static void FactoryMethod<TResult>(IBuilderContext context) where TResult : class, new()
            {

                var p1 = new DynamicMethodBuildPlan((DynamicBuildPlanMethod)
                             _resolveMethod.MakeGenericMethod(typeof(IConfigureOptions<TResult>))
                                           .CreateDelegate(typeof(DynamicBuildPlanMethod)));

                var a1 = p1.ExecuteBuildUp(context);

                var p2 = new DynamicMethodBuildPlan((DynamicBuildPlanMethod)
                           _resolveMethod.MakeGenericMethod(typeof(IPostConfigureOptions<TResult>))
                                         .CreateDelegate(typeof(DynamicBuildPlanMethod)));

                var a2 = p2.ExecuteBuildUp(context);

                context.Existing = new OptionsFactory<TResult>(a1 as IEnumerable<IConfigureOptions<TResult>>, a2 as IEnumerable<IPostConfigureOptions<TResult>>);
            }

            private static MethodInfo _resolveMethod = typeof(UnityContainer).GetTypeInfo().GetDeclaredMethod("ResolveEnumerable");
        }
    }
}

and

        /// <summary>
        /// Add AspNet Core Options support to the container
        /// </summary>
        /// <param name="container"></param>
        /// <returns></returns>
        public static IUnityContainer AddOptions(this IUnityContainer container)
        {
#if ASPNETCORE1
            return container.RegisterType(typeof(IOptions<>), typeof(OptionsManager<>))
                            .RegisterType(typeof(IOptionsSnapshot<>), typeof(OptionsManager<>), new HierarchicalLifetimeManager())
                            .RegisterType(typeof(IOptionsMonitor<>), typeof(OptionsMonitor<>), new ContainerControlledLifetimeManager());
#endif

#if NETSTANDARD2_0
            var ext = container.AddExtension(new IOptionsExtension()).Configure<IOptionsExtension>();

            return container.RegisterType(typeof(IOptions<>), typeof(OptionsManager<>), new InjectionSingletonLifetimeManager(ext.Lifetime))
                   .RegisterType(typeof(IOptionsSnapshot<>), typeof(OptionsManager<>), new HierarchicalLifetimeManager())
                   .RegisterType(typeof(IOptionsMonitor<>), typeof(OptionsMonitor<>), new InjectionSingletonLifetimeManager(ext.Lifetime))
                   .RegisterType(typeof(IOptionsFactory<>), typeof(OptionsFactory<>), new TransientLifetimeManager())
                   .RegisterType(typeof(IOptionsMonitorCache<>), typeof(OptionsCache<>), new InjectionSingletonLifetimeManager(ext.Lifetime));
#endif

        }

So i can on my new UnityContainer().AddOptions() If you dont need Options support outside the apsnet core scope of using unity, then its sufficient to add the IOptionsExtension to your container

ENikS commented 5 years ago

Now Unity supports Options.