Add support for Record types in JDK 14

[FrauBoes]

Here is an initial version of a record converter.

An additional profile is added to the root pom to only compile the test if JDK 14+ is available.

While @ChrisHegarty and I tried to cover our grounds and follow the XStream conventions, I'm sure there are things we missed or that can be improved. Any suggestions welcome!

[coveralls]

Coverage decreased (-0.9%) to 80.239% when pulling 5a20ac6 on oracle:records into 9d9a5bd on x-stream:master.

[ChrisHegarty]

The code coverage results show a marginal decrease, but that is easily explained. The record converter is only enabled when run on a Java Platform that is version 14 or greater.

I have successfully compiled and tested the changes on Java 8, Java 11, Java 14, and Java 15-ea.

[ChrisHegarty]

Can this PR please be linked to issue #210 - "Support for record types in JDK 14"

[FrauBoes]

Hi @joehni I saw the PR was added to the 1.5 milestone, great. If there is anything further I can help with, let me know.

[joehni]

Hello Julia,

I was busy releasing XStream 1.4.13 last week.

Thanks for your contribution. I had a look at it now, but we cannot accept it in its current form.

Any code of XStream is copyrighted either by Joe Walnes as project founder for the original parts or by the XStream Committers as team. If this is a real donation, we must be able to publish it with XStream Committer's copyright only. We won't add another copyright.

Of course we always give credits in documentation or code (using author tags), e.g. Henrik Ståhl formerly of BEA:

