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

May adapter input / output be null?

Open mkarg opened this issue 6 years ago • 11 comments

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

  • MUST a JsonbAdapter implementation expect to receive null?
  • MAY a JsonbAdapter implementation return null?

mkarg avatar Oct 01 '19 13:10 mkarg

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

rmannibucau avatar Oct 01 '19 16:10 rmannibucau

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

mkarg avatar Oct 01 '19 20:10 mkarg

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

m0mus avatar Oct 01 '19 21:10 m0mus

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

mkarg avatar Oct 01 '19 22:10 mkarg

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.

rmannibucau avatar Oct 02 '19 04:10 rmannibucau

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

mkarg avatar Oct 02 '19 05:10 mkarg

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

rmannibucau avatar Oct 02 '19 05:10 rmannibucau

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

mkarg avatar Oct 02 '19 09:10 mkarg

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

rmannibucau avatar Oct 02 '19 10:10 rmannibucau

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

mkarg avatar Oct 02 '19 12:10 mkarg