Skip to content

Commit

Permalink
fix(NODE-1502): command monitoring objects hold internal state refere…
Browse files Browse the repository at this point in the history
…nces (#2832)
  • Loading branch information
W-A-James authored Jun 14, 2021
1 parent 6bd506b commit a2887db
Show file tree
Hide file tree
Showing 4 changed files with 124 additions and 9 deletions.
28 changes: 20 additions & 8 deletions src/cmap/command_monitoring_events.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
};
Expand All @@ -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
Expand All @@ -208,15 +208,15 @@ 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]);
}
});
}

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]);
}
});

Expand All @@ -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<string, unknown> = {};
const clonedCommand: Record<string, unknown> = {};
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<string, unknown>)[k]);
}
return command.query ? clonedQuery : clonedCommand;
}

function extractReply(command: WriteProtocolMessageType, reply?: Document) {
Expand Down
37 changes: 37 additions & 0 deletions test/functional/apm.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -658,6 +658,43 @@ describe('APM', function () {
}
});

// NODE-1502
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 () {
before(function () {
return setupDatabase(this.configuration);
Expand Down
4 changes: 3 additions & 1 deletion test/functional/multiple_db.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
}
);
});
Expand Down
64 changes: 64 additions & 0 deletions test/unit/cmap/command_monitoring_events.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
'use strict';

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 () {
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] } } } }
];

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 !== 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 (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 !== command.cursorId).to.equal(true);
expect(ev.command.getMore).to.deep.equal(command.cursorId);

ev.command.getMore = Long.fromNumber(50128);
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]);
}
}
}
});
}
});

0 comments on commit a2887db

Please sign in to comment.