feat:Improve Binder change detection #19488

onuridrisoglu · 2024-05-31T10:09:58Z

Description

Fixes #19260

Type of change

Bugfix
Feature

Checklist

I have read the contribution guide: https://vaadin.com/docs/latest/guide/contributing/overview/
I have added a description following the guideline.
The issue is created in the corresponding repository and I have referenced it.
I have added tests to ensure my change is effective and works as intended.
New and existing tests are passing locally with my change.
I have performed self-review and corrected misspellings.

Additional for `Feature` type of change

Enhancement / new feature was discussed in a corresponding GitHub issue and Acceptance Criteria were created.

CLAassistant · 2024-05-31T10:10:04Z

All committers have signed the CLA.

CLAassistant · 2024-05-31T10:10:04Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

github-actions · 2024-05-31T10:25:20Z

Test Results

1 132 files ±0 1 132 suites ±0 1h 28m 45s ⏱️ - 1m 7s
7 356 tests +3 7 306 ✅ +3 50 💤 ±0 0 ❌ ±0
7 717 runs - 6 7 657 ✅ - 6 60 💤 ±0 0 ❌ ±0

Results for commit 326ebf5. ± Comparison against base commit 999db1c.

♻️ This comment has been updated with latest results.

tepi · 2024-06-10T05:28:03Z

flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java

