-
-
Notifications
You must be signed in to change notification settings - Fork 254
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
why MultiTenantDbContext requires ITenantInfo not just current TenantId ? #802
Comments
Hi, two main reasons.
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. |
Hi again, Also you forgot one possibility: I didn't design well and plan ahead when I started building this thing! |
the possibilities are many. let's put them aside. then, (again I am drawing the picture in my mind), we should use do you agree, that this is the valid straight forward approach utilizing your library? I can image not all cases are straight forward. |
You are correct but I have a PR coming soon that changes this. Soon only 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. |
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 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 If my belief is valid, we might want to add a warning, when accessing |
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. |
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. |
Wait. To have multi-tenancy support in a |
could you please explain to me why does
MultiTenantDbContext
requiresITenantInfo
when astring tenantId
parameter orseems to be enough?
The text was updated successfully, but these errors were encountered: