Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduce magic in the interaction of EF Core and DI #4668

Closed
divega opened this issue Feb 29, 2016 · 6 comments
Closed

Reduce magic in the interaction of EF Core and DI #4668

divega opened this issue Feb 29, 2016 · 6 comments
Assignees
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-unknown
Milestone

Comments

@divega
Copy link
Contributor

divega commented Feb 29, 2016

TL;DR

This is a proposal to remove the magic that allows derived DbContext types with default or parameter-less constructors to be used with DI. The general idea is to embrace the constructor injection pattern when using DI so that it becomes very clear what services are being injected and also so that the non-DI usages can be clearly differentiated.

Details

Currently we allow derived DbContexts that are supposed to be resolved from DI (e.g. the default application context in an ASP.NET Core application) to have parameter-less constructors. We actually went out of our way to enable this through some complex logic in DbContextActivator and the AddDbContext() method.

While this allows removing a lot of boilerplate code from derived DbContext constructors, it also has some undesired consequences:

  1. It is confusing: In the previews we have seen many customers mixing up the various patterns for setting the context options with DI and without DI (i.e. using the OnConfiguring() method) and then creating the context instance without DI (i.e. using the new keyword) or with DI, which in the worst cases will fail, and in some of the better cases may cause performance issues.
  2. The magic doesn't reflect or play well with the patterns customers actually need to learn in order to interact with the DI system correctly in the rest of their application.
  3. The resulting constructors are completely opaque about which application services the DbContext depends on (and everything downstream needs to use service locator pattern), let alone the fact that it is sharing the application's IServiceProvider to resolve its own internal services.

We have started brainstorming and we are going to continue discussing some ideas on how to improve this. Here are some initial thoughts around the alternative of removing the magic. The main disadvantage of this approach is that derived DbContext will need to define a constructor to be used with DI (until now we were getting away with default constructors). It seems that the level of control and clarity about what is going on you gain might be worth the pain though.

The changes would involve all of the following:

  1. Get rid of the magic: Remove DbContextActivator and the delegate registration that uses it in AddDbContext().
  2. Expand the DbContext constructors to include arguments for all the shared application services EF would use within a DbContext, including logging, diagnostics and memory cache.
  3. Keep the DbContextOptions argument to be able to resolve context options that are configured externally.
  4. Keep the IServiceProvider argument for cases in which a specific service provider (could be the application’s one or a different one) is configured externally.
  5. Keep the AddDbContext() method but do less magic in it, e.g. it should just be a shortcut for setting up the context as a scoped service and to setup its options. Also consider extending AddDbContext() to make it easier for customers to bring their own derived DbContextOptions type and get those injected instead of the default DbContextOptions or DbContextOptions<TContext> (perhaps an extra overload with a second generic argument could be enough).
  6. Choose what constructor we will have in ASP.NET Core templates and in documentation for the different scenarios. We should make sure Identity’s derived DbContext also expands its constructors to include the necessary services.

Some extra potential benefits of this change would be:

  1. The constructor becomes the truth about what services are getting injected, and even non-DI and hybrid scenarios become clearer.
  2. It can be a stepping stone for us to separate the mechanism used to resolve internal services from the application's DI container.
  3. Users become free to pass arbitrary services that the derived DbContext constructor, including any services necessary to flow configuration details into the instances.
@OlegKi
Copy link

OlegKi commented Mar 1, 2016

👍 The suggested changes are really good!

@Muchiachio
Copy link

@divega maybe this would fix #4558? I could close that one as duplicate.

@ajcvickers
Copy link
Member

@Muchiachio Yes, if we do this it should fix that issue. This was one of the motivating factors behind the discussion that led to this item being created. However, please leave that issue open for now because we aren't fully decided on the complete direction to take.

@rowanmiller rowanmiller added this to the 1.0.0 milestone Mar 2, 2016
@rowanmiller rowanmiller modified the milestones: 1.0.0-rc2, 1.0.0 Mar 2, 2016
@ajcvickers
Copy link
Member

I have done some investigation and prototyping and have the following proposal:

DbContext would have four constructors:

protected DbContext(
    [CanBeNull] ILoggerFactory loggerFactory = null,
    [CanBeNull] IMemoryCache memoryCache = null)

This constructor:

  • Must be used together with a derived context because OnConfiguring must be called to get options
  • Creates/caches an service provider separate from the application service provider for EFs internal services and scoping
  • Application level services used by EF can be optionally passed in
    • If this constructor is used with D.I. (such as when using AddDbContext, see below, then these services will be injected from the application service provider
    • If a service is not passed in (null), then EF will create and manage the service on behalf the application
protected DbContext(
    [NotNull] IServiceProvider internalServicesProvider
    [CanBeNull] ILoggerFactory loggerFactory = null,
    [CanBeNull] IMemoryCache memoryCache = null)

This constructor:

  • Is the same as the previous constructor, except that the application can pass in the service provider used by EF for its internal services
    • This can be used when the app wants to replace EF services or manage the lifetime of the service provider
    • This could be the application service provider, but usually probably should not be
public DbContext(
    [NotNull] DbContextOptions options, 
    [CanBeNull] ILoggerFactory loggerFactory = null,
    [CanBeNull] IMemoryCache memoryCache = null)

This constructor:

  • Doesn't require a derviced context because options are passed in. Otherwise works the same as the first constructor.
  • DbContextOptions could be ibjected from the application service provider, such as when using AddDbContext
  • This is the constructor we would use with the ASP.NET templates
public DbContext(
    [NotNull] IServiceProvider internalServicesProvider)
    [NotNull] DbContextOptions options, 
    [CanBeNull] ILoggerFactory loggerFactory = null,
    [CanBeNull] IMemoryCache memoryCache = null)

