avro icon indicating copy to clipboard operation
avro copied to clipboard

AVRO-3856: [C#] Hardcode MaxDepth property to 192 when using Newtonsoft JsonReader to load Avro schema up to 64 depth levels

Open tradercentric opened this issue 2 years ago • 5 comments

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 https://github.com/JamesNK/Newtonsoft.Json/pull/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

  • Does this pull request introduce a new feature? no
  • If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)

tradercentric avatar Sep 25 '23 05:09 tradercentric

Added comment of a workaround on end user/developer side in https://issues.apache.org/jira/browse/AVRO-3856.

tradercentric avatar Sep 27 '23 05:09 tradercentric

In my opinion, the code changes here are now OK to merge, but AVRO-3856 should either

  • be left open until applications can actually customize the max depth, or
  • be reworded to request only a larger constant max depth rather than the ability to customize.

KalleOlaviNiemitalo avatar Sep 27 '23 17:09 KalleOlaviNiemitalo

Thanks @tradercentric @KalleOlaviNiemitalo I've updated the issue so it corresponds to the fix.

emasab avatar Sep 28 '23 08:09 emasab

I noticed there is no deserialization error in Java Avro library with the same schema. May I ask whose Confluent developers qualified in improving Apache.Avro C# library can lead the effort to allow customization of max depth please?

tradercentric avatar Sep 28 '23 15:09 tradercentric

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?

tradercentric avatar Sep 28 '23 15:09 tradercentric