From aa1819bcc23ffc13505bfcbc55f6ad6ab782793e Mon Sep 17 00:00:00 2001 From: Himanshu Singh Date: Mon, 30 Jan 2023 13:19:30 +0100 Subject: [PATCH 1/6] db.currentOp() now works atop $currentOp operator --- packages/i18n/src/locales/en_US.ts | 2 +- packages/shell-api/src/database.spec.ts | 172 ++++++++++++++++++------ packages/shell-api/src/database.ts | 49 +++++-- 3 files changed, 172 insertions(+), 51 deletions(-) diff --git a/packages/i18n/src/locales/en_US.ts b/packages/i18n/src/locales/en_US.ts index f6c9065c5..695699343 100644 --- a/packages/i18n/src/locales/en_US.ts +++ b/packages/i18n/src/locales/en_US.ts @@ -1134,7 +1134,7 @@ const translations: Catalog = { }, currentOp: { link: 'https://docs.mongodb.com/manual/reference/method/db.currentOp', - description: 'Calls the currentOp command. Returns a document that contains information on in-progress operations for the database instance. The db.currentOp() method wraps the database command currentOp.', + description: 'Runs an aggregation using $currentOp operator. Returns an aggregated document that contains information on in-progress operations for the database instance. For further information, see $currentOp.', example: 'db.currentOp()' }, killOp: { diff --git a/packages/shell-api/src/database.spec.ts b/packages/shell-api/src/database.spec.ts index f1a7a3987..832a654e4 100644 --- a/packages/shell-api/src/database.spec.ts +++ b/packages/shell-api/src/database.spec.ts @@ -13,6 +13,7 @@ import { ServiceProvider, bson, ClientSession as ServiceProviderSession, + Document, } from '@mongosh/service-provider-core'; import ShellInstanceState from './shell-instance-state'; import crypto from 'crypto'; @@ -1571,64 +1572,153 @@ describe('Database', () => { }); describe('currentOp', () => { - it('calls serviceProvider.runCommandWithCheck on the database without options', async() => { - await database.currentOp(); + const currentOpStage = (args: Document = {}) => ({ + $currentOp: { + allUsers: false, + idleConnections: false, + truncateOps: false, + ...args + } + }); - expect(serviceProvider.runCommandWithCheck).to.have.been.calledWith( - ADMIN_DB, - { currentOp: 1 } - ); + const READ_PREFERENCE = { + '$readPreference': { + mode: 'primaryPreferred' + } + }; + + beforeEach(() => { + const tryNext = sinon.stub(); + tryNext.onCall(0).resolves({}); + tryNext.onCall(1).resolves(null); + serviceProvider.aggregateDb.returns({ tryNext } as any); }); - it('allows boolean parameter', async() => { - await database.currentOp(true); - expect(serviceProvider.runCommandWithCheck).to.have.been.calledWith( - ADMIN_DB, - { - currentOp: 1, - $all: true + context('when called with a falsy value', function() { + it('calls serviceProvider.aggregateDb with correct options', async function() { + await database.currentOp(); + await database.currentOp(false); + + for (const args of serviceProvider.aggregateDb.args) { + expect(args[0]).to.equal('admin'); + const [stageCurrentOp] = args[1]; + expect(stageCurrentOp).to.deep.equal(currentOpStage({ + allUsers: true, + idleConnections: false + })); + expect(serviceProvider.aggregateDb.firstCall.args[2]).to.deep.equal(READ_PREFERENCE); } - ); + }); }); - it('allows boolean parameter false', async() => { - await database.currentOp(false); - expect(serviceProvider.runCommandWithCheck).to.have.been.calledWith( - ADMIN_DB, - { - $all: false, - currentOp: 1 - } - ); + context('when called with a boolean - true', function() { + it('calls serviceProvider.aggregateDb with correct options', async function() { + await database.currentOp(true); + expect(serviceProvider.aggregateDb).to.have.been.calledOnce; + expect(serviceProvider.aggregateDb.firstCall.args[0]).to.equal('admin'); + const [stageCurrentOp] = serviceProvider.aggregateDb.firstCall.args[1]; + expect(stageCurrentOp).to.deep.equal(currentOpStage({ + allUsers: true, + idleConnections: true + })); + expect(serviceProvider.aggregateDb.firstCall.args[2]).to.deep.equal(READ_PREFERENCE); + }); }); - it('calls serviceProvider.runCommandWithCheck on the database with options', async() => { - await database.currentOp({ - $ownOps: true, - $all: true, - filter: 1 + + context('when called with options containing $all', function() { + it('calls serviceProvider.aggregateDb with correct options', async function() { + await database.currentOp({ $all: true }); + expect(serviceProvider.aggregateDb).to.have.been.calledOnce; + expect(serviceProvider.aggregateDb.firstCall.args[0]).to.equal('admin'); + const [stageCurrentOp, matchStage] = serviceProvider.aggregateDb.firstCall.args[1]; + expect(stageCurrentOp).to.deep.equal(currentOpStage({ + allUsers: true, + idleConnections: true + })); + expect(matchStage).to.deep.equals({ $match: {} }); + expect(serviceProvider.aggregateDb.firstCall.args[2]).to.deep.equal(READ_PREFERENCE); }); + }); - expect(serviceProvider.runCommandWithCheck).to.have.been.calledWith( - ADMIN_DB, - { - currentOp: 1, - $ownOps: true, + context('when called with options containing $ownOps', function() { + it('calls serviceProvider.aggregateDb with correct options', async function() { + await database.currentOp({ $ownOps: true }); + expect(serviceProvider.aggregateDb).to.have.been.calledOnce; + expect(serviceProvider.aggregateDb.firstCall.args[0]).to.equal('admin'); + const [stageCurrentOp, matchStage] = serviceProvider.aggregateDb.firstCall.args[1]; + expect(stageCurrentOp).to.deep.equal(currentOpStage({ + allUsers: false, + idleConnections: false + })); + expect(matchStage).to.deep.equals({ $match: {} }); + expect(serviceProvider.aggregateDb.firstCall.args[2]).to.deep.equal(READ_PREFERENCE); + }); + }); + + context('when called with options containing both $ownOps and $all', function() { + it('calls serviceProvider.aggregateDb with correct options', async function() { + await database.currentOp({ $all: true, $ownOps: true }); + expect(serviceProvider.aggregateDb).to.have.been.calledOnce; + expect(serviceProvider.aggregateDb.firstCall.args[0]).to.equal('admin'); + const [stageCurrentOp, matchStage] = serviceProvider.aggregateDb.firstCall.args[1]; + expect(stageCurrentOp).to.deep.equal(currentOpStage({ + allUsers: false, + idleConnections: true + })); + expect(matchStage).to.deep.equals({ $match: {} }); + expect(serviceProvider.aggregateDb.firstCall.args[2]).to.deep.equal(READ_PREFERENCE); + }); + }); + + context('when called with options containing filter conditions', function() { + it('calls serviceProvider.aggregateDb with correct options', async function() { + await database.currentOp({ $all: true, - filter: 1 - } - ); + waitingForLock: true + }); + + expect(serviceProvider.aggregateDb).to.have.been.calledOnce; + expect(serviceProvider.aggregateDb.firstCall.args[0]).to.equal('admin'); + const [stageCurrentOp, matchStage] = serviceProvider.aggregateDb.firstCall.args[1]; + expect(stageCurrentOp).to.deep.equal(currentOpStage({ + allUsers: true, + idleConnections: true + })); + expect(matchStage).to.deep.equals({ $match: { waitingForLock: true } }); + expect(serviceProvider.aggregateDb.firstCall.args[2]).to.deep.equal(READ_PREFERENCE); + }); + + it('ensures that $ownOps, $all and $truncateOps are never passed down as filters', async function() { + await database.currentOp({ + $all: true, + $ownOps: false, + $truncateOps: false, + waitingForLock: true + }); + + const [, matchStage] = serviceProvider.aggregateDb.firstCall.args[1]; + expect(matchStage).to.deep.equals({ $match: { waitingForLock: true } }); + }); }); - it('returns whatever serviceProvider.runCommandWithCheck returns', async() => { - const expectedResult = { ok: 1 }; - serviceProvider.runCommandWithCheck.resolves(expectedResult); + it('returns the result of serviceProvider.aggregateDb wrapped in an interface', async() => { + const expectedResult = { ok: 1, inprog: [] }; + + const tryNext = sinon.stub(); + tryNext.onCall(0).resolves(); + tryNext.onCall(1).resolves(null); + serviceProvider.aggregateDb.returns({ tryNext } as any); + const result = await database.currentOp(); expect(result).to.deep.equal(expectedResult); }); - it('throws if serviceProvider.runCommandWithCheck rejects', async() => { + it('throws if serviceProvider.aggregateDb rejects', async() => { const expectedError = new Error(); - serviceProvider.runCommandWithCheck.rejects(expectedError); + + const tryNext = sinon.stub(); + tryNext.onCall(0).rejects(expectedError); + serviceProvider.aggregateDb.returns({ tryNext } as any); const caughtError = await database.currentOp() .catch(e => e); expect(caughtError).to.equal(expectedError); diff --git a/packages/shell-api/src/database.ts b/packages/shell-api/src/database.ts index 271bc5ee2..34756b778 100644 --- a/packages/shell-api/src/database.ts +++ b/packages/shell-api/src/database.ts @@ -809,19 +809,50 @@ export default class Database extends ShellApiWithMongoClass { ); } + async _getCurrentOperations(opts: Document | boolean) { + const legacyCurrentOpOptions = typeof opts === 'boolean' + ? ({ '$all': opts, '$ownOps': false }) + : ({ '$all': !!opts.$all, '$ownOps': !!opts.$ownOps }); + + const pipeline: Document[] = [{ + $currentOp: { + 'allUsers': !legacyCurrentOpOptions.$ownOps, + 'idleConnections': legacyCurrentOpOptions.$all, + 'truncateOps': false, + } + }]; + + if (typeof opts === 'object') { + const matchingFilters: Document = {}; + for (const filtername in opts) { + if (Object.prototype.hasOwnProperty.call(opts, filtername)) { + if (filtername !== '$ownOps' && filtername !== '$all' && filtername !== '$truncateOps') { + matchingFilters[filtername] = opts[filtername]; + } + } + } + + pipeline.push({ $match: matchingFilters }); + } + + return (await this + .getSiblingDB('admin') + .aggregate(pipeline, { + '$readPreference': { + 'mode': 'primaryPreferred' + } + })).toArray(); + } + @returnsPromise @apiVersions([]) async currentOp(opts: Document | boolean = {}): Promise { this._emitDatabaseApiCall('currentOp', { opts: opts }); - if (typeof opts === 'boolean') { - opts = { $all: opts }; - } - return await this._runAdminCommand( - { - currentOp: 1, - ...opts - } - ); + const currentOps = await this._getCurrentOperations(opts); + return { + inprog: currentOps.length > 0 ? currentOps : [], + ok: 1 + }; } @returnsPromise From ff81508f867214c61ff1483405c6ae10fea3b0fd Mon Sep 17 00:00:00 2001 From: Himanshu Singh Date: Mon, 30 Jan 2023 14:17:35 +0100 Subject: [PATCH 2/6] Update packages/shell-api/src/database.ts Co-authored-by: Anna Henningsen --- packages/shell-api/src/database.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/shell-api/src/database.ts b/packages/shell-api/src/database.ts index 34756b778..d428bd3b5 100644 --- a/packages/shell-api/src/database.ts +++ b/packages/shell-api/src/database.ts @@ -850,7 +850,7 @@ export default class Database extends ShellApiWithMongoClass { this._emitDatabaseApiCall('currentOp', { opts: opts }); const currentOps = await this._getCurrentOperations(opts); return { - inprog: currentOps.length > 0 ? currentOps : [], + inprog: currentOps, ok: 1 }; } From 62a263787ab8132bc46c78213766ad32daca1f8f Mon Sep 17 00:00:00 2001 From: Himanshu Singh Date: Mon, 30 Jan 2023 14:18:16 +0100 Subject: [PATCH 3/6] Update packages/shell-api/src/database.ts Co-authored-by: Anna Henningsen --- packages/shell-api/src/database.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/shell-api/src/database.ts b/packages/shell-api/src/database.ts index d428bd3b5..25bb927c2 100644 --- a/packages/shell-api/src/database.ts +++ b/packages/shell-api/src/database.ts @@ -824,8 +824,7 @@ export default class Database extends ShellApiWithMongoClass { if (typeof opts === 'object') { const matchingFilters: Document = {}; - for (const filtername in opts) { - if (Object.prototype.hasOwnProperty.call(opts, filtername)) { + for (const filtername of Object.keys(opts)) { if (filtername !== '$ownOps' && filtername !== '$all' && filtername !== '$truncateOps') { matchingFilters[filtername] = opts[filtername]; } From 2964ddd5f215cf56fdb73fd2c6e91de6479377e4 Mon Sep 17 00:00:00 2001 From: Himanshu Singh Date: Mon, 30 Jan 2023 14:18:31 +0100 Subject: [PATCH 4/6] Update packages/shell-api/src/database.ts Co-authored-by: Anna Henningsen --- packages/shell-api/src/database.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/shell-api/src/database.ts b/packages/shell-api/src/database.ts index 25bb927c2..54e634267 100644 --- a/packages/shell-api/src/database.ts +++ b/packages/shell-api/src/database.ts @@ -809,7 +809,7 @@ export default class Database extends ShellApiWithMongoClass { ); } - async _getCurrentOperations(opts: Document | boolean) { + async _getCurrentOperations(opts: Document | boolean): Promise { const legacyCurrentOpOptions = typeof opts === 'boolean' ? ({ '$all': opts, '$ownOps': false }) : ({ '$all': !!opts.$all, '$ownOps': !!opts.$ownOps }); From 23652f423c58e1b14201348cca9e44d1bcdd0cb6 Mon Sep 17 00:00:00 2001 From: Himanshu Singh Date: Mon, 30 Jan 2023 14:19:54 +0100 Subject: [PATCH 5/6] implements feedback from PR review --- packages/i18n/src/locales/en_US.ts | 2 +- packages/shell-api/src/database.ts | 5 ++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/packages/i18n/src/locales/en_US.ts b/packages/i18n/src/locales/en_US.ts index 695699343..460c47224 100644 --- a/packages/i18n/src/locales/en_US.ts +++ b/packages/i18n/src/locales/en_US.ts @@ -1134,7 +1134,7 @@ const translations: Catalog = { }, currentOp: { link: 'https://docs.mongodb.com/manual/reference/method/db.currentOp', - description: 'Runs an aggregation using $currentOp operator. Returns an aggregated document that contains information on in-progress operations for the database instance. For further information, see $currentOp.', + description: 'Runs an aggregation using $currentOp operator. Returns a document that contains information on in-progress operations for the database instance. For further information, see $currentOp.', example: 'db.currentOp()' }, killOp: { diff --git a/packages/shell-api/src/database.ts b/packages/shell-api/src/database.ts index 54e634267..e9a05cde8 100644 --- a/packages/shell-api/src/database.ts +++ b/packages/shell-api/src/database.ts @@ -825,9 +825,8 @@ export default class Database extends ShellApiWithMongoClass { if (typeof opts === 'object') { const matchingFilters: Document = {}; for (const filtername of Object.keys(opts)) { - if (filtername !== '$ownOps' && filtername !== '$all' && filtername !== '$truncateOps') { - matchingFilters[filtername] = opts[filtername]; - } + if (filtername !== '$ownOps' && filtername !== '$all' && filtername !== '$truncateOps') { + matchingFilters[filtername] = opts[filtername]; } } From 07fbbb778e4551444e777e29d957200ad82639f5 Mon Sep 17 00:00:00 2001 From: Himanshu Singh Date: Mon, 30 Jan 2023 15:56:01 +0100 Subject: [PATCH 6/6] fixes e2e tests - currentOp does not use runCommandWithCheck any more and hence pushed to exceptions list --- packages/shell-api/src/database.spec.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/shell-api/src/database.spec.ts b/packages/shell-api/src/database.spec.ts index 832a654e4..51cb74d95 100644 --- a/packages/shell-api/src/database.spec.ts +++ b/packages/shell-api/src/database.spec.ts @@ -2730,6 +2730,7 @@ describe('Database', () => { const exceptions = { getCollectionNames: { m: 'listCollections' }, getCollectionInfos: { m: 'listCollections' }, + currentOp: { m: 'aggregateDb', a: [[]] }, aggregate: { m: 'aggregateDb', a: [[]] }, dropDatabase: { m: 'dropDatabase', i: 1 }, createCollection: { m: 'createCollection', a: ['coll'] }, @@ -2776,6 +2777,7 @@ describe('Database', () => { serviceProvider.runCommand.resolves({ ok: 1 }); serviceProvider.listCollections.resolves([]); serviceProvider.watch.returns({ closed: false, tryNext: async() => {} } as any); + serviceProvider.aggregateDb.returns({ tryNext: async() => {} } as any); const instanceState = new ShellInstanceState(serviceProvider, bus); const mongo = new Mongo(instanceState, undefined, undefined, undefined, serviceProvider); const session = mongo.startSession();