JsonSerializer throws with trailing whitespace or comments

[watfordgnf]

Passing JSON with trailing whitespace causes JsonSerializer.Parse to throw (dotnet core 3 preview 5):

{"server_id":"Nf5YR9YRKvA4Od1ofBkOxD","version":"1.0.4","go":"go1.8.3","host":"0.0.0.0","port":4222,"auth_required":false,"ssl_required":false,"tls_required":false,"tls_verify":false,"max_payload":1048576} 

(note single trailing space)

// string json = ... (or ReadOnlySpan)
var info = JsonSerializer.Parse<Info>(json);

// Exception thrown: 'System.Text.Json.JsonReaderException' in System.Text.Json.dll
// "The provided data of length 206 has remaining bytes 1."
//   at System.Text.Json.Serialization.JsonSerializer.ParseCore(String json, Type returnType, JsonSerializerOptions options)
//   at System.Text.Json.Serialization.JsonSerializer.Parse[TValue](String json, JsonSerializerOptions options)

[ahsonkhan]

cc @steveharter

[watfordgnf]

The following test also throws:

        [Fact]
        public static void ReadClassWithTrailingComments()
        {
            var options = new JsonSerializerOptions();
            options.ReadCommentHandling = JsonCommentHandling.Skip;

            ClassWithComplexObjects obj = JsonSerializer.Parse<ClassWithComplexObjects>(ClassWithComplexObjects.s_json + "/* Comment */", options);
            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<ClassWithComplexObjects>(reserialized);
            obj.Verify();
        }

[ahsonkhan]

The reader does support reading trailing white space/comments. Maybe the deserializer should keep reading until the reader returns false on .Read().

Otherwise, a check like this might throw prematurely:
https://github.com/dotnet/corefx/blob/f6762c0e37641bf31b9957dc51e74089bac5a310/src/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.String.cs#L76-L81

[watfordgnf]

It does appear to loop on reader.Read() until it is false within JsonDocument.TryParseValue. It seems to take subsequences that never include the trailing trivia (reader.BytesConsumed does not include the trivia).

[watfordgnf]

Ok, it does appear to not completely call Read after the final EndObject is encountered (lastCall == true). It looks like ReadCore should probably include a means to drain the reader.

[ahsonkhan]

JsonDocument.TryParseValue

Out of curiosity, why is the JsonDocument implementation relevant to the JsonSerializer.Parse issue you are seeing? Or was your intention just to provide additional context around how different APIs use the reader loop?

The relevant snippet is in ReadCore which returns early:
https://github.com/dotnet/corefx/blob/f6762c0e37641bf31b9957dc51e74089bac5a310/src/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.cs#L85

Edit:

It looks like ReadCore should probably include a means to drain the reader.

Precisely :)

[watfordgnf]

I had added trivia draining to Utf8JsonReader and was seeing it being called from JsonDocument. A red-herring as you noted.

[watfordgnf]

Alright, with trivia draining in the correct place these two cases work. I'm running the full test suite to see if this impacts any other areas.