why MultiTenantDbContext requires ITenantInfo not just current TenantId ?

[mhDuke]

could you please explain to me why does MultiTenantDbContext requires ITenantInfo when a string tenantId parameter or

interface ICurrentTenant 
{ 
    string TenantId { get; set; } 
}

seems to be enough?

[AndrewTriesToCode]

Hi, two main reasons. 1) dependency injection: it needs to work with DI and injecting a string type wasn’t feasible until keyed services very recently 2) users may want to use more of the properties in their db context e.g. connection string

I actually will be changing it so they accept IMultiTenantContextAccesor (in addition to the current). You can of course make your own constructor that takes a string, creates a tenant info instance, and passes it to the base constructor for convenience. I will probably add that soon as well.

[mhDuke]

cheers. thank you buddy for this great tool. we started using MultiTenant recently and it sure saved us a lot of work. Now I am sure I am ignorant on many of the use cases others have, but imo requiring ITenantInfo as MultiTenantDbContext constructor argument invites confusion, and abuse.

For example the guys working on fullstackhero template have this snippet in their db context

when I think about it, this cannmot be correct, DbContextOptions should be enough to connect to the correct database. relying on the ITenantInfo.ConnectionString tells me there is either a misunderstanding on their side, or it's a work around of some sort. I couldn't figure that out yet.

Again I assume I am not seeing the full picture, and perhaps it's misunderstanding on my side. your thoughts are much appreciated.

[AndrewTriesToCode]

Hi again, DbContextOptions and its builder don't really follow the normal options pattern and when used with AddDbContext it isn't obvious how/if per-tenant options can be applied -- so in this case they use the tenant details on OnConfiguring at runtime to have the dbcontext connect to the correct database for each tenant. Some might use the same database, others might have their own dedicated database.

Also you forgot one possibility: I didn't design well and plan ahead when I started building this thing!

[mhDuke]

the possibilities are many. let's put them aside. In my understanding (within web app context) services.AddMultiTenant<Tenant>().WithClaimsStrategy("tenant_claim_key") will somehow resolve the correct ITenantInfo for the tenant Id extracted from the user's tenant claim, and add it as scoped to the dependency container (given the user has a valid tenant id claim). we don't care how at the moment.

then, (again I am drawing the picture in my mind), we should use GetRequiredService<ITenantInfo>() in AddDbContext() in order to build the correct DbContextOptions that is used to build the tenant's DbContext.

do you agree, that this is the valid straight forward approach utilizing your library? I can image not all cases are straight forward.

[AndrewTriesToCode]

You are correct but I have a PR coming soon that changes this.

Soon only IMultiTenantContextAccessor will be available straight from DI. I’m doing this to more closely match how HttpContext works. You’ll notice that is never injected because it is not a service, just like ITenantInfo isn’t.

I’ll have some convenience methods to get it but not straight via DI. Another problem it has with DI is that sometimes there is no current tenant so it is null which doesn’t play well with DI. The accessor is never null.

For db context specifically I’ll probably keep the current constructor but also add ones for just tenantId and an accessor. I am also planning to add factory method support which will work better in Blazor and some use cases when you need a dbcontext for a given tenant that might not be the current tenant.

[mhDuke]

Good news. I appreciate you giving the time to discuss this matter.

As a side note, I believe that it's always best to assume that a DbContext inheriting from MultiTenantDbContext cannot be accessed without a valid tenant. If an application service depends on MultiTenantDbContext, then a valid tenant must be in place before using that service.

if a client (MultiTenant library user) has some data that is multi-tenant-based and some data that is not. then there should be a strong emphasis on having two DbContexts, one for the shared data, and another that inherits from MultiTenantDbContext. This way we close the door in the face of confusion and bad design.

If my belief is valid, we might want to add a warning, when accessing MultiTenantDbContext with illegal tenant object/id (null/default), discouraging users from going that direction, perhaps with a ObsoleteAttribute, label it as anti-pattern or so. I might be pushing things a bit too far though😅.

[AndrewTriesToCode]

I agree with yo that it is a best practice. There are some use cases where some entities make sense to not be multi tenant but part of the overall model. Right now a db context just throws exceptions on null tenant id. I want to add some smarter handling. Maybe it returns no results or maybe there is an option to have it return no results OR throw an exception during construction.

[AndrewTriesToCode]

Also it’s on my list to look at using interceptors for the functionality or maybe events. Neither were an option back when I started. That would make it easier to not have to inherit the Finbuckle context which is something not everyone can do.

[mhDuke]

Wait. To have multi-tenancy support in a DbContext inheriting from MultiTenantDbContext is not the only option? I need to revisit that part on the docs.

[AndrewTriesToCode]

Yep, here is the section: https://www.finbuckle.com/MultiTenant/Docs/v6.13.1/EFCore#adding-multitenant-functionality-to-an-existing-dbcontext