From 953a387113039491721a4f7efaa8a58de322dca3 Mon Sep 17 00:00:00 2001 From: Warren James Date: Wed, 15 Jan 2025 12:51:39 -0500 Subject: [PATCH 1/6] spreading changes --- src/cmap/command_monitoring_events.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/cmap/command_monitoring_events.ts b/src/cmap/command_monitoring_events.ts index a7e1653becc..c973c5ead2a 100644 --- a/src/cmap/command_monitoring_events.ts +++ b/src/cmap/command_monitoring_events.ts @@ -254,7 +254,7 @@ const OP_QUERY_KEYS = [ /** Extract the actual command from the query, possibly up-converting if it's a legacy format */ function extractCommand(command: WriteProtocolMessageType): Document { if (command instanceof OpMsgRequest) { - const cmd = deepCopy(command.command); + const cmd = { ...command.command }; // For OP_MSG with payload type 1 we need to pull the documents // array out of the document sequence for monitoring. if (cmd.ops instanceof DocumentSequence) { @@ -326,7 +326,7 @@ function extractReply(command: WriteProtocolMessageType, reply?: Document) { } if (command instanceof OpMsgRequest) { - return deepCopy(reply.result ? reply.result : reply); + return reply.result ? reply.result : reply; } // is this a legacy find command? @@ -334,14 +334,14 @@ function extractReply(command: WriteProtocolMessageType, reply?: Document) { return { ok: 1, cursor: { - id: deepCopy(reply.cursorId), + id: reply.cursorId, ns: namespace(command), - firstBatch: deepCopy(reply.documents) + firstBatch: reply.documents } }; } - return deepCopy(reply.result ? reply.result : reply); + return reply.result ? reply.result : reply; } function extractConnectionDetails(connection: Connection) { From 5c6c2655068869cc04c7a8e9804705853f9a3a56 Mon Sep 17 00:00:00 2001 From: Warren James Date: Wed, 15 Jan 2025 16:15:15 -0500 Subject: [PATCH 2/6] remove deep copy test --- .../command_monitoring.test.ts | 30 ------------------- 1 file changed, 30 deletions(-) diff --git a/test/integration/command-logging-and-monitoring/command_monitoring.test.ts b/test/integration/command-logging-and-monitoring/command_monitoring.test.ts index 124d5e7f9a2..7a7f9759879 100644 --- a/test/integration/command-logging-and-monitoring/command_monitoring.test.ts +++ b/test/integration/command-logging-and-monitoring/command_monitoring.test.ts @@ -617,35 +617,5 @@ describe('Command Monitoring', function () { afterEach(function (done) { client.close(done); }); - - // NODE-1502 - it('should not allow mutation of internal state from commands returned by event monitoring', function () { - const started = []; - const succeeded = []; - client.on('commandStarted', filterForCommands('insert', started)); - client.on('commandSucceeded', filterForCommands('insert', succeeded)); - const documentToInsert = { a: { b: 1 } }; - 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); - }); - }); }); }); From 0b2b7e7456e20e0d9c757b09b819f19248180974 Mon Sep 17 00:00:00 2001 From: Warren James Date: Wed, 15 Jan 2025 16:27:04 -0500 Subject: [PATCH 3/6] remove remaining deep copies --- src/cmap/command_monitoring_events.ts | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/src/cmap/command_monitoring_events.ts b/src/cmap/command_monitoring_events.ts index c973c5ead2a..223a5782860 100644 --- a/src/cmap/command_monitoring_events.ts +++ b/src/cmap/command_monitoring_events.ts @@ -6,7 +6,7 @@ import { LEGACY_HELLO_COMMAND, LEGACY_HELLO_COMMAND_CAMEL_CASE } from '../constants'; -import { calculateDurationInMs, deepCopy } from '../utils'; +import { calculateDurationInMs } from '../utils'; import { DocumentSequence, OpMsgRequest, @@ -276,7 +276,7 @@ function extractCommand(command: WriteProtocolMessageType): Document { result = { find: collectionName(command) }; Object.keys(LEGACY_FIND_QUERY_MAP).forEach(key => { if (command.query[key] != null) { - result[LEGACY_FIND_QUERY_MAP[key]] = deepCopy(command.query[key]); + result[LEGACY_FIND_QUERY_MAP[key]] = { ...command.query[key] }; } }); } @@ -284,7 +284,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 (command[legacyKey] != null) { - result[LEGACY_FIND_OPTIONS_MAP[legacyKey]] = deepCopy(command[legacyKey]); + result[LEGACY_FIND_OPTIONS_MAP[legacyKey]] = command[legacyKey]; } }); @@ -304,19 +304,13 @@ function extractCommand(command: WriteProtocolMessageType): Document { return result; } - const clonedQuery: Record = {}; - const clonedCommand: Record = {}; + let clonedQuery: Record = {}; + const clonedCommand: Record = { ...command }; if (command.query) { - for (const k in command.query) { - clonedQuery[k] = deepCopy(command.query[k]); - } + clonedQuery = { ...command.query }; 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; } From dcb063792bbedfb9c1920bacf140749870decebb Mon Sep 17 00:00:00 2001 From: Warren James Date: Wed, 15 Jan 2025 16:55:54 -0500 Subject: [PATCH 4/6] remove unused context block --- .../command_monitoring.test.ts | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/test/integration/command-logging-and-monitoring/command_monitoring.test.ts b/test/integration/command-logging-and-monitoring/command_monitoring.test.ts index 7a7f9759879..eb589346a0c 100644 --- a/test/integration/command-logging-and-monitoring/command_monitoring.test.ts +++ b/test/integration/command-logging-and-monitoring/command_monitoring.test.ts @@ -603,19 +603,4 @@ describe('Command Monitoring', function () { }); } }); - - describe('Internal state references', function () { - let client; - - beforeEach(function () { - client = this.configuration.newClient( - { writeConcern: { w: 1 } }, - { maxPoolSize: 1, monitorCommands: true } - ); - }); - - afterEach(function (done) { - client.close(done); - }); - }); }); From 251de2ac4c872bcf7b49707aaa8af6fd2c23e074 Mon Sep 17 00:00:00 2001 From: Warren James Date: Thu, 16 Jan 2025 17:05:55 -0500 Subject: [PATCH 5/6] remove dead code --- src/cmap/command_monitoring_events.ts | 13 ---------- src/utils.ts | 37 --------------------------- 2 files changed, 50 deletions(-) diff --git a/src/cmap/command_monitoring_events.ts b/src/cmap/command_monitoring_events.ts index 223a5782860..78f99a98362 100644 --- a/src/cmap/command_monitoring_events.ts +++ b/src/cmap/command_monitoring_events.ts @@ -288,19 +288,6 @@ function extractCommand(command: WriteProtocolMessageType): Document { } }); - OP_QUERY_KEYS.forEach(key => { - if (command[key]) { - result[key] = command[key]; - } - }); - - if (command.pre32Limit != null) { - result.limit = command.pre32Limit; - } - - if (command.query.$explain) { - return { explain: result }; - } return result; } diff --git a/src/utils.ts b/src/utils.ts index c23161612a8..55cf455676b 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -620,43 +620,6 @@ export function isRecord( return true; } -/** - * Make a deep copy of an object - * - * NOTE: This is not meant to be the perfect implementation of a deep copy, - * but instead something that is good enough for the purposes of - * command monitoring. - */ -export function deepCopy(value: T): T { - if (value == null) { - return value; - } else if (Array.isArray(value)) { - return value.map(item => deepCopy(item)) as unknown as T; - } else if (isRecord(value)) { - const res = {} as any; - for (const key in value) { - res[key] = deepCopy(value[key]); - } - return res; - } - - const ctor = (value as any).constructor; - if (ctor) { - switch (ctor.name.toLowerCase()) { - case 'date': - return new ctor(Number(value)); - case 'map': - return new Map(value as any) as unknown as T; - case 'set': - return new Set(value as any) as unknown as T; - case 'buffer': - return Buffer.from(value as unknown as Buffer) as unknown as T; - } - } - - return value; -} - type ListNode = { value: T; next: ListNode | HeadNode; From 7fb0c49e238e8849dba8c2f64661b74bc33f1db0 Mon Sep 17 00:00:00 2001 From: Warren James Date: Fri, 17 Jan 2025 12:42:37 -0500 Subject: [PATCH 6/6] remove dead code --- src/cmap/command_monitoring_events.ts | 30 ++------------------------- 1 file changed, 2 insertions(+), 28 deletions(-) diff --git a/src/cmap/command_monitoring_events.ts b/src/cmap/command_monitoring_events.ts index 78f99a98362..992b9dc47c9 100644 --- a/src/cmap/command_monitoring_events.ts +++ b/src/cmap/command_monitoring_events.ts @@ -125,7 +125,7 @@ export class CommandSucceededEvent { this.requestId = command.requestId; this.commandName = commandName; this.duration = calculateDurationInMs(started); - this.reply = maybeRedact(commandName, cmd, extractReply(command, reply)); + this.reply = maybeRedact(commandName, cmd, extractReply(reply)); this.serverConnectionId = serverConnectionId; } @@ -214,7 +214,6 @@ const HELLO_COMMANDS = new Set(['hello', LEGACY_HELLO_COMMAND, LEGACY_HELLO_COMM // helper methods const extractCommandName = (commandDoc: Document) => Object.keys(commandDoc)[0]; -const namespace = (command: OpQueryRequest) => command.ns; const collectionName = (command: OpQueryRequest) => command.ns.split('.')[1]; const maybeRedact = (commandName: string, commandDoc: Document, result: Error | Document) => SENSITIVE_COMMANDS.has(commandName) || @@ -242,15 +241,6 @@ const LEGACY_FIND_OPTIONS_MAP = { returnFieldSelector: 'projection' } as const; -const OP_QUERY_KEYS = [ - 'tailable', - 'oplogReplay', - 'noCursorTimeout', - 'awaitData', - 'partial', - 'exhaust' -] as const; - /** Extract the actual command from the query, possibly up-converting if it's a legacy format */ function extractCommand(command: WriteProtocolMessageType): Document { if (command instanceof OpMsgRequest) { @@ -301,27 +291,11 @@ function extractCommand(command: WriteProtocolMessageType): Document { return command.query ? clonedQuery : clonedCommand; } -function extractReply(command: WriteProtocolMessageType, reply?: Document) { +function extractReply(reply?: Document) { if (!reply) { return reply; } - if (command instanceof OpMsgRequest) { - return reply.result ? reply.result : reply; - } - - // is this a legacy find command? - if (command.query && command.query.$query != null) { - return { - ok: 1, - cursor: { - id: reply.cursorId, - ns: namespace(command), - firstBatch: reply.documents - } - }; - } - return reply.result ? reply.result : reply; }