Polymorphic deserialization fails with a JsonValue property

[zmarcos]

Describe the bug

Polymorphic deserialization throws exception when deserializing to a class with a JsonValue property.

jakarta.json.bind.JsonbException: Internal error: null

at org.eclipse.yasson.internal.DeserializationContextImpl.deserializeItem(DeserializationContextImpl.java:142)
at org.eclipse.yasson.internal.DeserializationContextImpl.deserialize(DeserializationContextImpl.java:127)
at org.eclipse.yasson.internal.JsonBinding.deserialize(JsonBinding.java:55)
at org.eclipse.yasson.internal.JsonBinding.fromJson(JsonBinding.java:62)

To Reproduce

@JsonbTypeInfo(
        key = "type",
        value = {@JsonbSubtype(alias = "jsv", type = PolyContainer.JsValueContainer.class)}
)
public interface PolyContainer {
    class JsValueContainer implements PolyContainer {
        public JsonValue jsValue;
    }
}

public void test() throws Exception {
        try (Jsonb jsonb = JsonbBuilder.create()) {
            var container = new PolyContainer.JsValueContainer();
            container.jsValue = Json.createValue("a json string");
            String containerSerialized = jsonb.toJson(container);
            assertNotNull(containerSerialized);
            var deserializedDirectly = jsonb.fromJson(containerSerialized, PolyContainer.JsValueContainer.class);//good
            assertNotNull(deserializedDirectly);
            var deserializedFromPoly = jsonb.fromJson(containerSerialized, PolyContainer.class);//bad
            assertNotNull(deserializedFromPoly);
        }
    }

Expected behavior

The object to be deserialized the same way as it does when not using polymorphism.

System information:

• OS: Windows 11
• Java Version: 21
• Yasson Version: 3.0.3

Additional context
Using a custom deserializer also does not work:

public class JSONGenericDeserializer implements JsonbDeserializer<JsonValue> {
    @Override
    public JsonValue deserialize(JsonParser jsonParser, DeserializationContext deserializationContext, Type type) {
        return jsonParser.getValue();
    }
}

[api-from-the-ion]

No offense, but I can't understand your example or test. What is the result you try to achieve? You try to deserialize into the interface, but this is impossible because interfaces have no fields. It's really hard to find something to serialize or deserialize in interfaces. ;-)

And I don't see any polymorphism here. This would work the same way without these annotations. Just take a look at the example in the documentation or specification : there you have more than one class derived from the interface. So it is nice to know exactly which class we try to serialize or deserialize.

I hope I could help you. Give me more information, please, if there is some misunderstanding.

[zmarcos]

Typically this is how I would use it:

@JsonbTypeInfo(
        key = "type",
        value = {
        @JsonbSubtype(alias = "jsv", type = PolyContainer.JsValueContainer.class),
        @JsonbSubtype(alias = "oth", type = PolyContainer.OtherContainer.class)
        }
)
public interface PolyContainer {
    class JsValueContainer implements PolyContainer {
        public JsonValue jsValue;
    }
    class OtherContainer implements PolyContainer {
        public String value1;
        public double value2;
    }
}

No offense, but I can't understand your example or test. What is the result you try to achieve?

If the type field is "jsv" it deserializes to PolyContainer.JsValueContainer, if "oth" to PolyContainer.OtherContainer.

I am sure this works, if you don't like interfaces change it to abstract class, it is the same problem.

[api-from-the-ion]

OK, thank you for the clarification. I got it: the issue isn't in polymorphism but in a JsonValue in the case of polymorphism. I don't see this with simple classes like String.

Furthermore, I think I found out why it is so and will try to produce a patch. I hope this will help. I would like to extend your test to cover more types.

[zmarcos]

OK, thank you for the clarification. I got it: the issue isn't in polymorphism but in a JsonValue in the case of polymorphism. I don't see this with simple classes like String.

Yes, yes, this is the issue.

Furthermore, I think I found out why it is so and will try to produce a patch. I hope this will help. I would like to extend your test to cover more types.

This is good news. I didn't test other JSON values, I only tried with the containing object being a record, had the same issue.

[api-from-the-ion]

I created a patch for this bug. Good news: your bug is solved. Not so good news: there are some other issues detected that need to be discussed. So I explain it step by step.

Your actual issue could really be explained and corrected quickly. Everything is in the stack trace. In cases like yours, where we have some JsonValue or its inheritance in a deserialized object, the own parser class implementation is used for parsing the already parsed JSON structure. So this isn't a real parser but a facade, which implements the parser methods by simply forwarding the parts of the already parsed JSON structure.

The issue in your case is that not all the methods of the parser are implemented. And the code you are calling is trying to use one of them. Which leads to the exception. So I implemented the methods, and now it should work in your case.

But during the implementation, I saw another problem. There is no real specification for the JSON-P. The official site states that JavaDoc is the official documentation for this API. And in the case of these methods, the documentation isn't 100 % clear, which leaves room for interpretation.

So, for example, the missing method in your case is getValue(), which should give you a JsonValue object. The description is clear about what should happen if the parser is at the position of an array or an object in the parsed JSON. But there is no clear description of the other cases.

