Skip to content

Support Record types in XStream2 #26354

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
subwaycookiecrunch wants to merge 7 commits into
base: master
Choose a base branch
from
+249 −50
Open

Support Record types in XStream2 #26354

Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f03431e
Fix Jenkins Issue 26077 - Support Record types in XStream2
Feb 22, 2026
5325fdc
Merge branch 'master' into fix-record-xstream2
subwaycookiecrunch Feb 23, 2026
4f41053
Add record deserialization tests to RobustReflectionConverterTest
Feb 24, 2026
963ea33
Retrigger CI
Feb 24, 2026
7e4426e
Merge branch 'master' into fix-record-xstream2
subwaycookiecrunch Feb 25, 2026
00ec81d
Retrigger CI
Feb 25, 2026
1e1ecf2
Add JavaDoc to unmarshalRecord and fix missing readResolve call
Mar 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
90 changes: 90 additions & 0 deletions core/src/main/java/hudson/util/RobustReflectionConverter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,11 +285,101 @@

@Override
public Object unmarshal(final HierarchicalStreamReader reader, final UnmarshallingContext context) {
String readResolveValue = reader.getAttribute(mapper.aliasForAttribute("resolves-to"));
Class type = readResolveValue != null ? mapper.realClass(readResolveValue) : context.getRequiredType();
if (type != null && type.isRecord()) {

Check warning on line 290 in core/src/main/java/hudson/util/RobustReflectionConverter.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Partially covered line 

Line 290 is only partially covered, 2 branches are missing
return unmarshalRecord(reader, context, type);
}

Object result = instantiateNewInstance(reader, context);
result = doUnmarshal(result, reader, context);
return serializationMethodInvoker.callReadResolve(result);
}

Copy link

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
*/

Copilot uses AI. Check for mistakes.
Copy link
Author

@subwaycookiecrunch subwaycookiecrunch Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

private Object unmarshalRecord(final HierarchicalStreamReader reader, final UnmarshallingContext context, Class type) {
Map<String, Object> values = new HashMap<>();

Iterator it = reader.getAttributeNames();
while (it.hasNext()) {
String attrAlias = (String) it.next();
String attrName = mapper.attributeForAlias(attrAlias);
Field field = reflectionProvider.getFieldOrNull(type, attrName);
if (field != null) {
SingleValueConverter converter = mapper.getConverterFromAttribute(field.getDeclaringClass(), attrName, field.getType());
Class fieldType = field.getType();
if (converter == null) {
converter = mapper.getConverterFromItemType(fieldType);
}
if (converter != null) {
Object value = converter.fromString(reader.getAttribute(attrAlias));
if (fieldType.isPrimitive()) {
fieldType = Primitives.box(fieldType);
}
if (value != null && !fieldType.isAssignableFrom(value.getClass())) {
throw new ConversionException("Cannot convert type " + value.getClass().getName() + " to type " + fieldType.getName());
}
values.put(attrName, value);
}
}
}

while (reader.hasMoreChildren()) {
reader.moveDown();
try {
String fieldName = mapper.realMember(type, reader.getNodeName());
Field field = reflectionProvider.getFieldOrNull(type, fieldName);
if (field != null) {
Class fieldType = field.getType();
Class xmlType = mapper.defaultImplementationOf(fieldType);
String classAttribute = reader.getAttribute(mapper.aliasForAttribute("class"));
if (classAttribute != null) {
Class specifiedType = mapper.realClass(classAttribute);
if (fieldType.isAssignableFrom(specifiedType)) {
xmlType = specifiedType;
}
}
Object value = unmarshalField(context, null, xmlType, field);
if (value != null && !xmlType.isAssignableFrom(value.getClass())) {
LOGGER.warning("Cannot convert type " + value.getClass().getName() + " to type " + xmlType.getName());
} else {
values.put(fieldName, value);
}
Comment on lines +347 to +377
Copy link

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Copy link
Author

@subwaycookiecrunch subwaycookiecrunch Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

} else {
Class itemType = mapper.getItemTypeForItemFieldName(type, fieldName);
Class xmlType = itemType != null ? itemType : mapper.realClass(reader.getNodeName());
context.convertAnother(null, xmlType);
}
} catch (CriticalXStreamException | InputManipulationException e) {
throw e;
} catch (XStreamException | LinkageError e) {
addErrorInContext(context, e);
}
reader.moveUp();
}
Comment on lines +356 to +389
Copy link

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Copy link
Author

@subwaycookiecrunch subwaycookiecrunch Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Copy link
Author

@subwaycookiecrunch subwaycookiecrunch Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +356 to +389
Copy link

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Copy link
Author

@subwaycookiecrunch subwaycookiecrunch Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


try {
java.lang.reflect.RecordComponent[] components = type.getRecordComponents();
Class[] parameterTypes = new Class[components.length];
Object[] args = new Object[components.length];
for (int i = 0; i < components.length; i++) {
java.lang.reflect.RecordComponent component = components[i];
Class pType = component.getType();
String name = component.getName();
parameterTypes[i] = pType;
Object val = values.get(name);
if (val == null && pType.isPrimitive()) {
val = java.lang.reflect.Array.get(java.lang.reflect.Array.newInstance(pType, 1), 0);
Copy link

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
}

Copilot uses AI. Check for mistakes.
Copy link
Author

@subwaycookiecrunch subwaycookiecrunch Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
args[i] = val;
}
java.lang.reflect.Constructor constructor = type.getDeclaredConstructor(parameterTypes);
constructor.setAccessible(true);
return constructor.newInstance(args);
} catch (Exception e) {
throw new ConversionException("Failed to instantiate record " + type.getName(), e);

Check warning on line 379 in core/src/main/java/hudson/util/RobustReflectionConverter.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered lines 

Lines 291-379 are not covered by tests
}
}
Copy link

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Copy link
Author

@subwaycookiecrunch subwaycookiecrunch Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Copy link
Author

@subwaycookiecrunch subwaycookiecrunch Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Copy link
Author

@subwaycookiecrunch subwaycookiecrunch Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


public Object doUnmarshal(final Object result, final HierarchicalStreamReader reader, final UnmarshallingContext context) {
final SeenFields seenFields = new SeenFields();
Iterator it = reader.getAttributeNames();
Expand Down
Loading