Composite primary key

[jimmymcpeter]

Great work on this library. I've been playing around with it and EFCore... I'm curious what your opinion is using a composite primary key of TenantId and Id in the AppDbContext? I had to use the fluent API extensively for this. The LocalTenant record is populated to the DB as part of provisioning a new tenant in the CatalogDbContext.

Adding ValueGeneratedOnAdd to the LocalTenant.cs TenantId kept EFCore from throwing System.InvalidOperationException: Unable to track an entity of type 'Currency' because primary key property 'TenantId' is null. when trying to Add and SaveChanges for a Currency record.

Startup.cs

// ...
public void ConfigureServices(IServiceCollection services)
{
    services.AddControllers();
    services.AddMultiTenant()
        .WithEFCoreStore<CatalogDbContext>()
        .WithStaticStrategy("initech"); // TODO

    // Register the db context, but do not specify a provider/connection string since
    // these vary by tenant.
    services.AddDbContext<AppDbContext>();
}
// ...

CatalogDbContext.cs

public class CatalogDbContext : EFCoreStoreDbContext
{
    public CatalogDbContext(DbContextOptions<CatalogDbContext>  options) : base(options)
    {
    }

    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
    {
        optionsBuilder.UseSqlite("Data Source=Data/Catalog.db"); // TODO change me to a real DB
        base.OnConfiguring(optionsBuilder);
    }
}

AppDbContext.cs

public class AppDbContext : MultiTenantDbContext
{
    public AppDbContext(TenantInfo tenantInfo) : base(tenantInfo)
    {
    }

    public AppDbContext(TenantInfo tenantInfo, DbContextOptions<AppDbContext> options) : base(tenantInfo, options)
    {
    }

    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
    {
        if (!optionsBuilder.IsConfigured)
        {
            optionsBuilder.UseSqlite(ConnectionString); // TODO change me to real DB
        }

        base.OnConfiguring(optionsBuilder);
    }

    protected override void OnModelCreating(ModelBuilder modelBuilder)
    {
        modelBuilder.ApplyConfigurationsFromAssembly(this.GetType().Assembly);

        base.OnModelCreating(modelBuilder);
    }

    public DbSet<LocalTenant> LocalTenants { get; set; }
    public DbSet<Currency> Currencies { get; set; }
}

AppDbContextFactory.cs

/// <summary>
/// The AppDbContext requires a tenant to function
/// Use a design-time factory and a dummy tenant to get around this for EF Migrations
/// https://docs.microsoft.com/en-us/ef/core/miscellaneous/cli/dbcontext-creation#from-a-design-time-factory
/// </summary>
public class AppDbContextFactory : IDesignTimeDbContextFactory<AppDbContext>
{
    public AppDbContext CreateDbContext(string[] args)
    {
        var dummy = new TenantInfo(
            "dummy", "dummy", "Dummy", "Data Source=Data/Seed.db", null
        );
        return new AppDbContext(dummy);
    }
}

Currency.cs

public class Currency
{
    public string TenantId { get; set; }
    public string Id { get; set; }
    public string NaturalId { get; set; }
    public string DisplayName { get; set; }
}

public class CurrencyEntityTypeConfiguration : IEntityTypeConfiguration<Currency>
{
    public void Configure(EntityTypeBuilder<Currency> builder)
    {
        builder
            .ToTable("currency");

        #region Primary Key and Indexes
        builder
            .IsMultiTenant()
            .HasKey(c => new {c.TenantId, c.Id});

        builder
            .HasIndex(c => new {c.TenantId, c.NaturalId})
            .IsUnique();
        #endregion

        #region Columns

        builder
            .Property(c => c.TenantId)
            .HasColumnName("tenant_id");
        builder
            .HasOne<LocalTenant>()
            .WithMany()
            .HasForeignKey(c => c.TenantId);
        builder
            .Property(c => c.Id)
            .HasColumnName("id");
        builder
            .Property(c => c.NaturalId)
            .HasColumnName("natural_id");
        builder
            .Property(c => c.DisplayName)
            .HasColumnName("display_name");
        #endregion
    }
}

LocalTenant.cs

public class LocalTenant
{
    public string TenantId { get; set; }
    public string NaturalId { get; set; }
    public string DisplayName { get; set; }
    public bool Active { get; set; }
    public bool Enabled { get; set; }
}

public class LocalTenantEntityTypeConfiguration : IEntityTypeConfiguration<LocalTenant>
{
    public void Configure(EntityTypeBuilder<LocalTenant> builder)
    {
        builder
            .ToTable("local_tenant");

        #region Primary Key and Indexes
        builder
            .IsMultiTenant()
            .HasKey(t => t.TenantId);

        builder
            .HasIndex(t => t.NaturalId)
            .IsUnique();
        #endregion

        #region Columns

        builder
            .Property(t => t.TenantId)
            .HasColumnName("tenant_id")
            .ValueGeneratedOnAdd(); // This was needed
        builder
            .Property(t => t.NaturalId)
            .HasColumnName("natural_id");
        builder
            .Property(t => t.DisplayName)
            .HasColumnName("display_name");
        builder
            .Property(t => t.Active)
            .HasColumnName("active");
        builder
            .Property(t => t.Enabled)
            .HasColumnName("enabled");
        #endregion
    }
}

[AndrewTriesToCode]

@jimmymcpeter

Hello, thanks for the question. I think it all depends on the specific application when its comes to including tenantId in multicolumn keys and indexes. I had to make sure Identity keys and indexes were adjusted accordingly. Your approach here looks pretty solid.

With respect to needing the ValueGenerateOnAdd, I don't think it should be needed because in a typical setup the library populates that column with the value of the dbcontext's tenantinfo during SaveChanges. Can you post the code where a new tenant is provisioned? Maybe I can give some more insight based on that.

[AndrewTriesToCode]

Also, I just want to make sure I understand -- the EFCoreStore database context is not part of the issue you are having is it?

[jimmymcpeter]

Thanks for replying so quickly, @achandlerwhite 👍

Also, I just want to make sure I understand -- the EFCoreStore database context is not part of the issue you are having is it?

Correct, I do not have any issues with this.

With respect to needing the ValueGenerateOnAdd, I don't think it should be needed because in a typical setup the library populates that column with the value of the dbcontext's tenantinfo during SaveChanges. Can you post the code where a new tenant is provisioned? Maybe I can give some more insight based on that.

Do you think it's because on my Currency model I have the relationship to LocalTenant

builder
    .HasOne<LocalTenant>()
    .WithMany()
    .HasForeignKey(c => c.TenantId);

and EFCore is checking for the TenantId before Finbuckle actually sets it when I run context.SaveChanges(); ?

[jimmymcpeter]

You're right on the ValueGenerateOnAdd. I built out a CurrencyController for HttpPost to create a Currency record

[HttpPost]
public async Task<ActionResult<CurrencyDtoOut>> CreateCurrency(CurrencyDtoIn currencyIn)
{
    var currency = new Currency()
    {
        Id = "blah",
        NaturalId = currencyIn.NaturalId,
        DisplayName = currencyIn.DisplayName,
    };
    _context.Currencies.Add(currency);
    await _context.SaveChangesAsync();

    return CreatedAtAction(nameof(GetCurrency), new {naturalId = currencyIn.NaturalId}, new CurrencyDtoOut()
    {
        NaturalId = currency.NaturalId,
        DisplayName = currency.DisplayName,
    });
}

I kept getting a MultiTenantException for the TenantId not matching when I sent in my HTTP Post to create a currency. I threw a breakpoint on the _context.Currencies.Add(currency); line and noticed a random GUID getting set for the TenantId. This led me to the docs on Value generated on add and this comment

Depending on the database provider being used, values may be generated client side by EF or in the database. If the value is generated by the database, then EF may assign a temporary value when you add the entity to the context. This temporary value will then be replaced by the database generated value during SaveChanges().

🤦‍♂ I removed the ValueGenerateOnAdd on all models, but then ran into System.InvalidOperationException: Unable to track an entity of type 'Currency' because primary key property 'TenantId' is null. Adding Finbuckle's _context.TenantNotSetMode = TenantNotSetMode.Overwrite; to my code did not let me bypass this exception. My workaround was to just set the TenantId property directly on the Currency with _context.TenantInfo.Id. This seems like the right thing to do, or get rid of the composite primary keys completely, but I'd prefer not to do that.

