Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

Add support for trailing commas within Utf8JsonReader #36690

Merged
merged 4 commits into from
Apr 8, 2019
Merged

Add support for trailing commas within Utf8JsonReader #36690

merged 4 commits into from
Apr 8, 2019

Conversation

ahsonkhan
Copy link
Member

@ahsonkhan ahsonkhan commented Apr 8, 2019 •
edited
Loading

Fixes https://github.com/dotnet/corefx/issues/34794.

Note that: "[,]" is still considered invalid (just like "{,}"). Json.NET allows "[,]", specifically, but that did not seem consistent to me.

Please review the test cases, specifically, for expected behavior:

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

Lines 2576 to 2684 in 76a2a65

public static IEnumerable<object[]> JsonWithValidTrailingCommas
{
get
{
return new List<object[]>
{
new object[] {"{\"name\": \"value\",}"},
new object[] {"{\"name\": [],}"},
new object[] {"{\"name\": 1,}"},
new object[] {"{\"name\": true,}"},
new object[] {"{\"name\": false,}"},
new object[] {"{\"name\": null,}"},
new object[] {"{\"name\": [{},],}"},
new object[] {"{\"first\" : \"value\", \"name\": [{},], \"last\":2 ,}"},
new object[] {"{\"prop\":{\"name\": 1,\"last\":2,},}"},
new object[] {"{\"prop\":[1,2,],}"},
new object[] {"[\"value\",]"},
new object[] {"[1,]"},
new object[] {"[true,]"},
new object[] {"[false,]"},
new object[] {"[null,]"},
new object[] {"[{},]"},
new object[] {"[{\"name\": [],},]"},
new object[] {"[1, {\"name\": [],},2 , ]"},
new object[] {"[[1,2,],]"},
new object[] {"[{\"name\": 1,\"last\":2,},]"},
};
}
}
public static IEnumerable<object[]> JsonWithValidTrailingCommasAndComments
{
get
{
return new List<object[]>
{
new object[] {"{\"name\": \"value\"/*comment*/,/*comment*/}"},
new object[] {"{\"name\": []/*comment*/,/*comment*/}"},
new object[] {"{\"name\": 1/*comment*/,/*comment*/}"},
new object[] {"{\"name\": true/*comment*/,/*comment*/}"},
new object[] {"{\"name\": false/*comment*/,/*comment*/}"},
new object[] {"{\"name\": null/*comment*/,/*comment*/}"},
new object[] {"{\"name\": [{},]/*comment*/,/*comment*/}"},
new object[] {"{\"first\" : \"value\", \"name\": [{},], \"last\":2 /*comment*/,/*comment*/}"},
new object[] {"{\"prop\":{\"name\": 1,\"last\":2,}/*comment*/,}"},
new object[] {"{\"prop\":[1,2,]/*comment*/,}"},
new object[] {"{\"prop\":1,/*comment*/}"},
new object[] {"[\"value\"/*comment*/,/*comment*/]"},
new object[] {"[1/*comment*/,/*comment*/]"},
new object[] {"[true/*comment*/,/*comment*/]"},
new object[] {"[false/*comment*/,/*comment*/]"},
new object[] {"[null/*comment*/,/*comment*/]"},
new object[] {"[{}/*comment*/,/*comment*/]"},
new object[] {"[{\"name\": [],}/*comment*/,/*comment*/]"},
new object[] {"[1, {\"name\": [],},2 /*comment*/,/*comment*/ ]"},
new object[] {"[[1,2,]/*comment*/,]"},
new object[] {"[{\"name\": 1,\"last\":2,}/*comment*/,]"},
new object[] {"[1,/*comment*/]"},
};
}
}
public static IEnumerable<object[]> JsonWithInvalidTrailingCommas
{
get
{
return new List<object[]>
{
new object[] {","},
new object[] {" , "},
new object[] {"{},"},
new object[] {"[],"},
new object[] {"1,"},
new object[] {"true,"},
new object[] {"false,"},
new object[] {"null,"},
new object[] {"{,}"},
new object[] {"{\"name\": 1,,}"},
new object[] {"{\"name\": 1,,\"last\":2,}"},
new object[] {"[,]"},
new object[] {"[1,,]"},
new object[] {"[1,,2,]"},
};
}
}
public static IEnumerable<object[]> JsonWithInvalidTrailingCommasAndComments
{
get
{
return new List<object[]>
{
new object[] {"/*comment*/ ,/*comment*/"},
new object[] {" /*comment*/ , /*comment*/ "},
new object[] {"{}/*comment*/,/*comment*/"},
new object[] {"[]/*comment*/,/*comment*/"},
new object[] {"1/*comment*/,/*comment*/"},
new object[] {"true/*comment*/,/*comment*/"},
new object[] {"false/*comment*/,/*comment*/"},
new object[] {"null/*comment*/,/*comment*/"},
new object[] {"{/*comment*/,/*comment*/}"},
new object[] {"{\"name\": 1/*comment*/,/*comment*/,/*comment*/}"},
new object[] {"{\"name\": 1,/*comment*/,\"last\":2,}"},
new object[] {"[/*comment*/,/*comment*/]"},
new object[] {"[1/*comment*/,/*comment*/,/*comment*/]"},
new object[] {"[1,/*comment*/,2,]"},
};
}
}

Only trailing commas within an object/array are allowed.

cc @JamesNK, @bartonjs

@ahsonkhan ahsonkhan added this to the 3.0 milestone Apr 8, 2019
@ahsonkhan
Copy link
Member Author

cc @steveharter, if this option needs to be used/exposed by the deserializer.

@ahsonkhan
Copy link
Member Author

ahsonkhan commented Apr 8, 2019 •
edited
Loading

The System.Drawing.Common.Tests crashes is a known, unrelated, issue: https://github.com/dotnet/corefx/issues/36683

@JamesNK
Copy link
Member

JamesNK commented Apr 8, 2019 •
edited
Loading

JSON.parse("[,]") is valid in JavaScript. The end result is 1 length array containing an undefined. But I don't think you need to support that.

@ahsonkhan
Copy link
Member Author

ahsonkhan commented Apr 8, 2019 •
edited
Loading

Since only the drawing tests are failing in CI (https://mc.dot.net/#/user/dotnet-bot/pr~2Fdotnet~2Fcorefx~2Frefs~2Fpull~2F36690~2Fmerge/test~2Ffunctional~2Fcli~2F/20190408.1/workItem/System.Drawing.Common.Tests), I am going to merge this in so up-stack consumers can leverage this option (see #36684). Please still feel free to review and I will address the feedback separately, especially if it's critical.

@ahsonkhan ahsonkhan merged commit 87fdc75 into dotnet:master Apr 8, 2019
@ahsonkhan ahsonkhan deleted the AddTrailingCommas branch April 8, 2019 09:55
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
@ahsonkhan
Add support for trailing commas within Utf8JsonReader (dotnet/corefx…
fed1a65 
…#36690)

* Add support for trailing commas with single-segment tests.

* Implement single-segment case and update tests.

* Update multi-segment path and add tests.

* Add more tests, add to state, and update rollback logic.


Commit migrated from dotnet/corefx@87fdc75
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
area-System.Text.Json
Projects
None yet
Milestone
3.0
Development

Successfully merging this pull request may close these issues.
2 participants
@ahsonkhan @JamesNK