A new pattern for adding multi-registration services

This is some cleanup of how we add multi-registration services to avoid
duplication when calling AddMvcServices multiple times. Also improved
tests to be more clear, and to verify all of our special cases
explicitly.

src/Microsoft.AspNet.Mvc/MvcServiceCollectionExtensions.cs

@@ -3,6 +3,7 @@
 
 using System;
 using System.Collections.Generic;
+using System.Diagnostics;
 using System.Linq;
 using System.Reflection;
 using Microsoft.AspNet.Mvc;
@@ -163,13 +164,23 @@ namespace Microsoft.Framework.DependencyInjection
         internal static void AddMvcServices(IServiceCollection services)
         {
             // Options - all of these are multi-registration
-            services.AddTransient<IConfigureOptions<MvcOptions>, MvcOptionsSetup>();
+            TryAddMultiRegistrationService(
-            services.AddTransient<IConfigureOptions<MvcOptions>, JsonMvcOptionsSetup>();
+                services,
-            services.AddTransient<IConfigureOptions<MvcFormatterMappingOptions>, JsonMvcFormatterMappingOptionsSetup>();
+                ServiceDescriptor.Transient<IConfigureOptions<MvcOptions>, MvcOptionsSetup>());
-            services.AddTransient<IConfigureOptions<MvcViewOptions>, MvcViewOptionsSetup>();
+            TryAddMultiRegistrationService(
-            services.AddTransient<IConfigureOptions<RazorViewEngineOptions>, RazorViewEngineOptionsSetup>();
+                services,
-
+                ServiceDescriptor.Transient<IConfigureOptions<MvcOptions>, JsonMvcOptionsSetup>());
-            services.TryAdd(ServiceDescriptor.Transient<IAssemblyProvider, DefaultAssemblyProvider>());
+            TryAddMultiRegistrationService(
+                services,
+                ServiceDescriptor
+                .Transient<IConfigureOptions<MvcFormatterMappingOptions>, JsonMvcFormatterMappingOptionsSetup>());
+            TryAddMultiRegistrationService(
+                services,
+                ServiceDescriptor.Transient<IConfigureOptions<MvcViewOptions>, MvcViewOptionsSetup>());
+            TryAddMultiRegistrationService(
+                services,
+                ServiceDescriptor
+                .Transient<IConfigureOptions<RazorViewEngineOptions>, RazorViewEngineOptionsSetup>());
 
             services.TryAdd(ServiceDescriptor.Transient<MvcMarkerService, MvcMarkerService>());
             services.TryAdd((ServiceDescriptor.Singleton<ITypeActivatorCache, DefaultTypeActivatorCache>()));
@@ -178,7 +189,17 @@ namespace Microsoft.Framework.DependencyInjection
             // Core action discovery, filters and action execution.
 
             // This are consumed only when creating action descriptors, then they can be de-allocated
-            services.TryAdd(ServiceDescriptor.Transient<IControllerTypeProvider, DefaultControllerTypeProvider>());;
+            services.TryAdd(ServiceDescriptor.Transient<IAssemblyProvider, DefaultAssemblyProvider>());
+            services.TryAdd(ServiceDescriptor.Transient<IControllerTypeProvider, DefaultControllerTypeProvider>()); ;
+            TryAddMultiRegistrationService(
+                services,
+                ServiceDescriptor.Transient<IApplicationModelProvider, DefaultApplicationModelProvider>());
+            TryAddMultiRegistrationService(
+                services,
+                ServiceDescriptor.Transient<IApplicationModelProvider, CorsApplicationModelProvider>());
+            TryAddMultiRegistrationService(
+                services,
+                ServiceDescriptor.Transient<IApplicationModelProvider, AuthorizationApplicationModelProvider>());
 
             // This has a cache, so it needs to be a singleton
             services.TryAdd(ServiceDescriptor.Singleton<IControllerFactory, DefaultControllerFactory>());
@@ -190,8 +211,9 @@ namespace Microsoft.Framework.DependencyInjection
 
             // This provider needs access to the per-request services, but might be used many times for a given
             // request.
-            // multiple registration service
+            TryAddMultiRegistrationService(
-            services.AddTransient<IActionConstraintProvider, DefaultActionConstraintProvider>();
+                services,
+                ServiceDescriptor.Transient<IActionConstraintProvider, DefaultActionConstraintProvider>());
 
             services.TryAdd(ServiceDescriptor
                 .Singleton<IActionSelectorDecisionTreeProvider, ActionSelectorDecisionTreeProvider>());
@@ -205,27 +227,28 @@ namespace Microsoft.Framework.DependencyInjection
                 return new DefaultObjectValidator(options.ValidationExcludeFilters, modelMetadataProvider);
             }));
 
