Create a Repo type and use throughout the project (#555)

Our data model orients on getting repos from GitHub, which are
alternatively represented as strings like "sourcecred/sourcecred", or
pairs of variables representing the owner and name, or objects with
owner and name properties. We also have a few different implementations
of repo validation, which are not applied consistently.

This commit changes all that. We now have a consistent Repo type which
is an object containing a string owner and string name. Thanks to a
clever suggestion by @wchargin, it is implemented as an opaque subtype
of an object containing those properties, so that the only valid way to
construct a Repo typed object is to use one of the functions that
consistently validates the repo.

As a fly-by fix, I noticed that there were some functions in the GitHub
query generation that didn't properly mark arguments as readOnly. I've
fixed these.

Test plan: No externally-observable behavior changes (except insofar as
there is a slight change in variable names in the GitHub graphql query,
which has also resulted in a snapshot diff). `yarn travis --full`
passes. `git grep repoOwner` presents no hits.

src/app/credExplorer/App.js

@@ -17,7 +17,7 @@ import {type EdgeEvaluator} from "../../core/attribution/pagerank";
 import {WeightConfig} from "./WeightConfig";
 import type {PagerankNodeDecomposition} from "../../core/attribution/pagerankNodeDecomposition";
 import RepositorySelect from "./RepositorySelect";
-import type {Repo} from "./repoRegistry";
+import type {Repo} from "../../core/repo";
 
 import * as NullUtil from "../../util/null";
 
@@ -135,18 +135,16 @@ export class App extends React.Component<Props, State> {
     }
 
     const githubPromise = new GithubAdapter()
-      .load(selectedRepo.owner, selectedRepo.name)
+      .load(selectedRepo)
       .then((adapter) => {
         const graph = adapter.graph();
         return {graph, adapter};
       });
 
-    const gitPromise = new GitAdapter()
-      .load(selectedRepo.owner, selectedRepo.name)
-      .then((adapter) => {
-        const graph = adapter.graph();
-        return {graph, adapter};
-      });
+    const gitPromise = new GitAdapter().load(selectedRepo).then((adapter) => {
+      const graph = adapter.graph();
+      return {graph, adapter};
+    });
 
     Promise.all([gitPromise, githubPromise]).then((graphsAndAdapters) => {
       const graph = Graph.merge(graphsAndAdapters.map((x) => x.graph));

src/app/credExplorer/PagerankTable.test.js

@@ -81,7 +81,7 @@ async function example() {
             backwardName: "is fooed by",
           },
         ],
-        load: (_unused_repoOwner, _unused_repoName) => {
+        load: (_unused_repo) => {
           throw new Error("unused");
         },
       }),
@@ -109,7 +109,7 @@ async function example() {
             backwardName: "is barred by",
           },
         ],
-        load: (_unused_repoOwner, _unused_repoName) => {
+        load: (_unused_repo) => {
           throw new Error("unused");
         },
       }),
@@ -125,7 +125,7 @@ async function example() {
         edgePrefix: () => EdgeAddress.fromParts(["xox"]),
         nodeTypes: () => [],
         edgeTypes: () => [],
-        load: (_unused_repoOwner, _unused_repoName) => {
+        load: (_unused_repo) => {
           throw new Error("unused");
         },
       }),
@@ -141,7 +141,7 @@ async function example() {
         nodeTypes: () => [],
         edgeTypes: () => [],
         name: () => "unused",
-        load: (_unused_repoOwner, _unused_repoName) => {
+        load: (_unused_repo) => {
           throw new Error("unused");
         },
       }),

src/app/credExplorer/RepositorySelect.js

@@ -7,7 +7,8 @@ import deepEqual from "lodash.isequal";
 import * as NullUtil from "../../util/null";
 import type {LocalStore} from "../localStore";
 
-import {type Repo, fromJSON, REPO_REGISTRY_API} from "./repoRegistry";
+import {fromJSON, REPO_REGISTRY_API} from "./repoRegistry";
+import {type Repo, stringToRepo, repoToString} from "../../core/repo";
 export const REPO_KEY = "selectedRepository";
 
 export type Status =
@@ -66,26 +67,6 @@ export default class RepositorySelect extends React.Component<Props, State> {
   }
 }
 
