From fdb828b5661d22fc355fd8a2f937a1beff8631ba Mon Sep 17 00:00:00 2001 From: Sophie Saskin Date: Mon, 18 Jun 2018 09:46:36 -0400 Subject: [PATCH] fix(collection): only send bypassDocumentValidation if true The bypassDocumentValidation key will only be set if it explicitly receives a true, otherwise it remains undefined. Fixes NODE-1492 --- lib/apm.js | 2 +- lib/bulk/ordered.js | 13 +- lib/bulk/unordered.js | 13 +- lib/collection.js | 2 +- lib/operations/collection_ops.js | 6 +- test/unit/bypass_validation_tests.js | 239 +++++++++++++++++++++++++++ 6 files changed, 260 insertions(+), 15 deletions(-) create mode 100644 test/unit/bypass_validation_tests.js diff --git a/lib/apm.js b/lib/apm.js index 3736e8fdda..f78e4aaff6 100644 --- a/lib/apm.js +++ b/lib/apm.js @@ -9,7 +9,7 @@ class Instrumentation extends EventEmitter { instrument(MongoClient, callback) { // store a reference to the original functions this.$MongoClient = MongoClient; - const $prototypeConnect = this.$prototypeConnect = MongoClient.prototype.connect; // eslint-disable-line + const $prototypeConnect = (this.$prototypeConnect = MongoClient.prototype.connect); const instrumentation = this; MongoClient.prototype.connect = function(callback) { diff --git a/lib/bulk/ordered.js b/lib/bulk/ordered.js index dcf01d1c5b..926412511e 100644 --- a/lib/bulk/ordered.js +++ b/lib/bulk/ordered.js @@ -300,14 +300,13 @@ function OrderedBulkOperation(topology, collection, options) { promiseLibrary: promiseLibrary, // Fundamental error err: null, - // Bypass validation - bypassDocumentValidation: - typeof options.bypassDocumentValidation === 'boolean' - ? options.bypassDocumentValidation - : false, // check keys checkKeys: typeof options.checkKeys === 'boolean' ? options.checkKeys : true }; + // bypass Validation + if (options.bypassDocumentValidation === true) { + this.s.bypassDocumentValidation = true; + } } OrderedBulkOperation.prototype.raw = function(op) { @@ -490,6 +489,10 @@ var executeCommands = function(self, options, callback) { finalOptions.writeConcern = self.s.writeConcern; } + if (finalOptions.bypassDocumentValidation !== true) { + delete finalOptions.bypassDocumentValidation; + } + // Set an operationIf if provided if (self.operationId) { resultHandler.operationId = self.operationId; diff --git a/lib/bulk/unordered.js b/lib/bulk/unordered.js index 68bd1cf05f..f9cf617c5a 100644 --- a/lib/bulk/unordered.js +++ b/lib/bulk/unordered.js @@ -309,14 +309,13 @@ var UnorderedBulkOperation = function(topology, collection, options) { collection: collection, // Promise Library promiseLibrary: promiseLibrary, - // Bypass validation - bypassDocumentValidation: - typeof options.bypassDocumentValidation === 'boolean' - ? options.bypassDocumentValidation - : false, // check keys checkKeys: typeof options.checkKeys === 'boolean' ? options.checkKeys : true }; + // bypass Validation + if (options.bypassDocumentValidation === true) { + this.s.bypassDocumentValidation = true; + } }; /** @@ -440,6 +439,10 @@ var executeBatch = function(self, batch, options, callback) { finalOptions.writeConcern = self.s.writeConcern; } + if (finalOptions.bypassDocumentValidation !== true) { + delete finalOptions.bypassDocumentValidation; + } + var resultHandler = function(err, result) { // Error is a driver related error not a bulk op error, terminate if ((err && err.driver) || (err && err.message)) { diff --git a/lib/collection.js b/lib/collection.js index 2f5a09841a..01796d6f9f 100644 --- a/lib/collection.js +++ b/lib/collection.js @@ -1720,7 +1720,7 @@ Collection.prototype.aggregate = function(pipeline, options, callback) { decorateWithCollation(command, this, options); // If we have bypassDocumentValidation set - if (typeof options.bypassDocumentValidation === 'boolean') { + if (options.bypassDocumentValidation === true) { command.bypassDocumentValidation = options.bypassDocumentValidation; } diff --git a/lib/operations/collection_ops.js b/lib/operations/collection_ops.js index 5925b89d02..f4a0684d94 100644 --- a/lib/operations/collection_ops.js +++ b/lib/operations/collection_ops.js @@ -491,7 +491,7 @@ function findAndModify(coll, query, sort, doc, options, callback) { } // Have we specified bypassDocumentValidation - if (typeof finalOptions.bypassDocumentValidation === 'boolean') { + if (finalOptions.bypassDocumentValidation === true) { queryObject.bypassDocumentValidation = finalOptions.bypassDocumentValidation; } @@ -874,7 +874,7 @@ function mapReduce(coll, map, reduce, options, callback) { }; // Exclusion list - const exclusionList = ['readPreference', 'session']; + const exclusionList = ['readPreference', 'session', 'bypassDocumentValidation']; // Add any other options passed in for (let n in options) { @@ -909,7 +909,7 @@ function mapReduce(coll, map, reduce, options, callback) { } // Is bypassDocumentValidation specified - if (typeof options.bypassDocumentValidation === 'boolean') { + if (options.bypassDocumentValidation === true) { mapCommandHash.bypassDocumentValidation = options.bypassDocumentValidation; } diff --git a/test/unit/bypass_validation_tests.js b/test/unit/bypass_validation_tests.js new file mode 100644 index 0000000000..a9b5d77ee5 --- /dev/null +++ b/test/unit/bypass_validation_tests.js @@ -0,0 +1,239 @@ +'use strict'; + +const MongoClient = require('../..').MongoClient; +const expect = require('chai').expect; +const mock = require('mongodb-mock-server'); + +describe('bypass document validation', function() { + const test = {}; + beforeEach(() => { + return mock.createServer().then(server => { + test.server = server; + }); + }); + afterEach(() => mock.cleanup()); + + // general test for aggregate function + function testAggregate(config, done) { + const client = new MongoClient(`mongodb://${test.server.uri()}/test`); + let close = e => { + close = () => {}; + client.close(() => done(e)); + }; + + test.server.setMessageHandler(request => { + const doc = request.document; + if (doc.aggregate) { + try { + expect(doc.bypassDocumentValidation).equal(config.expected); + request.reply({ + ok: 1, + cursor: { + firstBatch: [{}], + id: 23, + ns: 'test.test' + } + }); + } catch (e) { + close(e); + } + } + + if (doc.ismaster) { + request.reply(Object.assign({}, mock.DEFAULT_ISMASTER)); + } else if (doc.endSessions) { + request.reply({ ok: 1 }); + } + }); + + client.connect(function(err, client) { + expect(err).to.not.exist; + const db = client.db('test'); + const collection = db.collection('test_c'); + + const options = { bypassDocumentValidation: config.actual }; + + const pipeline = [ + { + $project: {} + } + ]; + collection.aggregate(pipeline, options).next(() => close()); + }); + } + // aggregate + it('should only set bypass document validation if strictly true in aggregate', function(done) { + testAggregate({ expected: true, actual: true }, done); + }); + + it('should not set bypass document validation if not strictly true in aggregate', function(done) { + testAggregate({ expected: undefined, actual: false }, done); + }); + + // general test for mapReduce function + function testMapReduce(config, done) { + const client = new MongoClient(`mongodb://${test.server.uri()}/test`); + let close = e => { + close = () => {}; + client.close(() => done(e)); + }; + + test.server.setMessageHandler(request => { + const doc = request.document; + if (doc.mapreduce) { + try { + expect(doc.bypassDocumentValidation).equal(config.expected); + request.reply({ + results: 't', + ok: 1 + }); + } catch (e) { + close(e); + } + } + + if (doc.ismaster) { + request.reply(Object.assign({}, mock.DEFAULT_ISMASTER)); + } else if (doc.endSessions) { + request.reply({ ok: 1 }); + } + }); + + client.connect(function(err, client) { + expect(err).to.not.exist; + const db = client.db('test'); + const collection = db.collection('test_c'); + + const options = { + out: 'test_c', + bypassDocumentValidation: config.actual + }; + + collection.mapReduce(function map() {}, function reduce() {}, options, e => { + close(e); + }); + }); + } + // map reduce + it('should only set bypass document validation if strictly true in mapReduce', function(done) { + testMapReduce({ expected: true, actual: true }, done); + }); + + it('should not set bypass document validation if not strictly true in mapReduce', function(done) { + testMapReduce({ expected: undefined, actual: false }, done); + }); + + // general test for findAndModify function + function testFindAndModify(config, done) { + const client = new MongoClient(`mongodb://${test.server.uri()}/test`); + let close = e => { + close = () => {}; + client.close(() => done(e)); + }; + + test.server.setMessageHandler(request => { + const doc = request.document; + if (doc.findAndModify) { + try { + expect(doc.bypassDocumentValidation).equal(config.expected); + request.reply({ + ok: 1 + }); + } catch (e) { + close(e); + } + } + + if (doc.ismaster) { + request.reply(Object.assign({}, mock.DEFAULT_ISMASTER)); + } else if (doc.endSessions) { + request.reply({ ok: 1 }); + } + }); + + client.connect(function(err, client) { + expect(err).to.not.exist; + const db = client.db('test'); + const collection = db.collection('test_c'); + + const options = { bypassDocumentValidation: config.actual }; + + collection.findAndModify( + { name: 'Andy' }, + { rating: 1 }, + { $inc: { score: 1 } }, + options, + e => { + close(e); + } + ); + }); + } + // find and modify + it('should only set bypass document validation if strictly true in findAndModify', function(done) { + testFindAndModify({ expected: true, actual: true }, done); + }); + + it('should not set bypass document validation if not strictly true in findAndModify', function(done) { + testFindAndModify({ expected: undefined, actual: false }, done); + }); + + // general test for BlukWrite to test changes made in ordered.js and unordered.js + function testBulkWrite(config, done) { + const client = new MongoClient(`mongodb://${test.server.uri()}/test`); + let close = e => { + close = () => {}; + client.close(() => done(e)); + }; + + test.server.setMessageHandler(request => { + const doc = request.document; + if (doc.insert) { + try { + expect(doc.bypassDocumentValidation).equal(config.expected); + request.reply({ + ok: 1 + }); + } catch (e) { + close(e); + } + } + + if (doc.ismaster) { + request.reply(Object.assign({}, mock.DEFAULT_ISMASTER)); + } else if (doc.endSessions) { + request.reply({ ok: 1 }); + } + }); + + client.connect(function(err, client) { + expect(err).to.not.exist; + const db = client.db('test'); + const collection = db.collection('test_c'); + + const options = { + bypassDocumentValidation: config.actual, + ordered: config.ordered + }; + + collection.bulkWrite([{ insertOne: { document: { a: 1 } } }], options, () => close()); + }); + } + // ordered bulk write, testing change in ordered.js + it('should only set bypass document validation if strictly true in ordered bulkWrite', function(done) { + testBulkWrite({ expected: true, actual: true, ordered: true }, done); + }); + + it('should not set bypass document validation if not strictly true in ordered bulkWrite', function(done) { + testBulkWrite({ expected: undefined, actual: false, ordered: true }, done); + }); + + // unordered bulk write, testing change in ordered.js + it('should only set bypass document validation if strictly true in unordered bulkWrite', function(done) { + testBulkWrite({ expected: true, actual: true, ordered: false }, done); + }); + + it('should not set bypass document validation if not strictly true in unordered bulkWrite', function(done) { + testBulkWrite({ expected: undefined, actual: false, ordered: false }, done); + }); +});