From b368c11cbf8277b1a7a0e244e324b4340cc5c04c Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 24 Apr 2024 12:51:51 +0200 Subject: [PATCH 1/3] fix(cli-repl): do not load connectionInfo in quiet non-REPL mode MONGOSH-1765 Do not call `fetchConnectionInfo()` and similar methods when they are not needed. This hopefully improves startup performance in real-world scenarios a bit further. In order to achieve this: - Implement a cache/lazy-loading ability for the connection info in the shell-api `ShellInstanceState` class. We now only fetch connection info here if requested, and only refresh it if the service provider instance changed (and not just the database itself). - Adjust usage of the `fetchConnectionInfo()` method to only be called if we believe it to be necessary, and for its result to be awaited only if we need it immediately. This also adds type safety by removing the `any` typing for `connectionInfo` in `ShellInstanceState`. I admittedly thought that this would be a quick fix, but unfortunately (as seen in the commit diff), this spiraled a bit into different packages and tests as a larger change to align the typings for this object. --- packages/autocomplete/src/index.ts | 8 +- packages/browser-repl/src/sandbox.tsx | 4 +- .../src/open-context-runtime.ts | 2 +- packages/cli-repl/src/mongosh-repl.spec.ts | 36 ++++++ packages/cli-repl/src/mongosh-repl.ts | 47 +++++--- .../logging/src/setup-logger-and-telemetry.ts | 2 +- packages/service-provider-core/src/admin.ts | 10 +- .../service-provider-core/src/connect-info.ts | 30 ++--- packages/service-provider-core/src/index.ts | 7 +- .../src/cli-service-provider.ts | 12 +- packages/shell-api/src/mongo.spec.ts | 16 ++- packages/shell-api/src/mongo.ts | 3 +- packages/shell-api/src/shard.spec.ts | 12 +- packages/shell-api/src/shard.ts | 10 +- .../src/shell-instance-state.spec.ts | 27 ++++- .../shell-api/src/shell-instance-state.ts | 110 +++++++++++++----- packages/types/src/index.ts | 20 ++-- 17 files changed, 255 insertions(+), 101 deletions(-) diff --git a/packages/autocomplete/src/index.ts b/packages/autocomplete/src/index.ts index 17dcbca71..63b6120c9 100644 --- a/packages/autocomplete/src/index.ts +++ b/packages/autocomplete/src/index.ts @@ -21,10 +21,10 @@ export interface AutocompleteParameters { connectionInfo: () => | undefined | { - is_atlas: boolean; - is_data_federation: boolean; - server_version: string; - is_local_atlas: boolean; + is_atlas?: boolean; + is_data_federation?: boolean; + server_version?: string; + is_local_atlas?: boolean; }; apiVersionInfo: () => { version: string; strict: boolean } | undefined; getCollectionCompletionsForCurrentDb: ( diff --git a/packages/browser-repl/src/sandbox.tsx b/packages/browser-repl/src/sandbox.tsx index 1bf7f5d33..a7fb33d54 100644 --- a/packages/browser-repl/src/sandbox.tsx +++ b/packages/browser-repl/src/sandbox.tsx @@ -14,6 +14,7 @@ import { import { IframeRuntime } from './iframe-runtime'; import { Shell } from './index'; import type { ShellOutputEntry } from './components/shell-output-line'; +import type { ConnectionInfo } from '@mongosh/service-provider-core'; injectGlobal({ body: { @@ -94,12 +95,13 @@ class DemoServiceProvider { }; } - async getConnectionInfo(): Promise { + async getConnectionInfo(): Promise { return { buildInfo: await this.buildInfo(), extraInfo: { uri: 'mongodb://localhost/', }, + topology: null, }; } diff --git a/packages/browser-runtime-core/src/open-context-runtime.ts b/packages/browser-runtime-core/src/open-context-runtime.ts index e5f38a70d..50d5ea97c 100644 --- a/packages/browser-runtime-core/src/open-context-runtime.ts +++ b/packages/browser-runtime-core/src/open-context-runtime.ts @@ -33,7 +33,7 @@ export class OpenContextRuntime implements Runtime { private shellEvaluator: ShellEvaluator; private instanceState: ShellInstanceState; private evaluationListener: RuntimeEvaluationListener | null = null; - private updatedConnectionInfoPromise: Promise | null = null; + private updatedConnectionInfoPromise: Promise | null = null; constructor( serviceProvider: ServiceProvider, diff --git a/packages/cli-repl/src/mongosh-repl.spec.ts b/packages/cli-repl/src/mongosh-repl.spec.ts index 508e01599..8cae1f0aa 100644 --- a/packages/cli-repl/src/mongosh-repl.spec.ts +++ b/packages/cli-repl/src/mongosh-repl.spec.ts @@ -45,6 +45,9 @@ describe('MongoshNodeRepl', function () { let ioProvider: MongoshIOProvider; let sp: StubbedInstance; let serviceProvider: ServiceProvider; + let calledServiceProviderFunctions: () => Partial< + Record + >; let config: Record; const tmpdir = useTmpdir(); @@ -83,9 +86,16 @@ describe('MongoshNodeRepl', function () { buildInfo: { version: '4.4.1', }, + topology: null, }); sp.runCommandWithCheck.resolves({ ok: 1 }); serviceProvider = sp; + calledServiceProviderFunctions = () => + Object.fromEntries( + Object.keys(sp) + .map((key) => [key, sp[key]?.callCount]) + .filter(([, count]) => !!count) + ); mongoshReplOptions = { input: input, @@ -128,6 +138,7 @@ describe('MongoshNodeRepl', function () { /You can opt-out by running the .*disableTelemetry\(\).* command/ ); expect(config.disableGreetingMessage).to.equal(true); + expect(sp.getConnectionInfo).to.have.been.calledOnce; }); it('evaluates javascript', async function () { @@ -1248,6 +1259,7 @@ describe('MongoshNodeRepl', function () { version: '4.4.1', modules: ['enterprise'], }, + topology: null, }); const initialized = await mongoshRepl.initialize(serviceProvider); @@ -1279,6 +1291,7 @@ describe('MongoshNodeRepl', function () { version: '4.4.1', modules: ['enterprise'], }, + topology: null, }; sp.getConnectionInfo.resolves(connectionInfo); @@ -1408,6 +1421,7 @@ describe('MongoshNodeRepl', function () { buildInfo: { version: '4.4.1', }, + topology: null, }); mongoshReplOptions.shellCliOptions = { nodb: false, @@ -1438,4 +1452,26 @@ describe('MongoshNodeRepl', function () { expect(warnings).to.have.lengthOf(0); }); }); + + context('interactions with the server during startup', function () { + it('calls a number of service provider functions by default', async function () { + await mongoshRepl.initialize(serviceProvider); + const calledFunctions = calledServiceProviderFunctions(); + expect(calledFunctions).to.include.keys( + 'getConnectionInfo', + 'runCommandWithCheck' + ); + }); + + it('does not get connection info in --quiet no-REPL mode', async function () { + mongoshRepl.shellCliOptions.quiet = true; + mongoshRepl.shellCliOptions.jsContext = 'plain-vm'; + await mongoshRepl.initialize(serviceProvider); + const calledFunctions = calledServiceProviderFunctions(); + expect(Object.keys(calledFunctions).sort()).to.deep.equal([ + 'getFleOptions', + 'getURI', + ]); + }); + }); }); diff --git a/packages/cli-repl/src/mongosh-repl.ts b/packages/cli-repl/src/mongosh-repl.ts index fd3e742db..2d9a6d957 100644 --- a/packages/cli-repl/src/mongosh-repl.ts +++ b/packages/cli-repl/src/mongosh-repl.ts @@ -179,11 +179,14 @@ class MongoshNodeRepl implements EvaluationListener { serviceProvider: ServiceProvider, moreRecentMongoshVersion?: string | null ): Promise { + const usePlainVMContext = this.shellCliOptions.jsContext === 'plain-vm'; + const instanceState = new ShellInstanceState( serviceProvider, this.bus, this.shellCliOptions ); + const shellEvaluator = new ShellEvaluator( instanceState, (value: any) => value, @@ -191,20 +194,34 @@ class MongoshNodeRepl implements EvaluationListener { !!this.shellCliOptions.exposeAsyncRewriter ); instanceState.setEvaluationListener(this); - await instanceState.fetchConnectionInfo(); - markTime(TimingCategories.REPLInstantiation, 'fetched connection info'); - - const { buildInfo, extraInfo } = instanceState.connectionInfo; - let mongodVersion = extraInfo?.is_stream - ? 'Atlas Stream Processing' - : buildInfo?.version; - const apiVersion = serviceProvider.getRawClient()?.serverApi?.version; - if (apiVersion) { - mongodVersion = - (mongodVersion ? mongodVersion + ' ' : '') + - `(API Version ${apiVersion})`; + + // Fetch connection metadata if not in quiet mode or in REPL mode: + // not-quiet mode -> We'll need it for the greeting message (and need it now) + // REPL mode -> We'll want it for fast autocomplete (and need it soon-ish, but not now) + instanceState.setPreFetchCollectionAndDatabaseNames(!usePlainVMContext); + if (!this.shellCliOptions.quiet || !usePlainVMContext) { + const connectionInfoPromise = instanceState.fetchConnectionInfo(); + connectionInfoPromise.catch(() => { + // Ignore potential unhandled rejection warning + }); + if (!this.shellCliOptions.quiet) { + const connectionInfo = await connectionInfoPromise; + markTime(TimingCategories.REPLInstantiation, 'fetched connection info'); + + const { buildInfo, extraInfo } = connectionInfo ?? {}; + let mongodVersion = extraInfo?.is_stream + ? 'Atlas Stream Processing' + : buildInfo?.version; + const apiVersion = serviceProvider.getRawClient()?.serverApi?.version; + if (apiVersion) { + mongodVersion = + (mongodVersion ? mongodVersion + ' ' : '') + + `(API Version ${apiVersion})`; + } + await this.greet(mongodVersion, moreRecentMongoshVersion); + } } - await this.greet(mongodVersion, moreRecentMongoshVersion); + await this.printBasicConnectivityWarning(instanceState); markTime(TimingCategories.REPLInstantiation, 'greeted'); @@ -220,7 +237,7 @@ class MongoshNodeRepl implements EvaluationListener { let repl: REPLServer | null = null; let context: Context; - if (this.shellCliOptions.jsContext !== 'plain-vm') { + if (!usePlainVMContext) { repl = asyncRepl.start({ // 'repl' is not supported in startup snapshots yet. // eslint-disable-next-line @typescript-eslint/no-var-requires @@ -791,7 +808,7 @@ class MongoshNodeRepl implements EvaluationListener { this.output.write('Stopping execution...'); const mongodVersion: string | undefined = - instanceState.connectionInfo.buildInfo?.version; + instanceState.cachedConnectionInfo()?.buildInfo?.version; if (mongodVersion?.match(/^(4\.0\.|3\.)\d+/)) { this.output.write( this.clr( diff --git a/packages/logging/src/setup-logger-and-telemetry.ts b/packages/logging/src/setup-logger-and-telemetry.ts index d326cb41f..c2ce8834f 100644 --- a/packages/logging/src/setup-logger-and-telemetry.ts +++ b/packages/logging/src/setup-logger-and-telemetry.ts @@ -141,7 +141,7 @@ export function setupLoggerAndTelemetry( ); bus.on('mongosh:connect', function (args: ConnectEvent) { - const connectionUri = redactURICredentials(args.uri); + const connectionUri = args.uri && redactURICredentials(args.uri); // eslint-disable-next-line @typescript-eslint/no-unused-vars const { uri: _uri, ...argsWithoutUri } = args; const params = { diff --git a/packages/service-provider-core/src/admin.ts b/packages/service-provider-core/src/admin.ts index 39bcecf1a..d1f2797c7 100644 --- a/packages/service-provider-core/src/admin.ts +++ b/packages/service-provider-core/src/admin.ts @@ -13,7 +13,7 @@ import type { AutoEncryptionOptions, Collection, } from './all-transport-types'; -import type { bson as BSON } from './index'; +import type { bson as BSON, ConnectionExtraInfo } from './index'; import type { ReplPlatform } from './platform'; import type { AWSEncryptionKeyOptions, @@ -43,6 +43,12 @@ export interface CheckMetadataConsistencyOptions { checkIndexes?: 1; } +export interface ConnectionInfo { + buildInfo: Document | null; + topology: any | null; + extraInfo: (ConnectionExtraInfo & { fcv?: string }) | null; +} + export default interface Admin { /** * What platform (Compass/CLI/Browser) @@ -87,7 +93,7 @@ export default interface Admin { /** * Return connection info */ - getConnectionInfo(): Promise; + getConnectionInfo(): Promise; /** * Authenticate diff --git a/packages/service-provider-core/src/connect-info.ts b/packages/service-provider-core/src/connect-info.ts index 688f7db2f..b16fc1ec5 100644 --- a/packages/service-provider-core/src/connect-info.ts +++ b/packages/service-provider-core/src/connect-info.ts @@ -2,35 +2,35 @@ import getBuildInfo from 'mongodb-build-info'; -export interface ConnectInfo { - is_atlas: boolean; - is_localhost: boolean; - is_do: boolean; - server_version: string; - mongosh_version: string; +export interface ConnectionExtraInfo { + is_atlas?: boolean; + is_localhost?: boolean; + is_do?: boolean; + server_version?: string; + mongosh_version?: string; server_os?: string; server_arch?: string; - is_enterprise: boolean; + is_enterprise?: boolean; auth_type?: string; - is_data_federation: boolean; - is_stream: boolean; + is_data_federation?: boolean; + is_stream?: boolean; dl_version?: string; atlas_version?: string; - is_genuine: boolean; - non_genuine_server_name: string; - node_version: string; + is_genuine?: boolean; + non_genuine_server_name?: string; + node_version?: string; uri: string; - is_local_atlas: boolean; + is_local_atlas?: boolean; } -export default function getConnectInfo( +export default function getConnectExtraInfo( uri: string, mongoshVersion: string, buildInfo: any, atlasVersion: any, topology: any, isLocalAtlas: boolean -): ConnectInfo { +): ConnectionExtraInfo { buildInfo ??= {}; // We're currently not getting buildInfo with --apiStrict. const { isGenuine: is_genuine, serverName: non_genuine_server_name } = getBuildInfo.getGenuineMongoDB(uri); diff --git a/packages/service-provider-core/src/index.ts b/packages/service-provider-core/src/index.ts index 91ad7881c..ed965048f 100644 --- a/packages/service-provider-core/src/index.ts +++ b/packages/service-provider-core/src/index.ts @@ -1,6 +1,6 @@ import './textencoder-polyfill'; // for mongodb-connection-string-url in the java-shell import ServiceProvider, { ServiceProviderCore } from './service-provider'; -import getConnectInfo, { ConnectInfo } from './connect-info'; +import getConnectExtraInfo, { ConnectionExtraInfo } from './connect-info'; import type { ReplPlatform } from './platform'; const DEFAULT_DB = 'test'; import { bsonStringifiers } from './printable-bson'; @@ -13,6 +13,7 @@ export { MapReduceOptions, FinalizeFunction } from './map-reduce-options'; export { CreateEncryptedCollectionOptions, CheckMetadataConsistencyOptions, + ConnectionInfo, } from './admin'; export { bson } from './bson-export'; @@ -20,10 +21,10 @@ export { bson } from './bson-export'; export { ServiceProvider, ShellAuthOptions, - getConnectInfo, + getConnectExtraInfo, ReplPlatform, DEFAULT_DB, ServiceProviderCore, bsonStringifiers, - ConnectInfo, + ConnectionExtraInfo, }; diff --git a/packages/service-provider-server/src/cli-service-provider.ts b/packages/service-provider-server/src/cli-service-provider.ts index b79fd305c..c1b371e1f 100644 --- a/packages/service-provider-server/src/cli-service-provider.ts +++ b/packages/service-provider-server/src/cli-service-provider.ts @@ -62,9 +62,10 @@ import type { ChangeStream, AutoEncryptionOptions, ClientEncryption as MongoCryptClientEncryption, + ConnectionInfo, } from '@mongosh/service-provider-core'; import { - getConnectInfo, + getConnectExtraInfo, DEFAULT_DB, ServiceProviderCore, } from '@mongosh/service-provider-core'; @@ -130,13 +131,6 @@ type DropDatabaseResult = { dropped?: string; }; -type ConnectionInfo = { - buildInfo: any; - topology: any; - extraInfo: ExtraConnectionInfo; -}; -type ExtraConnectionInfo = ReturnType & { fcv?: string }; - /** * Default driver options we always use. */ @@ -436,7 +430,7 @@ class CliServiceProvider const isLocalAtlasCli = !!atlascliInfo; - const extraConnectionInfo = getConnectInfo( + const extraConnectionInfo = getConnectExtraInfo( this.uri?.toString() ?? '', version, buildInfo, diff --git a/packages/shell-api/src/mongo.spec.ts b/packages/shell-api/src/mongo.spec.ts index 6302840a4..4f22f630b 100644 --- a/packages/shell-api/src/mongo.spec.ts +++ b/packages/shell-api/src/mongo.spec.ts @@ -437,9 +437,11 @@ describe('Mongo', function () { describe('nonGenuineMongoDBCheck', function () { it('returns no warnings for a genuine mongodb connection', async function () { - instanceState.connectionInfo = { - extraInfo: { is_genuine: true }, - }; + serviceProvider.getConnectionInfo.resolves({ + extraInfo: { is_genuine: true, uri: '' }, + buildInfo: {}, + topology: null, + }); const result = await mongo.show('nonGenuineMongoDBCheck'); expect(result.type).to.equal('ShowBannerResult'); @@ -450,9 +452,11 @@ describe('Mongo', function () { 'when connected deployment is not a genuine mongodb deployment', function () { beforeEach(function () { - instanceState.connectionInfo = { - extraInfo: { is_genuine: false }, - }; + serviceProvider.getConnectionInfo.resolves({ + extraInfo: { is_genuine: false, uri: '' }, + buildInfo: {}, + topology: null, + }); }); const warning = [ diff --git a/packages/shell-api/src/mongo.ts b/packages/shell-api/src/mongo.ts index 49e003423..7285e5862 100644 --- a/packages/shell-api/src/mongo.ts +++ b/packages/shell-api/src/mongo.ts @@ -485,7 +485,8 @@ export default class Mongo extends ShellApiClass { // Although very unlikely but if we cannot determine wether we are connected to a fake mongodb // or not, we assume that we are connected to a real mongodb and won't show the warning const isGenuine = - this._instanceState.connectionInfo?.extraInfo?.is_genuine ?? true; + (await this._instanceState.fetchConnectionInfo())?.extraInfo + ?.is_genuine ?? true; if (isGenuine) { return new CommandResult('ShowBannerResult', null); } diff --git a/packages/shell-api/src/shard.spec.ts b/packages/shell-api/src/shard.spec.ts index dfeb46bcd..498a2a0ac 100644 --- a/packages/shell-api/src/shard.spec.ts +++ b/packages/shell-api/src/shard.spec.ts @@ -866,7 +866,11 @@ describe('Shard', function () { }); it('prints a deprecation warning for mongodb >= 6.0.3', async function () { - instanceState.connectionInfo.buildInfo.version = '6.0.3-alpha0'; + serviceProvider.getConnectionInfo.resolves({ + extraInfo: { uri: '' }, + buildInfo: { version: '6.0.3-alpha0' }, + topology: null, + }); serviceProvider.runCommandWithCheck.resolves({ ok: 1 }); serviceProvider.updateOne.resolves({ acknowledged: 1 } as any); await shard.enableAutoSplit(); @@ -943,7 +947,11 @@ describe('Shard', function () { }); it('prints a deprecation warning for mongodb >= 6.0.3', async function () { - instanceState.connectionInfo.buildInfo.version = '6.0.3-alpha0'; + serviceProvider.getConnectionInfo.resolves({ + extraInfo: { uri: '' }, + buildInfo: { version: '6.0.3-alpha0' }, + topology: null, + }); serviceProvider.runCommandWithCheck.resolves({ ok: 1 }); serviceProvider.updateOne.resolves({ acknowledged: 1 } as any); await shard.disableAutoSplit(); diff --git a/packages/shell-api/src/shard.ts b/packages/shell-api/src/shard.ts index ee0538a36..9ee7ffa1f 100644 --- a/packages/shell-api/src/shard.ts +++ b/packages/shell-api/src/shard.ts @@ -394,9 +394,10 @@ export default class Shard extends ShellApiWithMongoClass { @apiVersions([1]) @serverVersions(['3.4.0', '6.0.2']) async enableAutoSplit(): Promise { + const connectionInfo = await this._instanceState.fetchConnectionInfo(); if ( - this._instanceState.connectionInfo.buildInfo.version && - semver.gte(this._instanceState.connectionInfo.buildInfo.version, '6.0.3') + connectionInfo?.buildInfo?.version && + semver.gte(connectionInfo.buildInfo.version, '6.0.3') ) { await this._instanceState.printDeprecationWarning( 'Starting in MongoDB 6.0.3, automatic chunk splitting is not performed. This is because of balancing policy improvements. Auto-splitting commands still exist, but do not perform an operation. For details, see Balancing Policy Changes: https://www.mongodb.com/docs/manual/release-notes/6.0/#balancing-policy-changes\n' @@ -417,9 +418,10 @@ export default class Shard extends ShellApiWithMongoClass { @apiVersions([1]) @serverVersions(['3.4.0', '6.0.2']) async disableAutoSplit(): Promise { + const connectionInfo = await this._instanceState.fetchConnectionInfo(); if ( - this._instanceState.connectionInfo.buildInfo.version && - semver.gte(this._instanceState.connectionInfo.buildInfo.version, '6.0.3') + connectionInfo?.buildInfo?.version && + semver.gte(connectionInfo.buildInfo.version, '6.0.3') ) { await this._instanceState.printDeprecationWarning( 'Starting in MongoDB 6.0.3, automatic chunk splitting is not performed. This is because of balancing policy improvements. Auto-splitting commands still exist, but do not perform an operation. For details, see Balancing Policy Changes: https://www.mongodb.com/docs/manual/release-notes/6.0/#balancing-policy-changes\n' diff --git a/packages/shell-api/src/shell-instance-state.spec.ts b/packages/shell-api/src/shell-instance-state.spec.ts index ece9398d8..b13b1cf57 100644 --- a/packages/shell-api/src/shell-instance-state.spec.ts +++ b/packages/shell-api/src/shell-instance-state.spec.ts @@ -22,6 +22,8 @@ describe('ShellInstanceState', function () { serviceProvider.bsonLibrary = bson; serviceProvider.getConnectionInfo.resolves({ extraInfo: { uri: 'mongodb://localhost/' }, + buildInfo: {}, + topology: null, }); evaluationListener = stubInterface(); instanceState = new ShellInstanceState(serviceProvider); @@ -41,7 +43,7 @@ describe('ShellInstanceState', function () { it('provides printing ability for shell API objects', async function () { await run('print(db)'); - expect(evaluationListener.onPrint.lastCall.args[0][0].type).to.equal( + expect(evaluationListener.onPrint?.lastCall.args[0][0].type).to.equal( 'Database' ); }); @@ -84,6 +86,8 @@ describe('ShellInstanceState', function () { const setupServiceProviderWithTopology = (topology: any) => { serviceProvider.getConnectionInfo.resolves({ extraInfo: { uri: 'mongodb://localhost/' }, + buildInfo: {}, + topology: null, }); serviceProvider.getTopology.returns(topology); }; @@ -94,6 +98,8 @@ describe('ShellInstanceState', function () { serviceProvider.bsonLibrary = bson; serviceProvider.getConnectionInfo.resolves({ extraInfo: { uri: 'mongodb://localhost/' }, + buildInfo: {}, + topology: null, }); instanceState = new ShellInstanceState( serviceProvider, @@ -114,6 +120,8 @@ describe('ShellInstanceState', function () { uri: 'mongodb://atlas-stream-65a5f1cd6d50457be377be7b-1dekw.virginia-usa.a.query.mongodb-dev.net/', is_stream: true, }, + buildInfo: {}, + topology: null, }); await instanceState.fetchConnectionInfo(); @@ -128,6 +136,8 @@ describe('ShellInstanceState', function () { uri: 'mongodb://localhost/', is_data_federation: true, }, + buildInfo: {}, + topology: null, }); await instanceState.fetchConnectionInfo(); @@ -143,6 +153,8 @@ describe('ShellInstanceState', function () { is_atlas: true, is_data_federation: true, }, + buildInfo: {}, + topology: null, }); await instanceState.fetchConnectionInfo(); @@ -158,6 +170,8 @@ describe('ShellInstanceState', function () { uri: 'mongodb://localhost/', is_atlas: true, }, + buildInfo: {}, + topology: null, }); await instanceState.fetchConnectionInfo(); @@ -172,6 +186,8 @@ describe('ShellInstanceState', function () { is_enterprise: true, is_atlas: true, }, + buildInfo: {}, + topology: null, }); await instanceState.fetchConnectionInfo(); @@ -185,6 +201,8 @@ describe('ShellInstanceState', function () { uri: 'mongodb://localhost/', is_local_atlas: true, }, + buildInfo: {}, + topology: null, }); await instanceState.fetchConnectionInfo(); @@ -197,6 +215,8 @@ describe('ShellInstanceState', function () { it('inferred from extraInfo', async function () { serviceProvider.getConnectionInfo.resolves({ extraInfo: { uri: 'mongodb://localhost/', is_enterprise: true }, + buildInfo: {}, + topology: null, }); await instanceState.fetchConnectionInfo(); @@ -208,6 +228,7 @@ describe('ShellInstanceState', function () { serviceProvider.getConnectionInfo.resolves({ extraInfo: { uri: 'mongodb://localhost/' }, buildInfo: { modules: ['other', 'enterprise'] }, + topology: null, }); await instanceState.fetchConnectionInfo(); @@ -345,6 +366,8 @@ describe('ShellInstanceState', function () { is_atlas: true, atlas_version: '20210330.0.0.1617063608', }, + buildInfo: {}, + topology: null, }); await instanceState.fetchConnectionInfo(); @@ -381,6 +404,8 @@ describe('ShellInstanceState', function () { is_atlas: true, atlas_version: '20210330.0.0.1617063608', }, + buildInfo: {}, + topology: null, }); await instanceState.fetchConnectionInfo(); diff --git a/packages/shell-api/src/shell-instance-state.ts b/packages/shell-api/src/shell-instance-state.ts index 3ec67224c..9f3d1b01f 100644 --- a/packages/shell-api/src/shell-instance-state.ts +++ b/packages/shell-api/src/shell-instance-state.ts @@ -1,7 +1,8 @@ import { CommonErrors, MongoshInvalidInputError } from '@mongosh/errors'; import type { AutoEncryptionOptions, - ConnectInfo, + ConnectionExtraInfo, + ConnectionInfo, ServerApi, ServiceProvider, TopologyDescription, @@ -50,7 +51,7 @@ export interface ShellCliOptions { export interface AutocompleteParameters { topology: () => Topologies; apiVersionInfo: () => Required | undefined; - connectionInfo: () => ConnectInfo | undefined; + connectionInfo: () => ConnectionExtraInfo | undefined; getCollectionCompletionsForCurrentDb: (collName: string) => Promise; getDatabaseCompletions: (dbName: string) => Promise; } @@ -142,7 +143,16 @@ export default class ShellInstanceState { public currentDb: Database; public messageBus: MongoshBus; public initialServiceProvider: ServiceProvider; // the initial service provider - public connectionInfo: any; + private connectionInfoCache: { + // Caching/lazy-loading functionality for the ServiceProvider's getConnectionInfo() + // return value. We store the ServiceProvider instance for which we are + // fetching/have fetched connection info to avoid duplicate fetches. + forSp: ServiceProvider; + // If fetching is in progress, this is a Promise, otherwise the resolved + // return value (or undefined if we have not fetched yet). + // Autocompletion makes use of the ability to access this purely synchronously. + info: Promise | ConnectionInfo | undefined; + }; public context: any; public mongos: Mongo[]; public shellApi: ShellApi; @@ -164,6 +174,7 @@ export default class ShellInstanceState { private plugins: ShellPlugin[] = [new TransformMongoErrorPlugin()]; private alreadyTransformedErrors = new WeakMap(); + private preFetchCollectionAndDatabaseNames = true; constructor( initialServiceProvider: ServiceProvider, @@ -180,7 +191,10 @@ export default class ShellInstanceState { } ); this.mongos = []; - this.connectionInfo = { buildInfo: {} }; + this.connectionInfoCache = { + forSp: this.initialServiceProvider, + info: undefined, + }; if (!cliOptions.nodb) { const mongo = new Mongo( this, @@ -202,19 +216,48 @@ export default class ShellInstanceState { this.evaluationListener = {}; } - async fetchConnectionInfo(): Promise { + async fetchConnectionInfo(): Promise { if (!this.cliOptions.nodb) { - this.connectionInfo = - await this.currentServiceProvider.getConnectionInfo(); + const serviceProvider = this.currentServiceProvider; + if ( + serviceProvider === this.connectionInfoCache.forSp && + this.connectionInfoCache.info + ) { + // Already fetched connection info for the current service provider. + return this.connectionInfoCache.info; + } + const connectionInfoPromise = serviceProvider.getConnectionInfo(); + this.connectionInfoCache = { + forSp: serviceProvider, + info: connectionInfoPromise, + }; + let connectionInfo: ConnectionInfo | undefined; + try { + connectionInfo = await connectionInfoPromise; + } finally { + if (this.connectionInfoCache.info === connectionInfoPromise) + this.connectionInfoCache.info = connectionInfo; + } + const apiVersionInfo = this.apiVersionInfo(); this.messageBus.emit('mongosh:connect', { - ...this.connectionInfo.extraInfo, + ...connectionInfo?.extraInfo, api_version: apiVersionInfo?.version, api_strict: apiVersionInfo?.strict, api_deprecation_errors: apiVersionInfo?.deprecationErrors, - uri: redactInfo(this.connectionInfo.extraInfo.uri), + uri: redactInfo(connectionInfo?.extraInfo?.uri), }); + return connectionInfo; } + return undefined; + } + + cachedConnectionInfo(): ConnectionInfo | undefined { + const connectionInfo = this.connectionInfoCache.info; + return ( + (connectionInfo && 'extraInfo' in connectionInfo && connectionInfo) || + undefined + ); } async close(force: boolean): Promise { @@ -223,6 +266,10 @@ export default class ShellInstanceState { } } + public setPreFetchCollectionAndDatabaseNames(value: boolean): void { + this.preFetchCollectionAndDatabaseNames = value; + } + public setDbFunc(newDb: any): Database { this.currentDb = newDb; this.context.rs = new ReplicaSet(this.currentDb); @@ -231,13 +278,19 @@ export default class ShellInstanceState { this.fetchConnectionInfo().catch((err) => this.messageBus.emit('mongosh:error', err, 'shell-api') ); - // Pre-fetch for autocompletion. - this.currentDb - ._getCollectionNamesForCompletion() - .catch((err) => this.messageBus.emit('mongosh:error', err, 'shell-api')); - this.currentDb._mongo - ._getDatabaseNamesForCompletion() - .catch((err) => this.messageBus.emit('mongosh:error', err, 'shell-api')); + if (this.preFetchCollectionAndDatabaseNames) { + // Pre-fetch for autocompletion. + this.currentDb + ._getCollectionNamesForCompletion() + .catch((err) => + this.messageBus.emit('mongosh:error', err, 'shell-api') + ); + this.currentDb._mongo + ._getDatabaseNamesForCompletion() + .catch((err) => + this.messageBus.emit('mongosh:error', err, 'shell-api') + ); + } this.currentCursor = null; return newDb; } @@ -310,7 +363,9 @@ export default class ShellInstanceState { get currentServiceProvider(): ServiceProvider { try { - return this.currentDb._mongo._serviceProvider; + return ( + this.currentDb._mongo._serviceProvider ?? this.initialServiceProvider + ); } catch (err: any) { if (err?.code === ShellApiErrors.NotConnected) { return this.initialServiceProvider; @@ -387,7 +442,7 @@ export default class ShellInstanceState { return this.apiVersionInfo(); }, connectionInfo: () => { - return this.connectionInfo.extraInfo; + return this.cachedConnectionInfo()?.extraInfo ?? undefined; }, getCollectionCompletionsForCurrentDb: async ( collName: string @@ -484,12 +539,13 @@ export default class ShellInstanceState { // eslint-disable-next-line @typescript-eslint/require-await async getDefaultPrompt(): Promise { - if (this.connectionInfo?.extraInfo?.is_stream) { + const connectionInfo = await this.fetchConnectionInfo(); + if (connectionInfo?.extraInfo?.is_stream) { return 'AtlasStreamProcessing> '; } - const prefix = this.getDefaultPromptPrefix(); - const topologyInfo = this.getTopologySpecificPrompt(); + const prefix = await this.getDefaultPromptPrefix(); + const topologyInfo = await this.getTopologySpecificPrompt(); let dbname = ''; try { dbname = this.currentDb.getName(); @@ -499,8 +555,9 @@ export default class ShellInstanceState { return `${[prefix, topologyInfo, dbname].filter(Boolean).join(' ')}> `; } - private getDefaultPromptPrefix(): string { - const extraConnectionInfo = this.connectionInfo?.extraInfo; + private async getDefaultPromptPrefix(): Promise { + const connectionInfo = await this.fetchConnectionInfo(); + const extraConnectionInfo = connectionInfo?.extraInfo; if (extraConnectionInfo?.is_data_federation) { return 'AtlasDataFederation'; } else if (extraConnectionInfo?.is_local_atlas) { @@ -509,14 +566,15 @@ export default class ShellInstanceState { return 'Atlas'; } else if ( extraConnectionInfo?.is_enterprise || - this.connectionInfo?.buildInfo?.modules?.indexOf('enterprise') >= 0 + connectionInfo?.buildInfo?.modules?.indexOf('enterprise') >= 0 ) { return 'Enterprise'; } return ''; } - private getTopologySpecificPrompt(): string { + private async getTopologySpecificPrompt(): Promise { + const connectionInfo = await this.fetchConnectionInfo(); // TODO: once a driver with NODE-3011 is available set type to TopologyDescription const description = this.currentServiceProvider.getTopology()?.description; if (!description) { @@ -541,7 +599,7 @@ export default class ShellInstanceState { serverTypePrompt = '[primary]'; break; case 'Sharded': - serverTypePrompt = this.connectionInfo?.extraInfo?.atlas_version + serverTypePrompt = connectionInfo?.extraInfo?.atlas_version ? '' : '[mongos]'; break; diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index b32268e40..9a8420c98 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -43,23 +43,23 @@ export interface ShowEvent { } export interface ConnectEvent { - is_atlas: boolean; - is_localhost: boolean; - is_do: boolean; - server_version: string; + is_atlas?: boolean; + is_localhost?: boolean; + is_do?: boolean; + server_version?: string; server_os?: string; server_arch?: string; - is_enterprise: boolean; + is_enterprise?: boolean; auth_type?: string; - is_data_federation: boolean; + is_data_federation?: boolean; dl_version?: string; - is_genuine: boolean; - non_genuine_server_name: string; + is_genuine?: boolean; + non_genuine_server_name?: string; api_version?: string; api_strict?: boolean; api_deprecation_errors?: boolean; - node_version: string; - uri: string; + node_version?: string; + uri?: string; } export interface ScriptLoadFileEvent { From 40d6d23e025ea17720694c01cf6a0212ffee757a Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 24 Apr 2024 13:48:38 +0200 Subject: [PATCH 2/3] fixup: tests --- packages/cli-repl/src/cli-repl.spec.ts | 34 +++++++++++++++++++++- packages/shell-api/src/integration.spec.ts | 4 ++- 2 files changed, 36 insertions(+), 2 deletions(-) diff --git a/packages/cli-repl/src/cli-repl.spec.ts b/packages/cli-repl/src/cli-repl.spec.ts index 06515d1f7..2660b9d4b 100644 --- a/packages/cli-repl/src/cli-repl.spec.ts +++ b/packages/cli-repl/src/cli-repl.spec.ts @@ -1519,6 +1519,38 @@ describe('CliRepl', function () { cliRepl, await testServer.connectionString() ); + expect( + requests + .flatMap((req) => + JSON.parse(req.body).batch.map((entry) => entry.event) + ) + .sort() + .filter(Boolean) + ).to.deep.equal(['API Call', 'Script Evaluated', 'Startup Time']); + expect(totalEventsTracked).to.equal(4); + }); + + it('includes an additional event for connection info if not in quiet mode', async function () { + cliReplOptions.shellCliOptions.eval = ['db.hello()']; + cliReplOptions.shellCliOptions.quiet = false; + cliRepl = new CliRepl(cliReplOptions); + await startWithExpectedImmediateExit( + cliRepl, + await testServer.connectionString() + ); + expect( + requests + .flatMap((req) => + JSON.parse(req.body).batch.map((entry) => entry.event) + ) + .sort() + .filter(Boolean) + ).to.deep.equal([ + 'API Call', + 'New Connection', + 'Script Evaluated', + 'Startup Time', + ]); expect(totalEventsTracked).to.equal(5); }); @@ -1532,7 +1564,7 @@ describe('CliRepl', function () { cliRepl, await testServer.connectionString() ); - expect(totalEventsTracked).to.equal(7); + expect(totalEventsTracked).to.equal(6); const apiEvents = requests .map((req) => diff --git a/packages/shell-api/src/integration.spec.ts b/packages/shell-api/src/integration.spec.ts index 038787489..8847ef9a3 100644 --- a/packages/shell-api/src/integration.spec.ts +++ b/packages/shell-api/src/integration.spec.ts @@ -2716,9 +2716,11 @@ describe('Shell API (integration)', function () { skipIfApiStrict(); it('returns information about the connection', async function () { - expect(instanceState.connectionInfo.buildInfo.version).to.equal( + const fetchedInfo = await instanceState.fetchConnectionInfo(); + expect(fetchedInfo.buildInfo.version).to.equal( await database.version() ); + expect(instanceState.cachedConnectionInfo()).to.equal(fetchedInfo); }); }); From 70b6007cc6a99e50d109d470864f3d9bb726ba51 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 24 Apr 2024 13:55:03 +0200 Subject: [PATCH 3/3] fixup: unconditionally call fetchConnections() on startup (but do not always wait for result) --- packages/cli-repl/src/cli-repl.spec.ts | 21 +------------------- packages/cli-repl/src/mongosh-repl.spec.ts | 23 +++++++++++++--------- packages/cli-repl/src/mongosh-repl.ts | 5 ++++- 3 files changed, 19 insertions(+), 30 deletions(-) diff --git a/packages/cli-repl/src/cli-repl.spec.ts b/packages/cli-repl/src/cli-repl.spec.ts index 2660b9d4b..e7e460f0d 100644 --- a/packages/cli-repl/src/cli-repl.spec.ts +++ b/packages/cli-repl/src/cli-repl.spec.ts @@ -1519,25 +1519,6 @@ describe('CliRepl', function () { cliRepl, await testServer.connectionString() ); - expect( - requests - .flatMap((req) => - JSON.parse(req.body).batch.map((entry) => entry.event) - ) - .sort() - .filter(Boolean) - ).to.deep.equal(['API Call', 'Script Evaluated', 'Startup Time']); - expect(totalEventsTracked).to.equal(4); - }); - - it('includes an additional event for connection info if not in quiet mode', async function () { - cliReplOptions.shellCliOptions.eval = ['db.hello()']; - cliReplOptions.shellCliOptions.quiet = false; - cliRepl = new CliRepl(cliReplOptions); - await startWithExpectedImmediateExit( - cliRepl, - await testServer.connectionString() - ); expect( requests .flatMap((req) => @@ -1564,7 +1545,7 @@ describe('CliRepl', function () { cliRepl, await testServer.connectionString() ); - expect(totalEventsTracked).to.equal(6); + expect(totalEventsTracked).to.equal(7); const apiEvents = requests .map((req) => diff --git a/packages/cli-repl/src/mongosh-repl.spec.ts b/packages/cli-repl/src/mongosh-repl.spec.ts index 8cae1f0aa..8c6947caa 100644 --- a/packages/cli-repl/src/mongosh-repl.spec.ts +++ b/packages/cli-repl/src/mongosh-repl.spec.ts @@ -1457,21 +1457,26 @@ describe('MongoshNodeRepl', function () { it('calls a number of service provider functions by default', async function () { await mongoshRepl.initialize(serviceProvider); const calledFunctions = calledServiceProviderFunctions(); - expect(calledFunctions).to.include.keys( + expect(Object.keys(calledFunctions).sort()).to.deep.equal([ 'getConnectionInfo', - 'runCommandWithCheck' - ); + 'getFleOptions', + 'getRawClient', + 'getURI', + 'runCommandWithCheck', + ]); }); - it('does not get connection info in --quiet no-REPL mode', async function () { + it('does not wait for getConnectionInfo in quiet plain-vm mode', async function () { mongoshRepl.shellCliOptions.quiet = true; mongoshRepl.shellCliOptions.jsContext = 'plain-vm'; + sp.getConnectionInfo.callsFake( + () => + new Promise(() => { + /* never resolve */ + }) + ); await mongoshRepl.initialize(serviceProvider); - const calledFunctions = calledServiceProviderFunctions(); - expect(Object.keys(calledFunctions).sort()).to.deep.equal([ - 'getFleOptions', - 'getURI', - ]); + expect(serviceProvider.getConnectionInfo).to.have.been.calledOnce; }); }); }); diff --git a/packages/cli-repl/src/mongosh-repl.ts b/packages/cli-repl/src/mongosh-repl.ts index 2d9a6d957..b9a126dbc 100644 --- a/packages/cli-repl/src/mongosh-repl.ts +++ b/packages/cli-repl/src/mongosh-repl.ts @@ -199,7 +199,10 @@ class MongoshNodeRepl implements EvaluationListener { // not-quiet mode -> We'll need it for the greeting message (and need it now) // REPL mode -> We'll want it for fast autocomplete (and need it soon-ish, but not now) instanceState.setPreFetchCollectionAndDatabaseNames(!usePlainVMContext); - if (!this.shellCliOptions.quiet || !usePlainVMContext) { + // `if` commented out because we currently still want the connection info for + // logging/telemetry but we may want to revisit that in the future: + // if (!this.shellCliOptions.quiet || !usePlainVMContext) + { const connectionInfoPromise = instanceState.fetchConnectionInfo(); connectionInfoPromise.catch(() => { // Ignore potential unhandled rejection warning