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).

vursen · 2026-08-03T16:01:29Z

+            rootCache.clear();
+            rootCache.setSize(getDataProviderChildCount(null));
+            requestFlush().invalidateViewport();


With refreshChildren = true, the behavior seems to be equivalent to calling reset().

With refreshChildren = false, something feels a bit off to me: we are explicitly asking not to refresh the children, yet the virtual root's children get refreshed, while refreshItem(item, false) with a non-null item leaves them untouched. If the behavior doesn't follow the flag either way, maybe differentiating on it just adds confusion? Always calling refreshAll might be simpler and more consistent. @arimu1 do you have a use case that would require these two to behave differently?

vursen · 2026-08-03T16:29:42Z

+            T rootItem = rootCache.getItem(i);
+            keyMapper.refresh(rootItem);
+            dataGenerator.refreshData(rootItem);


Does this actually accomplish anything? Refreshing an item with the same cached reference looks like a no-op to me.

vursen · 2026-08-03T16:50:19Z

+            T rootItem = rootCache.getItem(i);
+            keyMapper.refresh(rootItem);
+            dataGenerator.refreshData(rootItem);
+            requestFlush().invalidateItem(rootItem);


So this effectively just resends to the client the generated data of the root items within the viewport. What scenario needs that? Maybe exposing refreshViewport would be a better fit?

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.

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

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

Navigation Menu

Uh oh!

fix: treat HierarchicalDataProvider.refreshItem(null) as virtual root - #25100

fix: treat HierarchicalDataProvider.refreshItem(null) as virtual root#25100
arimu1 wants to merge 6 commits into
vaadin:mainfrom
arimu1:fix/19377-refreshitem-null

arimu1 commented Jul 31, 2026 •

edited

Loading

Uh oh!

cla-assistant Bot commented Jul 31, 2026 •

edited

Loading

Uh oh!

arimu1 commented Aug 1, 2026

Uh oh!

arimu1 commented Aug 2, 2026

Uh oh!

vursen Aug 3, 2026 •

edited

Loading

Uh oh!

vursen Aug 3, 2026 •

edited

Loading

Uh oh!

vursen Aug 3, 2026

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

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!

vursen Aug 3, 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 3, 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 3, 2026

Choose a reason for hiding this comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

arimu1 commented Jul 31, 2026 •

edited

Loading

cla-assistant Bot commented Jul 31, 2026 •

edited

Loading

vursen Aug 3, 2026 •

edited

Loading

vursen Aug 3, 2026 •

edited

Loading