From 467b78178809affeeee81b231a73b25fc81d80e5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dandelion=20Man=C3=A9?= Date: Sun, 19 May 2019 15:49:24 +0300 Subject: [PATCH] Remove the fallback adapter (#1145) Node and Edge types are increasingly important in SourceCred, as we use them to decide what weights to provide, what description logic to use, etc. In #631, we hit a bug around not finding a type matching a particular node, and in #640 I responded by creating a "FallbackAdapter", which matches any node, and a "Static/DynamicAdapterSet", which are abstractions for finding adapters in a way that guarantees the presence of the fallback adapter. I think this solution is actually pretty brittle. It only works if we are disciplined in only ever accessing node and edge types using a context that included the FallbackAdapter, and it requires us to centralize all of the "type-not-found" logic in one place (the fallback declaration) irrespective of what the locally appropriate solution is. Looking back at #631, the core issue was that we had a getter that promised to always give a matching type and adapter, rather than returning a possibly undefined adapter. We fixed this by attempting to ensure that there always would be a matching adapter. I think it would be better to have the methods honestly report that the adapter might be null/undefined, and let the client code handle it appropriately. This commit implements that change. It's motivated by me being frustrated at the fallback adapter while doing other refactoring. Test plan: Unit tests have been updated and `yarn test` passes. Also, I experimentally removed the Git plugin from the list of adapters for both the backend and frontend, and verified that the frontend UI and the pagerank and export-graph commands still worked. --- src/analysis/fallbackAdapter.js | 21 ------ src/analysis/fallbackDeclaration.js | 12 --- src/analysis/weightsToEdgeEvaluator.js | 8 +- src/analysis/weightsToEdgeEvaluator.test.js | 12 --- src/cli/pagerank.js | 2 - src/cli/pagerank.test.js | 17 +---- src/core/trie.js | 7 +- src/core/trie.test.js | 4 +- src/explorer/adapters/explorerAdapterSet.js | 27 ++++--- .../adapters/explorerAdapterSet.test.js | 74 +++++++++---------- src/explorer/adapters/fallbackAdapter.js | 34 --------- src/explorer/pagerankTable/Table.js | 34 ++++----- src/explorer/pagerankTable/Table.test.js | 11 +-- src/explorer/pagerankTable/aggregate.js | 27 ++++++- src/explorer/pagerankTable/aggregate.test.js | 28 +++++-- src/explorer/pagerankTable/shared.js | 3 + .../weights/PluginWeightConfig.test.js | 20 ----- src/explorer/weights/WeightConfig.js | 74 +++++++++---------- src/explorer/weights/WeightConfig.test.js | 16 ---- 19 files changed, 167 insertions(+), 264 deletions(-) delete mode 100644 src/analysis/fallbackAdapter.js delete mode 100644 src/explorer/adapters/fallbackAdapter.js diff --git a/src/analysis/fallbackAdapter.js b/src/analysis/fallbackAdapter.js deleted file mode 100644 index 29dfc42..0000000 --- a/src/analysis/fallbackAdapter.js +++ /dev/null @@ -1,21 +0,0 @@ -// @flow - -import {Graph} from "../core/graph"; -import type {RepoId} from "../core/repoId"; -import type {PluginDeclaration} from "./pluginDeclaration"; -import type {IAnalysisAdapter} from "./analysisAdapter"; - -import {fallbackDeclaration} from "./fallbackDeclaration"; - -export class FallbackAdapter implements IAnalysisAdapter { - declaration(): PluginDeclaration { - return fallbackDeclaration; - } - - load( - _unused_sourcecredDirectory: string, - _unused_repoId: RepoId - ): Promise { - return Promise.resolve(new Graph()); - } -} diff --git a/src/analysis/fallbackDeclaration.js b/src/analysis/fallbackDeclaration.js index 1ebe692..eb696a7 100644 --- a/src/analysis/fallbackDeclaration.js +++ b/src/analysis/fallbackDeclaration.js @@ -2,12 +2,8 @@ import {NodeAddress, EdgeAddress} from "../core/graph"; -import type {PluginDeclaration} from "./pluginDeclaration"; - import type {EdgeType, NodeType} from "./types"; -export const FALLBACK_NAME = "FALLBACK_ADAPTER"; - export const fallbackNodeType: NodeType = Object.freeze({ name: "node", pluralName: "nodes", @@ -25,11 +21,3 @@ export const fallbackEdgeType: EdgeType = Object.freeze({ description: "The fallback EdgeType for edges which don't have any other type", }); - -export const fallbackDeclaration: PluginDeclaration = Object.freeze({ - name: FALLBACK_NAME, - nodePrefix: NodeAddress.empty, - edgePrefix: EdgeAddress.empty, - nodeTypes: Object.freeze([fallbackNodeType]), - edgeTypes: Object.freeze([fallbackEdgeType]), -}); diff --git a/src/analysis/weightsToEdgeEvaluator.js b/src/analysis/weightsToEdgeEvaluator.js index 85af74e..22f62e2 100644 --- a/src/analysis/weightsToEdgeEvaluator.js +++ b/src/analysis/weightsToEdgeEvaluator.js @@ -20,7 +20,7 @@ export function weightsToEdgeEvaluator( } function nodeWeight(n: NodeAddressT): number { - const typeWeight = nodeTrie.getLast(n); + const typeWeight = NullUtil.orElse(nodeTrie.getLast(n), 1); const manualWeight = NullUtil.orElse(manualWeights.get(n), 1); return typeWeight * manualWeight; } @@ -28,7 +28,11 @@ export function weightsToEdgeEvaluator( return function evaluator(edge: Edge) { const srcWeight = nodeWeight(edge.src); const dstWeight = nodeWeight(edge.dst); - const {forwards, backwards} = edgeTrie.getLast(edge.address); + const edgeWeight = NullUtil.orElse(edgeTrie.getLast(edge.address), { + forwards: 1, + backwards: 1, + }); + const {forwards, backwards} = edgeWeight; return { toWeight: dstWeight * forwards, froWeight: srcWeight * backwards, diff --git a/src/analysis/weightsToEdgeEvaluator.test.js b/src/analysis/weightsToEdgeEvaluator.test.js index ed909ea..a4fb77e 100644 --- a/src/analysis/weightsToEdgeEvaluator.test.js +++ b/src/analysis/weightsToEdgeEvaluator.test.js @@ -1,7 +1,6 @@ // @flow import * as NullUtil from "../util/null"; -import {fallbackNodeType, fallbackEdgeType} from "./fallbackDeclaration"; import { inserterNodeType, machineNodeType, @@ -29,16 +28,12 @@ describe("analysis/weightsToEdgeEvaluator", () => { function weights({ assemblesForward, assemblesBackward, - baseForward, - baseBackward, inserter, machine, - baseNode, }: WeightArgs) { const nodes = [ {weight: NullUtil.orElse(inserter, 1), type: inserterNodeType}, {weight: NullUtil.orElse(machine, 1), type: machineNodeType}, - {weight: NullUtil.orElse(baseNode, 1), type: fallbackNodeType}, ]; const nodesMap = new Map(nodes.map((x) => [x.type.prefix, x])); const edges = [ @@ -49,13 +44,6 @@ describe("analysis/weightsToEdgeEvaluator", () => { }, type: assemblesEdgeType, }, - { - weight: { - forwards: NullUtil.orElse(baseForward, 1), - backwards: NullUtil.orElse(baseBackward, 1), - }, - type: fallbackEdgeType, - }, ]; const edgesMap = new Map(edges.map((x) => [x.type.prefix, x])); return {nodes: nodesMap, edges: edgesMap}; diff --git a/src/cli/pagerank.js b/src/cli/pagerank.js index d1ace65..67b539b 100644 --- a/src/cli/pagerank.js +++ b/src/cli/pagerank.js @@ -28,7 +28,6 @@ import {weightsToEdgeEvaluator} from "../analysis/weightsToEdgeEvaluator"; import type {IAnalysisAdapter} from "../analysis/analysisAdapter"; import {AnalysisAdapter as GithubAnalysisAdapter} from "../plugins/github/analysisAdapter"; import {AnalysisAdapter as GitAnalysisAdapter} from "../plugins/git/analysisAdapter"; -import {FallbackAdapter} from "../analysis/fallbackAdapter"; function usage(print: (string) => void): void { print( @@ -184,7 +183,6 @@ function weightsForAdapters( export const defaultAdapters = () => [ new GithubAnalysisAdapter(), new GitAnalysisAdapter(), - new FallbackAdapter(), ]; const defaultLoader = (r: RepoId) => loadGraph(Common.sourcecredDirectory(), defaultAdapters(), r); diff --git a/src/cli/pagerank.test.js b/src/cli/pagerank.test.js index 5e54ffe..d661e8c 100644 --- a/src/cli/pagerank.test.js +++ b/src/cli/pagerank.test.js @@ -25,11 +25,7 @@ import { DEFAULT_MAX_ITERATIONS, } from "../core/pagerankGraph"; import type {NodeType, EdgeType} from "../analysis/types"; -import {fallbackDeclaration} from "../analysis/fallbackDeclaration"; -import { - defaultWeightsForDeclaration, - combineWeights, -} from "../analysis/weights"; +import {defaultWeightsForDeclaration} from "../analysis/weights"; import {weightsToEdgeEvaluator} from "../analysis/weightsToEdgeEvaluator"; @@ -221,16 +217,7 @@ describe("cli/pagerank", () => { edgeTypes: [edgeType], }; - const exampleWeightedTypes = defaultWeightsForDeclaration( - exampleDeclaration - ); - const fallbackWeightedTypes = defaultWeightsForDeclaration( - fallbackDeclaration - ); - const weightedTypes = combineWeights([ - exampleWeightedTypes, - fallbackWeightedTypes, - ]); + const weightedTypes = defaultWeightsForDeclaration(exampleDeclaration); const manualWeights = new Map(); manualWeights.set(advancedGraph().nodes.src(), 4); diff --git a/src/core/trie.js b/src/core/trie.js index 12329f3..d8f9d7e 100644 --- a/src/core/trie.js +++ b/src/core/trie.js @@ -87,13 +87,10 @@ class BaseTrie { /** * Get the last stored value `v` in the path to key `k`. - * Throws an error if no value is available. + * Returns undefined if no value is available. */ - getLast(k: K): V { + getLast(k: K): ?V { const path = this.get(k); - if (path.length === 0) { - throw new Error("Tried to getLast, but no matching entry existed"); - } return path[path.length - 1]; } } diff --git a/src/core/trie.test.js b/src/core/trie.test.js index 845995d..4b2c651 100644 --- a/src/core/trie.test.js +++ b/src/core/trie.test.js @@ -90,8 +90,8 @@ describe("core/trie", () => { expect(x.getLast(fooBarZod)).toEqual(3); }); - it("getLast throws an error if no value is available", () => { - expect(() => new NodeTrie().getLast(foo)).toThrowError("no matching entry"); + it("getLast returns undefined if no value is available", () => { + expect(new NodeTrie().getLast(foo)).toEqual(undefined); }); it("overwriting a value is illegal", () => { diff --git a/src/explorer/adapters/explorerAdapterSet.js b/src/explorer/adapters/explorerAdapterSet.js index 85346ae..a62f244 100644 --- a/src/explorer/adapters/explorerAdapterSet.js +++ b/src/explorer/adapters/explorerAdapterSet.js @@ -1,4 +1,15 @@ // @flow +/** + * This module is deprecated; logic for aggregating over different plugins, + * and matching adapters for a particular or edge, should live at the + * core plugin level, not at the level of particular adapters. Otherwise, + * we will find ourselves re-implementing the same logic repetitiously for + * every new adapter. + * + * If considering adding new dependencies on this file or adding new features, + * please consider simply re-writing it to live in the analysis module and + * be paramterized over the adapter choice. + */ import {Graph, type NodeAddressT, type EdgeAddressT} from "../../core/graph"; import {NodeTrie, EdgeTrie} from "../../core/trie"; @@ -11,8 +22,6 @@ import type { } from "./explorerAdapter"; import type {EdgeType, NodeType} from "../../analysis/types"; -import {FallbackStaticAdapter} from "./fallbackAdapter"; - export class StaticExplorerAdapterSet { _adapters: $ReadOnlyArray; _adapterNodeTrie: NodeTrie; @@ -21,7 +30,7 @@ export class StaticExplorerAdapterSet { _typeEdgeTrie: EdgeTrie; constructor(adapters: $ReadOnlyArray): void { - this._adapters = [new FallbackStaticAdapter(), ...adapters]; + this._adapters = adapters; this._adapterNodeTrie = new NodeTrie(); this._adapterEdgeTrie = new EdgeTrie(); this._typeNodeTrie = new NodeTrie(); @@ -52,19 +61,19 @@ export class StaticExplorerAdapterSet { return [].concat(...this._adapters.map((x) => x.declaration().edgeTypes)); } - adapterMatchingNode(x: NodeAddressT): StaticExplorerAdapter { + adapterMatchingNode(x: NodeAddressT): ?StaticExplorerAdapter { return this._adapterNodeTrie.getLast(x); } - adapterMatchingEdge(x: EdgeAddressT): StaticExplorerAdapter { + adapterMatchingEdge(x: EdgeAddressT): ?StaticExplorerAdapter { return this._adapterEdgeTrie.getLast(x); } - typeMatchingNode(x: NodeAddressT): NodeType { + typeMatchingNode(x: NodeAddressT): ?NodeType { return this._typeNodeTrie.getLast(x); } - typeMatchingEdge(x: EdgeAddressT): EdgeType { + typeMatchingEdge(x: EdgeAddressT): ?EdgeType { return this._typeEdgeTrie.getLast(x); } @@ -95,11 +104,11 @@ export class DynamicExplorerAdapterSet { }); } - adapterMatchingNode(x: NodeAddressT): DynamicExplorerAdapter { + adapterMatchingNode(x: NodeAddressT): ?DynamicExplorerAdapter { return this._adapterNodeTrie.getLast(x); } - adapterMatchingEdge(x: EdgeAddressT): DynamicExplorerAdapter { + adapterMatchingEdge(x: EdgeAddressT): ?DynamicExplorerAdapter { return this._adapterEdgeTrie.getLast(x); } diff --git a/src/explorer/adapters/explorerAdapterSet.test.js b/src/explorer/adapters/explorerAdapterSet.test.js index 9b3ba01..0f80587 100644 --- a/src/explorer/adapters/explorerAdapterSet.test.js +++ b/src/explorer/adapters/explorerAdapterSet.test.js @@ -3,103 +3,97 @@ import {NodeAddress, EdgeAddress, Graph} from "../../core/graph"; import {FactorioStaticAdapter} from "../../plugins/demo/explorerAdapter"; import {StaticExplorerAdapterSet} from "./explorerAdapterSet"; -import {FallbackStaticAdapter} from "./fallbackAdapter"; -import { - FALLBACK_NAME, - fallbackNodeType, - fallbackEdgeType, -} from "../../analysis/fallbackDeclaration"; import {Assets} from "../../webutil/assets"; import {makeRepoId} from "../../core/repoId"; +import * as NullUtil from "../../util/null"; describe("explorer/adapters/explorerAdapterSet", () => { describe("StaticExplorerAdapterSet", () => { function example() { const x = new FactorioStaticAdapter(); - const fallback = new FallbackStaticAdapter(); const sas = new StaticExplorerAdapterSet([x]); - return {x, fallback, sas}; + return {x, sas}; } it("errors if two plugins have the same name", () => { const x = new FactorioStaticAdapter(); const shouldError = () => new StaticExplorerAdapterSet([x, x]); expect(shouldError).toThrowError("Multiple plugins with name"); }); - it("always includes the fallback plugin", () => { - const {sas} = example(); - expect(sas.adapters()[0].declaration().name).toBe(FALLBACK_NAME); - }); it("includes the manually provided plugin adapters", () => { const {x, sas} = example(); - expect(sas.adapters()[1].declaration().name).toBe(x.declaration().name); + expect(sas.adapters()[0].declaration().name).toBe(x.declaration().name); }); it("aggregates NodeTypes across plugins", () => { const {sas} = example(); const nodeTypes = sas.nodeTypes(); - const expectedNumNodeTypes = - new FactorioStaticAdapter().declaration().nodeTypes.length + - new FallbackStaticAdapter().declaration().nodeTypes.length; + const expectedNumNodeTypes = new FactorioStaticAdapter().declaration() + .nodeTypes.length; expect(nodeTypes).toHaveLength(expectedNumNodeTypes); }); it("aggregates EdgeTypes across plugins", () => { const {sas} = example(); const edgeTypes = sas.edgeTypes(); - const expectedNumEdgeTypes = - new FactorioStaticAdapter().declaration().edgeTypes.length + - new FallbackStaticAdapter().declaration().edgeTypes.length; + const expectedNumEdgeTypes = new FactorioStaticAdapter().declaration() + .edgeTypes.length; expect(edgeTypes).toHaveLength(expectedNumEdgeTypes); }); it("finds adapter matching a node", () => { const {x, sas} = example(); - const matching = sas.adapterMatchingNode( + let matching = sas.adapterMatchingNode( NodeAddress.fromParts(["factorio", "inserter"]) ); + matching = NullUtil.get(matching); expect(matching.declaration().name).toBe(x.declaration().name); }); it("finds adapter matching an edge", () => { const {x, sas} = example(); - const matching = sas.adapterMatchingEdge( + let matching = sas.adapterMatchingEdge( EdgeAddress.fromParts(["factorio", "assembles"]) ); + matching = NullUtil.get(matching); expect(matching.declaration().name).toBe(x.declaration().name); }); - it("finds fallback adapter for unregistered node", () => { + it("finds no adapter for unregistered node", () => { const {sas} = example(); const adapter = sas.adapterMatchingNode(NodeAddress.fromParts(["weird"])); - expect(adapter.declaration().name).toBe(FALLBACK_NAME); + expect(adapter).toBe(undefined); }); - it("finds fallback adapter for unregistered edge", () => { + it("finds no adapter for unregistered edge", () => { const {sas} = example(); const adapter = sas.adapterMatchingEdge(EdgeAddress.fromParts(["weird"])); - expect(adapter.declaration().name).toBe(FALLBACK_NAME); + expect(adapter).toBe(undefined); }); it("finds type matching a node", () => { const {sas} = example(); - const type = sas.typeMatchingNode( - NodeAddress.fromParts(["factorio", "inserter", "1", "foo"]) + const type = NullUtil.get( + sas.typeMatchingNode( + NodeAddress.fromParts(["factorio", "inserter", "1", "foo"]) + ) ); expect(type.name).toBe("inserter"); }); it("finds type matching an edge", () => { const {sas} = example(); - const type = sas.typeMatchingEdge( - EdgeAddress.fromParts(["factorio", "assembles", "other", "1", "foo"]) + const type = NullUtil.get( + sas.typeMatchingEdge( + EdgeAddress.fromParts(["factorio", "assembles", "other", "1", "foo"]) + ) ); expect(type.forwardName).toBe("assembles"); }); - it("finds fallback type for unregistered node", () => { + it("finds no type for unregistered node", () => { const {sas} = example(); const type = sas.typeMatchingNode( NodeAddress.fromParts(["wombat", "1", "foo"]) ); - expect(type).toBe(fallbackNodeType); + expect(type).toBe(undefined); }); - it("finds fallback type for unregistered edge", () => { + it("finds no type for unregistered edge", () => { const {sas} = example(); const type = sas.typeMatchingEdge( EdgeAddress.fromParts(["wombat", "1", "foo"]) ); - expect(type).toBe(fallbackEdgeType); + expect(type).toBe(undefined); }); it("loads a dynamicExplorerAdapterSet", async () => { const {x, sas} = example(); @@ -143,27 +137,29 @@ describe("explorer/adapters/explorerAdapterSet", () => { }); it("finds adapter matching a node", async () => { const {x, das} = await example(); - const matching = das.adapterMatchingNode( + let matching = das.adapterMatchingNode( NodeAddress.fromParts(["factorio", "inserter"]) ); + matching = NullUtil.get(matching); expect(matching.static().declaration().name).toBe(x.declaration().name); }); it("finds adapter matching an edge", async () => { const {x, das} = await example(); - const matching = das.adapterMatchingEdge( + let matching = das.adapterMatchingEdge( EdgeAddress.fromParts(["factorio", "assembles"]) ); + matching = NullUtil.get(matching); expect(matching.static().declaration().name).toBe(x.declaration().name); }); - it("finds fallback adapter for unregistered node", async () => { + it("finds no adapter for unregistered node", async () => { const {das} = await example(); const adapter = das.adapterMatchingNode(NodeAddress.fromParts(["weird"])); - expect(adapter.static().declaration().name).toBe(FALLBACK_NAME); + expect(adapter).toBe(undefined); }); - it("finds fallback adapter for unregistered edge", async () => { + it("finds no adapter for unregistered edge", async () => { const {das} = await example(); const adapter = das.adapterMatchingEdge(EdgeAddress.fromParts(["weird"])); - expect(adapter.static().declaration().name).toBe(FALLBACK_NAME); + expect(adapter).toBe(undefined); }); }); }); diff --git a/src/explorer/adapters/fallbackAdapter.js b/src/explorer/adapters/fallbackAdapter.js deleted file mode 100644 index f382187..0000000 --- a/src/explorer/adapters/fallbackAdapter.js +++ /dev/null @@ -1,34 +0,0 @@ -// @flow - -import {fallbackDeclaration} from "../../analysis/fallbackDeclaration"; -import type { - StaticExplorerAdapter, - DynamicExplorerAdapter, -} from "./explorerAdapter"; -import {Assets} from "../../webutil/assets"; -import {type RepoId} from "../../core/repoId"; -import {Graph, NodeAddress, type NodeAddressT} from "../../core/graph"; - -export class FallbackStaticAdapter implements StaticExplorerAdapter { - declaration() { - return fallbackDeclaration; - } - - load(_unused_assets: Assets, _unused_repoId: RepoId) { - return Promise.resolve(new FallbackDynamicAdapter()); - } -} - -export class FallbackDynamicAdapter implements DynamicExplorerAdapter { - graph() { - return new Graph(); - } - - nodeDescription(x: NodeAddressT) { - return `[fallback]: ${NodeAddress.toString(x)}`; - } - - static(): FallbackStaticAdapter { - return new FallbackStaticAdapter(); - } -} diff --git a/src/explorer/pagerankTable/Table.js b/src/explorer/pagerankTable/Table.js index 573fd0e..91090c3 100644 --- a/src/explorer/pagerankTable/Table.js +++ b/src/explorer/pagerankTable/Table.js @@ -2,18 +2,15 @@ import React from "react"; import sortBy from "lodash.sortby"; -import * as NullUtil from "../../util/null"; import {NodeAddress, type NodeAddressT} from "../../core/graph"; import type {PagerankNodeDecomposition} from "../../analysis/pagerankNodeDecomposition"; import {DynamicExplorerAdapterSet} from "../adapters/explorerAdapterSet"; import type {DynamicExplorerAdapter} from "../adapters/explorerAdapter"; -import {FALLBACK_NAME} from "../../analysis/fallbackDeclaration"; import type {WeightedTypes} from "../../analysis/weights"; import {WeightConfig} from "../weights/WeightConfig"; import {NodeRowList} from "./Node"; import {type NodeType} from "../../analysis/types"; -import {fallbackNodeType} from "../../analysis/fallbackDeclaration"; type PagerankTableProps = {| +pnd: PagerankNodeDecomposition, @@ -26,7 +23,7 @@ type PagerankTableProps = {| +onManualWeightsChange: (NodeAddressT, number) => void, |}; type PagerankTableState = {| - selectedNodeType: NodeType, + selectedNodeTypePrefix: NodeAddressT, showWeightConfig: boolean, |}; export class PagerankTable extends React.PureComponent< @@ -35,11 +32,13 @@ export class PagerankTable extends React.PureComponent< > { constructor(props: PagerankTableProps): void { super(); - const selectedNodeType = NullUtil.orElse( - props.defaultNodeType, - fallbackNodeType - ); - this.state = {selectedNodeType, showWeightConfig: false}; + const {defaultNodeType} = props; + const selectedNodeTypePrefix = + defaultNodeType != null ? defaultNodeType.prefix : NodeAddress.empty; + this.state = { + selectedNodeTypePrefix, + showWeightConfig: false, + }; } renderConfigurationRow() { @@ -110,20 +109,16 @@ export class PagerankTable extends React.PureComponent< ); @@ -140,7 +135,6 @@ export class PagerankTable extends React.PureComponent< if (pnd == null || adapters == null || maxEntriesPerList == null) { throw new Error("Impossible."); } - const selectedNodeTypePrefix = this.state.selectedNodeType.prefix; const sharedProps = { pnd, adapters, @@ -169,7 +163,7 @@ export class PagerankTable extends React.PureComponent< - NodeAddress.hasPrefix(node, selectedNodeTypePrefix) + NodeAddress.hasPrefix(node, this.state.selectedNodeTypePrefix) )} /> diff --git a/src/explorer/pagerankTable/Table.test.js b/src/explorer/pagerankTable/Table.test.js index fc54dde..91f0a89 100644 --- a/src/explorer/pagerankTable/Table.test.js +++ b/src/explorer/pagerankTable/Table.test.js @@ -12,7 +12,6 @@ import {WeightConfig} from "../weights/WeightConfig"; import {defaultWeightsForAdapter} from "../weights/weights"; import {FactorioStaticAdapter} from "../../plugins/demo/explorerAdapter"; import {type NodeType} from "../../analysis/types"; -import {fallbackNodeType} from "../../analysis/fallbackDeclaration"; require("../../webutil/testUtil").configureEnzyme(); describe("explorer/pagerankTable/Table", () => { @@ -146,18 +145,20 @@ describe("explorer/pagerankTable/Table", () => { }); it("filter defaults to show all if defaultNodeType not passed", async () => { const {element} = await setup(); - expect(element.state().selectedNodeType).toEqual(fallbackNodeType); + expect(element.state().selectedNodeTypePrefix).toEqual( + NodeAddress.empty + ); }); - it("selectedNodeType defaults to defaultNodeType if available", async () => { + it("selectedNodeTypePrefix defaults to provided NodeType, if available", async () => { const nodeType: NodeType = { name: "testNodeType", pluralName: "testNodeTypes", - prefix: NodeAddress.empty, + prefix: NodeAddress.fromParts(["foo"]), defaultWeight: 1, description: "test type", }; const {element} = await setup(nodeType); - expect(element.state().selectedNodeType).toEqual(nodeType); + expect(element.state().selectedNodeTypePrefix).toEqual(nodeType.prefix); }); }); diff --git a/src/explorer/pagerankTable/aggregate.js b/src/explorer/pagerankTable/aggregate.js index 1313a85..c6c6a2f 100644 --- a/src/explorer/pagerankTable/aggregate.js +++ b/src/explorer/pagerankTable/aggregate.js @@ -3,8 +3,10 @@ import sortBy from "lodash.sortby"; import stringify from "json-stable-stringify"; import * as MapUtil from "../../util/map"; +import * as NullUtil from "../../util/null"; import {NodeTrie, EdgeTrie} from "../../core/trie"; -import type {EdgeType, NodeType} from "../../analysis/types"; +import {NodeAddress, EdgeAddress} from "../../core/graph"; +import {type EdgeType, type NodeType} from "../../analysis/types"; import type {ScoredConnection} from "../../analysis/pagerankNodeDecomposition"; // Sorted by descending `summary.score` @@ -43,6 +45,22 @@ export type FlatAggregation = {| +connections: $ReadOnlyArray, |}; +export const fallbackNodeType: NodeType = Object.freeze({ + name: "node", + pluralName: "nodes", + prefix: NodeAddress.empty, + defaultWeight: 1, + description: "", +}); + +export const fallbackEdgeType: EdgeType = Object.freeze({ + forwardName: "has edge to", + backwardName: "has edge from", + defaultWeight: Object.freeze({forwards: 1, backwards: 1}), + prefix: EdgeAddress.empty, + description: "", +}); + export function aggregateByNodeType( xs: $ReadOnlyArray, nodeTypes: $ReadOnlyArray @@ -53,7 +71,7 @@ export function aggregateByNodeType( } const nodeTypeToConnections = new Map(); for (const x of xs) { - const type = typeTrie.getLast(x.source); + const type = NullUtil.orElse(typeTrie.getLast(x.source), fallbackNodeType); MapUtil.pushValue(nodeTypeToConnections, type, x); } const aggregations: NodeAggregation[] = []; @@ -105,7 +123,10 @@ export function aggregateByConnectionType( throw new Error((x.connection.adjacency.type: empty)); } const edge = x.connection.adjacency.edge; - const type = typeTrie.getLast(edge.address); + const type = NullUtil.orElse( + typeTrie.getLast(edge.address), + fallbackEdgeType + ); MapUtil.pushValue(relevantMap, type, x); } diff --git a/src/explorer/pagerankTable/aggregate.test.js b/src/explorer/pagerankTable/aggregate.test.js index a92618d..8b6dec6 100644 --- a/src/explorer/pagerankTable/aggregate.test.js +++ b/src/explorer/pagerankTable/aggregate.test.js @@ -8,8 +8,10 @@ import { flattenAggregation, aggregateFlat, aggregationKey, + fallbackEdgeType, + fallbackNodeType, } from "./aggregate"; -import type {NodeType} from "../../analysis/types"; +import {type NodeType} from "../../analysis/types"; describe("explorer/pagerankTable/aggregate", () => { // TODO: If making major modifications to these tests, consider switching @@ -238,10 +240,11 @@ describe("explorer/pagerankTable/aggregate", () => { const aggregations = aggregateByNodeType([], nodeTypesArray); expect(aggregations).toHaveLength(0); }); - it("errors if any connection has no matching type", () => { + it("uses fallbackNodeType if necessary", () => { const {scoredConnectionsArray} = example(); - const shouldFail = () => aggregateByNodeType(scoredConnectionsArray, []); - expect(shouldFail).toThrowError("no matching entry"); + const aggregations = aggregateByNodeType(scoredConnectionsArray, []); + expect(aggregations).toHaveLength(1); + expect(aggregations[0].nodeType).toEqual(fallbackNodeType); }); it("sorts the aggregations by total score", () => { let lastSeenScore = Infinity; @@ -354,11 +357,20 @@ describe("explorer/pagerankTable/aggregate", () => { ); expect(aggregations).toHaveLength(0); }); - it("errors if any connection has no matching type", () => { + it("aggregates with fallback edge type for connections with no type", () => { const {scoredConnectionsArray, nodeTypesArray} = example(); - const shouldFail = () => - aggregateByConnectionType(scoredConnectionsArray, nodeTypesArray, []); - expect(shouldFail).toThrowError("no matching entry"); + const aggregations = aggregateByConnectionType( + scoredConnectionsArray, + nodeTypesArray, + [] + ); + expect(aggregations).toHaveLength(3); + for (const {connectionType} of aggregations) { + if (connectionType.type === "SYNTHETIC_LOOP") { + continue; + } + expect(connectionType.edgeType).toBe(fallbackEdgeType); + } }); it("sorts the aggregations by total score", () => { let lastSeenScore = Infinity; diff --git a/src/explorer/pagerankTable/shared.js b/src/explorer/pagerankTable/shared.js index 2950fcb..7b95909 100644 --- a/src/explorer/pagerankTable/shared.js +++ b/src/explorer/pagerankTable/shared.js @@ -16,6 +16,9 @@ export function nodeDescription( adapters: DynamicExplorerAdapterSet ): ReactNode { const adapter = adapters.adapterMatchingNode(address); + if (adapter == null) { + return NodeAddress.toString(address); + } try { return adapter.nodeDescription(address); } catch (e) { diff --git a/src/explorer/weights/PluginWeightConfig.test.js b/src/explorer/weights/PluginWeightConfig.test.js index d163703..80f4bee 100644 --- a/src/explorer/weights/PluginWeightConfig.test.js +++ b/src/explorer/weights/PluginWeightConfig.test.js @@ -8,10 +8,6 @@ import { assemblesEdgeType, } from "../../plugins/demo/declaration"; import {FactorioStaticAdapter} from "../../plugins/demo/explorerAdapter"; -import { - fallbackNodeType, - fallbackEdgeType, -} from "../../analysis/fallbackDeclaration"; import {NodeTypeConfig} from "./NodeTypeConfig"; import {EdgeTypeConfig} from "./EdgeTypeConfig"; import { @@ -119,22 +115,6 @@ describe("explorer/weights/PluginWeightConfig", () => { badTypes.edges.delete(assemblesEdgeType.prefix); checkError(badTypes); }); - it("extra node prefix in weighted types", () => { - const badTypes = defaultWeightsForAdapter(new FactorioStaticAdapter()); - badTypes.nodes.set(fallbackNodeType.prefix, { - weight: 5, - type: fallbackNodeType, - }); - checkError(badTypes); - }); - it("extra edge prefix in weighted types", () => { - const badTypes = defaultWeightsForAdapter(new FactorioStaticAdapter()); - badTypes.edges.set(fallbackEdgeType.prefix, { - weight: {forwards: 5, backwards: 3}, - type: fallbackEdgeType, - }); - checkError(badTypes); - }); }); }); }); diff --git a/src/explorer/weights/WeightConfig.js b/src/explorer/weights/WeightConfig.js index 2ada83c..6709f06 100644 --- a/src/explorer/weights/WeightConfig.js +++ b/src/explorer/weights/WeightConfig.js @@ -7,7 +7,6 @@ import * as MapUtil from "../../util/map"; import type {StaticExplorerAdapterSet} from "../adapters/explorerAdapterSet"; import type {WeightedTypes} from "../../analysis/weights"; import {PluginWeightConfig} from "./PluginWeightConfig"; -import {FALLBACK_NAME} from "../../analysis/fallbackDeclaration"; type Props = {| +adapters: StaticExplorerAdapterSet, @@ -37,47 +36,44 @@ export class WeightConfig extends React.Component { } _renderPluginWeightConfigs(): ReactNode { - return this.props.adapters - .adapters() - .filter((x) => x.declaration().name !== FALLBACK_NAME) - .map((adapter) => { - const onChange = (weightedTypes) => { - const newWeightedTypes = { - nodes: MapUtil.copy(this.props.weightedTypes.nodes), - edges: MapUtil.copy(this.props.weightedTypes.edges), - }; - for (const [key, val] of weightedTypes.nodes.entries()) { - newWeightedTypes.nodes.set(key, val); - } - for (const [key, val] of weightedTypes.edges.entries()) { - newWeightedTypes.edges.set(key, val); - } - this.props.onChange(newWeightedTypes); + return this.props.adapters.adapters().map((adapter) => { + const onChange = (weightedTypes) => { + const newWeightedTypes = { + nodes: MapUtil.copy(this.props.weightedTypes.nodes), + edges: MapUtil.copy(this.props.weightedTypes.edges), }; - const pluginScopedWeightedTypes = { - nodes: new Map(), - edges: new Map(), - }; - for (const {prefix} of adapter.declaration().nodeTypes) { - pluginScopedWeightedTypes.nodes.set( - prefix, - NullUtil.get(this.props.weightedTypes.nodes.get(prefix)) - ); + for (const [key, val] of weightedTypes.nodes.entries()) { + newWeightedTypes.nodes.set(key, val); } - for (const {prefix} of adapter.declaration().edgeTypes) { - pluginScopedWeightedTypes.edges.set( - prefix, - NullUtil.get(this.props.weightedTypes.edges.get(prefix)) - ); + for (const [key, val] of weightedTypes.edges.entries()) { + newWeightedTypes.edges.set(key, val); } - return ( - + this.props.onChange(newWeightedTypes); + }; + const pluginScopedWeightedTypes = { + nodes: new Map(), + edges: new Map(), + }; + for (const {prefix} of adapter.declaration().nodeTypes) { + pluginScopedWeightedTypes.nodes.set( + prefix, + NullUtil.get(this.props.weightedTypes.nodes.get(prefix)) ); - }); + } + for (const {prefix} of adapter.declaration().edgeTypes) { + pluginScopedWeightedTypes.edges.set( + prefix, + NullUtil.get(this.props.weightedTypes.edges.get(prefix)) + ); + } + return ( + + ); + }); } } diff --git a/src/explorer/weights/WeightConfig.test.js b/src/explorer/weights/WeightConfig.test.js index c240991..51973ff 100644 --- a/src/explorer/weights/WeightConfig.test.js +++ b/src/explorer/weights/WeightConfig.test.js @@ -8,7 +8,6 @@ import { staticExplorerAdapterSet, } from "../../plugins/demo/explorerAdapter"; import {inserterNodeType} from "../../plugins/demo/declaration"; -import {FALLBACK_NAME} from "../../analysis/fallbackDeclaration"; import {defaultWeightsForAdapterSet, defaultWeightsForAdapter} from "./weights"; import {WeightConfig} from "./WeightConfig"; @@ -33,21 +32,6 @@ describe("explorer/weights/WeightConfig", () => { ); return {el, adapters, types, onChange}; } - it("creates a PluginWeightConfig for every non-fallback adapter", () => { - const {el, adapters} = example(); - const pwcs = el.find(PluginWeightConfig); - expect(pwcs).toHaveLength(adapters.adapters().length - 1); - for (const adapter of adapters.adapters()) { - if (adapter.declaration().name === FALLBACK_NAME) { - continue; - } - const pwc = pwcs.findWhere( - (x) => - x.props().adapter.declaration().name === adapter.declaration().name - ); - expect(pwc).toHaveLength(1); - } - }); it("sets the PluginWeightConfig weights properly", () => { const {el} = example(); const pwc = el