fix: Binder validation fixes (#18535)

Changes:

- Once Binder.handleFieldValueChange runs for a binding when readBean was used, the whole binder will be silently validated also. BinderValidationStatusHandler is called like before (only contains status from changed binding), but StatusChangeEvent is now fired considering all bindings and if possible bean validators as well.
- Once Binder.handleFieldValueChange runs for a binding when setBean was used, doWriteIfValid will validate all bindings, not only the changed ones. This prevents writing an invalid bean in cases where one or more of the initial values are in invalid state (but not marked as such since setBean resets validation status), but they have not been changed from their initial value(s).
 Calling setAsRequiredEnabled with a changed value no longer triggers validation, since that validation is now handled elsewhere when needed as stated above.

Minor Javadoc fixes, trying to align Javadocs with code.

Fixes #18163
Fixes #4988
Fixes #17515

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

@@ -1548,7 +1548,6 @@ public void setAsRequiredEnabled(boolean asRequiredEnabled) {
             }
             if (asRequiredEnabled != isAsRequiredEnabled()) {
                 field.setRequiredIndicatorVisible(asRequiredEnabled);
-                validate();
             }
         }
 
@@ -1827,7 +1826,7 @@ public static <BEAN> Binder<BEAN> withPropertySet(
      * Informs the Binder that a value in Binding was changed. This method will
      * trigger validating and writing of the whole bean if using
      * {@link #setBean(Object)}. If using {@link #readBean(Object)} only the
-     * field validation is run.
+     * field validation for the given Binding is run.
      *
      * @param binding
      *            the binding whose value has been changed
@@ -1837,7 +1836,7 @@ protected void handleFieldValueChange(Binding<BEAN, ?> binding) {
         if (getBean() != null) {
             doWriteIfValid(getBean(), changedBindings);
         } else {
-            binding.validate();
+            validate(binding);
         }
     }
 
@@ -2127,9 +2126,9 @@ public <FIELDVALUE> Binding<BEAN, FIELDVALUE> bindReadOnly(
      * back to their corresponding property values of the bean as long as the
      * bean is bound.
      * <p>
-     * Any change made in the fields also runs validation for the field
-     * {@link Binding} and bean level validation for this binder (bean level
-     * validators are added using {@link Binder#withValidator(Validator)}.
+     * Any change made in one of the fields also runs validation for all the
+     * fields {@link Binding} and bean level validation for this binder (bean
+     * level validators are added using {@link Binder#withValidator(Validator)}.
      * <p>
      * After updating each field, the value is read back from the field and the
      * bean's property value is updated if it has been changed from the original
@@ -2348,10 +2347,9 @@ private BinderValidationStatus<BEAN> doWriteIfValid(BEAN bean,
                 bindings);
 
         // First run fields level validation, if no validation errors then
-        // update bean
-        List<BindingValidationStatus<?>> bindingResults = currentBindings
-                .stream().map(b -> b.validate(false))
-                .collect(Collectors.toList());
+        // update bean. Note that this will validate all bindings.
+        List<BindingValidationStatus<?>> bindingResults = getBindings().stream()
+                .map(b -> b.validate(false)).collect(Collectors.toList());
 
         if (bindingResults.stream()
                 .noneMatch(BindingValidationStatus::isError)) {
@@ -2617,6 +2615,38 @@ protected BinderValidationStatus<BEAN> validate(boolean fireEvent) {
         return validationStatus;
     }
 
+    /**
+     * Validates the target binding. Also runs validation for all other
+     * bindings, and if possible, bean-level validations as well.
+     *
+     * {@link BinderValidationStatusHandler} is called with only the status of
+     * the target binding.
+     *
+     * {@link StatusChangeEvent} is fired with current binder validation status
+     *
+     * @param targetBinding
+     *            target binding for validation
+     */
+    private void validate(Binding<BEAN, ?> targetBinding) {
+        List<BindingValidationStatus<?>> bindingValidationStatuses = validateBindings();
+
+        List<ValidationResult> beanStatuses = new ArrayList<>();
+        if (getBean() != null) {
+            beanStatuses.addAll(validateBean(getBean()));
+        }
+        BindingValidationStatus<?> status = bindingValidationStatuses.stream()
+                .filter(s -> targetBinding.equals(s.getBinding())).findFirst()
+                .orElse(null);
+
+        getValidationStatusHandler().statusChange(new BinderValidationStatus<>(
+                this, Collections.singletonList(status),
+                Collections.emptyList()));
+
+        fireStatusChangeEvent(bindingValidationStatuses.stream()
+                .anyMatch(BindingValidationStatus::isError)
+                || beanStatuses.stream().anyMatch(ValidationResult::isError));
+    }
+
     /**
      * Runs all currently configured field level validators, as well as all bean
      * level validators if a bean is currently set with

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

@@ -51,6 +51,7 @@
 import com.vaadin.flow.data.validator.IntegerRangeValidator;
 import com.vaadin.flow.data.validator.NotEmptyValidator;
 import com.vaadin.flow.data.validator.StringLengthValidator;
+import com.vaadin.flow.function.ValueProvider;
 import com.vaadin.flow.internal.CurrentInstance;
 import com.vaadin.flow.tests.data.bean.Person;
 import com.vaadin.flow.tests.data.bean.Sex;
@@ -938,6 +939,36 @@ public String convertToPresentation(String value,
         assertTrue(textField.isRequiredIndicatorVisible());
     }
 
+    @Test
+    public void setRequiredAsEnabled_shouldNotTriggerValidation() {
+        AtomicBoolean hasErrors = new AtomicBoolean();
+        // Binding is required but has setAsRequiredEnabled set to false
+        Binding<Person, String> nameBinding = binder.forField(nameField)
+                .asRequired("Name is required")
+                .bind(Person::getFirstName, Person::setFirstName);
+
+        binder.addStatusChangeListener(
+                status -> hasErrors.getAndSet(status.hasValidationErrors()));
+        binder.setBean(new Person());
+
+        // Base state -> valid
+        Assert.assertFalse("binder should not have errors", hasErrors.get());
+        Assert.assertEquals("Name field should not be in error.", "",
+                nameField.getErrorMessage());
+
+        // Set setAsRequiredEnabled false -> should still be valid
+        nameBinding.setAsRequiredEnabled(false);
+        Assert.assertFalse("binder should not have errors", hasErrors.get());
+        Assert.assertEquals("Name field should not be in error.", "",
+                nameField.getErrorMessage());
+
+        // Set setAsRequiredEnabled true -> should still be valid
+        nameBinding.setAsRequiredEnabled(true);
+        Assert.assertFalse("binder should not have errors", hasErrors.get());
+        Assert.assertEquals("Name field should not be in error.", "",
+                nameField.getErrorMessage());
+    }
+
     @Test
     public void validationStatusHandler_onlyRunForChangedField() {
         TestTextField firstNameField = new TestTextField();
@@ -1468,6 +1499,37 @@ public void two_asRequired_fields_without_initial_values() {
         assertThat("Name with a value should not be an error",
                 nameField.getErrorMessage(), isEmptyString());
 
+        assertNotNull(
+                "Age field should now be in error, since setBean is used.",
+                ageField.getErrorMessage());
+
+        nameField.setValue("");
+        assertNotNull("Empty name should now be in error.",
+                nameField.getErrorMessage());
+
+        assertNotNull("Age field should still be in error.",
+                ageField.getErrorMessage());
+    }
+
+    @Test
+    public void two_asRequired_fields_without_initial_values_readBean() {
+        binder.forField(nameField).asRequired("Empty name").bind(p -> "",
+                (p, s) -> {
+                });
+        binder.forField(ageField).asRequired("Empty age").bind(p -> "",
+                (p, s) -> {
+                });
+
+        binder.readBean(item);
+        assertThat("Initially there should be no errors",
+                nameField.getErrorMessage(), isEmptyString());
+        assertThat("Initially there should be no errors",
+                ageField.getErrorMessage(), isEmptyString());
+
+        nameField.setValue("Foo");
+        assertThat("Name with a value should not be an error",
+                nameField.getErrorMessage(), isEmptyString());
+
         assertThat(
                 "Age field should not be in error, since it has not been modified.",
                 ageField.getErrorMessage(), isEmptyString());