From 41ab7aa7290e1c6dabee3e66b5632ffd1d6810ed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dandelion=20Man=C3=A9?= Date: Mon, 28 May 2018 15:36:38 -0700 Subject: [PATCH] Create an explicit `PluginType` (#310) Currently, when filtering by type (e.g. in neighbors), we require only a type string. This is a design flaw, as if two plugins both define an "ISSUE" type, either plugin may unexpectedly receive the other plugin's nodes or edges. We fix the flaw by explicitly binding the plugin name and type field together as `PluginType`. Test plan: Flow passes, and so does the following invocation to check our design doc: ``` cat <(printf '// @flow\n') \ <(awk -v RS='```[a-z]*' '(NR+1)%2' \ src/core2/address_payload_unification_design.md) \ | yarn -s flow check-contents ``` --- src/core2/__snapshots__/address.test.js.snap | 30 ++++++---- src/core2/address.js | 58 ++++++++++--------- src/core2/address.test.js | 9 +-- .../address_payload_unification_design.md | 28 +++++---- src/core2/graph.js | 10 ++-- src/core2/graphDemoData.js | 5 +- 6 files changed, 76 insertions(+), 64 deletions(-) diff --git a/src/core2/__snapshots__/address.test.js.snap b/src/core2/__snapshots__/address.test.js.snap index 6327ecc..b42ea19 100644 --- a/src/core2/__snapshots__/address.test.js.snap +++ b/src/core2/__snapshots__/address.test.js.snap @@ -4,14 +4,14 @@ exports[`address AddressMap stringifies to JSON 1`] = ` Array [ Object { "type": "sourcecred/sourcecred/AddressMap", - "version": "0.1.0", + "version": "0.2.0", }, Object { - "{\\"id\\":\\"mansion\\",\\"pluginName\\":\\"houseville\\",\\"type\\":\\"HOME\\"}": Object { + "{\\"id\\":\\"mansion\\",\\"owner\\":{\\"plugin\\":\\"houseville\\",\\"type\\":\\"HOME\\"}}": Object { "baths": 5, "beds": 10, }, - "{\\"id\\":\\"mattressStore\\",\\"pluginName\\":\\"houseville\\",\\"type\\":\\"BUSINESS\\"}": Object { + "{\\"id\\":\\"mattressStore\\",\\"owner\\":{\\"plugin\\":\\"houseville\\",\\"type\\":\\"BUSINESS\\"}}": Object { "baths": 1, "beds": 99, }, @@ -24,26 +24,32 @@ Array [ Array [ Object { "id": "mansion", - "pluginName": "houseville", - "type": "HOME", + "owner": Object { + "plugin": "houseville", + "type": "HOME", + }, }, - "{\\"id\\":\\"mansion\\",\\"pluginName\\":\\"houseville\\",\\"type\\":\\"HOME\\"}", + "{\\"id\\":\\"mansion\\",\\"owner\\":{\\"plugin\\":\\"houseville\\",\\"type\\":\\"HOME\\"}}", ], Array [ Object { "id": "mansion", - "pluginName": "houseville", - "type": "HOME", + "owner": Object { + "plugin": "houseville", + "type": "HOME", + }, }, - "{\\"id\\":\\"mansion\\",\\"pluginName\\":\\"houseville\\",\\"type\\":\\"HOME\\"}", + "{\\"id\\":\\"mansion\\",\\"owner\\":{\\"plugin\\":\\"houseville\\",\\"type\\":\\"HOME\\"}}", ], Array [ Object { "id": "mattressStore", - "pluginName": "houseville", - "type": "BUSINESS", + "owner": Object { + "plugin": "houseville", + "type": "BUSINESS", + }, }, - "{\\"id\\":\\"mattressStore\\",\\"pluginName\\":\\"houseville\\",\\"type\\":\\"BUSINESS\\"}", + "{\\"id\\":\\"mattressStore\\",\\"owner\\":{\\"plugin\\":\\"houseville\\",\\"type\\":\\"BUSINESS\\"}}", ], ] `; diff --git a/src/core2/address.js b/src/core2/address.js index 9dbcbc0..828f245 100644 --- a/src/core2/address.js +++ b/src/core2/address.js @@ -6,10 +6,11 @@ import stringify from "json-stable-stringify"; import {toCompat, fromCompat} from "../util/compat"; import type {Compatible} from "../util/compat"; +export type PluginType = {|+plugin: string, +type: string|}; + export type Address = {| - +pluginName: string, + +owner: PluginType, +id: string, - +type: string, |}; export interface Addressable { @@ -23,7 +24,7 @@ export type AddressMapJSON = Compatible<{ }>; export const COMPAT_TYPE = "sourcecred/sourcecred/AddressMap"; -export const COMPAT_VERSION = "0.1.0"; +export const COMPAT_VERSION = "0.2.0"; /** * A data structure for storing addressable objects, keyed by their @@ -35,7 +36,7 @@ export class AddressMap { * * It is an representation invariant that there are no empty objects * in this structure (except possibly the top-level object). In - * particular, if `_data[somePluginName]` is not `undefined`, then it + * particular, if `_data[somePlugin]` is not `undefined`, then it * is a non-empty object. This is required so that two `AddressMap`s * are logically equal exactly if their `_data` fields are deep-equal. * @@ -44,7 +45,7 @@ export class AddressMap { * For basic performance tests, see: * https://jsperf.com/address-string-302039074 */ - _data: {[pluginName: string]: {[type: string]: {[id: string]: T}}}; + _data: {[plugin: string]: {[type: string]: {[id: string]: T}}}; /** * Create an empty `AddressMap`. @@ -64,12 +65,13 @@ export class AddressMap { toJSON(): AddressMapJSON { const result = {}; - Object.keys(this._data).forEach((pluginName) => { - const dataForPluginName = this._data[pluginName]; - Object.keys(dataForPluginName).forEach((type) => { - const dataForType = dataForPluginName[type]; + Object.keys(this._data).forEach((plugin) => { + const dataForPlugin = this._data[plugin]; + Object.keys(dataForPlugin).forEach((type) => { + const dataForType = dataForPlugin[type]; Object.keys(dataForType).forEach((id) => { - const address = {pluginName, id, type}; + const owner = {plugin, type}; + const address = {owner, id}; const datum = {...dataForType[id]}; delete datum.address; result[toString(address)] = datum; @@ -111,13 +113,13 @@ export class AddressMap { if (address == null) { throw new Error(`address is ${String(t.address)}`); } - let dataForPluginName = this._data[address.pluginName]; - if (dataForPluginName === undefined) { - this._data[address.pluginName] = dataForPluginName = {}; + let dataForPlugin = this._data[address.owner.plugin]; + if (dataForPlugin === undefined) { + this._data[address.owner.plugin] = dataForPlugin = {}; } - let dataForType = dataForPluginName[address.type]; + let dataForType = dataForPlugin[address.owner.type]; if (dataForType === undefined) { - dataForPluginName[address.type] = dataForType = {}; + dataForPlugin[address.owner.type] = dataForType = {}; } dataForType[address.id] = t; return this; @@ -131,11 +133,11 @@ export class AddressMap { if (address == null) { throw new Error(`address is ${String(address)}`); } - const dataForPluginName = this._data[address.pluginName]; - if (dataForPluginName === undefined) { + const dataForPlugin = this._data[address.owner.plugin]; + if (dataForPlugin === undefined) { return (undefined: any); } - const dataForType = dataForPluginName[address.type]; + const dataForType = dataForPlugin[address.owner.type]; if (dataForType === undefined) { return (undefined: any); } @@ -147,10 +149,10 @@ export class AddressMap { */ getAll(): T[] { const result = []; - Object.keys(this._data).forEach((pluginName) => { - const dataForPluginName = this._data[pluginName]; - Object.keys(dataForPluginName).forEach((type) => { - const dataForType = dataForPluginName[type]; + Object.keys(this._data).forEach((plugin) => { + const dataForPlugin = this._data[plugin]; + Object.keys(dataForPlugin).forEach((type) => { + const dataForType = dataForPlugin[type]; Object.keys(dataForType).forEach((id) => { result.push(dataForType[id]); }); @@ -167,19 +169,19 @@ export class AddressMap { if (address == null) { throw new Error(`address is ${String(address)}`); } - const dataForPluginName = this._data[address.pluginName]; - if (dataForPluginName === undefined) { + const dataForPlugin = this._data[address.owner.plugin]; + if (dataForPlugin === undefined) { return this; } - const dataForType = dataForPluginName[address.type]; + const dataForType = dataForPlugin[address.owner.type]; if (dataForType === undefined) { return this; } delete dataForType[address.id]; if (Object.keys(dataForType).length === 0) { - delete dataForPluginName[address.type]; - if (Object.keys(dataForPluginName).length === 0) { - delete this._data[address.pluginName]; + delete dataForPlugin[address.owner.type]; + if (Object.keys(dataForPlugin).length === 0) { + delete this._data[address.owner.plugin]; } } return this; diff --git a/src/core2/address.test.js b/src/core2/address.test.js index 03b9923..ae0206e 100644 --- a/src/core2/address.test.js +++ b/src/core2/address.test.js @@ -22,9 +22,8 @@ describe("address", () => { |}; function makeAddress(type: "HOME" | "BUSINESS", id: string): Address { return { - pluginName: "houseville", + owner: {plugin: "houseville", type}, id, - type, }; } const mansion = (): House => ({ @@ -167,14 +166,12 @@ describe("address", () => { }); it("Order of insertion does not matter", () => { const a1 = { - pluginName: "foo", - type: "bar", + owner: {plugin: "foo", type: "bar"}, id: "zoombat", }; const a2 = { id: "zoombat", - type: "bar", - pluginName: "foo", + owner: {type: "bar", plugin: "foo"}, }; expect(toString(a1)).toEqual(toString(a2)); }); diff --git a/src/core2/address_payload_unification_design.md b/src/core2/address_payload_unification_design.md index eaab9b7..be8beba 100644 --- a/src/core2/address_payload_unification_design.md +++ b/src/core2/address_payload_unification_design.md @@ -17,16 +17,25 @@ point. ## Core structures -The `Address` type is, for now, unchanged: +The `Address` type is modified to have an explicit `PluginType`. ```javascript -type Address = {| - +pluginName: string, +type PluginType = {| + +plugin: string, +type: string, +|}; + +type Address = {| + +owner: PluginType, +id: string, |}; ``` +(This fixes a design flaw where callers would frequently filter only on +`type`, naively assuming that all instances of that type were generated +by plugins the author was aware of. Filtering options now take a +`PluginType` rather than a raw `type`.) + At the core are two interfaces. First is `NodePayload`: ```javascript @@ -52,8 +61,7 @@ class GithubNodePayload<+T: GithubNodeRecord> implements NodePayload { } address() { return { - pluginName: "sourcecred/github-beta", - type: this._type, + owner: {plugin: "sourcecred/github-beta", type: this._type}, id: this.url(), }; } @@ -219,8 +227,8 @@ class GithubNodeReference extends DelegateNodeReference { constructor(base: NodeReference) { super(base); const address = base.address(); - if (address.pluginName !== "sourcecred/github-beta") { - throw new Error("pluginName: " + address.pluginName); + if (address.owner.plugin !== "sourcecred/github-beta") { + throw new Error("plugin: " + address.owner.plugin); } } } @@ -283,10 +291,10 @@ class GithubPluginHandler implements PluginHandler> { createReference(reference) { const address = reference.address(); - if (address.pluginName !== GITHUB_PLUGIN_NAME) { - throw new Error("pluginName: " + address.pluginName); + if (address.owner.plugin !== GITHUB_PLUGIN_NAME) { + throw new Error("pluginName: " + address.owner.plugin); } - const type: GithubNodeType = ((address.type: string): any); + const type: GithubNodeType = ((address.owner.type: string): any); switch (type) { case "PULL_REQUEST": return new PullRequestReference(reference); diff --git a/src/core2/graph.js b/src/core2/graph.js index a0dd098..69f3921 100644 --- a/src/core2/graph.js +++ b/src/core2/graph.js @@ -1,6 +1,6 @@ // @flow -import type {Address} from "./address"; +import type {Address, PluginType} from "./address"; import type {Compatible} from "../util/compat"; export type Node = {| @@ -38,8 +38,8 @@ export type Edge<+T> = {| |}; export type NeighborsOptions = {| - +nodeType?: string, - +edgeType?: string, + +node?: PluginType, + +edge?: PluginType, +direction?: "IN" | "OUT" | "ANY", |}; @@ -86,7 +86,7 @@ export class Graph { * * If filter is provided, it will return only nodes with the requested type. */ - nodes(filter?: {|+type?: string|}): Iterator> { + nodes(filter?: PluginType): Iterator> { const _ = filter; throw new Error("Graphv2 is not yet implemented"); } @@ -101,7 +101,7 @@ export class Graph { * * If filter is provided, it will return only edges with the requested type. */ - edges(filter?: {|+type?: string|}): Iterator> { + edges(filter?: PluginType): Iterator> { const _ = filter; throw new Error("Graphv2 is not yet implemented"); } diff --git a/src/core2/graphDemoData.js b/src/core2/graphDemoData.js index 57c0e66..f5bfcbc 100644 --- a/src/core2/graphDemoData.js +++ b/src/core2/graphDemoData.js @@ -12,7 +12,7 @@ export const PLUGIN_NAME = "sourcecred/demo/cooking"; export class Handler implements PluginHandler> { createReference(ref: NodeReference) { - switch (ref.address().type) { + switch (ref.address().owner.type) { case "PC": return new HeroReference(ref); case "INGREDIENT": @@ -61,8 +61,7 @@ export class DemoPayload<+T> implements NodePayload { address() { return { - pluginName: PLUGIN_NAME, - type: this._type, + owner: {plugin: PLUGIN_NAME, type: this._type}, id: String(this._id), }; }