-            // multiple registration service
+            TryAddMultiRegistrationService(
-            services.AddTransient<IActionDescriptorProvider, ControllerActionDescriptorProvider>();
+                services,
+                ServiceDescriptor.Transient<IActionDescriptorProvider, ControllerActionDescriptorProvider>());
 
-            // multiple registration service
+            TryAddMultiRegistrationService(
-            services.AddTransient<IActionInvokerProvider, ControllerActionInvokerProvider>();
+                services,
+                ServiceDescriptor.Transient<IActionInvokerProvider, ControllerActionInvokerProvider>());
 
             services.TryAdd(ServiceDescriptor
                 .Singleton<IActionDescriptorsCollectionProvider, DefaultActionDescriptorsCollectionProvider>());
 
-            // multiple registration service
+            TryAddMultiRegistrationService(
-            services.AddTransient<IFilterProvider, DefaultFilterProvider>();
+                services,
+                ServiceDescriptor.Transient<IFilterProvider, DefaultFilterProvider>());
 
-            // multiple registration service
+            TryAddMultiRegistrationService(
-            services.AddTransient<IApplicationModelProvider, DefaultApplicationModelProvider>();
+                services,
-
+                ServiceDescriptor.Transient<IControllerPropertyActivator, DefaultControllerPropertyActivator>());
-            // multiple registration services
+            TryAddMultiRegistrationService(
-            services.AddTransient<IApplicationModelProvider, CorsApplicationModelProvider>();
+                services,
-            services.AddTransient<IApplicationModelProvider, AuthorizationApplicationModelProvider>();
+                ServiceDescriptor
-
+                    .Transient<IControllerPropertyActivator, ViewDataDictionaryControllerPropertyActivator>());
-            services.AddTransient<IControllerPropertyActivator, DefaultControllerPropertyActivator>();
-            services.AddTransient<IControllerPropertyActivator, ViewDataDictionaryControllerPropertyActivator>();
 
             services.TryAdd(ServiceDescriptor.Transient<FormatFilter, FormatFilter>());
             services.TryAdd(ServiceDescriptor.Transient<CorsAuthorizationFilter, CorsAuthorizationFilter>());
@@ -322,8 +345,9 @@ namespace Microsoft.Framework.DependencyInjection
             // Api Description
             services.TryAdd(ServiceDescriptor
                 .Singleton<IApiDescriptionGroupCollectionProvider, ApiDescriptionGroupCollectionProvider>());
-            // multiple registration service
+            TryAddMultiRegistrationService(
-            services.AddTransient<IApiDescriptionProvider, DefaultApiDescriptionProvider>();
+                services,
+                ServiceDescriptor.Transient<IApiDescriptionProvider, DefaultApiDescriptionProvider>());
 
             // Temp Data
             services.TryAdd(ServiceDescriptor.Scoped<ITempDataDictionary, TempDataDictionary>());
@@ -359,6 +383,27 @@ namespace Microsoft.Framework.DependencyInjection
             return services;
         }
 
+        // Adds a service if the service type and implementation type hasn't been added yet. This is needed for
+        // services like IConfigureOptions<MvcOptions> or IApplicationModelProvider where you need the ability
+        // to register multiple implementation types for the same service type.
+        private static bool TryAddMultiRegistrationService(IServiceCollection services, ServiceDescriptor descriptor)
+        {
+            // This can't work when registering a factory or instance, you have to register a type.
+            // Additionally, if any existing registrations use a factory or instance, we can't check those, but we don't
+            // assert that because it might be added by user-code.
+            Debug.Assert(descriptor.ImplementationType != null);
+
+            if (services.Any(d =>
+                d.ServiceType == descriptor.ServiceType &&
+                d.ImplementationType == descriptor.ImplementationType))
+            {
+                return false;
+            }
+
+            services.Add(descriptor);
+            return true;
+        }
+
         private static void ConfigureDefaultServices(IServiceCollection services)
         {
             services.AddOptions();

test/Microsoft.AspNet.Mvc.Test/MvcServiceCollectionExtensionsTest.cs

@@ -9,10 +9,12 @@ using Microsoft.AspNet.Mvc.ActionConstraints;
 using Microsoft.AspNet.Mvc.ApiExplorer;
 using Microsoft.AspNet.Mvc.ApplicationModels;
 using Microsoft.AspNet.Mvc.Core;
+using Microsoft.AspNet.Mvc.Filters;
 using Microsoft.AspNet.Mvc.MvcServiceCollectionExtensionsTestControllers;
 using Microsoft.AspNet.Mvc.Razor;
 using Microsoft.Framework.DependencyInjection;
 using Microsoft.Framework.OptionsModel;
+using Moq;
 using Xunit;
 
 namespace Microsoft.AspNet.Mvc
@@ -86,24 +88,35 @@ namespace Microsoft.AspNet.Mvc
 
         // Some MVC services can be registered multiple times, for example, 'IConfigureOptions<MvcOptions>' can
         // be registered by calling 'ConfigureMvc(...)' before the call to 'AddMvc()' in which case the options
-        // configiuration is run in the order they were registered.
+        // configuration is run in the order they were registered.
-        // For these kind of multi registration service types, we want to make sure that MVC appends its services
+        //
-        // to the list i.e it does a 'Add' rather than 'TryAdd' on the ServiceCollection.
+        // For these kind of multi registration service types, we want to make sure that MVC will still add its
+        // services if the implementation type is different.
         [Fact]
         public void MultiRegistrationServiceTypes_AreRegistered_MultipleTimes()
         {
             // Arrange
             var services = new ServiceCollection();
-            var multiRegistrationServiceTypes = MutliRegistrationServiceTypes;
+
+            // Register a mock implementation of each service, AddMvcServices should add another implemenetation.
+            foreach (var serviceType in MutliRegistrationServiceTypes)
+            {
+                var mockType = typeof(Mock<>).MakeGenericType(serviceType.Key);
+                services.Add(ServiceDescriptor.Transient(serviceType.Key, mockType));
+            }
 
             // Act
             MvcServiceCollectionExtensions.AddMvcServices(services);
-            MvcServiceCollectionExtensions.AddMvcServices(services);
 
             // Assert
-            foreach (var serviceType in multiRegistrationServiceTypes)
+            foreach (var serviceType in MutliRegistrationServiceTypes)
             {
-                AssertServiceCountEquals(services, serviceType.Key, serviceType.Value);
+                AssertServiceCountEquals(services, serviceType.Key, serviceType.Value.Length + 1);
+
+                foreach (var implementationType in serviceType.Value)
+                {
+                    AssertContainsSingle(services, serviceType.Key, implementationType);
+                }
             }
         }
 
@@ -112,40 +125,152 @@ namespace Microsoft.AspNet.Mvc
         {
             // Arrange
             var services = new ServiceCollection();
-            var multiRegistrationServiceTypes = MutliRegistrationServiceTypes;
+
+            // Register a mock implementation of each service, AddMvcServices should not replace it.
+            foreach (var serviceType in SingleRegistrationServiceTypes)
+            {
+                var mockType = typeof(Mock<>).MakeGenericType(serviceType);
+                services.Add(ServiceDescriptor.Transient(serviceType, mockType));
+            }
+
+            // Act
+            MvcServiceCollectionExtensions.AddMvcServices(services);
+
+            // Assert
+            foreach (var singleRegistrationType in SingleRegistrationServiceTypes)
+            {
+                AssertServiceCountEquals(services, singleRegistrationType, 1);
+            }
+        }
+
+        [Fact]
+        public void AddMvcServicesTwice_DoesNotAddDuplicates()
+        {
+            // Arrange
+            var services = new ServiceCollection();
 
             // Act
             MvcServiceCollectionExtensions.AddMvcServices(services);
             MvcServiceCollectionExtensions.AddMvcServices(services);
 
             // Assert
-            var singleRegistrationServiceTypes = services
+            var singleRegistrationServiceTypes = SingleRegistrationServiceTypes;
-                .Where(serviceDescriptor => !multiRegistrationServiceTypes.Keys.Contains(serviceDescriptor.ServiceType))
+            foreach (var service in services)
-                .Select(serviceDescriptor => serviceDescriptor.ServiceType);
-
-            foreach (var singleRegistrationType in singleRegistrationServiceTypes)
             {
-                AssertServiceCountEquals(services, singleRegistrationType, 1);
+                if (singleRegistrationServiceTypes.Contains(service.ServiceType))
+                {
+                    // 'single-registration' services should only have one implementation registered.
+                    AssertServiceCountEquals(services, service.ServiceType, 1);
+                }
+                else
+                {
+                    // 'multi-registration' services should only have one *instance* of each implementation registered.
+                    AssertContainsSingle(services, service.ServiceType, service.ImplementationType);
+                }
             }
         }
 
-        private Dictionary<Type, int> MutliRegistrationServiceTypes
+        private IEnumerable<Type> SingleRegistrationServiceTypes
         {
             get
             {
-                return new Dictionary<Type, int>()
+                var services = new ServiceCollection();
+                MvcServiceCollectionExtensions.AddMvcServices(services);
+
+                var multiRegistrationServiceTypes = MutliRegistrationServiceTypes;
+                return services
+                    .Where(sd => !multiRegistrationServiceTypes.Keys.Contains(sd.ServiceType))
+                    .Select(sd => sd.ServiceType);
+            }
+        }
+
+        private Dictionary<Type, Type[]> MutliRegistrationServiceTypes
+        {
+            get
+            {
+                return new Dictionary<Type, Type[]>()
                 {
-                    { typeof(IConfigureOptions<MvcOptions>), 4 },
+                    {
-                    { typeof(IConfigureOptions<MvcFormatterMappingOptions>), 2 },
+                        typeof(IConfigureOptions<MvcOptions>),
-                    { typeof(IConfigureOptions<MvcViewOptions>), 2 }, 
+                        new Type[]
-                    { typeof(IConfigureOptions<RazorViewEngineOptions>), 2 },
+                        {
-                    { typeof(IActionConstraintProvider), 2 },
+                            typeof(MvcOptionsSetup),
-                    { typeof(IActionDescriptorProvider), 2 },
+                            typeof(JsonMvcOptionsSetup),
-                    { typeof(IActionInvokerProvider), 2 },
+                        }
-                    { typeof(IControllerPropertyActivator), 4 },
+                    },
-                    { typeof(IFilterProvider), 2 },
+                    {
-                    { typeof(IApiDescriptionProvider), 2 },
+                        typeof(IConfigureOptions<MvcFormatterMappingOptions>),
-                    { typeof(IApplicationModelProvider), 6 },
+                        new Type[]
+                        {
+                            typeof(JsonMvcFormatterMappingOptionsSetup),
+                        }
+                    },
+                    {
+                        typeof(IConfigureOptions<MvcViewOptions>),
+                        new Type[]
+                        {
+                            typeof(MvcViewOptionsSetup),
+                        }
+                    },
+                    {
+                        typeof(IConfigureOptions<RazorViewEngineOptions>),
+                        new Type[]
+                        {
+                            typeof(RazorViewEngineOptionsSetup),
+                        }
+                    },
+                    {
+                        typeof(IActionConstraintProvider),
+                        new Type[]
+                        {
+                            typeof(DefaultActionConstraintProvider),
+                        }
+                    },
+                    {
+                        typeof(IActionDescriptorProvider),
+                        new Type[]
+                        {
+                            typeof(ControllerActionDescriptorProvider),
+                        }
+                    },
+                    {
+                        typeof(IActionInvokerProvider),
+                        new Type[]
+                        {
+                            typeof(ControllerActionInvokerProvider),
+                        }
+                    },
+                    {
+                        typeof(IFilterProvider),
+                        new Type[]
+                        {
+                            typeof(DefaultFilterProvider),
+                        }
+                    },
+                    {
+                        typeof(IApiDescriptionProvider),
+                        new Type[]
+                        {
+                            typeof(DefaultApiDescriptionProvider),
+                        }
+                    },
+                    {
+                        typeof(IControllerPropertyActivator),
+                        new Type[]
+                        {
+                            typeof(DefaultControllerPropertyActivator),
+                            typeof(ViewDataDictionaryControllerPropertyActivator),
+                        }
+                    },
+                    {
+                        typeof(IApplicationModelProvider),
+                        new Type[]
+                        {
+                            typeof(DefaultApplicationModelProvider),
+                            typeof(CorsApplicationModelProvider),
+                            typeof(AuthorizationApplicationModelProvider),
+                        }
+                    },
                 };
             }
         }
@@ -164,6 +289,31 @@ namespace Microsoft.AspNet.Mvc
                 $" time(s) but was actually registered {actual} time(s).");
         }
 
+        private void AssertContainsSingle(
+            IServiceCollection services,
+            Type serviceType,
+            Type implementationType)
+        {
+            var matches = services
+                .Where(sd =>
+                    sd.ServiceType == serviceType &&
+                    sd.ImplementationType == implementationType)
+                .ToArray();
+
+            if (matches.Length == 0)
+            {
+                Assert.True(
+                    false,
+                    $"Could not find an instance of {implementationType} registered as {serviceType}");
+            }
+            else if (matches.Length > 1)
+            {
+                Assert.True(
+                    false,
+                    $"Found multiple instances of {implementationType} registered as {serviceType}");
+            }
+        }
+
         private class CustomActivator : IControllerActivator
         {
             public object Create(ActionContext context, Type controllerType)