search

maartenba / MvcSiteMapProvider

An ASP.NET MVC SiteMapProvider implementation for the ASP.NET MVC framework.
Microsoft Public License
538 stars 218 forks source link

The Unity MvcSiteMapProviderContainerExtension creates all instances at composition time #409

Closed crebain closed 8 years ago

crebain commented 9 years ago

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 commented 9 years ago

Could you provide an example?

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

crebain commented 9 years ago

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 commented 8 years ago

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"))));