From d71f44276af2b6d0bc427f6c1bc7368c0769bbf2 Mon Sep 17 00:00:00 2001 From: Juan Sebastian Hoyos Ayala Date: Thu, 23 Sep 2021 16:13:40 -0700 Subject: [PATCH 1/2] Fix behavior ObjectCollection for single item contains --- .../Net/Http/Headers/ObjectCollection.cs | 7 +++--- .../UnitTests/Headers/ObjectCollectionTest.cs | 22 ++++++++++++++++++- 2 files changed, 25 insertions(+), 4 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 index e7b3c4e952238..f9082fa61b7cf 100644 --- 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 || _items is null ? 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,7 +121,7 @@ public void CopyTo(T[] array, int arrayIndex) public bool Remove(T item) { - if (ReferenceEquals(_items, item)) + if (_items is T o && o.Equals(item)) { _items = null; _size = 0; diff --git a/src/libraries/System.Net.Http/tests/UnitTests/Headers/ObjectCollectionTest.cs b/src/libraries/System.Net.Http/tests/UnitTests/Headers/ObjectCollectionTest.cs index d8ffd2c4ee1c7..c9ec54b555b89 100644 --- 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,29 @@ public void Ctor_ExecuteBothOverloads_MatchExpectation() c.Add("value1"); Assert.Throws(() => { c.Add(null); }); + } + + [Fact] + public void ContainsAndRemove_UsesEqualitySemantics() + { + // Use default validator + ObjectCollection c = new ObjectCollection(); + + Assert.Equal(0, c.Count); + Assert.False(c.Contains("value" + 1)); + + c.Add("value" + 1); Assert.Equal(1, c.Count); - Assert.True(c.Contains("value1")); + // Force the reference to be different to ensure we are checking for semantic equality + // and not reference equality. + Assert.True(c.Contains("value" + 1)); + c.Add("value" + 2); + + Assert.True(c.Remove("value" + 1)); + Assert.Equal(1, c.Count); + + Assert.True(c.Contains("value" + 2)); } } } From 38e9690102620ead989b1cfe950b820ddc340823 Mon Sep 17 00:00:00 2001 From: Juan Sebastian Hoyos Ayala Date: Fri, 24 Sep 2021 10:16:05 -0700 Subject: [PATCH 2/2] Address PR feedback --- .../Net/Http/Headers/ObjectCollection.cs | 16 ++--- .../UnitTests/Headers/ObjectCollectionTest.cs | 63 ++++++++++++++++--- 2 files changed, 63 insertions(+), 16 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 index f9082fa61b7cf..2486c55638697 100644 --- 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,7 +94,7 @@ public void Clear() } public bool Contains(T item) => - _size == 0 || _items is null ? false : + _size <= 0 ? false : _items is T o ? o.Equals(item) : _items is T[] items && Array.IndexOf(items, item, 0, _size) != -1; @@ -121,14 +121,16 @@ public void CopyTo(T[] array, int arrayIndex) public bool Remove(T item) { - if (_items is T o && o.Equals(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 index c9ec54b555b89..3a8f69d9a4438 100644 --- a/src/libraries/System.Net.Http/tests/UnitTests/Headers/ObjectCollectionTest.cs +++ b/src/libraries/System.Net.Http/tests/UnitTests/Headers/ObjectCollectionTest.cs @@ -41,24 +41,69 @@ public void Ctor_ExecuteBothOverloads_MatchExpectation() [Fact] public void ContainsAndRemove_UsesEqualitySemantics() { - // Use default validator ObjectCollection c = new ObjectCollection(); + 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(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("value" + 1)); + Assert.False(c.Contains(val1)); - c.Add("value" + 1); + // 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); - // Force the reference to be different to ensure we are checking for semantic equality - // and not reference equality. - Assert.True(c.Contains("value" + 1)); - c.Add("value" + 2); - Assert.True(c.Remove("value" + 1)); + // Removal of non-existent + Assert.False(c.Remove(val3)); + Assert.False(c.Remove(val1DifferentReference)); Assert.Equal(1, c.Count); + Assert.True(c.Contains(val2DifferentReference)); - Assert.True(c.Contains("value" + 2)); + // 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); } } }