diff options
| -rw-r--r-- | all/src/main/templates/release-notes.html | 3 | ||||
| -rw-r--r-- | server/src/main/java/com/vaadin/data/Binder.java | 263 | ||||
| -rw-r--r-- | server/src/test/java/com/vaadin/data/BinderInstanceFieldTest.java | 14 | ||||
| -rw-r--r-- | server/src/test/java/com/vaadin/data/BinderTest.java | 111 |
4 files changed, 275 insertions, 116 deletions
diff --git a/all/src/main/templates/release-notes.html b/all/src/main/templates/release-notes.html index 594eae8d65..d488f3e5e0 100644 --- a/all/src/main/templates/release-notes.html +++ b/all/src/main/templates/release-notes.html @@ -100,10 +100,11 @@ <h2 id="incompatible">Incompatible or Behavior-altering Changes in @version-minor@</h2> - <li>Row height in <tt>Grid</tt> is replaced with separate header, body and footer row heights</li> + <li>Row height in <tt>Grid</tt> is replaced with separate header, body and footer row heights</li> <li><tt>GridState</tt> variable <tt>rowHeight</tt> has been replaced by three variables.</li> <li><tt>Button</tt> has a new constructor that may cause constructor calls with null as first parameter to be ambiguous.</li> <li><tt>DataCommunicator</tt> method <tt>getDataProviderSize</tt> is now <tt>public</tt>, not <tt>protected</tt>.</li> + <li><tt>Binder</tt> method <tt>getBindings</tt> now returns a Collection, not a Set.</li> <h2>For incompatible or behaviour-altering changes in 8.1, please see <a href="https://vaadin.com/download/release/8.1/8.1.0/release-notes.html#incompatible">8.1 release notes</a></h2> 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())); } } |