[BEANUTILS-417] Write DynaProperty default fields first while reading legacy streams (1.X)

pom.xml

@@ -103,6 +103,12 @@
       <classifier>tests</classifier>
       <scope>test</scope>
     </dependency>
+    <dependency>
+      <groupId>org.jboss.marshalling</groupId>
+      <artifactId>jboss-marshalling</artifactId>
+      <version>2.0.12.Final</version>
+      <scope>test</scope>
+    </dependency>
     <dependency>
       <groupId>org.junit.vintage</groupId>
       <artifactId>junit-vintage-engine</artifactId>

src/changes/changes.xml

@@ -30,6 +30,7 @@
   <body>
     <release version="1.11.1" date="YYYY-MM-DD" description="This is a maintenance release and requires Java 8.">
       <!-- FIX -->
+      <action issue="BEANUTILS-417" type="fix" due-to="agamal026">Write DynaProperty default fields before custom metadata while retaining support for reading historical serialized data.</action>
       <action dev="ggregory" type="fix" due-to="Gary Gregory">Fix typo in exception message throws from org.apache.commons.beanutils.PropertyUtilsBean.setMappedProperty(Object, String, String, Object).</action>
       <action dev="ggregory" type="fix" due-to="Basil Crow, Piotr P. Karwasz, Gary Gregory">Simplify FluentPropertyBeanIntrospector#propertyName (#363).</action>
       <action dev="ggregory" type="fix" due-to="Gary Gregory">Fix malformed Javadoc comments.</action>

src/main/java/org/apache/commons/beanutils/DynaProperty.java

@@ -35,9 +35,13 @@
  * Alternatively, an implementatin may choose to ignore this property.
  * All keys for maps must be of type String so no meta data is needed for map keys.</p>
  *
+ * <p>Since 1.11.1, serialization writes default fields before custom class metadata.
+ * Deserialization also accepts the historical format, which wrote these values in
+ * the opposite order. The new format is not intended for readers from earlier releases.</p>
+ *
  */
 