-    protected void handleFieldValueChange(Binding<BEAN, ?> binding) {
-        changedBindings.add(binding);
-
+    protected void handleFieldValueChange(Binding<BEAN, ?> binding, ValueChangeEvent<?> event) {


Please avoid changing this method signature since it would be a breaking change and require waiting for the next major. Instead, one option could be to deprecate the current one, and if that is used, just skip the new change detection logic and function as before.

Added the current handleFieldValueChange back as deprecated

tepi · 2024-06-10T05:28:51Z

flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java

+     * @return true if the field value is reverted
+     */
+    protected boolean isRevertedToInitialValue(Binding<BEAN, ?> binding, Object newValue) {
+		return Objects.equals(bindingInitialValuesMap.get(binding), newValue);


The logic here relies on equals of the value type being implemented properly. That is something the Binder has not required before, so it's potentially a breaking change.

Also, it migh make more sense to add Binding-level API for overriding the logic for checking if value is reverted. That way if custom logic is needed, one does not need to extend Binder to do it.

introduced an equality predicate (I'm open for naming suggestions) that uses Objects::equals as default, but it can be overridden in binding-level

tepi · 2024-06-10T05:30:57Z

@mshabarov This change will introduce a slight change in behavior of Binder change tracking due to requiring equals to be implemented properly by value types. As well as reverting the tracked change if the value is reverted, which is not currently done (that is, the binding is considered as changed anyway). Do you think we'll need an enable/disable switch for this new change tracking?

and deprecated the old handleFieldValueChange method

knoobie · 2024-06-17T12:09:06Z

flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java

+         * @param equalityPredicate the predicate to use for equality comparison
+         * @return this {@code BindingBuilder}, for method chaining
+         */
+        public BindingBuilder<BEAN, TARGET> withEqualityPredicate(SerializableBiPredicate<Object, Object> equalityPredicate);


Vote 4 default implementation; otherwise this is a breaking change

sonarqubecloud · 2024-06-17T16:46:36Z

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

mshabarov · 2024-06-25T11:53:16Z

flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java

@@ -320,6 +321,8 @@ default BindingValidationStatus<TARGET> validate() {
         *         changes, otherwise {@literal false}.
         */
        boolean hasChanges();
+
+        SerializableBiPredicate<Object, Object> getEqualityPredicate();


This misses Javadoc.

Can the type be instead TARGET (data type of the binding), not Object ?

mshabarov · 2024-06-25T13:05:12Z

flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java

@@ -1810,6 +1850,7 @@ void setIdentity() {

    private BindingExceptionHandler exceptionHandler = new DefaultBindingExceptionHandler();

+    private Map<Binding<BEAN, ?>, Object> bindingInitialValuesMap = new HashMap<>();


What if the initial values would be a class field in the BindingImpl instead and have a getter method in Binding? Then you can call newly added method Binding::getInitialValue.

I refactored the changes and removed bindingInitialValuesMap. Now the initial value is kept in Binding level which is set during readBean.

mshabarov · 2024-06-25T13:08:34Z

@mshabarov This change will introduce a slight change in behavior of Binder change tracking due to requiring equals to be implemented properly by value types. As well as reverting the tracked change if the value is reverted, which is not currently done (that is, the binding is considered as changed anyway). Do you think we'll need an enable/disable switch for this new change tracking?

I'd propose to set the equality predicate null by default (or some other value that marks it not defined), that means the change detection feature is not enabled by default. Then if this predicate is set by BindingBuilder::withEqualityPredicate, then this feature is enabled. In this way this would be backwards compatible.

- 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

onuridrisoglu · 2024-08-07T09:52:10Z

@mshabarov This change will introduce a slight change in behavior of Binder change tracking due to requiring equals to be implemented properly by value types. As well as reverting the tracked change if the value is reverted, which is not currently done (that is, the binding is considered as changed anyway). Do you think we'll need an enable/disable switch for this new change tracking?

I'd propose to set the equality predicate null by default (or some other value that marks it not defined), that means the change detection feature is not enabled by default. Then if this predicate is set by BindingBuilder::withEqualityPredicate, then this feature is enabled. In this way this would be backwards compatible.

I did the changes, however my only concern is now withEqualityPredicate should be called for each binding in order to activate this feature.

mshabarov

Looks good to me, but I haven't tested it.

I agree with the concern about having this API only on a binding level, would be good to allow doing the same for the whole form.

mshabarov · 2024-08-23T06:20:41Z

Just discussed with @tepi, the Binder-level API could be done in a way:

Adding a method on Binder's level that enables/disables usage of these predicates on bindings level.
Then, if enabled, Binder will use a configured predicate for a binding, if it exists, otherwise it uses equals as a fallback. If disabled, nothing is used (default).

mshabarov

Setting to request changes

tepi · 2024-08-23T08:14:41Z

Just discussed with @tepi, the Binder-level API could be done in a way:

1. Adding a method on Binder's level that enables/disables usage of these predicates on bindings level.

2. Then, if enabled, Binder will use a configured predicate for a binding, if it exists, otherwise it uses `equals` as a fallback. If disabled, nothing is used (default).

@onuridrisoglu fyi, I'll pick this up and do the changes

sonarqubecloud · 2024-08-23T09:00:57Z

Quality Gate passed

Issues
7 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

Introduced binder change detection for readBean case

d201945

mcollovati added the Contribution PRs coming from the community or external to the team label May 31, 2024

vaadin-bot added the +1.0.0 label May 31, 2024

mshabarov requested a review from tepi June 3, 2024 11:36

Merge branch 'main' into issues/19260-binder-change-detection

c6f5f92

tepi reviewed Jun 10, 2024

View reviewed changes

mshabarov self-requested a review June 10, 2024 11:37

Added equality predicate to Binding level

e48ac6b

and deprecated the old handleFieldValueChange method

knoobie reviewed Jun 17, 2024

View reviewed changes

onuridrisoglu added 2 commits June 17, 2024 18:30

Fixed unit test error

75a5fba

Added default implementation for the new withEqualityPredicate method

75cfc93

onuridrisoglu marked this pull request as ready for review June 17, 2024 15:49

Added missing line from the deprecated method

267fad0

mshabarov requested changes Jun 25, 2024

View reviewed changes

onuridrisoglu and others added 2 commits August 7, 2024 12:45

Changes for review comments:

88f974b

- 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

Merge branch 'main' into issues/19260-binder-change-detection

6faa162

vaadin-bot added +0.0.0 and removed +1.0.0 labels Aug 7, 2024

fixed format issues

d6f7d55

onuridrisoglu requested review from mshabarov and tepi August 7, 2024 09:53

vaadin-bot added the +0.1.0 label Aug 7, 2024

mcollovati removed the +0.0.0 label Aug 7, 2024

Merge branch 'main' into issues/19260-binder-change-detection

b570d42

mshabarov approved these changes Aug 23, 2024

View reviewed changes

mshabarov requested changes Aug 23, 2024

View reviewed changes

tepi added 3 commits August 23, 2024 11:44

Add global setting and default comparison

158e743

Add tests

c79b79f

Merge branch 'main' into issues/19260-binder-change-detection

326ebf5

mshabarov approved these changes Aug 23, 2024

View reviewed changes

mshabarov enabled auto-merge (squash) August 23, 2024 08:59

mshabarov mentioned this pull request Aug 23, 2024

Docs for Binder change detection improvements vaadin/docs#3612

Closed

mshabarov merged commit 7973103 into main Aug 23, 2024
25 of 26 checks passed

mshabarov deleted the issues/19260-binder-change-detection branch August 23, 2024 09:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat:Improve Binder change detection #19488

feat:Improve Binder change detection #19488

onuridrisoglu commented May 31, 2024

CLAassistant commented May 31, 2024 •

edited

Loading

CLAassistant commented May 31, 2024

github-actions bot commented May 31, 2024 •

edited

Loading

tepi Jun 10, 2024

onuridrisoglu Jun 17, 2024

tepi Jun 10, 2024

tepi Jun 10, 2024

onuridrisoglu Jun 17, 2024

tepi commented Jun 10, 2024

knoobie Jun 17, 2024

onuridrisoglu Jun 17, 2024

sonarqubecloud bot commented Jun 17, 2024

mshabarov Jun 25, 2024

mshabarov Jun 25, 2024

onuridrisoglu Aug 7, 2024

mshabarov Jun 25, 2024

onuridrisoglu Aug 7, 2024

mshabarov commented Jun 25, 2024

onuridrisoglu commented Aug 7, 2024

mshabarov left a comment

mshabarov commented Aug 23, 2024

mshabarov left a comment

tepi commented Aug 23, 2024

sonarqubecloud bot commented Aug 23, 2024

		@@ -1810,6 +1850,7 @@ void setIdentity() {

		private BindingExceptionHandler exceptionHandler = new DefaultBindingExceptionHandler();

		private Map<Binding<BEAN, ?>, Object> bindingInitialValuesMap = new HashMap<>();

feat:Improve Binder change detection #19488

feat:Improve Binder change detection #19488

Conversation

onuridrisoglu commented May 31, 2024

Description

Type of change

Checklist

Additional for Feature type of change

CLAassistant commented May 31, 2024 • edited Loading

CLAassistant commented May 31, 2024

github-actions bot commented May 31, 2024 • edited Loading

Test Results

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tepi commented Jun 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jun 17, 2024

Quality Gate passed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mshabarov commented Jun 25, 2024

onuridrisoglu commented Aug 7, 2024

mshabarov left a comment

Choose a reason for hiding this comment

mshabarov commented Aug 23, 2024

mshabarov left a comment

Choose a reason for hiding this comment

tepi commented Aug 23, 2024

sonarqubecloud bot commented Aug 23, 2024

Quality Gate passed

Additional for `Feature` type of change

CLAassistant commented May 31, 2024 •

edited

Loading

github-actions bot commented May 31, 2024 •

edited

Loading