Fix behavior ObjectCollection for single item contains (#59547)

* Fix behavior ObjectCollection for single item contains * Address PR feedback
dotnet · Sep 24, 2021 · 49f22ce · 49f22ce
1 parent 2e08554
commit 49f22ce
Show file tree

Hide file tree

Showing 2 changed files with 77 additions and 9 deletions.
diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/ObjectCollection.cs b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/ObjectCollection.cs
@@ -94,8 +94,9 @@ public void Clear()
         }
 
         public bool Contains(T item) =>
-            ReferenceEquals(item, _items) ||
-            (_size != 0 && _items is T[] items && Array.IndexOf(items, item, 0, _size) != -1);
+            _size <= 0 ? false :
+            _items is T o ? o.Equals(item) :
+            _items is T[] items && Array.IndexOf(items, item, 0, _size) != -1;
 
         public void CopyTo(T[] array, int arrayIndex)
         {
@@ -120,14 +121,16 @@ public void CopyTo(T[] array, int arrayIndex)
 
         public bool Remove(T item)
         {
-            if (ReferenceEquals(_items, item))
+            if (_items is T o)
             {
-                _items = null;
-                _size = 0;
-                return true;
+                if (o.Equals(item))
+                {
+                    _items = null;
+                    _size = 0;
+                    return true;
+                }
             }
-
-            if (_items is T[] items)
+            else if (_items is T[] items)
             {
                 int index = Array.IndexOf(items, item, 0, _size);
                 if (index != -1)

diff --git a/src/libraries/System.Net.Http/tests/UnitTests/Headers/ObjectCollectionTest.cs b/src/libraries/System.Net.Http/tests/UnitTests/Headers/ObjectCollectionTest.cs
@@ -36,9 +36,74 @@ public void Ctor_ExecuteBothOverloads_MatchExpectation()
             c.Add("value1");
 
             Assert.Throws<InvalidOperationException>(() => { c.Add(null); });
+        }
+
+        [Fact]
+        public void ContainsAndRemove_UsesEqualitySemantics()
+        {
+            ObjectCollection<string> c = new ObjectCollection<string>();
 
+            string val1 = "value1";
+            string val1DifferentReference = "value" + 1;
+            Assert.NotSame(val1, val1DifferentReference);
+            Assert.Equal(val1, val1DifferentReference);
+
+            string val2 = "value2";
+            string val2DifferentReference = "value" + 2;
+            Assert.NotSame(val2, val2DifferentReference);
+            Assert.Equal(val2, val2DifferentReference);
+
+            string val3 = "value3";
+
+            // Start empty
+            Assert.Equal(0, c.Count);
+            Assert.False(c.Contains(val1));
+
+            // Single item
+            c.Add(val1);
             Assert.Equal(1, c.Count);
-            Assert.True(c.Contains("value1"));
+            Assert.True(c.Contains(val1));
+            Assert.True(c.Contains(val1DifferentReference));
+            Assert.False(c.Contains(val2));
+
+            // Single item removal
+            Assert.True(c.Remove(val1));
+            Assert.Equal(0, c.Count);
+            Assert.False(c.Contains(val1));
+
+            // Multi-value
+            c.Add(val1);
+            c.Add(val2);
+            Assert.Equal(2, c.Count);
+            Assert.True(c.Contains(val1));
+            Assert.True(c.Contains(val1DifferentReference));
+            Assert.True(c.Contains(val2));
+            Assert.True(c.Contains(val1DifferentReference));
+            Assert.False(c.Contains(val3));
+
+            // Removal when multiple exist, using different reference.
+            Assert.True(c.Remove(val1));
+            Assert.False(c.Contains(val1));
+            Assert.True(c.Contains(val2));
+            Assert.Equal(1, c.Count);
+
+            // Removal of non-existent
+            Assert.False(c.Remove(val3));
+            Assert.False(c.Remove(val1DifferentReference));
+            Assert.Equal(1, c.Count);
+            Assert.True(c.Contains(val2DifferentReference));
+
+            // Removal last item
+            Assert.True(c.Remove(val2DifferentReference));
+            Assert.Equal(0, c.Count);
+            Assert.False(c.Contains(val2));
+            Assert.False(c.Contains(val1));
+
+            // Remove from empty
+            Assert.False(c.Remove(val1));
+            Assert.False(c.Remove(val2));
+            Assert.False(c.Remove(val3));
+            Assert.Equal(0, c.Count);
         }
     }
 }