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

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

Open
MorenoGentili opened this issue Aug 11, 2019 · 20 comments · Fixed by #24768
Open

Comments

@MorenoGentili
Copy link

MorenoGentili commented Aug 11, 2019

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
Copy link
Author

MorenoGentili commented Aug 12, 2019

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
Copy link
Member

smitpatel commented Aug 12, 2019

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
Copy link
Member

@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
Copy link
Author

MorenoGentili commented Aug 12, 2019

@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
Copy link
Member

@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
Copy link
Author

MorenoGentili commented Aug 12, 2019

@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
Copy link
Member

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
Copy link
Member

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

@MorenoGentili
Copy link
Author

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
Copy link
Member

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
Copy link
Author

MorenoGentili commented Aug 13, 2019

@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
Copy link
Member

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

  • Query filters are not intended to work by creating a new model for each value that needs to be filtered on. In other words, query filters should not be requiring OnModelCreating to run more than once for a given DbContext type.
  • The common way query filters are used without building a new model each time is to have some property, say TenantId on the DbContext which is then referenced in the query filter. EF will then pickup the current value of that property when filtering. Nothing needs to be changed in the model.
  • The default behavior of AddDbContext results is resolving the context instance for each request from D.I. This allows request-scoped services to be constructor-injected in the normal manner.
  • DbContext pooling explicitly changes that so that the same context instance is used in multiple requests. It is not possible to resolve request-scoped services since the instance is no longer necessarily resolved from D.I. in the request that it is used. So if the context instance needs to depend on request-scoped services, then don't use pooling.

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
Copy link
Author

MorenoGentili commented Aug 14, 2019

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
Copy link
Member

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

@ajcvickers ajcvickers changed the title [Feature request] Allow multi-tenancy (global filters) with context pooling Add hook to allow actions on the context after it is leased from the pool Aug 14, 2019
@ajcvickers ajcvickers added this to the Backlog milestone Aug 14, 2019
@ajcvickers ajcvickers changed the title Add hook to allow actions on the context after it is leased from the pool Add hook to allow actions on the context after it is leased/returned from/to the pool Aug 14, 2019
@ajcvickers ajcvickers added the good first issue This issue should be relatively straightforward to fix. label Sep 2, 2019
@ajcvickers ajcvickers removed this from the Backlog milestone Dec 7, 2019
@ajcvickers
Copy link
Member

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

@isen-ng
Copy link

isen-ng commented Mar 17, 2020

Coming over to this issue from npgsql/npgsql#2885

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

  • Technically, now my dbcontext needs to understand that my DbConnection is a NpgsqlConnection
    • this is less than ideal, but we can work with it
  • Does this meant that I will be able to set the PasswordCallbackProvider delegate before the DbConnection is open?
    • If I had to instantiate a new NpgsqlConnection for every OnRetrievedFromPool, then we are really not benefitting from pooling at all, isn't it?

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
Copy link
Member

@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
Copy link

isen-ng commented Mar 17, 2020

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 ajcvickers added closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. and removed good first issue This issue should be relatively straightforward to fix. labels Apr 26, 2021
@ajcvickers ajcvickers added area-dbcontext and removed closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. labels May 6, 2021
@AndriySvyryd AndriySvyryd added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Jun 9, 2021
AndriySvyryd pushed a commit that referenced this issue Jun 9, 2021
…e pool.

Fixes #19193
Fixes #17086

Notes:
* We now no longer reset context events, as per #19193
* However, I left the code that resets options like QueryTrackingBehavior since that still seems useful, and makes this less of a break
* Added events to DbContext. These are easy and decoupled, but sync only.
* A derived DbContext can override the On... event methods. This allows async hooks.
* DbContextFactory now has a CreateDbContextAsync method such that a context can be rented from the pool and the hook applied in an async context.
AndriySvyryd pushed a commit that referenced this issue Jun 11, 2021
…e pool.

Fixes #19193
Fixes #17086

Notes:
* We now no longer reset context events, as per #19193
* However, I left the code that resets options like QueryTrackingBehavior since that still seems useful, and makes this less of a break
* Added events to DbContext. These are easy and decoupled, but sync only.
* A derived DbContext can override the On... event methods. This allows async hooks.
* DbContextFactory now has a CreateDbContextAsync method such that a context can be rented from the pool and the hook applied in an async context.
@AndriySvyryd AndriySvyryd added needs-design propose-punt and removed closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. labels Jul 28, 2021
@roji roji reopened this Jul 30, 2021
@roji roji removed this from the 6.0.0 milestone Jul 30, 2021
@roji
Copy link
Member

roji commented Jul 30, 2021

#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.

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