$ find xstream -type f -exec grep Henrik {} +
./xstream-distribution/src/content/team.html:      <li>Henrik St&aring;hl of BEA</li>
./xstream-distribution/src/content/news.html:        <li>Support for BEA JRockit starting with R25.1.0 (contributed by Henrik St&aring;hl of BEA).</li>
./xstream-distribution/src/content/changes.html:                <li>JIRA:XSTR-90 and JIRA:XSTR-311, Support for BEA JRockit starting with R25.1.0 (contributed by Henrik

Either you can remove the Oracle copyright in the pull request or we cannot add this, unfortunately.

Kind regards,
Jörg

[FrauBoes]

@joehni Thanks for reviewing the contribution. Regarding the copyright header, let me look into this and get back to you.

[joehni]

That would be great. I'd love to add this.

[FrauBoes]

Hi @joehni, I'm happy to say that we can move ahead with your suggestion. I've added @author tags and updated the list of contributors. I compiled and tested the changes on Java 8, Java 11, Java 14, and Java 15 with no problem so I believe the failed CI job with jdk 12 is an intermittent failure (the job log suggests so, too). Would it be possible to trigger another run?

[FrauBoes]

Hi Jörg, have you had a chance to look at this? It would be great if we could move this PR forward.

[joehni]

Hi, yes I had a look and the contribution looks fine now. I also restarted Travis for this CI, but it failed now multiple times for Java 8, but that's not a problem, I can deal with it. It's just that I am currently quite busy with other stuff...

[FrauBoes]

Thanks for rerunning the CI jobs. I can't quite make sense of the logs but the failure with Java 8 seems to be environment specific; I can't reproduce it running the same command, it runs successfully on my end.

[FrauBoes]

Hi @joehni, I had a closer look at the CI failure for Java 8. The cause is the maven coveralls plugin (only run with Java 8), which requires a unique serviceJobId. If not provided, this parameter is set based on the supported service environments [1]. I'm not sure what value is assigned exactly in our case. However, setting serviceJobId explicitly to the TRAVIS_JOB_ID env variable seems to solve the problem [2]. The CI builds are all clear now.

[1] https://github.com/trautonen/coveralls-maven-plugin
[2] https://stackoverflow.com/questions/28120891/maven-coveralls-plugin

[FrauBoes]

Now that the CI is clear, is there anything else that needs doing to progress this PR?

[joehni]

It's just that I am quite busy writing a lot CVE reports. Simply higher priority...

[joehni]

Hi Julia,

there's a problem with the JDK support. The patch tries to register the RecoredConverter in XStream.setupConverters() starting with JDK 14. However, this implies that the classes had been built for Java 14, which does not work when using the Java 15 compiler, because it cannot be used to create classes using preview stuff of Java 14.

Same applies now after Java 16 is out. When using Java 16 to build (especially to build later a release of XStream), I cannot compile the RecordConverter for Java 15 or 14, because it uses preview stuff. The preview flag seems only valid targeting the current Java version. If I try to target an older Java version, the Java 16 compiler refuses to build.

So my current solution will be to use Java 16 as target for the new RecordConverter.

Regards,
Jörg

[ChrisHegarty]

@joehni, It is certainly reasonable to support records when running on Java 16+, since Java 16 is generally available since March 16th 2021, and records are a final feature in 16. We can update this PR with that, if that is your preference.

That said, RecordConverter.java has no static dependencies on language features or APIs beyond that of what is in Java 8. When I build this branch, RecordConverter.class has a major version of 52 - the class file version of Java 8. It is possible to enable RecordConverter on any version that is Java 8 or greater, just that there is no real point since record classes must be class file version 58 or greater.

Preview-ness must be enabled to work with records in Java 14 or Java 15 - so the test does that. It would be possible to simplify this test to just run with Java 16+, thus avoiding the need for compile and run with --enable-preview.

[joehni]

Thanks again for the contribution. I compile now the converter itself for Java 8 with the rest of the code base and use Java 15 or higher for the unit test. I will probably drop the support for Records in Java 14 and 15 in future (and get rid of the reflection stuff), because I don't expect people to run these JDKs in preview mode for very long now with Java 16 available.

BTW: I've seen that you register Records as immutable types ... is that really what you want? While these types are technically immutable, it means for XStream that it will not use references if the same object is serialized multiple times. This is useful for elements with single values, but not necessarily for complete structures.

[FrauBoes]

BTW: I've seen that you register Records as immutable types ... is that really what you want? While these types are
technically immutable, it means for XStream that it will not use references if the same object is serialized multiple times. This is useful for elements with single values, but not necessarily for complete structures.

That's a valid observation, in that case we shouldn't register them as immutable types.

Thanks for working with us on this PR, and great to see records supported in XStream.

[toby1984]

While testing our application I came across this issue (XStream unable to serialize record instances) and after some more digging I realized that XStream 1.5.0 is not released yet so I build & tested the current HEAD of the 'master' branch.

Unfortunately de-serialization is not working for me with the provided patch, I keep getting a ClassNotFoundException right at the start of the RecordConverter#unmarshal() method where it's trying to use Class#forName() to resolve the record class.

I was able to fix this by adding the following line to the RecordConverter class:

Note that this change was necessary with both the v1.4.x branch as well as the current HEAD (aa25350, I tried both branches to rule out the converter had some implicit dependency on other changes on the 'master' branch).

Unfortunately I don't have the time to try coming up with a minimal test case (600 kLOC codebase...) and seeing the PR came with IMHO quite comprehensive test coverage, I would be surprised if the issue is actually with the PR. Seems more likely that my XStream initialization (lots of custom converters) is doing something funny.

Just mentioning all of this here in case it actually is an issue with the PR and somebody else stumbles across the same ClassNotFoundException issue I encountered.

[toby1984]

For the record, this is how I create the XStream instance:

private XStream createXStream()
{
    final XStream xstream = new XStream( new StaxDriver() ) {
        @Override
		protected MapperWrapper wrapMapper(MapperWrapper next) {
            return new WicketProxyMapper( new CGLIBMapper( new HibernateMapper(next) ) );
        }
    };

	xstream.addPermission( AnyTypePermission.ANY);

    // use PRIORITY_VERY_HIGH here to override the built-in DynamicProxyConverter that
    // chokes on Apache Wicket proxies
     xstream.registerConverter( new WicketProxyConverter() , XStream.PRIORITY_VERY_HIGH );

    // register Hibernate-specific converters
    xstream.registerConverter(new HibernatePersistentCollectionConverter(xstream.getMapper()) );
    xstream.registerConverter(new HibernatePersistentMapConverter(xstream.getMapper()) );
    xstream.registerConverter(new HibernatePersistentSortedMapConverter(xstream.getMapper()) );
    xstream.registerConverter(new HibernatePersistentSortedSetConverter(xstream.getMapper()) );
    xstream.registerConverter(new HibernateProxyConverter() );

    // register converter for JDK14+ "records" feature
    xstream.registerConverter( new RecordConverter(xstream.getMapper()) );

    // register CGLLib converters
    xstream.registerConverter(new CGLIBEnhancedConverter(xstream.getMapper(), xstream.getReflectionProvider() , getClass().getClassLoader() ) );
    return xstream;
}

The class tripping the exception upon de-serialization looks somewhat like this

public class A {

    public static record X(<multiple fields>) {}
    X someField;

}

and the ClassNotFoundException happens because Class#forName() is only getting passed 'someField' (as reader.getAttribute("class") returns NULL inside RecordCoverter#findClass(HierarchicalStreamReader)) instead of 'A$X'.

[toby1984]

This is the exception I got:

 com.thoughtworks.xstream.converters.ConversionException: Class not found.
---- Debugging information ----
message             : Class not found.
cause-exception     : java.lang.ClassNotFoundException
cause-message       : searchCriteria
class               : com.vodecc.voipmng.boundary.wicket.controlplanemonitor.ControlPlaneMonitorData$SearchCriteria
required-type       : com.vodecc.voipmng.boundary.wicket.controlplanemonitor.ControlPlaneMonitorData$SearchCriteria
converter-type      : com.thoughtworks.xstream.converters.extended.RecordConverter
path                : /com.vodecc.voipmng.boundary.wicket.controlplanemonitor.ControlPlaneMonitorPage/controlPlaneMonitorData/signalingQualityData/searchCriteria
line number         : 1
class[1]            : com.vodecc.voipmng.boundary.wicket.controlplanemonitor.ControlPlaneMonitorData
required-type[1]    : com.vodecc.voipmng.boundary.wicket.controlplanemonitor.ControlPlaneMonitorData
converter-type[1]   : com.thoughtworks.xstream.converters.reflection.ReflectionConverter
class[2]            : com.vodecc.voipmng.boundary.wicket.controlplanemonitor.ControlPlaneMonitorDataBuilder$ControlPlaneMonitorDataData
required-type[2]    : com.vodecc.voipmng.boundary.wicket.controlplanemonitor.ControlPlaneMonitorDataBuilder$ControlPlaneMonitorDataData
class[3]            : com.vodecc.voipmng.boundary.wicket.controlplanemonitor.ControlPlaneMonitorPage
required-type[3]    : com.vodecc.voipmng.boundary.wicket.controlplanemonitor.ControlPlaneMonitorPage
version             : not available
-------------------------------
at com.thoughtworks.xstream.converters.extended.RecordConverter.classForName(RecordConverter.java:224) ~[xstream-1.5.0-SNAPSHOT.jar:1.5.0-SNAPSHOT]
at com.thoughtworks.xstream.converters.extended.RecordConverter.findClass(RecordConverter.java:231) ~[xstream-1.5.0-SNAPSHOT.jar:1.5.0-SNAPSHOT]
at com.thoughtworks.xstream.converters.extended.RecordConverter.unmarshal(RecordConverter.java:156) ~[xstream-1.5.0-SNAPSHOT.jar:1.5.0-SNAPSHOT]
at com.thoughtworks.xstream.core.TreeUnmarshaller.convert(TreeUnmarshaller.java:73) ~[xstream-1.5.0-SNAPSHOT.jar:1.5.0-SNAPSHOT]
at com.thoughtworks.xstream.core.AbstractReferenceUnmarshaller.convert(AbstractReferenceUnmarshaller.java:76) ~[xstream-1.5.0-SNAPSHOT.jar:1.5.0-SNAPSHOT]
at com.thoughtworks.xstream.core.TreeUnmarshaller.convertAnother(TreeUnmarshaller.java:67) ~[xstream-1.5.0-SNAPSHOT.jar:1.5.0-SNAPSHOT]
at com.thoughtworks.xstream.converters.reflection.AbstractReflectionConverter.unmarshallField(AbstractReflectionConverter.java:487) ~[xstream-1.5.0-SNAPSHOT.jar:1.5.0-SNAPSHOT]
at com.thoughtworks.xstream.converters.reflection.AbstractReflectionConverter.doUnmarshal(AbstractReflectionConverter.java:419) ~[xstream-1.5.0-SNAPSHOT.jar:1.5.0-SNAPSHOT]
at com.thoughtworks.xstream.converters.reflection.AbstractReflectionConverter.unmarshal(AbstractReflectionConverter.java:275) ~[xstream-1.5.0-SNAPSHOT.jar:1.5.0-SNAPSHOT]
at com.thoughtworks.xstream.core.TreeUnmarshaller.convert(TreeUnmarshaller.java:73) ~[xstream-1.5.0-SNAPSHOT.jar:1.5.0-SNAPSHOT]
at com.thoughtworks.xstream.core.AbstractReferenceUnmarshaller.convert(AbstractReferenceUnmarshaller.java:76) ~[xstream-1.5.0-SNAPSHOT.jar:1.5.0-SNAPSHOT]
at com.thoughtworks.xstream.core.TreeUnmarshaller.convertAnother(TreeUnmarshaller.java:67) ~[xstream-1.5.0-SNAPSHOT.jar:1.5.0-SNAPSHOT]
at com.thoughtworks.xstream.converters.reflection.AbstractReflectionConverter.unmarshallField(AbstractReflectionConverter.java:487) ~[xstream-1.5.0-SNAPSHOT.jar:1.5.0-SNAPSHOT]
at com.thoughtworks.xstream.converters.reflection.AbstractReflectionConverter.doUnmarshal(AbstractReflectionConverter.java:419) ~[xstream-1.5.0-SNAPSHOT.jar:1.5.0-SNAPSHOT]
at com.thoughtworks.xstream.converters.reflection.AbstractReflectionConverter.unmarshal(AbstractReflectionConverter.java:275) ~[xstream-1.5.0-SNAPSHOT.jar:1.5.0-SNAPSHOT]
at com.thoughtworks.xstream.core.TreeUnmarshaller.convert(TreeUnmarshaller.java:73) ~[xstream-1.5.0-SNAPSHOT.jar:1.5.0-SNAPSHOT]
at com.thoughtworks.xstream.core.AbstractReferenceUnmarshaller.convert(AbstractReferenceUnmarshaller.java:76) ~[xstream-1.5.0-SNAPSHOT.jar:1.5.0-SNAPSHOT]
at com.thoughtworks.xstream.core.TreeUnmarshaller.convertAnother(TreeUnmarshaller.java:67) ~[xstream-1.5.0-SNAPSHOT.jar:1.5.0-SNAPSHOT]
at com.thoughtworks.xstream.converters.reflection.AbstractReflectionConverter.unmarshallField(AbstractReflectionConverter.java:487) ~[xstream-1.5.0-SNAPSHOT.jar:1.5.0-SNAPSHOT]
at com.thoughtworks.xstream.converters.reflection.AbstractReflectionConverter.doUnmarshal(AbstractReflectionConverter.java:419) ~[xstream-1.5.0-SNAPSHOT.jar:1.5.0-SNAPSHOT]
at com.thoughtworks.xstream.converters.reflection.AbstractReflectionConverter.unmarshal(AbstractReflectionConverter.java:275) ~[xstream-1.5.0-SNAPSHOT.jar:1.5.0-SNAPSHOT]
at com.thoughtworks.xstream.core.TreeUnmarshaller.convert(TreeUnmarshaller.java:73) ~[xstream-1.5.0-SNAPSHOT.jar:1.5.0-SNAPSHOT]
at com.thoughtworks.xstream.core.AbstractReferenceUnmarshaller.convert(AbstractReferenceUnmarshaller.java:76) ~[xstream-1.5.0-SNAPSHOT.jar:1.5.0-SNAPSHOT]
at com.thoughtworks.xstream.core.TreeUnmarshaller.convertAnother(TreeUnmarshaller.java:67) ~[xstream-1.5.0-SNAPSHOT.jar:1.5.0-SNAPSHOT]
at com.thoughtworks.xstream.core.TreeUnmarshaller.convertAnother(TreeUnmarshaller.java:51) ~[xstream-1.5.0-SNAPSHOT.jar:1.5.0-SNAPSHOT]
at com.thoughtworks.xstream.core.TreeUnmarshaller.start(TreeUnmarshaller.java:142) ~[xstream-1.5.0-SNAPSHOT.jar:1.5.0-SNAPSHOT]
at com.thoughtworks.xstream.core.AbstractTreeMarshallingStrategy.unmarshal(AbstractTreeMarshallingStrategy.java:34) ~[xstream-1.5.0-SNAPSHOT.jar:1.5.0-SNAPSHOT]
at com.thoughtworks.xstream.XStream.unmarshal(XStream.java:1350) ~[xstream-1.5.0-SNAPSHOT.jar:1.5.0-SNAPSHOT]
at com.thoughtworks.xstream.XStream.unmarshal(XStream.java:1334) ~[xstream-1.5.0-SNAPSHOT.jar:1.5.0-SNAPSHOT]
at com.thoughtworks.xstream.XStream.fromXML(XStream.java:1227) ~[xstream-1.5.0-SNAPSHOT.jar:1.5.0-SNAPSHOT]

[mrt181]

Any plans to release RecordConverter?

[joehni]

It currently supports only the plain object, i.e. it is not possible to alias something, no local converters, no implicit collections, no attribute support, ...