Give hint about missing null representation in error message (#6757)

A binding doesn't have an automatic null representation configured if a
converter is also defined. This may cause surprises with fields that
don't accept null values, e.g. TextField.

The easiest fix in this situation is to explicitly define a null
representation. This patch adds an error message that reminds the
developer about the existence of that feature so that they wouldn't have
to make more elaborate workarounds.

Fixes #6746

(cherry picked from commit 3e30650)

flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java

@@ -37,10 +37,10 @@
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
+import com.googlecode.gentyref.GenericTypeReflector;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
-import com.googlecode.gentyref.GenericTypeReflector;
 import com.vaadin.flow.component.Component;
 import com.vaadin.flow.component.HasText;
 import com.vaadin.flow.component.HasValidation;
@@ -1047,8 +1047,8 @@ public BindingValidationStatus<TARGET> validate(boolean fireEvent) {
 
         /**
          * Removes this binding from its binder and unregisters the
-         * {@code ValueChangeListener} from any bound {@code HasValue}.
-         * It does nothing if it is called for an already unbound binding.
+         * {@code ValueChangeListener} from any bound {@code HasValue}. It does
+         * nothing if it is called for an already unbound binding.
          */
         @Override
         public void unbind() {
@@ -1057,7 +1057,7 @@ public void unbind() {
                 onValueChange = null;
             }
 
-            if(binder != null) {
+            if (binder != null) {
                 binder.removeBindingInternal(this);
                 binder = null;
             }
@@ -1130,7 +1130,7 @@ private void initFieldValue(BEAN bean, boolean writeBackChangedValues) {
             valueInit = true;
             try {
                 TARGET originalValue = getter.apply(bean);
-                field.setValue(convertToFieldType(originalValue));
+                convertAndSetFieldValue(originalValue);
 
                 if (writeBackChangedValues && setter != null) {
                     doConversion().ifOk(convertedValue -> {
@@ -1203,7 +1203,31 @@ public BindingValidationStatusHandler getValidationStatusHandler() {
 
         @Override
         public void read(BEAN bean) {
-            field.setValue(convertToFieldType(getter.apply(bean)));
+            convertAndSetFieldValue(getter.apply(bean));
+        }
+
+        private void convertAndSetFieldValue(TARGET modelValue) {
+            FIELDVALUE convertedValue = convertToFieldType(modelValue);
+            try {
+                field.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 && field.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
@@ -1693,16 +1717,17 @@ public void readBean(BEAN bean) {
             clearFields();
         } else {
             changedBindings.clear();
-            getBindings()
-                    .forEach(binding -> {
-                        // Some bindings may have been removed from binder
-                        // during readBean. We should skip those bindings to
-                        // avoid NPE inside initFieldValue. It happens e.g. when
-                        // we unbind a binding in valueChangeListener of another
-                        // field.
-                        if(binding.getField() != null)
-                            binding.initFieldValue(bean, false);
-                    });
+            getBindings().forEach(binding -> {
+                /*
+                 * Some bindings may have been removed from binder during
+                 * readBean. We should skip those bindings to avoid NPE inside
+                 * initFieldValue. It happens e.g. when we unbind a binding in
+                 * valueChangeListener of another field.
+                 */
+                if (binding.getField() != null) {
+                    binding.initFieldValue(bean, false);
+                }
+            });
             getValidationStatusHandler().statusChange(
                     BinderValidationStatus.createUnresolvedStatus(this));
             fireStatusChangeEvent(false);

flow-data/src/test/java/com/vaadin/flow/data/binder/BinderTest.java

@@ -22,13 +22,17 @@
 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.apache.commons.lang3.StringUtils;
+import org.hamcrest.CoreMatchers;
 import org.junit.After;
 import org.junit.Assert;
 import org.junit.Before;
+import org.junit.Rule;
 import org.junit.Test;
+import org.junit.rules.ExpectedException;
 
 import com.vaadin.flow.component.HasValue;
 import com.vaadin.flow.component.UI;
@@ -59,6 +63,13 @@ public class BinderTest extends BinderTestBase<Binder<Person>, Person> {
 
     private Map<HasValue<?, ?>, String> componentErrors = new HashMap<>();
 
+    @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<Person>() {
@@ -457,9 +468,8 @@ public void beanBinder_withConverter_nullRepresentationIsNotDisabled() {
         String customNullPointerRepresentation = "foo";
         Binder<Person> binder = new Binder<>(Person.class);
         binder.forField(nameField)
-                .withConverter(value -> value,
-                        value -> value == null ? customNullPointerRepresentation
-                                : value)
+                .withConverter(value -> value, value -> value == null
+                        ? customNullPointerRepresentation : value)
                 .bind("firstName");
 
         Person person = new Person();
@@ -1337,4 +1347,79 @@ public void valueChangeListenerOrder() {
 
         nameField.setValue("Foo");
     }
+
+    @Test
+    public void nullRejetingField_nullValue_wrappedExceptionMentionsNullRepresentation() {
+        TestTextField field = createNullRejectingFieldWithEmptyValue("");
+
+        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() {
+        TestTextField field = createNullRejectingFieldWithEmptyValue("");
+
+        Binder<AtomicReference<Integer>> binder = createIntegerConverterBinder(
+                field);
+
+        exceptionRule.expect(IllegalArgumentException.class);
+        exceptionRule.expectMessage("42");
+
+        binder.readBean(new AtomicReference<>(Integer.valueOf(42)));
+    }
+
+    @Test
+    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.
+         */
+        TestTextField field = createNullRejectingFieldWithEmptyValue(null);
+
+        Binder<AtomicReference<Integer>> binder = createIntegerConverterBinder(
+                field);
+
+        exceptionRule.expect(NullPointerException.class);
+
+        binder.readBean(new AtomicReference<>(null));
+    }
+
+    private TestTextField createNullRejectingFieldWithEmptyValue(
+            String emptyValue) {
+        return new TestTextField() {
+            @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(
+            TestTextField field) {
+        Binder<AtomicReference<Integer>> binder = new Binder<>();
+        binder.forField(field)
+                .withConverter(new StringToIntegerConverter("Must have number"))
+                .bind(AtomicReference::get, AtomicReference::set);
+        return binder;
+    }
 }