From 22ca77ed05468194681381d90b1bc35a12996a45 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dandelion=20Man=C3=A9?= <dl@dandelion.io>
Date: Mon, 30 Apr 2018 10:07:23 -0700
Subject: [PATCH] Add safe type coercion for GitHub api (#173)

In general, methods in the porcelain GitHub api may return multiple
types; e.g. a reference could be to an Issue, PullRequest, Comment,
Author (or more). To make working with the api more convenient while
maintaining safety, this commit adds a static `asType` method to each
Entity class, which confirms that type coercion is safe, and errors if
not.

This commit also adds `issueOrPRByNumber`, a convenience method, to
api.test.js.

Test plan: Check the API usage and verify that it is reasonable.
---
 src/plugins/github/api.js      | 31 ++++++++++++++++++++++-
 src/plugins/github/api.test.js | 45 +++++++++++++++++++++++++---------
 2 files changed, 63 insertions(+), 13 deletions(-)

diff --git a/src/plugins/github/api.js b/src/plugins/github/api.js
index 9489cb5..a518a3c 100644
--- a/src/plugins/github/api.js
+++ b/src/plugins/github/api.js
@@ -1,5 +1,7 @@
 // @flow
 
+import stringify from "json-stable-stringify";
+
 import {Graph} from "../../core/graph";
 import type {Node} from "../../core/graph";
 import type {Address} from "../../core/address";
@@ -23,6 +25,16 @@ import {
   PULL_REQUEST_NODE_TYPE,
 } from "./types";
 
+export type Entity = Issue | PullRequest | Comment | Author;
+
+function assertEntityType(e: Entity, t: NodeType) {
+  if (e.type() !== t) {
+    throw new Error(
+      `Expected entity at ${stringify(e.address())} to have type ${t}`
+    );
+  }
+}
+
 export class Repository {
   repositoryName: string;
   graph: Graph<NodePayload, EdgePayload>;
@@ -123,6 +135,10 @@ class Commentable<T: IssueNodePayload | PullRequestNodePayload> extends Post<
 }
 
 export class Author extends GithubEntity<AuthorNodePayload> {
+  static from(e: Entity): Author {
+    assertEntityType(e, AUTHOR_NODE_TYPE);
+    return (e: any);
+  }
   login(): string {
     return this.node().payload.login;
   }
@@ -133,6 +149,10 @@ export class Author extends GithubEntity<AuthorNodePayload> {
 }
 
 export class PullRequest extends Commentable<PullRequestNodePayload> {
+  static from(e: Entity): PullRequest {
+    assertEntityType(e, PULL_REQUEST_NODE_TYPE);
+    return (e: any);
+  }
   number(): number {
     return this.node().payload.number;
   }
@@ -142,6 +162,10 @@ export class PullRequest extends Commentable<PullRequestNodePayload> {
 }
 
 export class Issue extends Commentable<IssueNodePayload> {
+  static from(e: Entity): Issue {
+    assertEntityType(e, ISSUE_NODE_TYPE);
+    return (e: any);
+  }
   number(): number {
     return this.node().payload.number;
   }
@@ -150,4 +174,9 @@ export class Issue extends Commentable<IssueNodePayload> {
   }
 }
 
-export class Comment extends Post<CommentNodePayload> {}
+export class Comment extends Post<CommentNodePayload> {
+  static from(e: Entity): Comment {
+    assertEntityType(e, COMMENT_NODE_TYPE);
+    return (e: any);
+  }
+}
diff --git a/src/plugins/github/api.test.js b/src/plugins/github/api.test.js
index f12ddbe..7aa17db 100644
--- a/src/plugins/github/api.test.js
+++ b/src/plugins/github/api.test.js
@@ -12,12 +12,16 @@ import {
 describe("GitHub porcelain API", () => {
   const graph = parse("sourcecred/example-repo", exampleRepoData);
   const repo = new Repository("sourcecred/example-repo", graph);
+  function issueOrPRByNumber(n: number): Issue | PullRequest {
+    const result = repo.issueOrPRByNumber(n);
+    if (result == null) {
+      throw new Error(`Expected Issue/PR ${n} to exist`);
+    }
+    return result;
+  }
 
   it("Issue", () => {
-    const issue = repo.issueOrPRByNumber(1);
-    if (issue == null) {
-      throw new Error("Issue reaching issue!");
-    }
+    const issue = issueOrPRByNumber(1);
     expect(issue.title()).toBe("An example issue.");
     expect(issue.body()).toBe("This is just an example issue.");
     expect(issue.number()).toBe(1);
@@ -31,10 +35,7 @@ describe("GitHub porcelain API", () => {
   });
 
   it("PullRequest", () => {
-    const pullRequest = repo.issueOrPRByNumber(3);
-    if (pullRequest == null) {
-      throw new Error("Issue reaching PR!");
-    }
+    const pullRequest = issueOrPRByNumber(3);
     expect(pullRequest.body()).toBe("Oh look, it's a pull request.");
     expect(pullRequest.url()).toBe(
       "https://github.com/sourcecred/example-repo/pull/3"
@@ -46,10 +47,7 @@ describe("GitHub porcelain API", () => {
   });
 
   it("Comment", () => {
-    const issue = repo.issueOrPRByNumber(6);
-    if (issue == null) {
-      throw new Error("Issue reaching issue!");
-    }
+    const issue = issueOrPRByNumber(6);
     const comments = issue.comments();
     expect(comments.length).toMatchSnapshot();
     const comment = comments[0];
@@ -78,4 +76,27 @@ describe("GitHub porcelain API", () => {
     expect(decentralion.node()).toMatchSnapshot();
     expect(decentralion.address()).toEqual(decentralion.node().address);
   });
+
+  describe("type coercion", () => {
+    it("type coercion works when typed correctly", () => {
+      const issue: Issue = Issue.from(issueOrPRByNumber(1));
+      const pr: PullRequest = PullRequest.from(issueOrPRByNumber(3));
+      const author: Author = Author.from(issueOrPRByNumber(3).authors()[0]);
+      const comment: Comment = Comment.from(issueOrPRByNumber(2).comments()[0]);
+    });
+    it("type coercion throws error when typed incorrectly", () => {
+      expect(() => PullRequest.from(issueOrPRByNumber(1))).toThrowError(
+        "to have type PULL_REQUEST"
+      );
+      expect(() => Issue.from(issueOrPRByNumber(3))).toThrowError(
+        "to have type ISSUE"
+      );
+      expect(() =>
+        Comment.from(issueOrPRByNumber(3).authors()[0])
+      ).toThrowError("to have type COMMENT");
+      expect(() =>
+        Author.from(issueOrPRByNumber(2).comments()[0])
+      ).toThrowError("to have type AUTHOR");
+    });
+  });
 });