From 4987f4b2868d0a100f28b63757e22aae5f2ca743 Mon Sep 17 00:00:00 2001 From: Chris R Date: Tue, 13 Jul 2021 12:16:11 -0700 Subject: [PATCH 1/3] Fix AdaptiveCapacityDictionary assert with 11 elements #34307 --- .../src/Internal/RequestCookieCollection.cs | 4 +++- .../test/RequestCookiesCollectionTests.cs | 8 ++++++++ .../Dictionary/AdaptiveCapacityDictionary.cs | 2 +- .../AdaptiveCapacityDictionaryTests.cs | 20 +++++++++++++++++++ 4 files changed, 32 insertions(+), 2 deletions(-) diff --git a/src/Http/Http/src/Internal/RequestCookieCollection.cs b/src/Http/Http/src/Internal/RequestCookieCollection.cs index e3554d5a711b..feda7c2afb52 100644 --- a/src/Http/Http/src/Internal/RequestCookieCollection.cs +++ b/src/Http/Http/src/Internal/RequestCookieCollection.cs @@ -70,7 +70,9 @@ internal static RequestCookieCollection ParseInternal(StringValues values, bool { return Empty; } - var collection = new RequestCookieCollection(values.Count); + + // Do not set the collection capacity based on StringValues.Count, the Cookie header is supposed to be a single combine value. + var collection = new RequestCookieCollection(); var store = collection.Store!; if (CookieHeaderParserShared.TryParseValues(values, store, enableCookieNameEncoding, supportsMultipleValues: true)) diff --git a/src/Http/Http/test/RequestCookiesCollectionTests.cs b/src/Http/Http/test/RequestCookiesCollectionTests.cs index 8d3dad77f620..1e886bc2f3cf 100644 --- a/src/Http/Http/test/RequestCookiesCollectionTests.cs +++ b/src/Http/Http/test/RequestCookiesCollectionTests.cs @@ -38,5 +38,13 @@ public void AppContextSwitchUnEscapesKeysAndValues(string input, string expected Assert.Equal(expectedKey, cookies.Keys.Single()); Assert.Equal(expectedValue, cookies[expectedKey]); } + + [Fact] + public void ParseManyCookies() + { + var cookies = RequestCookieCollection.Parse(new StringValues(new[] { "a=a", "b=b", "c=c", "d=d", "e=e", "f=f", "g=g", "h=h", "i=i", "j=j", "k=k", "l=l" })); + + Assert.Equal(12, cookies.Count); + } } } diff --git a/src/Shared/Dictionary/AdaptiveCapacityDictionary.cs b/src/Shared/Dictionary/AdaptiveCapacityDictionary.cs index 910e3e9244da..414ea7660fc4 100644 --- a/src/Shared/Dictionary/AdaptiveCapacityDictionary.cs +++ b/src/Shared/Dictionary/AdaptiveCapacityDictionary.cs @@ -77,7 +77,6 @@ public AdaptiveCapacityDictionary(int capacity, IEqualityComparer comparer else { _dictionaryStorage = new Dictionary(capacity); - _arrayStorage = Array.Empty>(); } } @@ -95,6 +94,7 @@ internal AdaptiveCapacityDictionary(IEnumerable> valu { _comparer = comparer ?? EqualityComparer.Default; + Debug.Assert(capacity <= DefaultArrayThreshold); _arrayStorage = new KeyValuePair[capacity]; foreach (var kvp in values) diff --git a/src/Shared/test/Shared.Tests/AdaptiveCapacityDictionaryTests.cs b/src/Shared/test/Shared.Tests/AdaptiveCapacityDictionaryTests.cs index b31ae8e14928..8208952c63cd 100644 --- a/src/Shared/test/Shared.Tests/AdaptiveCapacityDictionaryTests.cs +++ b/src/Shared/test/Shared.Tests/AdaptiveCapacityDictionaryTests.cs @@ -3,6 +3,7 @@ using System; using System.Collections.Generic; +using System.Globalization; using System.Linq; using Microsoft.AspNetCore.Internal; using Microsoft.AspNetCore.Testing; @@ -100,6 +101,25 @@ public void CreateFromIEnumerableStringValuePair_CopiesValues() kvp => { Assert.Equal("Middle Name", kvp.Key); Assert.Equal("Bob", kvp.Value); }); } + [Fact] + public void CreateWithCapacityOverDefaultLimit() + { + // The default threshold between array and dictionary is 10. If we created one over that limit it should go directly to a dictionary. + var dict = new AdaptiveCapacityDictionary(capacity: 12, StringComparer.OrdinalIgnoreCase); + + Assert.Null(dict._arrayStorage); + Assert.NotNull(dict._dictionaryStorage); + + for (var i = 0; i < 12; i++) + { + dict[i.ToString(CultureInfo.InvariantCulture)] = i.ToString(CultureInfo.InvariantCulture); + } + + Assert.Null(dict._arrayStorage); + Assert.NotNull(dict._dictionaryStorage); + Assert.Equal(12, dict.Count); + } + [Fact] public void CreateFromIEnumerableKeyValuePair_ThrowsExceptionForDuplicateKey() { From 41255197fedbef6f3a16be948c9c86cad673c90e Mon Sep 17 00:00:00 2001 From: Chris R Date: Tue, 13 Jul 2021 13:15:36 -0700 Subject: [PATCH 2/3] Remove test extra constructor --- .../AdaptiveCapacityDictionaryBenchmark.cs | 6 +- .../Dictionary/AdaptiveCapacityDictionary.cs | 23 ------ .../AdaptiveCapacityDictionaryTests.cs | 79 ------------------- 3 files changed, 5 insertions(+), 103 deletions(-) diff --git a/src/Http/Http/perf/Microbenchmarks/AdaptiveCapacityDictionaryBenchmark.cs b/src/Http/Http/perf/Microbenchmarks/AdaptiveCapacityDictionaryBenchmark.cs index 40684acda48f..fd7a2e06db2a 100644 --- a/src/Http/Http/perf/Microbenchmarks/AdaptiveCapacityDictionaryBenchmark.cs +++ b/src/Http/Http/perf/Microbenchmarks/AdaptiveCapacityDictionaryBenchmark.cs @@ -40,7 +40,11 @@ public void Setup() _smallCapDict = new AdaptiveCapacityDictionary(capacity: 1, StringComparer.OrdinalIgnoreCase); _smallCapDictTen = new AdaptiveCapacityDictionary(capacity: 10, StringComparer.OrdinalIgnoreCase); - _filledSmallDictionary = new AdaptiveCapacityDictionary(_tenValues, capacity: 10, StringComparer.OrdinalIgnoreCase); + _filledSmallDictionary = new AdaptiveCapacityDictionary(capacity: 10, StringComparer.OrdinalIgnoreCase); + foreach (var a in _tenValues) + { + _filledSmallDictionary[a.Key] = a.Value; + } _dict = new Dictionary(1, StringComparer.OrdinalIgnoreCase); _dictTen = new Dictionary(10, StringComparer.OrdinalIgnoreCase); diff --git a/src/Shared/Dictionary/AdaptiveCapacityDictionary.cs b/src/Shared/Dictionary/AdaptiveCapacityDictionary.cs index 414ea7660fc4..d39f2647a9ff 100644 --- a/src/Shared/Dictionary/AdaptiveCapacityDictionary.cs +++ b/src/Shared/Dictionary/AdaptiveCapacityDictionary.cs @@ -80,29 +80,6 @@ public AdaptiveCapacityDictionary(int capacity, IEqualityComparer comparer } } - /// - /// Creates a initialized with the specified . - /// - /// An object to initialize the dictionary. The value can be of type - /// or - /// or an object with public properties as key-value pairs. - /// - /// This constructor is unoptimized and primarily used for tests. - /// Equality comparison. - /// Initial capacity. - internal AdaptiveCapacityDictionary(IEnumerable> values, int capacity, IEqualityComparer comparer) - { - _comparer = comparer ?? EqualityComparer.Default; - - Debug.Assert(capacity <= DefaultArrayThreshold); - _arrayStorage = new KeyValuePair[capacity]; - - foreach (var kvp in values) - { - Add(kvp.Key, kvp.Value); - } - } - /// /// Creates a initialized with the specified . /// diff --git a/src/Shared/test/Shared.Tests/AdaptiveCapacityDictionaryTests.cs b/src/Shared/test/Shared.Tests/AdaptiveCapacityDictionaryTests.cs index 8208952c63cd..e8be275fe43d 100644 --- a/src/Shared/test/Shared.Tests/AdaptiveCapacityDictionaryTests.cs +++ b/src/Shared/test/Shared.Tests/AdaptiveCapacityDictionaryTests.cs @@ -39,68 +39,6 @@ public void CreateFromNull() Assert.Null(dict._dictionaryStorage); } - public static KeyValuePair[] IEnumerableKeyValuePairData - { - get - { - return new[] - { - new KeyValuePair("Name", "James"), - new KeyValuePair("Age", 30), - new KeyValuePair("Address", new Address() { City = "Redmond", State = "WA" }) - }; - } - } - - public static KeyValuePair[] IEnumerableStringValuePairData - { - get - { - return new[] - { - new KeyValuePair("First Name", "James"), - new KeyValuePair("Last Name", "Henrik"), - new KeyValuePair("Middle Name", "Bob") - }; - } - } - - [Fact] - public void CreateFromIEnumerableKeyValuePair_CopiesValues() - { - // Arrange & Act - var dict = new AdaptiveCapacityDictionary(IEnumerableKeyValuePairData, capacity: IEnumerableKeyValuePairData.Length, EqualityComparer.Default); - - // Assert - Assert.IsType[]>(dict._arrayStorage); - Assert.Collection( - dict.OrderBy(kvp => kvp.Key), - kvp => - { - Assert.Equal("Address", kvp.Key); - var address = Assert.IsType
(kvp.Value); - Assert.Equal("Redmond", address.City); - Assert.Equal("WA", address.State); - }, - kvp => { Assert.Equal("Age", kvp.Key); Assert.Equal(30, kvp.Value); }, - kvp => { Assert.Equal("Name", kvp.Key); Assert.Equal("James", kvp.Value); }); - } - - [Fact] - public void CreateFromIEnumerableStringValuePair_CopiesValues() - { - // Arrange & Act - var dict = new AdaptiveCapacityDictionary(IEnumerableStringValuePairData, capacity: 3, StringComparer.OrdinalIgnoreCase); - - // Assert - Assert.IsType[]>(dict._arrayStorage); - Assert.Collection( - dict.OrderBy(kvp => kvp.Key), - kvp => { Assert.Equal("First Name", kvp.Key); Assert.Equal("James", kvp.Value); }, - kvp => { Assert.Equal("Last Name", kvp.Key); Assert.Equal("Henrik", kvp.Value); }, - kvp => { Assert.Equal("Middle Name", kvp.Key); Assert.Equal("Bob", kvp.Value); }); - } - [Fact] public void CreateWithCapacityOverDefaultLimit() { @@ -134,23 +72,6 @@ public void CreateFromIEnumerableKeyValuePair_ThrowsExceptionForDuplicateKey() $"An element with the key 'Name' already exists in the {nameof(AdaptiveCapacityDictionary)}."); } - [Fact] - public void CreateFromIEnumerableStringValuePair_ThrowsExceptionForDuplicateKey() - { - // Arrange - var values = new List>() - { - new KeyValuePair("name", "Billy"), - new KeyValuePair("Name", "Joey"), - }; - - // Act & Assert - ExceptionAssert.ThrowsArgument( - () => new AdaptiveCapacityDictionary(values, capacity: 3, StringComparer.OrdinalIgnoreCase), - "key", - $"An element with the key 'Name' already exists in the {nameof(AdaptiveCapacityDictionary)}."); - } - [Fact] public void Comparer_IsOrdinalIgnoreCase() { From fac713dafbb02af07b4375bb61df81f5cc9fabfc Mon Sep 17 00:00:00 2001 From: Chris R Date: Tue, 13 Jul 2021 13:16:26 -0700 Subject: [PATCH 3/3] Spelling --- src/Http/Http/src/Internal/RequestCookieCollection.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Http/Http/src/Internal/RequestCookieCollection.cs b/src/Http/Http/src/Internal/RequestCookieCollection.cs index feda7c2afb52..b9036c5572da 100644 --- a/src/Http/Http/src/Internal/RequestCookieCollection.cs +++ b/src/Http/Http/src/Internal/RequestCookieCollection.cs @@ -71,7 +71,7 @@ internal static RequestCookieCollection ParseInternal(StringValues values, bool return Empty; } - // Do not set the collection capacity based on StringValues.Count, the Cookie header is supposed to be a single combine value. + // Do not set the collection capacity based on StringValues.Count, the Cookie header is supposed to be a single combined value. var collection = new RequestCookieCollection(); var store = collection.Store!;