Use TryAdd in AddXxx methods to only add services if not already pres…

…ent. Issue #956 and also see pull request #1129. This change updates all of our AddXxx methods to use TryAdd when adding to the service collection such that only services that are not already registered will be registered. This also means that the code we had to check the service collection for "hosting" services is no longer needed. Note that services for which it is expected that multiple instances can be registered (e.g. listeners and data sources) must not use TryAdd since otherwise only the first registered will be used.
dotnet · Dec 2, 2014 · a365c55 · a365c55
1 parent 134d72e
commit a365c55
Show file tree

Hide file tree

Showing 16 changed files with 252 additions and 85 deletions.
diff --git a/src/EntityFramework.InMemory/Extensions/InMemoryEntityServicesBuilderExtensions.cs b/src/EntityFramework.InMemory/Extensions/InMemoryEntityServicesBuilderExtensions.cs
@@ -19,16 +19,17 @@ public static EntityServicesBuilder AddInMemoryStore([NotNull] this EntityServic
             Check.NotNull(builder, "builder");
 
             builder.ServiceCollection
-                .AddSingleton<InMemoryValueGeneratorCache>()
-                .AddSingleton<InMemoryValueGeneratorSelector>()
-                .AddSingleton<SimpleValueGeneratorFactory<InMemoryValueGenerator>>()
-                .AddSingleton<InMemoryDatabase>()
                 .AddScoped<DataStoreSource, InMemoryDataStoreSource>()
-                .AddScoped<InMemoryDataStoreServices>()
-                .AddScoped<InMemoryDatabaseFacade>()
-                .AddScoped<InMemoryDataStore>()
-                .AddScoped<InMemoryConnection>()
-                .AddScoped<InMemoryDataStoreCreator>();
+                .TryAdd(new ServiceCollection()
+                    .AddSingleton<InMemoryValueGeneratorCache>()
+                    .AddSingleton<InMemoryValueGeneratorSelector>()
+                    .AddSingleton<SimpleValueGeneratorFactory<InMemoryValueGenerator>>()
+                    .AddSingleton<InMemoryDatabase>()
+                    .AddScoped<InMemoryDataStoreServices>()
+                    .AddScoped<InMemoryDatabaseFacade>()
+                    .AddScoped<InMemoryDataStore>()
+                    .AddScoped<InMemoryConnection>()
+                    .AddScoped<InMemoryDataStoreCreator>());
 
             return builder;
         }

diff --git a/src/EntityFramework.Migrations/MigrationsEntityServicesBuilderExtensions.cs b/src/EntityFramework.Migrations/MigrationsEntityServicesBuilderExtensions.cs
@@ -17,9 +17,10 @@ public static EntityServicesBuilder AddMigrations([NotNull] this EntityServicesB
 
             builder
                 .AddRelational().ServiceCollection
-                .AddScoped<MigrationAssembly>()
-                .AddScoped<HistoryRepository>()
-                .AddScoped(MigrationsDataStoreServices.MigratorFactory);
+                .TryAdd(new ServiceCollection()
+                    .AddScoped<MigrationAssembly>()
+                    .AddScoped<HistoryRepository>()
+                    .AddScoped(MigrationsDataStoreServices.MigratorFactory));
 
             return builder;
         }

