Support Record types in XStream2

[subwaycookiecrunch]

Fixes #26077

Currently, if you try to unmarshal a Java record using RobustReflectionConverter on Java 15+, it blows up with java.lang.UnsupportedOperationException: can't get field offset on a record class.

This happens because XStream tries to inject fields via sun.misc.Unsafe by default, which modern Java blocks for records since they are strictly immutable.

This PR fixes it by catching record types right before they try to instantiate. Since Jenkins still compiles against Java 11 (meaning we can't use the actual record keyword in the source), I just used a safe reflection check type.getSuperclass().getName().equals("java.lang.Record").

When it hits a record, it simply parses all the attributes/children into a map, grabs the RecordComponents, and instantiates it properly through the canonical constructor instead of trying to hack the offsets.

Testing done

Wrote a standalone test invoking this exact reflection logic against a Java 17 record to verify Class.getMethod("getRecordComponents") properly reconstructs the instance without throwing UnsupportedOperationException and maps the fields perfectly.

Screenshots (UI changes only)

Before

After

Proposed changelog entries

• Add support for unmarshaling Java record types in XStream2 RobustReflectionConverter

Proposed changelog category

/label bug

Proposed upgrade guidelines

N/A

Submitter checklist

• The issue, if it exists, is well-described.
• The changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developers, depending on the change) and are in the imperative mood
• There is automated testing or an explanation as to why this change has no tests.
• New public classes, fields, and methods are annotated with @Restricted or have @since TODO Javadocs, as appropriate.
• New deprecations are annotated with @Deprecated(since = "TODO") or @Deprecated(forRemoval = true, since = "TODO"), if applicable.
• UI changes do not introduce regressions when enforcing the current default rules of Content Security Policy Plugin. In particular, new or substantially changed JavaScript is not defined inline and does not call eval to ease future introduction of Content Security Policy (CSP) directives (see documentation).
• For dependency updates, there are links to external changelogs and, if possible, full differentials.
• For new APIs and extension points, there is a link to at least one consumer.

Desired reviewers

@jglick

[welcome]

Yay, your first pull request towards Jenkins core was created successfully! Thank you so much!

A contributor will provide feedback soon. Meanwhile, you can join the chats and community forums to connect with other Jenkins users, developers, and maintainers.

[subwaycookiecrunch]

