Chore/fix sync bug multiple depedent changes (#16)

* create SetDefinitionPartOfSpeechChange, make a FK constraint between words and definitions

* write a failing test due to how saving changes are ordered

* Fix adding intermediate snapshots that reference new entities

---------

Co-authored-by: Tim Haasdyk <[email protected]>

src/SIL.Harmony.Sample/Changes/SetDefinitionPartOfSpeechChange.cs

@@ -0,0 +1,16 @@
+using SIL.Harmony.Changes;
+using SIL.Harmony.Entities;
+using SIL.Harmony.Sample.Models;
+
+namespace SIL.Harmony.Sample.Changes;
+
+public class SetDefinitionPartOfSpeechChange(Guid entityId, string partOfSpeech) : EditChange<Definition>(entityId), ISelfNamedType<SetDefinitionPartOfSpeechChange>
+{
+    public string PartOfSpeech { get; } = partOfSpeech;
+
+    public override ValueTask ApplyChange(Definition entity, ChangeContext context)
+    {
+        entity.PartOfSpeech = PartOfSpeech;
+        return ValueTask.CompletedTask;
+    }
+}

src/SIL.Harmony.Sample/CrdtSampleKernel.cs

@@ -45,13 +45,20 @@ public static IServiceCollection AddCrdtDataSample(this IServiceCollection servi
                 .Add<SetWordNoteChange>()
                 .Add<AddAntonymReferenceChange>()
                 .Add<SetOrderChange<Definition>>()
+                .Add<SetDefinitionPartOfSpeechChange>()
                 .Add<DeleteChange<Word>>()
                 .Add<DeleteChange<Definition>>()
                 .Add<DeleteChange<Example>>()
                 ;
             config.ObjectTypeListBuilder.DefaultAdapter()
                 .Add<Word>()
-                .Add<Definition>()
+                .Add<Definition>(builder =>
+                {
+                    builder.HasOne<Word>()
+                        .WithMany()
+                        .HasForeignKey(d => d.WordId)
+                        .OnDelete(DeleteBehavior.Cascade);
+                })
                 .Add<Example>();
         });
         return services;

src/SIL.Harmony.Tests/DataModelPerformanceTests.cs

@@ -31,7 +31,7 @@ public void AddingChangePerformance()
             );
         foreach (var benchmarkCase in summary.BenchmarksCases.Where(b => !summary.IsBaseline(b)))
         {
-            var ratio = double.Parse(BaselineRatioColumn.RatioMean.GetValue(summary, benchmarkCase));
+            var ratio = double.Parse(BaselineRatioColumn.RatioMean.GetValue(summary, benchmarkCase), System.Globalization.CultureInfo.InvariantCulture);
             //for now it just makes sure that no case is worse that 7x, this is based on the 10_000 test being 5 times worse.
             //it would be better to have this scale off the number of changes
             ratio.Should().BeInRange(0, 7, "performance should not get worse, benchmark " + benchmarkCase.DisplayInfo);

src/SIL.Harmony.Tests/DbContextTests.VerifyModel.verified.txt

@@ -90,13 +90,15 @@
       PartOfSpeech (string) Required
       SnapshotId (no field, Guid?) Shadow FK Index
       Text (string) Required
-      WordId (Guid) Required
+      WordId (Guid) Required FK Index
     Keys: 
       Id PK
     Foreign keys: 
       Definition {'SnapshotId'} -> ObjectSnapshot {'Id'} Unique SetNull
+      Definition {'WordId'} -> Word {'Id'} Required Cascade
     Indexes: 
       SnapshotId Unique
+      WordId
     Annotations: 
       DiscriminatorProperty: 
       Relational:FunctionName: 

src/SIL.Harmony.Tests/SyncTests.cs

@@ -1,4 +1,5 @@
-using SIL.Harmony.Sample.Models;
+using SIL.Harmony.Sample.Changes;
+using SIL.Harmony.Sample.Models;
 
 namespace SIL.Harmony.Tests;
 
@@ -103,4 +104,19 @@ public async Task SyncMultipleClientChanges(int clientCount)
             }
         }
     }
