From 88b152d6fe78aad02b7cbe4ab2a39a0bed05c057 Mon Sep 17 00:00:00 2001 From: Tim Haasdyk Date: Wed, 19 Mar 2025 13:07:45 +0100 Subject: [PATCH 1/4] Add failing tests --- .../DataModelReferenceTests.cs | 42 +++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/src/SIL.Harmony.Tests/DataModelReferenceTests.cs b/src/SIL.Harmony.Tests/DataModelReferenceTests.cs index 12d5fa1..012648e 100644 --- a/src/SIL.Harmony.Tests/DataModelReferenceTests.cs +++ b/src/SIL.Harmony.Tests/DataModelReferenceTests.cs @@ -85,6 +85,48 @@ await AddCommitsViaSync([ wordWithoutRef.AntonymId.Should().BeNull(); } + [Fact] + public async Task AddAndDeleteInSameCommitDeletesRefs() + { + var wordId = Guid.NewGuid(); + var definitionId = Guid.NewGuid(); + + await WriteNextChange( + [ + SetWord(wordId, "original"), + NewDefinition(wordId, "the shiny one everything started with", "adj", 0, definitionId), + new DeleteChange(wordId), + ]); + + var def = await DataModel.GetLatest(definitionId); + def.Should().NotBeNull(); + def.DeletedAt.Should().NotBeNull(); + } + + [Fact] + public async Task UpdateAndDeleteParentInSameCommitWorks() + { + var wordId = Guid.NewGuid(); + var definitionId = Guid.NewGuid(); + + await WriteNextChange( + [ + SetWord(wordId, "original"), + NewDefinition(wordId, "the shiny one everything started with", "adj", 0, definitionId), + ]); + + await WriteNextChange( + [ + new SetDefinitionPartOfSpeechChange(definitionId, "pos2"), + new DeleteChange(wordId), + ]); + + var def = await DataModel.GetLatest(definitionId); + def.Should().NotBeNull(); + def.PartOfSpeech.Should().Be("pos2"); + def.DeletedAt.Should().NotBeNull(); + } + [Fact] public async Task AddAndDeleteInSameSyncDeletesRefs() { From 4d4a2be723bb9ea6ab308ee0ef1342bdee8c1393 Mon Sep 17 00:00:00 2001 From: Tim Haasdyk Date: Wed, 19 Mar 2025 13:10:11 +0100 Subject: [PATCH 2/4] Fix bug --- src/SIL.Harmony/SnapshotWorker.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/SIL.Harmony/SnapshotWorker.cs b/src/SIL.Harmony/SnapshotWorker.cs index acc6248..5181eb2 100644 --- a/src/SIL.Harmony/SnapshotWorker.cs +++ b/src/SIL.Harmony/SnapshotWorker.cs @@ -146,17 +146,17 @@ private async ValueTask MarkDeleted(Guid deletedEntityId, Commit commit) foreach (var snapshot in toRemoveRefFrom) { - var hasBeenApplied = snapshot.CommitId == commit.Id; + var commited = snapshot.CommitId != commit.Id; var updatedEntry = snapshot.Entity.Copy(); var wasDeleted = updatedEntry.DeletedAt.HasValue; updatedEntry.RemoveReference(deletedEntityId, commit); var deletedByRemoveRef = !wasDeleted && updatedEntry.DeletedAt.HasValue; - //this snapshot has already been applied, we don't need to add it again - //but we did need to run apply again because we may need to mark other entities as deleted - if (!hasBeenApplied) + if (commited) // add a new snapshot AddSnapshot(new ObjectSnapshot(updatedEntry, commit, false)); + else // replace the uncommited one + AddSnapshot(new ObjectSnapshot(updatedEntry, commit, snapshot.IsRoot)); //we need to do this after we add the snapshot above otherwise we might get stuck in a loop of deletions if (deletedByRemoveRef) From b7ebb3ddcf92a60c6dc5f82dc08ed1b58f96983b Mon Sep 17 00:00:00 2001 From: Tim Haasdyk Date: Wed, 19 Mar 2025 13:29:15 +0100 Subject: [PATCH 3/4] Unify snapshot creation logic --- src/SIL.Harmony/Changes/ChangeContext.cs | 11 +++- src/SIL.Harmony/SnapshotWorker.cs | 72 ++++++++++++------------ 2 files changed, 46 insertions(+), 37 deletions(-) diff --git a/src/SIL.Harmony/Changes/ChangeContext.cs b/src/SIL.Harmony/Changes/ChangeContext.cs index 2e9f509..e00d721 100644 --- a/src/SIL.Harmony/Changes/ChangeContext.cs +++ b/src/SIL.Harmony/Changes/ChangeContext.cs @@ -1,3 +1,5 @@ +using SIL.Harmony.Db; + namespace SIL.Harmony.Changes; public class ChangeContext : IChangeContext @@ -5,14 +7,19 @@ public class ChangeContext : IChangeContext private readonly SnapshotWorker _worker; private readonly CrdtConfig _crdtConfig; - internal ChangeContext(Commit commit, SnapshotWorker worker, CrdtConfig crdtConfig) + internal ChangeContext(Commit commit, int commitIndex, IDictionary intermediateSnapshots, SnapshotWorker worker, CrdtConfig crdtConfig) { _worker = worker; _crdtConfig = crdtConfig; Commit = commit; + CommitIndex = commitIndex; + IntermediateSnapshots = intermediateSnapshots; } - public CommitBase Commit { get; } + CommitBase IChangeContext.Commit => Commit; + public Commit Commit { get; } + public int CommitIndex { get; } + public IDictionary IntermediateSnapshots { get; } public async ValueTask GetSnapshot(Guid entityId) => await _worker.GetSnapshot(entityId); public IAsyncEnumerable GetObjectsReferencing(Guid entityId, bool includeDeleted = false) { diff --git a/src/SIL.Harmony/SnapshotWorker.cs b/src/SIL.Harmony/SnapshotWorker.cs index 5181eb2..3a74225 100644 --- a/src/SIL.Harmony/SnapshotWorker.cs +++ b/src/SIL.Harmony/SnapshotWorker.cs @@ -80,7 +80,7 @@ private async ValueTask ApplyCommitChanges(IEnumerable commits, bool upd { IObjectBase entity; var prevSnapshot = await GetSnapshot(commitChange.EntityId); - var changeContext = new ChangeContext(commit, this, _crdtConfig); + var changeContext = new ChangeContext(commit, commitIndex, intermediateSnapshots, this, _crdtConfig); bool wasDeleted; if (prevSnapshot is not null) { @@ -98,34 +98,10 @@ private async ValueTask ApplyCommitChanges(IEnumerable commits, bool upd var deletedByChange = !wasDeleted && entity.DeletedAt.HasValue; if (deletedByChange) { - await MarkDeleted(entity.Id, commit); + await MarkDeleted(entity.Id, changeContext); } - - //to get the state in a point in time we would have to find a snapshot before that time, then apply any commits that came after that snapshot but still before the point in time. - //we would probably want the most recent snapshot to always follow current, so we might need to track the number of changes a given snapshot represents so we can - //decide when to create a new snapshot instead of replacing one inline. This would be done by using the current snapshots parent, instead of the snapshot itself. - // s0 -> s1 -> sCurrent - // if always taking snapshots would become - // s0 -> s1 -> sCurrent -> sNew - //but but to not snapshot every change we could do this instead - // s0 -> s1 -> sNew - - //when both snapshots are for the same commit we don't want to keep the previous, therefore the new snapshot should be root - var isRoot = prevSnapshot is null || (prevSnapshot.IsRoot && prevSnapshot.CommitId == commit.Id); - var newSnapshot = new ObjectSnapshot(entity, commit, isRoot); - //if both snapshots are for the same commit then we don't want to keep the previous snapshot - if (prevSnapshot is null || prevSnapshot.CommitId == commit.Id) - { - //do nothing, will cause prevSnapshot to be overriden in _pendingSnapshots if it exists - } - else if (commitIndex % 2 == 0 && !prevSnapshot.IsRoot && IsNew(prevSnapshot)) - { - intermediateSnapshots[prevSnapshot.Entity.Id] = prevSnapshot; - } - - await _crdtConfig.BeforeSaveObject.Invoke(entity.DbObject, newSnapshot); - - AddSnapshot(newSnapshot); + + await GenerateSnapshotForEntity(entity, prevSnapshot, changeContext); } _newIntermediateSnapshots.AddRange(intermediateSnapshots.Values); intermediateSnapshots.Clear(); @@ -137,31 +113,28 @@ private async ValueTask ApplyCommitChanges(IEnumerable commits, bool upd /// /// /// - private async ValueTask MarkDeleted(Guid deletedEntityId, Commit commit) + private async ValueTask MarkDeleted(Guid deletedEntityId, ChangeContext context) { // Including deleted shouldn't be necessary, because change objects are responsible for not adding references to deleted entities. // But maybe it's a good fallback. var toRemoveRefFrom = await GetSnapshotsReferencing(deletedEntityId, true) .ToArrayAsync(); + var commit = context.Commit; foreach (var snapshot in toRemoveRefFrom) { - var commited = snapshot.CommitId != commit.Id; var updatedEntry = snapshot.Entity.Copy(); var wasDeleted = updatedEntry.DeletedAt.HasValue; updatedEntry.RemoveReference(deletedEntityId, commit); var deletedByRemoveRef = !wasDeleted && updatedEntry.DeletedAt.HasValue; - if (commited) // add a new snapshot - AddSnapshot(new ObjectSnapshot(updatedEntry, commit, false)); - else // replace the uncommited one - AddSnapshot(new ObjectSnapshot(updatedEntry, commit, snapshot.IsRoot)); + await GenerateSnapshotForEntity(updatedEntry, snapshot, context); //we need to do this after we add the snapshot above otherwise we might get stuck in a loop of deletions if (deletedByRemoveRef) { - await MarkDeleted(updatedEntry.Id, commit); + await MarkDeleted(updatedEntry.Id, context); } } } @@ -223,6 +196,35 @@ internal async IAsyncEnumerable GetSnapshotsWhere(Expression s1 -> sCurrent + // if always taking snapshots would become + // s0 -> s1 -> sCurrent -> sNew + //but but to not snapshot every change we could do this instead + // s0 -> s1 -> sNew + + //when both snapshots are for the same commit we don't want to keep the previous, therefore the new snapshot should be root + var isRoot = prevSnapshot is null || (prevSnapshot.IsRoot && prevSnapshot.CommitId == context.Commit.Id); + var newSnapshot = new ObjectSnapshot(entity, context.Commit, isRoot); + //if both snapshots are for the same commit then we don't want to keep the previous snapshot + if (prevSnapshot is null || prevSnapshot.CommitId == context.Commit.Id) + { + //do nothing, will cause prevSnapshot to be overriden in _pendingSnapshots if it exists + } + else if (context.CommitIndex % 2 == 0 && !prevSnapshot.IsRoot && IsNew(prevSnapshot)) + { + context.IntermediateSnapshots[prevSnapshot.Entity.Id] = prevSnapshot; + } + + await _crdtConfig.BeforeSaveObject.Invoke(entity.DbObject, newSnapshot); + + AddSnapshot(newSnapshot); + } + private void AddSnapshot(ObjectSnapshot snapshot) { if (snapshot.IsRoot) From 4009ed779f167968099255002ea8f72853ed4a2f Mon Sep 17 00:00:00 2001 From: Tim Haasdyk Date: Mon, 24 Mar 2025 14:00:25 +0100 Subject: [PATCH 4/4] Make ChangeContext class internal --- src/SIL.Harmony/Changes/ChangeContext.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/SIL.Harmony/Changes/ChangeContext.cs b/src/SIL.Harmony/Changes/ChangeContext.cs index e00d721..97ab601 100644 --- a/src/SIL.Harmony/Changes/ChangeContext.cs +++ b/src/SIL.Harmony/Changes/ChangeContext.cs @@ -2,7 +2,7 @@ namespace SIL.Harmony.Changes; -public class ChangeContext : IChangeContext +internal class ChangeContext : IChangeContext { private readonly SnapshotWorker _worker; private readonly CrdtConfig _crdtConfig;