From d8cb48b411bfffc2648081b2d45de7ede49c6063 Mon Sep 17 00:00:00 2001 From: Tim Haasdyk Date: Tue, 3 Dec 2024 16:32:29 +0100 Subject: [PATCH 01/28] Add collection position diffing --- .../Api/FwDataMiniLcmApi.cs | 6 + .../FwLiteProjectSync/DryRunMiniLcmApi.cs | 6 + .../LcmCrdt/Changes/CreateSenseChange.cs | 2 + .../FwLite/LcmCrdt/Changes/SetOrderChange.cs | 41 ++++++ backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs | 23 ++- backend/FwLite/LcmCrdt/LcmCrdtKernel.cs | 8 +- backend/FwLite/LcmCrdt/QueryHelpers.cs | 11 ++ .../MiniLcm.Tests/DiffCollectionTests.cs | 33 +++++ .../FwLite/MiniLcm.Tests/MiniLcm.Tests.csproj | 1 + backend/FwLite/MiniLcm/IMiniLcmApi.cs | 8 +- backend/FwLite/MiniLcm/IMiniLcmWriteApi.cs | 1 + backend/FwLite/MiniLcm/MiniLcm.csproj | 1 + .../MiniLcm/Models/ComplexFormComponent.cs | 8 +- .../FwLite/MiniLcm/Models/ExampleSentence.cs | 8 +- backend/FwLite/MiniLcm/Models/IOrderable.cs | 7 + backend/FwLite/MiniLcm/Models/Sense.cs | 8 +- .../MiniLcm/SyncHelpers/DiffCollection.cs | 136 +++++++++++++++++- .../FwLite/MiniLcm/SyncHelpers/EntrySync.cs | 49 ++++++- 18 files changed, 341 insertions(+), 16 deletions(-) create mode 100644 backend/FwLite/LcmCrdt/Changes/SetOrderChange.cs create mode 100644 backend/FwLite/LcmCrdt/QueryHelpers.cs create mode 100644 backend/FwLite/MiniLcm.Tests/DiffCollectionTests.cs create mode 100644 backend/FwLite/MiniLcm/Models/IOrderable.cs diff --git a/backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs b/backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs index c5ffc2974..976ff4fd6 100644 --- a/backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs +++ b/backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs @@ -955,6 +955,12 @@ await Cache.DoUsingNewOrCurrentUOW("Update Sense", return await GetSense(entryId, after.Id) ?? throw new NullReferenceException("unable to find sense with id " + after.Id); } + public Task MoveSense(Guid entryId, Sense afterSense, int to) + { + // todo: to is the final destination + return Task.FromResult(afterSense); + } + public Task AddSemanticDomainToSense(Guid senseId, SemanticDomain semanticDomain) { UndoableUnitOfWorkHelper.DoUsingNewOrCurrentUOW("Add Semantic Domain to Sense", diff --git a/backend/FwLite/FwLiteProjectSync/DryRunMiniLcmApi.cs b/backend/FwLite/FwLiteProjectSync/DryRunMiniLcmApi.cs index 82d5190ca..0e205d744 100644 --- a/backend/FwLite/FwLiteProjectSync/DryRunMiniLcmApi.cs +++ b/backend/FwLite/FwLiteProjectSync/DryRunMiniLcmApi.cs @@ -218,6 +218,12 @@ public async Task UpdateSense(Guid entryId, Sense before, Sense after) return await GetSense(entryId, after.Id) ?? throw new NullReferenceException($"unable to find sense with id {after.Id}"); } + public Task MoveSense(Guid entryId, Sense sense) + { + DryRunRecords.Add(new DryRunRecord(nameof(MoveSense), $"Move sense {sense.Gloss} to {sense.Order}")); + return Task.FromResult(sense); + } + public Task DeleteSense(Guid entryId, Guid senseId) { DryRunRecords.Add(new DryRunRecord(nameof(DeleteSense), $"Delete sense {senseId}")); diff --git a/backend/FwLite/LcmCrdt/Changes/CreateSenseChange.cs b/backend/FwLite/LcmCrdt/Changes/CreateSenseChange.cs index 4682ae30d..8f2dc2bb4 100644 --- a/backend/FwLite/LcmCrdt/Changes/CreateSenseChange.cs +++ b/backend/FwLite/LcmCrdt/Changes/CreateSenseChange.cs @@ -25,6 +25,7 @@ private CreateSenseChange(Guid entityId, Guid entryId) : base(entityId) } public Guid EntryId { get; set; } + public double Order { get; set; } public MultiString? Definition { get; set; } public MultiString? Gloss { get; set; } public string? PartOfSpeech { get; set; } @@ -36,6 +37,7 @@ public override async ValueTask NewEntity(Commit commit, ChangeContext co return new Sense { Id = EntityId, + Order = Order, EntryId = EntryId, Definition = Definition ?? new MultiString(), Gloss = Gloss ?? new MultiString(), diff --git a/backend/FwLite/LcmCrdt/Changes/SetOrderChange.cs b/backend/FwLite/LcmCrdt/Changes/SetOrderChange.cs new file mode 100644 index 000000000..6342dcfee --- /dev/null +++ b/backend/FwLite/LcmCrdt/Changes/SetOrderChange.cs @@ -0,0 +1,41 @@ +using SIL.Harmony.Changes; +using SIL.Harmony.Entities; + +namespace LcmCrdt.Changes; + +public class SetOrderChange : EditChange, ISelfNamedType> + where T : class, IOrderable +{ + public static IChange Between(Guid entityId, T left, T right) + { + return new SetOrderChange(entityId, (left.Order + right.Order) / 2); + } + + public static IChange After(Guid entityId, T previous) + { + return new SetOrderChange(entityId, previous.Order + 1); + } + + public static IChange Before(Guid entityId, T preceding) + { + return new SetOrderChange(entityId, preceding.Order - 1); + } + + public static IChange To(Guid entityId, double order) + { + return new SetOrderChange(entityId, order); + } + + protected SetOrderChange(Guid entityId, double order) : base(entityId) + { + Order = order; + } + + public double Order { get; init; } + + public override ValueTask ApplyChange(T entity, ChangeContext context) + { + entity.Order = Order; + return ValueTask.CompletedTask; + } +} diff --git a/backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs b/backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs index 3955ab957..d12a96ece 100644 --- a/backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs +++ b/backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs @@ -267,9 +267,10 @@ private async IAsyncEnumerable GetEntries( if (sortWs is null) throw new NullReferenceException($"sort writing system {options.Order.WritingSystem} not found"); queryable = queryable - .LoadWith(e => e.Senses).ThenLoad(s => s.ExampleSentences) + .LoadWith(e => e.Senses.DefaultOrder()) + .ThenLoad(s => s.ExampleSentences.DefaultOrder()) .LoadWith(e => e.ComplexForms) - .LoadWith(e => e.Components) + .LoadWith(e => e.Components.DefaultOrder()) .AsQueryable() .OrderBy(e => e.Headword(sortWs.WsId).CollateUnicode(sortWs)) .ThenBy(e => e.Id); @@ -358,7 +359,11 @@ await dataModel.AddChanges(ClientId, [ new CreateEntryChange(entry), ..await entry.Senses.ToAsyncEnumerable() - .SelectMany(s => CreateSenseChanges(entry.Id, s)) + .SelectMany((s, i) => + { + s.Order = i + 1; + return CreateSenseChanges(entry.Id, s); + }) .ToArrayAsync(), ..await ToComplexFormComponents(entry.Components).ToArrayAsync(), ..await ToComplexFormComponents(entry.ComplexForms).ToArrayAsync(), @@ -476,6 +481,11 @@ private async IAsyncEnumerable CreateSenseChanges(Guid entryId, Sense s public async Task CreateSense(Guid entryId, Sense sense) { + if (sense.Order == default) + { + var entry = await GetEntry(entryId) ?? throw new NullReferenceException($"unable to find entry with id {entryId}"); + sense.Order = entry.Senses.Max(s => s.Order) + 1; + } await dataModel.AddChanges(ClientId, await CreateSenseChanges(entryId, sense).ToArrayAsync()); return await dataModel.GetLatest(sense.Id) ?? throw new NullReferenceException(); } @@ -496,6 +506,12 @@ public async Task UpdateSense(Guid entryId, Sense before, Sense after) return await GetSense(entryId, after.Id) ?? throw new NullReferenceException("unable to find sense with id " + after.Id); } + public async Task MoveSense(Guid entryId, Sense sense) + { + await dataModel.AddChange(ClientId, Changes.SetOrderChange.To(sense.Id, sense.Order)); + return await dataModel.GetLatest(sense.Id) ?? throw new NullReferenceException(); + } + public async Task DeleteSense(Guid entryId, Guid senseId) { await dataModel.AddChange(ClientId, new DeleteChange(senseId)); @@ -551,5 +567,4 @@ public async Task DeleteExampleSentence(Guid entryId, Guid senseId, Guid example { await dataModel.AddChange(ClientId, new DeleteChange(exampleSentenceId)); } - } diff --git a/backend/FwLite/LcmCrdt/LcmCrdtKernel.cs b/backend/FwLite/LcmCrdt/LcmCrdtKernel.cs index 08557d6bc..6ac69f5a1 100644 --- a/backend/FwLite/LcmCrdt/LcmCrdtKernel.cs +++ b/backend/FwLite/LcmCrdt/LcmCrdtKernel.cs @@ -159,6 +159,11 @@ public static void ConfigureCrdt(CrdtConfig config) }).IsUnique().HasFilter($"{componentSenseId} IS NULL"); }); + config.ObjectTypeListBuilder.OrderConfig(new OrderConfig( + o => o.Order, + o => o.Id + )); + config.ChangeTypeListBuilder.Add>() .Add>() .Add>() @@ -188,7 +193,8 @@ public static void ConfigureCrdt(CrdtConfig config) .Add() .Add() .Add() - .Add(); + .Add() + .Add>(); } public static Task OpenCrdtProject(this IServiceProvider services, CrdtProject project) diff --git a/backend/FwLite/LcmCrdt/QueryHelpers.cs b/backend/FwLite/LcmCrdt/QueryHelpers.cs new file mode 100644 index 000000000..b64988a1a --- /dev/null +++ b/backend/FwLite/LcmCrdt/QueryHelpers.cs @@ -0,0 +1,11 @@ +namespace LcmCrdt; + +public static class QueryHelpers +{ + public static IEnumerable DefaultOrder(this IEnumerable queryable) where T : IOrderable + { + return queryable + .OrderBy(e => e.Order) + .ThenBy(e => e.Id); + } +} diff --git a/backend/FwLite/MiniLcm.Tests/DiffCollectionTests.cs b/backend/FwLite/MiniLcm.Tests/DiffCollectionTests.cs new file mode 100644 index 000000000..241611c9b --- /dev/null +++ b/backend/FwLite/MiniLcm.Tests/DiffCollectionTests.cs @@ -0,0 +1,33 @@ +using MiniLcm.SyncHelpers; +using Moq; + +namespace MiniLcm.Tests; + +public class DiffCollectionTests +{ + [Fact] + public async void FirstTest() + { + var api = new Mock(); + var changeCount = await DiffCollection.DiffOrderable(api.Object, new List(), new List(), + (value) => value.Id, + (_api, value, newI, stable) => Task.FromResult(1), + (_api, value) => Task.FromResult(1), + (_api, value, newI, stable) => Task.FromResult(1), + (_api, oldValue, newValue) => Task.FromResult(1)); + + changeCount.Should().Be(0); + api.VerifyNoOtherCalls(); + } + + // [Theory] + // [InlineData("gx")] + // [InlineData("oo")] + // [InlineData("eng")] // Three-letter codes not allowed when there's a valid two-letter code + // [InlineData("eng-Zxxx-x-audio")] + // [InlineData("nonsense")] + // public void InvalidWritingSystemId_ShouldThrow(string code) + // { + // Assert.Throws(() => new WritingSystemId(code)); + // } +} diff --git a/backend/FwLite/MiniLcm.Tests/MiniLcm.Tests.csproj b/backend/FwLite/MiniLcm.Tests/MiniLcm.Tests.csproj index 1e5de3a28..9c6ac6602 100644 --- a/backend/FwLite/MiniLcm.Tests/MiniLcm.Tests.csproj +++ b/backend/FwLite/MiniLcm.Tests/MiniLcm.Tests.csproj @@ -21,6 +21,7 @@ + diff --git a/backend/FwLite/MiniLcm/IMiniLcmApi.cs b/backend/FwLite/MiniLcm/IMiniLcmApi.cs index 6beb6db07..7c6179282 100644 --- a/backend/FwLite/MiniLcm/IMiniLcmApi.cs +++ b/backend/FwLite/MiniLcm/IMiniLcmApi.cs @@ -1,3 +1,7 @@ -namespace MiniLcm; +using MiniLcm.Models; -public interface IMiniLcmApi: IMiniLcmReadApi, IMiniLcmWriteApi; +namespace MiniLcm; + +public interface IMiniLcmApi : IMiniLcmReadApi, IMiniLcmWriteApi +{ +} diff --git a/backend/FwLite/MiniLcm/IMiniLcmWriteApi.cs b/backend/FwLite/MiniLcm/IMiniLcmWriteApi.cs index b41040cb1..2f7f41962 100644 --- a/backend/FwLite/MiniLcm/IMiniLcmWriteApi.cs +++ b/backend/FwLite/MiniLcm/IMiniLcmWriteApi.cs @@ -49,6 +49,7 @@ Task UpdateWritingSystem(WritingSystemId id, Task CreateSense(Guid entryId, Sense sense); Task UpdateSense(Guid entryId, Guid senseId, UpdateObjectInput update); Task UpdateSense(Guid entryId, Sense before, Sense after); + Task MoveSense(Guid entryId, Sense afterSense); Task DeleteSense(Guid entryId, Guid senseId); Task AddSemanticDomainToSense(Guid senseId, SemanticDomain semanticDomain); Task RemoveSemanticDomainFromSense(Guid senseId, Guid semanticDomainId); diff --git a/backend/FwLite/MiniLcm/MiniLcm.csproj b/backend/FwLite/MiniLcm/MiniLcm.csproj index e806ed4ff..7e8a23dde 100644 --- a/backend/FwLite/MiniLcm/MiniLcm.csproj +++ b/backend/FwLite/MiniLcm/MiniLcm.csproj @@ -10,6 +10,7 @@ + diff --git a/backend/FwLite/MiniLcm/Models/ComplexFormComponent.cs b/backend/FwLite/MiniLcm/Models/ComplexFormComponent.cs index cbea10589..f42bf06c3 100644 --- a/backend/FwLite/MiniLcm/Models/ComplexFormComponent.cs +++ b/backend/FwLite/MiniLcm/Models/ComplexFormComponent.cs @@ -1,6 +1,8 @@ -namespace MiniLcm.Models; +using System.Text.Json.Serialization; -public record ComplexFormComponent : IObjectWithId +namespace MiniLcm.Models; + +public record ComplexFormComponent : IObjectWithId, IOrderable { public static ComplexFormComponent FromEntries(Entry complexFormEntry, Entry componentEntry, @@ -20,6 +22,8 @@ public static ComplexFormComponent FromEntries(Entry complexFormEntry, } public Guid Id { get; set; } + [JsonIgnore] + public double Order { get; set; } public DateTimeOffset? DeletedAt { get; set; } public virtual required Guid ComplexFormEntryId { get; set; } public string? ComplexFormHeadword { get; set; } diff --git a/backend/FwLite/MiniLcm/Models/ExampleSentence.cs b/backend/FwLite/MiniLcm/Models/ExampleSentence.cs index dde441994..e461dd49f 100644 --- a/backend/FwLite/MiniLcm/Models/ExampleSentence.cs +++ b/backend/FwLite/MiniLcm/Models/ExampleSentence.cs @@ -1,8 +1,12 @@ -namespace MiniLcm.Models; +using System.Text.Json.Serialization; -public class ExampleSentence : IObjectWithId +namespace MiniLcm.Models; + +public class ExampleSentence : IObjectWithId, IOrderable { public virtual Guid Id { get; set; } + [JsonIgnore] + public double Order { get; set; } public virtual MultiString Sentence { get; set; } = new(); public virtual MultiString Translation { get; set; } = new(); public virtual string? Reference { get; set; } = null; diff --git a/backend/FwLite/MiniLcm/Models/IOrderable.cs b/backend/FwLite/MiniLcm/Models/IOrderable.cs new file mode 100644 index 000000000..c5d1cd223 --- /dev/null +++ b/backend/FwLite/MiniLcm/Models/IOrderable.cs @@ -0,0 +1,7 @@ +namespace MiniLcm.Models; + +public interface IOrderable +{ + public Guid Id { get; } + public double Order { get; set; } +} diff --git a/backend/FwLite/MiniLcm/Models/Sense.cs b/backend/FwLite/MiniLcm/Models/Sense.cs index 2117e2738..d91c70dbd 100644 --- a/backend/FwLite/MiniLcm/Models/Sense.cs +++ b/backend/FwLite/MiniLcm/Models/Sense.cs @@ -1,8 +1,12 @@ -namespace MiniLcm.Models; +using System.Text.Json.Serialization; -public class Sense : IObjectWithId +namespace MiniLcm.Models; + +public class Sense : IObjectWithId, IOrderable { public virtual Guid Id { get; set; } + [JsonIgnore] + public double Order { get; set; } public DateTimeOffset? DeletedAt { get; set; } public Guid EntryId { get; set; } public virtual MultiString Definition { get; set; } = new(); diff --git a/backend/FwLite/MiniLcm/SyncHelpers/DiffCollection.cs b/backend/FwLite/MiniLcm/SyncHelpers/DiffCollection.cs index 33bda1e46..1e8ad1b75 100644 --- a/backend/FwLite/MiniLcm/SyncHelpers/DiffCollection.cs +++ b/backend/FwLite/MiniLcm/SyncHelpers/DiffCollection.cs @@ -1,4 +1,7 @@ -using MiniLcm.Models; +using System.Text.Json; +using System.Text.Json.JsonDiffPatch; +using System.Text.Json.Nodes; +using MiniLcm.Models; namespace MiniLcm.SyncHelpers; @@ -104,6 +107,71 @@ public static async Task Diff( return changes; } + /// + /// + /// + /// + /// + /// + /// + /// + /// + /// api, before, after is the parameter order + /// + /// + /// + public static async Task DiffOrderable( + IMiniLcmApi api, + IList before, + IList after, + Func identity, + Func, Task> add, + Func> remove, + Func, Task> move, + Func> replace) where TId : notnull where T : IOrderable + { + var positionDiffs = DiffPositions(before, after, identity) + // Order: Deletes first, then adds and moves from lowest to highest new index + // important, because new indexes represent final positions, which might not exist yet in the before list + // With this order, callers don't have to account for potential gaps + .OrderBy(d => d.To ?? -1).ToList(); + + var unstableIndexes = positionDiffs.Select(diff => diff.From).Where(i => i is not null).ToList(); + var stable = before.Select((value, index) => new { value, index }) + .Where(x => !unstableIndexes.Contains(x.index)) + .ToDictionary(x => x.index, x => x.value); + + var changes = 0; + foreach (var diff in positionDiffs) + { + if (diff.From is not null && diff.To is not null) + { + var afterEntry = after[diff.To.Value]; + changes += await move(api, afterEntry, diff.To.Value, stable); + } + else if (diff.From is not null) + { + changes += await remove(api, before[diff.From.Value]); + } + else if (diff.To is not null) + { + var afterEntry = after[diff.To.Value]; + changes += await add(api, afterEntry, diff.To.Value, stable); + } + } + + var afterEntriesDict = after.ToDictionary(identity); + foreach (var beforeEntry in before) + { + if (afterEntriesDict.TryGetValue(identity(beforeEntry), out var afterEntry)) + { + changes += await replace(api, beforeEntry, afterEntry); + } + } + + return changes; + } + public static async Task Diff( IMiniLcmApi api, IList before, @@ -115,6 +183,18 @@ public static async Task Diff( return await Diff(api, before, after, t => t.Id, add, remove, replace); } + public static async Task DiffOrderable( + IMiniLcmApi api, + IList before, + IList after, + Func, Task> add, + Func> remove, + Func, Task> move, + Func> replace) where T : IObjectWithId, IOrderable + { + return await DiffOrderable(api, before, after, t => (t as IObjectWithId).Id, add, remove, move, replace); + } + public static async Task Diff( IMiniLcmApi api, IList before, @@ -138,4 +218,58 @@ public static async Task Diff( }, async (api, beforeEntry, afterEntry) => await replace(api, beforeEntry, afterEntry)); } + + private static List DiffPositions( + IList before, + IList after, + Func identity) + { + var beforeJson = new JsonArray(before.Select(item => JsonValue.Create(identity(item))).ToArray()); + var afterJson = new JsonArray(after.Select(item => JsonValue.Create(identity(item))).ToArray()); + var result = JsonDiffPatcher.Diff(beforeJson, afterJson) as JsonObject; + + static IEnumerable GetDiff(JsonObject result) + { + foreach (var prop in result) + { + if (prop.Key == "_t") // diff type + { + if (prop.Value!.ToString() != "a") // we're only using the library for diffing shallow arrays + { + throw new InvalidOperationException("Only array diff results are supported"); + } + continue; + } + else if (prop.Key.StartsWith("_")) // e.g _4 => the key represents an old index (removed or moved) + { + var previousIndex = int.Parse(prop.Key[1..]); + var delta = prop.Value!.AsArray(); + var wasMoved = delta[2]!.GetValue() == 3; // 3 is magic number for a move operation + int? newIndex = wasMoved ? delta[1]!.GetValue() : null; // if it was moved, the new index is at index 1 + if (newIndex is not null) + { + yield return new PositionDiff { From = previousIndex, To = newIndex }; // move + } + else + { + yield return new PositionDiff { From = previousIndex }; // remove + } + } + else // e.g. 4 => the key represents a new index + { + var addIndex = int.Parse(prop.Key); + var value = JsonSerializer.Deserialize(prop.Value!.ToString()); + yield return new PositionDiff { To = addIndex }; // add + } + } + } + + return GetDiff(result!).ToList(); + } + + private class PositionDiff + { + public int? From { get; init; } + public int? To { get; init; } + } } diff --git a/backend/FwLite/MiniLcm/SyncHelpers/EntrySync.cs b/backend/FwLite/MiniLcm/SyncHelpers/EntrySync.cs index c8f2db5a3..41ae8b4a8 100644 --- a/backend/FwLite/MiniLcm/SyncHelpers/EntrySync.cs +++ b/backend/FwLite/MiniLcm/SyncHelpers/EntrySync.cs @@ -109,14 +109,59 @@ static async (api, beforeComponent) => ); } + private static double CalcOrderValueForIndex(int i, IDictionary stable, double prevMin) where T : IOrderable + { + double? stableOrderBefore = null; + double? stableOrderAfter = null; + + for (var j = i - 1; j >= 0; j--) + { + if (stable.TryGetValue(j, out var stableItem)) + { + stableOrderBefore = stableItem.Order; + break; + } + } + + for (var j = i + 1; j < stable.Count; j++) + { + if (stable.TryGetValue(j, out var stableItem)) + { + stableOrderAfter = stableItem.Order; + break; + } + } + + if (stableOrderBefore is not null && stableOrderAfter is not null) + return (stableOrderBefore.Value + stableOrderAfter.Value) / 2; + if (stableOrderBefore is not null) + return stableOrderBefore.Value + 1; + if (stableOrderAfter is not null) + return stableOrderAfter.Value - 1; + return prevMin; + } + private static async Task SensesSync(Guid entryId, IList afterSenses, IList beforeSenses, IMiniLcmApi api) { - Func> add = async (api, afterSense) => + var prevMin = beforeSenses.Min(s => s.Order); + Func, Task> add = async (api, afterSense, to, stable) => { await api.CreateSense(entryId, afterSense); + // todo: calculating + var order = CalcOrderValueForIndex(to, stable, prevMin); + afterSense.Order = order; + stable.Add(to, afterSense); + return 1; + }; + Func, Task> move = async (api, afterSense, to, stable) => + { + var order = CalcOrderValueForIndex(to, stable, prevMin); + afterSense.Order = order; + stable.Add(to, afterSense); + await api.MoveSense(entryId, afterSense); return 1; }; Func> remove = async (api, beforeSense) => @@ -125,7 +170,7 @@ private static async Task SensesSync(Guid entryId, return 1; }; Func> replace = async (api, beforeSense, afterSense) => await SenseSync.Sync(entryId, afterSense, beforeSense, api); - return await DiffCollection.Diff(api, beforeSenses, afterSenses, add, remove, replace); + return await DiffCollection.DiffOrderable(api, beforeSenses, afterSenses, add, remove, move, replace); } public static UpdateObjectInput? EntryDiffToUpdate(Entry beforeEntry, Entry afterEntry) From 62864b24be78165870c34ff915e3fa1aee6d068c Mon Sep 17 00:00:00 2001 From: Tim Haasdyk Date: Wed, 4 Dec 2024 17:42:53 +0100 Subject: [PATCH 02/28] Add some collection diff tests and redesign to use after/before positioning --- .../Api/FwDataMiniLcmApi.cs | 46 +++- .../FwLiteProjectSync/DryRunMiniLcmApi.cs | 6 +- .../LcmCrdt/Changes/CreateSenseChange.cs | 1 + backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs | 26 +- backend/FwLite/LcmCrdt/LcmCrdtKernel.cs | 5 - backend/FwLite/LcmCrdt/OrderPicker.cs | 25 ++ .../MiniLcm.Tests/DiffCollectionTests.cs | 248 ++++++++++++++++-- backend/FwLite/MiniLcm/IMiniLcmWriteApi.cs | 4 +- .../MiniLcm/SyncHelpers/DiffCollection.cs | 71 +++-- .../FwLite/MiniLcm/SyncHelpers/EntrySync.cs | 49 ++-- frontend/viewer/src/lib/Editor.svelte | 1 + 11 files changed, 366 insertions(+), 116 deletions(-) create mode 100644 backend/FwLite/LcmCrdt/OrderPicker.cs diff --git a/backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs b/backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs index 976ff4fd6..52b3b15b5 100644 --- a/backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs +++ b/backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs @@ -874,9 +874,9 @@ public Task DeleteEntry(Guid id) return Task.CompletedTask; } - internal void CreateSense(ILexEntry lexEntry, Sense sense) + internal void CreateSense(ILexEntry lexEntry, Sense sense, Guid? afterSenseId = null, Guid? beforeSenseId = null) { - var lexSense = LexSenseFactory.Create(sense.Id, lexEntry); + var lexSense = LexSenseFactory.Create(sense.Id); var msa = new SandboxGenericMSA() { MsaType = lexSense.GetDesiredMsaType() }; if (sense.PartOfSpeechId.HasValue && PartOfSpeechRepository.TryGetObject(sense.PartOfSpeechId.Value, out var pos)) { @@ -884,6 +884,33 @@ internal void CreateSense(ILexEntry lexEntry, Sense sense) } lexSense.SandboxMSA = msa; ApplySenseToLexSense(sense, lexSense); + InsertSense(lexEntry, lexSense, afterSenseId, beforeSenseId); + } + + internal void InsertSense(ILexEntry lexEntry, ILexSense lexSense, Guid? afterSenseId = null, Guid? beforeSenseId = null) + { + if (beforeSenseId.HasValue || afterSenseId.HasValue) + { + var senseDict = lexEntry.SensesOS.ToDictionary(s => s.Guid); + if (afterSenseId.HasValue && senseDict.TryGetValue(afterSenseId.Value, out var afterSense)) + { + var insertI = lexEntry.SensesOS.IndexOf(afterSense) + 1; + lexEntry.SensesOS.Insert(insertI, lexSense); + } + else if (beforeSenseId.HasValue && senseDict.TryGetValue(beforeSenseId.Value, out var beforeSense)) + { + var insertI = lexEntry.SensesOS.IndexOf(beforeSense); + lexEntry.SensesOS.Insert(insertI, lexSense); + } + else + { + lexEntry.SensesOS.Add(lexSense); + } + } + else + { + lexEntry.SensesOS.Add(lexSense); + } } private void ApplySenseToLexSense(Sense sense, ILexSense lexSense) @@ -917,7 +944,7 @@ private void ApplySenseToLexSense(Sense sense, ILexSense lexSense) return Task.FromResult(lcmSense is null ? null : FromLexSense(lcmSense)); } - public Task CreateSense(Guid entryId, Sense sense) + public Task CreateSense(Guid entryId, Sense sense, Guid? afterSenseId = null, Guid? beforeSenseId = null) { if (sense.Id == default) sense.Id = Guid.NewGuid(); if (!EntriesRepository.TryGetObject(entryId, out var lexEntry)) @@ -925,7 +952,7 @@ public Task CreateSense(Guid entryId, Sense sense) UndoableUnitOfWorkHelper.DoUsingNewOrCurrentUOW("Create Sense", "Remove sense", Cache.ServiceLocator.ActionHandler, - () => CreateSense(lexEntry, sense)); + () => CreateSense(lexEntry, sense, beforeSenseId, afterSenseId)); return Task.FromResult(FromLexSense(SenseRepository.GetObject(sense.Id))); } @@ -955,10 +982,15 @@ await Cache.DoUsingNewOrCurrentUOW("Update Sense", return await GetSense(entryId, after.Id) ?? throw new NullReferenceException("unable to find sense with id " + after.Id); } - public Task MoveSense(Guid entryId, Sense afterSense, int to) + public Task MoveSense(Guid entryId, Sense sense, Guid? afterSenseId = null, Guid? beforeSenseId = null) { - // todo: to is the final destination - return Task.FromResult(afterSense); + if (!EntriesRepository.TryGetObject(entryId, out var lexEntry)) + throw new InvalidOperationException("Entry not found"); + if (!SenseRepository.TryGetObject(sense.Id, out var lexSense)) + throw new InvalidOperationException("Sense not found"); + lexEntry.SensesOS.Remove(lexSense); + InsertSense(lexEntry, lexSense, afterSenseId, beforeSenseId); + return Task.FromResult(sense); } public Task AddSemanticDomainToSense(Guid senseId, SemanticDomain semanticDomain) diff --git a/backend/FwLite/FwLiteProjectSync/DryRunMiniLcmApi.cs b/backend/FwLite/FwLiteProjectSync/DryRunMiniLcmApi.cs index 0e205d744..4e9fdad9c 100644 --- a/backend/FwLite/FwLiteProjectSync/DryRunMiniLcmApi.cs +++ b/backend/FwLite/FwLiteProjectSync/DryRunMiniLcmApi.cs @@ -195,7 +195,7 @@ public async Task RemoveComplexFormType(Guid entryId, Guid complexFormTypeId) return api.GetSense(entryId, id); } - public Task CreateSense(Guid entryId, Sense sense) + public Task CreateSense(Guid entryId, Sense sense, Guid? afterSenseId = null, Guid? beforeSenseId = null) { DryRunRecords.Add(new DryRunRecord(nameof(CreateSense), $"Create sense {sense.Gloss}")); return Task.FromResult(sense); @@ -218,9 +218,9 @@ public async Task UpdateSense(Guid entryId, Sense before, Sense after) return await GetSense(entryId, after.Id) ?? throw new NullReferenceException($"unable to find sense with id {after.Id}"); } - public Task MoveSense(Guid entryId, Sense sense) + public Task MoveSense(Guid entryId, Sense sense, Guid? afterSenseId = null, Guid? beforeSenseId = null) { - DryRunRecords.Add(new DryRunRecord(nameof(MoveSense), $"Move sense {sense.Gloss} to {sense.Order}")); + DryRunRecords.Add(new DryRunRecord(nameof(MoveSense), $"Move sense {sense.Gloss} between {beforeSenseId} and {afterSenseId}")); return Task.FromResult(sense); } diff --git a/backend/FwLite/LcmCrdt/Changes/CreateSenseChange.cs b/backend/FwLite/LcmCrdt/Changes/CreateSenseChange.cs index 8f2dc2bb4..5b3649e01 100644 --- a/backend/FwLite/LcmCrdt/Changes/CreateSenseChange.cs +++ b/backend/FwLite/LcmCrdt/Changes/CreateSenseChange.cs @@ -11,6 +11,7 @@ public CreateSenseChange(Sense sense, Guid entryId) : base(sense.Id == Guid.Empt { sense.Id = EntityId; EntryId = entryId; + Order = sense.Order; Definition = sense.Definition; SemanticDomains = sense.SemanticDomains; Gloss = sense.Gloss; diff --git a/backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs b/backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs index d12a96ece..e5fe2c97c 100644 --- a/backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs +++ b/backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs @@ -267,10 +267,10 @@ private async IAsyncEnumerable GetEntries( if (sortWs is null) throw new NullReferenceException($"sort writing system {options.Order.WritingSystem} not found"); queryable = queryable - .LoadWith(e => e.Senses.DefaultOrder()) - .ThenLoad(s => s.ExampleSentences.DefaultOrder()) + .LoadWith(e => e.Senses) + .ThenLoad(s => s.ExampleSentences) .LoadWith(e => e.ComplexForms) - .LoadWith(e => e.Components.DefaultOrder()) + .LoadWith(e => e.Components) .AsQueryable() .OrderBy(e => e.Headword(sortWs.WsId).CollateUnicode(sortWs)) .ThenBy(e => e.Id); @@ -278,6 +278,13 @@ private async IAsyncEnumerable GetEntries( var entries = queryable.AsAsyncEnumerable(); await foreach (var entry in entries) { + entry.Senses = entry.Senses.DefaultOrder().ToList(); + entry.ComplexForms = entry.ComplexForms.DefaultOrder().ToList(); + entry.Components = entry.Components.DefaultOrder().ToList(); + foreach (var sense in entry.Senses) + { + sense.ExampleSentences = sense.ExampleSentences.DefaultOrder().ToList(); + } yield return entry; } } @@ -479,13 +486,11 @@ private async IAsyncEnumerable CreateSenseChanges(Guid entryId, Sense s return entry?.Senses.FirstOrDefault(s => s.Id == id); } - public async Task CreateSense(Guid entryId, Sense sense) + public async Task CreateSense(Guid entryId, Sense sense, Guid? afterSenseId = null, Guid? beforeSenseId = null) { - if (sense.Order == default) - { - var entry = await GetEntry(entryId) ?? throw new NullReferenceException($"unable to find entry with id {entryId}"); - sense.Order = entry.Senses.Max(s => s.Order) + 1; - } + if (sense.Order != default) // we don't anticipate this being necessary, so we'll be strict for now + throw new InvalidOperationException("Order should not be provided when creating a sense"); + sense.Order = await dataModel.PickOrder(afterSenseId, beforeSenseId); await dataModel.AddChanges(ClientId, await CreateSenseChanges(entryId, sense).ToArrayAsync()); return await dataModel.GetLatest(sense.Id) ?? throw new NullReferenceException(); } @@ -506,8 +511,9 @@ public async Task UpdateSense(Guid entryId, Sense before, Sense after) return await GetSense(entryId, after.Id) ?? throw new NullReferenceException("unable to find sense with id " + after.Id); } - public async Task MoveSense(Guid entryId, Sense sense) + public async Task MoveSense(Guid entryId, Sense sense, Guid? afterSenseId = null, Guid? beforeSenseId = null) { + sense.Order = await dataModel.PickOrder(afterSenseId, beforeSenseId); await dataModel.AddChange(ClientId, Changes.SetOrderChange.To(sense.Id, sense.Order)); return await dataModel.GetLatest(sense.Id) ?? throw new NullReferenceException(); } diff --git a/backend/FwLite/LcmCrdt/LcmCrdtKernel.cs b/backend/FwLite/LcmCrdt/LcmCrdtKernel.cs index 6ac69f5a1..30a740e08 100644 --- a/backend/FwLite/LcmCrdt/LcmCrdtKernel.cs +++ b/backend/FwLite/LcmCrdt/LcmCrdtKernel.cs @@ -159,11 +159,6 @@ public static void ConfigureCrdt(CrdtConfig config) }).IsUnique().HasFilter($"{componentSenseId} IS NULL"); }); - config.ObjectTypeListBuilder.OrderConfig(new OrderConfig( - o => o.Order, - o => o.Id - )); - config.ChangeTypeListBuilder.Add>() .Add>() .Add>() diff --git a/backend/FwLite/LcmCrdt/OrderPicker.cs b/backend/FwLite/LcmCrdt/OrderPicker.cs new file mode 100644 index 000000000..40c63dd5d --- /dev/null +++ b/backend/FwLite/LcmCrdt/OrderPicker.cs @@ -0,0 +1,25 @@ + +using SIL.Harmony; + +namespace LcmCrdt; + +public static class OrderPicker +{ + public static async Task PickOrder(this DataModel dataModel, Guid? afterId = null, Guid? beforeId = null) where T : class, IOrderable + { + var after = afterId is not null ? + await dataModel.GetLatest(afterId.Value) ?? throw new NullReferenceException($"unable to find {typeof(T).Name} with id {afterId}") + : null; + var before = beforeId is not null ? + await dataModel.GetLatest(beforeId.Value) ?? throw new NullReferenceException($"unable to find {typeof(T).Name} with id {beforeId}") + : null; + + return (after, before) switch + { + (null, null) => 1, + (null, _) => before.Order - 1, + (_, null) => after.Order + 1, + _ => (before.Order + after.Order) / 2, + }; + } +} diff --git a/backend/FwLite/MiniLcm.Tests/DiffCollectionTests.cs b/backend/FwLite/MiniLcm.Tests/DiffCollectionTests.cs index 241611c9b..d7a23a11a 100644 --- a/backend/FwLite/MiniLcm.Tests/DiffCollectionTests.cs +++ b/backend/FwLite/MiniLcm.Tests/DiffCollectionTests.cs @@ -1,3 +1,4 @@ +using FluentAssertions.Execution; using MiniLcm.SyncHelpers; using Moq; @@ -6,28 +7,233 @@ namespace MiniLcm.Tests; public class DiffCollectionTests { [Fact] - public async void FirstTest() + public async Task MatchingCollections_NoChangesAreGenerated() { - var api = new Mock(); - var changeCount = await DiffCollection.DiffOrderable(api.Object, new List(), new List(), - (value) => value.Id, - (_api, value, newI, stable) => Task.FromResult(1), - (_api, value) => Task.FromResult(1), - (_api, value, newI, stable) => Task.FromResult(1), - (_api, oldValue, newValue) => Task.FromResult(1)); + var value1 = Orderable(1, Guid.NewGuid()); + var value2 = Orderable(2, Guid.NewGuid()); + var (changeCount, _, _) = await Diff([value1, value2], [value1, value2]); changeCount.Should().Be(0); - api.VerifyNoOtherCalls(); - } - - // [Theory] - // [InlineData("gx")] - // [InlineData("oo")] - // [InlineData("eng")] // Three-letter codes not allowed when there's a valid two-letter code - // [InlineData("eng-Zxxx-x-audio")] - // [InlineData("nonsense")] - // public void InvalidWritingSystemId_ShouldThrow(string code) - // { - // Assert.Throws(() => new WritingSystemId(code)); - // } + } + + [Fact] + public async Task AddedValues() + { + var value1 = Orderable(1, Guid.NewGuid()); + var value2 = Orderable(2, Guid.NewGuid()); + var value3 = Orderable(3, Guid.NewGuid()); + var (changeCount, diffApi, api) = await Diff([value1], [value2, value1, value3]); + + changeCount.Should().Be(2); + diffApi.Verify(dApi => dApi.Add(api, value2, 0, It.IsAny>()), Times.Once); + diffApi.Verify(dApi => dApi.Add(api, value3, 2, It.IsAny>()), Times.Once); + diffApi.Verify(dApi => dApi.Replace(api, value1, value1), Times.Once); + diffApi.VerifyNoOtherCalls(); + } + + [Fact] + public async Task RemovedValues() + { + var value1 = Orderable(1, Guid.NewGuid()); + var value2 = Orderable(2, Guid.NewGuid()); + var value3 = Orderable(3, Guid.NewGuid()); + var (changeCount, diffApi, api) = await Diff([value2, value1, value3], [value1]); + + changeCount.Should().Be(2); + diffApi.Verify(dApi => dApi.Remove(api, value2), Times.Once); + diffApi.Verify(dApi => dApi.Remove(api, value3), Times.Once); + diffApi.Verify(dApi => dApi.Replace(api, value1, value1), Times.Once); + diffApi.VerifyNoOtherCalls(); + } + + [Fact] + public async Task SwappedValues() + { + var value1 = Orderable(1, Guid.NewGuid()); + var value2 = Orderable(2, Guid.NewGuid()); + var (changeCount, diffApi, api) = await Diff([value1, value2], [value2, value1]); + + changeCount.Should().Be(1); + diffApi.Verify(dApi => dApi.Move(api, value1, 1, It.IsAny>()), Times.Once); + diffApi.Verify(dApi => dApi.Replace(api, value1, value1), Times.Once); + diffApi.Verify(dApi => dApi.Replace(api, value2, value2), Times.Once); + diffApi.VerifyNoOtherCalls(); + } + + public static IEnumerable CollectionDiffTestCaseData() + { + var _1 = Orderable(1, Guid.NewGuid()); + var _2 = Orderable(2, Guid.NewGuid()); + var _3 = Orderable(3, Guid.NewGuid()); + var _4 = Orderable(4, Guid.NewGuid()); + yield return [new CollectionDiffTestCase + { + OldValues = [_1, _2, _3, _4], + NewValues = [_1, _4, _2, _3], + ExpectedOperations = [ + new() { From = 3, To = 1 }, + ], + }]; + yield return [new CollectionDiffTestCase + { + OldValues = [_1, _2, _3, _4], + NewValues = [_2, _1, _4, _3], + ExpectedOperations = [ // Moving the 2 outsides to middle is represented slightly differently: + new() { From = 0, To = 1 }, + new() { From = 2, To = 3 }, + ], + }]; + + var _5 = Orderable(5, Guid.NewGuid()); + var _6 = Orderable(6, Guid.NewGuid()); + var _7 = Orderable(7, Guid.NewGuid()); + yield return [new CollectionDiffTestCase + { + OldValues = [_1, _2, _3, _4, _5], + NewValues = [_6, _4, _2, _7], + ExpectedOperations = [ + new() { From = 0 }, // delete _1 + new() { From = 2 }, // delete _3 + new() { From = 4 }, // delete _5 + new() { To = 0 }, // add _6 + new() { From = 1, To = 2 }, // move _2 back + new() { To = 3 }, // add _7 + ], + }]; + } + + [Theory] + [MemberData(nameof(CollectionDiffTestCaseData))] + public async Task DiffTests(CollectionDiffTestCase testCase) + { + var (changeCount, diffApi, api) = await Diff(testCase.OldValues, testCase.NewValues); + + using var scope = new AssertionScope(); + + changeCount.Should().Be(testCase.ExpectedOperations.Count); + + var expectedReplaceCount = testCase.OldValues.Select(v => v.Id).Intersect(testCase.NewValues.Select(v => v.Id)).Count(); + diffApi.Verify(dApi => dApi.Replace(api, It.IsAny(), It.IsAny()), Times.Exactly(expectedReplaceCount)); + + foreach (var operation in testCase.ExpectedOperations) + { + try + { + if (operation.From is not null && operation.To is not null) + { + var movedValue = testCase.OldValues[operation.From.Value]; + diffApi.Verify( + dApi => dApi.Move( + api, + movedValue, + operation.To.Value, + It.IsAny>() + ), + Times.Once + ); + } + else if (operation.From is not null) + { + var removedValue = testCase.OldValues[operation.From.Value]; + diffApi.Verify( + dApi => dApi.Remove( + api, + removedValue + ), + Times.Once + ); + } + else if (operation.To is not null) + { + var addedValue = testCase.NewValues[operation.To.Value]; + diffApi.Verify( + dApi => dApi.Add( + api, + addedValue, + operation.To.Value, + It.IsAny>() + ), + Times.Once + ); + } + } + catch (Exception ex) + { + scope.AddPreFormattedFailure($"{ex.Message} From: {operation.From} To: {operation.To}"); + } + } + + diffApi.VerifyNoOtherCalls(); + } + + private static IOrderable Orderable(double order, Guid? id = null) + { + id ??= Guid.NewGuid(); + var orderable = new Mock(); + orderable.SetupGet(o => o.Order).Returns(order); + orderable.SetupGet(o => o.Id).Returns(id.Value); + return orderable.Object; + } + + private static async Task<(int, Mock, IMiniLcmApi)> Diff(IOrderable[] oldValues, IOrderable[] newValues) + { + var api = new Mock().Object; + var diffApi = new Mock(); + diffApi.Setup(f => f.Add(api, It.IsAny(), It.IsAny(), It.IsAny>())) + .ReturnsAsync(1); + diffApi.Setup(f => f.Remove(api, It.IsAny())) + .ReturnsAsync(1); + diffApi.Setup(f => f.Move(api, It.IsAny(), It.IsAny(), It.IsAny>())) + .ReturnsAsync(1); + diffApi.Setup(f => f.Replace(api, It.IsAny(), It.IsAny())) + .Returns((IMiniLcmApi api, IOrderable oldValue, IOrderable newValue) => + { + try + { + oldValue.Should().BeEquivalentTo(newValue); + return Task.FromResult(0); + } + catch + { + return Task.FromResult(1); + } + }); + + var changeCount = await DiffCollection.DiffOrderable(api, oldValues, newValues, + (value) => value.Id, + diffApi.Object.Add, + diffApi.Object.Remove, + diffApi.Object.Move, + diffApi.Object.Replace); + + return (changeCount, diffApi, api); + } +} + +public class DiffResult +{ + public required int ChangeCount { get; init; } + public required Mock DiffApi { get; init; } + public required IMiniLcmApi Api { get; init; } +} + +public interface DiffApi +{ + Task Add(IMiniLcmApi api, IOrderable value, int newI, IDictionary stable); + Task Remove(IMiniLcmApi api, IOrderable value); + Task Move(IMiniLcmApi api, IOrderable value, int newI, IDictionary stable); + Task Replace(IMiniLcmApi api, IOrderable oldValue, IOrderable newValue); +} + +public class CollectionDiffTestCase +{ + public required IOrderable[] OldValues { get; init; } + public required IOrderable[] NewValues { get; init; } + public required List ExpectedOperations { get; init; } +} + +public class CollectionDiffOperation +{ + public int? From { get; init; } + public int? To { get; init; } } diff --git a/backend/FwLite/MiniLcm/IMiniLcmWriteApi.cs b/backend/FwLite/MiniLcm/IMiniLcmWriteApi.cs index 2f7f41962..6e3d098fe 100644 --- a/backend/FwLite/MiniLcm/IMiniLcmWriteApi.cs +++ b/backend/FwLite/MiniLcm/IMiniLcmWriteApi.cs @@ -46,10 +46,10 @@ Task UpdateWritingSystem(WritingSystemId id, #endregion #region Sense - Task CreateSense(Guid entryId, Sense sense); + Task CreateSense(Guid entryId, Sense sense, Guid? afterSenseId = null, Guid? beforeSenseId = null); Task UpdateSense(Guid entryId, Guid senseId, UpdateObjectInput update); Task UpdateSense(Guid entryId, Sense before, Sense after); - Task MoveSense(Guid entryId, Sense afterSense); + Task MoveSense(Guid entryId, Sense sense, Guid? afterSenseId = null, Guid? beforeSenseId = null); Task DeleteSense(Guid entryId, Guid senseId); Task AddSemanticDomainToSense(Guid senseId, SemanticDomain semanticDomain); Task RemoveSemanticDomainFromSense(Guid senseId, Guid semanticDomainId); diff --git a/backend/FwLite/MiniLcm/SyncHelpers/DiffCollection.cs b/backend/FwLite/MiniLcm/SyncHelpers/DiffCollection.cs index 1e8ad1b75..57371c565 100644 --- a/backend/FwLite/MiniLcm/SyncHelpers/DiffCollection.cs +++ b/backend/FwLite/MiniLcm/SyncHelpers/DiffCollection.cs @@ -125,9 +125,9 @@ public static async Task DiffOrderable( IList before, IList after, Func identity, - Func, Task> add, + Func, Task> add, Func> remove, - Func, Task> move, + Func, Task> move, Func> replace) where TId : notnull where T : IOrderable { var positionDiffs = DiffPositions(before, after, identity) @@ -137,9 +137,7 @@ public static async Task DiffOrderable( .OrderBy(d => d.To ?? -1).ToList(); var unstableIndexes = positionDiffs.Select(diff => diff.From).Where(i => i is not null).ToList(); - var stable = before.Select((value, index) => new { value, index }) - .Where(x => !unstableIndexes.Contains(x.index)) - .ToDictionary(x => x.index, x => x.value); + var stableIds = before.Where((_, i) => !unstableIndexes.Contains(i)).Select(item => item.Id).ToList(); var changes = 0; foreach (var diff in positionDiffs) @@ -147,7 +145,7 @@ public static async Task DiffOrderable( if (diff.From is not null && diff.To is not null) { var afterEntry = after[diff.To.Value]; - changes += await move(api, afterEntry, diff.To.Value, stable); + changes += await move(api, afterEntry, diff.To.Value, stableIds); } else if (diff.From is not null) { @@ -156,7 +154,7 @@ public static async Task DiffOrderable( else if (diff.To is not null) { var afterEntry = after[diff.To.Value]; - changes += await add(api, afterEntry, diff.To.Value, stable); + changes += await add(api, afterEntry, diff.To.Value, stableIds); } } @@ -187,9 +185,9 @@ public static async Task DiffOrderable( IMiniLcmApi api, IList before, IList after, - Func, Task> add, + Func, Task> add, Func> remove, - Func, Task> move, + Func, Task> move, Func> replace) where T : IObjectWithId, IOrderable { return await DiffOrderable(api, before, after, t => (t as IObjectWithId).Id, add, remove, move, replace); @@ -219,52 +217,51 @@ public static async Task Diff( async (api, beforeEntry, afterEntry) => await replace(api, beforeEntry, afterEntry)); } - private static List DiffPositions( + private static IEnumerable DiffPositions( IList before, IList after, Func identity) { var beforeJson = new JsonArray(before.Select(item => JsonValue.Create(identity(item))).ToArray()); var afterJson = new JsonArray(after.Select(item => JsonValue.Create(identity(item))).ToArray()); - var result = JsonDiffPatcher.Diff(beforeJson, afterJson) as JsonObject; - static IEnumerable GetDiff(JsonObject result) + if (JsonDiffPatcher.Diff(beforeJson, afterJson) is not JsonObject result) { - foreach (var prop in result) + yield break; // no changes + } + + foreach (var prop in result!) + { + if (prop.Key == "_t") // diff type { - if (prop.Key == "_t") // diff type + if (prop.Value!.ToString() != "a") // we're only using the library for diffing shallow arrays { - if (prop.Value!.ToString() != "a") // we're only using the library for diffing shallow arrays - { - throw new InvalidOperationException("Only array diff results are supported"); - } - continue; + throw new InvalidOperationException("Only array diff results are supported"); } - else if (prop.Key.StartsWith("_")) // e.g _4 => the key represents an old index (removed or moved) + continue; + } + else if (prop.Key.StartsWith("_")) // e.g _4 => the key represents an old index (removed or moved) + { + var previousIndex = int.Parse(prop.Key[1..]); + var delta = prop.Value!.AsArray(); + var wasMoved = delta[2]!.GetValue() == 3; // 3 is magic number for a move operation + int? newIndex = wasMoved ? delta[1]!.GetValue() : null; // if it was moved, the new index is at index 1 + if (newIndex is not null) { - var previousIndex = int.Parse(prop.Key[1..]); - var delta = prop.Value!.AsArray(); - var wasMoved = delta[2]!.GetValue() == 3; // 3 is magic number for a move operation - int? newIndex = wasMoved ? delta[1]!.GetValue() : null; // if it was moved, the new index is at index 1 - if (newIndex is not null) - { - yield return new PositionDiff { From = previousIndex, To = newIndex }; // move - } - else - { - yield return new PositionDiff { From = previousIndex }; // remove - } + yield return new PositionDiff { From = previousIndex, To = newIndex }; // move } - else // e.g. 4 => the key represents a new index + else { - var addIndex = int.Parse(prop.Key); - var value = JsonSerializer.Deserialize(prop.Value!.ToString()); - yield return new PositionDiff { To = addIndex }; // add + yield return new PositionDiff { From = previousIndex }; // remove } } + else // e.g. 4 => the key represents a new index + { + var addIndex = int.Parse(prop.Key); + yield return new PositionDiff { To = addIndex }; // add + } } - return GetDiff(result!).ToList(); } private class PositionDiff diff --git a/backend/FwLite/MiniLcm/SyncHelpers/EntrySync.cs b/backend/FwLite/MiniLcm/SyncHelpers/EntrySync.cs index 41ae8b4a8..9450bfd6c 100644 --- a/backend/FwLite/MiniLcm/SyncHelpers/EntrySync.cs +++ b/backend/FwLite/MiniLcm/SyncHelpers/EntrySync.cs @@ -109,64 +109,51 @@ static async (api, beforeComponent) => ); } - private static double CalcOrderValueForIndex(int i, IDictionary stable, double prevMin) where T : IOrderable + private static (Guid? beforeId, Guid? afterId) GetSurroundingIds(int i, IList current, List stable) where T : class, IOrderable { - double? stableOrderBefore = null; - double? stableOrderAfter = null; - + T? beforeEntity = null; + T? afterEntity = null; for (var j = i - 1; j >= 0; j--) { - if (stable.TryGetValue(j, out var stableItem)) + if (stable.Contains(current[j].Id)) { - stableOrderBefore = stableItem.Order; + beforeEntity = current[j]; break; } } - for (var j = i + 1; j < stable.Count; j++) { - if (stable.TryGetValue(j, out var stableItem)) + if (stable.Contains(current[j].Id)) { - stableOrderAfter = stableItem.Order; + afterEntity = current[j]; break; } } - - if (stableOrderBefore is not null && stableOrderAfter is not null) - return (stableOrderBefore.Value + stableOrderAfter.Value) / 2; - if (stableOrderBefore is not null) - return stableOrderBefore.Value + 1; - if (stableOrderAfter is not null) - return stableOrderAfter.Value - 1; - return prevMin; + return (beforeEntity?.Id, afterEntity?.Id); } - private static async Task SensesSync(Guid entryId, IList afterSenses, IList beforeSenses, IMiniLcmApi api) { var prevMin = beforeSenses.Min(s => s.Order); - Func, Task> add = async (api, afterSense, to, stable) => + Func, Task> add = async (api, sense, to, stable) => { - await api.CreateSense(entryId, afterSense); - // todo: calculating - var order = CalcOrderValueForIndex(to, stable, prevMin); - afterSense.Order = order; - stable.Add(to, afterSense); + var (beforeId, afterId) = GetSurroundingIds(to, afterSenses, stable); + await api.CreateSense(entryId, sense, beforeId, afterId); + stable.Add(sense.Id); return 1; }; - Func, Task> move = async (api, afterSense, to, stable) => + Func, Task> move = async (api, sense, to, stable) => { - var order = CalcOrderValueForIndex(to, stable, prevMin); - afterSense.Order = order; - stable.Add(to, afterSense); - await api.MoveSense(entryId, afterSense); + var (beforeId, afterId) = GetSurroundingIds(to, afterSenses, stable); + await api.MoveSense(entryId, sense, beforeId, afterId); + stable.Add(sense.Id); return 1; }; - Func> remove = async (api, beforeSense) => + Func> remove = async (api, sense) => { - await api.DeleteSense(entryId, beforeSense.Id); + await api.DeleteSense(entryId, sense.Id); return 1; }; Func> replace = async (api, beforeSense, afterSense) => await SenseSync.Sync(entryId, afterSense, beforeSense, api); diff --git a/frontend/viewer/src/lib/Editor.svelte b/frontend/viewer/src/lib/Editor.svelte index bb56788bc..9e26781c6 100644 --- a/frontend/viewer/src/lib/Editor.svelte +++ b/frontend/viewer/src/lib/Editor.svelte @@ -48,6 +48,7 @@ async function updateEntry(updatedEntry: IEntry) { if (entry.id != updatedEntry.id) throw new Error('Entry id mismatch'); + console.debug('Updating entry', updatedEntry); await saveHandler(() => lexboxApi.UpdateEntry(initialEntry, updatedEntry)); } From 80850a7809c33f529b5a7b158df8e84ee7b1d24c Mon Sep 17 00:00:00 2001 From: Tim Haasdyk Date: Thu, 5 Dec 2024 17:35:24 +0100 Subject: [PATCH 03/28] Exclude Order from external serialization, refactor, add more tests --- .../Api/FwDataMiniLcmApi.cs | 28 +++--- .../FwLiteProjectSync.Tests/ReorderTests.cs | 53 ++++++++++ .../FwLiteProjectSync/DryRunMiniLcmApi.cs | 9 +- .../LcmCrdt/Changes/CreateSenseChange.cs | 2 +- .../FwLite/LcmCrdt/Changes/SetOrderChange.cs | 4 +- backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs | 23 ++--- backend/FwLite/LcmCrdt/Json.cs | 9 ++ backend/FwLite/LcmCrdt/LcmCrdtKernel.cs | 8 +- backend/FwLite/LcmCrdt/OrderPicker.cs | 19 ++-- backend/FwLite/LcmCrdt/QueryHelpers.cs | 4 + backend/FwLite/LocalWebApp/LocalAppKernel.cs | 11 +-- .../MiniLcm.Tests/DiffCollectionTests.cs | 97 ++++++++++++++----- .../Attributes/MiniLcmInternalAttribute.cs | 6 ++ backend/FwLite/MiniLcm/IMiniLcmWriteApi.cs | 5 +- backend/FwLite/MiniLcm/MiniLcmJson.cs | 22 +++++ .../MiniLcm/Models/ComplexFormComponent.cs | 2 +- .../FwLite/MiniLcm/Models/ExampleSentence.cs | 2 +- backend/FwLite/MiniLcm/Models/IOrderable.cs | 4 +- backend/FwLite/MiniLcm/Models/Sense.cs | 7 +- .../MiniLcm/SyncHelpers/DiffCollection.cs | 82 ++++++++++++++-- .../FwLite/MiniLcm/SyncHelpers/EntrySync.cs | 34 +------ .../lib/services/service-provider-signalr.ts | 8 +- 22 files changed, 315 insertions(+), 124 deletions(-) create mode 100644 backend/FwLite/FwLiteProjectSync.Tests/ReorderTests.cs create mode 100644 backend/FwLite/MiniLcm/Attributes/MiniLcmInternalAttribute.cs create mode 100644 backend/FwLite/MiniLcm/MiniLcmJson.cs diff --git a/backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs b/backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs index 52b3b15b5..4d8b32d6f 100644 --- a/backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs +++ b/backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs @@ -874,9 +874,10 @@ public Task DeleteEntry(Guid id) return Task.CompletedTask; } - internal void CreateSense(ILexEntry lexEntry, Sense sense, Guid? afterSenseId = null, Guid? beforeSenseId = null) + internal void CreateSense(ILexEntry lexEntry, Sense sense, BetweenPosition? between = null) { var lexSense = LexSenseFactory.Create(sense.Id); + InsertSense(lexEntry, lexSense, between); var msa = new SandboxGenericMSA() { MsaType = lexSense.GetDesiredMsaType() }; if (sense.PartOfSpeechId.HasValue && PartOfSpeechRepository.TryGetObject(sense.PartOfSpeechId.Value, out var pos)) { @@ -884,22 +885,23 @@ internal void CreateSense(ILexEntry lexEntry, Sense sense, Guid? afterSenseId = } lexSense.SandboxMSA = msa; ApplySenseToLexSense(sense, lexSense); - InsertSense(lexEntry, lexSense, afterSenseId, beforeSenseId); } - internal void InsertSense(ILexEntry lexEntry, ILexSense lexSense, Guid? afterSenseId = null, Guid? beforeSenseId = null) + internal void InsertSense(ILexEntry lexEntry, ILexSense lexSense, BetweenPosition? between = null) { - if (beforeSenseId.HasValue || afterSenseId.HasValue) + if (between?.Before.HasValue is not null || between?.After is not null) { + var beforeSenseId = between?.Before; + var afterSenseId = between?.After; var senseDict = lexEntry.SensesOS.ToDictionary(s => s.Guid); - if (afterSenseId.HasValue && senseDict.TryGetValue(afterSenseId.Value, out var afterSense)) + if (beforeSenseId.HasValue && senseDict.TryGetValue(beforeSenseId.Value, out var beforeSense)) { - var insertI = lexEntry.SensesOS.IndexOf(afterSense) + 1; + var insertI = lexEntry.SensesOS.IndexOf(beforeSense) + 1; lexEntry.SensesOS.Insert(insertI, lexSense); } - else if (beforeSenseId.HasValue && senseDict.TryGetValue(beforeSenseId.Value, out var beforeSense)) + else if (afterSenseId.HasValue && senseDict.TryGetValue(afterSenseId.Value, out var afterSense)) { - var insertI = lexEntry.SensesOS.IndexOf(beforeSense); + var insertI = lexEntry.SensesOS.IndexOf(afterSense); lexEntry.SensesOS.Insert(insertI, lexSense); } else @@ -944,7 +946,7 @@ private void ApplySenseToLexSense(Sense sense, ILexSense lexSense) return Task.FromResult(lcmSense is null ? null : FromLexSense(lcmSense)); } - public Task CreateSense(Guid entryId, Sense sense, Guid? afterSenseId = null, Guid? beforeSenseId = null) + public Task CreateSense(Guid entryId, Sense sense, BetweenPosition? between = null) { if (sense.Id == default) sense.Id = Guid.NewGuid(); if (!EntriesRepository.TryGetObject(entryId, out var lexEntry)) @@ -952,7 +954,7 @@ public Task CreateSense(Guid entryId, Sense sense, Guid? afterSenseId = n UndoableUnitOfWorkHelper.DoUsingNewOrCurrentUOW("Create Sense", "Remove sense", Cache.ServiceLocator.ActionHandler, - () => CreateSense(lexEntry, sense, beforeSenseId, afterSenseId)); + () => CreateSense(lexEntry, sense, between)); return Task.FromResult(FromLexSense(SenseRepository.GetObject(sense.Id))); } @@ -982,14 +984,14 @@ await Cache.DoUsingNewOrCurrentUOW("Update Sense", return await GetSense(entryId, after.Id) ?? throw new NullReferenceException("unable to find sense with id " + after.Id); } - public Task MoveSense(Guid entryId, Sense sense, Guid? afterSenseId = null, Guid? beforeSenseId = null) + public Task MoveSense(Guid entryId, Sense sense, BetweenPosition between) { if (!EntriesRepository.TryGetObject(entryId, out var lexEntry)) throw new InvalidOperationException("Entry not found"); if (!SenseRepository.TryGetObject(sense.Id, out var lexSense)) throw new InvalidOperationException("Sense not found"); - lexEntry.SensesOS.Remove(lexSense); - InsertSense(lexEntry, lexSense, afterSenseId, beforeSenseId); + // LibLCM treats an insert as a move if the sense is already in the entry + InsertSense(lexEntry, lexSense, between); return Task.FromResult(sense); } diff --git a/backend/FwLite/FwLiteProjectSync.Tests/ReorderTests.cs b/backend/FwLite/FwLiteProjectSync.Tests/ReorderTests.cs new file mode 100644 index 000000000..a18af4619 --- /dev/null +++ b/backend/FwLite/FwLiteProjectSync.Tests/ReorderTests.cs @@ -0,0 +1,53 @@ +using FwLiteProjectSync.Tests.Fixtures; +using MiniLcm.Models; +using MiniLcm.SyncHelpers; +using MiniLcm.Tests.AutoFakerHelpers; +using Soenneker.Utils.AutoBogus; + +namespace FwLiteProjectSync.Tests; + +public class ReorderTests : IClassFixture +{ + private static readonly AutoFaker AutoFaker = new(builder => builder.WithOverride(new MultiStringOverride()).WithOverride(new ObjectWithIdOverride())); + public ReorderTests(SyncFixture fixture) + { + _fixture = fixture; + } + + private readonly SyncFixture _fixture; + + + [Fact] + public async Task CanReorderSensesViaSync() + { + var entry = await _fixture.CrdtApi.CreateEntry(new() { LexemeForm = { { "en", "complexForm1" } } }); + var sense1 = await _fixture.CrdtApi.CreateSense(entry.Id, new Sense() + { + Gloss = { { "en", "1" } }, + }); + var sense2 = await _fixture.CrdtApi.CreateSense(entry.Id, new Sense() + { + Gloss = { { "en", "2" } }, + }); + var sense3 = await _fixture.CrdtApi.CreateSense(entry.Id, new Sense() + { + Gloss = { { "en", "3" } }, + }); + + var before = (await _fixture.CrdtApi.GetEntry(entry.Id))!; + before.Senses.Should().BeEquivalentTo([sense1, sense2, sense3], options => options.WithStrictOrdering()); + before.Senses.Select(s => s.Order).Should().BeEquivalentTo([1, 2, 3]); + + var after = before!.Copy(); + after.Senses = [sense3, sense2, sense1]; + + await EntrySync.Sync(before, after, _fixture.CrdtApi); + + var actual = await _fixture.CrdtApi.GetEntry(after.Id); + actual.Should().NotBeNull(); + for (var i = 0; i < after.Senses.Count; i++) + { + actual!.Senses[i].Should().BeEquivalentTo(after.Senses[i], options => options.Excluding(x => x.Order)); + } + } +} diff --git a/backend/FwLite/FwLiteProjectSync/DryRunMiniLcmApi.cs b/backend/FwLite/FwLiteProjectSync/DryRunMiniLcmApi.cs index 4e9fdad9c..39662a344 100644 --- a/backend/FwLite/FwLiteProjectSync/DryRunMiniLcmApi.cs +++ b/backend/FwLite/FwLiteProjectSync/DryRunMiniLcmApi.cs @@ -1,5 +1,6 @@ using MiniLcm; using MiniLcm.Models; +using MiniLcm.SyncHelpers; namespace FwLiteProjectSync; @@ -195,9 +196,9 @@ public async Task RemoveComplexFormType(Guid entryId, Guid complexFormTypeId) return api.GetSense(entryId, id); } - public Task CreateSense(Guid entryId, Sense sense, Guid? afterSenseId = null, Guid? beforeSenseId = null) + public Task CreateSense(Guid entryId, Sense sense, BetweenPosition? position = null) { - DryRunRecords.Add(new DryRunRecord(nameof(CreateSense), $"Create sense {sense.Gloss}")); + DryRunRecords.Add(new DryRunRecord(nameof(CreateSense), $"Create sense {sense.Gloss} between {position?.Before} and {position?.After}")); return Task.FromResult(sense); } @@ -218,9 +219,9 @@ public async Task UpdateSense(Guid entryId, Sense before, Sense after) return await GetSense(entryId, after.Id) ?? throw new NullReferenceException($"unable to find sense with id {after.Id}"); } - public Task MoveSense(Guid entryId, Sense sense, Guid? afterSenseId = null, Guid? beforeSenseId = null) + public Task MoveSense(Guid entryId, Sense sense, BetweenPosition between) { - DryRunRecords.Add(new DryRunRecord(nameof(MoveSense), $"Move sense {sense.Gloss} between {beforeSenseId} and {afterSenseId}")); + DryRunRecords.Add(new DryRunRecord(nameof(MoveSense), $"Move sense {sense.Gloss} between {between.Before} and {between.After}")); return Task.FromResult(sense); } diff --git a/backend/FwLite/LcmCrdt/Changes/CreateSenseChange.cs b/backend/FwLite/LcmCrdt/Changes/CreateSenseChange.cs index 5b3649e01..dd034e2ed 100644 --- a/backend/FwLite/LcmCrdt/Changes/CreateSenseChange.cs +++ b/backend/FwLite/LcmCrdt/Changes/CreateSenseChange.cs @@ -38,8 +38,8 @@ public override async ValueTask NewEntity(Commit commit, ChangeContext co return new Sense { Id = EntityId, - Order = Order, EntryId = EntryId, + Order = Order, Definition = Definition ?? new MultiString(), Gloss = Gloss ?? new MultiString(), PartOfSpeech = PartOfSpeech ?? string.Empty, diff --git a/backend/FwLite/LcmCrdt/Changes/SetOrderChange.cs b/backend/FwLite/LcmCrdt/Changes/SetOrderChange.cs index 6342dcfee..00e976e4b 100644 --- a/backend/FwLite/LcmCrdt/Changes/SetOrderChange.cs +++ b/backend/FwLite/LcmCrdt/Changes/SetOrderChange.cs @@ -1,4 +1,5 @@ -using SIL.Harmony.Changes; +using System.Text.Json.Serialization; +using SIL.Harmony.Changes; using SIL.Harmony.Entities; namespace LcmCrdt.Changes; @@ -26,6 +27,7 @@ public static IChange To(Guid entityId, double order) return new SetOrderChange(entityId, order); } + [JsonConstructor] protected SetOrderChange(Guid entityId, double order) : base(entityId) { Order = order; diff --git a/backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs b/backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs index e5fe2c97c..973c6ae00 100644 --- a/backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs +++ b/backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs @@ -278,13 +278,7 @@ private async IAsyncEnumerable GetEntries( var entries = queryable.AsAsyncEnumerable(); await foreach (var entry in entries) { - entry.Senses = entry.Senses.DefaultOrder().ToList(); - entry.ComplexForms = entry.ComplexForms.DefaultOrder().ToList(); - entry.Components = entry.Components.DefaultOrder().ToList(); - foreach (var sense in entry.Senses) - { - sense.ExampleSentences = sense.ExampleSentences.DefaultOrder().ToList(); - } + entry.DefaultOrder(); yield return entry; } } @@ -298,6 +292,7 @@ private async IAsyncEnumerable GetEntries( .LoadWith(e => e.Components) .AsQueryable() .SingleOrDefaultAsync(e => e.Id == id); + entry?.DefaultOrder(); return entry; } @@ -486,11 +481,12 @@ private async IAsyncEnumerable CreateSenseChanges(Guid entryId, Sense s return entry?.Senses.FirstOrDefault(s => s.Id == id); } - public async Task CreateSense(Guid entryId, Sense sense, Guid? afterSenseId = null, Guid? beforeSenseId = null) + public async Task CreateSense(Guid entryId, Sense sense, BetweenPosition? between = null) { if (sense.Order != default) // we don't anticipate this being necessary, so we'll be strict for now throw new InvalidOperationException("Order should not be provided when creating a sense"); - sense.Order = await dataModel.PickOrder(afterSenseId, beforeSenseId); + + sense.Order = await dataModel.PickOrder(between, Senses.Where(s => s.EntryId == entryId)); await dataModel.AddChanges(ClientId, await CreateSenseChanges(entryId, sense).ToArrayAsync()); return await dataModel.GetLatest(sense.Id) ?? throw new NullReferenceException(); } @@ -511,11 +507,12 @@ public async Task UpdateSense(Guid entryId, Sense before, Sense after) return await GetSense(entryId, after.Id) ?? throw new NullReferenceException("unable to find sense with id " + after.Id); } - public async Task MoveSense(Guid entryId, Sense sense, Guid? afterSenseId = null, Guid? beforeSenseId = null) + public async Task MoveSense(Guid entryId, Sense sense, BetweenPosition between) { - sense.Order = await dataModel.PickOrder(afterSenseId, beforeSenseId); - await dataModel.AddChange(ClientId, Changes.SetOrderChange.To(sense.Id, sense.Order)); - return await dataModel.GetLatest(sense.Id) ?? throw new NullReferenceException(); + var order = await dataModel.PickOrder(between); + await dataModel.AddChange(ClientId, Changes.SetOrderChange.To(sense.Id, order)); + var updatedSense = await dataModel.GetLatest(sense.Id) ?? throw new NullReferenceException(); + return updatedSense; } public async Task DeleteSense(Guid entryId, Guid senseId) diff --git a/backend/FwLite/LcmCrdt/Json.cs b/backend/FwLite/LcmCrdt/Json.cs index 694d8c0f1..c0a8df033 100644 --- a/backend/FwLite/LcmCrdt/Json.cs +++ b/backend/FwLite/LcmCrdt/Json.cs @@ -1,8 +1,10 @@ using System.Linq.Expressions; using System.Reflection; +using System.Text.Json.Serialization.Metadata; using LinqToDB; using LinqToDB.Common; using LinqToDB.SqlQuery; +using SIL.Harmony; namespace LcmCrdt; @@ -109,4 +111,11 @@ public static bool IsIndexerPropertyMethod(MethodInfo method) { return valueAccess(prop); } + + public static IJsonTypeInfoResolver MakeLcmCrdtExternalJsonTypeResolver(this CrdtConfig config) + { + var resolver = config.MakeJsonTypeResolver(); + resolver = resolver.AddMiniLcmModifiers(); + return resolver; + } } diff --git a/backend/FwLite/LcmCrdt/LcmCrdtKernel.cs b/backend/FwLite/LcmCrdt/LcmCrdtKernel.cs index 30a740e08..e7c22230e 100644 --- a/backend/FwLite/LcmCrdt/LcmCrdtKernel.cs +++ b/backend/FwLite/LcmCrdt/LcmCrdtKernel.cs @@ -1,5 +1,4 @@ -using System.Linq.Expressions; -using System.Text.Json; +using System.Text.Json; using SIL.Harmony; using SIL.Harmony.Core; using SIL.Harmony.Changes; @@ -19,7 +18,6 @@ using Microsoft.Extensions.Options; using MiniLcm.Validators; using Refit; -using SIL.Harmony.Db; namespace LcmCrdt; @@ -44,12 +42,12 @@ public static IServiceCollection AddLcmCrdtClient(this IServiceCollection servic services.AddSingleton(); services.AddHttpClient(); - services.AddSingleton(provider => new RefitSettings + services.AddSingleton(provider => new RefitSettings { ContentSerializer = new SystemTextJsonContentSerializer(new(JsonSerializerDefaults.Web) { TypeInfoResolver = provider.GetRequiredService>().Value - .MakeJsonTypeResolver() + .MakeLcmCrdtExternalJsonTypeResolver() }) }); services.AddSingleton(); diff --git a/backend/FwLite/LcmCrdt/OrderPicker.cs b/backend/FwLite/LcmCrdt/OrderPicker.cs index 40c63dd5d..74286d853 100644 --- a/backend/FwLite/LcmCrdt/OrderPicker.cs +++ b/backend/FwLite/LcmCrdt/OrderPicker.cs @@ -1,24 +1,27 @@ +using MiniLcm.SyncHelpers; using SIL.Harmony; namespace LcmCrdt; public static class OrderPicker { - public static async Task PickOrder(this DataModel dataModel, Guid? afterId = null, Guid? beforeId = null) where T : class, IOrderable + public static async Task PickOrder(this DataModel dataModel, BetweenPosition? between = null, IQueryable? siblings = null) where T : class, IOrderable { - var after = afterId is not null ? - await dataModel.GetLatest(afterId.Value) ?? throw new NullReferenceException($"unable to find {typeof(T).Name} with id {afterId}") - : null; + var beforeId = between?.Before; + var afterId = between?.After; var before = beforeId is not null ? await dataModel.GetLatest(beforeId.Value) ?? throw new NullReferenceException($"unable to find {typeof(T).Name} with id {beforeId}") : null; + var after = afterId is not null ? + await dataModel.GetLatest(afterId.Value) ?? throw new NullReferenceException($"unable to find {typeof(T).Name} with id {afterId}") + : null; - return (after, before) switch + return (before, after) switch { - (null, null) => 1, - (null, _) => before.Order - 1, - (_, null) => after.Order + 1, + (null, null) => siblings?.Select(s => s.Order).DefaultIfEmpty().Max(order => order) + 1 ?? 1, + (_, null) => before.Order + 1, + (null, _) => after.Order - 1, _ => (before.Order + after.Order) / 2, }; } diff --git a/backend/FwLite/LcmCrdt/QueryHelpers.cs b/backend/FwLite/LcmCrdt/QueryHelpers.cs index b64988a1a..d9eb62d25 100644 --- a/backend/FwLite/LcmCrdt/QueryHelpers.cs +++ b/backend/FwLite/LcmCrdt/QueryHelpers.cs @@ -2,6 +2,10 @@ namespace LcmCrdt; public static class QueryHelpers { + public static void DefaultOrder(this Entry entry) + { + entry.Senses = entry.Senses.DefaultOrder().ToList(); + } public static IEnumerable DefaultOrder(this IEnumerable queryable) where T : IOrderable { return queryable diff --git a/backend/FwLite/LocalWebApp/LocalAppKernel.cs b/backend/FwLite/LocalWebApp/LocalAppKernel.cs index 4b375f243..5903f4f35 100644 --- a/backend/FwLite/LocalWebApp/LocalAppKernel.cs +++ b/backend/FwLite/LocalWebApp/LocalAppKernel.cs @@ -1,16 +1,11 @@ -using System.Text.Json; -using SIL.Harmony; -using FwLiteProjectSync; -using FwDataMiniLcmBridge; +using SIL.Harmony; using FwLiteShared; using FwLiteShared.Auth; -using FwLiteShared.Sync; using LcmCrdt; using LocalWebApp.Services; using Microsoft.AspNetCore.Http.Json; using Microsoft.AspNetCore.SignalR; using Microsoft.Extensions.Options; -using Refit; namespace LocalWebApp; @@ -27,13 +22,13 @@ public static IServiceCollection AddLocalAppServices(this IServiceCollection ser services.AddOptions().PostConfigure>((jsonOptions, crdtConfig) => { - jsonOptions.SerializerOptions.TypeInfoResolver = crdtConfig.Value.MakeJsonTypeResolver(); + jsonOptions.SerializerOptions.TypeInfoResolver = crdtConfig.Value.MakeLcmCrdtExternalJsonTypeResolver(); }); services.AddOptions().PostConfigure>( (jsonOptions, crdtConfig) => { - jsonOptions.PayloadSerializerOptions.TypeInfoResolver = crdtConfig.Value.MakeJsonTypeResolver(); + jsonOptions.PayloadSerializerOptions.TypeInfoResolver = crdtConfig.Value.MakeLcmCrdtExternalJsonTypeResolver(); }); return services; } diff --git a/backend/FwLite/MiniLcm.Tests/DiffCollectionTests.cs b/backend/FwLite/MiniLcm.Tests/DiffCollectionTests.cs index d7a23a11a..b711cbc5d 100644 --- a/backend/FwLite/MiniLcm.Tests/DiffCollectionTests.cs +++ b/backend/FwLite/MiniLcm.Tests/DiffCollectionTests.cs @@ -25,8 +25,13 @@ public async Task AddedValues() var (changeCount, diffApi, api) = await Diff([value1], [value2, value1, value3]); changeCount.Should().Be(2); - diffApi.Verify(dApi => dApi.Add(api, value2, 0, It.IsAny>()), Times.Once); - diffApi.Verify(dApi => dApi.Add(api, value3, 2, It.IsAny>()), Times.Once); + + var after1 = Between(after: value1); + diffApi.Verify(dApi => dApi.Add(api, value2, after1), Times.Once); + + var before1 = Between(before: value1); + diffApi.Verify(dApi => dApi.Add(api, value3, before1), Times.Once); + diffApi.Verify(dApi => dApi.Replace(api, value1, value1), Times.Once); diffApi.VerifyNoOtherCalls(); } @@ -54,7 +59,8 @@ public async Task SwappedValues() var (changeCount, diffApi, api) = await Diff([value1, value2], [value2, value1]); changeCount.Should().Be(1); - diffApi.Verify(dApi => dApi.Move(api, value1, 1, It.IsAny>()), Times.Once); + var before2 = Between(before: value2); + diffApi.Verify(dApi => dApi.Move(api, value1, before2), Times.Once); diffApi.Verify(dApi => dApi.Replace(api, value1, value1), Times.Once); diffApi.Verify(dApi => dApi.Replace(api, value2, value2), Times.Once); diffApi.VerifyNoOtherCalls(); @@ -67,37 +73,57 @@ public static IEnumerable CollectionDiffTestCaseData() var _3 = Orderable(3, Guid.NewGuid()); var _4 = Orderable(4, Guid.NewGuid()); yield return [new CollectionDiffTestCase + { + OldValues = [_1, _2, _3], + NewValues = [_3, _2, _1], + ExpectedOperations = [ + new(_1) { From = 0, To = 2, Between = Between(_2) }, + ], + }]; + yield return [new CollectionDiffTestCase { OldValues = [_1, _2, _3, _4], NewValues = [_1, _4, _2, _3], ExpectedOperations = [ - new() { From = 3, To = 1 }, + new(_4) { From = 3, To = 1, Between = Between(_1, _2) }, ], }]; yield return [new CollectionDiffTestCase { OldValues = [_1, _2, _3, _4], NewValues = [_2, _1, _4, _3], - ExpectedOperations = [ // Moving the 2 outsides to middle is represented slightly differently: - new() { From = 0, To = 1 }, - new() { From = 2, To = 3 }, + ExpectedOperations = [ // When only 4, moving the 2 outsides to middle is represented slightly differently: + new(_1) { From = 0, To = 1, Between = Between(_2, _4) }, + new(_3) { From = 2, To = 3, Between = Between(_4, null) }, ], }]; var _5 = Orderable(5, Guid.NewGuid()); var _6 = Orderable(6, Guid.NewGuid()); + yield return [new CollectionDiffTestCase + { + OldValues = [_1, _2, _3, _4, _5, _6], + NewValues = [_2, _3, _1, _6, _4, _5], + ExpectedOperations = [ // When more than 4, the 2 outsides to middle is represented slightly differently: + new(_1) { From = 0, To = 2, Between = Between(_3, _4) }, + new(_6) { From = 5, To = 3, Between = Between(_1, _4) }, + ], + }]; + var _7 = Orderable(7, Guid.NewGuid()); + var _8 = Orderable(8, Guid.NewGuid()); yield return [new CollectionDiffTestCase { OldValues = [_1, _2, _3, _4, _5], - NewValues = [_6, _4, _2, _7], + NewValues = [_6, _8, _4, _2, _7], ExpectedOperations = [ - new() { From = 0 }, // delete _1 - new() { From = 2 }, // delete _3 - new() { From = 4 }, // delete _5 - new() { To = 0 }, // add _6 - new() { From = 1, To = 2 }, // move _2 back - new() { To = 3 }, // add _7 + new(_1) { From = 0 }, + new(_3) { From = 2 }, + new(_5) { From = 4 }, + new(_6) { To = 0, Between = Between(after: _4) }, // (not after: _8, because _8 is not "stable") + new(_8) { To = 1, Between = Between(_6, _4) }, + new(_2) { From = 1, To = 3, Between = Between(before: _4) }, + new(_7) { To = 4, Between = Between(before: _2) }, ], }]; } @@ -106,6 +132,15 @@ public static IEnumerable CollectionDiffTestCaseData() [MemberData(nameof(CollectionDiffTestCaseData))] public async Task DiffTests(CollectionDiffTestCase testCase) { + // Check for silly mistakes + foreach (var operation in testCase.ExpectedOperations) + { + if (operation.From is not null) + testCase.OldValues[operation.From.Value].Should().Be(operation.Value); + if (operation.To is not null) + testCase.NewValues[operation.To.Value].Should().Be(operation.Value); + } + var (changeCount, diffApi, api) = await Diff(testCase.OldValues, testCase.NewValues); using var scope = new AssertionScope(); @@ -121,13 +156,13 @@ public async Task DiffTests(CollectionDiffTestCase testCase) { if (operation.From is not null && operation.To is not null) { + operation.Between.Should().NotBeNull(); var movedValue = testCase.OldValues[operation.From.Value]; diffApi.Verify( dApi => dApi.Move( api, movedValue, - operation.To.Value, - It.IsAny>() + operation.Between ), Times.Once ); @@ -145,13 +180,13 @@ public async Task DiffTests(CollectionDiffTestCase testCase) } else if (operation.To is not null) { + operation.Between.Should().NotBeNull(); var addedValue = testCase.NewValues[operation.To.Value]; diffApi.Verify( dApi => dApi.Add( api, addedValue, - operation.To.Value, - It.IsAny>() + operation.Between ), Times.Once ); @@ -175,15 +210,29 @@ private static IOrderable Orderable(double order, Guid? id = null) return orderable.Object; } + private static BetweenPosition Between(IOrderable? before = null, IOrderable? after = null) + { + return Between(before?.Id, after?.Id); + } + + private static BetweenPosition Between(Guid? before = null, Guid? after = null) + { + return new BetweenPosition + { + Before = before, + After = after + }; + } + private static async Task<(int, Mock, IMiniLcmApi)> Diff(IOrderable[] oldValues, IOrderable[] newValues) { var api = new Mock().Object; var diffApi = new Mock(); - diffApi.Setup(f => f.Add(api, It.IsAny(), It.IsAny(), It.IsAny>())) + diffApi.Setup(f => f.Add(api, It.IsAny(), It.IsAny())) .ReturnsAsync(1); diffApi.Setup(f => f.Remove(api, It.IsAny())) .ReturnsAsync(1); - diffApi.Setup(f => f.Move(api, It.IsAny(), It.IsAny(), It.IsAny>())) + diffApi.Setup(f => f.Move(api, It.IsAny(), It.IsAny())) .ReturnsAsync(1); diffApi.Setup(f => f.Replace(api, It.IsAny(), It.IsAny())) .Returns((IMiniLcmApi api, IOrderable oldValue, IOrderable newValue) => @@ -219,9 +268,9 @@ public class DiffResult public interface DiffApi { - Task Add(IMiniLcmApi api, IOrderable value, int newI, IDictionary stable); + Task Add(IMiniLcmApi api, IOrderable value, BetweenPosition? between); Task Remove(IMiniLcmApi api, IOrderable value); - Task Move(IMiniLcmApi api, IOrderable value, int newI, IDictionary stable); + Task Move(IMiniLcmApi api, IOrderable value, BetweenPosition? between); Task Replace(IMiniLcmApi api, IOrderable oldValue, IOrderable newValue); } @@ -232,8 +281,10 @@ public class CollectionDiffTestCase public required List ExpectedOperations { get; init; } } -public class CollectionDiffOperation +public class CollectionDiffOperation(IOrderable value) { + public IOrderable Value { get; init; } = value; public int? From { get; init; } public int? To { get; init; } + public BetweenPosition? Between { get; init; } } diff --git a/backend/FwLite/MiniLcm/Attributes/MiniLcmInternalAttribute.cs b/backend/FwLite/MiniLcm/Attributes/MiniLcmInternalAttribute.cs new file mode 100644 index 000000000..f6498ba79 --- /dev/null +++ b/backend/FwLite/MiniLcm/Attributes/MiniLcmInternalAttribute.cs @@ -0,0 +1,6 @@ +namespace MiniLcm.Attributes; + +[AttributeUsage(AttributeTargets.Property | AttributeTargets.Field, Inherited = true)] +public class MiniLcmInternalAttribute : Attribute +{ +} diff --git a/backend/FwLite/MiniLcm/IMiniLcmWriteApi.cs b/backend/FwLite/MiniLcm/IMiniLcmWriteApi.cs index 6e3d098fe..1f0611eef 100644 --- a/backend/FwLite/MiniLcm/IMiniLcmWriteApi.cs +++ b/backend/FwLite/MiniLcm/IMiniLcmWriteApi.cs @@ -1,5 +1,6 @@ using System.Linq.Expressions; using MiniLcm.Models; +using MiniLcm.SyncHelpers; using SystemTextJsonPatch; namespace MiniLcm; @@ -46,10 +47,10 @@ Task UpdateWritingSystem(WritingSystemId id, #endregion #region Sense - Task CreateSense(Guid entryId, Sense sense, Guid? afterSenseId = null, Guid? beforeSenseId = null); + Task CreateSense(Guid entryId, Sense sense, BetweenPosition? position = null); Task UpdateSense(Guid entryId, Guid senseId, UpdateObjectInput update); Task UpdateSense(Guid entryId, Sense before, Sense after); - Task MoveSense(Guid entryId, Sense sense, Guid? afterSenseId = null, Guid? beforeSenseId = null); + Task MoveSense(Guid entryId, Sense sense, BetweenPosition position); Task DeleteSense(Guid entryId, Guid senseId); Task AddSemanticDomainToSense(Guid senseId, SemanticDomain semanticDomain); Task RemoveSemanticDomainFromSense(Guid senseId, Guid semanticDomainId); diff --git a/backend/FwLite/MiniLcm/MiniLcmJson.cs b/backend/FwLite/MiniLcm/MiniLcmJson.cs new file mode 100644 index 000000000..7460ec6d1 --- /dev/null +++ b/backend/FwLite/MiniLcm/MiniLcmJson.cs @@ -0,0 +1,22 @@ +using System.Text.Json.Serialization.Metadata; +using MiniLcm.Attributes; + +public static class MiniLcmJson +{ + public static IJsonTypeInfoResolver AddMiniLcmModifiers(this IJsonTypeInfoResolver resolver) + { + resolver = resolver.WithAddedModifier(IgnoreInternalMiniLcmProperties); + return resolver; + } + + private static void IgnoreInternalMiniLcmProperties(JsonTypeInfo typeInfo) + { + foreach (var prop in typeInfo.Properties) + { + if (prop.AttributeProvider?.IsDefined(typeof(MiniLcmInternalAttribute), inherit: true) ?? false) + { + prop.ShouldSerialize = (_, _) => false; + } + } + } +} diff --git a/backend/FwLite/MiniLcm/Models/ComplexFormComponent.cs b/backend/FwLite/MiniLcm/Models/ComplexFormComponent.cs index f42bf06c3..ce4847316 100644 --- a/backend/FwLite/MiniLcm/Models/ComplexFormComponent.cs +++ b/backend/FwLite/MiniLcm/Models/ComplexFormComponent.cs @@ -2,7 +2,7 @@ namespace MiniLcm.Models; -public record ComplexFormComponent : IObjectWithId, IOrderable +public record ComplexFormComponent : IObjectWithId//, IOrderable { public static ComplexFormComponent FromEntries(Entry complexFormEntry, Entry componentEntry, diff --git a/backend/FwLite/MiniLcm/Models/ExampleSentence.cs b/backend/FwLite/MiniLcm/Models/ExampleSentence.cs index e461dd49f..840795632 100644 --- a/backend/FwLite/MiniLcm/Models/ExampleSentence.cs +++ b/backend/FwLite/MiniLcm/Models/ExampleSentence.cs @@ -2,7 +2,7 @@ namespace MiniLcm.Models; -public class ExampleSentence : IObjectWithId, IOrderable +public class ExampleSentence : IObjectWithId//, IOrderable { public virtual Guid Id { get; set; } [JsonIgnore] diff --git a/backend/FwLite/MiniLcm/Models/IOrderable.cs b/backend/FwLite/MiniLcm/Models/IOrderable.cs index c5d1cd223..994e2e31c 100644 --- a/backend/FwLite/MiniLcm/Models/IOrderable.cs +++ b/backend/FwLite/MiniLcm/Models/IOrderable.cs @@ -2,6 +2,6 @@ public interface IOrderable { - public Guid Id { get; } - public double Order { get; set; } + Guid Id { get; } + double Order { get; set; } } diff --git a/backend/FwLite/MiniLcm/Models/Sense.cs b/backend/FwLite/MiniLcm/Models/Sense.cs index d91c70dbd..5fd27a34b 100644 --- a/backend/FwLite/MiniLcm/Models/Sense.cs +++ b/backend/FwLite/MiniLcm/Models/Sense.cs @@ -1,11 +1,11 @@ -using System.Text.Json.Serialization; +using MiniLcm.Attributes; namespace MiniLcm.Models; -public class Sense : IObjectWithId, IOrderable +public partial class Sense : IObjectWithId, IOrderable { public virtual Guid Id { get; set; } - [JsonIgnore] + [MiniLcmInternal] public double Order { get; set; } public DateTimeOffset? DeletedAt { get; set; } public Guid EntryId { get; set; } @@ -37,6 +37,7 @@ public IObjectWithId Copy() { Id = Id, EntryId = EntryId, + Order = Order, DeletedAt = DeletedAt, Definition = Definition.Copy(), Gloss = Gloss.Copy(), diff --git a/backend/FwLite/MiniLcm/SyncHelpers/DiffCollection.cs b/backend/FwLite/MiniLcm/SyncHelpers/DiffCollection.cs index 57371c565..6a5459b44 100644 --- a/backend/FwLite/MiniLcm/SyncHelpers/DiffCollection.cs +++ b/backend/FwLite/MiniLcm/SyncHelpers/DiffCollection.cs @@ -1,4 +1,5 @@ -using System.Text.Json; +using System.ComponentModel; +using System.Text.Json; using System.Text.Json.JsonDiffPatch; using System.Text.Json.Nodes; using MiniLcm.Models; @@ -125,9 +126,9 @@ public static async Task DiffOrderable( IList before, IList after, Func identity, - Func, Task> add, + Func> add, Func> remove, - Func, Task> move, + Func> move, Func> replace) where TId : notnull where T : IOrderable { var positionDiffs = DiffPositions(before, after, identity) @@ -145,7 +146,9 @@ public static async Task DiffOrderable( if (diff.From is not null && diff.To is not null) { var afterEntry = after[diff.To.Value]; - changes += await move(api, afterEntry, diff.To.Value, stableIds); + var between = GetSurroundingStableIds(diff.To.Value, after, stableIds); + changes += await move(api, afterEntry, between); + stableIds.Add(afterEntry.Id); } else if (diff.From is not null) { @@ -154,7 +157,9 @@ public static async Task DiffOrderable( else if (diff.To is not null) { var afterEntry = after[diff.To.Value]; - changes += await add(api, afterEntry, diff.To.Value, stableIds); + var between = GetSurroundingStableIds(diff.To.Value, after, stableIds); + changes += await add(api, afterEntry, between); + stableIds.Add(afterEntry.Id); } } @@ -185,9 +190,9 @@ public static async Task DiffOrderable( IMiniLcmApi api, IList before, IList after, - Func, Task> add, + Func> add, Func> remove, - Func, Task> move, + Func> move, Func> replace) where T : IObjectWithId, IOrderable { return await DiffOrderable(api, before, after, t => (t as IObjectWithId).Id, add, remove, move, replace); @@ -217,6 +222,33 @@ public static async Task Diff( async (api, beforeEntry, afterEntry) => await replace(api, beforeEntry, afterEntry)); } + private static BetweenPosition GetSurroundingStableIds(int i, IList current, IReadOnlyList stable) where T : IOrderable + { + T? beforeEntity = default; + T? afterEntity = default; + for (var j = i - 1; j >= 0; j--) + { + if (stable.Contains(current[j].Id)) + { + beforeEntity = current[j]; + break; + } + } + for (var j = i + 1; j < current.Count; j++) + { + if (stable.Contains(current[j].Id)) + { + afterEntity = current[j]; + break; + } + } + return new BetweenPosition + { + Before = beforeEntity?.Id, + After = afterEntity?.Id + }; + } + private static IEnumerable DiffPositions( IList before, IList after, @@ -270,3 +302,39 @@ private class PositionDiff public int? To { get; init; } } } + +public class BetweenPosition : IEquatable +{ + // The ID of the item before the new item (not the ID of the item that the new item should be before) + public Guid? Before { get; set; } + // The ID of the item after the new item (not the ID of the item that the new item should be after) + public Guid? After { get; set; } + + public override bool Equals(object? obj) + { + return Equals(obj as BetweenPosition); + } + + public bool Equals(BetweenPosition? other) + { + if (other is null) + return false; + + return Before == other.Before && After == other.After; + } + + public override int GetHashCode() + { + return HashCode.Combine(Before, After); + } + + public static bool operator ==(BetweenPosition left, BetweenPosition right) + { + return EqualityComparer.Default.Equals(left, right); + } + + public static bool operator !=(BetweenPosition left, BetweenPosition right) + { + return !(left == right); + } +} diff --git a/backend/FwLite/MiniLcm/SyncHelpers/EntrySync.cs b/backend/FwLite/MiniLcm/SyncHelpers/EntrySync.cs index 9450bfd6c..acff77b06 100644 --- a/backend/FwLite/MiniLcm/SyncHelpers/EntrySync.cs +++ b/backend/FwLite/MiniLcm/SyncHelpers/EntrySync.cs @@ -109,46 +109,20 @@ static async (api, beforeComponent) => ); } - private static (Guid? beforeId, Guid? afterId) GetSurroundingIds(int i, IList current, List stable) where T : class, IOrderable - { - T? beforeEntity = null; - T? afterEntity = null; - for (var j = i - 1; j >= 0; j--) - { - if (stable.Contains(current[j].Id)) - { - beforeEntity = current[j]; - break; - } - } - for (var j = i + 1; j < stable.Count; j++) - { - if (stable.Contains(current[j].Id)) - { - afterEntity = current[j]; - break; - } - } - return (beforeEntity?.Id, afterEntity?.Id); - } private static async Task SensesSync(Guid entryId, IList afterSenses, IList beforeSenses, IMiniLcmApi api) { var prevMin = beforeSenses.Min(s => s.Order); - Func, Task> add = async (api, sense, to, stable) => + Func> add = async (api, sense, between) => { - var (beforeId, afterId) = GetSurroundingIds(to, afterSenses, stable); - await api.CreateSense(entryId, sense, beforeId, afterId); - stable.Add(sense.Id); + await api.CreateSense(entryId, sense, between); return 1; }; - Func, Task> move = async (api, sense, to, stable) => + Func> move = async (api, sense, between) => { - var (beforeId, afterId) = GetSurroundingIds(to, afterSenses, stable); - await api.MoveSense(entryId, sense, beforeId, afterId); - stable.Add(sense.Id); + await api.MoveSense(entryId, sense, between); return 1; }; Func> remove = async (api, sense) => diff --git a/frontend/viewer/src/lib/services/service-provider-signalr.ts b/frontend/viewer/src/lib/services/service-provider-signalr.ts index 08d31e703..049245904 100644 --- a/frontend/viewer/src/lib/services/service-provider-signalr.ts +++ b/frontend/viewer/src/lib/services/service-provider-signalr.ts @@ -1,5 +1,5 @@ /* eslint-disable @typescript-eslint/naming-convention */ -import { getHubProxyFactory, getReceiverRegister } from '../generated-signalr-client/TypedSignalR.Client'; +import {getHubProxyFactory, getReceiverRegister} from '../generated-signalr-client/TypedSignalR.Client'; import { type HubConnection, @@ -7,7 +7,7 @@ import { HubConnectionState, type IHttpConnectionOptions } from '@microsoft/signalr'; -import type { LexboxApiClient, LexboxApiFeatures, LexboxApiMetadata } from './lexbox-api'; +import type {LexboxApiClient, LexboxApiFeatures, LexboxApiMetadata} from './lexbox-api'; import {LexboxService} from './service-provider'; import {onDestroy} from 'svelte'; import {type Readable, type Writable, writable} from 'svelte/store'; @@ -67,6 +67,10 @@ function setupConnection(url: string, options: IHttpConnectionOptions, onError: .withUrl(url, options) .withAutomaticReconnect() .build(); + + if (import.meta.env.DEV) connection.serverTimeoutInMilliseconds = 60_000 * 10; + console.debug('SignalR connection timeout', connection.serverTimeoutInMilliseconds); + onDestroy(() => connection.stop()); connection.onclose((error) => { connected.set(false); From df2a15e3d1ab8faf715715ac62a4d2a2625854f6 Mon Sep 17 00:00:00 2001 From: Tim Haasdyk Date: Fri, 6 Dec 2024 15:08:08 +0100 Subject: [PATCH 04/28] Update/extend entry tests for sense reordering and cleanup --- .../MiniLcmTests/UpdateEntryTests.cs | 2 + .../FwLiteProjectSync.Tests/EntrySyncTests.cs | 24 +++++++- .../FwLiteProjectSync.Tests/ReorderTests.cs | 53 ----------------- backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs | 3 + .../AutoFakerHelpers/OrderableOverride.cs | 21 +++++++ .../MiniLcm.Tests/DiffCollectionTests.cs | 23 +++++--- .../MiniLcm.Tests/UpdateEntryTestsBase.cs | 57 ++++++++++++++++++- .../MiniLcm/Models/ComplexFormComponent.cs | 8 +-- .../FwLite/MiniLcm/Models/ExampleSentence.cs | 8 +-- 9 files changed, 123 insertions(+), 76 deletions(-) delete mode 100644 backend/FwLite/FwLiteProjectSync.Tests/ReorderTests.cs create mode 100644 backend/FwLite/MiniLcm.Tests/AutoFakerHelpers/OrderableOverride.cs diff --git a/backend/FwLite/FwDataMiniLcmBridge.Tests/MiniLcmTests/UpdateEntryTests.cs b/backend/FwLite/FwDataMiniLcmBridge.Tests/MiniLcmTests/UpdateEntryTests.cs index a3efbc833..70d1a88a4 100644 --- a/backend/FwLite/FwDataMiniLcmBridge.Tests/MiniLcmTests/UpdateEntryTests.cs +++ b/backend/FwLite/FwDataMiniLcmBridge.Tests/MiniLcmTests/UpdateEntryTests.cs @@ -9,4 +9,6 @@ protected override Task NewApi() { return Task.FromResult(fixture.NewProjectApi("update-entry-test", "en", "en")); } + + protected override bool ApiUsesImplicitOrdering => true; } diff --git a/backend/FwLite/FwLiteProjectSync.Tests/EntrySyncTests.cs b/backend/FwLite/FwLiteProjectSync.Tests/EntrySyncTests.cs index b2377ae5c..ffd5c0309 100644 --- a/backend/FwLite/FwLiteProjectSync.Tests/EntrySyncTests.cs +++ b/backend/FwLite/FwLiteProjectSync.Tests/EntrySyncTests.cs @@ -1,15 +1,25 @@ using FwLiteProjectSync.Tests.Fixtures; using MiniLcm.Models; using MiniLcm.SyncHelpers; -using MiniLcm.Tests; using MiniLcm.Tests.AutoFakerHelpers; using Soenneker.Utils.AutoBogus; +using Soenneker.Utils.AutoBogus.Config; namespace FwLiteProjectSync.Tests; public class EntrySyncTests : IClassFixture { - private static readonly AutoFaker AutoFaker = new(builder => builder.WithOverride(new MultiStringOverride()).WithOverride(new ObjectWithIdOverride())); + private static readonly AutoFaker AutoFaker = new(new AutoFakerConfig() + { + RepeatCount = 5, + Overrides = + [ + new MultiStringOverride(), + new ObjectWithIdOverride(), + new OrderableOverride(), + ] + }); + public EntrySyncTests(SyncFixture fixture) { _fixture = fixture; @@ -22,6 +32,16 @@ public async Task CanSyncRandomEntries() { var createdEntry = await _fixture.CrdtApi.CreateEntry(await AutoFaker.EntryReadyForCreation(_fixture.CrdtApi)); var after = await AutoFaker.EntryReadyForCreation(_fixture.CrdtApi, entryId: createdEntry.Id); + + // copy some senses over, so moves happen + List createdSenses = [.. createdEntry.Senses]; + for (var i = 0; i < Random.Shared.Next(AutoFaker.Config.RepeatCount); i++) + { + var copy = createdSenses[Random.Shared.Next(createdSenses.Count - 1)]; + createdSenses.Remove(copy); // we don't want duplicates + after.Senses.Insert(Random.Shared.Next(after.Senses.Count), copy); + } + await EntrySync.Sync(after, createdEntry, _fixture.CrdtApi); var actual = await _fixture.CrdtApi.GetEntry(after.Id); actual.Should().NotBeNull(); diff --git a/backend/FwLite/FwLiteProjectSync.Tests/ReorderTests.cs b/backend/FwLite/FwLiteProjectSync.Tests/ReorderTests.cs deleted file mode 100644 index a18af4619..000000000 --- a/backend/FwLite/FwLiteProjectSync.Tests/ReorderTests.cs +++ /dev/null @@ -1,53 +0,0 @@ -using FwLiteProjectSync.Tests.Fixtures; -using MiniLcm.Models; -using MiniLcm.SyncHelpers; -using MiniLcm.Tests.AutoFakerHelpers; -using Soenneker.Utils.AutoBogus; - -namespace FwLiteProjectSync.Tests; - -public class ReorderTests : IClassFixture -{ - private static readonly AutoFaker AutoFaker = new(builder => builder.WithOverride(new MultiStringOverride()).WithOverride(new ObjectWithIdOverride())); - public ReorderTests(SyncFixture fixture) - { - _fixture = fixture; - } - - private readonly SyncFixture _fixture; - - - [Fact] - public async Task CanReorderSensesViaSync() - { - var entry = await _fixture.CrdtApi.CreateEntry(new() { LexemeForm = { { "en", "complexForm1" } } }); - var sense1 = await _fixture.CrdtApi.CreateSense(entry.Id, new Sense() - { - Gloss = { { "en", "1" } }, - }); - var sense2 = await _fixture.CrdtApi.CreateSense(entry.Id, new Sense() - { - Gloss = { { "en", "2" } }, - }); - var sense3 = await _fixture.CrdtApi.CreateSense(entry.Id, new Sense() - { - Gloss = { { "en", "3" } }, - }); - - var before = (await _fixture.CrdtApi.GetEntry(entry.Id))!; - before.Senses.Should().BeEquivalentTo([sense1, sense2, sense3], options => options.WithStrictOrdering()); - before.Senses.Select(s => s.Order).Should().BeEquivalentTo([1, 2, 3]); - - var after = before!.Copy(); - after.Senses = [sense3, sense2, sense1]; - - await EntrySync.Sync(before, after, _fixture.CrdtApi); - - var actual = await _fixture.CrdtApi.GetEntry(after.Id); - actual.Should().NotBeNull(); - for (var i = 0; i < after.Senses.Count; i++) - { - actual!.Senses[i].Should().BeEquivalentTo(after.Senses[i], options => options.Excluding(x => x.Order)); - } - } -} diff --git a/backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs b/backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs index 973c6ae00..dbb6588e7 100644 --- a/backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs +++ b/backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs @@ -363,6 +363,8 @@ await dataModel.AddChanges(ClientId, ..await entry.Senses.ToAsyncEnumerable() .SelectMany((s, i) => { + if (s.Order != default) // we don't anticipate this being necessary, so we'll be strict for now + throw new InvalidOperationException("Order should not be provided when creating a sense"); s.Order = i + 1; return CreateSenseChanges(entry.Id, s); }) @@ -510,6 +512,7 @@ public async Task UpdateSense(Guid entryId, Sense before, Sense after) public async Task MoveSense(Guid entryId, Sense sense, BetweenPosition between) { var order = await dataModel.PickOrder(between); + sense.Order = order; await dataModel.AddChange(ClientId, Changes.SetOrderChange.To(sense.Id, order)); var updatedSense = await dataModel.GetLatest(sense.Id) ?? throw new NullReferenceException(); return updatedSense; diff --git a/backend/FwLite/MiniLcm.Tests/AutoFakerHelpers/OrderableOverride.cs b/backend/FwLite/MiniLcm.Tests/AutoFakerHelpers/OrderableOverride.cs new file mode 100644 index 000000000..34e02e836 --- /dev/null +++ b/backend/FwLite/MiniLcm.Tests/AutoFakerHelpers/OrderableOverride.cs @@ -0,0 +1,21 @@ +using Soenneker.Utils.AutoBogus.Context; +using Soenneker.Utils.AutoBogus.Generators; + +namespace MiniLcm.Tests.AutoFakerHelpers; + +public class OrderableOverride : AutoFakerGeneratorOverride +{ + public override bool CanOverride(AutoFakerContext context) + { + return context.GenerateType.IsAssignableTo(typeof(IOrderable)); + } + + public override void Generate(AutoFakerOverrideContext context) + { + if (context.Instance is IOrderable obj) + { + // Order should never be predefined + obj.Order = 0; + } + } +} diff --git a/backend/FwLite/MiniLcm.Tests/DiffCollectionTests.cs b/backend/FwLite/MiniLcm.Tests/DiffCollectionTests.cs index b711cbc5d..eda795f0f 100644 --- a/backend/FwLite/MiniLcm.Tests/DiffCollectionTests.cs +++ b/backend/FwLite/MiniLcm.Tests/DiffCollectionTests.cs @@ -77,7 +77,8 @@ public static IEnumerable CollectionDiffTestCaseData() OldValues = [_1, _2, _3], NewValues = [_3, _2, _1], ExpectedOperations = [ - new(_1) { From = 0, To = 2, Between = Between(_2) }, + new(_2) { From = 1, To = 1, Between = Between(before: _3) }, + new(_1) { From = 0, To = 2, Between = Between(before: _2) }, ], }]; yield return [new CollectionDiffTestCase @@ -104,7 +105,7 @@ public static IEnumerable CollectionDiffTestCaseData() { OldValues = [_1, _2, _3, _4, _5, _6], NewValues = [_2, _3, _1, _6, _4, _5], - ExpectedOperations = [ // When more than 4, the 2 outsides to middle is represented slightly differently: + ExpectedOperations = [ // When 6+, moving the 2 outsides to middle is represented as such: new(_1) { From = 0, To = 2, Between = Between(_3, _4) }, new(_6) { From = 5, To = 3, Between = Between(_1, _4) }, ], @@ -132,13 +133,19 @@ public static IEnumerable CollectionDiffTestCaseData() [MemberData(nameof(CollectionDiffTestCaseData))] public async Task DiffTests(CollectionDiffTestCase testCase) { - // Check for silly mistakes - foreach (var operation in testCase.ExpectedOperations) + using (new AssertionScope("Check for silly test case mistakes")) { - if (operation.From is not null) - testCase.OldValues[operation.From.Value].Should().Be(operation.Value); - if (operation.To is not null) - testCase.NewValues[operation.To.Value].Should().Be(operation.Value); + foreach (var operation in testCase.ExpectedOperations) + { + if (operation.From is not null) + testCase.OldValues[operation.From.Value].Should().Be(operation.Value); + if (operation.To is not null) + testCase.NewValues[operation.To.Value].Should().Be(operation.Value); + if (operation.Between?.Before is not null) + testCase.NewValues.Should().ContainSingle(v => v.Id == operation.Between.Before); + if (operation.Between?.After is not null) + testCase.NewValues.Should().ContainSingle(v => v.Id == operation.Between.After); + } } var (changeCount, diffApi, api) = await Diff(testCase.OldValues, testCase.NewValues); diff --git a/backend/FwLite/MiniLcm.Tests/UpdateEntryTestsBase.cs b/backend/FwLite/MiniLcm.Tests/UpdateEntryTestsBase.cs index 73e978afd..e9e10ec14 100644 --- a/backend/FwLite/MiniLcm.Tests/UpdateEntryTestsBase.cs +++ b/backend/FwLite/MiniLcm.Tests/UpdateEntryTestsBase.cs @@ -1,10 +1,14 @@ -namespace MiniLcm.Tests; +using System.Globalization; + +namespace MiniLcm.Tests; public abstract class UpdateEntryTestsBase : MiniLcmTestBase { protected readonly Guid Entry1Id = new Guid("a3f5aa5a-578f-4181-8f38-eaaf27f01f1c"); protected readonly Guid Entry2Id = new Guid("2de6c334-58fa-4844-b0fd-0bc2ce4ef835"); + protected virtual bool ApiUsesImplicitOrdering => false; + public override async Task InitializeAsync() { await base.InitializeAsync(); @@ -102,4 +106,55 @@ public async Task UpdateEntry_CanUseSameVersionMultipleTimes() updatedEntry2.Should().BeEquivalentTo(update2, options => options.Excluding(e => e.LexemeForm)); } + + [Theory] + [InlineData("1,2", "1,2,3,4", "1,2,3,4")] // append + [InlineData("1,2", "3,1,2", "0,1,2")] // single prepend + [InlineData("1,2", "4,3,1,2", "0,0.5,1,2")] // multi prepend + [InlineData("1,2,3,4", "4,1,2,3", "0,1,2,3")] // move to back + [InlineData("1,2,3,4", "2,3,4,1", "2,3,4,5")] // move to front + [InlineData("1,2,3,4,5", "1,2,5,3,4", "1,2,2.5,3,4")] // move to middle + [InlineData("1,2,3", "3,2,1", "3,4,5")] // reverse + [InlineData("1,2,3,4", "4,2,3,1", "1,2,3,4")] // swap + public async Task UpdateEntry_CanReorderSenses(string before, string after, string expectedOrderValues) + { + // arrange + var entryId = Guid.NewGuid(); + var senseIds = before.Split(',').Concat(after.Split(',')).Distinct() + .ToDictionary(i => i, _ => Guid.NewGuid()); + var beforeSenses = before.Split(',').Select(i => new Sense() { Id = senseIds[i], EntryId = entryId, Gloss = { { "en", i } } }).ToList(); + var afterSenses = after.Split(',').Select(i => new Sense() { Id = senseIds[i], EntryId = entryId, Gloss = { { "en", i } } }).ToList(); + + var beforeEntry = await Api.CreateEntry(new() + { + Id = entryId, + LexemeForm = { { "en", "order" } }, + Senses = beforeSenses, + }); + + var afterEntry = beforeEntry!.Copy(); + afterEntry.Senses = afterSenses; + + // sanity checks + beforeEntry.Senses.Should().BeEquivalentTo(beforeSenses, options => options.WithStrictOrdering()); + if (!ApiUsesImplicitOrdering) + { + beforeEntry.Senses.Select(s => s.Order).Should() + .BeEquivalentTo(Enumerable.Range(1, beforeSenses.Count), options => options.WithStrictOrdering()); + } + + // act + await Api.UpdateEntry(beforeEntry, afterEntry); + var actual = await Api.GetEntry(afterEntry.Id); + + // assert + actual.Should().NotBeNull(); + actual.Senses.Should().BeEquivalentTo(afterEntry.Senses, options => options.WithStrictOrdering()); + + if (!ApiUsesImplicitOrdering) + { + var actualOrderValues = string.Join(',', actual.Senses.Select(s => s.Order.ToString(CultureInfo.GetCultureInfo("en-US")))); + actualOrderValues.Should().Be(expectedOrderValues); + } + } } diff --git a/backend/FwLite/MiniLcm/Models/ComplexFormComponent.cs b/backend/FwLite/MiniLcm/Models/ComplexFormComponent.cs index ce4847316..cbea10589 100644 --- a/backend/FwLite/MiniLcm/Models/ComplexFormComponent.cs +++ b/backend/FwLite/MiniLcm/Models/ComplexFormComponent.cs @@ -1,8 +1,6 @@ -using System.Text.Json.Serialization; +namespace MiniLcm.Models; -namespace MiniLcm.Models; - -public record ComplexFormComponent : IObjectWithId//, IOrderable +public record ComplexFormComponent : IObjectWithId { public static ComplexFormComponent FromEntries(Entry complexFormEntry, Entry componentEntry, @@ -22,8 +20,6 @@ public static ComplexFormComponent FromEntries(Entry complexFormEntry, } public Guid Id { get; set; } - [JsonIgnore] - public double Order { get; set; } public DateTimeOffset? DeletedAt { get; set; } public virtual required Guid ComplexFormEntryId { get; set; } public string? ComplexFormHeadword { get; set; } diff --git a/backend/FwLite/MiniLcm/Models/ExampleSentence.cs b/backend/FwLite/MiniLcm/Models/ExampleSentence.cs index 840795632..dde441994 100644 --- a/backend/FwLite/MiniLcm/Models/ExampleSentence.cs +++ b/backend/FwLite/MiniLcm/Models/ExampleSentence.cs @@ -1,12 +1,8 @@ -using System.Text.Json.Serialization; +namespace MiniLcm.Models; -namespace MiniLcm.Models; - -public class ExampleSentence : IObjectWithId//, IOrderable +public class ExampleSentence : IObjectWithId { public virtual Guid Id { get; set; } - [JsonIgnore] - public double Order { get; set; } public virtual MultiString Sentence { get; set; } = new(); public virtual MultiString Translation { get; set; } = new(); public virtual string? Reference { get; set; } = null; From 028203f6268ad3905ea926770a348936a261620a Mon Sep 17 00:00:00 2001 From: Tim Haasdyk Date: Fri, 6 Dec 2024 15:58:04 +0100 Subject: [PATCH 05/28] Rename method --- backend/FwLite/LcmCrdt/Json.cs | 2 +- backend/FwLite/MiniLcm/MiniLcmJson.cs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/backend/FwLite/LcmCrdt/Json.cs b/backend/FwLite/LcmCrdt/Json.cs index c0a8df033..e12f0c14b 100644 --- a/backend/FwLite/LcmCrdt/Json.cs +++ b/backend/FwLite/LcmCrdt/Json.cs @@ -115,7 +115,7 @@ public static bool IsIndexerPropertyMethod(MethodInfo method) public static IJsonTypeInfoResolver MakeLcmCrdtExternalJsonTypeResolver(this CrdtConfig config) { var resolver = config.MakeJsonTypeResolver(); - resolver = resolver.AddMiniLcmModifiers(); + resolver = resolver.AddExternalMiniLcmModifiers(); return resolver; } } diff --git a/backend/FwLite/MiniLcm/MiniLcmJson.cs b/backend/FwLite/MiniLcm/MiniLcmJson.cs index 7460ec6d1..174fc417b 100644 --- a/backend/FwLite/MiniLcm/MiniLcmJson.cs +++ b/backend/FwLite/MiniLcm/MiniLcmJson.cs @@ -3,7 +3,7 @@ public static class MiniLcmJson { - public static IJsonTypeInfoResolver AddMiniLcmModifiers(this IJsonTypeInfoResolver resolver) + public static IJsonTypeInfoResolver AddExternalMiniLcmModifiers(this IJsonTypeInfoResolver resolver) { resolver = resolver.WithAddedModifier(IgnoreInternalMiniLcmProperties); return resolver; From 7df8aea5890803c30aea32949ce8feaa92b9bb86 Mon Sep 17 00:00:00 2001 From: Tim Haasdyk Date: Tue, 10 Dec 2024 10:47:03 +0100 Subject: [PATCH 06/28] Don't set order on moved sense, because it shouldn't be necessary to set an internal property for an external caller. --- backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs b/backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs index dbb6588e7..aed03bb45 100644 --- a/backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs +++ b/backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs @@ -512,7 +512,6 @@ public async Task UpdateSense(Guid entryId, Sense before, Sense after) public async Task MoveSense(Guid entryId, Sense sense, BetweenPosition between) { var order = await dataModel.PickOrder(between); - sense.Order = order; await dataModel.AddChange(ClientId, Changes.SetOrderChange.To(sense.Id, order)); var updatedSense = await dataModel.GetLatest(sense.Id) ?? throw new NullReferenceException(); return updatedSense; From 1726b5a8672780e6ad2c83851fda9bebac164952 Mon Sep 17 00:00:00 2001 From: Tim Haasdyk Date: Tue, 10 Dec 2024 10:48:12 +0100 Subject: [PATCH 07/28] Set sense order when creating entries in bulk --- backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs b/backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs index aed03bb45..f6d6e94a4 100644 --- a/backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs +++ b/backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs @@ -336,6 +336,7 @@ private IEnumerable CreateEntryChanges(Entry entry, Dictionary CreateEntryChanges(Entry entry, Dictionary Date: Tue, 10 Dec 2024 10:50:01 +0100 Subject: [PATCH 08/28] Standardize using AllSenses (not SensesOS) and fix moving between subsenses --- .../Api/FwDataMiniLcmApi.cs | 58 ++++++++++++------- .../Api/UpdateProxy/UpdateEntryProxy.cs | 4 +- 2 files changed, 40 insertions(+), 22 deletions(-) diff --git a/backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs b/backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs index 4d8b32d6f..a4db123d5 100644 --- a/backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs +++ b/backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs @@ -1,6 +1,5 @@ using System.Collections.Frozen; using System.Globalization; -using System.Reflection; using System.Text; using FluentValidation; using FwDataMiniLcmBridge.Api.UpdateProxy; @@ -649,7 +648,7 @@ public IAsyncEnumerable SearchEntries(string query, QueryOptions? options var entries = GetEntries(e => e.CitationForm.SearchValue(query) || e.LexemeFormOA.Form.SearchValue(query) || - e.SensesOS.Any(s => s.Gloss.SearchValue(query)), options); + e.AllSenses.Any(s => s.Gloss.SearchValue(query)), options); return entries; } @@ -889,30 +888,42 @@ internal void CreateSense(ILexEntry lexEntry, Sense sense, BetweenPosition? betw internal void InsertSense(ILexEntry lexEntry, ILexSense lexSense, BetweenPosition? between = null) { - if (between?.Before.HasValue is not null || between?.After is not null) + var beforeSenseId = between?.Before; + var afterSenseId = between?.After; + + var beforeSense = beforeSenseId.HasValue ? lexEntry.AllSenses.Find(s => s.Guid == beforeSenseId) : null; + if (beforeSense is not null) { - var beforeSenseId = between?.Before; - var afterSenseId = between?.After; - var senseDict = lexEntry.SensesOS.ToDictionary(s => s.Guid); - if (beforeSenseId.HasValue && senseDict.TryGetValue(beforeSenseId.Value, out var beforeSense)) - { - var insertI = lexEntry.SensesOS.IndexOf(beforeSense) + 1; - lexEntry.SensesOS.Insert(insertI, lexSense); - } - else if (afterSenseId.HasValue && senseDict.TryGetValue(afterSenseId.Value, out var afterSense)) + if (beforeSense.SensesOS.Count > 0) { - var insertI = lexEntry.SensesOS.IndexOf(afterSense); - lexEntry.SensesOS.Insert(insertI, lexSense); + // if the sense has sub-senses, our sense will only come directly after it if it is the first sub-sense + beforeSense.SensesOS.Insert(0, lexSense); } else { - lexEntry.SensesOS.Add(lexSense); + // todo the user might have wanted it to be a subsense of beforeSense + var allSiblings = beforeSense.Owner == lexEntry ? lexEntry.SensesOS + : beforeSense.Owner is ILexSense parentSense ? parentSense.SensesOS + : throw new InvalidOperationException("Sense parent is not a sense or the expected entry"); + var insertI = allSiblings.IndexOf(beforeSense) + 1; + lexEntry.SensesOS.Insert(insertI, lexSense); } + return; } - else + + var afterSense = afterSenseId.HasValue ? lexEntry.AllSenses.Find(s => s.Guid == afterSenseId) : null; + if (afterSense is not null) { - lexEntry.SensesOS.Add(lexSense); + // todo the user might have wanted it to be a subsense of whatever is before afterSense + var allSiblings = afterSense.Owner == lexEntry ? lexEntry.SensesOS + : afterSense.Owner is ILexSense parentSense ? parentSense.SensesOS + : throw new InvalidOperationException("Sense parent is not a sense or the expected entry"); + var insertI = allSiblings.IndexOf(afterSense); + lexEntry.SensesOS.Insert(insertI, lexSense); + return; } + + lexEntry.SensesOS.Add(lexSense); } private void ApplySenseToLexSense(Sense sense, ILexSense lexSense) @@ -990,9 +1001,16 @@ public Task MoveSense(Guid entryId, Sense sense, BetweenPosition between) throw new InvalidOperationException("Entry not found"); if (!SenseRepository.TryGetObject(sense.Id, out var lexSense)) throw new InvalidOperationException("Sense not found"); - // LibLCM treats an insert as a move if the sense is already in the entry - InsertSense(lexEntry, lexSense, between); - return Task.FromResult(sense); + + UndoableUnitOfWorkHelper.DoUsingNewOrCurrentUOW("Move Sense", + "Move Sense back", + Cache.ServiceLocator.ActionHandler, + () => + { + // LibLCM treats an insert as a move if the sense is already in the entry + InsertSense(lexEntry, lexSense, between); + }); + return Task.FromResult(FromLexSense(lexSense)); } public Task AddSemanticDomainToSense(Guid senseId, SemanticDomain semanticDomain) diff --git a/backend/FwLite/FwDataMiniLcmBridge/Api/UpdateProxy/UpdateEntryProxy.cs b/backend/FwLite/FwDataMiniLcmBridge/Api/UpdateProxy/UpdateEntryProxy.cs index d0defe8ad..e63ae481e 100644 --- a/backend/FwLite/FwDataMiniLcmBridge/Api/UpdateProxy/UpdateEntryProxy.cs +++ b/backend/FwLite/FwDataMiniLcmBridge/Api/UpdateProxy/UpdateEntryProxy.cs @@ -51,8 +51,8 @@ public override IList Senses new UpdateListProxy( sense => _lexboxLcmApi.CreateSense(_lcmEntry, sense), sense => _lexboxLcmApi.DeleteSense(Id, sense.Id), - i => new UpdateSenseProxy(_lcmEntry.SensesOS[i], _lexboxLcmApi), - _lcmEntry.SensesOS.Count + i => new UpdateSenseProxy(_lcmEntry.AllSenses[i], _lexboxLcmApi), + _lcmEntry.AllSenses.Count ); set => throw new NotImplementedException(); } From c5f30ca13689c5c7e917eda9b5c8cb6526e55dfd Mon Sep 17 00:00:00 2001 From: Tim Haasdyk Date: Tue, 10 Dec 2024 10:50:22 +0100 Subject: [PATCH 09/28] Cleanup --- backend/FwLite/MiniLcm/SyncHelpers/DiffCollection.cs | 2 +- backend/FwLite/MiniLcm/SyncHelpers/EntrySync.cs | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/backend/FwLite/MiniLcm/SyncHelpers/DiffCollection.cs b/backend/FwLite/MiniLcm/SyncHelpers/DiffCollection.cs index 6a5459b44..2fc8a83e6 100644 --- a/backend/FwLite/MiniLcm/SyncHelpers/DiffCollection.cs +++ b/backend/FwLite/MiniLcm/SyncHelpers/DiffCollection.cs @@ -262,7 +262,7 @@ private static IEnumerable DiffPositions( yield break; // no changes } - foreach (var prop in result!) + foreach (var prop in result) { if (prop.Key == "_t") // diff type { diff --git a/backend/FwLite/MiniLcm/SyncHelpers/EntrySync.cs b/backend/FwLite/MiniLcm/SyncHelpers/EntrySync.cs index acff77b06..1a0097873 100644 --- a/backend/FwLite/MiniLcm/SyncHelpers/EntrySync.cs +++ b/backend/FwLite/MiniLcm/SyncHelpers/EntrySync.cs @@ -114,7 +114,6 @@ private static async Task SensesSync(Guid entryId, IList beforeSenses, IMiniLcmApi api) { - var prevMin = beforeSenses.Min(s => s.Order); Func> add = async (api, sense, between) => { await api.CreateSense(entryId, sense, between); From 970a43c7a07f530b38726b4605a128660f4ee6ce Mon Sep 17 00:00:00 2001 From: Tim Haasdyk Date: Tue, 10 Dec 2024 10:50:34 +0100 Subject: [PATCH 10/28] Fix tests --- .../FwLiteProjectSync.Tests/Sena3SyncTests.cs | 4 ++- .../FwLiteProjectSync.Tests/SyncTests.cs | 25 ++++++++++++++----- .../UpdateDiffTests.cs | 11 +++++--- ...pshotTests.VerifyChangeModels.verified.txt | 4 +++ ...elSnapshotTests.VerifyDbModel.verified.txt | 1 + .../LcmCrdt.Tests/DataModelSnapshotTests.cs | 10 ++++++-- .../LcmCrdt.Tests/EntityCopyMethodTests.cs | 13 ++++++---- backend/FwLite/LcmCrdt/QueryHelpers.cs | 3 +++ .../FwLite/MiniLcm.Tests/MiniLcmTestBase.cs | 16 ++++++++---- .../MiniLcm.Tests/UpdateEntryTestsBase.cs | 3 ++- 10 files changed, 67 insertions(+), 23 deletions(-) diff --git a/backend/FwLite/FwLiteProjectSync.Tests/Sena3SyncTests.cs b/backend/FwLite/FwLiteProjectSync.Tests/Sena3SyncTests.cs index 54e991041..51db068e8 100644 --- a/backend/FwLite/FwLiteProjectSync.Tests/Sena3SyncTests.cs +++ b/backend/FwLite/FwLiteProjectSync.Tests/Sena3SyncTests.cs @@ -51,7 +51,8 @@ private void ShouldAllBeEquivalentTo(Dictionary crdtEntries, Dictio crdtEntry.Should().BeEquivalentTo(fwdataEntry, options => options .For(e => e.Components).Exclude(c => c.Id) - .For(e => e.ComplexForms).Exclude(c => c.Id), + .For(e => e.ComplexForms).Exclude(c => c.Id) + .For(e => e.Senses).Exclude(s => s.Order), $"CRDT entry {crdtEntry.Id} was synced with FwData"); } } @@ -150,6 +151,7 @@ public async Task SyncWithoutImport_CrdtShouldMatchFwdata() [Fact] public async Task SecondSena3SyncDoesNothing() { + await _syncService.Sync(_crdtApi, _fwDataApi); await _syncService.Sync(_crdtApi, _fwDataApi); var secondSync = await _syncService.Sync(_crdtApi, _fwDataApi); secondSync.CrdtChanges.Should().Be(0); diff --git a/backend/FwLite/FwLiteProjectSync.Tests/SyncTests.cs b/backend/FwLite/FwLiteProjectSync.Tests/SyncTests.cs index 49a9fd860..a6f822d99 100644 --- a/backend/FwLite/FwLiteProjectSync.Tests/SyncTests.cs +++ b/backend/FwLite/FwLiteProjectSync.Tests/SyncTests.cs @@ -83,7 +83,9 @@ public async Task FirstSyncJustDoesAnImport() var crdtEntries = await crdtApi.GetAllEntries().ToArrayAsync(); var fwdataEntries = await fwdataApi.GetAllEntries().ToArrayAsync(); crdtEntries.Should().BeEquivalentTo(fwdataEntries, - options => options.For(e => e.Components).Exclude(c => c.Id) + options => options + .For(e => e.Senses).Exclude(s => s.Order) + .For(e => e.Components).Exclude(c => c.Id) .For(e => e.ComplexForms).Exclude(c => c.Id)); } @@ -145,7 +147,9 @@ await crdtApi.CreateEntry(new Entry() var crdtEntries = await crdtApi.GetAllEntries().ToArrayAsync(); var fwdataEntries = await fwdataApi.GetAllEntries().ToArrayAsync(); crdtEntries.Should().BeEquivalentTo(fwdataEntries, - options => options.For(e => e.Components).Exclude(c => c.Id) + options => options + .For(e => e.Senses).Exclude(s => s.Order) + .For(e => e.Components).Exclude(c => c.Id) .For(e => e.ComplexForms).Exclude(c => c.Id)); } @@ -223,7 +227,9 @@ public async Task CreatingAComplexEntryInFwDataSyncsWithoutIssue() var crdtEntries = await crdtApi.GetAllEntries().ToArrayAsync(); var fwdataEntries = await fwdataApi.GetAllEntries().ToArrayAsync(); crdtEntries.Should().BeEquivalentTo(fwdataEntries, - options => options.For(e => e.Components).Exclude(c => c.Id) + options => options + .For(e => e.Senses).Exclude(s => s.Order) + .For(e => e.Components).Exclude(c => c.Id) .For(e => e.ComplexForms).Exclude(c => c.Id)); // Sync again, ensure no problems or changes @@ -305,7 +311,9 @@ await crdtApi.CreateEntry(new Entry() var crdtEntries = await crdtApi.GetAllEntries().ToArrayAsync(); var fwdataEntries = await fwdataApi.GetAllEntries().ToArrayAsync(); crdtEntries.Should().BeEquivalentTo(fwdataEntries, - options => options.For(e => e.Components).Exclude(c => c.Id) + options => options + .For(e => e.Senses).Exclude(s => s.Order) + .For(e => e.Components).Exclude(c => c.Id) .For(e => e.ComplexForms).Exclude(c => c.Id)); } @@ -384,7 +392,9 @@ await crdtApi.CreateEntry(new Entry() var crdtEntries = await crdtApi.GetAllEntries().ToArrayAsync(); var fwdataEntries = await fwdataApi.GetAllEntries().ToArrayAsync(); crdtEntries.Should().BeEquivalentTo(fwdataEntries, - options => options.For(e => e.Components).Exclude(c => c.Id) + options => options + .For(e => e.Senses).Exclude(s => s.Order) + .For(e => e.Components).Exclude(c => c.Id) .For(e => e.ComplexForms).Exclude(c => c.Id)); } @@ -408,6 +418,7 @@ public async Task UpdatingAnEntryInEachProjectSyncsAcrossBoth() var fwdataEntries = await fwdataApi.GetAllEntries().ToArrayAsync(); crdtEntries.Should().BeEquivalentTo(fwdataEntries, options => options + .For(e => e.Senses).Exclude(s => s.Order) .For(e => e.Components).Exclude(c => c.Id) //todo the headword should be changed .For(e => e.Components).Exclude(c => c.ComponentHeadword) @@ -475,7 +486,9 @@ public async Task AddingASenseToAnEntryInEachProjectSyncsAcrossBoth() var crdtEntries = await crdtApi.GetAllEntries().ToArrayAsync(); var fwdataEntries = await fwdataApi.GetAllEntries().ToArrayAsync(); crdtEntries.Should().BeEquivalentTo(fwdataEntries, - options => options.For(e => e.Components).Exclude(c => c.Id) + options => options + .For(e => e.Senses).Exclude(s => s.Order) + .For(e => e.Components).Exclude(c => c.Id) .For(e => e.ComplexForms).Exclude(c => c.Id)); } diff --git a/backend/FwLite/FwLiteProjectSync.Tests/UpdateDiffTests.cs b/backend/FwLite/FwLiteProjectSync.Tests/UpdateDiffTests.cs index 37d4b7026..1f3523981 100644 --- a/backend/FwLite/FwLiteProjectSync.Tests/UpdateDiffTests.cs +++ b/backend/FwLite/FwLiteProjectSync.Tests/UpdateDiffTests.cs @@ -1,5 +1,4 @@ -using FwLiteProjectSync.Tests.Fixtures; -using MiniLcm.Models; +using MiniLcm.Models; using MiniLcm.SyncHelpers; using MiniLcm.Tests.AutoFakerHelpers; using Soenneker.Utils.AutoBogus; @@ -11,7 +10,13 @@ public class UpdateDiffTests { private static readonly AutoFaker AutoFaker = new(new AutoFakerConfig() { - Overrides = [new MultiStringOverride(), new WritingSystemIdOverride()] + RepeatCount = 5, + Overrides = + [ + new MultiStringOverride(), + new WritingSystemIdOverride(), + new OrderableOverride(), + ] }); [Fact] diff --git a/backend/FwLite/LcmCrdt.Tests/DataModelSnapshotTests.VerifyChangeModels.verified.txt b/backend/FwLite/LcmCrdt.Tests/DataModelSnapshotTests.VerifyChangeModels.verified.txt index 685df4f41..8660f761d 100644 --- a/backend/FwLite/LcmCrdt.Tests/DataModelSnapshotTests.VerifyChangeModels.verified.txt +++ b/backend/FwLite/LcmCrdt.Tests/DataModelSnapshotTests.VerifyChangeModels.verified.txt @@ -119,6 +119,10 @@ { DerivedType: CreateComplexFormType, TypeDiscriminator: CreateComplexFormType + }, + { + DerivedType: SetOrderChange, + TypeDiscriminator: SetOrderChange`1 } ], IgnoreUnrecognizedTypeDiscriminators: false, diff --git a/backend/FwLite/LcmCrdt.Tests/DataModelSnapshotTests.VerifyDbModel.verified.txt b/backend/FwLite/LcmCrdt.Tests/DataModelSnapshotTests.VerifyDbModel.verified.txt index b1ae8d40a..40983e166 100644 --- a/backend/FwLite/LcmCrdt.Tests/DataModelSnapshotTests.VerifyDbModel.verified.txt +++ b/backend/FwLite/LcmCrdt.Tests/DataModelSnapshotTests.VerifyDbModel.verified.txt @@ -200,6 +200,7 @@ Gloss (MultiString) Required Annotations: Relational:ColumnType: jsonb + Order (double) Required PartOfSpeech (string) Required PartOfSpeechId (Guid?) SemanticDomains (IList) Required diff --git a/backend/FwLite/LcmCrdt.Tests/DataModelSnapshotTests.cs b/backend/FwLite/LcmCrdt.Tests/DataModelSnapshotTests.cs index 3173e290d..8b6403340 100644 --- a/backend/FwLite/LcmCrdt.Tests/DataModelSnapshotTests.cs +++ b/backend/FwLite/LcmCrdt.Tests/DataModelSnapshotTests.cs @@ -17,9 +17,15 @@ namespace LcmCrdt.Tests; public class DataModelSnapshotTests : IAsyncLifetime { - private static readonly AutoFaker Faker = new AutoFaker(new AutoFakerConfig() + private static readonly AutoFaker Faker = new(new AutoFakerConfig() { - Overrides = [new MultiStringOverride(), new WritingSystemIdOverride()] + RepeatCount = 5, + Overrides = + [ + new MultiStringOverride(), + new WritingSystemIdOverride(), + new OrderableOverride(), + ] }); protected readonly AsyncServiceScope _services; diff --git a/backend/FwLite/LcmCrdt.Tests/EntityCopyMethodTests.cs b/backend/FwLite/LcmCrdt.Tests/EntityCopyMethodTests.cs index 10387a6ef..cc60a393b 100644 --- a/backend/FwLite/LcmCrdt.Tests/EntityCopyMethodTests.cs +++ b/backend/FwLite/LcmCrdt.Tests/EntityCopyMethodTests.cs @@ -1,7 +1,4 @@ -using SIL.Harmony.Entities; -using LcmCrdt.Objects; -using MiniLcm.Tests.AutoFakerHelpers; -using SIL.Harmony; +using MiniLcm.Tests.AutoFakerHelpers; using Soenneker.Utils.AutoBogus; using Soenneker.Utils.AutoBogus.Config; @@ -11,7 +8,13 @@ public class EntityCopyMethodTests { private static readonly AutoFaker AutoFaker = new(new AutoFakerConfig() { - Overrides = [new MultiStringOverride(), new WritingSystemIdOverride()] + RepeatCount = 5, + Overrides = + [ + new MultiStringOverride(), + new WritingSystemIdOverride(), + new OrderableOverride(), + ], }); public static IEnumerable GetEntityTypes() diff --git a/backend/FwLite/LcmCrdt/QueryHelpers.cs b/backend/FwLite/LcmCrdt/QueryHelpers.cs index d9eb62d25..4863f9cd0 100644 --- a/backend/FwLite/LcmCrdt/QueryHelpers.cs +++ b/backend/FwLite/LcmCrdt/QueryHelpers.cs @@ -1,9 +1,12 @@ +using System.Collections; + namespace LcmCrdt; public static class QueryHelpers { public static void DefaultOrder(this Entry entry) { + // ArrayList.Adapter((IList)entry.Senses).Sort(); entry.Senses = entry.Senses.DefaultOrder().ToList(); } public static IEnumerable DefaultOrder(this IEnumerable queryable) where T : IOrderable diff --git a/backend/FwLite/MiniLcm.Tests/MiniLcmTestBase.cs b/backend/FwLite/MiniLcm.Tests/MiniLcmTestBase.cs index 1b5601be2..b71c57a53 100644 --- a/backend/FwLite/MiniLcm.Tests/MiniLcmTestBase.cs +++ b/backend/FwLite/MiniLcm.Tests/MiniLcmTestBase.cs @@ -1,15 +1,21 @@ using MiniLcm.Tests.AutoFakerHelpers; using Soenneker.Utils.AutoBogus; +using Soenneker.Utils.AutoBogus.Config; namespace MiniLcm.Tests; public abstract class MiniLcmTestBase : IAsyncLifetime { - - protected static readonly AutoFaker AutoFaker = new(builder => - builder.WithOverride(new MultiStringOverride(["en"])) - .WithOverride(new ObjectWithIdOverride()) - ); + protected static readonly AutoFaker AutoFaker = new(new AutoFakerConfig() + { + RepeatCount = 5, + Overrides = + [ + new MultiStringOverride(["en"]), + new ObjectWithIdOverride(), + new OrderableOverride(), + ] + }); protected IMiniLcmApi Api = null!; protected abstract Task NewApi(); diff --git a/backend/FwLite/MiniLcm.Tests/UpdateEntryTestsBase.cs b/backend/FwLite/MiniLcm.Tests/UpdateEntryTestsBase.cs index e9e10ec14..09069fe90 100644 --- a/backend/FwLite/MiniLcm.Tests/UpdateEntryTestsBase.cs +++ b/backend/FwLite/MiniLcm.Tests/UpdateEntryTestsBase.cs @@ -149,7 +149,8 @@ public async Task UpdateEntry_CanReorderSenses(string before, string after, stri // assert actual.Should().NotBeNull(); - actual.Senses.Should().BeEquivalentTo(afterEntry.Senses, options => options.WithStrictOrdering()); + actual.Senses.Should().BeEquivalentTo(afterEntry.Senses, + options => options.WithStrictOrdering().Excluding(s => s.Order)); if (!ApiUsesImplicitOrdering) { From b3bf2be967eac7c92334ff31cafade86cb166516 Mon Sep 17 00:00:00 2001 From: Tim Haasdyk Date: Tue, 10 Dec 2024 10:51:39 +0100 Subject: [PATCH 11/28] Revert making Sense partial --- backend/FwLite/MiniLcm/Models/Sense.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/backend/FwLite/MiniLcm/Models/Sense.cs b/backend/FwLite/MiniLcm/Models/Sense.cs index 5fd27a34b..d774a0fae 100644 --- a/backend/FwLite/MiniLcm/Models/Sense.cs +++ b/backend/FwLite/MiniLcm/Models/Sense.cs @@ -2,7 +2,7 @@ namespace MiniLcm.Models; -public partial class Sense : IObjectWithId, IOrderable +public class Sense : IObjectWithId, IOrderable { public virtual Guid Id { get; set; } [MiniLcmInternal] From 5f5a492d002493030ae99742fa029bb1cadb721e Mon Sep 17 00:00:00 2001 From: Tim Haasdyk Date: Tue, 10 Dec 2024 11:48:15 +0100 Subject: [PATCH 12/28] Sort senses in place --- .../Api/UpdateProxy/UpdateEntryProxy.cs | 10 ++------- ...elSnapshotTests.VerifyDbModel.verified.txt | 2 +- backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs | 4 ++-- backend/FwLite/LcmCrdt/QueryHelpers.cs | 22 +++++++++++-------- backend/FwLite/MiniLcm/Models/Entry.cs | 2 +- 5 files changed, 19 insertions(+), 21 deletions(-) diff --git a/backend/FwLite/FwDataMiniLcmBridge/Api/UpdateProxy/UpdateEntryProxy.cs b/backend/FwLite/FwDataMiniLcmBridge/Api/UpdateProxy/UpdateEntryProxy.cs index e63ae481e..548c1d1f5 100644 --- a/backend/FwLite/FwDataMiniLcmBridge/Api/UpdateProxy/UpdateEntryProxy.cs +++ b/backend/FwLite/FwDataMiniLcmBridge/Api/UpdateProxy/UpdateEntryProxy.cs @@ -45,15 +45,9 @@ public override MultiString LiteralMeaning set => throw new NotImplementedException(); } - public override IList Senses + public override List Senses { - get => - new UpdateListProxy( - sense => _lexboxLcmApi.CreateSense(_lcmEntry, sense), - sense => _lexboxLcmApi.DeleteSense(Id, sense.Id), - i => new UpdateSenseProxy(_lcmEntry.AllSenses[i], _lexboxLcmApi), - _lcmEntry.AllSenses.Count - ); + get => throw new NotImplementedException(); set => throw new NotImplementedException(); } diff --git a/backend/FwLite/LcmCrdt.Tests/DataModelSnapshotTests.VerifyDbModel.verified.txt b/backend/FwLite/LcmCrdt.Tests/DataModelSnapshotTests.VerifyDbModel.verified.txt index 40983e166..e33723359 100644 --- a/backend/FwLite/LcmCrdt.Tests/DataModelSnapshotTests.VerifyDbModel.verified.txt +++ b/backend/FwLite/LcmCrdt.Tests/DataModelSnapshotTests.VerifyDbModel.verified.txt @@ -98,7 +98,7 @@ Navigations: ComplexForms (IList) Collection ToDependent ComplexFormComponent Components (IList) Collection ToDependent ComplexFormComponent - Senses (IList) Collection ToDependent Sense + Senses (List) Collection ToDependent Sense Keys: Id PK Foreign keys: diff --git a/backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs b/backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs index f6d6e94a4..89f1e70f5 100644 --- a/backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs +++ b/backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs @@ -278,7 +278,7 @@ private async IAsyncEnumerable GetEntries( var entries = queryable.AsAsyncEnumerable(); await foreach (var entry in entries) { - entry.DefaultOrder(); + entry.ApplySortOrder(); yield return entry; } } @@ -292,7 +292,7 @@ private async IAsyncEnumerable GetEntries( .LoadWith(e => e.Components) .AsQueryable() .SingleOrDefaultAsync(e => e.Id == id); - entry?.DefaultOrder(); + entry?.ApplySortOrder(); return entry; } diff --git a/backend/FwLite/LcmCrdt/QueryHelpers.cs b/backend/FwLite/LcmCrdt/QueryHelpers.cs index 4863f9cd0..b2564ba58 100644 --- a/backend/FwLite/LcmCrdt/QueryHelpers.cs +++ b/backend/FwLite/LcmCrdt/QueryHelpers.cs @@ -1,18 +1,22 @@ -using System.Collections; - namespace LcmCrdt; public static class QueryHelpers { - public static void DefaultOrder(this Entry entry) + public static void ApplySortOrder(this Entry entry) { - // ArrayList.Adapter((IList)entry.Senses).Sort(); - entry.Senses = entry.Senses.DefaultOrder().ToList(); + entry.Senses.ApplySortOrder(); } - public static IEnumerable DefaultOrder(this IEnumerable queryable) where T : IOrderable + + public static void ApplySortOrder(this List items) where T : IOrderable { - return queryable - .OrderBy(e => e.Order) - .ThenBy(e => e.Id); + items.Sort((x, y) => + { + var result = x.Order.CompareTo(y.Order); + if (result == 0) + { + result = x.Id.CompareTo(y.Id); + } + return result; + }); } } diff --git a/backend/FwLite/MiniLcm/Models/Entry.cs b/backend/FwLite/MiniLcm/Models/Entry.cs index cabb8c9bb..8123b0c72 100644 --- a/backend/FwLite/MiniLcm/Models/Entry.cs +++ b/backend/FwLite/MiniLcm/Models/Entry.cs @@ -10,7 +10,7 @@ public record Entry : IObjectWithId public virtual MultiString CitationForm { get; set; } = new(); public virtual MultiString LiteralMeaning { get; set; } = new(); - public virtual IList Senses { get; set; } = []; + public virtual List Senses { get; set; } = []; public virtual MultiString Note { get; set; } = new(); From a87b7ba422984b07960333d31748c3fcd8af9ed0 Mon Sep 17 00:00:00 2001 From: Tim Haasdyk Date: Tue, 10 Dec 2024 12:23:28 +0100 Subject: [PATCH 13/28] Refactor OrderPicker to require siblings --- backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs | 4 +-- backend/FwLite/LcmCrdt/OrderPicker.cs | 32 ++++++++++++++++-------- 2 files changed, 23 insertions(+), 13 deletions(-) diff --git a/backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs b/backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs index 89f1e70f5..766609670 100644 --- a/backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs +++ b/backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs @@ -492,7 +492,7 @@ public async Task CreateSense(Guid entryId, Sense sense, BetweenPosition? if (sense.Order != default) // we don't anticipate this being necessary, so we'll be strict for now throw new InvalidOperationException("Order should not be provided when creating a sense"); - sense.Order = await dataModel.PickOrder(between, Senses.Where(s => s.EntryId == entryId)); + sense.Order = await OrderPicker.PickOrder(Senses.Where(s => s.EntryId == entryId), between); await dataModel.AddChanges(ClientId, await CreateSenseChanges(entryId, sense).ToArrayAsync()); return await dataModel.GetLatest(sense.Id) ?? throw new NullReferenceException(); } @@ -515,7 +515,7 @@ public async Task UpdateSense(Guid entryId, Sense before, Sense after) public async Task MoveSense(Guid entryId, Sense sense, BetweenPosition between) { - var order = await dataModel.PickOrder(between); + var order = await OrderPicker.PickOrder(Senses.Where(s => s.EntryId == entryId), between); await dataModel.AddChange(ClientId, Changes.SetOrderChange.To(sense.Id, order)); var updatedSense = await dataModel.GetLatest(sense.Id) ?? throw new NullReferenceException(); return updatedSense; diff --git a/backend/FwLite/LcmCrdt/OrderPicker.cs b/backend/FwLite/LcmCrdt/OrderPicker.cs index 74286d853..b540c6c68 100644 --- a/backend/FwLite/LcmCrdt/OrderPicker.cs +++ b/backend/FwLite/LcmCrdt/OrderPicker.cs @@ -1,28 +1,38 @@ - +using Microsoft.EntityFrameworkCore; using MiniLcm.SyncHelpers; -using SIL.Harmony; namespace LcmCrdt; public static class OrderPicker { - public static async Task PickOrder(this DataModel dataModel, BetweenPosition? between = null, IQueryable? siblings = null) where T : class, IOrderable + public static async Task PickOrder(IQueryable siblings, BetweenPosition? between = null) where T : class, IOrderable { + // a common case that we can optimize by not querying whole objects + if (between is null or { Before: null, After: null }) + { + var currMaxOrder = siblings.Select(s => s.Order).DefaultIfEmpty().Max(); + return currMaxOrder + 1; + } + + var items = await siblings.ToListAsync(); var beforeId = between?.Before; var afterId = between?.After; - var before = beforeId is not null ? - await dataModel.GetLatest(beforeId.Value) ?? throw new NullReferenceException($"unable to find {typeof(T).Name} with id {beforeId}") - : null; - var after = afterId is not null ? - await dataModel.GetLatest(afterId.Value) ?? throw new NullReferenceException($"unable to find {typeof(T).Name} with id {afterId}") - : null; + var before = beforeId is not null ? items.Find(item => item.Id == beforeId) : null; + var after = afterId is not null ? items.Find(item => item.Id == afterId) : null; + // There are various things we chould check for such as whether + // before.Order + 1 actually puts it after before (i.e. there isn't an item at before.Order + <1) + // but even if that were the case, there's about a 50/50 chance that that's what actually should happen. + // So, overthinking it is probably not valuable. return (before, after) switch { - (null, null) => siblings?.Select(s => s.Order).DefaultIfEmpty().Max(order => order) + 1 ?? 1, + // another user deleted items in the meantime? + (null, null) => siblings.Select(s => s.Order).DefaultIfEmpty().Max() + 1, (_, null) => before.Order + 1, (null, _) => after.Order - 1, - _ => (before.Order + after.Order) / 2, + // If the after item has been shifted before the before item, then between is likely not the actual intent, + // so we revert to only using before + _ => before.Order < after.Order ? (before.Order + after.Order) / 2 : before.Order + 1, }; } } From cebb3d56d2060e59c077b0015e92ee3e74acdfdf Mon Sep 17 00:00:00 2001 From: Tim Haasdyk Date: Tue, 10 Dec 2024 13:13:07 +0100 Subject: [PATCH 14/28] Rename BetweenPosition props and simplify orderable methods --- .../Api/FwDataMiniLcmApi.cs | 32 +++++------ .../FwLiteProjectSync/DryRunMiniLcmApi.cs | 4 +- .../FwLite/LcmCrdt/Changes/SetOrderChange.cs | 33 +---------- backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs | 2 +- backend/FwLite/LcmCrdt/OrderPicker.cs | 24 ++++---- .../MiniLcm.Tests/DiffCollectionTests.cs | 41 +++++++------- .../MiniLcm/SyncHelpers/DiffCollection.cs | 56 +++++++------------ 7 files changed, 73 insertions(+), 119 deletions(-) diff --git a/backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs b/backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs index a4db123d5..6244cbd24 100644 --- a/backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs +++ b/backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs @@ -888,37 +888,37 @@ internal void CreateSense(ILexEntry lexEntry, Sense sense, BetweenPosition? betw internal void InsertSense(ILexEntry lexEntry, ILexSense lexSense, BetweenPosition? between = null) { - var beforeSenseId = between?.Before; - var afterSenseId = between?.After; + var previousSenseId = between?.Previous; + var nextSenseId = between?.Next; - var beforeSense = beforeSenseId.HasValue ? lexEntry.AllSenses.Find(s => s.Guid == beforeSenseId) : null; - if (beforeSense is not null) + var previousSense = previousSenseId.HasValue ? lexEntry.AllSenses.Find(s => s.Guid == previousSenseId) : null; + if (previousSense is not null) { - if (beforeSense.SensesOS.Count > 0) + if (previousSense.SensesOS.Count > 0) { // if the sense has sub-senses, our sense will only come directly after it if it is the first sub-sense - beforeSense.SensesOS.Insert(0, lexSense); + previousSense.SensesOS.Insert(0, lexSense); } else { - // todo the user might have wanted it to be a subsense of beforeSense - var allSiblings = beforeSense.Owner == lexEntry ? lexEntry.SensesOS - : beforeSense.Owner is ILexSense parentSense ? parentSense.SensesOS + // todo the user might have wanted it to be a subsense of previousSense + var allSiblings = previousSense.Owner == lexEntry ? lexEntry.SensesOS + : previousSense.Owner is ILexSense parentSense ? parentSense.SensesOS : throw new InvalidOperationException("Sense parent is not a sense or the expected entry"); - var insertI = allSiblings.IndexOf(beforeSense) + 1; + var insertI = allSiblings.IndexOf(previousSense) + 1; lexEntry.SensesOS.Insert(insertI, lexSense); } return; } - var afterSense = afterSenseId.HasValue ? lexEntry.AllSenses.Find(s => s.Guid == afterSenseId) : null; - if (afterSense is not null) + var nextSense = nextSenseId.HasValue ? lexEntry.AllSenses.Find(s => s.Guid == nextSenseId) : null; + if (nextSense is not null) { - // todo the user might have wanted it to be a subsense of whatever is before afterSense - var allSiblings = afterSense.Owner == lexEntry ? lexEntry.SensesOS - : afterSense.Owner is ILexSense parentSense ? parentSense.SensesOS + // todo the user might have wanted it to be a subsense of whatever is before nextSense + var allSiblings = nextSense.Owner == lexEntry ? lexEntry.SensesOS + : nextSense.Owner is ILexSense parentSense ? parentSense.SensesOS : throw new InvalidOperationException("Sense parent is not a sense or the expected entry"); - var insertI = allSiblings.IndexOf(afterSense); + var insertI = allSiblings.IndexOf(nextSense); lexEntry.SensesOS.Insert(insertI, lexSense); return; } diff --git a/backend/FwLite/FwLiteProjectSync/DryRunMiniLcmApi.cs b/backend/FwLite/FwLiteProjectSync/DryRunMiniLcmApi.cs index 39662a344..626da4aca 100644 --- a/backend/FwLite/FwLiteProjectSync/DryRunMiniLcmApi.cs +++ b/backend/FwLite/FwLiteProjectSync/DryRunMiniLcmApi.cs @@ -198,7 +198,7 @@ public async Task RemoveComplexFormType(Guid entryId, Guid complexFormTypeId) public Task CreateSense(Guid entryId, Sense sense, BetweenPosition? position = null) { - DryRunRecords.Add(new DryRunRecord(nameof(CreateSense), $"Create sense {sense.Gloss} between {position?.Before} and {position?.After}")); + DryRunRecords.Add(new DryRunRecord(nameof(CreateSense), $"Create sense {sense.Gloss} between {position?.Previous} and {position?.Next}")); return Task.FromResult(sense); } @@ -221,7 +221,7 @@ public async Task UpdateSense(Guid entryId, Sense before, Sense after) public Task MoveSense(Guid entryId, Sense sense, BetweenPosition between) { - DryRunRecords.Add(new DryRunRecord(nameof(MoveSense), $"Move sense {sense.Gloss} between {between.Before} and {between.After}")); + DryRunRecords.Add(new DryRunRecord(nameof(MoveSense), $"Move sense {sense.Gloss} between {between.Previous} and {between.Next}")); return Task.FromResult(sense); } diff --git a/backend/FwLite/LcmCrdt/Changes/SetOrderChange.cs b/backend/FwLite/LcmCrdt/Changes/SetOrderChange.cs index 00e976e4b..4e75ca09f 100644 --- a/backend/FwLite/LcmCrdt/Changes/SetOrderChange.cs +++ b/backend/FwLite/LcmCrdt/Changes/SetOrderChange.cs @@ -1,39 +1,12 @@ -using System.Text.Json.Serialization; -using SIL.Harmony.Changes; +using SIL.Harmony.Changes; using SIL.Harmony.Entities; namespace LcmCrdt.Changes; -public class SetOrderChange : EditChange, ISelfNamedType> +public class SetOrderChange(Guid entityId, double order) : EditChange(entityId), ISelfNamedType> where T : class, IOrderable { - public static IChange Between(Guid entityId, T left, T right) - { - return new SetOrderChange(entityId, (left.Order + right.Order) / 2); - } - - public static IChange After(Guid entityId, T previous) - { - return new SetOrderChange(entityId, previous.Order + 1); - } - - public static IChange Before(Guid entityId, T preceding) - { - return new SetOrderChange(entityId, preceding.Order - 1); - } - - public static IChange To(Guid entityId, double order) - { - return new SetOrderChange(entityId, order); - } - - [JsonConstructor] - protected SetOrderChange(Guid entityId, double order) : base(entityId) - { - Order = order; - } - - public double Order { get; init; } + public double Order { get; init; } = order; public override ValueTask ApplyChange(T entity, ChangeContext context) { diff --git a/backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs b/backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs index 766609670..3a572ff7f 100644 --- a/backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs +++ b/backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs @@ -516,7 +516,7 @@ public async Task UpdateSense(Guid entryId, Sense before, Sense after) public async Task MoveSense(Guid entryId, Sense sense, BetweenPosition between) { var order = await OrderPicker.PickOrder(Senses.Where(s => s.EntryId == entryId), between); - await dataModel.AddChange(ClientId, Changes.SetOrderChange.To(sense.Id, order)); + await dataModel.AddChange(ClientId, new Changes.SetOrderChange(sense.Id, order)); var updatedSense = await dataModel.GetLatest(sense.Id) ?? throw new NullReferenceException(); return updatedSense; } diff --git a/backend/FwLite/LcmCrdt/OrderPicker.cs b/backend/FwLite/LcmCrdt/OrderPicker.cs index b540c6c68..dc1118313 100644 --- a/backend/FwLite/LcmCrdt/OrderPicker.cs +++ b/backend/FwLite/LcmCrdt/OrderPicker.cs @@ -8,31 +8,31 @@ public static class OrderPicker public static async Task PickOrder(IQueryable siblings, BetweenPosition? between = null) where T : class, IOrderable { // a common case that we can optimize by not querying whole objects - if (between is null or { Before: null, After: null }) + if (between is null or { Previous: null, Next: null }) { var currMaxOrder = siblings.Select(s => s.Order).DefaultIfEmpty().Max(); return currMaxOrder + 1; } var items = await siblings.ToListAsync(); - var beforeId = between?.Before; - var afterId = between?.After; - var before = beforeId is not null ? items.Find(item => item.Id == beforeId) : null; - var after = afterId is not null ? items.Find(item => item.Id == afterId) : null; + var previousId = between?.Previous; + var nextId = between?.Next; + var previous = previousId is not null ? items.Find(item => item.Id == previousId) : null; + var next = nextId is not null ? items.Find(item => item.Id == nextId) : null; // There are various things we chould check for such as whether - // before.Order + 1 actually puts it after before (i.e. there isn't an item at before.Order + <1) + // previous.Order + 1 actually puts it after previous (i.e. there isn't an item at previous.Order + <1) // but even if that were the case, there's about a 50/50 chance that that's what actually should happen. // So, overthinking it is probably not valuable. - return (before, after) switch + return (previous, next) switch { // another user deleted items in the meantime? (null, null) => siblings.Select(s => s.Order).DefaultIfEmpty().Max() + 1, - (_, null) => before.Order + 1, - (null, _) => after.Order - 1, - // If the after item has been shifted before the before item, then between is likely not the actual intent, - // so we revert to only using before - _ => before.Order < after.Order ? (before.Order + after.Order) / 2 : before.Order + 1, + (_, null) => previous.Order + 1, + (null, _) => next.Order - 1, + // If the next item has been shifted previous the previous item, then between is likely not the actual intent, + // so we revert to only using previous + _ => previous.Order < next.Order ? (previous.Order + next.Order) / 2 : previous.Order + 1, }; } } diff --git a/backend/FwLite/MiniLcm.Tests/DiffCollectionTests.cs b/backend/FwLite/MiniLcm.Tests/DiffCollectionTests.cs index eda795f0f..5f72cbce6 100644 --- a/backend/FwLite/MiniLcm.Tests/DiffCollectionTests.cs +++ b/backend/FwLite/MiniLcm.Tests/DiffCollectionTests.cs @@ -26,11 +26,11 @@ public async Task AddedValues() changeCount.Should().Be(2); - var after1 = Between(after: value1); - diffApi.Verify(dApi => dApi.Add(api, value2, after1), Times.Once); + var x_v1 = Between(next: value1); + diffApi.Verify(dApi => dApi.Add(api, value2, x_v1), Times.Once); - var before1 = Between(before: value1); - diffApi.Verify(dApi => dApi.Add(api, value3, before1), Times.Once); + var v1_x = Between(previous: value1); + diffApi.Verify(dApi => dApi.Add(api, value3, v1_x), Times.Once); diffApi.Verify(dApi => dApi.Replace(api, value1, value1), Times.Once); diffApi.VerifyNoOtherCalls(); @@ -59,8 +59,8 @@ public async Task SwappedValues() var (changeCount, diffApi, api) = await Diff([value1, value2], [value2, value1]); changeCount.Should().Be(1); - var before2 = Between(before: value2); - diffApi.Verify(dApi => dApi.Move(api, value1, before2), Times.Once); + var v2_x = Between(previous: value2); + diffApi.Verify(dApi => dApi.Move(api, value1, v2_x), Times.Once); diffApi.Verify(dApi => dApi.Replace(api, value1, value1), Times.Once); diffApi.Verify(dApi => dApi.Replace(api, value2, value2), Times.Once); diffApi.VerifyNoOtherCalls(); @@ -77,8 +77,8 @@ public static IEnumerable CollectionDiffTestCaseData() OldValues = [_1, _2, _3], NewValues = [_3, _2, _1], ExpectedOperations = [ - new(_2) { From = 1, To = 1, Between = Between(before: _3) }, - new(_1) { From = 0, To = 2, Between = Between(before: _2) }, + new(_2) { From = 1, To = 1, Between = Between(previous: _3) }, + new(_1) { From = 0, To = 2, Between = Between(previous: _2) }, ], }]; yield return [new CollectionDiffTestCase @@ -121,10 +121,10 @@ public static IEnumerable CollectionDiffTestCaseData() new(_1) { From = 0 }, new(_3) { From = 2 }, new(_5) { From = 4 }, - new(_6) { To = 0, Between = Between(after: _4) }, // (not after: _8, because _8 is not "stable") + new(_6) { To = 0, Between = Between(next: _4) }, // (not next: _8, because _8 is not "stable") new(_8) { To = 1, Between = Between(_6, _4) }, - new(_2) { From = 1, To = 3, Between = Between(before: _4) }, - new(_7) { To = 4, Between = Between(before: _2) }, + new(_2) { From = 1, To = 3, Between = Between(previous: _4) }, + new(_7) { To = 4, Between = Between(previous: _2) }, ], }]; } @@ -141,10 +141,10 @@ public async Task DiffTests(CollectionDiffTestCase testCase) testCase.OldValues[operation.From.Value].Should().Be(operation.Value); if (operation.To is not null) testCase.NewValues[operation.To.Value].Should().Be(operation.Value); - if (operation.Between?.Before is not null) - testCase.NewValues.Should().ContainSingle(v => v.Id == operation.Between.Before); - if (operation.Between?.After is not null) - testCase.NewValues.Should().ContainSingle(v => v.Id == operation.Between.After); + if (operation.Between?.Previous is not null) + testCase.NewValues.Should().ContainSingle(v => v.Id == operation.Between.Previous); + if (operation.Between?.Next is not null) + testCase.NewValues.Should().ContainSingle(v => v.Id == operation.Between.Next); } } @@ -217,17 +217,17 @@ private static IOrderable Orderable(double order, Guid? id = null) return orderable.Object; } - private static BetweenPosition Between(IOrderable? before = null, IOrderable? after = null) + private static BetweenPosition Between(IOrderable? previous = null, IOrderable? next = null) { - return Between(before?.Id, after?.Id); + return Between(previous?.Id, next?.Id); } - private static BetweenPosition Between(Guid? before = null, Guid? after = null) + private static BetweenPosition Between(Guid? previous = null, Guid? next = null) { return new BetweenPosition { - Before = before, - After = after + Previous = previous, + Next = next }; } @@ -256,7 +256,6 @@ private static BetweenPosition Between(Guid? before = null, Guid? after = null) }); var changeCount = await DiffCollection.DiffOrderable(api, oldValues, newValues, - (value) => value.Id, diffApi.Object.Add, diffApi.Object.Remove, diffApi.Object.Move, diff --git a/backend/FwLite/MiniLcm/SyncHelpers/DiffCollection.cs b/backend/FwLite/MiniLcm/SyncHelpers/DiffCollection.cs index 2fc8a83e6..289b9b587 100644 --- a/backend/FwLite/MiniLcm/SyncHelpers/DiffCollection.cs +++ b/backend/FwLite/MiniLcm/SyncHelpers/DiffCollection.cs @@ -1,6 +1,4 @@ -using System.ComponentModel; -using System.Text.Json; -using System.Text.Json.JsonDiffPatch; +using System.Text.Json.JsonDiffPatch; using System.Text.Json.Nodes; using MiniLcm.Models; @@ -121,17 +119,16 @@ public static async Task Diff( /// /// /// - public static async Task DiffOrderable( + public static async Task DiffOrderable( IMiniLcmApi api, IList before, IList after, - Func identity, Func> add, Func> remove, Func> move, - Func> replace) where TId : notnull where T : IOrderable + Func> replace) where T : IOrderable { - var positionDiffs = DiffPositions(before, after, identity) + var positionDiffs = DiffPositions(before, after) // Order: Deletes first, then adds and moves from lowest to highest new index // important, because new indexes represent final positions, which might not exist yet in the before list // With this order, callers don't have to account for potential gaps @@ -146,7 +143,7 @@ public static async Task DiffOrderable( if (diff.From is not null && diff.To is not null) { var afterEntry = after[diff.To.Value]; - var between = GetSurroundingStableIds(diff.To.Value, after, stableIds); + var between = GetStableBetween(diff.To.Value, after, stableIds); changes += await move(api, afterEntry, between); stableIds.Add(afterEntry.Id); } @@ -157,16 +154,16 @@ public static async Task DiffOrderable( else if (diff.To is not null) { var afterEntry = after[diff.To.Value]; - var between = GetSurroundingStableIds(diff.To.Value, after, stableIds); + var between = GetStableBetween(diff.To.Value, after, stableIds); changes += await add(api, afterEntry, between); stableIds.Add(afterEntry.Id); } } - var afterEntriesDict = after.ToDictionary(identity); + var afterEntriesDict = after.ToDictionary(entry => entry.Id); foreach (var beforeEntry in before) { - if (afterEntriesDict.TryGetValue(identity(beforeEntry), out var afterEntry)) + if (afterEntriesDict.TryGetValue(beforeEntry.Id, out var afterEntry)) { changes += await replace(api, beforeEntry, afterEntry); } @@ -186,18 +183,6 @@ public static async Task Diff( return await Diff(api, before, after, t => t.Id, add, remove, replace); } - public static async Task DiffOrderable( - IMiniLcmApi api, - IList before, - IList after, - Func> add, - Func> remove, - Func> move, - Func> replace) where T : IObjectWithId, IOrderable - { - return await DiffOrderable(api, before, after, t => (t as IObjectWithId).Id, add, remove, move, replace); - } - public static async Task Diff( IMiniLcmApi api, IList before, @@ -222,7 +207,7 @@ public static async Task Diff( async (api, beforeEntry, afterEntry) => await replace(api, beforeEntry, afterEntry)); } - private static BetweenPosition GetSurroundingStableIds(int i, IList current, IReadOnlyList stable) where T : IOrderable + private static BetweenPosition GetStableBetween(int i, IList current, IReadOnlyList stable) where T : IOrderable { T? beforeEntity = default; T? afterEntity = default; @@ -244,18 +229,17 @@ private static BetweenPosition GetSurroundingStableIds(int i, IList curren } return new BetweenPosition { - Before = beforeEntity?.Id, - After = afterEntity?.Id + Previous = beforeEntity?.Id, + Next = afterEntity?.Id }; } - private static IEnumerable DiffPositions( + private static IEnumerable DiffPositions( IList before, - IList after, - Func identity) + IList after) where T : IOrderable { - var beforeJson = new JsonArray(before.Select(item => JsonValue.Create(identity(item))).ToArray()); - var afterJson = new JsonArray(after.Select(item => JsonValue.Create(identity(item))).ToArray()); + var beforeJson = new JsonArray(before.Select(item => JsonValue.Create(item.Id)).ToArray()); + var afterJson = new JsonArray(after.Select(item => JsonValue.Create(item.Id)).ToArray()); if (JsonDiffPatcher.Diff(beforeJson, afterJson) is not JsonObject result) { @@ -305,10 +289,8 @@ private class PositionDiff public class BetweenPosition : IEquatable { - // The ID of the item before the new item (not the ID of the item that the new item should be before) - public Guid? Before { get; set; } - // The ID of the item after the new item (not the ID of the item that the new item should be after) - public Guid? After { get; set; } + public Guid? Previous { get; set; } + public Guid? Next { get; set; } public override bool Equals(object? obj) { @@ -320,12 +302,12 @@ public bool Equals(BetweenPosition? other) if (other is null) return false; - return Before == other.Before && After == other.After; + return Previous == other.Previous && Next == other.Next; } public override int GetHashCode() { - return HashCode.Combine(Before, After); + return HashCode.Combine(Previous, Next); } public static bool operator ==(BetweenPosition left, BetweenPosition right) From b82d56b6cac3b3fc712620db072cc6bccfaab19e Mon Sep 17 00:00:00 2001 From: Tim Haasdyk Date: Tue, 10 Dec 2024 14:45:11 +0100 Subject: [PATCH 15/28] Refactor collection diffing to use class/object for add, delete, move & replace --- .../MiniLcm.Tests/DiffCollectionTests.cs | 64 +++--- .../SyncHelpers/ComplexFormTypeSync.cs | 35 +-- .../MiniLcm/SyncHelpers/DiffCollection.cs | 157 +++++--------- .../FwLite/MiniLcm/SyncHelpers/EntrySync.cs | 200 +++++++++++------- .../SyncHelpers/ExampleSentenceSync.cs | 39 ++-- .../MiniLcm/SyncHelpers/PartOfSpeechSync.cs | 35 +-- .../MiniLcm/SyncHelpers/SemanticDomainSync.cs | 35 +-- .../FwLite/MiniLcm/SyncHelpers/SenseSync.cs | 38 ++-- .../MiniLcm/SyncHelpers/WritingSystemSync.cs | 45 ++-- 9 files changed, 335 insertions(+), 313 deletions(-) diff --git a/backend/FwLite/MiniLcm.Tests/DiffCollectionTests.cs b/backend/FwLite/MiniLcm.Tests/DiffCollectionTests.cs index 5f72cbce6..a24887c96 100644 --- a/backend/FwLite/MiniLcm.Tests/DiffCollectionTests.cs +++ b/backend/FwLite/MiniLcm.Tests/DiffCollectionTests.cs @@ -11,7 +11,7 @@ public async Task MatchingCollections_NoChangesAreGenerated() { var value1 = Orderable(1, Guid.NewGuid()); var value2 = Orderable(2, Guid.NewGuid()); - var (changeCount, _, _) = await Diff([value1, value2], [value1, value2]); + var (changeCount, _) = await Diff([value1, value2], [value1, value2]); changeCount.Should().Be(0); } @@ -22,17 +22,17 @@ public async Task AddedValues() var value1 = Orderable(1, Guid.NewGuid()); var value2 = Orderable(2, Guid.NewGuid()); var value3 = Orderable(3, Guid.NewGuid()); - var (changeCount, diffApi, api) = await Diff([value1], [value2, value1, value3]); + var (changeCount, diffApi) = await Diff([value1], [value2, value1, value3]); changeCount.Should().Be(2); var x_v1 = Between(next: value1); - diffApi.Verify(dApi => dApi.Add(api, value2, x_v1), Times.Once); + diffApi.Verify(dApi => dApi.Add(value2, x_v1), Times.Once); var v1_x = Between(previous: value1); - diffApi.Verify(dApi => dApi.Add(api, value3, v1_x), Times.Once); + diffApi.Verify(dApi => dApi.Add(value3, v1_x), Times.Once); - diffApi.Verify(dApi => dApi.Replace(api, value1, value1), Times.Once); + diffApi.Verify(dApi => dApi.Replace(value1, value1), Times.Once); diffApi.VerifyNoOtherCalls(); } @@ -42,12 +42,12 @@ public async Task RemovedValues() var value1 = Orderable(1, Guid.NewGuid()); var value2 = Orderable(2, Guid.NewGuid()); var value3 = Orderable(3, Guid.NewGuid()); - var (changeCount, diffApi, api) = await Diff([value2, value1, value3], [value1]); + var (changeCount, diffApi) = await Diff([value2, value1, value3], [value1]); changeCount.Should().Be(2); - diffApi.Verify(dApi => dApi.Remove(api, value2), Times.Once); - diffApi.Verify(dApi => dApi.Remove(api, value3), Times.Once); - diffApi.Verify(dApi => dApi.Replace(api, value1, value1), Times.Once); + diffApi.Verify(dApi => dApi.Remove(value2), Times.Once); + diffApi.Verify(dApi => dApi.Remove(value3), Times.Once); + diffApi.Verify(dApi => dApi.Replace(value1, value1), Times.Once); diffApi.VerifyNoOtherCalls(); } @@ -56,13 +56,13 @@ public async Task SwappedValues() { var value1 = Orderable(1, Guid.NewGuid()); var value2 = Orderable(2, Guid.NewGuid()); - var (changeCount, diffApi, api) = await Diff([value1, value2], [value2, value1]); + var (changeCount, diffApi) = await Diff([value1, value2], [value2, value1]); changeCount.Should().Be(1); var v2_x = Between(previous: value2); - diffApi.Verify(dApi => dApi.Move(api, value1, v2_x), Times.Once); - diffApi.Verify(dApi => dApi.Replace(api, value1, value1), Times.Once); - diffApi.Verify(dApi => dApi.Replace(api, value2, value2), Times.Once); + diffApi.Verify(dApi => dApi.Move(value1, v2_x), Times.Once); + diffApi.Verify(dApi => dApi.Replace(value1, value1), Times.Once); + diffApi.Verify(dApi => dApi.Replace(value2, value2), Times.Once); diffApi.VerifyNoOtherCalls(); } @@ -148,14 +148,14 @@ public async Task DiffTests(CollectionDiffTestCase testCase) } } - var (changeCount, diffApi, api) = await Diff(testCase.OldValues, testCase.NewValues); + var (changeCount, diffApi) = await Diff(testCase.OldValues, testCase.NewValues); using var scope = new AssertionScope(); changeCount.Should().Be(testCase.ExpectedOperations.Count); var expectedReplaceCount = testCase.OldValues.Select(v => v.Id).Intersect(testCase.NewValues.Select(v => v.Id)).Count(); - diffApi.Verify(dApi => dApi.Replace(api, It.IsAny(), It.IsAny()), Times.Exactly(expectedReplaceCount)); + diffApi.Verify(dApi => dApi.Replace(It.IsAny(), It.IsAny()), Times.Exactly(expectedReplaceCount)); foreach (var operation in testCase.ExpectedOperations) { @@ -167,7 +167,6 @@ public async Task DiffTests(CollectionDiffTestCase testCase) var movedValue = testCase.OldValues[operation.From.Value]; diffApi.Verify( dApi => dApi.Move( - api, movedValue, operation.Between ), @@ -179,7 +178,6 @@ public async Task DiffTests(CollectionDiffTestCase testCase) var removedValue = testCase.OldValues[operation.From.Value]; diffApi.Verify( dApi => dApi.Remove( - api, removedValue ), Times.Once @@ -191,7 +189,6 @@ public async Task DiffTests(CollectionDiffTestCase testCase) var addedValue = testCase.NewValues[operation.To.Value]; diffApi.Verify( dApi => dApi.Add( - api, addedValue, operation.Between ), @@ -231,17 +228,16 @@ private static BetweenPosition Between(Guid? previous = null, Guid? next = null) }; } - private static async Task<(int, Mock, IMiniLcmApi)> Diff(IOrderable[] oldValues, IOrderable[] newValues) + private static async Task<(int, Mock>)> Diff(IOrderable[] oldValues, IOrderable[] newValues) { - var api = new Mock().Object; - var diffApi = new Mock(); - diffApi.Setup(f => f.Add(api, It.IsAny(), It.IsAny())) + var diffApi = new Mock>(); + diffApi.Setup(f => f.Add(It.IsAny(), It.IsAny())) .ReturnsAsync(1); - diffApi.Setup(f => f.Remove(api, It.IsAny())) + diffApi.Setup(f => f.Remove(It.IsAny())) .ReturnsAsync(1); - diffApi.Setup(f => f.Move(api, It.IsAny(), It.IsAny())) + diffApi.Setup(f => f.Move(It.IsAny(), It.IsAny())) .ReturnsAsync(1); - diffApi.Setup(f => f.Replace(api, It.IsAny(), It.IsAny())) + diffApi.Setup(f => f.Replace(It.IsAny(), It.IsAny())) .Returns((IMiniLcmApi api, IOrderable oldValue, IOrderable newValue) => { try @@ -255,31 +251,19 @@ private static BetweenPosition Between(Guid? previous = null, Guid? next = null) } }); - var changeCount = await DiffCollection.DiffOrderable(api, oldValues, newValues, - diffApi.Object.Add, - diffApi.Object.Remove, - diffApi.Object.Move, - diffApi.Object.Replace); + var changeCount = await DiffCollection.DiffOrderable(oldValues, newValues, diffApi.Object); - return (changeCount, diffApi, api); + return (changeCount, diffApi); } } public class DiffResult { public required int ChangeCount { get; init; } - public required Mock DiffApi { get; init; } + public required Mock> DiffApi { get; init; } public required IMiniLcmApi Api { get; init; } } -public interface DiffApi -{ - Task Add(IMiniLcmApi api, IOrderable value, BetweenPosition? between); - Task Remove(IMiniLcmApi api, IOrderable value); - Task Move(IMiniLcmApi api, IOrderable value, BetweenPosition? between); - Task Replace(IMiniLcmApi api, IOrderable oldValue, IOrderable newValue); -} - public class CollectionDiffTestCase { public required IOrderable[] OldValues { get; init; } diff --git a/backend/FwLite/MiniLcm/SyncHelpers/ComplexFormTypeSync.cs b/backend/FwLite/MiniLcm/SyncHelpers/ComplexFormTypeSync.cs index e7ae771cf..05a92442f 100644 --- a/backend/FwLite/MiniLcm/SyncHelpers/ComplexFormTypeSync.cs +++ b/backend/FwLite/MiniLcm/SyncHelpers/ComplexFormTypeSync.cs @@ -9,21 +9,10 @@ public static async Task Sync(ComplexFormType[] afterComplexFormTypes, ComplexFormType[] beforeComplexFormTypes, IMiniLcmApi api) { - return await DiffCollection.Diff(api, + return await DiffCollection.Diff( beforeComplexFormTypes, afterComplexFormTypes, - complexFormType => complexFormType.Id, - static async (api, afterComplexFormType) => - { - await api.CreateComplexFormType(afterComplexFormType); - return 1; - }, - static async (api, beforeComplexFormType) => - { - await api.DeleteComplexFormType(beforeComplexFormType.Id); - return 1; - }, - static (api, beforeComplexFormType, afterComplexFormType) => Sync(beforeComplexFormType, afterComplexFormType, api)); + new ComplexFormTypesDiffApi(api)); } public static async Task Sync(ComplexFormType before, @@ -44,4 +33,24 @@ public static async Task Sync(ComplexFormType before, if (patchDocument.Operations.Count == 0) return null; return new UpdateObjectInput(patchDocument); } + + private class ComplexFormTypesDiffApi(IMiniLcmApi api) : ObjectWithIdCollectionDiffApi + { + public override async Task Add(ComplexFormType afterComplexFormType) + { + await api.CreateComplexFormType(afterComplexFormType); + return 1; + } + + public override async Task Remove(ComplexFormType beforeComplexFormType) + { + await api.DeleteComplexFormType(beforeComplexFormType.Id); + return 1; + } + + public override Task Replace(ComplexFormType beforeComplexFormType, ComplexFormType afterComplexFormType) + { + return Sync(beforeComplexFormType, afterComplexFormType, api); + } + } } diff --git a/backend/FwLite/MiniLcm/SyncHelpers/DiffCollection.cs b/backend/FwLite/MiniLcm/SyncHelpers/DiffCollection.cs index 289b9b587..e75dfe768 100644 --- a/backend/FwLite/MiniLcm/SyncHelpers/DiffCollection.cs +++ b/backend/FwLite/MiniLcm/SyncHelpers/DiffCollection.cs @@ -4,129 +4,111 @@ namespace MiniLcm.SyncHelpers; +public abstract class CollectionDiffApi where TId : notnull +{ + public abstract Task Add(T value); + public virtual async Task<(int, T)> AddAndGet(T value) + { + var changes = await Add(value); + return (changes, value); + } + public abstract Task Remove(T value); + public abstract Task Replace(T before, T after); + public abstract TId GetId(T value); +} + +public abstract class ObjectWithIdCollectionDiffApi : CollectionDiffApi where T : IObjectWithId +{ + public override Guid GetId(T value) + { + return value.Id; + } +} + +public interface OrderableCollectionDiffApi where T : IOrderable +{ + Task Add(T value, BetweenPosition between); + Task Remove(T value); + Task Move(T value, BetweenPosition between); + Task Replace(T before, T after); +} + public static class DiffCollection { /// /// Diffs a list, for new items calls add, it will then call update for the item returned from the add, using that as the before item for the replace call /// - /// - /// - /// - /// - /// api, value, return value to be used as the before item for the replace call - /// - /// api, before, after is the parameter order - /// - /// - /// public static async Task DiffAddThenUpdate( - IMiniLcmApi api, IList before, IList after, - Func identity, - Func> add, - Func> remove, - Func> replace) where TId : notnull + CollectionDiffApi diffApi) where TId : notnull { var changes = 0; - var afterEntriesDict = after.ToDictionary(identity); + var afterEntriesDict = after.ToDictionary(diffApi.GetId); foreach (var beforeEntry in before) { - if (afterEntriesDict.TryGetValue(identity(beforeEntry), out var afterEntry)) + if (afterEntriesDict.TryGetValue(diffApi.GetId(beforeEntry), out var afterEntry)) { - changes += await replace(api, beforeEntry, afterEntry); + changes += await diffApi.Replace(beforeEntry, afterEntry); } else { - changes += await remove(api, beforeEntry); + changes += await diffApi.Remove(beforeEntry); } - afterEntriesDict.Remove(identity(beforeEntry)); + afterEntriesDict.Remove(diffApi.GetId(beforeEntry)); } var postAddUpdates = new List<(T created, T after)>(afterEntriesDict.Values.Count); foreach (var value in afterEntriesDict.Values) { - changes++; - postAddUpdates.Add((await add(api, value), value)); + var (change, created) = await diffApi.AddAndGet(value); + changes += change; + postAddUpdates.Add((created, value)); } - foreach ((T createdItem, T afterItem) in postAddUpdates) + foreach ((var createdItem, var afterItem) in postAddUpdates) { //todo this may do a lot more work than it needs to, eg sense will be created during add, but they will be checked again here when we know they didn't change - await replace(api, createdItem, afterItem); + await diffApi.Replace(createdItem, afterItem); } return changes; } - /// - /// - /// - /// - /// - /// - /// - /// - /// - /// api, before, after is the parameter order - /// - /// - /// public static async Task Diff( - IMiniLcmApi api, IList before, IList after, - Func identity, - Func> add, - Func> remove, - Func> replace) where TId : notnull + CollectionDiffApi diffApi) where TId : notnull { var changes = 0; - var afterEntriesDict = after.ToDictionary(identity); + var afterEntriesDict = after.ToDictionary(diffApi.GetId); foreach (var beforeEntry in before) { - if (afterEntriesDict.TryGetValue(identity(beforeEntry), out var afterEntry)) + if (afterEntriesDict.TryGetValue(diffApi.GetId(beforeEntry), out var afterEntry)) { - changes += await replace(api, beforeEntry, afterEntry); + changes += await diffApi.Replace(beforeEntry, afterEntry); } else { - changes += await remove(api, beforeEntry); + changes += await diffApi.Remove(beforeEntry); } - afterEntriesDict.Remove(identity(beforeEntry)); + afterEntriesDict.Remove(diffApi.GetId(beforeEntry)); } foreach (var value in afterEntriesDict.Values) { - changes += await add(api, value); + changes += await diffApi.Add(value); } return changes; } - /// - /// - /// - /// - /// - /// - /// - /// - /// - /// api, before, after is the parameter order - /// - /// - /// public static async Task DiffOrderable( - IMiniLcmApi api, IList before, IList after, - Func> add, - Func> remove, - Func> move, - Func> replace) where T : IOrderable + OrderableCollectionDiffApi diffApi) where T : IOrderable { var positionDiffs = DiffPositions(before, after) // Order: Deletes first, then adds and moves from lowest to highest new index @@ -144,18 +126,18 @@ public static async Task DiffOrderable( { var afterEntry = after[diff.To.Value]; var between = GetStableBetween(diff.To.Value, after, stableIds); - changes += await move(api, afterEntry, between); + changes += await diffApi.Move(afterEntry, between); stableIds.Add(afterEntry.Id); } else if (diff.From is not null) { - changes += await remove(api, before[diff.From.Value]); + changes += await diffApi.Remove(before[diff.From.Value]); } else if (diff.To is not null) { var afterEntry = after[diff.To.Value]; var between = GetStableBetween(diff.To.Value, after, stableIds); - changes += await add(api, afterEntry, between); + changes += await diffApi.Add(afterEntry, between); stableIds.Add(afterEntry.Id); } } @@ -165,48 +147,13 @@ public static async Task DiffOrderable( { if (afterEntriesDict.TryGetValue(beforeEntry.Id, out var afterEntry)) { - changes += await replace(api, beforeEntry, afterEntry); + changes += await diffApi.Replace(beforeEntry, afterEntry); } } return changes; } - public static async Task Diff( - IMiniLcmApi api, - IList before, - IList after, - Func> add, - Func> remove, - Func> replace) where T : IObjectWithId - { - return await Diff(api, before, after, t => t.Id, add, remove, replace); - } - - public static async Task Diff( - IMiniLcmApi api, - IList before, - IList after, - Func add, - Func remove, - Func> replace) where T : IObjectWithId - { - return await Diff(api, - before, - after, - async (api, entry) => - { - await add(api, entry); - return 1; - }, - async (api, entry) => - { - await remove(api, entry); - return 1; - }, - async (api, beforeEntry, afterEntry) => await replace(api, beforeEntry, afterEntry)); - } - private static BetweenPosition GetStableBetween(int i, IList current, IReadOnlyList stable) where T : IOrderable { T? beforeEntity = default; diff --git a/backend/FwLite/MiniLcm/SyncHelpers/EntrySync.cs b/backend/FwLite/MiniLcm/SyncHelpers/EntrySync.cs index 1a0097873..aba125deb 100644 --- a/backend/FwLite/MiniLcm/SyncHelpers/EntrySync.cs +++ b/backend/FwLite/MiniLcm/SyncHelpers/EntrySync.cs @@ -10,21 +10,7 @@ public static async Task Sync(Entry[] afterEntries, Entry[] beforeEntries, IMiniLcmApi api) { - Func> add = static async (api, afterEntry) => - { - //create each entry without components. - //After each entry is created, then replace will be called to create those components - var entryWithoutEntryRefs = afterEntry.WithoutEntryRefs(); - await api.CreateEntry(entryWithoutEntryRefs); - return entryWithoutEntryRefs; - }; - Func> remove = static async (api, beforeEntry) => - { - await api.DeleteEntry(beforeEntry.Id); - return 1; - }; - Func> replace = static async (api, beforeEntry, afterEntry) => await Sync(afterEntry, beforeEntry, api); - return await DiffCollection.DiffAddThenUpdate(api, beforeEntries, afterEntries, entry => entry.Id, add, remove, replace); + return await DiffCollection.DiffAddThenUpdate(beforeEntries, afterEntries, new EntriesDiffApi(api)); } public static async Task Sync(Entry afterEntry, Entry beforeEntry, IMiniLcmApi api) @@ -51,61 +37,18 @@ private static async Task Sync(Guid entryId, IList beforeComplexFormTypes, IMiniLcmApi api) { - return await DiffCollection.Diff(api, + return await DiffCollection.Diff( beforeComplexFormTypes, afterComplexFormTypes, - complexFormType => complexFormType.Id, - async (api, afterComplexFormType) => - { - await api.AddComplexFormType(entryId, afterComplexFormType.Id); - return 1; - }, - async (api, beforeComplexFormType) => - { - await api.RemoveComplexFormType(entryId, beforeComplexFormType.Id); - return 1; - }, - //do nothing, complex form types are not editable, ignore any changes to them here - static (api, beforeComplexFormType, afterComplexFormType) => Task.FromResult(0)); + new ComplexFormTypesDiffApi(api, entryId)); } private static async Task Sync(IList afterComponents, IList beforeComponents, IMiniLcmApi api) { - return await DiffCollection.Diff(api, + return await DiffCollection.Diff( beforeComponents, afterComponents, - //we can't use the ID as there's none defined by Fw so it won't work as a sync key - component => (component.ComplexFormEntryId, component.ComponentEntryId, component.ComponentSenseId), - static async (api, afterComponent) => - { - //change id, since we're not using the id as the key for this collection - //the id may be the same, which is not what we want here - afterComponent.Id = Guid.NewGuid(); - try - { - await api.CreateComplexFormComponent(afterComponent); - } - catch (NotFoundException) - { - //this can happen if the entry was deleted, so we can just ignore it - } - return 1; - }, - static async (api, beforeComponent) => - { - await api.DeleteComplexFormComponent(beforeComponent); - return 1; - }, - static (api, beforeComponent, afterComponent) => - { - if (beforeComponent.ComplexFormEntryId == afterComponent.ComplexFormEntryId && - beforeComponent.ComponentEntryId == afterComponent.ComponentEntryId && - beforeComponent.ComponentSenseId == afterComponent.ComponentSenseId) - { - return Task.FromResult(0); - } - throw new InvalidOperationException($"changing complex form components is not supported, they should just be deleted and recreated"); - } + new ComplexFormComponentsDiffApi(api) ); } @@ -114,23 +57,7 @@ private static async Task SensesSync(Guid entryId, IList beforeSenses, IMiniLcmApi api) { - Func> add = async (api, sense, between) => - { - await api.CreateSense(entryId, sense, between); - return 1; - }; - Func> move = async (api, sense, between) => - { - await api.MoveSense(entryId, sense, between); - return 1; - }; - Func> remove = async (api, sense) => - { - await api.DeleteSense(entryId, sense.Id); - return 1; - }; - Func> replace = async (api, beforeSense, afterSense) => await SenseSync.Sync(entryId, afterSense, beforeSense, api); - return await DiffCollection.DiffOrderable(api, beforeSenses, afterSenses, add, remove, move, replace); + return await DiffCollection.DiffOrderable(beforeSenses, afterSenses, new SensesDiffApi(api, entryId)); } public static UpdateObjectInput? EntryDiffToUpdate(Entry beforeEntry, Entry afterEntry) @@ -143,4 +70,119 @@ private static async Task SensesSync(Guid entryId, if (patchDocument.Operations.Count == 0) return null; return new UpdateObjectInput(patchDocument); } + + private class EntriesDiffApi(IMiniLcmApi api) : ObjectWithIdCollectionDiffApi + { + public override async Task<(int, Entry)> AddAndGet(Entry afterEntry) + { + //create each entry without components. + //After each entry is created, then replace will be called to create those components + var entryWithoutEntryRefs = afterEntry.WithoutEntryRefs(); + var changes = await Add(entryWithoutEntryRefs); + return (changes, entryWithoutEntryRefs); + } + + public override async Task Add(Entry afterEntry) + { + await api.CreateEntry(afterEntry); + return 1; + } + + public override async Task Remove(Entry entry) + { + await api.DeleteEntry(entry.Id); + return 1; + } + + public override Task Replace(Entry before, Entry after) + { + return Sync(after, before, api); + } + } + + private class ComplexFormTypesDiffApi(IMiniLcmApi api, Guid entryId) : ObjectWithIdCollectionDiffApi + { + public override async Task Add(ComplexFormType afterComplexFormType) + { + await api.AddComplexFormType(entryId, afterComplexFormType.Id); + return 1; + } + + public override async Task Remove(ComplexFormType beforeComplexFormType) + { + await api.RemoveComplexFormType(entryId, beforeComplexFormType.Id); + return 1; + } + + public override Task Replace(ComplexFormType before, ComplexFormType after) + { + return Task.FromResult(0); + } + } + + private class ComplexFormComponentsDiffApi(IMiniLcmApi api) : CollectionDiffApi + { + public override (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.ComplexFormEntryId, component.ComponentEntryId, component.ComponentSenseId); + } + + public override async Task Add(ComplexFormComponent afterComplexFormType) + { + afterComplexFormType.Id = Guid.NewGuid(); + try + { + await api.CreateComplexFormComponent(afterComplexFormType); + } + catch (NotFoundException) + { + //this can happen if the entry was deleted, so we can just ignore it + } + return 1; + } + + public override async Task Remove(ComplexFormComponent beforeComplexFormType) + { + await api.DeleteComplexFormComponent(beforeComplexFormType); + return 1; + } + + public override Task Replace(ComplexFormComponent beforeComponent, ComplexFormComponent afterComponent) + { + if (beforeComponent.ComplexFormEntryId == afterComponent.ComplexFormEntryId && + beforeComponent.ComponentEntryId == afterComponent.ComponentEntryId && + beforeComponent.ComponentSenseId == afterComponent.ComponentSenseId) + { + return Task.FromResult(0); + } + throw new InvalidOperationException($"changing complex form components is not supported, they should just be deleted and recreated"); + } + } + + private class SensesDiffApi(IMiniLcmApi api, Guid entryId) : OrderableCollectionDiffApi + { + public async Task Add(Sense sense, BetweenPosition between) + { + await api.CreateSense(entryId, sense, between); + return 1; + } + + public async Task Move(Sense sense, BetweenPosition between) + { + await api.MoveSense(entryId, sense, between); + return 1; + } + + public async Task Remove(Sense sense) + { + await api.DeleteSense(entryId, sense.Id); + return 1; + } + + public Task Replace(Sense before, Sense after) + { + return SenseSync.Sync(entryId, after, before, api); + } + } } diff --git a/backend/FwLite/MiniLcm/SyncHelpers/ExampleSentenceSync.cs b/backend/FwLite/MiniLcm/SyncHelpers/ExampleSentenceSync.cs index ef2e6cf1a..f685e9650 100644 --- a/backend/FwLite/MiniLcm/SyncHelpers/ExampleSentenceSync.cs +++ b/backend/FwLite/MiniLcm/SyncHelpers/ExampleSentenceSync.cs @@ -11,25 +11,10 @@ public static async Task Sync(Guid entryId, IList beforeExampleSentences, IMiniLcmApi api) { - Func> add = async (api, afterExampleSentence) => - { - await api.CreateExampleSentence(entryId, senseId, afterExampleSentence); - return 1; - }; - Func> remove = async (api, beforeExampleSentence) => - { - await api.DeleteExampleSentence(entryId, senseId, beforeExampleSentence.Id); - return 1; - }; - Func> replace = - (api, beforeExampleSentence, afterExampleSentence) => - Sync(entryId, senseId, afterExampleSentence, beforeExampleSentence, api); - return await DiffCollection.Diff(api, + return await DiffCollection.Diff( beforeExampleSentences, afterExampleSentences, - add, - remove, - replace); + new ExampleSentencesDiffApi(api, entryId, senseId)); } public static async Task Sync(Guid entryId, @@ -64,4 +49,24 @@ public static async Task Sync(Guid entryId, if (patchDocument.Operations.Count == 0) return null; return new UpdateObjectInput(patchDocument); } + + private class ExampleSentencesDiffApi(IMiniLcmApi api, Guid entryId, Guid senseId) : ObjectWithIdCollectionDiffApi + { + public override async Task Add(ExampleSentence afterExampleSentence) + { + await api.CreateExampleSentence(entryId, senseId, afterExampleSentence); + return 1; + } + + public override async Task Remove(ExampleSentence beforeExampleSentence) + { + await api.DeleteExampleSentence(entryId, senseId, beforeExampleSentence.Id); + return 1; + } + + public override Task Replace(ExampleSentence beforeExampleSentence, ExampleSentence afterExampleSentence) + { + return Sync(entryId, senseId, afterExampleSentence, beforeExampleSentence, api); + } + } } diff --git a/backend/FwLite/MiniLcm/SyncHelpers/PartOfSpeechSync.cs b/backend/FwLite/MiniLcm/SyncHelpers/PartOfSpeechSync.cs index 9a7cf0bba..be6eff5a1 100644 --- a/backend/FwLite/MiniLcm/SyncHelpers/PartOfSpeechSync.cs +++ b/backend/FwLite/MiniLcm/SyncHelpers/PartOfSpeechSync.cs @@ -9,21 +9,10 @@ public static async Task Sync(PartOfSpeech[] currentPartsOfSpeech, PartOfSpeech[] previousPartsOfSpeech, IMiniLcmApi api) { - return await DiffCollection.Diff(api, + return await DiffCollection.Diff( previousPartsOfSpeech, currentPartsOfSpeech, - pos => pos.Id, - async (api, currentPos) => - { - await api.CreatePartOfSpeech(currentPos); - return 1; - }, - async (api, previousPos) => - { - await api.DeletePartOfSpeech(previousPos.Id); - return 1; - }, - (api, previousPos, currentPos) => Sync(previousPos, currentPos, api)); + new PartsOfSpeechDiffApi(api)); } public static async Task Sync(PartOfSpeech before, @@ -48,4 +37,24 @@ public static async Task Sync(PartOfSpeech before, if (patchDocument.Operations.Count == 0) return null; return new UpdateObjectInput(patchDocument); } + + private class PartsOfSpeechDiffApi(IMiniLcmApi api) : ObjectWithIdCollectionDiffApi + { + public override async Task Add(PartOfSpeech currentPos) + { + await api.CreatePartOfSpeech(currentPos); + return 1; + } + + public override async Task Remove(PartOfSpeech previousPos) + { + await api.DeletePartOfSpeech(previousPos.Id); + return 1; + } + + public override Task Replace(PartOfSpeech previousPos, PartOfSpeech currentPos) + { + return Sync(previousPos, currentPos, api); + } + } } diff --git a/backend/FwLite/MiniLcm/SyncHelpers/SemanticDomainSync.cs b/backend/FwLite/MiniLcm/SyncHelpers/SemanticDomainSync.cs index 3892e8ed1..b6bc793fc 100644 --- a/backend/FwLite/MiniLcm/SyncHelpers/SemanticDomainSync.cs +++ b/backend/FwLite/MiniLcm/SyncHelpers/SemanticDomainSync.cs @@ -9,21 +9,10 @@ public static async Task Sync(SemanticDomain[] currentSemanticDomains, SemanticDomain[] previousSemanticDomains, IMiniLcmApi api) { - return await DiffCollection.Diff(api, + return await DiffCollection.Diff( previousSemanticDomains, currentSemanticDomains, - pos => pos.Id, - async (api, currentPos) => - { - await api.CreateSemanticDomain(currentPos); - return 1; - }, - async (api, previousPos) => - { - await api.DeleteSemanticDomain(previousPos.Id); - return 1; - }, - (api, previousSemdom, currentSemdom) => Sync(previousSemdom, currentSemdom, api)); + new SemanticDomainsDiffApi(api)); } public static async Task Sync(SemanticDomain before, @@ -48,4 +37,24 @@ public static async Task Sync(SemanticDomain before, if (patchDocument.Operations.Count == 0) return null; return new UpdateObjectInput(patchDocument); } + + private class SemanticDomainsDiffApi(IMiniLcmApi api) : ObjectWithIdCollectionDiffApi + { + public override async Task Add(SemanticDomain currentSemDom) + { + await api.CreateSemanticDomain(currentSemDom); + return 1; + } + + public override async Task Remove(SemanticDomain previousSemDom) + { + await api.DeleteSemanticDomain(previousSemDom.Id); + return 1; + } + + public override Task Replace(SemanticDomain previousSemDom, SemanticDomain currentSemDom) + { + return Sync(previousSemDom, currentSemDom, api); + } + } } diff --git a/backend/FwLite/MiniLcm/SyncHelpers/SenseSync.cs b/backend/FwLite/MiniLcm/SyncHelpers/SenseSync.cs index 181107105..6c028cd56 100644 --- a/backend/FwLite/MiniLcm/SyncHelpers/SenseSync.cs +++ b/backend/FwLite/MiniLcm/SyncHelpers/SenseSync.cs @@ -17,24 +17,10 @@ public static async Task Sync(Guid entryId, afterSense.ExampleSentences, beforeSense.ExampleSentences, api); - changes += await DiffCollection.Diff(api, + changes += await DiffCollection.Diff( beforeSense.SemanticDomains, afterSense.SemanticDomains, - async (api, domain) => - { - await api.AddSemanticDomainToSense(beforeSense.Id, domain); - return 1; - }, - async (api, beforeDomain) => - { - await api.RemoveSemanticDomainFromSense(beforeSense.Id, beforeDomain.Id); - return 1; - }, - (_, beforeDomain, afterDomain) => - { - //do nothing, semantic domains are not editable here - return Task.FromResult(0); - }); + new SenseSemanticDomainsDiffApi(api, beforeSense.Id)); return changes + (updateObjectInput is null ? 0 : 1); } @@ -59,4 +45,24 @@ public static async Task Sync(Guid entryId, if (patchDocument.Operations.Count == 0) return null; return new UpdateObjectInput(patchDocument); } + + private class SenseSemanticDomainsDiffApi(IMiniLcmApi api, Guid senseId) : ObjectWithIdCollectionDiffApi + { + public override async Task Add(SemanticDomain domain) + { + await api.AddSemanticDomainToSense(senseId, domain); + return 1; + } + + public override async Task Remove(SemanticDomain beforeDomain) + { + await api.RemoveSemanticDomainFromSense(senseId, beforeDomain.Id); + return 1; + } + + public override Task Replace(SemanticDomain previousSemDom, SemanticDomain currentSemDom) + { + return Task.FromResult(0); + } + } } diff --git a/backend/FwLite/MiniLcm/SyncHelpers/WritingSystemSync.cs b/backend/FwLite/MiniLcm/SyncHelpers/WritingSystemSync.cs index 142ae97e7..548edd0e9 100644 --- a/backend/FwLite/MiniLcm/SyncHelpers/WritingSystemSync.cs +++ b/backend/FwLite/MiniLcm/SyncHelpers/WritingSystemSync.cs @@ -16,25 +16,10 @@ public static async Task Sync(WritingSystem[] currentWritingSystems, WritingSystem[] previousWritingSystems, IMiniLcmApi api) { - return await DiffCollection.Diff(api, + return await DiffCollection.Diff( previousWritingSystems, currentWritingSystems, - ws => (ws.WsId, ws.Type), - async (api, currentWs) => - { - await api.CreateWritingSystem(currentWs.Type, currentWs); - return 1; - }, - async (api, previousWs) => - { - // await api.DeleteWritingSystem(previousWs.Id); // Deleting writing systems is dangerous as it causes cascading data deletion. Needs careful thought. - // TODO: should we throw an exception? - return 0; - }, - async (api, previousWs, currentWs) => - { - return await Sync(currentWs, previousWs, api); - }); + new WritingSystemsDiffApi(api)); } public static async Task Sync(WritingSystem afterWs, WritingSystem beforeWs, IMiniLcmApi api) @@ -65,4 +50,30 @@ public static async Task Sync(WritingSystem afterWs, WritingSystem beforeWs if (patchDocument.Operations.Count == 0) return null; return new UpdateObjectInput(patchDocument); } + + private class WritingSystemsDiffApi(IMiniLcmApi api) : CollectionDiffApi + { + public override (WritingSystemId, WritingSystemType) GetId(WritingSystem value) + { + return (value.WsId, value.Type); + } + + public override async Task Add(WritingSystem currentWs) + { + await api.CreateWritingSystem(currentWs.Type, currentWs); + return 1; + } + + public override Task Remove(WritingSystem beforeDomain) + { + // await api.DeleteWritingSystem(previousWs.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); + } + + public override Task Replace(WritingSystem previousWs, WritingSystem currentWs) + { + return Sync(currentWs, previousWs, api); + } + } } From 4eda0bdcef660446b2700538972ccadca6b1f7d5 Mon Sep 17 00:00:00 2001 From: Kevin Hahn Date: Wed, 11 Dec 2024 11:10:37 +0700 Subject: [PATCH 16/28] add back a comment which got lost --- backend/FwLite/MiniLcm/SyncHelpers/EntrySync.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/backend/FwLite/MiniLcm/SyncHelpers/EntrySync.cs b/backend/FwLite/MiniLcm/SyncHelpers/EntrySync.cs index aba125deb..e48165bbd 100644 --- a/backend/FwLite/MiniLcm/SyncHelpers/EntrySync.cs +++ b/backend/FwLite/MiniLcm/SyncHelpers/EntrySync.cs @@ -130,6 +130,8 @@ public override (Guid, Guid, Guid?) GetId(ComplexFormComponent component) public override async Task Add(ComplexFormComponent afterComplexFormType) { + //change id, since we're not using the id as the key for this collection + //the id may be the same, which is not what we want here afterComplexFormType.Id = Guid.NewGuid(); try { From 4085b1057a098f53518d8c62709c6aae8866847d Mon Sep 17 00:00:00 2001 From: Kevin Hahn Date: Wed, 11 Dec 2024 13:00:48 +0700 Subject: [PATCH 17/28] simplify generating random moves for entry sync tests --- .../FwLiteProjectSync.Tests/EntrySyncTests.cs | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/backend/FwLite/FwLiteProjectSync.Tests/EntrySyncTests.cs b/backend/FwLite/FwLiteProjectSync.Tests/EntrySyncTests.cs index ffd5c0309..8078d54cc 100644 --- a/backend/FwLite/FwLiteProjectSync.Tests/EntrySyncTests.cs +++ b/backend/FwLite/FwLiteProjectSync.Tests/EntrySyncTests.cs @@ -33,14 +33,11 @@ public async Task CanSyncRandomEntries() var createdEntry = await _fixture.CrdtApi.CreateEntry(await AutoFaker.EntryReadyForCreation(_fixture.CrdtApi)); var after = await AutoFaker.EntryReadyForCreation(_fixture.CrdtApi, entryId: createdEntry.Id); - // copy some senses over, so moves happen - List createdSenses = [.. createdEntry.Senses]; - for (var i = 0; i < Random.Shared.Next(AutoFaker.Config.RepeatCount); i++) - { - var copy = createdSenses[Random.Shared.Next(createdSenses.Count - 1)]; - createdSenses.Remove(copy); // we don't want duplicates - after.Senses.Insert(Random.Shared.Next(after.Senses.Count), copy); - } + after.Senses = AutoFaker.Faker.Random.Shuffle([ + // copy some senses over, so moves happen + ..AutoFaker.Faker.Random.ListItems(createdEntry.Senses), + ..(after.Senses) + ]).ToList(); await EntrySync.Sync(after, createdEntry, _fixture.CrdtApi); var actual = await _fixture.CrdtApi.GetEntry(after.Id); From 01106a445f014f9094084ae0e115610990f8908b Mon Sep 17 00:00:00 2001 From: Kevin Hahn Date: Wed, 11 Dec 2024 13:12:52 +0700 Subject: [PATCH 18/28] specify a namespace for the MiniLcmJson class --- backend/FwLite/MiniLcm/MiniLcmJson.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/backend/FwLite/MiniLcm/MiniLcmJson.cs b/backend/FwLite/MiniLcm/MiniLcmJson.cs index 174fc417b..64fe276be 100644 --- a/backend/FwLite/MiniLcm/MiniLcmJson.cs +++ b/backend/FwLite/MiniLcm/MiniLcmJson.cs @@ -1,6 +1,8 @@ using System.Text.Json.Serialization.Metadata; using MiniLcm.Attributes; +namespace MiniLcm; + public static class MiniLcmJson { public static IJsonTypeInfoResolver AddExternalMiniLcmModifiers(this IJsonTypeInfoResolver resolver) From 5ebbced39bd6e2676c8fc246d34ac625f27e434a Mon Sep 17 00:00:00 2001 From: Kevin Hahn Date: Wed, 11 Dec 2024 13:16:42 +0700 Subject: [PATCH 19/28] add comment about what the MiniLcmInternalAttribute.cs is used for --- backend/FwLite/MiniLcm/Attributes/MiniLcmInternalAttribute.cs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/backend/FwLite/MiniLcm/Attributes/MiniLcmInternalAttribute.cs b/backend/FwLite/MiniLcm/Attributes/MiniLcmInternalAttribute.cs index f6498ba79..e577ac38f 100644 --- a/backend/FwLite/MiniLcm/Attributes/MiniLcmInternalAttribute.cs +++ b/backend/FwLite/MiniLcm/Attributes/MiniLcmInternalAttribute.cs @@ -1,5 +1,7 @@ namespace MiniLcm.Attributes; - +/// +/// For now this just controls whether the property should be serialized or not when leaving dotnet +/// [AttributeUsage(AttributeTargets.Property | AttributeTargets.Field, Inherited = true)] public class MiniLcmInternalAttribute : Attribute { From 490ac239749df7c4eea6c55acffcbce40fad7beb Mon Sep 17 00:00:00 2001 From: Tim Haasdyk Date: Thu, 12 Dec 2024 15:34:35 +0100 Subject: [PATCH 20/28] Fix failing test --- backend/FwLite/FwLiteProjectSync.Tests/EntrySyncTests.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/backend/FwLite/FwLiteProjectSync.Tests/EntrySyncTests.cs b/backend/FwLite/FwLiteProjectSync.Tests/EntrySyncTests.cs index 8078d54cc..83dfa17a3 100644 --- a/backend/FwLite/FwLiteProjectSync.Tests/EntrySyncTests.cs +++ b/backend/FwLite/FwLiteProjectSync.Tests/EntrySyncTests.cs @@ -42,7 +42,8 @@ public async Task CanSyncRandomEntries() await EntrySync.Sync(after, createdEntry, _fixture.CrdtApi); var actual = await _fixture.CrdtApi.GetEntry(after.Id); actual.Should().NotBeNull(); - actual.Should().BeEquivalentTo(after, options => options); + actual.Should().BeEquivalentTo(after, options => options + .For(e => e.Senses).Exclude(s => s.Order)); } [Fact] From 7c9450b33e3bce72175e54d9c97c442254b18d48 Mon Sep 17 00:00:00 2001 From: Tim Haasdyk Date: Thu, 12 Dec 2024 15:37:39 +0100 Subject: [PATCH 21/28] Only pass senseId to MoveSense --- backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs | 6 +++--- backend/FwLite/FwLiteProjectSync/DryRunMiniLcmApi.cs | 6 +++--- backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs | 6 ++---- backend/FwLite/MiniLcm/IMiniLcmWriteApi.cs | 2 +- backend/FwLite/MiniLcm/SyncHelpers/EntrySync.cs | 2 +- 5 files changed, 10 insertions(+), 12 deletions(-) diff --git a/backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs b/backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs index 6244cbd24..0a60d09a9 100644 --- a/backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs +++ b/backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs @@ -995,11 +995,11 @@ await Cache.DoUsingNewOrCurrentUOW("Update Sense", return await GetSense(entryId, after.Id) ?? throw new NullReferenceException("unable to find sense with id " + after.Id); } - public Task MoveSense(Guid entryId, Sense sense, BetweenPosition between) + public Task MoveSense(Guid entryId, Guid senseId, BetweenPosition between) { if (!EntriesRepository.TryGetObject(entryId, out var lexEntry)) throw new InvalidOperationException("Entry not found"); - if (!SenseRepository.TryGetObject(sense.Id, out var lexSense)) + if (!SenseRepository.TryGetObject(senseId, out var lexSense)) throw new InvalidOperationException("Sense not found"); UndoableUnitOfWorkHelper.DoUsingNewOrCurrentUOW("Move Sense", @@ -1010,7 +1010,7 @@ public Task MoveSense(Guid entryId, Sense sense, BetweenPosition between) // LibLCM treats an insert as a move if the sense is already in the entry InsertSense(lexEntry, lexSense, between); }); - return Task.FromResult(FromLexSense(lexSense)); + return Task.CompletedTask; } public Task AddSemanticDomainToSense(Guid senseId, SemanticDomain semanticDomain) diff --git a/backend/FwLite/FwLiteProjectSync/DryRunMiniLcmApi.cs b/backend/FwLite/FwLiteProjectSync/DryRunMiniLcmApi.cs index 626da4aca..bdda169aa 100644 --- a/backend/FwLite/FwLiteProjectSync/DryRunMiniLcmApi.cs +++ b/backend/FwLite/FwLiteProjectSync/DryRunMiniLcmApi.cs @@ -219,10 +219,10 @@ public async Task UpdateSense(Guid entryId, Sense before, Sense after) return await GetSense(entryId, after.Id) ?? throw new NullReferenceException($"unable to find sense with id {after.Id}"); } - public Task MoveSense(Guid entryId, Sense sense, BetweenPosition between) + public Task MoveSense(Guid entryId, Guid senseId, BetweenPosition between) { - DryRunRecords.Add(new DryRunRecord(nameof(MoveSense), $"Move sense {sense.Gloss} between {between.Previous} and {between.Next}")); - return Task.FromResult(sense); + DryRunRecords.Add(new DryRunRecord(nameof(MoveSense), $"Move sense {senseId} between {between.Previous} and {between.Next}")); + return Task.CompletedTask; } public Task DeleteSense(Guid entryId, Guid senseId) diff --git a/backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs b/backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs index 3a572ff7f..c13168e3c 100644 --- a/backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs +++ b/backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs @@ -513,12 +513,10 @@ public async Task UpdateSense(Guid entryId, Sense before, Sense after) return await GetSense(entryId, after.Id) ?? throw new NullReferenceException("unable to find sense with id " + after.Id); } - public async Task MoveSense(Guid entryId, Sense sense, BetweenPosition between) + public async Task MoveSense(Guid entryId, Guid senseId, BetweenPosition between) { var order = await OrderPicker.PickOrder(Senses.Where(s => s.EntryId == entryId), between); - await dataModel.AddChange(ClientId, new Changes.SetOrderChange(sense.Id, order)); - var updatedSense = await dataModel.GetLatest(sense.Id) ?? throw new NullReferenceException(); - return updatedSense; + await dataModel.AddChange(ClientId, new Changes.SetOrderChange(senseId, order)); } public async Task DeleteSense(Guid entryId, Guid senseId) diff --git a/backend/FwLite/MiniLcm/IMiniLcmWriteApi.cs b/backend/FwLite/MiniLcm/IMiniLcmWriteApi.cs index 1f0611eef..c9e74442b 100644 --- a/backend/FwLite/MiniLcm/IMiniLcmWriteApi.cs +++ b/backend/FwLite/MiniLcm/IMiniLcmWriteApi.cs @@ -50,7 +50,7 @@ Task UpdateWritingSystem(WritingSystemId id, Task CreateSense(Guid entryId, Sense sense, BetweenPosition? position = null); Task UpdateSense(Guid entryId, Guid senseId, UpdateObjectInput update); Task UpdateSense(Guid entryId, Sense before, Sense after); - Task MoveSense(Guid entryId, Sense sense, BetweenPosition position); + Task MoveSense(Guid entryId, Guid senseId, BetweenPosition position); Task DeleteSense(Guid entryId, Guid senseId); Task AddSemanticDomainToSense(Guid senseId, SemanticDomain semanticDomain); Task RemoveSemanticDomainFromSense(Guid senseId, Guid semanticDomainId); diff --git a/backend/FwLite/MiniLcm/SyncHelpers/EntrySync.cs b/backend/FwLite/MiniLcm/SyncHelpers/EntrySync.cs index e48165bbd..805e1b385 100644 --- a/backend/FwLite/MiniLcm/SyncHelpers/EntrySync.cs +++ b/backend/FwLite/MiniLcm/SyncHelpers/EntrySync.cs @@ -172,7 +172,7 @@ public async Task Add(Sense sense, BetweenPosition between) public async Task Move(Sense sense, BetweenPosition between) { - await api.MoveSense(entryId, sense, between); + await api.MoveSense(entryId, sense.Id, between); return 1; } From 121ebf3b0728702315152047d245d4b07fafa318 Mon Sep 17 00:00:00 2001 From: Tim Haasdyk Date: Thu, 12 Dec 2024 15:50:06 +0100 Subject: [PATCH 22/28] Use letters for test glosses, because orders use int --- .../FwLite/MiniLcm.Tests/UpdateEntryTestsBase.cs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/backend/FwLite/MiniLcm.Tests/UpdateEntryTestsBase.cs b/backend/FwLite/MiniLcm.Tests/UpdateEntryTestsBase.cs index 09069fe90..1c7e21886 100644 --- a/backend/FwLite/MiniLcm.Tests/UpdateEntryTestsBase.cs +++ b/backend/FwLite/MiniLcm.Tests/UpdateEntryTestsBase.cs @@ -108,14 +108,14 @@ public async Task UpdateEntry_CanUseSameVersionMultipleTimes() } [Theory] - [InlineData("1,2", "1,2,3,4", "1,2,3,4")] // append - [InlineData("1,2", "3,1,2", "0,1,2")] // single prepend - [InlineData("1,2", "4,3,1,2", "0,0.5,1,2")] // multi prepend - [InlineData("1,2,3,4", "4,1,2,3", "0,1,2,3")] // move to back - [InlineData("1,2,3,4", "2,3,4,1", "2,3,4,5")] // move to front - [InlineData("1,2,3,4,5", "1,2,5,3,4", "1,2,2.5,3,4")] // move to middle - [InlineData("1,2,3", "3,2,1", "3,4,5")] // reverse - [InlineData("1,2,3,4", "4,2,3,1", "1,2,3,4")] // swap + [InlineData("a,b", "a,b,c,d", "1,2,3,4")] // append + [InlineData("a,2", "c,a,b", "0,1,2")] // single prepend + [InlineData("a,b", "d,c,a,b", "0,0.5,1,2")] // multi prepend + [InlineData("a,b,c,d", "d,a,b,c", "0,1,2,3")] // move to back + [InlineData("a,b,c,d", "b,c,d,a", "2,3,4,5")] // move to front + [InlineData("a,b,c,d,e", "a,b,e,c,d", "1,2,2.5,3,4")] // move to middle + [InlineData("a,b,c", "c,b,a", "3,4,5")] // reverse + [InlineData("a,b,c,d", "d,b,c,a", "1,2,3,4")] // swap public async Task UpdateEntry_CanReorderSenses(string before, string after, string expectedOrderValues) { // arrange From 59b1db51313b9e22d9a65725fb02ed718ac3505a Mon Sep 17 00:00:00 2001 From: Tim Haasdyk Date: Thu, 12 Dec 2024 15:51:16 +0100 Subject: [PATCH 23/28] Don't mock IOrderable in tests --- .../MiniLcm.Tests/DiffCollectionTests.cs | 23 +++++++++---------- 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/backend/FwLite/MiniLcm.Tests/DiffCollectionTests.cs b/backend/FwLite/MiniLcm.Tests/DiffCollectionTests.cs index a24887c96..f349f734e 100644 --- a/backend/FwLite/MiniLcm.Tests/DiffCollectionTests.cs +++ b/backend/FwLite/MiniLcm.Tests/DiffCollectionTests.cs @@ -207,11 +207,11 @@ public async Task DiffTests(CollectionDiffTestCase testCase) private static IOrderable Orderable(double order, Guid? id = null) { - id ??= Guid.NewGuid(); - var orderable = new Mock(); - orderable.SetupGet(o => o.Order).Returns(order); - orderable.SetupGet(o => o.Id).Returns(id.Value); - return orderable.Object; + return new TestOrderable() + { + Order = order, + Id = id ?? Guid.NewGuid(), + }; } private static BetweenPosition Between(IOrderable? previous = null, IOrderable? next = null) @@ -257,13 +257,6 @@ private static BetweenPosition Between(Guid? previous = null, Guid? next = null) } } -public class DiffResult -{ - public required int ChangeCount { get; init; } - public required Mock> DiffApi { get; init; } - public required IMiniLcmApi Api { get; init; } -} - public class CollectionDiffTestCase { public required IOrderable[] OldValues { get; init; } @@ -278,3 +271,9 @@ public class CollectionDiffOperation(IOrderable value) public int? To { get; init; } public BetweenPosition? Between { get; init; } } + +public class TestOrderable : IOrderable +{ + public required Guid Id { get; set; } + public required double Order { get; set; } +} From 68587bf30d8bfc354a69ad4b2222e2f347f6eca8 Mon Sep 17 00:00:00 2001 From: Tim Haasdyk Date: Thu, 12 Dec 2024 15:52:10 +0100 Subject: [PATCH 24/28] Revert unintended extra sync --- backend/FwLite/FwLiteProjectSync.Tests/Sena3SyncTests.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/backend/FwLite/FwLiteProjectSync.Tests/Sena3SyncTests.cs b/backend/FwLite/FwLiteProjectSync.Tests/Sena3SyncTests.cs index 51db068e8..80b2e8079 100644 --- a/backend/FwLite/FwLiteProjectSync.Tests/Sena3SyncTests.cs +++ b/backend/FwLite/FwLiteProjectSync.Tests/Sena3SyncTests.cs @@ -151,7 +151,6 @@ public async Task SyncWithoutImport_CrdtShouldMatchFwdata() [Fact] public async Task SecondSena3SyncDoesNothing() { - await _syncService.Sync(_crdtApi, _fwDataApi); await _syncService.Sync(_crdtApi, _fwDataApi); var secondSync = await _syncService.Sync(_crdtApi, _fwDataApi); secondSync.CrdtChanges.Should().Be(0); From eabc7a7efb2820d8036ffa069979fadad7f5ef62 Mon Sep 17 00:00:00 2001 From: Tim Haasdyk Date: Thu, 12 Dec 2024 15:57:52 +0100 Subject: [PATCH 25/28] Fix SetOrderChange type discriminator(s) --- .../DataModelSnapshotTests.VerifyChangeModels.verified.txt | 2 +- backend/FwLite/LcmCrdt/Changes/SetOrderChange.cs | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/backend/FwLite/LcmCrdt.Tests/DataModelSnapshotTests.VerifyChangeModels.verified.txt b/backend/FwLite/LcmCrdt.Tests/DataModelSnapshotTests.VerifyChangeModels.verified.txt index 8660f761d..578529de3 100644 --- a/backend/FwLite/LcmCrdt.Tests/DataModelSnapshotTests.VerifyChangeModels.verified.txt +++ b/backend/FwLite/LcmCrdt.Tests/DataModelSnapshotTests.VerifyChangeModels.verified.txt @@ -122,7 +122,7 @@ }, { DerivedType: SetOrderChange, - TypeDiscriminator: SetOrderChange`1 + TypeDiscriminator: SetOrderChange:Sense } ], IgnoreUnrecognizedTypeDiscriminators: false, diff --git a/backend/FwLite/LcmCrdt/Changes/SetOrderChange.cs b/backend/FwLite/LcmCrdt/Changes/SetOrderChange.cs index 4e75ca09f..8b56f923b 100644 --- a/backend/FwLite/LcmCrdt/Changes/SetOrderChange.cs +++ b/backend/FwLite/LcmCrdt/Changes/SetOrderChange.cs @@ -3,9 +3,11 @@ namespace LcmCrdt.Changes; -public class SetOrderChange(Guid entityId, double order) : EditChange(entityId), ISelfNamedType> +public class SetOrderChange(Guid entityId, double order) : EditChange(entityId), IPolyType where T : class, IOrderable { + public static string TypeName => $"{nameof(SetOrderChange)}:" + typeof(T).Name; + public double Order { get; init; } = order; public override ValueTask ApplyChange(T entity, ChangeContext context) From 1e4d48541d39da93eecdd850fa198af23e8210ac Mon Sep 17 00:00:00 2001 From: Tim Haasdyk Date: Fri, 13 Dec 2024 11:26:21 +0100 Subject: [PATCH 26/28] Optimize order picker --- backend/FwLite/LcmCrdt/OrderPicker.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/backend/FwLite/LcmCrdt/OrderPicker.cs b/backend/FwLite/LcmCrdt/OrderPicker.cs index dc1118313..e044ee110 100644 --- a/backend/FwLite/LcmCrdt/OrderPicker.cs +++ b/backend/FwLite/LcmCrdt/OrderPicker.cs @@ -10,7 +10,7 @@ public static async Task PickOrder(IQueryable siblings, BetweenPos // a common case that we can optimize by not querying whole objects if (between is null or { Previous: null, Next: null }) { - var currMaxOrder = siblings.Select(s => s.Order).DefaultIfEmpty().Max(); + var currMaxOrder = await siblings.Select(s => s.Order).DefaultIfEmpty().MaxAsync(); return currMaxOrder + 1; } @@ -27,7 +27,7 @@ public static async Task PickOrder(IQueryable siblings, BetweenPos return (previous, next) switch { // another user deleted items in the meantime? - (null, null) => siblings.Select(s => s.Order).DefaultIfEmpty().Max() + 1, + (null, null) => items.Select(s => s.Order).DefaultIfEmpty().Max() + 1, (_, null) => previous.Order + 1, (null, _) => next.Order - 1, // If the next item has been shifted previous the previous item, then between is likely not the actual intent, From ea863541c8600d961d81f9bd3a5610dd47f3b849 Mon Sep 17 00:00:00 2001 From: Tim Haasdyk Date: Fri, 13 Dec 2024 14:53:59 +0100 Subject: [PATCH 27/28] Refactor Orderable diffing with custom diff formatter --- .../MiniLcm.Tests/DiffCollectionTests.cs | 294 +++++++++--------- .../MiniLcm/SyncHelpers/DiffCollection.cs | 134 ++++---- 2 files changed, 219 insertions(+), 209 deletions(-) diff --git a/backend/FwLite/MiniLcm.Tests/DiffCollectionTests.cs b/backend/FwLite/MiniLcm.Tests/DiffCollectionTests.cs index f349f734e..10d28a674 100644 --- a/backend/FwLite/MiniLcm.Tests/DiffCollectionTests.cs +++ b/backend/FwLite/MiniLcm.Tests/DiffCollectionTests.cs @@ -1,6 +1,4 @@ -using FluentAssertions.Execution; using MiniLcm.SyncHelpers; -using Moq; namespace MiniLcm.Tests; @@ -9,61 +7,75 @@ public class DiffCollectionTests [Fact] public async Task MatchingCollections_NoChangesAreGenerated() { + // arrange var value1 = Orderable(1, Guid.NewGuid()); var value2 = Orderable(2, Guid.NewGuid()); - var (changeCount, _) = await Diff([value1, value2], [value1, value2]); + // act + var (changeCount, diffOperations, replacements) = await Diff([value1, value2], [value1, value2]); + + // assert changeCount.Should().Be(0); + diffOperations.Should().BeEmpty(); + replacements.Should().BeEquivalentTo([(value1, value1), (value2, value2)]); } [Fact] public async Task AddedValues() { + // arrange var value1 = Orderable(1, Guid.NewGuid()); var value2 = Orderable(2, Guid.NewGuid()); var value3 = Orderable(3, Guid.NewGuid()); - var (changeCount, diffApi) = await Diff([value1], [value2, value1, value3]); - - changeCount.Should().Be(2); - - var x_v1 = Between(next: value1); - diffApi.Verify(dApi => dApi.Add(value2, x_v1), Times.Once); - var v1_x = Between(previous: value1); - diffApi.Verify(dApi => dApi.Add(value3, v1_x), Times.Once); + // act + var (changeCount, diffOperations, replacements) = await Diff([value1], [value2, value1, value3]); - diffApi.Verify(dApi => dApi.Replace(value1, value1), Times.Once); - diffApi.VerifyNoOtherCalls(); + // assert + changeCount.Should().Be(2); + replacements.Should().BeEquivalentTo([(value1, value1)]); + diffOperations.Should().BeEquivalentTo([ + Add(value2, Between(null, value1)), + Add(value3, Between(value1, null)), + ], options => options.WithStrictOrdering()); } [Fact] public async Task RemovedValues() { + // arrange var value1 = Orderable(1, Guid.NewGuid()); var value2 = Orderable(2, Guid.NewGuid()); var value3 = Orderable(3, Guid.NewGuid()); - var (changeCount, diffApi) = await Diff([value2, value1, value3], [value1]); + // act + var (changeCount, diffOperations, replacements) = await Diff([value2, value1, value3], [value1]); + + // assert changeCount.Should().Be(2); - diffApi.Verify(dApi => dApi.Remove(value2), Times.Once); - diffApi.Verify(dApi => dApi.Remove(value3), Times.Once); - diffApi.Verify(dApi => dApi.Replace(value1, value1), Times.Once); - diffApi.VerifyNoOtherCalls(); + replacements.Should().BeEquivalentTo([(value1, value1)]); + diffOperations.Should().BeEquivalentTo([ + Remove(value3), + Remove(value2), + ], options => options.WithStrictOrdering()); } [Fact] public async Task SwappedValues() { + // arrange var value1 = Orderable(1, Guid.NewGuid()); var value2 = Orderable(2, Guid.NewGuid()); - var (changeCount, diffApi) = await Diff([value1, value2], [value2, value1]); + // act + var (changeCount, diffOperations, replacements) = await Diff([value1, value2], [value2, value1]); + + // assert changeCount.Should().Be(1); - var v2_x = Between(previous: value2); - diffApi.Verify(dApi => dApi.Move(value1, v2_x), Times.Once); - diffApi.Verify(dApi => dApi.Replace(value1, value1), Times.Once); - diffApi.Verify(dApi => dApi.Replace(value2, value2), Times.Once); - diffApi.VerifyNoOtherCalls(); + replacements.Should().BeEquivalentTo([(value1, value1), (value2, value2)]); + diffOperations.Should().BeEquivalentTo([ + Move(value1, Between(value2, null)), + ]); } public static IEnumerable CollectionDiffTestCaseData() @@ -77,8 +89,8 @@ public static IEnumerable CollectionDiffTestCaseData() OldValues = [_1, _2, _3], NewValues = [_3, _2, _1], ExpectedOperations = [ - new(_2) { From = 1, To = 1, Between = Between(previous: _3) }, - new(_1) { From = 0, To = 2, Between = Between(previous: _2) }, + Move(_2, Between(_3, null)), + Move(_1, Between(_2, null)), ], }]; yield return [new CollectionDiffTestCase @@ -86,7 +98,7 @@ public static IEnumerable CollectionDiffTestCaseData() OldValues = [_1, _2, _3, _4], NewValues = [_1, _4, _2, _3], ExpectedOperations = [ - new(_4) { From = 3, To = 1, Between = Between(_1, _2) }, + Move(_4, Between(_1, _2)), ], }]; yield return [new CollectionDiffTestCase @@ -94,8 +106,8 @@ public static IEnumerable CollectionDiffTestCaseData() OldValues = [_1, _2, _3, _4], NewValues = [_2, _1, _4, _3], ExpectedOperations = [ // When only 4, moving the 2 outsides to middle is represented slightly differently: - new(_1) { From = 0, To = 1, Between = Between(_2, _4) }, - new(_3) { From = 2, To = 3, Between = Between(_4, null) }, + Move(_1, Between(_2, _4)), + Move(_3, Between(_4, null)), ], }]; @@ -106,8 +118,8 @@ public static IEnumerable CollectionDiffTestCaseData() OldValues = [_1, _2, _3, _4, _5, _6], NewValues = [_2, _3, _1, _6, _4, _5], ExpectedOperations = [ // When 6+, moving the 2 outsides to middle is represented as such: - new(_1) { From = 0, To = 2, Between = Between(_3, _4) }, - new(_6) { From = 5, To = 3, Between = Between(_1, _4) }, + Move(_1, Between(_3, _4)), + Move(_6, Between(_1, _4)), ], }]; @@ -118,13 +130,13 @@ public static IEnumerable CollectionDiffTestCaseData() OldValues = [_1, _2, _3, _4, _5], NewValues = [_6, _8, _4, _2, _7], ExpectedOperations = [ - new(_1) { From = 0 }, - new(_3) { From = 2 }, - new(_5) { From = 4 }, - new(_6) { To = 0, Between = Between(next: _4) }, // (not next: _8, because _8 is not "stable") - new(_8) { To = 1, Between = Between(_6, _4) }, - new(_2) { From = 1, To = 3, Between = Between(previous: _4) }, - new(_7) { To = 4, Between = Between(previous: _2) }, + Remove(_5), + Remove(_3), + Remove(_1), + Add(_6, Between(null, _4)), // (not next: _8, because _8 is not "stable") + Add(_8, Between(_6, _4)), + Move(_2, Between(_4, null)), + Add(_7, Between(_2, null)), ], }]; } @@ -133,79 +145,29 @@ public static IEnumerable CollectionDiffTestCaseData() [MemberData(nameof(CollectionDiffTestCaseData))] public async Task DiffTests(CollectionDiffTestCase testCase) { - using (new AssertionScope("Check for silly test case mistakes")) - { - foreach (var operation in testCase.ExpectedOperations) - { - if (operation.From is not null) - testCase.OldValues[operation.From.Value].Should().Be(operation.Value); - if (operation.To is not null) - testCase.NewValues[operation.To.Value].Should().Be(operation.Value); - if (operation.Between?.Previous is not null) - testCase.NewValues.Should().ContainSingle(v => v.Id == operation.Between.Previous); - if (operation.Between?.Next is not null) - testCase.NewValues.Should().ContainSingle(v => v.Id == operation.Between.Next); - } - } - - var (changeCount, diffApi) = await Diff(testCase.OldValues, testCase.NewValues); - - using var scope = new AssertionScope(); + // act + var (changeCount, diffOperations, replacements) = await Diff(testCase.OldValues, testCase.NewValues); + // assert changeCount.Should().Be(testCase.ExpectedOperations.Count); + diffOperations.Should().BeEquivalentTo(testCase.ExpectedOperations, options => options.WithStrictOrdering()); + } - var expectedReplaceCount = testCase.OldValues.Select(v => v.Id).Intersect(testCase.NewValues.Select(v => v.Id)).Count(); - diffApi.Verify(dApi => dApi.Replace(It.IsAny(), It.IsAny()), Times.Exactly(expectedReplaceCount)); - - foreach (var operation in testCase.ExpectedOperations) - { - try - { - if (operation.From is not null && operation.To is not null) - { - operation.Between.Should().NotBeNull(); - var movedValue = testCase.OldValues[operation.From.Value]; - diffApi.Verify( - dApi => dApi.Move( - movedValue, - operation.Between - ), - Times.Once - ); - } - else if (operation.From is not null) - { - var removedValue = testCase.OldValues[operation.From.Value]; - diffApi.Verify( - dApi => dApi.Remove( - removedValue - ), - Times.Once - ); - } - else if (operation.To is not null) - { - operation.Between.Should().NotBeNull(); - var addedValue = testCase.NewValues[operation.To.Value]; - diffApi.Verify( - dApi => dApi.Add( - addedValue, - operation.Between - ), - Times.Once - ); - } - } - catch (Exception ex) - { - scope.AddPreFormattedFailure($"{ex.Message} From: {operation.From} To: {operation.To}"); - } - } - - diffApi.VerifyNoOtherCalls(); + private static async Task<( + int changeCount, + List DiffOperations, + List<(TestOrderable before, TestOrderable after)> Replacements + )> Diff(TestOrderable[] oldValues, TestOrderable[] newValues) + { + var diffApi = new TestOrderableDiffApi(oldValues); + var changeCount = await DiffCollection.DiffOrderable(oldValues, newValues, diffApi); + diffApi.Current.Should().BeEquivalentTo(newValues); + var expectedReplacements = oldValues.Join(newValues, o => o.Id, o => o.Id, (o, n) => (o, n)).ToList(); + diffApi.Replacements.Should().BeEquivalentTo(expectedReplacements); + return (changeCount, diffApi.DiffOperations, diffApi.Replacements); } - private static IOrderable Orderable(double order, Guid? id = null) + private static TestOrderable Orderable(double order, Guid? id = null) { return new TestOrderable() { @@ -214,7 +176,7 @@ private static IOrderable Orderable(double order, Guid? id = null) }; } - private static BetweenPosition Between(IOrderable? previous = null, IOrderable? next = null) + private static BetweenPosition Between(TestOrderable? previous, TestOrderable? next) { return Between(previous?.Id, next?.Id); } @@ -228,52 +190,102 @@ private static BetweenPosition Between(Guid? previous = null, Guid? next = null) }; } - private static async Task<(int, Mock>)> Diff(IOrderable[] oldValues, IOrderable[] newValues) + private static CollectionDiffOperation Move(TestOrderable value, BetweenPosition between) + { + return new CollectionDiffOperation(value, PositionDiffKind.Move, between); + } + + private static CollectionDiffOperation Add(TestOrderable value, BetweenPosition between) + { + return new CollectionDiffOperation(value, PositionDiffKind.Add, between); + } + + private static CollectionDiffOperation Remove(TestOrderable value) { - var diffApi = new Mock>(); - diffApi.Setup(f => f.Add(It.IsAny(), It.IsAny())) - .ReturnsAsync(1); - diffApi.Setup(f => f.Remove(It.IsAny())) - .ReturnsAsync(1); - diffApi.Setup(f => f.Move(It.IsAny(), It.IsAny())) - .ReturnsAsync(1); - diffApi.Setup(f => f.Replace(It.IsAny(), It.IsAny())) - .Returns((IMiniLcmApi api, IOrderable oldValue, IOrderable newValue) => - { - try - { - oldValue.Should().BeEquivalentTo(newValue); - return Task.FromResult(0); - } - catch - { - return Task.FromResult(1); - } - }); - - var changeCount = await DiffCollection.DiffOrderable(oldValues, newValues, diffApi.Object); - - return (changeCount, diffApi); + return new CollectionDiffOperation(value, PositionDiffKind.Remove); } } public class CollectionDiffTestCase { - public required IOrderable[] OldValues { get; init; } - public required IOrderable[] NewValues { get; init; } + public required TestOrderable[] OldValues { get; init; } + public required TestOrderable[] NewValues { get; init; } public required List ExpectedOperations { get; init; } } -public class CollectionDiffOperation(IOrderable value) -{ - public IOrderable Value { get; init; } = value; - public int? From { get; init; } - public int? To { get; init; } - public BetweenPosition? Between { get; init; } -} +public record CollectionDiffOperation(TestOrderable Value, PositionDiffKind Kind, BetweenPosition? Between = null); public class TestOrderable : IOrderable { public required Guid Id { get; set; } public required double Order { get; set; } } + +public class TestOrderableDiffApi(TestOrderable[] before) : OrderableCollectionDiffApi +{ + public List Current { get; } = [.. before]; + public List DiffOperations = new(); + public List<(TestOrderable before, TestOrderable after)> Replacements = new(); + + 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) + { + if (between.Previous is not null) + { + var previousIndex = Current.FindIndex(v => v.Id == between.Previous); + Current.Insert(previousIndex + 1, value); + } + else if (between.Next is not null) + { + var nextIndex = Current.FindIndex(v => v.Id == between.Next); + Current.Insert(nextIndex, value); + } + else + { + Current.Add(value); + } + return Task.FromResult(1); + } + + public Task Remove(TestOrderable value) + { + DiffOperations.Add(new CollectionDiffOperation(value, PositionDiffKind.Remove)); + return RemoveInternal(value); + } + + public Task RemoveInternal(TestOrderable value) + { + var removeCount = Current.RemoveAll(v => v.Id == value.Id); + removeCount.Should().Be(1); + return Task.FromResult(1); + } + + public async Task Move(TestOrderable value, BetweenPosition between) + { + DiffOperations.Add(new CollectionDiffOperation(value, PositionDiffKind.Move, between)); + await RemoveInternal(value); + await AddInternal(value, between); + return 1; + } + + public Task Replace(TestOrderable before, TestOrderable after) + { + Replacements.Add((before, after)); + before.Id.Should().Be(after.Id); + Current[Current.FindIndex(v => v.Id == before.Id)] = after; + try + { + before.Should().BeEquivalentTo(after); + return Task.FromResult(0); + } + catch + { + return Task.FromResult(1); + } + } +} diff --git a/backend/FwLite/MiniLcm/SyncHelpers/DiffCollection.cs b/backend/FwLite/MiniLcm/SyncHelpers/DiffCollection.cs index e75dfe768..0d16d316d 100644 --- a/backend/FwLite/MiniLcm/SyncHelpers/DiffCollection.cs +++ b/backend/FwLite/MiniLcm/SyncHelpers/DiffCollection.cs @@ -1,4 +1,7 @@ -using System.Text.Json.JsonDiffPatch; +using System.Collections.Immutable; +using System.Text.Json.JsonDiffPatch; +using System.Text.Json.JsonDiffPatch.Diffs; +using System.Text.Json.JsonDiffPatch.Diffs.Formatters; using System.Text.Json.Nodes; using MiniLcm.Models; @@ -110,35 +113,37 @@ public static async Task DiffOrderable( IList after, OrderableCollectionDiffApi diffApi) where T : IOrderable { - var positionDiffs = DiffPositions(before, after) - // Order: Deletes first, then adds and moves from lowest to highest new index - // important, because new indexes represent final positions, which might not exist yet in the before list - // With this order, callers don't have to account for potential gaps - .OrderBy(d => d.To ?? -1).ToList(); - - var unstableIndexes = positionDiffs.Select(diff => diff.From).Where(i => i is not null).ToList(); - var stableIds = before.Where((_, i) => !unstableIndexes.Contains(i)).Select(item => item.Id).ToList(); - var changes = 0; - foreach (var diff in positionDiffs) + + var positionDiffs = DiffPositions(before, after); + if (positionDiffs is not null) { - if (diff.From is not null && diff.To is not null) - { - var afterEntry = after[diff.To.Value]; - var between = GetStableBetween(diff.To.Value, after, stableIds); - changes += await diffApi.Move(afterEntry, between); - stableIds.Add(afterEntry.Id); - } - else if (diff.From is not null) - { - changes += await diffApi.Remove(before[diff.From.Value]); - } - else if (diff.To is not null) + var stableIds = after.Where((_, i) => !positionDiffs.ContainsKey(i)) + .Select(item => item.Id) + .ToHashSet(); + + foreach (var (_, diff) in positionDiffs) { - var afterEntry = after[diff.To.Value]; - var between = GetStableBetween(diff.To.Value, after, stableIds); - changes += await diffApi.Add(afterEntry, between); - stableIds.Add(afterEntry.Id); + switch (diff.Kind) + { + case PositionDiffKind.Move: + var movedEntry = after[diff.Index]; + var between = GetStableBetween(diff.Index, after, stableIds); + changes += await diffApi.Move(movedEntry, between); + stableIds.Add(movedEntry.Id); + break; + case PositionDiffKind.Remove: + changes += await diffApi.Remove(before[diff.Index]); + break; + case PositionDiffKind.Add: + var addedEntry = after[diff.Index]; + between = GetStableBetween(diff.Index, after, stableIds); + changes += await diffApi.Add(addedEntry, between); + stableIds.Add(addedEntry.Id); + break; + default: + throw new InvalidOperationException($"Unsupported position diff kind {diff.Kind}"); + } } } @@ -154,7 +159,7 @@ public static async Task DiffOrderable( return changes; } - private static BetweenPosition GetStableBetween(int i, IList current, IReadOnlyList stable) where T : IOrderable + private static BetweenPosition GetStableBetween(int i, IList current, HashSet stable) where T : IOrderable { T? beforeEntity = default; T? afterEntity = default; @@ -181,57 +186,50 @@ private static BetweenPosition GetStableBetween(int i, IList current, IRea }; } - private static IEnumerable DiffPositions( + private static ImmutableSortedDictionary? DiffPositions( IList before, IList after) where T : IOrderable { - var beforeJson = new JsonArray(before.Select(item => JsonValue.Create(item.Id)).ToArray()); - var afterJson = new JsonArray(after.Select(item => JsonValue.Create(item.Id)).ToArray()); + var beforeJson = new JsonArray([.. before.Select(item => JsonValue.Create(item.Id))]); + var afterJson = new JsonArray([.. after.Select(item => JsonValue.Create(item.Id))]); + return JsonDiffPatcher.Diff(beforeJson, afterJson, DiffFormatter.Instance); + } - if (JsonDiffPatcher.Diff(beforeJson, afterJson) is not JsonObject result) - { - yield break; // no changes - } +#pragma warning disable IDE0072 // Add missing cases + /// + /// Formats Json array diffs into a list sorted by: + /// - deletes first (with arbitrary negative indexes) + /// - added and moved in order of new/current index (using current index) + /// + private class DiffFormatter : IJsonDiffDeltaFormatter> + { + public static readonly DiffFormatter Instance = new(); + + private DiffFormatter() { } - foreach (var prop in result) + public ImmutableSortedDictionary? Format(ref JsonDiffDelta delta, JsonNode? left) { - if (prop.Key == "_t") // diff type - { - if (prop.Value!.ToString() != "a") // we're only using the library for diffing shallow arrays - { - throw new InvalidOperationException("Only array diff results are supported"); - } - continue; - } - else if (prop.Key.StartsWith("_")) // e.g _4 => the key represents an old index (removed or moved) + if (delta.Kind == DeltaKind.None) return null; + + return delta.GetArrayChangeEnumerable().Select(change => { - var previousIndex = int.Parse(prop.Key[1..]); - var delta = prop.Value!.AsArray(); - var wasMoved = delta[2]!.GetValue() == 3; // 3 is magic number for a move operation - int? newIndex = wasMoved ? delta[1]!.GetValue() : null; // if it was moved, the new index is at index 1 - if (newIndex is not null) - { - yield return new PositionDiff { From = previousIndex, To = newIndex }; // move - } - else + return change.Diff.Kind switch { - yield return new PositionDiff { From = previousIndex }; // remove - } - } - else // e.g. 4 => the key represents a new index - { - var addIndex = int.Parse(prop.Key); - yield return new PositionDiff { To = addIndex }; // add - } + DeltaKind.ArrayMove => new PositionDiff(change.Diff.GetNewIndex(), PositionDiffKind.Move), + DeltaKind.Added => new PositionDiff(change.Index, PositionDiffKind.Add), + DeltaKind.Deleted => new PositionDiff(change.Index, PositionDiffKind.Remove), + _ => throw new InvalidOperationException("Only array diff results are supported"), + }; + }).ToImmutableSortedDictionary(diff => diff.SortIndex, diff => diff); } - } +#pragma warning restore IDE0072 // Add missing cases +} - private class PositionDiff - { - public int? From { get; init; } - public int? To { get; init; } - } +public enum PositionDiffKind { Add, Remove, Move } +public record PositionDiff(int Index, PositionDiffKind Kind) +{ + public int SortIndex => Kind == PositionDiffKind.Remove ? -Index - 1 : Index; } public class BetweenPosition : IEquatable From 42a04184abcfd61b6cc38646945cd466569cee17 Mon Sep 17 00:00:00 2001 From: Tim Haasdyk Date: Wed, 18 Dec 2024 11:13:31 +0100 Subject: [PATCH 28/28] Incorporated PR feedback --- .../MiniLcm.Tests/DiffCollectionTests.cs | 133 +++------------ .../MiniLcm.Tests/TestOrderableDiffApi.cs | 160 ++++++++++++++++++ .../MiniLcm/SyncHelpers/DiffCollection.cs | 53 ++---- .../FwLite/MiniLcm/SyncHelpers/EntrySync.cs | 2 +- 4 files changed, 196 insertions(+), 152 deletions(-) create mode 100644 backend/FwLite/MiniLcm.Tests/TestOrderableDiffApi.cs diff --git a/backend/FwLite/MiniLcm.Tests/DiffCollectionTests.cs b/backend/FwLite/MiniLcm.Tests/DiffCollectionTests.cs index 10d28a674..1a1e9dfb0 100644 --- a/backend/FwLite/MiniLcm.Tests/DiffCollectionTests.cs +++ b/backend/FwLite/MiniLcm.Tests/DiffCollectionTests.cs @@ -8,8 +8,8 @@ public class DiffCollectionTests public async Task MatchingCollections_NoChangesAreGenerated() { // arrange - var value1 = Orderable(1, Guid.NewGuid()); - var value2 = Orderable(2, Guid.NewGuid()); + var value1 = new TestOrderable(1, Guid.NewGuid()); + var value2 = new TestOrderable(2, Guid.NewGuid()); // act var (changeCount, diffOperations, replacements) = await Diff([value1, value2], [value1, value2]); @@ -24,9 +24,9 @@ public async Task MatchingCollections_NoChangesAreGenerated() public async Task AddedValues() { // arrange - var value1 = Orderable(1, Guid.NewGuid()); - var value2 = Orderable(2, Guid.NewGuid()); - var value3 = Orderable(3, Guid.NewGuid()); + var value1 = new TestOrderable(1, Guid.NewGuid()); + var value2 = new TestOrderable(2, Guid.NewGuid()); + var value3 = new TestOrderable(3, Guid.NewGuid()); // act var (changeCount, diffOperations, replacements) = await Diff([value1], [value2, value1, value3]); @@ -44,9 +44,9 @@ public async Task AddedValues() public async Task RemovedValues() { // arrange - var value1 = Orderable(1, Guid.NewGuid()); - var value2 = Orderable(2, Guid.NewGuid()); - var value3 = Orderable(3, Guid.NewGuid()); + var value1 = new TestOrderable(1, Guid.NewGuid()); + var value2 = new TestOrderable(2, Guid.NewGuid()); + var value3 = new TestOrderable(3, Guid.NewGuid()); // act var (changeCount, diffOperations, replacements) = await Diff([value2, value1, value3], [value1]); @@ -64,8 +64,8 @@ public async Task RemovedValues() public async Task SwappedValues() { // arrange - var value1 = Orderable(1, Guid.NewGuid()); - var value2 = Orderable(2, Guid.NewGuid()); + var value1 = new TestOrderable(1, Guid.NewGuid()); + var value2 = new TestOrderable(2, Guid.NewGuid()); // act var (changeCount, diffOperations, replacements) = await Diff([value1, value2], [value2, value1]); @@ -80,10 +80,10 @@ public async Task SwappedValues() public static IEnumerable CollectionDiffTestCaseData() { - var _1 = Orderable(1, Guid.NewGuid()); - var _2 = Orderable(2, Guid.NewGuid()); - var _3 = Orderable(3, Guid.NewGuid()); - var _4 = Orderable(4, Guid.NewGuid()); + var _1 = new TestOrderable(1, Guid.NewGuid()); + var _2 = new TestOrderable(2, Guid.NewGuid()); + var _3 = new TestOrderable(3, Guid.NewGuid()); + var _4 = new TestOrderable(4, Guid.NewGuid()); yield return [new CollectionDiffTestCase { OldValues = [_1, _2, _3], @@ -111,8 +111,8 @@ public static IEnumerable CollectionDiffTestCaseData() ], }]; - var _5 = Orderable(5, Guid.NewGuid()); - var _6 = Orderable(6, Guid.NewGuid()); + var _5 = new TestOrderable(5, Guid.NewGuid()); + var _6 = new TestOrderable(6, Guid.NewGuid()); yield return [new CollectionDiffTestCase { OldValues = [_1, _2, _3, _4, _5, _6], @@ -123,8 +123,8 @@ public static IEnumerable CollectionDiffTestCaseData() ], }]; - var _7 = Orderable(7, Guid.NewGuid()); - var _8 = Orderable(8, Guid.NewGuid()); + var _7 = new TestOrderable(7, Guid.NewGuid()); + var _8 = new TestOrderable(8, Guid.NewGuid()); yield return [new CollectionDiffTestCase { OldValues = [_1, _2, _3, _4, _5], @@ -161,21 +161,13 @@ public async Task DiffTests(CollectionDiffTestCase testCase) { var diffApi = new TestOrderableDiffApi(oldValues); var changeCount = await DiffCollection.DiffOrderable(oldValues, newValues, diffApi); - diffApi.Current.Should().BeEquivalentTo(newValues); + // verify that the operations did in fact transform the old collection into the new collection + diffApi.Current.Should().BeEquivalentTo(newValues, options => options.WithStrictOrdering()); var expectedReplacements = oldValues.Join(newValues, o => o.Id, o => o.Id, (o, n) => (o, n)).ToList(); - diffApi.Replacements.Should().BeEquivalentTo(expectedReplacements); + diffApi.Replacements.Should().BeEquivalentTo(expectedReplacements, options => options.WithStrictOrdering()); return (changeCount, diffApi.DiffOperations, diffApi.Replacements); } - private static TestOrderable Orderable(double order, Guid? id = null) - { - return new TestOrderable() - { - Order = order, - Id = id ?? Guid.NewGuid(), - }; - } - private static BetweenPosition Between(TestOrderable? previous, TestOrderable? next) { return Between(previous?.Id, next?.Id); @@ -183,11 +175,7 @@ private static BetweenPosition Between(TestOrderable? previous, TestOrderable? n private static BetweenPosition Between(Guid? previous = null, Guid? next = null) { - return new BetweenPosition - { - Previous = previous, - Next = next - }; + return new BetweenPosition(previous, next); } private static CollectionDiffOperation Move(TestOrderable value, BetweenPosition between) @@ -212,80 +200,3 @@ public class CollectionDiffTestCase public required TestOrderable[] NewValues { get; init; } public required List ExpectedOperations { get; init; } } - -public record CollectionDiffOperation(TestOrderable Value, PositionDiffKind Kind, BetweenPosition? Between = null); - -public class TestOrderable : IOrderable -{ - public required Guid Id { get; set; } - public required double Order { get; set; } -} - -public class TestOrderableDiffApi(TestOrderable[] before) : OrderableCollectionDiffApi -{ - public List Current { get; } = [.. before]; - public List DiffOperations = new(); - public List<(TestOrderable before, TestOrderable after)> Replacements = new(); - - 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) - { - if (between.Previous is not null) - { - var previousIndex = Current.FindIndex(v => v.Id == between.Previous); - Current.Insert(previousIndex + 1, value); - } - else if (between.Next is not null) - { - var nextIndex = Current.FindIndex(v => v.Id == between.Next); - Current.Insert(nextIndex, value); - } - else - { - Current.Add(value); - } - return Task.FromResult(1); - } - - public Task Remove(TestOrderable value) - { - DiffOperations.Add(new CollectionDiffOperation(value, PositionDiffKind.Remove)); - return RemoveInternal(value); - } - - public Task RemoveInternal(TestOrderable value) - { - var removeCount = Current.RemoveAll(v => v.Id == value.Id); - removeCount.Should().Be(1); - return Task.FromResult(1); - } - - public async Task Move(TestOrderable value, BetweenPosition between) - { - DiffOperations.Add(new CollectionDiffOperation(value, PositionDiffKind.Move, between)); - await RemoveInternal(value); - await AddInternal(value, between); - return 1; - } - - public Task Replace(TestOrderable before, TestOrderable after) - { - Replacements.Add((before, after)); - before.Id.Should().Be(after.Id); - Current[Current.FindIndex(v => v.Id == before.Id)] = after; - try - { - before.Should().BeEquivalentTo(after); - return Task.FromResult(0); - } - catch - { - return Task.FromResult(1); - } - } -} diff --git a/backend/FwLite/MiniLcm.Tests/TestOrderableDiffApi.cs b/backend/FwLite/MiniLcm.Tests/TestOrderableDiffApi.cs new file mode 100644 index 000000000..230ec70a5 --- /dev/null +++ b/backend/FwLite/MiniLcm.Tests/TestOrderableDiffApi.cs @@ -0,0 +1,160 @@ +using MiniLcm.SyncHelpers; + +namespace MiniLcm.Tests; + +public class TestOrderableDiffApiTests +{ + [Fact] + public async Task Add() + { + // arrange + var value1 = new TestOrderable(1, Guid.NewGuid()); + var value2 = new TestOrderable(2, Guid.NewGuid()); + + // act + var diffApi = new TestOrderableDiffApi([value1]); + var position = new BetweenPosition(value1.Id, null); + await diffApi.Add(value2, position); + + // assert + diffApi.DiffOperations.Should().BeEquivalentTo([ + new CollectionDiffOperation(value2, PositionDiffKind.Add, position) + ]); + diffApi.Replacements.Should().BeEquivalentTo(Array.Empty<(TestOrderable, TestOrderable)>()); + diffApi.Current.Should().BeEquivalentTo([value1, value2]); + } + + [Fact] + public async Task Remove() + { + // arrange + var value1 = new TestOrderable(1, Guid.NewGuid()); + var value2 = new TestOrderable(2, Guid.NewGuid()); + + // act + var diffApi = new TestOrderableDiffApi([value1, value2]); + await diffApi.Remove(value1); + + // assert + diffApi.DiffOperations.Should().BeEquivalentTo([ + new CollectionDiffOperation(value1, PositionDiffKind.Remove) + ]); + diffApi.Replacements.Should().BeEquivalentTo(Array.Empty<(TestOrderable, TestOrderable)>()); + diffApi.Current.Should().BeEquivalentTo([value2]); + } + + [Fact] + public async Task Move() + { + // arrange + var value1 = new TestOrderable(1, Guid.NewGuid()); + var value2 = new TestOrderable(2, Guid.NewGuid()); + var value3 = new TestOrderable(3, Guid.NewGuid()); + + // act + var diffApi = new TestOrderableDiffApi([value1, value2, value3]); + var position = new BetweenPosition(value1.Id, value2.Id); + await diffApi.Move(value3, position); + + // assert + diffApi.DiffOperations.Should().BeEquivalentTo([ + new CollectionDiffOperation(value3, PositionDiffKind.Move, position) + ]); + diffApi.Replacements.Should().BeEquivalentTo(Array.Empty<(TestOrderable, TestOrderable)>()); + diffApi.Current.Should().BeEquivalentTo([value1, value3, value2]); + } + + [Fact] + public async Task Replace() + { + // arrange + var oldValue = new TestOrderable(1, Guid.NewGuid()); + var newValue = new TestOrderable(2, oldValue.Id); + + // act + var diffApi = new TestOrderableDiffApi([oldValue]); + await diffApi.Replace(oldValue, newValue); + + // assert + diffApi.DiffOperations.Should().BeEquivalentTo(Array.Empty()); + diffApi.Replacements.Should().BeEquivalentTo([(oldValue, newValue)]); + diffApi.Current.Should().BeEquivalentTo([newValue]); + } +} + +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) + { + DiffOperations.Add(new CollectionDiffOperation(value, PositionDiffKind.Add, between)); + return AddInternal(value, between); + } + + private Task AddInternal(TestOrderable value, BetweenPosition between) + { + if (between.Previous is not null) + { + var previousIndex = Current.FindIndex(v => v.Id == between.Previous); + Current.Insert(previousIndex + 1, value); + } + else if (between.Next is not null) + { + var nextIndex = Current.FindIndex(v => v.Id == between.Next); + Current.Insert(nextIndex, value); + } + else + { + Current.Add(value); + } + return Task.FromResult(1); + } + + public Task Remove(TestOrderable value) + { + DiffOperations.Add(new CollectionDiffOperation(value, PositionDiffKind.Remove)); + return RemoveInternal(value); + } + + public Task RemoveInternal(TestOrderable value) + { + var removeCount = Current.RemoveAll(v => v.Id == value.Id); + removeCount.Should().Be(1); + return Task.FromResult(1); + } + + public async Task Move(TestOrderable value, BetweenPosition between) + { + DiffOperations.Add(new CollectionDiffOperation(value, PositionDiffKind.Move, between)); + await RemoveInternal(value); + await AddInternal(value, between); + return 1; + } + + public Task Replace(TestOrderable before, TestOrderable after) + { + Replacements.Add((before, after)); + before.Id.Should().Be(after.Id); + Current[Current.FindIndex(v => v.Id == before.Id)] = after; + try + { + before.Should().BeEquivalentTo(after); + return Task.FromResult(0); + } + catch + { + return Task.FromResult(1); + } + } +} + +public record CollectionDiffOperation(TestOrderable Value, PositionDiffKind Kind, BetweenPosition? Between = null); + +public class TestOrderable(double order, Guid id) : IOrderable +{ + public Guid Id { get; set; } = id; + public double Order { get; set; } = order; +} diff --git a/backend/FwLite/MiniLcm/SyncHelpers/DiffCollection.cs b/backend/FwLite/MiniLcm/SyncHelpers/DiffCollection.cs index 0d16d316d..23d70249f 100644 --- a/backend/FwLite/MiniLcm/SyncHelpers/DiffCollection.cs +++ b/backend/FwLite/MiniLcm/SyncHelpers/DiffCollection.cs @@ -28,7 +28,7 @@ public override Guid GetId(T value) } } -public interface OrderableCollectionDiffApi where T : IOrderable +public interface IOrderableCollectionDiffApi where T : IOrderable { Task Add(T value, BetweenPosition between); Task Remove(T value); @@ -111,13 +111,15 @@ public static async Task Diff( public static async Task DiffOrderable( IList before, IList after, - OrderableCollectionDiffApi diffApi) where T : IOrderable + IOrderableCollectionDiffApi diffApi) where T : IOrderable { var changes = 0; var positionDiffs = DiffPositions(before, after); if (positionDiffs is not null) { + // The positive keys in positionDiffs are the indexes of added or moved items. I.e. they're the unstable ones. + // Deleted items are given a negative index. So, they aren't picked up here. They also don't exist in the after list, so they're not relevant. var stableIds = after.Where((_, i) => !positionDiffs.ContainsKey(i)) .Select(item => item.Id) .ToHashSet(); @@ -179,11 +181,7 @@ private static BetweenPosition GetStableBetween(int i, IList current, Hash break; } } - return new BetweenPosition - { - Previous = beforeEntity?.Id, - Next = afterEntity?.Id - }; + return new BetweenPosition(beforeEntity?.Id, afterEntity?.Id); } private static ImmutableSortedDictionary? DiffPositions( @@ -229,39 +227,14 @@ private DiffFormatter() { } public enum PositionDiffKind { Add, Remove, Move } public record PositionDiff(int Index, PositionDiffKind Kind) { + // Indexes for add and move operations represent final positions. + // I.e. the order of the diffs doesn't really have meaning, but rather the caller is expected to make sure that's where the item ends up. + // Also, final position indexes might not yet exist in the current list. + + // So, the easiest way to make sure the caller will be able to apply the diffs sequentially is to order them so that: + // - Deletes happens first + // - Adds and moves are then ordered by the new index (i.e. we work from front to back) public int SortIndex => Kind == PositionDiffKind.Remove ? -Index - 1 : Index; } -public class BetweenPosition : IEquatable -{ - public Guid? Previous { get; set; } - public Guid? Next { get; set; } - - public override bool Equals(object? obj) - { - return Equals(obj as BetweenPosition); - } - - public bool Equals(BetweenPosition? other) - { - if (other is null) - return false; - - return Previous == other.Previous && Next == other.Next; - } - - public override int GetHashCode() - { - return HashCode.Combine(Previous, Next); - } - - public static bool operator ==(BetweenPosition left, BetweenPosition right) - { - return EqualityComparer.Default.Equals(left, right); - } - - public static bool operator !=(BetweenPosition left, BetweenPosition right) - { - return !(left == right); - } -} +public record BetweenPosition(Guid? Previous, Guid? Next); diff --git a/backend/FwLite/MiniLcm/SyncHelpers/EntrySync.cs b/backend/FwLite/MiniLcm/SyncHelpers/EntrySync.cs index 805e1b385..15dd6463a 100644 --- a/backend/FwLite/MiniLcm/SyncHelpers/EntrySync.cs +++ b/backend/FwLite/MiniLcm/SyncHelpers/EntrySync.cs @@ -162,7 +162,7 @@ public override Task Replace(ComplexFormComponent beforeComponent, ComplexF } } - private class SensesDiffApi(IMiniLcmApi api, Guid entryId) : OrderableCollectionDiffApi + private class SensesDiffApi(IMiniLcmApi api, Guid entryId) : IOrderableCollectionDiffApi { public async Task Add(Sense sense, BetweenPosition between) {