-function validateRepo(repo: Repo) {
-  const validRe = /^[A-Za-z0-9_-]+$/;
-  if (!repo.owner.match(validRe)) {
-    throw new Error(`Invalid repository owner: ${JSON.stringify(repo.owner)}`);
-  }
-  if (!repo.name.match(validRe)) {
-    throw new Error(`Invalid repository name: ${JSON.stringify(repo.name)}`);
-  }
-}
-
-function repoStringToRepo(x: string): Repo {
-  const pieces = x.split("/");
-  if (pieces.length !== 2) {
-    throw new Error(`Invalid repo string: ${x}`);
-  }
-  const repo = {owner: pieces[0], name: pieces[1]};
-  validateRepo(repo);
-  return repo;
-}
-
 export async function loadStatus(localStore: LocalStore): Promise<Status> {
   try {
     const response = await fetch(REPO_REGISTRY_API);
@@ -147,15 +128,14 @@ export class PureRepositorySelect extends React.PureComponent<
         <span>Please choose a repository to inspect:</span>{" "}
         {selectedRepo != null && (
           <select
-            value={`${selectedRepo.owner}/${selectedRepo.name}`}
+            value={repoToString(selectedRepo)}
             onChange={(e) => {
-              const repoString = e.target.value;
-              const repo = repoStringToRepo(repoString);
+              const repo = stringToRepo(e.target.value);
               this.props.onChange(repo);
             }}
           >
-            {availableRepos.map(({owner, name}) => {
-              const repoString = `${owner}/${name}`;
+            {availableRepos.map((repo) => {
+              const repoString = repoToString(repo);
               return (
                 <option value={repoString} key={repoString}>
                   {repoString}

src/app/credExplorer/RepositorySelect.test.js

@@ -13,6 +13,7 @@ import RepositorySelect, {
 } from "./RepositorySelect";
 
 import {toJSON, type RepoRegistry, REPO_REGISTRY_API} from "./repoRegistry";
+import {makeRepo} from "../../core/repo";
 
 require("../testUtil").configureEnzyme();
 require("../testUtil").configureAphrodite();
@@ -63,10 +64,7 @@ describe("app/credExplorer/RepositorySelect", () => {
       );
     });
     it("renders a select with all available repos as options", () => {
-      const availableRepos = [
-        {owner: "foo", name: "bar"},
-        {owner: "zod", name: "zoink"},
-      ];
+      const availableRepos = [makeRepo("foo", "bar"), makeRepo("zod", "zoink")];
       const selectedRepo = availableRepos[0];
       const e = shallow(
         <PureRepositorySelect
@@ -78,10 +76,7 @@ describe("app/credExplorer/RepositorySelect", () => {
       expect(options.map((x) => x.text())).toEqual(["foo/bar", "zod/zoink"]);
     });
     it("the selectedRepo is selected", () => {
-      const availableRepos = [
-        {owner: "foo", name: "bar"},
-        {owner: "zod", name: "zoink"},
-      ];
+      const availableRepos = [makeRepo("foo", "bar"), makeRepo("zod", "zoink")];
       const selectedRepo = availableRepos[0];
       const e = shallow(
         <PureRepositorySelect
@@ -92,10 +87,7 @@ describe("app/credExplorer/RepositorySelect", () => {
       expect(e.find("select").prop("value")).toBe("foo/bar");
     });
     it("clicking an option triggers the onChange", () => {
-      const availableRepos = [
-        {owner: "foo", name: "bar"},
-        {owner: "zod", name: "zoink"},
-      ];
+      const availableRepos = [makeRepo("foo", "bar"), makeRepo("zod", "zoink")];
       const onChange = jest.fn();
       const e = shallow(
         <PureRepositorySelect
@@ -133,15 +125,15 @@ describe("app/credExplorer/RepositorySelect", () => {
       });
     }
     it("calls fetch and handles a simple success", () => {
-      mockRegistry([{owner: "foo", name: "bar"}]);
-      const repo = {owner: "foo", name: "bar"};
+      const repo = makeRepo("foo", "bar");
+      mockRegistry([repo]);
       return expectLoadValidStatus(testLocalStore(), [repo], repo);
     });
     it("returns repos in sorted order, and selects the last repo", () => {
       const repos = [
-        {owner: "a", name: "b"},
-        {owner: "a", name: "z"},
-        {owner: "foo", name: "bar"},
+        makeRepo("a", "b"),
+        makeRepo("a", "z"),
+        makeRepo("foo", "bar"),
       ];
       const nonSortedRepos = [repos[2], repos[0], repos[1]];
       mockRegistry(nonSortedRepos);
@@ -176,9 +168,9 @@ describe("app/credExplorer/RepositorySelect", () => {
     });
     it("loads selectedRepo from localStore, if available", () => {
       const repos = [
-        {owner: "a", name: "b"},
-        {owner: "a", name: "z"},
-        {owner: "foo", name: "bar"},
+        makeRepo("a", "b"),
+        makeRepo("a", "z"),
+        makeRepo("foo", "bar"),
       ];
       mockRegistry(repos);
       const localStore = testLocalStore();
@@ -187,9 +179,9 @@ describe("app/credExplorer/RepositorySelect", () => {
     });
     it("ignores selectedRepo from localStore, if not available", () => {
       const repos = [
-        {owner: "a", name: "b"},
-        {owner: "a", name: "z"},
-        {owner: "foo", name: "bar"},
+        makeRepo("a", "b"),
+        makeRepo("a", "z"),
+        makeRepo("foo", "bar"),
       ];
       mockRegistry(repos);
       const localStore = testLocalStore();
@@ -198,9 +190,9 @@ describe("app/credExplorer/RepositorySelect", () => {
     });
     it("ignores malformed value in localStore", () => {
       const repos = [
-        {owner: "a", name: "b"},
-        {owner: "a", name: "z"},
-        {owner: "foo", name: "bar"},
+        makeRepo("a", "b"),
+        makeRepo("a", "z"),
+        makeRepo("foo", "bar"),
       ];
       mockRegistry(repos);
       const localStore = testLocalStore();
@@ -273,7 +265,7 @@ describe("app/credExplorer/RepositorySelect", () => {
 
   describe("RepositorySelect", () => {
     it("initially renders a LocalStoreRepositorySelect with status LOADING", () => {
-      mockRegistry([{owner: "irrelevant", name: "unused"}]);
+      mockRegistry([makeRepo("irrelevant", "unused")]);
       const e = shallow(
         <RepositorySelect onChange={jest.fn()} localStore={testLocalStore()} />
       );
@@ -296,7 +288,7 @@ describe("app/credExplorer/RepositorySelect", () => {
 
     it("on successful load, sets the status on the child", async () => {
       const onChange = jest.fn();
-      const selectedRepo = {owner: "foo", name: "bar"};
+      const selectedRepo = makeRepo("foo", "bar");
       mockRegistry([selectedRepo]);
       const e = shallow(
         <RepositorySelect onChange={onChange} localStore={testLocalStore()} />
@@ -313,10 +305,7 @@ describe("app/credExplorer/RepositorySelect", () => {
 
     it("on successful load, passes the status to the onChange", async () => {
       const onChange = jest.fn();
-      const repo = {
-        owner: "foo",
-        name: "bar",
-      };
+      const repo = makeRepo("foo", "bar");
       mockRegistry([repo]);
       const e = shallow(
         <RepositorySelect onChange={onChange} localStore={testLocalStore()} />
@@ -342,7 +331,7 @@ describe("app/credExplorer/RepositorySelect", () => {
 
     it("child onChange triggers parent onChange", () => {
       const onChange = jest.fn();
-      const repo = {owner: "foo", name: "bar"};
+      const repo = makeRepo("foo", "bar");
       mockRegistry([repo]);
       const e = mount(
         <RepositorySelect onChange={onChange} localStore={testLocalStore()} />
@@ -355,7 +344,7 @@ describe("app/credExplorer/RepositorySelect", () => {
 
     it("selecting child option updates top-level state", async () => {
       const onChange = jest.fn();
-      const repos = [{owner: "foo", name: "bar"}, {owner: "z", name: "a"}];
+      const repos = [makeRepo("foo", "bar"), makeRepo("z", "a")];
       mockRegistry(repos);
       const e = mount(
         <RepositorySelect onChange={onChange} localStore={testLocalStore()} />

src/app/credExplorer/repoRegistry.js

@@ -5,13 +5,13 @@
 // (src/app/credExplorer/RepositorySelect.js)
 import deepEqual from "lodash.isequal";
 import {toCompat, fromCompat, type Compatible} from "../../util/compat";
+import type {Repo} from "../../core/repo";
 
 export const REPO_REGISTRY_FILE = "repositoryRegistry.json";
 export const REPO_REGISTRY_API = "/api/v1/data/repositoryRegistry.json";
 
 const REPO_REGISTRY_COMPAT = {type: "REPO_REGISTRY", version: "0.1.0"};
 
-export type Repo = {|+name: string, +owner: string|};
 export type RepoRegistry = $ReadOnlyArray<Repo>;
 export type RepoRegistryJSON = Compatible<RepoRegistry>;
 

src/app/credExplorer/repoRegistry.test.js

@@ -8,8 +8,9 @@ import {
   type RepoRegistry,
 } from "./repoRegistry";
 
+import {makeRepo} from "../../core/repo";
+
 describe("app/credExplorer/repoRegistry", () => {
-  const r = (owner, name) => ({owner, name});
   describe("to/fromJSON compose on", () => {
     function checkExample(x: RepoRegistry) {
       expect(fromJSON(toJSON(x))).toEqual(x);
@@ -19,31 +20,31 @@ describe("app/credExplorer/repoRegistry", () => {
       checkExample(emptyRegistry());
     });
     it("nonempty registry", () => {
-      checkExample([r("foo", "bar"), r("zoo", "zod")]);
+      checkExample([makeRepo("foo", "bar"), makeRepo("zoo", "zod")]);
     });
   });
   describe("addRepo", () => {
     it("adds to empty registry", () => {
-      expect(addRepo(r("foo", "bar"), emptyRegistry())).toEqual([
-        r("foo", "bar"),
+      expect(addRepo(makeRepo("foo", "bar"), emptyRegistry())).toEqual([
+        makeRepo("foo", "bar"),
       ]);
     });
     it("adds to nonempty registry", () => {
-      const registry = [r("foo", "bar")];
-      expect(addRepo(r("zoo", "zod"), registry)).toEqual([
-        r("foo", "bar"),
-        r("zoo", "zod"),
+      const registry = [makeRepo("foo", "bar")];
+      expect(addRepo(makeRepo("zoo", "zod"), registry)).toEqual([
+        makeRepo("foo", "bar"),
+        makeRepo("zoo", "zod"),
       ]);
     });
     it("adding repo that is already the last has no effect", () => {
-      const registry = [r("zoo", "zod"), r("foo", "bar")];
-      expect(addRepo(r("foo", "bar"), registry)).toEqual(registry);
+      const registry = [makeRepo("zoo", "zod"), makeRepo("foo", "bar")];
+      expect(addRepo(makeRepo("foo", "bar"), registry)).toEqual(registry);
     });
     it("adding already-existing repo shifts it to the end", () => {
-      const registry = [r("zoo", "zod"), r("foo", "bar")];
-      expect(addRepo(r("zoo", "zod"), registry)).toEqual([
-        r("foo", "bar"),
-        r("zoo", "zod"),
+      const registry = [makeRepo("zoo", "zod"), makeRepo("foo", "bar")];
+      expect(addRepo(makeRepo("zoo", "zod"), registry)).toEqual([
+        makeRepo("foo", "bar"),
+        makeRepo("zoo", "zod"),
       ]);
     });
   });

src/app/pluginAdapter.js

@@ -1,6 +1,7 @@
 // @flow
 
 import type {Graph, NodeAddressT, EdgeAddressT} from "../core/graph";
+import type {Repo} from "../core/repo";
 
 export interface StaticPluginAdapter {
   name(): string;
@@ -16,7 +17,7 @@ export interface StaticPluginAdapter {
     +backwardName: string,
     +prefix: EdgeAddressT,
   |}>;
-  load(repoOwner: string, repoName: string): Promise<DynamicPluginAdapter>;
+  load(repo: Repo): Promise<DynamicPluginAdapter>;
 }
 
 export interface DynamicPluginAdapter {

src/cli/commands/load.js

@@ -14,6 +14,8 @@ import {
   sourcecredDirectoryFlag,
 } from "../common";
 
+import {makeRepo} from "../../core/repo";
+
 import {
   toJSON,
   fromJSON,
@@ -59,7 +61,7 @@ export default class PluginGraphCommand extends Command {
 
   async run() {
     const {
-      args: {repo_owner: repoOwner, repo_name: repoName},
+      args: {repo_owner: owner, repo_name: name},
       flags: {
         "github-token": githubToken,
         "sourcecred-directory": basedir,
@@ -67,28 +69,22 @@ export default class PluginGraphCommand extends Command {
         plugin,
       },
     } = this.parse(PluginGraphCommand);
+    const repo = makeRepo(owner, name);
     if (!plugin) {
       loadAllPlugins({
         basedir,
         plugin,
-        repoOwner,
-        repoName,
+        repo,
         githubToken,
         maxOldSpaceSize,
       });
     } else {
-      loadPlugin({basedir, plugin, repoOwner, repoName, githubToken});
+      loadPlugin({basedir, plugin, repo, githubToken});
     }
   }
 }
 
-function loadAllPlugins({
-  basedir,
-  repoOwner,
-  repoName,
-  githubToken,
-  maxOldSpaceSize,
-}) {
+function loadAllPlugins({basedir, repo, githubToken, maxOldSpaceSize}) {
   if (githubToken == null) {
     // TODO: This check should be abstracted so that plugins can
     // specify their argument dependencies and get nicely
@@ -105,8 +101,8 @@ function loadAllPlugins({
         `--max_old_space_size=${maxOldSpaceSize}`,
         "./bin/sourcecred.js",
         "load",
-        repoOwner,
-        repoName,
+        repo.owner,
+        repo.name,
         "--plugin",
         pluginName,
         "--github-token",
@@ -117,18 +113,18 @@ function loadAllPlugins({
   ];
   execDependencyGraph(tasks, {taskPassLabel: "DONE"}).then(({success}) => {
     if (success) {
-      addToRepoRegistry({basedir, repoOwner, repoName});
+      addToRepoRegistry({basedir, repo});
     }
     process.exitCode = success ? 0 : 1;
   });
 }
 
-function loadPlugin({basedir, plugin, repoOwner, repoName, githubToken}) {
+function loadPlugin({basedir, plugin, repo, githubToken}) {
   const outputDirectory = path.join(
     basedir,
     "data",
-    repoOwner,
-    repoName,
+    repo.owner,
+    repo.name,
     plugin
   );
   mkdirp.sync(outputDirectory);
@@ -144,14 +140,13 @@ function loadPlugin({basedir, plugin, repoOwner, repoName, githubToken}) {
       } else {
         loadGithubData({
           token: githubToken,
-          repoOwner,
-          repoName,
+          repo,
           outputDirectory,
         });
       }
       break;
     case "git":
-      loadGitData({repoOwner, repoName, outputDirectory});
+      loadGitData({repo, outputDirectory});
       break;
     default:
       console.error("fatal: Unknown plugin: " + (plugin: empty));
@@ -163,8 +158,7 @@ function loadPlugin({basedir, plugin, repoOwner, repoName, githubToken}) {
 function addToRepoRegistry(options) {
   // TODO: Make this function transactional before loading repositories in
   // parallel.
-  const {basedir, repoOwner, repoName} = options;
-  const repo = {owner: repoOwner, name: repoName};
+  const {basedir, repo} = options;
   const outputFile = path.join(basedir, REPO_REGISTRY_FILE);
   let registry = null;
   if (fs.existsSync(outputFile)) {

src/core/repo.js

@@ -0,0 +1,29 @@
+// @flow
+
+export opaque type Repo: {|+name: string, +owner: string|} = {|
+  +name: string,
+  +owner: string,
+|};
+
+export function makeRepo(owner: string, name: string): Repo {
+  const validRe = /^[A-Za-z0-9-.]+$/;
+  if (!owner.match(validRe)) {
+    throw new Error(`Invalid repository owner: ${JSON.stringify(owner)}`);
+  }
+  if (!name.match(validRe)) {
+    throw new Error(`Invalid repository name: ${JSON.stringify(name)}`);
+  }
+  return {owner, name};
+}
+
+export function stringToRepo(x: string): Repo {
+  const pieces = x.split("/");
+  if (pieces.length !== 2) {
+    throw new Error(`Invalid repo string: ${x}`);
+  }
+  return makeRepo(pieces[0], pieces[1]);
+}
+
+export function repoToString(x: Repo): string {
+  return `${x.owner}/${x.name}`;
+}

src/core/repo.test.js

@@ -0,0 +1,51 @@
+// @flow
+
+import {makeRepo, stringToRepo, repoToString, type Repo} from "./repo";
+
+describe("core/repo", () => {
+  describe("Repo type", () => {
+    it("manually constructing a Repo is illegal", () => {
+      // $ExpectFlowError
+      const _unused_repo: Repo = {owner: "foo", name: "bar"};
+    });
+    it("destructuring repo properties is legal", () => {
+      const repo: Repo = makeRepo("foo", "bar");
+      const _unused_owner: string = repo.owner;
+      const _unused_name: string = repo.name;
+    });
+  });
+  describe("makeRepoRepo", () => {
+    it("allows a simple repo", () => {
+      makeRepo("sourcecred", "sourcecred");
+    });
+    it("allows a repo with periods in name", () => {
+      makeRepo("sourcecred", "sourcecred.github.io");
+    });
+    it("allows a repo with hyphens", () => {
+      makeRepo("foo", "something-good");
+    });
+    it("disallows a repo with no owner", () => {
+      expect(() => makeRepo("", "foo")).toThrow("Invalid");
+    });
+    it("disallows a repo with no name", () => {
+      expect(() => makeRepo("foo", "")).toThrow("Invalid");
+    });
+    it("disallows a repo with underscores", () => {
+      expect(() => makeRepo("yep", "something_bad")).toThrow("Invalid");
+    });
+  });
+  describe("repo<->string", () => {
+    function testInvertible(owner, name) {
+      const repo = makeRepo(owner, name);
+      const string = `${owner}/${name}`;
+      expect(stringToRepo(string)).toEqual(repo);
+      expect(repoToString(repo)).toEqual(string);
+    }
+    it("works for simple case", () => {
+      testInvertible("sourcecred", "sourcecred");
+    });
+    it("works for a complicated case", () => {
+      testInvertible("fooolio", "foo-bar.bar-99");
+    });
+  });
+});

src/graphql/__snapshots__/queries.test.js.snap

@@ -413,11 +413,11 @@ Array [
     "name": "FetchData",
     "params": Array [
       Object {
-        "name": "repoOwner",
+        "name": "owner",
         "type": "String!",
       },
       Object {
-        "name": "repoName",
+        "name": "name",
         "type": "String!",
       },
     ],
@@ -426,11 +426,11 @@ Array [
         "alias": null,
         "args": Object {
           "name": Object {
-            "data": "repoName",
+            "data": "name",
             "type": "VARIABLE",
           },
           "owner": Object {
-            "data": "repoOwner",
+            "data": "owner",
             "type": "VARIABLE",
           },
         },
@@ -926,11 +926,11 @@ Array [
 ]
 `;
 
-exports[`queries end-to-end-test cases for a useful query should stringify as inline 1`] = `"query FetchData($repoOwner: String! $repoName: String!) { repository(owner: $repoOwner name: $repoName) { issues(first: 100) { pageInfo { hasNextPage } nodes { id title body number author { ...whoami } comments(first: 20) { pageInfo { hasNextPage } nodes { id author { ...whoami } body url } } } } pullRequests(first: 100) { pageInfo { hasNextPage } nodes { id title body number author { ...whoami } comments(first: 20) { pageInfo { hasNextPage } nodes { id author { ...whoami } body url } } reviews(first: 10) { pageInfo { hasNextPage } nodes { id body author { ...whoami } state comments(first: 10) { pageInfo { hasNextPage } nodes { id body author { ...whoami } } } } } } } } } fragment whoami on Actor { __typename login ... on User { id } ... on Organization { id } ... on Bot { id } }"`;
+exports[`queries end-to-end-test cases for a useful query should stringify as inline 1`] = `"query FetchData($owner: String! $name: String!) { repository(owner: $owner name: $name) { issues(first: 100) { pageInfo { hasNextPage } nodes { id title body number author { ...whoami } comments(first: 20) { pageInfo { hasNextPage } nodes { id author { ...whoami } body url } } } } pullRequests(first: 100) { pageInfo { hasNextPage } nodes { id title body number author { ...whoami } comments(first: 20) { pageInfo { hasNextPage } nodes { id author { ...whoami } body url } } reviews(first: 10) { pageInfo { hasNextPage } nodes { id body author { ...whoami } state comments(first: 10) { pageInfo { hasNextPage } nodes { id body author { ...whoami } } } } } } } } } fragment whoami on Actor { __typename login ... on User { id } ... on Organization { id } ... on Bot { id } }"`;
 
 exports[`queries end-to-end-test cases for a useful query should stringify as multiline 1`] = `
-"query FetchData($repoOwner: String! $repoName: String!) {
-  repository(owner: $repoOwner name: $repoName) {
+"query FetchData($owner: String! $name: String!) {
+  repository(owner: $owner name: $name) {
     issues(first: 100) {
       pageInfo {
         hasNextPage

src/graphql/queries.test.js

@@ -126,11 +126,11 @@ function usefulQuery(): Body {
   const body: Body = [
     b.query(
       "FetchData",
-      [b.param("repoOwner", "String!"), b.param("repoName", "String!")],
+      [b.param("owner", "String!"), b.param("name", "String!")],
       [
         b.field(
           "repository",
-          {owner: b.variable("repoOwner"), name: b.variable("repoName")},
+          {owner: b.variable("owner"), name: b.variable("name")},
           [
             b.field("issues", {first: b.literal(100)}, [
               makePageInfo(),

src/plugins/git/cloneAndLoadRepository.js

@@ -4,22 +4,18 @@ import tmp from "tmp";
 import {localGit} from "./gitUtils";
 import type {Repository} from "./types";
 import {loadRepository} from "./loadRepository";
+import type {Repo} from "../../core/repo";
 
 /**
  * Load Git Repository data from a fresh clone of a GitHub repo.
  *
- * @param {String} repoOwner
- *   the GitHub username of the owner of the repository to be cloned
- * @param {String} repoName
- *   the name of the repository to be cloned
+ * @param {Repo} repo
+ *   the GitHub repository to be cloned
  * @return {Repository}
  *   the parsed Repository from the cloned repo
  */
-export default function cloneAndLoadRepository(
-  repoOwner: string,
-  repoName: string
-): Repository {
-  const cloneUrl = `https://github.com/${repoOwner}/${repoName}.git`;
+export default function cloneAndLoadRepository(repo: Repo): Repository {
+  const cloneUrl = `https://github.com/${repo.owner}/${repo.name}.git`;
   const tmpdir = tmp.dirSync({unsafeCleanup: true});
   const git = localGit(tmpdir.name);
   git(["clone", cloneUrl, ".", "--quiet"]);

src/plugins/git/loadGitData.js

@@ -5,18 +5,15 @@ import path from "path";
 
 import cloneAndLoadRepository from "./cloneAndLoadRepository";
 import {createGraph} from "./createGraph";
+import type {Repo} from "../../core/repo";
 
 export type Options = {|
-  +repoOwner: string,
-  +repoName: string,
+  +repo: Repo,
   +outputDirectory: string,
 |};
 
 export function loadGitData(options: Options): Promise<void> {
-  const repository = cloneAndLoadRepository(
-    options.repoOwner,
-    options.repoName
-  );
+  const repository = cloneAndLoadRepository(options.repo);
   const graph = createGraph(repository);
   const blob = JSON.stringify(graph);
   const outputFilename = path.join(options.outputDirectory, "graph.json");

src/plugins/git/pluginAdapter.js

@@ -7,6 +7,7 @@ import {Graph} from "../../core/graph";
 import * as N from "./nodes";
 import * as E from "./edges";
 import {description} from "./render";
+import type {Repo} from "../../core/repo";
 
 export class StaticPluginAdapter implements IStaticPluginAdapter {
   name() {
@@ -55,11 +56,8 @@ export class StaticPluginAdapter implements IStaticPluginAdapter {
       },
     ];
   }
-  async load(
-    repoOwner: string,
-    repoName: string
-  ): Promise<IDynamicPluginAdapter> {
-    const url = `/api/v1/data/data/${repoOwner}/${repoName}/git/graph.json`;
+  async load(repo: Repo): Promise<IDynamicPluginAdapter> {
+    const url = `/api/v1/data/data/${repo.owner}/${repo.name}/git/graph.json`;
     const response = await fetch(url);
     if (!response.ok) {
       return Promise.reject(response);

src/plugins/github/__snapshots__/graphql.test.js.snap

@@ -202,8 +202,8 @@ Object {
 `;
 
 exports[`graphql creates a query 1`] = `
-"query FetchData($repoOwner: String! $repoName: String!) {
-  repository(owner: $repoOwner name: $repoName) {
+"query FetchData($owner: String! $name: String!) {
+  repository(owner: $owner name: $name) {
     url
     name
     owner {

src/plugins/github/bin/fetchAndPrintGithubRepo.js

@@ -14,6 +14,7 @@
 
 import fetchGithubRepo from "../fetchGithubRepo";
 import stringify from "json-stable-stringify";
+import {makeRepo} from "../../../core/repo";
 
 function parseArgs() {
   const argv = process.argv.slice(2);
@@ -24,8 +25,8 @@ function parseArgs() {
   if (argv.length < 2) {
     fail();
   }
-  const [repoOwner, repoName, githubToken, ...rest] = argv;
-  const result = {repoOwner, repoName, githubToken};
+  const [owner, name, githubToken, ...rest] = argv;
+  const result = {owner, name, githubToken};
   if (rest.length > 0) {
     fail();
   }
@@ -34,7 +35,8 @@ function parseArgs() {
 
 function main() {
   const args = parseArgs();
-  fetchGithubRepo(args.repoOwner, args.repoName, args.githubToken)
+  const repo = makeRepo(args.owner, args.name);
+  fetchGithubRepo(repo, args.githubToken)
     .then((data) => {
       console.log(stringify(data, {space: 4}));
     })

src/plugins/github/fetchGithubRepo.js

@@ -9,14 +9,13 @@ import fetch from "isomorphic-fetch";
 import {stringify, inlineLayout} from "../../graphql/queries";
 import {createQuery, createVariables, postQueryExhaustive} from "./graphql";
 import type {GithubResponseJSON} from "./graphql";
+import type {Repo} from "../../core/repo";
 
 /**
  * Scrape data from a GitHub repo using the GitHub API.
  *
- * @param {String} repoOwner
- *    the GitHub username of the owner of the repository to be scraped
- * @param {String} repoName
- *    the name of the repository to be scraped
+ * @param {Repo} repo
+ *    the GitHub repository to be scraped
  * @param {String} token
  *    authentication token to be used for the GitHub API; generate a
  *    token at: https://github.com/settings/tokens
@@ -26,28 +25,18 @@ import type {GithubResponseJSON} from "./graphql";
  *    later
  */
 export default function fetchGithubRepo(
-  repoOwner: string,
-  repoName: string,
+  repo: Repo,
   token: string
 ): Promise<GithubResponseJSON> {
-  repoOwner = String(repoOwner);
-  repoName = String(repoName);
   token = String(token);
 
-  const validName = /^[A-Za-z0-9_-]*$/;
-  if (!validName.test(repoOwner)) {
-    throw new Error(`Invalid repoOwner: ${repoOwner}`);
-  }
-  if (!validName.test(repoName)) {
-    throw new Error(`Invalid repoName: ${repoName}`);
-  }
   const validToken = /^[A-Fa-f0-9]{40}$/;
   if (!validToken.test(token)) {
     throw new Error(`Invalid token: ${token}`);
   }
 
   const body = createQuery();
-  const variables = createVariables(repoOwner, repoName);
+  const variables = createVariables(repo);
   const payload = {body, variables};
   return postQueryExhaustive(
     (somePayload) => postQuery(somePayload, token),

src/plugins/github/graphql.js

@@ -7,6 +7,7 @@ import type {
   QueryDefinition,
 } from "../../graphql/queries";
 import {build} from "../../graphql/queries";
+import type {Repo} from "../../core/repo";
 
 /**
  * This module defines the GraphQL query that we use to access the
@@ -119,11 +120,11 @@ export function createQuery(): Body {
   const body: Body = [
     b.query(
       "FetchData",
-      [b.param("repoOwner", "String!"), b.param("repoName", "String!")],
+      [b.param("owner", "String!"), b.param("name", "String!")],
       [
         b.field(
           "repository",
-          {owner: b.variable("repoOwner"), name: b.variable("repoName")},
+          {owner: b.variable("owner"), name: b.variable("name")},
           [
             b.field("url"),
             b.field("name"),
@@ -378,8 +379,8 @@ function* continuationsFromReview(
  * results. The `postQuery` function may be called multiple times.
  */
 export async function postQueryExhaustive(
-  postQuery: ({body: Body, variables: {[string]: any}}) => Promise<any>,
-  payload: {body: Body, variables: {[string]: any}}
+  postQuery: ({body: Body, variables: {+[string]: any}}) => Promise<any>,
+  payload: {body: Body, variables: {+[string]: any}}
 ) {
   const originalResult = await postQuery(payload);
   return resolveContinuations(
@@ -394,7 +395,7 @@ export async function postQueryExhaustive(
  * resolve the continuations and return the merged results.
  */
 async function resolveContinuations(
-  postQuery: ({body: Body, variables: {[string]: any}}) => Promise<any>,
+  postQuery: ({body: Body, variables: {+[string]: any}}) => Promise<any>,
   originalResult: any,
   continuations: $ReadOnlyArray<Continuation>
 ): Promise<any> {
@@ -820,6 +821,6 @@ export function createFragments(): FragmentDefinition[] {
   ];
 }
 
-export function createVariables(repoOwner: string, repoName: string) {
-  return {repoOwner, repoName};
+export function createVariables(repo: Repo): {+[string]: any} {
+  return repo;
 }

src/plugins/github/graphql.test.js

@@ -13,6 +13,7 @@ import {
   postQueryExhaustive,
   requiredFragments,
 } from "./graphql";
+import {makeRepo} from "../../core/repo";
 
 describe("graphql", () => {
   describe("creates continuations", () => {
@@ -944,7 +945,7 @@ describe("graphql", () => {
 
       const result = await postQueryExhaustive(postQuery, {
         body: createQuery(),
-        variables: createVariables("sourcecred", "discussion"),
+        variables: createVariables(makeRepo("sourcecred", "discussion")),
       });
       expect(postQuery).toHaveBeenCalledTimes(3);
 

src/plugins/github/loadGithubData.js

@@ -5,20 +5,16 @@ import path from "path";
 
 import fetchGithubRepo from "./fetchGithubRepo";
 import {RelationalView} from "./relationalView";
+import type {Repo} from "../../core/repo";
 
 export type Options = {|
   +token: string,
-  +repoOwner: string,
-  +repoName: string,
+  +repo: Repo,
   +outputDirectory: string,
 |};
 
 export async function loadGithubData(options: Options): Promise<void> {
-  const response = await fetchGithubRepo(
-    options.repoOwner,
-    options.repoName,
-    options.token
-  );
+  const response = await fetchGithubRepo(options.repo, options.token);
   const view = new RelationalView();
   view.addData(response);
   const blob = JSON.stringify(view);

src/plugins/github/pluginAdapter.js

@@ -9,6 +9,7 @@ import * as N from "./nodes";
 import * as E from "./edges";
 import {RelationalView} from "./relationalView";
 import {description} from "./render";
+import type {Repo} from "../../core/repo";
 
 export class StaticPluginAdapter implements IStaticPluginAdapter {
   name() {
@@ -54,11 +55,8 @@ export class StaticPluginAdapter implements IStaticPluginAdapter {
       },
     ];
   }
-  async load(
-    repoOwner: string,
-    repoName: string
-  ): Promise<IDynamicPluginAdapater> {
-    const url = `/api/v1/data/data/${repoOwner}/${repoName}/github/view.json`;
+  async load(repo: Repo): Promise<IDynamicPluginAdapater> {
+    const url = `/api/v1/data/data/${repo.owner}/${repo.name}/github/view.json`;
     const response = await fetch(url);
     if (!response.ok) {
       return Promise.reject(response);