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.

3 files changed, 90 insertions, 54 deletions

diff --git a/server/src/main/java/com/vaadin/data/HierarchyData.java b/server/src/main/java/com/vaadin/data/HierarchyData.java
index df5fbbbbf5..0cf680db2f 100644
--- a/server/src/main/java/com/vaadin/data/HierarchyData.java
+++ b/server/src/main/java/com/vaadin/data/HierarchyData.java
@@ -27,7 +27,7 @@ import java.util.stream.Stream;
 
 /**
  * Class for representing hierarchical data.
- * 
+ *
  * @author Vaadin Ltd
  * @since 8.1
  *
@@ -95,13 +95,13 @@ public class HierarchyData<T> implements Serializable {
      * 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 class HierarchyData<T> implements Serializable {
      * {@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 class HierarchyData<T> implements Serializable {
      * {@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 class HierarchyData<T> implements Serializable {
      * 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 class HierarchyData<T> implements Serializable {
     /**
      * 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 class HierarchyData<T> implements Serializable {
         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 class HierarchyData<T> implements Serializable {
         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);
     }
 
diff --git a/server/src/main/java/com/vaadin/data/provider/HierarchicalDataCommunicator.java b/server/src/main/java/com/vaadin/data/provider/HierarchicalDataCommunicator.java
index 820e3d8f2c..5d3bd81a05 100644
--- a/server/src/main/java/com/vaadin/data/provider/HierarchicalDataCommunicator.java
+++ b/server/src/main/java/com/vaadin/data/provider/HierarchicalDataCommunicator.java
@@ -22,7 +22,6 @@ import java.util.List;
 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 @@ public class HierarchicalDataCommunicator<T> extends DataCommunicator<T> {
     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,7 +186,7 @@ public class HierarchicalDataCommunicator<T> extends DataCommunicator<T> {
             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))
@@ -181,33 +194,31 @@ public class HierarchicalDataCommunicator<T> extends DataCommunicator<T> {
             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 @@ public class HierarchicalDataCommunicator<T> extends DataCommunicator<T> {
             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 class HierarchicalDataCommunicator<T> extends DataCommunicator<T> {
                 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 class HierarchicalDataCommunicator<T> extends DataCommunicator<T> {
 
         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 class HierarchicalDataCommunicator<T> extends DataCommunicator<T> {
                 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;
     }
 
     /**
diff --git a/server/src/main/java/com/vaadin/data/provider/InMemoryHierarchicalDataProvider.java b/server/src/main/java/com/vaadin/data/provider/InMemoryHierarchicalDataProvider.java
index b86c3186c6..8f70dd8e8c 100644
--- a/server/src/main/java/com/vaadin/data/provider/InMemoryHierarchicalDataProvider.java
+++ b/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 class InMemoryHierarchicalDataProvider<T> extends
 
     /**
      * Return the underlying hierarchical data of this provider.
-     * 
+     *
      * @return the underlying data of this provider
      */
     public HierarchyData<T> getData() {
@@ -77,6 +77,12 @@ public class InMemoryHierarchicalDataProvider<T> extends
 
     @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 class InMemoryHierarchicalDataProvider<T> extends
     @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());