Yasson is unable to deserialize an Optional<String> in native

[loicmathieu]

Describe the bug
Yasson is unable to deserialize a POJO with an Optional<String> attribute in native mode.
There is no issue in JVM mode.

There is no stacktrace, only the following SEVER log:

2020-07-15 15:15:49,462 SEVERE [org.ecl.yas.int.Unmarshaller] (executor-thread-1) Unable to deserialize property 'optionalString' because of: Cannot create instance of a class: class java.lang.String, No default constructor found.

Expected behavior
Deserializing an Optional<String> works in both native and JVM mode.

To Reproduce
Running the mongodb-client integration test from the following branch: https://github.com/loicmathieu/quarkus/tree/mongodb/property-codec-provider/integration-tests/mongodb-client

Environment (please complete the following information):

• Output of uname -a or ver: Linux 5.4.0-40-generic Arc - request context propagation #44-Ubuntu SMP Tue Jun 23 00:01:04 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux
• Output of java -version: openjdk version "11.0.7" 2020-04-14
• GraalVM version (if different from Java): GraalVM Version 20.1.0 (Java Version 11.0.7)
• Quarkus version or git rev: current master

[loicmathieu]

/cc @aguibert

[geoand]

@loicmathieu would registering String's constructors for reflection in Quarkus not be enough?

[loicmathieu]

@geoand no idea, I didn't have time yet to investiguate on this issue. But I'm very surprise String didn't works OOTB with GraalVM ...

[geoand]

Well, I think it's more that the String is created with reflection that causes the problem.

I am pretty sure that if we register it for reflection, we'll be fine. We probably need to introspect the return types of the codecs and make sure we register things for reflection (similar to what we do for JAX-RS for example).

[loicmathieu]

OK, I think I understand what you means now ;)
I'll have a look next week ...

[loicmathieu]

@geoand adding the following reflection-config.json file works so yes, the issue is that the java.lang.String type is not registered for reflection.

[
  {
    "name" : "java.lang.String",
    "allDeclaredConstructors" : true,
    "allPublicConstructors" : true,
    "allDeclaredMethods" : true,
    "allPublicMethods" : true,
    "allDeclaredFields" : true,
    "allPublicFields" : true
  }
]

Here is my test entity:

public class Pojo {
    public ObjectId id;
    public String description;
    public Optional<String> optionalString;
}

And here is my test resource:

@Path("/pojos")
@Produces(MediaType.APPLICATION_JSON)
@Consumes(MediaType.APPLICATION_JSON)
public class PojoResource {
    @Inject
    MongoClient client;

    private MongoCollection<Pojo> collection;

    @PostConstruct
    public void init() {
        MongoDatabase database = client.getDatabase("books");
        collection = database.getCollection("pojo", Pojo.class);

    }

    @GET
    public List<Pojo> getPojos() {
        FindIterable<Pojo> iterable = collection.find();
        List<Pojo> pojos = new ArrayList<>();
        for (Pojo doc : iterable) {
            pojos.add(doc);
        }
        return pojos;
    }

    @POST
    public Response addPojo(Pojo pojo) throws UnsupportedEncodingException {
        collection.insertOne(pojo);
        return Response
                .created(URI.create("/pojos/" + URLEncoder.encode(pojo.id.toString(), StandardCharsets.UTF_8.toString())))
                .build();
    }
}

I would expect as the Pojo class has an Optional<String> field and is taken as paratemer from my endpoint that the resteasy extension would register Strong for relfection.

So, for me, it's a bug in the resteasy extension right ?

[geoand]

Yeah, that sounds true

[gsmet]

We exclude most of the java.lang types if not all.

[loicmathieu]

The Pojo class is correctly registered for reflection with it's fields.
But GraalVM when it registers the Optional type didn't register the parameter type of the Optional (here the String class).

I don't know if we can do something about it, and if it's an issue on GraalVM side or on our side.

For the moment I found a workaround by registering String for reflection from my Pojo Class via @RegisterForReflection(targets = String.class).
And by the way this functionality is not included in the native guide but is so much elegant than providing a JSON configuration file that I will open a PR to add it ;)

[gsmet]

As I said, it's us not registering the java.lang types when registering the hierarchy for reflection.

[loicmathieu]

@gsmet didn't find this in the code of resteasy or on core deployement steps related to registering types for reflection ...

