diff --git a/src/SIL.Harmony.Tests/DataModelSimpleChanges.cs b/src/SIL.Harmony.Tests/DataModelSimpleChanges.cs index 284d2cc..7d989c1 100644 --- a/src/SIL.Harmony.Tests/DataModelSimpleChanges.cs +++ b/src/SIL.Harmony.Tests/DataModelSimpleChanges.cs @@ -190,17 +190,35 @@ public async Task CanDeleteAnEntry() } [Fact] - public async Task CanModifyAnEntryAfterDelete() + public async Task ApplyChange_ModifiesADeletedEntry() { await WriteNextChange(SetWord(_entity1Id, "test-value")); var deleteCommit = await WriteNextChange(new DeleteChange(_entity1Id)); - await WriteNextChange(SetWord(_entity1Id, "after-delete")); + var newNoteChange = new SetWordNoteChange(_entity1Id, "note-after-delete"); + newNoteChange.SupportsApplyChange().Should().BeTrue(); + newNoteChange.SupportsNewEntity().Should().BeFalse(); // otherwise it will override the delete + await WriteNextChange(newNoteChange); var snapshot = await DbContext.Snapshots.DefaultOrder().LastAsync(); var word = snapshot.Entity.Is(); - word.Text.Should().Be("after-delete"); + word.Text.Should().Be("test-value"); + word.Note.Should().Be("note-after-delete"); word.DeletedAt.Should().Be(deleteCommit.DateTime); } + [Fact] + public async Task NewEntity_UndeletesAnEntry() + { + await WriteNextChange(SetWord(_entity1Id, "test-value")); + await WriteNextChange(new DeleteChange(_entity1Id)); + var recreateChange = SetWord(_entity1Id, "after-undo-delete"); + recreateChange.SupportsNewEntity().Should().BeTrue(); + await WriteNextChange(recreateChange); + var snapshot = await DbContext.Snapshots.DefaultOrder().LastAsync(); + var word = snapshot.Entity.Is(); + word.Text.Should().Be("after-undo-delete"); + word.DeletedAt.Should().Be(null); + } + [Fact] public async Task ChangesToSnapshotsAreNotSaved() { @@ -208,10 +226,10 @@ public async Task ChangesToSnapshotsAreNotSaved() var word = await DataModel.GetLatest(_entity1Id); word!.Text.Should().Be("test-value"); word.Note.Should().BeNull(); - + //change made outside the model, should not be saved when writing the next change word.Note = "a note"; - + var commit = await WriteNextChange(SetWord(_entity1Id, "after-change")); var objectSnapshot = commit.Snapshots.Should().ContainSingle().Subject; objectSnapshot.Entity.Is().Text.Should().Be("after-change"); diff --git a/src/SIL.Harmony.Tests/DeleteAndCreateTests.cs b/src/SIL.Harmony.Tests/DeleteAndCreateTests.cs new file mode 100644 index 0000000..409f8bd --- /dev/null +++ b/src/SIL.Harmony.Tests/DeleteAndCreateTests.cs @@ -0,0 +1,215 @@ +using Microsoft.EntityFrameworkCore; +using SIL.Harmony.Changes; +using SIL.Harmony.Sample.Changes; +using SIL.Harmony.Sample.Models; + +namespace SIL.Harmony.Tests; + +public class DeleteAndCreateTests : DataModelTestBase +{ + [Fact] + public async Task DeleteAndUndeleteInSameCommitWorks() + { + var wordId = Guid.NewGuid(); + + await WriteNextChange(new NewWordChange(wordId, "original")); + + await WriteNextChange( + [ + new DeleteChange(wordId), + new NewWordChange(wordId, "Undeleted"), + ]); + + var word = await DataModel.GetLatest(wordId); + word.Should().NotBeNull(); + word.DeletedAt.Should().BeNull(); + word.Text.Should().Be("Undeleted"); + + var entityWord = await DataModel.QueryLatest().Where(w => w.Id == wordId).SingleOrDefaultAsync(); + entityWord.Should().NotBeNull(); + entityWord.DeletedAt.Should().BeNull(); + entityWord.Text.Should().Be("Undeleted"); + } + + [Fact] + public async Task DeleteAndUndeleteInSameSyncWorks() + { + var wordId = Guid.NewGuid(); + + await WriteNextChange(new NewWordChange(wordId, "original")); + + await AddCommitsViaSync([ + await WriteNextChange(new DeleteChange(wordId), add: false), + await WriteNextChange(new NewWordChange(wordId, "Undeleted"), add: false), + ]); + + var word = await DataModel.GetLatest(wordId); + word.Should().NotBeNull(); + word.DeletedAt.Should().BeNull(); + word.Text.Should().Be("Undeleted"); + + var entityWord = await DataModel.QueryLatest().Where(w => w.Id == wordId).SingleOrDefaultAsync(); + entityWord.Should().NotBeNull(); + entityWord.DeletedAt.Should().BeNull(); + entityWord.Text.Should().Be("Undeleted"); + } + + [Fact] + public async Task UpdateAndUndeleteInSameCommitWorks() + { + var wordId = Guid.NewGuid(); + + await WriteNextChange(new NewWordChange(wordId, "original")); + await WriteNextChange(new DeleteChange(wordId)); + + await WriteNextChange([ + new SetWordNoteChange(wordId, "overridden-note"), + new NewWordChange(wordId, "Undeleted"), + ]); + + var word = await DataModel.GetLatest(wordId); + word.Should().NotBeNull(); + word.DeletedAt.Should().BeNull(); + word.Text.Should().Be("Undeleted"); + + var entityWord = await DataModel.QueryLatest().Where(w => w.Id == wordId).SingleOrDefaultAsync(); + entityWord.Should().NotBeNull(); + entityWord.DeletedAt.Should().BeNull(); + entityWord.Text.Should().Be("Undeleted"); + } + + [Fact] + public async Task UpdateAndUndeleteInSameSyncWorks() + { + var wordId = Guid.NewGuid(); + + await WriteNextChange(new NewWordChange(wordId, "original")); + await WriteNextChange(new DeleteChange(wordId)); + + await AddCommitsViaSync([ + await WriteNextChange(new SetWordNoteChange(wordId, "overridden-note"), add: false), + await WriteNextChange(new NewWordChange(wordId, "Undeleted"), add: false), + ]); + + var word = await DataModel.GetLatest(wordId); + word.Should().NotBeNull(); + word.DeletedAt.Should().BeNull(); + word.Text.Should().Be("Undeleted"); + + var entityWord = await DataModel.QueryLatest().Where(w => w.Id == wordId).SingleOrDefaultAsync(); + entityWord.Should().NotBeNull(); + entityWord.DeletedAt.Should().BeNull(); + entityWord.Text.Should().Be("Undeleted"); + } + + [Fact] + public async Task CreateDeleteAndUndeleteInSameCommitWorks() + { + var wordId = Guid.NewGuid(); + + await WriteNextChange( + [ + new NewWordChange(wordId, "original"), + new DeleteChange(wordId), + new NewWordChange(wordId, "Undeleted"), + ]); + + var word = await DataModel.GetLatest(wordId); + word.Should().NotBeNull(); + word.DeletedAt.Should().BeNull(); + word.Text.Should().Be("Undeleted"); + + var entityWord = await DataModel.QueryLatest().Where(w => w.Id == wordId).SingleOrDefaultAsync(); + entityWord.Should().NotBeNull(); + entityWord.DeletedAt.Should().BeNull(); + entityWord.Text.Should().Be("Undeleted"); + } + + [Fact] + public async Task CreateDeleteAndUndeleteInSameSyncWorks() + { + var wordId = Guid.NewGuid(); + + await AddCommitsViaSync([ + await WriteNextChange(new NewWordChange(wordId, "original"), add: false), + await WriteNextChange(new DeleteChange(wordId), add: false), + await WriteNextChange(new NewWordChange(wordId, "Undeleted"), add: false), + ]); + + var word = await DataModel.GetLatest(wordId); + word.Should().NotBeNull(); + word.DeletedAt.Should().BeNull(); + word.Text.Should().Be("Undeleted"); + + var entityWord = await DataModel.QueryLatest().Where(w => w.Id == wordId).SingleOrDefaultAsync(); + entityWord.Should().NotBeNull(); + entityWord.DeletedAt.Should().BeNull(); + entityWord.Text.Should().Be("Undeleted"); + } + + [Fact] + public async Task CreateAndDeleteInSameCommitWorks() + { + var wordId = Guid.NewGuid(); + + await WriteNextChange( + [ + new NewWordChange(wordId, "original"), + new DeleteChange(wordId), + ]); + + var word = await DataModel.GetLatest(wordId); + word.Should().NotBeNull(); + word.DeletedAt.Should().NotBeNull(); + word.Text.Should().Be("original"); + + var entityWord = await DataModel.QueryLatest().Where(w => w.Id == wordId).SingleOrDefaultAsync(); + entityWord.Should().BeNull(); + } + + [Fact] + public async Task CreateAndDeleteInSameSyncWorks() + { + var wordId = Guid.NewGuid(); + + await AddCommitsViaSync([ + await WriteNextChange(new NewWordChange(wordId, "original"), add: false), + await WriteNextChange(new DeleteChange(wordId), add: false), + ]); + + var word = await DataModel.GetLatest(wordId); + word.Should().NotBeNull(); + word.DeletedAt.Should().NotBeNull(); + word.Text.Should().Be("original"); + + var entityWord = await DataModel.QueryLatest().Where(w => w.Id == wordId).SingleOrDefaultAsync(); + entityWord.Should().BeNull(); + } + + [Fact] + public async Task NewEntityOnExistingEntityIsNoOp() + { + var wordId = Guid.NewGuid(); + + await WriteNextChange(new NewWordChange(wordId, "original")); + var snapshotsBefore = await DbContext.Snapshots.Where(s => s.EntityId == wordId).ToArrayAsync(); + + await WriteNextChange( + [ + new NewWordChange(wordId, "Undeleted"), + ]); + + var word = await DataModel.GetLatest(wordId); + word.Should().NotBeNull(); + word.DeletedAt.Should().BeNull(); + word.Text.Should().Be("original"); + + var entityWord = await DataModel.QueryLatest().Where(w => w.Id == wordId).SingleOrDefaultAsync(); + entityWord.Should().NotBeNull(); + entityWord.DeletedAt.Should().BeNull(); + entityWord.Text.Should().Be("original"); + + var snapshotsAfter = await DbContext.Snapshots.Where(s => s.EntityId == wordId).ToArrayAsync(); + snapshotsAfter.Select(s => s.Id).Should().BeEquivalentTo(snapshotsBefore.Select(s => s.Id)); + } +} diff --git a/src/SIL.Harmony.Tests/SnapshotTests.cs b/src/SIL.Harmony.Tests/SnapshotTests.cs index 248d574..1a4ff0e 100644 --- a/src/SIL.Harmony.Tests/SnapshotTests.cs +++ b/src/SIL.Harmony.Tests/SnapshotTests.cs @@ -1,4 +1,6 @@ using SIL.Harmony.Sample.Models; +using SIL.Harmony.Sample.Changes; +using SIL.Harmony.Changes; using Microsoft.EntityFrameworkCore; namespace SIL.Harmony.Tests; @@ -19,7 +21,7 @@ public async Task MultipleChangesPreservesRootSnapshot() { var entityId = Guid.NewGuid(); var commits = new List(); - for (int i = 0; i < 4; i++) + for (var i = 0; i < 4; i++) { commits.Add(await WriteChange(_localClientId, new DateTimeOffset(2000, 1, 1, 0, 0, 0, TimeSpan.Zero).AddHours(i), @@ -39,7 +41,7 @@ public async Task MultipleChangesPreservesSomeIntermediateSnapshots() { var entityId = Guid.NewGuid(); var commits = new List(); - for (int i = 0; i < 6; i++) + for (var i = 0; i < 6; i++) { commits.Add(await WriteChange(_localClientId, new DateTimeOffset(2000, 1, 1, 0, 0, 0, TimeSpan.Zero).AddHours(i), @@ -135,7 +137,7 @@ await WriteNextChange( var tagCreation = await WriteNextChange(TagWord(wordId, tagId)); await WriteChangeBefore(tagCreation, TagWord(wordId, tagId)); - var word = await DataModel.QueryLatest(q=> q.Include(w => w.Tags) + var word = await DataModel.QueryLatest(q => q.Include(w => w.Tags) .Where(w => w.Id == wordId)).FirstOrDefaultAsync(); word.Should().NotBeNull(); word.Tags.Should().BeEquivalentTo([new Tag { Id = tagId, Text = "tag-1" }]); diff --git a/src/SIL.Harmony/Changes/Change.cs b/src/SIL.Harmony/Changes/Change.cs index 3695c95..6908536 100644 --- a/src/SIL.Harmony/Changes/Change.cs +++ b/src/SIL.Harmony/Changes/Change.cs @@ -1,3 +1,4 @@ +using System.Diagnostics; using System.Text.Json.Serialization; namespace SIL.Harmony.Changes; @@ -16,6 +17,17 @@ public interface IChange ValueTask ApplyChange(IObjectBase entity, IChangeContext context); ValueTask NewEntity(Commit commit, IChangeContext context); + /// + /// Indicates whether this change supports applying changes to an existing entity (whether deleted or not). + /// Essentially just avoids creating redundant snapshots for change objects that won't apply changes. + /// (e.g. redundant change objects intended only for NewEntity) + /// + bool SupportsApplyChange(); + /// + /// Indicates whether this change supports creating entities (both creating brand new entities as well as recreating deleted entities). + /// Primarily for differentiating between updating vs recreating deleted entities. + /// + bool SupportsNewEntity(); } /// @@ -44,8 +56,12 @@ async ValueTask IChange.NewEntity(Commit commit, IChangeContext con public async ValueTask ApplyChange(IObjectBase entity, IChangeContext context) { - if (this is CreateChange) + if (!SupportsApplyChange()) + { + Debug.Fail("ApplyChange called on a Change that does not support it"); return; // skip attempting to apply changes on CreateChange as it does not support apply changes + } + if (entity.DbObject is T entityT) { await ApplyChange(entityT, context); @@ -56,6 +72,16 @@ public async ValueTask ApplyChange(IObjectBase entity, IChangeContext context) } } + public virtual bool SupportsApplyChange() + { + return this is not CreateChange; + } + + public virtual bool SupportsNewEntity() + { + return this is not EditChange; + } + [JsonIgnore] public Type EntityType => typeof(T); } diff --git a/src/SIL.Harmony/Db/CrdtRepository.cs b/src/SIL.Harmony/Db/CrdtRepository.cs index a7145cb..98d1210 100644 --- a/src/SIL.Harmony/Db/CrdtRepository.cs +++ b/src/SIL.Harmony/Db/CrdtRepository.cs @@ -299,16 +299,24 @@ public async Task> GetChanges(SyncState remoteState) public async Task AddSnapshots(IEnumerable snapshots) { - var projectedEntityIds = new HashSet(); + var latestProjectByEntityId = new Dictionary(); foreach (var grouping in snapshots.GroupBy(s => s.EntityIsDeleted).OrderByDescending(g => g.Key))//execute deletes first { foreach (var snapshot in grouping.DefaultOrderDescending()) { _dbContext.Add(snapshot); - if (projectedEntityIds.Add(snapshot.EntityId)) + if (latestProjectByEntityId.TryGetValue(snapshot.EntityId, out var latestProjected)) { - await ProjectSnapshot(snapshot); + // there might be a deleted and un-deleted snapshot for the same entity in the same batch + // in that case there's only a 50% chance that they're in the right order, so we need to explicitly only project the latest one + if (snapshot.Commit.CompareKey.CompareTo(latestProjected) < 0) + { + continue; + } } + latestProjectByEntityId[snapshot.EntityId] = snapshot.Commit.CompareKey; + + await ProjectSnapshot(snapshot); } try @@ -323,6 +331,20 @@ public async Task AddSnapshots(IEnumerable snapshots) throw new DbUpdateException(message, e); } } + + // this extra try/catch was added as a quick way to get the NewEntityOnExistingEntityIsNoOp test to pass + // it will be removed again in a larger refactor in https://github.com/sillsdev/harmony/pull/56 + try + { + await _dbContext.SaveChangesAsync(); + } + catch (DbUpdateException e) + { + var entries = string.Join(Environment.NewLine, e.Entries.Select(entry => entry.ToString())); + var message = $"Error saving snapshots: {e.Message}{Environment.NewLine}{entries}"; + _logger.LogError(e, message); + throw new DbUpdateException(message, e); + } } private async ValueTask ProjectSnapshot(ObjectSnapshot objectSnapshot) diff --git a/src/SIL.Harmony/SnapshotWorker.cs b/src/SIL.Harmony/SnapshotWorker.cs index 612346f..0cd822a 100644 --- a/src/SIL.Harmony/SnapshotWorker.cs +++ b/src/SIL.Harmony/SnapshotWorker.cs @@ -81,26 +81,37 @@ private async ValueTask ApplyCommitChanges(IEnumerable commits, bool upd IObjectBase entity; var prevSnapshot = await GetSnapshot(commitChange.EntityId); var changeContext = new ChangeContext(commit, commitIndex, intermediateSnapshots, this, _crdtConfig); - bool wasDeleted; - if (prevSnapshot is not null) + + if (prevSnapshot is null) { - entity = prevSnapshot.Entity.Copy(); - wasDeleted = entity.DeletedAt.HasValue; + // create brand new entity - this will (and should) throw if the change doesn't support NewEntity + entity = await commitChange.Change.NewEntity(commit, changeContext); } - else + else if (prevSnapshot.EntityIsDeleted && commitChange.Change.SupportsNewEntity()) { + // revive deleted entity entity = await commitChange.Change.NewEntity(commit, changeContext); - wasDeleted = false; } - - await commitChange.Change.ApplyChange(entity, changeContext); - - var deletedByChange = !wasDeleted && entity.DeletedAt.HasValue; - if (deletedByChange) + else if (commitChange.Change.SupportsApplyChange()) { - await MarkDeleted(entity.Id, changeContext); + // update existing entity + entity = prevSnapshot.Entity.Copy(); + var wasDeleted = prevSnapshot.EntityIsDeleted; + await commitChange.Change.ApplyChange(entity, changeContext); + var deletedByChange = !wasDeleted && entity.DeletedAt.HasValue; + if (deletedByChange) + { + await MarkDeleted(entity.Id, changeContext); + } } - + else + { + // Entity already exists (and is not deleted) + // and change does not support updating existing entities, + // so do nothing. + continue; + } + await GenerateSnapshotForEntity(entity, prevSnapshot, changeContext); } _newIntermediateSnapshots.AddRange(intermediateSnapshots.Values);