Always return a fresh copy of BeanPropertySet from cache (#10635)

3 files changed, 176 insertions, 63 deletions

diff --git a/server/src/main/java/com/vaadin/data/BeanPropertySet.java b/server/src/main/java/com/vaadin/data/BeanPropertySet.java
index b877fdf939..6b234703e5 100644
--- a/server/src/main/java/com/vaadin/data/BeanPropertySet.java
+++ b/server/src/main/java/com/vaadin/data/BeanPropertySet.java
@@ -55,11 +55,11 @@ public class BeanPropertySet<T> implements PropertySet<T> {
      * @see #readResolve()
      * @see BeanPropertyDefinition#writeReplace()
      */
-    private static class SerializedPropertySet implements Serializable {
-        private final Class<?> beanType;
+    private static class SerializedPropertySet<T> implements Serializable {
+        private final InstanceKey<T> instanceKey;
 
-        private SerializedPropertySet(Class<?> beanType) {
-            this.beanType = beanType;
+        private SerializedPropertySet(InstanceKey<T> instanceKey) {
+            this.instanceKey = instanceKey;
         }
 
         private Object readResolve() {
@@ -67,7 +67,8 @@ public class BeanPropertySet<T> implements PropertySet<T> {
              * When this instance is deserialized, it will be replaced with a
              * property set for the corresponding bean type and property name.
              */
-            return get(beanType);
+            return get(instanceKey.type,instanceKey.checkNestedDefinitions,
+                    new PropertyFilterDefinition(instanceKey.depth,instanceKey.ignorePackageNames));
         }
     }
 
@@ -143,7 +144,7 @@ public class BeanPropertySet<T> implements PropertySet<T> {
              * serialize a DTO that when deserialized will get the corresponding
              * property definition from the cache.
              */
-            return new SerializedPropertyDefinition(getPropertySet().beanType,
+            return new SerializedPropertyDefinition(getPropertySet().instanceKey.type,
                     getName());
         }
     }
@@ -221,7 +222,7 @@ public class BeanPropertySet<T> implements PropertySet<T> {
              * serialize a DTO that when deserialized will get the corresponding
              * property definition from the cache.
              */
-            return new SerializedPropertyDefinition(getPropertySet().beanType,
+            return new SerializedPropertyDefinition(getPropertySet().instanceKey.type,
                     getName());
         }
         
@@ -241,13 +242,13 @@ public class BeanPropertySet<T> implements PropertySet<T> {
      *
      * @since 8.2
      */
-    private static class InstanceKey implements Serializable {
-        private Class<?> type;
+    private static class InstanceKey<T> implements Serializable {
+        private Class<T> type;
         private boolean checkNestedDefinitions;
         private int depth;
         private List<String> ignorePackageNames;
 
-        public InstanceKey(Class<?> type, boolean checkNestedDefinitions,
+        public InstanceKey(Class<T> type, boolean checkNestedDefinitions,
                 int depth, List<String> ignorePackageNames) {
             this.type = type;
             this.checkNestedDefinitions = checkNestedDefinitions;
@@ -292,45 +293,43 @@ public class BeanPropertySet<T> implements PropertySet<T> {
             } else if (!ignorePackageNames.equals(other.ignorePackageNames)) {
                 return false;
             }
-            if (type == null) {
-                if (other.type != null) {
-                    return false;
-                }
-            } else if (!type.equals(other.type)) {
-                return false;
-            }
-            return true;
+            return Objects.equals(type, other.type);
         }
 
     }
 
-    private static final ConcurrentMap<InstanceKey, BeanPropertySet<?>> INSTANCES = new ConcurrentHashMap<>();
+    private static final ConcurrentMap<InstanceKey<?>, BeanPropertySet<?>> INSTANCES = new ConcurrentHashMap<>();
 
-    private final Class<T> beanType;
+    private final InstanceKey<T> instanceKey;
 
     private final Map<String, PropertyDefinition<T, ?>> definitions;
 
-    private BeanPropertySet(Class<T> beanType) {
-        this.beanType = beanType;
+    private BeanPropertySet(InstanceKey<T> instanceKey) {
+        this.instanceKey = instanceKey;
 
         try {
-            definitions = BeanUtil.getBeanPropertyDescriptors(beanType).stream()
+            definitions = BeanUtil.getBeanPropertyDescriptors(instanceKey.type).stream()
                     .filter(BeanPropertySet::hasNonObjectReadMethod)
                     .map(descriptor -> new BeanPropertyDefinition<>(this,
-                            beanType, descriptor))
+                            instanceKey.type, descriptor))
                     .collect(Collectors.toMap(PropertyDefinition::getName,
                             Function.identity()));
         } catch (IntrospectionException e) {
             throw new IllegalArgumentException(
                     "Cannot find property descriptors for "
-                            + beanType.getName(),
+                            + instanceKey.type.getName(),
                     e);
         }
     }
 
-    private BeanPropertySet(Class<T> beanType, boolean checkNestedDefinitions,
-            PropertyFilterDefinition propertyFilterDefinition) {
-        this(beanType);
+    private BeanPropertySet(InstanceKey<T> instanceKey, Map<String, PropertyDefinition<T, ?>> definitions) {
+        this.instanceKey = instanceKey;
+        this.definitions = new HashMap<>(definitions);
+    }
+
+    private BeanPropertySet(InstanceKey<T> instanceKey, boolean checkNestedDefinitions,
+                            PropertyFilterDefinition propertyFilterDefinition) {
+        this(instanceKey);
         if (checkNestedDefinitions) {
             Objects.requireNonNull(propertyFilterDefinition,
                     "You must define a property filter callback if using nested property scan.");
@@ -370,7 +369,7 @@ public class BeanPropertySet<T> implements PropertySet<T> {
                     String name = parentPropertyKey + "."
                             + descriptor.getName();
                     PropertyDescriptor subDescriptor = BeanUtil
-                            .getPropertyDescriptor(beanType, name);
+                            .getPropertyDescriptor(instanceKey.type, name);
                     moreProps.put(name, new NestedBeanPropertyDefinition<>(this,
                             parentProperty, subDescriptor));
 
@@ -402,7 +401,11 @@ public class BeanPropertySet<T> implements PropertySet<T> {
         InstanceKey key = new InstanceKey(beanType, false, 0, null);
         // Cache the reflection results
         return (PropertySet<T>) INSTANCES.computeIfAbsent(key,
-                ignored -> new BeanPropertySet<>(beanType));
+                ignored -> new BeanPropertySet<>(key)).copy();
+    }
+
+    private BeanPropertySet<T> copy() {
+        return new BeanPropertySet<>(instanceKey,definitions);
     }
 
     /**
@@ -426,8 +429,8 @@ public class BeanPropertySet<T> implements PropertySet<T> {
                 filterDefinition.getMaxNestingDepth(),
                 filterDefinition.getIgnorePackageNamesStartingWith());
         return (PropertySet<T>) INSTANCES.computeIfAbsent(key,
-                k -> new BeanPropertySet<>(beanType, checkNestedDefinitions,
-                        filterDefinition));
+                k -> new BeanPropertySet<>(key, checkNestedDefinitions,
+                        filterDefinition)).copy();
     }
 
     @Override
@@ -448,11 +451,11 @@ public class BeanPropertySet<T> implements PropertySet<T> {
                 if (!parent.isPresent()) {
                     throw new IllegalArgumentException(
                             "Cannot find property descriptor [" + parentName
-                                    + "] for " + beanType.getName());
+                                    + "] for " + instanceKey.type.getName());
                 }
 
                 Optional<PropertyDescriptor> descriptor = Optional.ofNullable(
-                        BeanUtil.getPropertyDescriptor(beanType, name));
+                        BeanUtil.getPropertyDescriptor(instanceKey.type, name));
                 if (descriptor.isPresent()) {
                     NestedBeanPropertyDefinition<T, ?> nestedDefinition = new NestedBeanPropertyDefinition<>(
                             this, parent.get(), descriptor.get());
@@ -461,13 +464,13 @@ public class BeanPropertySet<T> implements PropertySet<T> {
                 } else {
                     throw new IllegalArgumentException(
                             "Cannot find property descriptor [" + name
-                                    + "] for " + beanType.getName());
+                                    + "] for " + instanceKey.type.getName());
                 }
 
             } catch (IntrospectionException e) {
                 throw new IllegalArgumentException(
                         "Cannot find property descriptors for "
-                                + beanType.getName(),
+                                + instanceKey.type.getName(),
                         e);
             }
         }
@@ -481,7 +484,7 @@ public class BeanPropertySet<T> implements PropertySet<T> {
      * @return the bean type of this bean property set
      */
     public Class<T> getBeanType() {
-        return beanType;
+        return instanceKey.type;
     }
 
     private static boolean hasNonObjectReadMethod(
@@ -502,7 +505,7 @@ public class BeanPropertySet<T> implements PropertySet<T> {
 
     @Override
     public String toString() {
-        return "Property set for bean " + beanType.getName();
+        return "Property set for bean " + instanceKey.type.getName();
     }
 
     private Object writeReplace() {
@@ -511,6 +514,6 @@ public class BeanPropertySet<T> implements PropertySet<T> {
          * that when deserialized will get the corresponding property set from
          * the cache.
          */
-        return new SerializedPropertySet(beanType);
+        return new SerializedPropertySet(instanceKey);
     }
 }
diff --git a/server/src/test/java/com/vaadin/data/BeanPropertySetTest.java b/server/src/test/java/com/vaadin/data/BeanPropertySetTest.java
index 6b43bdfa24..d319a1c52a 100644
--- a/server/src/test/java/com/vaadin/data/BeanPropertySetTest.java
+++ b/server/src/test/java/com/vaadin/data/BeanPropertySetTest.java
@@ -15,21 +15,19 @@
  */
 package com.vaadin.data;
 
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertNotSame;
-import static org.junit.Assert.assertSame;
-
 import java.io.ByteArrayInputStream;
 import java.io.ByteArrayOutputStream;
 import java.io.ObjectInputStream;
 import java.io.ObjectOutputStream;
 import java.lang.reflect.Field;
 import java.util.Arrays;
+import java.util.Comparator;
 import java.util.HashSet;
 import java.util.Map;
 import java.util.Set;
 import java.util.stream.Collectors;
 
+import junit.framework.AssertionFailedError;
 import org.junit.Test;
 
 import com.vaadin.data.provider.bov.Person;
@@ -39,6 +37,11 @@ import com.vaadin.tests.data.bean.FatherAndSon;
 import com.vaadin.tests.data.bean.Sex;
 import com.vaadin.tests.server.ClassesSerializableTest;
 
+import static com.vaadin.data.PropertyFilterDefinition.getDefaultFilter;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertSame;
+import static org.junit.Assert.assertNotSame;
+
 public class BeanPropertySetTest {
     @Test
     public void testSerializeDeserialize_propertySet() throws Exception {
@@ -48,8 +51,23 @@ public class BeanPropertySetTest {
         PropertySet<Person> deserializedPropertySet = ClassesSerializableTest
                 .serializeAndDeserialize(originalPropertySet);
 
-        assertSame("Deserialized instance should be the same as the original",
-                originalPropertySet, deserializedPropertySet);
+        comparePropertySet(originalPropertySet, deserializedPropertySet, "Deserialized instance should be the same as the original");
+    }
+
+    private void comparePropertySet(PropertySet<?> propertySetA, PropertySet<?> propertySetB, String message) {
+
+        PropertyDefinition<?, ?>[] propertiesA = propertySetA.getProperties()
+                .sorted(Comparator.comparing(PropertyDefinition::getName))
+                .toArray(PropertyDefinition<?, ?>[]::new);
+        PropertyDefinition<?, ?>[] propertiesB = propertySetA.getProperties()
+                .sorted(Comparator.comparing(PropertyDefinition::getName))
+                .toArray(PropertyDefinition<?, ?>[]::new);
+
+        assertEquals(message, propertiesA.length, propertiesB.length);
+        for (int i = 0; i < propertiesB.length; i++) {
+            assertSame(message,
+                    propertiesA[i], propertiesB[i]);
+        }
     }
 
     @Test
@@ -76,8 +94,7 @@ public class BeanPropertySetTest {
         PropertySet<Person> deserializedPropertySet = (PropertySet<Person>) in
                 .readObject();
 
-        assertSame("Deserialized instance should be the same as in the cache",
-                BeanPropertySet.get(Person.class), deserializedPropertySet);
+        comparePropertySet(BeanPropertySet.get(Person.class), deserializedPropertySet, "Deserialized instance should be the same as in the cache");
         assertNotSame(
                 "Deserialized instance should not be the same as the original",
                 originalPropertySet, deserializedPropertySet);
@@ -108,10 +125,11 @@ public class BeanPropertySetTest {
     @Test
     public void testSerializeDeserialize_nestedPropertyDefinition()
             throws Exception {
+
         PropertyDefinition<com.vaadin.tests.data.bean.Person, ?> definition = BeanPropertySet
-                .get(com.vaadin.tests.data.bean.Person.class)
-                .getProperty("address.postalCode")
-                .orElseThrow(RuntimeException::new);
+                .get(com.vaadin.tests.data.bean.Person.class,true, getDefaultFilter())
+                .getProperty("address.postalCode").orElseThrow(AssertionFailedError::new);
+
 
         PropertyDefinition<com.vaadin.tests.data.bean.Person, ?> deserializedDefinition = ClassesSerializableTest
                 .serializeAndDeserialize(definition);
@@ -128,16 +146,10 @@ public class BeanPropertySetTest {
         assertEquals("Deserialized definition should be functional",
                 address.getPostalCode(), postalCode);
 
-        assertSame("Deserialized instance should be the same as in the cache",
-                BeanPropertySet.get(com.vaadin.tests.data.bean.Person.class)
-                        .getProperty("address.postalCode").orElseThrow(
-                                RuntimeException::new),
-                deserializedDefinition);
     }
 
     @Test
-    public void nestedPropertyDefinition_samePropertyNameOnMultipleLevels()
-            throws Exception {
+    public void nestedPropertyDefinition_samePropertyNameOnMultipleLevels() {
         PropertyDefinition<FatherAndSon, ?> definition = BeanPropertySet
                 .get(FatherAndSon.class).getProperty("father.father.firstName")
                 .orElseThrow(RuntimeException::new);
@@ -156,8 +168,7 @@ public class BeanPropertySetTest {
     }
 
     @Test(expected = NullPointerException.class)
-    public void nestedPropertyDefinition_propertyChainBroken()
-            throws Exception {
+    public void nestedPropertyDefinition_propertyChainBroken() {
         PropertyDefinition<FatherAndSon, ?> definition = BeanPropertySet
                 .get(FatherAndSon.class).getProperty("father.firstName")
                 .orElseThrow(RuntimeException::new);
@@ -166,15 +177,13 @@ public class BeanPropertySetTest {
     }
 
     @Test(expected = IllegalArgumentException.class)
-    public void nestedPropertyDefinition_invalidPropertyNameInChain()
-            throws Exception {
+    public void nestedPropertyDefinition_invalidPropertyNameInChain() {
         BeanPropertySet.get(FatherAndSon.class)
                 .getProperty("grandfather.firstName");
     }
 
     @Test(expected = IllegalArgumentException.class)
-    public void nestedPropertyDefinition_invalidPropertyNameAtChainEnd()
-            throws Exception {
+    public void nestedPropertyDefinition_invalidPropertyNameAtChainEnd() {
         BeanPropertySet.get(FatherAndSon.class).getProperty("father.age");
     }
 
diff --git a/server/src/test/java/com/vaadin/data/PropertyRetrospectionTest.java b/server/src/test/java/com/vaadin/data/PropertyRetrospectionTest.java
new file mode 100644
index 0000000000..3b3a611345
--- /dev/null
+++ b/server/src/test/java/com/vaadin/data/PropertyRetrospectionTest.java
@@ -0,0 +1,101 @@
+package com.vaadin.data;
+
+import org.junit.Test;
+
+import com.vaadin.ui.Grid;
+import com.vaadin.ui.TextField;
+
+import static org.junit.Assert.assertEquals;
+
+public class PropertyRetrospectionTest {
+
+    @SuppressWarnings("unused")
+    public static class InnerBean {
+        private String innerString;
+
+        public String getInnerString() {
+            return innerString;
+        }
+
+        public void setInnerString(String innerString) {
+            this.innerString = innerString;
+        }
+    }
+
+    @SuppressWarnings("unused")
+    public static class BeanOne {
+        private String someString;
+        private InnerBean innerBean;
+
+        public String getSomeString() {
+            return someString;
+        }
+
+        public void setSomeString(String someString) {
+            this.someString = someString;
+        }
+
+        public InnerBean getInnerBean() {
+            return innerBean;
+        }
+
+        public void setInnerBean(InnerBean innerBean) {
+            this.innerBean = innerBean;
+        }
+    }
+
+    @SuppressWarnings("unused")
+    public static class BeanTwo {
+        private String someString;
+        private InnerBean innerBean;
+
+        public String getSomeString() {
+            return someString;
+        }
+
+        public void setSomeString(String someString) {
+            this.someString = someString;
+        }
+
+        public InnerBean getInnerBean() {
+            return innerBean;
+        }
+
+        public void setInnerBean(InnerBean innerBean) {
+            this.innerBean = innerBean;
+        }
+    }
+
+    @Test
+    public void testGridBeanProperties()
+    {
+        Grid<BeanOne> grid1 = new Grid<>(BeanOne.class);
+        assertEquals(2,BeanPropertySet.get(BeanOne.class).getProperties().count());
+        assertEquals(2,grid1.getColumns().size());
+        grid1.addColumn("innerBean.innerString");
+        assertEquals(3,grid1.getColumns().size());
+        assertEquals(2,BeanPropertySet.get(BeanOne.class).getProperties().count());
+
+
+        Grid<BeanOne> grid2 = new Grid<>(BeanOne.class);
+        assertEquals(2,BeanPropertySet.get(BeanOne.class).getProperties().count());
+        assertEquals(2,grid2.getColumns().size());
+        grid2.addColumn("innerBean.innerString");
+        assertEquals(3,grid2.getColumns().size());
+        assertEquals(2,BeanPropertySet.get(BeanOne.class).getProperties().count());
+    }
+
+    @Test
+    public void testBinder()
+    {
+        Binder<BeanTwo> binder1 = new Binder<>(BeanTwo.class);
+        assertEquals(2,BeanPropertySet.get(BeanTwo.class).getProperties().count());
+        binder1.forField(new TextField()).bind("innerBean.innerString");
+        assertEquals(2,BeanPropertySet.get(BeanTwo.class).getProperties().count());
+
+        Binder<BeanTwo> binder2 = new Binder<>(BeanTwo.class);
+        assertEquals(2,BeanPropertySet.get(BeanTwo.class).getProperties().count());
+        binder2.forField(new TextField()).bind("innerBean.innerString");
+        assertEquals(2,BeanPropertySet.get(BeanTwo.class).getProperties().count());
+    }
+}