Fix : Cannot read Optionals written with StdTypeResolverBuilder, from [jackson-modules-java8#86]

[JooHyukKim]

1. This PR fixes Cannot read Optionals written with StdTypeResolverBuilder  jackson-modules-java8#86
2. Fixed tests are written in java8-module PR Test #86 Cannot read Optionals written with StdTypeResolverBuilder jackson-modules-java8#335
3. Integrated tests covering 1 and 2 via PR in my personal repo Integrated test  JooHyukKim/jackson-databind#8

[cowtowncoder]

@JooHyukKim Ok a good start, but we also need unit tests with AtomicReference to mimic Optional one(s), if possible (I think it is), mostly for deserialization. I think they fit in existing JDKAtomicTypesDeserTest, no need to add more test classes (easier to merge to master if so).

[cowtowncoder]

Ok. I think this makes sense, except for one problem -- JsonToken.VALUE_STRING works for some cases, not for others. Problem is, we do not actually know the expected shape of contained value. Later on, test can be broken; I'll add a note where and how, below.

[cowtowncoder]

... I am really not sure how to fix this. I understand why type id would be needed, for AtomicReference / Optional wrapper. Unfortunately there is a key problem: nothing else will be written for these wrappers.
Unless... unless I guess if we decide that for the specific case of polymorphic value, there is.

And I think the only possibility, there, is extra wrapper array. I'll see if that works.

[cowtowncoder]

Sigh. Nope. Was not able to make this work overall.

[JooHyukKim]

At least if we have more test case 👍🏼👍🏼

[JooHyukKim]

@cowtowncoder I modified the test via de92568, to configure StdTypeResolverBuilder to use appropriate inclusion scheme for each mapper. Seems like that's way to go?

[cowtowncoder]

Perhaps? I am not sure which cases to focus most; ones with @JsonTypeInfo introduced ones or Default Typing (I know, it's reported wrt Default Typing but in a way @JsonTypeInfo cases are more important -- Default Typing having security concerns).

Another confounding factor is that for String and Integer, type id MUST NOT be written ("natural" types), nor expected.
It's a mess. :)

[cowtowncoder]

But no matter what, we really cannot use JsonToken.VALUE_STRING for all cases because some values are serialized as Objects (Maps, most POJOs), others as Arrays (Collections) and those cases would not work with this type id.

Yet there is also no way for ReferecenTypeSerializer to know for sure value shape.

This is why I think we actually must use JSON Array wrapper for Type Id case to know exact shape for just this case (in theory JSON Object but that would require bogus key for single entry; array seems cleaner).

[JooHyukKim]

I am starting to think this situation (Default Typing overrides@JsonTypeInfo) is inevitable, due to Jackson does heavy lifiting during serialization.
I don't think there is easy, sensible way to introspect all the way from Foo<T> -> AtomicRference<T> -> Animal to get configured type information, during runtime.

So maybe we can either...

1. separate concerns, resolve very specific issue here around Optional<String> and create separate milestone for this override case by pushing some tests as tofix.. or
2. Just add all tests to tofix and come back later in the future when we are in best shape to do so.

[cowtowncoder]

I think the problem is more with the "missing" serialization for Optional, actually. For non-polymorphic case it is sort of ok, as long as POJO typing is defined. Basically following 2 serialize to identical JSON:

static class Required {
   public MyPojo value = new MyPojo();
}

static class Optional {
  public Optional<MyPojo> value = Optional.of(new MyPojo());
}

// Both become something like:

{ "value": {
   "myPojoProp1": 1,
   "myPojoProp2" : 2
}

// Compared to say

static class Multiple {
   public List<MyPojo> values = List.of(new MyPojo());'
}

// which becomes

{ "values": [
   {
   "myPojoProp1": 1,
   "myPojoProp2" : 2
  }
]}

this is ok because no Type Id needs to be added anywhere.
But if (polymorphic) Type Id needs to be added, there is only place for one of Type Ids (MyPojo) -- for Optional there is no JSON value to modify/include because it is invisible, unlike List and others for which there is JSON Array.
If there was JSON Array to represent Optional level things would be easier.

So for Collection case, As.WRAPPER_ARRAY we might have something like:

[ "com.xxx.Multiple",
   { "values": [ "java.util.ArrayList",  [
    [ "com.xxx.MyPojo",  {
        "myPojoProp1": 1,
       "myPojoProp2" : 2
      }
    ]
  ]
  ]
]}]

where extra "wrapper Array" is added around Multiple, List as well as MyPojo element)s_s (depending on details of @JsonTypeInfo or Default Typing -- here assume it is applicable to all of Multiple, List and MyPojo).

This is not really possible with Optional because nothing is written to represent Optional

Now: in theory we could consider doing something similar to EnumSet where Class name as type id is actually polymorphic -- and we might be able to do the same with optional. But it would have to replace Type Id of thing contained (and only works for Class Name type id, but that's probably to be expected).
But I suspect that would be even more work.

As to "solving" reported case... I am still worried I don't fully understand the behavior before and after change. We'd need to, I think, verify exact JSON being produced and not simply checking round-tripping.
My concern is specifically that we end up changing serialization output and breaking other usage that is not covered by test cases.

[JooHyukKim]

True, this is getting bigger than I imagined I agree with your concerns wrt serialization output. Knowing that the serialization output changed versions back, (2.8 -> 2.9) complicates things I think.

We might even consider current output as expected behavior and suggest to try work around or something, for cases where they update [ 2.8 -> 2.9 or later like 2.18 .

In any case, I will close this off for now and would like to get back to taking steps toward 3.0 -- I will leave some closing comments

[cowtowncoder]

If changing As.WRAPPER_OBJECT to any other choice -- specifically, As.PROPERTY or As.WRAPPER_ARRAY, test fails, because of earlier write of TypeId assuming output value is a scalar (need not be JSON String).

[JooHyukKim]

added some comment above

[cowtowncoder]

Quick note: Default Typing does not override @JsonTypeInfo since annotation is more specific than general setting. This is why we probably need separate types.
Although it also gets bit more complicated to reason about combinations.

We also need to try As.PROPERTY as mechanism.

[JooHyukKim]

True, general one should not take effect if there is more specific one.

[JooHyukKim]

Reverting test modification that I made.

[JooHyukKim]

Closing -- because the work itself complicates things around serialization output change that happened 2.8 -> 2.9 and it's already 2.18 making new behavior de-facto standard 🤔.

Anyone is welcome to pick the work anytime. I myself would go do more 3.0 related work.