Improve nested property support for Binder (#9925)

Fixes #9210

4 files changed, 477 insertions, 12 deletions

diff --git a/server/src/main/java/com/vaadin/data/BeanPropertySet.java b/server/src/main/java/com/vaadin/data/BeanPropertySet.java
index dcbff134d4..1371f769d6 100644
--- a/server/src/main/java/com/vaadin/data/BeanPropertySet.java
+++ b/server/src/main/java/com/vaadin/data/BeanPropertySet.java
@@ -21,6 +21,9 @@ import java.io.IOException;
 import java.io.Serializable;
 import java.lang.reflect.InvocationTargetException;
 import java.lang.reflect.Method;
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.List;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Optional;
@@ -213,8 +216,74 @@ public class BeanPropertySet<T> implements PropertySet<T> {
     public static class NestedBeanPropertyDefinition<T, V>
             extends AbstractBeanPropertyDefinition<T, V> {
 
+        /**
+         * Default maximum depth for scanning nested properties.
+         *
+         * @since
+         */
+        protected static final int MAX_PROPERTY_NESTING_DEPTH = 10;
+
+        /**
+         * Class containing the constraints for filtering nested properties.
+         *
+         * @since
+         *
+         */
+        protected static class PropertyFilterDefinition
+                implements Serializable {
+            private int maxNestingDepth;
+            private List<String> ignorePackageNamesStartingWith;
+
+            /**
+             * Create a property filter with max nesting depth and package names
+             * to ignore.
+             *
+             * @param maxNestingDepth
+             *            The maximum amount of nesting levels for
+             *            sub-properties.
+             * @param ignorePackageNamesStartingWith
+             *            Ignore package names that start with this string, for
+             *            example "java.lang".
+             */
+            public PropertyFilterDefinition(int maxNestingDepth,
+                    List<String> ignorePackageNamesStartingWith) {
+                this.maxNestingDepth = maxNestingDepth;
+                this.ignorePackageNamesStartingWith = ignorePackageNamesStartingWith;
+            }
+
+            /**
+             * Returns the maximum amount of nesting levels for sub-properties.
+             *
+             * @return maximum nesting depth
+             */
+            public int getMaxNestingDepth() {
+                return maxNestingDepth;
+            }
+
+            /**
+             * Returns a list of package name prefixes to ignore.
+             *
+             * @return list of strings that
+             */
+            public List<String> getIgnorePackageNamesStartingWith() {
+                return ignorePackageNamesStartingWith;
+            }
+
+            /**
+             * Get the default nested property filtering conditions.
+             *
+             * @return default property filter
+             */
+            public static PropertyFilterDefinition getDefaultFilter() {
+                return new PropertyFilterDefinition(MAX_PROPERTY_NESTING_DEPTH,
+                        Arrays.asList("java"));
+            }
+        }
+
         private final PropertyDefinition<T, ?> parent;
 
+        private boolean useLongFormName = false;
+
         public NestedBeanPropertyDefinition(BeanPropertySet<T> propertySet,
                 PropertyDefinition<T, ?> parent,
                 PropertyDescriptor descriptor) {
@@ -222,6 +291,28 @@ public class BeanPropertySet<T> implements PropertySet<T> {
             this.parent = parent;
         }
 
+        /**
+         * Create nested property definition. Allows use of a long form name.
+         *
+         * @param propertySet
+         *            property set this property belongs is.
+         * @param parent
+         *            parent property for this nested property
+         * @param descriptor
+         *            property descriptor
+         * @param useLongFormName
+         *            use format grandparent.parent.property for name if
+         *            {@code true}, needed when creating nested definitions
+         *            recursively like in findNestedDefinitions
+         * @since
+         */
+        public NestedBeanPropertyDefinition(BeanPropertySet<T> propertySet,
+                PropertyDefinition<T, ?> parent, PropertyDescriptor descriptor,
+                boolean useLongFormName) {
+            this(propertySet, parent, descriptor);
+            this.useLongFormName = useLongFormName;
+        }
+
         @Override
         public ValueProvider<T, V> getGetter() {
             return bean -> {
@@ -257,7 +348,7 @@ public class BeanPropertySet<T> implements PropertySet<T> {
              * property definition from the cache.
              */
             return new SerializedPropertyDefinition(getPropertySet().beanType,
-                    parent.getName() + "." + getName());
+                    parent.getName() + "." + super.getName());
         }
 
         /**
@@ -268,9 +359,86 @@ public class BeanPropertySet<T> implements PropertySet<T> {
         public PropertyDefinition<T, ?> getParent() {
             return parent;
         }
+
+        @Override
+        public String getName() {
+            if (useLongFormName) {
+                return parent.getName() + "." + super.getName();
+            }
+            return super.getName();
+        }
+
+    }
+
+    /**
+     * Key for identifying cached BeanPropertySet instances.
+     *
+     * @since
+     */
+    private static class InstanceKey implements Serializable {
+        private Class<?> type;
+        private boolean checkNestedDefinitions;
+        private int depth;
+        private List<String> ignorePackageNames;
+
+        public InstanceKey(Class<?> type, boolean checkNestedDefinitions,
+                int depth, List<String> ignorePackageNames) {
+            this.type = type;
+            this.checkNestedDefinitions = checkNestedDefinitions;
+            this.depth = depth;
+            this.ignorePackageNames = ignorePackageNames;
+        }
+
+        @Override
+        public int hashCode() {
+            final int prime = 31;
+            int result = 1;
+            result = prime * result + (checkNestedDefinitions ? 1231 : 1237);
+            result = prime * result + depth;
+            result = prime * result + ((ignorePackageNames == null) ? 0
+                    : ignorePackageNames.hashCode());
+            result = prime * result + ((type == null) ? 0 : type.hashCode());
+            return result;
+        }
+
+        @Override
+        public boolean equals(Object obj) {
+            if (this == obj) {
+                return true;
+            }
+            if (obj == null) {
+                return false;
+            }
+            if (getClass() != obj.getClass()) {
+                return false;
+            }
+            InstanceKey other = (InstanceKey) obj;
+            if (checkNestedDefinitions != other.checkNestedDefinitions) {
+                return false;
+            }
+            if (depth != other.depth) {
+                return false;
+            }
+            if (ignorePackageNames == null) {
+                if (other.ignorePackageNames != null) {
+                    return false;
+                }
+            } 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;
+        }
+
     }
 
-    private static final ConcurrentMap<Class<?>, BeanPropertySet<?>> INSTANCES = new ConcurrentHashMap<>();
+    private static final ConcurrentMap<InstanceKey, BeanPropertySet<?>> INSTANCES = new ConcurrentHashMap<>();
 
     private final Class<T> beanType;
 
@@ -294,6 +462,67 @@ public class BeanPropertySet<T> implements PropertySet<T> {
         }
     }
 
+    private BeanPropertySet(Class<T> beanType, boolean checkNestedDefinitions,
+            NestedBeanPropertyDefinition.PropertyFilterDefinition propertyFilterDefinition) {
+        this(beanType);
+        if (checkNestedDefinitions) {
+            Objects.requireNonNull(propertyFilterDefinition,
+                    "You must define a property filter callback if using nested property scan.");
+            findNestedDefinitions(definitions, 0, propertyFilterDefinition);
+        }
+    }
+
+    private void findNestedDefinitions(
+            Map<String, PropertyDefinition<T, ?>> parentDefinitions, int depth,
+            NestedBeanPropertyDefinition.PropertyFilterDefinition filterCallback) {
+        if (depth >= filterCallback.getMaxNestingDepth()) {
+            return;
+        }
+        if (parentDefinitions == null) {
+            return;
+        }
+        Map<String, PropertyDefinition<T, ?>> moreProps = new HashMap<>();
+        for (String parentPropertyKey : parentDefinitions.keySet()) {
+            PropertyDefinition<T, ?> parentProperty = parentDefinitions
+                    .get(parentPropertyKey);
+            Class<?> type = parentProperty.getType();
+            if (type.getPackage() == null || type.isEnum()) {
+                continue;
+            }
+            String packageName = type.getPackage().getName();
+            if (filterCallback.getIgnorePackageNamesStartingWith().stream()
+                    .anyMatch(prefix -> packageName.startsWith(prefix))) {
+                continue;
+            }
+
+            try {
+                List<PropertyDescriptor> descriptors = BeanUtil
+                        .getBeanPropertyDescriptors(type).stream()
+                        .filter(BeanPropertySet::hasNonObjectReadMethod)
+                        .collect(Collectors.toList());
+                for (PropertyDescriptor descriptor : descriptors) {
+                    String name = parentPropertyKey + "."
+                            + descriptor.getName();
+                    PropertyDescriptor subDescriptor = BeanUtil
+                            .getPropertyDescriptor(beanType, name);
+                    moreProps.put(name, new NestedBeanPropertyDefinition<>(this,
+                            parentProperty, subDescriptor, true));
+
+                }
+            } catch (IntrospectionException e) {
+                throw new IllegalArgumentException(
+                        "Error finding nested property descriptors for "
+                                + type.getName(),
+                        e);
+            }
+        }
+        if (moreProps.size() > 0) {
+            definitions.putAll(moreProps);
+            findNestedDefinitions(moreProps, ++depth, filterCallback);
+        }
+
+    }
+
     /**
      * Gets a {@link BeanPropertySet} for the given bean type.
      *
@@ -304,10 +533,35 @@ public class BeanPropertySet<T> implements PropertySet<T> {
     @SuppressWarnings("unchecked")
     public static <T> PropertySet<T> get(Class<? extends T> beanType) {
         Objects.requireNonNull(beanType, "Bean type cannot be null");
-
+        InstanceKey key = new InstanceKey(beanType, false, 0, null);
         // Cache the reflection results
-        return (PropertySet<T>) INSTANCES.computeIfAbsent(beanType,
-                BeanPropertySet::new);
+        return (PropertySet<T>) INSTANCES.computeIfAbsent(key,
+                ignored -> new BeanPropertySet<>(beanType));
+    }
+
+    /**
+     * Gets a {@link BeanPropertySet} for the given bean type.
+     *
+     * @param beanType
+     *            the bean type to get a property set for, not <code>null</code>
+     * @param checkNestedDefinitions
+     *            whether to scan for nested definitions in beanType
+     * @param filterDefinition
+     *            filtering conditions for nested properties
+     * @return the bean property set, not <code>null</code>
+     * @since
+     */
+    @SuppressWarnings("unchecked")
+    public static <T> PropertySet<T> get(Class<? extends T> beanType,
+            boolean checkNestedDefinitions,
+            NestedBeanPropertyDefinition.PropertyFilterDefinition filterDefinition) {
+        Objects.requireNonNull(beanType, "Bean type cannot be null");
+        InstanceKey key = new InstanceKey(beanType, false,
+                filterDefinition.getMaxNestingDepth(),
+                filterDefinition.getIgnorePackageNamesStartingWith());
+        return (PropertySet<T>) INSTANCES.computeIfAbsent(key,
+                k -> new BeanPropertySet<>(beanType, checkNestedDefinitions,
+                        filterDefinition));
     }
 
     @Override
diff --git a/server/src/main/java/com/vaadin/data/Binder.java b/server/src/main/java/com/vaadin/data/Binder.java
index 03a6c1eb52..c7cce9cb95 100644
--- a/server/src/main/java/com/vaadin/data/Binder.java
+++ b/server/src/main/java/com/vaadin/data/Binder.java
@@ -38,6 +38,7 @@ import java.util.stream.Stream;
 
 import com.googlecode.gentyref.GenericTypeReflector;
 import com.vaadin.annotations.PropertyId;
+import com.vaadin.data.BeanPropertySet.NestedBeanPropertyDefinition.PropertyFilterDefinition;
 import com.vaadin.data.HasValue.ValueChangeEvent;
 import com.vaadin.data.HasValue.ValueChangeListener;
 import com.vaadin.data.converter.StringToIntegerConverter;
@@ -1064,8 +1065,10 @@ public class Binder<BEAN> implements Serializable {
         }
 
         private FIELDVALUE convertDataToFieldType(BEAN bean) {
-            return converterValidatorChain.convertToPresentation(
-                    getter.apply(bean), createValueContext());
+            TARGET target = getter.apply(bean);
+            ValueContext valueContext = createValueContext();
+            return converterValidatorChain.convertToPresentation(target,
+                    valueContext);
         }
 
         /**
@@ -1267,6 +1270,21 @@ public class Binder<BEAN> implements Serializable {
     }
 
     /**
+     * Creates a new binder that uses reflection based on the provided bean type
+     * to resolve bean properties.
+     *
+     * @param beanType
+     *            the bean type to use, not {@code null}
+     * @param scanNestedDefinitions
+     *            if {@code true}, scan for nested property definitions as well
+     * @since
+     */
+    public Binder(Class<BEAN> beanType, boolean scanNestedDefinitions) {
+        this(BeanPropertySet.get(beanType, scanNestedDefinitions,
+                PropertyFilterDefinition.getDefaultFilter()));
+    }
+
+    /**
      * Creates a new binder without support for creating bindings based on
      * property names. Use an alternative constructor, such as
      * {@link Binder#Binder(Class)}, to create a binder that support creating
@@ -2612,7 +2630,6 @@ public class Binder<BEAN> implements Serializable {
     private Optional<PropertyDefinition<BEAN, ?>> getPropertyDescriptor(
             Field field) {
         PropertyId propertyIdAnnotation = field.getAnnotation(PropertyId.class);
-
         String propertyId;
         if (propertyIdAnnotation != null) {
             // @PropertyId(propertyId) always overrides property id
@@ -2620,9 +2637,7 @@ public class Binder<BEAN> implements Serializable {
         } else {
             propertyId = field.getName();
         }
-
         String minifiedFieldName = minifyFieldName(propertyId);
-
         return propertySet.getProperties().map(PropertyDefinition::getName)
                 .filter(name -> minifyFieldName(name).equals(minifiedFieldName))
                 .findFirst().flatMap(propertySet::getProperty);
diff --git a/server/src/test/java/com/vaadin/data/BeanPropertySetTest.java b/server/src/test/java/com/vaadin/data/BeanPropertySetTest.java
index d7bc8f4c44..6b43bdfa24 100644
--- a/server/src/test/java/com/vaadin/data/BeanPropertySetTest.java
+++ b/server/src/test/java/com/vaadin/data/BeanPropertySetTest.java
@@ -161,9 +161,7 @@ public class BeanPropertySetTest {
         PropertyDefinition<FatherAndSon, ?> definition = BeanPropertySet
                 .get(FatherAndSon.class).getProperty("father.firstName")
                 .orElseThrow(RuntimeException::new);
-
         ValueProvider<FatherAndSon, ?> getter = definition.getGetter();
-
         getter.apply(new FatherAndSon("Jon", "Doe", null, null));
     }
 
diff --git a/server/src/test/java/com/vaadin/data/BinderInstanceFieldTest.java b/server/src/test/java/com/vaadin/data/BinderInstanceFieldTest.java
index a05f2eaae4..03f98bdc08 100644
--- a/server/src/test/java/com/vaadin/data/BinderInstanceFieldTest.java
+++ b/server/src/test/java/com/vaadin/data/BinderInstanceFieldTest.java
@@ -20,12 +20,16 @@ import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNull;
 
 import java.time.LocalDate;
+import java.util.Arrays;
 
+import org.junit.Assert;
 import org.junit.Test;
 
 import com.vaadin.annotations.PropertyId;
+import com.vaadin.data.BeanPropertySet.NestedBeanPropertyDefinition.PropertyFilterDefinition;
 import com.vaadin.data.converter.StringToIntegerConverter;
 import com.vaadin.data.validator.StringLengthValidator;
+import com.vaadin.tests.data.bean.Address;
 import com.vaadin.tests.data.bean.Person;
 import com.vaadin.ui.AbstractField;
 import com.vaadin.ui.AbstractTextField;
@@ -48,6 +52,34 @@ public class BinderInstanceFieldTest {
         private DateField birthDateField;
     }
 
+    public static class BindNestedFieldsUsingAnnotation extends FormLayout {
+        @PropertyId("address.streetAddress")
+        private TextField streetAddressField;
+    }
+
+    public static class BindDeepNestedFieldsUsingAnnotation extends FormLayout {
+        @PropertyId("first.address.streetAddress")
+        private TextField firstStreetField;
+
+        @PropertyId("second.address.streetAddress")
+        private TextField secondStreetField;
+    }
+
+    public static class BindDeepNestingFieldsWithCircularStructure
+            extends FormLayout {
+        @PropertyId("child.name")
+        private TextField childName;
+
+        @PropertyId("child.child.name")
+        private TextField grandchildName;
+
+        @PropertyId("child.child.child.child.child.child.child.child.name")
+        private TextField eighthLevelGrandchildName;
+
+        @PropertyId("child.child.child.child.child.child.child.child.child.child.child.child.child.name")
+        private TextField distantGreatGrandchildName;
+    }
+
     public static class BindOnlyOneField extends FormLayout {
         private TextField firstName;
         private TextField noFieldInPerson;
@@ -138,6 +170,48 @@ public class BinderInstanceFieldTest {
 
     }
 
+    final static class Couple {
+        Person first;
+        Person second;
+
+        public Person getFirst() {
+            return first;
+        }
+
+        public Person getSecond() {
+            return second;
+        }
+
+        public void setFirst(Person first) {
+            this.first = first;
+        }
+
+        public void setSecond(Person second) {
+            this.second = second;
+        }
+    }
+
+    final class NestingStructure {
+        NestingStructure child;
+        String name;
+
+        public NestingStructure getChild() {
+            return child;
+        }
+
+        public void setChild(NestingStructure child) {
+            this.child = child;
+        }
+
+        public String getName() {
+            return name;
+        }
+
+        public void setName(String name) {
+            this.name = name;
+        }
+    }
+
     @Test
     public void bindInstanceFields_bindAllFields() {
         BindAllFields form = new BindAllFields();
@@ -318,6 +392,130 @@ public class BinderInstanceFieldTest {
     }
 
     @Test
+    public void bindInstanceFields_bindNestedFieldUsingAnnotation() {
+        BindNestedFieldsUsingAnnotation form = new BindNestedFieldsUsingAnnotation();
+        Binder<Person> binder = new Binder<>(Person.class, true);
+        binder.bindInstanceFields(form);
+
+        Person person = new Person();
+        Address address = new Address();
+        address.setStreetAddress("Foo st.");
+        person.setAddress(address);
+
+        binder.setBean(person);
+
+        Assert.assertEquals("Reading nested properties bound using annotation",
+                person.getAddress().getStreetAddress(),
+                form.streetAddressField.getValue());
+
+        form.streetAddressField.setValue("Bar ave.");
+        Assert.assertEquals("Changing nested properties bound using annotation",
+                form.streetAddressField.getValue(),
+                person.getAddress().getStreetAddress());
+    }
+
+    @Test
+    public void bindInstanceFields_bindDeepNestedFieldsUsingAnnotation() {
+        BindDeepNestedFieldsUsingAnnotation form = new BindDeepNestedFieldsUsingAnnotation();
+        Binder<Couple> binder = new Binder<>(Couple.class, true);
+        binder.bindInstanceFields(form);
+        Person first = new Person();
+        Person second = new Person();
+        Address firstAddress = new Address();
+        firstAddress.setStreetAddress("Foo st.");
+        first.setAddress(firstAddress);
+        Address secondAddress = new Address();
+        second.setAddress(secondAddress);
+        secondAddress.setStreetAddress("Bar ave.");
+        Couple couple = new Couple();
+        couple.setFirst(first);
+        couple.setSecond(second);
+
+        binder.setBean(couple);
+
+        Assert.assertEquals("Binding deep nested properties using annotation",
+                couple.first.getAddress().getStreetAddress(),
+                form.firstStreetField.getValue());
+        Assert.assertEquals(
+                "Binding parallel deep nested properties using annotation",
+                couple.second.getAddress().getStreetAddress(),
+                form.secondStreetField.getValue());
+
+        form.firstStreetField.setValue(second.getAddress().getStreetAddress());
+        Assert.assertEquals("Updating value in deep nested properties",
+                form.firstStreetField.getValue(),
+                first.getAddress().getStreetAddress());
+    }
+
+    @Test
+    public void bindInstanceFields_circular() {
+        BindDeepNestingFieldsWithCircularStructure form = new BindDeepNestingFieldsWithCircularStructure();
+        Binder<NestingStructure> binder = new Binder<>(NestingStructure.class,
+                true);
+        binder.bindInstanceFields(form);
+        NestingStructure parent = new NestingStructure();
+        parent.setName("parent");
+        NestingStructure child = new NestingStructure();
+        child.setName("child");
+        parent.setChild(child);
+        NestingStructure grandchild = new NestingStructure();
+        grandchild.setName("grandchild");
+        child.setChild(grandchild);
+        NestingStructure root = grandchild;
+        for (int i = 1; i < 15; i++) {
+            NestingStructure ns = new NestingStructure();
+            ns.setName("great " + root.getName());
+            root.setChild(ns);
+            root = ns;
+        }
+        binder.setBean(parent);
+        Assert.assertEquals(child.getName(), form.childName.getValue());
+        Assert.assertEquals(grandchild.getName(),
+                form.grandchildName.getValue());
+        Assert.assertNotNull(
+                "Reading nested properties within default supported nested depth (max 10 levels)",
+                form.eighthLevelGrandchildName);
+        // only 10 levels of nesting properties are scanned by default
+        Assert.assertNull(
+                "By default, only 10 levels of nesting properties are scanned.",
+                form.distantGreatGrandchildName);
+    }
+
+    @Test
+    public void bindInstanceFields_customNestingLevel() {
+        BindDeepNestingFieldsWithCircularStructure form = new BindDeepNestingFieldsWithCircularStructure();
+        int customScanningDepth = 5;
+        PropertyFilterDefinition shallowFilter = new PropertyFilterDefinition(
+                customScanningDepth, Arrays.asList("java.lang"));
+        Binder<NestingStructure> binder = new Binder<>(BeanPropertySet
+                .get(NestingStructure.class, true, shallowFilter));
+        binder.bindInstanceFields(form);
+        NestingStructure parent = new NestingStructure();
+        parent.setName("parent");
+        NestingStructure child = new NestingStructure();
+        child.setName("child");
+        parent.setChild(child);
+        NestingStructure grandchild = new NestingStructure();
+        grandchild.setName("grandchild");
+        child.setChild(grandchild);
+        NestingStructure root = grandchild;
+        for (int i = 1; i < 15; i++) {
+            NestingStructure ns = new NestingStructure();
+            ns.setName("great " + root.getName());
+            root.setChild(ns);
+            root = ns;
+        }
+        binder.setBean(parent);
+        Assert.assertEquals(child.getName(), form.childName.getValue());
+        Assert.assertEquals(
+                "Reading 3rd level nesting works when custom scanning depth is 5",
+                grandchild.getName(), form.grandchildName.getValue());
+        Assert.assertNull(
+                "Reading eighth level nesting doesn't work when custom scanning depth is 5",
+                form.eighthLevelGrandchildName);
+    }
+
+    @Test
     public void bindInstanceFields_bindNotBoundFieldsOnly_customBindingIsNotReplaced() {
         BindAllFields form = new BindAllFields();
         Binder<Person> binder = new Binder<>(Person.class);