Skip to content

Fixed bogus lookup logic within list-land .TransformAsync(),… #1171

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Merged
JakenVeina merged 12 commits into from
Oct 13, 2026
Merged

Fixed bogus lookup logic within list-land .TransformAsync(),… #1171

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
18b0f53
Fixed bogus lookup logic within list-land `.TransformAsync()`, for pr…
JakenVeina Sep 1, 2026
f13952f
Merge branch 'main' into issues/1169
JakenVeina Sep 28, 2026
403d0ec
Merge branch 'main' into issues/1169
JakenVeina Sep 28, 2026
06d2dbe
Merge branch 'main' into issues/1169
JakenVeina Sep 29, 2026
35551ba
Merge branch 'main' into issues/1169
JakenVeina Sep 30, 2026
1dc7af0
Merge branch 'main' into issues/1169
JakenVeina Oct 4, 2026
c341cfa
Merge branch 'main' into issues/1169
JakenVeina Oct 5, 2026
101fb75
Merge branch 'main' into issues/1169
JakenVeina Oct 5, 2026
deddae5
Merge branch 'main' into issues/1169
JakenVeina Oct 10, 2026
4d81cb1
Added equivalent fixes (and tests) to `.Transform()`, and consolidate…
JakenVeina Oct 10, 2026
cd3a996
Merge branch 'main' into issues/1169
JakenVeina Oct 11, 2026
387fc12
Review fixes.
JakenVeina Oct 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 65 additions & 3 deletions src/DynamicData.Tests/List/TransformAsyncFixture.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
using System;
using System.Linq;
using System.Threading;
using System.Reactive.Linq;
using System.Threading.Tasks;
using DynamicData.Kernel;
using DynamicData.Tests.Domain;

using FluentAssertions;
using Xunit;

using DynamicData.Tests.Domain;
using DynamicData.Tests.Utilities;

namespace DynamicData.Tests.List;

[Obsolete("Not obsolete - test commented out due to test run freezing on Appveyor")]
Expand Down Expand Up @@ -187,4 +189,64 @@ public void TransformAsyncCancelsTokenOnUnSubscribe()
sub.Dispose();
Assert.True(tcs.Task.IsCanceled);
}

// Covers https://github.com/reactivemarbles/DynamicData/issues/1169
[Fact]
public void RemoveWithoutIndex()
{
var source = Enumerable.Empty<IChangeSet<int>>()
.Append(new ChangeSet<int>()
{
new Change<int>(
reason: ListChangeReason.AddRange,
items: new[] { 1, 2, 3, 2, 1 }) // Include a duplicate of the item to be removed
})
.Append(new ChangeSet<int>()
{
new Change<int>(
reason: ListChangeReason.Remove,
current: 2)
})
.ToObservable();

using var subscription = source
.TransformAsync(static item => Task.FromResult(item.ToString()))
.ValidateChangeSets()
.RecordListItems(out var results);

results.Error.Should().BeNull("no errors should have occurred");
results.RecordedChangeSets.Count.Should().Be(2, "2 source operations were performed");
results.RecordedItems.Should().BeEquivalentTo(new[] { "1", "3", "2", "1" }, static options => options.WithStrictOrdering(), "The first occurrence of the given item should have been removed");
results.HasCompleted.Should().BeTrue("the source, and all asynchronous operations, have completed");
}

