status-im
/
sourcecred
mirror of https://github.com/status-im/sourcecred.git
2
0
Fork 0
Code Issues Projects Releases Wiki Activity

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
```
This commit is contained in:
Dandelion Mané 2018-05-28 15:36:38 -07:00 committed by GitHub
parent 7562453f02
commit 41ab7aa729
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
6 changed files with 76 additions and 64 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

24
src/core2/__snapshots__/address.test.js.snap
View File

@ -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",
"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",
"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",
"owner": Object {
"plugin": "houseville",
"type": "BUSINESS",
},
"{\\"id\\":\\"mattressStore\\",\\"pluginName\\":\\"houseville\\",\\"type\\":\\"BUSINESS\\"}",
},
"{\\"id\\":\\"mattressStore\\",\\"owner\\":{\\"plugin\\":\\"houseville\\",\\"type\\":\\"BUSINESS\\"}}",
],
]
`;

58
src/core2/address.js
View File

@ -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<T: Addressable> = 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<T: Addressable> {
*
* 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<T: Addressable> {
* 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<T: Addressable> {
toJSON(): AddressMapJSON<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) => {
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<T: Addressable> {
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<T: Addressable> {
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<T: Addressable> {
*/
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<T: Addressable> {
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;

9
src/core2/address.test.js
View File

@ -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));
});

28
src/core2/address_payload_unification_design.md
View File

@ -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<GithubNodeReference, GithubNodePayload<any>> {
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);

10
src/core2/graph.js
View File

@ -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<NR: NodeReference, NP: NodePayload> = {|
@ -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<Node<any, any>> {
nodes(filter?: PluginType): Iterator<Node<any, any>> {
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<Edge<any>> {
edges(filter?: PluginType): Iterator<Edge<any>> {
const _ = filter;
throw new Error("Graphv2 is not yet implemented");
}

5
src/core2/graphDemoData.js
View File

@ -12,7 +12,7 @@ export const PLUGIN_NAME = "sourcecred/demo/cooking";
export class Handler implements PluginHandler<DemoReference, DemoPayload<any>> {
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),
};
}