Merge pull request 7534 from hotfix/v4.4.7 into master

FiftyOne.Pipeline.Core/FlowElements/PipelineBuilder.cs

@@ -81,6 +81,13 @@ public PipelineBuilder(ILoggerFactory loggerFactory,
             IServiceProvider services)
             : this(loggerFactory)
         {
+            // If we're using a FiftyOneServiceProvider and it does not have an ILoggerFactory
+            // instance then add one. This is to provide backward compatibility.
+            if (services is FiftyOneServiceProvider fiftyOneProvider &&
+                fiftyOneProvider.GetService(typeof(ILoggerFactory)) == null)
+            {
+                fiftyOneProvider.AddService(loggerFactory);
+            }
             _services = services;
         }
 
@@ -247,8 +254,7 @@ private void AddElementToList(
             }
 
             object builderInstance = null;
-            if (_services != null &&
-                _services.GetType().Equals(typeof(FiftyOneServiceProvider)) == false)
+            if (_services != null)
             {
                 // Try to get a a builder instance from the service collection.
                 builderInstance = _services.GetRequiredService(builderType);
@@ -436,75 +442,6 @@ private void AddParallelElementsToList(
             elements.Add(parallelInstance);
         }
 
-        /// <summary>
-        /// Get the services required for the constructor, and call it with them.
-        /// </summary>
-        /// <param name="constructor">
-        /// The constructor to call.
-        /// </param>
-        /// <returns>
-        /// Instance returned by the constructor.
-        /// </returns>
-        private object CallConstructorWithServicesForAssemblies(
-            ConstructorInfo constructor)
-        {
-            ParameterInfo[] parameters = constructor.GetParameters();
-            object[] services = new object[parameters.Length];
-            for (int i = 0; i < parameters.Length; i++)
-            {
-                if (parameters[i].ParameterType.Equals(typeof(ILoggerFactory)))
-                {
-                    services[i] = LoggerFactory;
-                }
-                else
-                {
-                    services[i] = _services.GetService(parameters[i].ParameterType);
-                }
-            }
-            return Activator.CreateInstance(constructor.DeclaringType, services);
-        }
-
-
-        /// <summary>
-        /// Get the best constructor for the list of constructors. Best meaning
-        /// the constructor with the most parameters which can be fulfilled.
-        /// </summary>
-        /// <param name="constructors">
-        /// Constructors to get the best of.
-        /// </param>
-        /// <returns>
-        /// Best constructor or null if none have parameters that can be
-        /// fulfilled.
-        /// </returns>
-        private ConstructorInfo GetBestConstructorForAssemblies(
-            IEnumerable<ConstructorInfo> constructors)
-        {
-            ConstructorInfo bestConstructor = null;
-            foreach (var constructor in constructors)
-            {
-                if (bestConstructor == null ||
-                    constructor.GetParameters().Length >
-                    bestConstructor.GetParameters().Length)
-                {
-                    var hasServices = true;
-                    foreach (var param in constructor.GetParameters())
-                    {
-                        if (param.ParameterType.Equals(typeof(ILoggerFactory)) == false &&
-                            _services.GetService(param.ParameterType) == null)
-                        {
-                            hasServices = false;
-                            break;
-                        }
-                    }
-                    if (hasServices == true)
-                    {
-                        bestConstructor = constructor;
-                    }
-                }
-            }
-            return bestConstructor;
-        }
-
         /// <summary>
         /// Instantiate a new builder instance from the assemblies which are
         /// currently loaded.
@@ -521,29 +458,17 @@ private object GetBuilderFromAssemlies(Type builderType)
             var loggerConstructors = builderType.GetConstructors()
                 .Where(c => c.GetParameters().Length == 1 &&
                 c.GetParameters()[0].ParameterType == typeof(ILoggerFactory));
-            var serviceConstructors = builderType.GetConstructors()
-                .Where(c => c.GetParameters().Length > 1 &&
-                c.GetParameters().All(p => p.ParameterType.Equals(typeof(ILoggerFactory)) ||
-                (_services != null &&
-                _services.GetService(p.ParameterType) != null)));
 
             if (defaultConstructors.Any() == false &&
-                loggerConstructors.Any() == false &&
-                serviceConstructors.Any() == false)
+                loggerConstructors.Any() == false)
             {
                 return null;
             }
 
             // Create the builder instance using the constructor with a logger
             // factory, or the default constructor if one taking a logger
             // factory is not available.
