fix: Skip value change event from writing back of converted value (#12367)

(#12360)

This is both a optimization by skipping duplicate validation round and
avoids ConcurrentModificationExpectation being thrown certain corner
cases.

Co-authored-by: Tatu Lund <tatu@vaadin.com>

diff --git a/server/src/main/java/com/vaadin/data/Binder.java b/server/src/main/java/com/vaadin/data/Binder.java
index 1fb64c837fc75e27ef33a7528084a8d3e4809cf3..110e260b0c4d1b8e53a6572a07352c2dbb7a24e4 100644 (file)
--- a/server/src/main/java/com/vaadin/data/Binder.java
+++ b/server/src/main/java/com/vaadin/data/Binder.java
@@ -1103,6 +1103,7 @@ public class Binder<BEAN> implements Serializable {
 
         private Registration onValueChange;
         private boolean valueInit = false;
+        private boolean convertedBack = false;
 
         /**
          * Contains all converters and validators chained together in the
@@ -1283,7 +1284,8 @@ public class Binder<BEAN> implements Serializable {
         private void handleFieldValueChange(
                 ValueChangeEvent<FIELDVALUE> event) {
             // Don't handle change events when setting initial value
-            if (valueInit) {
+            if (valueInit || convertedBack) {
+                convertedBack = false;
                 return;
             }
 
@@ -1313,6 +1315,7 @@ public class Binder<BEAN> implements Serializable {
                     if (convertBackToPresentation && value != null) {
                         FIELDVALUE converted = convertToFieldType(value);
                         if (!Objects.equals(field.getValue(), converted)) {
+                            convertedBack = true;
                             getField().setValue(converted);
                         }
                     }

diff --git a/server/src/test/java/com/vaadin/data/BinderTest.java b/server/src/test/java/com/vaadin/data/BinderTest.java
index 98f72e1197b11a2c0e0901b292954f093d97f253..fe529fa8f5db5e8a534ffa2009018466bd510463 100644 (file)
--- a/server/src/test/java/com/vaadin/data/BinderTest.java
+++ b/server/src/test/java/com/vaadin/data/BinderTest.java
@@ -29,6 +29,7 @@ import org.junit.rules.ExpectedException;
 import com.vaadin.data.Binder.Binding;
 import com.vaadin.data.Binder.BindingBuilder;
 import com.vaadin.data.converter.StringToBigDecimalConverter;
+import com.vaadin.data.converter.StringToDoubleConverter;
 import com.vaadin.data.converter.StringToIntegerConverter;
 import com.vaadin.data.validator.IntegerRangeValidator;
 import com.vaadin.data.validator.NotEmptyValidator;
@@ -43,6 +44,8 @@ import org.hamcrest.CoreMatchers;
 
 public class BinderTest extends BinderTestBase<Binder<Person>, Person> {
 
+    private int count;
+
     @Rule
     /*
      * transient to avoid interfering with serialization tests that capture a
@@ -1487,6 +1490,34 @@ public class BinderTest extends BinderTestBase<Binder<Person>, Person> {
         binder.readBean(new AtomicReference<>(null));
     }
 
+    // See: https://github.com/vaadin/framework/issues/12356
+    @Test
+    public void validationShouldNotRunTwice() {
+        TextField salaryField = new TextField();
+        count = 0;
+        item.setSalaryDouble(100d);
+        binder.forField(salaryField)
+            .withConverter(new StringToDoubleConverter(""))
+            .bind(Person::getSalaryDouble, Person::setSalaryDouble);
+        binder.setBean(item);
+        binder.addValueChangeListener(event -> {
+               count++;
+        });
+
+        salaryField.setValue("1000");
+        assertTrue(binder.isValid());
+
+        salaryField.setValue("salary");
+        assertFalse(binder.isValid());
+
+        salaryField.setValue("2000");
+
+        // Without fix for #12356 count will be 5
+        assertEquals(3, count);
+
+        assertEquals(new Double(2000), item.getSalaryDouble());
+    }
+
     private TextField createNullAnd42RejectingFieldWithEmptyValue(
             String emptyValue) {
         return new TextField() {