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

Start steering users away from AddDbContext/OnConfiguring #35481

Open
roji opened this issue Jan 15, 2025 · 9 comments
Open

Start steering users away from AddDbContext/OnConfiguring #35481

roji opened this issue Jan 15, 2025 · 9 comments

Comments

@roji
Copy link
Member

roji commented Jan 15, 2025

tl;dr start moving users towards factory-based patterns for configuring DbContext, to stop relying on EF's internal service provider cache to resolve the right service provider & singleton resources. This is important for supporting Cosmos, PostgreSQL, and possibly other providers.

When users specify options via AddDbContext/OnConfiguring, the lambda they provide for setting up the options gets executed repeatedly, possibly yielding different options for different contexts. While this can be useful in some scenarios (e.g. specifying different connection strings for different tenants), it also creates significant issues. To properly support this, EF manages an internal cache keyed on the options, so that the same service provider can be used if (and only if) singleton options do not have different values. This in turn requires EF to always be able to compare the options when doing cache lookups; but providers have arbitrary option configuration which may not be comparable at all: Cosmos has CosmosClientOptions, EFCore.PG has ConfigureDataSource() (accepting a lambda to configure the data source), etc.

We've discussed this, an there seems to be general agreement that it would be best to start moving away from these problematic configuration mechanisms, and towards factory-based DbContext instantiation, where the options are calculated only once, and then used for all contexts.

  • Currently, the DI AddPooledDbContextFactory only registers a factory (though unpooled AddDbContextFactory does register both), making it harder to use (controllers can't get injected with a DbContext directly - not good). #35484 tracks registering both DbContext and IDbContextFactory in DI - I think that should be a prerequisite to this, to ensure a good experience.
  • I think this means we end up discouraging AddDbContext, and recommending users always use AddDbContextFactory; this is slightly odd as users don't necessarily want to be injected with AddDbContextFactory - we only want them to use a configuration mechanism which computes the options once. Maybe introducing a new method with a nicer name would be better (??).
  • We could think about this as a docs/guidance-only transition, or something more; for example, we could start with a warning when AddDbContext/OnConfiguring are used, gradually transition to obsoleting them, and then possibly consider if we want to actually remove them.
  • Once this is all done, we could optionally also disable the service provider cache when the context factory is used; the factory would directly "own" the service provider, and any singleton resources inside it.
  • Users who do need to vary options per context (e.g. connection string per tenant) would ideally have some mechanism for managing multiple IDbContextFactories - one per tenant. A DI registry for this, mapping the tenant ID to an IDbContextFactory, could make this experience good.
  • Note that this whole issue is essentially the same as the ADO.NET connection string mess, where users instantiate a DbConnection with a connection string, and the driver internally manages a cache of connection pools. DbDataSource was introduced to ADO.NET to solve this problem, and is analogous to IDbContextFactory (basically an explicit source of the objects - DbContext or DbConnection - removing the need for any sort of internal lookup/caching).

Note: some of the above was already previously discussed in #29597.

/cc @NinoFloris with whom I've discussed this in past.

@stevendarby
Copy link
Contributor

Currently, the DI AddDbContextFactory (and pooling version) only registers a factory

According to the documentation, AddDbContextFactory does register the context as a scoped service. I think it's just the pooling version that doesn't.

The factory extension allows registering a scoped factory too, which I think would have the same problems, so this would be about guiding people towards singleton factories specifically.

@roji
Copy link
Member Author

roji commented Jan 15, 2025

Thanks for that @stevendarby, sounds right. It also makes this more difficult.

@AndriySvyryd
Copy link
Member

Maybe introducing a new method with a nicer name would be better (??).

We already have ConfigureDbContext that registers options as a singleton by default, so we could discourage/obsolete only the AddDbContext overloads that take an options lambda

@roji
Copy link
Member Author

roji commented Jan 16, 2025

Makes sense @AndriySvyryd.

@stevendarby's re your comment above, looking at the source code, unless I'm missing something, AddDbContextFactory does register the factory with a Singleton lifetime by default - which documentation do you see which says it does Scoped?

But regardless, you're right that if a user uses AddDbContextFactory but explicitly passes Scoped/Transient, they're effectively in the same situation as AddDbContext/OnConfiguring, since the options lambda will get reevaluated multiple times. We'd need to think about possibly discouraging that too...

@stevendarby
Copy link
Contributor

stevendarby commented Jan 16, 2025

@roji the documentation (in the remarks) says the context is registered as scoped, counter to your point that it only registers a factory.

@roji
Copy link
Member Author

roji commented Jan 16, 2025

Ah you're right - I see now (docs):

For convenience, this method also registers the context type itself as a scoped service. This allows a context instance to be resolved from a dependency injection scope directly or created by the factory, as appropriate.

I've also confirmed that this is actually the case in the code. This indeed makes things more difficult. However, if I'm reading the code right, the options are still registered as singleton, so although the DbContext's lifetime is Scoped (which it must be), the options are only calculated once - which is what we want.

Unless I'm mistaken, that means that #26528 is effectively already implemented - users can simply use AddDbContextFactory and get both factory and context from DI (singleton factory + options, scoped context).

@stevendarby
Copy link
Contributor

stevendarby commented Jan 16, 2025

@roji I thought your point (first bullet point) was that it'd be a pre-requisite for this work. I was just pointing out it's already done for the regular factory method, just not the pooled one yet. Sorry if I'm misunderstanding though, I think I'm confusing matters more than help clarify them!

@stevendarby
Copy link
Contributor

@roji, just read your edit :)

#26528 is still needed because it's for the pooled factory.

@roji
Copy link
Member Author

roji commented Jan 16, 2025

@stevendarby yep! That's being done in #35484 (PR out).

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