sillsdev / sillsdev/languageforge-lexbox

Move CRDT commits to a composite (ProjectId, Id) PK + add unique index on Id

Open
#2,365 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

enhancement
Dominant language
C#
Stars
9
Forks
8
Avg merge
2d 13h
Merged PRs (30d)
49

Description

Background

Split out of the template-based project-creation work (branch claude/investigate-issue-1920-f6yHG, PR #2281). That PR pivoted to importing a JSON project snapshot and no longer needs this server-side schema change, so it's being deferred to its own change rather than riding along as unrelated scope.

What this is

Changes the server-side CrdtCommits primary key from (Id) to a composite (ProjectId, Id), and adjusts CommitEntityConfiguration, CrdtCommitService, and the tests to match.

Also wanted: a UNIQUE index on Id

We still never expect commit Ids to overlap — especially now that Harmony no longer lets a caller specify a commit's Id (they're minted internally). A composite (ProjectId, Id) PK on its own would permit the same Id under two different ProjectIds. Add a unique index on Id alongside the composite PK so global commit-Id uniqueness is actually enforced.

Patch

This is the non-generated part of the change only. The EF model snapshots — *.Designer.cs and LexBoxDbContextModelSnapshot.cs — are regenerated by dotnet ef migrations add, so they're omitted here; regenerate them against develop when re-adding the migration.

diff --git a/backend/LexBoxApi/Services/CrdtCommitService.cs b/backend/LexBoxApi/Services/CrdtCommitService.cs
index d4ec51f8c..6fa4768cb 100644
--- a/backend/LexBoxApi/Services/CrdtCommitService.cs
+++ b/backend/LexBoxApi/Services/CrdtCommitService.cs
@@ -16,7 +16,10 @@ public class CrdtCommitService(LexBoxDbContext dbContext)
         await using var transaction = await dbContext.Database.BeginTransactionAsync(token);
         var linqToDbContext = dbContext.CreateLinqToDBContext();
         await using var tmpTable = await linqToDbContext.CreateTempTableAsync<ServerCommit>($"tmp_crdt_commit_import_{projectId}__{Guid.NewGuid()}", cancellationToken: token);
-        //Stamp ProjectId while streaming so the merge below can be a plain column-to-column copy.
+        //Stamp ProjectId while streaming so the merge below can be a plain column-to-column copy
+        //AND so OnTargetKey (composite PK (ProjectId, Id)) matches against the right tenant —
+        //client-supplied ProjectId on the wire is untrusted; the URL path's projectId already
+        //cleared permissionService.
         //A projection lambda here would let linq2db v6 wrap our Sql.Expr<...>::jsonb cast in the
         //EF value-converter (JsonSerializer.Serialize) and fail SQL translation.
         var stampedCommits = commits.Select(c => { c.ProjectId = projectId; return c; });
