[AVRO] #589: Fix schema not including base class for records with subclasses

[MichalFoksa]

Supported cases:

1. Concrete class annotated with @JsonSubTypes

When a concrete (non-abstract) class is annotated with @JsonSubTypes, then Avro type of the annotated class is part of resulting union.

 @JsonSubTypes({
        @JsonSubTypes.Type(value = Apple.class),
        @JsonSubTypes.Type(value = Pear.class),
 })
class Fruit { }

class Apple extends Fruit {}
class Pear extends Fruit {}

Fruit schema:

[
  {name: Fruit, type: record, fields: [...]},
  {name: Apple, type: record, fields: [...]},
  {name: Orange, type: record, fields: [...]}
]

Annotated class, Fruit, is part of the schema:

This was not supported case, annotated class was missing in union.

2. Abstract class annotated with @JsonSubTypes

When an abstract class is annotated with @JsonSubTypes, then abstract class is not in union.

 @JsonSubTypes({
        @JsonSubTypes.Type(value = Apple.class),
        @JsonSubTypes.Type(value = Pear.class),
 })
class AbstractFruit { }          // <----  abstract class

class Apple extends Fruit {}
class Pear extends Fruit {}

AbstractFruit schema:

[
  {name: Apple, type: record, fields: [...]},
  {name: Orange, type: record, fields: [...]}
]

Annotated abstract base class is not part of the schema:

This was supported case.

3. Abstract class somewhere in the middle of @JsonSubTypes hierarchy

When an abstract class is somewhere along @JsonSubTypes hierarchy, it does not end up in union.

@JsonSubTypes({
        @JsonSubTypes.Type(value = AbstractWaterVehicle.class),
})
class Vehicle {
}

@JsonSubTypes({
        @JsonSubTypes.Type(value = Boat.class),
})
abstract class AbstractWaterVehicle extends Vehicle { }   // <---- abstract class
class Boat extends AbstractWaterVehicle { }

Vehicle schema:

[
  {name: Vehicle, type: record, fields: [...]},
  {name: Boat, type: record, fields: [...]},
]

AbstractWaterVehicle class is not in union.

This was not supported case, AbstractWaterVehicle would be part of union

4. Class referenced twice in @JsonSubTypes hierarchy

When a class is referenced twice in @JsonSubTypes hierarchy, it occurs only once in union.

// Helium is twice in subtypes hierarchy, once as `ElementInterface` subtype and second time as subtype of `AbstractGas` subtype.
@JsonSubTypes({
        @JsonSubTypes.Type(value = Gas.class),
        @JsonSubTypes.Type(value = Helium.class),       // <---- 1. Helium 
})
interface ElementInterface { }

@JsonSubTypes({
        @JsonSubTypes.Type(value = Helium.class),       // <---- 2. Helium
        @JsonSubTypes.Type(value = Oxygen.class),
})
static class Gas implements ElementInterface { }

class Helium extends Gas { }
class Oxygen extends Gas { }

ElementInterface schema:

[
  {name: Gas, type: record, fields: [...]},
  {name: Helium, type: record, fields: [...]},
  {name: Oxygen, type: record, fields: [...]}
]

Helium class is not duplicated in union.

5. Base class explicitly in @JsonSubTypes or @union annotations

When a class is subclass of itself.

 @JsonSubTypes({
        @JsonSubTypes.Type(value = Fruit.class),   // <----  Fruit is annotated class and also in @JsonSubTypes
        @JsonSubTypes.Type(value = Apple.class),
        @JsonSubTypes.Type(value = Pear.class),
 })
class Fruit { } // or interface

class Apple extends Fruit {}
class Pear extends Fruit {}

 @Uion({
        Fruit.class,                              // <----  Fruit is annotated class and also in @Union
        Apple.class, Pear.class,
 })
class Fruit { }  // or interface

class Apple extends Fruit {}
class Pear extends Fruit {}

Both cases above would lead to endless loop and StackOverflowError.

---

See PolymorphicTypeAnnotationsTest.class.

[cowtowncoder]

Sounds good. Was about to suggest that due to RecordVisitor changes (addition of a Field) should go against 2.x not 2.19 but since it's private field maybe it can be considered internal (non-API) change.

[MichalFoksa]

I have problem to choose most obvious handling of duplicates in unionSchemas (inside if (subTypes != null && !subTypes.isEmpty()) condition).

