From ea863541c8600d961d81f9bd3a5610dd47f3b849 Mon Sep 17 00:00:00 2001 From: Tim Haasdyk Date: Fri, 13 Dec 2024 14:53:59 +0100 Subject: [PATCH] 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