// Covers https://github.com/reactivemarbles/DynamicData/issues/1169
[Fact]
public void RemoveRangeWithoutIndex()
{
var source = Enumerable.Empty<IChangeSet<int>>()
.Append(new ChangeSet<int>()
{
new Change<int>(
reason: ListChangeReason.AddRange,
items: new[] { 1, 2, 3, 4, 5, 4, 3, 2, 1 }) // Include duplicates of the items to be removed.
})
.Append(new ChangeSet<int>()
{
new Change<int>(
reason: ListChangeReason.RemoveRange,
items: new[] { 1, 3, 5 })
})
.ToObservable();

using var subscription = source
.TransformAsync(static item => Task.FromResult(item.ToString()))
.ValidateChangeSets()
.RecordListItems(out var results);

results.Error.Should().BeNull("no errors should have occurred");
results.RecordedChangeSets.Count.Should().Be(2, "2 source operations were performed");
results.RecordedItems.Should().BeEquivalentTo(new[] { "2", "4", "4", "3", "2", "1" }, static options => options.WithStrictOrdering(), "The first occurrence of each odd-numbered item in the list should have been removed");
results.HasCompleted.Should().BeTrue("the source, and all asynchronous operations, have completed");
}
}
64 changes: 63 additions & 1 deletion src/DynamicData.Tests/List/TransformFixture.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
using System;
using System.Linq;
using System.Reactive.Linq;

using DynamicData.Tests.Domain;
using DynamicData.Tests.Utilities;

using FluentAssertions;
using Xunit;
Expand Down Expand Up @@ -88,7 +90,7 @@ public void Remove()
}

[Fact]
public void RemoveWithoutIndex()
public void RemoveObjectWithoutIndex()
{
const string key = "Adult1";
var person = new Person(key, 50);
Expand All @@ -104,6 +106,66 @@ public void RemoveWithoutIndex()
results.Data.Count.Should().Be(0, "Should be nothing cached");
}

// Mirrors coverage for https://github.com/reactivemarbles/DynamicData/issues/1169
[Fact]
public void RemoveRangeWithoutIndex()
{
var source = Enumerable.Empty<IChangeSet<int>>()
.Append(new ChangeSet<int>()
{
new Change<int>(
reason: ListChangeReason.AddRange,
items: new[] { 1, 2, 3, 4, 5, 4, 3, 2, 1 })
})
.Append(new ChangeSet<int>()
{
new Change<int>(
reason: ListChangeReason.RemoveRange,
items: new[] { 1, 3, 5 })
})
.ToObservable();

using var subscription = source
.Transform(static item => item.ToString())
.ValidateChangeSets()
.RecordListItems(out var results);

results.Error.Should().BeNull("no errors should have occurred");
results.RecordedChangeSets.Count.Should().Be(2, "2 source operations were performed");
results.RecordedItems.Should().BeEquivalentTo(new[] { "2", "4", "4", "3", "2", "1" }, static options => options.WithStrictOrdering(), "The first occurrence of each odd-numbered item in the list should have been removed");
results.HasCompleted.Should().BeTrue("the source, and all asynchronous operations, have completed");
}

// Mirrors coverage for https://github.com/reactivemarbles/DynamicData/issues/1169

@dwcullop dwcullop Oct 12, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we add a duplicate-values case (e.g. [1,2,1,2] with an unindexed RemoveRange of [1,2]) and an unindexed Replace for both operators? The duplicates test would have caught the extra loop in the async version. Also, small thing: the // Covers / // Mirrors coverage comments and the trailing whitespace are still there.

@JakenVeina JakenVeina Oct 13, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just added some duplicate items to the existing tests. It's not like we lose any coverage by dropping the no-duplicates scenario.

Also, small thing: the // Covers / // Mirrors coverage comments and the trailing whitespace are still there.

Uhhh, yeah, why wouldn't they be? And what trailing whitespace?

[Fact]
public void RemoveValueWithoutIndex()
{
var source = Enumerable.Empty<IChangeSet<int>>()
.Append(new ChangeSet<int>()
{
new Change<int>(
reason: ListChangeReason.AddRange,
items: new[] { 1, 2, 3, 2, 1 })
})
.Append(new ChangeSet<int>()
{
new Change<int>(
reason: ListChangeReason.Remove,
current: 2)
})
.ToObservable();

using var subscription = source
.Transform(static item => item.ToString())
.ValidateChangeSets()
.RecordListItems(out var results);

results.Error.Should().BeNull("no errors should have occurred");
results.RecordedChangeSets.Count.Should().Be(2, "2 source operations were performed");
results.RecordedItems.Should().BeEquivalentTo(new[] { "1", "3", "2", "1" }, static options => options.WithStrictOrdering(), "The first occurrence of the given item should have been removed");
results.HasCompleted.Should().BeTrue("the source, and all asynchronous operations, have completed");
}

