From 08616127797ea14b2738cce995fa90ee7ab59458 Mon Sep 17 00:00:00 2001 From: Ryan Nowak Date: Tue, 9 Jun 2015 14:58:58 -0700 Subject: [PATCH] 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. --- .../MvcServiceCollectionExtensions.cs | 99 ++++++--- .../MvcServiceCollectionExtensionsTest.cs | 204 +++++++++++++++--- 2 files changed, 249 insertions(+), 54 deletions(-) diff --git a/src/Microsoft.AspNet.Mvc/MvcServiceCollectionExtensions.cs b/src/Microsoft.AspNet.Mvc/MvcServiceCollectionExtensions.cs index 3ec3e18080..64d2f3dacd 100644 --- a/src/Microsoft.AspNet.Mvc/MvcServiceCollectionExtensions.cs +++ b/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, MvcOptionsSetup>(); - services.AddTransient, JsonMvcOptionsSetup>(); - services.AddTransient, JsonMvcFormatterMappingOptionsSetup>(); - services.AddTransient, MvcViewOptionsSetup>(); - services.AddTransient, RazorViewEngineOptionsSetup>(); - - services.TryAdd(ServiceDescriptor.Transient()); + TryAddMultiRegistrationService( + services, + ServiceDescriptor.Transient, MvcOptionsSetup>()); + TryAddMultiRegistrationService( + services, + ServiceDescriptor.Transient, JsonMvcOptionsSetup>()); + TryAddMultiRegistrationService( + services, + ServiceDescriptor + .Transient, JsonMvcFormatterMappingOptionsSetup>()); + TryAddMultiRegistrationService( + services, + ServiceDescriptor.Transient, MvcViewOptionsSetup>()); + TryAddMultiRegistrationService( + services, + ServiceDescriptor + .Transient, RazorViewEngineOptionsSetup>()); services.TryAdd(ServiceDescriptor.Transient()); services.TryAdd((ServiceDescriptor.Singleton())); @@ -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());; + services.TryAdd(ServiceDescriptor.Transient()); + services.TryAdd(ServiceDescriptor.Transient()); ; + TryAddMultiRegistrationService( + services, + ServiceDescriptor.Transient()); + TryAddMultiRegistrationService( + services, + ServiceDescriptor.Transient()); + TryAddMultiRegistrationService( + services, + ServiceDescriptor.Transient()); // This has a cache, so it needs to be a singleton services.TryAdd(ServiceDescriptor.Singleton()); @@ -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 - services.AddTransient(); + TryAddMultiRegistrationService( + services, + ServiceDescriptor.Transient()); services.TryAdd(ServiceDescriptor .Singleton()); @@ -205,27 +227,28 @@ namespace Microsoft.Framework.DependencyInjection return new DefaultObjectValidator(options.ValidationExcludeFilters, modelMetadataProvider); })); - // multiple registration service - services.AddTransient(); + TryAddMultiRegistrationService( + services, + ServiceDescriptor.Transient()); - // multiple registration service - services.AddTransient(); + TryAddMultiRegistrationService( + services, + ServiceDescriptor.Transient()); services.TryAdd(ServiceDescriptor .Singleton()); - // multiple registration service - services.AddTransient(); + TryAddMultiRegistrationService( + services, + ServiceDescriptor.Transient()); - // multiple registration service - services.AddTransient(); - - // multiple registration services - services.AddTransient(); - services.AddTransient(); - - services.AddTransient(); - services.AddTransient(); + TryAddMultiRegistrationService( + services, + ServiceDescriptor.Transient()); + TryAddMultiRegistrationService( + services, + ServiceDescriptor + .Transient()); services.TryAdd(ServiceDescriptor.Transient()); services.TryAdd(ServiceDescriptor.Transient()); @@ -322,8 +345,9 @@ namespace Microsoft.Framework.DependencyInjection // Api Description services.TryAdd(ServiceDescriptor .Singleton()); - // multiple registration service - services.AddTransient(); + TryAddMultiRegistrationService( + services, + ServiceDescriptor.Transient()); // Temp Data services.TryAdd(ServiceDescriptor.Scoped()); @@ -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 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(); diff --git a/test/Microsoft.AspNet.Mvc.Test/MvcServiceCollectionExtensionsTest.cs b/test/Microsoft.AspNet.Mvc.Test/MvcServiceCollectionExtensionsTest.cs index e3145f9442..cdfc623e65 100644 --- a/test/Microsoft.AspNet.Mvc.Test/MvcServiceCollectionExtensionsTest.cs +++ b/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' 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. - // 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. + // configuration is run in the order they were registered. + // + // 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 - .Where(serviceDescriptor => !multiRegistrationServiceTypes.Keys.Contains(serviceDescriptor.ServiceType)) - .Select(serviceDescriptor => serviceDescriptor.ServiceType); - - foreach (var singleRegistrationType in singleRegistrationServiceTypes) + var singleRegistrationServiceTypes = SingleRegistrationServiceTypes; + foreach (var service in services) { - 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 MutliRegistrationServiceTypes + private IEnumerable SingleRegistrationServiceTypes { get { - return new Dictionary() + 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 MutliRegistrationServiceTypes + { + get + { + return new Dictionary() { - { typeof(IConfigureOptions), 4 }, - { typeof(IConfigureOptions), 2 }, - { typeof(IConfigureOptions), 2 }, - { typeof(IConfigureOptions), 2 }, - { typeof(IActionConstraintProvider), 2 }, - { typeof(IActionDescriptorProvider), 2 }, - { typeof(IActionInvokerProvider), 2 }, - { typeof(IControllerPropertyActivator), 4 }, - { typeof(IFilterProvider), 2 }, - { typeof(IApiDescriptionProvider), 2 }, - { typeof(IApplicationModelProvider), 6 }, + { + typeof(IConfigureOptions), + new Type[] + { + typeof(MvcOptionsSetup), + typeof(JsonMvcOptionsSetup), + } + }, + { + typeof(IConfigureOptions), + new Type[] + { + typeof(JsonMvcFormatterMappingOptionsSetup), + } + }, + { + typeof(IConfigureOptions), + new Type[] + { + typeof(MvcViewOptionsSetup), + } + }, + { + typeof(IConfigureOptions), + 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)