jsonb-api icon indicating copy to clipboard operation
jsonb-api copied to clipboard

Apply adapter introduced with annotations for related types

Open oliviercailloux opened this issue 4 years ago • 10 comments

Currently, when annotating a field of type F with @JsonbTypeAdapter(MyAdapter.class), the adapter must have F as original parameterized type (or compatible with F). But it would be very useful to be able to use adapters more generally on common collection types. Example: annotate a field of type List<F> to make it use an adapter <F, JsonObject>. Currently, if I understand correctly, I need to create an adapter from List<F> to JsonObject to adapt such a field, which reduces significantly the power of Jsonb.

oliviercailloux avatar Jun 02 '20 08:06 oliviercailloux

Example: annotate a field of type List<F> to make it use an adapter <F, JsonObject>. Currently, if I understand correctly, I need to create an adapter from List<F> to JsonObject to adapt such a field, which reduces significantly the power of Jsonb.

Instead of creating an Adapter<List<F>, JsonObject> and annotating the field, you can create an Adapter<F, JsonObject> and register it globally. This adapter will be used when serializing values of type F, independent of whether they occur inside a list, map, optional, etc. Does this give you the flexibility you need?

martijndwars avatar Jun 02 '20 10:06 martijndwars

Indeed a suitable workaround, which I currently use. But this request is about using an annotation to avoid having to register adapters globally, which can be impossible (in case two fields with same type in an object require different adapters), which I find inelegant (soon you lose track of which object exactly requires which adapters and what could break if you remove or change one), and which has other drawbacks (such as the users of my library having to think about registering the right adapters before converting).

Basically the lack of lifting (by which I mean automatic conversion of an adapter of F to an adapter of List<F>, say) when using the annotations defeats the purpose of the JsonbTypeAdapter annotation, which as I understand is there precisely to permit using an annotation exactly at the right place (the field or method that requires it); and is unexpected, given that the adapters used in a JsonbConfig are (quite conveniently) lifted automagically.

oliviercailloux avatar Jun 02 '20 12:06 oliviercailloux

Relatedly (not sure it should be the same issue?), I’d love to be able to specify (by annotation) mutliple adapters on a field, and that these adapters be all used as if they were registered before conversion, only for the conversion related to that field. This would be useful when some field has a type that I can’t change.

oliviercailloux avatar Jun 02 '20 12:06 oliviercailloux

hi @oliviercailloux, does this issue fully capture what you are asking for? https://github.com/eclipse-ee4j/jsonb-api/issues/66

It would allow usage such as:

List<@JsonbTypeAdapter(MyAdapter.class) F> myField;

aguibert avatar Jun 02 '20 14:06 aguibert

Guess we can also just enable to mark it on the field (method) instead of the inside type:

@JsonbTypeAdapter(MyAdapterForF.class) List<F> myField;

here the impl just check if the adapter matches the container or the contained type (this is what we do in johnzon since some years). I don't know if it is habit but it is simpler to read in general, avoids weird formatting and enable to stay compatible with java 8 libs (often < java 8 as base) which is still > 50% of current apps.

rmannibucau avatar Jun 02 '20 14:06 rmannibucau

JSON-B has Java 8 as a baseline prereq so I'm not concerned about avoiding Java 8 features.

I agree the field-annotated approach is a bit more readable, but would this approach still hold up when multiple generic types are present? For example:

Map<A,B> aMap;

This would be clear with the original proposal because the generic type is directly annotated:

Map<@JsonbTypeAdapter(AdapterA.class) A, @JsonbTypeAdapter(AdapterB.class) B> aMap;

With field annotations we'd need to make @JsonbTypeAdapter a repeatable annotation which seems odd (also a Java 8 feature btw) and it's less clear what the annotation applies to for multiple generic types:

@JsonbTypeAdapter(AdapterA.class)
@JsonbTypeAdapter(AdapterB.class)
Map<A, B> aMap;

aguibert avatar Jun 02 '20 14:06 aguibert

@aguibert you can also say the opposite:

@JsonbTypeAdapter(AdapterA.class)
Map<A, ? extends A> aMap;

Will be way more concise.

That said I didn't want to avoid j8 feature, more to propose to support this simple matching too since it stays more natural (on that bval shows us it kind of fails to only use this notation IMHO).

rmannibucau avatar Jun 02 '20 14:06 rmannibucau

I was thinking about something like the following.

@JsonbTypeAdapters({AdapterA.class, AdapterB.class})
Map<A, B> aMap;

or even the following, where the type B uses internally some type C that json needs to convert to or from:

@JsonbTypeAdapters({AdapterA.class, AdapterC.class})
Map<A, B> aMap;

In this last example, jsonb would behave as if passing instances of AdapterA and AdapterC to its configuration, that it, it would find out when AdapterC should be used and use it automatically as required.

oliviercailloux avatar Jun 02 '20 15:06 oliviercailloux

I don't see that example as a counter-example, you could simply do:

Map<@JsonbTypeAdapter(AdapterA.class) A, @JsonbTypeAdapter(AdapterA.class) ? extends A> aMap;

Obviously I'm in favor of more concise approaches, but I think we need to weight that against the amount we are trying to guess user intent. With the approach the user would need to mentally be aware of the following resolution rules:

  1. First @JsonbTypeAdapter applies to the field type
  2. If the adapter is not assignable to the field type, if it has generic type elements, the adapter applies to all (?) applicable generic type elements
  3. If multiple @JsonbTypeAdapter elements are present on the field (assuming we make the anno repeatable) and multiple adpaters match to the same element, then an error is raised.

We should also keep in mind that this is seeming more and more like a corner case. Where the same type needs different adapters at different places. If JsonbConfig seems clunky, you can also apply @JsonbTypeAdapter directly to the class in question btw:

@JsonbTypeAdapter(AdapterA.class)
public class A { ... }

aguibert avatar Jun 02 '20 15:06 aguibert

@aguibert rules is simpler: create a set of all types of the field (container, contained, potentially contained of contained) and apply the adapter to all matching types (wildcard not being supported indeed). If ambiguous and it can't be refined (if A extends B and you have A and B adapters then you take A one) then it fails.

That said I can join you on the fact it is likely saner to put it on the class - and likely reuse customizations there for cases where the model is immutable.

rmannibucau avatar Jun 02 '20 15:06 rmannibucau