From cfa2715cf18e4c0a22ef172e2c383d3014ee1d96 Mon Sep 17 00:00:00 2001 From: Sergey Petushkov Date: Fri, 17 Jan 2025 18:10:44 +0100 Subject: [PATCH 1/2] chore(web): catch and log errors when building connection info instead of throwing --- .../src/connection-storage.spec.ts | 137 +++++++++++++++++- .../compass-web/src/connection-storage.tsx | 119 ++++++++++----- 2 files changed, 214 insertions(+), 42 deletions(-) diff --git a/packages/compass-web/src/connection-storage.spec.ts b/packages/compass-web/src/connection-storage.spec.ts index 17a5ddbc8e3..a08b8089d47 100644 --- a/packages/compass-web/src/connection-storage.spec.ts +++ b/packages/compass-web/src/connection-storage.spec.ts @@ -1,5 +1,9 @@ import { expect } from 'chai'; -import { buildConnectionInfoFromClusterDescription } from './connection-storage'; +import { createNoopLogger } from '@mongodb-js/compass-logging/provider'; +import { + buildConnectionInfoFromClusterDescription, + AtlasCloudConnectionStorage, +} from './connection-storage'; import type { ClusterDescriptionWithDataProcessingRegion } from './connection-storage'; const deployment = { @@ -140,7 +144,7 @@ describe('buildConnectionInfoFromClusterDescription', function () { connectionString ); - expect(connectionInfo.connectionOptions.lookup()).to.deep.eq({ + expect(connectionInfo.connectionOptions.lookup?.()).to.deep.eq({ wsURL: 'ws://test', projectId: 'abc', clusterName: `Cluster0-${type}`, @@ -172,4 +176,133 @@ describe('buildConnectionInfoFromClusterDescription', function () { }); }); } + + it('should throw if deployment item is missing', function () { + try { + buildConnectionInfoFromClusterDescription( + 'ws://test', + '123', + 'abc', + { + '@provider': 'mock', + uniqueId: 'abc', + groupId: 'abc', + name: 'Cluster0', + clusterType: 'REPLICASET', + srvAddress: 'test', + state: 'test', + deploymentItemName: 'test', + dataProcessingRegion: { regionalUrl: 'test' }, + }, + deployment + ); + expect.fail('Expected method to throw'); + } catch (err) { + expect(err).to.have.property( + 'message', + "Can't build metrics info when deployment item is not found" + ); + } + }); +}); + +describe('AtlasCloudConnectionStorage', function () { + const testClusters: Record< + string, + Partial + > = { + Cluster0: { + '@provider': 'AWS', + groupId: 'abc', + name: 'Cluster0', + clusterType: 'REPLICASET', + srvAddress: 'test', + state: 'test', + deploymentItemName: 'replicaSet-xxx', + dataProcessingRegion: { regionalUrl: 'test' }, + }, + NoDeploymentItem: { + '@provider': 'AWS', + groupId: 'abc', + name: 'NoDeploymentItem', + clusterType: 'REPLICASET', + srvAddress: 'test', + state: 'test', + deploymentItemName: 'not-found', + dataProcessingRegion: { regionalUrl: 'test' }, + }, + NoSrvAddress: { + '@provider': 'AWS', + name: 'NoSrvAddress', + }, + Paused: { + '@provider': 'AWS', + name: 'Paused', + isPaused: true, + }, + WillThrowOnFetch: { + '@provider': 'AWS', + name: 'WillThrowOnFetch', + }, + }; + + describe('#loadAll', function () { + it('should load connection descriptions filtering out the ones that failed to fetch', async function () { + const atlasService = { + cloudEndpoint(path: string) { + return path; + }, + driverProxyEndpoint(path: string) { + return path; + }, + authenticatedFetch(path: string) { + let payload: any; + if (path === '/deployment/abc') { + payload = deployment; + } + if (path === '/nds/clusters/abc') { + payload = Array.from(Object.values(testClusters)); + } + const { groups } = + /\/nds\/clusters\/abc\/(?.+?)\/+?/.exec(path) ?? { + groups: undefined, + }; + if (groups?.clusterName) { + if (groups?.clusterName === 'WillThrowOnFetch') { + return Promise.reject( + new Error('Failed to fetch cluster description') + ); + } + payload = testClusters[groups.clusterName]; + } + return Promise.resolve({ + json() { + return payload; + }, + }); + }, + }; + const logger = createNoopLogger(); + const connectionStorage = new AtlasCloudConnectionStorage( + atlasService as any, + '123', + 'abc', + logger + ); + + const connectionsPromise = connectionStorage.loadAll(); + + expect(connectionsPromise).to.eq( + connectionStorage.loadAll(), + 'Expected loadAll to return the same instance of the loading promise while connections are loading' + ); + + const connections = await connectionsPromise; + + // We expect all other clusters to be filtered out for one reason or + // another + expect(connections).to.have.lengthOf(1); + expect(connections[0]).to.have.property('id', 'Cluster0'); + }); + }); }); diff --git a/packages/compass-web/src/connection-storage.tsx b/packages/compass-web/src/connection-storage.tsx index 027f9d9b88e..6a69149a48d 100644 --- a/packages/compass-web/src/connection-storage.tsx +++ b/packages/compass-web/src/connection-storage.tsx @@ -11,6 +11,11 @@ import ConnectionString from 'mongodb-connection-string-url'; import { createServiceProvider } from 'hadron-app-registry'; import type { AtlasService } from '@mongodb-js/atlas-service/provider'; import { atlasServiceLocator } from '@mongodb-js/atlas-service/provider'; +import { + mongoLogId, + useLogger, + type Logger, +} from '@mongodb-js/compass-logging/provider'; type ElectableSpecs = { instanceSize?: string; @@ -156,7 +161,7 @@ export function buildConnectionInfoFromClusterDescription( description: ClusterDescriptionWithDataProcessingRegion, deployment: Deployment, extraConnectionOptions?: Record -) { +): ConnectionInfo { const connectionString = new ConnectionString( `mongodb+srv://${description.srvAddress}` ); @@ -221,7 +226,10 @@ export function buildConnectionInfoFromClusterDescription( }; } -class AtlasCloudConnectionStorage +/** + * @internal exported for testing purposes + */ +export class AtlasCloudConnectionStorage extends InMemoryConnectionStorage implements ConnectionStorage { @@ -230,6 +238,7 @@ class AtlasCloudConnectionStorage private atlasService: AtlasService, private orgId: string, private projectId: string, + private logger: Logger, private extraConnectionOptions?: Record ) { super(); @@ -249,67 +258,95 @@ class AtlasCloudConnectionStorage // TODO(CLOUDP-249088): replace with the list request that already // contains regional data when it exists instead of fetching // one-by-one after the list fetch - this.atlasService.cloudEndpoint(`nds/clusters/${this.projectId}`) + this.atlasService.cloudEndpoint(`/nds/clusters/${this.projectId}`) ) .then((res) => { return res.json() as Promise; }) .then((descriptions) => { return Promise.all( - descriptions - .filter((description) => { - // Only list fully deployed clusters - // TODO(COMPASS-8228): We should probably list all and just - // account in the UI for a special state of a deployment as - // clusters can become inactive during their runtime and it's - // valuable UI info to display - return !description.isPaused && !!description.srvAddress; - }) - .map(async (description) => { - // Even though nds/clusters will list serverless clusters, to get - // the regional description we need to change the url - const clusterDescriptionType = isServerless(description) - ? 'serverless' - : 'clusters'; + descriptions.map(async (description) => { + // Even though nds/clusters will list serverless clusters, to get + // the regional description we need to change the url + const clusterDescriptionType = isServerless(description) + ? 'serverless' + : 'clusters'; + try { const res = await this.atlasService.authenticatedFetch( this.atlasService.cloudEndpoint( - `nds/${clusterDescriptionType}/${this.projectId}/${description.name}/regional/clusterDescription` + `/nds/${clusterDescriptionType}/${this.projectId}/${description.name}/regional/clusterDescription` ) ); return await (res.json() as Promise); - }) + } catch (err) { + this.logger.log.error( + mongoLogId(1_001_000_303), + 'LoadAndNormalizeClusterDescriptionInfo', + 'Failed to fetch cluster description for cluster', + { clusterName: description.name, error: (err as Error).stack } + ); + return null; + } + }) ); }), this.atlasService .authenticatedFetch( - this.atlasService.cloudEndpoint(`deployment/${this.projectId}`) + this.atlasService.cloudEndpoint(`/deployment/${this.projectId}`) ) .then((res) => { return res.json() as Promise; }), ]); - return clusterDescriptions.map((description) => { - return buildConnectionInfoFromClusterDescription( - this.atlasService.driverProxyEndpoint( - `/clusterConnection/${this.projectId}` - ), - this.orgId, - this.projectId, - description, - deployment, - this.extraConnectionOptions - ); - }); + return clusterDescriptions + .map((description) => { + // Clear cases where cluster doesn't have enough metadata + // - Failed to get the description + // - Cluster is paused + // - Cluster is missing an srv address (happens during deployment / + // termination) + if (!description || !!description.isPaused || !description.srvAddress) { + return null; + } + + try { + // We will always try to build the metadata, it can fail if deployment + // item for the cluster is missing even when description exists + // (happens during deployment / termination / weird corner cases of + // atlas cluster state) + return buildConnectionInfoFromClusterDescription( + this.atlasService.driverProxyEndpoint( + `/clusterConnection/${this.projectId}` + ), + this.orgId, + this.projectId, + description, + deployment, + this.extraConnectionOptions + ); + } catch (err) { + this.logger.log.error( + mongoLogId(1_001_000_304), + 'LoadAndNormalizeClusterDescriptionInfo', + 'Failed to build connection info from cluster description', + { clusterName: description.name, error: (err as Error).stack } + ); + + return null; + } + }) + .filter((connectionInfo): connectionInfo is ConnectionInfo => { + return !!connectionInfo; + }); } - async loadAll(): Promise { - try { - return (this.loadAllPromise ??= - this._loadAndNormalizeClusterDescriptionInfo()); - } finally { - delete this.loadAllPromise; - } + loadAll(): Promise { + this.loadAllPromise ??= + this._loadAndNormalizeClusterDescriptionInfo().finally(() => { + delete this.loadAllPromise; + }); + return this.loadAllPromise; } } @@ -358,12 +395,14 @@ export const AtlasCloudConnectionStorageProvider = createServiceProvider( const extraConnectionOptions = useContext( SandboxExtraConnectionOptionsContext ); + const logger = useLogger('ATLAS-CLOUD-CONNECTION-STORAGE'); const atlasService = atlasServiceLocator(); const storage = useRef( new AtlasCloudConnectionStorage( atlasService, orgId, projectId, + logger, extraConnectionOptions ) ); From 8d9d460cb5cacebc668cc04ee7b4658754fa4cbf Mon Sep 17 00:00:00 2001 From: Sergey Petushkov Date: Mon, 20 Jan 2025 17:42:40 +0100 Subject: [PATCH 2/2] chore(web): stricter regex in test Co-authored-by: Anna Henningsen --- packages/compass-web/src/connection-storage.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/compass-web/src/connection-storage.spec.ts b/packages/compass-web/src/connection-storage.spec.ts index a08b8089d47..7d2df316020 100644 --- a/packages/compass-web/src/connection-storage.spec.ts +++ b/packages/compass-web/src/connection-storage.spec.ts @@ -264,7 +264,7 @@ describe('AtlasCloudConnectionStorage', function () { payload = Array.from(Object.values(testClusters)); } const { groups } = - /\/nds\/clusters\/abc\/(?.+?)\/+?/.exec(path) ?? { + /^\/nds\/clusters\/abc\/(?.+?)\/.+?$/.exec(path) ?? { groups: undefined, }; if (groups?.clusterName) {