From 839474800c014dc64a77fdf6bbbb1327f7122890 Mon Sep 17 00:00:00 2001 From: Ramin Tadayon Date: Tue, 21 Jan 2025 10:13:31 -0700 Subject: [PATCH] Fixes uncaught errors with cloud self hosted integrations --- src/constants.integrations.ts | 4 + src/git/remotes/remoteProviders.ts | 7 +- .../integrationAuthentication.ts | 11 +-- src/plus/integrations/integration.ts | 8 +- src/plus/integrations/integrationService.ts | 76 ++++++++----------- src/plus/integrations/providers/models.ts | 6 +- src/plus/integrations/providers/utils.ts | 5 +- src/plus/launchpad/launchpad.ts | 1 + src/plus/launchpad/launchpadProvider.ts | 16 ++-- src/plus/startWork/startWork.ts | 5 ++ 10 files changed, 74 insertions(+), 65 deletions(-) diff --git a/src/constants.integrations.ts b/src/constants.integrations.ts index 209498fa25844..89770d71ba757 100644 --- a/src/constants.integrations.ts +++ b/src/constants.integrations.ts @@ -12,6 +12,10 @@ export enum SelfHostedIntegrationId { GitLabSelfHosted = 'gitlab-self-hosted', } +export type CloudSelfHostedIntegrationId = + | SelfHostedIntegrationId.CloudGitHubEnterprise + | SelfHostedIntegrationId.CloudGitLabSelfHosted; + export enum IssueIntegrationId { Jira = 'jira', Trello = 'trello', diff --git a/src/git/remotes/remoteProviders.ts b/src/git/remotes/remoteProviders.ts index 1596ba63f7afa..09fe2c2d2392c 100644 --- a/src/git/remotes/remoteProviders.ts +++ b/src/git/remotes/remoteProviders.ts @@ -2,6 +2,7 @@ import type { RemotesConfig } from '../../config'; import { SelfHostedIntegrationId } from '../../constants.integrations'; import type { Container } from '../../container'; import type { ConfiguredIntegrationDescriptor } from '../../plus/integrations/authentication/models'; +import { isCloudSelfHostedIntegrationId } from '../../plus/integrations/providers/models'; import { configuration } from '../../system/-webview/configuration'; import { Logger } from '../../system/logger'; import { AzureDevOpsRemote } from './azure-devops'; @@ -104,11 +105,7 @@ export function loadRemoteProviders( if (configuredIntegrations?.length) { for (const ci of configuredIntegrations) { - if ( - (ci.integrationId === SelfHostedIntegrationId.CloudGitHubEnterprise || - ci.integrationId === SelfHostedIntegrationId.CloudGitLabSelfHosted) && - ci.domain - ) { + if (isCloudSelfHostedIntegrationId(ci.integrationId) && ci.domain) { const matcher = ci.domain.toLocaleLowerCase(); const providerCreator = (_container: Container, domain: string, path: string) => ci.integrationId === SelfHostedIntegrationId.CloudGitHubEnterprise diff --git a/src/plus/integrations/authentication/integrationAuthentication.ts b/src/plus/integrations/authentication/integrationAuthentication.ts index ed3a69a7e8267..9d6f5f418733b 100644 --- a/src/plus/integrations/authentication/integrationAuthentication.ts +++ b/src/plus/integrations/authentication/integrationAuthentication.ts @@ -9,7 +9,11 @@ import type { Container } from '../../../container'; import { gate } from '../../../system/decorators/-webview/gate'; import { debug, log } from '../../../system/decorators/log'; import type { DeferredEventExecutor } from '../../../system/event'; -import { isSelfHostedIntegrationId, supportedIntegrationIds } from '../providers/models'; +import { + isCloudSelfHostedIntegrationId, + isSelfHostedIntegrationId, + supportedIntegrationIds, +} from '../providers/models'; import type { ConfiguredIntegrationDescriptor, ProviderAuthenticationSession } from './models'; import { isSupportedCloudIntegrationId } from './models'; @@ -375,10 +379,7 @@ export abstract class CloudIntegrationAuthenticationProvider< // Make an exception for GitHub because they always return 0 if ( session?.expiresIn === 0 && - (this.authProviderId === HostingIntegrationId.GitHub || - this.authProviderId === SelfHostedIntegrationId.CloudGitHubEnterprise || - // Note: added GitLab self managed here because the cloud token is always a PAT, and the api does not know when it expires, nor can it refresh it - this.authProviderId === SelfHostedIntegrationId.CloudGitLabSelfHosted) + (this.authProviderId === HostingIntegrationId.GitHub || isCloudSelfHostedIntegrationId(this.authProviderId)) ) { // It never expires so don't refresh it frequently: session.expiresIn = maxSmallIntegerV8; // maximum expiration length diff --git a/src/plus/integrations/integration.ts b/src/plus/integrations/integration.ts index 550b62ac4aac4..cbc66f80482f1 100644 --- a/src/plus/integrations/integration.ts +++ b/src/plus/integrations/integration.ts @@ -1,7 +1,12 @@ import type { CancellationToken, Event, MessageItem } from 'vscode'; import { EventEmitter, window } from 'vscode'; import type { AutolinkReference, DynamicAutolinkReference } from '../../autolinks'; -import type { IntegrationId, IssueIntegrationId, SelfHostedIntegrationId } from '../../constants.integrations'; +import type { + CloudSelfHostedIntegrationId, + IntegrationId, + IssueIntegrationId, + SelfHostedIntegrationId, +} from '../../constants.integrations'; import { HostingIntegrationId } from '../../constants.integrations'; import type { Sources } from '../../constants.telemetry'; import type { Container } from '../../container'; @@ -55,6 +60,7 @@ export type IntegrationResult = export type SupportedIntegrationIds = IntegrationId; export type SupportedHostingIntegrationIds = HostingIntegrationId; +export type SupportedCloudSelfHostedIntegrationIds = CloudSelfHostedIntegrationId; export type SupportedIssueIntegrationIds = IssueIntegrationId; export type SupportedSelfHostedIntegrationIds = SelfHostedIntegrationId; diff --git a/src/plus/integrations/integrationService.ts b/src/plus/integrations/integrationService.ts index 4ed7d3dc5b2fb..d2bb09d6156b1 100644 --- a/src/plus/integrations/integrationService.ts +++ b/src/plus/integrations/integrationService.ts @@ -1,7 +1,11 @@ import type { AuthenticationSessionsChangeEvent, CancellationToken, Event } from 'vscode'; import { authentication, Disposable, env, EventEmitter, ProgressLocation, Uri, window } from 'vscode'; import { isWeb } from '@env/platform'; -import type { IntegrationId, SupportedCloudIntegrationIds } from '../../constants.integrations'; +import type { + CloudSelfHostedIntegrationId, + IntegrationId, + SupportedCloudIntegrationIds, +} from '../../constants.integrations'; import { HostingIntegrationId, IssueIntegrationId, SelfHostedIntegrationId } from '../../constants.integrations'; import type { Source } from '../../constants.telemetry'; import { sourceToContext } from '../../constants.telemetry'; @@ -38,12 +42,13 @@ import type { IntegrationType, IssueIntegration, ResourceDescriptor, + SupportedCloudSelfHostedIntegrationIds, SupportedHostingIntegrationIds, SupportedIntegrationIds, SupportedIssueIntegrationIds, SupportedSelfHostedIntegrationIds, } from './integration'; -import { isHostingIntegrationId, isSelfHostedIntegrationId } from './providers/models'; +import { isCloudSelfHostedIntegrationId, isHostingIntegrationId, isSelfHostedIntegrationId } from './providers/models'; import type { ProvidersApi } from './providers/providersApi'; import { isGitHubDotCom, isGitLabDotCom } from './providers/utils'; @@ -136,20 +141,22 @@ export class IntegrationService implements Disposable { private async *getSupportedCloudIntegrations(domainsById: Map): AsyncIterable { for (const id of getSupportedCloudIntegrationIds()) { - if ( - (id === SelfHostedIntegrationId.CloudGitHubEnterprise || - id === SelfHostedIntegrationId.CloudGitLabSelfHosted) && - !domainsById.has(id) - ) { + if (isCloudSelfHostedIntegrationId(id) && !domainsById.has(id)) { try { // Try getting whatever we have now because we will need to disconnect - yield this.get(id); + const integration = await this.get(id, undefined); + if (integration != null) { + yield integration; + } } catch { // Ignore this exception and continue, // because we probably haven't ever had an instance of this integration } } else { - yield this.get(id, domainsById.get(id)); + const integration = await this.get(id, domainsById.get(id)); + if (integration != null) { + yield integration; + } } } } @@ -245,6 +252,7 @@ export class IntegrationService implements Disposable { for (const integrationId of integrationIds) { try { const integration = await this.get(integrationId); + if (integration == null) continue; if (integration.maybeConnected ?? (await integration.isConnected())) { connectedIntegrations.add(integrationId); } @@ -368,6 +376,7 @@ export class IntegrationService implements Disposable { if (integrationIds != null) { for (const integrationId of integrationIds) { const integration = await this.get(integrationId); + if (integration == null) continue; const connected = integration.maybeConnected ?? (await integration.isConnected()); if (connected && !connectedIntegrations.has(integrationId)) { return true; @@ -455,19 +464,18 @@ export class IntegrationService implements Disposable { return key == null ? this._connectedCache.size !== 0 : this._connectedCache.has(key); } - get( - id: - | SupportedHostingIntegrationIds - | SelfHostedIntegrationId.CloudGitHubEnterprise - | SelfHostedIntegrationId.CloudGitLabSelfHosted, - ): Promise; + get(id: SupportedHostingIntegrationIds): Promise; get(id: SupportedIssueIntegrationIds): Promise; - get(id: SupportedSelfHostedIntegrationIds, domain: string): Promise; - get(id: SupportedIntegrationIds, domain?: string): Promise; + get( + id: SupportedHostingIntegrationIds | SupportedCloudSelfHostedIntegrationIds, + domain?: string, + ): Promise; + get(id: SupportedSelfHostedIntegrationIds, domain: string): Promise; + get(id: SupportedIntegrationIds, domain?: string): Promise; async get( id: SupportedHostingIntegrationIds | SupportedIssueIntegrationIds | SupportedSelfHostedIntegrationIds, domain?: string, - ): Promise { + ): Promise { let integration = this.getCached(id, domain); if (integration == null) { switch (id) { @@ -504,7 +512,7 @@ export class IntegrationService implements Disposable { break; } - throw new Error(`Domain is required for '${id}' integration`); + return undefined; } integration = new ( @@ -562,7 +570,7 @@ export class IntegrationService implements Disposable { break; } - throw new Error(`Domain is required for '${id}' integration`); + return undefined; } integration = new ( @@ -825,25 +833,9 @@ export class IntegrationService implements Disposable { args: { 0: integrationIds => (integrationIds?.length ? integrationIds.join(',') : '') }, }) async getMyCurrentAccounts( - integrationIds: ( - | HostingIntegrationId - | SelfHostedIntegrationId.CloudGitHubEnterprise - | SelfHostedIntegrationId.CloudGitLabSelfHosted - )[], - ): Promise< - Map< - | HostingIntegrationId - | SelfHostedIntegrationId.CloudGitHubEnterprise - | SelfHostedIntegrationId.CloudGitLabSelfHosted, - Account - > - > { - const accounts = new Map< - | HostingIntegrationId - | SelfHostedIntegrationId.CloudGitHubEnterprise - | SelfHostedIntegrationId.CloudGitLabSelfHosted, - Account - >(); + integrationIds: (HostingIntegrationId | CloudSelfHostedIntegrationId)[], + ): Promise> { + const accounts = new Map(); await Promise.allSettled( integrationIds.map(async integrationId => { const integration = await this.get(integrationId); @@ -862,11 +854,7 @@ export class IntegrationService implements Disposable { args: { 0: integrationIds => (integrationIds?.length ? integrationIds.join(',') : ''), 1: false }, }) async getMyPullRequests( - integrationIds?: ( - | HostingIntegrationId - | SelfHostedIntegrationId.CloudGitHubEnterprise - | SelfHostedIntegrationId.CloudGitLabSelfHosted - )[], + integrationIds?: (HostingIntegrationId | CloudSelfHostedIntegrationId)[], cancellation?: CancellationToken, silent?: boolean, ): Promise> { diff --git a/src/plus/integrations/providers/models.ts b/src/plus/integrations/providers/models.ts index c83caa6f4fe0a..7c219a4ad6c99 100644 --- a/src/plus/integrations/providers/models.ts +++ b/src/plus/integrations/providers/models.ts @@ -32,7 +32,7 @@ import { GitPullRequestReviewState, GitPullRequestState, } from '@gitkraken/provider-apis'; -import type { IntegrationId } from '../../../constants.integrations'; +import type { CloudSelfHostedIntegrationId, IntegrationId } from '../../../constants.integrations'; import { HostingIntegrationId, IssueIntegrationId, SelfHostedIntegrationId } from '../../../constants.integrations'; import type { Account as UserAccount } from '../../../git/models/author'; import type { IssueMember, SearchedIssue } from '../../../git/models/issue'; @@ -103,6 +103,10 @@ export function isHostingIntegrationId(id: IntegrationId): id is HostingIntegrat ].includes(id as HostingIntegrationId); } +export function isCloudSelfHostedIntegrationId(id: IntegrationId): id is CloudSelfHostedIntegrationId { + return id === SelfHostedIntegrationId.CloudGitHubEnterprise || id === SelfHostedIntegrationId.CloudGitLabSelfHosted; +} + export enum PullRequestFilter { Author = 'author', Assignee = 'assignee', diff --git a/src/plus/integrations/providers/utils.ts b/src/plus/integrations/providers/utils.ts index e269030086f94..6d58484f18da6 100644 --- a/src/plus/integrations/providers/utils.ts +++ b/src/plus/integrations/providers/utils.ts @@ -12,6 +12,7 @@ import type { LaunchpadItem } from '../../launchpad/launchpadProvider'; import type { IssueResourceDescriptor, RepositoryDescriptor } from '../integration'; import { isIssueResourceDescriptor, isRepositoryDescriptor } from '../integration'; import type { GitConfigEntityIdentifier } from './models'; +import { isCloudSelfHostedIntegrationId } from './models'; export function isGitHubDotCom(domain: string): boolean { return equalsIgnoreCase(domain, 'github.com'); @@ -134,9 +135,7 @@ export function encodeIssueOrPullRequestForGitConfig( id: entity.id, owner: encodedOwner, createdDate: new Date().toISOString(), - isCloudEnterprise: - entity.provider.id === SelfHostedIntegrationId.CloudGitHubEnterprise || - entity.provider.id === SelfHostedIntegrationId.CloudGitLabSelfHosted, + isCloudEnterprise: isCloudSelfHostedIntegrationId(entity.provider.id as IntegrationId), }, }; } diff --git a/src/plus/launchpad/launchpad.ts b/src/plus/launchpad/launchpad.ts index 60a3df301ab11..2bd18e946450f 100644 --- a/src/plus/launchpad/launchpad.ts +++ b/src/plus/launchpad/launchpad.ts @@ -199,6 +199,7 @@ export class LaunchpadCommand extends QuickCommand { private async ensureIntegrationConnected(id: IntegrationId) { const integration = await this.container.integrations.get(id); + if (integration == null) return false; let connected = integration.maybeConnected ?? (await integration.isConnected()); if (!connected) { connected = await integration.connect('launchpad'); diff --git a/src/plus/launchpad/launchpadProvider.ts b/src/plus/launchpad/launchpadProvider.ts index 80b70ae67b9cc..e2852767c0709 100644 --- a/src/plus/launchpad/launchpadProvider.ts +++ b/src/plus/launchpad/launchpadProvider.ts @@ -7,7 +7,7 @@ import type { CancellationToken, ConfigurationChangeEvent } from 'vscode'; import { Disposable, env, EventEmitter, Uri, window } from 'vscode'; import { md5 } from '@env/crypto'; import { GlCommand } from '../../constants.commands'; -import type { IntegrationId } from '../../constants.integrations'; +import type { CloudSelfHostedIntegrationId, IntegrationId } from '../../constants.integrations'; import { HostingIntegrationId, SelfHostedIntegrationId } from '../../constants.integrations'; import type { Container } from '../../container'; import { CancellationError } from '../../errors'; @@ -107,11 +107,7 @@ type PullRequestsWithSuggestionCounts = { export type LaunchpadRefreshEvent = LaunchpadCategorizedResult; -export const supportedLaunchpadIntegrations: ( - | HostingIntegrationId - | SelfHostedIntegrationId.CloudGitHubEnterprise - | SelfHostedIntegrationId.CloudGitLabSelfHosted -)[] = [ +export const supportedLaunchpadIntegrations: (HostingIntegrationId | CloudSelfHostedIntegrationId)[] = [ HostingIntegrationId.GitHub, SelfHostedIntegrationId.CloudGitHubEnterprise, HostingIntegrationId.GitLab, @@ -278,6 +274,7 @@ export class LaunchpadProvider implements Disposable { ) .map(async (id: SupportedLaunchpadIntegrationIds) => { const integration = await this.container.integrations.get(id); + if (integration == null) return; const searchResult = await searchIntegrationPRs(integration); const prs = searchResult?.value; if (prs) { @@ -412,6 +409,7 @@ export class LaunchpadProvider implements Disposable { }); if (confirm !== 'Merge') return; const integration = await this.container.integrations.get(integrationId); + if (integration == null) return; const pr: PullRequest = fromProviderPullRequest(item, integration); await integration.mergePullRequest(pr); this.refresh(); @@ -604,6 +602,7 @@ export class LaunchpadProvider implements Disposable { for (const integrationId of supportedLaunchpadIntegrations) { if (connectedIntegrations.get(integrationId)) { const integration = await this.container.integrations.get(integrationId); + if (integration == null) continue; const prIdentity = integration.getPullRequestIdentityFromMaybeUrl(search); if (prIdentity) { return prIdentity; @@ -873,6 +872,7 @@ export class LaunchpadProvider implements Disposable { async hasConnectedIntegration(): Promise { for (const integrationId of supportedLaunchpadIntegrations) { const integration = await this.container.integrations.get(integrationId); + if (integration == null) continue; if (integration.maybeConnected ?? (await integration.isConnected())) { return true; } @@ -887,6 +887,10 @@ export class LaunchpadProvider implements Disposable { await Promise.allSettled( supportedLaunchpadIntegrations.map(async integrationId => { const integration = await this.container.integrations.get(integrationId); + if (integration == null) { + connected.set(integrationId, false); + return; + } const isConnected = integration.maybeConnected ?? (await integration.isConnected()); const hasAccess = isConnected && (await integration.access()); connected.set(integrationId, hasAccess); diff --git a/src/plus/startWork/startWork.ts b/src/plus/startWork/startWork.ts index 582bd354986fa..d6d10ecc1e549 100644 --- a/src/plus/startWork/startWork.ts +++ b/src/plus/startWork/startWork.ts @@ -316,6 +316,7 @@ export abstract class StartWorkBaseCommand extends QuickCommand { private async ensureIntegrationConnected(id: IntegrationId) { const integration = await this.container.integrations.get(id); + if (integration == null) return false; let connected = integration.maybeConnected ?? (await integration.isConnected()); if (!connected) { connected = await integration.connect(this.overrides?.ownSource ?? 'startWork'); @@ -726,6 +727,10 @@ async function getConnectedIntegrations(container: Container): Promise { const integration = await container.integrations.get(integrationId); + if (integration == null) { + connected.set(integrationId, false); + return; + } const isConnected = integration.maybeConnected ?? (await integration.isConnected()); const hasAccess = isConnected && (await integration.access()); connected.set(integrationId, hasAccess);