From 8ae14669fbf7469c8674c590842bbe7a519fdca4 Mon Sep 17 00:00:00 2001 From: Eric Amodio Date: Tue, 18 Jan 2022 02:39:33 -0500 Subject: [PATCH] Adds better provider (GitHub) error handling --- .vscode/launch.json | 3 +- src/errors.ts | 12 +++++- src/git/models/log.ts | 1 + src/git/remotes/provider.ts | 14 +++---- src/github/github.ts | 99 ++++++++++++++++++++++----------------------- 5 files changed, 68 insertions(+), 61 deletions(-) diff --git a/.vscode/launch.json b/.vscode/launch.json index efb77e7..76a3f3f 100644 --- a/.vscode/launch.json +++ b/.vscode/launch.json @@ -78,6 +78,7 @@ "request": "launch", "runtimeExecutable": "${execPath}", "args": [ + // "--folder-uri=vscode-vfs://github/gitkraken/vscode-gitlens", "--disable-extension=eamodio.gitlens-insiders", "--extensionDevelopmentPath=${workspaceFolder}", "--extensionDevelopmentKind=web" @@ -88,7 +89,7 @@ "sourceMaps": true, "webRoot": "${workspaceFolder}/src" }, - "outFiles": ["${workspaceFolder}/dist/**/*.js"], + "outFiles": ["${workspaceFolder}/dist/browser/**/*.js"], "preLaunchTask": "${defaultBuildTask}", "presentation": { "group": "1_watch", diff --git a/src/errors.ts b/src/errors.ts index 64967fa..38e0e70 100644 --- a/src/errors.ts +++ b/src/errors.ts @@ -53,10 +53,18 @@ export class ProviderNotFoundError extends Error { } } -export class ProviderRequestError extends Error { +export class ProviderRequestClientError extends Error { constructor(public readonly original: Error) { super(original.message); - Error.captureStackTrace?.(this, ProviderRequestError); + Error.captureStackTrace?.(this, ProviderRequestClientError); + } +} + +export class ProviderRequestNotFoundError extends Error { + constructor(public readonly original: Error) { + super(original.message); + + Error.captureStackTrace?.(this, ProviderRequestNotFoundError); } } diff --git a/src/git/models/log.ts b/src/git/models/log.ts index 5adf270..eaacd5d 100644 --- a/src/git/models/log.ts +++ b/src/git/models/log.ts @@ -14,6 +14,7 @@ export interface GitLog { readonly count: number; readonly limit: number | undefined; readonly hasMore: boolean; + readonly cursor?: string; query?(limit: number | undefined): Promise; more?(limit: number | { until?: string } | undefined): Promise; diff --git a/src/git/remotes/provider.ts b/src/git/remotes/provider.ts index 15f8cf6..a2ac620 100644 --- a/src/git/remotes/provider.ts +++ b/src/git/remotes/provider.ts @@ -13,7 +13,7 @@ import { DynamicAutolinkReference } from '../../annotations/autolinks'; import { AutolinkReference } from '../../config'; import { WorkspaceState } from '../../constants'; import { Container } from '../../container'; -import { AuthenticationError, ProviderRequestError } from '../../errors'; +import { AuthenticationError, ProviderRequestClientError } from '../../errors'; import { Logger } from '../../logger'; import { debug, Encoding, gate, log, Promises } from '../../system'; import { @@ -381,7 +381,7 @@ export abstract class RichRemoteProvider extends RemoteProvider { } catch (ex) { Logger.error(ex, cc); - if (ex instanceof ProviderRequestError || ex instanceof AuthenticationError) { + if (ex instanceof AuthenticationError || ex instanceof ProviderRequestClientError) { this.handleClientException(); } return undefined; @@ -416,7 +416,7 @@ export abstract class RichRemoteProvider extends RemoteProvider { } catch (ex) { Logger.error(ex, cc); - if (ex instanceof ProviderRequestError || ex instanceof AuthenticationError) { + if (ex instanceof AuthenticationError || ex instanceof ProviderRequestClientError) { this.handleClientException(); } return undefined; @@ -446,7 +446,7 @@ export abstract class RichRemoteProvider extends RemoteProvider { } catch (ex) { Logger.error(ex, cc); - if (ex instanceof ProviderRequestError || ex instanceof AuthenticationError) { + if (ex instanceof AuthenticationError || ex instanceof ProviderRequestClientError) { this.handleClientException(); } return undefined; @@ -472,7 +472,7 @@ export abstract class RichRemoteProvider extends RemoteProvider { } catch (ex) { Logger.error(ex, cc); - if (ex instanceof ProviderRequestError || ex instanceof AuthenticationError) { + if (ex instanceof AuthenticationError || ex instanceof ProviderRequestClientError) { this.handleClientException(); } return undefined; @@ -505,7 +505,7 @@ export abstract class RichRemoteProvider extends RemoteProvider { } catch (ex) { Logger.error(ex, cc); - if (ex instanceof ProviderRequestError || ex instanceof AuthenticationError) { + if (ex instanceof AuthenticationError || ex instanceof ProviderRequestClientError) { this.handleClientException(); } return undefined; @@ -552,7 +552,7 @@ export abstract class RichRemoteProvider extends RemoteProvider { this._prsByCommit.delete(ref); - if (ex instanceof ProviderRequestError || ex instanceof AuthenticationError) { + if (ex instanceof AuthenticationError || ex instanceof ProviderRequestClientError) { this.handleClientException(); } return null; diff --git a/src/github/github.ts b/src/github/github.ts index d997a21..350f2d5 100644 --- a/src/github/github.ts +++ b/src/github/github.ts @@ -5,7 +5,12 @@ import { RequestError } from '@octokit/request-error'; import type { Endpoints, OctokitResponse, RequestParameters } from '@octokit/types'; import fetch from '@env/fetch'; import { isWeb } from '@env/platform'; -import { AuthenticationError, AuthenticationErrorReason, ProviderRequestError } from '../errors'; +import { + AuthenticationError, + AuthenticationErrorReason, + ProviderRequestClientError, + ProviderRequestNotFoundError, +} from '../errors'; import { type DefaultBranch, type IssueOrPullRequest, @@ -88,8 +93,7 @@ export class GitHubApi { avatarUrl: author.avatarUrl, }; } catch (ex) { - debugger; - throw this.handleRequestErrors(ex, cc); + return this.handleRequestError(ex, cc, undefined); } } @@ -156,8 +160,7 @@ export class GitHubApi { avatarUrl: author.avatarUrl, }; } catch (ex) { - debugger; - throw this.handleRequestErrors(ex, cc); + return this.handleRequestError(ex, cc, undefined); } } @@ -207,8 +210,7 @@ export class GitHubApi { name: defaultBranch, }; } catch (ex) { - debugger; - throw this.handleRequestErrors(ex, cc); + return this.handleRequestError(ex, cc, undefined); } } @@ -277,8 +279,7 @@ export class GitHubApi { url: issue.url, }; } catch (ex) { - debugger; - throw this.handleRequestErrors(ex, cc); + return this.handleRequestError(ex, cc, undefined); } } @@ -377,8 +378,7 @@ export class GitHubApi { return GitHubPullRequest.from(prs[0], provider); } catch (ex) { - debugger; - throw this.handleRequestErrors(ex, cc); + return this.handleRequestError(ex, cc, undefined); } } @@ -470,8 +470,7 @@ export class GitHubApi { return GitHubPullRequest.from(prs[0], provider); } catch (ex) { - debugger; - throw this.handleRequestErrors(ex, cc); + return this.handleRequestError(ex, cc, undefined); } } @@ -525,17 +524,20 @@ export class GitHubApi { return octokit; } - private async graphql( - token: string, - query: string, - variables: { [key: string]: any }, - cc?: LogCorrelationContext, - ): Promise { + private async graphql(token: string, query: string, variables: { [key: string]: any }): Promise { try { return await this.octokit(token).graphql(query, variables); } catch (ex) { - debugger; - throw this.handleRequestErrors(ex, cc); + if (ex instanceof GraphqlResponseError) { + switch (ex.errors?.[0]?.type) { + case 'NOT_FOUND': + throw new ProviderRequestNotFoundError(ex); + case 'FORBIDDEN': + throw new AuthenticationError('github', AuthenticationErrorReason.Forbidden, ex); + } + } + + throw ex; } } @@ -547,42 +549,37 @@ export class GitHubApi { try { return (await this.octokit(token).request(route, options)) as any; } catch (ex) { - this.handleRequestErrors(ex); + if (ex instanceof RequestError) { + switch (ex.status) { + case 404: // Not found + case 410: // Gone + throw new ProviderRequestNotFoundError(ex); + // case 429: //Too Many Requests + case 401: // Unauthorized + throw new AuthenticationError('github', AuthenticationErrorReason.Unauthorized, ex); + case 403: // Forbidden + throw new AuthenticationError('github', AuthenticationErrorReason.Forbidden, ex); + case 500: // Internal Server Error + if (ex.response != null) { + // TODO@eamodio: Handle GitHub down errors + } + break; + default: + if (ex.status >= 400 && ex.status < 500) throw new ProviderRequestClientError(ex); + break; + } + } + throw ex; } } - private handleRequestErrors( - ex: unknown | RequestError | GraphqlResponseError, - cc?: LogCorrelationContext, - ): unknown { + private handleRequestError(ex: unknown | Error, cc: LogCorrelationContext | undefined, defaultValue: T): T { + if (ex instanceof ProviderRequestNotFoundError) return defaultValue; + Logger.error(ex, cc); debugger; - - if (ex instanceof RequestError) { - switch (ex.status) { - case 401: - return new AuthenticationError('github', AuthenticationErrorReason.Unauthorized, ex); - case 403: - return new AuthenticationError('github', AuthenticationErrorReason.Forbidden, ex); - case 500: - if (ex.response != null) { - // TODO@eamodio: Handle GitHub down errors - } - break; - default: - if (ex.status >= 400 && ex.status <= 500) return new ProviderRequestError(ex); - break; - } - - return ex; - } - - if (ex instanceof GraphqlResponseError && ex.errors?.[0]?.type === 'FORBIDDEN') { - return new AuthenticationError('github', AuthenticationErrorReason.Forbidden, ex); - } - - return ex; + throw ex; } }