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.68k stars 3.16k forks source link

Add hook to allow actions on the context after it is leased/returned from/to the pool #17086

Open MorenoGentili opened 5 years ago

MorenoGentili commented 5 years ago

Hello, this issue is somewhat related to #14625 but I'd like to suggest something slightly different.

Basically, I need global filter values to be resolved on a per-request basis, even if the DbContext instance comes from the pool. Please, let's not discuss performance gains here, like in #14625, and just assume my app DOES benefit from using the pool.

Here's my suggestion for a new overload of the HasQueryFilter API. First, I'd register a global filter in the OnModelCreating like this. Please note no actual value is provided here, but just a link between the TenantId property of an entity class and a string identifier such as "TenantId".

modelBuilder.Entity<Product>().HasQueryFilter(p => p.TenantId, "TenantId");

Then, each time a DbContext is needed (e.g. user sending a request), the identifier "TenantId" has to be resolved to an actual value. So, I would need to add a method to the DbContext to do just that. Maybe marked by an atttribute such as GlobalFilterResolverAttribute. Please note this method must be dependency injection enabled so I can get a reference to scoped services like the ITenantProvider here.

[GlobalFilterResolver("TenantId")]
protected object ResolveTenantId(ITenantProvider provider)
{
  return provider.GetTenantId();
}

What do you think? Thanks for your attention. Moreno

MorenoGentili commented 5 years ago

Or, at least when used with ASP.NET Core, it could work like this, with a generic HasQueryFilter method.

modelBuilder.Entity<Product>().HasQueryFilter<IMyResolver>(p => p.TenantId);

Where IMyResolver is one of my services deriving from an interface provided by EFCore, such as IGlobalValueResolver.

smitpatel commented 5 years ago

I fail to see any filter or predicate in any of above examples. A filter/predicate in linq is a func which returns bool like Func<T, bool>

ajcvickers commented 5 years ago

@MorenoGentili If a query filter references a property on the context instance, and if the correct value is set when the instance is resolved from D.I., then the query filter should work correctly. So it's not clear to us what your proposal is addressing.

MorenoGentili commented 5 years ago

@ajcvickers Please clarify this: when using DbContext pooling, is a DbContext instance reused across multiple HTTP requests? i.e. Is the OnModelCreating method invoked or not after a DbContext instance has been retrieved from the pool?

In my scenario, HTTP requests come from multiple tenants. I'm afraid a DbContext instance that's been configured with a global filter for TenantId=1 will get reused for another tenant. I get from this answer DbContext pooling should not be used in this scenario. https://stackoverflow.com/questions/54553678/ef-core-with-dbcontext-pool-and-multi-tenancy-database-per-tenant#answers-header

@smitpatel I know and that's why this is a feature request.

smitpatel commented 5 years ago

@MorenoGentili - If you plan to use query filter then where is the filter? As for your question, EF Core inject tenant values from current context being used regardless of which context called OnModelCreating code.

MorenoGentili commented 5 years ago

@smitpatel A filter would be automatically configured for me by EFCore. A simple filter such as entity => entity.TenantId == resolvedTenantIdValue. Or I would configure it like this.

modelBuilder.Entity<Product>().HasQueryFilter("TenantId");

And then

[GlobalFilterResolver("TenantId")]
protected Func<Entity, bool> ResolveTenantId(ITenantProvider provider)
{
  string tenantId = provider.GetTenantId();
  Func<Entity, bool> filter = entity.TenantId == resolvedTenantIdValue;
  return filter;
}

EF Core inject tenant values from current context

I'm sorry. I still don't understand. Let's read the documentation. https://docs.microsoft.com/it-it/ef/core/querying/filters

Note the use of a DbContext instance level field: _tenantId used to set the current tenant. Model-level filters will use the value from the correct context instance (that is, the instance that is executing the query).

If a tenant id has to come from an instance field, and if DbContext instances are reused across HTTP requests, I don't see how a filter could be updated after the instance has been created and OnModelCreating has been called. So, this is a crucial point for me and I'm asking again: do DbContext instances get reused across HTTP requests when DbContext pooling is in place?

Thanks

smitpatel commented 5 years ago

So, this is a crucial point for me and I'm asking again: do DbContext instances get reused across HTTP requests when DbContext pooling is in place?

Of course. Isn't that the purpose of pooling? You cannot use DbContext pooling if your DbContext is depending on services which cannot be reused across HTTP requests.

smitpatel commented 5 years ago

https://docs.microsoft.com/en-us/ef/core/what-is-new/ef-core-2.0#limitations-1

MorenoGentili commented 5 years ago

Of course. Isn't that the purpose of pooling? You cannot use DbContext pooling if your DbContext is depending on services which cannot be reused across HTTP requests.

Ok, thank you for the clarification. Now I'm going back to my original question: would you implement a feature that lets a pooled DbContext depend on services that cannot be reused across requests?

