Graphv2: enable adding and retrieving nodes (#312)

This commit adds the following methods to the `Graph`:

* `addNode`
* `removeNode`
* `ref`
* `node`
* `nodes`
* `equals`

The graph now supports adding nodes, with thorough testing. Other
methods were implemented as necessary to test that `addNode` was
implemented properly.

Also, we've made a slight change to spec: `nodes` (and other filter
options) accept a `PluginFilter` object, which, if present, must
specify a plugin and may specify a type.

I've taken the opportunity to re-write the graph test code. Instead of
having a complicated `graphDemoData` file that creates a graph with many
different nodes, I've created an `examplePlugin` which makes it trivial
to instantiate new simple Foo and Bar nodes on the fly. Then, test cases
can construct a small graph that is clearly appropriate for whatever
functionality they are testing.

Test plan: Unit tests were added, travis passes.

src/core2/address_payload_unification_design.md

@@ -119,6 +119,7 @@ interface NodeReference {
 
   neighbors(options?: NeighborsOptions): Iterator<Neighbor<any>>;
 }
+export type PluginFilter = {|+plugin: string, +type?: string|};
 type NeighborsOptions = {|
   +nodeType?: string,
   +edgeType?: string,
@@ -374,8 +375,8 @@ declare class Graph /* no type parameters! */ {
   edge(address: Address): ?Edge<any>;
   ref(address: Address): NodeReference;
 
-  nodes(filter?: {|+type?: string|}): Iterator<Node<any, any>>;
+  nodes(filter?: PluginFilter): Iterator<Node<any, any>>;
-  edges(filter?: {|+type?: string|}): Iterator<Edge<any>>;
+  edges(filter?: PluginFilter): Iterator<Edge<any>>;
 
   static mergeConservative(Iterable<Graph>): Graph;
 

src/core2/examplePlugin.js

@@ -0,0 +1,98 @@
+// @flow
+
+import type {Address} from "./address";
+import {DelegateNodeReference} from "./graph";
+import type {NodeReference, NodePayload, PluginHandler} from "./graph";
+
+export type NodeType = "FOO" | "BAR";
+export const EXAMPLE_PLUGIN_NAME = "sourcecred/graph-demo-plugin";
+
+export class FooPayload implements NodePayload {
+  address() {
+    // There is only ever one Foo
+    return {owner: {plugin: EXAMPLE_PLUGIN_NAME, type: "FOO"}, id: ""};
+  }
+
+  toJSON() {
+    return {type: "FOO"};
+  }
+}
+
+export class BarPayload implements NodePayload {
+  _id: number;
+  _catchphrase: string;
+  constructor(id: number, catchphrase: string) {
+    this._id = id;
+    this._catchphrase = catchphrase;
+  }
+
+  address(): Address {
+    return {
+      owner: {plugin: EXAMPLE_PLUGIN_NAME, type: "BAR"},
+      id: this._id.toString(),
+    };
+  }
+
+  id(): number {
+    return this._id;
+  }
+
+  catchphrase(): string {
+    return this._catchphrase;
+  }
+
+  toJSON() {
+    return {type: "BAR", id: this._id, catchphrase: this._catchphrase};
+  }
+}
+
+export class FooReference extends DelegateNodeReference {
+  constructor(ref: NodeReference) {
+    super(ref);
+  }
+
+  // Return the number of adjacent BarNodes
+  numberOfBars(): number {
+    throw new Error("Requires neighborhood to be implemented first");
+  }
+}
+
+export class BarReference extends DelegateNodeReference {
+  constructor(ref: NodeReference) {
+    super(ref);
+  }
+}
+
+export class Handler implements PluginHandler<NodeReference, NodePayload> {
+  createReference(ref: NodeReference) {
+    const type: NodeType = (ref.address().owner.type: any);
+    switch (type) {
+      case "FOO":
+        return new FooReference(ref);
+      case "BAR":
+        return new BarReference(ref);
+      default:
+        // eslint-disable-next-line no-unused-expressions
+        (type: empty);
+        throw new Error(`Unexpected NodeType: ${type}`);
+    }
+  }
+
+  createPayload(json: any) {
+    const type: NodeType = json.type;
+    switch (type) {
+      case "FOO":
+        return new FooPayload();
+      case "BAR":
+        return new BarPayload(json.id, json.catchphrase);
+      default:
+        // eslint-disable-next-line no-unused-expressions
+        (type: empty);
+        throw new Error(`Unexpected NodeType: ${type}`);
+    }
+  }
+
+  pluginName() {
+    return EXAMPLE_PLUGIN_NAME;
+  }
+}

src/core2/graph.js

@@ -1,6 +1,8 @@
 // @flow
 
+import deepEqual from "lodash.isequal";
 import type {Address, PluginType} from "./address";
+import {AddressMap} from "./address";
 import type {Compatible} from "../util/compat";
 
 export type Node<NR: NodeReference, NP: NodePayload> = {|
@@ -37,9 +39,10 @@ export type Edge<+T> = {|
   +payload: T,
 |};
 
+export type PluginFilter = {|+plugin: string, +type?: string|};
 export type NeighborsOptions = {|
-  +node?: PluginType,
+  +node?: PluginFilter,
-  +edge?: PluginType,
+  +edge?: PluginFilter,
   +direction?: "IN" | "OUT" | "ANY",
 |};
 
@@ -64,31 +67,66 @@ export interface PluginHandler<NR: NodeReference, NP: NodePayload> {
 
 export type Plugins = $ReadOnlyArray<PluginHandler<any, any>>;
 
+type MaybeNode = {|+address: Address, +node: Node<any, any> | void|};
+type Integer = number;
+
 export class Graph {
   _plugins: Plugins;
+  _pluginMap: PluginMap;
+  _nodeIndices: AddressMap<{|+address: Address, +index: Integer|}>;
+  _nodes: MaybeNode[];
 
   constructor(plugins: Plugins) {
     this._plugins = plugins.slice();
+    this._pluginMap = createPluginMap(this._plugins);
+    this._nodes = [];
+    this._nodeIndices = new AddressMap();
   }
 
   ref(address: Address): NodeReference {
-    const _ = address;
+    if (address == null) {
-    throw new Error("Graphv2 is not yet implemented");
+      throw new Error(`address is ${String(address)}`);
+    }
+    // If node has an index and is still present, return the existing ref
+    const indexDatum = this._nodeIndices.get(address);
+    if (indexDatum != null) {
+      const node = this._nodes[indexDatum.index].node;
+      if (node != null) {
+        return node.ref;
+      }
+    }
+    // Otherwise, create a "dummy ref" that isn't backed by a node.
+    const handler = findHandler(this._pluginMap, address.owner.plugin);
+    return handler.createReference(new InternalReference(this, address));
   }
 
   node(address: Address): ?Node<any, any> {
-    const _ = address;
+    return this.ref(address).get();
-    throw new Error("Graphv2 is not yet implemented");
   }
 
   /**
    * Get nodes in the graph, in unspecified order.
    *
-   * If filter is provided, it will return only nodes with the requested type.
+   * If filter is provided, it will return only nodes with the requested plugin name
+   * (and, optionally, type).
    */
-  nodes(filter?: PluginType): Iterator<Node<any, any>> {
+  *nodes(filter?: PluginFilter): Iterator<Node<any, any>> {
-    const _ = filter;
+    for (const maybeNode of this._nodes) {
-    throw new Error("Graphv2 is not yet implemented");
+      const node = maybeNode.node;
+      if (node == null) {
+        continue;
+      }
+      if (filter != null) {
+        const owner = node.address.owner;
+        if (owner.plugin !== filter.plugin) {
+          continue;
+        }
+        if (filter.type != null && owner.type !== filter.type) {
+          continue;
+        }
+      }
+      yield node;
+    }
   }
 
   edge(address: Address): Edge<any> {
@@ -106,14 +144,49 @@ export class Graph {
     throw new Error("Graphv2 is not yet implemented");
   }
 
+  _addNodeAddress(address: Address): Integer {
+    const indexDatum = this._nodeIndices.get(address);
+    if (indexDatum != null) {
+      return indexDatum.index;
+    } else {
+      const index = this._nodes.length;
+      this._nodeIndices.add({address, index});
+      this._nodes.push({address, node: undefined});
+      return index;
+    }
+  }
+
   addNode(payload: NodePayload): this {
-    const _ = payload;
+    if (payload == null) {
-    throw new Error("Graphv2 is not yet implemented");
+      throw new Error(`payload is ${String(payload)}`);
+    }
+    const address = payload.address();
+    const index = this._addNodeAddress(address);
+    const maybeNode = this._nodes[index];
+    if (maybeNode.node !== undefined) {
+      if (deepEqual(maybeNode.node.payload, payload)) {
+        return this;
+      } else {
+        throw new Error(
+          `node at address ${JSON.stringify(
+            address
+          )} exists with distinct contents`
+        );
+      }
+    }
+    const handler = findHandler(this._pluginMap, address.owner.plugin);
+    const ref = handler.createReference(new InternalReference(this, address));
+    const node = {ref, payload, address};
+    this._nodes[index] = {address, node};
+    return this;
   }
 
   removeNode(address: Address): this {
-    const _ = address;
+    const indexDatum = this._nodeIndices.get(address);
-    throw new Error("Graphv2 is not yet implemented");
+    if (indexDatum != null) {
+      this._nodes[indexDatum.index] = {address, node: undefined};
+    }
+    return this;
   }
 
   addEdge(edge: Edge<any>): this {
@@ -144,9 +217,24 @@ export class Graph {
     throw new Error("Graphv2 is not yet implemented");
   }
 
+  /**
+   * Check the equality of two graphs. This verifies that the node and edge
+   * contents are identical; it does not check which plugin handlers are
+   * registered.
+   */
   equals(that: Graph): boolean {
-    const _ = that;
+    const theseNodes = Array.from(this.nodes());
-    throw new Error("Graphv2 is not yet implemented");
+    const thoseNodes = Array.from(that.nodes());
+    if (theseNodes.length !== thoseNodes.length) {
+      return false;
+    }
+
+    for (const node of theseNodes) {
+      if (!deepEqual(node, that.node(node.address))) {
+        return false;
+      }
+    }
+    return true;
   }
 
   copy(): Graph {
@@ -169,6 +257,25 @@ export class Graph {
 
 export type GraphJSON = any;
 
+type PluginMap = {[pluginName: string]: PluginHandler<any, any>};
+function createPluginMap(plugins: Plugins): PluginMap {
+  const pluginMap = {};
+  plugins.forEach((p) => {
+    const name = p.pluginName();
+    if (pluginMap[name] != null) {
+      throw new Error(`Duplicate plugin handler for "${name}"`);
+    }
+    pluginMap[name] = p;
+  });
+  return pluginMap;
+}
+function findHandler(pluginMap: PluginMap, pluginName: string) {
+  if (pluginMap[pluginName] == null) {
+    throw new Error(`No plugin handler for "${pluginName}"`);
+  }
+  return pluginMap[pluginName];
+}
+
 export class DelegateNodeReference implements NodeReference {
   // TODO(@wchargin): Use a Symbol here.
   __DelegateNodeReference_base: NodeReference;
@@ -188,3 +295,34 @@ export class DelegateNodeReference implements NodeReference {
     return this.__DelegateNodeReference_base.neighbors(options);
   }
 }
+
+class InternalReference implements NodeReference {
+  _graph: Graph;
+  _address: Address;
+
+  constructor(graph: Graph, address: Address) {
+    this._graph = graph;
+    this._address = address;
+  }
+
+  graph(): Graph {
+    return this._graph;
+  }
+  address(): Address {
+    return this._address;
+  }
+  get(): ?Node<any, any> {
+    const indexDatum = this._graph._nodeIndices.get(this._address);
+    if (indexDatum == null) {
+      return undefined;
+    }
+    return this._graph._nodes[indexDatum.index].node;
+  }
+
+  neighbors(
+    options?: NeighborsOptions
+  ): Iterator<{|+ref: NodeReference, +edge: Edge<any>|}> {
+    const _ = options;
+    throw new Error("Not implemented");
+  }
+}

src/core2/graph.test.js

@@ -1,27 +1,215 @@
 // @flow
-import * as demo from "./graphDemoData";
+import stringify from "json-stable-stringify";
+import sortBy from "lodash.sortby";
+
+import type {Node} from "./graph";
 import {Graph} from "./graph";
 
+import {
+  FooPayload,
+  FooReference,
+  BarPayload,
+  Handler,
+  EXAMPLE_PLUGIN_NAME,
+} from "./examplePlugin";
+
 describe("graph", () => {
+  function expectNodesSameSorted(
+    actual: Iterable<?Node<any, any>>,
+    expected: Iterable<?Node<any, any>>
+  ) {
+    const sort = (xs) =>
+      sortBy(Array.from(xs), (x) => (x == null ? "" : stringify(x.address)));
+    expect(sort(actual)).toEqual(sort(expected));
+  }
+
+  const newGraph = () => new Graph([new Handler()]);
+
   describe("plugin handlers", () => {
     it("Graph stores plugins", () => {
-      const plugins = demo.plugins();
+      const plugins = [new Handler()];
       const graph = new Graph(plugins);
       expect(graph.plugins()).toEqual(plugins);
     });
-
     it("Graph stored a slice of the plugins", () => {
       const plugins = [];
       const graph = new Graph(plugins);
-      plugins.push(new demo.Handler());
+      plugins.push(new Handler());
       expect(graph.plugins()).toHaveLength(0);
     });
-
     it("Graph returns a slice of the plugins", () => {
       const graph = new Graph([]);
       const plugins = graph.plugins();
-      (plugins: any).push(new demo.Handler());
+      (plugins: any).push(new Handler());
       expect(graph.plugins()).toHaveLength(0);
     });
   });
+
+  describe("ref", () => {
+    const ref = () => newGraph().ref(new FooPayload().address());
+    it(".address", () => {
+      expect(ref().address()).toEqual(new FooPayload().address());
+    });
+    it(".graph", () => {
+      expect(ref().graph()).toEqual(newGraph());
+    });
+    it(".get returns undefined when node not present", () => {
+      expect(ref().get()).toEqual(undefined);
+    });
+    it(".get returns node if node later added", () => {
+      const g = newGraph();
+      const address = new FooPayload().address();
+      const r = g.ref(address);
+      g.addNode(new FooPayload());
+      expect(r.get()).toEqual(g.node(address));
+    });
+    it("instantiates specific class using plugin handler", () => {
+      expect(ref()).toBeInstanceOf(FooReference);
+    });
+    it("errors for null or undefined address", () => {
+      const graph = newGraph();
+      expect(() => graph.ref((null: any))).toThrow("null");
+      expect(() => graph.ref((undefined: any))).toThrow("undefined");
+    });
+  });
+
+  describe("node", () => {
+    const withNode = () => newGraph().addNode(new FooPayload());
+    const address = () => new FooPayload().address();
+    const theNode = () => {
+      const x = withNode().node(address());
+      if (x == null) {
+        throw new Error("Persuade Flow this is non-null");
+      }
+      return x;
+    };
+    it("returns non-null when present", () => {
+      expect(theNode()).toEqual(expect.anything());
+    });
+    it("has an address", () => {
+      expect(theNode().address).toEqual(address());
+    });
+    it("has a ref", () => {
+      expect(theNode().ref).toEqual(withNode().ref(address()));
+    });
+    it("has a payload", () => {
+      expect(theNode().payload).toEqual(new FooPayload());
+    });
+    it("instantiates payload class", () => {
+      expect(theNode().payload).toBeInstanceOf(FooPayload);
+    });
+    it("returns null for an absent address", () => {
+      expect(newGraph().node(address())).toEqual(undefined);
+    });
+  });
+
+  describe("nodes", () => {
+    const barPayload = () => new BarPayload(1, "hello");
+    const twoNodes = () =>
+      newGraph()
+        .addNode(new FooPayload())
+        .addNode(barPayload());
+    const fooNode = () => twoNodes().node(new FooPayload().address());
+    const barNode = () => twoNodes().node(barPayload().address());
+    it("returns an empty list on empty graph", () => {
+      expect(Array.from(newGraph().nodes())).toHaveLength(0);
+    });
+    it("returns a list containing graph nodes", () => {
+      const nodes = Array.from(twoNodes().nodes());
+      expectNodesSameSorted(nodes, [fooNode(), barNode()]);
+    });
+    it("supports filtering by plugin", () => {
+      expect(Array.from(twoNodes().nodes({plugin: "xoombiazar"}))).toHaveLength(
+        0
+      );
+      expect(
+        Array.from(twoNodes().nodes({plugin: EXAMPLE_PLUGIN_NAME}))
+      ).toHaveLength(2);
+    });
+    it("supports filtering by plugin and type", () => {
+      const fooNodes = Array.from(
+        twoNodes().nodes({plugin: EXAMPLE_PLUGIN_NAME, type: "FOO"})
+      );
+      const barNodes = Array.from(
+        twoNodes().nodes({plugin: EXAMPLE_PLUGIN_NAME, type: "BAR"})
+      );
+      expect(fooNodes).toEqual([fooNode()]);
+      expect(barNodes).toEqual([barNode()]);
+    });
+    it("does not return removed nodes", () => {
+      const g = newGraph()
+        .addNode(barPayload())
+        .removeNode(barPayload().address());
+      const nodes = Array.from(g.nodes());
+      expect(nodes).toHaveLength(0);
+    });
+  });
+
+  describe("addNode", () => {
+    it("results in retrievable nodes", () => {
+      const graph = newGraph().addNode(new FooPayload());
+      expect(graph.node(new FooPayload().address())).toEqual(expect.anything());
+    });
+    it("is idempotent", () => {
+      const g1 = newGraph().addNode(new FooPayload());
+      const g2 = newGraph()
+        .addNode(new FooPayload())
+        .addNode(new FooPayload());
+      expect(g1.equals(g2)).toBe(true);
+      expect(Array.from(g1.nodes())).toEqual(Array.from(g2.nodes()));
+    });
+    it("throws an error if distinct payloads with the same address are added", () => {
+      const fail = () =>
+        newGraph()
+          .addNode(new BarPayload(1, "why hello"))
+          .addNode(new BarPayload(1, "there"));
+      expect(fail).toThrow("exists with distinct contents");
+    });
+    it("errors for null or undefined payload", () => {
+      expect(() => newGraph().addNode((null: any))).toThrow("null");
+      expect(() => newGraph().addNode((undefined: any))).toThrow("undefined");
+    });
+  });
+
+  describe("removeNode", () => {
+    it("removing a nonexistent node is not an error", () => {
+      const g = newGraph().removeNode(new FooPayload().address());
+      expect(g.equals(newGraph())).toBe(true);
+    });
+    it("removed nodes are not accessible", () => {
+      const g = newGraph().addNode(new FooPayload());
+      const address = new FooPayload().address();
+      const ref = g.ref(address);
+      g.removeNode(address);
+      expect(ref.get()).toEqual(undefined);
+      expect(g.node(address)).toEqual(undefined);
+    });
+  });
+
+  describe("equals", () => {
+    it("empty graphs are equal", () => {
+      expect(newGraph().equals(newGraph())).toBe(true);
+    });
+    it("graphs may be equal despite distinct plugin handlers", () => {
+      const g0 = new Graph([]);
+      const g1 = new Graph([new Handler()]);
+      expect(g0.equals(g1)).toBe(true);
+    });
+    it("graphs with different nodes are not equal", () => {
+      const g0 = newGraph().addNode(new FooPayload());
+      const g1 = newGraph().addNode(new BarPayload(1, "hello"));
+      expect(g0.equals(g1)).toBe(false);
+    });
+    it("graphs with different payloads at same address are not equal", () => {
+      const g0 = newGraph().addNode(new BarPayload(1, "hello"));
+      const g1 = newGraph().addNode(new BarPayload(1, "there"));
+      expect(g0.equals(g1)).toBe(false);
+    });
+    it("adding and removing a node doesn't change equality", () => {
+      const g = newGraph()
+        .addNode(new FooPayload())
+        .removeNode(new FooPayload().address());
+      expect(g.equals(newGraph())).toBe(true);
+    });
+  });
 });

src/core2/graphDemoData.js

@@ -1,111 +0,0 @@
-// @flow
-// This module provides some small demo graphs, which report
-// on a hero's adventures in cooking a seafood fruit mix.
-// It is factored as its own module so that it may be depended on by
-// multiple test and demo consumers.
-
-import type {NodeReference, NodePayload, PluginHandler} from "./graph";
-
-import {DelegateNodeReference} from "./graph";
-
-export const PLUGIN_NAME = "sourcecred/demo/cooking";
-
-export class Handler implements PluginHandler<DemoReference, DemoPayload<any>> {
-  createReference(ref: NodeReference) {
-    switch (ref.address().owner.type) {
-      case "PC":
-        return new HeroReference(ref);
-      case "INGREDIENT":
-        return new IngredientReference(ref);
-      case "MEAL":
-        return new MealReference(ref);
-      default:
-        return new DemoReference(ref);
-    }
-  }
-
-  createPayload(json: any) {
-    switch (json.type) {
-      case "PC":
-        return new HeroPayload();
-      case "INGREDIENT":
-        return new IngredientPayload(json.id, json.data.name);
-      case "MEAL":
-        return new MealPayload(json.id, json.data.name, json.data.effects);
-      default:
-        return new DemoPayload(json);
-    }
-  }
-
-  pluginName() {
-    return PLUGIN_NAME;
-  }
-}
-
-export class DemoReference extends DelegateNodeReference {
-  constructor(ref: NodeReference) {
-    super(ref);
-  }
-}
-
-export class DemoPayload<+T> implements NodePayload {
-  +_id: number;
-  +_type: string;
-  +_data: T;
-
-  constructor(json: {type: string, id: number, data: T}) {
-    this._id = json.id;
-    this._type = json.type;
-    this._data = json.data;
-  }
-
-  address() {
-    return {
-      owner: {plugin: PLUGIN_NAME, type: this._type},
-      id: String(this._id),
-    };
-  }
-
-  toJSON() {
-    return {type: this._type, id: this._id, data: this._data};
-  }
-}
-
-export class HeroReference extends DemoReference {
-  constructor(ref: NodeReference) {
-    super(ref);
-  }
-}
-export class HeroPayload extends DemoPayload<{}> {
-  // The chef that sears the darkness
-  constructor() {
-    super({id: 0, type: "PC", data: {}});
-  }
-}
-
-export class IngredientReference extends DemoReference {
-  constructor(ref: NodeReference) {
-    super(ref);
-  }
-}
-export class IngredientPayload extends DemoPayload<{|+name: string|}> {
-  constructor(id: number, name: string) {
-    super({id, type: "INGREDIENT", data: {name}});
-  }
-}
-
-export class MealReference extends DemoReference {
-  constructor(ref: NodeReference) {
-    super(ref);
-  }
-}
-export class MealPayload extends DemoPayload<{|
-  +name: string,
-  +effects: ?[string, number],
-|}> {
-  constructor(id: number, name: string, effects?: [string, number]) {
-    super({id, type: "MEAL", data: {name, effects}});
-  }
-}
-
-export const plugins = () => [new Handler()];