Allow (de)serializing records using Bean(De)SerializerModifier even when reflection is unavailable

[yawkat]

First of all, apologies for not reporting this issue separately before making a PR. I had to write the patch anyway for internal bureaucracy reasons, so I couldn't wait for more feedback on whether this is the right approach to fix this issue.

---

With graal native image, reflection information for records is opt-in. When it is missing, jackson will report a bad definition:

Failures (1):
  JUnit Jupiter:RecordTest:test()
    MethodSource [className = 'RecordTest', methodName = 'test', methodParameterTypes = '']
    => com.fasterxml.jackson.databind.exc.InvalidDefinitionException: Failed to access RecordComponents of type `Rec`
       com.fasterxml.jackson.databind.SerializerProvider.reportBadDefinition(SerializerProvider.java:1300)
       com.fasterxml.jackson.databind.SerializerProvider._createAndCacheUntypedSerializer(SerializerProvider.java:1447)
       com.fasterxml.jackson.databind.SerializerProvider.findValueSerializer(SerializerProvider.java:544)
       com.fasterxml.jackson.databind.SerializerProvider.findTypedValueSerializer(SerializerProvider.java:822)
       com.fasterxml.jackson.databind.ser.DefaultSerializerProvider.serializeValue(DefaultSerializerProvider.java:308)
       com.fasterxml.jackson.databind.ObjectMapper._writeValueAndClose(ObjectMapper.java:4568)
       com.fasterxml.jackson.databind.ObjectMapper.writeValueAsString(ObjectMapper.java:3821)
       RecordTest.test(RecordTest.java:41)
       java.lang.reflect.Method.invoke(Method.java:568)
       org.junit.platform.commons.util.ReflectionUtils.invokeMethod(ReflectionUtils.java:725)
       [...]

(similar for deserialization)

This is usually fine, but when the ObjectMapper is configured with a BeanDeserializerModifier and BeanSerializerModifier that implement access to the record components on their own, this is an issue. The error will happen even if the modifiers could (de)serialize a record. A very simple test case is available here, just run ./gradlew nativeTest: https://github.com/yawkat/graal-record-jackson/blob/master/src/test/java/RecordTest.java

---

This PR changes JDK14Util recordComponents to instead return null if the call fails due to a graal native image error. It compares the error class, which is unfortunately the only way to do this right now, according to the graal team at oracle. Downstream users of this class are also changed to account for this.

However I see two issues with this PR. First, the errors people get when they don't use a modifier are now worse. For serialization:

Failures (1):
  JUnit Jupiter:RecordTest:test()
    MethodSource [className = 'RecordTest', methodName = 'test', methodParameterTypes = '']
    => org.opentest4j.AssertionFailedError: expected: <{"foo":"bar"}> but was: <{}>
       org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:55)
       org.junit.jupiter.api.AssertionUtils.failNotEqual(AssertionUtils.java:62)
       org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:182)
       org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:177)
       org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:1141)
       RecordTest.test(RecordTest.java:41)
       java.lang.reflect.Method.invoke(Method.java:568)
       org.junit.platform.commons.util.ReflectionUtils.invokeMethod(ReflectionUtils.java:725)
       org.junit.jupiter.engine.execution.MethodInvocation.proceed(MethodInvocation.java:60)
       org.junit.jupiter.engine.execution.InvocationInterceptorChain$ValidatingInvocation.proceed(InvocationInterceptorChain.java:131)
       [...]

For deserialization:

