fix: treat HierarchicalDataProvider.refreshItem(null) as virtual root by arimu1 · Pull Request #25100 · vaadin/flow

arimu1 · 2026-07-31T16:10:31Z

Description

HierarchicalDataProvider.refreshItem(null) threw a NullPointerException because DataRefreshEvent required a non-null item.

In hierarchical data models such as TreeData, null is the virtual root (the parent of root-level items). Call sites that do refreshItem(getParent(item), true) therefore hit NPE when the item is a root.

Semantics (simplified after review)

Per @vursen and the original expectation on #19377: refreshItem(null) is equivalent to refreshAll() / communicator reset(), regardless of refreshChildren. Differentiating on that flag for the virtual root was inconsistent with non-null item semantics; the same-reference in-place path was a no-op and was removed.

DataRefreshEvent still allows null so the hierarchical virtual-root path does not throw.

Changes

DataChangeEvent.DataRefreshEvent: allow null item (virtual root)
AbstractHierarchicalDataProvider: refreshItem(null[, *]) → refreshAll()
HierarchicalDataCommunicator.refresh(null, *) → reset()
Unit tests for null virtual-root full hierarchy refresh

Fixes #19377

Local verification

mvn -pl flow-data -Dtest=HierarchicalDataCommunicatorDataRefreshTest test — 19/19 (JDK 21)
mvn -pl flow-data spotless:check — clean

cla-assistant · 2026-07-31T16:10:48Z

All committers have signed the CLA.

arimu1 · 2026-08-01T01:36:35Z

Re-triggered CI after CLA was signed.

Local verification (JBR 17):

mvn -pl flow-data spotless:check — clean
mvn -pl flow-data -Dtest=HierarchicalDataCommunicatorDataRefreshTest test — green

Null item is treated as the virtual hierarchy root → refreshAll() / reset().

arimu1 · 2026-08-02T10:44:45Z

Fixed TeamCity Formatter failures: Spotless javadoc line wraps in DataChangeEvent and HierarchicalDataCommunicator (local mvn -pl flow-data spotless:check clean; HierarchicalDataCommunicatorDataRefreshTest green on JDK 21).

arimu1 · 2026-08-03T23:54:33Z

@vursen Thanks for the review — simplified per your suggestions.

A. refreshChildren on null / virtual root
Agreed the dual path was overfitted. null is the virtual hierarchy root (call sites use refreshItem(getParent(item), true) for root-level items → null). Differentiating refreshChildren for that case was inconsistent with non-null item semantics (false still touched root children).

Simplified: refreshItem(null) / refreshItem(null, *) always maps to refreshAll() (data provider) / reset() (hierarchical communicator), ignoring refreshChildren. That also matches the original expectation on #19377.

B. Same-reference in-place refresh
Agreed — refreshing cached root items with the same references was effectively a no-op for the intended use. Removed that path entirely with the full-reset simplification.

C. Root viewport resend / refreshViewport
Exposing refreshViewport as public API is a larger surface change and out of scope for the NPE fix. Happy to follow up if maintainers want a separate API for “re-render current viewport without hierarchy rebuild.”

NPE fix retained: DataRefreshEvent still allows null so hierarchical virtual-root refresh does not throw.

Local: HierarchicalDataCommunicatorDataRefreshTest 19/19 green; spotless:check clean (JDK 21).

arimu1 · 2026-08-04T00:18:58Z

Follow-up after TeamCity red + local investigation:

Local HierarchicalDataCommunicatorDataRefreshTest was failing on refreshNullItem_fullHierarchyRefresh (expected size 3, actual 4). Root cause was a wrong test assumption that reset() clears expand state — it does not (only setDataProvider clears expandedItemIds). Fixed the expectation to match real reset() / refreshAll behavior.

Locally now:

mvn -pl flow-data spotless:check clean
HierarchicalDataCommunicatorDataRefreshTest 19/19 green (JDK 21)

Also rebased onto current main for a cleaner TeamCity run.

null is the virtual root (parent of root-level items) in TreeData-style hierarchies. Calling refreshItem(null) previously threw NPE in DataRefreshEvent. Allow null in DataRefreshEvent for hierarchical virtual-root semantics. refreshItem(null) refreshes root-level items only; refreshItem(null, true) re-fetches root children via HierarchicalDataCommunicator without a full reset()/refreshAll() (acknowledging that equating null to refreshAll is too broad). Fixes vaadin#19377

TeamCity Formatter failed on javadoc line wrapping in DataChangeEvent and HierarchicalDataCommunicator. spotless:apply only; behavior unchanged.

@vursen

Per maintainer review (@vursen): differentiating refreshChildren for the virtual root was inconsistent with non-null item semantics and the same-reference in-place path was effectively a no-op. Null is the virtual hierarchy root (call sites use refreshItem(getParent(item), true) for roots → null). Always map it to refreshAll()/reset() regardless of refreshChildren. Keep DataRefreshEvent allowing null so the NPE from vaadin#19377 stays fixed.

