HierarchicalDataProvider.refreshItem(null) throws NullPointerException, see video

[enver-haase]

Description of the bug

The 'parent' of a so-called "root node" is NULL. Hence it is very surprising that I cannot refresh the NULL root node. (Or shall I call it 'super root' now that the tree root's child nodes are called the 'root nodes'?)

Expected behavior

I expect refreshItem(null) to be equivalent to refreshAll().

Minimal reproducible example

package org.vaadin.example;

import com.vaadin.flow.component.grid.Grid;
import com.vaadin.flow.component.grid.dnd.GridDropLocation;
import com.vaadin.flow.component.grid.dnd.GridDropMode;
import com.vaadin.flow.component.notification.Notification;
import com.vaadin.flow.component.treegrid.TreeGrid;

import com.vaadin.flow.component.orderedlayout.VerticalLayout;
import com.vaadin.flow.data.provider.hierarchy.TreeData;
import com.vaadin.flow.router.Route;

import java.util.ArrayList;
import java.util.List;

@Route
public class MainView extends VerticalLayout {

    private static class DemoTreeData extends TreeData<String> {
        private DemoTreeData() {
            final String[] five = {"A", "B", "C", "D", "E"};

            addRootItems(five);

            for (String s : five) {
                List<String> children = new ArrayList<>();
                for (String ch : five) {
                    String child = s+ch;
                    children.add(child);
                }

                addItems(s, children);
            }
        }
    }
    public MainView() {

        TreeGrid<String> treeGrid = new TreeGrid<>();

        treeGrid.addHierarchyColumn(String::toString).setHeader("Name");

        treeGrid.setTreeData(new DemoTreeData());

        treeGrid.setDropMode(GridDropMode.ON_TOP_OR_BETWEEN);
        treeGrid.setSelectionMode(Grid.SelectionMode.SINGLE);
        treeGrid.setRowsDraggable(true);

        List<String> dragged = new ArrayList<>();
        treeGrid.addDragStartListener( evt -> {
            dragged.clear();
            dragged.addAll(evt.getDraggedItems());
        } );

        treeGrid.addDropListener( evt -> {
            try {
                if (GridDropLocation.ON_TOP == evt.getDropLocation()) {
                    evt.getDropTargetItem().ifPresent(s -> {
                        for (String drag : dragged) {
                            String oldParent = treeGrid.getTreeData().getParent(s);
                            treeGrid.getTreeData().setParent(drag, s);

                            //if (oldParent != null) {
                                // This throws a NullPointerException for the NULL parent.
                                treeGrid.getDataProvider().refreshItem(oldParent, true);
                                treeGrid.getDataProvider().refreshItem(s, true);
                            //}
                            //else {
                            //    treeGrid.getDataProvider().refreshAll();
                            //}
                        }
                    });
                }
            }
            catch (Exception e) {
                Notification.show("Exception: "+e);
            }
        } );

        treeGrid.setWidthFull();
        treeGrid.setHeight("800px");
        add(treeGrid);
    }

}

Versions

• Vaadin / Flow version: 24.3.10
• Java version: n/a
• OS version: n/a
• Browser version (if applicable): n/a
• Application Server (if applicable): n/a
• IDE (if applicable): n/a

[enver-haase]

https://github.com/vaadin/flow/assets/7870436/d84b0e41-6657-40cf-aba8-e4920734bd0d

[mshabarov]

To me the connection between null parent item in a TreeGrid and expectation that the refreshItem(null) has the same effect as refreshAll() is questionable.

If we apply this logic, this could be a problem potentially, because Flow would refresh all the items in component once you have unexpected null value for an item that is passed into this method.

It's probably better to have a reserved constant or object for the root element, or an additional method in TreeGrid.

By the way, why don't you just call refreshAll() ?

[enver-haase]

Not using refreshAll() to save resources, and also to point out crazy behaviours of TreeGrid and friends, because I need it fixed for a project. refreshAll() also exposes https://github.com/vaadin/flow-components/issues/1236 .

[mshabarov]

We can consider having refreshItem(null), but if the expectation of refresh logic for it would be different, e.g. having a refresh for small group of item, like for those that are on the level one of the hierarchy. If the result is supposed to be the same as refreshAll, then I don't see any point of it.

Regarding the issues that you've mentioned, we need to fix them and probably close this ticket.

[enver-haase]

refreshItem(null) would of course refresh the "root items". Whether or not it would recursively also refresh their children, grandchildren and so on, depends on the current implementation of 'refreshItem(T item)' without the second argument as in 'refreshItem(T item, boolean refreshChildren)'.

The unlucky default Interface implementation of 'refreshItem(T, boolean)' in DataProvider<T, F> does not even honour the boolean, but at least the documentation mandates overriding for hierarchical data providers.

The counter conclusion is that the one-parameter 'refreshItem(null)' would indeed only refresh the root items.

However, 'refreshItems(null, true)' should also not throw NPE.

The reason why NULL should be allowed is that traversal of the tree such as refreshItem(getParent(item), true); should not need special cases. refreshAll() could and probably should then be implemented simply as refreshItem(null, true).

Note also that at this time refreshAll() scrolls a TreeGrid to the top [which is usually unwanted and undocumented] (https://github.com/vaadin/flow-components/issues/1236) .

[mshabarov]

I believe we have to anyway document or to change implementation, or even API to make it clear what Flow does in case someone calls refreshItem(null).