This constructor:

  • Is the same as the previous constructor, but app passes in the provider for EF internal services

AddDbContext would now hang directly off ServcieCollection:

Before:

services
    .AddEntityFramework()
        .AddSqlServer()
        .AddDbContext<BloggingContext>(options => options.UseSqlServer(connection));

Now:

services
    .AddDbContext<BloggingContext>(options => options.UseSqlServer(connection));

AddDbContext will now not add any internal EF services to the application D.I. container. It will also not do any magic to get that service provider into the context. Instead it will just:

  • Add the application's DBContext to the application service provider
    • Probably by default as scoped, but this is really entirely up to the app now
  • Add DbContextOptions<TContext> to the application's service provider
  • Add DbContextOptions to the application's service provider (Can be used when the app only has one DbContext)

We should probably move the AddEntityFramework and AddMyProvider methods to a different namespace since most of the time they will not be used.

Notes:

  • We will need to make sure that memory cache and logging are added somewhere in Startup.cs, but I think this happens now anyway.
  • If the app explicitly passes in caching and logging, then the app is responsible for appropriate scoping

@rowanmiller
Copy link
Contributor

👍

@bricelam
Copy link
Contributor

bricelam commented Mar 4, 2016

We also need to be cognizant of how our design-time will work. Currently, AddDbContext<TContext> does essentially the following.

serviceCollection.AddSingleton<DbContextOptions, DbContextOptions<TContext>>();

Then at design-time, we get the list of DbContext types like this:

var contextTypes = from options in serviceProvider.GetServices<DbContextOptions>()
                   let genericOptions = options.GetType() // DbContextOptions<TContext>
                   select genericOptions.GenericTypeArguments[0]; // TContext

ajcvickers added a commit that referenced this issue Mar 8, 2016
See issue #4668 for full description of changes.

This change removes EntityFrameworkServicesBuilder since it is no longer needed for AddDbContext and moves all EF methods that add services to IServiceCollection. Providers will need to react to this by moving AddMyProvider methods to be extension methods of IServiceCollection.
ajcvickers added a commit that referenced this issue Mar 10, 2016
See issue #4668. This is an update following a discussion yesterday that moves all configuration onto DbContextOptions and makes this the only object passed to the DbContext constructor. This allows the builder pattern that already exists for DbContextOptions to be used for external services and for setting the internal service provider. AddDbContext does this automatically for external services, but a DbContextOptions object can also be easily built and either configured in DI or passed directly to the context with new.

In addition, the UseMyProvider methods have been updated to use nested closure for provider-specific configuration. This makes for a very nice chaining story.

There is a new overload of AddDbContext that makes the application service provider available to teh configuration delegate. This is rarely needed, but one use is to set the application service provider to be used as the internal EF service provider if this is needed/desired.

Still to do:
- Look at negative cases when services are not setup and create better exception messages.
- Rename AddSqlServer, etc to AddEntityFrameworkSqlServer, etc.
- Extract diagnostics as one of the external services on DbContextOptions.
ajcvickers added a commit that referenced this issue Mar 10, 2016
See issue #4668 for full description of changes.

This change removes EntityFrameworkServicesBuilder since it is no longer needed for AddDbContext and moves all EF methods that add services to IServiceCollection. Providers will need to react to this by moving AddMyProvider methods to be extension methods of IServiceCollection.
ajcvickers added a commit that referenced this issue Mar 10, 2016
See issue #4668. This is an update following a discussion yesterday that moves all configuration onto DbContextOptions and makes this the only object passed to the DbContext constructor. This allows the builder pattern that already exists for DbContextOptions to be used for external services and for setting the internal service provider. AddDbContext does this automatically for external services, but a DbContextOptions object can also be easily built and either configured in DI or passed directly to the context with new.

In addition, the UseMyProvider methods have been updated to use nested closure for provider-specific configuration. This makes for a very nice chaining story.

There is a new overload of AddDbContext that makes the application service provider available to teh configuration delegate. This is rarely needed, but one use is to set the application service provider to be used as the internal EF service provider if this is needed/desired.

Still to do:
- Look at negative cases when services are not setup and create better exception messages.
- Rename AddSqlServer, etc to AddEntityFrameworkSqlServer, etc.
- Extract diagnostics as one of the external services on DbContextOptions.
@ajcvickers ajcvickers removed this from the 1.0.0-rc2 milestone Oct 15, 2022
@ajcvickers ajcvickers added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Oct 15, 2022
@ajcvickers ajcvickers added this to the 1.0.0 milestone Oct 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-unknown
Projects
None yet
Development

No branches or pull requests

6 participants