Make InsertProvider idempotent and remove reflection

Author: ronaldbarendse

src/ImageSharp.Web/DependencyInjection/ImageSharpBuilderExtensions.cs

@@ -3,7 +3,6 @@
 
 using System;
 using System.Linq;
-using System.Reflection;
 using Microsoft.Extensions.Configuration;
 using Microsoft.Extensions.DependencyInjection;
 using Microsoft.Extensions.DependencyInjection.Extensions;
@@ -166,11 +165,12 @@ public static IImageSharpBuilder InsertProvider<TProvider>(this IImageSharpBuild
             where TProvider : class, IImageProvider
         {
             var descriptors = builder.Services.Where(x => x.ServiceType == typeof(IImageProvider)).ToList();
-            builder.ClearProviders();
-
+            descriptors.RemoveAll(x => x.GetImplementationType() == typeof(TProvider));
             descriptors.Insert(index, ServiceDescriptor.Singleton<IImageProvider, TProvider>());
 
+            builder.ClearProviders();
             builder.Services.TryAddEnumerable(descriptors);
+
             return builder;
         }
 
@@ -186,11 +186,12 @@ public static IImageSharpBuilder InsertProvider<TProvider>(this IImageSharpBuild
             where TProvider : class, IImageProvider
         {
             var descriptors = builder.Services.Where(x => x.ServiceType == typeof(IImageProvider)).ToList();
-            builder.ClearProviders();
-
+            descriptors.RemoveAll(x => x.GetImplementationType() == typeof(TProvider));
             descriptors.Insert(index, ServiceDescriptor.Singleton<IImageProvider>(implementationFactory));
 
+            builder.ClearProviders();
             builder.Services.TryAddEnumerable(descriptors);
+
             return builder;
         }
 
@@ -203,11 +204,7 @@ public static IImageSharpBuilder InsertProvider<TProvider>(this IImageSharpBuild
         public static IImageSharpBuilder RemoveProvider<TProvider>(this IImageSharpBuilder builder)
             where TProvider : class, IImageProvider
         {
-            ServiceDescriptor descriptor = builder.Services.FirstOrDefault(x =>
-                x.ServiceType == typeof(IImageProvider)
-                && (x.ImplementationType == typeof(TProvider)
-                || (x.ImplementationFactory?.GetMethodInfo().ReturnType == typeof(TProvider))));
-
+            ServiceDescriptor descriptor = builder.Services.FirstOrDefault(x => x.ServiceType == typeof(IImageProvider) && x.GetImplementationType() == typeof(TProvider));
             if (descriptor != null)
             {
                 builder.Services.Remove(descriptor);
@@ -264,12 +261,7 @@ public static IImageSharpBuilder AddProcessor<TProcessor>(this IImageSharpBuilde
         public static IImageSharpBuilder RemoveProcessor<TProcessor>(this IImageSharpBuilder builder)
             where TProcessor : class, IImageWebProcessor
         {
-            ServiceDescriptor descriptor = builder.Services.FirstOrDefault(x =>
-                x.ServiceType == typeof(IImageWebProcessor)
-                && x.Lifetime == ServiceLifetime.Singleton
-                && (x.ImplementationType == typeof(TProcessor)
-                || (x.ImplementationFactory?.GetMethodInfo().ReturnType == typeof(TProcessor))));
-
+            ServiceDescriptor descriptor = builder.Services.FirstOrDefault(x => x.ServiceType == typeof(IImageWebProcessor) && x.GetImplementationType() == typeof(TProcessor));
             if (descriptor != null)
             {
                 builder.Services.Remove(descriptor);
@@ -326,12 +318,7 @@ public static IImageSharpBuilder AddConverter<TConverter>(this IImageSharpBuilde
         public static IImageSharpBuilder RemoveConverter<TConverter>(this IImageSharpBuilder builder)
             where TConverter : class, ICommandConverter
         {
-            ServiceDescriptor descriptor = builder.Services.FirstOrDefault(x =>
-                x.ServiceType == typeof(ICommandConverter)
-                && x.Lifetime == ServiceLifetime.Singleton
-                && (x.ImplementationType == typeof(TConverter)
-                || (x.ImplementationFactory?.GetMethodInfo().ReturnType == typeof(TConverter))));
-
+            ServiceDescriptor descriptor = builder.Services.FirstOrDefault(x => x.ServiceType == typeof(ICommandConverter) && x.GetImplementationType() == typeof(TConverter));
             if (descriptor != null)
             {
                 builder.Services.Remove(descriptor);
@@ -379,5 +366,10 @@ public static IImageSharpBuilder Configure<TOptions>(this IImageSharpBuilder bui
             builder.Services.Configure(configureOptions);
             return builder;
         }
+
+        private static Type GetImplementationType(this ServiceDescriptor descriptor)
+            => descriptor.ImplementationType
+            ?? descriptor.ImplementationInstance?.GetType()
+            ?? descriptor.ImplementationFactory?.GetType().GenericTypeArguments[1];
     }
 }

tests/ImageSharp.Web.Tests/DependencyInjection/ServiceRegistrationExtensionsTests.cs

@@ -1,6 +1,7 @@
 // Copyright (c) Six Labors.
 // Licensed under the Apache License, Version 2.0.
 
+using System;
 using System.Collections.Generic;
 using System.Linq;
 using System.Reflection;
@@ -191,11 +192,32 @@ public void CanInsertRemoveImageProviders()
             IReadOnlyList<ServiceDescriptor> providers = GetCollection<IImageProvider>(services);
             Assert.Equal(2, providers.Count);
             Assert.True(IsService<IImageProvider, MockImageProvider>(providers[0]));
+            Assert.True(IsServiceImplementationType<IImageProvider, MockImageProvider>(providers[0]));
 
             builder.RemoveProvider<MockImageProvider>();
             Assert.DoesNotContain(services, IsService<IImageProvider, MockImageProvider>);
         }
 
+        [Fact]
+        public void CanInsertIdempotentImageProviders()
+        {
+            var services = new ServiceCollection();
+            IImageSharpBuilder builder = services.AddImageSharp();
+
+            builder.InsertProvider<MockImageProvider>(0);
+            builder.InsertProvider<MockImageProvider>(1);
+
+            Assert.Throws<ArgumentOutOfRangeException>(() => builder.InsertProvider<MockImageProvider>(2));
+
+            Assert.Single(services, IsService<IImageProvider, MockImageProvider>);
+            Assert.Single(services, IsServiceImplementationType<IImageProvider, MockImageProvider>);
+
+            IReadOnlyList<ServiceDescriptor> providers = GetCollection<IImageProvider>(services);
+            Assert.Equal(2, providers.Count);
+            Assert.True(IsService<IImageProvider, MockImageProvider>(providers[1]));
+            Assert.True(IsServiceImplementationType<IImageProvider, MockImageProvider>(providers[1]));
+        }
+
         [Fact]
         public void CanAddRemoveFactoryImageProviders()
         {
@@ -210,6 +232,26 @@ public void CanAddRemoveFactoryImageProviders()
             Assert.DoesNotContain(services, IsService<IImageProvider, MockImageProvider>);
         }
 
+        [Fact]
+        public void CanInsertIdempotentFactoryImageProviders()
+        {
+            var services = new ServiceCollection();
+            IImageSharpBuilder builder = services.AddImageSharp();
+
+            builder.InsertProvider(0, _ => new MockImageProvider());
+            builder.InsertProvider(1, _ => new MockImageProvider());
+
+            Assert.Throws<ArgumentOutOfRangeException>(() => builder.InsertProvider(2, _ => new MockImageProvider()));
+
+            Assert.Single(services, IsService<IImageProvider, MockImageProvider>);
+            Assert.Single(services, IsServiceImplementationFactory<IImageProvider, MockImageProvider>);
+
+            IReadOnlyList<ServiceDescriptor> providers = GetCollection<IImageProvider>(services);
+            Assert.Equal(2, providers.Count);
+            Assert.True(IsService<IImageProvider, MockImageProvider>(providers[1]));
+            Assert.True(IsServiceImplementationFactory<IImageProvider, MockImageProvider>(providers[1]));
+        }
+
         [Fact]
         public void CanInsertRemoveFactoryImageProviders()
         {