From f5d8dd7bf4a58af048f0ee13061b1bde5a95facb Mon Sep 17 00:00:00 2001 From: Teemu Suo-Anttila Date: Mon, 25 Sep 2017 11:28:40 +0300 Subject: Improve Binder value change handling with bean validation (#9988) Changed values are now only applied to the bean when they pass both field and bean validation. Any change that breaks validation will be pending and is attempted to apply when another change event comes. This patch also makes the order of status change event handler and listener call order. This addresses some of the issues raised in #9955. --- server/src/main/java/com/vaadin/data/Binder.java | 263 +++++++++++++-------- .../com/vaadin/data/BinderInstanceFieldTest.java | 14 +- .../src/test/java/com/vaadin/data/BinderTest.java | 111 ++++++++- 3 files changed, 273 insertions(+), 115 deletions(-) (limited to 'server/src') diff --git a/server/src/main/java/com/vaadin/data/Binder.java b/server/src/main/java/com/vaadin/data/Binder.java index 75d210eb61..afb81efa67 100644 --- a/server/src/main/java/com/vaadin/data/Binder.java +++ b/server/src/main/java/com/vaadin/data/Binder.java @@ -21,6 +21,7 @@ import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Type; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collection; import java.util.Collections; import java.util.HashMap; import java.util.IdentityHashMap; @@ -944,30 +945,8 @@ public class Binder implements Serializable { */ private void handleFieldValueChange( ValueChangeEvent event) { - getBinder().setHasChanges(true); - List binderValidationResults = Collections - .emptyList(); - BindingValidationStatus fieldValidationStatus; - if (getBinder().getBean() != null) { - BEAN bean = getBinder().getBean(); - fieldValidationStatus = writeFieldValue(bean); - if (!getBinder().bindings.stream() - .map(BindingImpl::doValidation) - .anyMatch(BindingValidationStatus::isError)) { - binderValidationResults = getBinder().validateBean(bean); - if (!binderValidationResults.stream() - .anyMatch(ValidationResult::isError)) { - getBinder().setHasChanges(false); - } - } - } else { - fieldValidationStatus = doValidation(); - } - BinderValidationStatus status = new BinderValidationStatus<>( - getBinder(), Arrays.asList(fieldValidationStatus), - binderValidationResults); - getBinder().getValidationStatusHandler().statusChange(status); - getBinder().fireStatusChangeEvent(status.hasErrors()); + // Inform binder of changes; if setBean: writeIfValid + getBinder().handleFieldValueChange(this); getBinder().fireValueChangeEvent(event); } @@ -1095,7 +1074,7 @@ public class Binder implements Serializable { private BEAN bean; - private final Set> bindings = new LinkedHashSet<>(); + private final Collection> bindings = new ArrayList<>(); private final Map, BindingBuilder> incompleteBindings = new IdentityHashMap<>(); @@ -1109,7 +1088,7 @@ public class Binder implements Serializable { private BinderValidationStatusHandler statusHandler; - private boolean hasChanges = false; + private Set> changedBindings = new LinkedHashSet<>(); /** * Creates a binder using a custom {@link PropertySet} implementation for @@ -1125,6 +1104,24 @@ public class Binder implements Serializable { this.propertySet = propertySet; } + /** + * 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. + * + * @param binding + * the binding whose value has been changed + */ + protected void handleFieldValueChange(Binding binding) { + changedBindings.add(binding); + if (getBean() != null) { + doWriteIfValid(getBean(), changedBindings); + } else { + binding.validate(); + } + } + /** * Creates a new binder that uses reflection based on the provided bean type * to resolve bean properties. @@ -1378,7 +1375,7 @@ public class Binder implements Serializable { } else { doRemoveBean(false); this.bean = bean; - bindings.forEach(b -> b.initFieldValue(bean)); + getBindings().forEach(b -> b.initFieldValue(bean)); // if there has been field value change listeners that trigger // validation, need to make sure the validation errors are cleared getValidationStatusHandler().statusChange( @@ -1418,8 +1415,8 @@ public class Binder implements Serializable { if (bean == null) { clearFields(); } else { - setHasChanges(false); - bindings.forEach(binding -> binding.initFieldValue(bean)); + changedBindings.clear(); + getBindings().forEach(binding -> binding.initFieldValue(bean)); getValidationStatusHandler().statusChange( BinderValidationStatus.createUnresolvedStatus(this)); fireStatusChangeEvent(false); @@ -1449,7 +1446,7 @@ 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); + BinderValidationStatus status = doWriteIfValid(bean, bindings); if (status.hasErrors()) { throw new ValidationException(status.getFieldValidationErrors(), status.getBeanValidationErrors()); @@ -1478,7 +1475,7 @@ public class Binder implements Serializable { * updated, {@code false} otherwise */ public boolean writeBeanIfValid(BEAN bean) { - return doWriteIfValid(bean).isOk(); + return doWriteIfValid(bean, bindings).isOk(); } /** @@ -1487,43 +1484,99 @@ public class Binder implements Serializable { * * @param bean * the bean to write field values into + * @param bindings + * the set of bindings to write to the bean * @return a list of field validation errors if such occur, otherwise a list * of bean validation errors. */ - @SuppressWarnings({ "rawtypes", "unchecked" }) - private BinderValidationStatus doWriteIfValid(BEAN bean) { + @SuppressWarnings({ "unchecked" }) + private BinderValidationStatus doWriteIfValid(BEAN bean, + Collection> bindings) { Objects.requireNonNull(bean, "bean cannot be null"); + List binderResults = Collections.emptyList(); + // First run fields level validation List> bindingStatuses = validateBindings(); + // If no validation errors then update bean - if (bindingStatuses.stream().filter(BindingValidationStatus::isError) - .findAny().isPresent()) { - fireStatusChangeEvent(true); - return new BinderValidationStatus<>(this, bindingStatuses, - Collections.emptyList()); + // TODO: Enable partial write when some fields don't pass + if (bindingStatuses.stream() + .noneMatch(BindingValidationStatus::isError)) { + // Store old bean values so we can restore them if validators fail + Map, Object> oldValues = getBeanState(bean, + bindings); + + bindings.forEach(binding -> ((BindingImpl) binding) + .writeFieldValue(bean)); + // Now run bean level validation against the updated bean + binderResults = validateBean(bean); + if (binderResults.stream().anyMatch(ValidationResult::isError)) { + // Bean validator failed, revert values + restoreBeanState(bean, oldValues); + } else if (getBean() == null || bean.equals(getBean())) { + /* + * Changes have been succesfully 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. + */ + changedBindings.clear(); + } } - // Store old bean values so we can restore them if validators fail + // Generate status object and fire events. + BinderValidationStatus status = new BinderValidationStatus<>(this, + bindingStatuses, binderResults); + getValidationStatusHandler().statusChange(status); + fireStatusChangeEvent(!status.isOk()); + return status; + } + + /** + * Restores the state of the bean from the given values. + * + * @param bean + * the bean + * @param oldValues + * the old values + * + * @since 8.2 + */ + @SuppressWarnings({ "unchecked", "rawtypes" }) + protected void restoreBeanState(BEAN bean, + Map, Object> oldValues) { + getBindings().stream().filter(oldValues::containsKey) + .forEach(binding -> { + Setter setter = binding.setter; + if (setter != null) { + setter.accept(bean, oldValues.get(binding)); + } + }); + } + + /** + * Stores the state of the given bean. + * + * @param bean + * the bean to store the state of + * @param bindings + * the bindings to store + * + * @return map from binding to value + * + * @since 8.2 + */ + @SuppressWarnings({ "unchecked", "rawtypes" }) + protected Map, Object> getBeanState(BEAN bean, + Collection> bindings) { Map, Object> oldValues = new HashMap<>(); - bindings.forEach( - binding -> oldValues.put(binding, binding.getter.apply(bean))); - - bindings.forEach(binding -> binding.writeFieldValue(bean)); - // Now run bean level validation against the updated bean - List binderResults = validateBean(bean); - boolean hasErrors = binderResults.stream() - .filter(ValidationResult::isError).findAny().isPresent(); - if (hasErrors) { - // Bean validator failed, revert values - bindings.forEach((BindingImpl binding) -> binding.setter - .accept(bean, oldValues.get(binding))); - } else { - // Write successful, reset hasChanges to false - setHasChanges(false); - } - fireStatusChangeEvent(hasErrors); - return new BinderValidationStatus<>(this, bindingStatuses, - binderResults); + bindings.stream().map(binding -> (BindingImpl) binding) + .filter(binding -> binding.setter != null) + .forEach(binding -> oldValues.put(binding, + binding.getter.apply(bean))); + return oldValues; } /** @@ -1608,7 +1661,7 @@ public class Binder implements Serializable { if (hasChanges()) { fireStatusChangeEvent(false); } - setHasChanges(false); + changedBindings.clear(); } /** @@ -1620,23 +1673,55 @@ public class Binder implements Serializable { * level validators are ignored if there is no bound bean or if any field * level validator fails. *

+ * Note: This method will attempt to temporarily apply all + * current changes to the bean and run full bean validation for it. The + * changes are reverted after bean validation. * * @return validation status for the binder */ public BinderValidationStatus validate() { + return validate(true); + } + + /** + * Validates the values of all bound fields and returns the validation + * status. This method can skip firing the event, based on the given + * {@code boolean}. + * + * @param fireEvent + * {@code true} to fire validation status events; {@code false} + * to not + * @return validation status for the binder + * + * @since 8.2 + */ + protected BinderValidationStatus validate(boolean fireEvent) { + if (getBean() == null && !validators.isEmpty()) { + throw new IllegalStateException("Cannot validate binder: " + + "bean level validators have been configured " + + "but no bean is currently set"); + } List> bindingStatuses = validateBindings(); BinderValidationStatus validationStatus; - if (bindingStatuses.stream().filter(BindingValidationStatus::isError) - .findAny().isPresent() || bean == null) { + if (validators.isEmpty() || bindingStatuses.stream() + .anyMatch(BindingValidationStatus::isError)) { validationStatus = new BinderValidationStatus<>(this, bindingStatuses, Collections.emptyList()); } else { + Map, Object> beanState = getBeanState(getBean(), + changedBindings); + changedBindings + .forEach(binding -> ((BindingImpl) binding) + .writeFieldValue(getBean())); validationStatus = new BinderValidationStatus<>(this, - bindingStatuses, validateBean(bean)); + bindingStatuses, validateBean(getBean())); + restoreBeanState(getBean(), beanState); + } + if (fireEvent) { + getValidationStatusHandler().statusChange(validationStatus); + fireStatusChangeEvent(validationStatus.hasErrors()); } - getValidationStatusHandler().statusChange(validationStatus); - fireStatusChangeEvent(validationStatus.hasErrors()); return validationStatus; } @@ -1649,6 +1734,10 @@ public class Binder implements Serializable { * Note: Calling this method will not trigger status change events, * unlike {@link #validate()} and will not modify the UI. To also update * error indicators on fields, use {@code validate().isOk()}. + *

+ * Note: This method will attempt to temporarily apply all + * current changes to the bean and run full bean validation for it. The + * changes are reverted after bean validation. * * @see #validate() * @@ -1658,20 +1747,7 @@ public class Binder implements Serializable { * currently set */ public boolean isValid() { - if (getBean() == null && !validators.isEmpty()) { - throw new IllegalStateException("Cannot validate binder: " - + "bean level validators have been configured " - + "but no bean is currently set"); - } - if (validateBindings().stream().filter(BindingValidationStatus::isError) - .findAny().isPresent()) { - return false; - } - if (getBean() != null && validateBean(getBean()).stream() - .filter(ValidationResult::isError).findAny().isPresent()) { - return false; - } - return true; + return validate(false).isOk(); } /** @@ -1685,11 +1761,8 @@ public class Binder implements Serializable { * @return an immutable list of validation results for bindings */ private List> validateBindings() { - List> results = new ArrayList<>(); - for (BindingImpl binding : bindings) { - results.add(binding.doValidation()); - } - return results; + return getBindings().stream().map(BindingImpl::doValidation) + .collect(Collectors.toList()); } /** @@ -1937,10 +2010,11 @@ public class Binder implements Serializable { /** * Returns the bindings for this binder. * - * @return a set of the bindings + * @return a list of the bindings */ - protected Set> getBindings() { - return bindings; + protected Collection> getBindings() { + return bindings.stream().map(b -> ((BindingImpl) b)) + .collect(Collectors.toList()); } /** @@ -1968,17 +2042,6 @@ public class Binder implements Serializable { } } - /** - * Sets whether the values of the fields this binder is bound to have - * changed since the last explicit call to either bind, write or read. - * - * @param hasChanges - * whether this binder should be marked to have changes - */ - private void setHasChanges(boolean hasChanges) { - this.hasChanges = hasChanges; - } - /** * Check whether any of the bound fields' have uncommitted changes since * last explicit call to {@link #readBean(Object)}, {@link #removeBean()}, @@ -2024,7 +2087,7 @@ public class Binder implements Serializable { * setBean, readBean, writeBean or writeBeanIfValid */ public boolean hasChanges() { - return hasChanges; + return !changedBindings.isEmpty(); } /** @@ -2073,7 +2136,7 @@ public class Binder implements Serializable { } private void doRemoveBean(boolean fireStatusEvent) { - setHasChanges(false); + changedBindings.clear(); if (bean != null) { bean = null; } @@ -2432,7 +2495,7 @@ public class Binder implements Serializable { */ public void removeBinding(HasValue field) { Objects.requireNonNull(field, "Field can not be null"); - Set> toRemove = bindings.stream() + Set> toRemove = getBindings().stream() .filter(binding -> field.equals(binding.getField())) .collect(Collectors.toSet()); toRemove.forEach(Binding::unbind); diff --git a/server/src/test/java/com/vaadin/data/BinderInstanceFieldTest.java b/server/src/test/java/com/vaadin/data/BinderInstanceFieldTest.java index b09283936d..5b2e0a2490 100644 --- a/server/src/test/java/com/vaadin/data/BinderInstanceFieldTest.java +++ b/server/src/test/java/com/vaadin/data/BinderInstanceFieldTest.java @@ -342,11 +342,13 @@ public class BinderInstanceFieldTest { // the instance is not overridden Assert.assertEquals(name, form.firstName); - form.firstName.setValue("aa"); + // Test automatic binding form.birthDate.setValue(person.getBirthDate().plusDays(345)); + Assert.assertEquals(form.birthDate.getValue(), person.getBirthDate()); + // Test custom binding + form.firstName.setValue("aa"); Assert.assertEquals(personName, person.getFirstName()); - Assert.assertEquals(form.birthDate.getValue(), person.getBirthDate()); Assert.assertFalse(binder.validate().isOk()); } @@ -385,14 +387,16 @@ public class BinderInstanceFieldTest { Assert.assertEquals(name, form.firstName); Assert.assertEquals(ageField, form.noFieldInPerson); - form.firstName.setValue("aa"); + // Test correct age age += 56; form.noFieldInPerson.setValue(String.valueOf(age)); - - Assert.assertEquals(personName, person.getFirstName()); Assert.assertEquals(form.noFieldInPerson.getValue(), String.valueOf(person.getAge())); + // Test incorrect name + form.firstName.setValue("aa"); + Assert.assertEquals(personName, person.getFirstName()); + Assert.assertFalse(binder.validate().isOk()); } diff --git a/server/src/test/java/com/vaadin/data/BinderTest.java b/server/src/test/java/com/vaadin/data/BinderTest.java index 4d3fe2db9b..af84d7a808 100644 --- a/server/src/test/java/com/vaadin/data/BinderTest.java +++ b/server/src/test/java/com/vaadin/data/BinderTest.java @@ -1,5 +1,12 @@ 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; @@ -19,8 +26,6 @@ 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, Person> { @Before @@ -484,9 +489,8 @@ public class BinderTest extends BinderTestBase, Person> { ErrorMessage errorMessage = textField.getErrorMessage(); Assert.assertNotNull(errorMessage); Assert.assertEquals("foobar", errorMessage.getFormattedHtmlMessage()); - // validation is run twice, once for the field, then for all the fields - // for cross field validation... - Assert.assertEquals(2, invokes.get()); + // validation is done for the whole bean at once. + Assert.assertEquals(1, invokes.get()); textField.setValue("value"); Assert.assertNull(textField.getErrorMessage()); @@ -749,6 +753,93 @@ public class BinderTest extends BinderTestBase, Person> { String.valueOf(item.getAge()), ageField.getValue()); } + @Test + public void beanvalidation_two_fields_not_equal() { + TextField lastNameField = new TextField(); + setBeanValidationFirstNameNotEqualsLastName(nameField, lastNameField); + + item.setLastName("Valid"); + binder.setBean(item); + + Assert.assertFalse("Should not have changes initially", + binder.hasChanges()); + Assert.assertTrue("Should be ok initially", binder.validate().isOk()); + Assert.assertNotEquals( + "First name and last name are not same initially", + item.getFirstName(), item.getLastName()); + + nameField.setValue("Invalid"); + + Assert.assertFalse("First name change not handled", + binder.hasChanges()); + Assert.assertTrue( + "Changing first name to something else than last name should be ok", + binder.validate().isOk()); + + lastNameField.setValue("Invalid"); + + Assert.assertTrue("Last name should not be saved yet", + binder.hasChanges()); + Assert.assertFalse( + "Binder validation should fail with pending illegal value", + binder.validate().isOk()); + Assert.assertNotEquals("Illegal last name should not be stored to bean", + item.getFirstName(), item.getLastName()); + + nameField.setValue("Valid"); + + Assert.assertFalse("With new first name both changes should be saved", + binder.hasChanges()); + Assert.assertTrue("Everything should be ok for 'Valid Invalid'", + binder.validate().isOk()); + Assert.assertNotEquals("First name and last name should never match.", + item.getFirstName(), item.getLastName()); + } + + @Test + public void beanvalidation_initially_broken_bean() { + TextField lastNameField = new TextField(); + setBeanValidationFirstNameNotEqualsLastName(nameField, lastNameField); + + item.setLastName(item.getFirstName()); + binder.setBean(item); + + Assert.assertFalse(binder.isValid()); + Assert.assertFalse(binder.validate().isOk()); + } + + @Test(expected = IllegalStateException.class) + public void beanvalidation_isValid_throws_with_readBean() { + TextField lastNameField = new TextField(); + setBeanValidationFirstNameNotEqualsLastName(nameField, lastNameField); + + binder.readBean(item); + + Assert.assertTrue(binder.isValid()); + } + + @Test(expected = IllegalStateException.class) + public void beanvalidation_validate_throws_with_readBean() { + TextField lastNameField = new TextField(); + setBeanValidationFirstNameNotEqualsLastName(nameField, lastNameField); + + binder.readBean(item); + + Assert.assertTrue(binder.validate().isOk()); + } + + protected void setBeanValidationFirstNameNotEqualsLastName( + TextField firstNameField, TextField lastNameField) { + binder.bind(firstNameField, Person::getFirstName, Person::setFirstName); + binder.forField(lastNameField) + .withValidator(t -> !"foo".equals(t), + "Last name cannot be 'foo'") + .bind(Person::getLastName, Person::setLastName); + + binder.withValidator(p -> !p.getFirstName().equals(p.getLastName()), + "First name and last name can't be the same"); + } + static class MyBindingHandler implements BindingValidationStatusHandler { boolean expectingError = false; @@ -807,15 +898,15 @@ public class BinderTest extends BinderTestBase, Person> { // Assert that the handler was called. Assert.assertEquals( - "Unexpected callCount to binding validation status handler", 4, + "Unexpected callCount to binding validation status handler", 6, bindingHandler.callCount); } - + @Test public void removed_binding_not_updates_value() { Binding binding = binder.forField(ageField) - .withConverter(new StringToIntegerConverter("Can't convert")) - .bind(Person::getAge, Person::setAge); + .withConverter(new StringToIntegerConverter("Can't convert")) + .bind(Person::getAge, Person::setAge); binder.setBean(item); @@ -827,6 +918,6 @@ public class BinderTest extends BinderTestBase, Person> { ageField.setValue(modifiedAge); Assert.assertEquals("Binding still affects bean even after unbind", - ageBeforeUnbind, String.valueOf(item.getAge())); + ageBeforeUnbind, String.valueOf(item.getAge())); } } -- cgit v1.2.3