diff options
author | Teemu Suo-Anttila <tsuoanttila@users.noreply.github.com> | 2017-09-25 11:28:40 +0300 |
---|---|---|
committer | Henri Sara <henri.sara@gmail.com> | 2017-09-25 11:28:40 +0300 |
commit | f5d8dd7bf4a58af048f0ee13061b1bde5a95facb (patch) | |
tree | 2e5cada783a193d2320e2971c3eeefc19297a7b0 /server | |
parent | bcf58e21450831c4956366222ee93af9e65d0034 (diff) | |
download | vaadin-framework-f5d8dd7bf4a58af048f0ee13061b1bde5a95facb.tar.gz vaadin-framework-f5d8dd7bf4a58af048f0ee13061b1bde5a95facb.zip |
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.
Diffstat (limited to 'server')
3 files changed, 273 insertions, 115 deletions
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<BEAN> implements Serializable { */ private void handleFieldValueChange( ValueChangeEvent<FIELDVALUE> event) { - getBinder().setHasChanges(true); - List<ValidationResult> binderValidationResults = Collections - .emptyList(); - BindingValidationStatus<TARGET> 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<BEAN> 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<BEAN> implements Serializable { private BEAN bean; - private final Set<BindingImpl<BEAN, ?, ?>> bindings = new LinkedHashSet<>(); + private final Collection<Binding<BEAN, ?>> bindings = new ArrayList<>(); private final Map<HasValue<?>, BindingBuilder<BEAN, ?>> incompleteBindings = new IdentityHashMap<>(); @@ -1109,7 +1088,7 @@ public class Binder<BEAN> implements Serializable { private BinderValidationStatusHandler<BEAN> statusHandler; - private boolean hasChanges = false; + private Set<Binding<BEAN, ?>> changedBindings = new LinkedHashSet<>(); /** * Creates a binder using a custom {@link PropertySet} implementation for @@ -1126,6 +1105,24 @@ public class Binder<BEAN> implements Serializable { } /** + * 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<BEAN, ?> 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<BEAN> 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<BEAN> 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<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); + BinderValidationStatus<BEAN> status = doWriteIfValid(bean, bindings); if (status.hasErrors()) { throw new ValidationException(status.getFieldValidationErrors(), status.getBeanValidationErrors()); @@ -1478,7 +1475,7 @@ public class Binder<BEAN> 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<BEAN> 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<BEAN> doWriteIfValid(BEAN bean) { + @SuppressWarnings({ "unchecked" }) + private BinderValidationStatus<BEAN> doWriteIfValid(BEAN bean, + Collection<Binding<BEAN, ?>> bindings) { Objects.requireNonNull(bean, "bean cannot be null"); + List<ValidationResult> binderResults = Collections.emptyList(); + // First run fields level validation List<BindingValidationStatus<?>> 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<Binding<BEAN, ?>, Object> oldValues = getBeanState(bean, + bindings); + + bindings.forEach(binding -> ((BindingImpl<BEAN, ?, ?>) 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<BEAN> 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<Binding<BEAN, ?>, 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<Binding<BEAN, ?>, Object> getBeanState(BEAN bean, + Collection<Binding<BEAN, ?>> bindings) { Map<Binding<BEAN, ?>, 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<ValidationResult> 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<BEAN> implements Serializable { if (hasChanges()) { fireStatusChangeEvent(false); } - setHasChanges(false); + changedBindings.clear(); } /** @@ -1620,23 +1673,55 @@ public class Binder<BEAN> implements Serializable { * level validators are ignored if there is no bound bean or if any field * level validator fails. * <p> + * <strong>Note:</strong> 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<BEAN> 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<BEAN> 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<BindingValidationStatus<?>> bindingStatuses = validateBindings(); BinderValidationStatus<BEAN> 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<Binding<BEAN, ?>, Object> beanState = getBeanState(getBean(), + changedBindings); + changedBindings + .forEach(binding -> ((BindingImpl<BEAN, ?, ?>) 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<BEAN> implements Serializable { * <b>Note:</b> 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()}. + * <p> + * <strong>Note:</strong> 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<BEAN> 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<BEAN> implements Serializable { * @return an immutable list of validation results for bindings */ private List<BindingValidationStatus<?>> validateBindings() { - List<BindingValidationStatus<?>> 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<BEAN> implements Serializable { /** * Returns the bindings for this binder. * - * @return a set of the bindings + * @return a list of the bindings */ - protected Set<BindingImpl<BEAN, ?, ?>> getBindings() { - return bindings; + protected Collection<BindingImpl<BEAN, ?, ?>> getBindings() { + return bindings.stream().map(b -> ((BindingImpl<BEAN, ?, ?>) b)) + .collect(Collectors.toList()); } /** @@ -1969,17 +2043,6 @@ public class Binder<BEAN> 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()}, * {@link #writeBean(Object)} or {@link #writeBeanIfValid(Object)}. @@ -2024,7 +2087,7 @@ public class Binder<BEAN> implements Serializable { * setBean, readBean, writeBean or writeBeanIfValid */ public boolean hasChanges() { - return hasChanges; + return !changedBindings.isEmpty(); } /** @@ -2073,7 +2136,7 @@ public class Binder<BEAN> implements Serializable { } private void doRemoveBean(boolean fireStatusEvent) { - setHasChanges(false); + changedBindings.clear(); if (bean != null) { bean = null; } @@ -2432,7 +2495,7 @@ public class Binder<BEAN> implements Serializable { */ public void removeBinding(HasValue<?> field) { Objects.requireNonNull(field, "Field can not be null"); - Set<BindingImpl<BEAN, ?, ?>> toRemove = bindings.stream() + Set<BindingImpl<BEAN, ?, ?>> 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<Binder<Person>, Person> { @Before @@ -484,9 +489,8 @@ public class BinderTest extends BinderTestBase<Binder<Person>, 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<Binder<Person>, 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<Binder<Person>, 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<Person, Integer> 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<Binder<Person>, Person> { ageField.setValue(modifiedAge); Assert.assertEquals("Binding still affects bean even after unbind", - ageBeforeUnbind, String.valueOf(item.getAge())); + ageBeforeUnbind, String.valueOf(item.getAge())); } } |