Items added by ConcurrentObservableDictionary.AddRange() cannot be removed

[TGirlChu]

While item added to ConcurrentObservableDictionary by Add can be removed by Remove/RemoveRange, those added by AddRange cannot.

I suspect the cause resides in ConcurrentObservableDictionary.AddRange. Due to the fact that the enumeration of nodes is deferred, two different set of identical ObservableDictionaryNode are created, one by Dictionary.AddRange(dictionaryEntries) and the other List.AddRange(nodes).

 public ImmutableDictionaryListPair<TKey, TValue> AddRange(IEnumerable<KeyValuePair<TKey, TValue>> pairs)
  {
      var endNode = List.Any() ? List[List.Count - 1] : null;

      // Create the list of nodes for the internal list
      var nodes = pairs.SelectWithPreviousResult(
        firstItem => new ObservableDictionaryNode<TKey, TValue>(firstItem, endNode),
        (previousNode, pair) => new ObservableDictionaryNode<TKey, TValue>(pair, previousNode));

      // create the key/value pairs for the internal dictionary
      var dictionaryEntries = nodes.Select(node => KeyValuePair.Create(node.Key, node));

      // create the new immutable Dictionary/List pair
      return new ImmutableDictionaryListPair<TKey, TValue>(Dictionary.AddRange(dictionaryEntries), List.AddRange(nodes));
  }

Since ObservableDictionaryNode is a reference type without Equals/GetHashCode overrides, items in Dictionary and those in List will not be considered equal, thus failing the equality check in ImmutableList.RemoveRange.

[stewienj]

Thanks for identifying this problem. Good work diagnosing the cause. This implementation of the ConcurrentObservableDictionary is really old too, for historical reasons (I started writing this in 2005) this class mimics the behavior of a list. I'll try to get a fix out soon when I get to my dev PC. For the next major version of this library I'm thinking about moving ConcurrentObservableDictionary to ConcurrentObservableDictionaryList and creating a new ConcurrentObservableDictionary based on ImmutableDictionary, similar to how the current ConcurrentObservableHashSet works, which is a much newer class.

[stewienj]

Remove() also fails if you just use Add(). I didn't pick this up because I'm using strings for keys everywhere in my unit tests, if I use classes instead then the unit tests fail. Damn....

[stewienj]

Scrub that last comment, I made an error in my unit test modifications

[stewienj]

Hi @TGirlChu I'm having trouble formulating a unit test that reproduces this issue. Would you be able to provide some sample code I can use to write a unit test. I've written the following 2 unit tests, but they are both passing, not failing as expected.

First unit test uses a class type for the Dictionary Key that doesn't override Equals() and GetHashCode()

[TestMethod]
public void AddRemoveRangeReferenceTypeTest()
{
    IEnumerable<KeyValuePair<KeyStringTest, string>> GetIEnumerable()
    {
        for (int i = 0; i < 10; ++i)
        {
            yield return new KeyValuePair<KeyStringTest, string>(new KeyStringTest(i.ToString()), i.ToString());
        }
    }

    // Need to convert to array so we have the same objects being used
    var itemsToAdd = GetIEnumerable().ToArray();
    var dictionary1 = new ConcurrentObservableDictionary<KeyStringTest, string>();
    dictionary1.AddRange(itemsToAdd);

    Assert.IsTrue(dictionary1.Count == itemsToAdd.Count(), "Count doesn't match number of items added");

    foreach (var item in itemsToAdd)
    {
        Assert.IsTrue(dictionary1.Remove(item.Key), "Problem removing item");
    }

    Assert.IsTrue(dictionary1.Count == 0, "Not all items removed");
}

The KeyStringTest class looks like this:

/// <summary>
/// A test class for creating objects to be used as Keys in Dictionary tests.
/// </summary>
public class KeyStringTest
{
    public KeyStringTest(string keyValue)
    {
        KeyValue = keyValue;
    }
    public string KeyValue { get; }
}

Second unit test uses a class type for the Dictionary Key that does override Equals() and GetHashCode()

[TestMethod]
public void AddRemoveRangeReferenceTypeWithOverrideTest()
{
    IEnumerable<KeyValuePair<KeyStringWithOverride, string>> GetIEnumerable()
    {
        for (int i = 0; i < 10; ++i)
        {
            yield return new KeyValuePair<KeyStringWithOverride, string>(new KeyStringWithOverride(i.ToString()), i.ToString());
        }
    }

    // Don't convert to array, testing the use of reference types that override Equals and GetHashCode
    var itemsToAdd = GetIEnumerable();
    var dictionary1 = new ConcurrentObservableDictionary<KeyStringWithOverride, string>();
    dictionary1.AddRange(itemsToAdd);

    Assert.IsTrue(dictionary1.Count == itemsToAdd.Count(), "Count doesn't match number of items added");

    foreach (var item in itemsToAdd)
    {
        Assert.IsTrue(dictionary1.Remove(item.Key), "Problem removing item");
    }

    Assert.IsTrue(dictionary1.Count == 0, "Not all items removed");
}

/// <summary>
/// A test class for creating objects to be used as Keys in Dictionary tests.
/// </summary>
public class KeyStringWithOverride
{
    public KeyStringWithOverride(string keyValue)
    {
        KeyValue = keyValue;
    }

    public override int GetHashCode()
    {
        return KeyValue.GetHashCode();
    }

    public override bool Equals(object obj)
    {
        if (obj is KeyStringWithOverride ex)
        {
            return KeyValue.Equals(ex.KeyValue);
        }
        else
        {
            return false;
        }

    }

    public string KeyValue { get; }
}

The above 2 test code blocks are now in the repo:

SwordfishCollections/ExamplesAndTests/Swordfish.NET.UnitTestV3/ConcurrentObservableDictionaryTests.cs

Line 593 in 147acaf

public void AddRemoveRangeReferenceTypeTest()

SwordfishCollections/ExamplesAndTests/Swordfish.NET.UnitTestV3/ConcurrentObservableDictionaryTests.cs

Line 619 in 147acaf

public void AddRemoveRangeReferenceTypeWithOverrideTest()

[TGirlChu]

Here is the test method I added in my own repo:

[TestMethod]
public void TestAddAndRemoveRange()
{
   var keyValuePairs = Enumerable.Range(0, 10)
       .Select(i => System.Collections.Generic.KeyValuePair.Create(new object(), i)).ToArray();

   var testCollection = new ConcurrentObservableDictionary<object, int>();
   testCollection.AddRange(keyValuePairs);
   testCollection.Should().BeEquivalentTo(keyValuePairs);
   testCollection.RemoveRange(keyValuePairs.Take(5).Select(pair => pair.Key));
   testCollection.Count.Should().Be(5);
   testCollection.Should().BeEquivalentTo(keyValuePairs.Skip(5));
}

This test case would pass if .ToArray() is appended to pairs.SelectWithPreviousResult(...) in ImmutableDictionaryListPair<TKey, TValue> AddRange().

[stewienj]

Thanks @TGirlChu, I reproduced this issue using the unit test you provided, I investigated it, tried a few solutions and ended up on the same solution you did. While working on this issue I also uplifted the source to to .NET Core 8.0 whilst maintaining support for netstandard2.0 and net48, I made a bunch of other small improvements, and I updated all the nuget packages. I'm now starting the automated publish process, which I haven't done in a while so hopefully it goes ok.

[stewienj]

New nuget package release is here https://www.nuget.org/packages/Swordfish.NET.CollectionsV3/3.3.13

[TGirlChu]

Thank you for your work!
.NET 8.0 support is welcome too :)