diff --git a/src/EntityFramework.Relational/RelationalEntityServicesBuilderExtensions.cs b/src/EntityFramework.Relational/RelationalEntityServicesBuilderExtensions.cs
@@ -18,11 +18,11 @@ public static EntityServicesBuilder AddRelational([NotNull] this EntityServicesB
         {
             Check.NotNull(builder, "builder");
 
-            builder.ServiceCollection
+            builder.ServiceCollection.TryAdd(new ServiceCollection()
                 .AddSingleton<RelationalObjectArrayValueReaderFactory>()
                 .AddSingleton<RelationalTypedValueReaderFactory>()
                 .AddSingleton<ParameterNameGeneratorFactory>()
-                .AddSingleton<ModificationCommandComparer>();
+                .AddSingleton<ModificationCommandComparer>());
 
             return builder;
         }

diff --git a/src/EntityFramework.SqlServer/Extensions/SqlServerEntityServicesBuilderExtensions.cs b/src/EntityFramework.SqlServer/Extensions/SqlServerEntityServicesBuilderExtensions.cs
@@ -4,7 +4,7 @@
 using JetBrains.Annotations;
 using Microsoft.Data.Entity;
 using Microsoft.Data.Entity.Identity;
-using Microsoft.Data.Entity.Migrations.Infrastructure;
+using Microsoft.Data.Entity.Migrations;
 using Microsoft.Data.Entity.Relational;
 using Microsoft.Data.Entity.SqlServer;
 using Microsoft.Data.Entity.SqlServer.Metadata;
@@ -22,31 +22,30 @@ public static EntityServicesBuilder AddSqlServer([NotNull] this EntityServicesBu
         {
             Check.NotNull(builder, "builder");
 
-            builder.AddRelational().ServiceCollection
-                .AddSingleton<SqlServerValueGeneratorCache>()
-                .AddSingleton<SqlServerValueGeneratorSelector>()
-                .AddSingleton<SimpleValueGeneratorFactory<SequentialGuidValueGenerator>>()
-                .AddSingleton<SqlServerSequenceValueGeneratorFactory>()
-                .AddSingleton<SqlServerSqlGenerator>()
-                .AddSingleton<SqlStatementExecutor>()
-                .AddSingleton<SqlServerTypeMapper>()
-                .AddSingleton<SqlServerModificationCommandBatchFactory>()
-                .AddSingleton<SqlServerCommandBatchPreparer>()
-                .AddSingleton<SqlServerMetadataExtensionProvider>()
-                .AddSingleton<SqlServerMigrationOperationFactory>()
-                .AddScoped<SqlServerBatchExecutor>()
+            builder.AddMigrations().ServiceCollection
                 .AddScoped<DataStoreSource, SqlServerDataStoreSource>()
-                .AddScoped<SqlServerDataStoreServices>()
-                .AddScoped<SqlServerDataStore>()
-                .AddScoped<SqlServerConnection>()                
-                .AddScoped<SqlServerMigrationOperationProcessor>()
-                .AddScoped<SqlServerModelDiffer>()
-                .AddScoped<SqlServerDatabase>()
-                .AddScoped<SqlServerMigrationOperationSqlGeneratorFactory>()
-                .AddScoped<SqlServerDataStoreCreator>()
-                .AddScoped<MigrationAssembly>()
-                .AddScoped<HistoryRepository>()
-                .AddScoped<SqlServerMigrator>();
+                .TryAdd(new ServiceCollection()
+                    .AddSingleton<SqlServerValueGeneratorCache>()
+                    .AddSingleton<SqlServerValueGeneratorSelector>()
+                    .AddSingleton<SimpleValueGeneratorFactory<SequentialGuidValueGenerator>>()
+                    .AddSingleton<SqlServerSequenceValueGeneratorFactory>()
+                    .AddSingleton<SqlServerSqlGenerator>()
+                    .AddSingleton<SqlStatementExecutor>()
+                    .AddSingleton<SqlServerTypeMapper>()
+                    .AddSingleton<SqlServerModificationCommandBatchFactory>()
+                    .AddSingleton<SqlServerCommandBatchPreparer>()
+                    .AddSingleton<SqlServerMetadataExtensionProvider>()
+                    .AddSingleton<SqlServerMigrationOperationFactory>()
+                    .AddScoped<SqlServerBatchExecutor>()
+                    .AddScoped<SqlServerDataStoreServices>()
+                    .AddScoped<SqlServerDataStore>()
+                    .AddScoped<SqlServerConnection>()
+                    .AddScoped<SqlServerMigrationOperationProcessor>()
+                    .AddScoped<SqlServerModelDiffer>()
+                    .AddScoped<SqlServerDatabase>()
+                    .AddScoped<SqlServerMigrationOperationSqlGeneratorFactory>()
+                    .AddScoped<SqlServerDataStoreCreator>()
+                    .AddScoped<SqlServerMigrator>());
 
             return builder;
         }

diff --git a/src/EntityFramework.SqlServer/SqlServerDataStore.cs b/src/EntityFramework.SqlServer/SqlServerDataStore.cs
@@ -1,6 +1,7 @@
 // Copyright (c) Microsoft Open Technologies, Inc. All rights reserved.
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
+using System;
 using JetBrains.Annotations;
 using Microsoft.Data.Entity.ChangeTracking;
 using Microsoft.Data.Entity.Infrastructure;
@@ -18,6 +19,15 @@ namespace Microsoft.Data.Entity.SqlServer
 {
     public class SqlServerDataStore : RelationalDataStore
     {
+        /// <summary>
+        ///     This constructor is intended only for use when creating test doubles that will override members
+        ///     with mocked or faked behavior. Use of this constructor for other purposes may result in unexpected
+        ///     behavior including but not limited to throwing <see cref="NullReferenceException" />.
+        /// </summary>
+        protected SqlServerDataStore()
+        {
+        }
+
         public SqlServerDataStore(
             [NotNull] StateManager stateManager,
             [NotNull] DbContextService<IModel> model,

diff --git a/src/EntityFramework/Extensions/EntityServiceCollectionExtensions.cs b/src/EntityFramework/Extensions/EntityServiceCollectionExtensions.cs
@@ -2,8 +2,6 @@
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
 using System;
-using System.Collections.Generic;
-using System.Linq;
 using JetBrains.Annotations;
 using Microsoft.Data.Entity;
 using Microsoft.Data.Entity.ChangeTracking;
@@ -31,7 +29,13 @@ public static EntityServicesBuilder AddEntityFramework(
         {
             Check.NotNull(serviceCollection, "serviceCollection");
 
+            // TODO: Is this the appropriate way to register listeners?
             serviceCollection
+                .AddScoped<IEntityStateListener>(p => p.GetService<NavigationFixer>())
+                .AddScoped<IRelationshipListener>(p => p.GetService<NavigationFixer>())
+                .AddScoped<IPropertyListener>(p => p.GetService<ChangeDetector>());
+
+            serviceCollection.TryAdd(new ServiceCollection()
                 .AddSingleton<IModelSource, DefaultModelSource>()
                 .AddSingleton<ModelBuilderFactory>()
                 .AddSingleton<SimpleValueGeneratorFactory<TemporaryValueGenerator>>()
@@ -58,10 +62,6 @@ public static EntityServicesBuilder AddEntityFramework(
                 .AddScoped<StateEntryFactory>()
                 .AddScoped<NavigationFixer>()
                 .AddScoped<ChangeDetector>()
-                // TODO: Is this the appropriate way to register listeners?
-                .AddScoped<IEntityStateListener>(p => p.GetService<NavigationFixer>())
-                .AddScoped<IRelationshipListener>(p => p.GetService<NavigationFixer>())
-                .AddScoped<IPropertyListener>(p => p.GetService<ChangeDetector>())
                 .AddScoped<StateEntryNotifier>()
                 .AddScoped<StateEntrySubscriber>()
                 .AddScoped<DbContextServices>()
@@ -77,71 +77,40 @@ public static EntityServicesBuilder AddEntityFramework(
                 .AddScoped(DataStoreServices.ConnectionFactory)
                 .AddScoped(DataStoreServices.DatabaseFactory)
                 .AddScoped(DataStoreServices.ValueGeneratorCacheFactory)
-                .AddScoped(DataStoreServices.DataStoreCreatorFactory);
-
-            EnsureLowLevelServices(serviceCollection);
+                .AddScoped(DataStoreServices.DataStoreCreatorFactory)
+                .AddSingleton<ILoggerFactory, LoggerFactory>()
+                .AddTypeActivator()
+                .AddOptions());
 
             return new EntityServicesBuilder(serviceCollection, configuration);
         }
 
-        private static void EnsureLowLevelServices(IServiceCollection serviceCollection)
-        {
-            var requiredServices = new List<Tuple<Type, Action<IServiceCollection>>>
-                {
-                    Tuple.Create<Type, Action<IServiceCollection>>(typeof(ILoggerFactory), c => c.AddSingleton<ILoggerFactory, LoggerFactory>()),
-                    Tuple.Create<Type, Action<IServiceCollection>>(typeof(ITypeActivator), c => c.AddTypeActivator()),
-                    Tuple.Create<Type, Action<IServiceCollection>>(typeof(IOptions<>), c => c.AddOptions()),
-                };
-
-            foreach (var descriptor in serviceCollection)
-            {
-                foreach (var serviceTuple in requiredServices)
-                {
-                    if (serviceTuple.Item1 == descriptor.ServiceType)
-                    {
-                        requiredServices.Remove(serviceTuple);
-                        break;
-                    }
-                }
-
-                if (!requiredServices.Any())
-                {
-                    break;
-                }
-            }
-
-            foreach (var serviceTuple in requiredServices)
-            {
-                serviceTuple.Item2(serviceCollection);
-            }
-        }
-
         public static EntityServicesBuilder AddDbContext<TContext>(
             [NotNull] this EntityServicesBuilder builder,
             [CanBeNull] Action<DbContextOptions> optionsAction = null)
             where TContext : DbContext
         {
             Check.NotNull(builder, "builder");
 
-            builder.ServiceCollection.AddSingleton<DbContextOptions<TContext>>(
+            builder.ServiceCollection.AddSingleton(
                 sp => sp.GetRequiredServiceChecked<IOptions<DbContextOptions<TContext>>>().Options);
 
             if (builder.Configuration != null)
             {
                 // TODO: Allows parser to be obtained from service provider. Issue #947
                 builder.ServiceCollection.ConfigureOptions(
                     new DbContextConfigureOptions<TContext>(builder.Configuration, new DbContextOptionsParser())
-                    {
-                        Order = ConfigurationOrder
-                    });
+                        {
+                            Order = ConfigurationOrder
+                        });
             }
 
             if (optionsAction != null)
             {
                 builder.ServiceCollection.Configure<DbContextOptions<TContext>>(optionsAction);
             }
 
-            builder.ServiceCollection.AddScoped(typeof(TContext), DbContextActivator.CreateInstance<TContext>);
+            ServiceCollectionExtensions.AddScoped(builder.ServiceCollection, typeof(TContext), (Func<IServiceProvider, object>)(DbContextActivator.CreateInstance<TContext>));
 
             return builder;
         }

diff --git a/test/EntityFramework.InMemory.Tests/InMemoryEntityServicesBuilderExtensionsTest.cs b/test/EntityFramework.InMemory.Tests/InMemoryEntityServicesBuilderExtensionsTest.cs
@@ -6,6 +6,7 @@
 using Microsoft.Data.Entity.Storage;
 using Microsoft.Data.Entity.Tests;
 using Microsoft.Framework.DependencyInjection;
+using Microsoft.Framework.DependencyInjection.Fallback;
 using Xunit;
 
 namespace Microsoft.Data.Entity.InMemory.Tests
@@ -33,5 +34,22 @@ public void Services_wire_up_correctly()
                 Assert.NotNull(scopedProvider.GetRequiredService<DataStoreSource>());
             }
         }
+
+        [Fact]
+        public void AddInMemoryStore_does_not_replace_services_already_registered()
+        {
+            var services = new ServiceCollection()
+                .AddSingleton<InMemoryDataStore, FakeInMemoryDataStore>();
+
+            services.AddEntityFramework().AddInMemoryStore();
+
+            var serviceProvider = services.BuildServiceProvider();
+
+            Assert.IsType<FakeInMemoryDataStore>(serviceProvider.GetRequiredService<InMemoryDataStore>());
+        }
+
+        private class FakeInMemoryDataStore : InMemoryDataStore
+        {
+        }
     }
 }
diff --git a/test/EntityFramework.Migrations.Tests/EntityFramework.Migrations.Tests.csproj b/test/EntityFramework.Migrations.Tests/EntityFramework.Migrations.Tests.csproj
@@ -88,6 +88,7 @@
     <Compile Include="MigrationOperationFactoryTest.cs" />
     <Compile Include="MigrationOperationSqlGeneratorTest.cs" />
     <Compile Include="MigrationsDatabaseExtensionsTest.cs" />
+    <Compile Include="MigrationsEntityServicesBuilderExtensionsTest.cs" />
     <Compile Include="MigrationTest.cs" />
     <Compile Include="ModelDifferTest.cs" />
     <Compile Include="Builders\ColumnBuilderTest.cs" />

diff --git a/test/EntityFramework.Migrations.Tests/MigrationsEntityServicesBuilderExtensionsTest.cs b/test/EntityFramework.Migrations.Tests/MigrationsEntityServicesBuilderExtensionsTest.cs
@@ -0,0 +1,30 @@
+// Copyright (c) Microsoft Open Technologies, Inc. All rights reserved.
+// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
+
+using Microsoft.Data.Entity.Migrations.Infrastructure;
+using Microsoft.Framework.DependencyInjection;
+using Microsoft.Framework.DependencyInjection.Fallback;
+using Xunit;
+
+namespace Microsoft.Data.Entity.Migrations.Tests
+{
+    public class MigrationsEntityServicesBuilderExtensionsTest
+    {
+        [Fact]
+        public void AddMigrations_does_not_replace_services_already_registered()
+        {
+            var services = new ServiceCollection()
+                .AddSingleton<MigrationAssembly, FakeMigrationAssembly>();
+
+            services.AddEntityFramework().AddMigrations();
+
+            var serviceProvider = services.BuildServiceProvider();
+
+            Assert.IsType<FakeMigrationAssembly>(serviceProvider.GetRequiredService<MigrationAssembly>());
+        }
+
+        private class FakeMigrationAssembly : MigrationAssembly
+        {
+        }
+    }
+}
diff --git a/test/EntityFramework.Redis.Tests/RedisEntityServicesBuilderExtensionsTest.cs b/test/EntityFramework.Redis.Tests/RedisEntityServicesBuilderExtensionsTest.cs
@@ -0,0 +1,29 @@
+// Copyright (c) Microsoft Open Technologies, Inc. All rights reserved.
+// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
+
+using Microsoft.Framework.DependencyInjection;
+using Microsoft.Framework.DependencyInjection.Fallback;
+using Xunit;
+
+namespace Microsoft.Data.Entity.Redis.Tests
+{
+    public class RedisEntityServicesBuilderExtensionsTest
+    {
+        [Fact]
+        public void AddRedis_does_not_replace_services_already_registered()
+        {
+            var services = new ServiceCollection()
+                .AddSingleton<RedisDataStore, FakeRedisDataStore>();
+
+            services.AddEntityFramework().AddRedis();
+
+            var serviceProvider = services.BuildServiceProvider();
+
+            Assert.IsType<FakeRedisDataStore>(serviceProvider.GetRequiredService<RedisDataStore>());
+        }
+
+        private class FakeRedisDataStore : RedisDataStore
+        {
+        }
+    }
+}
diff --git a/test/EntityFramework.Relational.Tests/EntityFramework.Relational.Tests.csproj b/test/EntityFramework.Relational.Tests/EntityFramework.Relational.Tests.csproj
@@ -86,6 +86,7 @@
     <Compile Include="RelationalDatabaseExtensionsTest.cs" />
     <Compile Include="RelationalDatabaseTest.cs" />
     <Compile Include="RelationalDataStoreTest.cs" />
+    <Compile Include="RelationalEntityServicesBuilderExtensionsTest.cs" />
     <Compile Include="RelationalObjectArrayValueReaderFactoryTest.cs" />
     <Compile Include="RelationalObjectArrayValueReaderTest.cs" />
     <Compile Include="RelationalTypedValueReaderFactoryTest.cs" />

diff --git a/test/EntityFramework.Relational.Tests/RelationalEntityServicesBuilderExtensionsTest.cs b/test/EntityFramework.Relational.Tests/RelationalEntityServicesBuilderExtensionsTest.cs
@@ -0,0 +1,30 @@
+// Copyright (c) Microsoft Open Technologies, Inc. All rights reserved.
+// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
+
+using Microsoft.Data.Entity.Relational.Update;
+using Microsoft.Framework.DependencyInjection;
+using Microsoft.Framework.DependencyInjection.Fallback;
+using Xunit;
+
+namespace Microsoft.Data.Entity.Relational.Tests
+{
+    public class RelationalEntityServicesBuilderExtensionsTest
+    {
+        [Fact]
+        public void AddRelational_does_not_replace_services_already_registered()
+        {
+            var services = new ServiceCollection()
+                .AddSingleton<ModificationCommandComparer, FakeModificationCommandComparer>();
+
+            services.AddEntityFramework().AddRelational();
+
+            var serviceProvider = services.BuildServiceProvider();
+
+            Assert.IsType<FakeModificationCommandComparer>(serviceProvider.GetRequiredService<ModificationCommandComparer>());
+        }
+
+        private class FakeModificationCommandComparer : ModificationCommandComparer
+        {
+        }
+    }
+}