Fix Binder bean writing to only validate and write given bindings (#10162)

3 files changed, 95 insertions, 33 deletions

diff --git a/server/src/main/java/com/vaadin/data/Binder.java b/server/src/main/java/com/vaadin/data/Binder.java
index 7f63cb3841..e28c191018 100644
--- a/server/src/main/java/com/vaadin/data/Binder.java
+++ b/server/src/main/java/com/vaadin/data/Binder.java
@@ -118,14 +118,33 @@ public class Binder<BEAN> implements Serializable {
 
         /**
          * Validates the field value and returns a {@code ValidationStatus}
-         * instance representing the outcome of the validation.
+         * instance representing the outcome of the validation. This method is a
+         * short-hand for calling {@link #validate(boolean)} with
+         * {@code fireEvent} {@code true}.
          *
+         * @see #validate(boolean)
          * @see Binder#validate()
          * @see Validator#apply(Object, ValueContext)
          *
          * @return the validation result.
          */
-        public BindingValidationStatus<TARGET> validate();
+        public default BindingValidationStatus<TARGET> validate() {
+            return validate(true);
+        }
+
+        /**
+         * Validates the field value and returns a {@code ValidationStatus}
+         * instance representing the outcome of the validation.
+         *
+         * @see #validate()
+         *
+         * @param fireEvent
+         *            {@code true} to fire status event; {@code false} to not
+         * @return the validation result.
+         *
+         * @since 8.2
+         */
+        public BindingValidationStatus<TARGET> validate(boolean fireEvent);
 
         /**
          * Gets the validation status handler for this Binding.
@@ -423,11 +442,9 @@ public class Binder<BEAN> implements Serializable {
                 TARGET nullRepresentation) {
             return withConverter(
                     fieldValue -> Objects.equals(fieldValue, nullRepresentation)
-                            ? null
-                            : fieldValue,
+                            ? null : fieldValue,
                     modelValue -> Objects.isNull(modelValue)
-                            ? nullRepresentation
-                            : modelValue);
+                            ? nullRepresentation : modelValue);
         }
 
         /**
@@ -842,14 +859,17 @@ public class Binder<BEAN> implements Serializable {
         }
 
         @Override
-        public BindingValidationStatus<TARGET> validate() {
+        public BindingValidationStatus<TARGET> validate(boolean fireEvent) {
             Objects.requireNonNull(binder,
                     "This Binding is no longer attached to a Binder");
             BindingValidationStatus<TARGET> status = doValidation();
-            getBinder().getValidationStatusHandler()
-                    .statusChange(new BinderValidationStatus<>(getBinder(),
-                            Arrays.asList(status), Collections.emptyList()));
-            getBinder().fireStatusChangeEvent(status.isError());
+            if (fireEvent) {
+                getBinder().getValidationStatusHandler()
+                        .statusChange(new BinderValidationStatus<>(getBinder(),
+                                Arrays.asList(status),
+                                Collections.emptyList()));
+                getBinder().fireStatusChangeEvent(status.isError());
+            }
             return status;
         }
 
@@ -1449,7 +1469,8 @@ public class Binder<BEAN> implements Serializable {
      *             if some of the bound field values fail to validate
      */
     public void writeBean(BEAN bean) throws ValidationException {
-        BinderValidationStatus<BEAN> status = doWriteIfValid(bean, bindings);
+        BinderValidationStatus<BEAN> status = doWriteIfValid(bean,
+                new ArrayList<>(bindings));
         if (status.hasErrors()) {
             throw new ValidationException(status.getFieldValidationErrors(),
                     status.getBeanValidationErrors());
@@ -1478,12 +1499,15 @@ public class Binder<BEAN> implements Serializable {
      *         updated, {@code false} otherwise
      */
     public boolean writeBeanIfValid(BEAN bean) {
-        return doWriteIfValid(bean, bindings).isOk();
+        return doWriteIfValid(bean, new ArrayList<>(bindings)).isOk();
     }
 
     /**
      * Writes the field values into the given bean if all field level validators
      * pass. Runs bean level validators on the bean after writing.
+     * <p>
+     * <strong>Note:</strong> The collection of bindings is cleared on
+     * successful save.
      *
      * @param bean
      *            the bean to write field values into
@@ -1498,12 +1522,12 @@ public class Binder<BEAN> implements Serializable {
         Objects.requireNonNull(bean, "bean cannot be null");
         List<ValidationResult> binderResults = Collections.emptyList();
 
-        // First run fields level validation
-        List<BindingValidationStatus<?>> bindingStatuses = validateBindings();
+        // First run fields level validation, if no validation errors then
+        // update bean
+        List<BindingValidationStatus<?>> bindingResults = bindings.stream()
+                .map(b -> b.validate(false)).collect(Collectors.toList());
 
-        // If no validation errors then update bean
-        // TODO: Enable partial write when some fields don't pass
-        if (bindingStatuses.stream()
+        if (bindingResults.stream()
                 .noneMatch(BindingValidationStatus::isError)) {
             // Store old bean values so we can restore them if validators fail
             Map<Binding<BEAN, ?>, Object> oldValues = getBeanState(bean,
@@ -1516,14 +1540,17 @@ public class Binder<BEAN> implements Serializable {
             if (binderResults.stream().anyMatch(ValidationResult::isError)) {
                 // Bean validator failed, revert values
                 restoreBeanState(bean, oldValues);
-            } else if (getBean() == null || bean.equals(getBean())) {
+            } else if (bean.equals(getBean())) {
                 /*
                  * Changes have been successfully saved. The set is only cleared
-                 * if using readBean/writeBean or when the changes are stored in
-                 * the currently set bean.
-                 *
-                 * Writing changes to another bean when using setBean does not
-                 * clear the set of changed bindings.
+                 * when the changes are stored in the currently set bean.
+                 */
+                bindings.clear();
+            } else if (getBean() == null) {
+                /*
+                 * When using readBean and writeBean there is no knowledge of
+                 * which bean the changes come from or are stored in. Binder is
+                 * no longer "changed" when saved succesfully to any bean.
                  */
                 changedBindings.clear();
             }
@@ -1531,7 +1558,7 @@ public class Binder<BEAN> implements Serializable {
 
         // Generate status object and fire events.
         BinderValidationStatus<BEAN> status = new BinderValidationStatus<>(this,
-                bindingStatuses, binderResults);
+                bindingResults, binderResults);
         getValidationStatusHandler().statusChange(status);
         fireStatusChangeEvent(!status.isOk());
         return status;
@@ -2160,8 +2187,7 @@ public class Binder<BEAN> implements Serializable {
         Converter<FIELDVALUE, FIELDVALUE> nullRepresentationConverter = Converter
                 .from(fieldValue -> fieldValue,
                         modelValue -> Objects.isNull(modelValue)
-                                ? field.getEmptyValue()
-                                : modelValue,
+                                ? field.getEmptyValue() : modelValue,
                         exception -> exception.getMessage());
         ConverterDelegate<FIELDVALUE> converter = new ConverterDelegate<>(
                 nullRepresentationConverter);
diff --git a/server/src/test/java/com/vaadin/data/BinderConverterValidatorTest.java b/server/src/test/java/com/vaadin/data/BinderConverterValidatorTest.java
index 4915bc9a5b..734aa5b194 100644
--- a/server/src/test/java/com/vaadin/data/BinderConverterValidatorTest.java
+++ b/server/src/test/java/com/vaadin/data/BinderConverterValidatorTest.java
@@ -621,14 +621,22 @@ public class BinderConverterValidatorTest
 
         // bind a new field that has invalid value in bean
         TextField lastNameField = new TextField();
-        person.setLastName("");
+
+        // The test starts with a valid value as the last name of the person,
+        // since the binder assumes any non-changed values to be valid.
+        person.setLastName("bar");
+
         BindingBuilder<Person, String> binding2 = binder.forField(lastNameField)
                 .withValidator(notEmpty);
         binding2.bind(Person::getLastName, Person::setLastName);
 
-        // should not have error shown
+        // should not have error shown when initialized
         assertNull(lastNameField.getComponentError());
 
+        // Set a value that breaks the validation
+        lastNameField.setValue("");
+        assertNotNull(lastNameField.getComponentError());
+
         // add status label to show bean level error
         Label statusLabel = new Label();
         binder.setStatusLabel(statusLabel);
diff --git a/server/src/test/java/com/vaadin/data/BinderTest.java b/server/src/test/java/com/vaadin/data/BinderTest.java
index 1169eb8a5d..a44e3b3e06 100644
--- a/server/src/test/java/com/vaadin/data/BinderTest.java
+++ b/server/src/test/java/com/vaadin/data/BinderTest.java
@@ -392,9 +392,8 @@ public class BinderTest extends BinderTestBase<Binder<Person>, Person> {
         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();
@@ -490,7 +489,7 @@ public class BinderTest extends BinderTestBase<Binder<Person>, Person> {
         ErrorMessage errorMessage = textField.getErrorMessage();
         assertNotNull(errorMessage);
         assertEquals("foobar", errorMessage.getFormattedHtmlMessage());
-        // validation is done for the whole bean at once.
+        // validation is done for all changed bindings once.
         assertEquals(1, invokes.get());
 
         textField.setValue("value");
@@ -942,4 +941,33 @@ public class BinderTest extends BinderTestBase<Binder<Person>, Person> {
         assertEquals("Binding still affects bean even after unbind",
                 ageBeforeUnbind, String.valueOf(item.getAge()));
     }
+
+    @Test
+    public void two_asRequired_fields_without_initial_values() {
+        binder.forField(nameField).asRequired("Empty name").bind(p -> "",
+                (p, s) -> {
+                });
+        binder.forField(ageField).asRequired("Empty age").bind(p -> "",
+                (p, s) -> {
+                });
+
+        binder.setBean(item);
+        assertNull("Initially there should be no errors",
+                nameField.getComponentError());
+        assertNull("Initially there should be no errors",
+                ageField.getComponentError());
+
+        nameField.setValue("Foo");
+        assertNull("Name with a value should not be an error",
+                nameField.getComponentError());
+        assertNull(
+                "Age field should not be in error, since it has not been modified.",
+                ageField.getComponentError());
+
+        nameField.setValue("");
+        assertNotNull("Empty name should now be in error.",
+                nameField.getComponentError());
+        assertNull("Age field should still be ok.",
+                ageField.getComponentError());
+    }
 }