From 8ab05989394500a801a2acda93315bcf9f2e4f50 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dandelion=20Man=C3=A9?= Date: Tue, 29 May 2018 12:14:50 -0700 Subject: [PATCH] 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. --- .../address_payload_unification_design.md | 5 +- src/core2/examplePlugin.js | 98 +++++++++ src/core2/graph.js | 170 +++++++++++++-- src/core2/graph.test.js | 200 +++++++++++++++++- src/core2/graphDemoData.js | 111 ---------- 5 files changed, 449 insertions(+), 135 deletions(-) create mode 100644 src/core2/examplePlugin.js delete mode 100644 src/core2/graphDemoData.js diff --git a/src/core2/address_payload_unification_design.md b/src/core2/address_payload_unification_design.md index be8beba..096c78b 100644 --- a/src/core2/address_payload_unification_design.md +++ b/src/core2/address_payload_unification_design.md @@ -119,6 +119,7 @@ interface NodeReference { neighbors(options?: NeighborsOptions): Iterator>; } +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; ref(address: Address): NodeReference; - nodes(filter?: {|+type?: string|}): Iterator>; - edges(filter?: {|+type?: string|}): Iterator>; + nodes(filter?: PluginFilter): Iterator>; + edges(filter?: PluginFilter): Iterator>; static mergeConservative(Iterable): Graph; diff --git a/src/core2/examplePlugin.js b/src/core2/examplePlugin.js new file mode 100644 index 0000000..6727c55 --- /dev/null +++ b/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 { + 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; + } +} diff --git a/src/core2/graph.js b/src/core2/graph.js index 69f3921..1118531 100644 --- a/src/core2/graph.js +++ b/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 = {| @@ -37,9 +39,10 @@ export type Edge<+T> = {| +payload: T, |}; +export type PluginFilter = {|+plugin: string, +type?: string|}; export type NeighborsOptions = {| - +node?: PluginType, - +edge?: PluginType, + +node?: PluginFilter, + +edge?: PluginFilter, +direction?: "IN" | "OUT" | "ANY", |}; @@ -64,31 +67,66 @@ export interface PluginHandler { export type Plugins = $ReadOnlyArray>; +type MaybeNode = {|+address: Address, +node: Node | 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; - throw new Error("Graphv2 is not yet implemented"); + if (address == null) { + 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 { - const _ = address; - throw new Error("Graphv2 is not yet implemented"); + return this.ref(address).get(); } /** * 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> { - const _ = filter; - throw new Error("Graphv2 is not yet implemented"); + *nodes(filter?: PluginFilter): Iterator> { + for (const maybeNode of this._nodes) { + 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 { @@ -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; - throw new Error("Graphv2 is not yet implemented"); + if (payload == null) { + 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; - throw new Error("Graphv2 is not yet implemented"); + const indexDatum = this._nodeIndices.get(address); + if (indexDatum != null) { + this._nodes[indexDatum.index] = {address, node: undefined}; + } + return this; } addEdge(edge: Edge): 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; - throw new Error("Graphv2 is not yet implemented"); + const theseNodes = Array.from(this.nodes()); + 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}; +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 { + 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|}> { + const _ = options; + throw new Error("Not implemented"); + } +} diff --git a/src/core2/graph.test.js b/src/core2/graph.test.js index 55b3cbf..8364975 100644 --- a/src/core2/graph.test.js +++ b/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>, + expected: Iterable> + ) { + 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); + }); + }); }); diff --git a/src/core2/graphDemoData.js b/src/core2/graphDemoData.js deleted file mode 100644 index f5bfcbc..0000000 --- a/src/core2/graphDemoData.js +++ /dev/null @@ -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> { - 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()];