From a72f7e60ffdd9f664423ec133ec516213b0da71a Mon Sep 17 00:00:00 2001 From: "google-labs-jules[bot]" <161369871+google-labs-jules[bot]@users.noreply.github.com> Date: Thu, 11 Jun 2026 03:49:21 +0000 Subject: [PATCH 1/2] Prevent clients from setting CommitId - Removed commitId parameter from DataModel.AddChange and AddChanges - Removed CommitId property from IChange interface and implementations - Updated tests to match the new API - Removed tests that relied on client-provided commitId Co-authored-by: hahn-kev <4575355+hahn-kev@users.noreply.github.com> --- src/SIL.Harmony.Tests/CommitTests.cs | 17 +++++++-------- .../DataModelIntegrityTests.cs | 14 ------------- .../DataModelPerformanceTests.cs | 19 +++++++---------- src/SIL.Harmony.Tests/DataModelTestBase.cs | 13 +++++++----- .../PersistExtraDataTests.cs | 8 +++---- src/SIL.Harmony/Changes/Change.cs | 6 ------ src/SIL.Harmony/DataModel.cs | 21 +++++-------------- 7 files changed, 32 insertions(+), 66 deletions(-) diff --git a/src/SIL.Harmony.Tests/CommitTests.cs b/src/SIL.Harmony.Tests/CommitTests.cs index e24d6c2..660f3ca 100644 --- a/src/SIL.Harmony.Tests/CommitTests.cs +++ b/src/SIL.Harmony.Tests/CommitTests.cs @@ -114,17 +114,14 @@ public void CanRoundTripCommitThroughJson() { ClientId = Guid.NewGuid(), HybridDateTime = Now(), - ChangeEntities = - { - new ChangeEntity - { - Change = change, - Index = 0, - CommitId = change.CommitId, - EntityId = change.EntityId - } - } }; + commit.ChangeEntities.Add(new ChangeEntity + { + 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(json, serializerOptions); diff --git a/src/SIL.Harmony.Tests/DataModelIntegrityTests.cs b/src/SIL.Harmony.Tests/DataModelIntegrityTests.cs index 9cacb88..95ab2f9 100644 --- a/src/SIL.Harmony.Tests/DataModelIntegrityTests.cs +++ b/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(entity1Id); - entry!.Text.Should().Be("entity1.1"); - } - [Fact] public async Task CanAddTheSameCommitMultipleTimesVisSync() { diff --git a/src/SIL.Harmony.Tests/DataModelPerformanceTests.cs b/src/SIL.Harmony.Tests/DataModelPerformanceTests.cs index 84398e5..67e03fc 100644 --- a/src/SIL.Harmony.Tests/DataModelPerformanceTests.cs +++ b/src/SIL.Harmony.Tests/DataModelPerformanceTests.cs @@ -108,18 +108,15 @@ internal static async Task BulkInsertChanges(DataModelTestBase dataModelTest, in var commit = new Commit(commitId) { ClientId = Guid.NewGuid(), - HybridDateTime = new HybridDateTime(dataModelTest.NextDate(), 0), - ChangeEntities = - [ - new ChangeEntity() - { - Change = change, - Index = 0, - CommitId = commitId, - EntityId = change.EntityId - } - ] + HybridDateTime = new HybridDateTime(dataModelTest.NextDate(), 0), }; + commit.ChangeEntities.Add(new ChangeEntity() + { + Change = change, + Index = 0, + CommitId = commit.Id, + EntityId = change.EntityId + }); commit.SetParentHash(parentHash); parentHash = commit.Hash; dataModelTest.DbContext.Add(commit); diff --git a/src/SIL.Harmony.Tests/DataModelTestBase.cs b/src/SIL.Harmony.Tests/DataModelTestBase.cs index ea4b305..7c9a0bb 100644 --- a/src/SIL.Harmony.Tests/DataModelTestBase.cs +++ b/src/SIL.Harmony.Tests/DataModelTestBase.cs @@ -105,15 +105,18 @@ protected async ValueTask 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 - { - Change = change, Index = index, CommitId = change.CommitId, EntityId = change.EntityId - }).ToList() }; + commit.ChangeEntities.AddRange(changes.Select((change, index) => new ChangeEntity + { + Change = change, Index = index, CommitId = commit.Id, EntityId = change.EntityId + })); + return commit; + } MockTimeProvider.SetNextDateTime(dateTime); return await DataModel.AddChanges(clientId, changes); } diff --git a/src/SIL.Harmony.Tests/PersistExtraDataTests.cs b/src/SIL.Harmony.Tests/PersistExtraDataTests.cs index efc5eb4..b46a588 100644 --- a/src/SIL.Harmony.Tests/PersistExtraDataTests.cs +++ b/src/SIL.Harmony.Tests/PersistExtraDataTests.cs @@ -23,7 +23,7 @@ public class ExtraDataModel : IObjectBase { 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().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); } diff --git a/src/SIL.Harmony/Changes/Change.cs b/src/SIL.Harmony/Changes/Change.cs index 3695c95..965639f 100644 --- a/src/SIL.Harmony/Changes/Change.cs +++ b/src/SIL.Harmony/Changes/Change.cs @@ -5,9 +5,6 @@ namespace SIL.Harmony.Changes; [JsonPolymorphic(TypeDiscriminatorPropertyName = CrdtConstants.ChangeDiscriminatorProperty)] public interface IChange { - [JsonIgnore] - Guid CommitId { get; set; } - [JsonIgnore] Guid EntityId { get; set; } @@ -29,9 +26,6 @@ protected Change(Guid entityId) EntityId = entityId; } - [JsonIgnore] - public Guid CommitId { get; set; } - public Guid EntityId { get; set; } async ValueTask IChange.NewEntity(Commit commit, IChangeContext context) diff --git a/src/SIL.Harmony/DataModel.cs b/src/SIL.Harmony/DataModel.cs index c0c0915..be640d1 100644 --- a/src/SIL.Harmony/DataModel.cs +++ b/src/SIL.Harmony/DataModel.cs @@ -51,41 +51,30 @@ internal DataModel(CrdtRepository crdtRepository, /// if the client id changes too much it could slow down the sync process /// /// change to be applied to the model - /// - /// 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 - /// /// used to store metadata on the commit, for example app version or author id /// the newly created commit public async Task 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 AddChanges( Guid clientId, IEnumerable changes, - Guid commitId = default, CommitMetadata? commitMetadata = null, bool deferCommit = false) { - commitId = commitId == default ? Guid.NewGuid() : commitId; - var commit = 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))); await Add(commit, deferCommit); return commit; } @@ -131,11 +120,11 @@ public async Task FlushDeferredCommits() } - private static ChangeEntity ToChangeEntity(IChange change, int index) + private static ChangeEntity ToChangeEntity(IChange change, int index, Guid commitId) { return new ChangeEntity() { - Change = change, CommitId = change.CommitId, EntityId = change.EntityId, Index = index + Change = change, CommitId = commitId, EntityId = change.EntityId, Index = index }; } From 8c876ee93b1203b966eed13bb0223d3a98196aea Mon Sep 17 00:00:00 2001 From: Kevin Hahn Date: Fri, 12 Jun 2026 09:11:31 +0700 Subject: [PATCH 2/2] revert unneded change --- .../DataModelPerformanceTests.cs | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/SIL.Harmony.Tests/DataModelPerformanceTests.cs b/src/SIL.Harmony.Tests/DataModelPerformanceTests.cs index 3ce0219..3e97ac8 100644 --- a/src/SIL.Harmony.Tests/DataModelPerformanceTests.cs +++ b/src/SIL.Harmony.Tests/DataModelPerformanceTests.cs @@ -111,14 +111,17 @@ internal static async Task BulkInsertChanges(DataModelTestBase dataModelTest, in { ClientId = Guid.NewGuid(), HybridDateTime = new HybridDateTime(dataModelTest.NextDate(), 0), + ChangeEntities = + [ + new ChangeEntity() + { + Change = change, + Index = 0, + CommitId = commitId, + EntityId = change.EntityId + } + ] }; - commit.ChangeEntities.Add(new ChangeEntity() - { - Change = change, - Index = 0, - CommitId = commit.Id, - EntityId = change.EntityId - }); commit.SetParentHash(parentHash); parentHash = commit.Hash; dataModelTest.DbContext.Add(commit);