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() { diff --git a/src/SIL.Harmony/Changes/ChangeContext.cs b/src/SIL.Harmony/Changes/ChangeContext.cs index 2e9f509..97ab601 100644 --- a/src/SIL.Harmony/Changes/ChangeContext.cs +++ b/src/SIL.Harmony/Changes/ChangeContext.cs @@ -1,18 +1,25 @@ +using SIL.Harmony.Db; + namespace SIL.Harmony.Changes; -public class ChangeContext : IChangeContext +internal 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 acc6248..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 hasBeenApplied = 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) - AddSnapshot(new ObjectSnapshot(updatedEntry, commit, false)); + 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)