Finbuckle / Finbuckle.MultiTenant

Finbuckle.MultiTenant is an open-source multitenancy middleware library for .NET. It enables tenant resolution, per-tenant app behavior, and per-tenant data isolation.
https://www.finbuckle.com/multitenant
Apache License 2.0
1.3k stars 265 forks source link

Should EFCoreStoreDbContext set TenantId property on model during SaveChangesAsync? #244

Closed ericbrumfield closed 4 years ago

ericbrumfield commented 4 years ago

Hi,

I've been following the tutorials and samples but can't seem to get the TenantId to be set automagically during SaveChangesAsync. I'm under the assumption/impression that EFCoreStoreDbContext will do this for me given that I've decorated the model with MultiTenant attribute or used IsMultiTenant() on the model builder. I also have declared a TenantId property on the model because I want to be able to use it some spots of code instead of it being a completely shadow property. I think the docs said that it will use this property when it's defined on the model.

Currently TenantInfo is recognizing my tenant setup using host strategy successfully from my logging, but when I save the model/entity, the TenantId property is null and it's failing. Should I override and add code to SaveChangesAsync on the context to populate this property on models instead?

Thanks!

AndrewTriesToCode commented 4 years ago

hi @ericbrumfield I'm sorry for the slow reply.

This is something I need to clarify in the docs I think. EFCoreStore itself isn't intended to be multitenant (although I guess it could in some specific use cases). It's just a multitenant store that happens to use EFCore to talk to a database to pull tenant info during tenant resolution. Most apps will only have a single store that stores all the tenant information.

On the other hand there is IMultiTenantDbContext and its supporting functionality (as well as MultiTenantDbContext and MultiTenantIdentityDbContext). These would be an app's regular database for generic app functionality where you want to have tenants share the same database but have it "isolated" to a degree. This is where each record on a multitenant entity will get the TenantId set automagically.

Both are EFCore related but serve different purposes. Does that explain it a little better?

If you give me an idea of what you'd like your app to do I can try to explain how this library might help you further.

ericbrumfield commented 4 years ago

No problem, it does help me, thank you. I actually dug into the source a bit ago and figured it out that I would indeed have to add the implementation to get it to save. I did switch to deriving from IMultiTenantDbContext and it's mostly working, but I'm hitting some DI related lifecycle problems getting TentnInfo to not be null during EF context construction. It feels like it's being injected prior to maybe some middleware running that populates TenantInfo.

My DbContext is deriving both EFCoreStoreDbContext, IMultiTenantDbContext. My DI in startup.cs lookes like this:

services.AddDbContext<SentinelHQDbContext>(options =>
                options.UseNpgsql(_config.GetConnectionString("DefaultConnection"))
            );
            services.AddMultiTenant()
                .WithEFCoreStore<AppDbContext>()
                .WithHostStrategy()
                .WithFallbackStrategy("hq");

Overall this project needs are basic for now. I'm co-mingling the tenant info and the tenant data in a single pgsql catalog and am using latest Entity Framework core in a .net 3.1 app. I mostly need the host routing strategy + the ease of filtering it provides me in pulling tenant data by the domain being used for now. I don't see us requiring to actually isolate tenant data to their own catalogs, and I do appreciate that feature of this library where that's an option.

I do have it working for reading/writing now, but the TenantInfo being injected into the ef db context constructor is null and to get around that I'm setting the TenantInfo property manually in my "service layer" that has the dbcontext getting injected.

I can open a different ticket and close this one if you want to discuss that at all, or if you know any tricks there I'd appreciate it.

Thanks again!

AndrewTriesToCode commented 4 years ago

Thanks for the detail. In your Configure method, do you have any middleware that might be triggering the dbcontext initialization prior to UseMultiTenant getting called?

ericbrumfield commented 4 years ago

I don't believe so but it could be possible, I'd have to dig. This is what my Configure method looks like:

public void Configure(IApplicationBuilder app, IWebHostEnvironment env)
        {
            if (env.IsDevelopment())
            {
                app.UseDeveloperExceptionPage();
            }

            // Enable middleware to serve generated Swagger as a JSON endpoint.
            app.UseSwagger(c => { c.SerializeAsV2 = true; });

            // Enable middleware to serve swagger-ui (HTML, JS, CSS, etc.), 
            // specifying the Swagger JSON endpoint.
            app.UseSwaggerUI(c => { c.SwaggerEndpoint("/swagger/v1/swagger.json", "HQ Api"); });

            app.UseHttpsRedirection();

            app.UseRouting();

            app.UseMultiTenant();

            app.UseAuthentication();

            app.UseAuthorization();

            app.UseEndpoints(endpoints =>
            {
                endpoints.MapControllers();
            });
        }

I'm going to try right now to pop in a Lazy injection wise for TenantInfo resolution in my context to see if I can get around it being null. I know I'm probably hacking this together at this point for my use case, but am learning this library some while doing it.

ericbrumfield commented 4 years ago

My injected Lazy TenantInfo just worked, so I added this to my startup ConfigureServices:

services.AddScoped<Lazy<TenantInfo>>(p =>
            {
                return new Lazy<TenantInfo>(() =>
                {
                    var accessor = p.GetService<IHttpContextAccessor>();
                    return accessor.HttpContext.GetMultiTenantContext().TenantInfo;
                }); 
            });

and then my DbContext looks like this for the required bits:

public class HQDbContext: EFCoreStoreDbContext, IMultiTenantDbContext/*MultiTenantDbContext, IMultiTenantStore*/
    {
        private readonly IHttpContextAccessor _httpContextAccessor;
        private readonly Lazy<TenantInfo> _lazyTenantInfo;

        public TenantInfo TenantInfo
        {
            get { return this._lazyTenantInfo.Value; }
        }

        public TenantMismatchMode TenantMismatchMode { get; }
        public TenantNotSetMode TenantNotSetMode { get; }

        public HQDbContext(Lazy<TenantInfo> tenantInfo, DbContextOptions<HQDbContext> options)
            : base(options)
        {
            this._lazyTenantInfo = tenantInfo;
        }

        public HQDbContext(Lazy<TenantInfo> tenantInfo, DbContextOptions<HQDbContext> options, IHttpContextAccessor httpContextAccessor)
            : base(options)
        {
            _httpContextAccessor = httpContextAccessor;
            this._lazyTenantInfo = tenantInfo;
        }

I'm using the HttpContextAccessor down in here because I'm using some shadow properties for audit fields (created by etc.) and am pulling the user id from JWT. App here already existed before introducting Finbuckle.

I don't know if this is the proper way, but it is working without me having to set the TenantInfo property up in my "service layer" manually now.

AndrewTriesToCode commented 4 years ago

Makes sense. In fact I use HttpContextAccessor for the same purpose within the library for per-tenant options.

ericbrumfield commented 4 years ago

Thanks again for the responses. In my use case I guess you have to have this one Lazily evaluate instead of a more common DI setup for TenantInfo to inject as the Finbuckle middleware runs later in the pipeline I guess, so TenantInfo is null without it.

Hopefully this is a useful scenario and work-around for the library and others. I'll close this out now, I think I'm good now and if we need to break out or migrate tenants out later this library gives us that option to do so.

Nice library by the way, pretty simple and not very intrusive.