A @QuarkusTest with a ParameterResolver that return a java record, throws an exception

[mmaggioni-wise]

Describe the bug

During a QuarkuTest, when using a junit "ParameterResolver" that return a java record with at least one field, xstream return this error:

com.thoughtworks.xstream.converters.ConversionException: 
---- Debugging information ----
cause-exception     : java.lang.UnsupportedOperationException
cause-message       : can't get field offset on a record class: private final java.lang.String com.example.TestRecord.test
class               : com.example.TestRecord
required-type       : com.example.TestRecord
converter-type      : com.thoughtworks.xstream.converters.reflection.ReflectionConverter
path                : /com.example.TestRecord/test
line number         : 2
version             : 1.4.15
-------------------------------

The problem in quarkus is at
io.quarkus.test.junit.internal.XStreamDeepClone.doClone(XStreamDeepClone.java:49)

As far as I understand, this class use xstream to just create a copy of the object (the java record) in the Quarkus ClassLoader.
If it's the only usage, i think we can use another library that support records (and maybe use json instead of XML).

what do you think?

The cause is:

java.lang.UnsupportedOperationException: can't get field offset on a record class: private final java.lang.String com.example.TestRecord.test
	at jdk.unsupported/sun.misc.Unsafe.objectFieldOffset(Unsafe.java:648)
	at com.thoughtworks.xstream.converters.reflection.SunUnsafeReflectionProvider.getFieldOffset(SunUnsafeReflectionProvider.java:105)

To Reproduce

It's very simple.

1- Create a Java record, with at least one field record TestRecord(String test)

2- Create a ParameterResolver that return the record new TestRecord("my test")

3- Create a quarkus test that use that ParameterResolver
eg:

@QuarkusTest
@ExtendWith(TestRecordResolver.class)
public class QuarkusRecordTest {
  @Test
  public void test(TestRecord record) {
    assert record != null;
  }

}

Environment:

Output of uname -a or ver

Darwin MacBookPro.lan 20.3.0 Darwin Kernel Version 20.3.0: Thu Jan 21 00:06:51 PST 2021; root:xnu-7195.81.3~1/RELEASE_ARM64_T8101 arm64

Output of java -version

openjdk 16 2021-03-16
OpenJDK Runtime Environment AdoptOpenJDK (build 16+36)
OpenJDK 64-Bit Server VM AdoptOpenJDK (build 16+36, mixed mode, sharing)

Quarkus version or git rev

Quarkus 1.12.2.Final

Build tool (ie. output of mvnw --version or gradlew --version)

Maven 3.6.3

Additional context

I reproduced it in a new empty project.
I can post it, if need, but it's super simple.

This occurs from java 15, for this reason
https://mail.openjdk.java.net/pipermail/amber-dev/2020-June/006241.html
https://medium.com/@atdsqdjyfkyziqkezu/java-15-breaks-deserialization-of-records-902fcc81253d

Complete stacktrace.txt

[famod]

Thanks for reporting this.

Upstream issue: x-stream/xstream#210

@mmaggioni-wise

i think we can use another library that support records (and maybe use json instead of XML)

Do you have something concrete in mind?

@geoand WDYT? XStream 1.5 is still far away, AFAICS.

[geoand]

We could try and replace xstream with java serialization and see how far that would get us.

[famod]

This would require implements Serializable which is kind of a breaking change.
Maybe we should add java serialization as an alternative option that can somehow be selected?

[geoand]

Maybe it could be engaged if the class is serializable?

[famod]

Yeah, why not! I don't see why someone would want xstream serialization when the class in question declares to be java serialization compatible.

[mmaggioni-wise]

Do you have something concrete in mind?

with this code it seems to works.
we use Johnzon instead of Yasson because it supports java records.
this is a quick replace of the XStream class, with a custom one that use jsonb.

  public String toXML(Object objectToClone) {
    return jsonb.toJson(objectToClone);
  }

  public Object fromXML(String serialized) {
     return jsonb.fromJson(serialized, classLoader.loadClass("com.example.TestRecord"));
  }

