Skip to content

Commit b092b25

Browse files
myieyemyieye
authored
Remove reference changes are lost (#31)
* Add failing tests * Fix bug * Unify snapshot creation logic * Make ChangeContext class internal
1 parent a82bc13 commit b092b25

File tree

3 files changed

+89
-38
lines changed

3 files changed

+89
-38
lines changed

‎src/SIL.Harmony.Tests/DataModelReferenceTests.cs

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,48 @@ await AddCommitsViaSync([
8585
wordWithoutRef.AntonymId.Should().BeNull();
8686
}
8787

88+
[Fact]
89+
public async Task AddAndDeleteInSameCommitDeletesRefs()
90+
{
91+
var wordId = Guid.NewGuid();
92+
var definitionId = Guid.NewGuid();
93+
94+
await WriteNextChange(
95+
[
96+
SetWord(wordId, "original"),
97+
NewDefinition(wordId, "the shiny one everything started with", "adj", 0, definitionId),
98+
new DeleteChange<Word>(wordId),
99+
]);
100+
101+
var def = await DataModel.GetLatest<Definition>(definitionId);
102+
def.Should().NotBeNull();
103+
def.DeletedAt.Should().NotBeNull();
104+
}
105+
106+
[Fact]
107+
public async Task UpdateAndDeleteParentInSameCommitWorks()
108+
{
109+
var wordId = Guid.NewGuid();
110+
var definitionId = Guid.NewGuid();
111+
112+
await WriteNextChange(
113+
[
114+
SetWord(wordId, "original"),
115+
NewDefinition(wordId, "the shiny one everything started with", "adj", 0, definitionId),
116+
]);
117+
118+
await WriteNextChange(
119+
[
120+
new SetDefinitionPartOfSpeechChange(definitionId, "pos2"),
121+
new DeleteChange<Word>(wordId),
122+
]);
123+
124+
var def = await DataModel.GetLatest<Definition>(definitionId);
125+
def.Should().NotBeNull();
126+
def.PartOfSpeech.Should().Be("pos2");
127+
def.DeletedAt.Should().NotBeNull();
128+
}
129+
88130
[Fact]
89131
public async Task AddAndDeleteInSameSyncDeletesRefs()
90132
{

‎src/SIL.Harmony/Changes/ChangeContext.cs

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,18 +1,25 @@
1+
using SIL.Harmony.Db;
2+
13
namespace SIL.Harmony.Changes;
24

3-
public class ChangeContext : IChangeContext
5+
internal class ChangeContext : IChangeContext
46
{
57
private readonly SnapshotWorker _worker;
68
private readonly CrdtConfig _crdtConfig;
79

8-
internal ChangeContext(Commit commit, SnapshotWorker worker, CrdtConfig crdtConfig)
10+
internal ChangeContext(Commit commit, int commitIndex, IDictionary<Guid, ObjectSnapshot> intermediateSnapshots, SnapshotWorker worker, CrdtConfig crdtConfig)
911
{
1012
_worker = worker;
1113
_crdtConfig = crdtConfig;
1214
Commit = commit;
15+
CommitIndex = commitIndex;
16+
IntermediateSnapshots = intermediateSnapshots;
1317
}
1418

15-
public CommitBase Commit { get; }
19+
CommitBase IChangeContext.Commit => Commit;
20+
public Commit Commit { get; }
21+
public int CommitIndex { get; }
22+
public IDictionary<Guid, ObjectSnapshot> IntermediateSnapshots { get; }
1623
public async ValueTask<IObjectSnapshot?> GetSnapshot(Guid entityId) => await _worker.GetSnapshot(entityId);
1724
public IAsyncEnumerable<object> GetObjectsReferencing(Guid entityId, bool includeDeleted = false)
1825
{

‎src/SIL.Harmony/SnapshotWorker.cs

Lines changed: 37 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,7 @@ private async ValueTask ApplyCommitChanges(IEnumerable<Commit> commits, bool upd
8080
{
8181
IObjectBase entity;
8282
var prevSnapshot = await GetSnapshot(commitChange.EntityId);
83-
var changeContext = new ChangeContext(commit, this, _crdtConfig);
83+
var changeContext = new ChangeContext(commit, commitIndex, intermediateSnapshots, this, _crdtConfig);
8484
bool wasDeleted;
8585
if (prevSnapshot is not null)
8686
{
@@ -98,34 +98,10 @@ private async ValueTask ApplyCommitChanges(IEnumerable<Commit> commits, bool upd
9898
var deletedByChange = !wasDeleted && entity.DeletedAt.HasValue;
9999
if (deletedByChange)
100100
{
101-
await MarkDeleted(entity.Id, commit);
101+
await MarkDeleted(entity.Id, changeContext);
102102
}
103-
104-
//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.
105-
//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
106-
//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.
107-
// s0 -> s1 -> sCurrent
108-
// if always taking snapshots would become
109-
// s0 -> s1 -> sCurrent -> sNew
110-
//but but to not snapshot every change we could do this instead
111-
// s0 -> s1 -> sNew
112-
113-
//when both snapshots are for the same commit we don't want to keep the previous, therefore the new snapshot should be root
114-
var isRoot = prevSnapshot is null || (prevSnapshot.IsRoot && prevSnapshot.CommitId == commit.Id);
115-
var newSnapshot = new ObjectSnapshot(entity, commit, isRoot);
116-
//if both snapshots are for the same commit then we don't want to keep the previous snapshot
117-
if (prevSnapshot is null || prevSnapshot.CommitId == commit.Id)
118-
{
119-
//do nothing, will cause prevSnapshot to be overriden in _pendingSnapshots if it exists
120-
}
121-
else if (commitIndex % 2 == 0 && !prevSnapshot.IsRoot && IsNew(prevSnapshot))
122-
{
123-
intermediateSnapshots[prevSnapshot.Entity.Id] = prevSnapshot;
124-
}
125-
126-
await _crdtConfig.BeforeSaveObject.Invoke(entity.DbObject, newSnapshot);
127-
128-
AddSnapshot(newSnapshot);
103+
104+
await GenerateSnapshotForEntity(entity, prevSnapshot, changeContext);
129105
}
130106
_newIntermediateSnapshots.AddRange(intermediateSnapshots.Values);
131107
intermediateSnapshots.Clear();
@@ -137,31 +113,28 @@ private async ValueTask ApplyCommitChanges(IEnumerable<Commit> commits, bool upd
137113
/// </summary>
138114
/// <param name="deletedEntityId"></param>
139115
/// <param name="commit"></param>
140-
private async ValueTask MarkDeleted(Guid deletedEntityId, Commit commit)
116+
private async ValueTask MarkDeleted(Guid deletedEntityId, ChangeContext context)
141117
{
142118
// Including deleted shouldn't be necessary, because change objects are responsible for not adding references to deleted entities.
143119
// But maybe it's a good fallback.
144120
var toRemoveRefFrom = await GetSnapshotsReferencing(deletedEntityId, true)
145121
.ToArrayAsync();
146122

123+
var commit = context.Commit;
147124
foreach (var snapshot in toRemoveRefFrom)
148125
{
149-
var hasBeenApplied = snapshot.CommitId == commit.Id;
150126
var updatedEntry = snapshot.Entity.Copy();
151127
var wasDeleted = updatedEntry.DeletedAt.HasValue;
152128

153129
updatedEntry.RemoveReference(deletedEntityId, commit);
154130
var deletedByRemoveRef = !wasDeleted && updatedEntry.DeletedAt.HasValue;
155131

156-
//this snapshot has already been applied, we don't need to add it again
157-
//but we did need to run apply again because we may need to mark other entities as deleted
158-
if (!hasBeenApplied)
159-
AddSnapshot(new ObjectSnapshot(updatedEntry, commit, false));
132+
await GenerateSnapshotForEntity(updatedEntry, snapshot, context);
160133

161134
//we need to do this after we add the snapshot above otherwise we might get stuck in a loop of deletions
162135
if (deletedByRemoveRef)
163136
{
164-
await MarkDeleted(updatedEntry.Id, commit);
137+
await MarkDeleted(updatedEntry.Id, context);
165138
}
166139
}
167140
}
@@ -223,6 +196,35 @@ internal async IAsyncEnumerable<ObjectSnapshot> GetSnapshotsWhere(Expression<Fun
223196
}
224197
}
225198

199+
private async Task GenerateSnapshotForEntity(IObjectBase entity, ObjectSnapshot? prevSnapshot, ChangeContext context)
200+
{
201+
//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.
202+
//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
203+
//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.
204+
// s0 -> s1 -> sCurrent
205+
// if always taking snapshots would become
206+
// s0 -> s1 -> sCurrent -> sNew
207+
//but but to not snapshot every change we could do this instead
208+
// s0 -> s1 -> sNew
209+
210+
//when both snapshots are for the same commit we don't want to keep the previous, therefore the new snapshot should be root
211+
var isRoot = prevSnapshot is null || (prevSnapshot.IsRoot && prevSnapshot.CommitId == context.Commit.Id);
212+
var newSnapshot = new ObjectSnapshot(entity, context.Commit, isRoot);
213+
//if both snapshots are for the same commit then we don't want to keep the previous snapshot
214+
if (prevSnapshot is null || prevSnapshot.CommitId == context.Commit.Id)
215+
{
216+
//do nothing, will cause prevSnapshot to be overriden in _pendingSnapshots if it exists
217+
}
218+
else if (context.CommitIndex % 2 == 0 && !prevSnapshot.IsRoot && IsNew(prevSnapshot))
219+
{
220+
context.IntermediateSnapshots[prevSnapshot.Entity.Id] = prevSnapshot;
221+
}
222+
223+
await _crdtConfig.BeforeSaveObject.Invoke(entity.DbObject, newSnapshot);
224+
225+
AddSnapshot(newSnapshot);
226+
}
227+
226228
private void AddSnapshot(ObjectSnapshot snapshot)
227229
{
228230
if (snapshot.IsRoot)

0 commit comments

Comments
 (0)