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

Clarification: What means the word "mappable" for adapters?

Open mkarg opened this issue 5 years ago • 12 comments

Given the following adapter is part of JSON-B configuration:

class MyCustomAdapter implements JsonbAdapter<X, Y> {
    @Override public Y adaptToJson(X obj) throws Exception { ... }
    @Override public X adaptFromJson(Y obj) throws Exception { ... }
}

When Y is String then Johnzon 1.2.1 uses this adapter to deserialize an instance of X. 👍 But when Y is URI then Johnzon 1.2.1 fails with the following exception: Missing a Converter for type class X to convert the JSON String '...' . Please register a custom converter for it. 👎

The JSON-B specification says: "The target type could be string or some mappable java type."

IIUC Team Johnzon then their opinion is that this is not clearly a bug in Johnzon (AFAIK because Johnzon passes TCK), but there has to be a clarification of the question: Which classes are meant with "mappable"?

  • Only String and JsonbObject?
  • Only those classes explicitly mentioned by chapter 3.4 of the JSON-B specification?
  • All classes which are effectively mappable, u. e. even unmappable classes for which a custom adapter is explicitly provided in the configuration (hence, building a chain of adapters)?
  • Something else...?

For providers of applications and custom components the clarification of this question is essential, otherwise it would be impossible to provide portable adapters.

mkarg avatar Oct 07 '19 12:10 mkarg

Hi @mkarg ,

I think we need to distinguish adapters registered on jsonbconfig - where only (json) primitives can be correctly and deterministicly handled - and adapters registered by annotations where a bit more is doable since it is explicit without any lookup need (the lookup being likely ambiguous, undeterministic etc for the JsonbConfig registration case).

Does it make sense?

Romain

rmannibucau avatar Oct 07 '19 12:10 rmannibucau

@rmannibucau From the aspect of an application provider / adapter provider I think it would be beneficial to no have a distinction among how / where adapters are registered. Such a discrimination is not yet mentioned in the spec, and would make it more problematic to understand by beginners in what case which adapter actually is used / not used.

mkarg avatar Oct 07 '19 13:10 mkarg

@mkarg even if I agree on the overall target, it also means adapter are useless if we dont do this distinction because one of both type is never usable :s. Implication is we should deprecate them for 1.1.

rmannibucau avatar Oct 07 '19 13:10 rmannibucau

@mkarg even if I agree on the overall target, it also means adapter are useless if we dont do this distinction because one of both type is never usable :s. Implication is we should deprecate them for 1.1.

I can't follow you. Why are adapters useless?

mkarg avatar Oct 07 '19 13:10 mkarg

@mkarg useless in the ultra generic definition. At the end we just have JsonValue (+ Number + String) -> JAVA adapters, it is not <A, B> in terms of generics. The useless was a language shortcut (mea culp on this one) to say that since they can trivially be replaced by (de)serializers, we can either refine the generics or just deprecate them and let them undefined.

rmannibucau avatar Oct 07 '19 13:10 rmannibucau

@mkarg useless in the ultra generic definition. At the end we just have JsonValue (+ Number + String) -> JAVA adapters, it is not <A, B> in terms of generics. The useless was a language shortcut (mea culp on this one) to say that since they can trivially be replaced by (de)serializers, we can either refine the generics or just deprecate them and let them undefined.

I do not see that adapters are useless as they can be replaced by deserializers: After writing several of both types, I really need to say that writing an adapter is much simpler and much less error prone than writing an adapter. Hence I use deserializers only in rare cases. Also, my personal belief is that adapter should not always map to JsonValue, but in fact we should be able to do JsonbAdapter<CustomType, URI> for example.

mkarg avatar Oct 07 '19 13:10 mkarg

@mkarg I agree on that but it can't be implemented. How do you:

  1. select the adapter to use? based on a single side of the types?
  2. ensure it does not loop/conflict and still behave as expected?

With a generic adapter API and a registration on JsonbConfig it just fails too easily - at least I don't see it working.

rmannibucau avatar Oct 07 '19 14:10 rmannibucau

@mkarg I agree on that but it can't be implemented. How do you:

  1. select the adapter to use? based on a single side of the types?
  2. ensure it does not loop/conflict and still behave as expected?

With a generic adapter API and a registration on JsonbConfig it just fails too easily - at least I don't see it working.

  1. Json.fromJson(json, <T>) --> For all globally registered JsonbAdapter<T, ...> where T is not natively mappable compute the shortest length of all possible chains. For this, build chains starting from <T> --> JsonAdapter<T, U> --> for all <U> which are not natively mappable do that recursively... until an a "right-side-type" is found which is natively mappable. Do the same algorithm for the case of deserializing one field only.

  2. Loop prevention should be as simple as holding set of adapters-to-use at chain building: If the adapter to check already is in the chain, that chain building attempt can be excepted (the chain will have a virtual length of INFINITE).

mkarg avatar Oct 07 '19 15:10 mkarg

mappable means all classes specified in "Default Mapping" (3) section of the spec.

m0mus avatar Oct 08 '19 10:10 m0mus

mappable means all classes specified in "Default Mapping" (3) section of the spec.

@m0mus To be 100% sure: This includes the ability to write an adapter which maps <X, Map> and <X, URI>, as both, Map and URI, are listed in section 3? Hence it is definitively a bug of Johnzon to reject adapters which map upon Map and URI?

mkarg avatar Oct 08 '19 15:10 mkarg

3.3 otherwise spec must define the resolution algo - as jaxrs defines its router - and we would be back to the mentionned issues.

Note that your proposed algorithm make it better but still has the issue of not handling polymorphic adapters or value based adapters (if value x then i do y) so it does not resolve the base problem which require a chain responsability api in adapter - like annotation processors - if we would like this feature (which is not needed IMO).

rmannibucau avatar Oct 08 '19 15:10 rmannibucau

Well, it would be nice if the two of you could find an agreement whether we talk about (3) or (3.3)... :-)

mkarg avatar Oct 08 '19 17:10 mkarg