Skip to content

Commit

Permalink
feat:Improve Binder change detection (#19488)
Browse files Browse the repository at this point in the history 
Fixes #19260

* Introduced binder change detection for readBean case

* Added equality predicate to Binding level

and deprecated the old handleFieldValueChange method

* Fixed unit test error

* Added default implementation for the new withEqualityPredicate method

* Added missing line from the deprecated method

* Changes for review comments:

- removed bindingInitialValuesMap from Binder, and kept the initial
value in binding level instead
- removed the default implementation for equalityPredicate and now this
feature will only be active if an equalityPredicate is set

* fixed format issues

* Add global setting and default comparison

* Add tests

---------

Co-authored-by: Teppo Kurki <[email protected]>
Co-authored-by: Mikhail Shabarov <[email protected]>
  • Loading branch information
@onuridrisoglu @tepi @mshabarov
3 people authored Aug 23, 2024
1 parent a3d45c8 commit 7973103
Show file tree
Hide file tree
Showing 2 changed files with 235 additions and 1 deletion.
132 changes: 131 additions & 1 deletion flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@
import com.vaadin.flow.data.converter.StringToIntegerConverter;
import com.vaadin.flow.data.validator.BeanValidator;
import com.vaadin.flow.dom.Style;
import com.vaadin.flow.function.SerializableBiPredicate;
import com.vaadin.flow.function.SerializableConsumer;
import com.vaadin.flow.function.SerializableFunction;
import com.vaadin.flow.function.SerializablePredicate;
Expand Down Expand Up @@ -321,6 +322,18 @@ default BindingValidationStatus<TARGET> validate() {
* changes, otherwise {@literal false}.
*/
boolean hasChanges();

/**
* Used in comparison of the current value of a field with its initial
* value.
* <p>
* Once set, the value of the field that binding uses will be compared
* with the initial value for hasChanged.
* </p>
*
* @return the predicate to use for equality comparison
*/
SerializableBiPredicate<TARGET, TARGET> getEqualityPredicate();
}

/**
Expand Down Expand Up @@ -951,6 +964,26 @@ BindingBuilder<BEAN, TARGET> asRequired(
*/
public BindingBuilder<BEAN, TARGET> asRequired(
Validator<TARGET> customRequiredValidator);

/**
* Sets the {@code equalityPredicate} used to compare the current value
* of a field with its initial value.
* <p>
* By default it is {@literal null}, meaning the initial value
* comparison is not active. Once it is set, the value of the field will
* be compared with its initial value. If the value of the field is set
* back to its initial value, it will not be considered as having
* uncommitted changes.
* </p>
*
* @param equalityPredicate
* the predicate to use for equality comparison
* @return this {@code BindingBuilder}, for method chaining
*/
public default BindingBuilder<BEAN, TARGET> withEqualityPredicate(
SerializableBiPredicate<TARGET, TARGET> equalityPredicate) {
return this;
}
}

/**
Expand Down Expand Up @@ -986,6 +1019,13 @@ protected static class BindingBuilderImpl<BEAN, FIELDVALUE, TARGET>

private Boolean defaultValidatorEnabled;

/**
* A predicate used to compare the current value of a field with its
* initial value. By default it is {@literal null} meaning that the
* initial value comparison is not active
*/
private SerializableBiPredicate<TARGET, TARGET> equalityPredicate = null;

/**
* Creates a new binding builder associated with the given field.
* Initializes the builder with the given converter chain and status
Expand Down Expand Up @@ -1204,6 +1244,15 @@ public BindingBuilder<BEAN, TARGET> asRequired(
});
}

@Override
public BindingBuilder<BEAN, TARGET> withEqualityPredicate(
SerializableBiPredicate<TARGET, TARGET> equalityPredicate) {
Objects.requireNonNull(equalityPredicate,
"equality predicate cannot be null");
this.equalityPredicate = equalityPredicate;
return this;
}

/**
* Implements {@link #withConverter(Converter)} method with additional
* possibility to disable (reset) default null representation converter.
Expand Down Expand Up @@ -1318,6 +1367,10 @@ protected static class BindingImpl<BEAN, FIELDVALUE, TARGET>

private Boolean defaultValidatorEnabled;

private SerializableBiPredicate<TARGET, TARGET> equalityPredicate;

private TARGET initialValue;

public BindingImpl(BindingBuilderImpl<BEAN, FIELDVALUE, TARGET> builder,
ValueProvider<BEAN, TARGET> getter,
Setter<BEAN, TARGET> setter) {
Expand All @@ -1329,6 +1382,8 @@ public BindingImpl(BindingBuilderImpl<BEAN, FIELDVALUE, TARGET> builder,

defaultValidatorEnabled = builder.defaultValidatorEnabled;

equalityPredicate = builder.equalityPredicate;

onValueChange = getField().addValueChangeListener(
event -> handleFieldValueChange(event));

Expand Down Expand Up @@ -1516,6 +1571,10 @@ private void handleFieldValueChange(
if (binder != null) {
// Inform binder of changes; if setBean: writeIfValid
getBinder().handleFieldValueChange(this);
// Compare the value with initial value, and remove the binder
// from changed bindings if reverted
removeFromChangedBindingsIfReverted(
getBinder()::removeFromChangedBindings);
getBinder().fireEvent(event);
}
}
Expand Down Expand Up @@ -1573,6 +1632,7 @@ private void convertAndSetFieldValue(TARGET modelValue) {
FIELDVALUE convertedValue = convertToFieldType(modelValue);
try {
field.setValue(convertedValue);
initialValue = modelValue;
} catch (RuntimeException e) {
/*
* Add an additional hint to the exception for the typical
Expand Down Expand Up @@ -1704,6 +1764,36 @@ public boolean hasChanges() throws IllegalStateException {

return this.binder.hasChanges(this);
}

@Override
public SerializableBiPredicate<TARGET, TARGET> getEqualityPredicate() {
return equalityPredicate;
}

/**
* compares the new value of the field with its initial value, and
* removes the current binding from the {@code changeBindings}, but only
* if {@code equalityPredicate} is set, or
* {@link #isChangeDetectionEnabled()} returns true.
*
* @param removeBindingAction
* the binding consumer that removes the binding from the
* {@code changeBindings}
*/
private void removeFromChangedBindingsIfReverted(
SerializableConsumer<Binding<BEAN, TARGET>> removeBindingAction) {
if (binder.isChangeDetectionEnabled()
|| equalityPredicate != null) {
doConversion().ifOk(convertedValue -> {
SerializableBiPredicate<TARGET, TARGET> effectivePredicate = equalityPredicate == null
? Objects::equals
: equalityPredicate;
if (effectivePredicate.test(initialValue, convertedValue)) {
removeBindingAction.accept(this);
}
});
}
}
}

/**
Expand Down Expand Up @@ -1824,6 +1914,8 @@ void setIdentity() {

private boolean defaultValidatorsEnabled = true;

private boolean changeDetectionEnabled = false;

/**
* Creates a binder using a custom {@link PropertySet} implementation for
* finding and resolving property names for
Expand Down Expand Up @@ -3902,10 +3994,17 @@ protected void removeBindingInternal(Binding<BEAN, ?> binding) {
if (bindings.remove(binding)) {
boundProperties.entrySet()
.removeIf(entry -> entry.getValue().equals(binding));
changedBindings.remove(binding);
removeFromChangedBindings(binding);
}
}

/**
* Removes (internally) the {@code Binding} from the changed bindings
*/
private void removeFromChangedBindings(Binding<BEAN, ?> binding) {
changedBindings.remove(binding);
}

/**
* Finds and removes the Binding for the given property name.
*
Expand Down Expand Up @@ -3963,6 +4062,37 @@ public boolean isFieldsValidationStatusChangeListenerEnabled() {
return fieldsValidationStatusChangeListenerEnabled;
}

/**
* Sets change/revert detection enabled or disabled. When set to
* {@literal true}, any binding that is first changed and then reverted to
* its original value will be removed from the list of changed bindings.
*
* By default, {@link Objects#equals(Object, Object)} is used for value
* comparison, but it can be overridden on binding level using
* {@link BindingBuilder#withEqualityPredicate(SerializableBiPredicate)}.
*
* @param changeDetectionEnabled
* Boolean value
*/
public void setChangeDetectionEnabled(boolean changeDetectionEnabled) {
this.changeDetectionEnabled = changeDetectionEnabled;
}

