Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Apply adapter introduced with annotations for related types #243

Open
oliviercailloux opened this issue Jun 2, 2020 · 10 comments
Open

Apply adapter introduced with annotations for related types #243

oliviercailloux opened this issue Jun 2, 2020 · 10 comments
Labels
enhancement New feature or request

Comments

@oliviercailloux
Copy link

oliviercailloux commented Jun 2, 2020

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.

@martijndwars
Copy link

martijndwars commented Jun 2, 2020

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?

@oliviercailloux
Copy link
Author

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
Copy link
Author

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.

@aguibert aguibert added the enhancement New feature or request label Jun 2, 2020
@aguibert
Copy link
Contributor

aguibert commented Jun 2, 2020

hi @oliviercailloux, does this issue fully capture what you are asking for?
#66

It would allow usage such as:

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

@rmannibucau
Copy link

rmannibucau commented Jun 2, 2020

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.

@aguibert
Copy link
Contributor

aguibert commented Jun 2, 2020

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;

@rmannibucau
Copy link

@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).

@oliviercailloux
Copy link
Author

oliviercailloux commented Jun 2, 2020

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.

@aguibert
Copy link
Contributor

aguibert commented Jun 2, 2020

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 { ... }

@rmannibucau
Copy link

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants