From e89ac653790209413820fc8463ad4b2b9e69cb61 Mon Sep 17 00:00:00 2001 From: Tim Haasdyk Date: Sat, 11 Apr 2026 21:53:48 +0200 Subject: [PATCH 1/2] Add test for syncing component when sense moves to different entry Reproduces a real sync failure where the snapshot has a component pointing to sense S on entry A, but in current FwData sense S has moved to entry B (e.g. after a merge). Co-Authored-By: Claude Opus 4.6 (1M context) --- .../FwLiteProjectSync.Tests/EntrySyncTests.cs | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/backend/FwLite/FwLiteProjectSync.Tests/EntrySyncTests.cs b/backend/FwLite/FwLiteProjectSync.Tests/EntrySyncTests.cs index 5daebecce7..4a7fa5895a 100644 --- a/backend/FwLite/FwLiteProjectSync.Tests/EntrySyncTests.cs +++ b/backend/FwLite/FwLiteProjectSync.Tests/EntrySyncTests.cs @@ -621,6 +621,40 @@ public async Task SyncComplexFormsAndComponents_ThrowsExceptionIfEntryNotInAfter await act.Should().ThrowAsync(); } + [Fact] + public async Task CanSyncComponentWhenSenseMovesToDifferentEntry() + { + // Reproduces a real sync scenario: the snapshot (before) has a component pointing to + // sense S on entry A. In current FwData (after), sense S has moved to entry B (I don't know when that actually happens). + // So: same ComponentSenseId, different ComponentEntryId. + var senseId = Guid.NewGuid(); + var oldComponentEntry = await Api.CreateEntry(new() { LexemeForm = { { "en", "old-component" } } }); + var newComponentEntry = await Api.CreateEntry(new() { LexemeForm = { { "en", "new-component" } }, Senses = [new() { Id = senseId }] }); + var complexFormId = Guid.NewGuid(); + // Create the complex form with the old component (pointing to sense on old entry) + var complexForm = await Api.CreateEntry(new() + { + Id = complexFormId, + LexemeForm = { { "en", "complex form" } }, + Components = [ComplexFormComponent.FromEntries(new Entry { Id = complexFormId }, oldComponentEntry, senseId)] + }); + + // after: FwData now has the component pointing to new entry with the same sense + var complexFormAfter = complexForm.Copy(); + complexFormAfter.Components = + [ + ComplexFormComponent.FromEntries(new Entry { Id = complexFormId }, newComponentEntry, senseId) + ]; + + await EntrySync.SyncComplexFormsAndComponents(complexForm, complexFormAfter, Api); + + var actual = await Api.GetEntry(complexFormId); + actual.Should().NotBeNull(); + actual.Components.Should().HaveCount(1); + actual.Components[0].ComponentEntryId.Should().Be(newComponentEntry.Id); + actual.Components[0].ComponentSenseId.Should().Be(senseId); + } + [Fact] public async Task SyncComplexFormsAndComponents_MovesComponentsToCorrectPosition() { From b4ceac2fe871ce3f3a7d1de20c221fdde0222bfb Mon Sep 17 00:00:00 2001 From: Tim Haasdyk Date: Sat, 11 Apr 2026 21:54:02 +0200 Subject: [PATCH 2/2] Use composite key for ComplexFormComponent diffing The diff was using ComponentSenseId as the identity key, so when a sense moved to a different entry (same sense ID, different entry ID) it called Replace instead of remove+add, throwing "changing complex form components is not supported". Make IOrderableCollectionDiffApi generic over TId (matching CollectionDiffApi), so ComplexFormComponentsDiffApi can use (ComplexFormEntryId, ComponentEntryId, ComponentSenseId) as a composite key. Also relaxes the IOrderable constraint to IOrderableNoId, eliminating the OrderableWs wrapper. Co-Authored-By: Claude Opus 4.6 (1M context) --- .../MiniLcm.Tests/DiffCollectionTests.cs | 13 ++-- .../MiniLcm.Tests/TestOrderableDiffApi.cs | 23 ++++--- .../MiniLcm/SyncHelpers/DiffCollection.cs | 56 +++++++++------- .../FwLite/MiniLcm/SyncHelpers/EntrySync.cs | 44 ++++++------- .../SyncHelpers/ExampleSentenceSync.cs | 15 +++-- .../MiniLcm/SyncHelpers/WritingSystemSync.cs | 66 +++++-------------- 6 files changed, 96 insertions(+), 121 deletions(-) diff --git a/backend/FwLite/MiniLcm.Tests/DiffCollectionTests.cs b/backend/FwLite/MiniLcm.Tests/DiffCollectionTests.cs index bff2618d45..cc534dea4c 100644 --- a/backend/FwLite/MiniLcm.Tests/DiffCollectionTests.cs +++ b/backend/FwLite/MiniLcm.Tests/DiffCollectionTests.cs @@ -169,22 +169,17 @@ public async Task DiffTests(CollectionDiffTestCase testCase) return (changeCount, diffApi.DiffOperations, diffApi.Replacements); } - private static BetweenPosition Between(TestOrderable? previous, TestOrderable? next) + private static BetweenPosition Between(TestOrderable? previous, TestOrderable? next) { - return Between(previous?.Id, next?.Id); + return new BetweenPosition(previous, next); } - private static BetweenPosition Between(Guid? previous = null, Guid? next = null) - { - return new BetweenPosition(previous, next); - } - - private static CollectionDiffOperation Move(TestOrderable value, BetweenPosition between) + private static CollectionDiffOperation Move(TestOrderable value, BetweenPosition between) { return new CollectionDiffOperation(value, PositionDiffKind.Move, between); } - private static CollectionDiffOperation Add(TestOrderable value, BetweenPosition between) + private static CollectionDiffOperation Add(TestOrderable value, BetweenPosition between) { return new CollectionDiffOperation(value, PositionDiffKind.Add, between); } diff --git a/backend/FwLite/MiniLcm.Tests/TestOrderableDiffApi.cs b/backend/FwLite/MiniLcm.Tests/TestOrderableDiffApi.cs index 230ec70a54..ffb99da8ea 100644 --- a/backend/FwLite/MiniLcm.Tests/TestOrderableDiffApi.cs +++ b/backend/FwLite/MiniLcm.Tests/TestOrderableDiffApi.cs @@ -13,7 +13,7 @@ public async Task Add() // act var diffApi = new TestOrderableDiffApi([value1]); - var position = new BetweenPosition(value1.Id, null); + var position = new BetweenPosition(value1, null); await diffApi.Add(value2, position); // assert @@ -53,7 +53,7 @@ public async Task Move() // act var diffApi = new TestOrderableDiffApi([value1, value2, value3]); - var position = new BetweenPosition(value1.Id, value2.Id); + var position = new BetweenPosition(value1, value2); await diffApi.Move(value3, position); // assert @@ -82,28 +82,33 @@ public async Task Replace() } } -public class TestOrderableDiffApi(TestOrderable[] before) : IOrderableCollectionDiffApi +public class TestOrderableDiffApi(TestOrderable[] before) : IOrderableCollectionDiffApi { public List Current { get; } = [.. before]; public List DiffOperations = []; public List<(TestOrderable before, TestOrderable after)> Replacements = []; - public Task Add(TestOrderable value, BetweenPosition between) + public Guid GetId(TestOrderable value) + { + return value.Id; + } + + public Task Add(TestOrderable value, BetweenPosition between) { DiffOperations.Add(new CollectionDiffOperation(value, PositionDiffKind.Add, between)); return AddInternal(value, between); } - private Task AddInternal(TestOrderable value, BetweenPosition between) + private Task AddInternal(TestOrderable value, BetweenPosition between) { if (between.Previous is not null) { - var previousIndex = Current.FindIndex(v => v.Id == between.Previous); + var previousIndex = Current.FindIndex(v => v.Id == between.Previous.Id); Current.Insert(previousIndex + 1, value); } else if (between.Next is not null) { - var nextIndex = Current.FindIndex(v => v.Id == between.Next); + var nextIndex = Current.FindIndex(v => v.Id == between.Next.Id); Current.Insert(nextIndex, value); } else @@ -126,7 +131,7 @@ public Task RemoveInternal(TestOrderable value) return Task.FromResult(1); } - public async Task Move(TestOrderable value, BetweenPosition between) + public async Task Move(TestOrderable value, BetweenPosition between) { DiffOperations.Add(new CollectionDiffOperation(value, PositionDiffKind.Move, between)); await RemoveInternal(value); @@ -151,7 +156,7 @@ public Task Replace(TestOrderable before, TestOrderable after) } } -public record CollectionDiffOperation(TestOrderable Value, PositionDiffKind Kind, BetweenPosition? Between = null); +public record CollectionDiffOperation(TestOrderable Value, PositionDiffKind Kind, BetweenPosition? Between = null); public class TestOrderable(double order, Guid id) : IOrderable { diff --git a/backend/FwLite/MiniLcm/SyncHelpers/DiffCollection.cs b/backend/FwLite/MiniLcm/SyncHelpers/DiffCollection.cs index c305136dea..b3a37e9239 100644 --- a/backend/FwLite/MiniLcm/SyncHelpers/DiffCollection.cs +++ b/backend/FwLite/MiniLcm/SyncHelpers/DiffCollection.cs @@ -50,17 +50,13 @@ public override async Task Replace(T before, T after) } } -public interface IOrderableCollectionDiffApi where T : IOrderable +public interface IOrderableCollectionDiffApi where T : IOrderableNoId where TId : notnull { - Task Add(T value, BetweenPosition between); + Task Add(T value, BetweenPosition between); Task Remove(T value); - Task Move(T value, BetweenPosition between); + Task Move(T value, BetweenPosition between); Task Replace(T before, T after); - - Guid GetId(T value) - { - return value.Id; - } + TId GetId(T value); } public static class DiffCollection @@ -106,10 +102,10 @@ public static async Task Diff( return changes; } - public static async Task DiffOrderable( + public static async Task DiffOrderable( IList before, IList after, - IOrderableCollectionDiffApi diffApi) where T : IOrderable + IOrderableCollectionDiffApi diffApi) where T : IOrderableNoId where TId : notnull { var changes = 0; @@ -159,38 +155,50 @@ public static async Task DiffOrderable( return changes; } - private static BetweenPosition GetStableBetween(int i, IList current, HashSet stable, Func GetId) + private static BetweenPosition GetStableBetween(int i, IList current, HashSet stable, Func getId) where TId : notnull { - T? beforeEntity = default; - T? afterEntity = default; + T? previous = default; + T? next = default; for (var j = i - 1; j >= 0; j--) { - if (stable.Contains(GetId(current[j]))) + if (stable.Contains(getId(current[j]))) { - beforeEntity = current[j]; + previous = current[j]; break; } } for (var j = i + 1; j < current.Count; j++) { - if (stable.Contains(GetId(current[j]))) + if (stable.Contains(getId(current[j]))) { - afterEntity = current[j]; + next = current[j]; break; } } - return new BetweenPosition( - beforeEntity is not null ? GetId(beforeEntity) : null, - afterEntity is not null ? GetId(afterEntity) : null); + return new BetweenPosition(previous, next); } - private static ImmutableSortedDictionary? DiffPositions( + private static ImmutableSortedDictionary? DiffPositions( IList before, IList after, - Func GetId) + Func getId) where TId : notnull { - var beforeJson = new JsonArray([.. before.Select(item => JsonValue.Create(GetId(item)))]); - var afterJson = new JsonArray([.. after.Select(item => JsonValue.Create(GetId(item)))]); + // Map TIds to integers for JSON serialization (supports non-Guid ID types like tuples) + var idToInt = new Dictionary(); + int idx = 0; + foreach (var item in before) + { + var id = getId(item); + if (!idToInt.ContainsKey(id)) idToInt[id] = idx++; + } + foreach (var item in after) + { + var id = getId(item); + if (!idToInt.ContainsKey(id)) idToInt[id] = idx++; + } + + var beforeJson = new JsonArray([.. before.Select(item => JsonValue.Create(idToInt[getId(item)]))]); + var afterJson = new JsonArray([.. after.Select(item => JsonValue.Create(idToInt[getId(item)]))]); return JsonDiffPatcher.Diff(beforeJson, afterJson, DiffFormatter.Instance); } diff --git a/backend/FwLite/MiniLcm/SyncHelpers/EntrySync.cs b/backend/FwLite/MiniLcm/SyncHelpers/EntrySync.cs index e157befc79..f69cfccb47 100644 --- a/backend/FwLite/MiniLcm/SyncHelpers/EntrySync.cs +++ b/backend/FwLite/MiniLcm/SyncHelpers/EntrySync.cs @@ -66,7 +66,7 @@ public static async Task SyncComplexFormsAndComponents(Entry beforeEntry, E try { var changes = 0; - changes += await SyncComplexFormComponents(afterEntry, beforeEntry.Components, afterEntry.Components, api); + changes += await SyncComplexFormComponents(beforeEntry.Components, afterEntry.Components, api); changes += await SyncComplexForms(beforeEntry.ComplexForms, afterEntry.ComplexForms, api); return changes; } @@ -98,12 +98,12 @@ private static async Task Sync(Guid entryId, new ComplexFormTypesDiffApi(api, entryId)); } - private static async Task SyncComplexFormComponents(Entry afterEntry, IList beforeComponents, IList afterComponents, IMiniLcmApi api) + private static async Task SyncComplexFormComponents(IList beforeComponents, IList afterComponents, IMiniLcmApi api) { return await DiffCollection.DiffOrderable( beforeComponents, afterComponents, - new ComplexFormComponentsDiffApi(afterEntry, api) + new ComplexFormComponentsDiffApi(api) ); } @@ -236,27 +236,19 @@ public override Task Replace(ComplexFormComponent beforeComponent, ComplexF } } - private class ComplexFormComponentsDiffApi(Entry afterEntry, IMiniLcmApi api) : IOrderableCollectionDiffApi + private class ComplexFormComponentsDiffApi(IMiniLcmApi api) : IOrderableCollectionDiffApi { - public Guid GetId(ComplexFormComponent component) + public (Guid, Guid, Guid?) GetId(ComplexFormComponent component) { // we can't use the ID as there's none defined by Fw so it won't work as a sync key - return component.ComponentSenseId ?? component.ComponentEntryId; - } - - private BetweenPosition MapBackToEntities(BetweenPosition between) - { - var previous = between!.Previous is null ? null : afterEntry.Components.Find(c => GetId(c) == between.Previous); - var next = between!.Next is null ? null : afterEntry.Components.Find(c => GetId(c) == between.Next); - return new BetweenPosition(previous, next); + return (component.ComplexFormEntryId, component.ComponentEntryId, component.ComponentSenseId); } - public async Task Add(ComplexFormComponent after, BetweenPosition between) + public async Task Add(ComplexFormComponent after, BetweenPosition between) { - var betweenComponents = MapBackToEntities(between); try { - await api.CreateComplexFormComponent(after, betweenComponents); + await api.CreateComplexFormComponent(after, between); } catch (NotFoundException) { @@ -265,10 +257,9 @@ public async Task Add(ComplexFormComponent after, BetweenPosition between) return 1; } - public async Task Move(ComplexFormComponent component, BetweenPosition between) + public async Task Move(ComplexFormComponent component, BetweenPosition between) { - var betweenComponents = MapBackToEntities(between); - await api.MoveComplexFormComponent(component, betweenComponents); + await api.MoveComplexFormComponent(component, between); return 1; } @@ -290,17 +281,22 @@ public Task Replace(ComplexFormComponent beforeComponent, ComplexFormCompon } } - private class SensesDiffApi(IMiniLcmApi api, Guid entryId) : IOrderableCollectionDiffApi + private class SensesDiffApi(IMiniLcmApi api, Guid entryId) : IOrderableCollectionDiffApi { - public async Task Add(Sense sense, BetweenPosition between) + public Guid GetId(Sense sense) + { + return sense.Id; + } + + public async Task Add(Sense sense, BetweenPosition between) { - await api.CreateSense(entryId, sense, between); + await api.CreateSense(entryId, sense, new BetweenPosition(between.Previous?.Id, between.Next?.Id)); return 1; } - public async Task Move(Sense sense, BetweenPosition between) + public async Task Move(Sense sense, BetweenPosition between) { - await api.MoveSense(entryId, sense.Id, between); + await api.MoveSense(entryId, sense.Id, new BetweenPosition(between.Previous?.Id, between.Next?.Id)); return 1; } diff --git a/backend/FwLite/MiniLcm/SyncHelpers/ExampleSentenceSync.cs b/backend/FwLite/MiniLcm/SyncHelpers/ExampleSentenceSync.cs index 94a242e2ea..9fda7c896d 100644 --- a/backend/FwLite/MiniLcm/SyncHelpers/ExampleSentenceSync.cs +++ b/backend/FwLite/MiniLcm/SyncHelpers/ExampleSentenceSync.cs @@ -88,17 +88,22 @@ public override Guid GetId(Translation value) } } - private class ExampleSentencesDiffApi(IMiniLcmApi api, Guid entryId, Guid senseId) : IOrderableCollectionDiffApi + private class ExampleSentencesDiffApi(IMiniLcmApi api, Guid entryId, Guid senseId) : IOrderableCollectionDiffApi { - public async Task Add(ExampleSentence afterExampleSentence, BetweenPosition between) + public Guid GetId(ExampleSentence value) { - await api.CreateExampleSentence(entryId, senseId, afterExampleSentence, between); + return value.Id; + } + + public async Task Add(ExampleSentence afterExampleSentence, BetweenPosition between) + { + await api.CreateExampleSentence(entryId, senseId, afterExampleSentence, new BetweenPosition(between.Previous?.Id, between.Next?.Id)); return 1; } - public async Task Move(ExampleSentence example, BetweenPosition between) + public async Task Move(ExampleSentence example, BetweenPosition between) { - await api.MoveExampleSentence(entryId, senseId, example.Id, between); + await api.MoveExampleSentence(entryId, senseId, example.Id, new BetweenPosition(between.Previous?.Id, between.Next?.Id)); return 1; } diff --git a/backend/FwLite/MiniLcm/SyncHelpers/WritingSystemSync.cs b/backend/FwLite/MiniLcm/SyncHelpers/WritingSystemSync.cs index 11cf303543..cb58ec4e48 100644 --- a/backend/FwLite/MiniLcm/SyncHelpers/WritingSystemSync.cs +++ b/backend/FwLite/MiniLcm/SyncHelpers/WritingSystemSync.cs @@ -1,5 +1,4 @@ using MiniLcm.Models; -using SIL.Extensions; using SystemTextJsonPatch; namespace MiniLcm.SyncHelpers; @@ -50,85 +49,52 @@ public static async Task Sync(WritingSystem beforeWs, WritingSystem afterWs return new UpdateObjectInput(patchDocument); } - private class WritingSystemsDiffApi(IMiniLcmApi api) : IOrderableCollectionDiffApi + private class WritingSystemsDiffApi(IMiniLcmApi api) : IOrderableCollectionDiffApi { - private Dictionary Mapping { get; } = new(); public async Task Diff(WritingSystem[] beforeWritingSystems, WritingSystem[] afterWritingSystems) { return await DiffCollection.DiffOrderable( - //diff collection must work with a Guid, and the id's must match between the two lists - //so we just generate the guids on the fly and make sure they're the same for the same wsId - beforeWritingSystems.Select(ws => new OrderableWs(ws, GetOrderableId(ws.WsId))).OrderBy(o => o.Order).ToList(), - afterWritingSystems.Select(ws => new OrderableWs(ws, GetOrderableId(ws.WsId))).OrderBy(o => o.Order).ToList(), + [.. beforeWritingSystems.OrderBy(ws => ws.Order)], + [.. afterWritingSystems.OrderBy(ws => ws.Order)], this ); } - private Guid GetOrderableId(WritingSystemId wsId) + public WritingSystemId GetId(WritingSystem value) { - foreach (var kvp in Mapping) - { - if (kvp.Value == wsId) - { - return kvp.Key; - } - } - var newId = Guid.NewGuid(); - Mapping[newId] = wsId; - return newId; + return value.WsId; } - private class OrderableWs : IOrderable - { - public WritingSystem Ws { get; } - //can't use Ws Guid because it is not set for FwData - public Guid Id { get; } - - public double Order - { - get => Ws.Order; - set => Ws.Order = value; - } - - public OrderableWs(WritingSystem ws, Guid id) - { - this.Ws = ws; - this.Id = id; - } - } - - async Task IOrderableCollectionDiffApi.Add(OrderableWs value, BetweenPosition between) + public async Task Add(WritingSystem value, BetweenPosition between) { var betweenWsId = new BetweenPosition( - //we can't use `null` and must use new WritingSystemId?() to set a nullable value to null - between.Previous is null ? new WritingSystemId?() : Mapping[between.Previous.Value], - between.Next is null ? new WritingSystemId?() : Mapping[between.Next.Value] + between.Previous is null ? new WritingSystemId?() : between.Previous.WsId, + between.Next is null ? new WritingSystemId?() : between.Next.WsId ); - await api.CreateWritingSystem(value.Ws, betweenWsId); + await api.CreateWritingSystem(value, betweenWsId); return 1; } - Task IOrderableCollectionDiffApi.Remove(OrderableWs value) + public Task Remove(WritingSystem value) { // await api.DeleteWritingSystem(beforeWs.Id); // Deleting writing systems is dangerous as it causes cascading data deletion. Needs careful thought. // TODO: should we throw an exception? return Task.FromResult(0); } - async Task IOrderableCollectionDiffApi.Move(OrderableWs value, BetweenPosition between) + public async Task Move(WritingSystem value, BetweenPosition between) { var betweenWsId = new BetweenPosition( - //we can't use `null` and must use new WritingSystemId?() to set a nullable value to null - between.Previous is null ? new WritingSystemId?() : Mapping[between.Previous.Value], - between.Next is null ? new WritingSystemId?() : Mapping[between.Next.Value] + between.Previous is null ? new WritingSystemId?() : between.Previous.WsId, + between.Next is null ? new WritingSystemId?() : between.Next.WsId ); - await api.MoveWritingSystem(value.Ws.WsId, value.Ws.Type, betweenWsId); + await api.MoveWritingSystem(value.WsId, value.Type, betweenWsId); return 1; } - Task IOrderableCollectionDiffApi.Replace(OrderableWs before, OrderableWs after) + public Task Replace(WritingSystem before, WritingSystem after) { - return Sync(before.Ws, after.Ws, api); + return Sync(before, after, api); } } }