Fix removeBinding logic (#10002)

Fixes and improves on PR #9932.

2 files changed, 73 insertions, 12 deletions

diff --git a/server/src/main/java/com/vaadin/data/Binder.java b/server/src/main/java/com/vaadin/data/Binder.java
index f76cdcda2c..0582182d16 100644
--- a/server/src/main/java/com/vaadin/data/Binder.java
+++ b/server/src/main/java/com/vaadin/data/Binder.java
@@ -126,6 +126,15 @@ public class Binder<BEAN> implements Serializable {
          */
         public BindingValidationStatus<TARGET> validate();
 
+        /**
+         * Unbinds the binding from its respective {@code Binder}
+         * Removes any {@code ValueChangeListener} {@code Registration} from
+         * associated {@code HasValue}
+         *
+         * @since 8.2
+         */
+        public void unbind();
+
     }
 
     /**
@@ -541,7 +550,7 @@ public class Binder<BEAN> implements Serializable {
     protected static class BindingBuilderImpl<BEAN, FIELDVALUE, TARGET>
             implements BindingBuilder<BEAN, TARGET> {
 
-        private final Binder<BEAN> binder;
+        private Binder<BEAN> binder;
 
         private final HasValue<FIELDVALUE> field;
         private BindingValidationStatusHandler statusHandler;
@@ -766,9 +775,9 @@ public class Binder<BEAN> implements Serializable {
     protected static class BindingImpl<BEAN, FIELDVALUE, TARGET>
             implements Binding<BEAN, TARGET> {
 
-        private final Binder<BEAN> binder;
+        private Binder<BEAN> binder;
 
-        private final HasValue<FIELDVALUE> field;
+        private HasValue<FIELDVALUE> field;
         private final BindingValidationStatusHandler statusHandler;
 
         private final SerializableFunction<BEAN, TARGET> getter;
@@ -824,6 +833,7 @@ public class Binder<BEAN> implements Serializable {
 
         @Override
         public BindingValidationStatus<TARGET> validate() {
+            Objects.requireNonNull(binder, "This Binding is no longer attached to a Binder");
             BindingValidationStatus<TARGET> status = doValidation();
             getBinder().getValidationStatusHandler()
                     .statusChange(new BinderValidationStatus<>(getBinder(),
@@ -833,6 +843,23 @@ public class Binder<BEAN> implements Serializable {
         }
 
         /**
+         * Removes this binding from its binder and unregisters the
+         * {@code ValueChangeListener} from any bound {@code HasValue}
+         *
+         * @since 8.2
+         */
+        @Override
+        public void unbind() {
+            if (onValueChange != null) {
+                onValueChange.remove();
+                onValueChange = null;
+            }
+            binder.removeBindingInternal(this);
+            binder = null;
+            field = null;
+        }
+
+        /**
          * Returns the field value run through all converters and validators,
          * but doesn't pass the {@link BindingValidationStatus} to any status
          * handler.
@@ -2401,7 +2428,7 @@ public class Binder<BEAN> implements Serializable {
         Set<BindingImpl<BEAN, ?, ?>> toRemove = bindings.stream()
                 .filter(binding -> field.equals(binding.getField()))
                 .collect(Collectors.toSet());
-        toRemove.forEach(this::removeBinding);
+        toRemove.forEach(Binding::unbind);
     }
 
     /**
@@ -2414,6 +2441,25 @@ public class Binder<BEAN> implements Serializable {
      */
     public void removeBinding(Binding<BEAN, ?> binding) {
         Objects.requireNonNull(binding, "Binding can not be null");
+        binding.unbind();
+    }
+
+    /**
+     * Removes (internally) the {@code Binding} from the bound properties map
+     * (if present) and from the list of {@code Binding}s. Note that this DOES
+     * NOT remove the {@code ValueChangeListener} that the {@code Binding} might
+     * have registered with any {@code HasValue}s or decouple the {@code Binder}
+     * from within the {@code Binding}. To do that, use
+     *
+     * {@link Binding#unbind()}
+     *
+     * This method should just be used for internal cleanup.
+     *
+     * @param binding The {@code Binding} to remove from the binding map
+     *
+     * @since 8.2
+     */
+    protected void removeBindingInternal(Binding<BEAN, ?> binding) {
         if (bindings.remove(binding)) {
             boundProperties.entrySet()
                     .removeIf(entry -> entry.getValue().equals(binding));
@@ -2431,6 +2477,6 @@ public class Binder<BEAN> implements Serializable {
     public void removeBinding(String propertyName) {
         Objects.requireNonNull(propertyName, "Property name can not be null");
         Optional.ofNullable(boundProperties.get(propertyName))
-                .ifPresent(this::removeBinding);
+                .ifPresent(Binding::unbind);
     }
 }
diff --git a/server/src/test/java/com/vaadin/data/BinderTest.java b/server/src/test/java/com/vaadin/data/BinderTest.java
index a3d4f6adff..99317745d5 100644
--- a/server/src/test/java/com/vaadin/data/BinderTest.java
+++ b/server/src/test/java/com/vaadin/data/BinderTest.java
@@ -1,12 +1,5 @@
 package com.vaadin.data;
 
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertFalse;
-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 java.util.Locale;
 import java.util.Objects;
 import java.util.concurrent.atomic.AtomicInteger;
@@ -26,6 +19,8 @@ import com.vaadin.tests.data.bean.Person;
 import com.vaadin.tests.data.bean.Sex;
 import com.vaadin.ui.TextField;
 
+import static org.junit.Assert.*;
+
 public class BinderTest extends BinderTestBase<Binder<Person>, Person> {
 
     @Before
@@ -753,4 +748,24 @@ public class BinderTest extends BinderTestBase<Binder<Person>, Person> {
         Assert.assertNotEquals("Binding was not removed",
                 String.valueOf(item.getAge()), ageField.getValue());
     }
+
+    @Test
+    public void removed_binding_not_updates_value() {
+        Binding<Person, Integer> binding = binder.forField(ageField)
+            .withConverter(new StringToIntegerConverter("Can't convert"))
+            .bind(Person::getAge, Person::setAge);
+
+        binder.setBean(item);
+
+        String modifiedAge = String.valueOf(item.getAge() + 10);
+        String ageBeforeUnbind = String.valueOf(item.getAge());
+
+        binder.removeBinding(binding);
+
+        ageField.setValue(modifiedAge);
+
+        Assert.assertEquals("Binding still affects bean even after unbind",
+            ageBeforeUnbind, String.valueOf(item.getAge()));
+
+    }
 }