Allow removing nodes and edges from the graph (#110)

Summary:
Wherein we change the semantics to allow\* dangling edges. This is
necessary for plugins that want to update nodes, such as changing a
description or other noncritical field.

\* (It was technically possible before by abusing `merge`, but now you
can just do it.)

Paired with @dandelionmane.

Test Plan:
Extensive tests added. Run `yarn flow` and `yarn test`.

wchargin-branch: allow-removing-from-graph

src/core/address.js

@@ -96,6 +96,19 @@ export class AddressMap<T: Addressable> {
   getAll(): T[] {
     return Object.keys(this._data).map((k) => this._data[k]);
   }
+
+  /**
+   * Remove any object with the given address. If none exists, this
+   * method does nothing.
+   */
+  remove(address: Address): this {
+    if (address == null) {
+      throw new Error(`address is ${String(address)}`);
+    }
+    const key = toString(address);
+    delete this._data[key];
+    return this;
+  }
 }
 
 /**

src/core/address.test.js

@@ -54,6 +54,14 @@ describe("address", () => {
       expect(sortedByAddress(actual)).toEqual(sortedByAddress(expected));
     });
 
+    it("removes objects by key", () => {
+      expect(
+        makeMap()
+          .remove(mansion().address)
+          .get(mansion().address)
+      ).toBeUndefined();
+    });
+
     it("stringifies to JSON", () => {
       expect(makeMap().toJSON()).toMatchSnapshot();
     });
@@ -112,6 +120,10 @@ describe("address", () => {
           const message = `address is ${String(bad)}`;
           expect(() => makeMap().get((bad: any))).toThrow(message);
         });
+        it(`when removing ${String(bad)} elements`, () => {
+          const message = `address is ${String(bad)}`;
+          expect(() => makeMap().remove((bad: any))).toThrow(message);
+        });
         it(`when adding elements with ${String(bad)} address`, () => {
           const message = `address is ${String(bad)}`;
           const element = {

src/core/graph.js

@@ -71,6 +71,14 @@ export class Graph<NP, EP> {
     return result;
   }
 
+  _lookupEdges(
+    map: AddressMap<{|+address: Address, +edges: Address[]|}>,
+    key: Address
+  ): Address[] {
+    const result = map.get(key);
+    return result ? result.edges : [];
+  }
+
   addNode(node: Node<NP>): Graph<NP, EP> {
     if (node == null) {
       throw new Error(`node is ${String(node)}`);
@@ -88,8 +96,19 @@ export class Graph<NP, EP> {
       }
     }
     this._nodes.add(node);
-    this._outEdges.add({address: node.address, edges: []});
+    this._outEdges.add({
-    this._inEdges.add({address: node.address, edges: []});
+      address: node.address,
+      edges: this._lookupEdges(this._outEdges, node.address),
+    });
+    this._inEdges.add({
+      address: node.address,
+      edges: this._lookupEdges(this._inEdges, node.address),
+    });
+    return this;
+  }
+
+  removeNode(address: Address): this {
+    this._nodes.remove(address);
     return this;
   }
 
@@ -109,15 +128,35 @@ export class Graph<NP, EP> {
         );
       }
     }
-    if (this.getNode(edge.src) === undefined) {
-      throw new Error(`source ${JSON.stringify(edge.src)} does not exist`);
-    }
-    if (this.getNode(edge.dst) === undefined) {
-      throw new Error(`source ${JSON.stringify(edge.dst)} does not exist`);
-    }
     this._edges.add(edge);
-    this._outEdges.get(edge.src).edges.push(edge.address);
+
-    this._inEdges.get(edge.dst).edges.push(edge.address);
+    const theseOutEdges = this._lookupEdges(this._outEdges, edge.src);
+    theseOutEdges.push(edge.address);
+    this._outEdges.add({address: edge.src, edges: theseOutEdges});
+
+    const theseInEdges = this._lookupEdges(this._inEdges, edge.dst);
+    theseInEdges.push(edge.address);
+    this._inEdges.add({address: edge.dst, edges: theseInEdges});
+
+    return this;
+  }
+
+  removeEdge(address: Address): this {
+    // TODO(perf): This is linear in the degree of the endpoints of the
+    // edge. Consider storing in non-list form.
+    const edge = this.getEdge(address);
+    if (edge) {
+      [
+        this._lookupEdges(this._inEdges, edge.dst),
+        this._lookupEdges(this._outEdges, edge.src),
+      ].forEach((edges) => {
+        const index = edges.findIndex((ea) => deepEqual(ea, address));
+        if (index !== -1) {
+          edges.splice(index, 1);
+        }
+      });
+    }
+    this._edges.remove(address);
     return this;
   }
 
@@ -137,11 +176,9 @@ export class Graph<NP, EP> {
     if (nodeAddress == null) {
       throw new Error(`address is ${String(nodeAddress)}`);
     }
-    const result = this._outEdges.get(nodeAddress);
+    return this._lookupEdges(this._outEdges, nodeAddress).map((e) =>
-    if (result === undefined) {
+      this.getEdge(e)
-      throw new Error(`no node for address ${JSON.stringify(nodeAddress)}`);
+    );
-    }
-    return result.edges.map((e) => this.getEdge(e));
   }
 
   /**
@@ -152,11 +189,9 @@ export class Graph<NP, EP> {
     if (nodeAddress == null) {
       throw new Error(`address is ${String(nodeAddress)}`);
     }
-    const result = this._inEdges.get(nodeAddress);
+    return this._lookupEdges(this._inEdges, nodeAddress).map((e) =>
-    if (result === undefined) {
+      this.getEdge(e)
-      throw new Error(`no node for address ${JSON.stringify(nodeAddress)}`);
+    );
-    }
-    return result.edges.map((e) => this.getEdge(e));
   }
 
   /**

src/core/graph.test.js

@@ -23,32 +23,6 @@ describe("graph", () => {
       it("works for an advanced graph", () => {
         demoData.advancedMealGraph();
       });
-
-      it("forbids adding an edge with dangling `dst`", () => {
-        expect(() => {
-          demoData.simpleMealGraph().addEdge({
-            address: demoData.makeAddress(
-              "treasure_octorok#5@helps_cook@seafood_fruit_mix#3"
-            ),
-            src: demoData.mealNode().address,
-            dst: demoData.makeAddress("treasure_octorok#5"),
-            payload: {},
-          });
-        }).toThrow(/does not exist/);
-      });
-
-      it("forbids adding an edge with dangling `src`", () => {
-        expect(() => {
-          demoData.simpleMealGraph().addEdge({
-            address: demoData.makeAddress(
-              "health_bar#6@healed_by@seafood_fruit_mix#3"
-            ),
-            src: demoData.makeAddress("health_bar#6"),
-            dst: demoData.mealNode().address,
-            payload: {},
-          });
-        }).toThrow(/does not exist/);
-      });
     });
 
     describe("has nice error messages for", () => {
@@ -67,6 +41,16 @@ describe("graph", () => {
             `edge is ${String(bad)}`
           );
         });
+        it(`removing ${String(bad)} nodes`, () => {
+          expect(() => new Graph().removeNode((bad: any))).toThrow(
+            `address is ${String(bad)}`
+          );
+        });
+        it(`removing ${String(bad)} edges`, () => {
+          expect(() => new Graph().removeEdge((bad: any))).toThrow(
+            `address is ${String(bad)}`
+          );
+        });
         it(`getting ${String(bad)} nodes`, () => {
           expect(() => new Graph().getNode((bad: any))).toThrow(
             `address is ${String(bad)}`
@@ -193,6 +177,32 @@ describe("graph", () => {
     });
 
     describe("creating nodes and edges", () => {
+      it("allows adding an edge with dangling `dst`", () => {
+        const edge = () => ({
+          address: demoData.makeAddress(
+            "treasure_octorok#5@helps_cook@seafood_fruit_mix#3"
+          ),
+          src: demoData.mealNode().address,
+          dst: demoData.makeAddress("treasure_octorok#5"),
+          payload: {},
+        });
+        const g = demoData.simpleMealGraph().addEdge(edge());
+        expect(g.getEdge(edge().address)).toEqual(edge());
+      });
+
+      it("allows adding an edge with dangling `src`", () => {
+        const edge = () => ({
+          address: demoData.makeAddress(
+            "health_bar#6@healed_by@seafood_fruit_mix#3"
+          ),
+          src: demoData.makeAddress("health_bar#6"),
+          dst: demoData.mealNode().address,
+          payload: {},
+        });
+        const g = demoData.simpleMealGraph().addEdge(edge());
+        expect(g.getEdge(edge().address)).toEqual(edge());
+      });
+
       it("forbids adding a node with existing address and different contents", () => {
         expect(() =>
           demoData.simpleMealGraph().addNode({
@@ -265,6 +275,47 @@ describe("graph", () => {
       });
     });
 
+    describe("removing nodes and edges", () => {
+      it("is a roundtrip to add and remove and add a node", () => {
+        const n = () => demoData.crabNode();
+
+        const g1 = () => new Graph();
+        expect(g1().getNode(n().address)).toBeUndefined();
+
+        const g2 = () => g1().addNode(n());
+        expect(g2().getNode(n().address)).toEqual(n());
+
+        const g3 = () => g2().removeNode(n().address);
+        expect(g3().getNode(n().address)).toBeUndefined();
+
+        const g4 = () => g3().addNode(n());
+        expect(g4().getNode(n().address)).toEqual(n());
+
+        expect(g1().equals(g3())).toBe(true);
+        expect(g2().equals(g4())).toBe(true);
+      });
+
+      it("is a roundtrip to add and remove and add an edge", () => {
+        const n = () => demoData.crabNode();
+        const e = () => demoData.crabLoopEdge();
+
+        const g1 = () => new Graph().addNode(n());
+        expect(g1().getEdge(e().address)).toBeUndefined();
+
+        const g2 = () => g1().addEdge(e());
+        expect(g2().getEdge(e().address)).toEqual(e());
+
+        const g3 = () => g2().removeEdge(e().address);
+        expect(g3().getEdge(e().address)).toBeUndefined();
+
+        const g4 = () => g3().addEdge(e());
+        expect(g4().getEdge(e().address)).toEqual(e());
+
+        expect(g1().equals(g3())).toBe(true);
+        expect(g2().equals(g4())).toBe(true);
+      });
+    });
+
     describe("in- and out-edges", () => {
       it("gets out-edges", () => {
         const nodeAndExpectedEdgePairs = [
@@ -311,16 +362,146 @@ describe("graph", () => {
         });
       });
 
-      it("fails to get out-edges for a nonexistent node", () => {
+      it("gets empty out-edges for a nonexistent node", () => {
-        expect(() => {
+        const result = demoData
-          demoData.simpleMealGraph().getOutEdges(demoData.makeAddress("hinox"));
+          .simpleMealGraph()
-        }).toThrow(/no node for address/);
+          .getOutEdges(demoData.makeAddress("hinox"));
+        expect(result).toEqual([]);
       });
 
-      it("fails to get in-edges for a nonexistent node", () => {
+      it("gets empty in-edges for a nonexistent node", () => {
-        expect(() => {
+        const result = demoData
-          demoData.simpleMealGraph().getInEdges(demoData.makeAddress("hinox"));
+          .simpleMealGraph()
-        }).toThrow(/no node for address/);
+          .getInEdges(demoData.makeAddress("hinox"));
+        expect(result).toEqual([]);
+      });
+
+      {
+        const danglingSrc = () => ({
+          address: demoData.makeAddress("meaty_rice_balls#8"),
+          payload: {meaty: true},
+        });
+        const danglingDst = () => ({
+          address: demoData.makeAddress("treasure_octorok#5"),
+          payload: {meaty: false},
+        });
+
+        // A valid edge neither of whose endpoints are in the default
+        // demo meal graph.
+        const fullyDanglingEdge = () => ({
+          address: demoData.makeAddress(
+            "treasure_octorok#5@helps_cook@meaty_rice_balls#8"
+          ),
+          src: danglingSrc().address,
+          dst: danglingDst().address,
+          payload: {},
+        });
+
+        it("has in-edges for deleted node with dangling edge", () => {
+          const g = demoData
+            .simpleMealGraph()
+            .addNode(danglingSrc())
+            .addNode(danglingDst())
+            .addEdge(fullyDanglingEdge())
+            .removeNode(danglingSrc().address)
+            .removeNode(danglingDst().address);
+          const inEdges = g.getInEdges(fullyDanglingEdge().dst);
+          expect(inEdges).toEqual([fullyDanglingEdge()]);
+        });
+
+        it("has out-edges for deleted node with dangling edge", () => {
+          const g = demoData
+            .simpleMealGraph()
+            .addNode(danglingSrc())
+            .addNode(danglingDst())
+            .addEdge(fullyDanglingEdge())
+            .removeNode(danglingSrc().address)
+            .removeNode(danglingDst().address);
+          const outEdges = g.getOutEdges(fullyDanglingEdge().src);
+          expect(outEdges).toEqual([fullyDanglingEdge()]);
+        });
+
+        it("has lack of in-edges for deleted edge", () => {
+          const g = demoData
+            .simpleMealGraph()
+            .addNode(danglingSrc())
+            .addNode(danglingDst())
+            .addEdge(fullyDanglingEdge())
+            .removeEdge(fullyDanglingEdge().address);
+          const outEdges = g.getInEdges(fullyDanglingEdge().dst);
+          expect(outEdges).toEqual([]);
+        });
+
+        it("has lack of out-edges for deleted edge", () => {
+          const g = demoData
+            .simpleMealGraph()
+            .addNode(danglingSrc())
+            .addNode(danglingDst())
+            .addEdge(fullyDanglingEdge())
+            .removeEdge(fullyDanglingEdge().address);
+          const outEdges = g.getOutEdges(fullyDanglingEdge().src);
+          expect(outEdges).toEqual([]);
+        });
+
+        it("has in-edges for non-existent node with dangling edge", () => {
+          const g = demoData.simpleMealGraph().addEdge(fullyDanglingEdge());
+          const inEdges = g.getInEdges(fullyDanglingEdge().dst);
+          expect(inEdges).toEqual([fullyDanglingEdge()]);
+        });
+
+        it("has out-edges for non-existent node with dangling edge", () => {
+          const g = demoData.simpleMealGraph().addEdge(fullyDanglingEdge());
+          const outEdges = g.getOutEdges(fullyDanglingEdge().src);
+          expect(outEdges).toEqual([fullyDanglingEdge()]);
+        });
+
+        it("has in-edges that were added before their endpoints", () => {
+          const g = demoData
+            .simpleMealGraph()
+            .addEdge(fullyDanglingEdge())
+            .addNode(danglingDst());
+          const inEdges = g.getInEdges(fullyDanglingEdge().dst);
+          expect(inEdges).toEqual([fullyDanglingEdge()]);
+        });
+
+        it("has out-edges that were added before their endpoints", () => {
+          const g = demoData
+            .simpleMealGraph()
+            .addEdge(fullyDanglingEdge())
+            .addNode(danglingSrc());
+          const outEdges = g.getOutEdges(fullyDanglingEdge().src);
+          expect(outEdges).toEqual([fullyDanglingEdge()]);
+        });
+      }
+    });
+
+    describe("when adding edges multiple times", () => {
+      const originalGraph = () => demoData.advancedMealGraph();
+      const targetEdge = () => demoData.crabLoopEdge();
+      const modifiedGraph = () => {
+        const g = originalGraph();
+        g.addEdge(targetEdge()); // should be redundant
+        g.addEdge(targetEdge()); // should be redundant
+        return g;
+      };
+      it("is idempotent in terms of graph equality", () => {
+        const g1 = originalGraph();
+        const g2 = modifiedGraph();
+        expect(g1.equals(g2)).toBe(true);
+      });
+      it("is idempotent in terms of in-edges", () => {
+        const g1 = originalGraph();
+        const g2 = modifiedGraph();
+        const e1 = sortedByAddress(g1.getInEdges(targetEdge().address));
+        const e2 = sortedByAddress(g2.getInEdges(targetEdge().address));
+        expect(e1).toEqual(e2);
+      });
+      it("is idempotent in terms of out-edges", () => {
+        const g1 = originalGraph();
+        const g2 = modifiedGraph();
+        const e1 = sortedByAddress(g1.getOutEdges(targetEdge().address));
+        const e2 = sortedByAddress(g2.getOutEdges(targetEdge().address));
+        expect(e1).toEqual(e2);
       });
     });