From 64da14f2aaefe5e550b04af48638a57d48365922 Mon Sep 17 00:00:00 2001 From: Christopher Watford Date: Thu, 9 May 2019 10:55:49 -0400 Subject: [PATCH 1/7] Add tests for trailing trivia to JsonSerializer Contributes to #37500 --- .../tests/Serialization/Null.ReadTests.cs | 19 +++++++++++ .../tests/Serialization/Object.ReadTests.cs | 26 +++++++++++++++ .../tests/Serialization/Stream.ReadTests.cs | 33 +++++++++++++++++++ .../tests/Serialization/Value.ReadTests.cs | 25 ++++++++++++++ 4 files changed, 103 insertions(+) diff --git a/src/System.Text.Json/tests/Serialization/Null.ReadTests.cs b/src/System.Text.Json/tests/Serialization/Null.ReadTests.cs index c61dab68fb67..ce121432ad53 100644 --- a/src/System.Text.Json/tests/Serialization/Null.ReadTests.cs +++ b/src/System.Text.Json/tests/Serialization/Null.ReadTests.cs @@ -72,5 +72,24 @@ public static void NullLiteralObjectInput() Assert.Equal("null", obj); } } + + [Fact] + public static void NullAcceptsLeadingAndTrailingTrivia() + { + { + TestClassWithNull obj = JsonSerializer.Parse(" null"); + Assert.Null(obj); + } + + { + object obj = JsonSerializer.Parse("null "); + Assert.Null(obj); + } + + { + object obj = JsonSerializer.Parse(" null\t"); + Assert.Null(obj); + } + } } } diff --git a/src/System.Text.Json/tests/Serialization/Object.ReadTests.cs b/src/System.Text.Json/tests/Serialization/Object.ReadTests.cs index bcc76b72396d..091da5425727 100644 --- a/src/System.Text.Json/tests/Serialization/Object.ReadTests.cs +++ b/src/System.Text.Json/tests/Serialization/Object.ReadTests.cs @@ -63,6 +63,32 @@ public static void ReadClassWithComplexObjects() obj.Verify(); } + [Fact] + public static void ReadClassWithTrailingWhitespace() + { + ClassWithComplexObjects obj = JsonSerializer.Parse(ClassWithComplexObjects.s_json + " \r\n\t"); + obj.Verify(); + string reserialized = JsonSerializer.ToString(obj); + + // Properties in the exported json will be in the order that they were reflected, doing a quick check to see that + // we end up with the same length (i.e. same amount of data) to start. + Assert.Equal(ClassWithComplexObjects.s_json.StripWhitespace().Length, reserialized.Length); + + // Shoving it back through the parser should validate round tripping. + obj = JsonSerializer.Parse(reserialized); + obj.Verify(); + } + + [Fact] + public static void ReadClassWithTrailingComments() + { + var options = new JsonSerializerOptions(); + options.ReadCommentHandling = JsonCommentHandling.Skip; + + ClassWithComplexObjects obj = JsonSerializer.Parse(ClassWithComplexObjects.s_json + "// Single Line\n/* Multi\nLine Comment */ ", options); + obj.Verify(); + } + [Fact] public static void ReadEmpty() { diff --git a/src/System.Text.Json/tests/Serialization/Stream.ReadTests.cs b/src/System.Text.Json/tests/Serialization/Stream.ReadTests.cs index 3e372b5e8c4d..4b1a3531f4b9 100644 --- a/src/System.Text.Json/tests/Serialization/Stream.ReadTests.cs +++ b/src/System.Text.Json/tests/Serialization/Stream.ReadTests.cs @@ -32,6 +32,23 @@ public static async Task ReadSimpleObjectAsync() } } + [Fact] + public static async Task ReadSimpleObjectWithTrailingTriviaAsync() + { + byte[] data = Encoding.UTF8.GetBytes(SimpleTestClass.s_json + " /* Multi\r\nLine Comment */\t"); + using (MemoryStream stream = new MemoryStream(data)) + { + JsonSerializerOptions options = new JsonSerializerOptions + { + DefaultBufferSize = 1, + ReadCommentHandling = JsonCommentHandling.Skip, + }; + + SimpleTestClass obj = await JsonSerializer.ReadAsync(stream, options); + obj.Verify(); + } + } + [Fact] public static async Task ReadPrimitivesAsync() { @@ -46,5 +63,21 @@ public static async Task ReadPrimitivesAsync() Assert.Equal(1, i); } } + + [Fact] + public static async Task ReadPrimitivesWithTrailingTriviaAsync() + { + using (MemoryStream stream = new MemoryStream(Encoding.UTF8.GetBytes(" 1\t// Comment\r\n/* Multi\r\nLine */"))) + { + JsonSerializerOptions options = new JsonSerializerOptions + { + DefaultBufferSize = 1, + ReadCommentHandling = JsonCommentHandling.Skip, + }; + + int i = await JsonSerializer.ReadAsync(stream, options); + Assert.Equal(1, i); + } + } } } diff --git a/src/System.Text.Json/tests/Serialization/Value.ReadTests.cs b/src/System.Text.Json/tests/Serialization/Value.ReadTests.cs index 1fdd0b05977f..f574ddf87a57 100644 --- a/src/System.Text.Json/tests/Serialization/Value.ReadTests.cs +++ b/src/System.Text.Json/tests/Serialization/Value.ReadTests.cs @@ -34,6 +34,31 @@ public static void ReadPrimitives() Assert.Equal("Hello", s2); } + [Fact] + public static void ReadPrimitivesWithWhitespace() + { + int i = JsonSerializer.Parse(Encoding.UTF8.GetBytes(@" 1 ")); + Assert.Equal(1, i); + + int i2 = JsonSerializer.Parse("2\t"); + Assert.Equal(2, i2); + + int? i3 = JsonSerializer.Parse("\r\nnull"); + Assert.Null(i3); + + long l = JsonSerializer.Parse(Encoding.UTF8.GetBytes("\t" + long.MaxValue.ToString())); + Assert.Equal(long.MaxValue, l); + + long l2 = JsonSerializer.Parse(long.MaxValue.ToString() + " \r\n"); + Assert.Equal(long.MaxValue, l2); + + string s = JsonSerializer.Parse(Encoding.UTF8.GetBytes(@"""Hello"" ")); + Assert.Equal("Hello", s); + + string s2 = JsonSerializer.Parse(@" ""Hello"" "); + Assert.Equal("Hello", s2); + } + [Fact] public static void ReadPrimitivesFail() { From e2eb92c85e75c7bd91b237fd229f35afa29b0a83 Mon Sep 17 00:00:00 2001 From: Christopher Watford Date: Thu, 9 May 2019 10:57:29 -0400 Subject: [PATCH 2/7] Consume trailing trivia in JsonSerializer.ReadCore Fixes #37500 --- .../Json/Serialization/JsonSerializer.Read.cs | 48 ++++++++++++++++--- 1 file changed, 42 insertions(+), 6 deletions(-) diff --git a/src/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.cs b/src/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.cs index 69cd87d3cf4f..5d1e221a24dc 100644 --- a/src/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.cs +++ b/src/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.cs @@ -33,7 +33,7 @@ private static void ReadCore( if (HandleValue(tokenType, options, ref reader, ref state)) { - return; + break; } } else if (tokenType == JsonTokenType.PropertyName) @@ -82,14 +82,14 @@ private static void ReadCore( } else if (HandleValue(tokenType, options, ref reader, ref state)) { - return; + break; } } else if (tokenType == JsonTokenType.EndObject) { if (HandleEndObject(options, ref state, ref reader)) { - return; + break; } } else if (tokenType == JsonTokenType.StartArray) @@ -100,21 +100,21 @@ private static void ReadCore( } else if (HandleValue(tokenType, options, ref reader, ref state)) { - return; + break; } } else if (tokenType == JsonTokenType.EndArray) { if (HandleEndArray(options, ref state, ref reader)) { - return; + break; } } else if (tokenType == JsonTokenType.Null) { if (HandleNull(ref reader, ref state, options)) { - return; + break; } } } @@ -125,9 +125,45 @@ private static void ReadCore( ThrowHelper.ReThrowWithPath(e, state.PropertyPath); } + // Ensure any trailing whitespace or comments (if allowed) are cleaned up + ConsumeTrailingWhitespaceAndComments(ref reader, options); + return; } + private static void ConsumeTrailingWhitespaceAndComments(ref Utf8JsonReader reader, JsonSerializerOptions options) + { + try + { + while (reader.Read()) + { + switch (reader.TokenType) + { + case JsonTokenType.None: + // Consume whitespace + continue; + + case JsonTokenType.Comment: + // Consume comments if allowed + if (options.ReadCommentHandling != JsonCommentHandling.Disallow) + { + continue; + } + goto default; + + default: + // Stop at any other token we find + return; + } + } + } + catch (JsonReaderException e) + { + // Re-throw without path information (outside of an object) + ThrowHelper.ReThrowWithPath(e, ""); + } + } + private static ReadOnlySpan GetUnescapedString(ReadOnlySpan utf8Source, int idx) { // The escaped name is always longer than the unescaped, so it is safe to use escaped name for the buffer length. From 66a0e5dc8ad5df025e835f20fb8b8d934575a92e Mon Sep 17 00:00:00 2001 From: Christopher Watford Date: Fri, 10 May 2019 09:36:54 -0400 Subject: [PATCH 3/7] Add tests for leading trivia too - Consolidates leading and trailing trivia tests into one --- .../tests/Serialization/Object.ReadTests.cs | 32 ++++++++----------- 1 file changed, 13 insertions(+), 19 deletions(-) diff --git a/src/System.Text.Json/tests/Serialization/Object.ReadTests.cs b/src/System.Text.Json/tests/Serialization/Object.ReadTests.cs index 091da5425727..a07afad59ccb 100644 --- a/src/System.Text.Json/tests/Serialization/Object.ReadTests.cs +++ b/src/System.Text.Json/tests/Serialization/Object.ReadTests.cs @@ -63,29 +63,23 @@ public static void ReadClassWithComplexObjects() obj.Verify(); } - [Fact] - public static void ReadClassWithTrailingWhitespace() - { - ClassWithComplexObjects obj = JsonSerializer.Parse(ClassWithComplexObjects.s_json + " \r\n\t"); - obj.Verify(); - string reserialized = JsonSerializer.ToString(obj); - - // Properties in the exported json will be in the order that they were reflected, doing a quick check to see that - // we end up with the same length (i.e. same amount of data) to start. - Assert.Equal(ClassWithComplexObjects.s_json.StripWhitespace().Length, reserialized.Length); - - // Shoving it back through the parser should validate round tripping. - obj = JsonSerializer.Parse(reserialized); - obj.Verify(); - } - - [Fact] - public static void ReadClassWithTrailingComments() + [Theory] + [InlineData("", " ")] + [InlineData("", "\t ")] + [InlineData("", "//Single Line Comment\r\n")] + [InlineData("", "/* Multi\nLine Comment */")] + [InlineData("", "\t\t\t\n// Both\n/* Comments */")] + [InlineData(" ", "")] + [InlineData("\t ", "")] + [InlineData("// Leading Comment\n", "")] + [InlineData("/* Multi\nLine\nComment */ ", "")] + [InlineData("/* Multi\nLine\nComment */ ", "\t// trailing comment\n ")] + public static void ReadClassIgnoresLeadingOrTrailingTrivia(string leadingTrivia, string trailingTrivia) { var options = new JsonSerializerOptions(); options.ReadCommentHandling = JsonCommentHandling.Skip; - ClassWithComplexObjects obj = JsonSerializer.Parse(ClassWithComplexObjects.s_json + "// Single Line\n/* Multi\nLine Comment */ ", options); + ClassWithComplexObjects obj = JsonSerializer.Parse(leadingTrivia + ClassWithComplexObjects.s_json + trailingTrivia, options); obj.Verify(); } From cd324b7617cf47421195f31d21f73e89e462bd61 Mon Sep 17 00:00:00 2001 From: Christopher Watford Date: Fri, 10 May 2019 09:37:55 -0400 Subject: [PATCH 4/7] Simplify ReadCore loop per @ahsonkhan review NOTE: throws NPE in state.PropertyPath if invalid JSON is in the trailer. --- .../Json/Serialization/JsonSerializer.Read.cs | 48 +++---------------- 1 file changed, 6 insertions(+), 42 deletions(-) diff --git a/src/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.cs b/src/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.cs index 5d1e221a24dc..59aa8e633bff 100644 --- a/src/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.cs +++ b/src/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.cs @@ -33,7 +33,7 @@ private static void ReadCore( if (HandleValue(tokenType, options, ref reader, ref state)) { - break; + continue; } } else if (tokenType == JsonTokenType.PropertyName) @@ -82,14 +82,14 @@ private static void ReadCore( } else if (HandleValue(tokenType, options, ref reader, ref state)) { - break; + continue; } } else if (tokenType == JsonTokenType.EndObject) { if (HandleEndObject(options, ref state, ref reader)) { - break; + continue; } } else if (tokenType == JsonTokenType.StartArray) @@ -100,21 +100,21 @@ private static void ReadCore( } else if (HandleValue(tokenType, options, ref reader, ref state)) { - break; + continue; } } else if (tokenType == JsonTokenType.EndArray) { if (HandleEndArray(options, ref state, ref reader)) { - break; + continue; } } else if (tokenType == JsonTokenType.Null) { if (HandleNull(ref reader, ref state, options)) { - break; + continue; } } } @@ -125,45 +125,9 @@ private static void ReadCore( ThrowHelper.ReThrowWithPath(e, state.PropertyPath); } - // Ensure any trailing whitespace or comments (if allowed) are cleaned up - ConsumeTrailingWhitespaceAndComments(ref reader, options); - return; } - private static void ConsumeTrailingWhitespaceAndComments(ref Utf8JsonReader reader, JsonSerializerOptions options) - { - try - { - while (reader.Read()) - { - switch (reader.TokenType) - { - case JsonTokenType.None: - // Consume whitespace - continue; - - case JsonTokenType.Comment: - // Consume comments if allowed - if (options.ReadCommentHandling != JsonCommentHandling.Disallow) - { - continue; - } - goto default; - - default: - // Stop at any other token we find - return; - } - } - } - catch (JsonReaderException e) - { - // Re-throw without path information (outside of an object) - ThrowHelper.ReThrowWithPath(e, ""); - } - } - private static ReadOnlySpan GetUnescapedString(ReadOnlySpan utf8Source, int idx) { // The escaped name is always longer than the unescaped, so it is safe to use escaped name for the buffer length. From a6da3d45a9aab7d4cfee565cebf1a9e229d55a45 Mon Sep 17 00:00:00 2001 From: Christopher Watford Date: Fri, 10 May 2019 09:46:00 -0400 Subject: [PATCH 5/7] Do not compute PropertyPath if outside document --- .../src/System/Text/Json/Serialization/ReadStack.cs | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/System.Text.Json/src/System/Text/Json/Serialization/ReadStack.cs b/src/System.Text.Json/src/System/Text/Json/Serialization/ReadStack.cs index 1e68821b443e..650db0a74c4e 100644 --- a/src/System.Text.Json/src/System/Text/Json/Serialization/ReadStack.cs +++ b/src/System.Text.Json/src/System/Text/Json/Serialization/ReadStack.cs @@ -53,15 +53,21 @@ public string PropertyPath { get { - StringBuilder path = new StringBuilder(); + StringBuilder path; if (_previous == null || _index == 0) { - path.Append($"[{Current.JsonClassInfo.Type.FullName}]"); + // No path if we've walked beyond the end of our JSON document + if (Current.JsonClassInfo == null) + { + return ""; + } + + path = new StringBuilder($"[{Current.JsonClassInfo.Type.FullName}]"); } else { - path.Append($"[{_previous[0].JsonClassInfo.Type.FullName}]"); + path = new StringBuilder($"[{_previous[0].JsonClassInfo.Type.FullName}]"); for (int i = 0; i < _index; i++) { From bbb432a4746c2ecd3faa4711cd545edc342461dc Mon Sep 17 00:00:00 2001 From: Christopher Watford Date: Mon, 13 May 2019 10:10:09 -0400 Subject: [PATCH 6/7] Add additional test cases for primitives --- .../tests/Serialization/Object.ReadTests.cs | 1 + .../tests/Serialization/Value.ReadTests.cs | 8 ++++++++ 2 files changed, 9 insertions(+) diff --git a/src/System.Text.Json/tests/Serialization/Object.ReadTests.cs b/src/System.Text.Json/tests/Serialization/Object.ReadTests.cs index a07afad59ccb..a9f6d58087f8 100644 --- a/src/System.Text.Json/tests/Serialization/Object.ReadTests.cs +++ b/src/System.Text.Json/tests/Serialization/Object.ReadTests.cs @@ -71,6 +71,7 @@ public static void ReadClassWithComplexObjects() [InlineData("", "\t\t\t\n// Both\n/* Comments */")] [InlineData(" ", "")] [InlineData("\t ", "")] + [InlineData(" \t", " \n")] [InlineData("// Leading Comment\n", "")] [InlineData("/* Multi\nLine\nComment */ ", "")] [InlineData("/* Multi\nLine\nComment */ ", "\t// trailing comment\n ")] diff --git a/src/System.Text.Json/tests/Serialization/Value.ReadTests.cs b/src/System.Text.Json/tests/Serialization/Value.ReadTests.cs index f574ddf87a57..0a561a34f8e6 100644 --- a/src/System.Text.Json/tests/Serialization/Value.ReadTests.cs +++ b/src/System.Text.Json/tests/Serialization/Value.ReadTests.cs @@ -57,6 +57,12 @@ public static void ReadPrimitivesWithWhitespace() string s2 = JsonSerializer.Parse(@" ""Hello"" "); Assert.Equal("Hello", s2); + + bool b = JsonSerializer.Parse(" \ttrue "); + Assert.Equal(true, b); + + bool b2 = JsonSerializer.Parse(" false\n"); + Assert.Equal(false, b2); } [Fact] @@ -109,6 +115,8 @@ public static void ReadPrimitiveExtraBytesFail() { Assert.Throws(() => JsonSerializer.Parse("[2] {3}")); Assert.Throws(() => JsonSerializer.Parse(Encoding.UTF8.GetBytes(@"[2] {3}"))); + Assert.Throws(() => JsonSerializer.Parse(@"""Hello"" 42")); + Assert.Throws(() => JsonSerializer.Parse(Encoding.UTF8.GetBytes(@"""Hello"" 42"))); } [Fact] From f3b45136ef4879503eb030844a7179dc7ade208a Mon Sep 17 00:00:00 2001 From: Christopher Watford Date: Mon, 13 May 2019 10:39:48 -0400 Subject: [PATCH 7/7] Clean up tests and add negative test for leading/trailing comments --- .../tests/Serialization/Object.ReadTests.cs | 52 ++++++++++++++++++- 1 file changed, 51 insertions(+), 1 deletion(-) diff --git a/src/System.Text.Json/tests/Serialization/Object.ReadTests.cs b/src/System.Text.Json/tests/Serialization/Object.ReadTests.cs index a9f6d58087f8..ecff3a232b40 100644 --- a/src/System.Text.Json/tests/Serialization/Object.ReadTests.cs +++ b/src/System.Text.Json/tests/Serialization/Object.ReadTests.cs @@ -15,6 +15,37 @@ public static void ReadSimpleClass() obj.Verify(); } + [Theory] + [InlineData("", " ")] + [InlineData("", "\t ")] + [InlineData("", "//Single Line Comment\r\n")] + [InlineData("", "/* Multi\nLine Comment */")] + [InlineData("", "\t\t\t\n// Both\n/* Comments */")] + [InlineData(" ", "")] + [InlineData("\t ", "")] + [InlineData(" \t", " \n")] + [InlineData("// Leading Comment\n", "")] + [InlineData("/* Multi\nLine\nComment */ ", "")] + [InlineData("/* Multi\nLine\nComment */ ", "\t// trailing comment\n ")] + public static void ReadSimpleClassIgnoresLeadingOrTrailingTrivia(string leadingTrivia, string trailingTrivia) + { + { + var options = new JsonSerializerOptions(); + options.ReadCommentHandling = JsonCommentHandling.Skip; + + SimpleTestClass obj = JsonSerializer.Parse(leadingTrivia + SimpleTestClass.s_json + trailingTrivia, options); + obj.Verify(); + } + + { + var options = new JsonSerializerOptions(); + options.ReadCommentHandling = JsonCommentHandling.Allow; + + SimpleTestClass obj = JsonSerializer.Parse(leadingTrivia + SimpleTestClass.s_json + trailingTrivia, options); + obj.Verify(); + } + } + [Fact] public static void ReadSimpleClassWithObject() { @@ -31,6 +62,18 @@ public static void ReadSimpleClassWithObject() obj.Verify(); } + [Theory] + [InlineData("", "//Single Line Comment\r\n")] + [InlineData("", "/* Multi\nLine Comment */")] + [InlineData("", "\t\t\t\n// Both\n/* Comments */")] + [InlineData("// Leading Comment\n", "")] + [InlineData("/* Multi\nLine\nComment */ ", "")] + [InlineData("/* Multi\nLine\nComment */ ", "\t// trailing comment\n ")] + public static void ReadClassWithCommentsThrowsIfDisallowed(string leadingTrivia, string trailingTrivia) + { + Assert.Throws(() => JsonSerializer.Parse(leadingTrivia + ClassWithComplexObjects.s_json + trailingTrivia)); + } + [Fact] public static void ReadSimpleClassWithObjectArray() { @@ -75,13 +118,20 @@ public static void ReadClassWithComplexObjects() [InlineData("// Leading Comment\n", "")] [InlineData("/* Multi\nLine\nComment */ ", "")] [InlineData("/* Multi\nLine\nComment */ ", "\t// trailing comment\n ")] - public static void ReadClassIgnoresLeadingOrTrailingTrivia(string leadingTrivia, string trailingTrivia) + public static void ReadComplexClassIgnoresLeadingOrTrailingTrivia(string leadingTrivia, string trailingTrivia) { var options = new JsonSerializerOptions(); options.ReadCommentHandling = JsonCommentHandling.Skip; ClassWithComplexObjects obj = JsonSerializer.Parse(leadingTrivia + ClassWithComplexObjects.s_json + trailingTrivia, options); obj.Verify(); + + // Throws due to JsonDocument.TryParse not supporting Allow + //var options = new JsonSerializerOptions(); + //options.ReadCommentHandling = JsonCommentHandling.Allow; + // + //obj = JsonSerializer.Parse(leadingTrivia + ClassWithComplexObjects.s_json + trailingTrivia, options); + //obj.Verify(); } [Fact]