Skip to content

Remove reference changes are lost #31

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
myieye merged 4 commits into from
Mar 28, 2025
Merged

Remove reference changes are lost #31

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
88b152d
Add failing tests
myieye Mar 19, 2025
4d4a2be
Fix bug
myieye Mar 19, 2025
b7ebb3d
Unify snapshot creation logic
myieye Mar 19, 2025
4009ed7
Make ChangeContext class internal
myieye Mar 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 42 additions & 0 deletions src/SIL.Harmony.Tests/DataModelReferenceTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<Word>(wordId),
]);

var def = await DataModel.GetLatest<Definition>(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<Word>(wordId),
]);

var def = await DataModel.GetLatest<Definition>(definitionId);
def.Should().NotBeNull();
def.PartOfSpeech.Should().Be("pos2");
def.DeletedAt.Should().NotBeNull();
}

[Fact]
public async Task AddAndDeleteInSameSyncDeletesRefs()
{
Expand Down
13 changes: 10 additions & 3 deletions src/SIL.Harmony/Changes/ChangeContext.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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<Guid, ObjectSnapshot> 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<Guid, ObjectSnapshot> IntermediateSnapshots { get; }
public async ValueTask<IObjectSnapshot?> GetSnapshot(Guid entityId) => await _worker.GetSnapshot(entityId);
public IAsyncEnumerable<object> GetObjectsReferencing(Guid entityId, bool includeDeleted = false)
{
Expand Down
72 changes: 37 additions & 35 deletions src/SIL.Harmony/SnapshotWorker.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ private async ValueTask ApplyCommitChanges(IEnumerable<Commit> 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)
{
Expand All @@ -98,34 +98,10 @@ private async ValueTask ApplyCommitChanges(IEnumerable<Commit> 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();
Expand All @@ -137,31 +113,28 @@ private async ValueTask ApplyCommitChanges(IEnumerable<Commit> commits, bool upd
/// </summary>
/// <param name="deletedEntityId"></param>
/// <param name="commit"></param>
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);
}
}
}
Expand Down Expand Up @@ -223,6 +196,35 @@ internal async IAsyncEnumerable<ObjectSnapshot> GetSnapshotsWhere(Expression<Fun
}
}

private async Task GenerateSnapshotForEntity(IObjectBase entity, ObjectSnapshot? prevSnapshot, ChangeContext context)
{
//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 == 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)
Expand Down