I ended up with a TDD approach. It looks like an old Glassfish implementation (from here go here) was moved to Jakarta. And it is separated into the API part (here) and the reference implementation part called Parsson. The latest is used by Yasson (indirectly through the API and by providing it as a dependency) to really parse the given JSON.

At the end, I just wrote the tests for the Parsson implementation and used the same data and results to test my implementation. This way, I can at least assert that there is no difference in behavior between these two implementations. Because this is one of the ideas behind the API and interfaces in common: one shouldn't think about which implementation is behind it, this should work as expected no matter which implementation is used behind the scenes.

But back to your example: Parsson's implementation of the getValue() method delivers the results not only for the values in the JSON but also for the keys. Because the description isn't clear here, which part of the JSON should be delivered. But in the case of the parser, which is a facade for already parsed JSON structures, we have a problem: the keys are not some of the JsonValue inheritance but simply the String! So one needs something to create a JsonString from the String. And right now, this something (like jsonContext) is missing here.

Right now, because I can't create a JsonString for the keys, this parser realization is different from Parsson's one. It could be seen in the test. I will provide another change set with forwarded context and the creation of the JsonString from the keys.

Maybe we need some discussion about the API to make it more clear. Maybe even the current reference implementation (Parsson) isn't right here. Fun fact: during the development of this fix, I found a similar facade parser in Parsson itself. So there are at least two implementations there: JsonParserImpl is used for the parsing of real JSON data coming from the reader or stream. And JsonStructureParser is a facade parser used for forwarding information from an already parsed JSON structure, like the one Yasson is using. But this implementation implements even fewer methods than the Yasson one! So one would have even more exceptions in the case it uses Parsson's parser from JsonObject or JsonArray.

Maybe we need to correct it also in the Parsson. So we can use all of Parsson's parsers without fear of the exceptions from not-implemented methods. And after that, we could also use the JsonStructureParser in Yasson. Maybe I should also fork that repository and provide a patch for this situation there.

I also implemented three stream methods: getArrayStream(), getObjectStream() and getValueStream(). But there, I have the same questions. It is clear what the type of return values for these methods should be. And it's clear when these methods should return some value and when there should be an exception because of the wrong parser state in the parsed JSON. But what should be the content of these return values?

Should getArrayStream() deliver the current array as a stream, or the elements in the current array in the stream? Because the current Parsson's implementation is doing the latter now. The getObjectStream() method delivers a stream of key-value pairs. But should arrays and objects also be automatically sub-streamed? Or is it an obligation of the developer to manually do so in case he finds out that the current element in the stream is an array or an object, not a simple value? And the getValueStream() method right now gives me a stream with only one object—the whole root JSON itself. But shouldn't it be a stream of all elements in this root object, or from the point where the parser currently is? Just like getObjectStream(), but without the keys? These are all somehow legit questions about the API itself.

We need an opinion from somebody else involved here, like @Verdent.

[zmarcos]

I suspected it could an issue on Parsson.

But during the implementation, I saw another problem. There is no real specification for the JSON-P. The official site states that JavaDoc is the official documentation for this API. And in the case of these methods, the documentation isn't 100 % clear, which leaves room for interpretation.

This is very concerning.
Well, I found the spec on https://github.com/jakartaee/jsonp-api/blob/2.1.3/spec/src/main/asciidoc/jsonp.adoc

Introduction
Jakarta JSON Processing defines a Java® based framework for parsing, generating, transforming, and querying JSON documents.

That is it, the entire spec is this, at least they should say "it is JSR 374, go look there", or better yet copy-paste the entire thing and call it a day.

So, for example, the missing method in your case is getValue(), which should give you a JsonValue object. The description is clear about what should happen if the parser is at the position of an array or an object in the parsed JSON. But there is no clear description of the other cases.

From the Jakarta EE javadoc: https://jakarta.ee/specifications/platform/10/apidocs/jakarta/json/stream/jsonparser#getValue()

default JsonValue getValue()

Returns a JsonValue at the current parser position. 
If the parser state is START_ARRAY, the behavior is the same as getArray(). 
If the parser state is START_OBJECT, the behavior is the same as getObject(). 
For all other cases, if applicable, the JSON value is read and returned.

Returns:
the JsonValue at the current parser position.

Throws:
JsonException - if an i/o error occurs (IOException would be cause of JsonException)
IllegalStateException - when the parser state is END_OBJECT or END_ARRAY
JsonParsingException - if the parser encounters invalid JSON when advancing to next state.
NoSuchElementException - if there are no more parsing states.
Since:
1.1

It clearly says:

For all other cases, if applicable, the JSON value is read and returned.

You can also find in the Jakarta EE javadoc more details about getArrayStream(), getObjectStream() and getValueStream(). I find them a bit problematic, having to decide when to call skip, very likely to lead to mistakes.

In the context of a custom JSONB deserializer, it would be the best if the specification makes clear the parser parameter is a section of the content, only the current element can be parsed, and making the close() and skip methods to be non-operational.