-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
AVRO-3856: [C#] Hardcode MaxDepth property to 192 when using Newtonsoft JsonReader to load Avro schema up to 64 depth levels #2519
base: main
Are you sure you want to change the base?
Conversation
…3x by setting JsonReader MaxDepth to 192. Also I took the Newtonsoft author advice to use JObject.Load(JsonReader) and JArray.Load(JsonReader) instead of their Parse method. So I can set the JsonReader MaxDepth.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does Avro.IO.JsonDecoder also need to be changed? I mean, if Avro supports a 64-level record schema, then it should also be possible to take construct an instance of such a record, encode it to JSON, and decode it back.
2. Followed JObject.Parse and JArray.Parse on reading the remaining json for error 3. Added a real world testcase of a schema where parsing failed. The Avro schema is around JsonReader defaulted 64 max depth level
Added comment of a workaround on end user/developer side in https://issues.apache.org/jira/browse/AVRO-3856. |
In my opinion, the code changes here are now OK to merge, but AVRO-3856 should either
|
Thanks @tradercentric @KalleOlaviNiemitalo I've updated the issue so it corresponds to the fix. |
I noticed there is no deserialization error in Java Avro library with the same schema. |
Hi @KalleOlaviNiemitalo, I have reworded the title to reflect the wish to hardcode value of 192 to support parsing of Avro schema up to 64 depth levels. May I ask if this is okay to merge for this tactical fix please? |
What is the purpose of the change
I am an end-user/developer to use Confluent Kafka to consume a Avro message where the number of nested structures is around 20. The producer/vendor cannot reduce the depth level of the Avro schema. Confluent support has assisted me to open an issue with Apache Avro project. The issue is logged in https://issues.apache.org/jira/browse/AVRO-3856. I am in need of code fix so I can continue.
The current code in Schema.cs parsing Avro schema using JObject.Parse(json), JArray.Parse(json) do not have option to override max depth level. On JamesNK/Newtonsoft.Json#2904, Newtonsoft Author advised to use JObject.Load(Jsonreader), so we can set the MaxDepth in JsonReader once instantiated.
In doing so, I also noticed the Avro schema depth level is not counted the same as in JsonReader's. The default depth level limit of 64 in JsonReader can only support around 20 levels of Avro schema.
When putting break point in JsonReader Push method (called by Load method) with various Avro schema depth levels, here is my observation:
Avro Schema Depth | JsonReader Depth Count
4 | 11
16 | 44
32 | 92
64 | 188
To compensate, I had hardcoded JsonReader MaxDepth to 192 to support parsing of Avro Schema with 64 level depth.
Verifying this change
This change added tests and can be verified as follows:
I added 3 test cases in SchemaTests.cs:
Parse16DepthLevelSchemaTest
Parse32DepthLevelSchemaTest
Parse64DepthLevelSchemaTest
Documentation