Address additional feedback from number-handling PRs (#42505)
authorLayomi Akinrinade <laakinri@microsoft.com>
Thu, 24 Sep 2020 16:30:29 +0000 (09:30 -0700)
committerGitHub <noreply@github.com>
Thu, 24 Sep 2020 16:30:29 +0000 (09:30 -0700)
* Address further feedback from number-handling PRs

* Address review feedback

src/libraries/System.Text.Json/src/System.Text.Json.csproj
src/libraries/System.Text.Json/src/System/Text/Json/JsonHelpers.cs
src/libraries/System.Text.Json/src/System/Text/Json/Reader/Utf8JsonReader.TryGet.cs
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Attributes/JsonNumberHandlingAttribute.cs
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/DictionaryDefaultConverter.cs
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonNumberHandling.cs
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.cs [deleted file]
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs
src/libraries/System.Text.Json/tests/Serialization/NumberHandlingTests.cs

index 69304ed..cc000f0 100644 (file)
     <Compile Include="System\Text\Json\Serialization\JsonPropertyInfoOfT.cs" />
     <Compile Include="System\Text\Json\Serialization\GenericMethodHolder.cs" />
     <Compile Include="System\Text\Json\Serialization\JsonResumableConverterOfT.cs" />
-    <Compile Include="System\Text\Json\Serialization\JsonSerializer.cs" />
     <Compile Include="System\Text\Json\Serialization\JsonSerializer.Read.HandleMetadata.cs" />
     <Compile Include="System\Text\Json\Serialization\JsonSerializer.Read.HandlePropertyName.cs" />
     <Compile Include="System\Text\Json\Serialization\JsonSerializer.Read.Helpers.cs" />
index fe3102e..813627a 100644 (file)
@@ -5,6 +5,7 @@ using System.Buffers;
 using System.Collections.Generic;
 using System.Diagnostics;
 using System.Runtime.CompilerServices;
+using System.Text.Json.Serialization;
 
 namespace System.Text.Json
 {
@@ -134,5 +135,13 @@ namespace System.Text.Json
             return !(float.IsNaN(value) || float.IsInfinity(value));
 #endif
         }
+
+        public static bool IsValidNumberHandlingValue(JsonNumberHandling handling) =>
+            IsInRangeInclusive((int)handling, 0,
+                (int)(
+                JsonNumberHandling.Strict |
+                JsonNumberHandling.AllowReadingFromString |
+                JsonNumberHandling.WriteAsString |
+                JsonNumberHandling.AllowNamedFloatingPointLiterals));
     }
 }
index faa0dc2..32947d9 100644 (file)
@@ -428,7 +428,7 @@ namespace System.Text.Json
                 // NETCOREAPP implementation of the TryParse method above permits case-insenstive variants of the
                 // float constants "NaN", "Infinity", "-Infinity". This differs from the NETFRAMEWORK implementation.
                 // The following logic reconciles the two implementations to enforce consistent behavior.
-                if (!float.IsNaN(value) && !float.IsPositiveInfinity(value) && !float.IsNegativeInfinity(value))
+                if (JsonHelpers.IsFinite(value))
                 {
                     return value;
                 }
@@ -487,7 +487,7 @@ namespace System.Text.Json
                 // NETCOREAPP implementation of the TryParse method above permits case-insenstive variants of the
                 // float constants "NaN", "Infinity", "-Infinity". This differs from the NETFRAMEWORK implementation.
                 // The following logic reconciles the two implementations to enforce consistent behavior.
-                if (!double.IsNaN(value) && !double.IsPositiveInfinity(value) && !double.IsNegativeInfinity(value))
+                if (JsonHelpers.IsFinite(value))
                 {
                     return value;
                 }
index ac7661b..e6c2448 100644 (file)
@@ -20,7 +20,7 @@ namespace System.Text.Json.Serialization
         /// </summary>
         public JsonNumberHandlingAttribute(JsonNumberHandling handling)
         {
-            if (!JsonSerializer.IsValidNumberHandlingValue(handling))
+            if (!JsonHelpers.IsValidNumberHandlingValue(handling))
             {
                 throw new ArgumentOutOfRangeException(nameof(handling));
             }
index d077ac1..ce41388 100644 (file)
@@ -29,14 +29,9 @@ namespace System.Text.Json.Serialization.Converters
         /// </summary>
         protected virtual void CreateCollection(ref Utf8JsonReader reader, ref ReadStack state) { }
 
-        private static Type s_valueType = typeof(TValue);
+        internal override Type ElementType => typeof(TValue);
 
-        internal override Type ElementType => s_valueType;
-
-        protected Type KeyType = typeof(TKey);
-        // For string keys we don't use a key converter
-        // in order to avoid performance regression on already supported types.
-        protected bool IsStringKey = typeof(TKey) == typeof(string);
+        protected static readonly Type KeyType = typeof(TKey);
 
         protected JsonConverter<TKey>? _keyConverter;
         protected JsonConverter<TValue>? _valueConverter;
@@ -93,7 +88,7 @@ namespace System.Text.Json.Serialization.Converters
 
                         // Read the value and add.
                         reader.ReadWithVerify();
-                        TValue element = valueConverter.Read(ref reader, s_valueType, options);
+                        TValue element = valueConverter.Read(ref reader, ElementType, options);
                         Add(key, element!, options, ref state);
                     }
                 }
@@ -118,7 +113,7 @@ namespace System.Text.Json.Serialization.Converters
                         reader.ReadWithVerify();
 
                         // Get the value from the converter and add it.
-                        valueConverter.TryRead(ref reader, s_valueType, options, ref state, out TValue element);
+                        valueConverter.TryRead(ref reader, ElementType, options, ref state, out TValue element);
                         Add(key, element!, options, ref state);
                     }
                 }
@@ -243,7 +238,7 @@ namespace System.Text.Json.Serialization.Converters
                 string unescapedPropertyNameAsString;
 
                 // Special case string to avoid calling GetString twice and save one allocation.
-                if (IsStringKey)
+                if (KeyType == typeof(string))
                 {
                     unescapedPropertyNameAsString = reader.GetString()!;
                     key = (TKey)(object)unescapedPropertyNameAsString;
index 0e79173..c8eabaa 100644 (file)
@@ -16,6 +16,8 @@ namespace System.Text.Json.Serialization
         /// <summary>
         /// Numbers can be read from <see cref="JsonTokenType.String"/> tokens.
         /// Does not prevent numbers from being read from <see cref="JsonTokenType.Number"/> token.
+        /// Strings that have escaped characters will be unescaped before reading.
+        /// Leading or trailing trivia within the string token, including whitespace, is not allowed.
         /// </summary>
         AllowReadingFromString = 0x1,
         /// <summary>
@@ -25,7 +27,10 @@ namespace System.Text.Json.Serialization
         /// <summary>
         /// The "NaN", "Infinity", and "-Infinity" <see cref="JsonTokenType.String"/> tokens can be read as
         /// floating-point constants, and the <see cref="float"/> and <see cref="double"/> values for these
-        /// constants will be written as their corresponding JSON string representations.
+        /// constants (such as <see cref="float.PositiveInfinity"/> and <see cref="double.NaN"/>)
+        /// will be written as their corresponding JSON string representations.
+        /// Strings that have escaped characters will be unescaped before reading.
+        /// Leading or trailing trivia within the string token, including whitespace, is not allowed.
         /// </summary>
         AllowNamedFloatingPointLiterals = 0x4
     }
diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.cs
deleted file mode 100644 (file)
index 8ddd013..0000000
+++ /dev/null
@@ -1,15 +0,0 @@
-// Licensed to the .NET Foundation under one or more agreements.
-// The .NET Foundation licenses this file to you under the MIT license.
-
-using System.Text.Json.Serialization;
-
-namespace System.Text.Json
-{
-    public static partial class JsonSerializer
-    {
-        internal static bool IsValidNumberHandlingValue(JsonNumberHandling handling)
-        {
-            return JsonHelpers.IsInRangeInclusive((int)handling, 0, 7);
-        }
-    }
-}
index 873046f..8ade082 100644 (file)
@@ -278,7 +278,7 @@ namespace System.Text.Json
             {
                 VerifyMutable();
 
-                if (!JsonSerializer.IsValidNumberHandlingValue(value))
+                if (!JsonHelpers.IsValidNumberHandlingValue(value))
                 {
                     throw new ArgumentOutOfRangeException(nameof(value));
                 }
index f306e3a..e132fb0 100644 (file)
@@ -588,17 +588,7 @@ namespace System.Text.Json.Serialization.Tests
                 }
                 else if (reader.TokenType == JsonTokenType.String)
                 {
-#if BUILDING_INBOX_LIBRARY
-                    Assert.False(reader.ValueSpan.Contains((byte)'\\'));
-#else
-                    foreach (byte val in reader.ValueSpan)
-                    {
-                        if (val == (byte)'\\')
-                        {
-                            Assert.True(false, "Unexpected escape token.");
-                        }
-                    }
-#endif
+                    Assert.True(reader.ValueSpan.IndexOf((byte)'\\') == -1);
                 }
                 else
                 {
@@ -755,6 +745,10 @@ namespace System.Text.Json.Serialization.Tests
             PerformFloatingPointSerialization("Infinity");
             PerformFloatingPointSerialization("-Infinity");
 
+            PerformFloatingPointSerialization("\u004EaN"); // NaN
+            PerformFloatingPointSerialization("Inf\u0069ni\u0074y"); // Infinity
+            PerformFloatingPointSerialization("\u002DInf\u0069nity"); // -Infinity
+
             static void PerformFloatingPointSerialization(string testString)
             {
                 string testStringAsJson = $@"""{testString}""";
@@ -814,19 +808,27 @@ namespace System.Text.Json.Serialization.Tests
         [InlineData("-infinitY")]
         [InlineData("-INFINITY")]
         [InlineData(" NaN")]
+        [InlineData("NaN ")]
         [InlineData(" Infinity")]
         [InlineData(" -Infinity")]
-        [InlineData("NaN ")]
         [InlineData("Infinity ")]
         [InlineData("-Infinity ")]
         [InlineData("a-Infinity")]
         [InlineData("NaNa")]
         [InlineData("Infinitya")]
         [InlineData("-Infinitya")]
+        [InlineData("\u006EaN")] // "naN"
+        [InlineData("\u0020Inf\u0069ni\u0074y")] // " Infinity"
+        [InlineData("\u002BInf\u0069nity")] // "+Infinity"
         public static void FloatingPointConstants_Fail(string testString)
         {
             string testStringAsJson = $@"""{testString}""";
-            string testJson = @$"{{""FloatNumber"":{testStringAsJson},""DoubleNumber"":{testStringAsJson}}}";
+
+            string testJson = @$"{{""FloatNumber"":{testStringAsJson}}}";
+            Assert.Throws<JsonException>(() => JsonSerializer.Deserialize<StructWithNumbers>(testJson, s_optionsAllowFloatConstants));
+            Assert.Throws<JsonException>(() => JsonSerializer.Deserialize<StructWithNumbers>(testJson, s_optionReadFromStr));
+
+            testJson = @$"{{""DoubleNumber"":{testStringAsJson}}}";
             Assert.Throws<JsonException>(() => JsonSerializer.Deserialize<StructWithNumbers>(testJson, s_optionsAllowFloatConstants));
             Assert.Throws<JsonException>(() => JsonSerializer.Deserialize<StructWithNumbers>(testJson, s_optionReadFromStr));
         }
@@ -1031,21 +1033,7 @@ namespace System.Text.Json.Serialization.Tests
                 else
                 {
                     Assert.Equal(JsonTokenType.String, reader.TokenType);
-#if BUILDING_INBOX_LIBRARY
-                    Assert.True(reader.ValueSpan.Contains((byte)'\\'));
-#else
-                    bool foundBackSlash = false;
-                    foreach (byte val in reader.ValueSpan)
-                    {
-                        if (val == (byte)'\\')
-                        {
-                            foundBackSlash = true;
-                            break;
-                        }
-                    }
-
-                    Assert.True(foundBackSlash, "Expected escape token.");
-#endif
+                    Assert.True(reader.ValueSpan.IndexOf((byte)'\\') != -1);
                 }
             }
         }
@@ -1063,17 +1051,7 @@ namespace System.Text.Json.Serialization.Tests
                 else
                 {
                     Assert.Equal(JsonTokenType.String, reader.TokenType);
-#if BUILDING_INBOX_LIBRARY
-                    Assert.False(reader.ValueSpan.Contains((byte)'\\'));
-#else
-                    foreach (byte val in reader.ValueSpan)
-                    {
-                        if (val == (byte)'\\')
-                        {
-                            Assert.True(false, "Unexpected escape token.");
-                        }
-                    }
-#endif
+                    Assert.True(reader.ValueSpan.IndexOf((byte)'\\') == -1);
                 }
             }
         }
@@ -1436,12 +1414,12 @@ namespace System.Text.Json.Serialization.Tests
         [Fact]
         public static void Attribute_OnType_NotRecursive()
         {
-            // Recursive behavior would allow a string number.
-            // This is not supported.
+            // Recursive behavior, where number handling setting on a property is applied to subsequent
+            // properties in its type closure, would allow a string number. This is not supported.
             string json = @"{""NestedClass"":{""MyInt"":""1""}}";
-            Assert.Throws<JsonException>(() => JsonSerializer.Deserialize<AttributeOnFirstLevel>(json));
+            Assert.Throws<JsonException>(() => JsonSerializer.Deserialize<AttributeAppliedToFirstLevelProp>(json));
 
-            var obj = new AttributeOnFirstLevel
+            var obj = new AttributeAppliedToFirstLevelProp
             {
                 NestedClass = new BadProperty { MyInt = 1 }
             };
@@ -1449,7 +1427,7 @@ namespace System.Text.Json.Serialization.Tests
         }
 
         [JsonNumberHandling(JsonNumberHandling.AllowReadingFromString | JsonNumberHandling.WriteAsString)]
-        public class AttributeOnFirstLevel
+        public class AttributeAppliedToFirstLevelProp
         {
             public BadProperty NestedClass { get; set; }
         }