Reset HierarchicalDataCommunicator on changes (#9275)

Reset HDC when encountering unexpected changes in the data.

Additionally this patch fixes an issue with client and server caches
getting out of sync during resets.

client/src/main/java/com/vaadin/client/data/AbstractRemoteDataSource.java

@@ -904,7 +904,8 @@ public int size() {
      */
     protected void resetDataAndSize(int newSize) {
         size = newSize;
-        dropFromCache(getCachedRange());
+        indexToRowMap.clear();
+        keyToIndexMap.clear();
         cached = Range.withLength(0, 0);
 
         getHandlers().forEach(dch -> dch.resetDataAndSize(newSize));

server/src/main/java/com/vaadin/data/HierarchyData.java

@@ -27,7 +27,7 @@
 
 /**
  * Class for representing hierarchical data.
- * 
+ *
  * @author Vaadin Ltd
  * @since 8.1
  *
@@ -95,13 +95,13 @@ public HierarchyData() {
      * Adds a data item as a child of {@code parent}. Call with {@code null} as
      * parent to add a root level item. The given parent item must already exist
      * in this structure, and an item can only be added to this structure once.
-     * 
+     *
      * @param parent
      *            the parent item for which the items are added as children
      * @param item
      *            the item to add
      * @return this
-     * 
+     *
      * @throws IllegalArgumentException
      *             if parent is not null and not already added to this structure
      * @throws IllegalArgumentException
@@ -129,13 +129,13 @@ public HierarchyData<T> addItem(T parent, T item) {
      * {@code null} as parent to add root level items. The given parent item
      * must already exist in this structure, and an item can only be added to
      * this structure once.
-     * 
+     *
      * @param parent
      *            the parent item for which the items are added as children
      * @param items
      *            the list of items to add
      * @return this
-     * 
+     *
      * @throws IllegalArgumentException
      *             if parent is not null and not already added to this structure
      * @throws IllegalArgumentException
@@ -155,13 +155,13 @@ public HierarchyData<T> addItems(T parent,
      * {@code null} as parent to add root level items. The given parent item
      * must already exist in this structure, and an item can only be added to
      * this structure once.
-     * 
+     *
      * @param parent
      *            the parent item for which the items are added as children
      * @param items
      *            the collection of items to add
      * @return this
-     * 
+     *
      * @throws IllegalArgumentException
      *             if parent is not null and not already added to this structure
      * @throws IllegalArgumentException
@@ -180,13 +180,13 @@ public HierarchyData<T> addItems(T parent, Collection<T> items) {
      * with {@code null} as parent to add root level items. The given parent
      * item must already exist in this structure, and an item can only be added
      * to this structure once.
-     * 
+     *
      * @param parent
      *            the parent item for which the items are added as children
      * @param items
      *            stream of items to add
      * @return this
-     * 
+     *
      * @throws IllegalArgumentException
      *             if parent is not null and not already added to this structure
      * @throws IllegalArgumentException
@@ -203,11 +203,11 @@ public HierarchyData<T> addItems(T parent, Stream<T> items) {
     /**
      * Remove a given item from this structure. Additionally, this will
      * recursively remove any descendants of the item.
-     * 
+     *
      * @param item
      *            the item to remove, or null to clear all data
      * @return this
-     * 
+     *
      * @throws IllegalArgumentException
      *             if the item does not exist in this structure
      */
@@ -219,25 +219,32 @@ public HierarchyData<T> removeItem(T item) {
         new ArrayList<>(getChildren(item)).forEach(child -> removeItem(child));
         itemToWrapperMap.get(itemToWrapperMap.get(item).getParent())
                 .removeChild(item);
+        if (item != null) {
+            // remove non root item from backing map
+            itemToWrapperMap.remove(item);
+        }
         return this;
     }
 
     /**
      * Clear all items from this structure. Shorthand for calling
      * {@link #removeItem(Object)} with null.
+     *
+     * @return this
      */
-    public void clear() {
+    public HierarchyData<T> clear() {
         removeItem(null);
+        return this;
     }
 
     /**
      * Get the immediate child items for the given item.
-     * 
+     *
      * @param item
      *            the item for which to retrieve child items for, null to
      *            retrieve all root items
      * @return a list of child items for the given item
-     * 
+     *
      * @throws IllegalArgumentException
      *             if the item does not exist in this structure
      */
@@ -249,7 +256,15 @@ public List<T> getChildren(T item) {
         return itemToWrapperMap.get(item).getChildren();
     }
 
-    private boolean contains(T item) {
+    /**
+     * Check whether the given item is in this hierarchy.
+     *
+     * @param item
+     *            the item to check
+     * @return {@code true} if the item is in this hierarchy, {@code false} if
+     *         not
+     */
+    public boolean contains(T item) {
         return itemToWrapperMap.containsKey(item);
     }
 

server/src/main/java/com/vaadin/data/provider/HierarchicalDataCommunicator.java

@@ -22,7 +22,6 @@
 import java.util.Objects;
 import java.util.Optional;
 import java.util.Set;
-import java.util.concurrent.atomic.AtomicInteger;
 import java.util.function.BiConsumer;
 import java.util.logging.Logger;
 import java.util.stream.Collectors;
@@ -153,13 +152,27 @@ private void loadInitialData() {
     private void loadRequestedRows() {
         final Range requestedRows = getPushRows();
         if (!requestedRows.isEmpty()) {
-            doPushRows(requestedRows);
+            doPushRows(requestedRows, 0);
         }
 
         setPushRows(Range.withLength(0, 0));
     }
 
-    private void doPushRows(final Range requestedRows) {
+    /**
+     * Attempts to push the requested range of rows to the client. Will trigger
+     * a reset if the data provider is unable to provide the requested number of
+     * items.
+     *
+     * @param requestedRows
+     *            the range of rows to push
+     * @param insertRowsCount
+     *            number of rows to insert, beginning at the start index of
+     *            {@code requestedRows}, 0 to not insert any rows
+     * @return {@code true} if the range was successfully pushed to the client,
+     *         {@code false} if the push was unsuccessful and a reset was
+     *         triggered
+     */
+    private boolean doPushRows(final Range requestedRows, int insertRowsCount) {
         Stream<TreeLevelQuery> levelQueries = mapper.splitRangeToLevelQueries(
                 requestedRows.getStart(), requestedRows.getEnd() - 1);
 
@@ -173,41 +186,39 @@ private void doPushRows(final Range requestedRows) {
             List<T> results = doFetchQuery(query.startIndex, query.size,
                     getKeyMapper().get(query.node.getParentKey()))
                             .collect(Collectors.toList());
-            // TODO if the size differers from expected, all goes to hell
+
             fetchedItems.addAll(results);
             List<JsonObject> rowData = results.stream()
                     .map(item -> createDataObject(item, query.depth))
                     .collect(Collectors.toList());
             mapper.reorderLevelQueryResultsToFlatOrdering(rowDataMapper, query,
                     rowData);
         });
-        verifyNoNullItems(dataObjects, requestedRows);
+
+        if (hasNullItems(dataObjects, requestedRows)) {
+            reset();
+            return false;
+        }
+
+        if (insertRowsCount > 0) {
+            getClientRpc().insertRows(requestedRows.getStart(),
+                    insertRowsCount);
+        }
 
         sendData(requestedRows.getStart(), Arrays.asList(dataObjects));
         getActiveDataHandler().addActiveData(fetchedItems.stream());
         getActiveDataHandler().cleanUp(fetchedItems.stream());
+        return true;
     }
 
-    /*
-     * Verify that there are no null objects in the array, to fail eagerly and
-     * not just on the client side.
-     */
-    private void verifyNoNullItems(JsonObject[] dataObjects,
+    private boolean hasNullItems(JsonObject[] dataObjects,
             Range requestedRange) {
-        List<Integer> nullItems = new ArrayList<>(0);
-        AtomicInteger indexCounter = new AtomicInteger();
-        Stream.of(dataObjects).forEach(object -> {
-            int index = indexCounter.getAndIncrement();
+        for (JsonObject object : dataObjects) {
             if (object == null) {
-                nullItems.add(index);
+                return true;
             }
-        });
-        if (!nullItems.isEmpty()) {
-            throw new IllegalStateException("For requested rows "
-                    + requestedRange + ", there was null items for indexes "
-                    + nullItems.stream().map(Object::toString)
-                            .collect(Collectors.joining(", ")));
         }
+        return false;
     }
 
     private JsonObject createDataObject(T item, int depth) {
@@ -288,9 +299,7 @@ protected void onDropRows(JsonArray keys) {
             String itemKey = keys.getString(i);
             if (!mapper.isKeyStored(itemKey)
                     && !rowKeysPendingExpand.contains(itemKey)) {
-                // FIXME: cache invalidated incorrectly, active keys being
-                // dropped prematurely
-                // getActiveDataHandler().dropActiveData(itemKey);
+                getActiveDataHandler().dropActiveData(itemKey);
             }
         }
     }
@@ -379,6 +388,7 @@ public boolean doCollapse(String collapsedRowKey, int collapsedRowIndex) {
                 collapsedRowIndex);
         getClientRpc().removeRows(collapsedRowIndex + 1,
                 collapsedSubTreeSize);
+
         // FIXME seems like a slight overkill to do this just for refreshing
         // expanded status
         refresh(collapsedItem);
@@ -414,9 +424,8 @@ public boolean doExpand(String expandedRowKey, final int expandedRowIndex,
 
         int expandedNodeSize = doSizeQuery(expandedItem);
         if (expandedNodeSize == 0) {
-            // TODO handle 0 size -> not expandable
-            throw new IllegalStateException("Row with index " + expandedRowIndex
-                    + " returned no child nodes.");
+            reset();
+            return false;
         }
 
         if (!mapper.isCollapsed(expandedRowKey)) {
@@ -426,17 +435,16 @@ public boolean doExpand(String expandedRowKey, final int expandedRowIndex,
                 expandedNodeSize);
         rowKeysPendingExpand.remove(expandedRowKey);
 
-        getClientRpc().insertRows(expandedRowIndex + 1, expandedNodeSize);
-        // TODO optimize by sending "just enough" of the expanded items
-        // directly
-        doPushRows(
-                Range.withLength(expandedRowIndex + 1, expandedNodeSize));
+        boolean success = doPushRows(Range.withLength(expandedRowIndex + 1,
+                Math.min(expandedNodeSize, latestCacheSize)), expandedNodeSize);
 
-        // expanded node needs to be updated to be marked as expanded
-        // FIXME seems like a slight overkill to do this just for refreshing
-        // expanded status
-        refresh(expandedItem);
-        return true;
+        if (success) {
+            // FIXME seems like a slight overkill to do this just for refreshing
+            // expanded status
+            refresh(expandedItem);
+            return true;
+        }
+        return false;
     }
 
     /**

server/src/main/java/com/vaadin/data/provider/InMemoryHierarchicalDataProvider.java

@@ -53,7 +53,7 @@ public class InMemoryHierarchicalDataProvider<T> extends
      * <p>
      * All changes made to the given HierarchyData object will also be visible
      * through this data provider.
-     * 
+     *
      * @param hierarchyData
      *            the backing HierarchyData for this provider
      */
@@ -63,7 +63,7 @@ public InMemoryHierarchicalDataProvider(HierarchyData<T> hierarchyData) {
 
     /**
      * Return the underlying hierarchical data of this provider.
-     * 
+     *
      * @return the underlying data of this provider
      */
     public HierarchyData<T> getData() {
@@ -77,6 +77,12 @@ public boolean isInMemory() {
 
     @Override
     public boolean hasChildren(T item) {
+        if (!hierarchyData.contains(item)) {
+            throw new IllegalArgumentException("Item " + item
+                    + " could not be found in the backing HierarchyData. "
+                    + "Did you forget to refresh this data provider after item removal?");
+        }
+
         return !hierarchyData.getChildren(item).isEmpty();
     }
 
@@ -89,6 +95,13 @@ public int getChildCount(
     @Override
     public Stream<T> fetchChildren(
             HierarchicalQuery<T, SerializablePredicate<T>> query) {
+        if (!hierarchyData.contains(query.getParent())) {
+            throw new IllegalArgumentException("The queried item "
+                    + query.getParent()
+                    + " could not be found in the backing HierarchyData. "
+                    + "Did you forget to refresh this data provider after item removal?");
+        }
+
         Stream<T> childStream = getFilteredStream(
                 hierarchyData.getChildren(query.getParent()).stream(),
                 query.getFilter());

server/src/test/java/com/vaadin/data/provider/InMemoryHierarchicalDataProviderTest.java

@@ -72,6 +72,13 @@ public void hierarchyData_clear() {
         Assert.assertTrue(data.getChildren(null).isEmpty());
     }
 
+    @Test
+    public void hierarchyData_re_add_removed_item() {
+        StrBean item = rootData.get(0);
+        data.removeItem(item).addItem(null, item);
+        Assert.assertTrue(data.getChildren(null).contains(item));
+    }
+
     @Test
     public void setFilter() {
         getDataProvider().setFilter(item -> item.getValue().equals("Xyz")

testbench-api/src/main/java/com/vaadin/testbench/elements/TreeGridElement.java

@@ -118,6 +118,24 @@ public boolean isRowCollapsed(int rowIndex, int hierarchyColumnIndex) {
         return !isRowExpanded(rowIndex, hierarchyColumnIndex);
     }
 
+    /**
+     * Check whether the given indices correspond to a cell that contains a
+     * visible hierarchy toggle element.
+     *
+     * @param rowIndex
+     *            0-based row index
+     * @param hierarchyColumnIndex
+     *            0-based index of the hierarchy column
+     * @return {@code true} if this cell has the expand toggle visible
+     */
+    public boolean hasExpandToggle(int rowIndex, int hierarchyColumnIndex) {
+        WebElement expandElement = getExpandElement(rowIndex,
+                hierarchyColumnIndex);
+        List<String> classes = Arrays
+                .asList(expandElement.getAttribute("class").split(" "));
+        return classes.contains("expanded") || classes.contains("collapsed");
+    }
+
     /**
      * Gets the expand/collapse element for the given row.
      *