diff --git a/src/plugins/github/__snapshots__/graphql.test.js.snap b/src/plugins/github/__snapshots__/graphql.test.js.snap new file mode 100644 index 0000000..f0d61f9 --- /dev/null +++ b/src/plugins/github/__snapshots__/graphql.test.js.snap @@ -0,0 +1,202 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`graphql #postQueryExhaustive resolves a representative query 1`] = ` +Object { + "repository": Object { + "id": "opaque-repo", + "issues": Object { + "nodes": Array [ + Object { + "author": Object { + "__typename": "User", + "id": "opaque-user-decentralion", + "login": "decentralion", + }, + "body": "Like it says, please comment!", + "comments": Object { + "nodes": Array [ + Object { + "author": Object { + "__typename": "User", + "id": "opaque-user-decentralion", + "login": "decentralion", + }, + "body": "Here: I'll start.", + "id": "opaque-issue1comment1", + "url": "opaque://issue/1/comment/1", + }, + Object { + "author": Object { + "__typename": "User", + "id": "opaque-user-wchargin", + "login": "wchargin", + }, + "body": "Closing due to no fun allowed.", + "id": "opaque-issue1comment2", + "url": "opaque://issue/1/comment/2", + }, + Object { + "author": Object { + "__typename": "User", + "id": "opaque-user-decentralion", + "login": "decentralion", + }, + "body": "That is not very nice.", + "id": "opaque-issue1comment3", + "url": "opaque://issue/1/comment/3", + }, + ], + "pageInfo": Object { + "endCursor": "opaque-cursor-issue1comments-v2", + "hasNextPage": false, + }, + }, + "id": "opaque-issue1", + "number": 1, + "title": "Request for comments", + }, + Object { + "author": Object { + "__typename": "User", + "id": "opaque-user-wchargin", + "login": "wchargin", + }, + "body": "You can comment here, too.", + "comments": Object { + "nodes": Array [ + Object { + "author": Object { + "__typename": "User", + "id": "opaque-user-decentralion", + "login": "decentralion", + }, + "body": "What fun!", + "id": "opaque-issue3comment1", + "url": "opaque://issue/3/comment/1", + }, + Object { + "author": Object { + "__typename": "User", + "id": "opaque-user-decentralion", + "login": "decentralion", + }, + "body": "I will comment on this issue for a second time.", + "id": "opaque-issue3comment2", + "url": "opaque://issue/1/comment/3", + }, + ], + "pageInfo": Object { + "endCursor": "opaque-cursor-issue3comments-v2", + "hasNextPage": false, + }, + }, + "id": "opaque-issue3", + "number": 2, + "title": "Another", + }, + Object { + "author": Object { + "__typename": "User", + "id": "opaque-user-wchargin", + "login": "wchargin", + }, + "body": "My mailbox is out of space", + "comments": Object { + "nodes": Array [ + Object { + "author": Object { + "__typename": "User", + "id": "opaque-user-decentralion", + "login": "decentralion", + }, + "body": "But you posted the last issue", + "id": "opaque-issue4comment1", + "url": "opaque://issue/4/comment/1", + }, + ], + "pageInfo": Object { + "endCursor": "opaque-cursor-issue4comments-v2", + "hasNextPage": false, + }, + }, + "id": "opaque-issue4", + "number": 4, + "title": "Please stop making issues", + }, + ], + "pageInfo": Object { + "endCursor": "opaque-cursor-issues-v2", + "hasNextPage": false, + }, + }, + "pullRequests": Object { + "nodes": Array [ + Object { + "author": Object { + "__typename": "User", + "id": "opaque-user-wchargin", + "login": "wchargin", + }, + "body": "Surely this deserves much cred.", + "comments": Object { + "nodes": Array [], + "pageInfo": Object { + "endCursor": null, + "hasNextPage": false, + }, + }, + "id": "opaque-pr2", + "number": 2, + "reviews": Object { + "nodes": Array [ + Object { + "author": Object { + "__typename": "User", + "id": "opaque-user-decentralion", + "login": "decentralion", + }, + "body": "You actually introduced a new typo instead.", + "comments": Object { + "nodes": Array [], + "pageInfo": Object { + "endCursor": null, + "hasNextPage": false, + }, + }, + "id": "opaque-pr2review1", + "state": "CHANGES_REQUESTED", + }, + Object { + "author": Object { + "__typename": "User", + "id": "opaque-user-decentralion", + "login": "decentralion", + }, + "body": "Looks godo to me.", + "comments": Object { + "nodes": Array [], + "pageInfo": Object { + "endCursor": null, + "hasNextPage": false, + }, + }, + "id": "opaque-pr2review2", + "state": "APPROVED", + }, + ], + "pageInfo": Object { + "endCursor": "opaque-cursor-pr2reviews-v1", + "hasNextPage": false, + }, + }, + "title": "Fix typo in README", + }, + ], + "pageInfo": Object { + "endCursor": "opaque-cursor-prs-v0", + "hasNextPage": false, + }, + }, + }, +} +`; diff --git a/src/plugins/github/fetchGithubRepo.js b/src/plugins/github/fetchGithubRepo.js index 789a20e..1e137f6 100644 --- a/src/plugins/github/fetchGithubRepo.js +++ b/src/plugins/github/fetchGithubRepo.js @@ -7,7 +7,7 @@ import fetch from "isomorphic-fetch"; import {stringify, inlineLayout} from "../../graphql/queries"; -import {createQuery, createVariables} from "./graphql"; +import {createQuery, createVariables, postQueryExhaustive} from "./graphql"; /** * Scrape data from a GitHub repo using the GitHub API. @@ -45,15 +45,28 @@ export default function fetchGithubRepo( throw new Error(`Invalid token: ${token}`); } - const query = stringify.body(createQuery(), inlineLayout()); + const body = createQuery(); const variables = createVariables(repoOwner, repoName); - const payload = {query, variables}; - return postQuery(payload, token); + const payload = {body, variables}; + return postQueryExhaustive( + (somePayload) => postQuery(somePayload, token), + payload + ).then((x) => { + ensureNoMorePages(x); + // TODO: We wrap back up in the "data" object to maintain + // compatibility. At some point, let's strip this out and modify + // clients of `example-data.json`. + return {data: x}; + }); } const GITHUB_GRAPHQL_SERVER = "https://api.github.com/graphql"; -function postQuery(payload, token) { +function postQuery({body, variables}, token) { + const payload = { + query: stringify.body(body, inlineLayout()), + variables: variables, + }; return fetch(GITHUB_GRAPHQL_SERVER, { method: "POST", body: JSON.stringify(payload), @@ -66,8 +79,7 @@ function postQuery(payload, token) { if (x.errors) { return Promise.reject(x); } - ensureNoMorePages(x); - return Promise.resolve(x); + return Promise.resolve(x.data); }); } diff --git a/src/plugins/github/graphql.js b/src/plugins/github/graphql.js index 3b2aebe..c175143 100644 --- a/src/plugins/github/graphql.js +++ b/src/plugins/github/graphql.js @@ -1,6 +1,11 @@ // @flow -import type {Body, FragmentDefinition, Selection} from "../../graphql/queries"; +import type { + Body, + FragmentDefinition, + Selection, + QueryDefinition, +} from "../../graphql/queries"; import {build} from "../../graphql/queries"; /** @@ -340,6 +345,156 @@ function* continuationsFromReview( } } +/** + * Execute the given query, returning all pages of data throughout the + * query. That is: post the query, then find any entities that require + * more pages of data, fetch those additional pages, and merge the + * results. The `postQuery` function may be called multiple times. + */ +export async function postQueryExhaustive( + postQuery: ({body: Body, variables: {[string]: any}}) => Promise, + payload: {body: Body, variables: {[string]: any}} +) { + const originalResult = await postQuery(payload); + return resolveContinuations( + postQuery, + originalResult, + Array.from(continuationsFromQuery(originalResult)) + ); +} + +/** + * Given the result of a query and the continuations for that query, + * resolve the continuations and return the merged results. + */ +async function resolveContinuations( + postQuery: ({body: Body, variables: {[string]: any}}) => Promise, + originalResult: any, + continuations: $ReadOnlyArray +): Promise { + if (!continuations.length) { + return originalResult; + } + + // Assign each continuation a nonce (unique name) so that we can refer + // to it unambiguously, and embed all the continuations into a query. + const embeddings = continuations.map((c, i) => ({ + continuation: c, + nonce: `_n${String(i)}`, + })); + const b = build; + const query = b.query( + "Continuations", + [], + embeddings.map(({continuation, nonce}) => + b.alias( + nonce, + b.field( + "node", + {id: b.literal(continuation.enclosingNodeId)}, + continuation.selections.slice() + ) + ) + ) + ); + const body = [query, ...requiredFragments(query)]; + const payload = {body, variables: {}}; + + // Send the continuation query, then merge these results into the + // original results---then recur, because the new results may + // themselves be incomplete. + const continuationResult = await postQuery(payload); + const mergedResults = embeddings.reduce((acc, {continuation, nonce}) => { + return merge(acc, continuationResult[nonce], continuation.destinationPath); + }, originalResult); + return resolveContinuations( + postQuery, + mergedResults, + Array.from(continuationsFromQuery(mergedResults)) + ); +} + +/** + * A GraphQL query includes a query body and some fragment definitions. + * It is an error to include unneeded fragment definitions. Therefore, + * given a standard set of fragments and an arbitrary query body, we + * need to be able to select just the right set of fragments for our + * particular query. + * + * This function finds all fragments that are transitively used by the + * given query. That is, it finds all fragments used by the query, all + * fragments used by those fragments, and so on. Note that the universe + * of fragments is considered to be the result of `createFragments`; it + * is an error to use a fragment not defined in the result of that + * function. + * + * Equivalently, construct a graph where the nodes are (a) the query and + * (b) all possible fragments, and there is an edge from `a` to `b` if + * `a` references `b`. Then, this function finds the set of vertices + * reachable from the query node. + */ +export function requiredFragments( + query: QueryDefinition +): FragmentDefinition[] { + const fragmentsByName = {}; + createFragments().forEach((fd) => { + fragmentsByName[fd.name] = fd; + }); + + // This function implements a BFS on the graph specified in the + // docstring, with the provisos that the nodes for fragments are the + // fragment names, not the fragments themselves, and that the query + // node is implicit (in the initial value of the `frontier`). + const requiredFragmentNames: Set = new Set(); + let frontier: Set = new Set(); + query.selections.forEach((selection) => { + for (const fragment of usedFragmentNames(selection)) { + frontier.add(fragment); + } + }); + + while (frontier.size > 0) { + frontier.forEach((name) => { + requiredFragmentNames.add(name); + }); + const newFrontier: Set = new Set(); + for (const name of frontier) { + const fragment = fragmentsByName[name]; + if (fragment == null) { + throw new Error(`Unknown fragment: "${fragment}"`); + } + fragment.selections.forEach((selection) => { + for (const fragment of usedFragmentNames(selection)) { + newFrontier.add(fragment); + } + }); + } + frontier = newFrontier; + } + + return createFragments().filter((fd) => requiredFragmentNames.has(fd.name)); +} + +/** + * Find all fragment names directly referenced by the given selection. + * This does not include transitive fragment references. + */ +function* usedFragmentNames(selection: Selection): Iterator { + switch (selection.type) { + case "FRAGMENT_SPREAD": + yield selection.fragmentName; + break; + case "FIELD": + case "INLINE_FRAGMENT": + for (const subselection of selection.selections) { + yield* usedFragmentNames(subselection); + } + break; + default: + throw new Error(`Unknown selection type: ${selection.type}`); + } +} + /** * Merge structured data from the given `source` into a given subpath of * the `destination`. The original inputs are not modified. @@ -456,7 +611,7 @@ function mergeDirect(destination: T, source: any): T { * These fragments are used to construct the root query, and also to * fetch more pages of specific entity types. */ -function createFragments(): FragmentDefinition[] { +export function createFragments(): FragmentDefinition[] { const b = build; const makePageInfo = () => b.field("pageInfo", {}, [b.field("hasNextPage"), b.field("endCursor")]); diff --git a/src/plugins/github/graphql.test.js b/src/plugins/github/graphql.test.js index 5dce47c..3b073ba 100644 --- a/src/plugins/github/graphql.test.js +++ b/src/plugins/github/graphql.test.js @@ -2,11 +2,17 @@ import type {Continuation} from "./graphql"; import {build} from "../../graphql/queries"; +import {stringify, multilineLayout} from "../../graphql/queries"; import { PAGE_LIMIT, + createQuery, + createVariables, continuationsFromQuery, continuationsFromContinuation, + createFragments, merge, + postQueryExhaustive, + requiredFragments, } from "./graphql"; describe("graphql", () => { @@ -631,4 +637,318 @@ describe("graphql", () => { }); }); }); + + describe("#postQueryExhaustive", () => { + it("finds no fragments in an empty query", () => { + const b = build; + const query = b.query("Noop", [], []); + expect(requiredFragments(query)).toEqual([]); + }); + + it("finds a fragment with no dependencies", () => { + const b = build; + const query = b.query( + "FindReviewComments", + [], + [ + b.field("node", {id: b.literal("some-user")}, [ + b.inlineFragment("Actor", [b.fragmentSpread("whoami")]), + ]), + ] + ); + const result = requiredFragments(query); + expect(result.map((fd) => fd.name).sort()).toEqual(["whoami"]); + result.forEach((fd) => expect(createFragments()).toContainEqual(fd)); + }); + + it("transitively finds dependent fragments", () => { + const b = build; + const query = b.query( + "FindReviewComments", + [], + [ + b.field("node", {id: b.literal("some-pull-request")}, [ + b.inlineFragment("PullRequest", [ + b.field( + "reviews", + { + first: b.literal(1), + }, + [b.fragmentSpread("reviews")] + ), + ]), + ]), + ] + ); + const result = requiredFragments(query); + expect(result.map((fd) => fd.name).sort()).toEqual([ + "reviewComments", + "reviews", + "whoami", + ]); + result.forEach((fd) => expect(createFragments()).toContainEqual(fd)); + }); + }); + + describe("#postQueryExhaustive", () => { + it("resolves a representative query", async () => { + const makeAuthor = (name) => ({ + __typename: "User", + login: name, + id: `opaque-user-${name}`, + }); + // We'll have three stages: + // - The original result will need more issues, and more + // comments for issue 1, and more reviews for PR 2. + // - The next result will need more issues, and comments for + // issues 1 (original issue) and 3 (new issue). + // - The final result will need no more data. + // We obey the contract pretty much exactly, except that we return + // far fewer results than are asked for by the query. + // + // Here is the response to the initial query. + const response0 = { + repository: { + id: "opaque-repo", + issues: { + pageInfo: { + hasNextPage: true, + endCursor: "opaque-cursor-issues-v0", + }, + nodes: [ + { + id: "opaque-issue1", + title: "Request for comments", + body: "Like it says, please comment!", + number: 1, + author: makeAuthor("decentralion"), + comments: { + pageInfo: { + hasNextPage: true, + endCursor: "opaque-cursor-issue1comments-v0", + }, + nodes: [ + { + id: "opaque-issue1comment1", + body: "Here: I'll start.", + url: "opaque://issue/1/comment/1", + author: makeAuthor("decentralion"), + }, + ], + }, + }, + ], + }, + pullRequests: { + pageInfo: { + hasNextPage: false, + endCursor: "opaque-cursor-prs-v0", + }, + nodes: [ + { + id: "opaque-pr2", + title: "Fix typo in README", + body: "Surely this deserves much cred.", + number: 2, + author: makeAuthor("wchargin"), + comments: { + pageInfo: { + hasNextPage: false, + endCursor: null, + }, + nodes: [], + }, + reviews: { + pageInfo: { + hasNextPage: true, + endCursor: "opaque-cursor-pr2reviews-v0", + }, + nodes: [ + { + id: "opaque-pr2review1", + body: "You actually introduced a new typo instead.", + author: makeAuthor("decentralion"), + state: "CHANGES_REQUESTED", + comments: { + pageInfo: { + hasNextPage: false, + endCursor: null, + }, + nodes: [], + }, + }, + ], + }, + }, + ], + }, + }, + }; + + // Here is the response to the continuations generated from the + // first query. + const response1 = { + _n0: { + // Requested more issues. + issues: { + pageInfo: { + hasNextPage: true, + endCursor: "opaque-cursor-issues-v1", + }, + nodes: [ + { + id: "opaque-issue3", + title: "Another", + body: "You can comment here, too.", + number: 2, + author: makeAuthor("wchargin"), + comments: { + pageInfo: { + hasNextPage: true, + endCursor: "opaque-cursor-issue3comments-v1", + }, + nodes: [ + { + id: "opaque-issue3comment1", + body: "What fun!", + url: "opaque://issue/3/comment/1", + author: makeAuthor("decentralion"), + }, + ], + }, + }, + ], + }, + }, + _n1: { + // Requested more comments for issue 1. + comments: { + pageInfo: { + hasNextPage: true, + endCursor: "opaque-cursor-issue1comments-v1", + }, + nodes: [ + { + id: "opaque-issue1comment2", + body: "Closing due to no fun allowed.", + url: "opaque://issue/1/comment/2", + author: makeAuthor("wchargin"), + }, + ], + }, + }, + _n2: { + // Requested more reviews for issue 2. + reviews: { + pageInfo: { + hasNextPage: false, + endCursor: "opaque-cursor-pr2reviews-v1", + }, + nodes: [ + { + id: "opaque-pr2review2", + body: "Looks godo to me.", + author: makeAuthor("decentralion"), + state: "APPROVED", + comments: { + pageInfo: { + hasNextPage: false, + endCursor: null, + }, + nodes: [], + }, + }, + ], + }, + }, + }; + + // Here is the response to the continuations generated from the + // second query. + const response2 = { + _n0: { + // Requested more issues. + issues: { + pageInfo: { + hasNextPage: false, + endCursor: "opaque-cursor-issues-v2", + }, + nodes: [ + { + id: "opaque-issue4", + title: "Please stop making issues", + body: "My mailbox is out of space", + number: 4, + author: makeAuthor("wchargin"), + comments: { + pageInfo: { + hasNextPage: false, + endCursor: "opaque-cursor-issue4comments-v2", + }, + nodes: [ + { + id: "opaque-issue4comment1", + body: "But you posted the last issue", + url: "opaque://issue/4/comment/1", + author: makeAuthor("decentralion"), + }, + ], + }, + }, + ], + }, + }, + _n1: { + // Requested more comments for issue 1. + comments: { + pageInfo: { + hasNextPage: false, + endCursor: "opaque-cursor-issue1comments-v2", + }, + nodes: [ + { + id: "opaque-issue1comment3", + body: "That is not very nice.", + url: "opaque://issue/1/comment/3", + author: makeAuthor("decentralion"), + }, + ], + }, + }, + _n2: { + // Requested more comments for issue 3. + comments: { + pageInfo: { + hasNextPage: false, + endCursor: "opaque-cursor-issue3comments-v2", + }, + nodes: [ + { + id: "opaque-issue3comment2", + body: "I will comment on this issue for a second time.", + url: "opaque://issue/1/comment/3", + author: makeAuthor("decentralion"), + }, + ], + }, + }, + }; + + const postQuery = jest + .fn() + .mockReturnValueOnce(Promise.resolve(response0)) + .mockReturnValueOnce(Promise.resolve(response1)) + .mockReturnValueOnce(Promise.resolve(response2)); + + const result = await postQueryExhaustive(postQuery, { + body: createQuery(), + variables: createVariables("sourcecred", "discussion"), + }); + expect(postQuery).toHaveBeenCalledTimes(3); + + // Save the result snapshot for inspection. In particular, there + // shouldn't be any nodes in the snapshot that have more pages. + expect(result).toMatchSnapshot(); + }); + }); });