Cherry picks of Binder fixes in Flow (#11758)

* Cherry picks of Binder fixes in Flow

Addresses: https://github.com/vaadin/framework/issues/9000

Addresses:  https://github.com/vaadin/framework/issues/11109

These changes are adopted from https://github.com/vaadin/flow/pull/4138 and https://github.com/vaadin/flow/pull/6757

3 files changed, 184 insertions, 21 deletions

diff --git a/server/src/main/java/com/vaadin/data/Binder.java b/server/src/main/java/com/vaadin/data/Binder.java
index 22ef7d7782..699185dea3 100644
--- a/server/src/main/java/com/vaadin/data/Binder.java
+++ b/server/src/main/java/com/vaadin/data/Binder.java
@@ -816,7 +816,7 @@ public class Binder<BEAN> implements Serializable {
 
             getBinder().bindings.add(binding);
             if (getBinder().getBean() != null) {
-                binding.initFieldValue(getBinder().getBean());
+                binding.initFieldValue(getBinder().getBean(), true);
             }
             if (setter == null) {
                 binding.getField().setReadOnly(true);
@@ -1146,20 +1146,33 @@ public class Binder<BEAN> implements Serializable {
          *
          * @param bean
          *            the bean to fetch the property value from
+         * @param writeBackChangedValues
+         *            <code>true</code> if the bean value should be updated if
+         *            the value is different after converting to and from the
+         *            presentation value; <code>false</code> to avoid updating
+         *            the bean value
          */
-        private void initFieldValue(BEAN bean) {
+        private void initFieldValue(BEAN bean, boolean writeBackChangedValues) {
             assert bean != null;
             assert onValueChange != null;
             valueInit = true;
             try {
-                getField().setValue(convertDataToFieldType(bean));
+                TARGET originalValue = getter.apply(bean);
+                convertAndSetFieldValue(originalValue);
+
+                if (writeBackChangedValues && setter != null) {
+                    doConversion().ifOk(convertedValue -> {
+                        if (!Objects.equals(originalValue, convertedValue)) {
+                            setter.accept(bean, convertedValue);
+                        }
+                    });
+                }
             } finally {
                 valueInit = false;
             }
         }
 
-        private FIELDVALUE convertDataToFieldType(BEAN bean) {
-            TARGET target = getter.apply(bean);
+        private FIELDVALUE convertToFieldType(TARGET target) {
             ValueContext valueContext = createValueContext();
             return converterValidatorChain.convertToPresentation(target,
                     valueContext);
@@ -1218,7 +1231,31 @@ public class Binder<BEAN> implements Serializable {
 
         @Override
         public void read(BEAN bean) {
-            getField().setValue(convertDataToFieldType(bean));
+            convertAndSetFieldValue(getter.apply(bean));
+        }
+
+        private void convertAndSetFieldValue(TARGET modelValue) {
+            FIELDVALUE convertedValue = convertToFieldType(modelValue);
+            try {
+                getField().setValue(convertedValue);
+            } catch (RuntimeException e) {
+                /*
+                 * Add an additional hint to the exception for the typical case
+                 * with a field that doesn't accept null values. The non-null
+                 * empty value is used as a heuristic to determine that the
+                 * field doesn't accept null rather than throwing for some other
+                 * reason.
+                 */
+                if (convertedValue == null && getField().getEmptyValue() != null) {
+                    throw new IllegalStateException(String.format(
+                            "A field of type %s didn't accept a null value."
+                                    + " If null values are expected, then configure a null representation for the binding.",
+                            field.getClass().getName()), e);
+                } else {
+                    // Otherwise, let the original exception speak for itself
+                    throw e;
+                }
+            }
         }
 
         @Override
@@ -1639,6 +1676,10 @@ public class Binder<BEAN> implements Serializable {
      * Any change made in the fields also runs validation for the field
      * {@link Binding} and bean level validation for this binder (bean level
      * validators are added using {@link Binder#withValidator(Validator)}.
+     * <p>
+     * After updating each field, the value is read back from the field and the
+     * bean's property value is updated if it has been changed from the original
+     * value by the field or a converter.     
      *
      * @see #readBean(Object)
      * @see #writeBean(Object)
@@ -1658,7 +1699,7 @@ public class Binder<BEAN> implements Serializable {
         } else {
             doRemoveBean(false);
             this.bean = bean;
-            getBindings().forEach(b -> b.initFieldValue(bean));
+            getBindings().forEach(b -> b.initFieldValue(bean, true));
             // if there has been field value change listeners that trigger
             // validation, need to make sure the validation errors are cleared
             getValidationStatusHandler().statusChange(
@@ -1706,7 +1747,7 @@ public class Binder<BEAN> implements Serializable {
                 // we unbind a binding in valueChangeListener of another
                 // field.
                 if (binding.getField() != null)
-                    binding.initFieldValue(bean);
+                    binding.initFieldValue(bean, false);
             });
             getValidationStatusHandler().statusChange(
                     BinderValidationStatus.createUnresolvedStatus(this));
diff --git a/server/src/test/java/com/vaadin/data/BinderComponentTest.java b/server/src/test/java/com/vaadin/data/BinderComponentTest.java
index 776816be5e..ab5122cc76 100644
--- a/server/src/test/java/com/vaadin/data/BinderComponentTest.java
+++ b/server/src/test/java/com/vaadin/data/BinderComponentTest.java
@@ -66,17 +66,13 @@ public class BinderComponentTest
 
     private <T> void testFieldNullRepresentation(T initialValue,
             HasValue<T> field) {
-        binder.bind(field, t -> null, (str, val) -> {
-            assertEquals("Value update with initial value failed.",
-                    initialValue, field.getValue());
-        });
+        binder.bind(field, t -> null, (str, val) -> {});
         field.setValue(initialValue);
         assertEquals("Initial value of field unexpected", initialValue,
                 field.getValue());
         binder.setBean(item);
         assertEquals("Null representation for field failed",
                 field.getEmptyValue(), field.getValue());
-        field.setValue(initialValue);
     }
 
 }
diff --git a/server/src/test/java/com/vaadin/data/BinderTest.java b/server/src/test/java/com/vaadin/data/BinderTest.java
index f4515dc211..3e27cc5f9b 100644
--- a/server/src/test/java/com/vaadin/data/BinderTest.java
+++ b/server/src/test/java/com/vaadin/data/BinderTest.java
@@ -8,16 +8,20 @@ import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertSame;
 import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
 
 import java.util.Locale;
 import java.util.Objects;
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicInteger;
+import java.util.concurrent.atomic.AtomicReference;
 import java.util.stream.Stream;
 
 import org.junit.Assert;
 import org.junit.Before;
 import org.junit.Test;
+import org.junit.Rule;
+import org.junit.rules.ExpectedException;
 
 import com.vaadin.data.Binder.Binding;
 import com.vaadin.data.Binder.BindingBuilder;
@@ -31,9 +35,17 @@ import com.vaadin.tests.data.bean.Person;
 import com.vaadin.tests.data.bean.Sex;
 import com.vaadin.ui.TextField;
 import org.apache.commons.lang.StringUtils;
+import org.hamcrest.CoreMatchers;
 
 public class BinderTest extends BinderTestBase<Binder<Person>, Person> {
 
+    @Rule
+    /*
+     * transient to avoid interfering with serialization tests that capture a
+     * test instance in a closure
+     */
+    public transient ExpectedException exceptionRule = ExpectedException.none();
+
     @Before
     public void setUp() {
         binder = new Binder<>();
@@ -333,7 +345,7 @@ public class BinderTest extends BinderTestBase<Binder<Person>, Person> {
         binder.setBean(namelessPerson);
 
         assertTrue(nullTextField.isEmpty());
-        assertEquals(null, namelessPerson.getFirstName());
+        assertEquals("null", namelessPerson.getFirstName());
 
         // Change value, see that textfield is not empty and bean is updated.
         nullTextField.setValue("");
@@ -525,7 +537,7 @@ public class BinderTest extends BinderTestBase<Binder<Person>, Person> {
         binding.bind(Person::getFirstName, Person::setFirstName);
         binder.setBean(item);
         assertNull(textField.getErrorMessage());
-        assertEquals(0, invokes.get());
+        assertEquals(1, invokes.get());
 
         textField.setValue("        ");
         ErrorMessage errorMessage = textField.getErrorMessage();
@@ -533,7 +545,7 @@ public class BinderTest extends BinderTestBase<Binder<Person>, Person> {
         assertEquals("Input&#32;is&#32;required&#46;",
                 errorMessage.getFormattedHtmlMessage());
         // validation is done for all changed bindings once.
-        assertEquals(1, invokes.get());
+        assertEquals(2, invokes.get());
 
         textField.setValue("value");
         assertNull(textField.getErrorMessage());
@@ -582,7 +594,7 @@ public class BinderTest extends BinderTestBase<Binder<Person>, Person> {
 
         binder.setBean(item);
         assertNull(textField.getErrorMessage());
-        assertEquals(0, invokes.get());
+        assertEquals(1, invokes.get());
 
         textField.setValue("        ");
         ErrorMessage errorMessage = textField.getErrorMessage();
@@ -590,7 +602,7 @@ public class BinderTest extends BinderTestBase<Binder<Person>, Person> {
         assertEquals("Input&#32;required&#46;",
                 errorMessage.getFormattedHtmlMessage());
         // validation is done for all changed bindings once.
-        assertEquals(1, invokes.get());
+        assertEquals(2, invokes.get());
 
         textField.setValue("value");
         assertNull(textField.getErrorMessage());
@@ -1099,12 +1111,12 @@ public class BinderTest extends BinderTestBase<Binder<Person>, Person> {
 
         binder.setBean(item);
         ageField.setValue("3");
-        Assert.assertEquals(infoMessage,
+        assertEquals(infoMessage,
                 ageField.getComponentError().getFormattedHtmlMessage());
-        Assert.assertEquals(ErrorLevel.INFO,
+        assertEquals(ErrorLevel.INFO,
                 ageField.getComponentError().getErrorLevel());
 
-        Assert.assertEquals(3, item.getAge());
+        assertEquals(3, item.getAge());
     }
 
     @Test
@@ -1246,4 +1258,118 @@ public class BinderTest extends BinderTestBase<Binder<Person>, Person> {
 
         nameField.setValue("Foo");
     }
+
+    @Test
+    public void nonSymetricValue_setBean_writtenToBean() {
+        binder.bind(nameField, Person::getLastName, Person::setLastName);
+
+        assertNull(item.getLastName());
+
+        binder.setBean(item);
+
+        assertEquals("", item.getLastName());
+    }
+
+    @Test
+    public void nonSymmetricValue_readBean_beanNotTouched() {
+        binder.bind(nameField, Person::getLastName, Person::setLastName);
+        binder.addValueChangeListener(
+                event -> fail("No value change event should be fired"));
+
+        assertNull(item.getLastName());
+
+        binder.readBean(item);
+
+        assertNull(item.getLastName());
+    }
+
+    @Test
+    public void symetricValue_setBean_beanNotUpdated() {
+        binder.bind(nameField, Person::getFirstName, Person::setFirstName);
+
+        binder.setBean(new Person() {
+            @Override
+            public String getFirstName() {
+                return "First";
+            }
+
+            @Override
+            public void setFirstName(String firstName) {
+                fail("Setter should not be called");
+            }
+        });
+    }
+
+    @Test
+    public void nullRejetingField_nullValue_wrappedExceptionMentionsNullRepresentation() {
+        TextField field = createNullAnd42RejectingFieldWithEmptyValue("");
+
+        Binder<AtomicReference<Integer>> binder = createIntegerConverterBinder(
+                field);
+
+        exceptionRule.expect(IllegalStateException.class);
+        exceptionRule.expectMessage("null representation");
+        exceptionRule.expectCause(CoreMatchers.isA(NullPointerException.class));
+
+        binder.readBean(new AtomicReference<>());
+    }
+
+
+    @Test
+    public void nullRejetingField_otherRejectedValue_originalExceptionIsThrown() {
+        TextField field = createNullAnd42RejectingFieldWithEmptyValue("");
+
+        Binder<AtomicReference<Integer>> binder = createIntegerConverterBinder(
+                field);
+
+        exceptionRule.expect(IllegalArgumentException.class);
+        exceptionRule.expectMessage("42");
+
+        binder.readBean(new AtomicReference<>(Integer.valueOf(42)));
+    }
+
+    @Test(expected = NullPointerException.class)
+    public void nullAcceptingField_nullValue_originalExceptionIsThrown() {
+        /*
+         * Edge case with a field that throws for null but has null as the empty
+         * value. This is most likely the case if the field doesn't explicitly
+         * reject null values but is instead somehow broken so that any value is
+         * rejected.
+         */
+        TextField field = createNullAnd42RejectingFieldWithEmptyValue(null);
+
+        Binder<AtomicReference<Integer>> binder = createIntegerConverterBinder(
+                field);
+
+        binder.readBean(new AtomicReference<>(null));
+    }
+
+    private TextField createNullAnd42RejectingFieldWithEmptyValue(
+            String emptyValue) {
+        return new TextField() {
+            @Override
+            public void setValue(String value) {
+                if (value == null) {
+                    throw new NullPointerException("Null value");
+                } else if ("42".equals(value)) {
+                    throw new IllegalArgumentException("42 is not allowed");
+                }
+                super.setValue(value);
+            }
+
+            @Override
+            public String getEmptyValue() {
+                return emptyValue;
+            }
+        };
+    }
+
+    private Binder<AtomicReference<Integer>> createIntegerConverterBinder(
+            TextField field) {
+        Binder<AtomicReference<Integer>> binder = new Binder<>();
+        binder.forField(field)
+                .withConverter(new StringToIntegerConverter("Must have number"))
+                .bind(AtomicReference::get, AtomicReference::set);
+        return binder;
+    }
 }