[HttpPost]
public async Task<ActionResult<CurrencyDtoOut>> CreateCurrency(CurrencyDtoIn currencyIn)
{
    var currency = new Currency()
    {
        TenantId = _context.TenantInfo.Id, // <--- !!!
        Id = "blah",
        NaturalId = currencyIn.NaturalId,
        DisplayName = currencyIn.DisplayName,
    };
    _context.Currencies.Add(currency);
    await _context.SaveChangesAsync();

    return CreatedAtAction(nameof(GetCurrency), new {naturalId = currencyIn.NaturalId}, new CurrencyDtoOut()
    {
        NaturalId = currency.NaturalId,
        DisplayName = currency.DisplayName,
    });
}

[AndrewTriesToCode]

@jimmymcpeter This is an interesting find, thanks for providing all the detail. I usually use a GUID for my Id and so haven't personally hit this problem with a composite key. I'm glad you found a workaround.

One more thing, what is the purpose of the LocalTenants entity? I ask because the name implies it might be serving the same purpose as the tenant store (Storing basic info on the tenant), in which case you might want to consider using the same DbContext for the app and for the EFCoreStore -- just a thought.

[jimmymcpeter]

No problem! I have some other things I've found I will probably put in a separate issue for your comments. My local_tenant table is for keeping referential integrity of whatever tenants are on that specific database.

[dirraj]

Hey @jimmymcpeter and anyone else stumbling upon this.

Three years later I've got to say I also struggled using Finbuckle and composite keys.

I initially adopted a strict Id+TenantId composite key policy for all entities, trying to use it as an extra layer of protection and prevent data leaks.

builder.Entity<EntityBase>().IsMultiTenant().AdjustKey(builder.Entity<EntityBase>().Metadata.GetKeys().First(), builder).AdjustIndexes();

But trying to add a new entity using the navigation of another entity, I've went in circles to work around the two different problems which arise: Tenant cannot but null vs FK cannot be changed.

This is having used or not used Guid.Empty.ToString() as default TenantId in ctor. And used or not used TenantNotSetMode.Overwrite and TenantMismatchMode.Overwrite.

Of course setting the TenantId explicitly worked perfectly well but I didn't want to do this.

In the end I've decided to give up the composite key and use merely indexes instead, knowing that Finbuckle will still use the shadow property in it's query filtering.

[jtanios]

I'm running into the same issue.

Wouldn't it be better to add the TenantId to the entity by overriding the Add() or AddAsync() method? We'd still call EnforceMultiTenant() on SaveChanges to ensure the correct assignment of the TenantId.

[rinkeb]

It looks like this now fixed in 7.0:

Bug Fixes AdjustKey correctly adding TenantId to primary and foreign keys (613b4a8)

[shainegordon]

I think this is still an issue, on 7.0.1

I have a very basic model

