fix: register data provider listener only if it has not been registered

[TatuLund]

fixes: vaadin/flow-components#1914

[denis-anisimov]

This looks good but I spent some time to understand why it's needed.
From the code I see paired handleDetach method which do the clean up.
So assuming the expected workflow detach should always happen after attach and this check should not be needed at all.

The problem is in the method names which are uses in two cases:

• when the node is attached/detached (this is a normal expected workflow). And method names are fine for this situation.
• when data provider is set: handleDetach and then handleAttach are called .

In the second case the method names are not suitable. Since the same methods are used in two totally different cases they should be renamed.

This is one thing which I would like to see changed here.

Another : it's still not obvious that setDataProvider resets the listener in the same way as attach/detach does.
So even though the null check is quite safe change I would like to see a unit test which will allow to prevent introducing such regression back.

[Legioth]

This looks good but I spent some time to understand why it's needed.

That's because the PR doesn't contain any tests

[Legioth]

The conventional way of dealing with cases like this is to remove() the old registration if one exists before writing a new value to the field, instead of assuming that the old registration can be reused. That approach is slightly more robust since it would also cover the case of changing from one data provider to another without "detach" in between - this isn't present in the code today but there's no guarantee that it won't occur as a result of future changes.

[TatuLund]

@Legioth , good point, I improved my solution.

[mshabarov]

@TatuLund I'm going to add a test for this fix to move this important change forward, so if you currently are working on it also, please let me know :)

[TatuLund]

@mshabarov Please do, I have not figured out good test yet.

[vaadin-bot]

SonarQube analysis reported 2 issues

1. DataCommunicator.java#L1075: Refactor this method to reduce its Cognitive Complexity from 16 to the 15 allowed.
2. DataCommunicator.java#L483: Complete the task associated to this TODO comment.

[vaadin-bot]

Hi @TatuLund and @denis-anisimov, when i performed cherry-pick to this commit to 2.7, i have encountered the following issue. Can you take a look and pick it manually?
Error Message:
Error: Command failed: git cherry-pick 067f63f
error: could not apply 067f63f... fix: register data provider listener only if it has not been registered (#11471)
hint: after resolving the conflicts, mark the corrected paths
hint: with 'git add ' or 'git rm '
hint: and commit the result with 'git commit'

[vaadin-bot]

Hi @TatuLund and @denis-anisimov, when i performed cherry-pick to this commit to 2.6, i have encountered the following issue. Can you take a look and pick it manually?
Error Message:
Error: Command failed: git cherry-pick 067f63f
error: could not apply 067f63f... fix: register data provider listener only if it has not been registered (#11471)
hint: after resolving the conflicts, mark the corrected paths
hint: with 'git add ' or 'git rm '
hint: and commit the result with 'git commit'

[vaadin-bot]

This ticket/PR has been released with platform 22.0.0.alpha2 and is also targeting the upcoming stable 22.0.0 version.