-            if (serviceConstructors.Any() &&
-                GetBestConstructorForAssemblies(serviceConstructors) != null)
-            {
-                return CallConstructorWithServicesForAssemblies(
-                    GetBestConstructorForAssemblies(serviceConstructors));
-            }
-            else if (loggerConstructors.Any())
+            if (loggerConstructors.Any())
             {
                 return Activator.CreateInstance(builderType, LoggerFactory);
             }

FiftyOne.Pipeline.Core/Services/FiftyOneServiceProvider.cs

@@ -22,6 +22,8 @@
 
 using System;
 using System.Collections.Generic;
+using System.Linq;
+using System.Reflection;
 using System.Text;
 
 namespace FiftyOne.Pipeline.Core.Services
@@ -50,8 +52,9 @@ public void AddService(object service)
 
 
         /// <summary>
-        /// Get the service from the service collection if it exists, otherwise
-        /// return null.
+        /// Get the service from the service collection if it exists.
+        /// If it does not exist, but we can create a new instance, then do so.
+        /// If we cannot create a new instance, return null.
         /// Note that if more than one instance implementing the same service
         /// is added to the services, the first will be returned.
         /// </summary>
@@ -72,8 +75,63 @@ public object GetService(Type serviceType)
                         return service;
                     }
                 }
+                // We don't have the requested service.
+                // Do we have the services to create a new instance?
+                return CreateService(serviceType);
             }
             return null;
         }
+
+        private object CreateService(Type serviceType)
+        {
+            object result = null;
+            var constructor = GetConstructor(serviceType);
+            if(constructor != null)
+            {
+                result = CreateInstance(constructor);
+            }
+            return result;
+        }
+
+        /// <summary>
+        /// Get the services required for the constructor, and call it with them.
+        /// </summary>
+        /// <param name="constructor">
+        /// The constructor to call.
+        /// </param>
+        /// <returns>
+        /// Instance returned by the constructor.
+        /// </returns>
+        private object CreateInstance(ConstructorInfo constructor)
+        {
+            ParameterInfo[] parameters = constructor.GetParameters();
+            object[] services = new object[parameters.Length];
+            for (int i = 0; i < parameters.Length; i++)
+            {
+                services[i] = GetService(parameters[i].ParameterType);
+            }
+            return Activator.CreateInstance(constructor.DeclaringType, services);
+        }
+
+
+        /// <summary>
+        /// Get the best constructor for the list of constructors. Best meaning
+        /// the constructor with the most parameters which can be fulfilled.
+        /// </summary>
+        /// <param name="requiredType">
+        /// The type we want a constructor for
+        /// </param>
+        /// <returns>
+        /// Best constructor or null if none have parameters that can be
+        /// fulfilled.
+        /// </returns>
+        private ConstructorInfo GetConstructor(Type requiredType)
+        {
+            var constructors = requiredType.GetConstructors()
+                .OrderByDescending(c => c.GetParameters().Length)
+                .Where(c => c.GetParameters().All(p => GetService(p.ParameterType) != null));
+
+            return constructors.FirstOrDefault();
+        }
     }
 }

Tests/FiftyOne.Pipeline.Core.Tests/FlowElements/PipelineBuilderTests.cs

@@ -748,7 +748,9 @@ public void PipelineBuilder_BuildFromConfiguration_AssemblyServices_NotAvailable
                 .BuildFromConfiguration(opts);
 
             Assert.IsNotNull(pipeline.GetElement<RequiredServiceElement>().LoggerFactory);
-            Assert.IsNull(pipeline.GetElement<RequiredServiceElement>().Service);
+            // The service is not available in the service provider, but it can be created
+            // by the service provider.
+            Assert.IsNotNull(pipeline.GetElement<RequiredServiceElement>().Service);
             Assert.IsNull(pipeline.GetElement<RequiredServiceElement>().UpdateService);
         }
 

Tests/FiftyOne.Pipeline.Core.Tests/Services/FiftyOneServiceProviderTests.cs

