From 1f717f8253461ec255f49fd51882a623b1f17827 Mon Sep 17 00:00:00 2001 From: feiyun0112 Date: Sat, 12 Feb 2022 18:19:51 +0800 Subject: [PATCH 01/27] Do not reorder HTTP header values --- .../System/Net/Http/Headers/HttpHeaders.cs | 257 ++++++++++++------ .../UnitTests/Headers/HttpHeadersTest.cs | 18 +- .../Headers/HttpRequestHeadersTest.cs | 22 +- 3 files changed, 209 insertions(+), 88 deletions(-) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs index ee53b9e626c75..0ba63951534bb 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs @@ -66,15 +66,42 @@ internal void Add(HeaderDescriptor descriptor, string? value) // HeaderStoreItemInfo object and try to parse the value. If it works, we'll add the header. PrepareHeaderInfoForAdd(descriptor, out HeaderStoreItemInfo info, out bool addToStore); ParseAndAddValue(descriptor, info, value); - + // If we get here, then the value could be parsed correctly. If we created a new HeaderStoreItemInfo, add // it to the store if we added at least one value. - if (addToStore && (info.ParsedValue != null)) + if (addToStore && ExistsParsedValue(info)) { AddHeaderToStore(descriptor, info); } } + private static bool ExistsParsedValue(HeaderStoreItemInfo info) + { + foreach (var item in info.ParsedAndInvalidValues) + { + if (item.Type == HeaderStoreItemInfoValueType.Parsed) + { + return true; + } + } + + return false; + } + + + private static bool ExistsInvalidValue(HeaderStoreItemInfo info) + { + foreach (var item in info.ParsedAndInvalidValues) + { + if (item.Type == HeaderStoreItemInfoValueType.Invalid) + { + return true; + } + } + + return false; + } + public void Add(string name, IEnumerable values) => Add(GetHeaderDescriptor(name), values); internal void Add(HeaderDescriptor descriptor, IEnumerable values) @@ -102,7 +129,7 @@ internal void Add(HeaderDescriptor descriptor, IEnumerable values) // get added. Same here: If multiple values get added to a _new_ header, make sure the header gets added // with the valid values. // However, if all values for a _new_ header were invalid, then don't add the header. - if (addToStore && (info.ParsedValue != null)) + if (addToStore && ExistsParsedValue(info)) { AddHeaderToStore(descriptor, info); } @@ -358,10 +385,8 @@ internal void SetParsedValue(HeaderDescriptor descriptor, object value) // This method will first clear all values. This is used e.g. when setting the 'Date' or 'Host' header. // i.e. headers not supporting collections. - HeaderStoreItemInfo info = GetOrCreateHeaderInfo(descriptor, parseRawValues: true); - - info.InvalidValue = null; - info.ParsedValue = null; + HeaderStoreItemInfo info = GetOrCreateHeaderInfo(descriptor, parseRawValues: true); + info.ParsedAndInvalidValues = new List(); info.RawValue = null; AddParsedValue(info, value); @@ -401,7 +426,7 @@ internal bool RemoveParsedValue(HeaderDescriptor descriptor, object value) "This method should not be used for single-value headers. Use Remove(string) instead."); // If there is no entry, just return. - if (info.ParsedValue == null) + if (!ExistsParsedValue(info)) { return false; } @@ -409,42 +434,25 @@ internal bool RemoveParsedValue(HeaderDescriptor descriptor, object value) bool result = false; IEqualityComparer? comparer = descriptor.Parser.Comparer; - List? parsedValues = info.ParsedValue as List; - if (parsedValues == null) + foreach (var item in info.ParsedAndInvalidValues) { - Debug.Assert(info.ParsedValue.GetType() == value.GetType(), - "Stored value does not have the same type as 'value'."); - - if (AreEqual(value, info.ParsedValue, comparer)) - { - info.ParsedValue = null; - result = true; - } - } - else - { - foreach (object item in parsedValues) + if (item.Type == HeaderStoreItemInfoValueType.Parsed) { - Debug.Assert(item.GetType() == value.GetType(), + Debug.Assert(item.Value.GetType() == value.GetType(), "One of the stored values does not have the same type as 'value'."); - if (AreEqual(value, item, comparer)) + if (AreEqual(value, item.Value, comparer)) { // Remove 'item' rather than 'value', since the 'comparer' may consider two values // equal even though the default obj.Equals() may not (e.g. if 'comparer' does // case-insensitive comparison for strings, but string.Equals() is case-sensitive). - result = parsedValues.Remove(item); + result = info.ParsedAndInvalidValues.Remove(item); break; } } - - // If we removed the last item in a list, remove the list. - if (parsedValues.Count == 0) - { - info.ParsedValue = null; - } } + // If there is no value for the header left, remove the header. if (info.IsEmpty) { @@ -458,6 +466,42 @@ internal bool RemoveParsedValue(HeaderDescriptor descriptor, object value) return false; } + private void RemoveParsedValue(HeaderStoreItemInfo info) + { + var result = new List(); + foreach (var item in info.ParsedAndInvalidValues) + { + if (item.Type != HeaderStoreItemInfoValueType.Parsed) + { + result.Add(item); + } + } + + info.ParsedAndInvalidValues = result; + } + + private static object? GetParsedValues(HeaderStoreItemInfo info) + { + var results = new List(); + foreach (var item in info.ParsedAndInvalidValues) + { + if (item.Type == HeaderStoreItemInfoValueType.Parsed) + results.Add(item.Value); + } + + if (results.Count > 0) + { + if (results.Count == 1) + { + return results[0]; + } + + return results; + } + + return null; + } + internal bool ContainsParsedValue(HeaderDescriptor descriptor, object value) { Debug.Assert(value != null); @@ -476,21 +520,27 @@ internal bool ContainsParsedValue(HeaderDescriptor descriptor, object value) "This method should not be used for single-value headers. Use equality comparer instead."); // If there is no entry, just return. - if (info.ParsedValue == null) + if (!ExistsParsedValue(info)) { return false; } - List? parsedValues = info.ParsedValue as List; + var origParsedValues = GetParsedValues(info); + List? parsedValues = origParsedValues as List; IEqualityComparer? comparer = descriptor.Parser.Comparer; if (parsedValues == null) { - Debug.Assert(info.ParsedValue.GetType() == value.GetType(), - "Stored value does not have the same type as 'value'."); + if (origParsedValues != null) + { + Debug.Assert(origParsedValues.GetType() == value.GetType(), + "Stored value does not have the same type as 'value'."); + + return AreEqual(value, origParsedValues, comparer); + } - return AreEqual(value, info.ParsedValue, comparer); + return false; } else { @@ -557,32 +607,30 @@ private void AddHeaderInfo(HeaderDescriptor descriptor, HeaderStoreItemInfo sour { // We have custom header values. The parsed values are strings. // Custom header values are always stored as string or list of strings. - Debug.Assert(sourceInfo.InvalidValue == null, "No invalid values expected for custom headers."); - destinationInfo.ParsedValue = CloneStringHeaderInfoValues(sourceInfo.ParsedValue); + Debug.Assert(!ExistsInvalidValue(sourceInfo), "No invalid values expected for custom headers."); + CloneAndAddValue(destinationInfo, sourceInfo.ParsedAndInvalidValues); } else { - // We have a parser, so we also have to copy invalid values and clone parsed values. - - // Invalid values are always strings. Strings are immutable. So we only have to clone the - // collection (if there is one). - destinationInfo.InvalidValue = CloneStringHeaderInfoValues(sourceInfo.InvalidValue); + CloneAndAddValue(destinationInfo, sourceInfo.ParsedAndInvalidValues); + } + } - // Now clone and add parsed values (if any). - if (sourceInfo.ParsedValue != null) + private void CloneAndAddValue(HeaderStoreItemInfo destinationInfo, List source) + { + destinationInfo.ParsedAndInvalidValues = new List(); + foreach (var item in source) + { + switch(item.Type) { - List? sourceValues = sourceInfo.ParsedValue as List; - if (sourceValues == null) - { - CloneAndAddValue(destinationInfo, sourceInfo.ParsedValue); - } - else - { - foreach (object item in sourceValues) - { - CloneAndAddValue(destinationInfo, item); - } - } + case HeaderStoreItemInfoValueType.Invalid: + destinationInfo.ParsedAndInvalidValues.Add(new HeaderStoreItemInfoValue { Value = CloneStringHeaderInfoValues(item.Value), Type = HeaderStoreItemInfoValueType.Invalid }); + break; + case HeaderStoreItemInfoValueType.Parsed: + CloneAndAddValue(destinationInfo, item.Value); + break; + default: + throw new NotImplementedException(item.Type.ToString()); } } } @@ -732,7 +780,7 @@ private bool ParseRawHeaderValues(HeaderDescriptor descriptor, HeaderStoreItemIn // During parsing, we removed the value since it contains newline chars. Return false to indicate that // this is an empty header. If the caller specified to remove empty headers, we'll remove the header before // returning. - if ((info.InvalidValue == null) && (info.ParsedValue == null)) + if (info.ParsedAndInvalidValues.Count == 0) { if (removeEmptyHeader) { @@ -804,7 +852,7 @@ internal bool TryParseAndAddValue(HeaderDescriptor descriptor, string? value) bool result = TryParseAndAddRawHeaderValue(descriptor, info, value, false); - if (result && addToStore && (info.ParsedValue != null)) + if (result && addToStore && ExistsParsedValue(info)) { // If we get here, then the value could be parsed correctly. If we created a new HeaderStoreItemInfo, add // it to the store if we added at least one value. @@ -835,7 +883,7 @@ private static bool TryParseAndAddRawHeaderValue(HeaderDescriptor descriptor, He int index = 0; - if (descriptor.Parser.TryParseValue(value, info.ParsedValue, ref index, out object? parsedValue)) + if (descriptor.Parser.TryParseValue(value, GetParsedValues(info), ref index, out object? parsedValue)) { // The raw string only represented one value (which was successfully parsed). Add the value and return. if ((value == null) || (index == value.Length)) @@ -858,7 +906,7 @@ private static bool TryParseAndAddRawHeaderValue(HeaderDescriptor descriptor, He while (index < value.Length) { - if (descriptor.Parser.TryParseValue(value, info.ParsedValue, ref index, out parsedValue)) + if (descriptor.Parser.TryParseValue(value, info.ParsedAndInvalidValues, ref index, out parsedValue)) { if (parsedValue != null) { @@ -897,12 +945,12 @@ private static void AddParsedValue(HeaderStoreItemInfo info, object value) "Header value types must not derive from List since this type is used internally to store " + "lists of values. So we would not be able to distinguish between a single value and a list of values."); - AddValueToStoreValue(value, ref info.ParsedValue); + AddValueToStoreValue(value, HeaderStoreItemInfoValueType.Parsed, info.ParsedAndInvalidValues); } private static void AddInvalidValue(HeaderStoreItemInfo info, string value) { - AddValueToStoreValue(value, ref info.InvalidValue); + AddValueToStoreValue(value, HeaderStoreItemInfoValueType.Invalid, info.ParsedAndInvalidValues); } private static void AddRawValue(HeaderStoreItemInfo info, string value) @@ -935,6 +983,11 @@ private static void AddValueToStoreValue(T value, ref object? currentStoreVal } } + private static void AddValueToStoreValue(T value, HeaderStoreItemInfoValueType type, List list) where T : class + { + list.Add(new HeaderStoreItemInfoValue { Value = value, Type = type }); + } + // Since most of the time we just have 1 value, we don't create a List for one value, but we change // the return type to 'object'. The caller has to deal with the return type (object vs. List). This // is to optimize the most common scenario where a header has only one value. @@ -945,7 +998,7 @@ private static void AddValueToStoreValue(T value, ref object? currentStoreVal return null; } - return info.ParsedValue; + return GetParsedValues(info); } internal virtual bool IsAllowedHeaderName(HeaderDescriptor descriptor) => true; @@ -986,7 +1039,7 @@ private void ParseAndAddValue(HeaderDescriptor descriptor, HeaderStoreItemInfo i } int index = 0; - object parsedValue = descriptor.Parser.ParseValue(value, info.ParsedValue, ref index); + object parsedValue = descriptor.Parser.ParseValue(value,GetParsedValues(info), ref index); // The raw string only represented one value (which was successfully parsed). Add the value and return. // If value is null we still have to first call ParseValue() to allow the parser to decide whether null is @@ -1013,7 +1066,7 @@ private void ParseAndAddValue(HeaderDescriptor descriptor, HeaderStoreItemInfo i while (index < value.Length) { - parsedValue = descriptor.Parser.ParseValue(value, info.ParsedValue, ref index); + parsedValue = descriptor.Parser.ParseValue(value, GetParsedValues(info), ref index); if (parsedValue != null) { parsedValues.Add(parsedValue); @@ -1133,8 +1186,21 @@ internal static void GetStoreValuesAsStringOrStringArray(HeaderDescriptor descri int currentIndex = 0; ReadStoreValues(values, info.RawValue, null, ref currentIndex); - ReadStoreValues(values, info.ParsedValue, descriptor.Parser, ref currentIndex); - ReadStoreValues(values, info.InvalidValue, null, ref currentIndex); + foreach (var item in info.ParsedAndInvalidValues) + { + switch (item.Type) + { + case HeaderStoreItemInfoValueType.Invalid: + ReadStoreValues(values, item.Value, null, ref currentIndex); + break; + case HeaderStoreItemInfoValueType.Parsed: + ReadStoreValues(values, item.Value, descriptor.Parser, ref currentIndex); + break; + default: + throw new NotImplementedException(item.Type.ToString()); + } + } + Debug.Assert(currentIndex == length); } @@ -1167,8 +1233,21 @@ internal static int GetStoreValuesIntoStringArray(HeaderDescriptor descriptor, o int currentIndex = 0; ReadStoreValues(values, info.RawValue, null, ref currentIndex); - ReadStoreValues(values, info.ParsedValue, descriptor.Parser, ref currentIndex); - ReadStoreValues(values, info.InvalidValue, null, ref currentIndex); + foreach (var item in info.ParsedAndInvalidValues) + { + switch (item.Type) + { + case HeaderStoreItemInfoValueType.Invalid: + ReadStoreValues(values, item.Value, null, ref currentIndex); + break; + case HeaderStoreItemInfoValueType.Parsed: + ReadStoreValues(values, item.Value, descriptor.Parser, ref currentIndex); + break; + default: + throw new NotImplementedException(item.Type.ToString()); + } + } + Debug.Assert(currentIndex == length); } @@ -1180,8 +1259,21 @@ private static int GetValueCount(HeaderStoreItemInfo info) Debug.Assert(info != null); int valueCount = Count(info.RawValue); - valueCount += Count(info.InvalidValue); - valueCount += Count(info.ParsedValue); + foreach (var item in info.ParsedAndInvalidValues) + { + switch (item.Type) + { + case HeaderStoreItemInfoValueType.Invalid: + valueCount += Count(item.Value); + break; + case HeaderStoreItemInfoValueType.Parsed: + valueCount += Count(item.Value); + break; + default: + throw new NotImplementedException(item.Type.ToString()); + } + } + return valueCount; static int Count(object? valueStore) => @@ -1226,14 +1318,25 @@ private bool AreEqual(object value, object? storeValue, IEqualityComparer? compa return value.Equals(storeValue); } + internal sealed class HeaderStoreItemInfoValue + { + internal object? Value; + internal HeaderStoreItemInfoValueType Type; + } + + internal enum HeaderStoreItemInfoValueType + { + Parsed = 0, + Invalid + } + internal sealed class HeaderStoreItemInfo { internal HeaderStoreItemInfo() { } internal object? RawValue; - internal object? InvalidValue; - internal object? ParsedValue; - + + internal List ParsedAndInvalidValues = new List(); internal bool CanAddParsedValue(HttpHeaderParser parser) { Debug.Assert(parser != null, "There should be no reason to call CanAddValue if there is no parser for the current header."); @@ -1247,10 +1350,10 @@ internal bool CanAddParsedValue(HttpHeaderParser parser) // supporting 1 value. When the first value gets parsed, CanAddValue returns true and we add the // parsed value to ParsedValue. When the second value is parsed, CanAddValue returns false, because // we have already a parsed value. - return parser.SupportsMultipleValues || ((InvalidValue == null) && (ParsedValue == null)); + return parser.SupportsMultipleValues || ParsedAndInvalidValues.Count == 0; } - internal bool IsEmpty => (RawValue == null) && (InvalidValue == null) && (ParsedValue == null); + internal bool IsEmpty => (RawValue == null) && ParsedAndInvalidValues.Count == 0; } } } diff --git a/src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpHeadersTest.cs b/src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpHeadersTest.cs index 139422163a95b..2844bda182409 100644 --- a/src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpHeadersTest.cs +++ b/src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpHeadersTest.cs @@ -199,12 +199,12 @@ public void TryAddWithoutValidation_AddValidAndInvalidValueString_BothValuesPars // - The header value has an invalid format (very rare for standard headers) AND // - There are multiple header values (some valid, some invalid) AND // - The order of the headers matters (e.g. Transfer-Encoding) - Assert.Equal(parsedPrefix, headers.First().Value.ElementAt(0)); - Assert.Equal(invalidHeaderValue, headers.First().Value.ElementAt(1)); - + Assert.Equal(invalidHeaderValue, headers.First().Value.ElementAt(0)); + Assert.Equal(parsedPrefix, headers.First().Value.ElementAt(1)); + Assert.Equal(2, headers.Parser.TryParseValueCallCount); - string expected = headers.Descriptor.Name + ": " + parsedPrefix + ", " + invalidHeaderValue + Environment.NewLine; + string expected = headers.Descriptor.Name + ": " + invalidHeaderValue + ", " + parsedPrefix + Environment.NewLine; Assert.Equal(expected, headers.ToString()); } @@ -616,8 +616,8 @@ public void Add_SingleFirstTryAddWithoutValidationForInvalidValueThenAdd_TwoPars Assert.Equal(1, headers.Count()); Assert.Equal(2, headers.First().Value.Count()); - Assert.Equal(parsedPrefix + "1", headers.First().Value.ElementAt(0)); - Assert.Equal(invalidHeaderValue, headers.First().Value.ElementAt(1)); + Assert.Equal(invalidHeaderValue, headers.First().Value.ElementAt(0)); + Assert.Equal(parsedPrefix + "1", headers.First().Value.ElementAt(1)); // Value is already parsed. There shouldn't be additional calls to the parser. Assert.Equal(2, headers.Parser.TryParseValueCallCount); @@ -2047,9 +2047,9 @@ public void AddHeaders_SourceAndDestinationStoreHaveMultipleHeaders_OnlyHeadersN // invalid value. Assert.Equal(3, destination.GetValues(known2Header).Count()); Assert.Equal(parsedPrefix + "5", destination.GetValues(known2Header).ElementAt(0)); - Assert.Equal(parsedPrefix + "7", destination.GetValues(known2Header).ElementAt(1)); - Assert.Equal(invalidHeaderValue, destination.GetValues(known2Header).ElementAt(2)); - + Assert.Equal(invalidHeaderValue, destination.GetValues(known2Header).ElementAt(1)); + Assert.Equal(parsedPrefix + "7", destination.GetValues(known2Header).ElementAt(2)); + // Header 'known3' should not be copied, since it doesn't contain any values. Assert.False(destination.Contains(known3Header), "'known3' header value count."); diff --git a/src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpRequestHeadersTest.cs b/src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpRequestHeadersTest.cs index 81fa6560bc60c..81c520110397c 100644 --- a/src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpRequestHeadersTest.cs +++ b/src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpRequestHeadersTest.cs @@ -162,7 +162,7 @@ public void AcceptCharset_AddMultipleValuesAndGetValueString_AllValuesAddedUsing foreach (var header in headers.NonValidated) { Assert.Equal("Accept-Charset", header.Key); - Assert.Equal("utf-8, iso-8859-5; q=0.5, invalid value", header.Value.ToString()); + Assert.Equal("invalid value, utf-8, iso-8859-5; q=0.5", header.Value.ToString()); } } @@ -667,7 +667,7 @@ public void UserAgent_AddMultipleValuesAndGetValueString_AllValuesAddedUsingTheC foreach (var header in headers.NonValidated) { Assert.Equal("User-Agent", header.Key); - Assert.Equal("custom2/1.1 (comment) custom\u4F1A", header.Value.ToString()); + Assert.Equal("custom\u4F1A custom2/1.1 (comment)", header.Value.ToString()); } } @@ -1553,6 +1553,24 @@ public void Pragma_UseAddMethodWithInvalidValue_InvalidValueRecognized() #endregion + [Fact] + public void NonValidatedHeader_Orders() + { + HttpRequestMessage request = new HttpRequestMessage(HttpMethod.Get, "http://microsoft.com"); + request.Headers.TryAddWithoutValidation("Accept", "text/bar"); + request.Headers.TryAddWithoutValidation("Accept", "invalid"); + request.Headers.TryAddWithoutValidation("Accept", "text/baz"); + + // Force parsing + _ = request.Headers.Accept.Count; + + Assert.Equal(1, request.Headers.NonValidated.Count); + Assert.Equal(3, request.Headers.NonValidated.ElementAt(0).Value.Count); + Assert.Equal("text/bar", request.Headers.NonValidated.ElementAt(0).Value.ElementAt(0)); + Assert.Equal("invalid", request.Headers.NonValidated.ElementAt(0).Value.ElementAt(1)); + Assert.Equal("text/baz", request.Headers.NonValidated.ElementAt(0).Value.ElementAt(2)); + } + [Fact] public void ToString_SeveralRequestHeaders_Success() { From 04e4718c7ff60b6c7efc8d1885a747098903aa58 Mon Sep 17 00:00:00 2001 From: feiyun0112 Date: Sun, 13 Feb 2022 22:27:44 +0800 Subject: [PATCH 02/27] fix complie error --- .../System/Net/Http/Headers/HttpHeaders.cs | 43 ++++++++++--------- 1 file changed, 22 insertions(+), 21 deletions(-) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs index b563086bc0b31..09ea3bfe95bf3 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs @@ -82,7 +82,7 @@ internal void Add(HeaderDescriptor descriptor, string? value) // HeaderStoreItemInfo object and try to parse the value. If it works, we'll add the header. PrepareHeaderInfoForAdd(descriptor, out HeaderStoreItemInfo info, out bool addToStore); ParseAndAddValue(descriptor, info, value); - + // If we get here, then the value could be parsed correctly. If we created a new HeaderStoreItemInfo, add // it to the store if we added at least one value. if (addToStore && ExistsParsedValue(info)) @@ -410,8 +410,7 @@ internal void SetParsedValue(HeaderDescriptor descriptor, object value) // This method will first clear all values. This is used e.g. when setting the 'Date' or 'Host' header. // i.e. headers not supporting collections. - - HeaderStoreItemInfo info = GetOrCreateHeaderInfo(descriptor, parseRawValues: true); + HeaderStoreItemInfo info = GetOrCreateHeaderInfo(descriptor); info.ParsedAndInvalidValues = new List(); info.RawValue = null; @@ -456,7 +455,7 @@ internal bool RemoveParsedValue(HeaderDescriptor descriptor, object value) foreach (var item in info.ParsedAndInvalidValues) { - if (item.Type == HeaderStoreItemInfoValueType.Parsed) + if (item.Type == HeaderStoreItemInfoValueType.Parsed && item.Value != null) { Debug.Assert(item.Value.GetType() == value.GetType(), "One of the stored values does not have the same type as 'value'."); @@ -472,7 +471,6 @@ internal bool RemoveParsedValue(HeaderDescriptor descriptor, object value) } } - // If there is no value for the header left, remove the header. if (info.IsEmpty) { @@ -505,7 +503,7 @@ private void RemoveParsedValue(HeaderStoreItemInfo info) var results = new List(); foreach (var item in info.ParsedAndInvalidValues) { - if (item.Type == HeaderStoreItemInfoValueType.Parsed) + if (item.Type == HeaderStoreItemInfoValueType.Parsed && item.Value != null) results.Add(item.Value); } @@ -646,6 +644,8 @@ private HeaderStoreItemInfo CloneHeaderInfo(HeaderDescriptor descriptor, HeaderS { CloneAndAddValue(destinationInfo, sourceInfo.ParsedAndInvalidValues); } + + return destinationInfo; } private void CloneAndAddValue(HeaderStoreItemInfo destinationInfo, List source) @@ -653,20 +653,21 @@ private void CloneAndAddValue(HeaderStoreItemInfo destinationInfo, List(); foreach (var item in source) { - switch(item.Type) + if (item.Value != null) { - case HeaderStoreItemInfoValueType.Invalid: - destinationInfo.ParsedAndInvalidValues.Add(new HeaderStoreItemInfoValue { Value = CloneStringHeaderInfoValues(item.Value), Type = HeaderStoreItemInfoValueType.Invalid }); - break; - case HeaderStoreItemInfoValueType.Parsed: - CloneAndAddValue(destinationInfo, item.Value); - break; - default: - throw new NotImplementedException(item.Type.ToString()); + switch (item.Type) + { + case HeaderStoreItemInfoValueType.Invalid: + destinationInfo.ParsedAndInvalidValues.Add(new HeaderStoreItemInfoValue { Value = CloneStringHeaderInfoValues(item.Value), Type = HeaderStoreItemInfoValueType.Invalid }); + break; + case HeaderStoreItemInfoValueType.Parsed: + CloneAndAddValue(destinationInfo, item.Value); + break; + default: + throw new NotImplementedException(item.Type.ToString()); + } } } - - return destinationInfo; } private static void CloneAndAddValue(HeaderStoreItemInfo destinationInfo, object source) @@ -1052,7 +1053,7 @@ private void ParseAndAddValue(HeaderDescriptor descriptor, HeaderStoreItemInfo i } int index = 0; - object parsedValue = descriptor.Parser.ParseValue(value,GetParsedValues(info), ref index); + object parsedValue = descriptor.Parser.ParseValue(value, GetParsedValues(info), ref index); // The raw string only represented one value (which was successfully parsed). Add the value and return. // If value is null we still have to first call ParseValue() to allow the parser to decide whether null is @@ -1216,7 +1217,7 @@ internal static void GetStoreValuesAsStringOrStringArray(HeaderDescriptor descri throw new NotImplementedException(item.Type.ToString()); } } - + Debug.Assert(currentIndex == length); } @@ -1289,7 +1290,7 @@ private static int GetValueCount(HeaderStoreItemInfo info) throw new NotImplementedException(item.Type.ToString()); } } - + return valueCount; static int Count(object? valueStore) => @@ -1351,7 +1352,7 @@ internal sealed class HeaderStoreItemInfo internal HeaderStoreItemInfo() { } internal object? RawValue; - + internal List ParsedAndInvalidValues = new List(); internal bool CanAddParsedValue(HttpHeaderParser parser) { From c4f503baad5020a77fd919c41ad710fd9b03e21c Mon Sep 17 00:00:00 2001 From: feiyun0112 Date: Tue, 15 Feb 2022 12:32:46 +0800 Subject: [PATCH 03/27] make the changes @ danmoseley recommended --- .../src/System/Net/Http/Headers/HttpHeaders.cs | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs index 09ea3bfe95bf3..9c09fd824de3e 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs @@ -105,7 +105,6 @@ private static bool ExistsParsedValue(HeaderStoreItemInfo info) return false; } - private static bool ExistsInvalidValue(HeaderStoreItemInfo info) { foreach (var item in info.ParsedAndInvalidValues) @@ -121,7 +120,7 @@ private static bool ExistsInvalidValue(HeaderStoreItemInfo info) public void Add(string name, IEnumerable values) => Add(GetHeaderDescriptor(name), values); - internal void Add(HeaderDescriptor descriptor, IEnumerable values!!) + internal void Add(HeaderDescriptor descriptor, IEnumerable values) { PrepareHeaderInfoForAdd(descriptor, out HeaderStoreItemInfo info, out bool addToStore); @@ -187,7 +186,7 @@ public bool TryAddWithoutValidation(string name, IEnumerable values) => TryGetHeaderDescriptor(name, out HeaderDescriptor descriptor) && TryAddWithoutValidation(descriptor, values); - internal bool TryAddWithoutValidation(HeaderDescriptor descriptor, IEnumerable values!!) + internal bool TryAddWithoutValidation(HeaderDescriptor descriptor, IEnumerable values) { using IEnumerator enumerator = values.GetEnumerator(); if (enumerator.MoveNext()) @@ -664,7 +663,8 @@ private void CloneAndAddValue(HeaderStoreItemInfo destinationInfo, List(values, item.Value, descriptor.Parser, ref currentIndex); break; default: - throw new NotImplementedException(item.Type.ToString()); + Debug.Fail($@"Get value reach an invalid type: {item.Type}"); + break; } } @@ -1261,7 +1262,8 @@ internal static int GetStoreValuesIntoStringArray(HeaderDescriptor descriptor, o ReadStoreValues(values, item.Value, descriptor.Parser, ref currentIndex); break; default: - throw new NotImplementedException(item.Type.ToString()); + Debug.Fail($@"Get value reach an invalid type: {item.Type}"); + break; } } @@ -1287,7 +1289,8 @@ private static int GetValueCount(HeaderStoreItemInfo info) valueCount += Count(item.Value); break; default: - throw new NotImplementedException(item.Type.ToString()); + Debug.Fail($@"Get value reach an invalid type: {item.Type}"); + break; } } From f5b1cb00cff81f9b96bb907284f743bdc0326fb8 Mon Sep 17 00:00:00 2001 From: feiyun0112 Date: Wed, 16 Feb 2022 12:02:40 +0800 Subject: [PATCH 04/27] make the changes @MihaZupan recommended --- .../System/Net/Http/Headers/HttpHeaders.cs | 106 ++++++++---------- 1 file changed, 46 insertions(+), 60 deletions(-) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs index 9c09fd824de3e..a331a915f1d70 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs @@ -85,26 +85,13 @@ internal void Add(HeaderDescriptor descriptor, string? value) // If we get here, then the value could be parsed correctly. If we created a new HeaderStoreItemInfo, add // it to the store if we added at least one value. - if (addToStore && ExistsParsedValue(info)) + if (addToStore && info.ExistsParsedValue()) { Debug.Assert(!ContainsKey(descriptor)); AddEntryToStore(new HeaderEntry(descriptor, info)); } } - private static bool ExistsParsedValue(HeaderStoreItemInfo info) - { - foreach (var item in info.ParsedAndInvalidValues) - { - if (item.Type == HeaderStoreItemInfoValueType.Parsed) - { - return true; - } - } - - return false; - } - private static bool ExistsInvalidValue(HeaderStoreItemInfo info) { foreach (var item in info.ParsedAndInvalidValues) @@ -120,7 +107,7 @@ private static bool ExistsInvalidValue(HeaderStoreItemInfo info) public void Add(string name, IEnumerable values) => Add(GetHeaderDescriptor(name), values); - internal void Add(HeaderDescriptor descriptor, IEnumerable values) + internal void Add(HeaderDescriptor descriptor, IEnumerable values!!) { PrepareHeaderInfoForAdd(descriptor, out HeaderStoreItemInfo info, out bool addToStore); @@ -140,7 +127,7 @@ internal void Add(HeaderDescriptor descriptor, IEnumerable values) // get added. Same here: If multiple values get added to a _new_ header, make sure the header gets added // with the valid values. // However, if all values for a _new_ header were invalid, then don't add the header. - if (addToStore && ExistsParsedValue(info)) + if (addToStore && info.ExistsParsedValue()) { Debug.Assert(!ContainsKey(descriptor)); AddEntryToStore(new HeaderEntry(descriptor, info)); @@ -444,7 +431,7 @@ internal bool RemoveParsedValue(HeaderDescriptor descriptor, object value) "This method should not be used for single-value headers. Use Remove(string) instead."); // If there is no entry, just return. - if (!ExistsParsedValue(info)) + if (!info.ExistsParsedValue()) { return false; } @@ -483,42 +470,6 @@ internal bool RemoveParsedValue(HeaderDescriptor descriptor, object value) return false; } - private void RemoveParsedValue(HeaderStoreItemInfo info) - { - var result = new List(); - foreach (var item in info.ParsedAndInvalidValues) - { - if (item.Type != HeaderStoreItemInfoValueType.Parsed) - { - result.Add(item); - } - } - - info.ParsedAndInvalidValues = result; - } - - private static object? GetParsedValues(HeaderStoreItemInfo info) - { - var results = new List(); - foreach (var item in info.ParsedAndInvalidValues) - { - if (item.Type == HeaderStoreItemInfoValueType.Parsed && item.Value != null) - results.Add(item.Value); - } - - if (results.Count > 0) - { - if (results.Count == 1) - { - return results[0]; - } - - return results; - } - - return null; - } - internal bool ContainsParsedValue(HeaderDescriptor descriptor, object value) { Debug.Assert(value != null); @@ -532,12 +483,12 @@ internal bool ContainsParsedValue(HeaderDescriptor descriptor, object value) "This method should not be used for single-value headers. Use equality comparer instead."); // If there is no entry, just return. - if (!ExistsParsedValue(info)) + if (!info.ExistsParsedValue()) { return false; } - var origParsedValues = GetParsedValues(info); + var origParsedValues = info.GetParsedValues(); List? parsedValues = origParsedValues as List; IEqualityComparer? comparer = descriptor.Parser.Comparer; @@ -865,7 +816,7 @@ internal bool TryParseAndAddValue(HeaderDescriptor descriptor, string? value) bool result = TryParseAndAddRawHeaderValue(descriptor, info, value, false); - if (result && addToStore && ExistsParsedValue(info)) + if (result && addToStore && info.ExistsParsedValue()) { // If we get here, then the value could be parsed correctly. If we created a new HeaderStoreItemInfo, add // it to the store if we added at least one value. @@ -897,7 +848,7 @@ private static bool TryParseAndAddRawHeaderValue(HeaderDescriptor descriptor, He int index = 0; - if (descriptor.Parser.TryParseValue(value, GetParsedValues(info), ref index, out object? parsedValue)) + if (descriptor.Parser.TryParseValue(value, info.GetParsedValues(), ref index, out object? parsedValue)) { // The raw string only represented one value (which was successfully parsed). Add the value and return. if ((value == null) || (index == value.Length)) @@ -1012,7 +963,7 @@ private static void AddValueToStoreValue(T value, HeaderStoreItemInfoValueTyp return null; } - return GetParsedValues(info); + return info.GetParsedValues(); } internal virtual bool IsAllowedHeaderName(HeaderDescriptor descriptor) => true; @@ -1053,7 +1004,7 @@ private void ParseAndAddValue(HeaderDescriptor descriptor, HeaderStoreItemInfo i } int index = 0; - object parsedValue = descriptor.Parser.ParseValue(value, GetParsedValues(info), ref index); + object parsedValue = descriptor.Parser.ParseValue(value, info.GetParsedValues(), ref index); // The raw string only represented one value (which was successfully parsed). Add the value and return. // If value is null we still have to first call ParseValue() to allow the parser to decide whether null is @@ -1080,7 +1031,7 @@ private void ParseAndAddValue(HeaderDescriptor descriptor, HeaderStoreItemInfo i while (index < value.Length) { - parsedValue = descriptor.Parser.ParseValue(value, GetParsedValues(info), ref index); + parsedValue = descriptor.Parser.ParseValue(value, info.GetParsedValues(), ref index); if (parsedValue != null) { parsedValues.Add(parsedValue); @@ -1374,6 +1325,41 @@ internal bool CanAddParsedValue(HttpHeaderParser parser) } internal bool IsEmpty => (RawValue == null) && ParsedAndInvalidValues.Count == 0; + + internal bool ExistsParsedValue() + { + foreach (var item in ParsedAndInvalidValues) + { + if (item.Type == HeaderStoreItemInfoValueType.Parsed) + { + return true; + } + } + + return false; + } + + internal object? GetParsedValues() + { + var results = new List(); + foreach (var item in ParsedAndInvalidValues) + { + if (item.Type == HeaderStoreItemInfoValueType.Parsed && item.Value != null) + results.Add(item.Value); + } + + if (results.Count > 0) + { + if (results.Count == 1) + { + return results[0]; + } + + return results; + } + + return null; + } } From 2fd48a61526a912fceca182eca517cb46f03a883 Mon Sep 17 00:00:00 2001 From: feiyun0112 Date: Sat, 19 Feb 2022 20:02:12 +0800 Subject: [PATCH 05/27] make the changes @MihaZupan recommended --- .../System/Net/Http/Headers/HttpHeaders.cs | 317 ++++++++---------- 1 file changed, 148 insertions(+), 169 deletions(-) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs index a331a915f1d70..69191bdcf28f8 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs @@ -85,26 +85,13 @@ internal void Add(HeaderDescriptor descriptor, string? value) // If we get here, then the value could be parsed correctly. If we created a new HeaderStoreItemInfo, add // it to the store if we added at least one value. - if (addToStore && info.ExistsParsedValue()) + if (addToStore && (info.GetParsedValue() != null)) { Debug.Assert(!ContainsKey(descriptor)); AddEntryToStore(new HeaderEntry(descriptor, info)); } } - private static bool ExistsInvalidValue(HeaderStoreItemInfo info) - { - foreach (var item in info.ParsedAndInvalidValues) - { - if (item.Type == HeaderStoreItemInfoValueType.Invalid) - { - return true; - } - } - - return false; - } - public void Add(string name, IEnumerable values) => Add(GetHeaderDescriptor(name), values); internal void Add(HeaderDescriptor descriptor, IEnumerable values!!) @@ -127,7 +114,7 @@ internal void Add(HeaderDescriptor descriptor, IEnumerable values!!) // get added. Same here: If multiple values get added to a _new_ header, make sure the header gets added // with the valid values. // However, if all values for a _new_ header were invalid, then don't add the header. - if (addToStore && info.ExistsParsedValue()) + if (addToStore && (info.GetParsedValue() != null)) { Debug.Assert(!ContainsKey(descriptor)); AddEntryToStore(new HeaderEntry(descriptor, info)); @@ -173,7 +160,7 @@ public bool TryAddWithoutValidation(string name, IEnumerable values) => TryGetHeaderDescriptor(name, out HeaderDescriptor descriptor) && TryAddWithoutValidation(descriptor, values); - internal bool TryAddWithoutValidation(HeaderDescriptor descriptor, IEnumerable values) + internal bool TryAddWithoutValidation(HeaderDescriptor descriptor, IEnumerable values!!) { using IEnumerator enumerator = values.GetEnumerator(); if (enumerator.MoveNext()) @@ -397,8 +384,9 @@ internal void SetParsedValue(HeaderDescriptor descriptor, object value) // This method will first clear all values. This is used e.g. when setting the 'Date' or 'Host' header. // i.e. headers not supporting collections. HeaderStoreItemInfo info = GetOrCreateHeaderInfo(descriptor); - info.ParsedAndInvalidValues = new List(); + info.InvalidValue = null; + info.ParsedAndInvalidValues = null; info.RawValue = null; AddParsedValue(info, value); @@ -431,7 +419,8 @@ internal bool RemoveParsedValue(HeaderDescriptor descriptor, object value) "This method should not be used for single-value headers. Use Remove(string) instead."); // If there is no entry, just return. - if (!info.ExistsParsedValue()) + var parsedValue = info.GetParsedValue(); + if (parsedValue == null) { return false; } @@ -439,22 +428,40 @@ internal bool RemoveParsedValue(HeaderDescriptor descriptor, object value) bool result = false; IEqualityComparer? comparer = descriptor.Parser.Comparer; - foreach (var item in info.ParsedAndInvalidValues) + List? parsedValues = parsedValue as List; + if (parsedValues == null) { - if (item.Type == HeaderStoreItemInfoValueType.Parsed && item.Value != null) + Debug.Assert(parsedValue.GetType() == value.GetType(), + "Stored value does not have the same type as 'value'."); + + if (AreEqual(value, parsedValue, comparer)) { - Debug.Assert(item.Value.GetType() == value.GetType(), + info.ParsedAndInvalidValues = info.InvalidValue; + result = true; + } + } + else + { + foreach (object item in parsedValues) + { + Debug.Assert(item.GetType() == value.GetType(), "One of the stored values does not have the same type as 'value'."); - if (AreEqual(value, item.Value, comparer)) + if (AreEqual(value, item, comparer)) { // Remove 'item' rather than 'value', since the 'comparer' may consider two values // equal even though the default obj.Equals() may not (e.g. if 'comparer' does // case-insensitive comparison for strings, but string.Equals() is case-sensitive). - result = info.ParsedAndInvalidValues.Remove(item); + result = parsedValues.Remove(item); break; } } + + // If we removed the last item in a list, remove the list. + if (parsedValues.Count == 0) + { + info.ParsedAndInvalidValues = info.InvalidValue; + } } // If there is no value for the header left, remove the header. @@ -483,27 +490,22 @@ internal bool ContainsParsedValue(HeaderDescriptor descriptor, object value) "This method should not be used for single-value headers. Use equality comparer instead."); // If there is no entry, just return. - if (!info.ExistsParsedValue()) + var parsedValue = info.GetParsedValue(); + if (parsedValue == null) { return false; } - var origParsedValues = info.GetParsedValues(); - List? parsedValues = origParsedValues as List; + List? parsedValues = parsedValue as List; IEqualityComparer? comparer = descriptor.Parser.Comparer; if (parsedValues == null) { - if (origParsedValues != null) - { - Debug.Assert(origParsedValues.GetType() == value.GetType(), - "Stored value does not have the same type as 'value'."); + Debug.Assert(parsedValue.GetType() == value.GetType(), + "Stored value does not have the same type as 'value'."); - return AreEqual(value, origParsedValues, comparer); - } - - return false; + return AreEqual(value, parsedValue, comparer); } else { @@ -587,38 +589,36 @@ private HeaderStoreItemInfo CloneHeaderInfo(HeaderDescriptor descriptor, HeaderS { // We have custom header values. The parsed values are strings. // Custom header values are always stored as string or list of strings. - Debug.Assert(!ExistsInvalidValue(sourceInfo), "No invalid values expected for custom headers."); - CloneAndAddValue(destinationInfo, sourceInfo.ParsedAndInvalidValues); + Debug.Assert(sourceInfo.InvalidValue == null, "No invalid values expected for custom headers."); + destinationInfo.ParsedAndInvalidValues = CloneStringHeaderInfoValues(sourceInfo.ParsedAndInvalidValues); } else { - CloneAndAddValue(destinationInfo, sourceInfo.ParsedAndInvalidValues); - } + // We have a parser, so we also have to copy invalid values and clone parsed values. - return destinationInfo; - } + // Invalid values are always strings. Strings are immutable. So we only have to clone the + // collection (if there is one). + destinationInfo.InvalidValue = CloneStringHeaderInfoValues(sourceInfo.InvalidValue); - private void CloneAndAddValue(HeaderStoreItemInfo destinationInfo, List source) - { - destinationInfo.ParsedAndInvalidValues = new List(); - foreach (var item in source) - { - if (item.Value != null) + // Now clone and add parsed values (if any). + if (sourceInfo.ParsedAndInvalidValues != null) { - switch (item.Type) + List? sourceValues = sourceInfo.ParsedAndInvalidValues as List; + if (sourceValues == null) { - case HeaderStoreItemInfoValueType.Invalid: - destinationInfo.ParsedAndInvalidValues.Add(new HeaderStoreItemInfoValue { Value = CloneStringHeaderInfoValues(item.Value), Type = HeaderStoreItemInfoValueType.Invalid }); - break; - case HeaderStoreItemInfoValueType.Parsed: - CloneAndAddValue(destinationInfo, item.Value); - break; - default: - Debug.Fail($@"Get value reach an invalid type: {item.Type}"); - break; + CloneAndAddValue(destinationInfo, sourceInfo.ParsedAndInvalidValues); + } + else + { + foreach (object item in sourceValues) + { + CloneAndAddValue(destinationInfo, item); + } } } } + + return destinationInfo; } private static void CloneAndAddValue(HeaderStoreItemInfo destinationInfo, object source) @@ -745,9 +745,8 @@ private bool ParseRawHeaderValues(HeaderDescriptor descriptor, HeaderStoreItemIn info.RawValue = null; // During parsing, we removed the value since it contains newline chars. Return false to indicate that - // this is an empty header. If the caller specified to remove empty headers, we'll remove the header before - // returning. - if (info.ParsedAndInvalidValues.Count == 0) + // this is an empty header. + if ((info.InvalidValue == null) && (info.ParsedAndInvalidValues == null)) { // After parsing the raw value, no value is left because all values contain newline chars. Debug.Assert(_count > 0); @@ -816,7 +815,7 @@ internal bool TryParseAndAddValue(HeaderDescriptor descriptor, string? value) bool result = TryParseAndAddRawHeaderValue(descriptor, info, value, false); - if (result && addToStore && info.ExistsParsedValue()) + if (result && addToStore && (info.GetParsedValue() != null)) { // If we get here, then the value could be parsed correctly. If we created a new HeaderStoreItemInfo, add // it to the store if we added at least one value. @@ -848,7 +847,7 @@ private static bool TryParseAndAddRawHeaderValue(HeaderDescriptor descriptor, He int index = 0; - if (descriptor.Parser.TryParseValue(value, info.GetParsedValues(), ref index, out object? parsedValue)) + if (descriptor.Parser.TryParseValue(value, info.GetParsedValue(), ref index, out object? parsedValue)) { // The raw string only represented one value (which was successfully parsed). Add the value and return. if ((value == null) || (index == value.Length)) @@ -871,7 +870,7 @@ private static bool TryParseAndAddRawHeaderValue(HeaderDescriptor descriptor, He while (index < value.Length) { - if (descriptor.Parser.TryParseValue(value, info.ParsedAndInvalidValues, ref index, out parsedValue)) + if (descriptor.Parser.TryParseValue(value, info.GetParsedValue(), ref index, out parsedValue)) { if (parsedValue != null) { @@ -910,12 +909,13 @@ private static void AddParsedValue(HeaderStoreItemInfo info, object value) "Header value types must not derive from List since this type is used internally to store " + "lists of values. So we would not be able to distinguish between a single value and a list of values."); - AddValueToStoreValue(value, HeaderStoreItemInfoValueType.Parsed, info.ParsedAndInvalidValues); + AddValueToStoreValue(value, ref info.ParsedAndInvalidValues); } private static void AddInvalidValue(HeaderStoreItemInfo info, string value) { - AddValueToStoreValue(value, HeaderStoreItemInfoValueType.Invalid, info.ParsedAndInvalidValues); + AddValueToStoreValue(value, ref info.InvalidValue); + AddValueToStoreValue(value, ref info.ParsedAndInvalidValues); } private static void AddRawValue(HeaderStoreItemInfo info, string value) @@ -948,11 +948,6 @@ private static void AddValueToStoreValue(T value, ref object? currentStoreVal } } - private static void AddValueToStoreValue(T value, HeaderStoreItemInfoValueType type, List list) where T : class - { - list.Add(new HeaderStoreItemInfoValue { Value = value, Type = type }); - } - // Since most of the time we just have 1 value, we don't create a List for one value, but we change // the return type to 'object'. The caller has to deal with the return type (object vs. List). This // is to optimize the most common scenario where a header has only one value. @@ -963,7 +958,7 @@ private static void AddValueToStoreValue(T value, HeaderStoreItemInfoValueTyp return null; } - return info.GetParsedValues(); + return info.GetParsedValue(); } internal virtual bool IsAllowedHeaderName(HeaderDescriptor descriptor) => true; @@ -1004,7 +999,7 @@ private void ParseAndAddValue(HeaderDescriptor descriptor, HeaderStoreItemInfo i } int index = 0; - object parsedValue = descriptor.Parser.ParseValue(value, info.GetParsedValues(), ref index); + object parsedValue = descriptor.Parser.ParseValue(value, info.GetParsedValue(), ref index); // The raw string only represented one value (which was successfully parsed). Add the value and return. // If value is null we still have to first call ParseValue() to allow the parser to decide whether null is @@ -1031,7 +1026,7 @@ private void ParseAndAddValue(HeaderDescriptor descriptor, HeaderStoreItemInfo i while (index < value.Length) { - parsedValue = descriptor.Parser.ParseValue(value, info.GetParsedValues(), ref index); + parsedValue = descriptor.Parser.ParseValue(value, info.GetParsedValue(), ref index); if (parsedValue != null) { parsedValues.Add(parsedValue); @@ -1153,22 +1148,8 @@ internal static void GetStoreValuesAsStringOrStringArray(HeaderDescriptor descri } int currentIndex = 0; - ReadStoreValues(values, info.RawValue, null, ref currentIndex); - foreach (var item in info.ParsedAndInvalidValues) - { - switch (item.Type) - { - case HeaderStoreItemInfoValueType.Invalid: - ReadStoreValues(values, item.Value, null, ref currentIndex); - break; - case HeaderStoreItemInfoValueType.Parsed: - ReadStoreValues(values, item.Value, descriptor.Parser, ref currentIndex); - break; - default: - Debug.Fail($@"Get value reach an invalid type: {item.Type}"); - break; - } - } + ReadStoreValues(values, info.RawValue, null, null, ref currentIndex); + ReadStoreValues(values, info.ParsedAndInvalidValues, info, descriptor.Parser, ref currentIndex); Debug.Assert(currentIndex == length); } @@ -1201,23 +1182,8 @@ internal static int GetStoreValuesIntoStringArray(HeaderDescriptor descriptor, o } int currentIndex = 0; - ReadStoreValues(values, info.RawValue, null, ref currentIndex); - foreach (var item in info.ParsedAndInvalidValues) - { - switch (item.Type) - { - case HeaderStoreItemInfoValueType.Invalid: - ReadStoreValues(values, item.Value, null, ref currentIndex); - break; - case HeaderStoreItemInfoValueType.Parsed: - ReadStoreValues(values, item.Value, descriptor.Parser, ref currentIndex); - break; - default: - Debug.Fail($@"Get value reach an invalid type: {item.Type}"); - break; - } - } - + ReadStoreValues(values, info.RawValue, null, null, ref currentIndex); + ReadStoreValues(values, info.ParsedAndInvalidValues, info, descriptor.Parser, ref currentIndex); Debug.Assert(currentIndex == length); } @@ -1229,22 +1195,7 @@ private static int GetValueCount(HeaderStoreItemInfo info) Debug.Assert(info != null); int valueCount = Count(info.RawValue); - foreach (var item in info.ParsedAndInvalidValues) - { - switch (item.Type) - { - case HeaderStoreItemInfoValueType.Invalid: - valueCount += Count(item.Value); - break; - case HeaderStoreItemInfoValueType.Parsed: - valueCount += Count(item.Value); - break; - default: - Debug.Fail($@"Get value reach an invalid type: {item.Type}"); - break; - } - } - + valueCount += Count(info.ParsedAndInvalidValues); return valueCount; static int Count(object? valueStore) => @@ -1253,7 +1204,7 @@ static int Count(object? valueStore) => 1; } - private static void ReadStoreValues(Span values, object? storeValue, HttpHeaderParser? parser, ref int currentIndex) + private static void ReadStoreValues(Span values, object? storeValue, HeaderStoreItemInfo? info, HttpHeaderParser? parser, ref int currentIndex) { if (storeValue != null) { @@ -1261,7 +1212,7 @@ private static void ReadStoreValues(Span values, object? storeValue, if (storeValues == null) { - values[currentIndex] = parser == null ? storeValue.ToString() : parser.ToString(storeValue); + values[currentIndex] = parser == null || (info != null && info.IsInvalidValue(storeValue)) ? storeValue.ToString() : parser.ToString(storeValue); currentIndex++; } else @@ -1269,7 +1220,7 @@ private static void ReadStoreValues(Span values, object? storeValue, foreach (object? item in storeValues) { Debug.Assert(item != null); - values[currentIndex] = parser == null ? item.ToString() : parser.ToString(item); + values[currentIndex] = parser == null || (info != null && info.IsInvalidValue(item)) ? item.ToString() : parser.ToString(item); currentIndex++; } } @@ -1289,77 +1240,105 @@ private bool AreEqual(object value, object? storeValue, IEqualityComparer? compa return value.Equals(storeValue); } - internal sealed class HeaderStoreItemInfoValue - { - internal object? Value; - internal HeaderStoreItemInfoValueType Type; - } - - internal enum HeaderStoreItemInfoValueType - { - Parsed = 0, - Invalid - } - internal sealed class HeaderStoreItemInfo { internal HeaderStoreItemInfo() { } internal object? RawValue; + internal object? InvalidValue; + internal object? ParsedAndInvalidValues; - internal List ParsedAndInvalidValues = new List(); - internal bool CanAddParsedValue(HttpHeaderParser parser) + internal object? GetParsedValue() { - Debug.Assert(parser != null, "There should be no reason to call CanAddValue if there is no parser for the current header."); - - // If the header only supports one value, and we have already a value set, then we can't add - // another value. E.g. the 'Date' header only supports one value. We can't add multiple timestamps - // to 'Date'. - // So if this is a known header, ask the parser if it supports multiple values and check whether - // we already have a (valid or invalid) value. - // Note that we ignore the rawValue by purpose: E.g. we are parsing 2 raw values for a header only - // supporting 1 value. When the first value gets parsed, CanAddValue returns true and we add the - // parsed value to ParsedValue. When the second value is parsed, CanAddValue returns false, because - // we have already a parsed value. - return parser.SupportsMultipleValues || ParsedAndInvalidValues.Count == 0; - } + if (InvalidValue is null) + { + return ParsedAndInvalidValues; + } - internal bool IsEmpty => (RawValue == null) && ParsedAndInvalidValues.Count == 0; + var parsedAndInvalidValues = ParsedAndInvalidValues; + if (parsedAndInvalidValues is null) + { + return null; + } - internal bool ExistsParsedValue() - { - foreach (var item in ParsedAndInvalidValues) + if (parsedAndInvalidValues is List list) { - if (item.Type == HeaderStoreItemInfoValueType.Parsed) + object? parsedValue = null; + foreach (object value in list) { - return true; + if (!IsInvalidValue(value)) + { + if (parsedValue == null) + { + parsedValue = value; + } + else + { + List? storeValues = parsedValue as List; + + if (storeValues == null) + { + storeValues = new List(2); + storeValues.Add(parsedValue); + parsedValue = storeValues; + } + storeValues.Add(value); + } + } } + + return parsedValue; } + else + { + if (!IsInvalidValue(parsedAndInvalidValues)) + { + return parsedAndInvalidValues; + } - return false; + return null; + } } - internal object? GetParsedValues() + internal bool IsInvalidValue(object value) { - var results = new List(); - foreach (var item in ParsedAndInvalidValues) + if (InvalidValue == null) { - if (item.Type == HeaderStoreItemInfoValueType.Parsed && item.Value != null) - results.Add(item.Value); + return false; } - - if (results.Count > 0) + if (InvalidValue is List list) { - if (results.Count == 1) + foreach (string item in list) { - return results[0]; + if (item.Equals(value)) + { + return true; + } } - return results; + return false; } - return null; + return InvalidValue.Equals(value); } + + internal bool CanAddParsedValue(HttpHeaderParser parser) + { + Debug.Assert(parser != null, "There should be no reason to call CanAddValue if there is no parser for the current header."); + + // If the header only supports one value, and we have already a value set, then we can't add + // another value. E.g. the 'Date' header only supports one value. We can't add multiple timestamps + // to 'Date'. + // So if this is a known header, ask the parser if it supports multiple values and check whether + // we already have a (valid or invalid) value. + // Note that we ignore the rawValue by purpose: E.g. we are parsing 2 raw values for a header only + // supporting 1 value. When the first value gets parsed, CanAddValue returns true and we add the + // parsed value to ParsedValue. When the second value is parsed, CanAddValue returns false, because + // we have already a parsed value. + return parser.SupportsMultipleValues || ((InvalidValue == null) && (ParsedAndInvalidValues == null)); + } + + internal bool IsEmpty => (RawValue == null) && (InvalidValue == null) && (ParsedAndInvalidValues == null); } From cf3716ba7cc69c6916d7a67cbc550d9ef0b73984 Mon Sep 17 00:00:00 2001 From: feiyun0112 Date: Mon, 21 Feb 2022 15:45:04 +0800 Subject: [PATCH 06/27] make the changes @MihaZupan recommended --- .../System/Net/Http/Headers/HttpHeaders.cs | 229 ++++++++---------- 1 file changed, 101 insertions(+), 128 deletions(-) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs index 69191bdcf28f8..6d964f35c47fb 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs @@ -85,7 +85,7 @@ internal void Add(HeaderDescriptor descriptor, string? value) // If we get here, then the value could be parsed correctly. If we created a new HeaderStoreItemInfo, add // it to the store if we added at least one value. - if (addToStore && (info.GetParsedValue() != null)) + if (addToStore && (info.ParsedAndInvalidValues != null)) { Debug.Assert(!ContainsKey(descriptor)); AddEntryToStore(new HeaderEntry(descriptor, info)); @@ -114,7 +114,7 @@ internal void Add(HeaderDescriptor descriptor, IEnumerable values!!) // get added. Same here: If multiple values get added to a _new_ header, make sure the header gets added // with the valid values. // However, if all values for a _new_ header were invalid, then don't add the header. - if (addToStore && (info.GetParsedValue() != null)) + if (addToStore && (info.ParsedAndInvalidValues != null)) { Debug.Assert(!ContainsKey(descriptor)); AddEntryToStore(new HeaderEntry(descriptor, info)); @@ -385,7 +385,6 @@ internal void SetParsedValue(HeaderDescriptor descriptor, object value) // i.e. headers not supporting collections. HeaderStoreItemInfo info = GetOrCreateHeaderInfo(descriptor); - info.InvalidValue = null; info.ParsedAndInvalidValues = null; info.RawValue = null; @@ -419,7 +418,7 @@ internal bool RemoveParsedValue(HeaderDescriptor descriptor, object value) "This method should not be used for single-value headers. Use Remove(string) instead."); // If there is no entry, just return. - var parsedValue = info.GetParsedValue(); + var parsedValue = info.ParsedAndInvalidValues; if (parsedValue == null) { return false; @@ -431,36 +430,42 @@ internal bool RemoveParsedValue(HeaderDescriptor descriptor, object value) List? parsedValues = parsedValue as List; if (parsedValues == null) { - Debug.Assert(parsedValue.GetType() == value.GetType(), - "Stored value does not have the same type as 'value'."); - - if (AreEqual(value, parsedValue, comparer)) + if (parsedValue is not InvalidValue) { - info.ParsedAndInvalidValues = info.InvalidValue; - result = true; + Debug.Assert(parsedValue.GetType() == value.GetType(), + "Stored value does not have the same type as 'value'."); + + if (AreEqual(value, parsedValue, comparer)) + { + info.ParsedAndInvalidValues = null; + result = true; + } } } else { foreach (object item in parsedValues) { - Debug.Assert(item.GetType() == value.GetType(), + if (parsedValue is not InvalidValue) + { + Debug.Assert(item.GetType() == value.GetType(), "One of the stored values does not have the same type as 'value'."); - if (AreEqual(value, item, comparer)) - { - // Remove 'item' rather than 'value', since the 'comparer' may consider two values - // equal even though the default obj.Equals() may not (e.g. if 'comparer' does - // case-insensitive comparison for strings, but string.Equals() is case-sensitive). - result = parsedValues.Remove(item); - break; + if (AreEqual(value, item, comparer)) + { + // Remove 'item' rather than 'value', since the 'comparer' may consider two values + // equal even though the default obj.Equals() may not (e.g. if 'comparer' does + // case-insensitive comparison for strings, but string.Equals() is case-sensitive). + result = parsedValues.Remove(item); + break; + } } } // If we removed the last item in a list, remove the list. if (parsedValues.Count == 0) { - info.ParsedAndInvalidValues = info.InvalidValue; + info.ParsedAndInvalidValues = null; } } @@ -490,7 +495,7 @@ internal bool ContainsParsedValue(HeaderDescriptor descriptor, object value) "This method should not be used for single-value headers. Use equality comparer instead."); // If there is no entry, just return. - var parsedValue = info.GetParsedValue(); + var parsedValue = info.ParsedAndInvalidValues; if (parsedValue == null) { return false; @@ -502,21 +507,27 @@ internal bool ContainsParsedValue(HeaderDescriptor descriptor, object value) if (parsedValues == null) { - Debug.Assert(parsedValue.GetType() == value.GetType(), + if (parsedValue is not InvalidValue) + { + Debug.Assert(parsedValue.GetType() == value.GetType(), "Stored value does not have the same type as 'value'."); - return AreEqual(value, parsedValue, comparer); + return AreEqual(value, parsedValue, comparer); + } } else { foreach (object item in parsedValues) { - Debug.Assert(item.GetType() == value.GetType(), + if (parsedValue is not InvalidValue) + { + Debug.Assert(item.GetType() == value.GetType(), "One of the stored values does not have the same type as 'value'."); - if (AreEqual(value, item, comparer)) - { - return true; + if (AreEqual(value, item, comparer)) + { + return true; + } } } @@ -587,19 +598,10 @@ private HeaderStoreItemInfo CloneHeaderInfo(HeaderDescriptor descriptor, HeaderS if (descriptor.Parser == null) { - // We have custom header values. The parsed values are strings. - // Custom header values are always stored as string or list of strings. - Debug.Assert(sourceInfo.InvalidValue == null, "No invalid values expected for custom headers."); destinationInfo.ParsedAndInvalidValues = CloneStringHeaderInfoValues(sourceInfo.ParsedAndInvalidValues); } else { - // We have a parser, so we also have to copy invalid values and clone parsed values. - - // Invalid values are always strings. Strings are immutable. So we only have to clone the - // collection (if there is one). - destinationInfo.InvalidValue = CloneStringHeaderInfoValues(sourceInfo.InvalidValue); - // Now clone and add parsed values (if any). if (sourceInfo.ParsedAndInvalidValues != null) { @@ -746,7 +748,7 @@ private bool ParseRawHeaderValues(HeaderDescriptor descriptor, HeaderStoreItemIn // During parsing, we removed the value since it contains newline chars. Return false to indicate that // this is an empty header. - if ((info.InvalidValue == null) && (info.ParsedAndInvalidValues == null)) + if (info.ParsedAndInvalidValues == null) { // After parsing the raw value, no value is left because all values contain newline chars. Debug.Assert(_count > 0); @@ -815,7 +817,7 @@ internal bool TryParseAndAddValue(HeaderDescriptor descriptor, string? value) bool result = TryParseAndAddRawHeaderValue(descriptor, info, value, false); - if (result && addToStore && (info.GetParsedValue() != null)) + if (result && addToStore && (info.ParsedAndInvalidValues != null)) { // If we get here, then the value could be parsed correctly. If we created a new HeaderStoreItemInfo, add // it to the store if we added at least one value. @@ -847,7 +849,7 @@ private static bool TryParseAndAddRawHeaderValue(HeaderDescriptor descriptor, He int index = 0; - if (descriptor.Parser.TryParseValue(value, info.GetParsedValue(), ref index, out object? parsedValue)) + if (descriptor.Parser.TryParseValue(value, info.ParsedAndInvalidValues, ref index, out object? parsedValue)) { // The raw string only represented one value (which was successfully parsed). Add the value and return. if ((value == null) || (index == value.Length)) @@ -870,7 +872,7 @@ private static bool TryParseAndAddRawHeaderValue(HeaderDescriptor descriptor, He while (index < value.Length) { - if (descriptor.Parser.TryParseValue(value, info.GetParsedValue(), ref index, out parsedValue)) + if (descriptor.Parser.TryParseValue(value, info.ParsedAndInvalidValues, ref index, out parsedValue)) { if (parsedValue != null) { @@ -914,8 +916,7 @@ private static void AddParsedValue(HeaderStoreItemInfo info, object value) private static void AddInvalidValue(HeaderStoreItemInfo info, string value) { - AddValueToStoreValue(value, ref info.InvalidValue); - AddValueToStoreValue(value, ref info.ParsedAndInvalidValues); + AddValueToStoreValue(new InvalidValue(value), ref info.ParsedAndInvalidValues); } private static void AddRawValue(HeaderStoreItemInfo info, string value) @@ -958,7 +959,49 @@ private static void AddValueToStoreValue(T value, ref object? currentStoreVal return null; } - return info.GetParsedValue(); + var parsedAndInvalidValues = info.ParsedAndInvalidValues; + if (parsedAndInvalidValues is null) + { + return null; + } + + if (parsedAndInvalidValues is List list) + { + object? parsedValue = null; + foreach (object value in list) + { + if (value is not InvalidValue) + { + if (parsedValue == null) + { + parsedValue = value; + } + else + { + List? storeValues = parsedValue as List; + + if (storeValues == null) + { + storeValues = new List(2); + storeValues.Add(parsedValue); + parsedValue = storeValues; + } + storeValues.Add(value); + } + } + } + + return parsedValue; + } + else + { + if (parsedAndInvalidValues is not InvalidValue) + { + return parsedAndInvalidValues; + } + + return null; + } } internal virtual bool IsAllowedHeaderName(HeaderDescriptor descriptor) => true; @@ -999,7 +1042,7 @@ private void ParseAndAddValue(HeaderDescriptor descriptor, HeaderStoreItemInfo i } int index = 0; - object parsedValue = descriptor.Parser.ParseValue(value, info.GetParsedValue(), ref index); + object parsedValue = descriptor.Parser.ParseValue(value, info.ParsedAndInvalidValues, ref index); // The raw string only represented one value (which was successfully parsed). Add the value and return. // If value is null we still have to first call ParseValue() to allow the parser to decide whether null is @@ -1026,7 +1069,7 @@ private void ParseAndAddValue(HeaderDescriptor descriptor, HeaderStoreItemInfo i while (index < value.Length) { - parsedValue = descriptor.Parser.ParseValue(value, info.GetParsedValue(), ref index); + parsedValue = descriptor.Parser.ParseValue(value, info.ParsedAndInvalidValues, ref index); if (parsedValue != null) { parsedValues.Add(parsedValue); @@ -1148,8 +1191,8 @@ internal static void GetStoreValuesAsStringOrStringArray(HeaderDescriptor descri } int currentIndex = 0; - ReadStoreValues(values, info.RawValue, null, null, ref currentIndex); - ReadStoreValues(values, info.ParsedAndInvalidValues, info, descriptor.Parser, ref currentIndex); + ReadStoreValues(values, info.RawValue, null, ref currentIndex); + ReadStoreValues(values, info.ParsedAndInvalidValues, descriptor.Parser, ref currentIndex); Debug.Assert(currentIndex == length); } @@ -1182,8 +1225,8 @@ internal static int GetStoreValuesIntoStringArray(HeaderDescriptor descriptor, o } int currentIndex = 0; - ReadStoreValues(values, info.RawValue, null, null, ref currentIndex); - ReadStoreValues(values, info.ParsedAndInvalidValues, info, descriptor.Parser, ref currentIndex); + ReadStoreValues(values, info.RawValue, null, ref currentIndex); + ReadStoreValues(values, info.ParsedAndInvalidValues, descriptor.Parser, ref currentIndex); Debug.Assert(currentIndex == length); } @@ -1204,7 +1247,7 @@ static int Count(object? valueStore) => 1; } - private static void ReadStoreValues(Span values, object? storeValue, HeaderStoreItemInfo? info, HttpHeaderParser? parser, ref int currentIndex) + private static void ReadStoreValues(Span values, object? storeValue, HttpHeaderParser? parser, ref int currentIndex) { if (storeValue != null) { @@ -1212,7 +1255,7 @@ private static void ReadStoreValues(Span values, object? storeValue, if (storeValues == null) { - values[currentIndex] = parser == null || (info != null && info.IsInvalidValue(storeValue)) ? storeValue.ToString() : parser.ToString(storeValue); + values[currentIndex] = parser == null || storeValue is InvalidValue ? storeValue.ToString() : parser.ToString(storeValue); currentIndex++; } else @@ -1220,7 +1263,7 @@ private static void ReadStoreValues(Span values, object? storeValue, foreach (object? item in storeValues) { Debug.Assert(item != null); - values[currentIndex] = parser == null || (info != null && info.IsInvalidValue(item)) ? item.ToString() : parser.ToString(item); + values[currentIndex] = parser == null || item is InvalidValue ? item.ToString() : parser.ToString(item); currentIndex++; } } @@ -1240,88 +1283,18 @@ private bool AreEqual(object value, object? storeValue, IEqualityComparer? compa return value.Equals(storeValue); } + internal record InvalidValue(string value) + { + public override string ToString() => value; + } + internal sealed class HeaderStoreItemInfo { internal HeaderStoreItemInfo() { } internal object? RawValue; - internal object? InvalidValue; internal object? ParsedAndInvalidValues; - internal object? GetParsedValue() - { - if (InvalidValue is null) - { - return ParsedAndInvalidValues; - } - - var parsedAndInvalidValues = ParsedAndInvalidValues; - if (parsedAndInvalidValues is null) - { - return null; - } - - if (parsedAndInvalidValues is List list) - { - object? parsedValue = null; - foreach (object value in list) - { - if (!IsInvalidValue(value)) - { - if (parsedValue == null) - { - parsedValue = value; - } - else - { - List? storeValues = parsedValue as List; - - if (storeValues == null) - { - storeValues = new List(2); - storeValues.Add(parsedValue); - parsedValue = storeValues; - } - storeValues.Add(value); - } - } - } - - return parsedValue; - } - else - { - if (!IsInvalidValue(parsedAndInvalidValues)) - { - return parsedAndInvalidValues; - } - - return null; - } - } - - internal bool IsInvalidValue(object value) - { - if (InvalidValue == null) - { - return false; - } - if (InvalidValue is List list) - { - foreach (string item in list) - { - if (item.Equals(value)) - { - return true; - } - } - - return false; - } - - return InvalidValue.Equals(value); - } - internal bool CanAddParsedValue(HttpHeaderParser parser) { Debug.Assert(parser != null, "There should be no reason to call CanAddValue if there is no parser for the current header."); @@ -1335,10 +1308,10 @@ internal bool CanAddParsedValue(HttpHeaderParser parser) // supporting 1 value. When the first value gets parsed, CanAddValue returns true and we add the // parsed value to ParsedValue. When the second value is parsed, CanAddValue returns false, because // we have already a parsed value. - return parser.SupportsMultipleValues || ((InvalidValue == null) && (ParsedAndInvalidValues == null)); + return parser.SupportsMultipleValues || ParsedAndInvalidValues == null; } - internal bool IsEmpty => (RawValue == null) && (InvalidValue == null) && (ParsedAndInvalidValues == null); + internal bool IsEmpty => (RawValue == null) && ParsedAndInvalidValues == null; } From afd612a34b7ce1035f50a7e930cf4d1cf71dd8b9 Mon Sep 17 00:00:00 2001 From: feiyun0112 Date: Mon, 28 Feb 2022 13:34:07 +0800 Subject: [PATCH 07/27] make the changes @MihaZupan recommended --- .../Http/Headers/CacheControlHeaderParser.cs | 2 +- .../Net/Http/Headers/HeaderUtilities.cs | 4 +- .../Net/Http/Headers/HttpContentHeaders.cs | 12 +- .../Net/Http/Headers/HttpGeneralHeaders.cs | 2 +- .../Http/Headers/HttpHeaderValueCollection.cs | 28 +++- .../System/Net/Http/Headers/HttpHeaders.cs | 151 ++++++++++++------ .../Net/Http/Headers/HttpRequestHeaders.cs | 16 +- .../Net/Http/Headers/HttpResponseHeaders.cs | 6 +- .../Headers/HttpContentHeadersTest.cs | 22 +-- .../UnitTests/Headers/HttpHeadersTest.cs | 40 +++-- .../Headers/HttpRequestHeadersTest.cs | 55 ++++--- .../Headers/HttpResponseHeadersTest.cs | 14 +- 12 files changed, 215 insertions(+), 137 deletions(-) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/CacheControlHeaderParser.cs b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/CacheControlHeaderParser.cs index 90acab59864ed..b8b396f9ea613 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/CacheControlHeaderParser.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/CacheControlHeaderParser.cs @@ -1,4 +1,4 @@ -// Licensed to the .NET Foundation under one or more agreements. +// Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. using System.Diagnostics; diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HeaderUtilities.cs b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HeaderUtilities.cs index 6cc82433278f7..12ffbd45dee29 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HeaderUtilities.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HeaderUtilities.cs @@ -287,7 +287,7 @@ internal static int GetNextNonEmptyOrWhitespaceIndex(string input, int startInde { Debug.Assert(store != null); - object? storedValue = store.GetParsedValues(descriptor); + object? storedValue = store.GetSingleParsedValue(descriptor); if (storedValue != null) { return (DateTimeOffset)storedValue; @@ -304,7 +304,7 @@ internal static int GetNextNonEmptyOrWhitespaceIndex(string input, int startInde { Debug.Assert(store != null); - object? storedValue = store.GetParsedValues(descriptor); + object? storedValue = store.GetSingleParsedValue(descriptor); if (storedValue != null) { return (TimeSpan)storedValue; diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpContentHeaders.cs b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpContentHeaders.cs index 361e27bd8f059..73f93e6bfccf4 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpContentHeaders.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpContentHeaders.cs @@ -19,7 +19,7 @@ public sealed class HttpContentHeaders : HttpHeaders public ContentDispositionHeaderValue? ContentDisposition { - get { return (ContentDispositionHeaderValue?)GetParsedValues(KnownHeaders.ContentDisposition.Descriptor); } + get { return (ContentDispositionHeaderValue?)GetSingleParsedValue(KnownHeaders.ContentDisposition.Descriptor); } set { SetOrRemoveParsedValue(KnownHeaders.ContentDisposition.Descriptor, value); } } @@ -36,7 +36,7 @@ public long? ContentLength get { // 'Content-Length' can only hold one value. So either we get 'null' back or a boxed long value. - object? storedValue = GetParsedValues(KnownHeaders.ContentLength.Descriptor); + object? storedValue = GetSingleParsedValue(KnownHeaders.ContentLength.Descriptor); // Only try to calculate the length if the user didn't set the value explicitly using the setter. if (!_contentLengthSet && (storedValue == null)) @@ -72,25 +72,25 @@ public long? ContentLength public Uri? ContentLocation { - get { return (Uri?)GetParsedValues(KnownHeaders.ContentLocation.Descriptor); } + get { return (Uri?)GetSingleParsedValue(KnownHeaders.ContentLocation.Descriptor); } set { SetOrRemoveParsedValue(KnownHeaders.ContentLocation.Descriptor, value); } } public byte[]? ContentMD5 { - get { return (byte[]?)GetParsedValues(KnownHeaders.ContentMD5.Descriptor); } + get { return (byte[]?)GetSingleParsedValue(KnownHeaders.ContentMD5.Descriptor); } set { SetOrRemoveParsedValue(KnownHeaders.ContentMD5.Descriptor, value); } } public ContentRangeHeaderValue? ContentRange { - get { return (ContentRangeHeaderValue?)GetParsedValues(KnownHeaders.ContentRange.Descriptor); } + get { return (ContentRangeHeaderValue?)GetSingleParsedValue(KnownHeaders.ContentRange.Descriptor); } set { SetOrRemoveParsedValue(KnownHeaders.ContentRange.Descriptor, value); } } public MediaTypeHeaderValue? ContentType { - get { return (MediaTypeHeaderValue?)GetParsedValues(KnownHeaders.ContentType.Descriptor); } + get { return (MediaTypeHeaderValue?)GetSingleParsedValue(KnownHeaders.ContentType.Descriptor); } set { SetOrRemoveParsedValue(KnownHeaders.ContentType.Descriptor, value); } } diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpGeneralHeaders.cs b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpGeneralHeaders.cs index 4bd4694f4d5d3..036834930d820 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpGeneralHeaders.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpGeneralHeaders.cs @@ -22,7 +22,7 @@ internal sealed class HttpGeneralHeaders public CacheControlHeaderValue? CacheControl { - get { return (CacheControlHeaderValue?)_parent.GetParsedValues(KnownHeaders.CacheControl.Descriptor); } + get { return (CacheControlHeaderValue?)_parent.GetSingleParsedValue(KnownHeaders.CacheControl.Descriptor); } set { _parent.SetOrRemoveParsedValue(KnownHeaders.CacheControl.Descriptor, value); } } diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaderValueCollection.cs b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaderValueCollection.cs index 7ae54578ed03d..bbd7ea0d42897 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaderValueCollection.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaderValueCollection.cs @@ -86,7 +86,7 @@ public void CopyTo(T[] array!!, int arrayIndex) throw new ArgumentOutOfRangeException(nameof(arrayIndex)); } - object? storeValue = _store.GetParsedValues(_descriptor); + object? storeValue = _store.GetParsedAndInvalidValues(_descriptor); if (storeValue == null) { @@ -122,8 +122,8 @@ public bool Remove(T item) public IEnumerator GetEnumerator() { - object? storeValue = _store.GetParsedValues(_descriptor); - return storeValue is null ? + object? storeValue = _store.GetParsedAndInvalidValues(_descriptor); + return storeValue is null || storeValue is HttpHeaders.InvalidValue ? ((IEnumerable)Array.Empty()).GetEnumerator() : // use singleton empty array enumerator Iterate(storeValue); @@ -134,6 +134,10 @@ static IEnumerator Iterate(object storeValue) // We have multiple values. Iterate through the values and return them. foreach (object item in storeValues) { + if (item is HttpHeaders.InvalidValue) + { + continue; + } Debug.Assert(item is T); yield return (T)item; } @@ -178,7 +182,7 @@ private int GetCount() { // This is an O(n) operation. - object? storeValue = _store.GetParsedValues(_descriptor); + object? storeValue = _store.GetParsedAndInvalidValues(_descriptor); if (storeValue == null) { @@ -189,11 +193,23 @@ private int GetCount() if (storeValues == null) { - return 1; + if (storeValue is not HttpHeaders.InvalidValue) + { + return 1; + } + return 0; } else { - return storeValues.Count; + int count = 0; + foreach (object item in storeValues) + { + if (item is not HttpHeaders.InvalidValue) + { + count++; + } + } + return count; } } } diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs index 6d964f35c47fb..4f01d53f3e9a6 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs @@ -87,6 +87,7 @@ internal void Add(HeaderDescriptor descriptor, string? value) // it to the store if we added at least one value. if (addToStore && (info.ParsedAndInvalidValues != null)) { + info.AssertContainsNoInvalidValues(); Debug.Assert(!ContainsKey(descriptor)); AddEntryToStore(new HeaderEntry(descriptor, info)); } @@ -116,6 +117,7 @@ internal void Add(HeaderDescriptor descriptor, IEnumerable values!!) // However, if all values for a _new_ header were invalid, then don't add the header. if (addToStore && (info.ParsedAndInvalidValues != null)) { + info.AssertContainsNoInvalidValues(); Debug.Assert(!ContainsKey(descriptor)); AddEntryToStore(new HeaderEntry(descriptor, info)); } @@ -446,10 +448,10 @@ internal bool RemoveParsedValue(HeaderDescriptor descriptor, object value) { foreach (object item in parsedValues) { - if (parsedValue is not InvalidValue) + if (item is not InvalidValue) { Debug.Assert(item.GetType() == value.GetType(), - "One of the stored values does not have the same type as 'value'."); + "One of the stored values does not have the same type as 'value'."); if (AreEqual(value, item, comparer)) { @@ -465,6 +467,7 @@ internal bool RemoveParsedValue(HeaderDescriptor descriptor, object value) // If we removed the last item in a list, remove the list. if (parsedValues.Count == 0) { + info.AssertContainsNoInvalidValues(); info.ParsedAndInvalidValues = null; } } @@ -519,10 +522,10 @@ internal bool ContainsParsedValue(HeaderDescriptor descriptor, object value) { foreach (object item in parsedValues) { - if (parsedValue is not InvalidValue) + if (item is not InvalidValue) { Debug.Assert(item.GetType() == value.GetType(), - "One of the stored values does not have the same type as 'value'."); + "One of the stored values does not have the same type as 'value'."); if (AreEqual(value, item, comparer)) { @@ -819,6 +822,7 @@ internal bool TryParseAndAddValue(HeaderDescriptor descriptor, string? value) if (result && addToStore && (info.ParsedAndInvalidValues != null)) { + info.AssertContainsNoInvalidValues(); // If we get here, then the value could be parsed correctly. If we created a new HeaderStoreItemInfo, add // it to the store if we added at least one value. Debug.Assert(!ContainsKey(descriptor)); @@ -949,59 +953,24 @@ private static void AddValueToStoreValue(T value, ref object? currentStoreVal } } - // Since most of the time we just have 1 value, we don't create a List for one value, but we change - // the return type to 'object'. The caller has to deal with the return type (object vs. List). This - // is to optimize the most common scenario where a header has only one value. - internal object? GetParsedValues(HeaderDescriptor descriptor) + internal object? GetSingleParsedValue(HeaderDescriptor descriptor) { if (!TryGetAndParseHeaderInfo(descriptor, out HeaderStoreItemInfo? info)) { return null; } - var parsedAndInvalidValues = info.ParsedAndInvalidValues; - if (parsedAndInvalidValues is null) - { - return null; - } - - if (parsedAndInvalidValues is List list) - { - object? parsedValue = null; - foreach (object value in list) - { - if (value is not InvalidValue) - { - if (parsedValue == null) - { - parsedValue = value; - } - else - { - List? storeValues = parsedValue as List; - - if (storeValues == null) - { - storeValues = new List(2); - storeValues.Add(parsedValue); - parsedValue = storeValues; - } - storeValues.Add(value); - } - } - } + return info.GetSingleParsedValue(); + } - return parsedValue; - } - else + internal object? GetParsedAndInvalidValues(HeaderDescriptor descriptor) + { + if (!TryGetAndParseHeaderInfo(descriptor, out HeaderStoreItemInfo? info)) { - if (parsedAndInvalidValues is not InvalidValue) - { - return parsedAndInvalidValues; - } - return null; } + + return info.ParsedAndInvalidValues; } internal virtual bool IsAllowedHeaderName(HeaderDescriptor descriptor) => true; @@ -1308,9 +1277,95 @@ internal bool CanAddParsedValue(HttpHeaderParser parser) // supporting 1 value. When the first value gets parsed, CanAddValue returns true and we add the // parsed value to ParsedValue. When the second value is parsed, CanAddValue returns false, because // we have already a parsed value. - return parser.SupportsMultipleValues || ParsedAndInvalidValues == null; + return parser.SupportsMultipleValues || !ContainsParsedValue(); + } + + private bool ContainsParsedValue() + { + if (ParsedAndInvalidValues is not null) + { + if (ParsedAndInvalidValues is List list) + { + foreach (object item in list) + { + if (item is not InvalidValue) + { + return true; + } + } + } + else + { + if (ParsedAndInvalidValues is not InvalidValue) + { + return true; + } + } + } + + return false; } + [Conditional("DEBUG")] + public void AssertContainsNoInvalidValues() + { + if (ParsedAndInvalidValues is not null) + { + if (ParsedAndInvalidValues is List list) + { + foreach (object item in list) + { + Debug.Assert(item is not InvalidValue); + } + } + else + { + Debug.Assert(ParsedAndInvalidValues is not InvalidValue); + } + } + } + + internal object? GetSingleParsedValue() + { + if (ParsedAndInvalidValues is not null) + { + if (ParsedAndInvalidValues is List list) + { + AssertContainsSingleParsedValue(list); + foreach (object item in list) + { + if (item is not InvalidValue) + { + return item; + } + } + } + else + { + if (ParsedAndInvalidValues is not InvalidValue) + { + return ParsedAndInvalidValues; + } + } + } + + return null; + } + + [Conditional("DEBUG")] + public void AssertContainsSingleParsedValue(List list) + { + int count = 0; + foreach (object item in list) + { + if (item is not InvalidValue) + { + count++; + } + } + + Debug.Assert(count <= 1, "only allow single parsed value"); + } internal bool IsEmpty => (RawValue == null) && ParsedAndInvalidValues == null; } diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpRequestHeaders.cs b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpRequestHeaders.cs index 9a278ff118319..5da7cfb9844ed 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpRequestHeaders.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpRequestHeaders.cs @@ -47,7 +47,7 @@ private T GetSpecializedCollection(int slot, Func crea public AuthenticationHeaderValue? Authorization { - get { return (AuthenticationHeaderValue?)GetParsedValues(KnownHeaders.Authorization.Descriptor); } + get { return (AuthenticationHeaderValue?)GetSingleParsedValue(KnownHeaders.Authorization.Descriptor); } set { SetOrRemoveParsedValue(KnownHeaders.Authorization.Descriptor, value); } } @@ -87,7 +87,7 @@ public bool? ExpectContinue public string? From { - get { return (string?)GetParsedValues(KnownHeaders.From.Descriptor); } + get { return (string?)GetSingleParsedValue(KnownHeaders.From.Descriptor); } set { // Null and empty string are equivalent. In this case it means, remove the From header value (if any). @@ -104,7 +104,7 @@ public string? From public string? Host { - get { return (string?)GetParsedValues(KnownHeaders.Host.Descriptor); } + get { return (string?)GetSingleParsedValue(KnownHeaders.Host.Descriptor); } set { // Null and empty string are equivalent. In this case it means, remove the Host header value (if any). @@ -135,7 +135,7 @@ public DateTimeOffset? IfModifiedSince public RangeConditionHeaderValue? IfRange { - get { return (RangeConditionHeaderValue?)GetParsedValues(KnownHeaders.IfRange.Descriptor); } + get { return (RangeConditionHeaderValue?)GetSingleParsedValue(KnownHeaders.IfRange.Descriptor); } set { SetOrRemoveParsedValue(KnownHeaders.IfRange.Descriptor, value); } } @@ -149,7 +149,7 @@ public int? MaxForwards { get { - object? storedValue = GetParsedValues(KnownHeaders.MaxForwards.Descriptor); + object? storedValue = GetSingleParsedValue(KnownHeaders.MaxForwards.Descriptor); if (storedValue != null) { return (int)storedValue; @@ -162,19 +162,19 @@ public int? MaxForwards public AuthenticationHeaderValue? ProxyAuthorization { - get { return (AuthenticationHeaderValue?)GetParsedValues(KnownHeaders.ProxyAuthorization.Descriptor); } + get { return (AuthenticationHeaderValue?)GetSingleParsedValue(KnownHeaders.ProxyAuthorization.Descriptor); } set { SetOrRemoveParsedValue(KnownHeaders.ProxyAuthorization.Descriptor, value); } } public RangeHeaderValue? Range { - get { return (RangeHeaderValue?)GetParsedValues(KnownHeaders.Range.Descriptor); } + get { return (RangeHeaderValue?)GetSingleParsedValue(KnownHeaders.Range.Descriptor); } set { SetOrRemoveParsedValue(KnownHeaders.Range.Descriptor, value); } } public Uri? Referrer { - get { return (Uri?)GetParsedValues(KnownHeaders.Referer.Descriptor); } + get { return (Uri?)GetSingleParsedValue(KnownHeaders.Referer.Descriptor); } set { SetOrRemoveParsedValue(KnownHeaders.Referer.Descriptor, value); } } diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpResponseHeaders.cs b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpResponseHeaders.cs index 442db3beb2f49..8795e39f9ea5b 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpResponseHeaders.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpResponseHeaders.cs @@ -45,13 +45,13 @@ public TimeSpan? Age public EntityTagHeaderValue? ETag { - get { return (EntityTagHeaderValue?)GetParsedValues(KnownHeaders.ETag.Descriptor); } + get { return (EntityTagHeaderValue?)GetSingleParsedValue(KnownHeaders.ETag.Descriptor); } set { SetOrRemoveParsedValue(KnownHeaders.ETag.Descriptor, value); } } public Uri? Location { - get { return (Uri?)GetParsedValues(KnownHeaders.Location.Descriptor); } + get { return (Uri?)GetSingleParsedValue(KnownHeaders.Location.Descriptor); } set { SetOrRemoveParsedValue(KnownHeaders.Location.Descriptor, value); } } @@ -60,7 +60,7 @@ public Uri? Location public RetryConditionHeaderValue? RetryAfter { - get { return (RetryConditionHeaderValue?)GetParsedValues(KnownHeaders.RetryAfter.Descriptor); } + get { return (RetryConditionHeaderValue?)GetSingleParsedValue(KnownHeaders.RetryAfter.Descriptor); } set { SetOrRemoveParsedValue(KnownHeaders.RetryAfter.Descriptor, value); } } diff --git a/src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpContentHeadersTest.cs b/src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpContentHeadersTest.cs index b50b28b56fd1b..752926313ebe1 100644 --- a/src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpContentHeadersTest.cs +++ b/src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpContentHeadersTest.cs @@ -34,7 +34,7 @@ public void ContentLength_ReadValue_TryComputeLengthInvoked() // The delegate is invoked to return the length. Assert.Equal(15, _headers.ContentLength); - Assert.Equal((long)15, _headers.GetParsedValues(KnownHeaders.ContentLength.Descriptor)); + Assert.Equal((long)15, _headers.GetSingleParsedValue(KnownHeaders.ContentLength.Descriptor)); // After getting the calculated content length, set it to null. _headers.ContentLength = null; @@ -43,7 +43,7 @@ public void ContentLength_ReadValue_TryComputeLengthInvoked() _headers.ContentLength = 27; Assert.Equal((long)27, _headers.ContentLength); - Assert.Equal((long)27, _headers.GetParsedValues(KnownHeaders.ContentLength.Descriptor)); + Assert.Equal((long)27, _headers.GetSingleParsedValue(KnownHeaders.ContentLength.Descriptor)); } [Fact] @@ -53,7 +53,7 @@ public void ContentLength_SetCustomValue_TryComputeLengthNotInvoked() _headers.ContentLength = 27; Assert.Equal((long)27, _headers.ContentLength); - Assert.Equal((long)27, _headers.GetParsedValues(KnownHeaders.ContentLength.Descriptor)); + Assert.Equal((long)27, _headers.GetSingleParsedValue(KnownHeaders.ContentLength.Descriptor)); // After explicitly setting the content length, set it to null. _headers.ContentLength = null; @@ -177,13 +177,13 @@ public void ContentLocation_UseAddMethod_AddedValueCanBeRetrievedUsingProperty() public void ContentLocation_UseAddMethodWithInvalidValue_InvalidValueRecognized() { _headers.TryAddWithoutValidation("Content-Location", " http://example.com http://other"); - Assert.Null(_headers.GetParsedValues(KnownHeaders.ContentLocation.Descriptor)); + Assert.Null(_headers.GetSingleParsedValue(KnownHeaders.ContentLocation.Descriptor)); Assert.Equal(1, _headers.GetValues("Content-Location").Count()); Assert.Equal(" http://example.com http://other", _headers.GetValues("Content-Location").First()); _headers.Clear(); _headers.TryAddWithoutValidation("Content-Location", "http://host /other"); - Assert.Null(_headers.GetParsedValues(KnownHeaders.ContentLocation.Descriptor)); + Assert.Null(_headers.GetSingleParsedValue(KnownHeaders.ContentLocation.Descriptor)); Assert.Equal(1, _headers.GetValues("Content-Location").Count()); Assert.Equal("http://host /other", _headers.GetValues("Content-Location").First()); } @@ -315,13 +315,13 @@ public void ContentMD5_UseAddMethod_AddedValueCanBeRetrievedUsingProperty() public void ContentMD5_UseAddMethodWithInvalidValue_InvalidValueRecognized() { _headers.TryAddWithoutValidation("Content-MD5", "AQ--"); - Assert.Null(_headers.GetParsedValues(KnownHeaders.ContentMD5.Descriptor)); + Assert.Null(_headers.GetSingleParsedValue(KnownHeaders.ContentMD5.Descriptor)); Assert.Equal(1, _headers.GetValues("Content-MD5").Count()); Assert.Equal("AQ--", _headers.GetValues("Content-MD5").First()); _headers.Clear(); _headers.TryAddWithoutValidation("Content-MD5", "AQ==, CD"); - Assert.Null(_headers.GetParsedValues(KnownHeaders.ContentMD5.Descriptor)); + Assert.Null(_headers.GetSingleParsedValue(KnownHeaders.ContentMD5.Descriptor)); Assert.Equal(1, _headers.GetValues("Content-MD5").Count()); Assert.Equal("AQ==, CD", _headers.GetValues("Content-MD5").First()); } @@ -401,13 +401,13 @@ public void Expires_UseAddMethod_AddedValueCanBeRetrievedUsingProperty() public void Expires_UseAddMethodWithInvalidValue_InvalidValueRecognized() { _headers.TryAddWithoutValidation("Expires", " Sun, 06 Nov 1994 08:49:37 GMT ,"); - Assert.Null(_headers.GetParsedValues(KnownHeaders.Expires.Descriptor)); + Assert.Null(_headers.GetSingleParsedValue(KnownHeaders.Expires.Descriptor)); Assert.Equal(1, _headers.GetValues("Expires").Count()); Assert.Equal(" Sun, 06 Nov 1994 08:49:37 GMT ,", _headers.GetValues("Expires").First()); _headers.Clear(); _headers.TryAddWithoutValidation("Expires", " Sun, 06 Nov "); - Assert.Null(_headers.GetParsedValues(KnownHeaders.Expires.Descriptor)); + Assert.Null(_headers.GetSingleParsedValue(KnownHeaders.Expires.Descriptor)); Assert.Equal(1, _headers.GetValues("Expires").Count()); Assert.Equal(" Sun, 06 Nov ", _headers.GetValues("Expires").First()); } @@ -441,13 +441,13 @@ public void LastModified_UseAddMethod_AddedValueCanBeRetrievedUsingProperty() public void LastModified_UseAddMethodWithInvalidValue_InvalidValueRecognized() { _headers.TryAddWithoutValidation("Last-Modified", " Sun, 06 Nov 1994 08:49:37 GMT ,"); - Assert.Null(_headers.GetParsedValues(KnownHeaders.LastModified.Descriptor)); + Assert.Null(_headers.GetSingleParsedValue(KnownHeaders.LastModified.Descriptor)); Assert.Equal(1, _headers.GetValues("Last-Modified").Count()); Assert.Equal(" Sun, 06 Nov 1994 08:49:37 GMT ,", _headers.GetValues("Last-Modified").First()); _headers.Clear(); _headers.TryAddWithoutValidation("Last-Modified", " Sun, 06 Nov "); - Assert.Null(_headers.GetParsedValues(KnownHeaders.LastModified.Descriptor)); + Assert.Null(_headers.GetSingleParsedValue(KnownHeaders.LastModified.Descriptor)); Assert.Equal(1, _headers.GetValues("Last-Modified").Count()); Assert.Equal(" Sun, 06 Nov ", _headers.GetValues("Last-Modified").First()); } diff --git a/src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpHeadersTest.cs b/src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpHeadersTest.cs index 756788955ad9d..7ccb14524ffa6 100644 --- a/src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpHeadersTest.cs +++ b/src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpHeadersTest.cs @@ -200,8 +200,7 @@ public void TryAddWithoutValidation_AddValidAndInvalidValueString_BothValuesPars // - There are multiple header values (some valid, some invalid) AND // - The order of the headers matters (e.g. Transfer-Encoding) Assert.Equal(invalidHeaderValue, headers.First().Value.ElementAt(0)); - Assert.Equal(parsedPrefix, headers.First().Value.ElementAt(1)); - + Assert.Equal(parsedPrefix, headers.First().Value.ElementAt(1)); Assert.Equal(2, headers.Parser.TryParseValueCallCount); string expected = headers.Descriptor.Name + ": " + invalidHeaderValue + ", " + parsedPrefix + Environment.NewLine; @@ -1265,34 +1264,34 @@ public void GetValues_HeadersWithEmptyValues_ReturnsEmptyArray() } [Fact] - public void GetParsedValues_GetValuesFromUninitializedHeaderStore_ReturnsNull() + public void GetSingleParsedValue_GetValuesFromUninitializedHeaderStore_ReturnsNull() { MockHeaders headers = new MockHeaders(); // Get header values from uninitialized store (store collection is null). - object storeValue = headers.GetParsedValues(customHeader); + object storeValue = headers.GetSingleParsedValue(customHeader); Assert.Null(storeValue); } [Fact] - public void GetParsedValues_GetValuesForNonExistingHeader_ReturnsNull() + public void GetSingleParsedValue_GetValuesForNonExistingHeader_ReturnsNull() { MockHeaders headers = new MockHeaders(); headers.Add("custom1", "customValue1"); // Get header values for non-existing header (but other headers exist in the store). - object storeValue = headers.GetParsedValues(customHeader); + object storeValue = headers.GetSingleParsedValue(customHeader); Assert.Null(storeValue); } [Fact] - public void GetParsedValues_GetSingleValueForExistingHeader_ReturnsAddedValue() + public void GetSingleParsedValue_GetSingleValueForExistingHeader_ReturnsAddedValue() { MockHeaders headers = new MockHeaders(); headers.Add(customHeader.Name, "customValue1"); // Get header values for non-existing header (but other headers exist in the store). - object storeValue = headers.GetParsedValues(customHeader); + object storeValue = headers.GetSingleParsedValue(customHeader); Assert.NotNull(storeValue); // If we only have one value, then GetValues() should return just the value and not wrap it in a List. @@ -1300,17 +1299,17 @@ public void GetParsedValues_GetSingleValueForExistingHeader_ReturnsAddedValue() } [Fact] - public void GetParsedValues_HeaderWithEmptyValues_ReturnsEmpty() + public void GetSingleParsedValue_HeaderWithEmptyValues_ReturnsEmpty() { MockHeaders headers = new MockHeaders(); headers.Add(headers.Descriptor, string.Empty); - object storeValue = headers.GetParsedValues(headers.Descriptor); + object storeValue = headers.GetSingleParsedValue(headers.Descriptor); Assert.Null(storeValue); } [Fact] - public void GetParsedValues_GetMultipleValuesForExistingHeader_ReturnsListOfValues() + public void GetSingleParsedValue_GetMultipleValuesForExistingHeader_ReturnsListOfValues() { MockHeaders headers = new MockHeaders(); headers.TryAddWithoutValidation("custom", rawPrefix + "0"); // this must not influence the result. @@ -1320,7 +1319,7 @@ public void GetParsedValues_GetMultipleValuesForExistingHeader_ReturnsListOfValu // Only 1 value should get parsed (call to Add() with known header value). Assert.Equal(1, headers.Parser.TryParseValueCallCount); - object storeValue = headers.GetParsedValues(headers.Descriptor); + object storeValue = headers.GetParsedAndInvalidValues(headers.Descriptor); Assert.NotNull(storeValue); // GetValues() should trigger parsing of values added with TryAddWithoutValidation() @@ -1336,7 +1335,7 @@ public void GetParsedValues_GetMultipleValuesForExistingHeader_ReturnsListOfValu } [Fact] - public void GetParsedValues_GetValuesForExistingHeaderWithInvalidValues_ReturnsOnlyParsedValues() + public void GetSingleParsedValue_GetValuesForExistingHeaderWithInvalidValues_ReturnsOnlyParsedValues() { MockHeaders headers = new MockHeaders(); headers.Add(headers.Descriptor, rawPrefix); @@ -1349,7 +1348,7 @@ public void GetParsedValues_GetValuesForExistingHeaderWithInvalidValues_ReturnsO // Only 1 value should get parsed (call to Add() with known header value). Assert.Equal(1, headers.Parser.TryParseValueCallCount); - object storeValue = headers.GetParsedValues(headers.Descriptor); + object storeValue = headers.GetSingleParsedValue(headers.Descriptor); Assert.NotNull(storeValue); // GetValues() should trigger parsing of values added with TryAddWithoutValidation() @@ -1360,7 +1359,7 @@ public void GetParsedValues_GetValuesForExistingHeaderWithInvalidValues_ReturnsO } [Fact] - public void GetParsedValues_GetValuesForExistingHeaderWithOnlyInvalidValues_ReturnsEmptyEnumerator() + public void GetSingleParsedValue_GetValuesForExistingHeaderWithOnlyInvalidValues_ReturnsEmptyEnumerator() { MockHeaders headers = new MockHeaders(); @@ -1371,7 +1370,7 @@ public void GetParsedValues_GetValuesForExistingHeaderWithOnlyInvalidValues_Retu Assert.Equal(0, headers.Parser.TryParseValueCallCount); - object storeValue = headers.GetParsedValues(headers.Descriptor); + object storeValue = headers.GetSingleParsedValue(headers.Descriptor); Assert.Null(storeValue); // GetValues() should trigger parsing of values added with TryAddWithoutValidation() @@ -1379,20 +1378,20 @@ public void GetParsedValues_GetValuesForExistingHeaderWithOnlyInvalidValues_Retu } [Fact] - public void GetParsedValues_AddInvalidValueToHeader_HeaderGetsRemovedAndNullReturned() + public void GetSingleParsedValue_AddInvalidValueToHeader_HeaderGetsRemovedAndNullReturned() { MockHeaders headers = new MockHeaders(); headers.TryAddWithoutValidation(headers.Descriptor, invalidHeaderValue + "\r\ninvalid"); Assert.Equal(0, headers.Parser.TryParseValueCallCount); - object storeValue = headers.GetParsedValues(headers.Descriptor); + object storeValue = headers.GetSingleParsedValue(headers.Descriptor); Assert.Null(storeValue); Assert.False(headers.Contains(headers.Descriptor)); } [Fact] - public void GetParsedValues_GetParsedValuesForKnownHeaderWithNewlineChars_ReturnsNull() + public void GetSingleParsedValue_GetSingleParsedValueForKnownHeaderWithNewlineChars_ReturnsNull() { MockHeaders headers = new MockHeaders(); @@ -1400,7 +1399,7 @@ public void GetParsedValues_GetParsedValuesForKnownHeaderWithNewlineChars_Return headers.TryAddWithoutValidation(headers.Descriptor, invalidHeaderValue + "\r\ninvalid"); Assert.Equal(0, headers.Parser.TryParseValueCallCount); - Assert.Null(headers.GetParsedValues(headers.Descriptor)); + Assert.Null(headers.GetSingleParsedValue(headers.Descriptor)); Assert.Equal(0, headers.Count()); Assert.Equal(1, headers.Parser.TryParseValueCallCount); } @@ -2067,7 +2066,6 @@ public void AddHeaders_SourceAndDestinationStoreHaveMultipleHeaders_OnlyHeadersN Assert.Equal(parsedPrefix + "5", destination.GetValues(known2Header).ElementAt(0)); Assert.Equal(invalidHeaderValue, destination.GetValues(known2Header).ElementAt(1)); Assert.Equal(parsedPrefix + "7", destination.GetValues(known2Header).ElementAt(2)); - // Header 'known3' should not be copied, since it doesn't contain any values. Assert.False(destination.Contains(known3Header), "'known3' header value count."); diff --git a/src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpRequestHeadersTest.cs b/src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpRequestHeadersTest.cs index 81c520110397c..f1938f21ea3c6 100644 --- a/src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpRequestHeadersTest.cs +++ b/src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpRequestHeadersTest.cs @@ -633,26 +633,26 @@ public void UserAgent_TryGetValuesAndGetValues_Malformed() public void UserAgent_UseAddMethodWithInvalidValue_InvalidValueRecognized() { headers.TryAddWithoutValidation("User-Agent", "custom\u4F1A"); - Assert.Null(headers.GetParsedValues(KnownHeaders.UserAgent.Descriptor)); + Assert.Null(headers.GetSingleParsedValue(KnownHeaders.UserAgent.Descriptor)); Assert.Equal(1, headers.GetValues("User-Agent").Count()); Assert.Equal("custom\u4F1A", headers.GetValues("User-Agent").First()); headers.Clear(); // Note that "User-Agent" uses whitespace as separators, so the following is an invalid value headers.TryAddWithoutValidation("User-Agent", "custom1, custom2"); - Assert.Null(headers.GetParsedValues(KnownHeaders.UserAgent.Descriptor)); + Assert.Null(headers.GetSingleParsedValue(KnownHeaders.UserAgent.Descriptor)); Assert.Equal(1, headers.GetValues("User-Agent").Count()); Assert.Equal("custom1, custom2", headers.GetValues("User-Agent").First()); headers.Clear(); headers.TryAddWithoutValidation("User-Agent", "custom1, "); - Assert.Null(headers.GetParsedValues(KnownHeaders.UserAgent.Descriptor)); + Assert.Null(headers.GetSingleParsedValue(KnownHeaders.UserAgent.Descriptor)); Assert.Equal(1, headers.GetValues("User-Agent").Count()); Assert.Equal("custom1, ", headers.GetValues("User-Agent").First()); headers.Clear(); headers.TryAddWithoutValidation("User-Agent", ",custom1"); - Assert.Null(headers.GetParsedValues(KnownHeaders.UserAgent.Descriptor)); + Assert.Null(headers.GetSingleParsedValue(KnownHeaders.UserAgent.Descriptor)); Assert.Equal(1, headers.GetValues("User-Agent").Count()); Assert.Equal(",custom1", headers.GetValues("User-Agent").First()); } @@ -704,13 +704,13 @@ public void IfRange_UseAddMethod_AddedValueCanBeRetrievedUsingProperty() public void IfRange_UseAddMethodWithInvalidValue_InvalidValueRecognized() { headers.TryAddWithoutValidation("If-Range", "\"tag\"\u4F1A"); - Assert.Null(headers.GetParsedValues(KnownHeaders.IfRange.Descriptor)); + Assert.Null(headers.GetSingleParsedValue(KnownHeaders.IfRange.Descriptor)); Assert.Equal(1, headers.GetValues("If-Range").Count()); Assert.Equal("\"tag\"\u4F1A", headers.GetValues("If-Range").First()); headers.Clear(); headers.TryAddWithoutValidation("If-Range", " \"tag\", "); - Assert.Null(headers.GetParsedValues(KnownHeaders.IfRange.Descriptor)); + Assert.Null(headers.GetSingleParsedValue(KnownHeaders.IfRange.Descriptor)); Assert.Equal(1, headers.GetValues("If-Range").Count()); Assert.Equal(" \"tag\", ", headers.GetValues("If-Range").First()); } @@ -759,13 +759,13 @@ public void From_UseAddMethodWithInvalidValue_InvalidValueRecognized() { // values are not validated, so invalid values are accepted headers.TryAddWithoutValidation("From", " info@example.com ,"); - Assert.Equal("info@example.com ,", headers.GetParsedValues(KnownHeaders.From.Descriptor)); + Assert.Equal("info@example.com ,", headers.GetSingleParsedValue(KnownHeaders.From.Descriptor)); Assert.Equal(1, headers.GetValues("From").Count()); Assert.Equal("info@example.com ,", headers.GetValues("From").First()); headers.Clear(); headers.TryAddWithoutValidation("From", "info@"); - Assert.Equal("info@", headers.GetParsedValues(KnownHeaders.From.Descriptor)); + Assert.Equal("info@", headers.GetSingleParsedValue(KnownHeaders.From.Descriptor)); Assert.Equal(1, headers.GetValues("From").Count()); Assert.Equal("info@", headers.GetValues("From").First()); } @@ -807,13 +807,13 @@ public void IfModifiedSince_UseAddMethod_AddedValueCanBeRetrievedUsingProperty() public void IfModifiedSince_UseAddMethodWithInvalidValue_InvalidValueRecognized() { headers.TryAddWithoutValidation("If-Modified-Since", " Sun, 06 Nov 1994 08:49:37 GMT ,"); - Assert.Null(headers.GetParsedValues(KnownHeaders.IfModifiedSince.Descriptor)); + Assert.Null(headers.GetSingleParsedValue(KnownHeaders.IfModifiedSince.Descriptor)); Assert.Equal(1, headers.GetValues("If-Modified-Since").Count()); Assert.Equal(" Sun, 06 Nov 1994 08:49:37 GMT ,", headers.GetValues("If-Modified-Since").First()); headers.Clear(); headers.TryAddWithoutValidation("If-Modified-Since", " Sun, 06 Nov "); - Assert.Null(headers.GetParsedValues(KnownHeaders.IfModifiedSince.Descriptor)); + Assert.Null(headers.GetSingleParsedValue(KnownHeaders.IfModifiedSince.Descriptor)); Assert.Equal(1, headers.GetValues("If-Modified-Since").Count()); Assert.Equal(" Sun, 06 Nov ", headers.GetValues("If-Modified-Since").First()); } @@ -848,13 +848,13 @@ public void IfUnmodifiedSince_UseAddMethod_AddedValueCanBeRetrievedUsingProperty public void IfUnmodifiedSince_UseAddMethodWithInvalidValue_InvalidValueRecognized() { headers.TryAddWithoutValidation("If-Unmodified-Since", " Sun, 06 Nov 1994 08:49:37 GMT ,"); - Assert.Null(headers.GetParsedValues(KnownHeaders.IfUnmodifiedSince.Descriptor)); + Assert.Null(headers.GetSingleParsedValue(KnownHeaders.IfUnmodifiedSince.Descriptor)); Assert.Equal(1, headers.GetValues("If-Unmodified-Since").Count()); Assert.Equal(" Sun, 06 Nov 1994 08:49:37 GMT ,", headers.GetValues("If-Unmodified-Since").First()); headers.Clear(); headers.TryAddWithoutValidation("If-Unmodified-Since", " Sun, 06 Nov "); - Assert.Null(headers.GetParsedValues(KnownHeaders.IfUnmodifiedSince.Descriptor)); + Assert.Null(headers.GetSingleParsedValue(KnownHeaders.IfUnmodifiedSince.Descriptor)); Assert.Equal(1, headers.GetValues("If-Unmodified-Since").Count()); Assert.Equal(" Sun, 06 Nov ", headers.GetValues("If-Unmodified-Since").First()); } @@ -889,13 +889,13 @@ public void Referrer_UseAddMethod_AddedValueCanBeRetrievedUsingProperty() public void Referrer_UseAddMethodWithInvalidValue_InvalidValueRecognized() { headers.TryAddWithoutValidation("Referer", " http://example.com http://other"); - Assert.Null(headers.GetParsedValues(KnownHeaders.Referer.Descriptor)); + Assert.Null(headers.GetSingleParsedValue(KnownHeaders.Referer.Descriptor)); Assert.Equal(1, headers.GetValues("Referer").Count()); Assert.Equal(" http://example.com http://other", headers.GetValues("Referer").First()); headers.Clear(); headers.TryAddWithoutValidation("Referer", "http://host /other"); - Assert.Null(headers.GetParsedValues(KnownHeaders.Referer.Descriptor)); + Assert.Null(headers.GetSingleParsedValue(KnownHeaders.Referer.Descriptor)); Assert.Equal(1, headers.GetValues("Referer").Count()); Assert.Equal("http://host /other", headers.GetValues("Referer").First()); } @@ -933,13 +933,13 @@ public void MaxForwards_UseAddMethod_AddedValueCanBeRetrievedUsingProperty() public void MaxForwards_UseAddMethodWithInvalidValue_InvalidValueRecognized() { headers.TryAddWithoutValidation("Max-Forwards", "15,"); - Assert.Null(headers.GetParsedValues(KnownHeaders.MaxForwards.Descriptor)); + Assert.Null(headers.GetSingleParsedValue(KnownHeaders.MaxForwards.Descriptor)); Assert.Equal(1, headers.GetValues("Max-Forwards").Count()); Assert.Equal("15,", headers.GetValues("Max-Forwards").First()); headers.Clear(); headers.TryAddWithoutValidation("Max-Forwards", "1.0"); - Assert.Null(headers.GetParsedValues(KnownHeaders.MaxForwards.Descriptor)); + Assert.Null(headers.GetSingleParsedValue(KnownHeaders.MaxForwards.Descriptor)); Assert.Equal(1, headers.GetValues("Max-Forwards").Count()); Assert.Equal("1.0", headers.GetValues("Max-Forwards").First()); } @@ -1209,13 +1209,13 @@ public void TransferEncoding_UseAddMethod_AddedValueCanBeRetrievedUsingProperty( public void TransferEncoding_UseAddMethodWithInvalidValue_InvalidValueRecognized() { headers.TryAddWithoutValidation("Transfer-Encoding", "custom\u4F1A"); - Assert.Null(headers.GetParsedValues(KnownHeaders.TransferEncoding.Descriptor)); + Assert.Null(headers.GetSingleParsedValue(KnownHeaders.TransferEncoding.Descriptor)); Assert.Equal(1, headers.GetValues("Transfer-Encoding").Count()); Assert.Equal("custom\u4F1A", headers.GetValues("Transfer-Encoding").First()); headers.Clear(); headers.TryAddWithoutValidation("Transfer-Encoding", "custom1 custom2"); - Assert.Null(headers.GetParsedValues(KnownHeaders.TransferEncoding.Descriptor)); + Assert.Null(headers.GetSingleParsedValue(KnownHeaders.TransferEncoding.Descriptor)); Assert.Equal(1, headers.GetValues("Transfer-Encoding").Count()); Assert.Equal("custom1 custom2", headers.GetValues("Transfer-Encoding").First()); @@ -1263,13 +1263,13 @@ public void Upgrade_UseAddMethod_AddedValueCanBeRetrievedUsingProperty() public void Upgrade_UseAddMethodWithInvalidValue_InvalidValueRecognized() { headers.TryAddWithoutValidation("Upgrade", "custom\u4F1A"); - Assert.Null(headers.GetParsedValues(KnownHeaders.Upgrade.Descriptor)); + Assert.Null(headers.GetSingleParsedValue(KnownHeaders.Upgrade.Descriptor)); Assert.Equal(1, headers.GetValues("Upgrade").Count()); Assert.Equal("custom\u4F1A", headers.GetValues("Upgrade").First()); headers.Clear(); headers.TryAddWithoutValidation("Upgrade", "custom1 custom2"); - Assert.Null(headers.GetParsedValues(KnownHeaders.Upgrade.Descriptor)); + Assert.Null(headers.GetSingleParsedValue(KnownHeaders.Upgrade.Descriptor)); Assert.Equal(1, headers.GetValues("Upgrade").Count()); Assert.Equal("custom1 custom2", headers.GetValues("Upgrade").First()); } @@ -1308,13 +1308,13 @@ public void Date_UseAddMethod_AddedValueCanBeRetrievedUsingProperty() public void Date_UseAddMethodWithInvalidValue_InvalidValueRecognized() { headers.TryAddWithoutValidation("Date", " Sun, 06 Nov 1994 08:49:37 GMT ,"); - Assert.Null(headers.GetParsedValues(KnownHeaders.Date.Descriptor)); + Assert.Null(headers.GetSingleParsedValue(KnownHeaders.Date.Descriptor)); Assert.Equal(1, headers.GetValues("Date").Count()); Assert.Equal(" Sun, 06 Nov 1994 08:49:37 GMT ,", headers.GetValues("Date").First()); headers.Clear(); headers.TryAddWithoutValidation("Date", " Sun, 06 Nov "); - Assert.Null(headers.GetParsedValues(KnownHeaders.Date.Descriptor)); + Assert.Null(headers.GetSingleParsedValue(KnownHeaders.Date.Descriptor)); Assert.Equal(1, headers.GetValues("Date").Count()); Assert.Equal(" Sun, 06 Nov ", headers.GetValues("Date").First()); } @@ -1455,6 +1455,15 @@ public void CacheControl_UseAddMethod_AddedValueCanBeRetrievedUsingProperty() Assert.Equal(value, headers.CacheControl); } + [Fact] + public void CacheControl_ValidAndInvalidValues_ReturnValidValue() + { + headers.TryAddWithoutValidation("Cache-Control", "invalid"); + headers.TryAddWithoutValidation("Cache-Control", "no-cache=\"token1\", must-revalidate, max-age=3"); + + Assert.True(headers.CacheControl.NoCache); + } + [Fact] public void Trailer_ReadAndWriteProperty_ValueMatchesPriorSetValue() { @@ -1554,7 +1563,7 @@ public void Pragma_UseAddMethodWithInvalidValue_InvalidValueRecognized() #endregion [Fact] - public void NonValidatedHeader_Orders() + public void TryAddWithoutValidation_ValidAndInvalidValues_InsertionOrderIsPreserved() { HttpRequestMessage request = new HttpRequestMessage(HttpMethod.Get, "http://microsoft.com"); request.Headers.TryAddWithoutValidation("Accept", "text/bar"); diff --git a/src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpResponseHeadersTest.cs b/src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpResponseHeadersTest.cs index 696e3e656511f..b0222a76bc6c2 100644 --- a/src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpResponseHeadersTest.cs +++ b/src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpResponseHeadersTest.cs @@ -94,7 +94,7 @@ public void Location_UseAddMethod_AddedValueCanBeRetrievedUsingProperty() public void Location_UseAddMethodWithInvalidValue_InvalidValueRecognized() { headers.TryAddWithoutValidation("Location", " http://example.com http://other"); - Assert.Null(headers.GetParsedValues(KnownHeaders.Location.Descriptor)); + Assert.Null(headers.GetSingleParsedValue(KnownHeaders.Location.Descriptor)); Assert.Equal(1, headers.GetValues("Location").Count()); Assert.Equal(" http://example.com http://other", headers.GetValues("Location").First()); } @@ -348,26 +348,26 @@ public void Server_UseAddMethod_AddedValueCanBeRetrievedUsingProperty() public void Server_UseAddMethodWithInvalidValue_InvalidValueRecognized() { headers.TryAddWithoutValidation("Server", "custom\u4F1A"); - Assert.Null(headers.GetParsedValues(KnownHeaders.Server.Descriptor)); + Assert.Null(headers.GetSingleParsedValue(KnownHeaders.Server.Descriptor)); Assert.Equal(1, headers.GetValues("Server").Count()); Assert.Equal("custom\u4F1A", headers.GetValues("Server").First()); headers.Clear(); // Note that "Server" uses whitespace as separators, so the following is an invalid value headers.TryAddWithoutValidation("Server", "custom1, custom2"); - Assert.Null(headers.GetParsedValues(KnownHeaders.Server.Descriptor)); + Assert.Null(headers.GetSingleParsedValue(KnownHeaders.Server.Descriptor)); Assert.Equal(1, headers.GetValues("Server").Count()); Assert.Equal("custom1, custom2", headers.GetValues("Server").First()); headers.Clear(); headers.TryAddWithoutValidation("Server", "custom1, "); - Assert.Null(headers.GetParsedValues(KnownHeaders.Server.Descriptor)); + Assert.Null(headers.GetSingleParsedValue(KnownHeaders.Server.Descriptor)); Assert.Equal(1, headers.GetValues("Server").Count()); Assert.Equal("custom1, ", headers.GetValues("Server").First()); headers.Clear(); headers.TryAddWithoutValidation("Server", ",custom1"); - Assert.Null(headers.GetParsedValues(KnownHeaders.Server.Descriptor)); + Assert.Null(headers.GetSingleParsedValue(KnownHeaders.Server.Descriptor)); Assert.Equal(1, headers.GetValues("Server").Count()); Assert.Equal(",custom1", headers.GetValues("Server").First()); } @@ -491,13 +491,13 @@ public void Age_UseAddMethod_AddedValueCanBeRetrievedUsingProperty() public void Age_UseAddMethodWithInvalidValue_InvalidValueRecognized() { headers.TryAddWithoutValidation("Age", "10,"); - Assert.Null(headers.GetParsedValues(KnownHeaders.Age.Descriptor)); + Assert.Null(headers.GetSingleParsedValue(KnownHeaders.Age.Descriptor)); Assert.Equal(1, headers.GetValues("Age").Count()); Assert.Equal("10,", headers.GetValues("Age").First()); headers.Clear(); headers.TryAddWithoutValidation("Age", "1.1"); - Assert.Null(headers.GetParsedValues(KnownHeaders.Age.Descriptor)); + Assert.Null(headers.GetSingleParsedValue(KnownHeaders.Age.Descriptor)); Assert.Equal(1, headers.GetValues("Age").Count()); Assert.Equal("1.1", headers.GetValues("Age").First()); } From 92ac1a3a68099d8e451f12581f897842a7a75e8a Mon Sep 17 00:00:00 2001 From: feiyun0112 Date: Tue, 1 Mar 2022 14:02:08 +0800 Subject: [PATCH 08/27] make the changes @MihaZupan recommended --- .../Http/Headers/HttpHeaderValueCollection.cs | 23 +++++++++++++------ .../System/Net/Http/Headers/HttpHeaders.cs | 3 ++- .../UnitTests/Headers/HttpHeadersTest.cs | 18 +++++++++++++++ .../Headers/HttpRequestHeadersTest.cs | 18 --------------- 4 files changed, 36 insertions(+), 26 deletions(-) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaderValueCollection.cs b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaderValueCollection.cs index bbd7ea0d42897..2ced63217cff0 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaderValueCollection.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaderValueCollection.cs @@ -97,18 +97,27 @@ public void CopyTo(T[] array!!, int arrayIndex) if (storeValues == null) { - // We only have 1 value: If it is the "special value" just return, otherwise add the value to the - // array and return. - Debug.Assert(storeValue is T); - if (arrayIndex == array.Length) + if (storeValue is not HttpHeaders.InvalidValue) { - throw new ArgumentException(SR.net_http_copyto_array_too_small); + // We only have 1 value: If it is the "special value" just return, otherwise add the value to the + // array and return. + Debug.Assert(storeValue is T); + if (arrayIndex == array.Length) + { + throw new ArgumentException(SR.net_http_copyto_array_too_small); + } + array[arrayIndex] = (T)storeValue; } - array[arrayIndex] = (T)storeValue; } else { - storeValues.CopyTo(array, arrayIndex); + foreach (var item in storeValues) + { + if (item is not HttpHeaders.InvalidValue) + { + array[arrayIndex++] = (T)item; + } + } } } diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs index 4f01d53f3e9a6..241df730874b0 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs @@ -601,6 +601,7 @@ private HeaderStoreItemInfo CloneHeaderInfo(HeaderDescriptor descriptor, HeaderS if (descriptor.Parser == null) { + sourceInfo.AssertContainsNoInvalidValues(); destinationInfo.ParsedAndInvalidValues = CloneStringHeaderInfoValues(sourceInfo.ParsedAndInvalidValues); } else @@ -1366,7 +1367,7 @@ public void AssertContainsSingleParsedValue(List list) Debug.Assert(count <= 1, "only allow single parsed value"); } - internal bool IsEmpty => (RawValue == null) && ParsedAndInvalidValues == null; + internal bool IsEmpty => RawValue == null && ParsedAndInvalidValues == null; } diff --git a/src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpHeadersTest.cs b/src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpHeadersTest.cs index 7ccb14524ffa6..97252ff9c80d2 100644 --- a/src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpHeadersTest.cs +++ b/src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpHeadersTest.cs @@ -35,6 +35,24 @@ static HttpHeadersTest() private const string parsedPrefix = "parsed"; private const string invalidHeaderValue = "invalid"; + [Fact] + public void TryAddWithoutValidation_ValidAndInvalidValues_InsertionOrderIsPreserved() + { + HttpRequestMessage request = new HttpRequestMessage(HttpMethod.Get, "http://microsoft.com"); + request.Headers.TryAddWithoutValidation("Accept", "text/bar"); + request.Headers.TryAddWithoutValidation("Accept", "invalid"); + request.Headers.TryAddWithoutValidation("Accept", "text/baz"); + + // Force parsing + _ = request.Headers.Accept.Count; + + Assert.Equal(1, request.Headers.NonValidated.Count); + Assert.Equal(3, request.Headers.NonValidated.ElementAt(0).Value.Count); + Assert.Equal("text/bar", request.Headers.NonValidated.ElementAt(0).Value.ElementAt(0)); + Assert.Equal("invalid", request.Headers.NonValidated.ElementAt(0).Value.ElementAt(1)); + Assert.Equal("text/baz", request.Headers.NonValidated.ElementAt(0).Value.ElementAt(2)); + } + [Theory] [InlineData(null)] [InlineData("")] diff --git a/src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpRequestHeadersTest.cs b/src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpRequestHeadersTest.cs index f1938f21ea3c6..f25885daa8ada 100644 --- a/src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpRequestHeadersTest.cs +++ b/src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpRequestHeadersTest.cs @@ -1562,24 +1562,6 @@ public void Pragma_UseAddMethodWithInvalidValue_InvalidValueRecognized() #endregion - [Fact] - public void TryAddWithoutValidation_ValidAndInvalidValues_InsertionOrderIsPreserved() - { - HttpRequestMessage request = new HttpRequestMessage(HttpMethod.Get, "http://microsoft.com"); - request.Headers.TryAddWithoutValidation("Accept", "text/bar"); - request.Headers.TryAddWithoutValidation("Accept", "invalid"); - request.Headers.TryAddWithoutValidation("Accept", "text/baz"); - - // Force parsing - _ = request.Headers.Accept.Count; - - Assert.Equal(1, request.Headers.NonValidated.Count); - Assert.Equal(3, request.Headers.NonValidated.ElementAt(0).Value.Count); - Assert.Equal("text/bar", request.Headers.NonValidated.ElementAt(0).Value.ElementAt(0)); - Assert.Equal("invalid", request.Headers.NonValidated.ElementAt(0).Value.ElementAt(1)); - Assert.Equal("text/baz", request.Headers.NonValidated.ElementAt(0).Value.ElementAt(2)); - } - [Fact] public void ToString_SeveralRequestHeaders_Success() { From ad7aaf824bbdaf726d6b34f2247f5f0592cf3fdd Mon Sep 17 00:00:00 2001 From: feiyun0112 Date: Tue, 1 Mar 2022 16:02:12 +0800 Subject: [PATCH 09/27] check array length --- .../src/System/Net/Http/Headers/HttpHeaderValueCollection.cs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaderValueCollection.cs b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaderValueCollection.cs index 2ced63217cff0..4864e15ee1bf4 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaderValueCollection.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaderValueCollection.cs @@ -115,6 +115,11 @@ public void CopyTo(T[] array!!, int arrayIndex) { if (item is not HttpHeaders.InvalidValue) { + Debug.Assert(item is T); + if (arrayIndex == array.Length) + { + throw new ArgumentException(SR.net_http_copyto_array_too_small); + } array[arrayIndex++] = (T)item; } } From 6f5c6acc01a484fc0e492fafa4a2bde68eed281f Mon Sep 17 00:00:00 2001 From: feiyun0112 Date: Tue, 8 Mar 2022 12:38:13 +0800 Subject: [PATCH 10/27] fix unit test --- .../UnitTests/Headers/HttpHeaderValueCollectionTest.cs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpHeaderValueCollectionTest.cs b/src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpHeaderValueCollectionTest.cs index dec7eb661f7cf..ba9386bba50c6 100644 --- a/src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpHeaderValueCollectionTest.cs +++ b/src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpHeaderValueCollectionTest.cs @@ -262,7 +262,7 @@ public void CopyTo_CallWithStartIndexPlusElementCountGreaterArrayLength_Throw() Uri[] array = new Uri[2]; // startIndex + Count = 1 + 2 > array.Length - AssertExtensions.Throws("destinationArray", "", () => { collection.CopyTo(array, 1); }); + AssertExtensions.Throws(() => { collection.CopyTo(array, 1); }); } [Fact] @@ -346,18 +346,18 @@ public void CopyTo_ArrayTooSmall_Throw() headers.Add(knownStringHeader, "special"); headers.Add(knownStringHeader, "special"); array = new string[1]; - AssertExtensions.Throws("destinationArray", "", () => { collection.CopyTo(array, 0); }); + AssertExtensions.Throws(() => { collection.CopyTo(array, 0); }); headers.Add(knownStringHeader, "value1"); array = new string[0]; - AssertExtensions.Throws("destinationArray", "", () => { collection.CopyTo(array, 0); }); + AssertExtensions.Throws(() => { collection.CopyTo(array, 0); }); headers.Add(knownStringHeader, "value2"); array = new string[1]; - AssertExtensions.Throws("destinationArray", "", () => { collection.CopyTo(array, 0); }); + AssertExtensions.Throws(() => { collection.CopyTo(array, 0); }); array = new string[2]; - AssertExtensions.Throws("destinationArray", "", () => { collection.CopyTo(array, 1); }); + AssertExtensions.Throws(() => { collection.CopyTo(array, 1); }); } [Fact] From 263fe0e5c693df67a49837d87fab21eaf29e2312 Mon Sep 17 00:00:00 2001 From: feiyun0112 Date: Wed, 9 Mar 2022 09:15:10 +0800 Subject: [PATCH 11/27] Update src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpHeadersTest.cs Co-authored-by: Miha Zupan --- .../tests/UnitTests/Headers/HttpHeadersTest.cs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpHeadersTest.cs b/src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpHeadersTest.cs index 97252ff9c80d2..1e0bd527d44d2 100644 --- a/src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpHeadersTest.cs +++ b/src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpHeadersTest.cs @@ -47,10 +47,8 @@ public void TryAddWithoutValidation_ValidAndInvalidValues_InsertionOrderIsPreser _ = request.Headers.Accept.Count; Assert.Equal(1, request.Headers.NonValidated.Count); - Assert.Equal(3, request.Headers.NonValidated.ElementAt(0).Value.Count); - Assert.Equal("text/bar", request.Headers.NonValidated.ElementAt(0).Value.ElementAt(0)); - Assert.Equal("invalid", request.Headers.NonValidated.ElementAt(0).Value.ElementAt(1)); - Assert.Equal("text/baz", request.Headers.NonValidated.ElementAt(0).Value.ElementAt(2)); + HeaderStringValues accept = request.Headers.NonValidated["Accept"]; + Assert.Equal(new[] { "text/bar", "invalid", "text/baz" }, accept); } [Theory] From 7cf1a6b891f449ba39640ebe487c99a2a27f681e Mon Sep 17 00:00:00 2001 From: feiyun0112 Date: Wed, 9 Mar 2022 09:15:20 +0800 Subject: [PATCH 12/27] Update src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs Co-authored-by: Miha Zupan --- .../System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs index 241df730874b0..b7389f7c1e90c 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs @@ -513,7 +513,7 @@ internal bool ContainsParsedValue(HeaderDescriptor descriptor, object value) if (parsedValue is not InvalidValue) { Debug.Assert(parsedValue.GetType() == value.GetType(), - "Stored value does not have the same type as 'value'."); + "Stored value does not have the same type as 'value'."); return AreEqual(value, parsedValue, comparer); } From dfa81e73d4291f6028f882a98d7930e1726d01a8 Mon Sep 17 00:00:00 2001 From: feiyun0112 Date: Wed, 9 Mar 2022 09:15:33 +0800 Subject: [PATCH 13/27] Update src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs Co-authored-by: Miha Zupan --- .../System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs index b7389f7c1e90c..131378fab0bdd 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs @@ -1367,6 +1367,7 @@ public void AssertContainsSingleParsedValue(List list) Debug.Assert(count <= 1, "only allow single parsed value"); } + internal bool IsEmpty => RawValue == null && ParsedAndInvalidValues == null; } From 68a5b366f7dffdd8d6db771321f12a57aad69d0e Mon Sep 17 00:00:00 2001 From: feiyun0112 Date: Wed, 9 Mar 2022 09:15:45 +0800 Subject: [PATCH 14/27] Update src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs Co-authored-by: Miha Zupan --- .../System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs index 131378fab0bdd..c6d7bd6142122 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs @@ -1354,7 +1354,7 @@ public void AssertContainsNoInvalidValues() } [Conditional("DEBUG")] - public void AssertContainsSingleParsedValue(List list) + private static void AssertContainsSingleParsedValue(List list) { int count = 0; foreach (object item in list) From 017a558612540dd9188b486317aaa28e8ae150d8 Mon Sep 17 00:00:00 2001 From: feiyun0112 Date: Wed, 9 Mar 2022 09:16:41 +0800 Subject: [PATCH 15/27] make the changes @MihaZupan recommended --- .../Http/Headers/CacheControlHeaderParser.cs | 24 ++++++++++++++++++- .../Http/Headers/HttpHeaderValueCollection.cs | 2 +- 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/CacheControlHeaderParser.cs b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/CacheControlHeaderParser.cs index b8b396f9ea613..09ca1bfa5f635 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/CacheControlHeaderParser.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/CacheControlHeaderParser.cs @@ -1,6 +1,7 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System.Collections.Generic; using System.Diagnostics; namespace System.Net.Http.Headers @@ -22,7 +23,28 @@ private CacheControlHeaderParser() protected override int GetParsedValueLength(string value, int startIndex, object? storeValue, out object? parsedValue) { - CacheControlHeaderValue? temp = storeValue as CacheControlHeaderValue; + CacheControlHeaderValue? temp = null; + if (storeValue is not null) + { + if (storeValue is List list) + { + foreach (object item in list) + { + if (item is not HttpHeaders.InvalidValue) + { + temp = item as CacheControlHeaderValue; + break; + } + } + } + else + { + if (storeValue is not HttpHeaders.InvalidValue) + { + temp = storeValue as CacheControlHeaderValue; + } + } + } Debug.Assert(storeValue == null || temp != null, "'storeValue' is not of type CacheControlHeaderValue"); int resultLength = CacheControlHeaderValue.GetCacheControlLength(value, startIndex, temp, out temp); diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaderValueCollection.cs b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaderValueCollection.cs index 4864e15ee1bf4..b1468079cc6f7 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaderValueCollection.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaderValueCollection.cs @@ -111,7 +111,7 @@ public void CopyTo(T[] array!!, int arrayIndex) } else { - foreach (var item in storeValues) + foreach (object item in storeValues) { if (item is not HttpHeaders.InvalidValue) { From c69be947012220c03fe34f7a1814ab4fb56e8e5b Mon Sep 17 00:00:00 2001 From: feiyun0112 Date: Wed, 9 Mar 2022 16:46:22 +0800 Subject: [PATCH 16/27] Update src/libraries/System.Net.Http/src/System/Net/Http/Headers/CacheControlHeaderParser.cs Co-authored-by: Miha Zupan --- .../Http/Headers/CacheControlHeaderParser.cs | 24 +++++++------------ 1 file changed, 9 insertions(+), 15 deletions(-) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/CacheControlHeaderParser.cs b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/CacheControlHeaderParser.cs index 09ca1bfa5f635..17f718d96a7c0 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/CacheControlHeaderParser.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/CacheControlHeaderParser.cs @@ -24,27 +24,21 @@ protected override int GetParsedValueLength(string value, int startIndex, object out object? parsedValue) { CacheControlHeaderValue? temp = null; - if (storeValue is not null) + if (storeValue is List list) { - if (storeValue is List list) + foreach (object item in list) { - foreach (object item in list) + if (item is CacheControlHeaderValue cacheControl) { - if (item is not HttpHeaders.InvalidValue) - { - temp = item as CacheControlHeaderValue; - break; - } - } - } - else - { - if (storeValue is not HttpHeaders.InvalidValue) - { - temp = storeValue as CacheControlHeaderValue; + temp = cacheControl; + break; } } } + else + { + temp = storeValue as CacheControlHeaderValue; + } Debug.Assert(storeValue == null || temp != null, "'storeValue' is not of type CacheControlHeaderValue"); int resultLength = CacheControlHeaderValue.GetCacheControlLength(value, startIndex, temp, out temp); From 39c1a983ee598300cc5c9a1e1e0e7a544c71d089 Mon Sep 17 00:00:00 2001 From: feiyun0112 Date: Fri, 11 Mar 2022 21:18:16 +0800 Subject: [PATCH 17/27] chang unit test --- .../src/System/Net/Http/Headers/CacheControlHeaderParser.cs | 5 ++++- .../tests/UnitTests/Headers/HttpRequestHeadersTest.cs | 4 +++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/CacheControlHeaderParser.cs b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/CacheControlHeaderParser.cs index 09ca1bfa5f635..6ee87ae7a1f65 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/CacheControlHeaderParser.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/CacheControlHeaderParser.cs @@ -24,6 +24,7 @@ protected override int GetParsedValueLength(string value, int startIndex, object out object? parsedValue) { CacheControlHeaderValue? temp = null; + bool isInvalidValue = true; if (storeValue is not null) { if (storeValue is List list) @@ -32,6 +33,7 @@ protected override int GetParsedValueLength(string value, int startIndex, object { if (item is not HttpHeaders.InvalidValue) { + isInvalidValue = false; temp = item as CacheControlHeaderValue; break; } @@ -41,11 +43,12 @@ protected override int GetParsedValueLength(string value, int startIndex, object { if (storeValue is not HttpHeaders.InvalidValue) { + isInvalidValue = false; temp = storeValue as CacheControlHeaderValue; } } } - Debug.Assert(storeValue == null || temp != null, "'storeValue' is not of type CacheControlHeaderValue"); + Debug.Assert(isInvalidValue || storeValue == null || temp != null, "'storeValue' is not of type CacheControlHeaderValue"); int resultLength = CacheControlHeaderValue.GetCacheControlLength(value, startIndex, temp, out temp); diff --git a/src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpRequestHeadersTest.cs b/src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpRequestHeadersTest.cs index f25885daa8ada..182a855e0d033 100644 --- a/src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpRequestHeadersTest.cs +++ b/src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpRequestHeadersTest.cs @@ -1460,8 +1460,10 @@ public void CacheControl_ValidAndInvalidValues_ReturnValidValue() { headers.TryAddWithoutValidation("Cache-Control", "invalid"); headers.TryAddWithoutValidation("Cache-Control", "no-cache=\"token1\", must-revalidate, max-age=3"); - + headers.TryAddWithoutValidation("Cache-Control", "public, s-maxage=15"); Assert.True(headers.CacheControl.NoCache); + Assert.True(headers.NonValidated["Cache-Control"].Count == 1); + Assert.Equal("public, must-revalidate, no-cache=\"token1\", max-age=3, s-maxage=15, invalid", headers.NonValidated["Cache-Control"].ElementAt(0)); } [Fact] From a9695604fb0c31e1b889da545e3cb007e687aa35 Mon Sep 17 00:00:00 2001 From: feiyun0112 Date: Fri, 11 Mar 2022 21:20:46 +0800 Subject: [PATCH 18/27] GetParsedValueLength --- .../Http/Headers/CacheControlHeaderParser.cs | 28 +++++++++---------- 1 file changed, 13 insertions(+), 15 deletions(-) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/CacheControlHeaderParser.cs b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/CacheControlHeaderParser.cs index 331f225b39f1d..bdea8ff7abfde 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/CacheControlHeaderParser.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/CacheControlHeaderParser.cs @@ -25,27 +25,25 @@ protected override int GetParsedValueLength(string value, int startIndex, object { CacheControlHeaderValue? temp = null; bool isInvalidValue = true; - if (storeValue is not null) + + foreach (object item in list) { - foreach (object item in list) + if (item is CacheControlHeaderValue cacheControl) { - if (item is CacheControlHeaderValue cacheControl) + if (item is not HttpHeaders.InvalidValue) { - if (item is not HttpHeaders.InvalidValue) - { - isInvalidValue = false; - temp = item as CacheControlHeaderValue; - break; - } + isInvalidValue = false; + temp = item as CacheControlHeaderValue; + break; } } - else + } + else + { + if (storeValue is not HttpHeaders.InvalidValue) { - if (storeValue is not HttpHeaders.InvalidValue) - { - isInvalidValue = false; - temp = storeValue as CacheControlHeaderValue; - } + isInvalidValue = false; + temp = storeValue as CacheControlHeaderValue; } } Debug.Assert(isInvalidValue || storeValue == null || temp != null, "'storeValue' is not of type CacheControlHeaderValue"); From 3684810babb4a0979b6000d742009110f7adf0ec Mon Sep 17 00:00:00 2001 From: feiyun0112 Date: Sat, 12 Mar 2022 10:28:20 +0800 Subject: [PATCH 19/27] fix build error --- .../Net/Http/Headers/CacheControlHeaderParser.cs | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/CacheControlHeaderParser.cs b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/CacheControlHeaderParser.cs index bdea8ff7abfde..852eac2a03a53 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/CacheControlHeaderParser.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/CacheControlHeaderParser.cs @@ -25,16 +25,18 @@ protected override int GetParsedValueLength(string value, int startIndex, object { CacheControlHeaderValue? temp = null; bool isInvalidValue = true; - - foreach (object item in list) + if (storeValue is List list) { - if (item is CacheControlHeaderValue cacheControl) + foreach (object item in list) { - if (item is not HttpHeaders.InvalidValue) + if (item is CacheControlHeaderValue cacheControl) { - isInvalidValue = false; - temp = item as CacheControlHeaderValue; - break; + if (item is not HttpHeaders.InvalidValue) + { + isInvalidValue = false; + temp = item as CacheControlHeaderValue; + break; + } } } } From 876d1302ff4a59703de9492876935442cc2f9111 Mon Sep 17 00:00:00 2001 From: feiyun0112 Date: Wed, 16 Mar 2022 09:57:08 +0800 Subject: [PATCH 20/27] Update src/libraries/System.Net.Http/src/System/Net/Http/Headers/CacheControlHeaderParser.cs Co-authored-by: Miha Zupan --- .../Net/Http/Headers/CacheControlHeaderParser.cs | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/CacheControlHeaderParser.cs b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/CacheControlHeaderParser.cs index 852eac2a03a53..69609c8697cde 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/CacheControlHeaderParser.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/CacheControlHeaderParser.cs @@ -29,14 +29,11 @@ protected override int GetParsedValueLength(string value, int startIndex, object { foreach (object item in list) { - if (item is CacheControlHeaderValue cacheControl) + if (item is not HttpHeaders.InvalidValue) { - if (item is not HttpHeaders.InvalidValue) - { - isInvalidValue = false; - temp = item as CacheControlHeaderValue; - break; - } + isInvalidValue = false; + temp = item as CacheControlHeaderValue; + break; } } } From 8904a36b32e82e64078b488da7471e713ca428ab Mon Sep 17 00:00:00 2001 From: feiyun0112 Date: Wed, 16 Mar 2022 09:57:31 +0800 Subject: [PATCH 21/27] Update src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs Co-authored-by: Miha Zupan --- .../System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs index c6d7bd6142122..b97f63cc922fb 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs @@ -1365,7 +1365,7 @@ private static void AssertContainsSingleParsedValue(List list) } } - Debug.Assert(count <= 1, "only allow single parsed value"); + Debug.Assert(count == 1, "Only a single parsed value should be stored for this parser"); } internal bool IsEmpty => RawValue == null && ParsedAndInvalidValues == null; From c3cd0af475d85181e7fb3e0135688b75a96fa3e1 Mon Sep 17 00:00:00 2001 From: feiyun0112 Date: Wed, 16 Mar 2022 13:28:09 +0800 Subject: [PATCH 22/27] unit test --- .../tests/UnitTests/Headers/HttpRequestHeadersTest.cs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpRequestHeadersTest.cs b/src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpRequestHeadersTest.cs index 182a855e0d033..b174d077e4306 100644 --- a/src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpRequestHeadersTest.cs +++ b/src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpRequestHeadersTest.cs @@ -1458,12 +1458,13 @@ public void CacheControl_UseAddMethod_AddedValueCanBeRetrievedUsingProperty() [Fact] public void CacheControl_ValidAndInvalidValues_ReturnValidValue() { - headers.TryAddWithoutValidation("Cache-Control", "invalid"); + headers.TryAddWithoutValidation("Cache-Control", ""); headers.TryAddWithoutValidation("Cache-Control", "no-cache=\"token1\", must-revalidate, max-age=3"); headers.TryAddWithoutValidation("Cache-Control", "public, s-maxage=15"); Assert.True(headers.CacheControl.NoCache); - Assert.True(headers.NonValidated["Cache-Control"].Count == 1); - Assert.Equal("public, must-revalidate, no-cache=\"token1\", max-age=3, s-maxage=15, invalid", headers.NonValidated["Cache-Control"].ElementAt(0)); + Assert.True(headers.NonValidated["Cache-Control"].Count == 2); + Assert.Equal("", headers.NonValidated["Cache-Control"].ElementAt(0)); + Assert.Equal("public, must-revalidate, no-cache=\"token1\", max-age=3, s-maxage=15", headers.NonValidated["Cache-Control"].ElementAt(1)); } [Fact] From ef11a8a8ae6d75931dce682cb8a7fcab280ee6b9 Mon Sep 17 00:00:00 2001 From: feiyun0112 Date: Thu, 17 Mar 2022 13:38:05 +0800 Subject: [PATCH 23/27] make changes @geoffkizer recommended --- .../Net/Http/Headers/HttpHeaderValueCollection.cs | 2 -- .../src/System/Net/Http/Headers/HttpHeaders.cs | 11 ++++++++--- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaderValueCollection.cs b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaderValueCollection.cs index b1468079cc6f7..4d673c25261c3 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaderValueCollection.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaderValueCollection.cs @@ -99,8 +99,6 @@ public void CopyTo(T[] array!!, int arrayIndex) { if (storeValue is not HttpHeaders.InvalidValue) { - // We only have 1 value: If it is the "special value" just return, otherwise add the value to the - // array and return. Debug.Assert(storeValue is T); if (arrayIndex == array.Length) { diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs index b97f63cc922fb..ee78a75e788ae 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs @@ -1253,9 +1253,14 @@ private bool AreEqual(object value, object? storeValue, IEqualityComparer? compa return value.Equals(storeValue); } - internal record InvalidValue(string value) + internal class InvalidValue { - public override string ToString() => value; + public InvalidValue(string value) + { + Value = value; + } + public string Value { get; private set; } + public override string ToString() => Value; } internal sealed class HeaderStoreItemInfo @@ -1278,7 +1283,7 @@ internal bool CanAddParsedValue(HttpHeaderParser parser) // supporting 1 value. When the first value gets parsed, CanAddValue returns true and we add the // parsed value to ParsedValue. When the second value is parsed, CanAddValue returns false, because // we have already a parsed value. - return parser.SupportsMultipleValues || !ContainsParsedValue(); + return parser.SupportsMultipleValues || ParsedAndInvalidValues is null; } private bool ContainsParsedValue() From 8f69ba8c10f812a02db0b0f9b25785a42dab07a2 Mon Sep 17 00:00:00 2001 From: feiyun0112 Date: Mon, 21 Mar 2022 16:48:32 +0800 Subject: [PATCH 24/27] CloneAndAddValue for InvalidValues --- .../src/System/Net/Http/Headers/HttpHeaders.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs index ee78a75e788ae..d6e318626955e 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs @@ -606,7 +606,7 @@ private HeaderStoreItemInfo CloneHeaderInfo(HeaderDescriptor descriptor, HeaderS } else { - // Now clone and add parsed values (if any). + // We have a parser, so we also have to clone invalid values and parsed values. if (sourceInfo.ParsedAndInvalidValues != null) { List? sourceValues = sourceInfo.ParsedAndInvalidValues as List; @@ -630,7 +630,7 @@ private HeaderStoreItemInfo CloneHeaderInfo(HeaderDescriptor descriptor, HeaderS private static void CloneAndAddValue(HeaderStoreItemInfo destinationInfo, object source) { // We only have one value. Clone it and assign it to the store. - if (source is ICloneable cloneableValue) + if (source is not InvalidValue && source is ICloneable cloneableValue) { AddParsedValue(destinationInfo, cloneableValue.Clone()); } From 682d1466d2c658b0e55a586509584a3ce3ea01a0 Mon Sep 17 00:00:00 2001 From: feiyun0112 Date: Tue, 22 Mar 2022 14:32:47 +0800 Subject: [PATCH 25/27] Update src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs Co-authored-by: Miha Zupan --- .../System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs index d6e318626955e..c07bf18b2fc18 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs @@ -630,8 +630,9 @@ private HeaderStoreItemInfo CloneHeaderInfo(HeaderDescriptor descriptor, HeaderS private static void CloneAndAddValue(HeaderStoreItemInfo destinationInfo, object source) { // We only have one value. Clone it and assign it to the store. - if (source is not InvalidValue && source is ICloneable cloneableValue) + if (source is ICloneable cloneableValue) { + Debug.Assert(source is not InvalidValue); AddParsedValue(destinationInfo, cloneableValue.Clone()); } else From e50c36de0e9df58d0c670f52cf8585c1c7ebf83d Mon Sep 17 00:00:00 2001 From: feiyun0112 Date: Tue, 22 Mar 2022 14:33:04 +0800 Subject: [PATCH 26/27] Update src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs Co-authored-by: Miha Zupan --- .../System/Net/Http/Headers/HttpHeaders.cs | 26 ------------------- 1 file changed, 26 deletions(-) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs index c07bf18b2fc18..dc89369520704 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs @@ -1287,32 +1287,6 @@ internal bool CanAddParsedValue(HttpHeaderParser parser) return parser.SupportsMultipleValues || ParsedAndInvalidValues is null; } - private bool ContainsParsedValue() - { - if (ParsedAndInvalidValues is not null) - { - if (ParsedAndInvalidValues is List list) - { - foreach (object item in list) - { - if (item is not InvalidValue) - { - return true; - } - } - } - else - { - if (ParsedAndInvalidValues is not InvalidValue) - { - return true; - } - } - } - - return false; - } - [Conditional("DEBUG")] public void AssertContainsNoInvalidValues() { From 5badaae7b0b6577c27dc599dd44a85be667b224e Mon Sep 17 00:00:00 2001 From: Miha Zupan Date: Sun, 27 Mar 2022 15:23:42 +0200 Subject: [PATCH 27/27] Final nits --- .../src/System/Net/Http/Headers/HttpHeaders.cs | 17 ++++++++++------- .../tests/UnitTests/Headers/HttpHeadersTest.cs | 9 --------- 2 files changed, 10 insertions(+), 16 deletions(-) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs index dc89369520704..0d35fe05f5d76 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HttpHeaders.cs @@ -1254,14 +1254,17 @@ private bool AreEqual(object value, object? storeValue, IEqualityComparer? compa return value.Equals(storeValue); } - internal class InvalidValue + internal sealed class InvalidValue { + private readonly string _value; + public InvalidValue(string value) { - Value = value; + Debug.Assert(value is not null); + _value = value; } - public string Value { get; private set; } - public override string ToString() => Value; + + public override string ToString() => _value; } internal sealed class HeaderStoreItemInfo @@ -1271,7 +1274,7 @@ internal HeaderStoreItemInfo() { } internal object? RawValue; internal object? ParsedAndInvalidValues; - internal bool CanAddParsedValue(HttpHeaderParser parser) + public bool CanAddParsedValue(HttpHeaderParser parser) { Debug.Assert(parser != null, "There should be no reason to call CanAddValue if there is no parser for the current header."); @@ -1306,7 +1309,7 @@ public void AssertContainsNoInvalidValues() } } - internal object? GetSingleParsedValue() + public object? GetSingleParsedValue() { if (ParsedAndInvalidValues is not null) { @@ -1348,7 +1351,7 @@ private static void AssertContainsSingleParsedValue(List list) Debug.Assert(count == 1, "Only a single parsed value should be stored for this parser"); } - internal bool IsEmpty => RawValue == null && ParsedAndInvalidValues == null; + public bool IsEmpty => RawValue == null && ParsedAndInvalidValues == null; } diff --git a/src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpHeadersTest.cs b/src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpHeadersTest.cs index 1e0bd527d44d2..b86a2d72202e4 100644 --- a/src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpHeadersTest.cs +++ b/src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpHeadersTest.cs @@ -206,15 +206,6 @@ public void TryAddWithoutValidation_AddValidAndInvalidValueString_BothValuesPars Assert.Equal(1, headers.Count()); Assert.Equal(2, headers.First().Value.Count()); - // If you compare this test with the previous one: Note that we reversed the order of adding the invalid - // string and the valid string. However, when enumerating header values the order is still the same as in - // the previous test. - // We don't keep track of the order if we have both invalid & valid values. This would add complexity - // and additional memory to store the information. Given how rare this scenario is we consider this - // by design. Note that this scenario is only an issue if: - // - The header value has an invalid format (very rare for standard headers) AND - // - There are multiple header values (some valid, some invalid) AND - // - The order of the headers matters (e.g. Transfer-Encoding) Assert.Equal(invalidHeaderValue, headers.First().Value.ElementAt(0)); Assert.Equal(parsedPrefix, headers.First().Value.ElementAt(1)); Assert.Equal(2, headers.Parser.TryParseValueCallCount);