[Fact]
public void SameKeyChanges()
{
Expand Down
61 changes: 38 additions & 23 deletions src/DynamicData/List/Internal/TransformAsync.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,16 +134,24 @@ await _containerFactory(
case ListChangeReason.Refresh:
{
var change = item.Item;
var index = change.CurrentIndex;

if (index < 0)
{
index = transformed.FindItemIndex(change.Current);
if (index < 0)
break;
}

if (_transformOnRefresh)
{
Optional<TDestination> previous = transformed[change.CurrentIndex].Destination;
var container = await _containerFactory(change.Current, previous, change.CurrentIndex, cancel)
var container = await _containerFactory(change.Current, transformed[index].Destination, index, cancel)
.ConfigureAwait(false);
transformed[change.CurrentIndex] = container;
transformed[index] = container;
}
else
{
transformed.RefreshAt(change.CurrentIndex);
transformed.RefreshAt(index);
}

break;
Expand All @@ -152,16 +160,23 @@ await _containerFactory(
case ListChangeReason.Replace:
{
var change = item.Item;
var currentIndex = change.CurrentIndex;
var previousIndex = change.PreviousIndex;

if (previousIndex < 0)
previousIndex = transformed.FindItemIndex(change.Previous.Value);

@dwcullop dwcullop Oct 12, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the previous item isn't found, this falls through to transformed[-1] and throws ArgumentOutOfRangeException. Sync throws UnspecifiedIndexException here. Can we throw the same thing so the two match?

@JakenVeina JakenVeina Oct 13, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per my initial comment, I left in all the existing discrepancies in failed lookup behavior intact. They're not part of #1169, and and how an operator responds to being given corrupt changesets is really undefined behavior. I'm down with cleaning it up, but not here.


Optional<TDestination> previous = transformed[change.PreviousIndex].Destination;
if (change.CurrentIndex == change.PreviousIndex)
if (currentIndex < 0)
currentIndex = previousIndex;

if (currentIndex == previousIndex)
{
transformed[change.CurrentIndex] = await _containerFactory(change.Current, previous, change.CurrentIndex, cancel);
transformed[currentIndex] = await _containerFactory(change.Current, transformed[previousIndex].Destination, currentIndex, cancel);
}
else
{
transformed.RemoveAt(change.PreviousIndex);
transformed.Insert(change.CurrentIndex, await _containerFactory(change.Current, Optional<TDestination>.None, change.CurrentIndex, cancel));
transformed.RemoveAt(previousIndex);
transformed.Insert(currentIndex, await _containerFactory(change.Current, Optional<TDestination>.None, currentIndex, cancel));
}

break;
Expand All @@ -170,22 +185,16 @@ await _containerFactory(
case ListChangeReason.Remove:
{
var change = item.Item;
var hasIndex = change.CurrentIndex >= 0;
var index = change.CurrentIndex;

if (hasIndex)
if (index < 0)
{
transformed.RemoveAt(item.Item.CurrentIndex);
}
else
{
var toRemove = transformed.FirstOrDefault(t => ReferenceEquals(t.Source, t));

if (toRemove is not null)
{
transformed.Remove(toRemove);
}
index = transformed.FindItemIndex(change.Current);
if (index < 0)
break;
}

transformed.RemoveAt(index);
break;
}

Expand All @@ -197,8 +206,14 @@ await _containerFactory(
}
else
{
var toRemove = transformed.Where(t => ReferenceEquals(t.Source, t)).ToArray();
transformed.RemoveMany(toRemove);
foreach (var current in item.Range)
{
var index = transformed.FindItemIndex(current);
if (index < 0)
continue;

transformed.RemoveAt(index);
}
}

break;
Expand Down
Loading
Loading