From 688b5f7c402d4c263b570714d6ea44ef674709b4 Mon Sep 17 00:00:00 2001 From: Teppo Kurki Date: Tue, 20 Aug 2024 15:37:37 +0300 Subject: [PATCH 1/6] feat: Binder record support --- .../com/vaadin/flow/data/binder/Binder.java | 128 ++++++++++++++++-- .../vaadin/flow/data/binder/BinderTest.java | 37 +++++ 2 files changed, 152 insertions(+), 13 deletions(-) diff --git a/flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java b/flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java index 681bb793d64..ac8bacd7ca6 100644 --- a/flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java +++ b/flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java @@ -1053,7 +1053,7 @@ public Binding bind(ValueProvider getter, if (getBinder().getBean() != null) { binding.initFieldValue(getBinder().getBean(), true); } - if (setter == null) { + if (setter == null && !binder.isRecord) { binding.getField().setReadOnly(true); } getBinder().fireStatusChangeEvent(false); @@ -1792,6 +1792,10 @@ void setIdentity() { private BEAN bean; + private boolean isRecord; + + private Class beanType; + private final Collection> bindings = new ArrayList<>(); private Map, BindingBuilder> incompleteBindings; @@ -1834,8 +1838,8 @@ protected Binder(PropertySet propertySet) { } /** - * Creates a new binder that uses reflection based on the provided bean type - * to resolve bean properties. + * Creates a new binder that uses reflection based on the provided bean or + * record type to resolve its properties. * * Nested properties are resolved lazily, when bound to a field. * @@ -1844,6 +1848,10 @@ protected Binder(PropertySet propertySet) { */ public Binder(Class beanType) { this(BeanPropertySet.get(beanType)); + isRecord = beanType.isRecord(); + if (isRecord) { + this.beanType = beanType; + } } /** @@ -1874,8 +1882,8 @@ public Binder() { } /** - * Creates a new binder that uses reflection based on the provided bean type - * to resolve bean properties. + * Creates a new binder that uses reflection based on the provided bean or + * record type to resolve its properties. * * If {@code scanNestedDefinitions} is true, nested properties are detected * eagerly. Otherwise, they will be discovered lazily when the property is @@ -1889,6 +1897,10 @@ public Binder() { public Binder(Class beanType, boolean scanNestedDefinitions) { this(BeanPropertySet.get(beanType, scanNestedDefinitions, PropertyFilterDefinition.getDefaultFilter())); + isRecord = beanType.isRecord(); + if (isRecord) { + this.beanType = beanType; + } } /** @@ -2249,6 +2261,10 @@ public Binding bindReadOnly( * bean and clear bound fields */ public void setBean(BEAN bean) { + if (isRecord) { + throw new IllegalStateException( + "setBean can't be used with records, call readBean instead"); + } checkBindingsCompleted("setBean"); if (bean == null) { if (this.bean != null) { @@ -2278,20 +2294,21 @@ public void removeBean() { } /** - * Reads the bound property values from the given bean to the corresponding - * fields. + * Reads the bound property values from the given bean or record to the + * corresponding fields. *

- * The bean is not otherwise associated with this binder; in particular its - * property values are not bound to the field value changes. To achieve - * that, use {@link #setBean(Object)}. + * The bean or record is not otherwise associated with this binder; in + * particular its property values are not bound to the field value changes. + * To achieve that, use {@link #setBean(Object)}. * * @see #setBean(Object) * @see #writeBeanIfValid(Object) * @see #writeBean(Object) + * @see #writeRecord() * * @param bean - * the bean whose property values to read or {@code null} to - * clear bound fields + * the bean or record whose property values to read or + * {@code null} to clear bound fields */ public void readBean(BEAN bean) { checkBindingsCompleted("readBean"); @@ -2508,6 +2525,84 @@ public boolean writeBeanIfValid(BEAN bean) { return doWriteIfValid(bean, bindings).isOk(); } + /** + * Writes values from the bound fields to a new record instance if all + * validators (binding and bean level) pass. This method can only be used if + * Binder was originally configured to use a record type. + *

+ * If any field binding validator fails, no values are written and a + * {@code ValidationException} is thrown. + *

+ * If all field level validators pass, a record is intanciated and bean + * level validators are run on the new record. If any bean level validator + * fails a {@code ValidationException} is thrown. + * + * @see #readBean(Object) + * + * @return a record instance with current values + * @throws ValidationException + * if some of the bound field values fail to validate + */ + public BEAN writeRecord() throws ValidationException { + BEAN record = null; + List binderResults = Collections.emptyList(); + + // make a copy of the incoming bindings to avoid their modifications + // during validation + Collection> currentBindings = new ArrayList<>( + bindings); + + // First run fields level validation, if no validation errors then + // create a record. + List> bindingResults = currentBindings + .stream().map(b -> b.validate(false)) + .collect(Collectors.toList()); + + if (bindingResults.stream() + .noneMatch(BindingValidationStatus::isError)) { + // Field level validation can be skipped as it was done already + boolean validatorsDisabledStatus = isValidatorsDisabled(); + setValidatorsDisabled(true); + // Fetch all conversion results + List> values = currentBindings.stream() + .map(binding -> ((BindingImpl) binding) + .doConversion()) + .toList(); + setValidatorsDisabled(validatorsDisabledStatus); + + // Gather successfully converted values + final List convertedValues = new ArrayList<>(); + values.forEach(value -> value.ifOk(convertedValues::add)); + + try { + record = beanType.cast(beanType.getDeclaredConstructors()[0] + .newInstance(convertedValues.toArray())); + } catch (InstantiationException | IllegalAccessException + | InvocationTargetException e) { + // TODO replace with something better + throw new RuntimeException(e); + } + + // Now run bean level validation against the created record + bean = record; + binderResults = validateBean(bean); + bean = null; + if (binderResults.stream().noneMatch(ValidationResult::isError)) { + changedBindings.clear(); + } + } + + // Generate status object and fire events. + BinderValidationStatus status = new BinderValidationStatus<>(this, + bindingResults, binderResults); + getValidationStatusHandler().statusChange(status); + fireStatusChangeEvent(!status.isOk()); + if (!status.isOk()) { + throw new ValidationException(bindingResults, binderResults); + } + return record; + } + /** * Writes the field values into the given bean if all field level validators * pass. Runs bean level validators on the bean after writing. @@ -2525,6 +2620,10 @@ public boolean writeBeanIfValid(BEAN bean) { @SuppressWarnings("unchecked") private BinderValidationStatus doWriteIfValid(BEAN bean, Collection> bindings) { + if (isRecord) { + throw new IllegalStateException( + "writeBean methods can't be used with records, call writeRecord instead"); + } Objects.requireNonNull(bean, "bean cannot be null"); List binderResults = Collections.emptyList(); @@ -2597,7 +2696,10 @@ private BinderValidationStatus doWriteIfValid(BEAN bean, private void doWriteDraft(BEAN bean, Collection> bindings, boolean forced) { Objects.requireNonNull(bean, "bean cannot be null"); - + if (isRecord) { + throw new IllegalStateException( + "writeBean methods can't be used with records, call writeRecord instead"); + } if (!forced) { bindings.forEach(binding -> ((BindingImpl) binding) .writeFieldValue(bean)); diff --git a/flow-data/src/test/java/com/vaadin/flow/data/binder/BinderTest.java b/flow-data/src/test/java/com/vaadin/flow/data/binder/BinderTest.java index a51db02540a..3ba9d8e2a0b 100644 --- a/flow-data/src/test/java/com/vaadin/flow/data/binder/BinderTest.java +++ b/flow-data/src/test/java/com/vaadin/flow/data/binder/BinderTest.java @@ -2678,6 +2678,43 @@ public void withConverter_hasChangesFalse() { assertEquals("Name", nameField.getValue()); } + public record TestRecord(String name, int age) { + } + + @Test + public void readRecord_writeRecord() throws ValidationException { + Binder binder = new Binder<>(TestRecord.class); + + TestTextField nameField = new TestTextField(); + nameField.setValue(""); + TestTextField ageField = new TestTextField(); + ageField.setValue(""); + + binder.forField(nameField).bind("name"); + binder.forField(ageField) + .withConverter( + new StringToIntegerConverter(0, "Failed to convert")) + .bind("age"); + binder.readBean(new TestRecord("test", 42)); + + // Check that fields are enabled for records + Assert.assertFalse(nameField.isReadOnly()); + Assert.assertFalse(ageField.isReadOnly()); + + // Check valid record writing + nameField.setValue("foo"); + ageField.setValue("50"); + TestRecord testRecord = binder.writeRecord(); + Assert.assertEquals("foo", testRecord.name); + Assert.assertEquals(50, testRecord.age); + + // Check that invalid record writing fails + ageField.setValue("invalid value"); + assertThrows(ValidationException.class, () -> { + TestRecord failedRecord = binder.writeRecord(); + }); + } + private TestTextField createNullRejectingFieldWithEmptyValue( String emptyValue) { return new TestTextField() { From 330cda049ad801e39fd24636708997f1a4591b6f Mon Sep 17 00:00:00 2001 From: Teppo Kurki Date: Tue, 20 Aug 2024 17:48:49 +0300 Subject: [PATCH 2/6] extract ReflectTools exception handling --- .../vaadin/flow/internal/ReflectTools.java | 62 +++++++++++++------ 1 file changed, 43 insertions(+), 19 deletions(-) diff --git a/flow-server/src/main/java/com/vaadin/flow/internal/ReflectTools.java b/flow-server/src/main/java/com/vaadin/flow/internal/ReflectTools.java index acd10397fc1..250dc21e8d6 100644 --- a/flow-server/src/main/java/com/vaadin/flow/internal/ReflectTools.java +++ b/flow-server/src/main/java/com/vaadin/flow/internal/ReflectTools.java @@ -493,33 +493,57 @@ public static T createProxyInstance(Class proxyClass, return proxyClass.cast(constructor.get().newInstance( Array.newInstance(paramType.getComponentType(), 0))); } - } catch (InstantiationException e) { - if (originalClass.isMemberClass() - && !Modifier.isStatic(originalClass.getModifiers())) { - throw new IllegalArgumentException(String.format( + } catch (Exception e) { + throw convertInstantiationException(e, originalClass); + } + throw new IllegalArgumentException(String.format( + CREATE_INSTANCE_FAILED_NO_PUBLIC_NOARG_CONSTRUCTOR, + originalClass.getName())); + } + + /** + * Helper to handle all exceptions which might occur during class + * instantiation and throws an {@link IllegalArgumentException} with a + * descriptive error message hinting of what might be wrong with the class + * that could not be instantiated. Descriptive message is derived based on + * the information about the {@code clazz}. + * + * @param exception + * original exception + * @param clazz + * instantiation target class + */ + public static IllegalArgumentException convertInstantiationException( + Exception exception, Class clazz) { + if (exception instanceof InstantiationException) { + if (clazz.isMemberClass() + && !Modifier.isStatic(clazz.getModifiers())) { + return new IllegalArgumentException(String.format( CREATE_INSTANCE_FAILED_FOR_NON_STATIC_MEMBER_CLASS, - originalClass.getName()), e); + clazz.getName()), exception); } else { - throw new IllegalArgumentException(String.format( - CREATE_INSTANCE_FAILED, originalClass.getName()), e); + return new IllegalArgumentException( + String.format(CREATE_INSTANCE_FAILED, clazz.getName()), + exception); } - } catch (IllegalAccessException e) { - throw new IllegalArgumentException( + } else if (exception instanceof IllegalAccessException) { + return new IllegalArgumentException( String.format(CREATE_INSTANCE_FAILED_ACCESS_EXCEPTION, - originalClass.getName()), - e); - } catch (IllegalArgumentException e) { - throw new IllegalArgumentException(String.format( - CREATE_INSTANCE_FAILED, originalClass.getName()), e); - } catch (InvocationTargetException e) { - throw new IllegalArgumentException(String.format( + clazz.getName()), + exception); + } else if (exception instanceof IllegalArgumentException) { + return new IllegalArgumentException( + String.format(CREATE_INSTANCE_FAILED, clazz.getName()), + exception); + } else if (exception instanceof InvocationTargetException) { + return new IllegalArgumentException(String.format( CREATE_INSTANCE_FAILED_CONSTRUCTOR_THREW_EXCEPTION, - originalClass.getName()), e); + clazz.getName()), exception); } - throw new IllegalArgumentException(String.format( + return new IllegalArgumentException(String.format( CREATE_INSTANCE_FAILED_NO_PUBLIC_NOARG_CONSTRUCTOR, - originalClass.getName())); + clazz.getName())); } /** From 86781260515a6cc601000040acce9df8a8ca04ee Mon Sep 17 00:00:00 2001 From: Teppo Kurki Date: Tue, 20 Aug 2024 17:52:11 +0300 Subject: [PATCH 3/6] ensure record param order; fix exception handling --- .../com/vaadin/flow/data/binder/Binder.java | 19 +++++++++++-------- .../vaadin/flow/data/binder/BinderTest.java | 2 +- 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java b/flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java index ac8bacd7ca6..86f2701c9dd 100644 --- a/flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java +++ b/flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java @@ -18,6 +18,7 @@ import java.io.Serializable; import java.lang.reflect.Field; import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.RecordComponent; import java.lang.reflect.Type; import java.util.ArrayList; import java.util.Arrays; @@ -2542,6 +2543,8 @@ public boolean writeBeanIfValid(BEAN bean) { * @return a record instance with current values * @throws ValidationException * if some of the bound field values fail to validate + * @throws IllegalArgumentException + * if record instantiation fails for any reason */ public BEAN writeRecord() throws ValidationException { BEAN record = null; @@ -2564,10 +2567,12 @@ public BEAN writeRecord() throws ValidationException { boolean validatorsDisabledStatus = isValidatorsDisabled(); setValidatorsDisabled(true); // Fetch all conversion results - List> values = currentBindings.stream() - .map(binding -> ((BindingImpl) binding) - .doConversion()) - .toList(); + List> values = new ArrayList<>(); + for (RecordComponent rc : beanType.getRecordComponents()) { + Result value = ((BindingImpl) boundProperties + .get(rc.getName())).doConversion(); + values.add(value); + } setValidatorsDisabled(validatorsDisabledStatus); // Gather successfully converted values @@ -2577,10 +2582,8 @@ public BEAN writeRecord() throws ValidationException { try { record = beanType.cast(beanType.getDeclaredConstructors()[0] .newInstance(convertedValues.toArray())); - } catch (InstantiationException | IllegalAccessException - | InvocationTargetException e) { - // TODO replace with something better - throw new RuntimeException(e); + } catch (Exception e) { + throw ReflectTools.convertInstantiationException(e, beanType); } // Now run bean level validation against the created record diff --git a/flow-data/src/test/java/com/vaadin/flow/data/binder/BinderTest.java b/flow-data/src/test/java/com/vaadin/flow/data/binder/BinderTest.java index 3ba9d8e2a0b..156c8d34648 100644 --- a/flow-data/src/test/java/com/vaadin/flow/data/binder/BinderTest.java +++ b/flow-data/src/test/java/com/vaadin/flow/data/binder/BinderTest.java @@ -2690,11 +2690,11 @@ public void readRecord_writeRecord() throws ValidationException { TestTextField ageField = new TestTextField(); ageField.setValue(""); - binder.forField(nameField).bind("name"); binder.forField(ageField) .withConverter( new StringToIntegerConverter(0, "Failed to convert")) .bind("age"); + binder.forField(nameField).bind("name"); binder.readBean(new TestRecord("test", 42)); // Check that fields are enabled for records From 09839fb032f8ba36504a80f68d30efcc26260f52 Mon Sep 17 00:00:00 2001 From: Teppo Kurki Date: Wed, 21 Aug 2024 09:36:58 +0300 Subject: [PATCH 4/6] fix javadoc --- .../src/main/java/com/vaadin/flow/internal/ReflectTools.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/flow-server/src/main/java/com/vaadin/flow/internal/ReflectTools.java b/flow-server/src/main/java/com/vaadin/flow/internal/ReflectTools.java index 250dc21e8d6..4ebb236ca1a 100644 --- a/flow-server/src/main/java/com/vaadin/flow/internal/ReflectTools.java +++ b/flow-server/src/main/java/com/vaadin/flow/internal/ReflectTools.java @@ -503,7 +503,7 @@ public static T createProxyInstance(Class proxyClass, /** * Helper to handle all exceptions which might occur during class - * instantiation and throws an {@link IllegalArgumentException} with a + * instantiation and returns an {@link IllegalArgumentException} with a * descriptive error message hinting of what might be wrong with the class * that could not be instantiated. Descriptive message is derived based on * the information about the {@code clazz}. @@ -512,6 +512,7 @@ public static T createProxyInstance(Class proxyClass, * original exception * @param clazz * instantiation target class + * @return an IllegalArgumentException with descriptive message */ public static IllegalArgumentException convertInstantiationException( Exception exception, Class clazz) { From df4c7acf9449f6969a825528be5f94d769f31f09 Mon Sep 17 00:00:00 2001 From: Teppo Kurki Date: Wed, 21 Aug 2024 10:21:44 +0300 Subject: [PATCH 5/6] Check for and throw if binding is missing --- .../com/vaadin/flow/data/binder/Binder.java | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java b/flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java index 86f2701c9dd..24089794b1b 100644 --- a/flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java +++ b/flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java @@ -2543,6 +2543,8 @@ public boolean writeBeanIfValid(BEAN bean) { * @return a record instance with current values * @throws ValidationException * if some of the bound field values fail to validate + * @throws IllegalStateException + * if a record component does not have a binding * @throws IllegalArgumentException * if record instantiation fails for any reason */ @@ -2569,9 +2571,19 @@ public BEAN writeRecord() throws ValidationException { // Fetch all conversion results List> values = new ArrayList<>(); for (RecordComponent rc : beanType.getRecordComponents()) { - Result value = ((BindingImpl) boundProperties - .get(rc.getName())).doConversion(); - values.add(value); + String name = rc.getName(); + if (boundProperties.containsKey(name)) { + Result value = ((BindingImpl) boundProperties + .get(name)).doConversion(); + values.add(value); + } else { + throw new IllegalStateException( + "Unable to create record since no " + + "binding was found for record component '" + + name + + "'. Please create bindings for all record components " + + "using their names as the propertyName."); + } } setValidatorsDisabled(validatorsDisabledStatus); From d99d2f88090e4a5973fdfa5b5ee023ac32e511f1 Mon Sep 17 00:00:00 2001 From: Teppo Kurki Date: Thu, 22 Aug 2024 17:34:43 +0300 Subject: [PATCH 6/6] Improve javadocs and exceptions --- .../java/com/vaadin/flow/data/binder/Binder.java | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java b/flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java index 24089794b1b..bfd710832c4 100644 --- a/flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java +++ b/flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java @@ -2260,6 +2260,8 @@ public Binding bindReadOnly( * @param bean * the bean to edit, or {@code null} to remove a currently bound * bean and clear bound fields + * @throws IllegalStateException + * if the Binder's model type is record */ public void setBean(BEAN bean) { if (isRecord) { @@ -2544,11 +2546,16 @@ public boolean writeBeanIfValid(BEAN bean) { * @throws ValidationException * if some of the bound field values fail to validate * @throws IllegalStateException - * if a record component does not have a binding + * if a record component does not have a binding, or if the + * Binder's model type is bean * @throws IllegalArgumentException * if record instantiation fails for any reason */ public BEAN writeRecord() throws ValidationException { + if (!isRecord) { + throw new IllegalStateException( + "writeRecord methods can't be used with beans, call writeBean instead"); + } BEAN record = null; List binderResults = Collections.emptyList(); @@ -2631,6 +2638,8 @@ record = beanType.cast(beanType.getDeclaredConstructors()[0] * 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. + * @throws IllegalStateException + * if the Binder's model type is record */ @SuppressWarnings("unchecked") private BinderValidationStatus doWriteIfValid(BEAN bean, @@ -2706,6 +2715,8 @@ private BinderValidationStatus doWriteIfValid(BEAN bean, * the set of bindings to write to the bean * @param forced * disable validators during write if true + * @throws IllegalStateException + * if the Binder's model type is record */ @SuppressWarnings({ "unchecked" }) private void doWriteDraft(BEAN bean, Collection> bindings,