diff --git a/backend/LexData/Entities/CommitEntityConfiguration.cs b/backend/LexData/Entities/CommitEntityConfiguration.cs
index 99c7ee188..550787312 100644
--- a/backend/LexData/Entities/CommitEntityConfiguration.cs
+++ b/backend/LexData/Entities/CommitEntityConfiguration.cs
@@ -18,7 +18,12 @@ public class CommitEntityConfiguration : IEntityTypeConfiguration<ServerCommit>
     public void Configure(EntityTypeBuilder<ServerCommit> builder)
     {
         builder.ToTable("CrdtCommits");
-        builder.HasKey(c => c.Id);
+        // PK is (ProjectId, Id): Commit.Id alone is not unique across projects (FwLite clients
+        // can mint the same Id for two different projects — e.g. a shared seed-commit pattern
+        // that ignores ProjectId), and the old Id-only PK caused the second arriving copy to be
+        // silently dropped by CrdtCommitService's MERGE ON TARGET KEY. Scoping the key by
+        // ProjectId lets both projects keep their own row.
+        builder.HasKey(c => new { c.ProjectId, c.Id });
         builder.ComplexProperty(c => c.HybridDateTime);
         builder.HasOne<Project>().WithMany()
             .HasPrincipalKey(project => project.Id)
diff --git a/backend/LexData/Migrations/20260528184900_ChangeCrdtCommitsPkToCompositeProjectIdId.cs b/backend/LexData/Migrations/20260528184900_ChangeCrdtCommitsPkToCompositeProjectIdId.cs
new file mode 100644
index 000000000..def1be60a
--- /dev/null
+++ b/backend/LexData/Migrations/20260528184900_ChangeCrdtCommitsPkToCompositeProjectIdId.cs
@@ -0,0 +1,45 @@
+using Microsoft.EntityFrameworkCore.Migrations;
+
+#nullable disable
+
+namespace LexData.Migrations
+{
+    /// <inheritdoc />
+    public partial class ChangeCrdtCommitsPkToCompositeProjectIdId : Migration
+    {
+        /// <inheritdoc />
+        protected override void Up(MigrationBuilder migrationBuilder)
+        {
+            migrationBuilder.DropPrimaryKey(
+                name: "PK_CrdtCommits",
+                table: "CrdtCommits");
+
+            migrationBuilder.DropIndex(
+                name: "IX_CrdtCommits_ProjectId",
+                table: "CrdtCommits");
+
+            migrationBuilder.AddPrimaryKey(
+                name: "PK_CrdtCommits",
+                table: "CrdtCommits",
+                columns: new[] { "ProjectId", "Id" });
+        }
+
+        /// <inheritdoc />
+        protected override void Down(MigrationBuilder migrationBuilder)
+        {
+            migrationBuilder.DropPrimaryKey(
+                name: "PK_CrdtCommits",
+                table: "CrdtCommits");
+
+            migrationBuilder.AddPrimaryKey(
+                name: "PK_CrdtCommits",
+                table: "CrdtCommits",
+                column: "Id");
+
+            migrationBuilder.CreateIndex(
+                name: "IX_CrdtCommits_ProjectId",
+                table: "CrdtCommits",
+                column: "ProjectId");
+        }
+    }
+}
diff --git a/backend/Testing/LexCore/Services/CrdtCommitServiceTests.cs b/backend/Testing/LexCore/Services/CrdtCommitServiceTests.cs
index cfd853c6c..ee192a328 100644
--- a/backend/Testing/LexCore/Services/CrdtCommitServiceTests.cs
+++ b/backend/Testing/LexCore/Services/CrdtCommitServiceTests.cs
@@ -192,6 +192,27 @@ public class CrdtCommitServiceTests
         _lexBoxDbContext.CrdtCommits(commit.ProjectId).Should().HaveCountGreaterThan(0);
     }
 
+    [Fact]
+    public async Task SameCommitIdInTwoDifferentProjectsKeepsBoth()
+    {
+        // Regression: with an Id-only PK the second project's identical Commit.Id was silently dropped by the MERGE's OnTargetKey; composite (ProjectId, Id) keeps both.
+        var projects = await _lexBoxDbContext.Projects.Select(p => p.Id).Take(2).ToArrayAsync();
+        if (projects.Length < 2)
+            throw new InvalidOperationException("Test requires at least 2 seeded projects.");
+
+        var sharedCommitId = Guid.NewGuid();
+        var commitForA = CreateCommit(Guid.NewGuid(), Guid.NewGuid(), DateTime.UtcNow, sharedCommitId);
+        var commitForB = CreateCommit(Guid.NewGuid(), Guid.NewGuid(), DateTime.UtcNow, sharedCommitId);
+
+        await _crdtCommitService.AddCommits(projects[0], AsAsync([commitForA]));
+        await _crdtCommitService.AddCommits(projects[1], AsAsync([commitForB]));
+
+        var inA = await _lexBoxDbContext.CrdtCommits(projects[0]).Where(c => c.Id == sharedCommitId).ToArrayAsync();
+        var inB = await _lexBoxDbContext.CrdtCommits(projects[1]).Where(c => c.Id == sharedCommitId).ToArrayAsync();
+        inA.Should().ContainSingle().Which.ClientId.Should().Be(commitForA.ClientId);
+        inB.Should().ContainSingle().Which.ClientId.Should().Be(commitForB.ClientId);
+    }
+
 
     private async Task<ServerCommit> AddTestCommit()
     {

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Review backend/LexData/Entities/CommitEntityConfiguration.cs, backend/LexBoxApi/Services/CrdtCommitService.cs, the named migration, and CrdtCommitServiceTests.cs. Start with the existing composite-key regression test and migration, then regenerate the EF snapshots with dotnet ef migrations add; done means both projects retain the shared commit ID, uniqueness is enforced, and the tests pass.

Written by the indexing model from the issue text.

Assessment

Tech stack
csharp
Domain
backend, database
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.