Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added MaxDepth property to JsonLoadSettings to override the maxDepth … #2904

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tradercentric
Copy link

…of 64 levels limit in JsonReader when the reader is called by JObject.Parse, or JArray.Parse.

…of 64 levels limit in JsonReader when the reader is called by JObject.Parse, or JArray.Parse.
@tradercentric
Copy link
Author

tradercentric commented Sep 21, 2023

Hi, this is a fix for Issue #2900

Co-authored-by: Ivan Maximov <sungam3r@yandex.ru>
@tradercentric
Copy link
Author

I really need the issue fixed soon because I do have an Avro schema from the source team who cannot reduce the depth level the document from a vendor. I am open to alternative if my fix is not good enough. Please help.

@sungam3r
Copy link

@tradercentric if you in a hurry I really advise you to fork and compile your version. You may wait for applying/releasing for ages.

@JamesNK
Copy link
Owner

JamesNK commented Sep 24, 2023

I don't think this feature is valuable. There is already a JObject.Load(JsonReader reader, JsonLoadSettings? settings) method that lets you customize the reader.

Having a setting on JsonLoadSettings for max depth creates confusion about which is used. And then the question comes up whether all the other settings on JsonReader should be added to JsonLoadSettings.

@tradercentric
Copy link
Author

Thanks for the response. I am an user of the Apache Avro project which is using Newtonsoft's library, I will fork the Apache Avro project and see if I can instantiate JsonReader first before (to customize) before using JObject.Parse and JArray.Parse. That is to resolve the issue in https://github.com/apache/avro/blob/41b3c08ca5da192786c2b08546e691b3126e1856/lang/csharp/src/apache/main/Schema/Schema.cs#L250.

@tradercentric
Copy link
Author

tradercentric commented Sep 25, 2023

JsonReader.Load seems to count differently the depth levels of Avro schema, the following Avro schema has depth level 4. However in JsonReader's Push(JsonContainerType value), when I put a break point on the line "if (_maxDepth != null && Depth + 1 > _maxDepth && !_hasExceededMaxDepth)", it counted 11.

Here are the observation of over-counting depth level in Newtonsoft's JsonReader:
Avro Schema Depth, JsonReader Depth Level Count
4, 11
16, 44
32, 92
64, 188
So, roughly speaking, the depth level count is about 2.75 times of Avro schema depth.

Here is the sample Avro schema for the test:

var json = @"{
""type"": ""record"",
""name"": ""Level1"",
""fields"": [
{
""name"": ""field1"",
""type"": ""string""
},
{
""name"": ""field2"",
""type"": ""int""
},
{
""name"": ""level2"",
""type"": {
""type"": ""record"",
""name"": ""Level2"",
""fields"": [
{
""name"": ""field3"",
""type"": ""boolean""
},
{
""name"": ""field4"",
""type"": ""double""
},
{
""name"": ""level3"",
""type"": {
""type"": ""record"",
""name"": ""Level3"",
""fields"": [
{
""name"": ""field5"",
""type"": ""string""
},
{
""name"": ""field6"",
""type"": ""int""
},
{
""name"": ""level4"",
""type"": {
""type"": ""record"",
""name"": ""Level4"",
""fields"": [
{
""name"": ""field7"",
""type"": ""boolean""
},
{
""name"": ""field8"",
""type"": ""double""
}
]
}
}
]
}
}
]
}
}
]
}";

I also tested a 32 depth level Avro schema, and the Push method internal depth level count is 92. Here is the 32 level depth Avro schema:

{
""type"": ""record"",
""name"": ""Level1"",
""fields"": [
{
""name"": ""level2"",
""type"": {
""type"": ""record"",
""name"": ""Level2"",
""fields"": [
{
""name"": ""level3"",
""type"": {
""type"": ""record"",
""name"": ""Level3"",
""fields"": [
{
""name"": ""level4"",
""type"": {
""type"": ""record"",
""name"": ""Level4"",
""fields"": [
{
""name"": ""level5"",
""type"": {
""type"": ""record"",
""name"": ""Level5"",
""fields"": [
{
""name"": ""level6"",
""type"": {
""type"": ""record"",
""name"": ""Level6"",
""fields"": [
{
""name"": ""level7"",
""type"": {
""type"": ""record"",
""name"": ""Level7"",
""fields"": [
{
""name"": ""level8"",
""type"": {
""type"": ""record"",
""name"": ""Level8"",
""fields"": [
{
""name"": ""level9"",
""type"": {
""type"": ""record"",
""name"": ""Level9"",
""fields"": [
{
""name"": ""level10"",
""type"": {
""type"": ""record"",
""name"": ""Level10"",
""fields"": [
{
""name"": ""level11"",
""type"": {
""type"": ""record"",
""name"": ""Level11"",
""fields"": [
{
""name"": ""level12"",
""type"": {
""type"": ""record"",
""name"": ""Level12"",
""fields"": [
{
""name"": ""level13"",
""type"": {
""type"": ""record"",
""name"": ""Level13"",
""fields"": [
{
""name"": ""level14"",
""type"": {
""type"": ""record"",
""name"": ""Level14"",
""fields"": [
{
""name"": ""level15"",
""type"": {
""type"": ""record"",
""name"": ""Level15"",
""fields"": [
{
""name"": ""level16"",
""type"": {
""type"": ""record"",
""name"": ""Level16"",
""fields"": [
{
""name"": ""level17"",
""type"": {
""type"": ""record"",
""name"": ""Level17"",
""fields"": [
{
""name"": ""level18"",
""type"": {
""type"": ""record"",
""name"": ""Level18"",
""fields"": [
{
""name"": ""level19"",
""type"": {
""type"": ""record"",
""name"": ""Level19"",
""fields"": [
{
""name"": ""level20"",
""type"": {
""type"": ""record"",
""name"": ""Level20"",
""fields"": [
{
""name"": ""level21"",
""type"": {
""type"": ""record"",
""name"": ""Level21"",
""fields"": [
{
""name"": ""level22"",
""type"": {
""type"": ""record"",
""name"": ""Level22"",
""fields"": [
{
""name"": ""level23"",
""type"": {
""type"": ""record"",
""name"": ""Level23"",
""fields"": [
{
""name"": ""level24"",
""type"": {
""type"": ""record"",
""name"": ""Level24"",
""fields"": [
{
""name"": ""level25"",
""type"": {
""type"": ""record"",
""name"": ""Level25"",
""fields"": [
{
""name"": ""level26"",
""type"": {
""type"": ""record"",
""name"": ""Level26"",
""fields"": [
{
""name"": ""level27"",
""type"": {
""type"": ""record"",
""name"": ""Level27"",
""fields"": [
{
""name"": ""level28"",
""type"": {
""type"": ""record"",
""name"": ""Level28"",
""fields"": [
{
""name"": ""level29"",
""type"": {
""type"": ""record"",
""name"": ""Level29"",
""fields"": [
{
""name"": ""level30"",
""type"": {
""type"": ""record"",
""name"": ""Level30"",
""fields"": [
{
""name"": ""level31"",
""type"": {
""type"": ""record"",
""name"": ""Level31"",
""fields"": [
{
""name"": ""level32"",
""type"": ""string""
}
]
}
}
]
}
}
]
}
}
]
}
}
]
}
}
]
}
}
]
}
}
]
}
}
]
}
}
]
}
}
]
}
}
]
}
}
]
}
}
]
}
}
]
}
}
]
}
}
]
}
}
]
}
}
]
}
}
]
}
}
]
}
}
]
}
}
]
}
}
]
}
}
]
}
}
]
}
}
]
}
}
]
}
}
]
}
}
]
}
}
]
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants