diff --git a/src/gitProviders/GitHubContactServiceProvider.ts b/src/gitProviders/GitHubContactServiceProvider.ts index bc3973cfef..1d88f69a8e 100644 --- a/src/gitProviders/GitHubContactServiceProvider.ts +++ b/src/gitProviders/GitHubContactServiceProvider.ts @@ -125,7 +125,7 @@ export class GitHubContactServiceProvider implements ContactServiceProvider { } const origin = await this.pullRequestManager.folderManagers[0]?.getOrigin(); if (origin) { - const currentUser = origin.hub.currentUser ? await origin.hub.currentUser : undefined; + const currentUser = await origin.getAuthenticatedUser(); if (currentUser) { return currentUser.login; } diff --git a/src/github/credentials.ts b/src/github/credentials.ts index e1e9ae9ee1..e5d42b8c4d 100644 --- a/src/github/credentials.ts +++ b/src/github/credentials.ts @@ -551,19 +551,25 @@ export class CredentialStore extends Disposable { } public async isCurrentUser(authProviderId: AuthProvider, username: string): Promise { - const api = authProviderId === AuthProvider.github ? this._githubAPI : this._githubEnterpriseAPI; - return (await api?.currentUser)?.login === username; + return (await this.getCurrentUser(authProviderId))?.login === username; } public async getIsEmu(authProviderId: AuthProvider): Promise { const github = this.getHub(authProviderId); + this.ensureCurrentUser(github); return !!(await github?.isEmu); } public getCurrentUser(authProviderId: AuthProvider): Promise { const github = this.getHub(authProviderId); - const octokit = github?.octokit; - return (octokit && github?.currentUser)!; + this.ensureCurrentUser(github); + return github?.currentUser!; + } + + private ensureCurrentUser(github: GitHub | undefined): void { + if (github && (!github.currentUser || !github.isEmu)) { + this.setCurrentUser(github); + } } private setCurrentUser(github: GitHub): void { @@ -577,8 +583,27 @@ export class CredentialStore extends Disposable { reject(e); }); }); - github.currentUser = getUser.then(result => convertRESTUserToAccount(result.data)); - github.isEmu = getUser.then(result => result.data.plan?.name === 'emu_user'); + let currentUser: Promise; + let isEmu: Promise; + const clearFailedRequest = () => { + if (github.currentUser === currentUser && github.isEmu === isEmu) { + github.currentUser = undefined; + github.isEmu = undefined; + } + }; + currentUser = getUser.then(result => convertRESTUserToAccount(result.data), e => { + clearFailedRequest(); + throw e; + }); + isEmu = getUser.then(result => result.data.plan?.name === 'emu_user', e => { + clearFailedRequest(); + throw e; + }); + github.currentUser = currentUser; + github.isEmu = isEmu; + + void currentUser.catch(() => undefined); + void isEmu.catch(() => undefined); } private async getSession(authProviderId: AuthProvider, getAuthSessionOptions: vscode.AuthenticationGetSessionOptions, scopes: string[], requireScopes: boolean): Promise<{ session: vscode.AuthenticationSession | undefined, isNew: boolean, scopes: string[] }> { diff --git a/src/github/githubRepository.ts b/src/github/githubRepository.ts index fb6abd413d..7ed48ec22f 100644 --- a/src/github/githubRepository.ts +++ b/src/github/githubRepository.ts @@ -424,7 +424,7 @@ export class GitHubRepository extends Disposable { repo }); Logger.debug(`Fetch metadata for repo ${owner}/${repo} - done`, this.id); - const metadata = { ...result.data, currentUser: await this._hub?.currentUser }; + const metadata = { ...result.data, currentUser: await this.getAuthenticatedUser() }; return metadata; } @@ -437,15 +437,22 @@ export class GitHubRepository extends Disposable { Logger.debug(`Fetch metadata - enter`, this.id); const { remote } = await this.ensure(); - this._metadata = this.getMetadataForRepo(remote.owner, remote.repositoryName).catch(e => { - if ((getErrorCode(e) === '404') && !isSamlError(e) && !this._isInaccessible) { - this._isInaccessible = true; - Logger.warn(`Repository ${remote.owner}/${remote.repositoryName} from remote ${remote.remoteName} in workspace folder ${this.rootUri.fsPath} returned HTTP 404 and will be skipped for this session.`, this.id); + const metadata = this.getMetadataForRepo(remote.owner, remote.repositoryName).catch(e => { + if (this._metadata === metadata) { + if ((getErrorCode(e) === '404') && !isSamlError(e)) { + if (!this._isInaccessible) { + this._isInaccessible = true; + Logger.warn(`Repository ${remote.owner}/${remote.repositoryName} from remote ${remote.remoteName} in workspace folder ${this.rootUri.fsPath} returned HTTP 404 and will be skipped for this session.`, this.id); + } + } else { + this._metadata = undefined; + } } throw e; }); + this._metadata = metadata; Logger.debug(`Fetch metadata ${remote.owner}/${remote.repositoryName} - done`, this.id); - return this._metadata; + return metadata; } /** diff --git a/src/github/pullRequestOverview.ts b/src/github/pullRequestOverview.ts index c39e55f05d..3b8d07f7fc 100644 --- a/src/github/pullRequestOverview.ts +++ b/src/github/pullRequestOverview.ts @@ -44,6 +44,12 @@ import { asPromise, formatError } from '../common/utils'; import { IRequestMessage, PULL_REQUEST_OVERVIEW_VIEW_TYPE } from '../common/webview'; import { toCheckRunLogUri } from '../view/checkRunLogContentProvider'; +function withErrorContext(operation: string, promise: Promise): Promise { + return promise.catch(e => { + throw new Error(`${operation} failed: ${formatError(e)}`); + }); +} + export class PullRequestOverviewPanel extends IssueOverviewPanel { public static override ID: string = 'PullRequestOverviewPanel'; public static override readonly viewType = PULL_REQUEST_OVERVIEW_VIEW_TYPE; @@ -344,27 +350,27 @@ export class PullRequestOverviewPanel extends IssueOverviewPanel { if (this._updatingPromise === clearingPromise) { @@ -482,7 +488,7 @@ export class PullRequestOverviewPanel extends IssueOverviewPanel candidate === error); + strictEqual(getAuthenticatedUser.callCount, 1); + + const [currentUser, isEmu] = await Promise.all([ + credentialStore.getCurrentUser(AuthProvider.github), + credentialStore.getIsEmu(AuthProvider.github), + ]); + + deepStrictEqual({ + requests: getAuthenticatedUser.callCount, + login: currentUser.login, + isEmu, + }, { + requests: 2, + login: 'octocat', + isEmu: true, + }); + strictEqual(await credentialStore.isCurrentUser(AuthProvider.github, 'octocat'), true); + strictEqual(getAuthenticatedUser.callCount, 2); + }); }); diff --git a/src/test/github/githubRepository.test.ts b/src/test/github/githubRepository.test.ts index 946c2f7c54..515a93764a 100644 --- a/src/test/github/githubRepository.test.ts +++ b/src/test/github/githubRepository.test.ts @@ -56,6 +56,25 @@ describe('GitHubRepository', function () { }); }); + describe('getMetadata', function () { + it('retries after a transient failure and caches the successful result', 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); + sinon.stub(repo, 'ensure').resolves(repo); + const fetchMetadata = sinon.stub(repo as any, 'getMetadataForRepo'); + const error = new Error('Connect Timeout Error'); + const metadata = { name: 'repo', owner: { login: 'some' } }; + fetchMetadata.onFirstCall().rejects(error); + fetchMetadata.onSecondCall().resolves(metadata); + + await assert.rejects(repo.getMetadata(), candidate => candidate === error); + assert.strictEqual(await repo.getMetadata(), metadata); + assert.strictEqual(await repo.getMetadata(), metadata); + assert.strictEqual(fetchMetadata.callCount, 2); + }); + }); + describe('resolveRemote', function () { beforeEach(function () { sinon.stub(credentialStore, 'isAuthenticated').returns(true); diff --git a/src/test/github/pullRequestOverview.test.ts b/src/test/github/pullRequestOverview.test.ts index d0469014fa..a7afa1d237 100644 --- a/src/test/github/pullRequestOverview.test.ts +++ b/src/test/github/pullRequestOverview.test.ts @@ -99,6 +99,27 @@ describe('PullRequestOverview', function () { assert.notStrictEqual(PullRequestOverviewPanel.findPanel('aaa', 'bbb', 1000), undefined); }); + it('identifies the operation that failed while updating', async function () { + repo.addGraphQLPullRequest(builder => { + builder.pullRequest(response => { + response.repository(r => { + r.pullRequest(pr => pr.number(1000)); + }); + }); + }); + + const prItem = convertRESTPullRequestToRawPullRequest(new PullRequestBuilder().number(1000).build(), repo); + const prModel = new PullRequestModel(credentialStore, telemetry, repo, remote, prItem); + const identity = { owner: prModel.remote.owner, repo: prModel.remote.repositoryName, number: prModel.number }; + sinon.stub(pullRequestManager, 'getCurrentUser').rejects(new Error('Connect Timeout Error')); + const showErrorMessage = sinon.stub(vscode.window, 'showErrorMessage'); + + await PullRequestOverviewPanel.createOrShow(telemetry, EXTENSION_URI, pullRequestManager, identity, prModel); + + assert.strictEqual(showErrorMessage.callCount, 1); + assert.strictEqual(showErrorMessage.firstCall.args[0], 'Error updating pull request: Fetching current user failed: Connect Timeout Error'); + }); + it('reveals an existing panel for the same PR', async function () { const createWebviewPanel = sinon.spy(vscode.window, 'createWebviewPanel');