Fixing NPE in Binder, ported from Flow (#11290)

2 files changed, 73 insertions, 5 deletions

diff --git a/server/src/main/java/com/vaadin/data/Binder.java b/server/src/main/java/com/vaadin/data/Binder.java
index 88764a6a2f..d2aff412f6 100644
--- a/server/src/main/java/com/vaadin/data/Binder.java
+++ b/server/src/main/java/com/vaadin/data/Binder.java
@@ -1005,7 +1005,7 @@ public class Binder<BEAN> implements Serializable {
 
         private boolean readOnly;
 
-        private final Registration onValueChange;
+        private Registration onValueChange;
         private boolean valueInit = false;
 
         /**
@@ -1071,7 +1071,8 @@ public class Binder<BEAN> implements Serializable {
 
         /**
          * Removes this binding from its binder and unregisters the
-         * {@code ValueChangeListener} from any bound {@code HasValue}.
+         * {@code ValueChangeListener} from any bound {@code HasValue}. It does
+         * nothing if it is called for an already unbound binding.
          *
          * @since 8.2
          */
@@ -1079,9 +1080,14 @@ public class Binder<BEAN> implements Serializable {
         public void unbind() {
             if (onValueChange != null) {
                 onValueChange.remove();
+                onValueChange = null;
             }
-            binder.removeBindingInternal(this);
-            binder = null;
+
+            if (binder != null) {
+                binder.removeBindingInternal(this);
+                binder = null;
+            }
+
             field = null;
         }
 
@@ -1687,7 +1693,15 @@ public class Binder<BEAN> implements Serializable {
             clearFields();
         } else {
             changedBindings.clear();
-            getBindings().forEach(binding -> binding.initFieldValue(bean));
+            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);
+            });
             getValidationStatusHandler().statusChange(
                     BinderValidationStatus.createUnresolvedStatus(this));
             fireStatusChangeEvent(false);
diff --git a/server/src/test/java/com/vaadin/data/UnbindTest.java b/server/src/test/java/com/vaadin/data/UnbindTest.java
new file mode 100644
index 0000000000..ca26c34d4c
--- /dev/null
+++ b/server/src/test/java/com/vaadin/data/UnbindTest.java
@@ -0,0 +1,54 @@
+package com.vaadin.data;
+
+import com.vaadin.tests.data.bean.BeanToValidate;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+
+public class UnbindTest
+        extends BinderTestBase<Binder<BeanToValidate>, BeanToValidate> {
+    @Before
+    public void setUp() {
+        binder = new BeanValidationBinder<>(BeanToValidate.class);
+        item = new BeanToValidate();
+        item.setFirstname("Johannes");
+        item.setAge(32);
+    }
+
+    @Test
+    public void binding_unbind_shouldBeRemovedFromBindings() {
+        Binder.Binding<BeanToValidate, String> firstnameBinding = binder
+                .bind(nameField, "firstname");
+        Assert.assertEquals(1, binder.getBindings().size());
+        firstnameBinding.unbind();
+        Assert.assertTrue(binder.getBindings().isEmpty());
+        Assert.assertNull(firstnameBinding.getField());
+    }
+
+    @Test
+    public void binding_unbindDuringReadBean_shouldBeRemovedFromBindings() {
+        Binder.Binding<BeanToValidate, String> firstnameBinding = binder
+                .bind(nameField, "firstname");
+        Binder.Binding<BeanToValidate, String> ageBinding = binder
+                .bind(ageField, "age");
+        Assert.assertEquals(2, binder.getBindings().size());
+        nameField.addValueChangeListener(event -> {
+            if (event.getValue().length() > 0)
+                ageBinding.unbind();
+        });
+        binder.readBean(item);
+        Assert.assertEquals(1, binder.getBindings().size());
+        Assert.assertNull(ageBinding.getField());
+    }
+
+    @Test
+    public void binding_unbindTwice_shouldBeRemovedFromBindings() {
+        Binder.Binding<BeanToValidate, String> firstnameBinding = binder
+                .bind(nameField, "firstname");
+        Assert.assertEquals(1, binder.getBindings().size());
+        firstnameBinding.unbind();
+        firstnameBinding.unbind();
+        Assert.assertTrue(binder.getBindings().isEmpty());
+        Assert.assertNull(firstnameBinding.getField());
+    }
+}