Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 19 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
7089146
Add fix
JooHyukKim Dec 8, 2024
6d6de03
doc : clean up
JooHyukKim Dec 8, 2024
30d2cb0
Remove test
JooHyukKim Dec 8, 2024
cde7eb8
Add case for AtomicReference
JooHyukKim Dec 11, 2024
c36b8f2
Merge branch '2.19' into fix-jdk8-module-86
JooHyukKim Dec 11, 2024
a683b1a
Fix AtomicReference tests to actually make sense
JooHyukKim Dec 11, 2024
42ba9a6
doc: Add some comments to try to explain things
JooHyukKim Dec 11, 2024
7381be2
Add release note
JooHyukKim Dec 11, 2024
e800e18
Merge branch '2.19' into fix-jdk8-module-86
cowtowncoder Dec 12, 2024
636792a
Merge branch '2.18' into fix-jdk8-module-86
JooHyukKim Dec 19, 2024
e4fd2f0
Merge branch '2.19' into fix-jdk8-module-86
JooHyukKim Dec 19, 2024
0fc9a94
Add a failing case
cowtowncoder Dec 20, 2024
0f3ffcb
Merge branch '2.19' into fix-jdk8-module-86
cowtowncoder Dec 20, 2024
2f19d45
uncomment accidentally commented out test case
cowtowncoder Dec 20, 2024
e374faa
review: Use `assertInstanceOf()`
JooHyukKim Dec 20, 2024
de92568
Modify test to use inclusion as expected
JooHyukKim Dec 20, 2024
4ac30d3
Modify test names
JooHyukKim Dec 20, 2024
1b3f3d9
Revert modifying test (wrt varying Inclusion in default typing, when …
JooHyukKim Dec 21, 2024
940034d
Test : Add more
JooHyukKim Dec 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/main/java/com/fasterxml/jackson/databind/deser/std/ReferenceTypeDeserializer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,6 @@ public Object deserializeWithType(JsonParser p, DeserializationContext ctxt,
if (_valueTypeDeserializer == null) {
return deserialize(p, ctxt);
}
return referenceValue(_valueTypeDeserializer.deserializeTypedFromAny(p, ctxt));
return typeDeserializer.deserializeTypedFromAny(p, ctxt);
}
}
15 changes: 8 additions & 7 deletions src/main/java/com/fasterxml/jackson/databind/ser/std/ReferenceTypeSerializer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
import com.fasterxml.jackson.annotation.JsonInclude;

import com.fasterxml.jackson.core.JsonGenerator;
import com.fasterxml.jackson.core.JsonToken;
import com.fasterxml.jackson.core.type.WritableTypeId;
import com.fasterxml.jackson.databind.*;
import com.fasterxml.jackson.databind.annotation.JsonSerialize;
import com.fasterxml.jackson.databind.introspect.Annotated;
Expand Down Expand Up @@ -407,18 +409,17 @@ public void serializeWithType(T ref,
// 19-Apr-2016, tatu: In order to basically "skip" the whole wrapper level
// (which is what non-polymorphic serialization does too), we will need
// to simply delegate call, I think, and NOT try to use it here.

// Otherwise apply type-prefix/suffix, then std serialize:
/*
typeSer.writeTypePrefixForScalar(ref, g);
serialize(ref, g, provider);
typeSer.writeTypeSuffixForScalar(ref, g);
*/
// Otherwise apply in order [ type-prefix -> value -> type-suffix ] then std serialize:
// prefix
WritableTypeId typeId = typeSer.writeTypePrefix(g, typeSer.typeId(ref, JsonToken.VALUE_STRING));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@cowtowncoder cowtowncoder Dec 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// content
JsonSerializer<Object> ser = _valueSerializer;
if (ser == null) {
ser = _findCachedSerializer(provider, value.getClass());
}
ser.serializeWithType(value, g, provider, typeSer);
// suffix
typeSer.writeTypeSuffix(g, typeId);
}

/*
Expand Down