1. when I use HashSet then _typeSchema must be added into union last.

2. when I use ArrayList, then I have to call unionSchemas .contains(subTypeSchema) before adding subTypeSchema into unionSchemas. However ArrayList.contains() is only object reference-equality check.

3. If reference-equality check is enough for unionSchemas , then IdentityHashMap can be used to create a Set (Set<Schema> unionSchemas = Collections.newSetFromMap(new IdentityHashMap<>()) ) and we have simple code.

[cowtowncoder]

I have problem to choose most obvious handling of duplicates in unionSchemas (inside if (subTypes != null && !subTypes.isEmpty()) condition).

1. when I use HashSet then `_typeSchema` must be added into union last.

2. when I use ArrayList, then I have to call `unionSchemas .contains(subTypeSchema)` before adding subTypeSchema into `unionSchemas`. However  ArrayList.contains() is only object reference-equality check.

3. If reference-equality check is enough for `unionSchemas` , then `IdentityHashMap` can be used to create a Set (`Set<Schema> unionSchemas = Collections.newSetFromMap(new IdentityHashMap<>())` ) and we have simple code.

Would it make sense to first use a Map with Java type being resolved (either JavaType, if available, or Class if not) as key; and only after collecting use "values" to create whatever is needed (if it must be Set, then identity; otherwise List if we can use Collection)?

I may be missing/misunderstanding something, so apologies if above makes no sense.

[MichalFoksa]

@cowtowncoder

I am done. Have a look at it PLS.

[cowtowncoder]

We can do that as a follow-up for 2.20 (2.x branch)

[cowtowncoder]

@MichalFoksa Thank you! Had a quick look and things look better. Will try to properly review later tonight, get it merged hopefully.

[cowtowncoder]

But if we have identity-based Set, where the only match is pure identity, why not just use ArrayList instead of Set to add/addAll to? Or do we get literal same Schema instances somehow?

Making that chance will not fail any of added tests at least.

[MichalFoksa]

Or do we get literal same Schema instances somehow?

Yes. It happens when a class is references multiple times in hierarchy (maybe by mistake).

In the example below Helium is subtype of Element and also subtype of Gas.

Where Gas schema is union of

• Gas (itself),
• Helium
• Oxygen.

Element schema is union of all type in

• Gas schema (Gas, Helium and Oxygen)
• Helium

When you collect all types togetjer without identity check you would get invalid union of

• Gas
• Helium
• Oxygen
• Helium again

because Helium is twice.

See use case 4 "Class referenced twice in @JsonSubTypes hierarchy**" above or PolymorphicTypeAnnotationsTest.class_is_referenced_twice_in_hierarchy_test test.

Change unionSchemas to ArrayList and the test will fail with

Failed to generate AvroSchema for ElementInterface, problem: Duplicate in union: Helium

 @JsonSubTypes({
   Type( Gas.class )
   Type( Helium.class ) })                         <---- First Helium occurrence 
 +---------------------+
 |      Element        |
 |    ( interface )    |
 +---------------------+
     ▲              ▲   @JsonSubTypes({
     |               |    Type( Helium.class )     <---- Second Helium occurrence
     |               |    Type( Oxygen.class ) })
     |     +---------------------+
     |     |        Gas          |
     |     +---------------------+
     |              ▲
     |              | 
     |     +--------+--------+
     |     |                 |
+-----------------+  +-----------------+
|     Helium      |  |     Oxygen      |
+-----------------+  +-----------------+

[cowtowncoder]

Ok yes, right, but: I think duplicates are actually caught by alreadySeenClasses -- so I don't think unionSchemas prevent any duplicates. So that one could -- I think? -- be simple ArrayList

[MichalFoksa]

Both duplicate checks are needed, each check is on different level.

[cowtowncoder]

But like I said, changing Set to List did not fail any of the tests. So something is odd.
I think I'll try to do that again just to make sure I did not imagine it.

[MichalFoksa]

:( Hmmm, just tested:

final ArrayList<Schema> unionSchemas = new ArrayList<>();

Fails PolymorphicTypeAnnotationsTest#class_is_referenced_twice_in_hierarchy_test test.

[cowtowncoder]

Ok I must have been hallucinating. Apologies for noise.

[MichalFoksa]

no problem, it happens.

[cowtowncoder]

Merged all the way to 3.x, things working. Phew!

Next PR could be for minor changes for 2.20 (2.x), making fields final.