Allow trailing trivia in JsonSerializer.Parse and Read #37500 by watfordgnf · Pull Request #37549 · dotnet/corefx

watfordgnf · 2019-05-09T15:09:55Z

This fixes #37500 by consuming all leading and trailing trivia in JsonSerializer.ReadCore. Comments are only consumed if JsonCommentHandling is not Disallow.

Contributes to #37500

Fixes #37500

dnfclas · 2019-05-09T15:10:09Z

All CLA requirements met.

ahsonkhan

I think the fix/implementation can be simplified. I would consider adjusting the main reader loop within ReadCore itself for the fix.

ahsonkhan · 2019-05-10T01:11:16Z

@@ -33,7 +33,7 @@ private static void ReadCore(

                        if (HandleValue(tokenType, options, ref reader, ref state))


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".

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).

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.

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

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.

Does JsonDocument support trailing trivia?

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

ahsonkhan · 2019-05-10T01:16:07Z

+                JsonSerializerOptions options = new JsonSerializerOptions
+                {
+                    DefaultBufferSize = 1,
+                    ReadCommentHandling = JsonCommentHandling.Skip,


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

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

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).

Sounds good.

- Consolidates leading and trailing trivia tests into one

NOTE: throws NPE in state.PropertyPath if invalid JSON is in the trailer.

ahsonkhan

Some nits for more tests, but otherwise LGTM. Thanks for fixing the issue and the quick response :)

ahsonkhan · 2019-05-10T23:32:24Z

                else
                {
-                    path.Append($"[{_previous[0].JsonClassInfo.Type.FullName}]");
+                    path = new StringBuilder($"[{_previous[0].JsonClassInfo.Type.FullName}]");


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

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

Great. Good to know :)

ahsonkhan · 2019-05-10T23:33:07Z

                {
-                    path.Append($"[{Current.JsonClassInfo.Type.FullName}]");
+                    // No path if we've walked beyond the end of our JSON document
+                    if (Current.JsonClassInfo == null)


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

ahsonkhan · 2019-05-10T23:36:08Z

+                JsonSerializerOptions options = new JsonSerializerOptions
+                {
+                    DefaultBufferSize = 1,
+                    ReadCommentHandling = JsonCommentHandling.Skip,


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).

ahsonkhan · 2019-05-10T23:41:40Z

+            var options = new JsonSerializerOptions();
+            options.ReadCommentHandling = JsonCommentHandling.Skip;
+
+            ClassWithComplexObjects obj = JsonSerializer.Parse<ClassWithComplexObjects>(leadingTrivia + ClassWithComplexObjects.s_json + trailingTrivia, options);


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.

ahsonkhan · 2019-05-10T23:51:26Z

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


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");

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.

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

ahsonkhan · 2019-05-13T23:59:34Z

+            //var options = new JsonSerializerOptions();
+            //options.ReadCommentHandling = JsonCommentHandling.Allow;
+            //
+            //obj = JsonSerializer.Parse<ClassWithComplexObjects>(leadingTrivia + ClassWithComplexObjects.s_json + trailingTrivia, options);


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

ahsonkhan · 2019-05-14T03:59:31Z

@watfordgnf, thanks for the contribution. I am looking forward to working with you more, in the future! :)

watfordgnf · 2019-05-14T15:18:23Z

I'm glad I could help, and thank you for working with me to get this into the right shape.

@ahsonkhan

…37500 (dotnet/corefx#37549) * Add tests for trailing trivia to JsonSerializer Contributes to dotnet/corefx#37500 * Consume trailing trivia in JsonSerializer.ReadCore Fixes dotnet/corefx#37500 * Add tests for leading trivia too - Consolidates leading and trailing trivia tests into one * Simplify ReadCore loop per @ahsonkhan review NOTE: throws NPE in state.PropertyPath if invalid JSON is in the trailer. * Do not compute PropertyPath if outside document * Add additional test cases for primitives * Clean up tests and add negative test for leading/trailing comments Commit migrated from dotnet/corefx@2b8126e

watfordgnf added 2 commits May 9, 2019 11:07

Add tests for trailing trivia to JsonSerializer

64da14f

Contributes to #37500

Consume trailing trivia in JsonSerializer.ReadCore

e2eb92c

Fixes #37500

ahsonkhan added the area-System.Text.Json label May 10, 2019

ahsonkhan added this to the 3.0 milestone May 10, 2019

ahsonkhan requested review from JeremyKuhne and steveharter May 10, 2019 00:31

ahsonkhan reviewed May 10, 2019

View reviewed changes

Comment thread src/System.Text.Json/tests/Serialization/Object.ReadTests.cs Outdated

ahsonkhan reviewed May 10, 2019

View reviewed changes

watfordgnf added 3 commits May 10, 2019 09:36

Add tests for leading trivia too

66a0e5d

- Consolidates leading and trailing trivia tests into one

Simplify ReadCore loop per @ahsonkhan review

cd324b7

NOTE: throws NPE in state.PropertyPath if invalid JSON is in the trailer.

Do not compute PropertyPath if outside document

a6da3d4

ahsonkhan approved these changes May 10, 2019

View reviewed changes

ahsonkhan reviewed May 10, 2019

View reviewed changes

watfordgnf added 2 commits May 13, 2019 10:10

Add additional test cases for primitives

bbb432a

Clean up tests and add negative test for leading/trailing comments

f3b4513

ahsonkhan reviewed May 13, 2019

View reviewed changes

ahsonkhan approved these changes May 14, 2019

View reviewed changes

ahsonkhan merged commit 2b8126e into dotnet:master May 14, 2019

watfordgnf deleted the bug-37500-jsonserializer-throws-trailing-whitespace branch May 14, 2019 15:18

watfordgnf mentioned this pull request May 16, 2019

Add interleaved comments to JsonSerializer tests #37712

Merged

ahsonkhan mentioned this pull request Feb 1, 2020

JsonSerializerOptions doesn't work well with JsonCommentHandling.Allow and JsonDocument dotnet/runtime#29543

Closed

		@@ -33,7 +33,7 @@ private static void ReadCore(

		if (HandleValue(tokenType, options, ref reader, ref state))

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

Conversation

watfordgnf commented May 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dnfclas commented May 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ahsonkhan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahsonkhan May 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahsonkhan May 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahsonkhan May 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahsonkhan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahsonkhan May 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahsonkhan May 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahsonkhan May 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JamesNK May 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahsonkhan May 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

watfordgnf commented May 9, 2019 •

edited

Loading

dnfclas commented May 9, 2019 •

edited

Loading

ahsonkhan May 10, 2019 •

edited

Loading

ahsonkhan May 14, 2019 •

edited

Loading

ahsonkhan May 10, 2019 •

edited

Loading

ahsonkhan May 10, 2019 •

edited

Loading

ahsonkhan May 10, 2019 •

edited

Loading

ahsonkhan May 10, 2019 •

edited

Loading

JamesNK May 11, 2019 •

edited

Loading

ahsonkhan May 13, 2019 •

edited

Loading