JsonbSerializer order when using inheritance / polymorphism

[benneq]

I'm testing some Jsonb stuff with Quarkus 1.4.1 and quarkus-resteasy-jsonb, and I came across the following inconsistent behavior:

class Foo { ... }
class Bar extends Foo { ... }

class FooSerializer implements JsonbSerializer<Foo> { ... }
class BarSerializer implements JsonbSerializer<Bar> { ... }

class MyJsonbConfigCustomizer implements JsonbConfigCustomizer {
  public void customize(JsonbConfig config) {
    config.withSerializers(new FooSerializer(), new BarSerializer());
  }
}

@Path("/test")
public class TestResource {
  @GET
  public Bar test() { ... }
}

Now when I hit my /test Resource, the response isn't consistent between application restarts. Sometimes FooSerializer is called, and sometimes BarSerializer.

I was hoping that there's some kind of "most precise type" hierarchy, that automatically favors BarSerializer.

Is this intended? Or is this an error in Quarkus' / Resteasy's implementation?

[rmannibucau]

I think it is undefined today (https://github.com/eclipse-ee4j/jsonb-api/blob/f931164600d712b6067bf8279d7124e8723c9dc4/spec/src/main/asciidoc/jsonb.adoc#user-content-serializersdeserializers) but it sounds natural that exact matching is always preferred over parent matching so you can open an enhancement on your impl bugtracker. Here we should refine the requirement probably.

[benneq]

I'm not yet sure who's responsible for this.
Yasson: https://github.com/eclipse-ee4j/yasson/issues
or RestEasy: https://issues.redhat.com/projects/RESTEASY/issues
?

[rmannibucau]

@benneq or quarkus with its build steps. Maybe report it on quarkus bugtracker, I'm sure you'll find somebody able to help you and potentially redirect you if not the right place?

[aguibert]

Just noticing this issue now, at first glance I'm thinking it is a Yasson issue. (I work on Quarkus and Yasson so it would go to me either way though )

I think we should add some clarification wording to the spec about this. Ideally we can match on the most specific serializer/deserializer possible, but I will have to check and see if that is possible from an implementation first.

[benneq]

I guess the same should be done for JsonbAdapter.

And there are some edge cases:

• What happens if there are multiple serializers for a single class?
• What if there's a serializer and an adapter for the same class?

[rmannibucau]

Think we have multiple options:

1. Say it is undefined and up to vendors (and gather feedback before making it into the spec)
2. Define it this way
   2.a. If multiple serializers match, select the most direct one (class wins over interface, closer class in the hierarchy - in java there is no ambiguity there). If it matches by 2 interfaces then fail.
   2.b. I guess adapters and serializers are kind of transversal so not sure there is any issue there.

Even if it is always better to well define the behavior, I'm not sure it is worth it there since if you use that option you can always remove the issue by customizing the JsonbConfig. The instanceOf behavior is needed for framework subclasses ($$Proxy) but the ambiguity case is less urgent IMO, this is why I think we should focus first on the main case and maybe wait a bit for the case 2.a.

[benneq]

Yeah, I agree.

But maybe one last thing not to forget: It has to check (de)serializers and adapters at the same time. Because you can have an adapter for Foo and a (de)serializer for Bar extends Foo (or the other way round).

[rwperrott]

I suggest:

• An "SDA" container object for serializer, deserializer, and adapter references.
• An IdentityHashMap<Class<?>,SDA> in config, to map added serializers, deserializers, and adapters.
• An Jsonb instance has the builder copy of the base and config IdentityHashMap<Class<?>,SDA> which it gets direct, else iteratively in-depth by interfaces and sub-classes, with any in-depth results cached in the pre-lookup map by the original key class; with misses cached as a singleton SDA, with all null references.
• On a hit I assume that a non-null adapter reference, in the SDA object, should override any non-null serializer and deserializer references.

Using a single map approach with depth result caching, should be a lot faster than a sequential instanceof like approach.

[rmannibucau]

Adapters and serializers are not handled in the same "phase"so it is ok.
About the map: must not be specified since it does not change the api behavior so it stays an implementation detail and i think it shouldnt be specified otherwise we dont have a spec but an impl in a pdf which looses part of its goal IMHO - + identity map must be synchronized which would slow down the runtime.