From bbe8825216fd3af97eaa0a3e8c9a43f8bd99beef Mon Sep 17 00:00:00 2001 From: dot_net_junkie Date: Mon, 31 Aug 2015 21:12:43 +0200 Subject: [PATCH] Bug fixes with AllowOverridingRegistrations. Overriding a registration failed when done with same implementation, and registration of conditional non-generic registrations was accidentally allowed. (fixes #114) --- .../ContainerOptionsTests.cs | 114 ++++++++++++++++++ SimpleInjector.NET/Container.Common.cs | 4 +- .../Internals/GenericRegistrationEntry.cs | 25 +++- .../Internals/NonGenericRegistrationEntry.cs | 74 +++++++----- SimpleInjector.NET/StringResources.cs | 2 +- changes.txt | 9 +- 6 files changed, 190 insertions(+), 38 deletions(-) diff --git a/SimpleInjector.NET.Tests.Unit/ContainerOptionsTests.cs b/SimpleInjector.NET.Tests.Unit/ContainerOptionsTests.cs index 50fab09f9..0f8a97fa8 100644 --- a/SimpleInjector.NET.Tests.Unit/ContainerOptionsTests.cs +++ b/SimpleInjector.NET.Tests.Unit/ContainerOptionsTests.cs @@ -220,6 +220,120 @@ public void AllowOverridingRegistrations_SetToTrue_ContainerDoesAllowOverringCol AssertThat.IsInstanceOfType(typeof(InMemoryUserRepository), instance); } + + [TestMethod] + public void AllowOverridingRegistrations_SetToTrueWhileOverridingRegistrationWithSameImplementation_Succeeds() + { + // Arrange + var container = new Container(); + + container.Register(typeof(ILogger), typeof(NullLogger)); + container.Options.AllowOverridingRegistrations = true; + + // Act + container.Register(typeof(ILogger), typeof(NullLogger)); + } + + [TestMethod] + public void RegisterConditional_AllowOverridingRegistrationsSetTrueAndOverlappingConditionalExists_ThrowsException() + { + // Arrange + var container = new Container(); + + container.RegisterConditional(typeof(ILogger), typeof(NullLogger), c => true); + container.Options.AllowOverridingRegistrations = true; + + // Act + Action action = () => container.RegisterConditional(typeof(ILogger), typeof(ConsoleLogger), c => true); + + // Assert + AssertThat.ThrowsWithExceptionMessageContains( + "making of conditional registrations is not supported when AllowOverridingRegistrations is set", + action); + } + + [TestMethod] + public void AllowOverridingRegistrations_SetToTrueWhileOverridingRegistrationWithDifferentGenericImplementation_ResolvesNewType() + { + // Arrange + var container = new Container(); + + container.Register(typeof(ICommandHandler<>), typeof(NullCommandHandler<>)); + container.Options.AllowOverridingRegistrations = true; + + container.Register(typeof(ICommandHandler<>), typeof(DefaultCommandHandler<>)); + + // Act + var instance = container.GetInstance>(); + + // Assert + AssertThat.IsInstanceOfType(typeof(DefaultCommandHandler), instance); + } + + [TestMethod] + public void AllowOverridingRegistrations_SetToTrueWhileOverridingRegistrationWithSameGenericImplementation_Succeeds() + { + // Arrange + var container = new Container(); + + container.Register(typeof(ICommandHandler<>), typeof(NullCommandHandler<>)); + container.Options.AllowOverridingRegistrations = true; + + container.Register(typeof(ICommandHandler<>), typeof(NullCommandHandler<>)); + + // Act + container.GetInstance>(); + } + + [TestMethod] + public void AllowOverridingRegistrations_SetToFalseWhileOverridingRegistrationWithSameGenericImplementation_Succeeds() + { + // Arrange + var container = new Container(); + + container.Register(typeof(ICommandHandler<>), typeof(NullCommandHandler<>)); + container.Options.AllowOverridingRegistrations = false; + + // Act + Action action = () => container.Register(typeof(ICommandHandler<>), typeof(NullCommandHandler<>)); + + // Assert + AssertThat.Throws(action); + } + + [TestMethod] + public void AllowOverridingRegistrations_SetToTrueWhileOverridingGenericTypeWithoutConstraints_SuccessfullyReplacesTheOld() + { + // Arrange + var container = new Container(); + + container.Register(typeof(ICommandHandler<>), typeof(NullCommandHandler<>)); + container.Options.AllowOverridingRegistrations = true; + container.Register(typeof(ICommandHandler<>), typeof(DefaultCommandHandler<>)); + + // Act + var instance = container.GetInstance>(); + + // Assert + AssertThat.IsInstanceOfType(typeof(DefaultCommandHandler), instance); + } + + [TestMethod] + public void Verify_SingletonRegistrationOverriddenWithExactSameImplementation_DoesNotCauseTornLifestyleError() + { + // Arrange + var container = new Container(); + + container.Register(typeof(ILogger), typeof(NullLogger), Lifestyle.Singleton); + container.Options.AllowOverridingRegistrations = true; + + container.Register(typeof(ILogger), typeof(NullLogger), Lifestyle.Singleton); + + // Act + container.Verify(VerificationOption.VerifyAndDiagnose); + } + + [TestMethod] public void ConstructorResolutionBehavior_ChangedBeforeAnyRegistrations_ChangesThePropertyToTheSetInstance() { diff --git a/SimpleInjector.NET/Container.Common.cs b/SimpleInjector.NET/Container.Common.cs index 658012317..443200dab 100644 --- a/SimpleInjector.NET/Container.Common.cs +++ b/SimpleInjector.NET/Container.Common.cs @@ -113,8 +113,6 @@ public Container() { this.containerId = Interlocked.Increment(ref counter); - this.RegisterSingleton(this); - this.Options = new ContainerOptions(this) { EnableDynamicAssemblyCompilation = @@ -122,6 +120,8 @@ public Container() }; this.SelectionBasedLifestyle = new LifestyleSelectionBehaviorProxyLifestyle(this.Options); + + this.RegisterSingleton(this); } // Wrapper for instance initializer delegates diff --git a/SimpleInjector.NET/Internals/GenericRegistrationEntry.cs b/SimpleInjector.NET/Internals/GenericRegistrationEntry.cs index 04ae5d983..0b2cc0885 100644 --- a/SimpleInjector.NET/Internals/GenericRegistrationEntry.cs +++ b/SimpleInjector.NET/Internals/GenericRegistrationEntry.cs @@ -74,13 +74,19 @@ public void AddGeneric(Type serviceType, Type implementationType, Lifestyle lifestyle, Predicate predicate) { this.container.ThrowWhenContainerIsLocked(); - this.ThrowWhenConditionalIsRegisteredInOverridingMode(predicate); var provider = new OpenGenericToInstanceProducerProvider( serviceType, implementationType, lifestyle, predicate, this.container); + this.ThrowWhenConditionalIsRegisteredInOverridingMode(provider); + this.ThrowWhenProviderToRegisterOverlapsWithExistingProvider(provider); + if (provider.AppliesToAllClosedServiceTypes && this.container.Options.AllowOverridingRegistrations) + { + this.providers.RemoveAll(p => p.AppliesToAllClosedServiceTypes); + } + this.providers.Add(provider); } @@ -88,11 +94,12 @@ public void Add(Type serviceType, Func implementationT Lifestyle lifestyle, Predicate predicate) { this.container.ThrowWhenContainerIsLocked(); - this.ThrowWhenConditionalIsRegisteredInOverridingMode(predicate); var provider = new OpenGenericToInstanceProducerProvider( serviceType, implementationTypeFactory, lifestyle, predicate, this.container); + this.ThrowWhenConditionalIsRegisteredInOverridingMode(provider); + this.ThrowWhenProviderToRegisterOverlapsWithExistingProvider(provider); this.providers.Add(provider); @@ -147,9 +154,10 @@ where provider.OverlapsWith(producer.ServiceType) } } - private void ThrowWhenConditionalIsRegisteredInOverridingMode(Predicate predicate) + private void ThrowWhenConditionalIsRegisteredInOverridingMode( + OpenGenericToInstanceProducerProvider provider) { - if (predicate != null && this.container.Options.AllowOverridingRegistrations) + if (!provider.AppliesToAllClosedServiceTypes && this.container.Options.AllowOverridingRegistrations) { throw new NotSupportedException( StringResources.MakingConditionalRegistrationsInOverridingModeIsNotSupported()); @@ -162,11 +170,16 @@ private void ThrowWhenProviderToRegisterOverlapsWithExistingProvider( bool providerToRegisterIsSuperset = providerToRegister.AppliesToAllClosedServiceTypes && this.providers.Any(); + bool isReplacement = providerToRegister.AppliesToAllClosedServiceTypes + && this.container.Options.AllowOverridingRegistrations; + // A provider is a superset of the providerToRegister when it can be applied to ALL generic // types that the providerToRegister can be applied to as well. var supersetProviders = this.GetSupersetProvidersFor(providerToRegister.ImplementationType); - if (providerToRegisterIsSuperset || supersetProviders.Any()) + bool overlaps = providerToRegisterIsSuperset || supersetProviders.Any(); + + if (!isReplacement && overlaps) { var overlappingProvider = supersetProviders.FirstOrDefault() ?? this.providers.First(); @@ -188,7 +201,7 @@ private InvalidOperationException GetAnOverlappingGenericRegistrationExistsExcep IProducerProvider providerToRegister, IProducerProvider overlappingProvider) { return new InvalidOperationException( - StringResources.AnOverlappingGenericRegistrationExists( + StringResources.AnOverlappingRegistrationExists( providerToRegister.ServiceType, overlappingProvider.ImplementationType, overlappingProvider.IsConditional, diff --git a/SimpleInjector.NET/Internals/NonGenericRegistrationEntry.cs b/SimpleInjector.NET/Internals/NonGenericRegistrationEntry.cs index 4fd2f9234..ca033f0a3 100644 --- a/SimpleInjector.NET/Internals/NonGenericRegistrationEntry.cs +++ b/SimpleInjector.NET/Internals/NonGenericRegistrationEntry.cs @@ -54,9 +54,10 @@ public void Add(InstanceProducer producer) { this.container.ThrowWhenContainerIsLocked(); this.ThrowWhenConditionalAndUnconditionalAreMixed(producer); + this.ThrowWhenConditionalIsRegisteredInOverridingMode(producer); this.ThrowWhenTypeAlreadyRegistered(producer); - this.ThrowWhenProviderToRegisterOverlapsWithExistingProvider(producer); + this.ThrowWhenIdenticalImplementationIsAlreadyRegistered(producer); if (producer.IsUnconditional) { @@ -66,33 +67,6 @@ public void Add(InstanceProducer producer) this.providers.Add(new SingleInstanceProducerProvider(producer)); } - private void ThrowWhenProviderToRegisterOverlapsWithExistingProvider( - InstanceProducer producerToRegister) - { - // A provider is a superset of the providerToRegister when it can be applied to ALL generic - // types that the providerToRegister can be applied to as well. - var overlappingProducers = - from producer in this.CurrentProducers - where producer.ImplementationType != null - where !producer.Registration.WrapsInstanceCreationDelegate - where !producerToRegister.Registration.WrapsInstanceCreationDelegate - where producer.ImplementationType == producerToRegister.ImplementationType - select producer; - - if (overlappingProducers.Any()) - { - var overlappingProducer = overlappingProducers.FirstOrDefault(); - - throw new InvalidOperationException( - StringResources.AnOverlappingGenericRegistrationExists( - producerToRegister.ServiceType, - overlappingProducer.ImplementationType, - overlappingProducer.IsConditional, - producerToRegister.ImplementationType, - producerToRegister.IsConditional)); - } - } - public void Add(Type serviceType, Func implementationTypeFactory, Lifestyle lifestyle, Predicate predicate) { @@ -153,6 +127,41 @@ private void ThrowWhenTypeAlreadyRegistered(InstanceProducer producer) } } + private void ThrowWhenIdenticalImplementationIsAlreadyRegistered( + InstanceProducer producerToRegister) + { + // A provider overlaps the providerToRegister when it can be applied to ALL generic + // types that the providerToRegister can be applied to as well. + var overlappingProducers = this.GetOverlappingProducers(producerToRegister); + + bool isReplacement = + producerToRegister.IsUnconditional && this.container.Options.AllowOverridingRegistrations; + + if (!isReplacement && overlappingProducers.Any()) + { + var overlappingProducer = overlappingProducers.FirstOrDefault(); + + throw new InvalidOperationException( + StringResources.AnOverlappingRegistrationExists( + producerToRegister.ServiceType, + overlappingProducer.ImplementationType, + overlappingProducer.IsConditional, + producerToRegister.ImplementationType, + producerToRegister.IsConditional)); + } + } + + private IEnumerable GetOverlappingProducers(InstanceProducer producerToRegister) + { + return + from producer in this.CurrentProducers + where producer.ImplementationType != null + where !producer.Registration.WrapsInstanceCreationDelegate + where !producerToRegister.Registration.WrapsInstanceCreationDelegate + where producer.ImplementationType == producerToRegister.ImplementationType + select producer; + } + private ActivationException ThrowMultipleApplicableRegistrationsFound( InstanceProducer[] instanceProducers) { @@ -171,6 +180,15 @@ private void ThrowWhenConditionalAndUnconditionalAreMixed(InstanceProducer produ this.ThrowWhenNonGenericTypeAlreadyRegisteredAsConditionalRegistration(producer); } + private void ThrowWhenConditionalIsRegisteredInOverridingMode(InstanceProducer producer) + { + if (producer.IsConditional && this.container.Options.AllowOverridingRegistrations) + { + throw new NotSupportedException( + StringResources.MakingConditionalRegistrationsInOverridingModeIsNotSupported()); + } + } + private void ThrowWhenNonGenericTypeAlreadyRegisteredAsUnconditionalRegistration( InstanceProducer producer) { diff --git a/SimpleInjector.NET/StringResources.cs b/SimpleInjector.NET/StringResources.cs index e99d841de..28f3927a6 100644 --- a/SimpleInjector.NET/StringResources.cs +++ b/SimpleInjector.NET/StringResources.cs @@ -638,7 +638,7 @@ internal static string RegistrationForClosedServiceTypeOverlapsWithOpenGenericRe Helpers.ToCSharpFriendlyName(overlappingGenericImplementationType), nameof(Container.RegisterConditional)); - internal static string AnOverlappingGenericRegistrationExists(Type openGenericServiceType, + internal static string AnOverlappingRegistrationExists(Type openGenericServiceType, Type overlappingImplementationType, bool isExistingRegistrationConditional, Type implementationTypeOfNewRegistration, bool isNewRegistrationConditional) { diff --git a/changes.txt b/changes.txt index 641daa3ae..e3242acd1 100644 --- a/changes.txt +++ b/changes.txt @@ -1,7 +1,14 @@ Version [Version Number] ([Friendly Version Number]) [Release Date] -Version 3.0.3 +Version 3.0.4 + + Bug fixes: + -[Core] Overriding a registration failed when done with same implementation, and registration of + conditional non-generic registrations was accidentally allowed. (fixes #114) + + +Version 3.0.3 (v3.0.3 RTM) 2015-08-27 Bug fixes: -[Core] Container.Verify() does not dispose created instances, while running in an active scope.