jakartaee / jsonb-api

Jakarta JSON Binding
https://eclipse-ee4j.github.io/jsonb-api/
Other
78 stars 39 forks source link

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

Open mkarg opened 5 years ago

mkarg commented 5 years ago

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"?

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

rmannibucau commented 5 years ago

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

mkarg commented 5 years ago

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

rmannibucau commented 5 years ago

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

mkarg commented 5 years ago

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

rmannibucau commented 5 years ago

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

mkarg commented 5 years ago

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

rmannibucau commented 5 years ago

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

mkarg commented 5 years ago

@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, ) --> 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 --> JsonAdapter<T, U> --> for all 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).

m0mus commented 5 years ago

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

mkarg commented 5 years ago

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?

rmannibucau commented 5 years ago

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

mkarg commented 5 years ago

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