-}
+
+    [Fact]
+    public async Task CanSync_AddDependentWithMultipleChanges()
+    {
+        var entity1Id = Guid.NewGuid();
+        var definitionId = Guid.NewGuid();
+        await _client1.WriteNextChange(_client1.SetWord(entity1Id, "entity1"));
+        await _client1.WriteNextChange(_client1.NewDefinition(entity1Id, "def1", "pos1", definitionId: definitionId));
+        await _client1.WriteNextChange(new SetDefinitionPartOfSpeechChange(definitionId, "pos2"));
+
+        await _client2.DataModel.SyncWith(_client1.DataModel);
+
+        _client2.DataModel.GetLatestObjects<Definition>().Should()
+            .BeEquivalentTo(_client1.DataModel.GetLatestObjects<Definition>());
+    }
+}

src/SIL.Harmony/SnapshotWorker.cs

@@ -16,6 +16,7 @@ internal class SnapshotWorker
     private readonly CrdtRepository _crdtRepository;
     private readonly CrdtConfig _crdtConfig;
     private readonly Dictionary<Guid, ObjectSnapshot> _pendingSnapshots  = [];
+    private readonly Dictionary<Guid, ObjectSnapshot> _rootSnapshots = [];
     private readonly List<ObjectSnapshot> _newIntermediateSnapshots = [];
 
     private SnapshotWorker(Dictionary<Guid, ObjectSnapshot> snapshots,
@@ -56,8 +57,11 @@ public async Task UpdateSnapshots(Commit oldestAddedCommit, Commit[] newCommits)
         var commits = await _crdtRepository.GetCommitsAfter(previousCommit);
         await ApplyCommitChanges(commits.UnionBy(newCommits, c => c.Id), true, previousCommit?.Hash ?? CommitBase.NullParentHash);
 
-        //intermediate snapshots should be added first, as the last snapshot added for an entity will be used in the projected tables
+        // First add any new entities/snapshots as they might be referenced by intermediate snapshots
+        await _crdtRepository.AddSnapshots(_rootSnapshots.Values);
+        // Then add any intermediate snapshots we're hanging onto for performance benefits
         await _crdtRepository.AddIfNew(_newIntermediateSnapshots);
+        // And finally the up-to-date snapshots, which will be used in the projected tables
         await _crdtRepository.AddSnapshots(_pendingSnapshots.Values);
     }
 
@@ -117,7 +121,7 @@ private async ValueTask ApplyCommitChanges(IEnumerable<Commit> commits, bool upd
                 {
                     //do nothing, will cause prevSnapshot to be overriden in _pendingSnapshots if it exists
                 }
-                else if (commitIndex % 2 == 0 || prevSnapshot.IsRoot)
+                else if (commitIndex % 2 == 0 && !prevSnapshot.IsRoot)
                 {
                     intermediateSnapshots[prevSnapshot.Entity.Id] = prevSnapshot;
                 }
@@ -179,6 +183,11 @@ private async ValueTask MarkDeleted(Guid deletedEntityId, Commit commit)
             return snapshot;
         }
 
+        if (_rootSnapshots.TryGetValue(entityId, out var rootSnapshot))
+        {
+            return rootSnapshot;
+        }
+
         if (_snapshotLookup.TryGetValue(entityId, out var snapshotId))
         {
             if (snapshotId is null) return null;
@@ -193,7 +202,14 @@ private async ValueTask MarkDeleted(Guid deletedEntityId, Commit commit)
 
     private void AddSnapshot(ObjectSnapshot snapshot)
     {
-        //if there was already a pending snapshot there's no need to store it as both may point to the same commit
-        _pendingSnapshots[snapshot.Entity.Id] = snapshot;
+        if (snapshot.IsRoot)
+        {
+            _rootSnapshots[snapshot.Entity.Id] = snapshot;
+        }
+        else
+        {
+            //if there was already a pending snapshot there's no need to store it as both may point to the same commit
+            _pendingSnapshots[snapshot.Entity.Id] = snapshot;
+        }
     }
 }