The ResolveTenantId method I suggested would decouple the global filter from the model definition. This way, a global filter could be updated each time a DbContext instance is retrieved from the pool.

smitpatel commented 5 years ago

I don't think it could work that way. How do you expect us to retrieve the value when executing a query? Especially if it depends on a service which is outside the scope of EF. Or other way to put forward is, you need to put a new instance of your service each time after you get the context from pool. There is no bridge to EF Core query which makes it happen. (Even if we want to add what you are saying, we cannot). The best way for you to deal with this is to have a method on DbContext which you would call after resolving DbContext for each request and update your application service inside DbContext. Query filters would work just fine.

MorenoGentili commented 5 years ago

@smitpatel

How do you expect us to retrieve the value when executing a query? Especially if it depends on a service which is outside the scope of EF.

I'm not sure because I'm not too familiar with the internals of EFCore. I thought that maybe it could be possible since I can already resolve services from inside a DbContext instance by using this syntax.

var service = this.GetService<IMyService>();

However, I can understand the problem and now I wonder if it would be possible to at least expose an event or a protected overridable method, so I could execute code as soon as the DbContext instance has been retrieved from the pool and it's about to be used by services scoped to the current HTTP context.

So I could update the private field like this:

protected override void OnRetrievedFromPool()
{
  _tenantId = this.GetService<ITenantProvider>().GetTenantId(); //Service locator, but... whatever
}

Is this possible?

And finally:

The best way for you to deal with this is to have a method on DbContext which you would call after resolving DbContext for each request and update your application service inside DbContext

Indeed, if nothing else is possible I'd have to resort to that solution.

Thanks for your insight on the issue. Moreno

ajcvickers commented 5 years ago

@MorenoGentili Taking a step back for a moment, I would like to make a few general points:

Having said all that, a hook that is called after the context is obtained from the pool is a reasonable idea; we'll certainly consider this.

MorenoGentili commented 5 years ago

a hook that is called after the context is obtained from the pool is a reasonable idea; we'll certainly consider this.

Thank you @ajcvickers, I will use that hook to update the TenantId property. And inside the hook body I could get a reference to a scoped service like this:

TenantId = this.GetService<ITenantProvider>().GetTenantId();

Any opinion about this? Would it work?

ajcvickers commented 5 years ago

@MorenoGentili I think so, but I'm not 100% sure.

ajcvickers commented 4 years ago

See also #19193 when working on this. Also, removing from backlog to consider again in triage.

isen-ng commented 4 years ago

Coming over to this issue from https://github.com/npgsql/npgsql/issues/2885

Considering my specific use-case of setting Npgsql.PasswordCallbackProvider on a DbConnection before it is open, if there was a OnRetrievedFromPool method,

To be honest, if there are no issues with DbContextPooling when using

services.AddDbContextPooling(o -> 
{
    o.UseNpgsql(MyConnectionString);
});

it should be semantically no different from,

services.AddDbContextPooling(o -> 
{
    var connection = new NpgsqlConnection(MyConnectionString);
    o.UseNpgsql(connection);
});

Isn't it? Should I consider this a bug ..? Perhaps I'm understanding this wrongly.. ?

ajcvickers commented 4 years ago

@isen-ng

Technically, now my dbcontext needs to understand that my DbConnection is a NpgsqlConnection

It should not be hard to factor things so this is not the case.

If I had to instantiate a new NpgsqlConnection for every OnRetrievedFromPool, then we are really not benefitting from pooling at all, isn't it?

Two things. First, context pooling is not connection pooling. You'll still be using pooled actual connections to the database through normal ADO.NET connection pooling.

Second, the impression I got from the other thread was that you wanted to use a new scoped DbConnection object for each request. If you don't need to do that, then you could manipulate the existing connection object.

it should be semantically no different from,

The point of context pooling is that it lets you create and configure a DbContext once and then re-use it for multiple requests. Therefore, it is by-design that the code to resolve, create, and configure the context will only run once for a given context instance. If an object is created in that process, then that object will be reused.

isen-ng commented 4 years ago

I got from the other thread was that you wanted to use a new scoped DbConnection object for each request

This is what I want to do. I confused a DbConnection object with a physical pooled db connection. (They are handled separately right?)

Edit: hmm... if this feature is still open... then it will be implemented for EFCore 3.1 .... but I'm still on EFCore 2.2 ... D:

ajcvickers commented 4 years ago

@isen-ng Yes, connection pooling is separate.

This is currently a stretch goal for 5.0. (See release planning.) EF Core 2.2 is out-of-support.

roji commented 3 years ago

24768 originally added events and overridable methods as hooks for actions on leasing/returning, but we punted on that for 6.0. Next step is for me to bring this to a design discussion, with the alternative approach of handling these hooks via the factory instead.