public record Client : AbstractEntity, IArchivable
{
    public required string Name { get; set; }
    // snipped out some things
}

        public static void ConfigureMultiTenantEntities(this ModelBuilder modelBuilder)
        {
            modelBuilder.Entity<Client>().IsMultiTenant().AdjustUniqueIndexes().AdjustIndexes().AdjustKeys(modelBuilder);
        }

        private static MultiTenantEntityTypeBuilder AdjustKeys(this MultiTenantEntityTypeBuilder builder, ModelBuilder modelBuilder)
        {
            var keys = builder.Builder.Metadata.GetKeys();
            foreach (var key in keys.ToArray())
            {
                builder.AdjustKey(key, modelBuilder);
            }

            return builder;
        }

    public class ApplicationContext : MultiTenantIdentityDbContext<ApplicationUser, ApplicationRole, Ulid>
    {
        public DbSet<Client> Clients => Set<Client>();

        protected override void OnModelCreating(ModelBuilder modelBuilder)
        {
            modelBuilder.CreateIndexes();
            modelBuilder.ConfigureQueryFilters();
            modelBuilder.ConfigureMultiTenantEntities();
            base.OnModelCreating(modelBuilder);
        }

        public async Task<ErrorOr<CrudContracts.Client>> Handle(CreateClientWithContactsCommand command,
            CancellationToken cancellationToken)
        {
            var client = _mapper.Map<Client>(command) with { Id = Ulid.NewUlid() };

            _applicationContext.Clients.Add(client);
            await _applicationContext.SaveChangesAsync(cancellationToken);

            return _mapper.Map<CrudContracts.Client>(client);
        }

When I the line _applicationContext.Clients.Add(client); is executed, I'll get the following error:

System.InvalidOperationException: Unable to track an entity of type 'Client' because its primary key property 'TenantId' is null.
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.NullableKeyIdentityMap`1.Add(InternalEntityEntry entry)
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.StateManager.StartTracking(InternalEntityEntry entry)
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.InternalEntityEntry.SetEntityState(EntityState oldState, EntityState newState, Boolean acceptChanges, Boolean modifyProperties)
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.InternalEntityEntry.SetEntityState(EntityState entityState, Boolean acceptChanges, Boolean modifyProperties, Nullable`1 forceStateWhenUnknownKey, Nullable`1 fallbackState)
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.EntityGraphAttacher.PaintAction(EntityEntryGraphNode`1 node)
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.EntityEntryGraphIterator.TraverseGraph[TState](EntityEntryGraphNode`1 node, Func`2 handleNode)
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.EntityGraphAttacher.AttachGraph(InternalEntityEntry rootEntry, EntityState targetState, EntityState storeGeneratedWithKeySetTargetState, Boolean forceStateWhenUnknownKey)
   at Microsoft.EntityFrameworkCore.Internal.InternalDbSet`1.SetEntityState(InternalEntityEntry entry, EntityState entityState)
   at Microsoft.EntityFrameworkCore.Internal.InternalDbSet`1.Add(TEntity entity)

We are not using auto generated ID's, and they are of type Ulid and not GUID, but not sure this should make a difference, because Id on TenantInfo is just a string.

This first raised it's head with me when I was making sure that our oauth callbacks checks that the user is assigned to a tenant.

        var userLogins = await userManager.GetLoginsAsync(user);

        if (!userLogins.Any())
        {
            await userManager.AddLoginAsync(user, info);
        }

System.InvalidOperationException: Unable to track an entity of type 'IdentityUserLogin<Ulid>' because its primary key property 'TenantId' is null.
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.NullableKeyIdentityMap`1.Add(InternalEntityEntry entry)
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.StateManager.StartTracking(InternalEntityEntry entry)
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.InternalEntityEntry.SetEntityState(EntityState oldState, EntityState newState, Boolean acceptChanges, Boolean modifyProperties)
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.InternalEntityEntry.SetEntityState(EntityState entityState, Boolean acceptChanges, Boolean modifyProperties, Nullable`1 forceStateWhenUnknownKey, Nullable`1 fallbackState)
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.EntityGraphAttacher.PaintAction(EntityEntryGraphNode`1 node)
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.EntityEntryGraphIterator.TraverseGraph[TState](EntityEntryGraphNode`1 node, Func`2 handleNode)
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.EntityGraphAttacher.AttachGraph(InternalEntityEntry rootEntry, EntityState targetState, EntityState storeGeneratedWithKeySetTargetState, Boolean forceStateWhenUnknownKey)
   at Microsoft.EntityFrameworkCore.Internal.InternalDbSet`1.SetEntityState(InternalEntityEntry entry, EntityState entityState)
   at Microsoft.EntityFrameworkCore.Internal.InternalDbSet`1.Add(TEntity entity)
   at Microsoft.AspNetCore.Identity.EntityFrameworkCore.UserStore`9.AddLoginAsync(TUser user, UserLoginInfo login, CancellationToken cancellationToken)
   at Microsoft.AspNetCore.Identity.UserManager`1.AddLoginAsync(TUser user, UserLoginInfo login)

[shainegordon]

I'm going to clone the sample project and see if I can reproduce this there

[shainegordon]

I'm going to clone the sample project and see if I can reproduce this there

done: https://github.com/Finbuckle/Finbuckle.MultiTenant/pull/834

Apologies if creating a PR wasn't the correct way to go here, as I see this needs a CLA.

Happy for this to become part of the sample though

[alexTr3]

We need this fixed ASAP