From a219aff909ffbb8701e4fcf97bc746124a0255bc Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 7 Jun 2021 17:22:08 +0200 Subject: [PATCH 1/4] fix(shell-api): use hello instead of isMaster for internal calls MONGOSH-816 Otherwise commands like `sh.status()` unnecessarily fail when run under the versioned API. --- packages/shell-api/src/database.spec.ts | 40 +++++++++++----------- packages/shell-api/src/database.ts | 10 +++--- packages/shell-api/src/helpers.ts | 4 +-- packages/shell-api/src/replica-set.spec.ts | 27 +++++++++++++++ packages/shell-api/src/replica-set.ts | 12 ++++--- packages/shell-api/src/shard.spec.ts | 18 ++++++++++ 6 files changed, 79 insertions(+), 32 deletions(-) diff --git a/packages/shell-api/src/database.spec.ts b/packages/shell-api/src/database.spec.ts index d2246881b0..a7facb4d83 100644 --- a/packages/shell-api/src/database.spec.ts +++ b/packages/shell-api/src/database.spec.ts @@ -1988,7 +1988,7 @@ describe('Database', () => { describe('enableFreeMonitoring', () => { it('throws if serviceProvider isMaster is false', async() => { - serviceProvider.runCommand.resolves({ ismaster: false }); + serviceProvider.runCommandWithCheck.resolves({ isWritablePrimary: false }); const catchedError = await database.enableFreeMonitoring() .catch(e => e); expect(catchedError).to.be.instanceOf(MongoshInvalidInputError); @@ -1996,8 +1996,8 @@ describe('Database', () => { }); it('calls serviceProvider.runCommand on the database', async() => { - serviceProvider.runCommand.onCall(0).resolves({ ismaster: true }); - serviceProvider.runCommand.onCall(1).resolves({ ok: 1 }); + serviceProvider.runCommandWithCheck.resolves({ isWritablePrimary: true }); + serviceProvider.runCommand.onCall(0).resolves({ ok: 1 }); await database.enableFreeMonitoring(); expect(serviceProvider.runCommand).to.have.been.calledWith( @@ -2012,25 +2012,25 @@ describe('Database', () => { it('returns whatever serviceProvider.runCommand returns if enabled', async() => { const expectedFMState = { ok: 1, state: 'enabled' }; - serviceProvider.runCommand.onCall(0).resolves({ ismaster: true }); - serviceProvider.runCommand.onCall(1).resolves({ ok: 1 }); - serviceProvider.runCommand.onCall(2).resolves(expectedFMState); + serviceProvider.runCommandWithCheck.resolves({ isWritablePrimary: true }); + serviceProvider.runCommand.onCall(0).resolves({ ok: 1 }); + serviceProvider.runCommand.onCall(1).resolves(expectedFMState); const result = await database.enableFreeMonitoring(); expect(result).to.deep.equal(expectedFMState); }); it('returns warning if not enabled', async() => { - serviceProvider.runCommand.onCall(0).resolves({ ismaster: true }); - serviceProvider.runCommand.onCall(1).resolves({ ok: 1 }); - serviceProvider.runCommand.onCall(2).resolves({ ok: 1, enabled: false }); - serviceProvider.runCommand.onCall(3).resolves({ cloudFreeMonitoringEndpointURL: 'URL' }); + serviceProvider.runCommandWithCheck.resolves({ isWritablePrimary: true }); + serviceProvider.runCommand.onCall(0).resolves({ ok: 1 }); + serviceProvider.runCommand.onCall(1).resolves({ ok: 1, enabled: false }); + serviceProvider.runCommand.onCall(2).resolves({ cloudFreeMonitoringEndpointURL: 'URL' }); const result = await database.enableFreeMonitoring(); expect(result).to.include('URL'); }); it('returns warning if returns ok: 0 with auth error', async() => { - serviceProvider.runCommand.onCall(0).resolves({ ismaster: true }); - serviceProvider.runCommand.onCall(1).resolves({ ok: 1 }); - serviceProvider.runCommand.onCall(2).resolves({ ok: 0, codeName: 'Unauthorized' }); + serviceProvider.runCommandWithCheck.resolves({ isWritablePrimary: true }); + serviceProvider.runCommand.onCall(0).resolves({ ok: 1 }); + serviceProvider.runCommand.onCall(1).resolves({ ok: 0, codeName: 'Unauthorized' }); const result = await database.enableFreeMonitoring(); expect(result).to.be.a('string'); expect(result).to.include('privilege'); @@ -2038,18 +2038,18 @@ describe('Database', () => { it('returns warning if throws with auth error', async() => { const expectedError = new Error(); (expectedError as any).codeName = 'Unauthorized'; - serviceProvider.runCommand.onCall(0).resolves({ ismaster: true }); - serviceProvider.runCommand.onCall(1).resolves({ ok: 1 }); - serviceProvider.runCommand.onCall(2).rejects(expectedError); + serviceProvider.runCommandWithCheck.resolves({ isWritablePrimary: true }); + serviceProvider.runCommand.onCall(0).resolves({ ok: 1 }); + serviceProvider.runCommand.onCall(1).rejects(expectedError); const result = await database.enableFreeMonitoring(); expect(result).to.be.a('string'); expect(result).to.include('privilege'); }); it('throws if throws with non-auth error', async() => { - serviceProvider.runCommand.onCall(0).resolves({ ismaster: true }); - serviceProvider.runCommand.onCall(1).resolves({ ok: 1 }); - serviceProvider.runCommand.onCall(2).rejects(new Error()); + serviceProvider.runCommandWithCheck.resolves({ isWritablePrimary: true }); + serviceProvider.runCommand.onCall(0).resolves({ ok: 1 }); + serviceProvider.runCommand.onCall(1).rejects(new Error()); const error = await database.enableFreeMonitoring().catch(e => e); expect(error).to.be.instanceOf(MongoshRuntimeError); @@ -2058,7 +2058,7 @@ describe('Database', () => { it('throws if serviceProvider.runCommand rejects without auth error', async() => { const expectedError = new Error(); - serviceProvider.runCommand.rejects(expectedError); + serviceProvider.runCommandWithCheck.rejects(expectedError); const catchedError = await database.enableFreeMonitoring() .catch(e => e); expect(catchedError).to.equal(expectedError); diff --git a/packages/shell-api/src/database.ts b/packages/shell-api/src/database.ts index b7059dd8b7..4c7722d24e 100644 --- a/packages/shell-api/src/database.ts +++ b/packages/shell-api/src/database.ts @@ -968,8 +968,8 @@ export default class Database extends ShellApiWithMongoClass { @returnsPromise async enableFreeMonitoring(): Promise { this._emitDatabaseApiCall('enableFreeMonitoring', {}); - const isMaster = await this._mongo._serviceProvider.runCommand(this._name, { isMaster: 1 }, this._baseOptions); - if (!isMaster.ismaster) { + const helloResult = await this.hello(); + if (!helloResult.isWritablePrimary) { throw new MongoshInvalidInputError( 'db.enableFreeMonitoring() may only be run on a primary', CommonErrors.InvalidOperation @@ -1312,10 +1312,10 @@ export default class Database extends ShellApiWithMongoClass { try { replInfo = await this.getReplicationInfo(); } catch (error) { - const isMaster = await this._runCommand({ isMaster: 1 }); - if (isMaster.arbiterOnly) { + const helloResult = await this.hello(); + if (helloResult.arbiterOnly) { return new CommandResult('StatsResult', { message: 'cannot provide replication status from an arbiter' }); - } else if (!isMaster.ismaster) { + } else if (!helloResult.isWritablePrimary) { const secondaryInfo = await this.printSecondaryReplicationInfo(); return new CommandResult('StatsResult', { message: 'this is a secondary, printing secondary replication info.', diff --git a/packages/shell-api/src/helpers.ts b/packages/shell-api/src/helpers.ts index a035446794..286cf65a9b 100644 --- a/packages/shell-api/src/helpers.ts +++ b/packages/shell-api/src/helpers.ts @@ -487,8 +487,8 @@ export async function getPrintableShardStatus(db: Database, verbose: boolean): P } export async function getConfigDB(db: Database): Promise { - const isM = await db._runAdminCommand({ isMaster: 1 }); - if (isM.msg !== 'isdbgrid') { + const helloResult = await db.hello(); + if (helloResult.msg !== 'isdbgrid') { throw new MongoshInvalidInputError('Not connected to a mongos', ShellApiErrors.NotConnectedToMongos); } return db.getSiblingDB('config'); diff --git a/packages/shell-api/src/replica-set.spec.ts b/packages/shell-api/src/replica-set.spec.ts index 68b842d608..41a7afe138 100644 --- a/packages/shell-api/src/replica-set.spec.ts +++ b/packages/shell-api/src/replica-set.spec.ts @@ -355,6 +355,33 @@ describe('ReplicaSet', () => { expect(catchedError).to.equal(expectedError); }); }); + describe('hello', () => { + it('calls serviceProvider.runCommandWithCheck', async() => { + await rs.hello(); + + expect(serviceProvider.runCommandWithCheck).to.have.been.calledWith( + ADMIN_DB, + { + hello: 1 + } + ); + }); + + it('returns whatever serviceProvider.runCommandWithCheck returns', async() => { + const expectedResult = { ok: 1 }; + serviceProvider.runCommandWithCheck.resolves(expectedResult); + const result = await rs.hello(); + expect(result).to.deep.equal(expectedResult); + }); + + it('throws if serviceProvider.runCommandWithCheck rejects', async() => { + const expectedError = new Error(); + serviceProvider.runCommandWithCheck.rejects(expectedError); + const catchedError = await rs.hello() + .catch(e => e); + expect(catchedError).to.equal(expectedError); + }); + }); describe('add', () => { it('calls serviceProvider.runCommandWithCheck with no arb and string hostport', async() => { const configDoc = { version: 1, members: [{ _id: 0 }, { _id: 1 }] }; diff --git a/packages/shell-api/src/replica-set.ts b/packages/shell-api/src/replica-set.ts index 1e88e887c4..153969efe9 100644 --- a/packages/shell-api/src/replica-set.ts +++ b/packages/shell-api/src/replica-set.ts @@ -226,11 +226,13 @@ export default class ReplicaSet extends ShellApiWithMongoClass { @returnsPromise async isMaster(): Promise { this._emitReplicaSetApiCall('isMaster', {}); - return this._database._runAdminCommand( - { - isMaster: 1, - } - ); + return this._database.getSiblingDB('admin').isMaster(); + } + + @returnsPromise + async hello(): Promise { + this._emitReplicaSetApiCall('hello', {}); + return this._database.getSiblingDB('admin').hello(); } @returnsPromise diff --git a/packages/shell-api/src/shard.spec.ts b/packages/shell-api/src/shard.spec.ts index c670826688..ae966f5347 100644 --- a/packages/shell-api/src/shard.spec.ts +++ b/packages/shell-api/src/shard.spec.ts @@ -1240,6 +1240,24 @@ describe('Shard', () => { Object.keys(result.value).includes('active mongoses') || Object.keys(result.value).includes('most recently active mongoses')).to.be.true; }); + context('with 5.0+ server', () => { + skipIfServerVersion(mongos, '<= 4.4'); + + it('returns the status when used with apiStrict', async() => { + await serviceProvider.close(true); + serviceProvider = await CliServiceProvider.connect(await mongos.connectionString(), { + serverApi: { version: '1', strict: true } + }); + internalState = new ShellInternalState(serviceProvider); + sh = new Shard(internalState.currentDb); + + const result = await sh.status(); + expect(result.type).to.equal('StatsResult'); + expect(Object.keys(result.value)).to.include.members([ + 'shardingVersion', 'shards', 'autosplit', 'balancer', 'databases' + ]); + }); + }); }); describe('turn on sharding', () => { it('enableSharding for a db', async() => { From 4045aa405713d7da7e4955eb01e757acf79224c6 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 7 Jun 2021 19:15:37 +0200 Subject: [PATCH 2/4] fixup: i18n --- packages/i18n/src/locales/en_US.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/packages/i18n/src/locales/en_US.ts b/packages/i18n/src/locales/en_US.ts index 0d32eea8fd..bcc12c54f7 100644 --- a/packages/i18n/src/locales/en_US.ts +++ b/packages/i18n/src/locales/en_US.ts @@ -1420,6 +1420,11 @@ const translations: Catalog = { description: 'Calls isMaster', example: 'rs.isMaster()', }, + hello: { + link: 'https://docs.mongodb.com/manual/reference/method/rs.hello', + description: 'Calls hello', + example: 'rs.hello()', + }, printSecondaryReplicationInfo: { link: 'https://docs.mongodb.com/manual/reference/method/rs.printSecondaryReplicationInfo', description: 'Calls db.printSecondaryReplicationInfo', From f7d9b3608bc89732fc9564149f28426199a9d4d0 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 8 Jun 2021 11:15:35 +0200 Subject: [PATCH 3/4] fixup: test mock command uses hello --- packages/shell-api/src/helpers.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/shell-api/src/helpers.spec.ts b/packages/shell-api/src/helpers.spec.ts index ae10b1b920..4ea0c58d2a 100644 --- a/packages/shell-api/src/helpers.spec.ts +++ b/packages/shell-api/src/helpers.spec.ts @@ -96,7 +96,7 @@ describe('getPrintableShardStatus', () => { const origRunCommandWithCheck = serviceProvider.runCommandWithCheck; serviceProvider.runCommandWithCheck = async(db, cmd) => { - if (db === 'admin' && cmd.isMaster) { + if (cmd.hello) { return { ok: 1, msg: 'isdbgrid' }; } if (db === 'admin' && cmd.balancerStatus) { From 335c3fc5221f21f14075c613391c62ad5f03456b Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 8 Jun 2021 15:49:10 +0200 Subject: [PATCH 4/4] fixup: use separate service provider for apiStrict in test --- packages/shell-api/src/shard.spec.ts | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/packages/shell-api/src/shard.spec.ts b/packages/shell-api/src/shard.spec.ts index ae966f5347..93b8ab0be1 100644 --- a/packages/shell-api/src/shard.spec.ts +++ b/packages/shell-api/src/shard.spec.ts @@ -1242,14 +1242,21 @@ describe('Shard', () => { }); context('with 5.0+ server', () => { skipIfServerVersion(mongos, '<= 4.4'); + let apiStrictServiceProvider; - it('returns the status when used with apiStrict', async() => { - await serviceProvider.close(true); - serviceProvider = await CliServiceProvider.connect(await mongos.connectionString(), { + before(async() => { + apiStrictServiceProvider = await CliServiceProvider.connect(await mongos.connectionString(), { serverApi: { version: '1', strict: true } }); - internalState = new ShellInternalState(serviceProvider); - sh = new Shard(internalState.currentDb); + }); + + after(async() => { + await apiStrictServiceProvider.close(true); + }); + + it('returns the status when used with apiStrict', async() => { + const internalState = new ShellInternalState(apiStrictServiceProvider); + const sh = new Shard(internalState.currentDb); const result = await sh.status(); expect(result.type).to.equal('StatsResult');