Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Allow trailing trivia in JsonSerializer.Parse and Read #37500 #37549

Merged
ahsonkhan merged 7 commits into from
May 14, 2019
Merged

Allow trailing trivia in JsonSerializer.Parse and Read #37500 #37549

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
64da14f
Add tests for trailing trivia to JsonSerializer
watfordgnf May 9, 2019
e2eb92c
Consume trailing trivia in JsonSerializer.ReadCore
watfordgnf May 9, 2019
66a0e5d
Add tests for leading trivia too
watfordgnf May 10, 2019
cd324b7
Simplify ReadCore loop per @ahsonkhan review
watfordgnf May 10, 2019
a6da3d4
Do not compute PropertyPath if outside document
watfordgnf May 10, 2019
bbb432a
Add additional test cases for primitives
watfordgnf May 13, 2019
f3b4513
Clean up tests and add negative test for leading/trailing comments
watfordgnf May 13, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions src/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ private static void ReadCore(

if (HandleValue(tokenType, options, ref reader, ref state))
Copy link
Copy Markdown

@ahsonkhan ahsonkhan May 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As an aside, @steveharter, returning "true" in these HandleX helper methods to indicate there is no more data/we are done is the opposite of what I would have expected. We should consider inverting all these conditions so that "true" means success/keep reading, and false means "i'm done/return to the caller".

Copy link
Copy Markdown
Contributor Author

@watfordgnf watfordgnf May 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Provided I can address all of your other feedback, I'm up for making this change (unless it would be better done in a follow-up PR).

Copy link
Copy Markdown

@ahsonkhan ahsonkhan May 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Provided I can address all of your other feedback, I'm up for making this change

Great! cc @JeremyKuhne (in case you disagree with flipping the condition). I know that you were in favor of such a change, when we talked previously.

unless it would be better done in a follow-up PR

I think that would be best. Let's keep this PR isolated to the bug fix.

Copy link
Copy Markdown
Contributor Author

@watfordgnf watfordgnf May 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. I'll wait until the other PR's land that are affecting HandleXXX.

Copy link
Copy Markdown
Contributor

@steveharter steveharter May 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One benefit of the direct return like the original is to support future cases (in the upcoming extensibility model) where the reader may be passed into a new static serializer Parse() method. In that case we want to stop when we finish reading the current scope (object for example) and not throw an exception. There may be different ways to detect\support that but it is something to track when we add that feature.

Copy link
Copy Markdown
Contributor

@steveharter steveharter May 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does JsonDocument support trailing trivia?

Copy link
Copy Markdown

@ahsonkhan ahsonkhan May 14, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it does. We have a few tests around that:

corefx/src/System.Text.Json/tests/JsonDocumentTests.cs

Line 783 in 10d0327

using (JsonDocument doc = JsonDocument.Parse(" " + value + " "))

corefx/src/System.Text.Json/tests/JsonDocumentTests.cs

Lines 1443 to 1447 in 10d0327

[Theory]
[InlineData("\"hello\" ", "hello")]
[InlineData(" null ", (string)null)]
[InlineData("\"\\u0033\\u0031\"", "31")]
public static void ReadString(string json, string expectedValue)

cc @bartonjs

{
return;
continue;
}
}
else if (tokenType == JsonTokenType.PropertyName)
Expand Down Expand Up @@ -82,14 +82,14 @@ private static void ReadCore(
}
else if (HandleValue(tokenType, options, ref reader, ref state))
{
return;
continue;
}
}
else if (tokenType == JsonTokenType.EndObject)
{
if (HandleEndObject(options, ref state, ref reader))
{
return;
continue;
}
}
else if (tokenType == JsonTokenType.StartArray)
Expand All @@ -100,21 +100,21 @@ private static void ReadCore(
}
else if (HandleValue(tokenType, options, ref reader, ref state))
{
return;
continue;
}
}
else if (tokenType == JsonTokenType.EndArray)
{
if (HandleEndArray(options, ref state, ref reader))
{
return;
continue;
}
}
else if (tokenType == JsonTokenType.Null)
{
if (HandleNull(ref reader, ref state, options))
{
return;
continue;
}
}
}
Expand Down
12 changes: 9 additions & 3 deletions src/System.Text.Json/src/System/Text/Json/Serialization/ReadStack.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Copy Markdown

@ahsonkhan ahsonkhan May 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am just wondering if this is the only case we would want to return "<none>".

{
return "<none>";
}