-public class DynaProperty implements Serializable {
+public class DynaProperty extends DynaPropertySerialization implements Serializable {
 
     private static final long serialVersionUID = 1L;
     /*
@@ -262,12 +266,16 @@ 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 {
+        if (!isLegacyStream()) {
+            in.defaultReadObject();
+        }
         this.type = readAnyClass(in);
         if (isMapped() || isIndexed()) {
             this.contentType = readAnyClass(in);
         }
-        // read other values
-        in.defaultReadObject();
+        if (isLegacyStream()) {
+            in.defaultReadObject();
+        }
     }
 
     /**
@@ -330,14 +338,14 @@ private void writeAnyClass(final Class<?> clazz, final ObjectOutputStream out) t
      * workaround.
      *
      * @param out Where to write.
+     * @serialData Default fields, followed by the property type and, for indexed or mapped properties, the content type.
      * @throws IOException Thrown if I/O errors occur while writing to the underlying stream.
      */
     private void writeObject(final ObjectOutputStream out) throws IOException {
+        out.defaultWriteObject();
         writeAnyClass(this.type,out);
         if (isMapped() || isIndexed()) {
             writeAnyClass(this.contentType,out);
         }
-        // write out other values
-        out.defaultWriteObject();
     }
 }

src/main/java/org/apache/commons/beanutils/DynaPropertySerialization.java

@@ -0,0 +1,48 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.commons.beanutils;
+
+import java.io.Serializable;
+
+/**
+ * Distinguishes the historical {@link DynaProperty} serialized form before its data is read.
+ *
+ * <p>Historical streams do not contain this superclass, so deserialization invokes
+ * {@link #readObjectNoData()} before restoring {@code DynaProperty}. New streams include
+ * this empty serializable superclass and leave the transient flag false. This permits
+ * either field ordering to be read without consuming or rewinding the caller's stream.</p>
+ *
+ * <p>This class's name, serial version UID, empty persistent state and position in the
+ * hierarchy are part of the serialized format and must be retained in future releases.</p>
+ *
+ * @since 1.11.1
+ */
+class DynaPropertySerialization implements Serializable {
+
+    private static final long serialVersionUID = 1L;
+
+    private transient boolean legacyStream;
+
+    final boolean isLegacyStream() {
+        return legacyStream;
+    }
+
+    private void readObjectNoData() {
+        legacyStream = true;
+    }
+}

src/test/java/org/apache/commons/beanutils/DynaPropertySerializationTest.java

@@ -36,8 +36,15 @@
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.stream.Stream;
 
+import org.jboss.marshalling.cloner.ClassLoaderClassCloner;
+import org.jboss.marshalling.cloner.ClonerConfiguration;
+import org.jboss.marshalling.cloner.ObjectCloner;
+import org.jboss.marshalling.cloner.ObjectCloners;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.MethodSource;
 
 /**
  * Comprehensive serialization tests for {@link DynaProperty#readObject(ObjectInputStream)}.
@@ -50,6 +57,30 @@
  */
 class DynaPropertySerializationTest {
 
+    private static final class ExtendedProperty extends DynaProperty {
+        private static final long serialVersionUID = 1L;
+
+        private final int extraValue;
+
+        private ExtendedProperty(final int extraValue) {
+            super("extended", List.class, String.class);
+            this.extraValue = extraValue;
+        }
+    }
+
+    private static Stream<DynaProperty> clonerProperties() {
+        return Stream.of(new DynaProperty("object", String.class), new DynaProperty("primitive", Integer.TYPE),
+                new DynaProperty("array", String[].class), new DynaProperty("primitiveArray", int[].class),
+                new DynaProperty("list", List.class, String.class), new DynaProperty("map", Map.class, Integer.TYPE),
+                new DynaProperty("rawMap", Map.class), new DynaProperty("nullType", null));
+    }
+
+    private static ObjectCloner newSerializingCloner() {
+        final ClonerConfiguration configuration = new ClonerConfiguration();
+        configuration.setClassCloner(new ClassLoaderClassCloner(DynaPropertySerializationTest.class.getClassLoader()));
+        return ObjectCloners.getSerializingObjectClonerFactory().createCloner(configuration);
+    }
+
     /**
      * Assert that a round-tripped {@link DynaProperty} is equal to the original and is a distinct object.
      */
@@ -342,6 +373,40 @@ void testSerializationIsIdempotent() throws Exception {
         assertEquals(original, copy2);
     }
 
+    @ParameterizedTest
+    @MethodSource("clonerProperties")
+    void testSerializingObjectCloner(final DynaProperty original) throws Exception {
+        final DynaProperty copy = assertInstanceOf(DynaProperty.class, newSerializingCloner().clone(original));
+        assertNotSame(original, copy);
+        assertEquals(original, copy);
+    }
+
+    @Test
+    void testLegacyPropertyCanBeReserializedAndCloned() throws Exception {
+        final DynaProperty original;
+        try (ObjectInputStream input = new ObjectInputStream(getClass().getResourceAsStream("typedMapProp.ser"))) {
+            original = (DynaProperty) input.readObject();
+        }
+        assertEquals(new DynaProperty("typedMapProp", Map.class, String.class), original);
+        assertEquals(original, roundTrip(original));
+        final DynaProperty clone = assertInstanceOf(DynaProperty.class, newSerializingCloner().clone(original));
+        assertNotSame(original, clone);
+        assertEquals(original, clone);
+        assertEquals(original, roundTrip(clone));
+    }
+
+    @Test
+    void testSubclassCanBeReserializedAndCloned() throws Exception {
+        final ExtendedProperty original = new ExtendedProperty(42);
+        final ExtendedProperty roundTripped = assertInstanceOf(ExtendedProperty.class, roundTrip(original));
+        assertEquals(original, roundTripped);
+        assertEquals(42, roundTripped.extraValue);
+        final ExtendedProperty clone = assertInstanceOf(ExtendedProperty.class, newSerializingCloner().clone(original));
+        assertNotSame(original, clone);
+        assertEquals(original, clone);
+        assertEquals(42, clone.extraValue);
+    }
+
     @Test
     void testToStringAfterRoundTrip() throws Exception {
         final DynaProperty original = new DynaProperty("myProp", List.class, String.class);
@@ -360,15 +425,15 @@ void testTypeIsPreservedAfterRoundTripForEveryPrimitive() throws Exception {
 
     /**
      * Proves that for <strong>indexed and mapped properties</strong> the second {@code writeAnyClass} call (for {@code contentType}) also appears in the byte
-     * stream <em>before</em> the {@code name} field written by {@code defaultWriteObject}.
+     * stream <em>after</em> the {@code name} field written by {@code defaultWriteObject}.
      * <p>
      * Two mapped properties share the same name and the same {@code Map.class} type but differ in their {@code contentType} ({@code Boolean.TYPE} vs
      * {@code Byte.TYPE}). Because the type ({@code Map.class}) is identical, the streams are the same through the first {@code writeAnyClass} call. They
-     * diverge at the second {@code writeAnyClass} call (contentType constant), which must still precede the name.
+     * diverge at the second {@code writeAnyClass} call (contentType constant), which must still follow the name.
      * </p>
      */
     @Test
-    void testWireFormatContentTypeDataPrecedesNameFieldForMappedProperty() throws Exception {
+    void testWireFormatContentTypeDataFollowsNameFieldForMappedProperty() throws Exception {
         final String sharedName = "sharedMappedName";
         // Both use Map.class as the type (identical first writeAnyClass output).
         // They differ only in contentType: Boolean.TYPE (constant 1) vs Byte.TYPE (constant 2).
@@ -388,25 +453,25 @@ void testWireFormatContentTypeDataPrecedesNameFieldForMappedProperty() throws Ex
             }
         }
         assertTrue(firstDiff >= 0, "Streams must diverge at the contentType primitive-type constant");
-        // The second writeAnyClass output (contentType) must still precede the name
-        // (defaultWriteObject), confirming that both writeAnyClass calls happen before
+        // The second writeAnyClass output (contentType) must still follow the name
+        // (defaultWriteObject), confirming that both writeAnyClass calls happen after
         // defaultWriteObject is invoked.
-        assertTrue(firstDiff < namePosition, "The second writeAnyClass call (contentType) must precede defaultWriteObject: "
-                + "content-type divergence at byte " + firstDiff + " must precede name at byte " + namePosition);
+        assertTrue(firstDiff > namePosition, "The second writeAnyClass call (contentType) must follow defaultWriteObject: "
+                + "content-type divergence at byte " + firstDiff + " must follow name at byte " + namePosition);
         assertRoundTrip(new DynaProperty(sharedName, Map.class, Boolean.TYPE));
         assertRoundTrip(new DynaProperty(sharedName, Map.class, Byte.TYPE));
     }
 
     /**
-     * Same proof as {@link #testWireFormatPrimitiveTypeDataPrecedesNameField()} but for <strong>object (non-primitive) types</strong>.
+     * Same proof as {@link #testWireFormatPrimitiveTypeDataFollowsNameField()} but for <strong>object (non-primitive) types</strong>.
      * <p>
      * For object types {@code writeAnyClass} emits {@code writeBoolean(false)} followed by {@code writeObject(clazz)}. Two properties with the same name but
      * different object types ({@code String.class} vs {@code Integer.class}) differ in the class object written by {@code writeAnyClass}; the shared name is
-     * written later by {@code defaultWriteObject}.
+     * written earlier by {@code defaultWriteObject}.
      * </p>
      */
     @Test
-    void testWireFormatObjectTypeDataPrecedesNameField() throws Exception {
+    void testWireFormatObjectTypeDataFollowsNameField() throws Exception {
         final String sharedName = "sharedObjectName";
         // writeAnyClass for object type: writeBoolean(false) + writeObject(clazz).
         // String.class and Integer.class are serialised differently, so streams diverge
@@ -429,28 +494,28 @@ void testWireFormatObjectTypeDataPrecedesNameField() throws Exception {
             }
         }
         assertTrue(firstDiff >= 0, "Streams must diverge where the class objects differ");
-        // The divergence (type class data from writeAnyClass) must precede the name
+        // The divergence (type class data from writeAnyClass) must follow the name
         // (from defaultWriteObject) in BOTH streams.
-        assertTrue(firstDiff < namePositionInString, "writeAnyClass must be invoked before defaultWriteObject (String-type stream): "
-                + "type divergence at byte " + firstDiff + " must precede name at byte " + namePositionInString);
-        assertTrue(firstDiff < namePositionInInteger, "writeAnyClass must be invoked before defaultWriteObject (Integer-type stream): "
-                + "type divergence at byte " + firstDiff + " must precede name at byte " + namePositionInInteger);
+        assertTrue(firstDiff > namePositionInString, "writeAnyClass must be invoked after defaultWriteObject (String-type stream): "
+                + "type divergence at byte " + firstDiff + " must follow name at byte " + namePositionInString);
+        assertTrue(firstDiff > namePositionInInteger, "writeAnyClass must be invoked after defaultWriteObject (Integer-type stream): "
+                + "type divergence at byte " + firstDiff + " must follow name at byte " + namePositionInInteger);
         assertRoundTrip(new DynaProperty(sharedName, String.class));
         assertRoundTrip(new DynaProperty(sharedName, Integer.class));
     }
 
     /**
-     * Proves that {@link DynaProperty#writeObject(ObjectOutputStream)} calls {@code writeAnyClass} (which encodes the {@code type} field) <em>before</em>
+     * Proves that {@link DynaProperty#writeObject(ObjectOutputStream)} calls {@code writeAnyClass} (which encodes the {@code type} field) <em>after</em>
      * {@code defaultWriteObject} (which encodes the {@code name} field) for <strong>primitive types</strong>.
      * <p>
      * Strategy: serialise two {@link DynaProperty} instances that have an identical {@code name} but different primitive types. Because the name is identical,
      * both byte streams are the same from the class-descriptor through to the end of the default-field data. The streams diverge only at the
-     * {@code writeAnyClass} output (the primitive-type integer constant). If that divergence point is located <em>before</em> the name bytes in the stream, it
-     * proves that {@code writeAnyClass} is called first.
+     * {@code writeAnyClass} output (the primitive-type integer constant). If that divergence point is located <em>after</em> the name bytes in the stream, it
+     * proves that {@code defaultWriteObject} is called first.
      * </p>
      */
     @Test
-    void testWireFormatPrimitiveTypeDataPrecedesNameField() throws Exception {
+    void testWireFormatPrimitiveTypeDataFollowsNameField() throws Exception {
         final String sharedName = "sharedPrimitiveName";
         // Boolean.TYPE encodes as writeBoolean(true) + writeInt(BOOLEAN_TYPE=1).
         // Byte.TYPE encodes as writeBoolean(true) + writeInt(BYTE_TYPE=2).
@@ -474,9 +539,9 @@ void testWireFormatPrimitiveTypeDataPrecedesNameField() throws Exception {
         }
         assertTrue(firstDiff >= 0, "Streams must diverge at the primitive-type constant");
         // CRITICAL assertion: the divergence point (type data from writeAnyClass) must
-        // come BEFORE the name field (from defaultWriteObject).
-        assertTrue(firstDiff < namePosition, "writeAnyClass must be invoked before defaultWriteObject: " + "type-data divergence at byte " + firstDiff
-                + " must precede name field at byte " + namePosition);
+        // come AFTER the name field (from defaultWriteObject).
+        assertTrue(firstDiff > namePosition, "writeAnyClass must be invoked after defaultWriteObject: " + "type-data divergence at byte " + firstDiff
+                + " must follow name field at byte " + namePosition);
         // Sanity-check: both properties still round-trip correctly.
         assertRoundTrip(new DynaProperty(sharedName, Boolean.TYPE));
         assertRoundTrip(new DynaProperty(sharedName, Byte.TYPE));