Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The Unity MvcSiteMapProviderContainerExtension creates all instances at composition time #409

Closed
crebain opened this issue Sep 9, 2015 · 3 comments

Comments

@crebain
Copy link

crebain commented Sep 9, 2015

The UnityContainer.Resolve should not be used when registering types with dependencies. Using ResolvedParameter does the same thing without constructing all dependent instances at composition time.

@NightOwl888
Copy link
Collaborator

Could you provide an example?

Do note that the files are provided as code so you can edit the configuration however you like.

@crebain
Copy link
Author

crebain commented Sep 15, 2015

The fix is easy - replace all

new ResolvedArrayParameter<Interface> (this.Container.ResolveAll<Interface> ().ToArray ())

with

new ResolvedParameter<Interface[]> ()

or do not specify any injection parameters at all when there is a single constructor.
This will inject all named implementations for Interface as array elements.
Similarly, use InjectionFactory for types retrieved using a factory.

All of this allows registering additional elements to be used in the resolved array after the AddNewExtension call, and also allows replacing already registered ones with other implementations, as implementations are resolved all at once when resolving instead of at composition time.

Here is the diff:

diff --git a/Source/DI/Unity/ContainerExtensions/MvcSiteMapProviderContainerExtension.cs b/Source/DI/Unity/ContainerExtensions/MvcSiteMapProviderContainerExtension.cs
index 65c0bfa..e167663 100644
--- a/Source/DI/Unity/ContainerExtensions/MvcSiteMapProviderContainerExtension.cs
+++ b/Source/DI/Unity/ContainerExtensions/MvcSiteMapProviderContainerExtension.cs
@@ -1,6 +1,5 @@
 using System;
 using System.Collections.Generic;
-using System.Linq;
 using System.Reflection;
 using System.Web.Hosting;
 using System.Web.Mvc;
@@ -92,18 +91,18 @@ namespace DI.Unity.ContainerExtensions

             // Url Resolvers
             this.Container.RegisterType<ISiteMapNodeUrlResolverStrategy, SiteMapNodeUrlResolverStrategy> (new InjectionConstructor (
-                new ResolvedArrayParameter<ISiteMapNodeUrlResolver> (this.Container.ResolveAll<ISiteMapNodeUrlResolver> ().ToArray ())
+                new ResolvedParameter<ISiteMapNodeUrlResolver[]> ()
                 ));

             // Visibility Providers
             this.Container.RegisterType<ISiteMapNodeVisibilityProviderStrategy, SiteMapNodeVisibilityProviderStrategy> (new InjectionConstructor (
-                new ResolvedArrayParameter<ISiteMapNodeVisibilityProvider> (this.Container.ResolveAll<ISiteMapNodeVisibilityProvider> ().ToArray ()),
+                new ResolvedParameter<ISiteMapNodeVisibilityProvider[]> (),
                 new InjectionParameter<string> (string.Empty)
                 ));

             // Dynamic Node Providers
             this.Container.RegisterType<IDynamicNodeProviderStrategy, DynamicNodeProviderStrategy> (new InjectionConstructor (
-                new ResolvedArrayParameter<IDynamicNodeProvider> (this.Container.ResolveAll<IDynamicNodeProvider> ().ToArray ())
+                new ResolvedParameter<IDynamicNodeProvider[]> ()
                 ));


@@ -120,9 +119,7 @@ namespace DI.Unity.ContainerExtensions
             // IMPORTANT: Must give arrays of object a name in Unity in order for it to resolve them.
             this.Container.RegisterType<IAclModule, AuthorizeAttributeAclModule> ("authorizeAttribute");
             this.Container.RegisterType<IAclModule, XmlRolesAclModule> ("xmlRoles");
-            this.Container.RegisterType<IAclModule, CompositeAclModule> (new InjectionConstructor (new ResolvedArrayParameter<IAclModule> (
-                new ResolvedParameter<IAclModule> ("authorizeAttribute"),
-                new ResolvedParameter<IAclModule> ("xmlRoles"))));
+            this.Container.RegisterType<IAclModule, CompositeAclModule> (new InjectionConstructor (new ResolvedParameter<IAclModule[]> ()));



@@ -132,7 +129,7 @@ namespace DI.Unity.ContainerExtensions
             this.Container.RegisterInstance<System.Runtime.Caching.ObjectCache> (System.Runtime.Caching.MemoryCache.Default);
             this.Container.RegisterType (typeof (ICacheProvider<>), typeof (RuntimeCacheProvider<>));
             this.Container.RegisterType<ICacheDependency, RuntimeFileCacheDependency> (
-                "cacheDependency", new InjectionConstructor (absoluteFileName));
+                "cacheDependency", new InjectionConstructor (new InjectionParameter<string> (absoluteFileName)));

             this.Container.RegisterType<ICacheDetails, CacheDetails> ("cacheDetails",
                 new InjectionConstructor (absoluteCacheExpiration, TimeSpan.MinValue, new ResolvedParameter<ICacheDependency> ("cacheDependency")));
@@ -141,18 +138,16 @@ namespace DI.Unity.ContainerExtensions
             this.Container.RegisterType<ISiteMapNodeVisitor, UrlResolvingSiteMapNodeVisitor> ();

             // Prepare for the sitemap node providers
