Prevent clients from setting CommitId

src/SIL.Harmony.Tests/CommitTests.cs

@@ -114,17 +114,14 @@ public void CanRoundTripCommitThroughJson()
         {
             ClientId = Guid.NewGuid(),
             HybridDateTime = Now(),
-            ChangeEntities =
-            {
-                new ChangeEntity<IChange>
-                {
-                    Change = change,
-                    Index = 0,
-                    CommitId = change.CommitId,
-                    EntityId = change.EntityId
-                }
-            }
         };
+        commit.ChangeEntities.Add(new ChangeEntity<IChange>
+        {
+            Change = change,
+            Index = 0,
+            CommitId = commit.Id,
+            EntityId = change.EntityId
+        });
         commit.SetParentHash(Convert.ToHexString(XxHash64.Hash(Guid.NewGuid().ToByteArray())));
         var json = JsonSerializer.Serialize(commit, serializerOptions);
         var commit2 = JsonSerializer.Deserialize<Commit>(json, serializerOptions);

src/SIL.Harmony.Tests/DataModelIntegrityTests.cs

@@ -4,20 +4,6 @@ namespace SIL.Harmony.Tests;
 
 public class DataModelIntegrityTests : DataModelTestBase
 {
-    [Fact]
-    public async Task CanAddTheSameCommitMultipleTimes()
-    {
-        var entity1Id = Guid.NewGuid();
-        var change = SetWord(entity1Id, "entity1");
-        var first = await WriteNextChange(change);
-        await WriteNextChange(SetWord(entity1Id, "entity1.1"));
-        await DataModel.AddChange(_localClientId, change, first.Id);
-        await DataModel.AddChange(_localClientId, change, first.Id);
-
-        var entry = await DataModel.GetLatest<Word>(entity1Id);
-        entry!.Text.Should().Be("entity1.1");
-    }
-
     [Fact]
     public async Task CanAddTheSameCommitMultipleTimesVisSync()
     {

src/SIL.Harmony.Tests/DataModelPerformanceTests.cs

@@ -110,7 +110,7 @@ internal static async Task BulkInsertChanges(DataModelTestBase dataModelTest, in
             var commit = new Commit(commitId)
             {
                 ClientId = Guid.NewGuid(),
-                HybridDateTime = new HybridDateTime(dataModelTest.NextDate(), 0), 
+                HybridDateTime = new HybridDateTime(dataModelTest.NextDate(), 0),
                 ChangeEntities =
                 [
                     new ChangeEntity<IChange>()

src/SIL.Harmony.Tests/DataModelTestBase.cs

@@ -103,15 +103,18 @@ protected async ValueTask<Commit> WriteChange(Guid clientId,
         bool add = true)
     {
         if (!add)
-            return new Commit
+        {
+            var commit = new Commit
             {
                 ClientId = clientId,
                 HybridDateTime = new HybridDateTime(dateTime, 0),
-                ChangeEntities = changes.Select((change, index) => new ChangeEntity<IChange>
-                {
-                    Change = change, Index = index, CommitId = change.CommitId, EntityId = change.EntityId
-                }).ToList()
             };
+            commit.ChangeEntities.AddRange(changes.Select((change, index) => new ChangeEntity<IChange>
+            {
+                Change = change, Index = index, CommitId = commit.Id, EntityId = change.EntityId
+            }));
+            return commit;
+        }
         MockTimeProvider.SetNextDateTime(dateTime);
         return await DataModel.AddChanges(clientId, changes);
     }

src/SIL.Harmony.Tests/PersistExtraDataTests.cs

@@ -23,7 +23,7 @@ public class ExtraDataModel : IObjectBase<ExtraDataModel>
     {
         public Guid Id { get; set; }
         public DateTimeOffset? DeletedAt { get; set; }
-        public Guid CommitId { get; set; }
+        public Guid LastCommitId { get; set; }
         public DateTimeOffset? DateTime { get; set; }
         public long Counter { get; set; }
 
@@ -42,7 +42,7 @@ public IObjectBase Copy()
             {
                 Id = Id,
                 DeletedAt = DeletedAt,
-                CommitId = CommitId,
+                LastCommitId = LastCommitId,
                 DateTime = DateTime,
                 Counter = Counter
             };
@@ -61,7 +61,7 @@ public PersistExtraDataTests()
                 {
                     if (obj is ExtraDataModel extraDataModel)
                     {
-                        extraDataModel.CommitId = snapshot.CommitId;
+                        extraDataModel.LastCommitId = snapshot.CommitId;
                         extraDataModel.DateTime = snapshot.Commit.HybridDateTime.DateTime;
                         extraDataModel.Counter = snapshot.Commit.HybridDateTime.Counter;
                     }
@@ -78,7 +78,7 @@ public async Task CanPersistExtraData()
         var commit = await _dataModelTestBase.WriteNextChange(new CreateExtraDataModelChange(entityId));
         var extraDataModel = _dataModelTestBase.DataModel.QueryLatest<ExtraDataModel>().ToBlockingEnumerable(TestContext.Current.CancellationToken).Should().ContainSingle().Subject;
         extraDataModel.Id.Should().Be(entityId);
-        extraDataModel.CommitId.Should().Be(commit.Id);
+        extraDataModel.LastCommitId.Should().Be(commit.Id);
         extraDataModel.DateTime.Should().Be(commit.HybridDateTime.DateTime);
         extraDataModel.Counter.Should().Be(commit.HybridDateTime.Counter);
     }

src/SIL.Harmony/Changes/Change.cs

@@ -6,9 +6,6 @@ namespace SIL.Harmony.Changes;
 [JsonPolymorphic(TypeDiscriminatorPropertyName = CrdtConstants.ChangeDiscriminatorProperty)]
 public interface IChange
 {
-    [JsonIgnore]
-    Guid CommitId { get; set; }
-
     [JsonIgnore]
     Guid EntityId { get; set; }
 
@@ -41,9 +38,6 @@ protected Change(Guid entityId)
         EntityId = entityId;
     }
 
-    [JsonIgnore]
-    public Guid CommitId { get; set; }
-
     public Guid EntityId { get; set; }
 
     async ValueTask<IObjectBase> IChange.NewEntity(Commit commit, IChangeContext context)

src/SIL.Harmony/DataModel.cs

@@ -47,23 +47,14 @@ internal DataModel(CrdtRepositoryFactory crdtRepositoryFactory,
     /// if the client id changes too much it could slow down the sync process
     /// </param>
     /// <param name="change">change to be applied to the model</param>
-    /// <param name="commitId">
-    /// can be used by the application code to ensure a specific change is only applied once,
-    /// for example a one time migration or update of pre seeded data in the model, a hard coded guid could be used
-    /// which will ensure it's only applied once to the model, even if multiple clients update at the same time and all apply the same change.
-    /// typical changes should not specify the commitId and let a new guid to be generated for each commit.
-    /// This could also be useful if the application has a flaky connection with the DataModel and needs to retry the same change multiple times but ensure it's only applied once,
-    /// then the guid would be generated by the application
-    /// </param>
     /// <param name="commitMetadata">used to store metadata on the commit, for example app version or author id</param>
     /// <returns>the newly created commit</returns>
     public async Task<Commit> AddChange(
         Guid clientId,
         IChange change,
-        Guid commitId = default,
         CommitMetadata? commitMetadata = null)
     {
-        return await AddChanges(clientId, [change], commitId, commitMetadata);
+        return await AddChanges(clientId, [change], commitMetadata);
     }
 
     public async Task AddManyChanges(Guid clientId,
@@ -74,7 +65,7 @@ public async Task AddManyChanges(Guid clientId,
         await using var repo = await _crdtRepositoryFactory.CreateRepository();
         var commits = changes
             .Chunk(changesPerCommitMax)
-            .Select(chunk => NewCommit(Guid.NewGuid(), clientId, commitMetadata(), chunk))
+            .Select(chunk => NewCommit(clientId, commitMetadata(), chunk))
             .ToArray();
         if (commits is []) return;
         using var locked = await repo.Lock();
@@ -91,24 +82,23 @@ public async Task AddManyChanges(Guid clientId,
     public async Task<Commit> AddChanges(
         Guid clientId,
         IEnumerable<IChange> changes,
-        Guid commitId = default,
         CommitMetadata? commitMetadata = null)
     {
-        var commit = NewCommit(commitId, clientId, commitMetadata, changes);
+        var commit = NewCommit(clientId, commitMetadata, changes);
         await Add(commit);
         return commit;
     }
 
-    private Commit NewCommit(Guid commitId, Guid clientId, CommitMetadata? commitMetadata, IEnumerable<IChange> changes)
+    private Commit NewCommit(Guid clientId, CommitMetadata? commitMetadata, IEnumerable<IChange> changes)
     {
-        commitId = commitId == default ? Guid.NewGuid() : commitId;
-        return new Commit(commitId)
+        var commit = new Commit
         {
             ClientId = clientId,
             HybridDateTime = _timeProvider.GetDateTime(),
-            ChangeEntities = [..changes.Select(ToChangeEntity)],
             Metadata = commitMetadata ?? new()
         };
+        commit.ChangeEntities.AddRange(changes.Select((c, i) => ToChangeEntity(c, i, commit.Id)));
+        return commit;
     }
 
     private async Task Add(Commit commit)
@@ -133,11 +123,11 @@ public ValueTask DisposeAsync()
         return ValueTask.CompletedTask;
     }
 
-    private static ChangeEntity<IChange> ToChangeEntity(IChange change, int index)
+    private static ChangeEntity<IChange> ToChangeEntity(IChange change, int index, Guid commitId)
     {
         return new ChangeEntity<IChange>()
         {
-            Change = change, CommitId = change.CommitId, EntityId = change.EntityId, Index = index
+            Change = change, CommitId = commitId, EntityId = change.EntityId, Index = index
         };
     }