diffgraph: ensure we don't crash if a node get's deleted twice (#414)

Author: mpollmeier

core-tests/src/test/java/overflowdb/DiffGraphTest.java

@@ -0,0 +1,62 @@
+package overflowdb;
+
+import org.junit.Test;
+import overflowdb.testdomains.simple.SimpleDomain;
+import overflowdb.testdomains.simple.TestEdge;
+import overflowdb.testdomains.simple.TestNode;
+
+import static org.junit.Assert.assertEquals;
+
+public class DiffGraphTest {
+
+  @Test
+  public void nodeRemovalTest() {
+    try (Graph graph = SimpleDomain.newGraph()) {
+      BatchedUpdate.DiffGraphBuilder diff1 = new BatchedUpdate.DiffGraphBuilder();
+      DetachedNodeData newNode = new DetachedNodeGeneric(TestNode.LABEL, TestNode.STRING_PROPERTY, "node 1");
+      diff1.addNode(newNode);
+      BatchedUpdate.applyDiff(graph, diff1);
+      assertNodeCount(1, graph);
+      Node node = graph.nodes().next();
+
+      BatchedUpdate.DiffGraphBuilder diff2 = new BatchedUpdate.DiffGraphBuilder();
+      diff2.removeNode(node);
+      // it shouldn't matter (and especially not error) if we remove the same node twice
+      diff2.removeNode(node);
+      BatchedUpdate.applyDiff(graph, diff2);
+      assertNodeCount(0, graph);
+    }
+  }
+
+  @Test
+  public void edgeRemovalTest() {
+    try (Graph graph = SimpleDomain.newGraph()) {
+      BatchedUpdate.DiffGraphBuilder diff1 = new BatchedUpdate.DiffGraphBuilder();
+      DetachedNodeData n1D = new DetachedNodeGeneric(TestNode.LABEL, TestNode.STRING_PROPERTY, "node 1");
+      DetachedNodeData n2D = new DetachedNodeGeneric(TestNode.LABEL, TestNode.STRING_PROPERTY, "node 2");
+      diff1.addEdge(n1D, n2D,TestEdge.LABEL, TestEdge.LONG_PROPERTY, 99L);
+
+      BatchedUpdate.applyDiff(graph, diff1);
+      assertNodeCount(2, graph);
+      assertEdgeCount(1, graph);
+      Edge edge = graph.edges().next();
+
+      BatchedUpdate.DiffGraphBuilder diff2 = new BatchedUpdate.DiffGraphBuilder();
+      diff2.removeEdge(edge);
+      // it shouldn't matter (and especially not error) if we remove the same edge twice
+      diff2.removeEdge(edge);
+      BatchedUpdate.applyDiff(graph, diff2);
+      assertNodeCount(2, graph);
+      assertEdgeCount(0, graph);
+    }
+  }
+
+  private void assertNodeCount(int expected, Graph graph) {
+    assertEquals("node count different to expected", expected, graph.nodeCount());
+  }
+
+  private void assertEdgeCount(int expected, Graph graph) {
+    assertEquals("edge count different to expected", expected, graph.edgeCount());
+  }
+
+}

core/src/main/java/overflowdb/BatchedUpdate.java

@@ -2,8 +2,7 @@
 
 import overflowdb.util.IteratorUtils;
 
-import java.util.ArrayDeque;
-import java.util.Iterator;
+import java.util.*;
 
 public class BatchedUpdate {
 
@@ -199,6 +198,16 @@ public static class RemoveNode implements Change {
         public RemoveNode(Node node) {
             this.node = node;
         }
+
+        @Override
+        public boolean equals(Object obj) {
+            return (obj instanceof RemoveNode) && ((RemoveNode) obj).node.id() == node.id();
+        }
+
+        @Override
+        public int hashCode() {
+            return Objects.hash(node.id());
+        }
     }
 
     public static class CreateEdge implements Change {
@@ -235,6 +244,10 @@ private static class DiffGraphApplier {
         private final Graph graph;
         private int nChanges = 0;
 
+        /** We keep track of already removed nodes to ensure we don't fail to load them from storage when they are
+         * removed multiple times in the same Diff. */
+        private Set<RemoveNode> removedNodes = new HashSet<>();
+
         DiffGraphApplier(Graph graph, DiffOrBuilder diff, KeyPool keyPool, ModificationListener listener) {
             this.diff = diff;
             this.keyPool = keyPool;
@@ -311,11 +324,13 @@ private void applyChange(Change change) {
                 remove.edge.removeInternal();
             } else if (change instanceof RemoveNode) {
                 nChanges += 1;
-                RemoveNode remove = (RemoveNode) change;
-                if (listener != null)
-                    listener.onBeforeRemoveNode(remove.node);
-                remove.node.removeInternal();
-
+                if (!removedNodes.contains(change)) {
+                    RemoveNode remove = (RemoveNode) change;
+                    if (listener != null)
+                        listener.onBeforeRemoveNode(remove.node);
+                    remove.node.removeInternal();
+                    removedNodes.add(remove);
+                }
             } else if (change instanceof SetNodeProperty) {
                 nChanges += 1;
                 SetNodeProperty setProp = (SetNodeProperty) change;

project/build.properties

@@ -1 +1 @@
-sbt.version=1.9.0
+sbt.version=1.9.8