From 3a85e918b43dc8027147c1785e13b56c926a73b2 Mon Sep 17 00:00:00 2001 From: Tamir Duberstein Date: Thu, 20 Aug 2026 16:46:42 -0400 Subject: [PATCH] Require variables for legacy GraphQL queries fd5f8d8f643 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. --- src/github/githubRepository.ts | 2 +- src/github/pullRequestModel.ts | 18 ++++--- src/test/github/githubRepository.test.ts | 32 ++++++++++++ src/test/github/pullRequestModel.test.ts | 64 ++++++++++++++++++++++++ 4 files changed, 108 insertions(+), 8 deletions(-) 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(