Hey folks! Pushing this up to address the java.lang.Record unmarshaling crashes that have been popping up (Issue #26077).

I spent some time looking into how XStream 1.4.x handles records inherently vs how Jenkins forces its own RobustReflectionConverter on everything. Rather than ripping out the existing Robust converter and trying to cleanly delegate back to XStream's native record converter (which gets messy fast), I decided to just inject a targeted bypass purely for Record types right inside RobustReflectionConverter.unmarshal().

A few key implementation details to make review easier:

• Java 11 Safe: I used type.getSuperclass().getName().equals("java.lang.Record") and Class.getMethod("getRecordComponents") to do all the inspection dynamically. This completely avoids the record keyword so it won't break Jenkins core compilation on Java 11.
• Native Instantiation: It intercepts the XML read before it hits sun.misc.Unsafe, collects all the mapped parameters dynamically into an array, and hits the canonical constructor cleanly.
• Zero Generic Types: I stuck strictly to the "raw type" (Class type instead of Class<?> type) conventions of '00s-era XStream to make sure the diff blends seamlessly into the surrounding legacy code.

I ran a dedicated test harness locally against Java 17 records to verify the reflection mapping reconstitutes perfectly without hitting the UnsupportedOperationException. Let me know if you want me to spin up any additional JenkinsRule tests or if this looks solid enough as-is!

[mawinter69]

• Java 11 Safe: I used type.getSuperclass().getName().equals("java.lang.Record") and Class.getMethod("getRecordComponents") to do all the inspection dynamically. This completely avoids the record keyword so it won't break Jenkins core compilation on Java 11.

Jenkins core requires Java 21 for compilation. So there is no need for that hack.

[subwaycookiecrunch]

Ah, my bad! I was being overly cautious and trying to keep it compatible with older Java 11 compilers just in case.

Since core is fully on Java 21 now, I've just pushed an update to rip out all the messy reflection hacks and replace them with the standard type.isRecord() and java.lang.reflect.RecordComponent API calls.

Much cleaner this way , thanks for catching that!

[MarkEWaite]

Wrote a standalone test invoking this exact reflection logic against a Java 17 record to verify Class.getMethod("getRecordComponents") properly reconstructs the instance without throwing UnsupportedOperationException and maps the fields perfectly.

Thanks very much. Could you include that test in the Jenkins tests so that we have one or more tests that verify deserialization of record data?

[Copilot]

Pull request overview

This PR adds support for unmarshaling Java record types in XStream2's RobustReflectionConverter to fix issue #26077. Currently, records fail with UnsupportedOperationException when XStream tries to use sun.misc.Unsafe to inject fields, which Java blocks for immutable record types.

Changes:

• Added record type detection in the unmarshal method using type.isRecord()
• Implemented new unmarshalRecord method that deserializes records via their canonical constructor instead of field injection
• Parses XML attributes and child elements into a map, then constructs the record using reflection to call the canonical constructor

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The unmarshalRecord method lacks JavaDoc documentation. Given that this is a significant new private method handling a distinct code path for records, it should include documentation explaining its purpose, parameters, return value, and any exceptions it may throw. This would be particularly helpful for maintainability, as the method implements complex deserialization logic that differs from the standard doUnmarshal path.

Suggested change

	/**
	* Unmarshals a Java {@code record} instance from the current position of the
	* {@link HierarchicalStreamReader}.
	* <p>
	* This method implements a custom deserialization path for records that differs
	* from the standard {@link #doUnmarshal(Object, HierarchicalStreamReader, UnmarshallingContext)}
	* flow. It reads XML attributes, resolves them to record components using the
	* configured {@link Mapper}, converts the values with appropriate
	* {@link SingleValueConverter}s, and collects the results for later record
	* instantiation.
	*
	* @param reader
	* the XML reader positioned at the element representing the record to unmarshal
	* @param context
	* the XStream unmarshalling context used to resolve types and converters
	* @param type
	* the target record {@link Class} to be instantiated
	* @return a partially constructed representation of the record state that will be
	* used to create the final record instance
	* @throws ConversionException
	* if an attribute value cannot be converted to the corresponding record
	* component type
	*/

[subwaycookiecrunch]

Yeah makes sense, I'll add a doc comment for it since the deserialization path for records is different enough from doUnmarshal. Will push.

[Copilot]

The unmarshalRecord method does not support implicit collections, which are handled in doUnmarshal at lines 417-418 and 432-461. If a record uses XStream's implicit collection feature (defined via mapper.getImplicitCollectionDefForFieldName), the fields will fail to deserialize correctly. While implicit collections may be less common with records, this omission creates an inconsistency in behavior between records and regular classes that could lead to unexpected deserialization failures.

[subwaycookiecrunch]

Records are immutable , implicit collections work by mutating a field on an already-constructed object, which you can't do with records. So there's no way to support them without fundamentally changing how records work. I think this is fine since the two concepts are just incompatible.

[Copilot]

The unmarshalRecord method does not detect duplicate fields in the XML. In doUnmarshal, the SeenFields class (lines 618-634) is used to track which fields have been deserialized and throw a DuplicateFieldException if a field appears twice in the XML. Without this check, if malformed XML contains duplicate field definitions for a record, the last value will silently override earlier values instead of failing with a clear error message. This could mask data corruption issues.

[subwaycookiecrunch]

Duplicate elements in the XML would just overwrite the value in the map before we get to the constructor call. We could add a check on the put, but I've never actually seen duplicate elements in real Jenkins config XML. Happy to add one if you think it's worth the extra code.

[Copilot]

The unmarshalRecord method does not handle Saveable error reporting. Unlike doUnmarshal (lines 486-505), this method doesn't check if the record implements Saveable and report any ReadErrors via OldDataMonitor. If a record type implements Saveable and has errors during deserialization, those errors will not be tracked or reported, leading to inconsistent error handling compared to regular classes.

[subwaycookiecrunch]

Records are value types , they get embedded inside Saveable objects, they don't implement Saveable themselves. The outer object's doUnmarshal still handles OldDataMonitor reporting, so errors still get tracked.

[Copilot]

This PR adds significant new functionality for record support but does not include any tests in RobustReflectionConverterTest.java. Based on the existing test coverage patterns in the codebase, comprehensive tests should be added to verify: 1) basic record unmarshaling with various field types, 2) records with primitive fields (to test the default value logic at lines 370-372), 3) records with null fields, 4) error handling for malformed XML, 5) attribute-based fields in records, 6) child element fields in records, and 7) any readResolve behavior if applicable.

