Runtime: JsonSerializerOptions doesn't work well with JsonCommentHandling.Allow and JsonDocument

[ahsonkhan]

See https://github.com/dotnet/corefx/pull/37549#discussion_r282884005 for context.

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

JsonSerializerOptions should either support JsonCommentHandling.Allow in all cases (by transforming it to JsonCommentHandling.Skip before passing it to the JsonDocument), or just like JsonDocument, throw if the options is set to Allow. This comes up whenever a user has a System.Object property (which is where JsonDocument gets used).

I don't really see much benefit of Allow outside of JsonReader.

cc @steveharter, @bartonjs, @JamesNK, @rynowak, @JeremyKuhne, @watfordgnf

[watfordgnf]

I think it would be helpful if it transformed Allow to Skip, as you might share an options object for consistency.

[watfordgnf]

Interestingly enough it looks like Allow was already intended to be supported:
https://github.com/dotnet/corefx/blob/e12bfba58e3581dc475e804688bef1c66d70226c/src/System.Text.Json/tests/Serialization/OptionsTests.cs#L93-L106

[JamesNK]

IMO JsonDocument should allow JsonCommentHandling.Allow on the JSON reader and just ignore any comments that it comes across while reading JSON.

[watfordgnf]

Simply disabling the check for Allow within JsonDocument doesn't appear to negatively impact the test suite. Every test still passes (besides the one checking that it throws that). Reading through the docs, porting guide, and roadmap I cannot see the rationale for this prohibition.

[ahsonkhan]

IMO JsonDocument should allow JsonCommentHandling.Allow on the JSON reader and just ignore any comments that it comes across while reading JSON.

Reading through the docs, porting guide, and roadmap I cannot see the rationale for this prohibition.

I believe the main intention was to avoid confusion/ambiguity between "Allow" and "Skip". This difference is relevant to the reader but does not make sense on Document/Deserializer. Making "Allow" and "Skip" mean the same thing for other components could make the distinction blurry for where it matters. However, I don't feel strongly about this, and am perfectly fine with having "allow" semantics to mean "skip" for document/deserializer. The only implication here would be that JsonDocument would never be able to support accessing comments in the future, where disallowing "Allow" leaves that door open.

I would be interested in hearing thoughts from @bartonjs and @steveharter, especially if they have differing opinions.

[JamesNK]

The only implication here would be that JsonDocument would never be able to support accessing comments in the future, where disallowing "Allow" leaves that door open.

Comments don't really fit into a JSON document API. JSON document is key/value pairs and arrays.

{
   // comment 1!
   "Hello": /* comment 2 */ true, // comment 3!
   "World": [
       true // comment 4!
   ]
   // comment 5!
}

JToken use to attempt to include comments in the DOM, but the only place they could ever fit was as an item in an array, and it was weird that those comments were included but not comments within a JSON object.

[ahsonkhan]

Simply disabling the check for Allow within JsonDocument doesn't appear to negatively impact the test suite.

I think if we are doing this, we should coerce Allow to Skip (rather than just disabling the check) and add tests to validate that the comments are being "supported" and ignored (i.e. we are not throwing).

Interestingly enough it looks like Allow was already intended to be supported

That is somewhat odd behavior. We should probably return a null object in that case (or whatever we do for an empty payload, maybe throw?).

I think it would be helpful if it transformed Allow to Skip, as you might share an options object for consistency.

When would a user have Allow to begin with and what would prevent them from choosing Skip instead?

[watfordgnf]

When would a user have Allow to begin with and what would prevent them from choosing Skip instead?

If you don't do anything meaningful with Allow, my point is probably moot.

[bartonjs]

JsonDocument blocked it from the "here is some UTF-8 text" and "here's how I want you to read it" because it wanted to be clear that Allow didn't make sense.

When it started taking a reader it preserved the "Allow is unsupported" for clarity; but I did consider making it just skip over comments if it saw them from the reader.

So it's really just a question of where the fuzzy line lives. To me, "Allow" means "I want to know when comments appear, and what they are", and since JsonDocument can't represent them at all, it can't maintain that semantic. I wouldn't consider it a major violation of layering if it documented "a reader in Allow mode is permitted, but comments are skipped without being represented in the resulting document" -- but I (personally) think it should still throw when asked to do Allow mode from the "create a reader and do your own thing" methods.

[watfordgnf]

I have worked out the code changes and tests for either option:

1. Allow is transformed into Skip when passed from JsonSerializer to JsonDocument.
2. JsonSerializer utilizes the same checks as JsonDocument does prior to parsing (to avoid the unexpected behavior of Allow working sometimes but not all the time).

Let me know which is preferred and I'll get a PR up.

[ahsonkhan]

but I (personally) think it should still throw when asked to do Allow mode

Let me know which is preferred and I'll get a PR up.

Unless @JamesNK feel strongly, I would go with option 2 for now. We can always go from 2 to 1 in the future if customers require it.