diff --git a/FiftyOne.Pipeline.Core/FlowElements/PipelineBuilder.cs b/FiftyOne.Pipeline.Core/FlowElements/PipelineBuilder.cs index aef84859..84f90bca 100644 --- a/FiftyOne.Pipeline.Core/FlowElements/PipelineBuilder.cs +++ b/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); } - /// - /// Get the services required for the constructor, and call it with them. - /// - /// - /// The constructor to call. - /// - /// - /// Instance returned by the constructor. - /// - 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); - } - - - /// - /// Get the best constructor for the list of constructors. Best meaning - /// the constructor with the most parameters which can be fulfilled. - /// - /// - /// Constructors to get the best of. - /// - /// - /// Best constructor or null if none have parameters that can be - /// fulfilled. - /// - private ConstructorInfo GetBestConstructorForAssemblies( - IEnumerable 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; - } - /// /// Instantiate a new builder instance from the assemblies which are /// currently loaded. @@ -521,15 +458,9 @@ 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; } @@ -537,13 +468,7 @@ private object GetBuilderFromAssemlies(Type builderType) // 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); } diff --git a/FiftyOne.Pipeline.Core/Services/FiftyOneServiceProvider.cs b/FiftyOne.Pipeline.Core/Services/FiftyOneServiceProvider.cs index 26ac2891..babb23fb 100644 --- a/FiftyOne.Pipeline.Core/Services/FiftyOneServiceProvider.cs +++ b/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) /// - /// 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. /// @@ -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; + } + + /// + /// Get the services required for the constructor, and call it with them. + /// + /// + /// The constructor to call. + /// + /// + /// Instance returned by the constructor. + /// + 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); + } + + + /// + /// Get the best constructor for the list of constructors. Best meaning + /// the constructor with the most parameters which can be fulfilled. + /// + /// + /// The type we want a constructor for + /// + /// + /// Best constructor or null if none have parameters that can be + /// fulfilled. + /// + 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(); + } } } diff --git a/Tests/FiftyOne.Pipeline.Core.Tests/FlowElements/PipelineBuilderTests.cs b/Tests/FiftyOne.Pipeline.Core.Tests/FlowElements/PipelineBuilderTests.cs index 7878a365..009e72c6 100644 --- a/Tests/FiftyOne.Pipeline.Core.Tests/FlowElements/PipelineBuilderTests.cs +++ b/Tests/FiftyOne.Pipeline.Core.Tests/FlowElements/PipelineBuilderTests.cs @@ -748,7 +748,9 @@ public void PipelineBuilder_BuildFromConfiguration_AssemblyServices_NotAvailable .BuildFromConfiguration(opts); Assert.IsNotNull(pipeline.GetElement().LoggerFactory); - Assert.IsNull(pipeline.GetElement().Service); + // The service is not available in the service provider, but it can be created + // by the service provider. + Assert.IsNotNull(pipeline.GetElement().Service); Assert.IsNull(pipeline.GetElement().UpdateService); } diff --git a/Tests/FiftyOne.Pipeline.Core.Tests/Services/FiftyOneServiceProviderTests.cs b/Tests/FiftyOne.Pipeline.Core.Tests/Services/FiftyOneServiceProviderTests.cs new file mode 100644 index 00000000..84f2fc3e --- /dev/null +++ b/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 + { + /// + /// Empty interface used for testing FiftyOneServiceProvider + /// + public interface ITestService + { + } + + /// + /// Empty class used for testing FiftyOneServiceProvider + /// + public class TestService : ITestService + { + } + + /// + /// Class used for testing FiftyOneServiceProvider. + /// Takes a TestService as a constructor parameter. + /// + public class HighLevelService + { + public TestService TestService { get; set; } + + public HighLevelService(TestService testService) + { + TestService = testService; + } + } + + /// + /// Class used for testing FiftyOneServiceProvider. + /// Takes an ITestService as a constructor parameter. + /// + public class HighLevelServiceUsingInterface + { + public ITestService TestService { get; set; } + + public HighLevelServiceUsingInterface(ITestService testService) + { + TestService = testService; + } + } + + /// + /// Verify that the provider will return the expected service instance. + /// + [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()); + } + + /// + /// Verify that the provider will return the expected service instance if an + /// interface is requested. + /// + [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()); + } + + /// + /// 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. + /// + [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()); + } + + /// + /// 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. + /// + [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()); + } + } +} diff --git a/ci/common-ci b/ci/common-ci index c5729d99..0d579c44 160000 --- a/ci/common-ci +++ b/ci/common-ci @@ -1 +1 @@ -Subproject commit c5729d99fedc8cd42fca379f522655c76717ed3b +Subproject commit 0d579c4495061bad8e6de797012d2897a51c4b26