Skip to content

Design: Allow open generic implementations of IDbContextFactory #8331

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

Closed
bricelam opened this issue Apr 29, 2017 · 6 comments
Closed

Design: Allow open generic implementations of IDbContextFactory #8331

bricelam opened this issue Apr 29, 2017 · 6 comments
Assignees

Comments

@bricelam
Copy link
Contributor

bricelam commented Apr 29, 2017

I'm still not sure if this is a good idea, but it sounds cool. Imagine the following.

class ServicesDbContextFactory<TContext> : IDbContextFactory<TContext>
    where TContext : DbContext
{
    public TContext Create(string[] args)
        => Program.BuildWebHost(args).Services.GetRequiredService<TContext>();
}

Then, any context you used .AddDbContext() on in ConfigureServices would light-up at design-time without additional code.

There is one big downside I can see to this: What if you don't want a particular context to use it? (e.g. you want it to use its own factory or just its default constructor.

@divega
Copy link
Contributor

divega commented Apr 29, 2017

A few more thoughts on this:

  • Generic factories don't help with discovery of DbContext types, i.e. in order to use them we need to have discovered the type of the context first. This is a reason to introduce a non-generic one.
  • Do generic factories need to be generic on DbContext or can they be on a derived type?
  • Having more than one factory that can be used for a given DbContext type is a problem. How do we decide? Options:
    • Just prefer a non-generic if available. Consider throwing if we find to generic ones.
    • Prefer the best match. Consider throwing if faced with two equally good matches. Similar to overload resolution rules.
  • The previous bullet point is a solution for the problem mentioned by @bricelam: how to opt out of the generic factory? Bi introducing a specialized one.

@divega
Copy link
Contributor

divega commented Apr 29, 2017

@bricelam in the snippet, the class is missing the generic argument 😸

@ajcvickers ajcvickers added this to the 2.0.0 milestone May 1, 2017
@divega
Copy link
Contributor

divega commented May 1, 2017

We discussed in triage that we may want to morph this interface into something that facilitates discovery as well as instantiation of DbContext types. As it stands DbContext types registered in the service container in an ASP.NET Core application but defined in assemblies other than the startup project wouldn't be discovered. And that is a wasted opportunity since in the implementation of Create() in the template we actually already grab the service container so we could perform discovery.

@divega
Copy link
Contributor

divega commented May 2, 2017

After further discussion today with @ajcvickers and @bricelam we came up with the idea that we could define an interface containing a method that returns the application's service provider for design time work.

An implementation for the default template in ASP.NET Core 2.0 would just return Program.BuildWebHost(args).Services. It should be easy to adapt it if the application uses a different pattern, e.g. if it uses the default pattern from 1.0 or if the pattern changes in the future.

This would enable EF Core to discover DbContext types added to the service collection with AddDbContext as well as the instantiation of the contexts at design time. It could also be used by any other tool that needs access to the services.

There several open questions around naming, where the interface should be defined and also where we want to drop the implementation in the template.

cc @DamianEdwards

@divega
Copy link
Contributor

divega commented May 3, 2017

Opened aspnet/DependencyInjection#524 which likely supersedes this issue. Clearing up milestone to decided if there is anything here we still want to pursue.

@ajcvickers
Copy link
Contributor

Closing based on proposal in previous comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants