From 09485d529d085a380b347a118b54df9ae30fefd0 Mon Sep 17 00:00:00 2001 From: Teemu Suo-Anttila Date: Wed, 19 Oct 2016 14:54:33 +0300 Subject: [PATCH] Initial support for null representations in Binder Change-Id: If40bfa28764d1399b5ed4d5928988560e9989dce --- .../main/java/com/vaadin/data/BeanBinder.java | 4 +- .../src/main/java/com/vaadin/data/Binder.java | 49 ++++++++--- .../main/java/com/vaadin/data/HasValue.java | 28 +++++++ .../com/vaadin/ui/AbstractColorPicker.java | 5 ++ .../com/vaadin/ui/AbstractMultiSelect.java | 9 ++- .../java/com/vaadin/ui/AbstractTextField.java | 23 ++---- .../src/main/java/com/vaadin/ui/CheckBox.java | 5 ++ .../main/java/com/vaadin/ui/RichTextArea.java | 15 ++-- .../src/main/java/com/vaadin/ui/Slider.java | 5 ++ .../com/vaadin/data/BinderComponentTest.java | 81 +++++++++++++++++++ .../data/BinderConverterValidatorTest.java | 31 +++++-- .../test/java/com/vaadin/data/BinderTest.java | 81 ++++++++++++++++++- 12 files changed, 292 insertions(+), 44 deletions(-) create mode 100644 server/src/test/java/com/vaadin/data/BinderComponentTest.java diff --git a/server/src/main/java/com/vaadin/data/BeanBinder.java b/server/src/main/java/com/vaadin/data/BeanBinder.java index d8e8c0450d..96654e37a0 100644 --- a/server/src/main/java/com/vaadin/data/BeanBinder.java +++ b/server/src/main/java/com/vaadin/data/BeanBinder.java @@ -266,8 +266,8 @@ public class BeanBinder extends Binder { @Override public BeanBinding forField( HasValue field) { - return createBinding(field, Converter.identity(), - this::handleValidationStatus); + return (BeanBinding) super.forField( + field); } /** diff --git a/server/src/main/java/com/vaadin/data/Binder.java b/server/src/main/java/com/vaadin/data/Binder.java index 1dfed366ba..8356a03dda 100644 --- a/server/src/main/java/com/vaadin/data/Binder.java +++ b/server/src/main/java/com/vaadin/data/Binder.java @@ -279,6 +279,23 @@ public class Binder implements Serializable { exception -> errorMessage)); } + /** + * Maps binding value {@code null} to given null representation and back + * to {@code null} when converting back to model value. + * + * @param nullRepresentation + * the value to use instead of {@code null} + * @return a new binding with null representation handling. + */ + public default Binding withNullRepresentation( + TARGET nullRepresentation) { + return withConverter( + fieldValue -> Objects.equals(fieldValue, nullRepresentation) + ? null : fieldValue, + modelValue -> Objects.isNull(modelValue) + ? nullRepresentation : modelValue); + } + /** * Gets the field the binding uses. * @@ -615,10 +632,6 @@ public class Binder implements Serializable { return validationStatus; } - private void setBeanValue(BEAN bean, TARGET value) { - setter.accept(bean, value); - } - private void notifyStatusHandler(ValidationStatus status) { statusHandler.accept(status); } @@ -694,6 +707,13 @@ public class Binder implements Serializable { * {@link Binding#bind(Function, BiConsumer) Binding.bind} which completes * the binding. Until {@code Binding.bind} is called, the binding has no * effect. + *

+ * Note: Not all {@link HasValue} implementations support + * passing {@code null} as the value. For these the Binder will + * automatically change {@code null} to a null representation provided by + * {@link HasValue#getEmptyValue()}. This conversion is one-way only, if you + * want to have a two-way mapping back to {@code null}, use + * {@link Binding#withNullRepresentation(Object))}. * * @param * the value type of the field @@ -710,7 +730,10 @@ public class Binder implements Serializable { clearError(field); getStatusLabel().ifPresent(label -> label.setValue("")); - return createBinding(field, Converter.identity(), + return createBinding(field, Converter.from(fieldValue -> fieldValue, + modelValue -> Objects.isNull(modelValue) ? field.getEmptyValue() + : modelValue, + exception -> exception.getMessage()), this::handleValidationStatus); } @@ -722,6 +745,14 @@ public class Binder implements Serializable { * Use the {@link #forField(HasValue)} overload instead if you want to * further configure the new binding. *

+ * Note: Not all {@link HasValue} implementations support + * passing {@code null} as the value. For these the Binder will + * automatically change {@code null} to a null representation provided by + * {@link HasValue#getEmptyValue()}. This conversion is one-way only, if you + * want to have a two-way mapping back to {@code null}, use + * {@link #forField(HasValue)} and + * {@link Binding#withNullRepresentation(Object))}. + *

* When a bean is bound with {@link Binder#bind(BEAN)}, the field value is * set to the return value of the given getter. The property value is then * updated via the given setter whenever the field value changes. The setter @@ -905,8 +936,8 @@ public class Binder implements Serializable { // Save old bean values so we can restore them if validators fail Map, Object> oldValues = new HashMap<>(); - bindings.forEach(binding -> oldValues.put(binding, - binding.convertDataToFieldType(bean))); + bindings.forEach( + binding -> oldValues.put(binding, binding.getter.apply(bean))); bindings.forEach(binding -> binding.storeFieldValue(bean)); // Now run bean level validation against the updated bean @@ -915,8 +946,8 @@ public class Binder implements Serializable { .findAny().isPresent(); if (hasErrors) { // Bean validator failed, revert values - bindings.forEach((BindingImpl binding) -> binding.setBeanValue(bean, - oldValues.get(binding))); + bindings.forEach((BindingImpl binding) -> binding.setter + .accept(bean, oldValues.get(binding))); } else { // Save successful, reset hasChanges to false setHasChanges(false); diff --git a/server/src/main/java/com/vaadin/data/HasValue.java b/server/src/main/java/com/vaadin/data/HasValue.java index 89d8d69e66..0ad3203b78 100644 --- a/server/src/main/java/com/vaadin/data/HasValue.java +++ b/server/src/main/java/com/vaadin/data/HasValue.java @@ -17,6 +17,9 @@ package com.vaadin.data; import java.io.Serializable; import java.lang.reflect.Method; +import java.util.Objects; +import java.util.function.BiConsumer; +import java.util.function.Function; import com.vaadin.event.ConnectorEvent; import com.vaadin.event.EventListener; @@ -170,4 +173,29 @@ public interface HasValue extends Serializable { */ public Registration addValueChangeListener( ValueChangeListener listener); + + /** + * Returns the value that represents an empty value. + *

+ * By default {@link HasValue} is expected to support {@code null} as empty + * values. Specific implementations might not support this. + * + * @return empty value + * @see Binder#bind(HasValue, Function, BiConsumer) + */ + public default V getEmptyValue() { + return null; + } + + /** + * Returns whether this {@code HasValue} is considered to be empty. + *

+ * By default this is an equality check between current value and empty + * value. + * + * @return {@code true} if considered empty; {@code false} if not + */ + public default boolean isEmpty() { + return Objects.equals(getValue(), getEmptyValue()); + } } diff --git a/server/src/main/java/com/vaadin/ui/AbstractColorPicker.java b/server/src/main/java/com/vaadin/ui/AbstractColorPicker.java index dd482a5bd0..f256636845 100644 --- a/server/src/main/java/com/vaadin/ui/AbstractColorPicker.java +++ b/server/src/main/java/com/vaadin/ui/AbstractColorPicker.java @@ -534,4 +534,9 @@ public abstract class AbstractColorPicker extends AbstractField { this.color = color; getState().color = color.getCSS(); } + + @Override + public Color getEmptyValue() { + return Color.WHITE; + } } diff --git a/server/src/main/java/com/vaadin/ui/AbstractMultiSelect.java b/server/src/main/java/com/vaadin/ui/AbstractMultiSelect.java index 6c1fbde990..7332941f91 100644 --- a/server/src/main/java/com/vaadin/ui/AbstractMultiSelect.java +++ b/server/src/main/java/com/vaadin/ui/AbstractMultiSelect.java @@ -342,7 +342,7 @@ public abstract class AbstractMultiSelect * The call is delegated to {@link #getSelectedItems()} * * @return the current selection - * + * * @see #getSelectedItems() * @see SelectionModel#getSelectedItems */ @@ -378,11 +378,16 @@ public abstract class AbstractMultiSelect new LinkedHashSet<>(getSelectionModel().getSelectedItems())); } + @Override + public Set getEmptyValue() { + return Collections.emptySet(); + } + /** * Adds a value change listener. The listener is called when the selection * set of this multi select is changed either by the user or * programmatically. - * + * * @see #addSelectionListener(MultiSelectionListener) * * @param listener diff --git a/server/src/main/java/com/vaadin/ui/AbstractTextField.java b/server/src/main/java/com/vaadin/ui/AbstractTextField.java index b5119ec92c..9c2720efd8 100644 --- a/server/src/main/java/com/vaadin/ui/AbstractTextField.java +++ b/server/src/main/java/com/vaadin/ui/AbstractTextField.java @@ -17,6 +17,7 @@ package com.vaadin.ui; import java.util.Collection; +import java.util.Objects; import org.jsoup.nodes.Attributes; import org.jsoup.nodes.Element; @@ -80,11 +81,8 @@ public abstract class AbstractTextField extends AbstractField @Override public void setValue(String value) { - if (value == null) { - setValue("", false); - } else { - setValue(value, false); - } + Objects.requireNonNull(value, "Null value not supported"); + setValue(value, false); } /** @@ -270,16 +268,6 @@ public abstract class AbstractTextField extends AbstractField setValue(""); } - /** - * Checks if the field is empty. - * - * @return true if the field value is an empty string, - * false otherwise - */ - public boolean isEmpty() { - return "".equals(getValue()); - } - @Override public void writeDesign(Element design, DesignContext designContext) { super.writeDesign(design, designContext); @@ -299,4 +287,9 @@ public abstract class AbstractTextField extends AbstractField customAttributes.add("cursor-position"); return customAttributes; } + + @Override + public String getEmptyValue() { + return ""; + } } diff --git a/server/src/main/java/com/vaadin/ui/CheckBox.java b/server/src/main/java/com/vaadin/ui/CheckBox.java index 4fa08a82b5..c4bedbea0a 100644 --- a/server/src/main/java/com/vaadin/ui/CheckBox.java +++ b/server/src/main/java/com/vaadin/ui/CheckBox.java @@ -133,6 +133,11 @@ public class CheckBox extends AbstractField super.setValue(value); } + @Override + public Boolean getEmptyValue() { + return false; + } + @Override protected CheckBoxState getState() { return (CheckBoxState) super.getState(); diff --git a/server/src/main/java/com/vaadin/ui/RichTextArea.java b/server/src/main/java/com/vaadin/ui/RichTextArea.java index 49346928be..545166d33a 100644 --- a/server/src/main/java/com/vaadin/ui/RichTextArea.java +++ b/server/src/main/java/com/vaadin/ui/RichTextArea.java @@ -114,6 +114,11 @@ public class RichTextArea extends AbstractField return getState(false).value; } + @Override + public String getEmptyValue() { + return ""; + } + @Override protected void doSetValue(String value) { getState().value = value; @@ -151,16 +156,6 @@ public class RichTextArea extends AbstractField return getState(false).valueChangeTimeout; } - /** - * Checks if the field is empty. - * - * @return true if the field value is an empty string, - * false otherwise - */ - public boolean isEmpty() { - return getValue().length() == 0; - } - /** * Clears the value of this field. */ diff --git a/server/src/main/java/com/vaadin/ui/Slider.java b/server/src/main/java/com/vaadin/ui/Slider.java index 7a59b86fca..99ab476e2a 100644 --- a/server/src/main/java/com/vaadin/ui/Slider.java +++ b/server/src/main/java/com/vaadin/ui/Slider.java @@ -294,6 +294,11 @@ public class Slider extends AbstractField { return getState().value; } + @Override + public Double getEmptyValue() { + return getMin(); + } + /** * Thrown when the value of the slider is about to be set to a value that is * outside the valid range of the slider. diff --git a/server/src/test/java/com/vaadin/data/BinderComponentTest.java b/server/src/test/java/com/vaadin/data/BinderComponentTest.java new file mode 100644 index 0000000000..e91f085f7e --- /dev/null +++ b/server/src/test/java/com/vaadin/data/BinderComponentTest.java @@ -0,0 +1,81 @@ +package com.vaadin.data; + +import java.util.Collections; + +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; + +import com.vaadin.shared.ui.colorpicker.Color; +import com.vaadin.ui.CheckBox; +import com.vaadin.ui.CheckBoxGroup; +import com.vaadin.ui.ColorPicker; +import com.vaadin.ui.RichTextArea; +import com.vaadin.ui.Slider; + +public class BinderComponentTest + extends BinderTestBase, String> { + + enum TestValues { + TRUE, FALSE, FILE_NOT_FOUND + } + + @Before + public void setUp() { + binder = new Binder<>(); + item = "Foo"; + } + + @Test + public void slider_bind_null() { + double minValue = 10.5d; + double initialValue = 28.2d; + + Slider slider = new Slider(); + slider.setResolution(1); + slider.setMin(minValue); + + testFieldNullRepresentation(initialValue, slider); + } + + @Test + public void colorpicker_bind_null() { + testFieldNullRepresentation(new Color(123, 254, 213), + new ColorPicker()); + } + + @Test + public void richtextarea_bind_null() { + testFieldNullRepresentation("Test text", new RichTextArea()); + } + + @Test + public void checkbox_bind_null() { + testFieldNullRepresentation(true, new CheckBox()); + } + + @Test + public void checkboxgroup_bind_null() { + CheckBoxGroup checkBoxGroup = new CheckBoxGroup<>(); + checkBoxGroup.setItems(TestValues.values()); + testFieldNullRepresentation( + Collections.singleton(TestValues.FILE_NOT_FOUND), + checkBoxGroup); + } + + private void testFieldNullRepresentation(T initialValue, + HasValue field) { + binder.bind(field, t -> null, (str, val) -> { + Assert.assertEquals("Value update with initial value failed.", + initialValue, field.getValue()); + }); + field.setValue(initialValue); + Assert.assertEquals("Initial value of field unexpected", initialValue, + field.getValue()); + binder.bind(item); + Assert.assertEquals("Null representation for field failed", + field.getEmptyValue(), field.getValue()); + field.setValue(initialValue); + } + +} diff --git a/server/src/test/java/com/vaadin/data/BinderConverterValidatorTest.java b/server/src/test/java/com/vaadin/data/BinderConverterValidatorTest.java index d5959eb459..73443155c7 100644 --- a/server/src/test/java/com/vaadin/data/BinderConverterValidatorTest.java +++ b/server/src/test/java/com/vaadin/data/BinderConverterValidatorTest.java @@ -1,12 +1,12 @@ /* * Copyright 2000-2016 Vaadin Ltd. - * + * * Licensed under the Apache License, Version 2.0 (the "License"); you may not * use this file except in compliance with the License. You may obtain a copy of * the License at - * + * * http://www.apache.org/licenses/LICENSE-2.0 - * + * * Unless required by applicable law or agreed to in writing, software * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the @@ -30,6 +30,7 @@ import org.junit.Before; import org.junit.Test; import com.vaadin.data.Binder.Binding; +import com.vaadin.data.util.converter.StringToIntegerConverter; import com.vaadin.data.validator.NotEmptyValidator; import com.vaadin.server.AbstractErrorMessage; import com.vaadin.server.ErrorMessage; @@ -38,8 +39,8 @@ import com.vaadin.tests.data.bean.Person; import com.vaadin.ui.Label; import com.vaadin.ui.TextField; -public class BinderConverterValidatorTest extends - BinderTestBase, Person> { +public class BinderConverterValidatorTest + extends BinderTestBase, Person> { private static class StatusBean { private String status; @@ -684,4 +685,24 @@ public class BinderConverterValidatorTest extends .bind(Person::getAge, Person::setAge); binder.bind(item); } + + @Test(expected = ValidationException.class) + public void save_beanValidationErrorsWithConverter() + throws ValidationException { + Binder binder = new Binder<>(); + binder.forField(ageField) + .withConverter(new StringToIntegerConverter("Can't convert")) + .bind(Person::getAge, Person::setAge); + + binder.withValidator(Validator.from(person -> false, "b")); + + Person person = new Person(); + ageField.setValue("1"); + try { + binder.save(person); + } finally { + // Bean should have been updated for item validation but reverted + Assert.assertEquals(0, person.getAge()); + } + } } diff --git a/server/src/test/java/com/vaadin/data/BinderTest.java b/server/src/test/java/com/vaadin/data/BinderTest.java index b5e84cddd4..eb686acf9b 100644 --- a/server/src/test/java/com/vaadin/data/BinderTest.java +++ b/server/src/test/java/com/vaadin/data/BinderTest.java @@ -4,11 +4,15 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertSame; +import java.util.Objects; + import org.junit.Assert; import org.junit.Before; import org.junit.Test; import com.vaadin.tests.data.bean.Person; +import com.vaadin.tests.data.bean.Sex; +import com.vaadin.ui.TextField; public class BinderTest extends BinderTestBase, Person> { @@ -174,4 +178,79 @@ public class BinderTest extends BinderTestBase, Person> { binder.bind(nameField, Person::getFirstName, Person::setFirstName); binder.bind(item); } -} + + @Test + public void binding_with_null_representation() { + String nullRepresentation = "Some arbitrary text"; + String realName = "John"; + Person namelessPerson = new Person(null, "Doe", "", 25, Sex.UNKNOWN, + null); + + binder.forField(nameField).withNullRepresentation(nullRepresentation) + .bind(Person::getFirstName, Person::setFirstName); + + // Bind a person with null value and check that null representation is + // used + binder.bind(namelessPerson); + Assert.assertEquals( + "Null value from bean was not converted to explicit null representation", + nullRepresentation, nameField.getValue()); + + // Verify that changes are applied to bean + nameField.setValue(realName); + Assert.assertEquals( + "Bean was not correctly updated from a change in the field", + realName, namelessPerson.getFirstName()); + + // Verify conversion back to null + nameField.setValue(nullRepresentation); + Assert.assertEquals( + "Two-way null representation did not change value back to null", + null, namelessPerson.getFirstName()); + } + + @Test + public void binding_with_default_null_representation() { + TextField nullTextField = new TextField() { + @Override + public String getEmptyValue() { + return "null"; + } + }; + + Person namelessPerson = new Person(null, "Doe", "", 25, Sex.UNKNOWN, + null); + binder.bind(nullTextField, Person::getFirstName, Person::setFirstName); + binder.bind(namelessPerson); + + Assert.assertTrue(nullTextField.isEmpty()); + Assert.assertEquals(null, namelessPerson.getFirstName()); + + // Change value, see that textfield is not empty and bean is updated. + nullTextField.setValue(""); + Assert.assertFalse(nullTextField.isEmpty()); + Assert.assertEquals("First name of person was not properly updated", "", + namelessPerson.getFirstName()); + + // Verify that default null representation does not map back to null + nullTextField.setValue("null"); + Assert.assertTrue(nullTextField.isEmpty()); + Assert.assertEquals("Default one-way null representation failed.", + "null", namelessPerson.getFirstName()); + } + + @Test + public void binding_with_null_representation_value_not_null() { + String nullRepresentation = "Some arbitrary text"; + + binder.forField(nameField).withNullRepresentation(nullRepresentation) + .bind(Person::getFirstName, Person::setFirstName); + + Assert.assertFalse("First name in item should not be null", + Objects.isNull(item.getFirstName())); + binder.bind(item); + + Assert.assertEquals("Field value was not set correctly", + item.getFirstName(), nameField.getValue()); + } +} \ No newline at end of file -- 2.39.5