-            this.Container.RegisterType<IXmlSource, FileXmlSource> ("file1XmlSource", new InjectionConstructor (absoluteFileName));
+            this.Container.RegisterType<IXmlSource, FileXmlSource> ("file1XmlSource", new InjectionConstructor (new InjectionParameter<string> (absoluteFileName)));
             this.Container.RegisterType<IReservedAttributeNameProvider, ReservedAttributeNameProvider> (new InjectionConstructor (new List<string> ()));

             // IMPORTANT: Must give arrays of object a name in Unity in order for it to resolve them.
             // Register the sitemap node providers
-            this.Container.RegisterInstance<ISiteMapNodeProvider> ("xmlSiteMapNodeProvider1",
-                this.Container.Resolve<XmlSiteMapNodeProviderFactory> ().Create (this.Container.Resolve<IXmlSource> ("file1XmlSource")), new ContainerControlledLifetimeManager ());
-            this.Container.RegisterInstance<ISiteMapNodeProvider> ("reflectionSiteMapNodeProvider1",
-                this.Container.Resolve<ReflectionSiteMapNodeProviderFactory> ().Create (includeAssembliesForScan), new ContainerControlledLifetimeManager ());
-            this.Container.RegisterType<ISiteMapNodeProvider, CompositeSiteMapNodeProvider> (new InjectionConstructor (new ResolvedArrayParameter<ISiteMapNodeProvider> (
-                new ResolvedParameter<ISiteMapNodeProvider> ("xmlSiteMapNodeProvider1"),
-                new ResolvedParameter<ISiteMapNodeProvider> ("reflectionSiteMapNodeProvider1"))));
+            this.Container.RegisterType<ISiteMapNodeProvider> ("xmlSiteMapNodeProvider1", new ContainerControlledLifetimeManager (), new InjectionFactory (c =>
+                c.Resolve<XmlSiteMapNodeProviderFactory> ().Create (c.Resolve<IXmlSource> ("file1XmlSource"))));
+            this.Container.RegisterType<ISiteMapNodeProvider> ("reflectionSiteMapNodeProvider1", new ContainerControlledLifetimeManager (), new InjectionFactory (c =>
+                c.Resolve<ReflectionSiteMapNodeProviderFactory> ().Create (includeAssembliesForScan)));
+            this.Container.RegisterType<ISiteMapNodeProvider, CompositeSiteMapNodeProvider> (new InjectionConstructor (new ResolvedParameter<ISiteMapNodeProvider[]> ()));

             // Configure the builders
             this.Container.RegisterType<ISiteMapBuilder, SiteMapBuilder> ();
@@ -169,7 +164,7 @@ namespace DI.Unity.ContainerExtensions
                     new ResolvedParameter<ICacheDetails> ("cacheDetails")));

             this.Container.RegisterType<ISiteMapBuilderSetStrategy, SiteMapBuilderSetStrategy> (new InjectionConstructor (
-                new ResolvedArrayParameter<ISiteMapBuilderSet> (new ResolvedParameter<ISiteMapBuilderSet> ("builderSet1"))));
+                new ResolvedParameter<ISiteMapBuilderSet[]> ()));
             }
         }
     }

@NightOwl888
Copy link
Collaborator

I made the following changes to your solution because the only reason why XmlSiteMapProviderFactory and ReflectionSiteMapNodeProviderFactory exist is because I couldn't find a reasonable way to inject some dependencies into a constructor without having to resolve all of them (Unity and SimpleInjector). Your solution fixes this, so we just need to resolve the previously factory-created types with the container instead.

            // IMPORTANT: Must give arrays of object a name in Unity in order for it to resolve them.
            // Register the sitemap node providers
            this.Container.RegisterType<ISiteMapNodeProvider>(
                "xmlSiteMapNodeProvider1",
                new ContainerControlledLifetimeManager(),
                new InjectionFactory(c => c.Resolve<XmlSiteMapNodeProvider>(
                    new ParameterOverride("includeRootNode", true),
                    new ParameterOverride("useNestedDynamicNodeRecursion", false),
                    new DependencyOverride<IXmlSource>(c.Resolve<IXmlSource>("file1XmlSource"))
                    )));
            this.Container.RegisterType<ISiteMapNodeProvider>(
                "reflectionSiteMapNodeProvider1",
                new ContainerControlledLifetimeManager(),
                new InjectionFactory(c => c.Resolve<ReflectionSiteMapNodeProvider>(
                    new ParameterOverride("includeAssemblies", includeAssembliesForScan),
                    new ParameterOverride("excludeAssemblies", new string[0])
                    )));

I also left the following 2 blocks alone. The idea is that we want to hold the end user's hand getting through the process of creating multiple SiteMap instances, not necessairly to provide the minimal amount of configuration code.

But if there is a way to reduce the amount of code without giving up the easy-to-understand instance names, I welcome your suggestion.

            this.Container.RegisterType<ISiteMapNodeProvider, CompositeSiteMapNodeProvider>(new InjectionConstructor(new ResolvedArrayParameter<ISiteMapNodeProvider>(
                new ResolvedParameter<ISiteMapNodeProvider>("xmlSiteMapNodeProvider1"),
                new ResolvedParameter<ISiteMapNodeProvider>("reflectionSiteMapNodeProvider1"))));
        this.Container.RegisterType<ISiteMapBuilderSetStrategy, SiteMapBuilderSetStrategy>(new InjectionConstructor(
            new ResolvedArrayParameter<ISiteMapBuilderSet>(new ResolvedParameter<ISiteMapBuilderSet>("builderSet1"))));

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants