Copilot commented on code in PR #430:
URL: https://github.com/apache/commons-beanutils/pull/430#discussion_r3659605759
##########
src/main/java/org/apache/commons/beanutils/DynaProperty.java:
##########
@@ -262,12 +261,14 @@ private Class<?> readAnyClass(final ObjectInputStream in)
throws IOException, Cl
* @throws ClassNotFoundException Class of a serialized object cannot be
found.
*/
private void readObject(final ObjectInputStream in) throws IOException,
ClassNotFoundException {
+ // read default values
+ in.defaultReadObject();
+
+ // read custom values
this.type = readAnyClass(in);
if (isMapped() || isIndexed()) {
this.contentType = readAnyClass(in);
}
- // read other values
- in.defaultReadObject();
}
Review Comment:
Reordering `defaultReadObject()` / `defaultWriteObject()` changes the
on-the-wire serialization format, which will break deserialization of
`DynaProperty` instances serialized by earlier versions (where custom data
preceded the default fields). If backward compatibility is required, consider
(mandatory if compatibility is a goal): (1) bumping `serialVersionUID` to
explicitly signal the incompatibility, and/or (2) making `readObject` tolerant
by attempting the new order first and falling back to the legacy order when
reading older streams (e.g., via a guarded fallback on
`StreamCorruptedException`/`OptionalDataException`).
##########
src/main/java/org/apache/commons/beanutils/DynaProperty.java:
##########
@@ -333,11 +334,13 @@ private void writeAnyClass(final Class<?> clazz, final
ObjectOutputStream out) t
* @throws IOException if I/O errors occur while writing to the underlying
stream.
*/
private void writeObject(final ObjectOutputStream out) throws IOException {
- writeAnyClass(this.type,out);
+ // write out default
+ out.defaultWriteObject();
+
+ // write custom values
+ writeAnyClass(this.type, out);
if (isMapped() || isIndexed()) {
- writeAnyClass(this.contentType,out);
+ writeAnyClass(this.contentType, out);
}
- // write out other values
- out.defaultWriteObject();
}
Review Comment:
Reordering `defaultReadObject()` / `defaultWriteObject()` changes the
on-the-wire serialization format, which will break deserialization of
`DynaProperty` instances serialized by earlier versions (where custom data
preceded the default fields). If backward compatibility is required, consider
(mandatory if compatibility is a goal): (1) bumping `serialVersionUID` to
explicitly signal the incompatibility, and/or (2) making `readObject` tolerant
by attempting the new order first and falling back to the legacy order when
reading older streams (e.g., via a guarded fallback on
`StreamCorruptedException`/`OptionalDataException`).
##########
src/test/java/org/apache/commons/beanutils/DynaPropertyTest.java:
##########
@@ -96,4 +105,33 @@ public void testHashCode() {
assertEquals(testPropertyWithNameAndTypeAndContentType.hashCode(),
testProperty3Duplicate.hashCode());
assertEquals(initialHashCode,
testPropertyWithNameAndTypeAndContentType.hashCode());
}
+
+ /**
+ * Tests basic serialization and deserialization mechanism.
+ */
+ public void testSerialization() throws Exception {
+ ByteArrayOutputStream buffer = new ByteArrayOutputStream();
+ ObjectOutputStream oos = new ObjectOutputStream(buffer);
+ oos.writeObject(testPropertyWithNameAndTypeAndContentType);
+ oos.flush();
+ oos.close();
+
+ ObjectInputStream ois = new ObjectInputStream(new
ByteArrayInputStream(buffer.toByteArray()));
+ Object obj = ois.readObject();
+
+ assertEquals(testPropertyWithNameAndTypeAndContentType, obj);
+ }
Review Comment:
This test manually manages stream lifecycles and never closes
`ObjectInputStream` (`ois`). Prefer try-with-resources for both
`ObjectOutputStream` and `ObjectInputStream` so resources are reliably closed
even when assertions fail or `readObject()` throws.
##########
pom.xml:
##########
@@ -103,6 +103,12 @@
<classifier>tests</classifier>
<scope>test</scope>
</dependency>
+ <dependency>
+ <groupId>org.wildfly</groupId>
+ <artifactId>wildfly-ejb3</artifactId>
+ <version>9.0.2.Final</version>
+ <scope>test</scope>
+ </dependency>
Review Comment:
Pulling in `wildfly-ejb3` (even test-scoped) is a large dependency surface
for a narrow need (the `org.jboss.marshalling.cloner.*` APIs). To reduce build
time, transitive dependency risk, and improve long-term maintenance, consider
depending directly on the minimal JBoss Marshalling artifact that provides the
cloner classes (instead of the full WildFly EJB module).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
