fix: make property change listener trigger on removeProperty #19196

akhodyko · 2024-04-17T17:40:30Z

Removing element property by calling com.vaadin.flow.internal.nodefeature.AbstractPropertyMap#removeProperty does not trigger property change listener because of com.vaadin.flow.internal.nodefeature.ElementPropertyMap#remove method is skipped due to AbstractPropertyMap#removeProperty method calls super:

    public void removeProperty(String name) {
        super.remove(name);
    }

Fixes #3994

CLAassistant · 2024-04-17T17:40:35Z

All committers have signed the CLA.

github-actions · 2024-04-18T06:03:35Z

Test Results

1 099 files ±0 1 099 suites ±0 1h 16m 50s ⏱️ - 4m 26s
7 006 tests +9 6 957 ✅ +9 49 💤 ±0 0 ❌ ±0
7 365 runs +5 7 304 ✅ +5 61 💤 ±0 0 ❌ ±0

Results for commit cfb48e3. ± Comparison against base commit fa7d789.

♻️ This comment has been updated with latest results.

mshabarov · 2024-04-19T12:55:37Z

@akhodyko thanks for you contribution! Looks good to me! May I transfer this pull request to "ready for review" state?

akhodyko · 2024-04-21T19:48:26Z

@akhodyko thanks for you contribution! Looks good to me! May I transfer this pull request to "ready for review" state?

yes, you can, but there are some trouble with com.vaadin.flow.data.binder.BinderTest.readBean_converterThrows_exceptionHandlerSet_bindingExceptionIsThrown test yet.

mshabarov · 2024-04-22T06:28:17Z

@akhodyko thanks for you contribution! Looks good to me! May I transfer this pull request to "ready for review" state?

yes, you can, but there are some trouble with com.vaadin.flow.data.binder.BinderTest.readBean_converterThrows_exceptionHandlerSet_bindingExceptionIsThrown test yet.

That seems wrong, if the Binder doesn't throw while the exception happens in the converter. Weird side effect. We will take a look, thanks!

akhodyko · 2024-04-22T12:50:44Z

@mshabarov

I think readBean_converterThrows_exceptionHandlerSet_bindingExceptionIsThrown test is not fully correct.

I will try to explain:

Before this PR, setting null value to com.vaadin.flow.component.AbstractSinglePropertyField was not triggered property change listener on removing property here com.vaadin.flow.component.AbstractSinglePropertyField.TypeHandler#createWriter.
Because of point 1, com.vaadin.flow.component.internal.AbstractFieldSupport#pendingValueFromPresentation was not updated to actual value taking into account default value of field. This caused the field value change event to fire.
After that, value change event caused validation at com.vaadin.flow.data.binder.Binder#handleFieldValueChange followed with conversion field value to model.

According to that, because of we have in that test com.vaadin.flow.data.binder.testcomponents.TestTextField accepting null values with default empty string value, null value must be treated as empty string and should not trigger the change listener and toModel conversion.

Replacing bean Person to another with non null firstName field fixed the test.

mshabarov

As the property removal is now considered as non-client originated event, we have to change to Assert.assertFalse(event.get().isUserOriginated()); in ElementPropertyMapTest::removeProperty_fireEvent_listenerIsNotNotified

mshabarov · 2024-04-26T12:19:01Z

According to that, because of we have in that test com.vaadin.flow.data.binder.testcomponents.TestTextField accepting null values with default empty string value, null value must be treated as empty string and should not trigger the change listener and toModel conversion.
Replacing bean Person to another with non null firstName field fixed the test.

This makes sense, okay.

mshabarov · 2024-04-26T12:21:44Z

Requested another review from @caalador , just in case, if I missed something important, the overall fix looks good to me and correct.

akhodyko · 2024-04-26T14:20:03Z

As the property removal is now considered as non-client originated event, we have to change to Assert.assertFalse(event.get().isUserOriginated()); in ElementPropertyMapTest::removeProperty_fireEvent_listenerIsNotNotified

done

caalador

Looks good to me.

mshabarov · 2024-04-29T10:21:09Z

@akhodyko could you please run mvn formatter:format and push? Then we can merge this change.

akhodyko · 2024-05-02T08:23:57Z

@akhodyko could you please run mvn formatter:format and push? Then we can merge this change.

done

sonarcloud · 2024-05-02T08:36:21Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarCloud

vaadin-bot · 2024-05-02T09:31:59Z

Hi @akhodyko and @mshabarov, when i performed cherry-pick to this commit to 24.3, i have encountered the following issue. Can you take a look and pick it manually?
Error Message:
Error: Command failed: git cherry-pick 63e6dfd
error: could not apply 63e6dfd... fix: make property change listener trigger on removeProperty (#19196)
hint: After resolving the conflicts, mark them with
hint: "git add/rm ", then run
hint: "git cherry-pick --continue".
hint: You can instead skip this commit with "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".

…19196)" This reverts commit 63e6dfd.

…19196)" (#19310) This reverts commit 63e6dfd. The original fix for the property change event is correct per se, but, unfortunately, we faced an odd side effect in the RadioButtonGroup component that blocks our development process and would be a breaking change for RadioButtonGroup's users. Here are some details: The getValue() method of the RadioButtonGroup (and probably other similar components, like Select and CheckboxGroup) returned an item that was set in setValue() before, even if it was an invalid item, e.g. it's key/id wasn't in the list of items for this component. With this change, the PropertyChangeEvent is triggered, that causes handlePropertyChange and hasValidValue method to be called. hasValidValue doesn't properly check if the item is in the list of items on the server and returns true, that causes a state change in setModelValue, that eventually resets the value to null. In other words, if one wants to set a new value, which isn't present in the item's list, the component set the value to null, which sounds like even a bigger bug. The proper behaviour, in my opinion, should be to not accept invalid values and make hasValidValue implementation properly check if the item is present or not, but this turns into more complicated work that may even have a breaking changes in behaviour.

mshabarov · 2024-05-06T12:56:36Z

This has been reverted due to a regression in Vaadin Flow components (to be more precise, in RadioButtonGroup) that blocks our releases, see #19310.

For the record, the failed test is this:

[setIdentifierProviderOnId_setItemWithNullId_shouldFailToSelectExistingItemById] java.lang.NullPointerException: Cannot invoke "com.vaadin.flow.component.radiobutton.CustomItem.getId()" because the return value of "com.vaadin.flow.component.radiobutton.RadioButtonGroup.getValue()" is null
	at com.vaadin.flow.component.radiobutton.RadioButtonGroupTest.setIdentifierProviderOnId_setItemWithNullId_shouldFailToSelectExistingItemById(RadioButtonGroupTest.java:441)

I.e. the component reset the value to null, if setValue is called with an item, that is not present in the items list.
We plan to reconsider the logic in RadioButtonGroup regards invalid items and re-apply this patch again.

…19196)" (#19310) (#19311) This reverts commit 63e6dfd. The original fix for the property change event is correct per se, but, unfortunately, we faced an odd side effect in the RadioButtonGroup component that blocks our development process and would be a breaking change for RadioButtonGroup's users. Here are some details: The getValue() method of the RadioButtonGroup (and probably other similar components, like Select and CheckboxGroup) returned an item that was set in setValue() before, even if it was an invalid item, e.g. it's key/id wasn't in the list of items for this component. With this change, the PropertyChangeEvent is triggered, that causes handlePropertyChange and hasValidValue method to be called. hasValidValue doesn't properly check if the item is in the list of items on the server and returns true, that causes a state change in setModelValue, that eventually resets the value to null. In other words, if one wants to set a new value, which isn't present in the item's list, the component set the value to null, which sounds like even a bigger bug. The proper behaviour, in my opinion, should be to not accept invalid values and make hasValidValue implementation properly check if the item is present or not, but this turns into more complicated work that may even have a breaking changes in behaviour. Co-authored-by: Mikhail Shabarov <61410877+mshabarov@users.noreply.github.com>

vaadin-bot · 2024-05-08T14:42:24Z

This ticket/PR has been released with Vaadin 24.4.0.beta2 and is also targeting the upcoming stable 24.4.0 version.

vaadin-bot · 2024-05-31T11:28:06Z

This ticket/PR has been released with Vaadin 24.5.0.alpha1 and is also targeting the upcoming stable 24.5.0 version.

akhodyko force-pushed the trigger-on-prop-removing branch from 55f096a to c5eb084 Compare April 17, 2024 17:50

