From e5d43667cb539de5975bbdf70413eb7e7399f28d Mon Sep 17 00:00:00 2001 From: Warren James Date: Mon, 7 Jun 2021 14:50:38 -0400 Subject: [PATCH 01/16] stared fix for NODE-1502 --- src/cmap/connection.ts | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/src/cmap/connection.ts b/src/cmap/connection.ts index f0b8a434968..e6b2fcaf5a6 100644 --- a/src/cmap/connection.ts +++ b/src/cmap/connection.ts @@ -756,6 +756,25 @@ function streamIdentifier(stream: Stream) { return uuidV4().toString('hex'); } +// TODO: ensure this does a deep copy +function cloneCommand(command: WriteProtocolMessageType): WriteProtocolMessageType { + let clonedCommand; + if (command === null || command === undefined) { + clonedCommand = command; + } else if ( + command instanceof Query || + command instanceof Msg || + command instanceof GetMore || + command instanceof KillCursor + ) { + clonedCommand = { ...command }; + } else { + throw new Error(`Unrecognized command: ${command}`); + } + + return clonedCommand as WriteProtocolMessageType; +} + function write( conn: Connection, command: WriteProtocolMessageType, @@ -765,6 +784,7 @@ function write( if (typeof options === 'function') { callback = options; } + const clonedCommand = cloneCommand(command); options = options ?? {}; const operationDescription: OperationDescription = { @@ -799,25 +819,25 @@ function write( // if command monitoring is enabled we need to modify the callback here if (conn.monitorCommands) { - conn.emit(Connection.COMMAND_STARTED, new CommandStartedEvent(conn, command)); + conn.emit(Connection.COMMAND_STARTED, new CommandStartedEvent(conn, clonedCommand)); operationDescription.started = now(); operationDescription.cb = (err, reply) => { if (err) { conn.emit( Connection.COMMAND_FAILED, - new CommandFailedEvent(conn, command, err, operationDescription.started) + new CommandFailedEvent(conn, clonedCommand, err, operationDescription.started) ); } else { if (reply && (reply.ok === 0 || reply.$err)) { conn.emit( Connection.COMMAND_FAILED, - new CommandFailedEvent(conn, command, reply, operationDescription.started) + new CommandFailedEvent(conn, clonedCommand, reply, operationDescription.started) ); } else { conn.emit( Connection.COMMAND_SUCCEEDED, - new CommandSucceededEvent(conn, command, reply, operationDescription.started) + new CommandSucceededEvent(conn, clonedCommand, reply, operationDescription.started) ); } } From 5addcc6cede1954e7dfe1a2baf9be5aafe5b8d31 Mon Sep 17 00:00:00 2001 From: Warren James Date: Mon, 7 Jun 2021 15:39:03 -0400 Subject: [PATCH 02/16] finished NODE-1502 --- src/cmap/connection.ts | 37 ++++++++++++++++++++++++++----------- 1 file changed, 26 insertions(+), 11 deletions(-) diff --git a/src/cmap/connection.ts b/src/cmap/connection.ts index e6b2fcaf5a6..2ca27341dac 100644 --- a/src/cmap/connection.ts +++ b/src/cmap/connection.ts @@ -14,7 +14,8 @@ import { Callback, MongoDBNamespace, maxWireVersion, - HostAddress + HostAddress, + deepCopy } from '../utils'; import { AnyError, @@ -756,18 +757,32 @@ function streamIdentifier(stream: Stream) { return uuidV4().toString('hex'); } -// TODO: ensure this does a deep copy +// FIXME: ensure this does a deep copy function cloneCommand(command: WriteProtocolMessageType): WriteProtocolMessageType { - let clonedCommand; + let clonedCommand: WriteProtocolMessageType; if (command === null || command === undefined) { clonedCommand = command; - } else if ( - command instanceof Query || - command instanceof Msg || - command instanceof GetMore || - command instanceof KillCursor - ) { - clonedCommand = { ...command }; + } else if (command instanceof Query) { + clonedCommand = { ...command }; + clonedCommand.query = { ...command.query }; + clonedCommand.returnFieldSelector = { ...command.returnFieldSelector }; + } else if (command instanceof Msg) { + clonedCommand = { ...command }; + clonedCommand.command = { ...command.command }; + clonedCommand.options = { ...command.options }; + } else if (command instanceof GetMore) { + clonedCommand = { ...command }; + // FIXME: get this deep copy to work + clonedCommand.cursorId = Long.fromNumber(command.cursorId.toNumber()); + } else if (command instanceof KillCursor) { + //clonedCommand = _clone(command); + clonedCommand = { ...command }; + // FIXME: get this deep copy to work + const cursorIds: Long[] = []; + command.cursorIds.forEach(id => { + cursorIds.push(id); + }); + clonedCommand.cursorIds = cursorIds; } else { throw new Error(`Unrecognized command: ${command}`); } @@ -784,7 +799,7 @@ function write( if (typeof options === 'function') { callback = options; } - const clonedCommand = cloneCommand(command); + const clonedCommand = deepCopy(command); options = options ?? {}; const operationDescription: OperationDescription = { From 103b3d5d4e3c1eaabbeb97e53bedd302f8bd0829 Mon Sep 17 00:00:00 2001 From: Warren James Date: Mon, 7 Jun 2021 15:42:05 -0400 Subject: [PATCH 03/16] Fixed formatting --- src/cmap/connection.ts | 33 --------------------------------- 1 file changed, 33 deletions(-) diff --git a/src/cmap/connection.ts b/src/cmap/connection.ts index 2ca27341dac..9d7acb505b7 100644 --- a/src/cmap/connection.ts +++ b/src/cmap/connection.ts @@ -757,39 +757,6 @@ function streamIdentifier(stream: Stream) { return uuidV4().toString('hex'); } -// FIXME: ensure this does a deep copy -function cloneCommand(command: WriteProtocolMessageType): WriteProtocolMessageType { - let clonedCommand: WriteProtocolMessageType; - if (command === null || command === undefined) { - clonedCommand = command; - } else if (command instanceof Query) { - clonedCommand = { ...command }; - clonedCommand.query = { ...command.query }; - clonedCommand.returnFieldSelector = { ...command.returnFieldSelector }; - } else if (command instanceof Msg) { - clonedCommand = { ...command }; - clonedCommand.command = { ...command.command }; - clonedCommand.options = { ...command.options }; - } else if (command instanceof GetMore) { - clonedCommand = { ...command }; - // FIXME: get this deep copy to work - clonedCommand.cursorId = Long.fromNumber(command.cursorId.toNumber()); - } else if (command instanceof KillCursor) { - //clonedCommand = _clone(command); - clonedCommand = { ...command }; - // FIXME: get this deep copy to work - const cursorIds: Long[] = []; - command.cursorIds.forEach(id => { - cursorIds.push(id); - }); - clonedCommand.cursorIds = cursorIds; - } else { - throw new Error(`Unrecognized command: ${command}`); - } - - return clonedCommand as WriteProtocolMessageType; -} - function write( conn: Connection, command: WriteProtocolMessageType, From 88f30b455b6b7a92012c61b4a909739484f70b1a Mon Sep 17 00:00:00 2001 From: Warren James Date: Tue, 8 Jun 2021 15:00:32 -0400 Subject: [PATCH 04/16] Changed scope of clonedCommand variable --- src/cmap/connection.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cmap/connection.ts b/src/cmap/connection.ts index 9d7acb505b7..9dbdc3b69fc 100644 --- a/src/cmap/connection.ts +++ b/src/cmap/connection.ts @@ -766,7 +766,6 @@ function write( if (typeof options === 'function') { callback = options; } - const clonedCommand = deepCopy(command); options = options ?? {}; const operationDescription: OperationDescription = { @@ -801,6 +800,7 @@ function write( // if command monitoring is enabled we need to modify the callback here if (conn.monitorCommands) { + const clonedCommand: WriteProtocolMessageType = deepCopy(command); conn.emit(Connection.COMMAND_STARTED, new CommandStartedEvent(conn, clonedCommand)); operationDescription.started = now(); From 61328bf4139daa43fca09f56f81aa5db31e7133f Mon Sep 17 00:00:00 2001 From: Warren James Date: Tue, 8 Jun 2021 15:01:46 -0400 Subject: [PATCH 05/16] Added test to ensure that copying commands for command monitoring works as expected --- test/functional/apm.test.js | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/test/functional/apm.test.js b/test/functional/apm.test.js index 2d42305b920..3565bfb7ce0 100644 --- a/test/functional/apm.test.js +++ b/test/functional/apm.test.js @@ -658,6 +658,41 @@ describe('APM', function () { } }); + // NODE-1502 + it('should not allow mutation of internal state from commands returned by event monitoring', { + metadata: { requires: { topology: ['single', 'replicaset'], mongodb: '>=3.0.0' } }, + test: function () { + const self = this; + const started = []; + const succeeded = []; + const client = self.configuration.newClient( + { writeConcern: { w: 1 } }, + { maxPoolSize: 1, monitorCommands: true } + ); + client.on('commandStarted', filterForCommands('insert', started)); + client.on('commandSucceeded', filterForCommands('insert', succeeded)); + + return client.connect().then(client => { + const db = client.db(self.configuration.db); + let documentToInsert = { a: 1 }; + return db + .collection('apm_test') + .insertOne(documentToInsert, { forceServerObjectId: true }) + .then(r => { + // Check if the returned document is a clone of the original + expect(documentToInsert).to.not.equal(started[0].command.documents[0]); + + expect(r).property('insertedId').to.exist; + expect(started.length).to.equal(1); + expect(started[0].commandName).to.equal('insert'); + expect(started[0].command.insert).to.equal('apm_test'); + expect(succeeded.length).to.equal(1); + return client.close(); + }); + }); + } + }); + describe('spec tests', function () { before(function () { return setupDatabase(this.configuration); From c950d95a0392c919205ad320e2188317a9f6273a Mon Sep 17 00:00:00 2001 From: Warren James Date: Wed, 9 Jun 2021 12:20:18 -0400 Subject: [PATCH 06/16] Made deep copies of command fields when extracting commands in src/cmap/command_monitoring_events.ts --- src/cmap/command_monitoring_events.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/cmap/command_monitoring_events.ts b/src/cmap/command_monitoring_events.ts index 042fc5ea55d..f9461958adb 100644 --- a/src/cmap/command_monitoring_events.ts +++ b/src/cmap/command_monitoring_events.ts @@ -181,7 +181,7 @@ const OP_QUERY_KEYS = [ function extractCommand(command: WriteProtocolMessageType): Document { if (command instanceof GetMore) { return { - getMore: command.cursorId, + getMore: deepCopy(command.cursorId), collection: collectionName(command), batchSize: command.numberToReturn }; @@ -190,15 +190,15 @@ function extractCommand(command: WriteProtocolMessageType): Document { if (command instanceof KillCursor) { return { killCursors: collectionName(command), - cursors: command.cursorIds + cursors: deepCopy(command.cursorIds) }; } if (command instanceof Msg) { - return command.command; + return deepCopy(command.command); } - if (command.query && command.query.$query) { + if (command.query?.$query) { let result: Document; if (command.ns === 'admin.$cmd') { // up-convert legacy command @@ -208,7 +208,7 @@ function extractCommand(command: WriteProtocolMessageType): Document { result = { find: collectionName(command) }; Object.keys(LEGACY_FIND_QUERY_MAP).forEach(key => { if (typeof command.query[key] !== 'undefined') { - result[LEGACY_FIND_QUERY_MAP[key]] = command.query[key]; + result[LEGACY_FIND_QUERY_MAP[key]] = deepCopy(command.query[key]); } }); } @@ -216,7 +216,7 @@ function extractCommand(command: WriteProtocolMessageType): Document { Object.keys(LEGACY_FIND_OPTIONS_MAP).forEach(key => { const legacyKey = key as keyof typeof LEGACY_FIND_OPTIONS_MAP; if (typeof command[legacyKey] !== 'undefined') { - result[LEGACY_FIND_OPTIONS_MAP[legacyKey]] = command[legacyKey]; + result[LEGACY_FIND_OPTIONS_MAP[legacyKey]] = deepCopy(command[legacyKey]); } }); From 8164edd1da8c6ac1ba9dbf07031b41c6a2c2c994 Mon Sep 17 00:00:00 2001 From: Warren James Date: Wed, 9 Jun 2021 12:22:13 -0400 Subject: [PATCH 07/16] Reverted changes to src/cmap/connection.ts --- src/cmap/connection.ts | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/cmap/connection.ts b/src/cmap/connection.ts index 9dbdc3b69fc..f0b8a434968 100644 --- a/src/cmap/connection.ts +++ b/src/cmap/connection.ts @@ -14,8 +14,7 @@ import { Callback, MongoDBNamespace, maxWireVersion, - HostAddress, - deepCopy + HostAddress } from '../utils'; import { AnyError, @@ -800,26 +799,25 @@ function write( // if command monitoring is enabled we need to modify the callback here if (conn.monitorCommands) { - const clonedCommand: WriteProtocolMessageType = deepCopy(command); - conn.emit(Connection.COMMAND_STARTED, new CommandStartedEvent(conn, clonedCommand)); + conn.emit(Connection.COMMAND_STARTED, new CommandStartedEvent(conn, command)); operationDescription.started = now(); operationDescription.cb = (err, reply) => { if (err) { conn.emit( Connection.COMMAND_FAILED, - new CommandFailedEvent(conn, clonedCommand, err, operationDescription.started) + new CommandFailedEvent(conn, command, err, operationDescription.started) ); } else { if (reply && (reply.ok === 0 || reply.$err)) { conn.emit( Connection.COMMAND_FAILED, - new CommandFailedEvent(conn, clonedCommand, reply, operationDescription.started) + new CommandFailedEvent(conn, command, reply, operationDescription.started) ); } else { conn.emit( Connection.COMMAND_SUCCEEDED, - new CommandSucceededEvent(conn, clonedCommand, reply, operationDescription.started) + new CommandSucceededEvent(conn, command, reply, operationDescription.started) ); } } From 85eb06af9628a83fa8179e4ea20798eaa160a78b Mon Sep 17 00:00:00 2001 From: Warren James Date: Wed, 9 Jun 2021 12:22:48 -0400 Subject: [PATCH 08/16] Made test for NODE-1502 account for nested documents --- test/functional/apm.test.js | 61 ++++++++++++++++++------------------- 1 file changed, 30 insertions(+), 31 deletions(-) diff --git a/test/functional/apm.test.js b/test/functional/apm.test.js index 3565bfb7ce0..8f3796edd63 100644 --- a/test/functional/apm.test.js +++ b/test/functional/apm.test.js @@ -659,38 +659,37 @@ describe('APM', function () { }); // NODE-1502 - it('should not allow mutation of internal state from commands returned by event monitoring', { - metadata: { requires: { topology: ['single', 'replicaset'], mongodb: '>=3.0.0' } }, - test: function () { - const self = this; - const started = []; - const succeeded = []; - const client = self.configuration.newClient( - { writeConcern: { w: 1 } }, - { maxPoolSize: 1, monitorCommands: true } - ); - client.on('commandStarted', filterForCommands('insert', started)); - client.on('commandSucceeded', filterForCommands('insert', succeeded)); - - return client.connect().then(client => { - const db = client.db(self.configuration.db); - let documentToInsert = { a: 1 }; - return db - .collection('apm_test') - .insertOne(documentToInsert, { forceServerObjectId: true }) - .then(r => { - // Check if the returned document is a clone of the original - expect(documentToInsert).to.not.equal(started[0].command.documents[0]); + it('should not allow mutation of internal state from commands returned by event monitoring', function () { + const self = this; + const started = []; + const succeeded = []; + const client = self.configuration.newClient( + { writeConcern: { w: 1 } }, + { maxPoolSize: 1, monitorCommands: true } + ); + client.on('commandStarted', filterForCommands('insert', started)); + client.on('commandSucceeded', filterForCommands('insert', succeeded)); + + return client.connect().then(client => { + const db = client.db(self.configuration.db); + let documentToInsert = { a: { b: 1 } }; + return db + .collection('apm_test') + .insertOne(documentToInsert) + .then(r => { + expect(r).property('insertedId').to.exist; + expect(started.length).to.equal(1); + expect(documentToInsert).to.eql(started[0].command.documents[0]); + // Check if the returned document is a clone of the original + expect(documentToInsert).to.not.equal(started[0].command.documents[0]); + expect(documentToInsert.a).to.not.equal(started[0].command.documents[0].a); - expect(r).property('insertedId').to.exist; - expect(started.length).to.equal(1); - expect(started[0].commandName).to.equal('insert'); - expect(started[0].command.insert).to.equal('apm_test'); - expect(succeeded.length).to.equal(1); - return client.close(); - }); - }); - } + expect(started[0].commandName).to.equal('insert'); + expect(started[0].command.insert).to.equal('apm_test'); + expect(succeeded.length).to.equal(1); + return client.close(); + }); + }); }); describe('spec tests', function () { From 0761b5ee5c27a19e1ab74019c766fcb30ce0000b Mon Sep 17 00:00:00 2001 From: Warren James Date: Wed, 9 Jun 2021 13:33:20 -0400 Subject: [PATCH 09/16] Added test metadata to ensure test only runs on mongodb versions >=3.6.0 --- test/functional/apm.test.js | 63 +++++++++++++++++++------------------ 1 file changed, 33 insertions(+), 30 deletions(-) diff --git a/test/functional/apm.test.js b/test/functional/apm.test.js index 8f3796edd63..054f4aba8e2 100644 --- a/test/functional/apm.test.js +++ b/test/functional/apm.test.js @@ -659,37 +659,40 @@ describe('APM', function () { }); // NODE-1502 - it('should not allow mutation of internal state from commands returned by event monitoring', function () { - const self = this; - const started = []; - const succeeded = []; - const client = self.configuration.newClient( - { writeConcern: { w: 1 } }, - { maxPoolSize: 1, monitorCommands: true } - ); - client.on('commandStarted', filterForCommands('insert', started)); - client.on('commandSucceeded', filterForCommands('insert', succeeded)); - - return client.connect().then(client => { - const db = client.db(self.configuration.db); - let documentToInsert = { a: { b: 1 } }; - return db - .collection('apm_test') - .insertOne(documentToInsert) - .then(r => { - expect(r).property('insertedId').to.exist; - expect(started.length).to.equal(1); - expect(documentToInsert).to.eql(started[0].command.documents[0]); - // Check if the returned document is a clone of the original - expect(documentToInsert).to.not.equal(started[0].command.documents[0]); - expect(documentToInsert.a).to.not.equal(started[0].command.documents[0].a); + it('should not allow mutation of internal state from commands returned by event monitoring', { + metadata: { requires: { mongodb: '>= 3.6.0' } }, + test: function () { + const self = this; + const started = []; + const succeeded = []; + const client = self.configuration.newClient( + { writeConcern: { w: 1 } }, + { maxPoolSize: 1, monitorCommands: true } + ); + client.on('commandStarted', filterForCommands('insert', started)); + client.on('commandSucceeded', filterForCommands('insert', succeeded)); - expect(started[0].commandName).to.equal('insert'); - expect(started[0].command.insert).to.equal('apm_test'); - expect(succeeded.length).to.equal(1); - return client.close(); - }); - }); + return client.connect().then(client => { + const db = client.db(self.configuration.db); + let documentToInsert = { a: { b: 1 } }; + return db + .collection('apm_test') + .insertOne(documentToInsert) + .then(r => { + expect(r).property('insertedId').to.exist; + expect(started.length).to.equal(1); + expect(documentToInsert).to.eql(started[0].command.documents[0]); + // Check if the returned document is a clone of the original + expect(documentToInsert).to.not.equal(started[0].command.documents[0]); + expect(documentToInsert.a).to.not.equal(started[0].command.documents[0].a); + + expect(started[0].commandName).to.equal('insert'); + expect(started[0].command.insert).to.equal('apm_test'); + expect(succeeded.length).to.equal(1); + return client.close(); + }); + }); + } }); describe('spec tests', function () { From 2a332180bab7c55da5c8ac9cb39821cc8102fe17 Mon Sep 17 00:00:00 2001 From: Warren James Date: Wed, 9 Jun 2021 17:14:08 -0400 Subject: [PATCH 10/16] fixed bug extractCommand in src/cmap/command_monitoring_events.ts --- src/cmap/command_monitoring_events.ts | 18 +++++++++-- test/functional/apm.test.js | 45 ++++++++++++++------------- 2 files changed, 39 insertions(+), 24 deletions(-) diff --git a/src/cmap/command_monitoring_events.ts b/src/cmap/command_monitoring_events.ts index f9461958adb..0327c04f1bf 100644 --- a/src/cmap/command_monitoring_events.ts +++ b/src/cmap/command_monitoring_events.ts @@ -1,4 +1,4 @@ -import { GetMore, KillCursor, Msg, WriteProtocolMessageType } from './commands'; +import { GetMore, KillCursor, Msg, Query, WriteProtocolMessageType } from './commands'; import { calculateDurationInMs, deepCopy } from '../utils'; import type { ConnectionPool } from './connection_pool'; import type { Connection } from './connection'; @@ -234,11 +234,23 @@ function extractCommand(command: WriteProtocolMessageType): Document { if (command.query.$explain) { return { explain: result }; } - return result; } - return command.query ? command.query : command; + const clonedQuery: Record = {}; + const clonedCommand: Record = {}; + if (command.query) { + for (const k in command.query) { + clonedQuery[k] = deepCopy(command.query[k]); + } + clonedCommand.query = clonedQuery; + } + + for (const k in command) { + if (k === 'query') continue; + clonedCommand[k] = deepCopy(((command as unknown) as Record)[k]); + } + return command.query ? clonedQuery : clonedCommand; } function extractReply(command: WriteProtocolMessageType, reply?: Document) { diff --git a/test/functional/apm.test.js b/test/functional/apm.test.js index 054f4aba8e2..55c8c7c4997 100644 --- a/test/functional/apm.test.js +++ b/test/functional/apm.test.js @@ -660,7 +660,9 @@ describe('APM', function () { // NODE-1502 it('should not allow mutation of internal state from commands returned by event monitoring', { - metadata: { requires: { mongodb: '>= 3.6.0' } }, + metadata: { + /*requires: { mongodb: '>= 3.6.0' } */ + }, test: function () { const self = this; const started = []; @@ -671,27 +673,28 @@ describe('APM', function () { ); client.on('commandStarted', filterForCommands('insert', started)); client.on('commandSucceeded', filterForCommands('insert', succeeded)); + let documentToInsert = { a: { b: 1 } }; + return client + .connect() + .then(client => { + const db = client.db(self.configuration.db); + return db.collection('apm_test').insertOne(documentToInsert); + }) + .then(r => { + expect(r).to.have.property('insertedId').that.is.an('object'); + expect(started.length).to.equal(1); + // Check if contents of returned document are equal to document inserted (by value) + expect(documentToInsert).to.deep.equal(started[0].command.documents[0]); + // Check if the returned document is a clone of the original. This confirms that the + // reference is not the same. + expect(documentToInsert !== started[0].command.documents[0]).to.equal(true); + expect(documentToInsert.a !== started[0].command.documents[0].a).to.equal(true); - return client.connect().then(client => { - const db = client.db(self.configuration.db); - let documentToInsert = { a: { b: 1 } }; - return db - .collection('apm_test') - .insertOne(documentToInsert) - .then(r => { - expect(r).property('insertedId').to.exist; - expect(started.length).to.equal(1); - expect(documentToInsert).to.eql(started[0].command.documents[0]); - // Check if the returned document is a clone of the original - expect(documentToInsert).to.not.equal(started[0].command.documents[0]); - expect(documentToInsert.a).to.not.equal(started[0].command.documents[0].a); - - expect(started[0].commandName).to.equal('insert'); - expect(started[0].command.insert).to.equal('apm_test'); - expect(succeeded.length).to.equal(1); - return client.close(); - }); - }); + expect(started[0].commandName).to.equal('insert'); + expect(started[0].command.insert).to.equal('apm_test'); + expect(succeeded.length).to.equal(1); + return client.close(); + }); } }); From dcdecb12028d1ebf66c8f8bf93a5841203e0b173 Mon Sep 17 00:00:00 2001 From: Warren James Date: Wed, 9 Jun 2021 17:56:00 -0400 Subject: [PATCH 11/16] Removed unused import --- src/cmap/command_monitoring_events.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cmap/command_monitoring_events.ts b/src/cmap/command_monitoring_events.ts index 0327c04f1bf..e770f7fedca 100644 --- a/src/cmap/command_monitoring_events.ts +++ b/src/cmap/command_monitoring_events.ts @@ -1,4 +1,4 @@ -import { GetMore, KillCursor, Msg, Query, WriteProtocolMessageType } from './commands'; +import { GetMore, KillCursor, Msg, WriteProtocolMessageType } from './commands'; import { calculateDurationInMs, deepCopy } from '../utils'; import type { ConnectionPool } from './connection_pool'; import type { Connection } from './connection'; From 59a3e7335244863860c551894ff7c1fb5e9c91a8 Mon Sep 17 00:00:00 2001 From: Warren James Date: Thu, 10 Jun 2021 10:52:13 -0400 Subject: [PATCH 12/16] Resolved review comments --- test/functional/apm.test.js | 71 ++++++++++++++++++------------------- 1 file changed, 34 insertions(+), 37 deletions(-) diff --git a/test/functional/apm.test.js b/test/functional/apm.test.js index 55c8c7c4997..957ba615f3f 100644 --- a/test/functional/apm.test.js +++ b/test/functional/apm.test.js @@ -659,43 +659,40 @@ describe('APM', function () { }); // NODE-1502 - it('should not allow mutation of internal state from commands returned by event monitoring', { - metadata: { - /*requires: { mongodb: '>= 3.6.0' } */ - }, - test: function () { - const self = this; - const started = []; - const succeeded = []; - const client = self.configuration.newClient( - { writeConcern: { w: 1 } }, - { maxPoolSize: 1, monitorCommands: true } - ); - client.on('commandStarted', filterForCommands('insert', started)); - client.on('commandSucceeded', filterForCommands('insert', succeeded)); - let documentToInsert = { a: { b: 1 } }; - return client - .connect() - .then(client => { - const db = client.db(self.configuration.db); - return db.collection('apm_test').insertOne(documentToInsert); - }) - .then(r => { - expect(r).to.have.property('insertedId').that.is.an('object'); - expect(started.length).to.equal(1); - // Check if contents of returned document are equal to document inserted (by value) - expect(documentToInsert).to.deep.equal(started[0].command.documents[0]); - // Check if the returned document is a clone of the original. This confirms that the - // reference is not the same. - expect(documentToInsert !== started[0].command.documents[0]).to.equal(true); - expect(documentToInsert.a !== started[0].command.documents[0].a).to.equal(true); - - expect(started[0].commandName).to.equal('insert'); - expect(started[0].command.insert).to.equal('apm_test'); - expect(succeeded.length).to.equal(1); - return client.close(); - }); - } + it('should not allow mutation of internal state from commands returned by event monitoring', function () { + const started = []; + const succeeded = []; + const client = this.configuration.newClient( + { writeConcern: { w: 1 } }, + { maxPoolSize: 1, monitorCommands: true } + ); + client.on('commandStarted', filterForCommands('insert', started)); + client.on('commandSucceeded', filterForCommands('insert', succeeded)); + let documentToInsert = { a: { b: 1 } }; + return client + .connect() + .then(client => { + const db = client.db(this.configuration.db); + return db.collection('apm_test').insertOne(documentToInsert); + }) + .then(r => { + expect(r).to.have.property('insertedId').that.is.an('object'); + expect(started).to.have.lengthOf(1); + // Check if contents of returned document are equal to document inserted (by value) + expect(documentToInsert).to.deep.equal(started[0].command.documents[0]); + // Check if the returned document is a clone of the original. This confirms that the + // reference is not the same. + expect(documentToInsert !== started[0].command.documents[0]).to.equal(true); + expect(documentToInsert.a !== started[0].command.documents[0].a).to.equal(true); + + started[0].command.documents[0].a.b = 2; + expect(documentToInsert.a.b).to.equal(1); + + expect(started[0].commandName).to.equal('insert'); + expect(started[0].command.insert).to.equal('apm_test'); + expect(succeeded).to.have.lengthOf(1); + return client.close(); + }); }); describe('spec tests', function () { From a124325aa77a9983c83d78fdeda71fb225dee5cc Mon Sep 17 00:00:00 2001 From: Warren James Date: Thu, 10 Jun 2021 12:33:22 -0400 Subject: [PATCH 13/16] Cleaning up after test --- test/functional/multiple_db.test.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/functional/multiple_db.test.js b/test/functional/multiple_db.test.js index 740668d72ce..739d937641c 100644 --- a/test/functional/multiple_db.test.js +++ b/test/functional/multiple_db.test.js @@ -85,7 +85,9 @@ describe('Multiple Databases', function () { { returnDocument: ReturnDocument.AFTER }, function (err) { expect(err).to.not.exist; - client.close(done); + collection.drop(() => { + client.close(done); + }); } ); }); From 4560e9d581e66b0ea3056b21ecf3ba757e56807e Mon Sep 17 00:00:00 2001 From: Warren James Date: Thu, 10 Jun 2021 15:04:18 -0400 Subject: [PATCH 14/16] Added unit test to ensure the CommandStartedEvents do not hold references to commands passed into the constructor --- .../cmap/command_monitoring_events.test.js | 78 +++++++++++++++++++ 1 file changed, 78 insertions(+) create mode 100644 test/unit/cmap/command_monitoring_events.test.js diff --git a/test/unit/cmap/command_monitoring_events.test.js b/test/unit/cmap/command_monitoring_events.test.js new file mode 100644 index 00000000000..4458a9e74dc --- /dev/null +++ b/test/unit/cmap/command_monitoring_events.test.js @@ -0,0 +1,78 @@ +'use strict'; + +const mock = require('../../tools/mock'); +const { connect } = require('../../../src/cmap/connect'); +const { Connection } = require('../../../src/cmap/connection'); +const { Msg, Query, GetMore, KillCursor } = require('../../../src/cmap/commands'); +const { CommandStartedEvent } = require('../../../src/cmap/command_monitoring_events'); +const { expect } = require('chai'); +const { Long } = require('bson'); + +describe('Command Monitoring Events - unit/cmap', function () { + let server; + after(() => mock.cleanup()); + before(() => mock.createServer().then(s => (server = s))); + + it('should never hold references to commands passed into CommandStartedEvent objects', function (done) { + const commands = [ + new Query('admin.$cmd', { a: { b: 10 }, $query: { b: 10 } }, {}), + new Query('hello', { a: { b: 10 }, $query: { b: 10 } }, {}), + new Msg('admin.$cmd', { b: { c: 20 } }, {}), + new Msg('hello', { b: { c: 20 } }, {}), + new GetMore('admin.$cmd', Long.fromNumber(10)), + new GetMore('hello', Long.fromNumber(10)), + new KillCursor('admin.$cmd', [Long.fromNumber(100), Long.fromNumber(200)]), + new KillCursor('hello', [Long.fromNumber(100), Long.fromNumber(200)]) + ]; + + server.setMessageHandler(request => { + const doc = request.document; + if (doc.ismaster || doc.hello) { + request.reply(mock.DEFAULT_ISMASTER_36); + } + + // blackhole all other requests + }); + + connect({ connectionType: Connection, hostAddress: server.hostAddress() }, (err, conn) => { + expect(err).to.be.undefined; + expect(conn).to.not.be.undefined; + + commands.forEach(c => { + const ev = new CommandStartedEvent(conn, c); + if (c instanceof Query) { + if (c.ns === 'admin.$cmd') { + expect(ev.command !== c.query.$query).to.equal(true); + for (const k in c.query.$query) { + expect(ev.command[k]).to.deep.equal(c.query.$query[k]); + } + } else { + expect(ev.command.filter !== c.query.$query).to.equal(true); + for (const k in c.query.$query) { + expect(ev.command.filter[k]).to.deep.equal(c.query.$query[k]); + } + } + } else if (c instanceof Msg) { + expect(ev.command !== c.command).to.equal(true); + expect(ev.command).to.deep.equal(c.command); + } else if (c instanceof GetMore) { + // NOTE: BSON Longs pass strict equality when their internal values are equal + // i.e. + // let l1 = Long(10); + // let l2 = Long(10); + // l1 === l2 // returns true + // expect(ev.command.getMore !== c.cursorId).to.equal(true); + expect(ev.command.getMore).to.deep.equal(c.cursorId); + + ev.command.getMore = Long.fromNumber(50128); + expect(c.cursorId).to.not.deep.equal(ev.command.getMore); + } else if (c instanceof KillCursor) { + expect(ev.command.cursors !== c.cursorIds).to.equal(true); + expect(ev.command.cursors).to.deep.equal(c.cursorIds); + } + }); + + done(); + }); + }); +}); From 7fe88189add0b35ee036ba742d0f85f2623323b4 Mon Sep 17 00:00:00 2001 From: Warren James Date: Fri, 11 Jun 2021 12:44:30 -0400 Subject: [PATCH 15/16] fix: removed unneeded boilerplate from test --- .../cmap/command_monitoring_events.test.js | 111 ++++++++---------- 1 file changed, 49 insertions(+), 62 deletions(-) diff --git a/test/unit/cmap/command_monitoring_events.test.js b/test/unit/cmap/command_monitoring_events.test.js index 4458a9e74dc..bed26e8563d 100644 --- a/test/unit/cmap/command_monitoring_events.test.js +++ b/test/unit/cmap/command_monitoring_events.test.js @@ -1,77 +1,64 @@ 'use strict'; -const mock = require('../../tools/mock'); -const { connect } = require('../../../src/cmap/connect'); -const { Connection } = require('../../../src/cmap/connection'); const { Msg, Query, GetMore, KillCursor } = require('../../../src/cmap/commands'); const { CommandStartedEvent } = require('../../../src/cmap/command_monitoring_events'); const { expect } = require('chai'); const { Long } = require('bson'); describe('Command Monitoring Events - unit/cmap', function () { - let server; - after(() => mock.cleanup()); - before(() => mock.createServer().then(s => (server = s))); + const commands = [ + new Query('admin.$cmd', { a: { b: 10 }, $query: { b: 10 } }, {}), + new Query('hello', { a: { b: 10 }, $query: { b: 10 } }, {}), + new Msg('admin.$cmd', { b: { c: 20 } }, {}), + new Msg('hello', { b: { c: 20 } }, {}), + new GetMore('admin.$cmd', Long.fromNumber(10)), + new GetMore('hello', Long.fromNumber(10)), + new KillCursor('admin.$cmd', [Long.fromNumber(100), Long.fromNumber(200)]), + new KillCursor('hello', [Long.fromNumber(100), Long.fromNumber(200)]), + { ns: 'admin.$cmd', query: { $query: { a: 16 } } }, + { ns: 'hello there', f1: { h: { a: 52, b: { c: 10, d: [1, 2, 3, 5] } } } } + ]; - it('should never hold references to commands passed into CommandStartedEvent objects', function (done) { - const commands = [ - new Query('admin.$cmd', { a: { b: 10 }, $query: { b: 10 } }, {}), - new Query('hello', { a: { b: 10 }, $query: { b: 10 } }, {}), - new Msg('admin.$cmd', { b: { c: 20 } }, {}), - new Msg('hello', { b: { c: 20 } }, {}), - new GetMore('admin.$cmd', Long.fromNumber(10)), - new GetMore('hello', Long.fromNumber(10)), - new KillCursor('admin.$cmd', [Long.fromNumber(100), Long.fromNumber(200)]), - new KillCursor('hello', [Long.fromNumber(100), Long.fromNumber(200)]) - ]; - - server.setMessageHandler(request => { - const doc = request.document; - if (doc.ismaster || doc.hello) { - request.reply(mock.DEFAULT_ISMASTER_36); - } - - // blackhole all other requests - }); - - connect({ connectionType: Connection, hostAddress: server.hostAddress() }, (err, conn) => { - expect(err).to.be.undefined; - expect(conn).to.not.be.undefined; - - commands.forEach(c => { - const ev = new CommandStartedEvent(conn, c); - if (c instanceof Query) { - if (c.ns === 'admin.$cmd') { - expect(ev.command !== c.query.$query).to.equal(true); - for (const k in c.query.$query) { - expect(ev.command[k]).to.deep.equal(c.query.$query[k]); - } - } else { - expect(ev.command.filter !== c.query.$query).to.equal(true); - for (const k in c.query.$query) { - expect(ev.command.filter[k]).to.deep.equal(c.query.$query[k]); - } + commands.forEach(c => { + it(`should make a deep copy of object of type: ${c.constructor.name}`, done => { + const ev = new CommandStartedEvent({ id: 'someId', address: 'someHost' }, c); + if (c instanceof Query) { + if (c.ns === 'admin.$cmd') { + expect(ev.command !== c.query.$query).to.equal(true); + for (const k in c.query.$query) { + expect(ev.command[k]).to.deep.equal(c.query.$query[k]); + } + } else { + expect(ev.command.filter !== c.query.$query).to.equal(true); + for (const k in c.query.$query) { + expect(ev.command.filter[k]).to.deep.equal(c.query.$query[k]); } - } else if (c instanceof Msg) { - expect(ev.command !== c.command).to.equal(true); - expect(ev.command).to.deep.equal(c.command); - } else if (c instanceof GetMore) { - // NOTE: BSON Longs pass strict equality when their internal values are equal - // i.e. - // let l1 = Long(10); - // let l2 = Long(10); - // l1 === l2 // returns true - // expect(ev.command.getMore !== c.cursorId).to.equal(true); - expect(ev.command.getMore).to.deep.equal(c.cursorId); - - ev.command.getMore = Long.fromNumber(50128); - expect(c.cursorId).to.not.deep.equal(ev.command.getMore); - } else if (c instanceof KillCursor) { - expect(ev.command.cursors !== c.cursorIds).to.equal(true); - expect(ev.command.cursors).to.deep.equal(c.cursorIds); } - }); + } else if (c instanceof Msg) { + expect(ev.command !== c.command).to.equal(true); + expect(ev.command).to.deep.equal(c.command); + } else if (c instanceof GetMore) { + // NOTE: BSON Longs pass strict equality when their internal values are equal + // i.e. + // let l1 = Long(10); + // let l2 = Long(10); + // l1 === l2 // returns true + // expect(ev.command.getMore !== c.cursorId).to.equal(true); + expect(ev.command.getMore).to.deep.equal(c.cursorId); + ev.command.getMore = Long.fromNumber(50128); + expect(c.cursorId).to.not.deep.equal(ev.command.getMore); + } else if (c instanceof KillCursor) { + expect(ev.command.cursors !== c.cursorIds).to.equal(true); + expect(ev.command.cursors).to.deep.equal(c.cursorIds); + } else if (typeof c === 'object') { + if (c.ns === 'admin.$cmd') { + expect(ev.command !== c.query.$query).to.equal(true); + for (const k in c.query.$query) { + expect(ev.command[k]).to.deep.equal(c.query.$query[k]); + } + } + } done(); }); }); From b6c58ae35c5bba0568e8a83d45235ebe4678f4c4 Mon Sep 17 00:00:00 2001 From: Warren James Date: Fri, 11 Jun 2021 15:02:03 -0400 Subject: [PATCH 16/16] fix: removed done callback and changed forEach to for of loop --- .../cmap/command_monitoring_events.test.js | 55 +++++++++---------- 1 file changed, 27 insertions(+), 28 deletions(-) diff --git a/test/unit/cmap/command_monitoring_events.test.js b/test/unit/cmap/command_monitoring_events.test.js index bed26e8563d..7303bbe2622 100644 --- a/test/unit/cmap/command_monitoring_events.test.js +++ b/test/unit/cmap/command_monitoring_events.test.js @@ -19,47 +19,46 @@ describe('Command Monitoring Events - unit/cmap', function () { { ns: 'hello there', f1: { h: { a: 52, b: { c: 10, d: [1, 2, 3, 5] } } } } ]; - commands.forEach(c => { - it(`should make a deep copy of object of type: ${c.constructor.name}`, done => { - const ev = new CommandStartedEvent({ id: 'someId', address: 'someHost' }, c); - if (c instanceof Query) { - if (c.ns === 'admin.$cmd') { - expect(ev.command !== c.query.$query).to.equal(true); - for (const k in c.query.$query) { - expect(ev.command[k]).to.deep.equal(c.query.$query[k]); + for (const command of commands) { + it(`should make a deep copy of object of type: ${command.constructor.name}`, () => { + const ev = new CommandStartedEvent({ id: 'someId', address: 'someHost' }, command); + if (command instanceof Query) { + if (command.ns === 'admin.$cmd') { + expect(ev.command !== command.query.$query).to.equal(true); + for (const k in command.query.$query) { + expect(ev.command[k]).to.deep.equal(command.query.$query[k]); } } else { - expect(ev.command.filter !== c.query.$query).to.equal(true); - for (const k in c.query.$query) { - expect(ev.command.filter[k]).to.deep.equal(c.query.$query[k]); + expect(ev.command.filter !== command.query.$query).to.equal(true); + for (const k in command.query.$query) { + expect(ev.command.filter[k]).to.deep.equal(command.query.$query[k]); } } - } else if (c instanceof Msg) { - expect(ev.command !== c.command).to.equal(true); - expect(ev.command).to.deep.equal(c.command); - } else if (c instanceof GetMore) { + } else if (command instanceof Msg) { + expect(ev.command !== command.command).to.equal(true); + expect(ev.command).to.deep.equal(command.command); + } else if (command instanceof GetMore) { // NOTE: BSON Longs pass strict equality when their internal values are equal // i.e. // let l1 = Long(10); // let l2 = Long(10); // l1 === l2 // returns true - // expect(ev.command.getMore !== c.cursorId).to.equal(true); - expect(ev.command.getMore).to.deep.equal(c.cursorId); + // expect(ev.command.getMore !== command.cursorId).to.equal(true); + expect(ev.command.getMore).to.deep.equal(command.cursorId); ev.command.getMore = Long.fromNumber(50128); - expect(c.cursorId).to.not.deep.equal(ev.command.getMore); - } else if (c instanceof KillCursor) { - expect(ev.command.cursors !== c.cursorIds).to.equal(true); - expect(ev.command.cursors).to.deep.equal(c.cursorIds); - } else if (typeof c === 'object') { - if (c.ns === 'admin.$cmd') { - expect(ev.command !== c.query.$query).to.equal(true); - for (const k in c.query.$query) { - expect(ev.command[k]).to.deep.equal(c.query.$query[k]); + expect(command.cursorId).to.not.deep.equal(ev.command.getMore); + } else if (command instanceof KillCursor) { + expect(ev.command.cursors !== command.cursorIds).to.equal(true); + expect(ev.command.cursors).to.deep.equal(command.cursorIds); + } else if (typeof command === 'object') { + if (command.ns === 'admin.$cmd') { + expect(ev.command !== command.query.$query).to.equal(true); + for (const k in command.query.$query) { + expect(ev.command[k]).to.deep.equal(command.query.$query[k]); } } } - done(); }); - }); + } });