dotnet / efcore

EF Core is a modern object-database mapper for .NET. It supports LINQ queries, change tracking, updates, and schema migrations.
https://docs.microsoft.com/ef/
MIT License
13.73k stars 3.18k forks source link

Using ICurrentDbContext to access the current context during migrations #9302

Closed rmja closed 2 years ago

rmja commented 7 years ago

I am using a custom SqlServerMigrationsSqlGenerator to handle multi tenant migrations. In 1.1 it was possible to let this custom generator depend on ICurrentDbContext. I could then determine the name of the tenant from a string property on the context, and handle sql generation operations appropriately.

In 2.0, ICurrentDbContext resolves to null making it impossible to determine the name of the tenant when handling migrations.

Steps to reproduce

Create a custom migrations sql generator:

public class CustomSqlServerMigrationsSqlGenerator : SqlServerMigrationsSqlGenerator
    {
        public CustomSqlServerMigrationsSqlGenerator(MigrationsSqlGeneratorDependencies dependencies, IMigrationsAnnotationProvider migrationsAnnotations, ICurrentDbContext currentDbContextAccessor)
            : base(dependencies, migrationsAnnotations)
        {
            // currentDbContextAccessor is null
        }

        // The currentDbContextAccessor.Context is needed here to correctly handle migrations (the name of the tenant is needed)
    }

and register it with

.AddDbContext<Context>(options => options.ReplaceService<IMigrationsSqlGenerator, CustomSqlServerMigrationsSqlGenerator>())

Further technical details

EF Core version: 2.0-preview2 Database Provider: Microsoft.EntityFrameworkCore.SqlServer Operating system: Windows 10 IDE: Visual Studio 2017

divega commented 7 years ago

We wonder if there should be a way to ask the SQL Server migrations SQL generator to be scoped if customers want to be able to resolve scope services while using them.

rmja commented 7 years ago

@divega Do you have a suggestion for a temporary workaround?

divega commented 7 years ago

@rmja we wanted @ajcvickers to take a look but he won't be able to do it for a few days.

This is just a guess but I believe you could try re-registering SqlServerMigrationsSqlGenerator in DI as IMigrationsSqlGenerator but with a scoped lifetime.

Another approach would be to make the model tenant specific, e.g. adding an annotation with the tenant id to the model, which is already taken as a method parameter in IMigrationsSqlGenerator.Generate().

rmja commented 7 years ago

Oh yes, this seems to work:

var services = new ServiceCollection()
                .AddEntityFrameworkSqlServer()
                .AddScoped<IMigrationsSqlGenerator, CustomSqlServerMigrationsSqlGenerator>() // Scoped instead of singleton
                .BuildServiceProvider();

            builder
                .UseSqlServer(connectionString)
                .UseInternalServiceProvider(services);

Thanks

ajcvickers commented 7 years ago

Following up on @divega's comments, annotating the model would likely be the most idiomatic way of doing this.

Changing the scope of a service can be dangerous because it impacts any other service that depends on it. For example, if some singleton service is depending on IMigrationsSqlGenerator, then it will now not work correctly because a singleton service cannot depend on a scoped service. That being said, if there are no dependency issues like this, then making the service scoped is a reasonable solution.