Automatically complete custom bindings in BeanBinder#bindInstanceFields (#8012)

Closes vaadin/framework8-issues#511

3 files changed, 179 insertions, 3 deletions

diff --git a/server/src/main/java/com/vaadin/data/BeanBinder.java b/server/src/main/java/com/vaadin/data/BeanBinder.java
index 8f79ff4f88..898f2cd37d 100644
--- a/server/src/main/java/com/vaadin/data/BeanBinder.java
+++ b/server/src/main/java/com/vaadin/data/BeanBinder.java
@@ -25,8 +25,10 @@ import java.lang.reflect.Type;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.HashSet;
+import java.util.IdentityHashMap;
 import java.util.List;
 import java.util.Locale;
+import java.util.Map;
 import java.util.Objects;
 import java.util.Optional;
 import java.util.Set;
@@ -226,6 +228,7 @@ public class BeanBinder<BEAN> extends Binder<BEAN> {
                     errorMessageProvider);
         }
 
+        @SuppressWarnings("unchecked")
         @Override
         public Binding<BEAN, TARGET> bind(String propertyName) {
             checkUnbound();
@@ -252,6 +255,7 @@ public class BeanBinder<BEAN> extends Binder<BEAN> {
                                 value));
             } finally {
                 getBinder().boundProperties.add(propertyName);
+                getBinder().incompleteMemberFieldBindings.remove(getField());
             }
         }
 
@@ -315,7 +319,8 @@ public class BeanBinder<BEAN> extends Binder<BEAN> {
     }
 
     private final Class<? extends BEAN> beanType;
