Require variables for legacy GraphQL queries (#8889)

fd5f8d8 allowed legacy queries to replace their variables, but
left the fallback variables optional. PullRequestComments supplied
only a query, so retrying discarded owner, name, number, and the
pagination cursor and failed with invalid-variable errors.

Require variables whenever a legacy query is supplied. Pass the
review-comment variables explicitly on every page, while preserving
separate argument maps for queries with different inputs. Cover
replacement and legacy pagination at the repository boundary.

Handle missing repository data before reading review threads, so a
missing response produces the intended diagnostic.

Author: tamird

src/github/githubRepository.ts

@@ -328,7 +328,7 @@ export class GitHubRepository extends Disposable {
 		}
 	}
 
-	query = async <T>(query: QueryOptions, ignoreSamlErrors: boolean = false, legacyFallback?: { query: DocumentNode, variables?: OperationVariables }): Promise<ApolloQueryResult<T>> => {
+	query = async <T>(query: QueryOptions, ignoreSamlErrors: boolean = false, legacyFallback?: { query: DocumentNode, variables: OperationVariables }): Promise<ApolloQueryResult<T>> => {
 		const gql = this.authMatchesServer && this.hub && this.hub.graphql;
 		if (!gql) {
 			const logValue = (query.query.definitions[0] as { name: { value: string } | undefined }).name?.value;

src/github/pullRequestModel.ts

@@ -1497,16 +1497,20 @@ export class PullRequestModel extends IssueModel<PullRequest> implements IPullRe
 		const reviewThreads: ReviewThread[] = [];
 		try {
 			do {
+				const variables = {
+					owner: remote.owner,
+					name: remote.repositoryName,
+					number: this.number,
+					after,
+				};
 				const { data } = await query<PullRequestCommentsResponse>({
 					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;

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';

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(