Use `AddressMap` in `Graph` (#68)

Summary:
This commit simplifies the implementation of `Graph` without changing
its interface. We now use the `AddressMap` for all four instance fields
of `Graph`.

Test Plan:
All existing tests pass, and coverage is maintained.

wchargin-branch: use-address-map-in-graph

src/backend/graph.js

@@ -1,12 +1,8 @@
 // @flow
 
 import deepEqual from "lodash.isequal";
-
+import type {Address, Addressable} from "./address";
-export type Address = {|
+import {AddressMap} from "./address";
-  +repositoryName: string,
-  +pluginName: string,
-  +id: string,
-|};
 
 export type Node<T> = {|
   +address: Address,
@@ -21,28 +17,26 @@ export type Edge<T> = {|
 |};
 
 export class Graph {
-  _nodes: {[nodeAddress: string]: Node<mixed>};
+  _nodes: AddressMap<Node<mixed>>;
-  _edges: {[edgeAddress: string]: Edge<mixed>};
+  _edges: AddressMap<Edge<mixed>>;
 
   // The keyset of each of the following fields should equal the keyset
   // of `_nodes`. If `e` is an edge from `u` to `v`, then `e.address`
   // should appear exactly once in `_outEdges[u.address]` and exactly
   // once in `_inEdges[v.address]` (and every entry in `_inEdges` and
   // `_outEdges` should be of this form).
-  _outEdges: {[nodeAddress: string]: Address[]};
+  _outEdges: AddressMap<{|+address: Address, +edges: Address[]|}>;
-  _inEdges: {[nodeAddress: string]: Address[]};
+  _inEdges: AddressMap<{|+address: Address, +edges: Address[]|}>;
 
   constructor() {
-    this._nodes = {};
+    this._nodes = new AddressMap();
-    this._edges = {};
+    this._edges = new AddressMap();
-    this._outEdges = {};
+    this._outEdges = new AddressMap();
-    this._inEdges = {};
+    this._inEdges = new AddressMap();
   }
 
   equals(that: Graph): boolean {
-    return (
+    return this._nodes.equals(that._nodes) && this._edges.equals(that._edges);
-      deepEqual(this._nodes, that._nodes) && deepEqual(this._edges, that._edges)
-    );
   }
 
   addNode(node: Node<mixed>) {
@@ -54,10 +48,9 @@ export class Graph {
         `node at address ${JSON.stringify(node.address)} already exists`
       );
     }
-    const addressString = addressToString(node.address);
+    this._nodes.add(node);
-    this._nodes[addressString] = node;
+    this._outEdges.add({address: node.address, edges: []});
-    this._outEdges[addressString] = [];
+    this._inEdges.add({address: node.address, edges: []});
-    this._inEdges[addressString] = [];
     return this;
   }
 
@@ -76,18 +69,18 @@ export class Graph {
     if (this.getNode(edge.dst) === undefined) {
       throw new Error(`source ${JSON.stringify(edge.dst)} does not exist`);
     }
-    this._edges[addressToString(edge.address)] = edge;
+    this._edges.add(edge);
-    this._outEdges[addressToString(edge.src)].push(edge.address);
+    this._outEdges.get(edge.src).edges.push(edge.address);
-    this._inEdges[addressToString(edge.dst)].push(edge.address);
+    this._inEdges.get(edge.dst).edges.push(edge.address);
     return this;
   }
 
   getNode(address: Address): Node<mixed> {
-    return this._nodes[addressToString(address)];
+    return this._nodes.get(address);
   }
 
   getEdge(address: Address): Edge<mixed> {
-    return this._edges[addressToString(address)];
+    return this._edges.get(address);
   }
 
   /**
@@ -98,11 +91,11 @@ export class Graph {
     if (nodeAddress == null) {
       throw new Error(`address is ${String(nodeAddress)}`);
     }
-    const addresses = this._outEdges[addressToString(nodeAddress)];
+    const result = this._outEdges.get(nodeAddress);
-    if (addresses === undefined) {
+    if (result === undefined) {
       throw new Error(`no node for address ${JSON.stringify(nodeAddress)}`);
     }
-    return addresses.map((e) => this.getEdge(e));
+    return result.edges.map((e) => this.getEdge(e));
   }
 
   /**
@@ -113,25 +106,25 @@ export class Graph {
     if (nodeAddress == null) {
       throw new Error(`address is ${String(nodeAddress)}`);
     }
-    const addresses = this._inEdges[addressToString(nodeAddress)];
+    const result = this._inEdges.get(nodeAddress);
-    if (addresses === undefined) {
+    if (result === undefined) {
       throw new Error(`no node for address ${JSON.stringify(nodeAddress)}`);
     }
-    return addresses.map((e) => this.getEdge(e));
+    return result.edges.map((e) => this.getEdge(e));
   }
 
   /**
    * Gets all nodes in the graph, in unspecified order.
    */
   getAllNodes(): Node<mixed>[] {
-    return Object.keys(this._nodes).map((k) => this._nodes[k]);
+    return this._nodes.getAll();
   }
 
   /**
    * Gets all edges in the graph, in unspecified order.
    */
   getAllEdges(): Edge<mixed>[] {
-    return Object.keys(this._edges).map((k) => this._edges[k]);
+    return this._edges.getAll();
   }
 
   /**
@@ -185,7 +178,7 @@ export class Graph {
    * raise an error.
    */
   static mergeConservative(g1: Graph, g2: Graph) {
-    function conservativeReducer<T: {+address: Address}>(
+    function conservativeReducer<T: Addressable>(
       kinds: string /* used for an error message on mismatch */,
       a: T,
       b: T
@@ -194,7 +187,7 @@ export class Graph {
         return a;
       } else {
         throw new Error(
-          `distinct ${kinds} with address ${addressToString(a.address)}`
+          `distinct ${kinds} with address ${JSON.stringify(a.address)}`
         );
       }
     }
@@ -206,38 +199,3 @@ export class Graph {
     );
   }
 }
-
-export function addressToString(address: Address) {
-  if (address == null) {
-    throw new Error(`address is ${String(address)}`);
-  }
-  if (address.repositoryName.includes("$")) {
-    const escaped = JSON.stringify(address.repositoryName);
-    throw new Error(`address.repositoryName must not include "\$": ${escaped}`);
-  }
-  if (address.pluginName.includes("$")) {
-    const escaped = JSON.stringify(address.pluginName);
-    throw new Error(`address.pluginName must not include "\$": ${escaped}`);
-  }
-  if (address.id.includes("$")) {
-    const escaped = JSON.stringify(address.id);
-    throw new Error(`address.id must not include "\$": ${escaped}`);
-  }
-  return `${address.repositoryName}\$${address.pluginName}\$${address.id}`;
-}
-
-export function stringToAddress(string: string) {
-  if (string == null) {
-    throw new Error(`address string is ${String(string)}`);
-  }
-  const parts = string.split("$");
-  if (parts.length !== 3) {
-    const escaped = JSON.stringify(string);
-    throw new Error(`Input should have exactly two \$s: ${escaped}`);
-  }
-  return {
-    repositoryName: parts[0],
-    pluginName: parts[1],
-    id: parts[2],
-  };
-}

src/backend/graph.test.js

@@ -1,23 +1,15 @@
 // @flow
 
-import type {Address} from "./graph";
+import type {Address, Addressable} from "./address";
-import {Graph, addressToString, stringToAddress} from "./graph";
+import {sortedByAddress} from "./address";
+import {Graph} from "./graph";
 
 describe("graph", () => {
   describe("#Graph", () => {
     // Some Graph functions return a set of results represented as an
-    // array with undefined order. We use these functions to
+    // array with undefined order. We canonicalize the ordering so that
-    // canonicalize the ordering so that we can then test equality with
+    // we can then test equality with `expect(...).toEqual(...)`.
-    // `expect(...).toEqual(...)`.
+    function expectSameSorted<T: Addressable>(xs: T[], ys: T[]) {
-    function sortedByAddress<T: {+address: Address}>(xs: T[]) {
-      function cmp(x1: T, x2: T) {
-        const a1 = addressToString(x1.address);
-        const a2 = addressToString(x2.address);
-        return a1 > a2 ? 1 : a1 < a2 ? -1 : 0;
-      }
-      return [...xs].sort(cmp);
-    }
-    function expectSameSorted<T: {+address: Address}>(xs: T[], ys: T[]) {
       expect(sortedByAddress(xs)).toEqual(sortedByAddress(ys));
     }
 
@@ -564,97 +556,4 @@ describe("graph", () => {
       });
     });
   });
-
-  describe("string functions", () => {
-    describe("addressToString", () => {
-      const makeSimpleAddress = () => ({
-        repositoryName: "megacorp/megawidget",
-        pluginName: "widgets",
-        id: "issue#123",
-      });
-      it("stringifies a simple Address", () => {
-        const input = makeSimpleAddress();
-        const expected = "megacorp/megawidget$widgets$issue#123";
-        expect(addressToString(input)).toEqual(expected);
-      });
-      function expectRejection(attribute, value) {
-        const input = {...makeSimpleAddress(), [attribute]: value};
-        expect(() => addressToString(input)).toThrow(RegExp(attribute));
-        // (escaping regexp in JavaScript is a nightmare; ignore it)
-      }
-      it("rejects an Address with $-signs in plugin name", () => {
-        expectRejection("pluginName", "widg$ets");
-      });
-      it("rejects an Address with $-signs in repository name", () => {
-        expectRejection("repositoryName", "megacorp$megawidget");
-      });
-      it("rejects an Address with $-signs in id", () => {
-        expectRejection("id", "issue$123");
-      });
-      it("rejects a null address", () => {
-        expect(() => addressToString((null: any))).toThrow("address is null");
-      });
-      it("rejects a undefined address", () => {
-        expect(() => addressToString((undefined: any))).toThrow(
-          "address is undefined"
-        );
-      });
-    });
-
-    describe("stringToAddress", () => {
-      it("parses a simple Address-string", () => {
-        const input = "megacorp/megawidget$widgets$issue#123";
-        const expected = {
-          repositoryName: "megacorp/megawidget",
-          pluginName: "widgets",
-          id: "issue#123",
-        };
-        expect(stringToAddress(input)).toEqual(expected);
-      });
-      [0, 1, 3, 4].forEach((n) => {
-        it(`rejects an Address-string with ${n} occurrences of "\$"`, () => {
-          const dollars = Array(n + 1).join("$");
-          const input = `mega${dollars}corp`;
-          expect(() => stringToAddress(input)).toThrow(/exactly two \$s/);
-        });
-      });
-      it("rejects a null address string", () => {
-        expect(() => stringToAddress((null: any))).toThrow(
-          "address string is null"
-        );
-      });
-      it("rejects a undefined address string", () => {
-        expect(() => stringToAddress((undefined: any))).toThrow(
-          "address string is undefined"
-        );
-      });
-    });
-
-    describe("stringToAddress and addressToString interop", () => {
-      const examples = () => [
-        {
-          object: {
-            repositoryName: "megacorp/megawidget",
-            pluginName: "widgets",
-            id: "issue#123",
-          },
-          string: "megacorp/megawidget$widgets$issue#123",
-        },
-      ];
-      examples().forEach((example, index) => {
-        describe(`for example at 0-index ${index}`, () => {
-          it("has stringToAddress a left identity for addressToString", () => {
-            expect(stringToAddress(addressToString(example.object))).toEqual(
-              example.object
-            );
-          });
-          it("has stringToAddress a right identity for addressToString", () => {
-            expect(addressToString(stringToAddress(example.string))).toEqual(
-              example.string
-            );
-          });
-        });
-      });
-    });
-  });
 });