From dd806e8bb31ecc7bce50f3aed5ed3fab48364895 Mon Sep 17 00:00:00 2001 From: Teemu Suo-Anttila Date: Wed, 11 Oct 2017 15:15:46 +0300 Subject: [PATCH] Fix Binder bean writing to only validate and write given bindings (#10162) --- .../src/main/java/com/vaadin/data/Binder.java | 80 ++++++++++++------- .../data/BinderConverterValidatorTest.java | 12 ++- .../test/java/com/vaadin/data/BinderTest.java | 36 ++++++++- 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 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 validate(); + public default BindingValidationStatus 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 validate(boolean fireEvent); /** * Gets the validation status handler for this Binding. @@ -423,11 +442,9 @@ public class Binder 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 implements Serializable { } @Override - public BindingValidationStatus validate() { + public BindingValidationStatus validate(boolean fireEvent) { Objects.requireNonNull(binder, "This Binding is no longer attached to a Binder"); BindingValidationStatus 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 implements Serializable { * if some of the bound field values fail to validate */ public void writeBean(BEAN bean) throws ValidationException { - BinderValidationStatus status = doWriteIfValid(bean, bindings); + BinderValidationStatus status = doWriteIfValid(bean, + new ArrayList<>(bindings)); if (status.hasErrors()) { throw new ValidationException(status.getFieldValidationErrors(), status.getBeanValidationErrors()); @@ -1478,12 +1499,15 @@ public class Binder 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. + *

+ * Note: 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 implements Serializable { Objects.requireNonNull(bean, "bean cannot be null"); List binderResults = Collections.emptyList(); - // First run fields level validation - List> bindingStatuses = validateBindings(); + // First run fields level validation, if no validation errors then + // update bean + List> 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, Object> oldValues = getBeanState(bean, @@ -1516,14 +1540,17 @@ public class Binder 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 implements Serializable { // Generate status object and fire events. BinderValidationStatus status = new BinderValidationStatus<>(this, - bindingStatuses, binderResults); + bindingResults, binderResults); getValidationStatusHandler().statusChange(status); fireStatusChangeEvent(!status.isOk()); return status; @@ -2160,8 +2187,7 @@ public class Binder implements Serializable { Converter nullRepresentationConverter = Converter .from(fieldValue -> fieldValue, modelValue -> Objects.isNull(modelValue) - ? field.getEmptyValue() - : modelValue, + ? field.getEmptyValue() : modelValue, exception -> exception.getMessage()); ConverterDelegate 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 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, Person> { String customNullPointerRepresentation = "foo"; Binder 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, 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, 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()); + } } -- 2.39.5