Failures (1):
  JUnit Jupiter:RecordTest:test()
    MethodSource [className = 'RecordTest', methodName = 'test', methodParameterTypes = '']
    => com.fasterxml.jackson.databind.exc.InvalidDefinitionException: Cannot construct instance of `Rec` (no Creators, like default constructor, exist): cannot deserialize from Object value (no delegate- or property-based Creator)
 at [Source: (String)"{"foo":"bar"}"; line: 1, column: 2]
       com.fasterxml.jackson.databind.DeserializationContext.reportBadDefinition(DeserializationContext.java:1904)
       com.fasterxml.jackson.databind.DatabindContext.reportBadDefinition(DatabindContext.java:400)
       com.fasterxml.jackson.databind.DeserializationContext.handleMissingInstantiator(DeserializationContext.java:1349)
       com.fasterxml.jackson.databind.deser.BeanDeserializerBase.deserializeFromObjectUsingNonDefault(BeanDeserializerBase.java:1415)
       com.fasterxml.jackson.databind.deser.BeanDeserializer.deserializeFromObject(BeanDeserializer.java:351)
       com.fasterxml.jackson.databind.deser.BeanDeserializer.deserialize(BeanDeserializer.java:184)
       com.fasterxml.jackson.databind.deser.DefaultDeserializationContext.readRootValue(DefaultDeserializationContext.java:322)
       com.fasterxml.jackson.databind.ObjectMapper._readMapAndClose(ObjectMapper.java:4674)
       com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:3629)
       com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:3597)
       [...]

iirc this is consistent with what happens with non-record beans when reflection information is unavailable. But the failure mode is certainly more confusing than it is without this patch, which may lead to users not realizing they have to enable reflection for the record classes.

The second issue is testing this. I don't see a sensible way to add native image tests to jackson-databind, so this patch does not have a unit test. This also means it's very easy to introduce a regression here, should this code be touched again.

[cowtowncoder]

Ok. Hmmh. I really need to think this through.

On plus side I would love to help things work better on Graal/native. On downside, problems you listed are non-trivial... I think exception message improvements could make me more positive about PR. But I realize that if one blocks exceptions there may not be good way to signal (likely) nature of the problem through the call stack.

[yawkat]

I will spend some time tomorrow to take a stab at general graal error message improvements. While this PR makes the messages worse ofc, this is already the status quo for non-record classes – maybe it's possible to improve error handling for all classes that have reflection info missing, and solve the messages for records while I'm at it.

[yawkat]

@cowtowncoder take a look at the changes i added. Now, the error messages have additional information when running in native image mode and when no reflection data is detected. I've also amended https://github.com/yawkat/graal-record-jackson/blob/master/src/test/java/RecordTest.java with tests for this (still don't know how to get these tests into this PR)

[yawkat]

Oh and I added a dependency to graal-sdk to get the proper information. It is <scope>provided</scope> and the code works without it present (ie on a normal jvm). LMK if this is still an issue and I will make a reflection-based version instead.

[cowtowncoder]

Add a brief comment referring to this PR, purpose.

[cowtowncoder]

Ok, so, ideally it should be fine to have provided dependency this way, but I think some tools do get heartburn. So, I think that further encapsulation of this aspect should be done so that:

1. Class that does access Graal-VM stuff is separate (extract from within that Jdk14Util class)
2. Extracted class could use reflection for accessing functionality, if not too cumbersome -- but this is not a hard requirement. As long as any and all refs to graalvm helper library are from this separate class, attempt to load of which only occurs from Jdk14Util (which itself should only be called when dealing with Records -- so should be avoided on, say, Android)
3. But it is OK to depend on that library during compilation time (as provided), so can use constants or whatever, within extracted class

Basically I'd just want to minimize warnings on various platforms in case of missing functionality.

Does this make sense? I think we can avoid even attempt to trigger class loading on pre-Record platforms (android), to keep logs clean. :)

[yawkat]

@cowtowncoder Well, I asked the substratevm team at oracle, and it is not necessary after all to depend on graal-sdk. I've changed the code to depend on System.getProperty instead. This is also supported (the property name and value are guaranteed not to be changed in future graal versions).

Does this work for you?

[yawkat]

@cowtowncoder can you take another look at this please

[cowtowncoder]

Hi @yawkat -- apologies about slowness on my part; will try to review later today; adding to my TODO list.

[cowtowncoder]

Looks good as far as I can see, will mege.

[yawkat]

Thanks!