The class XStreamDeepClone seems to have everything we you need, it has the classloader and the object to copy.

I don t think forcing people to use the Serializable interface for all their records it's the best idea.
It seems that the java team wants to reworks how Serialization works

this is a quote from Mark Reinhold:

Until Oracle get rid of serialization, Reinhold said, those who use it would be well advised to use it only when they know where the data streams are coming from. He said users would be better off using JSON or XML.

edit:
i realized that maybe my code wasn t clear:

i think that the class XStreamDeepClone can be replaced with something like this:

private Object doClone(Object objectToClone) throws ClassNotFoundException {
    var jsonb = new JohnzonBuilder().build(); // it can be created only once in the constructor
    String serialized = jsonb.toJson(objectToClone);
    Object result = jsonb.fromJson(serialized, classLoader.loadClass(objectToClone.getClass().getName()));
    if (result == null) {
      throw new IllegalStateException("Unable to deep clone object of type '" + objectToClone.getClass().getName() + "'. Please report the issue on the Quarkus issue tracker.");
    } else {
      return result;
    }
  }

[famod]

@mmaggioni-wise Thanks a lot! Btw, pull requests are more than welcome! 😉

@geoand I've been thinking we could introduce Apache Johnzon, use that primarily, but fall back to XStream in case of errors together with logging a warning, asking the user to report the respective exception.
We could then kick out XStream in 1.15 or 1.16, after we have more confidence with Johnzon. WDYT?

[geoand]

Yeah, we can do certainly try that - there might be a problem though...
Having Johnzon on the test classpath might cause problems in the JSON-B resolution process when Yasson (which we use as our JSON-B impl of choice) is also present...

[mmaggioni-wise]

I did some other test, the problem with johnzon is that it require the class to have setters, a default constructor, and it doesn't work very well with collections.
The problem is that if you have a List of objects, the class type is a generic list so you will lose the object type, and when you try to convert it back to object it will result in a List of Map.
I guess with some magic this can be fixed, but it will require some work, and we still have the problem of the constructor and setters.

I tried with Gson and Jackson but they do not support records yet.

It seems that in xstream something is moving.

I guess that records are very new, so it will take some time to libraries to upgrade.

[famod]

After all that new info I'd say we should add java serialization support as a short term remedy and keep track of what is happening upstream.
Adding Serializable to your records seems like the lesser evil than anything else, until we have something better...

[stuartwdouglas]

We should look at adding some hooks into JUnit so we no longer require this copying.

[geoand]

That would definitely be the best thing to do

[stuartwdouglas]

junit-team/junit5#2579

[geoand]

Subscribed

[famod]

+💯 for the junit enhancement, but until that is available?

[famod]

Update: With #16302 now merged, Quarkus 2.0 will try plain java serialization first, falling back to XStream on error.

Unless that JUnit hook or XStream 1.5 becomes available before Quarkus 2.0 is released, you will need to have your records implement Serializable (and everything they reference).
This is only a workaround, which is why I'd leave this issue open. Your decision @geoand & @stuartwdouglas.

[geoand]

Let's keep it open for now

[edeandrea]

Hey folks! Just wanted to see if there has been any movement on this? I'm still experiencing this with Quarkus 3.5.0. I don't want to have my records implement Serializable

[magicDGS]

Same issue as @edeandrea here, as I don't need my records to be serializable. Testing right now with quarkus platform 3.10.0; test running properly without @QuarkusTest annotation.

[dmlloyd]

I am testing a change which uses JBoss Marshalling's object cloner instead of XStream and serialization. I'm not 100% sure it's going to work but so far it looks good (and maybe faster as well).

If it does work, then it should be able to transport records correctly, and other classes regardless of whether they are Serializable.

[geoand]

I am 100% sure it's better than what we have now :).

I'm actually ashamed of myself for not thinking of JBoss Marshalling...

[magicDGS]

Any reason that this was reverted in 3.12.*? Should this be re-open?

[geoand]

The fix is already in main and should be part of 3.13