Browse Source

Adds better handling of invalid credentials

Reworks timed out prs in line annotations
main
Eric Amodio 4 years ago
parent
commit
929ffa7a46
5 changed files with 121 additions and 42 deletions
  1. +36
    -31
      src/annotations/lineAnnotationController.ts
  2. +12
    -1
      src/credentials.ts
  3. +17
    -8
      src/git/remotes/github.ts
  4. +45
    -1
      src/git/remotes/provider.ts
  5. +11
    -1
      src/github/github.ts

+ 36
- 31
src/annotations/lineAnnotationController.ts View File

@ -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<string, PullRequest | undefined> }) {
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<string, Promise<PullRequest | undefined>> | 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<string, PullRequest | undefined>();
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 });
}
}

+ 12
- 1
src/credentials.ts View File

@ -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<CredentialChangeEvent>();
@ -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' });
}
}

+ 17
- 8
src/git/remotes/github.ts View File

@ -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();
});

+ 45
- 1
src/git/remotes/provider.ts View File

@ -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<T>(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();
}
}
}

+ 11
- 1
src/github/github.ts View File

@ -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;
}
}

Loading…
Cancel
Save