From 2a3984444531764a91443b6a8e3b1d84aef5e538 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dandelion=20Man=C3=A9?= Date: Mon, 23 Jul 2018 15:37:14 -0700 Subject: [PATCH] Split PluginAdapter into Dynamic and Static parts (#513) In some cases (e.g. WeightConfig) we want to have information from the PluginAdapater before loading any data from the server. In other cases, we need to combine the PluginAdapater with actual data, e.g. so we can get the description of a GitHub node. To support this, we split the PluginAdapter into a Static and Dynamic component. The Dynamic component has data needed to give node descriptions, etc. Given a static adapter, you can get a promise to load the dynamic adapter. Given the dynamic adapter, you can immediately get the static adapter. (There's a parallel to NodeReference (static) and NodePorcelain (dynamic)). Test plan: Travis passes, as does manual testing of the frontend. --- src/app/credExplorer/App.js | 26 +++--- src/app/credExplorer/PagerankTable.js | 42 ++++++---- src/app/credExplorer/PagerankTable.test.js | 98 +++++++++++++--------- src/app/pluginAdapter.js | 11 ++- src/plugins/git/pluginAdapter.js | 66 ++++++++------- src/plugins/github/pluginAdapter.js | 80 ++++++++++-------- 6 files changed, 184 insertions(+), 139 deletions(-) diff --git a/src/app/credExplorer/App.js b/src/app/credExplorer/App.js index d701975..ad19543 100644 --- a/src/app/credExplorer/App.js +++ b/src/app/credExplorer/App.js @@ -4,12 +4,12 @@ import React from "react"; import {StyleSheet, css} from "aphrodite/no-important"; import LocalStore from "./LocalStore"; -import {createPluginAdapter as createGithubAdapter} from "../../plugins/github/pluginAdapter"; -import {createPluginAdapter as createGitAdapter} from "../../plugins/git/pluginAdapter"; +import {StaticPluginAdapter as GithubAdapter} from "../../plugins/github/pluginAdapter"; +import {StaticPluginAdapter as GitAdapter} from "../../plugins/git/pluginAdapter"; import {Graph} from "../../core/graph"; import {pagerank} from "../../core/attribution/pagerank"; import {PagerankTable} from "./PagerankTable"; -import type {PluginAdapter} from "../pluginAdapter"; +import type {DynamicPluginAdapter} from "../pluginAdapter"; import {type EdgeEvaluator} from "../../core/attribution/pagerank"; import {WeightConfig} from "./WeightConfig"; import type {PagerankNodeDecomposition} from "../../core/attribution/pagerankNodeDecomposition"; @@ -23,7 +23,7 @@ type State = { data: {| graphWithMetadata: ?{| +graph: Graph, - +adapters: $ReadOnlyArray, + +adapters: $ReadOnlyArray, +nodeCount: number, +edgeCount: number, |}, @@ -146,17 +146,19 @@ export default class App extends React.Component { return; } - const githubPromise = createGithubAdapter(repoOwner, repoName).then( - (adapter) => { + const githubPromise = new GithubAdapter() + .load(repoOwner, repoName) + .then((adapter) => { const graph = adapter.graph(); return {graph, adapter}; - } - ); + }); - const gitPromise = createGitAdapter(repoOwner, repoName).then((adapter) => { - const graph = adapter.graph(); - return {graph, adapter}; - }); + const gitPromise = new GitAdapter() + .load(repoOwner, repoName) + .then((adapter) => { + const graph = adapter.graph(); + return {graph, adapter}; + }); Promise.all([gitPromise, githubPromise]).then((graphsAndAdapters) => { const graph = Graph.merge(graphsAndAdapters.map((x) => x.graph)); diff --git a/src/app/credExplorer/PagerankTable.js b/src/app/credExplorer/PagerankTable.js index 1f91ee7..8a2bb8b 100644 --- a/src/app/credExplorer/PagerankTable.js +++ b/src/app/credExplorer/PagerankTable.js @@ -14,16 +14,16 @@ import type { ScoredContribution, } from "../../core/attribution/pagerankNodeDecomposition"; import type {Contribution} from "../../core/attribution/graphToMarkovChain"; -import type {PluginAdapter} from "../pluginAdapter"; +import type {DynamicPluginAdapter} from "../pluginAdapter"; import * as NullUtil from "../../util/null"; // TODO: Factor this out and test it (#465) export function nodeDescription( address: NodeAddressT, - adapters: $ReadOnlyArray + adapters: $ReadOnlyArray ): string { const adapter = adapters.find((adapter) => - NodeAddress.hasPrefix(address, adapter.nodePrefix()) + NodeAddress.hasPrefix(address, adapter.static().nodePrefix()) ); if (adapter == null) { const result = NodeAddress.toString(address); @@ -43,10 +43,10 @@ export function nodeDescription( function edgeVerb( address: EdgeAddressT, direction: "FORWARD" | "BACKWARD", - adapters: $ReadOnlyArray + adapters: $ReadOnlyArray ): string { const adapter = adapters.find((adapter) => - EdgeAddress.hasPrefix(address, adapter.edgePrefix()) + EdgeAddress.hasPrefix(address, adapter.static().edgePrefix()) ); if (adapter == null) { const result = EdgeAddress.toString(address); @@ -55,6 +55,7 @@ function edgeVerb( } const edgeType = adapter + .static() .edgeTypes() .find(({prefix}) => EdgeAddress.hasPrefix(address, prefix)); if (edgeType == null) { @@ -72,13 +73,13 @@ function scoreDisplay(probability: number) { type SharedProps = {| +pnd: PagerankNodeDecomposition, - +adapters: $ReadOnlyArray, + +adapters: $ReadOnlyArray, +maxEntriesPerList: number, |}; type PagerankTableProps = {| +pnd: ?PagerankNodeDecomposition, - +adapters: ?$ReadOnlyArray, + +adapters: ?$ReadOnlyArray, +maxEntriesPerList: number, |}; type PagerankTableState = {|topLevelFilter: NodeAddressT|}; @@ -116,21 +117,24 @@ export class PagerankTable extends React.PureComponent< throw new Error("Impossible."); } - function optionGroup(adapter: PluginAdapter) { + function optionGroup(adapter: DynamicPluginAdapter) { const header = ( ); - const entries = adapter.nodeTypes().map((type) => ( - - )); + const entries = adapter + .static() + .nodeTypes() + .map((type) => ( + + )); return [header, ...entries]; } return ( @@ -143,7 +147,9 @@ export class PagerankTable extends React.PureComponent< }} > - {sortBy(adapters, (a) => a.name()).map(optionGroup)} + {sortBy(adapters, (a: DynamicPluginAdapter) => a.static().name()).map( + optionGroup + )} ); @@ -362,7 +368,7 @@ export class ContributionRow extends React.PureComponent< export class ContributionView extends React.PureComponent<{| +contribution: Contribution, - +adapters: $ReadOnlyArray, + +adapters: $ReadOnlyArray, |}> { render() { const {contribution, adapters} = this.props; diff --git a/src/app/credExplorer/PagerankTable.test.js b/src/app/credExplorer/PagerankTable.test.js index 02e2ce3..0e7e252 100644 --- a/src/app/credExplorer/PagerankTable.test.js +++ b/src/app/credExplorer/PagerankTable.test.js @@ -56,71 +56,87 @@ function example() { const adapters = [ { - name: () => "foo", + static: () => ({ + name: () => "foo", + nodePrefix: () => NodeAddress.fromParts(["foo"]), + edgePrefix: () => EdgeAddress.fromParts(["foo"]), + nodeTypes: () => [ + {name: "alpha", prefix: NodeAddress.fromParts(["foo", "a"])}, + {name: "beta", prefix: NodeAddress.fromParts(["foo", "b"])}, + ], + edgeTypes: () => [ + { + prefix: EdgeAddress.fromParts(["foo"]), + forwardName: "foos", + backwardName: "is fooed by", + }, + ], + load: (_unused_repoOwner, _unused_repoName) => { + throw new Error("unused"); + }, + }), graph: () => { throw new Error("unused"); }, - renderer: () => ({ - edgeVerb: (_unused_e, direction) => - direction === "FORWARD" ? "foos" : "is fooed by", - }), nodeDescription: (x) => `foo: ${NodeAddress.toString(x)}`, - nodePrefix: () => NodeAddress.fromParts(["foo"]), - edgePrefix: () => EdgeAddress.fromParts(["foo"]), - nodeTypes: () => [ - {name: "alpha", prefix: NodeAddress.fromParts(["foo", "a"])}, - {name: "beta", prefix: NodeAddress.fromParts(["foo", "b"])}, - ], - edgeTypes: () => [ - { - prefix: EdgeAddress.fromParts(["foo"]), - forwardName: "foos", - backwardName: "is fooed by", - }, - ], }, { - name: () => "bar", + static: () => ({ + name: () => "bar", + nodePrefix: () => NodeAddress.fromParts(["bar"]), + edgePrefix: () => EdgeAddress.fromParts(["bar"]), + nodeTypes: () => [ + {name: "alpha", prefix: NodeAddress.fromParts(["bar", "a"])}, + ], + edgeTypes: () => [ + { + prefix: EdgeAddress.fromParts(["bar"]), + forwardName: "bars", + backwardName: "is barred by", + }, + ], + load: (_unused_repoOwner, _unused_repoName) => { + throw new Error("unused"); + }, + }), graph: () => { throw new Error("unused"); }, nodeDescription: (x) => `bar: ${NodeAddress.toString(x)}`, - nodePrefix: () => NodeAddress.fromParts(["bar"]), - edgePrefix: () => EdgeAddress.fromParts(["bar"]), - nodeTypes: () => [ - {name: "alpha", prefix: NodeAddress.fromParts(["bar", "a"])}, - ], - edgeTypes: () => [ - { - prefix: EdgeAddress.fromParts(["bar"]), - forwardName: "bars", - backwardName: "is barred by", - }, - ], }, { - name: () => "xox", + static: () => ({ + name: () => "xox", + nodePrefix: () => NodeAddress.fromParts(["xox"]), + edgePrefix: () => EdgeAddress.fromParts(["xox"]), + nodeTypes: () => [], + edgeTypes: () => [], + load: (_unused_repoOwner, _unused_repoName) => { + throw new Error("unused"); + }, + }), graph: () => { throw new Error("unused"); }, nodeDescription: (_unused_arg) => `xox node!`, - nodePrefix: () => NodeAddress.fromParts(["xox"]), - edgePrefix: () => EdgeAddress.fromParts(["xox"]), - nodeTypes: () => [], - edgeTypes: () => [], }, { - name: () => "unused", + static: () => ({ + nodePrefix: () => NodeAddress.fromParts(["unused"]), + edgePrefix: () => EdgeAddress.fromParts(["unused"]), + nodeTypes: () => [], + edgeTypes: () => [], + name: () => "unused", + load: (_unused_repoOwner, _unused_repoName) => { + throw new Error("unused"); + }, + }), graph: () => { throw new Error("unused"); }, nodeDescription: () => { throw new Error("Unused"); }, - nodePrefix: () => NodeAddress.fromParts(["unused"]), - edgePrefix: () => EdgeAddress.fromParts(["unused"]), - nodeTypes: () => [], - edgeTypes: () => [], }, ]; diff --git a/src/app/pluginAdapter.js b/src/app/pluginAdapter.js index 37f5cb6..796f608 100644 --- a/src/app/pluginAdapter.js +++ b/src/app/pluginAdapter.js @@ -2,19 +2,24 @@ import type {Graph, NodeAddressT, EdgeAddressT} from "../core/graph"; -export interface PluginAdapter { +export interface StaticPluginAdapter { name(): string; - graph(): Graph; nodePrefix(): NodeAddressT; edgePrefix(): EdgeAddressT; nodeTypes(): Array<{| +name: string, +prefix: NodeAddressT, |}>; - nodeDescription(NodeAddressT): string; edgeTypes(): Array<{| +forwardName: string, +backwardName: string, +prefix: EdgeAddressT, |}>; + load(repoOwner: string, repoName: string): Promise; +} + +export interface DynamicPluginAdapter { + graph(): Graph; + nodeDescription(NodeAddressT): string; + static (): StaticPluginAdapter; } diff --git a/src/plugins/git/pluginAdapter.js b/src/plugins/git/pluginAdapter.js index 72bcb5e..8b22760 100644 --- a/src/plugins/git/pluginAdapter.js +++ b/src/plugins/git/pluginAdapter.js @@ -1,47 +1,23 @@ // @flow -import type {PluginAdapter as IPluginAdapter} from "../../app/pluginAdapter"; +import type { + StaticPluginAdapter as IStaticPluginAdapter, + DynamicPluginAdapter as IDynamicPluginAdapter, +} from "../../app/pluginAdapter"; import {Graph} from "../../core/graph"; import * as N from "./nodes"; import * as E from "./edges"; import {description} from "./render"; -export async function createPluginAdapter( - repoOwner: string, - repoName: string -): Promise { - const url = `/api/v1/data/data/${repoOwner}/${repoName}/git/graph.json`; - const response = await fetch(url); - if (!response.ok) { - return Promise.reject(response); - } - const json = await response.json(); - const graph = Graph.fromJSON(json); - return new PluginAdapter(graph); -} - -class PluginAdapter implements IPluginAdapter { - +_graph: Graph; - constructor(graph: Graph) { - this._graph = graph; - } +export class StaticPluginAdapter implements IStaticPluginAdapter { name() { return "Git"; } - graph() { - return this._graph; - } nodePrefix() { return N._Prefix.base; } edgePrefix() { return E._Prefix.base; } - nodeDescription(node) { - // This cast is unsound, and might throw at runtime, but won't have - // silent failures or cause problems down the road. - const address = N.fromRaw((node: any)); - return description(address); - } nodeTypes() { return [ {name: "Blob", prefix: N._Prefix.blob}, @@ -79,4 +55,36 @@ class PluginAdapter implements IPluginAdapter { }, ]; } + async load( + repoOwner: string, + repoName: string + ): Promise { + const url = `/api/v1/data/data/${repoOwner}/${repoName}/git/graph.json`; + const response = await fetch(url); + if (!response.ok) { + return Promise.reject(response); + } + const json = await response.json(); + const graph = Graph.fromJSON(json); + return new DynamicPluginAdapter(graph); + } +} + +class DynamicPluginAdapter implements IDynamicPluginAdapter { + +_graph: Graph; + constructor(graph: Graph) { + this._graph = graph; + } + graph() { + return this._graph; + } + nodeDescription(node) { + // This cast is unsound, and might throw at runtime, but won't have + // silent failures or cause problems down the road. + const address = N.fromRaw((node: any)); + return description(address); + } + static() { + return new StaticPluginAdapter(); + } } diff --git a/src/plugins/github/pluginAdapter.js b/src/plugins/github/pluginAdapter.js index 5283276..7a5d6dd 100644 --- a/src/plugins/github/pluginAdapter.js +++ b/src/plugins/github/pluginAdapter.js @@ -1,5 +1,8 @@ // @flow -import type {PluginAdapter as IPluginAdapter} from "../../app/pluginAdapter"; +import type { + StaticPluginAdapter as IStaticPluginAdapter, + DynamicPluginAdapter as IDynamicPluginAdapater, +} from "../../app/pluginAdapter"; import {type Graph, NodeAddress} from "../../core/graph"; import {createGraph} from "./createGraph"; import * as N from "./nodes"; @@ -7,44 +10,10 @@ import * as E from "./edges"; import {RelationalView} from "./relationalView"; import {description} from "./render"; -export async function createPluginAdapter( - repoOwner: string, - repoName: string -): Promise { - const url = `/api/v1/data/data/${repoOwner}/${repoName}/github/view.json`; - const response = await fetch(url); - if (!response.ok) { - return Promise.reject(response); - } - const json = await response.json(); - const view = RelationalView.fromJSON(json); - const graph = createGraph(view); - return new PluginAdapter(view, graph); -} - -class PluginAdapter implements IPluginAdapter { - +_view: RelationalView; - +_graph: Graph; - constructor(view: RelationalView, graph: Graph) { - this._view = view; - this._graph = graph; - } +export class StaticPluginAdapter implements IStaticPluginAdapter { name() { return "GitHub"; } - nodeDescription(node) { - // This cast is unsound, and might throw at runtime, but won't have - // silent failures or cause problems down the road. - const address = N.fromRaw((node: any)); - const entity = this._view.entity(address); - if (entity == null) { - throw new Error(`unknown entity: ${NodeAddress.toString(node)}`); - } - return description(entity); - } - graph() { - return this._graph; - } nodePrefix() { return N._Prefix.base; } @@ -85,4 +54,43 @@ class PluginAdapter implements IPluginAdapter { }, ]; } + async load( + repoOwner: string, + repoName: string + ): Promise { + const url = `/api/v1/data/data/${repoOwner}/${repoName}/github/view.json`; + const response = await fetch(url); + if (!response.ok) { + return Promise.reject(response); + } + const json = await response.json(); + const view = RelationalView.fromJSON(json); + const graph = createGraph(view); + return new DynamicPluginAdapter(view, graph); + } +} + +class DynamicPluginAdapter implements IDynamicPluginAdapater { + +_view: RelationalView; + +_graph: Graph; + constructor(view: RelationalView, graph: Graph) { + this._view = view; + this._graph = graph; + } + nodeDescription(node) { + // This cast is unsound, and might throw at runtime, but won't have + // silent failures or cause problems down the road. + const address = N.fromRaw((node: any)); + const entity = this._view.entity(address); + if (entity == null) { + throw new Error(`unknown entity: ${NodeAddress.toString(node)}`); + } + return description(entity); + } + graph() { + return this._graph; + } + static() { + return new StaticPluginAdapter(); + } }