Support of References for AVRO Schemas

[libretto]

This change adds support for references for AVRO schemas

We aim to add support for references for AVRO schemas to Karapace using a straightforward approach by leveraging the same API used in Protobuf references. Although Avro does not natively support references, we can implement this functionality by utilizing the property of Avro schemas that allows combining types. We utilize Names from avro.name and make_avsc_object to create an Avro schema that includes referenced schemas within as explained in #987.

[jjaakola-aiven]

Cleanup commits to single/coherent commits. Add tests for better coverage.

[libretto]

Cleanup commits to single/coherent commits. Add tests for better coverage.

@jjaakola-aiven Could you suggest the tests that should be added?

[libretto]

@jjaakola-aiven I'm still waiting for your suggestions regarding additional tests for the new functionality. Please keep in mind that we already have integration tests covering various aspects, including:

Basic Avro references
Avro references compatibility
Avro union references
Avro incompatible names in references

Additionally, the new code has successfully passed all existing tests related to Avro functionality. I also added a few unit tests for the AvroMerge class today.

[nosahama]

@libretto shall we rebase this work so we get upstream changes and then let's discuss around the comments if you can tell me when you'd like to, we can spare sometime and discuss in one thread here.

[nosahama]

@libretto The unit test for AvroMerge is gone and it seems that you have reverted some of your changes with your recent push.

[libretto]

@nosahama

@libretto The unit test for AvroMerge is gone and it seems that you have reverted some of your changes with your recent push.

Yes it was an issue with rebase I just restored banch, synced it with main and try rebase again...

[libretto]

@libretto shall we rebase this work so we get upstream changes and then let's discuss around the comments if you can tell me when you'd like to, we can spare sometime and discuss in one thread here.

rebased

[davide-armand]

@libretto Hi, sorry for the delay, this took some time to properly review.

I think that the implementation using Avro unions is risky and might not work in all cases (hard to tell).
A more "standard" solution using the Avro Python library instead of unions seems a better choice.

I have opened a draft PR implementing the merging using Avro lib here:
#987

Please have a look and consider that approach instead.

[libretto]

@libretto Hi, sorry for the delay, this took some time to properly review.

I think that the implementation using Avro unions is risky and might not work in all cases (hard to tell). A more "standard" solution using the Avro Python library instead of unions seems a better choice.

I have opened a draft PR implementing the merging using Avro lib here: #987

Please have a look and consider that approach instead.

@davide-armand Hi,

As I understand, the union approach concept has been confirmed by our teams and has passed all existing Karapace tests, including the references tests I provided. This demonstrates that the concept works well. I will try using Your approach with the Avro library to eliminate the need for the union method. I expect the results to be consistent.

Regarding tests, I noticed that You added a few tests for recursion. However, the direct recursion test You included is on the Avro-level parser and should be tested within the Avro tests, outside of the Karapace project. Indirect recursion (where Schema A references Schema B, which in turn references Schema A) is not possible in Karapace's design, as all schemas are added simultaneously. A schema cannot be added if any of its references are unresolved, which effectively prevents indirect recursion between schemas.

I will include the Your code for the test_avro_reference() test in the my branch tests.

[libretto]

@davide-armand I utilized the Avro library functionality as you suggested. References now work in the native Avro way.

[libretto]

#987 implemented there

[davide-armand]

@libretto Hi, I left a few comments, please have a look

[davide-armand]

resolve() and build() can be merged in a single method, since builder() is only called by resolve().
Maybe the whole AvroResolver class can be just a static method?

[libretto]

fixed

[libretto]

@davide-armand After this change, we have a pylint issue but all tests are passed. Please check.

[davide-armand]

@libretto Do you mean the error No overload variant of "reversed"?
It can be fixed by passing a list to reversed

[cjrolo]

Hello,
I would like to know if it is possible to have this re-reviewed? It seems that feedback was incorported into the lastest changes.

Thanks,

Carlos

[davide-armand]

validate_avro_enum_symbols and validate_avro_names should be passed to make_avsc_object().
I missed that in my PR

[davide-armand]

Please don't use reference to real companies (com.netapp).
Applies to other fixtures as well.

[cjrolo]

Thanks! Will look into this!

[cjrolo]

Fixed accordingly to the review. Hope I didn't miss anything, let me know!

Thanks for the help!