From 0e1b8f81492087332ce38117707ea7c1f9b4e699 Mon Sep 17 00:00:00 2001 From: "Darrin W. Cullop" Date: Wed, 22 Nov 2023 15:49:35 -0800 Subject: [PATCH] Index Safety for List Tranform Operator (#762) * Added safety check to the Transfomer operator and associated unit tests * PR Feedback * Handle null and missing index in a single check * Align List/Cache versions * Subtle syntax improvement * Rev the version * Add file to remove coverage details from PR diff --------- Co-authored-by: Chris Pulman --- codecov.yaml | 3 ++ .../List/TransformFixture.cs | 37 +++++++++++++++- src/DynamicData/List/Internal/Transformer.cs | 44 +++++++++++++++---- src/DynamicData/List/ObservableListEx.cs | 24 +++++----- version.json | 2 +- 5 files changed, 87 insertions(+), 23 deletions(-) create mode 100644 codecov.yaml diff --git a/codecov.yaml b/codecov.yaml new file mode 100644 index 000000000..f6786ff8d --- /dev/null +++ b/codecov.yaml @@ -0,0 +1,3 @@ +coverage: + status: #Code coverage status will be posted to pull requests based on targets defined below. + comments: off #Optional. Details are off by default. \ No newline at end of file diff --git a/src/DynamicData.Tests/List/TransformFixture.cs b/src/DynamicData.Tests/List/TransformFixture.cs index f8acd1d1b..b176c0ab4 100644 --- a/src/DynamicData.Tests/List/TransformFixture.cs +++ b/src/DynamicData.Tests/List/TransformFixture.cs @@ -4,7 +4,6 @@ using DynamicData.Tests.Domain; using FluentAssertions; - using Xunit; namespace DynamicData.Tests.List; @@ -186,4 +185,40 @@ public void TransformOnRefresh() results.Messages.Last().First().Reason.Should().Be(ListChangeReason.Replace); } + + [Fact] + public void TransformOnReplace() + { + // Arrange + var person1 = new Person("Bob", 37); + var person2 = new Person("Bob", 38); + + _source.Add(person1); + + // Act + _source.Replace(person1, person2); + + // Assert + _results.Messages.Count.Should().Be(2, "Should be 2 messages"); + _results.Data.Count.Should().Be(1, "Should be 1 item in the cache"); + _results.Data.Items.First().Should().Be(_transformFactory(person2), "Should be same person"); + } + + [Fact] + public void TransformOnReplaceWithoutIndex() + { + // Arrange + var person1 = new Person("Bob", 37); + var person2 = new Person("Bob", 38); + var results = _source.Connect().RemoveIndex().Transform(_transformFactory).AsAggregator(); + _source.Add(person1); + + // Act + _source.Replace(person1, person2); + + // Assert + results.Messages.Count.Should().Be(2, "Should be 2 messages"); + results.Data.Count.Should().Be(1, "Should be 1 item in the cache"); + results.Data.Items.First().Should().Be(_transformFactory(person2), "Should be same person"); + } } diff --git a/src/DynamicData/List/Internal/Transformer.cs b/src/DynamicData/List/Internal/Transformer.cs index c3709829d..12dc2e241 100644 --- a/src/DynamicData/List/Internal/Transformer.cs +++ b/src/DynamicData/List/Internal/Transformer.cs @@ -82,14 +82,26 @@ private void Transform(ChangeAwareList transformed, IC case ListChangeReason.Refresh: { var change = item.Item; + var index = change.CurrentIndex; + if (index < 0) + { + // Find the corresponding index + var current = transformed.FirstOrDefault(x => x.Source.Equals(change.Current)); + index = current switch + { + TransformedItemContainer tic when transformed.IndexOf(tic) is int i and (>= 0) => i, + _ => throw new UnspecifiedIndexException($"Cannot find index of {change.Current}"), + }; + } + if (_transformOnRefresh) { - Optional previous = transformed[change.CurrentIndex].Destination; - transformed[change.CurrentIndex] = _containerFactory(change.Current, previous, change.CurrentIndex); + Optional previous = transformed[index].Destination; + transformed[index] = _containerFactory(change.Current, previous, index); } else { - transformed.RefreshAt(change.CurrentIndex); + transformed.RefreshAt(index); } break; @@ -98,15 +110,31 @@ private void Transform(ChangeAwareList transformed, IC case ListChangeReason.Replace: { var change = item.Item; - Optional previous = transformed[change.PreviousIndex].Destination; - if (change.CurrentIndex == change.PreviousIndex) + + if (change.CurrentIndex == -1 || change.PreviousIndex == -1) { - transformed[change.CurrentIndex] = _containerFactory(change.Current, previous, change.CurrentIndex); + // Find the original, with it's corresponding index + var previous = transformed.First(x => x.Source.Equals(change.Previous.Value)); + var index = transformed.IndexOf(previous); + if (index == -1) + { + throw new UnspecifiedIndexException($"Cannot find index of {change.Previous.Value}"); + } + + transformed[index] = _containerFactory(change.Current, previous.Destination, index); } else { - transformed.RemoveAt(change.PreviousIndex); - transformed.Insert(change.CurrentIndex, _containerFactory(change.Current, Optional.None, change.CurrentIndex)); + Optional previous = transformed[change.PreviousIndex].Destination; + if (change.CurrentIndex == change.PreviousIndex) + { + transformed[change.CurrentIndex] = _containerFactory(change.Current, previous, change.CurrentIndex); + } + else + { + transformed.RemoveAt(change.PreviousIndex); + transformed.Insert(change.CurrentIndex, _containerFactory(change.Current, Optional.None, change.CurrentIndex)); + } } break; diff --git a/src/DynamicData/List/ObservableListEx.cs b/src/DynamicData/List/ObservableListEx.cs index ce90a9154..38aecd1d5 100644 --- a/src/DynamicData/List/ObservableListEx.cs +++ b/src/DynamicData/List/ObservableListEx.cs @@ -1543,19 +1543,7 @@ public static IObservable> SubscribeMany(this IObservableThe source observable change set. /// An observable which emits the change set. public static IObservable> SuppressRefresh(this IObservable> source) - where T : notnull - { - if (source == null) - { - throw new ArgumentNullException(nameof(source)); - } - - return source.Select(changes => - { - var filtered = changes.Where(c => c.Reason != ListChangeReason.Refresh); - return new ChangeSet(filtered); - }); - } + where T : notnull => source.WhereReasonsAreNot(ListChangeReason.Refresh); /// /// Transforms an observable sequence of observable lists into a single sequence @@ -2300,6 +2288,16 @@ public static IObservable> WhereReasonsAreNot(this IObservable< throw new ArgumentException("Must enter at least 1 reason", nameof(reasons)); } + if (reasons.Length == 1 && reasons[0] == ListChangeReason.Refresh) + { + // If only refresh changes are removed, then there's no need to remove the indexes + return source.Select(changes => + { + var filtered = changes.Where(c => c.Reason != ListChangeReason.Refresh); + return new ChangeSet(filtered); + }).NotEmpty(); + } + var matches = new HashSet(reasons); return source.Select( updates => diff --git a/version.json b/version.json index ed626a73f..be8018252 100644 --- a/version.json +++ b/version.json @@ -1,5 +1,5 @@ { - "version": "8.2", + "version": "8.3", "publicReleaseRefSpec": [ "^refs/heads/main$", // we release out of master "^refs/heads/preview/.*", // we release previews