From b613f7174a21aa01b4aca5453941e836fa0d72cb Mon Sep 17 00:00:00 2001 From: Eric Amodio Date: Mon, 1 Aug 2022 20:03:20 -0400 Subject: [PATCH] Fixes #2117 new limits/controls on failed requests - Changes request failures to only disconnect the integration for the current session - Limits to 5 failed request before disconnecting the integration - Avoids disconnect prompt for current session disconnections - Adds don't show again to request failure messages - Adds integration suspension message w/ don't show again option --- CHANGELOG.md | 1 + package.json | 20 +- src/config.ts | 32 +-- src/git/remotes/provider.ts | 120 ++++++----- src/messages.ts | 62 ++++-- src/plus/github/github.ts | 482 +++++++++++++++++++++++++++++--------------- src/plus/gitlab/gitlab.ts | 107 +++++++--- 7 files changed, 552 insertions(+), 272 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ac1056a..b7d832a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -202,6 +202,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/) and this p ### Fixed +- Fixes [#2117](https://github.com/gitkraken/vscode-gitlens/issues/2117) - Gitlens freaks out when I'm off VPN - Fixes [#1859](https://github.com/gitkraken/vscode-gitlens/issues/1859) - GitLens dates use system locale instead of vscode language setting - Fixes [#1854](https://github.com/gitkraken/vscode-gitlens/issues/1854) - All repos have the same name - Fixes [#1866](https://github.com/gitkraken/vscode-gitlens/issues/1866) - Copy SHA and Copy Message don't work from the views (commits, branches, etc) diff --git a/package.json b/package.json index 1e5c46c..8e07974 100644 --- a/package.json +++ b/package.json @@ -3011,7 +3011,10 @@ "suppressGitVersionWarning": false, "suppressLineUncommittedWarning": false, "suppressNoRepositoryWarning": false, - "suppressRebaseSwitchToTextWarning": false + "suppressRebaseSwitchToTextWarning": false, + "suppressIntegrationDisconnectedTooManyFailedRequestsWarning": false, + "suppressIntegrationRequestFailed500Warning": false, + "suppressIntegrationRequestTimedOutWarning": false }, "properties": { "suppressCommitHasNoPreviousCommitWarning": { @@ -3068,6 +3071,21 @@ "type": "boolean", "default": false, "description": "Rebase Switch To Text Warning" + }, + "suppressIntegrationDisconnectedTooManyFailedRequestsWarning": { + "type": "boolean", + "default": false, + "description": "Integration Disconnected; Too Many Failed Requests Warning" + }, + "suppressIntegrationRequestFailed500Warning": { + "type": "boolean", + "default": false, + "description": "Integration Request Failed (500 status code) Warning" + }, + "suppressIntegrationRequestTimedOutWarning": { + "type": "boolean", + "default": false, + "description": "Integration Request Timed Out Warning" } }, "additionalProperties": false, diff --git a/src/config.ts b/src/config.ts index 2ca0065..757a71f 100644 --- a/src/config.ts +++ b/src/config.ts @@ -363,19 +363,7 @@ export interface AdvancedConfig { fileHistoryShowAllBranches: boolean; maxListItems: number; maxSearchItems: number; - messages: { - suppressCommitHasNoPreviousCommitWarning: boolean; - suppressCommitNotFoundWarning: boolean; - suppressCreatePullRequestPrompt: boolean; - suppressDebugLoggingWarning: boolean; - suppressFileNotUnderSourceControlWarning: boolean; - suppressGitDisabledWarning: boolean; - suppressGitMissingWarning: boolean; - suppressGitVersionWarning: boolean; - suppressLineUncommittedWarning: boolean; - suppressNoRepositoryWarning: boolean; - suppressRebaseSwitchToTextWarning: boolean; - }; + messages: { [key in SuppressedMessages]: boolean }; quickPick: { closeOnFocusOut: boolean; }; @@ -510,6 +498,24 @@ export interface RemotesUrlsConfig { fileRange: string; } +// NOTE: Must be kept in sync with `gitlens.advanced.messages` setting in the package.json +export const enum SuppressedMessages { + CommitHasNoPreviousCommitWarning = 'suppressCommitHasNoPreviousCommitWarning', + CommitNotFoundWarning = 'suppressCommitNotFoundWarning', + CreatePullRequestPrompt = 'suppressCreatePullRequestPrompt', + SuppressDebugLoggingWarning = 'suppressDebugLoggingWarning', + FileNotUnderSourceControlWarning = 'suppressFileNotUnderSourceControlWarning', + GitDisabledWarning = 'suppressGitDisabledWarning', + GitMissingWarning = 'suppressGitMissingWarning', + GitVersionWarning = 'suppressGitVersionWarning', + LineUncommittedWarning = 'suppressLineUncommittedWarning', + NoRepositoryWarning = 'suppressNoRepositoryWarning', + RebaseSwitchToTextWarning = 'suppressRebaseSwitchToTextWarning', + IntegrationDisconnectedTooManyFailedRequestsWarning = 'suppressIntegrationDisconnectedTooManyFailedRequestsWarning', + IntegrationRequestFailed500Warning = 'suppressIntegrationRequestFailed500Warning', + IntegrationRequestTimedOutWarning = 'suppressIntegrationRequestTimedOutWarning', +} + export interface ViewsCommonConfig { defaultItemLimit: number; formats: { diff --git a/src/git/remotes/provider.ts b/src/git/remotes/provider.ts index 15ce47b..a5ac65b 100644 --- a/src/git/remotes/provider.ts +++ b/src/git/remotes/provider.ts @@ -18,6 +18,7 @@ import { configuration } from '../../configuration'; import { Container } from '../../container'; import { AuthenticationError, ProviderRequestClientError } from '../../errors'; import { Logger } from '../../logger'; +import { Messages } from '../../messages'; import type { IntegrationAuthenticationSessionDescriptor } from '../../plus/integrationAuthentication'; import { WorkspaceStorageKeys } from '../../storage'; import { gate } from '../../system/decorators/gate'; @@ -288,8 +289,6 @@ export abstract class RichRemoteProvider extends RemoteProvider { return this._onDidChange.event; } - private invalidClientExceptionCount = 0; - constructor(domain: string, path: string, protocol?: string, name?: string, custom?: boolean) { super(domain, path, protocol, name, custom); @@ -304,7 +303,7 @@ export abstract class RichRemoteProvider extends RemoteProvider { if (e.key !== this.key) return; if (e.reason === 'disconnected') { - void this.disconnect(true); + void this.disconnect({ silent: true }); } else if (e.reason === 'connected') { void this.ensureSession(false); } @@ -359,49 +358,58 @@ export abstract class RichRemoteProvider extends RemoteProvider { @gate() @log() - async disconnect(silent: boolean = false): Promise { + async disconnect(options?: { silent?: boolean; currentSessionOnly?: boolean }): Promise { + if (options?.currentSessionOnly && this._session === null) return; + const connected = this._session != null; const container = Container.instance; - if (connected && !silent) { - const disable = { title: 'Disable' }; - const signout = { title: 'Disable & Sign Out' }; - const cancel = { title: 'Cancel', isCloseAffordance: true }; - - let result: MessageItem | undefined; - if (container.integrationAuthentication.hasProvider(this.authProvider.id)) { - result = await window.showWarningMessage( - `Are you sure you want to disable the rich integration with ${this.name}?\n\nNote: signing out clears the saved authentication.`, - { modal: true }, - disable, - signout, - cancel, - ); + if (connected && !options?.silent) { + if (options?.currentSessionOnly) { + void Messages.showIntegrationDisconnectedTooManyFailedRequestsWarningMessage(this.name); } else { - result = await window.showWarningMessage( - `Are you sure you want to disable the rich integration with ${this.name}?`, - { modal: true }, - disable, - cancel, - ); - } + const disable = { title: 'Disable' }; + const signout = { title: 'Disable & Sign Out' }; + const cancel = { title: 'Cancel', isCloseAffordance: true }; + + let result: MessageItem | undefined; + if (container.integrationAuthentication.hasProvider(this.authProvider.id)) { + result = await window.showWarningMessage( + `Are you sure you want to disable the rich integration with ${this.name}?\n\nNote: signing out clears the saved authentication.`, + { modal: true }, + disable, + signout, + cancel, + ); + } else { + result = await window.showWarningMessage( + `Are you sure you want to disable the rich integration with ${this.name}?`, + { modal: true }, + disable, + cancel, + ); + } - if (result == null || result === cancel) return; - if (result === signout) { - void Container.instance.integrationAuthentication.deleteSession(this.id, this.authProviderDescriptor); + if (result == null || result === cancel) return; + if (result === signout) { + void container.integrationAuthentication.deleteSession(this.id, this.authProviderDescriptor); + } } } - this.invalidClientExceptionCount = 0; + this.resetRequestExceptionCount(); this._prsByCommit.clear(); this._session = null; if (connected) { - void container.storage.storeWorkspace(this.connectedKey, false); + // Don't store the disconnected flag if this only for this current VS Code session (will be re-connected on next restart) + if (!options?.currentSessionOnly) { + void container.storage.storeWorkspace(this.connectedKey, false); + } this._onDidChange.fire(); - if (!silent) { + if (!options?.silent && !options?.currentSessionOnly) { fireAuthenticationChanged(this.key, 'disconnected'); } } @@ -415,6 +423,21 @@ export abstract class RichRemoteProvider extends RemoteProvider { void (await this.ensureSession(true, true)); } + private requestExceptionCount = 0; + + resetRequestExceptionCount() { + this.requestExceptionCount = 0; + } + + @debug() + trackRequestException() { + this.requestExceptionCount++; + + if (this.requestExceptionCount >= 5 && this._session !== null) { + void this.disconnect({ currentSessionOnly: true }); + } + } + @gate() @debug({ exit: connected => `returned ${connected}`, @@ -438,13 +461,13 @@ export abstract class RichRemoteProvider extends RemoteProvider { try { const author = await this.getProviderAccountForCommit(this._session!, ref, options); - this.invalidClientExceptionCount = 0; + this.resetRequestExceptionCount(); return author; } catch (ex) { Logger.error(ex, cc); if (ex instanceof AuthenticationError || ex instanceof ProviderRequestClientError) { - this.handleClientException(); + this.trackRequestException(); } return undefined; } @@ -473,13 +496,13 @@ export abstract class RichRemoteProvider extends RemoteProvider { try { const author = await this.getProviderAccountForEmail(this._session!, email, options); - this.invalidClientExceptionCount = 0; + this.resetRequestExceptionCount(); return author; } catch (ex) { Logger.error(ex, cc); if (ex instanceof AuthenticationError || ex instanceof ProviderRequestClientError) { - this.handleClientException(); + this.trackRequestException(); } return undefined; } @@ -503,13 +526,13 @@ export abstract class RichRemoteProvider extends RemoteProvider { try { const defaultBranch = await this.getProviderDefaultBranch(this._session!); - this.invalidClientExceptionCount = 0; + this.resetRequestExceptionCount(); return defaultBranch; } catch (ex) { Logger.error(ex, cc); if (ex instanceof AuthenticationError || ex instanceof ProviderRequestClientError) { - this.handleClientException(); + this.trackRequestException(); } return undefined; } @@ -529,13 +552,13 @@ export abstract class RichRemoteProvider extends RemoteProvider { try { const issueOrPullRequest = await this.getProviderIssueOrPullRequest(this._session!, id); - this.invalidClientExceptionCount = 0; + this.resetRequestExceptionCount(); return issueOrPullRequest; } catch (ex) { Logger.error(ex, cc); if (ex instanceof AuthenticationError || ex instanceof ProviderRequestClientError) { - this.handleClientException(); + this.trackRequestException(); } return undefined; } @@ -578,13 +601,13 @@ export abstract class RichRemoteProvider extends RemoteProvider { try { const pr = await this.getProviderPullRequestForBranch(this._session!, branch, options); - this.invalidClientExceptionCount = 0; + this.resetRequestExceptionCount(); return pr; } catch (ex) { Logger.error(ex, cc); if (ex instanceof AuthenticationError || ex instanceof ProviderRequestClientError) { - this.handleClientException(); + this.trackRequestException(); } return undefined; } @@ -622,7 +645,7 @@ export abstract class RichRemoteProvider extends RemoteProvider { try { const pr = (await this.getProviderPullRequestForCommit(this._session!, ref)) ?? null; this._prsByCommit.set(ref, pr); - this.invalidClientExceptionCount = 0; + this.resetRequestExceptionCount(); return pr; } catch (ex) { Logger.error(ex, cc); @@ -630,7 +653,7 @@ export abstract class RichRemoteProvider extends RemoteProvider { this._prsByCommit.delete(ref); if (ex instanceof AuthenticationError || ex instanceof ProviderRequestClientError) { - this.handleClientException(); + this.trackRequestException(); } return null; } @@ -689,7 +712,7 @@ export abstract class RichRemoteProvider extends RemoteProvider { } this._session = session ?? null; - this.invalidClientExceptionCount = 0; + this.resetRequestExceptionCount(); if (session != null) { await container.storage.storeWorkspace(this.connectedKey, true); @@ -702,13 +725,4 @@ export abstract class RichRemoteProvider extends RemoteProvider { return session ?? undefined; } - - @debug() - private handleClientException() { - this.invalidClientExceptionCount++; - - if (this.invalidClientExceptionCount >= 5) { - void this.disconnect(); - } - } } diff --git a/src/messages.ts b/src/messages.ts index 4923d6a..7553788 100644 --- a/src/messages.ts +++ b/src/messages.ts @@ -1,24 +1,10 @@ import { ConfigurationTarget, MessageItem, window } from 'vscode'; -import { configuration } from './configuration'; +import { configuration, SuppressedMessages } from './configuration'; import { Commands } from './constants'; import { GitCommit } from './git/models'; import { Logger } from './logger'; import { executeCommand } from './system/command'; -export const enum SuppressedMessages { - CommitHasNoPreviousCommitWarning = 'suppressCommitHasNoPreviousCommitWarning', - CommitNotFoundWarning = 'suppressCommitNotFoundWarning', - CreatePullRequestPrompt = 'suppressCreatePullRequestPrompt', - SuppressDebugLoggingWarning = 'suppressDebugLoggingWarning', - FileNotUnderSourceControlWarning = 'suppressFileNotUnderSourceControlWarning', - GitDisabledWarning = 'suppressGitDisabledWarning', - GitMissingWarning = 'suppressGitMissingWarning', - GitVersionWarning = 'suppressGitVersionWarning', - LineUncommittedWarning = 'suppressLineUncommittedWarning', - NoRepositoryWarning = 'suppressNoRepositoryWarning', - RebaseSwitchToTextWarning = 'suppressRebaseSwitchToTextWarning', -} - export class Messages { static showCommitHasNoPreviousCommitWarningMessage(commit?: GitCommit): Promise { if (commit == null) { @@ -155,6 +141,44 @@ export class Messages { ); } + static showIntegrationDisconnectedTooManyFailedRequestsWarningMessage( + providerName: string, + ): Promise { + return Messages.showMessage( + 'error', + `Rich integration with ${providerName} has been disconnected for this session, because of too many failed requests.`, + SuppressedMessages.IntegrationDisconnectedTooManyFailedRequestsWarning, + undefined, + { + title: 'OK', + }, + ); + } + + static showIntegrationRequestFailed500WarningMessage(message: string): Promise { + return Messages.showMessage( + 'error', + message, + SuppressedMessages.IntegrationRequestFailed500Warning, + undefined, + { + title: 'OK', + }, + ); + } + + static showIntegrationRequestTimedOutWarningMessage(providerName: string): Promise { + return Messages.showMessage( + 'error', + `${providerName} request timed out.`, + SuppressedMessages.IntegrationRequestTimedOutWarning, + undefined, + { + title: 'OK', + }, + ); + } + static async showWhatsNewMessage(version: string) { const whatsnew = { title: "See What's New" }; const result = await Messages.showMessage( @@ -179,14 +203,14 @@ export class Messages { ): Promise { Logger.log(`ShowMessage(${type}, '${message}', ${suppressionKey}, ${JSON.stringify(dontShowAgain)})`); - if (suppressionKey !== undefined && configuration.get(`advanced.messages.${suppressionKey}` as const)) { + if (suppressionKey != null && configuration.get(`advanced.messages.${suppressionKey}` as const)) { Logger.log( `ShowMessage(${type}, '${message}', ${suppressionKey}, ${JSON.stringify(dontShowAgain)}) skipped`, ); return undefined; } - if (suppressionKey !== undefined && dontShowAgain !== null) { + if (suppressionKey != null && dontShowAgain !== null) { actions.push(dontShowAgain); } @@ -205,13 +229,13 @@ export class Messages { break; } - if ((suppressionKey !== undefined && dontShowAgain === null) || result === dontShowAgain) { + if (suppressionKey != null && (dontShowAgain === null || result === dontShowAgain)) { Logger.log( `ShowMessage(${type}, '${message}', ${suppressionKey}, ${JSON.stringify( dontShowAgain, )}) don't show again requested`, ); - await this.suppressedMessage(suppressionKey!); + await this.suppressedMessage(suppressionKey); if (result === dontShowAgain) return undefined; } diff --git a/src/plus/github/github.ts b/src/plus/github/github.ts index 4ed3803..de33a42 100644 --- a/src/plus/github/github.ts +++ b/src/plus/github/github.ts @@ -21,6 +21,7 @@ import type { Account } from '../../git/models/author'; import { getGitHubNoReplyAddressParts } from '../../git/remotes/github'; import type { RichRemoteProvider } from '../../git/remotes/provider'; import { LogCorrelationContext, Logger, LogLevel } from '../../logger'; +import { Messages } from '../../messages'; import { debug } from '../../system/decorators/log'; import { Stopwatch } from '../../system/stopwatch'; import { base64 } from '../../system/string'; @@ -138,12 +139,18 @@ export class GitHubApi implements Disposable { } }`; - const rsp = await this.graphql(provider, token, query, { - ...options, - owner: owner, - repo: repo, - ref: ref, - }); + const rsp = await this.graphql( + provider, + token, + query, + { + ...options, + owner: owner, + repo: repo, + ref: ref, + }, + cc, + ); const author = rsp?.repository?.object?.author; if (author == null) return undefined; @@ -219,12 +226,18 @@ export class GitHubApi implements Disposable { } }`; - const rsp = await this.graphql(provider, token, query, { - ...options, - owner: owner, - repo: repo, - emailQuery: `in:email ${email}`, - }); + const rsp = await this.graphql( + provider, + token, + query, + { + ...options, + owner: owner, + repo: repo, + emailQuery: `in:email ${email}`, + }, + cc, + ); const author = rsp?.search?.nodes?.[0]; if (author == null) return undefined; @@ -287,11 +300,17 @@ export class GitHubApi implements Disposable { } }`; - const rsp = await this.graphql(provider, token, query, { - ...options, - owner: owner, - repo: repo, - }); + const rsp = await this.graphql( + provider, + token, + query, + { + ...options, + owner: owner, + repo: repo, + }, + cc, + ); const defaultBranch = rsp?.repository?.defaultBranchRef?.name ?? undefined; if (defaultBranch == null) return undefined; @@ -351,12 +370,18 @@ export class GitHubApi implements Disposable { } }`; - const rsp = await this.graphql(provider, token, query, { - ...options, - owner: owner, - repo: repo, - number: number, - }); + const rsp = await this.graphql( + provider, + token, + query, + { + ...options, + owner: owner, + repo: repo, + number: number, + }, + cc, + ); const issue = rsp?.repository?.issueOrPullRequest; if (issue == null) return undefined; @@ -447,14 +472,20 @@ export class GitHubApi implements Disposable { } }`; - const rsp = await this.graphql(provider, token, query, { - ...options, - owner: owner, - repo: repo, - branch: branch, - // Since GitHub sort doesn't seem to really work, look for a max of 10 PRs and then sort them ourselves - limit: 10, - }); + const rsp = await this.graphql( + provider, + token, + query, + { + ...options, + owner: owner, + repo: repo, + branch: branch, + // Since GitHub sort doesn't seem to really work, look for a max of 10 PRs and then sort them ourselves + limit: 10, + }, + cc, + ); // If the pr is not from a fork, keep it e.g. show root pr's on forks, otherwise, ensure the repo owners match const prs = rsp?.repository?.refs.nodes[0]?.associatedPullRequests?.nodes?.filter( @@ -543,12 +574,18 @@ export class GitHubApi implements Disposable { } }`; - const rsp = await this.graphql(provider, token, query, { - ...options, - owner: owner, - repo: repo, - ref: ref, - }); + const rsp = await this.graphql( + provider, + token, + query, + { + ...options, + owner: owner, + repo: repo, + ref: ref, + }, + cc, + ); // If the pr is not from a fork, keep it e.g. show root pr's on forks, otherwise, ensure the repo owners match const prs = rsp?.repository?.object?.associatedPullRequests?.nodes?.filter( @@ -631,12 +668,18 @@ export class GitHubApi implements Disposable { } } }`; - const rsp = await this.graphql(undefined, token, query, { - owner: owner, - repo: repo, - ref: ref, - path: path, - }); + const rsp = await this.graphql( + undefined, + token, + query, + { + owner: owner, + repo: repo, + ref: ref, + path: path, + }, + cc, + ); if (rsp == null) return emptyBlameResult; const ranges = rsp.repository?.object?.blame?.ranges; @@ -703,13 +746,19 @@ export class GitHubApi implements Disposable { } }`; - const rsp = await this.graphql(undefined, token, query, { - owner: owner, - repo: repo, - branchQuery: options?.query, - cursor: options?.cursor, - limit: Math.min(100, options?.limit ?? 100), - }); + const rsp = await this.graphql( + undefined, + token, + query, + { + owner: owner, + repo: repo, + branchQuery: options?.query, + cursor: options?.cursor, + limit: Math.min(100, options?.limit ?? 100), + }, + cc, + ); if (rsp == null) return emptyPagedResult; const refs = rsp.repository?.refs; @@ -739,11 +788,17 @@ export class GitHubApi implements Disposable { const cc = Logger.getCorrelationContext(); try { - const rsp = await this.request(undefined, token, 'GET /repos/{owner}/{repo}/commits/{ref}', { - owner: owner, - repo: repo, - ref: ref, - }); + const rsp = await this.request( + undefined, + token, + 'GET /repos/{owner}/{repo}/commits/{ref}', + { + owner: owner, + repo: repo, + ref: ref, + }, + cc, + ); const result = rsp?.data; if (result == null) return undefined; @@ -840,12 +895,18 @@ export class GitHubApi implements Disposable { } } }`; - const rsp = await this.graphql(undefined, token, query, { - owner: owner, - repo: repo, - since: date.toISOString(), - until: date.toISOString(), - }); + const rsp = await this.graphql( + undefined, + token, + query, + { + owner: owner, + repo: repo, + since: date.toISOString(), + until: date.toISOString(), + }, + cc, + ); const nodes = rsp?.repository?.refs?.nodes; if (nodes == null) return []; @@ -902,11 +963,17 @@ export class GitHubApi implements Disposable { } }`; - const rsp = await this.graphql(undefined, token, query, { - owner: owner, - repo: repo, - ref: ref, - }); + const rsp = await this.graphql( + undefined, + token, + query, + { + owner: owner, + repo: repo, + ref: ref, + }, + cc, + ); const count = rsp?.repository?.ref?.target.history.totalCount; return count; @@ -959,13 +1026,19 @@ export class GitHubApi implements Disposable { } } }`; - const rsp = await this.graphql(undefined, token, query, { - owner: owner, - repo: repo, - ref: `refs/heads/${branch}`, - since: date.toISOString(), - until: date.toISOString(), - }); + const rsp = await this.graphql( + undefined, + token, + query, + { + owner: owner, + repo: repo, + ref: `refs/heads/${branch}`, + since: date.toISOString(), + until: date.toISOString(), + }, + cc, + ); const nodes = rsp?.repository?.ref.target.history.nodes; if (nodes == null) return []; @@ -1093,18 +1166,24 @@ export class GitHubApi implements Disposable { } } - const rsp = await this.graphql(undefined, token, query, { - owner: owner, - repo: repo, - ref: ref, - after: options?.after, - before: options?.before, - path: options?.path, - author: authors, - limit: Math.min(100, options?.limit ?? 100), - since: typeof options?.since === 'string' ? options?.since : options?.since?.toISOString(), - until: typeof options?.until === 'string' ? options?.until : options?.until?.toISOString(), - }); + const rsp = await this.graphql( + undefined, + token, + query, + { + owner: owner, + repo: repo, + ref: ref, + after: options?.after, + before: options?.before, + path: options?.path, + author: authors, + limit: Math.min(100, options?.limit ?? 100), + since: typeof options?.since === 'string' ? options?.since : options?.since?.toISOString(), + until: typeof options?.until === 'string' ? options?.until : options?.until?.toISOString(), + }, + cc, + ); const history = rsp?.repository?.object?.history; if (history == null) return emptyPagedResult; @@ -1171,11 +1250,17 @@ export class GitHubApi implements Disposable { } }`; - const rsp = await this.graphql(undefined, token, query, { - owner: owner, - repo: repo, - ref: ref, - }); + const rsp = await this.graphql( + undefined, + token, + query, + { + owner: owner, + repo: repo, + ref: ref, + }, + cc, + ); if (rsp == null) return emptyPagedResult; const commit = rsp.repository?.object; @@ -1249,18 +1334,24 @@ export class GitHubApi implements Disposable { } }`; - const rsp = await this.graphql(undefined, token, query, { - owner: owner, - repo: repo, - ref: ref, - path: options?.path, - first: options?.first, - last: options?.last, - after: options?.after, - before: options?.before, - since: options?.since, - until: options?.until, - }); + const rsp = await this.graphql( + undefined, + token, + query, + { + owner: owner, + repo: repo, + ref: ref, + path: options?.path, + first: options?.first, + last: options?.last, + after: options?.after, + before: options?.before, + since: options?.since, + until: options?.until, + }, + cc, + ); const history = rsp?.repository?.object?.history; if (history == null) return undefined; @@ -1341,11 +1432,17 @@ export class GitHubApi implements Disposable { } }`; - const rsp = await this.graphql(undefined, token, query, { - owner: owner, - repo: repo, - sha: sha, - }); + const rsp = await this.graphql( + undefined, + token, + query, + { + owner: owner, + repo: repo, + sha: sha, + }, + cc, + ); const date = rsp?.repository?.object?.committer.date; return date; } catch (ex) { @@ -1362,11 +1459,17 @@ export class GitHubApi implements Disposable { // TODO@eamodio implement pagination try { - const rsp = await this.request(undefined, token, 'GET /repos/{owner}/{repo}/contributors', { - owner: owner, - repo: repo, - per_page: 100, - }); + const rsp = await this.request( + undefined, + token, + 'GET /repos/{owner}/{repo}/contributors', + { + owner: owner, + repo: repo, + per_page: 100, + }, + cc, + ); const result = rsp?.data; if (result == null) return []; @@ -1404,10 +1507,16 @@ export class GitHubApi implements Disposable { } }`; - const rsp = await this.graphql(undefined, token, query, { - owner: owner, - repo: repo, - }); + const rsp = await this.graphql( + undefined, + token, + query, + { + owner: owner, + repo: repo, + }, + cc, + ); if (rsp == null) return undefined; return rsp.repository?.defaultBranchRef?.name ?? undefined; @@ -1441,10 +1550,16 @@ export class GitHubApi implements Disposable { repository(owner: $owner, name: $repo) { viewerPermission } }`; - const rsp = await this.graphql(undefined, token, query, { - owner: owner, - repo: repo, - }); + const rsp = await this.graphql( + undefined, + token, + query, + { + owner: owner, + repo: repo, + }, + cc, + ); if (rsp == null) return undefined; return { @@ -1487,10 +1602,16 @@ export class GitHubApi implements Disposable { } }`; - const rsp = await this.graphql(undefined, token, query, { - owner: owner, - repo: repo, - }); + const rsp = await this.graphql( + undefined, + token, + query, + { + owner: owner, + repo: repo, + }, + cc, + ); if (rsp?.repository?.visibility == null) return undefined; return rsp.repository.visibility === 'PUBLIC' ? RepositoryVisibility.Public : RepositoryVisibility.Private; @@ -1559,13 +1680,19 @@ export class GitHubApi implements Disposable { } }`; - const rsp = await this.graphql(undefined, token, query, { - owner: owner, - repo: repo, - tagQuery: options?.query, - cursor: options?.cursor, - limit: Math.min(100, options?.limit ?? 100), - }); + const rsp = await this.graphql( + undefined, + token, + query, + { + owner: owner, + repo: repo, + tagQuery: options?.query, + cursor: options?.cursor, + limit: Math.min(100, options?.limit ?? 100), + }, + cc, + ); if (rsp == null) return emptyPagedResult; const refs = rsp.repository?.refs; @@ -1613,11 +1740,17 @@ export class GitHubApi implements Disposable { } }`; - const rsp = await this.graphql(undefined, token, query, { - owner: owner, - repo: repo, - ref: ref, - }); + const rsp = await this.graphql( + undefined, + token, + query, + { + owner: owner, + repo: repo, + ref: ref, + }, + cc, + ); return rsp?.repository?.object?.oid ?? undefined; } @@ -1651,12 +1784,18 @@ export class GitHubApi implements Disposable { } }`; - const rsp = await this.graphql(undefined, token, query, { - owner: owner, - repo: repo, - ref: ref, - path: path, - }); + const rsp = await this.graphql( + undefined, + token, + query, + { + owner: owner, + repo: repo, + ref: ref, + path: path, + }, + cc, + ); return rsp?.repository?.object?.history.nodes?.[0]?.oid ?? undefined; } catch (ex) { if (ex instanceof ProviderRequestNotFoundError) return undefined; @@ -1696,13 +1835,19 @@ export class GitHubApi implements Disposable { } try { - const rsp = await this.request(undefined, token, 'GET /search/commits', { - q: query, - sort: options?.sort, - order: options?.order, - per_page: pageSize, - page: page, - }); + const rsp = await this.request( + undefined, + token, + 'GET /search/commits', + { + q: query, + sort: options?.sort, + order: options?.order, + per_page: pageSize, + page: page, + }, + cc, + ); const data = rsp?.data; if (data == null || data.items.length === 0) return undefined; @@ -1756,8 +1901,10 @@ export class GitHubApi implements Disposable { if (version != null) return version; if (version === null) return undefined; + const cc = Logger.getCorrelationContext(); + try { - const rsp = await this.request(provider, token, 'GET /meta', options); + const rsp = await this.request(provider, token, 'GET /meta', options, cc); const v = (rsp?.data as any)?.installed_version as string | null | undefined; version = v ? fromString(v) : null; } catch (ex) { @@ -1824,6 +1971,7 @@ export class GitHubApi implements Disposable { token: string, query: string, variables: { [key: string]: any }, + cc: LogCorrelationContext | undefined, ): Promise { try { return await wrapForForcedInsecureSSL(provider?.getIgnoreSSLErrors() ?? false, () => @@ -1855,7 +2003,7 @@ export class GitHubApi implements Disposable { void window.showErrorMessage(`GitHub request failed: ${ex.errors?.[0]?.message ?? ex.message}`); } } else if (ex instanceof RequestError) { - this.handleRequestError(ex, token); + this.handleRequestError(provider, token, ex, cc); } else if (Logger.isDebugging) { void window.showErrorMessage(`GitHub request failed: ${ex.message}`); } @@ -1868,7 +2016,10 @@ export class GitHubApi implements Disposable { provider: RichRemoteProvider | undefined, token: string, route: keyof Endpoints | R, - options?: R extends keyof Endpoints ? Endpoints[R]['parameters'] & RequestParameters : RequestParameters, + options: + | (R extends keyof Endpoints ? Endpoints[R]['parameters'] & RequestParameters : RequestParameters) + | undefined, + cc: LogCorrelationContext | undefined, ): Promise> { try { return (await wrapForForcedInsecureSSL(provider?.getIgnoreSSLErrors() ?? false, () => @@ -1876,7 +2027,7 @@ export class GitHubApi implements Disposable { )) as any; } catch (ex) { if (ex instanceof RequestError) { - this.handleRequestError(ex, token); + this.handleRequestError(provider, token, ex, cc); } else if (Logger.isDebugging) { void window.showErrorMessage(`GitHub request failed: ${ex.message}`); } @@ -1885,7 +2036,12 @@ export class GitHubApi implements Disposable { } } - private handleRequestError(ex: RequestError, token: string): void { + private handleRequestError( + provider: RichRemoteProvider | undefined, + token: string, + ex: RequestError, + cc: LogCorrelationContext | undefined, + ): void { switch (ex.status) { case 404: // Not found case 410: // Gone @@ -1910,17 +2066,24 @@ export class GitHubApi implements Disposable { } throw new AuthenticationError('github', AuthenticationErrorReason.Forbidden, ex); case 500: // Internal Server Error + Logger.error(ex, cc); if (ex.response != null) { - void window.showErrorMessage( - 'GitHub failed to respond and might be experiencing issues. Please visit the [GitHub status page](https://githubstatus.com) for more information.', - 'OK', + provider?.trackRequestException(); + void Messages.showIntegrationRequestFailed500WarningMessage( + `${provider?.name ?? 'GitHub'} failed to respond and might be experiencing issues.${ + !provider?.custom + ? ' Please visit the [GitHub status page](https://githubstatus.com) for more information.' + : '' + }`, ); } return; case 502: // Bad Gateway + Logger.error(ex, cc); // GitHub seems to return this status code for timeouts if (ex.message.includes('timeout')) { - void window.showErrorMessage('GitHub request timed out'); + provider?.trackRequestException(); + void Messages.showIntegrationRequestTimedOutWarningMessage(provider?.name ?? 'GitHub'); return; } break; @@ -1929,6 +2092,7 @@ export class GitHubApi implements Disposable { break; } + Logger.error(ex, cc); if (Logger.isDebugging) { void window.showErrorMessage( `GitHub request failed: ${(ex.response as any)?.errors?.[0]?.message ?? ex.message}`, @@ -1942,7 +2106,7 @@ export class GitHubApi implements Disposable { cc: LogCorrelationContext | undefined, ): Error { Logger.error(ex, cc); - debugger; + // debugger; if (ex instanceof AuthenticationError) { void this.showAuthenticationErrorMessage(ex, provider); diff --git a/src/plus/gitlab/gitlab.ts b/src/plus/gitlab/gitlab.ts index eb3855b..4e6dfa0 100644 --- a/src/plus/gitlab/gitlab.ts +++ b/src/plus/gitlab/gitlab.ts @@ -16,6 +16,7 @@ import { import { Account, DefaultBranch, IssueOrPullRequest, IssueOrPullRequestType, PullRequest } from '../../git/models'; import type { RichRemoteProvider } from '../../git/remotes/provider'; import { LogCorrelationContext, Logger, LogLevel } from '../../logger'; +import { Messages } from '../../messages'; import { debug } from '../../system/decorators/log'; import { Stopwatch } from '../../system/stopwatch'; import { equalsIgnoreCase } from '../../system/string'; @@ -94,6 +95,7 @@ export class GitLabApi implements Disposable { method: 'GET', // ...options, }, + cc, ); let user: GitLabUser | undefined; @@ -195,9 +197,16 @@ export class GitLabApi implements Disposable { } }`; - const rsp = await this.graphql(provider, token, options?.baseUrl, query, { - fullPath: `${owner}/${repo}`, - }); + const rsp = await this.graphql( + provider, + token, + options?.baseUrl, + query, + { + fullPath: `${owner}/${repo}`, + }, + cc, + ); const defaultBranch = rsp?.data?.project?.repository?.rootRef ?? undefined; if (defaultBranch == null) return undefined; @@ -274,10 +283,17 @@ export class GitLabApi implements Disposable { } }`; - const rsp = await this.graphql(provider, token, options?.baseUrl, query, { - fullPath: `${owner}/${repo}`, - iid: String(number), - }); + const rsp = await this.graphql( + provider, + token, + options?.baseUrl, + query, + { + fullPath: `${owner}/${repo}`, + iid: String(number), + }, + cc, + ); if (rsp?.data?.project?.issue != null) { const issue = rsp.data.project.issue; @@ -404,11 +420,18 @@ export class GitLabApi implements Disposable { } }`; - const rsp = await this.graphql(provider, token, options?.baseUrl, query, { - fullPath: `${owner}/${repo}`, - branches: [branch], - state: options?.include, - }); + const rsp = await this.graphql( + provider, + token, + options?.baseUrl, + query, + { + fullPath: `${owner}/${repo}`, + branches: [branch], + state: options?.include, + }, + cc, + ); let pr: GitLabMergeRequest | undefined; @@ -484,6 +507,7 @@ export class GitLabApi implements Disposable { method: 'GET', // ...options, }, + cc, ); if (mrs == null || mrs.length === 0) return undefined; @@ -547,9 +571,16 @@ $search: String! } } }`; - const rsp = await this.graphql(provider, token, options?.baseUrl, query, { - search: search, - }); + const rsp = await this.graphql( + provider, + token, + options?.baseUrl, + query, + { + search: search, + }, + cc, + ); const matches = rsp?.data?.users?.nodes; if (matches == null || matches.length === 0) return []; @@ -619,9 +650,16 @@ $search: String! id } }`; - const rsp = await this.graphql(provider, token, baseUrl, query, { - fullPath: `${group}/${repo}`, - }); + const rsp = await this.graphql( + provider, + token, + baseUrl, + query, + { + fullPath: `${group}/${repo}`, + }, + cc, + ); const gid = rsp?.data?.project?.id; if (gid == null) return undefined; @@ -649,6 +687,7 @@ $search: String! baseUrl: string | undefined, query: string, variables: { [key: string]: any }, + cc: LogCorrelationContext | undefined, ): Promise { let rsp: Response; try { @@ -685,7 +724,7 @@ $search: String! } } catch (ex) { if (ex instanceof ProviderFetchError) { - this.handleRequestError(ex, token); + this.handleRequestError(provider, token, ex, cc); } else if (Logger.isDebugging) { void window.showErrorMessage(`GitLab request failed: ${ex.message}`); } @@ -699,7 +738,8 @@ $search: String! token: string, baseUrl: string | undefined, route: string, - options?: { method: RequestInit['method'] } & Record, + options: { method: RequestInit['method'] } & Record, + cc: LogCorrelationContext | undefined, ): Promise { const url = `${baseUrl ?? 'https://gitlab.com/api'}/${route}`; @@ -732,7 +772,7 @@ $search: String! } } catch (ex) { if (ex instanceof ProviderFetchError) { - this.handleRequestError(ex, token); + this.handleRequestError(provider, token, ex, cc); } else if (Logger.isDebugging) { void window.showErrorMessage(`GitLab request failed: ${ex.message}`); } @@ -741,7 +781,12 @@ $search: String! } } - private handleRequestError(ex: ProviderFetchError, token: string): void { + private handleRequestError( + provider: RichRemoteProvider | undefined, + token: string, + ex: ProviderFetchError, + cc: LogCorrelationContext | undefined, + ): void { switch (ex.status) { case 404: // Not found case 410: // Gone @@ -766,17 +811,24 @@ $search: String! } throw new AuthenticationError('gitlab', AuthenticationErrorReason.Forbidden, ex); case 500: // Internal Server Error + Logger.error(ex, cc); if (ex.response != null) { - void window.showErrorMessage( - 'GitLab failed to respond and might be experiencing issues. Please visit the [GitLab status page](https://status.gitlab.com/) for more information.', - 'OK', + provider?.trackRequestException(); + void Messages.showIntegrationRequestFailed500WarningMessage( + `${provider?.name ?? 'GitLab'} failed to respond and might be experiencing issues.${ + !provider?.custom + ? ' Please visit the [GitLab status page](https://status.gitlab.com) for more information.' + : '' + }`, ); } return; case 502: // Bad Gateway + Logger.error(ex, cc); // GitHub seems to return this status code for timeouts if (ex.message.includes('timeout')) { - void window.showErrorMessage('GitLab request timed out'); + provider?.trackRequestException(); + void Messages.showIntegrationRequestTimedOutWarningMessage(provider?.name ?? 'GitLab'); return; } break; @@ -785,6 +837,7 @@ $search: String! break; } + Logger.error(ex, cc); if (Logger.isDebugging) { void window.showErrorMessage( `GitLab request failed: ${(ex.response as any)?.errors?.[0]?.message ?? ex.message}`, @@ -794,7 +847,7 @@ $search: String! private handleException(ex: Error, provider: RichRemoteProvider, cc: LogCorrelationContext | undefined): Error { Logger.error(ex, cc); - debugger; + // debugger; if (ex instanceof AuthenticationError) { void this.showAuthenticationErrorMessage(ex, provider);