diff --git a/src/github/githubRepository.ts b/src/github/githubRepository.ts index fb6abd413d..976c1e9edf 100644 --- a/src/github/githubRepository.ts +++ b/src/github/githubRepository.ts @@ -328,7 +328,7 @@ export class GitHubRepository extends Disposable { } } - query = async (query: QueryOptions, ignoreSamlErrors: boolean = false, legacyFallback?: { query: DocumentNode, variables?: OperationVariables }): Promise> => { + query = async (query: QueryOptions, ignoreSamlErrors: boolean = false, legacyFallback?: { query: DocumentNode, variables: OperationVariables }): Promise> => { const gql = this.authMatchesServer && this.hub && this.hub.graphql; if (!gql) { const logValue = (query.query.definitions[0] as { name: { value: string } | undefined }).name?.value; diff --git a/src/github/pullRequestModel.ts b/src/github/pullRequestModel.ts index 12baa0c246..1c7ad1eaa8 100644 --- a/src/github/pullRequestModel.ts +++ b/src/github/pullRequestModel.ts @@ -1497,16 +1497,20 @@ export class PullRequestModel extends IssueModel implements IPullRe const reviewThreads: ReviewThread[] = []; try { do { + const variables = { + owner: remote.owner, + name: remote.repositoryName, + number: this.number, + after, + }; const { data } = await query({ query: schema.PullRequestComments, - variables: { - owner: remote.owner, - name: remote.repositoryName, - number: this.number, - after - }, - }, false, { query: schema.LegacyPullRequestComments }); + variables, + }, false, { query: schema.LegacyPullRequestComments, variables }); + if (!data?.repository) { + throw new Error('Review comments response did not include a repository.'); + } reviewThreads.push(...data.repository.pullRequest.reviewThreads.nodes); hasNextPage = data.repository.pullRequest.reviewThreads.pageInfo.hasNextPage; diff --git a/src/test/github/githubRepository.test.ts b/src/test/github/githubRepository.test.ts index 946c2f7c54..1dbc91a3b9 100644 --- a/src/test/github/githubRepository.test.ts +++ b/src/test/github/githubRepository.test.ts @@ -4,6 +4,7 @@ *--------------------------------------------------------------------------------------------*/ import { default as assert } from 'assert'; +import { NetworkStatus } from 'apollo-boost'; import { SinonSandbox, createSandbox } from 'sinon'; import { CredentialStore } from '../../github/credentials'; import { MockCommandRegistry } from '../mocks/mockCommandRegistry'; @@ -18,6 +19,7 @@ import { GitHubServerType } from '../../common/authentication'; import { CheckState, PullRequestCheckStatus } from '../../github/interface'; import { PullRequestBuilder as GraphQLPullRequestBuilder } from '../builders/graphql/pullRequestBuilder'; import Logger from '../../common/logger'; +import { LoggingApolloClient, LoggingOctokit } from '../../github/loggingOctokit'; describe('GitHubRepository', function () { let sinon: SinonSandbox; @@ -38,6 +40,36 @@ describe('GitHubRepository', function () { sinon.restore(); }); + describe('query', function () { + it('replaces variables for a legacy query with different arguments', async function () { + const url = 'https://github.com/some/repo'; + const remote = new GitHubRemote('origin', url, new Protocol(url), GitHubServerType.GitHubDotCom); + const repo = new GitHubRepository(1, remote, Uri.file('/workspaces/repo'), credentialStore, telemetry, true); + const graphql = sinon.createStubInstance(LoggingApolloClient); + sinon.stub(credentialStore, 'isAuthenticated').returns(true); + sinon.stub(repo, 'hub').get(() => ({ graphql, octokit: sinon.createStubInstance(LoggingOctokit) })); + const variables = { owner: 'some', name: 'repo', first: 100, after: 'cursor' }; + const response = { data: {}, loading: false, stale: false, networkStatus: NetworkStatus.ready }; + graphql.query.onFirstCall().rejects(new Error('Unsupported query')); + graphql.query.onSecondCall().resolves(response); + + try { + const result = await repo.query({ + query: repo.schema.GetSuggestedActors, + variables: { ...variables, capabilities: ['CAN_BE_ASSIGNED'] }, + }, false, { query: repo.schema.GetAssignableUsers, variables }); + + assert.strictEqual(result, response); + assert.strictEqual(graphql.query.callCount, 2); + const [fallback] = graphql.query.secondCall.args; + assert.strictEqual(fallback.query, repo.schema.GetAssignableUsers); + assert.deepStrictEqual(fallback.variables, variables); + } finally { + repo.dispose(); + } + }); + }); + describe('isGitHubDotCom', function () { it('detects when the remote is pointing to github.com', function () { const url = 'https://github.com/some/repo'; diff --git a/src/test/github/pullRequestModel.test.ts b/src/test/github/pullRequestModel.test.ts index 5497cde4ab..14d1c86ae1 100644 --- a/src/test/github/pullRequestModel.test.ts +++ b/src/test/github/pullRequestModel.test.ts @@ -19,6 +19,9 @@ import { NetworkStatus } from 'apollo-client'; import { MockExtensionContext } from '../mocks/mockExtensionContext'; import { GitHubServerType } from '../../common/authentication'; import { mergeQuerySchemaWithShared } from '../../github/common'; +import { GitHubRepository } from '../../github/githubRepository'; +import { LoggingApolloClient, LoggingOctokit } from '../../github/loggingOctokit'; +import Logger from '../../common/logger'; const queries = mergeQuerySchemaWithShared(require('../../github/queries.gql'), require('../../github/queriesShared.gql')) as any; const telemetry = new MockTelemetry(); @@ -96,6 +99,67 @@ describe('PullRequestModel', function () { }); describe('reviewThreadCache', function () { + function page(id: string, endCursor: string | null) { + return { + data: { + repository: { + pullRequest: { + reviewThreads: { + nodes: [{ ...reviewThreadResponse, id }], + pageInfo: { hasNextPage: endCursor !== null, endCursor }, + }, + }, + }, + }, + loading: false, + stale: false, + networkStatus: NetworkStatus.ready, + }; + } + + it('passes review comment variables to every legacy page', async function () { + const repository = new GitHubRepository(1, remote, repo.rootUri, credentials, telemetry, true); + const graphql = sinon.createStubInstance(LoggingApolloClient); + sinon.stub(credentials, 'isAuthenticated').returns(true); + sinon.stub(repository, 'hub').get(() => ({ graphql, octokit: sinon.createStubInstance(LoggingOctokit) })); + sinon.stub(repository, 'ensure').resolves(repository); + graphql.query.onCall(0).rejects(new Error('Unsupported query')); + graphql.query.onCall(1).resolves(page('1', 'first')); + graphql.query.onCall(2).rejects(new Error('Unsupported query')); + graphql.query.onCall(3).resolves(page('2', null)); + + try { + const pr = new PullRequestBuilder().build(); + const model = new PullRequestModel(credentials, telemetry, repository, remote, convertRESTPullRequestToRawPullRequest(pr, repository)); + const threads = await model.getReviewThreads(); + + assert.deepStrictEqual(threads.map(thread => thread.id), ['1', '2']); + assert.strictEqual(graphql.query.callCount, 4); + for (const [call, after] of [[graphql.query.secondCall, null], [graphql.query.lastCall, 'first']] as const) { + const [fallback] = call.args; + assert.strictEqual(fallback.query, repository.schema.LegacyPullRequestComments); + assert.deepStrictEqual(fallback.variables, { + owner: remote.owner, name: remote.repositoryName, number: pr.number, after, + }); + } + } finally { + repository.dispose(); + } + }); + + it('reports missing review data without retrying', async function () { + const pr = new PullRequestBuilder().build(); + const model = new PullRequestModel(credentials, telemetry, repo, remote, convertRESTPullRequestToRawPullRequest(pr, repo)); + const query = sinon.stub(repo, 'query').resolves({ + data: null, loading: false, stale: false, networkStatus: NetworkStatus.error, + }); + const error = sinon.stub(Logger, 'error'); + + assert.deepStrictEqual(await model.getReviewThreads(), []); + assert.strictEqual(query.callCount, 1); + assert.strictEqual(error.lastCall.args[0], 'Failed to get pull request review comments: Error: Review comments response did not include a repository.'); + }); + it('should update the cache when then cache is initialized', async function () { const pr = new PullRequestBuilder().build(); const model = new PullRequestModel(