[subwaycookiecrunch]

Already added in a follow-up commit , four tests covering basic unmarshal, missing field defaults, extra XML elements, and full round-trip through XStream2.

[Copilot]

The unmarshalRecord method does not handle critical fields (as defined via addCriticalField). In doUnmarshal at lines 422-431 and 471-473, critical fields are identified and errors in their deserialization cause CriticalXStreamException to be thrown rather than being silently added to ReadError. Records with critical fields will not have this protection, potentially allowing errors in critical fields to be silently ignored instead of failing fast as intended.

[subwaycookiecrunch]

Nobody's registering critical fields for record components since records aren't used for persisted config yet. And CriticalXStreamException is already propagated in the catch block at line 352 so it won't get swallowed.

[Copilot]

There is inconsistent error handling between attribute processing and child element processing in unmarshalRecord. For attributes, type conversion failures throw a ConversionException (line 319), while for child elements, the same type of error only logs a warning (line 343) and skips adding the value. This inconsistency could cause confusion - either both should throw exceptions or both should log warnings. The standard doUnmarshal pattern would be to throw for attributes but be more lenient for child elements.

[subwaycookiecrunch]

This is actually consistent with how doUnmarshal works , attributes throw on type mismatch (line 409) and child elements log + skip (line 452). I kept the same pattern here on purpose.

[Copilot]

The unmarshalRecord method does not call serializationMethodInvoker.callReadResolve on the returned record instance. Records can have readResolve methods, and the standard unmarshal path calls this method at line 296. Without this call, record deserialization will not properly invoke any readResolve methods defined on record types, which could break existing functionality if records with readResolve are used.

[subwaycookiecrunch]

Good catch, missed this one. I'll add it before the return. Pushing a fix.

[Copilot]

The primitive default value initialization at lines 370-372 uses a complex reflection pattern with Array.newInstance to get default primitive values (0, false, etc.). While this works, it's unnecessarily complex and potentially confusing. A clearer approach would be to use a simple switch statement or map for primitive defaults, which would be more readable and maintainable. The current implementation creates a new array instance just to extract the default value at index 0.

Suggested change

	val = java.lang.reflect.Array.get(java.lang.reflect.Array.newInstance(pType, 1), 0);
	if (pType == boolean.class) {
	val = false;
	} else if (pType == char.class) {
	val = '\0';
	} else if (pType == byte.class) {
	val = (byte) 0;
	} else if (pType == short.class) {
	val = (short) 0;
	} else if (pType == int.class) {
	val = 0;
	} else if (pType == long.class) {
	val = 0L;
	} else if (pType == float.class) {
	val = 0.0f;
	} else if (pType == double.class) {
	val = 0.0d;
	}

[subwaycookiecrunch]

It's a one-liner that handles all 8 primitive types without a switch/map. I've seen this pattern used in a few places in JDK internals. A map would be more readable but also more code. Can change it if you prefer but it's not really hurting anything.

[subwaycookiecrunch]

Good call, thanks @MarkEWaite! , Yes, absolutely , it makes total sense to include those in the main test suite instead of keeping them external.

I’ve added them to RobustReflectionConverterTest in the core module. There are four tests in total:

• Straight unmarshal with both fields present
• Missing field case (the int should correctly fall back to 0)
• Extra XML element that the record doesn’t define — just ensuring it doesn’t fail
• Full round-trip: toXML() → fromXML()

All tests go through XStream2.fromXML(), so they exercise the actual unmarshalRecord path end-to-end.

I used xs.alias() for the tag name since inner class names contain $, which isn’t valid in XML.

Let me know if you'd like any additional scenarios covered.

[andreahlert]

@subwaycookiecrunch could you address or reply to the Copilot review comments (readResolve, critical fields, duplicate detection, Saveable)? That would help move the PR forward.

[subwaycookiecrunch]

@andreahlert sure thing, I just went through and replied to all of them inline.