@@ -0,0 +1,137 @@
+using FiftyOne.Pipeline.Core.Services;
+using Microsoft.VisualStudio.TestTools.UnitTesting;
+using System;
+using System.Collections.Generic;
+using System.Text;
+
+namespace FiftyOne.Pipeline.Core.Tests.Services
+{
+    [TestClass]
+    public class FiftyOneServiceProviderTests
+    {
+        /// <summary>
+        /// Empty interface used for testing FiftyOneServiceProvider
+        /// </summary>
+        public interface ITestService
+        {
+        }
+
+        /// <summary>
+        /// Empty class used for testing FiftyOneServiceProvider
+        /// </summary>
+        public class TestService : ITestService
+        {
+        }
+
+        /// <summary>
+        /// Class used for testing FiftyOneServiceProvider.
+        /// Takes a TestService as a constructor parameter.
+        /// </summary>
+        public class HighLevelService
+        {
+            public TestService TestService { get; set; }
+
+            public HighLevelService(TestService testService)            
+            {
+                TestService = testService;
+            }
+        }
+
+        /// <summary>
+        /// Class used for testing FiftyOneServiceProvider.
+        /// Takes an ITestService as a constructor parameter.
+        /// </summary>
+        public class HighLevelServiceUsingInterface
+        {
+            public ITestService TestService { get; set; }
+
+            public HighLevelServiceUsingInterface(ITestService testService)
+            {
+                TestService = testService;
+            }
+        }
+
+        /// <summary>
+        /// Verify that the provider will return the expected service instance.
+        /// </summary>
+        [TestMethod]
+        public void TestProvider()
+        {
+            var service = new TestService();
+            FiftyOneServiceProvider provider = new FiftyOneServiceProvider();
+            provider.AddService(service);
+
+            // Get the service from the provider
+            var suppliedService = provider.GetService(typeof(TestService));
+
+            // Verify the instance is the same as the one added to the provider. 
+            Assert.AreEqual(service.GetHashCode(), suppliedService.GetHashCode());
+        }
+
+        /// <summary>
+        /// Verify that the provider will return the expected service instance if an
+        /// interface is requested.
+        /// </summary>
+        [TestMethod]
+        public void TestProvider_Interface()
+        {
+            var service = new TestService();
+            FiftyOneServiceProvider provider = new FiftyOneServiceProvider();
+            provider.AddService(service);
+
+            var suppliedService = provider.GetService(typeof(ITestService));
+
+            Assert.AreEqual(service.GetHashCode(), suppliedService.GetHashCode());
+        }
+
+        /// <summary>
+        /// Verify that the provider will return the expected service instance if a service
+        /// is requested that does not exist in the provider. 
+        /// Instead, the provider contains a service which matches the type required by the 
+        /// constructor of the requested type.
+        /// </summary>
+        [TestMethod]
+        public void TestProvider_HighLevelService()
+        {
+            var service = new TestService();
+            FiftyOneServiceProvider provider = new FiftyOneServiceProvider();
+            provider.AddService(service);
+
+            var suppliedService = provider.GetService(typeof(HighLevelService)) as HighLevelService;
+            Assert.AreEqual(service.GetHashCode(), suppliedService.TestService.GetHashCode());
+
+            // Instances stored in FiftyOneServiceProvider are singletons.
+            // In contrast, instances created by it are transient.
+            // Verify this by requesting another instance of the same service and comparing
+            // their hash codes.
+            var suppliedService2 = provider.GetService(typeof(HighLevelService)) as HighLevelService;
+            Assert.AreNotEqual(suppliedService2.GetHashCode(), suppliedService.GetHashCode());
+        }
+
+        /// <summary>
+        /// Verify that the provider will return the expected service instance if a service
+        /// is requested that does not exist in the provider.
+        /// Instead, the provider contains a service that implements an interface which 
+        /// matches the interface required by the constructor of the requested type.
+        /// </summary>
+        [TestMethod]
+        public void TestProvider_HighLevelServiceUsingInterface()
+        {
+            var service = new TestService();
+            FiftyOneServiceProvider provider = new FiftyOneServiceProvider();
+            provider.AddService(service);
+
+            var suppliedService = provider.GetService(typeof(HighLevelServiceUsingInterface)) 
+                as HighLevelServiceUsingInterface;
+            Assert.AreEqual(service.GetHashCode(), suppliedService.TestService.GetHashCode());
+
+            // Instances stored in FiftyOneServiceProvider are singletons.
+            // In contrast, instances created by it are transient.
+            // Verify this by requesting another instance of the same service and comparing
+            // their hash codes.
+            var suppliedService2 = provider.GetService(typeof(HighLevelServiceUsingInterface)) 
+                as HighLevelServiceUsingInterface;
+            Assert.AreNotEqual(suppliedService2.GetHashCode(), suppliedService.GetHashCode());
+        }
+    }
+}