You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@johnzon.apache.org by David Blevins <da...@gmail.com> on 2022/05/17 03:42:04 UTC
Null in Converter, Adapter, JsonbAdapter (was Re: Review of exception handling)
There was one scenario from the report that I'd love to get some thoughts on if it is a bug or not.
We have some asymmetry on how we handle nulls to/from Adapter, Converter and JsonbAdapter. If it's a bug, I'll go hunt it down and submit a fix, file jiras, etc.
- Converter
- toString returning null fails
- fromString returning null ok
- Adapter
- B from(A) returning null B fails
- A to(B) returning null A ok
- JsonbAdapter
- B adaptToJson(A) returning null B fails
- A adaptFromJson(B) returning null A ok
So, bug or not bug?
My $0.02 is it seems like a bug.
-David
> On May 9, 2022, at 2:40 AM, David Blevins <da...@gmail.com> wrote:
>
> All,
>
> Can you take a look at the bold lines towards the bottom of these two documents?
>
> Mapper API handling of exceptions throw by users or during deserialization
>
> - https://gist.github.com/dblevins/3fbad79900e75e1970d32c9681657e5e
>
> Jsonb API handling of exceptions throw by users or during deserialization
>
> - https://gist.github.com/dblevins/ace48ca0aeb1e9e2b22d3bc31fee7d07
>
>
> Most everything that is not titled "deserialization" is testing what can be caught if the user throws a RuntimeException from their code, say via a setter, getter, constructor, adapter, serializer, etc.
>
> Where you see "MyException" it indicates the user cannot catch MapperException or JsonbException, but instead must/may catch their exception directly. It's the proposal that all of these get converted to either MapperException or JsonbException to make them more consistent with 90% of the other scenarios, but also so we can improve the error messages.
>
> Where you see NullPointerException, there is most likely a bug that needs fixing. Our Converter/Adapter code can handle nulls in one direction, but not the other. Likely, no exception is the best course.
>
> Where you see MapperException in the JsonbAPI-Exceptions.adoc, that also seems like a bug. It should probably at least be JsonbException.
>
> Let me know if you think the recommendations are good or bad.
>
>
> -David
>
Re: Null in Converter, Adapter, JsonbAdapter (was Re: Review of exception handling)
Posted by Romain Manni-Bucau <rm...@gmail.com>.
Le mar. 17 mai 2022 à 23:54, David Blevins <da...@gmail.com> a
écrit :
> > On May 16, 2022, at 11:29 PM, Romain Manni-Bucau <rm...@gmail.com>
> wrote:
> >
> > Strictly speaking - not saying we can't change it - it was the intent.
> > Rational (keep in mind originally point was that converter (string) were
> > always sufficient but jsonb made it complicated) was that binding null -
> > JsonValue.NULL, too big data, java out of bound value, .... - is ok but
> > serializing something in an object as null was a bug (lost of data).
>
> Yeah, I struggled with that one and couldn't quite reach a confident
> opinion as I can see it both ways.
>
> The thought that leads me to think it is not a bug is that there are no
> scenarios where we see a null in a json document and pass that in and say
> 'convert/adapt this" As well if you wanted null to be written, you could
> set the pre-converted value to be null.
>
> Then I think through scenarios where it might be
> hard/impractical/impossible to actually go through and null out properties
> in the object tree so null shows up in the json. Any final field would
> pose a challenge as would poking out null values from a large array or
> immutable collection. Even if the thing wasn't final, I'd probably still
> be mad I had to "pay twice" by walking the object tree, evaluate all my
> convertable/adaptable fields before serialization only to have to evaluate
> them all again during serialization.
>
> >
> > Now where it gets complicated:
> >
> > 1. Spec - jsonb - is ambiguous even if yasson supports null both ways
>
> I didn't check yasson.
>
> > 2. We cant break users by default (means a JsonbConfig toggle, default
> > being backward compat I'd say) if we want to support it
> >
> > Hope it helps a bit
>
> Sort of. Maybe the best approach then is to add that flag and then
> improve the NullPointerException so it says "set this property if you want
> to return null ... blah blah blah" Then users would see the solution in
> their exception.
>
Well, if we do I fear we have to make it work and skip exceptions. I hardly
see us breaking which means catching NPE around adapters and hoping we can
ignore it...which is inconsistent with your work on exceptions :s.
> Another thought which just occurred to me is what if we had an annotation
> that could be put on the converter/adapter? If we get a null, we check for
> the annotation. If we find it we allow the null, if not we default to
> throwing an exception.
>
Sounds not bad to me - just check the annot when creating the mapping model
and store it, not at runtime when "we get a null" please.
The "if not" can stay what we have I think, ie ignore which is consistent
with our and jsonb defaults to ignore null by default.
> Thinking out loud.
>
>
> -David
>
>
Re: Null in Converter, Adapter, JsonbAdapter (was Re: Review of exception handling)
Posted by David Blevins <da...@gmail.com>.
> On May 16, 2022, at 11:29 PM, Romain Manni-Bucau <rm...@gmail.com> wrote:
>
> Strictly speaking - not saying we can't change it - it was the intent.
> Rational (keep in mind originally point was that converter (string) were
> always sufficient but jsonb made it complicated) was that binding null -
> JsonValue.NULL, too big data, java out of bound value, .... - is ok but
> serializing something in an object as null was a bug (lost of data).
Yeah, I struggled with that one and couldn't quite reach a confident opinion as I can see it both ways.
The thought that leads me to think it is not a bug is that there are no scenarios where we see a null in a json document and pass that in and say 'convert/adapt this" As well if you wanted null to be written, you could set the pre-converted value to be null.
Then I think through scenarios where it might be hard/impractical/impossible to actually go through and null out properties in the object tree so null shows up in the json. Any final field would pose a challenge as would poking out null values from a large array or immutable collection. Even if the thing wasn't final, I'd probably still be mad I had to "pay twice" by walking the object tree, evaluate all my convertable/adaptable fields before serialization only to have to evaluate them all again during serialization.
>
> Now where it gets complicated:
>
> 1. Spec - jsonb - is ambiguous even if yasson supports null both ways
I didn't check yasson.
> 2. We cant break users by default (means a JsonbConfig toggle, default
> being backward compat I'd say) if we want to support it
>
> Hope it helps a bit
Sort of. Maybe the best approach then is to add that flag and then improve the NullPointerException so it says "set this property if you want to return null ... blah blah blah" Then users would see the solution in their exception.
Another thought which just occurred to me is what if we had an annotation that could be put on the converter/adapter? If we get a null, we check for the annotation. If we find it we allow the null, if not we default to throwing an exception.
Thinking out loud.
-David
Re: Null in Converter, Adapter, JsonbAdapter (was Re: Review of exception handling)
Posted by Romain Manni-Bucau <rm...@gmail.com>.
Hi,
Strictly speaking - not saying we can't change it - it was the intent.
Rational (keep in mind originally point was that converter (string) were
always sufficient but jsonb made it complicated) was that binding null -
JsonValue.NULL, too big data, java out of bound value, .... - is ok but
serializing something in an object as null was a bug (lost of data).
Now where it gets complicated:
1. Spec - jsonb - is ambiguous even if yasson supports null both ways
2. We cant break users by default (means a JsonbConfig toggle, default
being backward compat I'd say) if we want to support it
Hope it helps a bit
Romain
Le mar. 17 mai 2022 à 05:42, David Blevins <da...@gmail.com> a
écrit :
> There was one scenario from the report that I'd love to get some thoughts
> on if it is a bug or not.
>
> We have some asymmetry on how we handle nulls to/from Adapter, Converter
> and JsonbAdapter. If it's a bug, I'll go hunt it down and submit a fix,
> file jiras, etc.
>
>
> - Converter
> - toString returning null fails
> - fromString returning null ok
>
> - Adapter
> - B from(A) returning null B fails
> - A to(B) returning null A ok
>
> - JsonbAdapter
> - B adaptToJson(A) returning null B fails
> - A adaptFromJson(B) returning null A ok
>
> So, bug or not bug?
>
> My $0.02 is it seems like a bug.
>
>
> -David
>
> > On May 9, 2022, at 2:40 AM, David Blevins <da...@gmail.com>
> wrote:
> >
> > All,
> >
> > Can you take a look at the bold lines towards the bottom of these two
> documents?
> >
> > Mapper API handling of exceptions throw by users or during
> deserialization
> >
> > - https://gist.github.com/dblevins/3fbad79900e75e1970d32c9681657e5e
> >
> > Jsonb API handling of exceptions throw by users or during deserialization
> >
> > - https://gist.github.com/dblevins/ace48ca0aeb1e9e2b22d3bc31fee7d07
> >
> >
> > Most everything that is not titled "deserialization" is testing what can
> be caught if the user throws a RuntimeException from their code, say via a
> setter, getter, constructor, adapter, serializer, etc.
> >
> > Where you see "MyException" it indicates the user cannot catch
> MapperException or JsonbException, but instead must/may catch their
> exception directly. It's the proposal that all of these get converted to
> either MapperException or JsonbException to make them more consistent with
> 90% of the other scenarios, but also so we can improve the error messages.
> >
> > Where you see NullPointerException, there is most likely a bug that
> needs fixing. Our Converter/Adapter code can handle nulls in one
> direction, but not the other. Likely, no exception is the best course.
> >
> > Where you see MapperException in the JsonbAPI-Exceptions.adoc, that also
> seems like a bug. It should probably at least be JsonbException.
> >
> > Let me know if you think the recommendations are good or bad.
> >
> >
> > -David
> >
>
>