path = new StringBuilder($"[{Current.JsonClassInfo.Type.FullName}]");
}
else
{
path.Append($"[{_previous[0].JsonClassInfo.Type.FullName}]");
path = new StringBuilder($"[{_previous[0].JsonClassInfo.Type.FullName}]");
Copy link
Copy Markdown

@ahsonkhan ahsonkhan May 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can _previous[0].JsonClassInfo ever be null, similar to the if (Current.JsonClassInfo == null) case above?

Copy link
Copy Markdown
Contributor Author

@watfordgnf watfordgnf May 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried various levels of invalid nesting or unexpected types and could not elicit an NPE down this path.

Copy link
Copy Markdown

@ahsonkhan ahsonkhan May 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great. Good to know :)


for (int i = 0; i < _index; i++)
{
Expand Down
19 changes: 19 additions & 0 deletions src/System.Text.Json/tests/Serialization/Null.ReadTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,5 +72,24 @@ public static void NullLiteralObjectInput()
Assert.Equal("null", obj);
}
}

[Fact]
public static void NullAcceptsLeadingAndTrailingTrivia()
{
{
TestClassWithNull obj = JsonSerializer.Parse<TestClassWithNull>(" null");
Assert.Null(obj);
}

{
object obj = JsonSerializer.Parse<object>("null ");
Assert.Null(obj);
}

{
object obj = JsonSerializer.Parse<object>(" null\t");
Assert.Null(obj);
}
}
}
}
71 changes: 71 additions & 0 deletions src/System.Text.Json/tests/Serialization/Object.ReadTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<SimpleTestClass>(leadingTrivia + SimpleTestClass.s_json + trailingTrivia, options);
obj.Verify();
}

{
var options = new JsonSerializerOptions();
options.ReadCommentHandling = JsonCommentHandling.Allow;

SimpleTestClass obj = JsonSerializer.Parse<SimpleTestClass>(leadingTrivia + SimpleTestClass.s_json + trailingTrivia, options);
obj.Verify();
}
}

[Fact]
public static void ReadSimpleClassWithObject()
{
Expand All @@ -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<JsonException>(() => JsonSerializer.Parse<ClassWithComplexObjects>(leadingTrivia + ClassWithComplexObjects.s_json + trailingTrivia));
}

[Fact]
public static void ReadSimpleClassWithObjectArray()
{
Expand Down Expand Up @@ -63,6 +106,34 @@ public static void ReadClassWithComplexObjects()
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 ")]
Comment thread
watfordgnf marked this conversation as resolved.
public static void ReadComplexClassIgnoresLeadingOrTrailingTrivia(string leadingTrivia, string trailingTrivia)
{
var options = new JsonSerializerOptions();
options.ReadCommentHandling = JsonCommentHandling.Skip;
Comment thread
watfordgnf marked this conversation as resolved.

ClassWithComplexObjects obj = JsonSerializer.Parse<ClassWithComplexObjects>(leadingTrivia + ClassWithComplexObjects.s_json + trailingTrivia, options);
Copy link
Copy Markdown

@ahsonkhan ahsonkhan May 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As an aside (unrelated to this PR), we should consider adding tests where we have comments in-between the JSON objects as well to make sure the serializer continues to handle that. We have such tests for the reader/document, but I am not sure we have them for the serializer.

obj.Verify();

// Throws due to JsonDocument.TryParse not supporting Allow
//var options = new JsonSerializerOptions();
//options.ReadCommentHandling = JsonCommentHandling.Allow;
//
//obj = JsonSerializer.Parse<ClassWithComplexObjects>(leadingTrivia + ClassWithComplexObjects.s_json + trailingTrivia, options);
Copy link
Copy Markdown

@ahsonkhan ahsonkhan May 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I think the most intuitive solution here would be that the serializer rejects Allow comments, which is why I think we should remove the commented out test code like this. I have filed an issue instead: https://github.com/dotnet/corefx/issues/37634

//obj.Verify();
}

[Fact]
public static void ReadEmpty()
{
Expand Down
33 changes: 33 additions & 0 deletions src/System.Text.Json/tests/Serialization/Stream.ReadTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<SimpleTestClass>(stream, options);
obj.Verify();
}
}

