Deleting a model with a composite key bypassing multitenancy

[domn1995]

When deleting a multitenant entity that's configured to use a composite key in our database migrations scripts and with the EF Core ModelBuilder isn't respecting multitenancy.

The database constraint in our migrations script looks like: ALTER TABLE ONLY public."Examples" ADD CONSTRAINT "PK_Examples" PRIMARY KEY ("Number", "TenantId", "Username");

Our EF Core classes look like:

[MultiTenant]
public class ExampleModel 
{
    public string Username { get; set; }
    public string Number { get; set; }
}

public class TestDbContext : MultiTenantDbContext
{
    public DbSet<ExampleModel>? Examples { get; set; }

    public TestDbContext(TenantInfo tenantInfo,
        DbContextOptions options) :
        base(tenantInfo, options)
    {
    }

    protected override void OnModelCreating(ModelBuilder modelBuilder)
    {
        base.OnModelCreating(modelBuilder);
        modelBuilder.Entity<ExampleModel>().HasKey(b => new { b.Number, b.Username });
    }
}

When we delete an ExampleModel from the context, all other ExampleModels with the same Username and Number are deleted as well, even with different TenantIds.

Should Finbuckle be adding the TenantId to the composite key? Is there a way we can add it as part of the model builder such that the call to modelBuilder mirrors our database constraints? Is there some other solution?

[AndrewTriesToCode]

Thanks for reporting this issue. I'll take a closer look tomorrow. Finbuckle doesn't automatically add tenant id to the key but has a helper function to do so for entities that works in some but not all cases due to EF Core constraint priorities.

I'll have a more detailed answer for you soon.

[domn1995]

Thanks for swift reply.

Are you referring to the Keys and Indexes section of https://www.finbuckle.com/MultiTenant/Docs/v6.6.0/EFCore docs?

I was going to try that as a solution, but if it doesn't work I'll get you some nice repro code :)

[domn1995]

Still ran into issues with adjusting keys as per the https://www.finbuckle.com/MultiTenant/Docs/v6.6.0/EFCore Keys and Indexes docs. Working on a fork of the repo with some tests added that reproduce the issues we're running into.

[domn1995]

Here's a fork with some tests that surface some of the issues we're running into: https://github.com/domn1995/Finbuckle.MultiTenant/blob/main/test/Finbuckle.MultiTenant.EntityFrameworkCore.Test/CompositeKeyTests/CompositeKeysShould.cs

I would expect all of these tests to pass but only the HandleTenantIdInCompositeKeyAndExplicitTenantIdInModel() test passes.

[AndrewTriesToCode]

Ok, having dug in I can explain some parts but not all. Thank you for isolating the behavior so well.

• first of all only .NET 6 has a passing test. I need to look at the earlier versions closer.
• HandleNoTenantIdInCompositeKeyAndShadowTenantIdInModel and HandleNoTenantIdInCompositeKeyAndExplicitTenantIdInModel I would expect to fail because EFCore doesn't even know TenantId is intended to be part of the key. Finbuckle leans heavily on global query filters for tenant data isolation and unfortunately they don't apply to delete statements. EFCore seems to use the key columns it knows about to generate delete statements hence wiping the records for all tenants.
• For the other 2 tests, HandleTenantIdInCompositeKeyAndExplicitTenantIdInModel works as expected and I recommend you explicitly add TenantId to models if within your control -- shadow properties just cause so many headaches. For HandleAdjustedCompositeKeyAndShadowTenantIdInModel this should work but I think we might be hitting this bug so I posed a question there.
• In all of these cases the TenantNotSetMode is the default of throw so it should actually prevent the save attempt. This makes me realize the value generator for TenantId is masking this intended behavior. I'll have to take a closer look at this.

I hope this help and thanks for all the details.

[domn1995]

Thanks so much for looking into this so quickly!

We're moving forward with explicit TenantIds in our models. We didn't do so originally because the docs didn't really steer us that way. Take that feedback as you will. :)

Thanks again for the prompt support.

[AndrewTriesToCode]

Thanks so much for looking into this so quickly!

We're moving forward with explicit TenantIds in our models. We didn't do so originally because the docs didn't really steer us that way. Take that feedback as you will. :)

Thanks again for the prompt support.

Happy to help and thank you for the sponsorship.

[AndrewTriesToCode]

Alright I removed the generator and filed a bug with the efcore team. They said only on "adding" a tracked entity will it pick up a shadow key property. I voted on an issue to expand that to any method which attaches an entity.

The best option here is to do exactly what you did -- or to use the TenantNotSetMode which as of the latest release should work as intended with the generator removed.

Thanks!