jakartaee / jsonb-api

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

May adapter input / output be null? #193

Open mkarg opened 5 years ago

mkarg commented 5 years ago

Please add information to the JSON-B Spec / JavaDocs explicitly declaring how to handle null in JsonbAdapter implementations:

rmannibucau commented 5 years ago

Hi @mkarg ,

My understanding is an adapter does not receive null and can return null.

I'm not sure about Yasson but Johnzon and TCK comply with that.

@m0mus maybe you can confirm about Yasson?

Romain

mkarg commented 5 years ago

@rmannibucau Thanks, but in fact it would be good to clearly put that information into the JavaDocs.

m0mus commented 5 years ago

My understanding is that an adapter can receive null and can return null. The spec doesn't specify it, so it's allowed.

mkarg commented 5 years ago

According to the concencus between both of you, I'd like to kindly ask you to review and accept PR #194. :-)

rmannibucau commented 5 years ago

Hmm, not sure there is a consensus if you reread ;).

On a spec level it is also not really needed because it is about setting a default in the bean directly and the javadoc speaks about object or JSON data so not null (https://javaee.github.io/javaee-spec/javadocs/javax/json/bind/adapter/JsonbAdapter.html).

Guess that if we want to enable null we must add a @AcceptNull in next version.

mkarg commented 5 years ago

@rmannibucau Oops actually I completely missed the word "not" in your answer! Sorry for that!

Anyways it won't change my PR: To be one safe sid, an adapter vendor has to take care that Yasson will input null according to @m0mus. Hence, even if Johnzon does not, the check has to be there anyways.

Proposal: As there is disagreement on the input but not the output, an adapter de-facto wouldn't be unsafe if it does not check for null. Hence my PR still is correct to be on the safe side.

rmannibucau commented 5 years ago

@mkarg the reasoning is biaised but I guess iy is the game?

Now with a spec hat this change breaks backward compatibility. If it was ambiguous we must be conservative so I fear we have to use Johnzon option even if I would probably have desired to accept null.

Note that it does not limit any use case thanks deserializers which are iso for all the null cases.

mkarg commented 5 years ago

@rmannibucau It does not limit use case as long as you don't have to migrate an existing application from Johnzon to Yasson: To be on the safe side, you must change its code to either replace adapters by deserializers, or catch the null case in the adapters. As preventing code changes in applications in product migrations is one target of specifications, I do not see it as relaxed as you do. ;-)

rmannibucau commented 5 years ago

@mkarg I'm not following you. Here is my view:

Current state: null input in adapters is rather defined as forbidden and spec examples encourage that (unintentionally I guess but this is what users will have used as a start so the code they have written in their app). So we must consider it is unsafe to pass null to an adapter in JSON-B 1.0.

1.x, x > 0: we can enable to pass null in adapter if desired but it must be explicit by the user to not break existing code. Here I agree we must explicit in the spec it is not portable to handle null in adapters in 1.0 and clarify it in 1.1 which means we either add an option to let the user notify the impl it is ok to pass null or we just say it is not a case which is that important since deserializers handle 100% of these cases as easily as adapters in practise thanks to the deserialization context.

Indeed I think we just need to clarify that null are not passed to adapters and even actually recommand the deserializer usage rather than adapter one in cases not targetting a direct mapping (adapter are only nice to map a type to string or simple types both ways, a bit like enums do, for other cases they create headaches to end users because they never know if it loops if there are conflicting adapters, if it is automatically taken into account in all mapping calls etc....).

mkarg commented 5 years ago

Personally I have no bias. I'm good with either allowing or disallowing null. What I want is just unambiguous JavaDoc, Spec and Examples. If JSON-B Committers tells me their final decision, I will modify my PR accordingly. :-)

Ordiel commented 1 week ago

Sorry for waking up the zombies, yet this does not seem to be resolved. Instead of the proposed @AcceptNull, I think it would be best if deserializers (including the default ones) take into consideration both the:

@JsonbNillable

and

@JsonbProperty(nillable = true)

Those (by their javadoc) are somewhat specific for serialization indicating if it is desired to output them as null; yet I consider they should be respected when deserializing the values so that deserializer don't choke on such values being null


My current issue is with null deserialization of an enum value. I have an API which is returning a certain set of attributes selectivelly so sometimes my input stream (a.k.a: the raw JSON) does not contain such enum value and its causing it to trip. I consider writing a JsonbAdapter to be a bit too much for simply handling null