reset() preserves expand state; after refresh(null) with Item 0 expanded and a new root, flattened size is 4 not 3. Aligns with refreshAll behavior.

arimu1 · 2026-08-04T14:28:04Z

Rebased onto latest main and force-pushed to re-trigger TeamCity (Formatter / Gradle Tests / API-DIFF were still red after the null→refreshAll simplify). Local change set is still the minimal refreshItem(null) → refreshAll/reset path + fixed size expectations.

arimu1 · 2026-08-04T23:12:23Z

Hi @vursen — following up on the TeamCity failures. After the last rebase + force-push, all five TeamCity jobs (Formatter / Gradle Tests / API-DIFF labeling / Validation / Validation-SlowTests) are still red simultaneously.

I tried to read the TeamCity logs but they require authentication (no guest access succeeded). Locally I verified the minimal change set against upstream/main:

$ mvn -q -pl flow-data spotless:check -Dspotless.ratchetFrom= -DskipTests -o
$ echo $?
0

The spotless plugin is configured to enforce eclipse/VaadinJavaConventions.xml, so the Flow_FlowRunFormatter job should pass on this tip. The fact that all five jobs fail together (rather than, say, only a test job) suggests a checkout / fork-PR setup issue on the TeamCity side rather than a code or formatting problem.

Could you share the TeamCity log (or confirm whether this is a known infra issue for fork-based PRs)? Happy to adjust if there is a real finding — I just cannot see past the auth wall.

vursen · 2026-08-17T08:15:23Z

+        if (item == null) {
+            // Virtual root: always full hierarchy refresh. refreshChildren is
+            // intentionally ignored (same as HierarchicalDataCommunicator).
+            refreshAll();
+            return;
+        }


Is this required? This is already handled by HierarchicalDataCommunicator#refresh.

vursen · 2026-08-17T08:16:03Z

+        if (item == null) {
+            refreshAll();
+            return;
+        }


Is this required? This is already handled by HierarchicalDataCommunicator#refresh.

vursen · 2026-08-17T08:27:19Z

After giving it a second thought, I'm not sure the current change fully solves the problem. It removes one confusion, but adds another one. Previously, it wasn't possible to use refreshItem(null). Now it does refreshAll(), which updates children, and that's inconsistent with refreshItem(item). The effect of refreshAll is much broader, and it comes with quirks depending on the hierarchy format, which are easy to miss. It also diverges from the parent DataCommunicator, where refreshItem doesn't accept null and never falls back to refreshAll.

It now seems to me that, if anything, this change makes sense only for refreshItem(null, true):

It's the API you would likely use for recursive updates (it was used in the original example in the issue), unlike refreshItem(item), which doesn't update children and therefore doesn't really benefit from accepting null.
It's the API that works only with the nested format, which is the format where recursive updates might actually be needed. In the flattened format, you are always supposed to call refreshAll().
It's the API that clears children from the cache, so its effect is the most similar to refreshAll.

tltv · 2026-08-17T09:43:54Z

It now seems to me that, if anything, this change makes sense only for refreshItem(null, true):

I agree. For consistency reason I approved the current implementation already, but limiting it only for refreshItem(null, true) narrows the impact of this fix. I would rather not introduce new methods in DataProvider either, so fixing it just for refreshItem(null, true) makes sense.

Co-authored-by: Sergey Vinogradov <mr.vursen@gmail.com>

tltv · 2026-08-18T12:18:59Z

@claude Change this to only affect refreshItem(null, true) call as suggested in comments.

claude · 2026-08-18T12:19:22Z

Claude finished @tltv's task in 8m 10s —— View job

Done — narrowed to `refreshItem(null, true)`

As requested by @tltv (and per @vursen's review), the virtual-root behavior is now limited so that only refreshItem(null, true) maps to refreshAll() / reset().

AbstractHierarchicalDataProvider: removed the null → refreshAll() special-casing in both refreshItem overrides (it was redundant with the communicator). The single-arg override is gone; the two-arg override only guards the NESTED-format check for non-null items, so refreshItem(null, true) flows through to the communicator
HierarchicalDataCommunicator.refresh: refresh(null, true) → reset(); refresh(null, false) → no-op (the virtual root itself is not a rendered item)
Updated JavaDoc to the narrowed semantics
Updated unit tests (refreshNullItem_onlyRefreshChildrenCallsReset, refreshItemNullThroughDataProvider_onlyRefreshChildrenCallsReset, and the full-hierarchy tests now drive null, true)
HierarchicalDataCommunicatorDataRefreshTest — 19/19 green
spotless:check — clean
Committed & pushed

Behavior after this change

Call	Result
`refreshItem(null, true)`	full refresh — `refreshAll()` / `reset()`
`refreshItem(null, false)`	no-op
`refreshItem(null)`	no-op
`refreshItem(item, …)`	unchanged

DataRefreshEvent still permits a null item so the virtual-root event can reach the communicator without an NPE; the non-hierarchical DataCommunicator.refresh(T) continues to reject null via its existing requireNonNull, so nothing changes for flat data providers.

Note

Please review the changes yourself and mark the PR ready when you're happy with it.
• fix/19377-refreshitem-null

arimu1 force-pushed the fix/19377-refreshitem-null branch from 182a45e to 36738ba Compare July 31, 2026 16:39

arimu1 changed the title ~~fix: treat HierarchicalDataProvider.refreshItem(null) as refreshAll~~ fix: treat HierarchicalDataProvider.refreshItem(null) as virtual root Jul 31, 2026

mcollovati added the Contribution PRs coming from the community or external to the team label Aug 3, 2026

vursen reviewed Aug 3, 2026

View reviewed changes

Comment thread ...data/src/main/java/com/vaadin/flow/data/provider/hierarchy/HierarchicalDataCommunicator.java Outdated

vursen reviewed Aug 3, 2026

View reviewed changes

Comment thread ...data/src/main/java/com/vaadin/flow/data/provider/hierarchy/HierarchicalDataCommunicator.java Outdated

vursen reviewed Aug 3, 2026

View reviewed changes

Comment thread ...data/src/main/java/com/vaadin/flow/data/provider/hierarchy/HierarchicalDataCommunicator.java Outdated

arimu1 force-pushed the fix/19377-refreshitem-null branch from f05157e to 510ad78 Compare August 4, 2026 00:19

arimu1 added 6 commits August 4, 2026 21:13

ci: re-trigger TeamCity after CLA signed

36e9192

ci: re-trigger TeamCity validation

a1af09c

style: apply spotless to refreshItem(null) javadoc wraps

cede1b3

TeamCity Formatter failed on javadoc line wrapping in DataChangeEvent and HierarchicalDataCommunicator. spotless:apply only; behavior unchanged.

test: fix null-root refresh size expectations after reset

1816aec

reset() preserves expand state; after refresh(null) with Item 0 expanded and a new root, flattened size is 4 not 3. Aligns with refreshAll behavior.

arimu1 force-pushed the fix/19377-refreshitem-null branch from 510ad78 to 1816aec Compare August 4, 2026 14:27

mshabarov added this to Vaadin Flow | Hilla | Kits ongoing work Aug 12, 2026

mshabarov moved this to 🔎Iteration reviews in Vaadin Flow | Hilla | Kits ongoing work Aug 12, 2026

Merge branch 'main' into fix/19377-refreshitem-null

4b96fe4

tltv previously approved these changes Aug 14, 2026

View reviewed changes

tltv added this pull request to the merge queue Aug 14, 2026

tltv added target/25.1 target/25.2 labels Aug 14, 2026

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Aug 14, 2026

tltv added this pull request to the merge queue Aug 14, 2026

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Aug 14, 2026

Merge branch 'main' into fix/19377-refreshitem-null

1ae6d03

vursen reviewed Aug 17, 2026

View reviewed changes

Comment thread ...data/src/main/java/com/vaadin/flow/data/provider/hierarchy/HierarchicalDataCommunicator.java Outdated

vursen reviewed Aug 17, 2026

View reviewed changes

Comment thread .../src/main/java/com/vaadin/flow/data/provider/hierarchy/AbstractHierarchicalDataProvider.java Outdated

vursen reviewed Aug 17, 2026

View reviewed changes

Comment thread .../src/main/java/com/vaadin/flow/data/provider/hierarchy/AbstractHierarchicalDataProvider.java Outdated

vursen reviewed Aug 17, 2026

View reviewed changes

Apply suggestion from @vursen

0256259

Co-authored-by: Sergey Vinogradov <mr.vursen@gmail.com>

tltv dismissed their stale review via 0256259 August 17, 2026 12:17

Apply suggestions from code review

c54fb83

Co-authored-by: Sergey Vinogradov <mr.vursen@gmail.com>

Uh oh!

Conversation

arimu1 commented Jul 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Semantics (simplified after review)

Changes

Local verification

Uh oh!

cla-assistant Bot commented Jul 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arimu1 commented Aug 1, 2026

Uh oh!

arimu1 commented Aug 2, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

arimu1 commented Aug 3, 2026

Uh oh!

arimu1 commented Aug 4, 2026

Uh oh!

arimu1 commented Aug 4, 2026

Uh oh!

arimu1 commented Aug 4, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vursen Aug 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vursen Aug 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vursen commented Aug 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tltv commented Aug 17, 2026

Uh oh!

tltv commented Aug 18, 2026

Uh oh!

claude Bot commented Aug 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Done — narrowed to refreshItem(null, true)

Behavior after this change

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

arimu1 commented Jul 31, 2026 •

edited

Loading

cla-assistant Bot commented Jul 31, 2026 •

edited

Loading

vursen Aug 17, 2026 •

edited

Loading

vursen Aug 17, 2026 •

edited

Loading

vursen commented Aug 17, 2026 •

edited

Loading

claude Bot commented Aug 18, 2026 •

edited

Loading

Done — narrowed to `refreshItem(null, true)`