caalador added the Contribution PRs coming from the community or external to the team label Apr 18, 2024

vaadin-bot added the +0.0.1 label Apr 18, 2024

akhodyko marked this pull request as draft April 18, 2024 07:21

akhodyko force-pushed the trigger-on-prop-removing branch 2 times, most recently from c7cb2ff to 01f794d Compare April 18, 2024 10:08

mshabarov previously approved these changes Apr 19, 2024

View reviewed changes

akhodyko marked this pull request as ready for review April 21, 2024 19:38

akhodyko dismissed mshabarov’s stale review via c7f5c9d April 22, 2024 12:09

akhodyko force-pushed the trigger-on-prop-removing branch from 138983e to c7f5c9d Compare April 22, 2024 12:09

akhodyko force-pushed the trigger-on-prop-removing branch 2 times, most recently from 2ef3919 to e826351 Compare April 22, 2024 15:01

mshabarov requested changes Apr 26, 2024

View reviewed changes

mshabarov requested a review from caalador April 26, 2024 12:19

akhodyko force-pushed the trigger-on-prop-removing branch from fb694fb to 4c41f4f Compare April 26, 2024 13:52

akhodyko requested a review from mshabarov April 26, 2024 14:20

caalador reviewed Apr 29, 2024

View reviewed changes

fix: make property change listener trigger on removeProperty

cfb48e3

akhodyko force-pushed the trigger-on-prop-removing branch from 5f86d1a to cfb48e3 Compare May 2, 2024 08:22

mshabarov approved these changes May 2, 2024

View reviewed changes

mshabarov added the target/24.3 label May 2, 2024

mshabarov merged commit 63e6dfd into vaadin:main May 2, 2024
28 checks passed

vaadin-bot added the need to pick manually 24.3 label May 2, 2024

mshabarov added a commit that referenced this pull request May 6, 2024

Revert "fix: make property change listener trigger on removeProperty (#…

ef3d350

…19196)" This reverts commit 63e6dfd.

vaadin-bot added prerelease version for Vaadin 24.4.0 Released with Vaadin 24.4.0.beta2 labels May 8, 2024

vaadin-bot added prerelease version for Vaadin 24.5.0 Released with Vaadin 24.5.0.alpha1 labels May 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: make property change listener trigger on removeProperty #19196

fix: make property change listener trigger on removeProperty #19196

akhodyko commented Apr 17, 2024

CLAassistant commented Apr 17, 2024 •

edited

github-actions bot commented Apr 18, 2024 •

edited

mshabarov commented Apr 19, 2024

akhodyko commented Apr 21, 2024

mshabarov commented Apr 22, 2024

akhodyko commented Apr 22, 2024

mshabarov left a comment

mshabarov commented Apr 26, 2024

mshabarov commented Apr 26, 2024

akhodyko commented Apr 26, 2024

caalador left a comment

mshabarov commented Apr 29, 2024

akhodyko commented May 2, 2024

sonarcloud bot commented May 2, 2024

vaadin-bot commented May 2, 2024

mshabarov commented May 6, 2024

vaadin-bot commented May 8, 2024

vaadin-bot commented May 31, 2024

fix: make property change listener trigger on removeProperty #19196

fix: make property change listener trigger on removeProperty #19196

Conversation

akhodyko commented Apr 17, 2024

CLAassistant commented Apr 17, 2024 • edited

github-actions bot commented Apr 18, 2024 • edited

Test Results

mshabarov commented Apr 19, 2024

akhodyko commented Apr 21, 2024

mshabarov commented Apr 22, 2024

akhodyko commented Apr 22, 2024

mshabarov left a comment

Choose a reason for hiding this comment

mshabarov commented Apr 26, 2024

mshabarov commented Apr 26, 2024

akhodyko commented Apr 26, 2024

caalador left a comment

Choose a reason for hiding this comment

mshabarov commented Apr 29, 2024

akhodyko commented May 2, 2024

sonarcloud bot commented May 2, 2024

Quality Gate passed

vaadin-bot commented May 2, 2024

mshabarov commented May 6, 2024

vaadin-bot commented May 8, 2024

vaadin-bot commented May 31, 2024

CLAassistant commented Apr 17, 2024 •

edited

github-actions bot commented Apr 18, 2024 •

edited