From e57a16efbd8fc48a1b8beece5356b6eb84499279 Mon Sep 17 00:00:00 2001 From: William Chargin Date: Mon, 26 Mar 2018 17:40:19 -0700 Subject: [PATCH] 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 | 13 ++ src/core/address.test.js | 12 ++ src/core/graph.js | 75 ++++++++---- src/core/graph.test.js | 249 +++++++++++++++++++++++++++++++++------ 4 files changed, 295 insertions(+), 54 deletions(-) diff --git a/src/core/address.js b/src/core/address.js index c49d123..27edc14 100644 --- a/src/core/address.js +++ b/src/core/address.js @@ -96,6 +96,19 @@ export class AddressMap { 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; + } } /** diff --git a/src/core/address.test.js b/src/core/address.test.js index 414ec9a..c9d8694 100644 --- a/src/core/address.test.js +++ b/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 = { diff --git a/src/core/graph.js b/src/core/graph.js index d8c509b..bfef755 100644 --- a/src/core/graph.js +++ b/src/core/graph.js @@ -71,6 +71,14 @@ export class Graph { return result; } + _lookupEdges( + map: AddressMap<{|+address: Address, +edges: Address[]|}>, + key: Address + ): Address[] { + const result = map.get(key); + return result ? result.edges : []; + } + addNode(node: Node): Graph { if (node == null) { throw new Error(`node is ${String(node)}`); @@ -88,8 +96,19 @@ export class Graph { } } this._nodes.add(node); - this._outEdges.add({address: node.address, edges: []}); - this._inEdges.add({address: node.address, edges: []}); + this._outEdges.add({ + 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 { ); } } - 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 { if (nodeAddress == null) { throw new Error(`address is ${String(nodeAddress)}`); } - const result = this._outEdges.get(nodeAddress); - if (result === undefined) { - throw new Error(`no node for address ${JSON.stringify(nodeAddress)}`); - } - return result.edges.map((e) => this.getEdge(e)); + return this._lookupEdges(this._outEdges, nodeAddress).map((e) => + this.getEdge(e) + ); } /** @@ -152,11 +189,9 @@ export class Graph { if (nodeAddress == null) { throw new Error(`address is ${String(nodeAddress)}`); } - const result = this._inEdges.get(nodeAddress); - if (result === undefined) { - throw new Error(`no node for address ${JSON.stringify(nodeAddress)}`); - } - return result.edges.map((e) => this.getEdge(e)); + return this._lookupEdges(this._inEdges, nodeAddress).map((e) => + this.getEdge(e) + ); } /** diff --git a/src/core/graph.test.js b/src/core/graph.test.js index cf5b740..e9b8581 100644 --- a/src/core/graph.test.js +++ b/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", () => { - expect(() => { - demoData.simpleMealGraph().getOutEdges(demoData.makeAddress("hinox")); - }).toThrow(/no node for address/); + it("gets empty out-edges for a nonexistent node", () => { + const result = demoData + .simpleMealGraph() + .getOutEdges(demoData.makeAddress("hinox")); + expect(result).toEqual([]); }); - it("fails to get in-edges for a nonexistent node", () => { - expect(() => { - demoData.simpleMealGraph().getInEdges(demoData.makeAddress("hinox")); - }).toThrow(/no node for address/); + it("gets empty in-edges for a nonexistent node", () => { + const result = demoData + .simpleMealGraph() + .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); }); });