[Fact]
public static async Task ReadPrimitivesAsync()
{
Expand All @@ -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,
Copy link
Copy Markdown

@ahsonkhan ahsonkhan May 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would add a test for "Allow comments" as well (especially for more than 1 leading/trailing comment).

Copy link
Copy Markdown
Contributor Author

@watfordgnf watfordgnf May 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you nest complex documents, JsonSerializer uses JsonDocument which throws:

System.Text.Json.Serialization.Tests.ObjectTests.ReadClassIgnoresLeadingOrTrailingTrivia(leadingTrivia: "/* Multi\nLine\nComment */ ", trailingTrivia: "\t// trailing comment\n ") [FAIL]
      System.ArgumentException : Comments cannot be stored in a JsonDocument, only the Skip and Disallow comment handling modes are supported.
      Parameter name: reader

Copy link
Copy Markdown

@ahsonkhan ahsonkhan May 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. In that case, can we add a test for simple POCO (that doesn't have System.Object as a property), with allow comments mode? The JsonDocument only comes into play for types with System.Object properties (or Dictionary<string, object>, etc.)

@steveharter, @JeremyKuhne, @bartonjs - this is something we should fix up. Either the serialize should disallow "allow comments" similar to JsonDocument OR convert it to skip before passing it to JsonDocument since the "allow" mode doesn't make much sense in this context. The inconsistency in behavior is unnecessary and will cause confusion.

@watfordgnf, this can be deferred for now (i.e. the PR doesn't need to block on this discussion).

Copy link
Copy Markdown
Contributor Author

@watfordgnf watfordgnf May 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good.

};

int i = await JsonSerializer.ReadAsync<int>(stream, options);
Assert.Equal(1, i);
}
}
}
}
33 changes: 33 additions & 0 deletions src/System.Text.Json/tests/Serialization/Value.ReadTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,37 @@ public static void ReadPrimitives()
Assert.Equal("Hello", s2);
}

[Fact]
public static void ReadPrimitivesWithWhitespace()
{
int i = JsonSerializer.Parse<int>(Encoding.UTF8.GetBytes(@" 1 "));
Comment thread
watfordgnf marked this conversation as resolved.
Assert.Equal(1, i);

int i2 = JsonSerializer.Parse<int>("2\t");
Assert.Equal(2, i2);

int? i3 = JsonSerializer.Parse<int?>("\r\nnull");
Assert.Null(i3);

long l = JsonSerializer.Parse<long>(Encoding.UTF8.GetBytes("\t" + long.MaxValue.ToString()));
Assert.Equal(long.MaxValue, l);

long l2 = JsonSerializer.Parse<long>(long.MaxValue.ToString() + " \r\n");
Assert.Equal(long.MaxValue, l2);

string s = JsonSerializer.Parse<string>(Encoding.UTF8.GetBytes(@"""Hello"" "));
Assert.Equal("Hello", s);

string s2 = JsonSerializer.Parse<string>(@" ""Hello"" ");
Copy link
Copy Markdown

@ahsonkhan ahsonkhan May 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking a bit more about using continue within the main loop (rather than breaking and doing one final Read with validation), I wonder what would happen if we had 2 primitive tokens next to each other. This should continue to throw (presumably the underlying reader will catch such cases), so this is probably fine, but just making sure.

string s3 = JsonSerializer.Parse<string>(@"""Hello"" ""Hello""");
string s4 = JsonSerializer.Parse<string>(@"""Hello"" 42");

Copy link
Copy Markdown
Member

@JamesNK JamesNK May 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The serializer should definitely throw when given a string like this.

In Json.NET there is a setting on the JSON reader to allow multiple content. Additional content can optionally be delimited by a comma

{'hi':true}{'bye':true} and {'hi':true},{'bye':true} could be read by the reader when the flag was true.

It is a Useful Feature. People like it when streaming huge JSON content. You don't need it for 3.0.

Copy link
Copy Markdown
Contributor Author

@watfordgnf watfordgnf May 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added two extra cases to Value.ReadTests ReadPrimitiveExtraBytesFail to cover that scenari @ahsonkhan.

Assert.Equal("Hello", s2);

bool b = JsonSerializer.Parse<bool>(" \ttrue ");
Assert.Equal(true, b);

bool b2 = JsonSerializer.Parse<bool>(" false\n");
Assert.Equal(false, b2);
}

[Fact]
public static void ReadPrimitivesFail()
{
Expand Down Expand Up @@ -84,6 +115,8 @@ public static void ReadPrimitiveExtraBytesFail()
{
Assert.Throws<JsonException>(() => JsonSerializer.Parse<int[]>("[2] {3}"));
Assert.Throws<JsonException>(() => JsonSerializer.Parse<int[]>(Encoding.UTF8.GetBytes(@"[2] {3}")));
Assert.Throws<JsonException>(() => JsonSerializer.Parse<string>(@"""Hello"" 42"));
Assert.Throws<JsonException>(() => JsonSerializer.Parse<string>(Encoding.UTF8.GetBytes(@"""Hello"" 42")));
}

[Fact]
Expand Down