Fix BeanBinderPropertySet to not deserialize into multiple instances (#8305)

* Fix BeanBinderPropertySet to not deserialize into multiple instances

3 files changed, 146 insertions, 42 deletions

diff --git a/server/src/main/java/com/vaadin/data/BeanBinderPropertySet.java b/server/src/main/java/com/vaadin/data/BeanBinderPropertySet.java
index 3fcc40f6df..c167229f8c 100644
--- a/server/src/main/java/com/vaadin/data/BeanBinderPropertySet.java
+++ b/server/src/main/java/com/vaadin/data/BeanBinderPropertySet.java
@@ -18,8 +18,7 @@ package com.vaadin.data;
 import java.beans.IntrospectionException;
 import java.beans.PropertyDescriptor;
 import java.io.IOException;
-import java.io.ObjectInputStream;
-import java.io.ObjectOutputStream;
+import java.io.Serializable;
 import java.lang.reflect.InvocationTargetException;
 import java.lang.reflect.Method;
 import java.util.Map;
@@ -49,10 +48,66 @@ import com.vaadin.util.ReflectTools;
  */
 public class BeanBinderPropertySet<T> implements BinderPropertySet<T> {
 
+    /**
+     * Serialized form of a property set. When deserialized, the property set
+     * for the corresponding bean type is requested, which either returns the
+     * existing cached instance or creates a new one.
+     *
+     * @see #readResolve()
+     * @see BeanBinderPropertyDefinition#writeReplace()
+     */
+    private static class SerializedPropertySet implements Serializable {
+        private final Class<?> beanType;
+
+        private SerializedPropertySet(Class<?> beanType) {
+            this.beanType = beanType;
+        }
+
+        private Object readResolve() {
+            /*
+             * When this instance is deserialized, it will be replaced with a
+             * property set for the corresponding bean type and property name.
+             */
+            return get(beanType);
+        }
+    }
+
+    /**
+     * Serialized form of a property definition. When deserialized, the property
+     * set for the corresponding bean type is requested, which either returns
+     * the existing cached instance or creates a new one. The right property
+     * definition is then fetched from the property set.
+     *
+     * @see #readResolve()
+     * @see BeanBinderPropertySet#writeReplace()
+     */
+    private static class SerializedPropertyDefinition implements Serializable {
+        private final Class<?> beanType;
+        private final String propertyName;
+
+        private SerializedPropertyDefinition(Class<?> beanType,
+                String propertyName) {
+            this.beanType = beanType;
+            this.propertyName = propertyName;
+        }
+
+        private Object readResolve() throws IOException {
+            /*
+             * When this instance is deserialized, it will be replaced with a
+             * property definition for the corresponding bean type and property
+             * name.
+             */
+            return get(beanType).getProperty(propertyName)
+                    .orElseThrow(() -> new IOException(
+                            beanType + " no longer has a property named "
+                                    + propertyName));
+        }
+    }
+
     private static class BeanBinderPropertyDefinition<T, V>
             implements BinderPropertyDefinition<T, V> {
 
-        private transient PropertyDescriptor descriptor;
+        private final PropertyDescriptor descriptor;
         private final BeanBinderPropertySet<T> propertySet;
 
         public BeanBinderPropertyDefinition(
@@ -118,32 +173,14 @@ public class BeanBinderPropertySet<T> implements BinderPropertySet<T> {
             return propertySet;
         }
 
-        private void writeObject(ObjectOutputStream stream) throws IOException {
-            stream.defaultWriteObject();
-            stream.writeObject(descriptor.getName());
-            stream.writeObject(propertySet.beanType);
-        }
-
-        private void readObject(ObjectInputStream stream)
-                throws IOException, ClassNotFoundException {
-            stream.defaultReadObject();
-
-            String propertyName = (String) stream.readObject();
-            @SuppressWarnings("unchecked")
-            Class<T> beanType = (Class<T>) stream.readObject();
-
-            try {
-                descriptor = BeanUtil.getBeanPropertyDescriptors(beanType)
-                        .stream()
-                        .filter(descriptor -> descriptor.getName()
-                                .equals(propertyName))
-                        .findAny()
-                        .orElseThrow(() -> new IOException(
-                                "Property " + propertyName + " not found for "
-                                        + beanType.getName()));
-            } catch (IntrospectionException e) {
-                throw new IOException(e);
-            }
+        private Object writeReplace() {
+            /*
+             * Instead of serializing this actual property definition, only
+             * serialize a DTO that when deserialized will get the corresponding
+             * property definition from the cache.
+             */
+            return new SerializedPropertyDefinition(getPropertySet().beanType,
+                    getName());
         }
     }
 
@@ -214,4 +251,13 @@ public class BeanBinderPropertySet<T> implements BinderPropertySet<T> {
     public String toString() {
         return "Property set for bean " + beanType.getName();
     }
+
+    private Object writeReplace() {
+        /*
+         * Instead of serializing this actual property set, only serialize a DTO
+         * that when deserialized will get the corresponding property set from
+         * the cache.
+         */
+        return new SerializedPropertySet(beanType);
+    }
 }
diff --git a/server/src/test/java/com/vaadin/data/BeanBinderPropertySetTest.java b/server/src/test/java/com/vaadin/data/BeanBinderPropertySetTest.java
index d11541fb0c..1fa431ac20 100644
--- a/server/src/test/java/com/vaadin/data/BeanBinderPropertySetTest.java
+++ b/server/src/test/java/com/vaadin/data/BeanBinderPropertySetTest.java
@@ -19,34 +19,83 @@ import java.io.ByteArrayInputStream;
 import java.io.ByteArrayOutputStream;
 import java.io.ObjectInputStream;
 import java.io.ObjectOutputStream;
+import java.lang.reflect.Field;
+import java.util.Map;
 
 import org.junit.Assert;
 import org.junit.Test;
 
 import com.vaadin.data.provider.bov.Person;
+import com.vaadin.tests.server.ClassesSerializableTest;
 
 public class BeanBinderPropertySetTest {
     @Test
-    public void testSerializeDeserialize() throws Exception {
-        BinderPropertyDefinition<Person, ?> definition = BeanBinderPropertySet
-                .get(Person.class).getProperty("born")
-                .orElseThrow(RuntimeException::new);
+    public void testSerializeDeserialize_propertySet() throws Exception {
+        BinderPropertySet<Person> originalPropertySet = BeanBinderPropertySet
+                .get(Person.class);
 
-        ByteArrayOutputStream bos = new ByteArrayOutputStream();
-        ObjectOutputStream out = new ObjectOutputStream(bos);
-        out.writeObject(definition);
-        out.flush();
+        BinderPropertySet<Person> deserializedPropertySet = ClassesSerializableTest
+                .serializeAndDeserialize(originalPropertySet);
 
-        ObjectInputStream inputStream = new ObjectInputStream(
-                new ByteArrayInputStream(bos.toByteArray()));
+        Assert.assertSame(
+                "Deserialized instance should be the same as the original",
+                originalPropertySet, deserializedPropertySet);
+    }
 
-        BinderPropertyDefinition<Person, ?> deserializedDefinition = (BinderPropertyDefinition<Person, ?>) inputStream
+    @Test
+    public void testSerializeDeserialize_propertySet_cacheCleared()
+            throws Exception {
+        BinderPropertySet<Person> originalPropertySet = BeanBinderPropertySet
+                .get(Person.class);
+
+        ByteArrayOutputStream bs = new ByteArrayOutputStream();
+        ObjectOutputStream out = new ObjectOutputStream(bs);
+        out.writeObject(originalPropertySet);
+        byte[] data = bs.toByteArray();
+
+        // Simulate deserializing into a different JVM by clearing the instance
+        // map
+        Field instancesField = BeanBinderPropertySet.class
+                .getDeclaredField("instances");
+        instancesField.setAccessible(true);
+        Map<?, ?> instances = (Map<?, ?>) instancesField.get(null);
+        instances.clear();
+
+        ObjectInputStream in = new ObjectInputStream(
+                new ByteArrayInputStream(data));
+        BinderPropertySet<Person> deserializedPropertySet = (BinderPropertySet<Person>) in
                 .readObject();
 
+        Assert.assertSame(
+                "Deserialized instance should be the same as in the cache",
+                BeanBinderPropertySet.get(Person.class),
+                deserializedPropertySet);
+        Assert.assertNotSame(
+                "Deserialized instance should not be the same as the original",
+                originalPropertySet, deserializedPropertySet);
+    }
+
+    @Test
+    public void testSerializeDeserialize_propertyDefinition() throws Exception {
+        BinderPropertyDefinition<Person, ?> definition = BeanBinderPropertySet
+                .get(Person.class).getProperty("born")
+                .orElseThrow(RuntimeException::new);
+
+        BinderPropertyDefinition<Person, ?> deserializedDefinition = ClassesSerializableTest
+                .serializeAndDeserialize(definition);
+
         ValueProvider<Person, ?> getter = deserializedDefinition.getGetter();
         Person person = new Person("Milennial", 2000);
         Integer age = (Integer) getter.apply(person);
 
-        Assert.assertEquals(Integer.valueOf(2000), age);
+        Assert.assertEquals("Deserialized definition should be functional",
+                Integer.valueOf(2000), age);
+
+        Assert.assertSame(
+                "Deserialized instance should be the same as in the cache",
+                BeanBinderPropertySet.get(Person.class).getProperty("born")
+                        .orElseThrow(RuntimeException::new),
+                deserializedDefinition);
     }
+
 }
diff --git a/server/src/test/java/com/vaadin/tests/server/ClassesSerializableTest.java b/server/src/test/java/com/vaadin/tests/server/ClassesSerializableTest.java
index 8a97e2217c..fca87ccbdf 100644
--- a/server/src/test/java/com/vaadin/tests/server/ClassesSerializableTest.java
+++ b/server/src/test/java/com/vaadin/tests/server/ClassesSerializableTest.java
@@ -185,13 +185,22 @@ public class ClassesSerializableTest {
         }
         defaultCtor.get().setAccessible(true);
         Object instance = defaultCtor.get().newInstance();
+        serializeAndDeserialize(instance);
+    }
+
+    public static <T> T serializeAndDeserialize(T instance)
+            throws IOException, ClassNotFoundException {
         ByteArrayOutputStream bs = new ByteArrayOutputStream();
         ObjectOutputStream out = new ObjectOutputStream(bs);
         out.writeObject(instance);
         byte[] data = bs.toByteArray();
         ObjectInputStream in = new ObjectInputStream(
                 new ByteArrayInputStream(data));
-        in.readObject();
+
+        @SuppressWarnings("unchecked")
+        T readObject = (T) in.readObject();
+
+        return readObject;
     }
 
     private void failSerializableFields(