From 929ffa7a46c966cede61435bfa9c857c71d9c72c Mon Sep 17 00:00:00 2001 From: Eric Amodio Date: Sat, 13 Jun 2020 01:56:31 -0400 Subject: [PATCH] Adds better handling of invalid credentials Reworks timed out prs in line annotations --- src/annotations/lineAnnotationController.ts | 67 ++++++++++++++++------------- src/credentials.ts | 13 +++++- src/git/remotes/github.ts | 25 +++++++---- src/git/remotes/provider.ts | 46 +++++++++++++++++++- src/github/github.ts | 12 +++++- 5 files changed, 121 insertions(+), 42 deletions(-) diff --git a/src/annotations/lineAnnotationController.ts b/src/annotations/lineAnnotationController.ts index cab0a66..55215f6 100644 --- a/src/annotations/lineAnnotationController.ts +++ b/src/annotations/lineAnnotationController.ts @@ -9,14 +9,14 @@ import { TextEditorDecorationType, window, } from 'vscode'; +import { Annotations } from './annotations'; import { configuration } from '../configuration'; import { GlyphChars, isTextEditor } from '../constants'; import { Container } from '../container'; -import { LinesChangeEvent } from '../trackers/gitLineTracker'; -import { Annotations } from './annotations'; +import { CommitFormatter, GitBlameCommit, PullRequest } from '../git/git'; +import { LogCorrelationContext, Logger } from '../logger'; import { debug, Iterables, log, Promises } from '../system'; -import { Logger } from '../logger'; -import { CommitFormatter, GitBlameCommit } from '../git/git'; +import { LinesChangeEvent } from '../trackers/gitLineTracker'; const annotationDecoration: TextEditorDecorationType = window.createTextEditorDecorationType({ after: { @@ -174,7 +174,7 @@ export class LineAnnotationController implements Disposable { } @debug({ args: false }) - private async refresh(editor: TextEditor | undefined) { + private async refresh(editor: TextEditor | undefined, options?: { prs?: Map }) { if (editor === undefined && this._editor === undefined) return; const cc = Logger.getCorrelationContext(); @@ -250,7 +250,6 @@ export class LineAnnotationController implements Disposable { // TODO: Make this configurable? const timeout = 100; - const [getBranchAndTagTips, prs] = await Promise.all([ CommitFormatter.has(cfg.format, 'tips') ? Container.git.getBranchesAndTagsTipsFn(repoPath) : undefined, repoPath != null && @@ -263,7 +262,8 @@ export class LineAnnotationController implements Disposable { 'pullRequestDate', 'pullRequestState', ) - ? this.getPullRequests( + ? options?.prs ?? + this.getPullRequests( repoPath, commitLines.filter(([, commit]) => !commit.isUncommitted), { timeout: timeout }, @@ -271,30 +271,8 @@ export class LineAnnotationController implements Disposable { : undefined, ]); - if (prs !== undefined) { - const timeouts = [ - ...Iterables.filterMap(prs.values(), pr => - pr instanceof Promises.CancellationError ? pr.promise : undefined, - ), - ]; - - // If there are any PRs that timed out, refresh the annotation(s) once they complete - if (timeouts.length !== 0) { - Logger.debug( - cc, - `${GlyphChars.Dot} pull request queries (${timeouts.length}) took too long (over ${timeout} ms)`, - ); - void Promise.all(timeouts).then(() => { - if (editor === this._editor) { - Logger.debug( - cc, - `${GlyphChars.Dot} pull request queries (${timeouts.length}) completed; refreshing...`, - ); - - void this.refresh(editor); - } - }); - } + if (prs != null) { + void this.waitForAnyPendingPullRequests(editor, prs, timeout, cc); } const decorations = []; @@ -336,4 +314,31 @@ export class LineAnnotationController implements Disposable { Container.lineTracker.stop(this); } + + private async waitForAnyPendingPullRequests( + editor: TextEditor, + prs: Map< + string, + PullRequest | Promises.CancellationErrorWithId> | undefined + >, + timeout: number, + cc: LogCorrelationContext | undefined, + ) { + // If there are any PRs that timed out, refresh the annotation(s) once they complete + const count = Iterables.count(prs.values(), pr => pr instanceof Promises.CancellationError); + Logger.debug(cc, `${GlyphChars.Dot} ${count} pull request queries took too long (over ${timeout} ms)`); + + if (count === 0) return; + + const resolved = new Map(); + for (const [key, value] of prs) { + resolved.set(key, value instanceof Promises.CancellationError ? await value.promise : value); + } + + if (editor !== this._editor) return; + + Logger.debug(cc, `${GlyphChars.Dot} ${count} pull request queries completed; refreshing...`); + + void this.refresh(editor, { prs: resolved }); + } } diff --git a/src/credentials.ts b/src/credentials.ts index dbe3143..c001b90 100644 --- a/src/credentials.ts +++ b/src/credentials.ts @@ -29,7 +29,12 @@ interface CredentialClearEvent { reason: 'clear'; } -export type CredentialChangeEvent = CredentialSaveEvent | CredentialClearEvent; +interface CredentialInvalidEvent { + key: string | undefined; + reason: 'invalid'; +} + +export type CredentialChangeEvent = CredentialSaveEvent | CredentialClearEvent | CredentialInvalidEvent; export namespace CredentialManager { const _onDidChange = new EventEmitter(); @@ -89,4 +94,10 @@ export namespace CredentialManager { return JSON.parse(value) as T; } + + export function invalidate(key: string) { + if (!key) return; + + _onDidChange.fire({ key: key, reason: 'invalid' }); + } } diff --git a/src/git/remotes/github.ts b/src/git/remotes/github.ts index 8d50e78..22e21f3 100644 --- a/src/git/remotes/github.ts +++ b/src/git/remotes/github.ts @@ -11,9 +11,14 @@ const issueEnricher3rdParyRegex = /\b(\w+\\?-?\w+(?!\\?-)\/\w+\\?-?\w+(?!\\?-))\ export class GitHubRemote extends RemoteProviderWithApi<{ token: string }> { private readonly Buttons = class { - static readonly Help: QuickInputButton = { - iconPath: new ThemeIcon('question'), - tooltip: 'Help', + // static readonly Help: QuickInputButton = { + // iconPath: new ThemeIcon('question'), + // tooltip: 'Help', + // }; + + static readonly OpenPATs: QuickInputButton = { + iconPath: new ThemeIcon('globe'), + tooltip: 'Open Personal Access Tokens on GitHub', }; }; @@ -72,10 +77,14 @@ export class GitHubRemote extends RemoteProviderWithApi<{ token: string }> { disposable = Disposable.from( input.onDidHide(() => resolve(undefined)), input.onDidTriggerButton(e => { - if (e === this.Buttons.Help) { - // TODO@eamodio link to proper wiki - void env.openExternal(Uri.parse('https://github.com/eamodio/vscode-gitlens/wiki')); + if (e === this.Buttons.OpenPATs) { + void env.openExternal(Uri.parse('https://github.com/settings/tokens')); } + + // if (e === this.Buttons.Help) { + // // TODO@eamodio link to proper wiki + // void env.openExternal(Uri.parse('https://github.com/eamodio/vscode-gitlens/wiki')); + // } }), input.onDidChangeValue( e => @@ -88,10 +97,10 @@ export class GitHubRemote extends RemoteProviderWithApi<{ token: string }> { ); // TODO@eamodio add this button once we have a valid help link above - // input.buttons = [this.Buttons.Help]; + input.buttons = [this.Buttons.OpenPATs]; // [this.Buttons.Help]; input.title = `Connect to ${this.name}`; input.prompt = 'Enter a GitHub personal access token'; - input.placeholder = 'Generate a personal access token from github.com (required)'; + input.placeholder = 'Generate a personal access token (with repo access) from github.com (required)'; input.show(); }); diff --git a/src/git/remotes/provider.ts b/src/git/remotes/provider.ts index 7b8221b..c7fe88c 100644 --- a/src/git/remotes/provider.ts +++ b/src/git/remotes/provider.ts @@ -11,6 +11,14 @@ import { GitLogCommit } from '../models/logCommit'; import { PullRequest } from '../models/pullRequest'; import { debug, gate, Promises } from '../../system'; +export class CredentialError extends Error { + constructor(private original: Error) { + super(original.message); + + Error.captureStackTrace(this, CredentialError); + } +} + export enum RemoteResourceType { Branch = 'branch', Branches = 'branches', @@ -197,6 +205,8 @@ export abstract class RemoteProviderWithApi extends Remo return this._onDidChange.event; } + private badCredentialsCount = 0; + constructor(domain: string, path: string, protocol?: string, name?: string, custom?: boolean) { super(domain, path, protocol, name, custom); @@ -204,6 +214,13 @@ export abstract class RemoteProviderWithApi extends Remo } private onCredentialsChanged(e: CredentialChangeEvent) { + if (e.reason === 'invalid' && e.key === this.credentialsKey) { + this._credentials = null; + this._onDidChange.fire(); + + return; + } + if (e.reason === 'save' && e.key === this.credentialsKey) { if (this._credentials === null) { this._credentials = undefined; @@ -253,10 +270,15 @@ export abstract class RemoteProviderWithApi extends Remo if (!connected) return undefined; try { - return await this.onGetIssueOrPullRequest(this._credentials!, id); + const issueOrPullRequest = await this.onGetIssueOrPullRequest(this._credentials!, id); + this.badCredentialsCount = 0; + return issueOrPullRequest; } catch (ex) { Logger.error(ex, cc); + if (ex instanceof CredentialError) { + this.handleBadCredentials(); + } return undefined; } } @@ -283,6 +305,7 @@ export abstract class RemoteProviderWithApi extends Remo protected credentials() { if (this._credentials === undefined) { return CredentialManager.getAs(this.credentialsKey).then(c => { + this.badCredentialsCount = 0; this._credentials = c ?? null; return c ?? undefined; }); @@ -291,12 +314,20 @@ export abstract class RemoteProviderWithApi extends Remo } protected async clearCredentials() { + this.badCredentialsCount = 0; this._credentials = undefined; await CredentialManager.clear(this.credentialsKey); this._credentials = undefined; } + protected invalidateCredentials() { + this.badCredentialsCount = 0; + this._credentials = null; + CredentialManager.invalidate(this.credentialsKey); + } + protected saveCredentials(credentials: T) { + this.badCredentialsCount = 0; this._credentials = credentials; return CredentialManager.addOrUpdate(this.credentialsKey, credentials); } @@ -316,12 +347,25 @@ export abstract class RemoteProviderWithApi extends Remo try { const pr = (await this.onGetPullRequestForCommit(this._credentials!, ref)) ?? null; this._prsByCommit.set(ref, pr); + this.badCredentialsCount = 0; return pr; } catch (ex) { Logger.error(ex, cc); this._prsByCommit.delete(ref); + + if (ex instanceof CredentialError) { + this.handleBadCredentials(); + } return null; } } + + private handleBadCredentials() { + this.badCredentialsCount++; + + if (this.badCredentialsCount >= 5) { + this.invalidateCredentials(); + } + } } diff --git a/src/github/github.ts b/src/github/github.ts index 5f293cc..b02e454 100644 --- a/src/github/github.ts +++ b/src/github/github.ts @@ -2,7 +2,7 @@ import { graphql } from '@octokit/graphql'; import { Logger } from '../logger'; import { debug } from '../system'; -import { IssueOrPullRequest, PullRequest, PullRequestState } from '../git/git'; +import { CredentialError, IssueOrPullRequest, PullRequest, PullRequestState } from '../git/git'; export class GitHubApi { @debug({ @@ -64,6 +64,7 @@ export class GitHubApi { headers: { authorization: `Bearer ${token}` }, ...options, }); + const pr = rsp?.repository?.object?.associatedPullRequests?.nodes?.[0]; if (pr == null) return undefined; // GitHub seems to sometimes return PRs for forks @@ -85,6 +86,10 @@ export class GitHubApi { ); } catch (ex) { Logger.error(ex, cc); + + if (ex.code === 401) { + throw new CredentialError(ex); + } throw ex; } } @@ -135,6 +140,7 @@ export class GitHubApi { headers: { authorization: `Bearer ${token}` }, ...options, }); + const issue = rsp?.repository?.issueOrPullRequest; if (issue == null) return undefined; @@ -149,6 +155,10 @@ export class GitHubApi { }; } catch (ex) { Logger.error(ex, cc); + + if (ex.code === 401) { + throw new CredentialError(ex); + } throw ex; } }