From 8921b5b942f6b9d51604e4578a15f046d3783f0b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dandelion=20Man=C3=A9?= Date: Thu, 5 Jul 2018 14:06:20 -0700 Subject: [PATCH] Display edges in Cred Explorer (#489) Previously, when expanding a node in the cred explorer, it would display the neighboring nodes, but not any information about the edges linking to that node. If the same node was reached by multiple edges, this information was not communicated to the user. As of this commit, it now concisely communicates what kind of edge was connecting the chosen node to its adjacencies. There's a new `edgeVerb` method that plugin adapters must implement, which gives a direction-based verb descriptiong of the edge, e.g. "authors" or "is authored by". Test plan: Unit tests added to the PagerankTable tests, and hand inspection. Paired with @wchargin --- src/app/credExplorer/PagerankTable.js | 159 +++++++++++--- src/app/credExplorer/PagerankTable.test.js | 202 ++++++++++++------ .../__snapshots__/PagerankTable.test.js.snap | 11 +- src/app/pluginAdapter.js | 4 +- src/plugins/git/pluginAdapter.js | 9 +- src/plugins/git/render.js | 22 ++ src/plugins/github/pluginAdapter.js | 9 +- src/plugins/github/render.js | 20 ++ 8 files changed, 332 insertions(+), 104 deletions(-) diff --git a/src/app/credExplorer/PagerankTable.js b/src/app/credExplorer/PagerankTable.js index 5ac76eb..2434ba2 100644 --- a/src/app/credExplorer/PagerankTable.js +++ b/src/app/credExplorer/PagerankTable.js @@ -7,8 +7,11 @@ import { Graph, NodeAddress, type NodeAddressT, + type Neighbor, Direction, + type Edge, EdgeAddress, + type EdgeAddressT, } from "../../core/graph"; import type {PagerankResult} from "../../core/attribution/pagerank"; import type {PluginAdapter} from "../pluginAdapter"; @@ -48,6 +51,44 @@ export function nodeDescription( } } +function edgeVerb( + address: EdgeAddressT, + direction: "FORWARD" | "BACKWARD", + adapters: $ReadOnlyArray +): string { + const adapter = adapters.find((adapter) => + EdgeAddress.hasPrefix(address, adapter.edgePrefix()) + ); + if (adapter == null) { + const result = EdgeAddress.toString(address); + console.warn(`No adapter for ${result}`); + return result; + } + + try { + return adapter.renderer().edgeVerb(address, direction); + } catch (e) { + const result = EdgeAddress.toString(address); + console.error(`Error getting description for ${result}: ${e.message}`); + return result; + } +} + +export function neighborVerb( + {node, edge}: Neighbor, + adapters: $ReadOnlyArray +): string { + const forwardVerb = edgeVerb(edge.address, "FORWARD", adapters); + const backwardVerb = edgeVerb(edge.address, "BACKWARD", adapters); + if (edge.src === edge.dst) { + return `${forwardVerb} and ${backwardVerb}`; + } else if (edge.dst === node) { + return forwardVerb; + } else { + return backwardVerb; + } +} + export class PagerankTable extends React.PureComponent { constructor() { super(); @@ -132,7 +173,7 @@ export class PagerankTable extends React.PureComponent { - { type RTState = {expanded: boolean}; type RTProps = {| - +address: NodeAddressT, + +node: NodeAddressT, + // Present if this RT shows a neighbor (not a top-level node) + +edge: ?Edge, +graph: Graph, +pagerankResult: PagerankResult, +depth: number, @@ -167,13 +210,15 @@ class RecursiveTable extends React.PureComponent { } render() { - const {address, adapters, depth, graph, pagerankResult} = this.props; + const {node, edge, adapters, depth, graph, pagerankResult} = this.props; const {expanded} = this.state; - const probability = pagerankResult.get(address); + const probability = pagerankResult.get(node); if (probability == null) { - throw new Error(`no PageRank value for ${NodeAddress.toString(address)}`); + throw new Error(`no PageRank value for ${NodeAddress.toString(node)}`); } const modifiedLogScore = Math.log(probability) + 10; + const edgeVerbString = + edge == null ? null : neighborVerb({node, edge}, adapters); return [ { > {expanded ? "\u2212" : "+"} - {nodeDescription(address, adapters)} + + {edgeVerbString != null && ( + + + {edgeVerbString} + {" "} + + )} + {nodeDescription(node, adapters)} + {modifiedLogScore.toFixed(2)} , expanded && ( - neighbor.node) - ) + neighbors={Array.from( + graph.neighbors(node, { + direction: Direction.ANY, + nodePrefix: NodeAddress.empty, + edgePrefix: EdgeAddress.empty, + }) )} graph={graph} pagerankResult={pagerankResult} @@ -221,7 +278,7 @@ class RecursiveTable extends React.PureComponent { } } -type RecursiveTablesProps = {| +type NodesTablesProps = {| +addresses: $ReadOnlyArray, +graph: Graph, +pagerankResult: PagerankResult, @@ -229,27 +286,26 @@ type RecursiveTablesProps = {| +adapters: $ReadOnlyArray, |}; -class RecursiveTables extends React.PureComponent { +class NodesTables extends React.PureComponent { render() { const {addresses, graph, pagerankResult, depth, adapters} = this.props; - return addresses - .slice() - .sort((a, b) => { - const x = pagerankResult.get(a); - const y = pagerankResult.get(b); - if (x == null) { - throw new Error(`No pagerank result for ${NodeAddress.toString(a)}`); + return sortBy( + addresses, + (x) => { + const p = pagerankResult.get(x); + if (p == null) { + throw new Error(`No pagerank result for ${NodeAddress.toString(x)}`); } - if (y == null) { - throw new Error(`No pagerank result for ${NodeAddress.toString(b)}`); - } - return y - x; - }) + return -p; + }, + (x) => x + ) .slice(0, MAX_TABLE_ENTRIES) .map((address) => ( { )); } } + +type NeighborsTablesProps = {| + +neighbors: $ReadOnlyArray, + +graph: Graph, + +pagerankResult: PagerankResult, + +depth: number, + +adapters: $ReadOnlyArray, +|}; + +class NeighborsTables extends React.PureComponent { + render() { + const {neighbors, graph, pagerankResult, depth, adapters} = this.props; + return sortBy( + neighbors, + ({node}) => { + const p = pagerankResult.get(node); + if (p == null) { + throw new Error( + `No pagerank result for ${NodeAddress.toString(node)}` + ); + } + return -p; + }, + ({edge}) => edge + ) + .slice(0, MAX_TABLE_ENTRIES) + .map(({node, edge}) => ( + + )); + } +} diff --git a/src/app/credExplorer/PagerankTable.test.js b/src/app/credExplorer/PagerankTable.test.js index 35b586a..687002b 100644 --- a/src/app/credExplorer/PagerankTable.test.js +++ b/src/app/credExplorer/PagerankTable.test.js @@ -3,13 +3,14 @@ import React from "react"; import {mount, shallow} from "enzyme"; import enzymeToJSON from "enzyme-to-json"; -import {PagerankTable, nodeDescription} from "./PagerankTable"; +import {PagerankTable, nodeDescription, neighborVerb} from "./PagerankTable"; import {pagerank} from "../../core/attribution/pagerank"; import sortBy from "lodash.sortby"; import { Graph, type NodeAddressT, + Direction, NodeAddress, EdgeAddress, } from "../../core/graph"; @@ -31,14 +32,17 @@ function example() { function addEdge(parts, src, dst) { const edge = {address: EdgeAddress.fromParts(parts), src, dst}; graph.addEdge(edge); + return edge; } - addEdge(["a"], nodes.fooAlpha, nodes.fooBeta); - addEdge(["b"], nodes.fooAlpha, nodes.bar1); - addEdge(["c"], nodes.fooAlpha, nodes.xox); - addEdge(["d"], nodes.bar1, nodes.bar1); - addEdge(["e"], nodes.bar1, nodes.xox); - addEdge(["e'"], nodes.bar1, nodes.xox); + const edges = { + fooA: addEdge(["foo", "a"], nodes.fooAlpha, nodes.fooBeta), + fooB: addEdge(["foo", "b"], nodes.fooAlpha, nodes.bar1), + fooC: addEdge(["foo", "c"], nodes.fooAlpha, nodes.xox), + barD: addEdge(["bar", "d"], nodes.bar1, nodes.bar1), + barE: addEdge(["bar", "e"], nodes.bar1, nodes.xox), + barF: addEdge(["bar", "f"], nodes.bar1, nodes.xox), + }; const adapters = [ { @@ -48,8 +52,11 @@ function example() { }, renderer: () => ({ nodeDescription: (x) => `foo: ${NodeAddress.toString(x)}`, + edgeVerb: (_unused_e, direction) => + direction === "FORWARD" ? "foos" : "is fooed by", }), nodePrefix: () => NodeAddress.fromParts(["foo"]), + edgePrefix: () => EdgeAddress.fromParts(["foo"]), nodeTypes: () => [ {name: "alpha", prefix: NodeAddress.fromParts(["foo", "a"])}, {name: "beta", prefix: NodeAddress.fromParts(["foo", "b"])}, @@ -62,8 +69,11 @@ function example() { }, renderer: () => ({ nodeDescription: (x) => `bar: ${NodeAddress.toString(x)}`, + edgeVerb: (_unused_e, direction) => + direction === "FORWARD" ? "bars" : "is barred by", }), nodePrefix: () => NodeAddress.fromParts(["bar"]), + edgePrefix: () => EdgeAddress.fromParts(["bar"]), nodeTypes: () => [ {name: "alpha", prefix: NodeAddress.fromParts(["bar", "a"])}, ], @@ -75,8 +85,10 @@ function example() { }, renderer: () => ({ nodeDescription: (_unused_arg) => `xox node!`, + edgeVerb: (_unused_e, _unused_direction) => `xox'd`, }), nodePrefix: () => NodeAddress.fromParts(["xox"]), + edgePrefix: () => EdgeAddress.fromParts(["xox"]), nodeTypes: () => [], }, { @@ -88,6 +100,7 @@ function example() { throw new Error("Impossible!"); }, nodePrefix: () => NodeAddress.fromParts(["unused"]), + edgePrefix: () => EdgeAddress.fromParts(["unused"]), nodeTypes: () => [], }, ]; @@ -97,7 +110,7 @@ function example() { froWeight: 1, })); - return {adapters, nodes, graph, pagerankResult}; + return {adapters, nodes, edges, graph, pagerankResult}; } describe("app/credExplorer/PagerankTable", () => { @@ -165,7 +178,7 @@ describe("app/credExplorer/PagerankTable", () => { describe("full rendering", () => { function exampleRender() { - const {nodes, adapters, graph, pagerankResult} = example(); + const {nodes, edges, adapters, graph, pagerankResult} = example(); const element = mount( { verifyNoAdapterWarning(); const select = element.find("select"); expect(select).toHaveLength(1); - return {nodes, adapters, graph, pagerankResult, element, select}; + return {nodes, edges, adapters, graph, pagerankResult, element, select}; } it("full render doesn't crash or error", () => { example(); }); - describe("tables ", () => { - it("are sorted by score", () => { - const {element, graph, pagerankResult} = exampleRender(); - const rows = element.find("RecursiveTable"); - expect(rows).toHaveLength(Array.from(graph.nodes()).length); - const scores = rows.map((x) => pagerankResult.get(x.prop("address"))); - expect(scores).toEqual(sortBy(scores).reverse()); - }); + describe("tables", () => { function expectColumnCorrect( element: *, name: string, @@ -206,54 +212,114 @@ describe("app/credExplorer/PagerankTable", () => { const actual = tables.map((x) => tdToExpected(x.find("td").at(headerIndex)) ); - const expected = tables.map((x) => - addressToExpected(x.prop("address")) - ); + const expected = tables.map((x) => addressToExpected(x.prop("node"))); expect(actual).toEqual(expected); } - it("has a node description column", () => { - const {element, adapters} = exampleRender(); - expectColumnCorrect( - element, - "Node", - (td) => td.find("span").text(), - (address) => nodeDescription(address, adapters) - ); - verifyNoAdapterWarning(); - }); - it("has a log score column", () => { - const {element, pagerankResult} = exampleRender(); - expectColumnCorrect( - element, - "log(score)", - (td) => td.text(), - (address) => { - const probability = pagerankResult.get(address); - if (probability == null) { - throw new Error(address); - } - const modifiedLogScore = Math.log(probability) + 10; - return modifiedLogScore.toFixed(2); - } - ); - }); - it("subtables have depth-based styling", () => { - const {element} = exampleRender(); - const getLevel = (level) => { - const rt = element.find("RecursiveTable").at(level); - const button = rt.find("button").first(); - return {rt, button}; - }; - getLevel(0).button.simulate("click"); - getLevel(1).button.simulate("click"); - const f = ({rt, button}) => ({ - row: rt - .find("tr") - .first() - .prop("style"), - button: button.prop("style"), + describe("top-level", () => { + it("are sorted by score", () => { + const {element, graph, pagerankResult} = exampleRender(); + const rows = element.find("RecursiveTable"); + expect(rows).toHaveLength(Array.from(graph.nodes()).length); + const scores = rows.map((x) => pagerankResult.get(x.prop("node"))); + expect(scores.every((x) => x != null)).toBe(true); + expect(scores).toEqual(sortBy(scores).reverse()); + }); + it("has a node description column", () => { + const {element, adapters} = exampleRender(); + expectColumnCorrect( + element, + "Node", + (td) => td.find("span").text(), + (address) => nodeDescription(address, adapters) + ); + verifyNoAdapterWarning(); + }); + it("has a log score column", () => { + const {element, pagerankResult} = exampleRender(); + expectColumnCorrect( + element, + "log(score)", + (td) => td.text(), + (address) => { + const probability = pagerankResult.get(address); + if (probability == null) { + throw new Error(address); + } + const modifiedLogScore = Math.log(probability) + 10; + return modifiedLogScore.toFixed(2); + } + ); + }); + }); + describe("sub-tables", () => { + it("have depth-based styling", () => { + const {element} = exampleRender(); + const getLevel = (level) => { + const rt = element.find("RecursiveTable").at(level); + const button = rt.find("button").first(); + return {rt, button}; + }; + getLevel(0).button.simulate("click"); + getLevel(1).button.simulate("click"); + const f = ({rt, button}) => ({ + row: rt + .find("tr") + .first() + .prop("style"), + button: button.prop("style"), + }); + expect([0, 1, 2].map((x) => f(getLevel(x)))).toMatchSnapshot(); + }); + it("display extra information about edges", () => { + const {element, nodes, graph, adapters} = exampleRender(); + const getLevel = (level) => { + const rt = element.find("RecursiveTable").at(level); + const button = rt.find("button").first(); + return {rt, button}; + }; + getLevel(0).button.simulate("click"); + const nt = element.find("NeighborsTables"); + expect(nt).toHaveLength(1); + const expectedNeighbors = Array.from( + graph.neighbors(nodes.bar1, { + direction: Direction.ANY, + nodePrefix: NodeAddress.empty, + edgePrefix: EdgeAddress.empty, + }) + ); + expect(nt.prop("neighbors")).toEqual(expectedNeighbors); + const subTables = nt.find("RecursiveTable"); + expect(subTables).toHaveLength(expectedNeighbors.length); + const actualEdgeVerbs = subTables.map((x) => + x + .find("span") + .children() + .find("span") + .text() + ); + const expectedEdgeVerbs = subTables.map((x) => { + const edge = x.prop("edge"); + const node = x.prop("node"); + return neighborVerb({edge, node}, adapters); + }); + + expect(actualEdgeVerbs).toEqual(expectedEdgeVerbs); + const actualFullDescriptions = subTables.map((x) => + x + .find("span") + .first() + .text() + ); + const expectedFullDescriptions = subTables.map((x) => { + const edge = x.prop("edge"); + const node = x.prop("node"); + const nd = nodeDescription(node, adapters); + const ev = neighborVerb({node, edge}, adapters); + return `${ev} ${nd}`; + }); + expect(actualFullDescriptions).toEqual(expectedFullDescriptions); + expect(actualFullDescriptions).toMatchSnapshot(); }); - expect([0, 1, 2].map((x) => f(getLevel(x)))).toMatchSnapshot(); }); it("button toggles between +/- and adds sub-RecursiveTable", () => { const {element} = exampleRender(); @@ -261,15 +327,15 @@ describe("app/credExplorer/PagerankTable", () => { const button = rt().find("button"); expect(button).toEqual(expect.anything()); expect(button.text()).toEqual("+"); - expect(rt().find("RecursiveTables")).toHaveLength(0); + expect(rt().find("NeighborsTables")).toHaveLength(0); button.simulate("click"); expect(button.text()).toEqual("\u2212"); - expect(rt().find("RecursiveTables")).toHaveLength(1); + expect(rt().find("NeighborsTables")).toHaveLength(1); button.simulate("click"); expect(button.text()).toEqual("+"); - expect(rt().find("RecursiveTables")).toHaveLength(0); + expect(rt().find("NeighborsTables")).toHaveLength(0); }); }); @@ -305,7 +371,7 @@ describe("app/credExplorer/PagerankTable", () => { selectFilterByName(select, "\u2003beta"); const rt = element.find("RecursiveTable"); expect(rt).toHaveLength(1); - expect(rt.prop("address")).toEqual(example().nodes.fooBeta); + expect(rt.prop("node")).toEqual(example().nodes.fooBeta); }); it("filter doesn't apply to sub-tables", () => { const {select, element} = exampleRender(); @@ -319,7 +385,7 @@ describe("app/credExplorer/PagerankTable", () => { const rts = element.find("RecursiveTable"); expect(rts).toHaveLength(2); const subRt = rts.last(); - expect(subRt.prop("address")).toEqual(example().nodes.fooAlpha); + expect(subRt.prop("node")).toEqual(example().nodes.fooAlpha); }); }); }); diff --git a/src/app/credExplorer/__snapshots__/PagerankTable.test.js.snap b/src/app/credExplorer/__snapshots__/PagerankTable.test.js.snap index 1aace0d..cd52614 100644 --- a/src/app/credExplorer/__snapshots__/PagerankTable.test.js.snap +++ b/src/app/credExplorer/__snapshots__/PagerankTable.test.js.snap @@ -53,7 +53,16 @@ Array [ ] `; -exports[`app/credExplorer/PagerankTable full rendering tables subtables have depth-based styling 1`] = ` +exports[`app/credExplorer/PagerankTable full rendering tables sub-tables display extra information about edges 1`] = ` +Array [ + "bars and is barred by bar: NodeAddress[\\"bar\\",\\"a\\",\\"1\\"]", + "bars xox node!", + "bars xox node!", + "is fooed by foo: NodeAddress[\\"foo\\",\\"a\\",\\"1\\"]", +] +`; + +exports[`app/credExplorer/PagerankTable full rendering tables sub-tables have depth-based styling 1`] = ` Array [ Object { "button": Object { diff --git a/src/app/pluginAdapter.js b/src/app/pluginAdapter.js index 0da3665..9578d03 100644 --- a/src/app/pluginAdapter.js +++ b/src/app/pluginAdapter.js @@ -1,9 +1,10 @@ // @flow -import type {Graph, NodeAddressT} from "../core/graph"; +import type {Graph, NodeAddressT, EdgeAddressT} from "../core/graph"; export interface Renderer { nodeDescription(NodeAddressT): string; + edgeVerb(EdgeAddressT, "FORWARD" | "BACKWARD"): string; } export interface PluginAdapter { @@ -11,6 +12,7 @@ export interface PluginAdapter { graph(): Graph; renderer(): Renderer; nodePrefix(): NodeAddressT; + edgePrefix(): EdgeAddressT; nodeTypes(): Array<{| +name: string, +prefix: NodeAddressT, diff --git a/src/plugins/git/pluginAdapter.js b/src/plugins/git/pluginAdapter.js index 493c527..306e8c2 100644 --- a/src/plugins/git/pluginAdapter.js +++ b/src/plugins/git/pluginAdapter.js @@ -5,7 +5,8 @@ import type { } from "../../app/pluginAdapter"; import {Graph} from "../../core/graph"; import * as N from "./nodes"; -import {description} from "./render"; +import * as E from "./edges"; +import {description, edgeVerb} from "./render"; export async function createPluginAdapter( repoOwner: string, @@ -38,6 +39,9 @@ class PluginAdapter implements IPluginAdapter { nodePrefix() { return N._Prefix.base; } + edgePrefix() { + return E._Prefix.base; + } nodeTypes() { return [ {name: "Blob", prefix: N._Prefix.blob}, @@ -55,4 +59,7 @@ class Renderer implements IRenderer { const address = N.fromRaw((node: any)); return description(address); } + edgeVerb(edgeAddress, direction) { + return edgeVerb(E.fromRaw((edgeAddress: any)), direction); + } } diff --git a/src/plugins/git/render.js b/src/plugins/git/render.js index 04f0bee..56913e2 100644 --- a/src/plugins/git/render.js +++ b/src/plugins/git/render.js @@ -1,6 +1,7 @@ // @flow import * as N from "./nodes"; +import * as E from "./edges"; export function description(address: N.StructuredAddress) { switch (address.type) { @@ -18,3 +19,24 @@ export function description(address: N.StructuredAddress) { throw new Error(`unknown type: ${(address.type: empty)}`); } } + +export function edgeVerb( + address: E.StructuredAddress, + direction: "FORWARD" | "BACKWARD" +) { + const forward = direction === "FORWARD"; + switch (address.type) { + case "HAS_TREE": + return forward ? "has tree" : "owned by"; + case "HAS_PARENT": + return forward ? "has parent" : "is parent of"; + case "INCLUDES": + return forward ? "includes" : "is included by"; + case "BECOMES": + return forward ? "evolves to" : "evolves from"; + case "HAS_CONTENTS": + return forward ? "has contents" : "is contents of"; + default: + throw new Error(`unknown type: ${(address.type: empty)}`); + } +} diff --git a/src/plugins/github/pluginAdapter.js b/src/plugins/github/pluginAdapter.js index 9466374..c75b2a9 100644 --- a/src/plugins/github/pluginAdapter.js +++ b/src/plugins/github/pluginAdapter.js @@ -6,8 +6,9 @@ import type { import {type Graph, NodeAddress} from "../../core/graph"; import {createGraph} from "./createGraph"; import * as N from "./nodes"; +import * as E from "./edges"; import {RelationalView} from "./relationalView"; -import {description} from "./render"; +import {description, edgeVerb} from "./render"; export async function createPluginAdapter( repoOwner: string, @@ -43,6 +44,9 @@ class PluginAdapter implements IPluginAdapter { nodePrefix() { return N._Prefix.base; } + edgePrefix() { + return E._Prefix.base; + } nodeTypes() { return [ {name: "Repository", prefix: N._Prefix.repo}, @@ -70,4 +74,7 @@ class Renderer implements IRenderer { } return description(entity); } + edgeVerb(edgeAddress, direction) { + return edgeVerb(E.fromRaw((edgeAddress: any)), direction); + } } diff --git a/src/plugins/github/render.js b/src/plugins/github/render.js index a41c18d..8209643 100644 --- a/src/plugins/github/render.js +++ b/src/plugins/github/render.js @@ -1,6 +1,7 @@ // @flow import * as R from "./relationalView"; +import * as E from "./edges"; export function description(e: R.Entity) { const withAuthors = (x: R.AuthoredEntity) => { @@ -24,3 +25,22 @@ export function description(e: R.Entity) { }; return R.match(handlers, e); } + +export function edgeVerb( + e: E.StructuredAddress, + direction: "FORWARD" | "BACKWARD" +) { + const forward = direction === "FORWARD"; + switch (e.type) { + case "AUTHORS": + return forward ? "authors" : "is authored by"; + case "MERGED_AS": + return forward ? "merges" : "is merged by"; + case "HAS_PARENT": + return forward ? "has parent" : "has child"; + case "REFERENCES": + return forward ? "references" : "is referenced by"; + default: + throw new Error(`Unexpected type ${(e.type: empty)}`); + } +}