Fix #4938 Allow @JsonCreator to return null

[JooHyukKim]

resolves #4938

[cowtowncoder]

I am not sure this is sufficient -- couple of issues not covered by tests:

1. Unknown properties collected during (see line 469/472 below) need to be processed
2. This may be called in the middle of Object value and following properties need to be handled or skipped (otherwise parent deserializer gets control back in the middle of child Object value

I suspect that for (2) we need to either skip all content, or call handleUnknownProperty() all that may remain.

But the first thing is to write tests I think. Note, too, that it is possible to have properties that are not Creator properties. Those cannot be assigned when null is returned (but skipping probably makes sense).

[JooHyukKim]

Hmmm, if bean is null, I am not sure if there is anything else we can do than return, unless there is some sort of post-processing that can be done to a null value that I forgot to consider?

Regardless, added test case via 332ac77.

[cowtowncoder]

We can't do anything to null, but we must consume any other properties that may have been buffered (or just drop), but more importantly, ones that may come after Creator properties are collected: collection stops when value for everything needed is gathered which may be before JSON Object is fully read.

So, f.ex: @JsonCreator-annotated constructor (for Pojo that matches "pojo1") takes "a" and "b", and we have:

{
  "pojo1" : {
    "a": 2,
    "z": 4, // unknown, will be buffered
    "b": 3, // Creator called here when "a" and "b" gotten
    "c": 4 // but we MUST consume "c" and value, and END_OBJECT!
  } ,
  "pojo2": {
      "a":3,
      // and so on
   }
}

then Creator is called half-way through reading Object value of pojo1. But it cannot just leave without making sure full Object value is processed. All it can do is skip (cannot set), but without doing that, following deserializers will be called at weird/wrong state. Every deserializer must consume one full JSON value (either by itself, or "child" deserializers it calls).

I hope this makes more sense.

[JooHyukKim]

1. Added a test case with InputStream
2. And also modified the BeanDeserializer to not return null right away

[cowtowncoder]

Need to address case of properties other than those matched as Creator properties: ones collected before Creator called and ones that may come after it's called.

[cowtowncoder]

No,not quite -- cannot do regular handling since we have no bean. Instead, call a new method which will handle skipping, passing till the end.

[cowtowncoder]

This approach does not work: Jackson parsers buffer content so all content being read from InputStream does not actually necessarily mean all has been consumed.
There is a method in JsonParser for pushing back "unread" content but...

Instead of this, you could just enable DeserializationFeature.FAIL_ON_TRAILING_TOKENS to verify there is no more content to be read (tokens). That should catch the issue.

[JooHyukKim]

@cowtowncoder If you see current build failure there are two tests failing trying to handle unknown properties in following ways...

• One failing because of FAIL_ON_UNKNOWN_PROPERTIES enabled.
• The other because creator returned null and trying to set any-setter property on null bean

Both reaches BeanDeserializer.handleUnknownVanilla(), could it be what we already want?

Or are you saying we need the execution to continue to go on further down, to like ...

... here?

[cowtowncoder]

@cowtowncoder If you see current build failure there are two tests failing trying to handle unknown properties in following ways...

* One failing because of `FAIL_ON_UNKNOWN_PROPERTIES` enabled.

* The other because creator returned `null` and trying to set any-setter property on `null` bean

Both reaches BeanDeserializer.handleUnknownVanilla(), could it be what we already want?

Or are you saying we need the execution to continue to go on further down, to like ...

... here?

I think that if null is returned, we should probably do forced skipping and ignoring all properties until END_OBJECT.

But that handling should be extracted to a helper method just to keep logic cleaner and possibly change. And pass already collected information.

[JooHyukKim]

@cowtowncoder Just some updates,

• I added void testDeserializeReadingUntilEndObject() test to verify we read all the way to END_OBJECT.
• Still haven't managed to think of the RIGHT place to insert new method to handle unknown props 🥲

[JooHyukKim]

Alright, I think I'm stuck at trying to find the proper place/timing for method extraction.
May it be because current implementation doesn't have much changes tbh.

No rush for this work to be done, but any opinion much appreciated! 😆

[cowtowncoder]

Alright, I think I'm stuck at trying to find the proper place/timing for method extraction. May it be because current implementation doesn't have much changes tbh.

No rush for this work to be done, but any opinion much appreciated! 😆

I'm happy to have a look -- might take a while as I'm going for 1 week vacation tomorrow. But will help when I get back (unless I have extra time today).