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]

Is unique_id only used in tests?
Can you do without it?

[davide-armand]

regex does not seem to be used

[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?

[davide-armand]

I have realized now that it is possible to pass a list of schema strings to avro.schema.parse():
https://github.com/aiven/avro/blob/5a82d57f2a650fd87c819a30e433f1abb2c76ca2/lang/py/avro/schema.py#L1192

We currently use parse() to parse a single schema (see parse_avro_schema_definition()).

When called with a list of schemas it returns a UnionSchema (through make_avsc_object()), which itself contains a list of parsed schemas (UnionSchema.schemas).
The constructor of UnionSchema seems to be doing what we want (loop on the schemas, call make_avsc_object() and remember names).

I think passing to parse() the reference schemas + the main schema (the main schema should be passed as last in the list) and then getting the last schema from UnionSchema.schemas should be equivalent to what's the PR now.

If it works then we can delegate that logic to the Avro library