fix a bug in edge deletions (#255)

The diffgraph applier was missing a check to not overread deletions into the next seq, which could happen if the ordering of subseqs was unfortunate.
joernio · Sep 11, 2024 · 2e05d59 · 2e05d59
1 parent bb570af
commit 2e05d59
Show file tree

Hide file tree

Showing 2 changed files with 40 additions and 1 deletion.
diff --git a/core/src/main/scala/flatgraph/DiffGraphApplier.scala b/core/src/main/scala/flatgraph/DiffGraphApplier.scala
@@ -488,12 +488,13 @@ private[flatgraph] class DiffGraphApplier(graph: Graph, diff: DiffGraphBuilder,
         var idx      = 0
         while (idx < deletionSeqIndexEnd - deletionSeqIndexStart) {
           if (deletion != null && idx == deletion.subSeq - 1) {
-            deletionCounter += 1
             assert(
               deletion.dst == oldNeighbors(deletionSeqIndexStart + idx),
               s"deletion.dst was supposed to be `${oldNeighbors(deletionSeqIndexStart + idx)}`, but instead is ${deletion.dst}"
             )
+            deletionCounter += 1
             deletion = if (deletionCounter < deletions.size) deletions(deletionCounter) else null
+            if (deletion != null && deletion.src.seq() != deletionSeq) deletion = null
           } else {
             newNeighbors(deletionSeqIndexStart + idx - deletionCounter) = oldNeighbors(deletionSeqIndexStart + idx)
             if (oldProperty != null)

diff --git a/core/src/test/scala/flatgraph/GraphTests.scala b/core/src/test/scala/flatgraph/GraphTests.scala
@@ -426,6 +426,44 @@ class GraphTests extends AnyWordSpec with Matchers {
     debugDump(g) shouldBe expectation
   }
 
+  "permit edge deletions with unfortunate ordering" in {
+    val g  = new Graph(schema)
+    val v0 = new GenericDNode(0)
+    val v1 = new GenericDNode(0)
+
+    DiffGraphApplier.applyDiff(
+      g,
+      new DiffGraphBuilder(schema)
+        .addNode(v0)
+        .addNode(v1)
+        ._addEdge(v0, v0, 0)
+        ._addEdge(v0, v0, 0)
+        ._addEdge(v1, v1, 0)
+        ._addEdge(v1, v1, 0)
+    )
+    debugDump(g) shouldBe """#Node numbers (kindId, nnodes) (0: 2), total 2
+                            |Node kind 0. (eid, nEdgesOut, nEdgesIn): (0, 4 [dense], 4 [dense]),
+                            |   V0_0   [0] -> V0_0, V0_0
+                            |   V0_0   [0] <- V0_0, V0_0
+                            |   V0_1   [0] -> V0_1, V0_1
+                            |   V0_1   [0] <- V0_1, V0_1
+                            |""".stripMargin
+    DiffGraphApplier.applyDiff(
+      g,
+      new DiffGraphBuilder(schema)
+        .removeEdge(Accessors.getEdgesOut(v0.storedRef.get, 0)(0))
+        .removeEdge(Accessors.getEdgesOut(v1.storedRef.get, 0)(1))
+    )
+    debugDump(g) shouldBe """#Node numbers (kindId, nnodes) (0: 2), total 2
+                            |Node kind 0. (eid, nEdgesOut, nEdgesIn): (0, 2 [dense], 2 [dense]),
+                            |   V0_0   [0] -> V0_0
+                            |   V0_0   [0] <- V0_0
+                            |   V0_1   [0] -> V0_1
+                            |   V0_1   [0] <- V0_1
+                            |""".stripMargin
+
+  }
+
   "permit node deletion" in {
     var g = mkGraph()
     debugDump(g) shouldBe