Looking at the code inside NativeImageAutoFeatureStep I saw that fields types are registered for reflection, but when the type of a field is generic I don't see the type parameter of this generic type being registered for relfection.

[gsmet]

https://github.com/quarkusio/quarkus/blob/master/core/deployment/src/main/java/io/quarkus/deployment/builditem/nativeimage/ReflectiveHierarchyBuildItem.java#L98

[loicmathieu]

@gsmet thanks, adding java.lang.String to the WHITELISTED_FROM_IGNORED_PACKAGES of ReflectiveHierarchyBuildItem fixes the issue.

Should I provides a PR for this ?

[gsmet]

I don't know. I don't understand why Yasson would work with a plain String and bail out on String reflection for an Optional<String>. It doesn't sound normal to me.

[loicmathieu]

If I understand correctly how Yasson handle Optional, it creates new serializer / deserializer for the Optional value type and uses these, so normaly it should use the String Serializer / Deserializer that is used by the String attribute (the other attribute of my Pojo class). So it should work.

See https://github.com/eclipse-ee4j/yasson/blob/master/src/main/java/org/eclipse/yasson/internal/serializer/OptionalObjectDeserializer.java#L54 and https://github.com/eclipse-ee4j/yasson/blob/master/src/main/java/org/eclipse/yasson/internal/serializer/OptionalObjectSerializer.java#L87

Maybe @aguibert can help us on this ?

[jaikiran]

I think Yasson should perhaps check the "optionalValueType" and if it finds it to be a primitive type (like String) then use/delegate the serialization/deserialization to the existing serializer/deserializer they have for these primtive types.

Given that there already is a workaround (specifically adding the reflection configuration for Graal) to get past this, I think it's perhaps better to wait and hear if yasson is willing to do this change, before introducing any wider change in Quarkus itself.

[loicmathieu]

I think Yasson should perhaps check the "optionalValueType" and if it finds it to be a primitive type (like String) then use/delegate the serialization/deserialization to the existing serializer/deserializer they have for these primtive types.

@jaikiran my understanding is that Yasson already do this, that's how I understand the code here https://github.com/eclipse-ee4j/yasson/blob/master/src/main/java/org/eclipse/yasson/internal/serializer/OptionalObjectDeserializer.java#L54 and here https://github.com/eclipse-ee4j/yasson/blob/master/src/main/java/org/eclipse/yasson/internal/serializer/OptionalObjectSerializer.java#L87

[loicmathieu]

@aguibert did you have time to look at this one ?

I propose to add java.lang.String to the WHITELISTED_FROM_IGNORED_PACKAGES of ReflectiveHierarchyBuildItem it fixes the issue but I don't know if it can have side effects.

[loicmathieu]

@gsmet @geoand @aguibert I want to move on for this one as one of my PR to fix an issue is waiting on it.

Is adding java.lang.String to the WHITELISTED_FROM_IGNORED_PACKAGES of ReflectiveHierarchyBuildItem ok for you guys ? If fixes the issue but I don't know if it can have side effects?
I can easily provides a PR for this, it's a oneliner ;)

[geoand]

Yeah, I don't think we want to register String for reflection TBH... If it's absolutely necessary, then I assume we only want to register its constructors

[loicmathieu]

@geoand the issue is that in this case we don't know if it's needed or not to register it. ReflectiveHierarchyBuildItem harvest all possible classes to register for reflection so if we don't add String to this discovery mechanism how can we know that we need to register it ?

A solution will be to register it in any cases as it is highly probable that it is already registered for reflection and the case of this issue is just an odd case ...

[geoand]

So yeah, I would prefer if in the jsonb extension we just register String's constructor for reflection and nothing else.

[loicmathieu]

OK, I'll give it a try

[loicmathieu]

PR #12045 workaround the issue by registering the constructors of String inside the jsonb extension.
But this is a workaround so we need to better understand what's going on on Yasson side for Optional<String>.

@gsmet suggested on the following comment to open an issue on Yasson: https://github.com/quarkusio/quarkus/pull/12045/files/ec0c7aaa39d758c31db1ff2f756710f4f1e42f18#r487019525

I don't know if it's a good idea as it's an issue with the usage of Yasson inside Quarkus. And I don't how to describe such issue. Yasson would not work on native if Quarkus didn't register all discovered types for reflection so creating a reproducer out of Quarkus could be realy challenging.

[gastaldi]

Let's close this as it's no longer an issue and open a new one if still necessary