/**
* Returns if change/revert detection is enabled. When set to
* {@literal true}, any binding that is first changed and then reverted to
* its original value will be removed from the list of changed bindings.
*
* By default, {@link Objects#equals(Object, Object)} is used for value
* comparison, but it can be overridden on binding level using
* {@link BindingBuilder#withEqualityPredicate(SerializableBiPredicate)}.
*
* @return Boolean value
*/
public boolean isChangeDetectionEnabled() {
return changeDetectionEnabled;
}

/**
* Sets a {@code handler} to customize the {@link RuntimeException} thrown
* by delegates (like {@link Setter}, {@link ValueProvider},
Expand Down
104 changes: 104 additions & 0 deletions flow-data/src/test/java/com/vaadin/flow/data/binder/BinderTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -526,6 +526,110 @@ public void update_bound_propertyIsUpdated() throws ValidationException {
Assert.assertEquals(0, updatedPerson.getAge());
}

@Test
public void update_to_initial_value_removes_binding_from_changedBindings_with_set_predicates()
throws ValidationException {
Person person = new Person();
String initialName = "Foo";
person.setFirstName(initialName);
person.setAge(20);

Binder<Person> binder = new Binder<>();
Binding<Person, String> nameBinding = binder.forField(nameField)
.withEqualityPredicate(
(oldVal, newVal) -> Objects.equals(oldVal, newVal))
.bind(Person::getFirstName, Person::setFirstName);
Binding<Person, Integer> ageBinding = binder.forField(ageField)
.withConverter(new StringToIntegerConverter(""))
.withEqualityPredicate(
(oldVal, newVal) -> Objects.equals(oldVal, newVal))
.bind(Person::getAge, Person::setAge);

binder.readBean(person);
nameField.setValue("Bar");

assertEquals(1, binder.getChangedBindings().size());
assertTrue(binder.getChangedBindings().contains(nameBinding));

ageField.setValue("21");
assertEquals(2, binder.getChangedBindings().size());

nameField.setValue(initialName);

assertEquals(1, binder.getChangedBindings().size());
assertTrue(binder.getChangedBindings().contains(ageBinding));

ageField.setValue("20");
assertTrue(binder.getChangedBindings().isEmpty());
}

@Test
public void update_to_initial_value_removes_binding_from_changedBindings_with_default_predicates()
throws ValidationException {
Person person = new Person();
String initialName = "Foo";
person.setFirstName(initialName);
person.setAge(20);

Binder<Person> binder = new Binder<>();
binder.setChangeDetectionEnabled(true);
Binding<Person, String> nameBinding = binder.forField(nameField)
.bind(Person::getFirstName, Person::setFirstName);
Binding<Person, Integer> ageBinding = binder.forField(ageField)
.withConverter(new StringToIntegerConverter(""))
.bind(Person::getAge, Person::setAge);

binder.readBean(person);
nameField.setValue("Bar");

assertEquals(1, binder.getChangedBindings().size());
assertTrue(binder.getChangedBindings().contains(nameBinding));

ageField.setValue("21");
assertEquals(2, binder.getChangedBindings().size());

nameField.setValue(initialName);

assertEquals(1, binder.getChangedBindings().size());
assertTrue(binder.getChangedBindings().contains(ageBinding));

ageField.setValue("20");
assertTrue(binder.getChangedBindings().isEmpty());
}

@Test
public void update_to_initial_value_does_not_remove_binding_from_changedBindings_by_default()
throws ValidationException {
Person person = new Person();
String initialName = "Foo";
person.setFirstName(initialName);
person.setAge(20);

Binder<Person> binder = new Binder<>();
Binding<Person, String> nameBinding = binder.forField(nameField)
.bind(Person::getFirstName, Person::setFirstName);
Binding<Person, Integer> ageBinding = binder.forField(ageField)
.withConverter(new StringToIntegerConverter(""))
.bind(Person::getAge, Person::setAge);

binder.readBean(person);
nameField.setValue("Bar");

assertEquals(1, binder.getChangedBindings().size());
assertTrue(binder.getChangedBindings().contains(nameBinding));

ageField.setValue("21");
assertEquals(2, binder.getChangedBindings().size());
assertTrue(binder.getChangedBindings().contains(ageBinding));

nameField.setValue(initialName);

assertEquals(2, binder.getChangedBindings().size());

ageField.setValue("20");
assertEquals(2, binder.getChangedBindings().size());
}

@Test
public void save_bound_beanAsDraft() {
do_test_save_bound_beanAsDraft(false);
Expand Down

0 comments on commit 7973103

Please sign in to comment.