HHH-19964 Allow json serialization of Object valued attribute

[jrenaat]

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license
and can be relicensed under the terms of the LGPL v2.1 license in the future at the maintainers' discretion.
For more information on licensing, please check here.

---

https://hibernate.atlassian.net/browse/HHH-19964

[jrenaat]

@beikov removing the Object type check on the FormatMapper toString/fromString methods makes it work, but with some noteworthy restrictions:
1/ it only works for jackson, jsonb serializes the object to an empty json string {}
2/ the roundtrip from the database comes with its own particular quirk: the db json string makes it effectively impossible to distinguish between anything other than Maps and Lists, i.e. Sets and Queues get serialized back to List, and Dictionary, to Map
So, do we want to, or can we even, improve this such that we can pass the correct type to jackson for deserialization or, given these restrictions, would it be wiser to disallow this kind of mapping?

[beikov]

1/ it only works for jackson, jsonb serializes the object to an empty json string {}

I'd say this is fine. What will happen during serialization/deserialization is up to the FormatMapper, so differences between implementations are to be expected.

2/ the roundtrip from the database comes with its own particular quirk: the db json string makes it effectively impossible to distinguish between anything other than Maps and Lists, i.e. Sets and Queues get serialized back to List, and Dictionary, to Map

That is also expected. JSON obviously only knows "object" and "array" and it is up to the configured ObjectMapper to decide how to serialize/deserialize data. Since serialization of anything that is an instance of Collection/Iterable is probable going to end up as JSON array, the natural deserialization is a List. Instances of type Map are serialized as JSON object, and deserialization to Object can be anything, from Map to JsonNode, but the details are up to the ObjectMapper configuration.

So, do we want to, or can we even, improve this such that we can pass the correct type to jackson for deserialization or, given these restrictions, would it be wiser to disallow this kind of mapping?

I don't think we can improve anything, nor should we. These "restrictions" are fine IMO and simply a result of the fact that a user chose to statically type Object rather than anything else. Similar "quirks" apply when users serialize a Map<String, Object> when they put values of type Long/Integer/Short/Byte or similar and it deserializes to some other Number type. If you want, you can add a warning to the documentation about these sharp edges, but who knows if people that run into such trouble will even read the documentation..

[beikov]

Suggested change

	if ( type == String.class ) {
	if ( value instanceof String ) {

[beikov]

I don't understand this change. Are you saying that the JavaType for this payload reported Object.class? Because that would seem somewhat wrong to me, given that deserialization happens when we know about the concrete entity type and can resolve the type variable. Maybe there is another bug hiding here and the check for type == String.class would be more correct.

[jrenaat]

You are likely correct; so, in both versions of this test (i.e. the original, and my changed one) inside to/fromString the reported javaType.getJavaType() is Object. However, with value instanceof String indeed it just casts to String, whereas with value == String.class it serializes with jackson, leading in the former case to a ClasscastExceltion at String payload1 = a.getPayload(); and I (wrongly) assumed I needed to adapt the test.

[beikov]

Ok, I think we're hitting https://hibernate.atlassian.net/browse/HHH-18572 now. I debugged a bit and see that the generic property receives a type resolution containing the type bound of the type variable, which isn't wrong, but leads to this problem.
I have to think about https://hibernate.atlassian.net/browse/HHH-18572 and play around with a potential fix before we can continue with this PR, because changing this test is IMO wrong.

[jrenaat]

Waiting for resolution of https://hibernate.atlassian.net/browse/HHH-18572 to move this forward