-    private final Set<String> boundProperties;
+    private final Set<String> boundProperties = new HashSet<>();
+    private final Map<HasValue<?>, BeanBindingImpl<BEAN, ?, ?>> incompleteMemberFieldBindings = new IdentityHashMap<>();
 
     /**
      * Creates a new {@code BeanBinder} supporting beans of the given type.
@@ -326,7 +331,6 @@ public class BeanBinder<BEAN> extends Binder<BEAN> {
     public BeanBinder(Class<? extends BEAN> beanType) {
         BeanUtil.checkBeanValidationAvailable();
         this.beanType = beanType;
-        boundProperties = new HashSet<>();
     }
 
     @Override
@@ -336,6 +340,29 @@ public class BeanBinder<BEAN> extends Binder<BEAN> {
     }
 
     /**
+     * Creates a new binding for the given field. The returned builder may be
+     * further configured before invoking {@link #bindInstanceFields(Object)}.
+     * Unlike with the {@link #forField(HasValue)} method, no explicit call to
+     * {@link BeanBindingBuilder#bind(String)} is needed to complete this
+     * binding in the case that the name of the field matches a field name found
+     * in the bean.
+     *
+     * @param <FIELDVALUE>
+     *            the value type of the field
+     * @param field
+     *            the field to be bound, not null
+     * @return the new binding builder
+     * 
+     * @see #forField(HasValue)
+     * @see #bindInstanceFields(Object)
+     */
+    public <FIELDVALUE> BeanBindingBuilder<BEAN, FIELDVALUE> forMemberField(
+            HasValue<FIELDVALUE> field) {
+        incompleteMemberFieldBindings.put(field, null);
+        return forField(field);
+    }
+
+    /**
      * Binds the given field to the property with the given name. The getter and
      * setter methods of the property are looked up with bean introspection and
      * used to read and write the property value.
@@ -372,12 +399,28 @@ public class BeanBinder<BEAN> extends Binder<BEAN> {
         return (BeanBinder<BEAN>) super.withValidator(validator);
     }
 
+    @SuppressWarnings("unchecked")
     @Override
     protected <FIELDVALUE, TARGET> BeanBindingImpl<BEAN, FIELDVALUE, TARGET> createBinding(
             HasValue<FIELDVALUE> field, Converter<FIELDVALUE, TARGET> converter,
             BindingValidationStatusHandler handler) {
         Objects.requireNonNull(field, "field cannot be null");
         Objects.requireNonNull(converter, "converter cannot be null");
+        if (incompleteMemberFieldBindings.containsKey(field)) {
+            BeanBindingImpl<BEAN, FIELDVALUE, TARGET> newBinding = doCreateBinding(
+                    field, converter, handler);
+            incompleteMemberFieldBindings.put(field, newBinding);
+            return newBinding;
+        } else {
+            return (BeanBindingImpl<BEAN, FIELDVALUE, TARGET>) super.createBinding(
+                    field, converter, handler);
+        }
+    }
+
+    @Override
+    protected <FIELDVALUE, TARGET> BeanBindingImpl<BEAN, FIELDVALUE, TARGET> doCreateBinding(
+            HasValue<FIELDVALUE> field, Converter<FIELDVALUE, TARGET> converter,
+            BindingValidationStatusHandler handler) {
         return new BeanBindingImpl<>(this, field, converter, handler);
     }
 
@@ -431,10 +474,25 @@ public class BeanBinder<BEAN> extends Binder<BEAN> {
                 .filter(memberField -> HasValue.class
                         .isAssignableFrom(memberField.getType()))
                 .forEach(memberField -> handleProperty(memberField,
+                        objectWithMemberFields,
                         (property, type) -> bindProperty(objectWithMemberFields,
                                 memberField, property, type)));
     }
 
+    @SuppressWarnings("unchecked")
+    private BeanBindingImpl<BEAN, ?, ?> getIncompleteMemberFieldBinding(
+            Field memberField, Object objectWithMemberFields) {
+        memberField.setAccessible(true);
+        try {
+            return incompleteMemberFieldBindings
+                    .get(memberField.get(objectWithMemberFields));
+        } catch (IllegalArgumentException | IllegalAccessException e) {
+            throw new RuntimeException(e);
+        } finally {
+            memberField.setAccessible(false);
+        }
+    }
+
     /**
      * Binds {@code property} with {@code propertyType} to the field in the
      * {@code objectWithMemberFields} instance using {@code memberField} as a
@@ -537,6 +595,17 @@ public class BeanBinder<BEAN> extends Binder<BEAN> {
         return memberFieldInOrder;
     }
 
+    @Override
+    protected void checkBindingsCompleted(String methodName) {
+        if (!incompleteMemberFieldBindings.isEmpty()) {
+            throw new IllegalStateException(
+                    "All bindings created with forMemberField must "
+                            + "be completed with bindInstanceFields before calling "
+                            + methodName);
+        }
+        super.checkBindingsCompleted(methodName);
+    }
+
     private void initializeField(Object objectWithMemberFields,
             Field memberField, HasValue<?> value) {
         try {
@@ -551,8 +620,9 @@ public class BeanBinder<BEAN> extends Binder<BEAN> {
         }
     }
 
-    private void handleProperty(Field field,
+    private void handleProperty(Field field, Object objectWithMemberFields,
             BiConsumer<String, Class<?>> propertyHandler) {
+
         Optional<PropertyDescriptor> descriptor = getPropertyDescriptor(field);
 
         if (!descriptor.isPresent()) {
@@ -564,6 +634,13 @@ public class BeanBinder<BEAN> extends Binder<BEAN> {
             return;
         }
 
+        BeanBindingImpl<BEAN, ?, ?> tentativeBinding = getIncompleteMemberFieldBinding(
+                field, objectWithMemberFields);
+        if (tentativeBinding != null) {
+            tentativeBinding.bind(propertyName);
+            return;
+        }
+
         propertyHandler.accept(propertyName,
                 descriptor.get().getPropertyType());
         boundProperties.add(propertyName);
diff --git a/server/src/main/java/com/vaadin/data/Binder.java b/server/src/main/java/com/vaadin/data/Binder.java
index fab163a1c5..e818894f15 100644
--- a/server/src/main/java/com/vaadin/data/Binder.java
+++ b/server/src/main/java/com/vaadin/data/Binder.java
@@ -132,6 +132,13 @@ public class Binder<BEAN> implements Serializable {
     public interface BindingBuilder<BEAN, TARGET> extends Serializable {
 
         /**
+         * Gets the field the binding is being built for.
+         *
+         * @return the field this binding is being built for
+         */
+        public HasValue<?> getField();
+
+        /**
          * Completes this binding using the given getter and setter functions
          * representing a backing bean property. The functions are used to
          * update the field value from the property and to store the field value
@@ -547,6 +554,7 @@ public class Binder<BEAN> implements Serializable {
             getBinder().fireStatusChangeEvent(false);
 
             bound = true;
+            getBinder().incompleteBindings.remove(getField());
 
             return binding;
         }
@@ -650,6 +658,11 @@ public class Binder<BEAN> implements Serializable {
                         "cannot modify binding: already bound to a property");
             }
         }
+
+        @Override
+        public HasValue<FIELDVALUE> getField() {
+            return field;
+        }
     }
 
     /**
@@ -951,6 +964,8 @@ public class Binder<BEAN> implements Serializable {
 
     private final Set<BindingImpl<BEAN, ?, ?>> bindings = new LinkedHashSet<>();
 
+    private final Map<HasValue<?>, BindingBuilder<BEAN, ?>> incompleteBindings = new IdentityHashMap<>();
+
     private final List<Validator<? super BEAN>> validators = new ArrayList<>();
 
     private final Map<HasValue<?>, ConverterDelegate<?>> initialConverters = new IdentityHashMap<>();
@@ -1085,6 +1100,7 @@ public class Binder<BEAN> implements Serializable {
      *            bean
      */
     public void setBean(BEAN bean) {
+        checkBindingsCompleted("setBean");
         if (bean == null) {
             if (this.bean != null) {
                 doRemoveBean(true);
@@ -1128,6 +1144,7 @@ public class Binder<BEAN> implements Serializable {
      */
     public void readBean(BEAN bean) {
         Objects.requireNonNull(bean, "bean cannot be null");
+        checkBindingsCompleted("readBean");
         setHasChanges(false);
         bindings.forEach(binding -> binding.initFieldValue(bean));
 
@@ -1510,6 +1527,15 @@ public class Binder<BEAN> implements Serializable {
     protected <FIELDVALUE, TARGET> BindingBuilder<BEAN, TARGET> createBinding(
             HasValue<FIELDVALUE> field, Converter<FIELDVALUE, TARGET> converter,
             BindingValidationStatusHandler handler) {
+        BindingBuilder<BEAN, TARGET> newBinding = doCreateBinding(field,
+                converter, handler);
+        incompleteBindings.put(field, newBinding);
+        return newBinding;
+    }
+
+    protected <FIELDVALUE, TARGET> BindingBuilder<BEAN, TARGET> doCreateBinding(
+            HasValue<FIELDVALUE> field, Converter<FIELDVALUE, TARGET> converter,
+            BindingValidationStatusHandler handler) {
         return new BindingBuilderImpl<>(this, field, converter, handler);
     }
 
@@ -1694,4 +1720,19 @@ public class Binder<BEAN> implements Serializable {
         return converter;
     }
 
+    /**
+     * Throws if this binder has incomplete bindings.
+     * 
+     * @param methodName
+     *            name of the method where this call is originated from
+     * @throws IllegalStateException
+     *             if this binder has incomplete bindings
+     */
+    protected void checkBindingsCompleted(String methodName) {
+        if (!incompleteBindings.isEmpty()) {
+            throw new IllegalStateException(
+                    "All bindings created with forField must be completed before calling "
+                            + methodName);
+        }
+    }
 }
diff --git a/server/src/test/java/com/vaadin/data/BeanBinderTest.java b/server/src/test/java/com/vaadin/data/BeanBinderTest.java
index 1b9f2f0ac7..3433aee1a8 100644
--- a/server/src/test/java/com/vaadin/data/BeanBinderTest.java
+++ b/server/src/test/java/com/vaadin/data/BeanBinderTest.java
@@ -13,8 +13,10 @@ import org.junit.Test;
 
 import com.vaadin.data.BeanBinder.BeanBindingBuilder;
 import com.vaadin.data.Binder.BindingBuilder;
+import com.vaadin.data.converter.StringToIntegerConverter;
 import com.vaadin.tests.data.bean.BeanToValidate;
 import com.vaadin.ui.CheckBoxGroup;
+import com.vaadin.ui.TextField;
 
 public class BeanBinderTest
         extends BinderTestBase<BeanBinder<BeanToValidate>, BeanToValidate> {
@@ -24,10 +26,12 @@ public class BeanBinderTest
 
     private class TestClass {
         private CheckBoxGroup<TestEnum> enums;
+        private TextField number = new TextField();
     }
 
     private class TestBean {
         private Set<TestEnum> enums;
+        private int number;
 
         public Set<TestEnum> getEnums() {
             return enums;
@@ -36,6 +40,14 @@ public class BeanBinderTest
         public void setEnums(Set<TestEnum> enums) {
             this.enums = enums;
         }
+
+        public int getNumber() {
+            return number;
+        }
+
+        public void setNumber(int number) {
+            this.number = number;
+        }
     }
 
     @Before
@@ -50,9 +62,55 @@ public class BeanBinderTest
     public void bindInstanceFields_parameters_type_erased() {
         BeanBinder<TestBean> otherBinder = new BeanBinder<>(TestBean.class);
         TestClass testClass = new TestClass();
+        otherBinder.forField(testClass.number)
+                .withConverter(new StringToIntegerConverter(""))
+                .bind("number");
+
+        // Should correctly bind the enum field without throwing
+        otherBinder.bindInstanceFields(testClass);
+    }
+
+    @Test
+    public void bindInstanceFields_automatically_binds_incomplete_forMemberField_bindings() {
+        BeanBinder<TestBean> otherBinder = new BeanBinder<>(TestBean.class);
+        TestClass testClass = new TestClass();
+
+        otherBinder.forMemberField(testClass.number)
+                .withConverter(new StringToIntegerConverter(""));
+        otherBinder.bindInstanceFields(testClass);
+
+        TestBean bean = new TestBean();
+        otherBinder.setBean(bean);
+        testClass.number.setValue("50");
+        assertEquals(50, bean.number);
+    }
+
+    @Test(expected = IllegalStateException.class)
+    public void bindInstanceFields_does_not_automatically_bind_incomplete_forField_bindings() {
+        BeanBinder<TestBean> otherBinder = new BeanBinder<>(TestBean.class);
+        TestClass testClass = new TestClass();
+
+        otherBinder.forField(testClass.number)
+                .withConverter(new StringToIntegerConverter(""));
+
+        // Should throw an IllegalStateException since the binding for number is
+        // not completed with bind
         otherBinder.bindInstanceFields(testClass);
     }
 
+    @Test(expected = IllegalStateException.class)
+    public void incomplete_forMemberField_bindings() {
+        BeanBinder<TestBean> otherBinder = new BeanBinder<>(TestBean.class);
+        TestClass testClass = new TestClass();
+
+        otherBinder.forMemberField(testClass.number)
+                .withConverter(new StringToIntegerConverter(""));
+
+        // Should throw an IllegalStateException since the forMemberField
+        // binding has not been completed
+        otherBinder.setBean(new TestBean());
+    }
+
     @Test
     public void fieldBound_bindBean_fieldValueUpdated() {
         binder.bind(nameField, "firstname");