From 7adef2286ea0cb1c9c7f49146ac06113c1ac1af4 Mon Sep 17 00:00:00 2001 From: emadum Date: Tue, 5 May 2020 20:38:27 -0400 Subject: [PATCH 01/11] add tests demonstrating failure --- test/functional/shared.js | 11 ++++++-- test/functional/write_concern.test.js | 39 +++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 2 deletions(-) diff --git a/test/functional/shared.js b/test/functional/shared.js index 0e6f526ae7..a891d65071 100644 --- a/test/functional/shared.js +++ b/test/functional/shared.js @@ -192,13 +192,20 @@ class EventCollector { } } -function withMonitoredClient(commands, callback) { +function withMonitoredClient(commands, options, callback) { + if (arguments.length === 2) { + callback = options; + options = {}; + } if (!Object.prototype.hasOwnProperty.call(callback, 'prototype')) { throw new Error('withMonitoredClient callback can not be arrow function'); } return function(done) { const configuration = this.configuration; - const client = configuration.newClient({ monitorCommands: true }); + const client = configuration.newClient( + Object.assign({ monitorCommands: true }, options.dbOptions), + Object.assign({}, options.serverOptions) + ); const events = []; client.on('commandStarted', filterForCommands(commands, events)); client.connect((err, client) => { diff --git a/test/functional/write_concern.test.js b/test/functional/write_concern.test.js index 5e0ac4e0a8..89ea601caf 100644 --- a/test/functional/write_concern.test.js +++ b/test/functional/write_concern.test.js @@ -1,8 +1,11 @@ 'use strict'; +const chai = require('chai'); +const expect = chai.expect; const TestRunnerContext = require('./spec-runner').TestRunnerContext; const generateTopologyTests = require('./spec-runner').generateTopologyTests; const loadSpecTests = require('../spec').loadSpecTests; +const { withMonitoredClient } = require('./shared'); describe('Write Concern', function() { describe('spec tests', function() { @@ -16,4 +19,40 @@ describe('Write Concern', function() { generateTopologyTests(testSuites, testContext); }); + + // TODO - implement `read-write-concern/connection-string` spec tests + describe.only('test journal connection string option', function() { + const dbOptions = { journal: true }; + const serverOptions = { j: true }; + function writeConcernJournalOptionTest(client, events, done) { + expect(client).to.have.nested.property('s.options'); + const clientOptions = client.s.options; + expect(clientOptions).to.containSubset({ j: true }); + client + .db('test') + .collection('test') + .insertOne({ a: 1 }, (err, result) => { + expect(err).to.not.exist; + expect(result).to.exist; + expect(events) + .to.be.an('array') + .with.lengthOf(1); + expect(events[0]).to.containSubset({ + commandName: 'insert', + command: { + writeConcern: { j: true } + } + }); + done(); + }); + } + it( + 'should set write concern with journal=true connection string option', + withMonitoredClient('insert', { dbOptions }, writeConcernJournalOptionTest) + ); + it( + 'should set write concern with j: true client option', + withMonitoredClient('insert', { serverOptions }, writeConcernJournalOptionTest) + ); + }); }); From de0c7e185b16d03436fec48e40d4cc43f884d760 Mon Sep 17 00:00:00 2001 From: emadum Date: Tue, 5 May 2020 20:45:52 -0400 Subject: [PATCH 02/11] remove .only --- test/functional/write_concern.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/functional/write_concern.test.js b/test/functional/write_concern.test.js index 89ea601caf..1cfef33664 100644 --- a/test/functional/write_concern.test.js +++ b/test/functional/write_concern.test.js @@ -21,7 +21,7 @@ describe('Write Concern', function() { }); // TODO - implement `read-write-concern/connection-string` spec tests - describe.only('test journal connection string option', function() { + describe('test journal connection string option', function() { const dbOptions = { journal: true }; const serverOptions = { j: true }; function writeConcernJournalOptionTest(client, events, done) { From 95e109ba0b8e9b6889400f6fd292d8e88ea03570 Mon Sep 17 00:00:00 2001 From: emadum Date: Tue, 5 May 2020 21:13:38 -0400 Subject: [PATCH 03/11] translate 'journal' query option to 'j' for client --- lib/connection_string.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/lib/connection_string.js b/lib/connection_string.js index ceda32d6e5..257b7df03a 100644 --- a/lib/connection_string.js +++ b/lib/connection_string.js @@ -431,6 +431,12 @@ function parseQueryString(query, options) { console.warn('Unsupported option `wtimeout` specified'); } + // journal should be translated to j for the driver + if (result.journal) { + result.j = result.journal; + result.journal = undefined; + } + return Object.keys(result).length ? result : null; } From bb577d290e05584bb2babe19f630a8242703d310 Mon Sep 17 00:00:00 2001 From: emadum Date: Wed, 6 May 2020 06:16:35 -0400 Subject: [PATCH 04/11] cleanup --- lib/connection_string.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/connection_string.js b/lib/connection_string.js index 257b7df03a..773c22c04a 100644 --- a/lib/connection_string.js +++ b/lib/connection_string.js @@ -427,12 +427,12 @@ function parseQueryString(query, options) { // special cases for known deprecated options if (result.wtimeout && result.wtimeoutms) { - delete result.wtimeout; + result.wtimeout = undefined; console.warn('Unsupported option `wtimeout` specified'); } - // journal should be translated to j for the driver - if (result.journal) { + // `journal` should be translated to `j` for the driver + if (result.journal != null) { result.j = result.journal; result.journal = undefined; } From 4025139a885f4859a64c88a5f5a92ce088f582e5 Mon Sep 17 00:00:00 2001 From: emadum Date: Wed, 6 May 2020 07:35:55 -0400 Subject: [PATCH 05/11] move fix to correct place --- lib/connection_string.js | 6 ------ lib/operations/connect.js | 6 ++++++ 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/connection_string.js b/lib/connection_string.js index 773c22c04a..c55f56fd65 100644 --- a/lib/connection_string.js +++ b/lib/connection_string.js @@ -431,12 +431,6 @@ function parseQueryString(query, options) { console.warn('Unsupported option `wtimeout` specified'); } - // `journal` should be translated to `j` for the driver - if (result.journal != null) { - result.j = result.journal; - result.journal = undefined; - } - return Object.keys(result).length ? result : null; } diff --git a/lib/operations/connect.js b/lib/operations/connect.js index a4802bda0c..385e45b27c 100644 --- a/lib/operations/connect.js +++ b/lib/operations/connect.js @@ -212,6 +212,12 @@ function connect(mongoClient, url, options, callback) { delete _finalOptions.db_options.auth; } + // `journal` should be translated to `j` for the driver + if (_finalOptions.journal != null) { + _finalOptions.j = _finalOptions.journal; + _finalOptions.journal = undefined; + } + // resolve tls options if needed resolveTLSOptions(_finalOptions); From 0cedc6c3cb4247675e89e4facdab4bd9e673f60e Mon Sep 17 00:00:00 2001 From: emadum Date: Wed, 6 May 2020 08:13:07 -0400 Subject: [PATCH 06/11] [skip ci] doc: add some jsdoc --- test/functional/shared.js | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/test/functional/shared.js b/test/functional/shared.js index a891d65071..26b91224f0 100644 --- a/test/functional/shared.js +++ b/test/functional/shared.js @@ -75,6 +75,13 @@ function makeCleanupFn(client) { }; } +/** + * Safely perform a test with provided MongoClient, ensuring client won't leak. + * + * @param {MongoClient} client + * @param {Function|Promise} operation + * @param {Function|Promise} [errorHandler] + */ function withClient(client, operation, errorHandler) { const cleanup = makeCleanupFn(client); @@ -192,6 +199,15 @@ class EventCollector { } } +/** + * Perform a test with a monitored MongoClient that will filter for certain commands. + * + * @param {string|Array} commands commands to filter for + * @param {object} [options] options to pass on to configuration.newClient + * @param {object} [options.dbOptions] connection string options + * @param {object} [options.serverOptions] MongoClient options + * @param {withMonitoredClientCallback} callback the test function + */ function withMonitoredClient(commands, options, callback) { if (arguments.length === 2) { callback = options; @@ -218,6 +234,13 @@ function withMonitoredClient(commands, options, callback) { }; } +/** + * @callback withMonitoredClientCallback + * @param {MongoClient} client monitored client + * @param {Array} events record of monitored commands + * @param {Function} done trigger end of test and cleanup + */ + module.exports = { connectToDb, setupDatabase, From a020697f968070972e4f28b067f056d386e074d0 Mon Sep 17 00:00:00 2001 From: emadum Date: Wed, 6 May 2020 14:15:33 -0400 Subject: [PATCH 07/11] cleanup --- test/functional/write_concern.test.js | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/test/functional/write_concern.test.js b/test/functional/write_concern.test.js index 1cfef33664..974211a5ff 100644 --- a/test/functional/write_concern.test.js +++ b/test/functional/write_concern.test.js @@ -22,8 +22,6 @@ describe('Write Concern', function() { // TODO - implement `read-write-concern/connection-string` spec tests describe('test journal connection string option', function() { - const dbOptions = { journal: true }; - const serverOptions = { j: true }; function writeConcernJournalOptionTest(client, events, done) { expect(client).to.have.nested.property('s.options'); const clientOptions = client.s.options; @@ -46,13 +44,18 @@ describe('Write Concern', function() { done(); }); } + + // baseline to confirm client option is working it( - 'should set write concern with journal=true connection string option', - withMonitoredClient('insert', { dbOptions }, writeConcernJournalOptionTest) + 'should set write concern with j: true client option', + withMonitoredClient('insert', { j: true }, writeConcernJournalOptionTest) ); + + // ensure query option in connection string passes through it( - 'should set write concern with j: true client option', - withMonitoredClient('insert', { serverOptions }, writeConcernJournalOptionTest) + 'should set write concern with journal=true connection string option', + withMonitoredClient('insert', { journal: true }, writeConcernJournalOptionTest) ); + }); }); From 8d8db4539564b783acda11c2c4188cca54945858 Mon Sep 17 00:00:00 2001 From: emadum Date: Wed, 6 May 2020 14:29:20 -0400 Subject: [PATCH 08/11] cleanup^2 --- test/functional/write_concern.test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/functional/write_concern.test.js b/test/functional/write_concern.test.js index 974211a5ff..16a21a38fc 100644 --- a/test/functional/write_concern.test.js +++ b/test/functional/write_concern.test.js @@ -48,13 +48,13 @@ describe('Write Concern', function() { // baseline to confirm client option is working it( 'should set write concern with j: true client option', - withMonitoredClient('insert', { j: true }, writeConcernJournalOptionTest) + withMonitoredClient('insert', { serverOptions: { j: true } }, writeConcernJournalOptionTest) ); // ensure query option in connection string passes through it( 'should set write concern with journal=true connection string option', - withMonitoredClient('insert', { journal: true }, writeConcernJournalOptionTest) + withMonitoredClient('insert', { dbOptions: { journal: true } }, writeConcernJournalOptionTest) ); }); From ea7781db3cf9999edf8e0399d43cd30f082390e8 Mon Sep 17 00:00:00 2001 From: emadum Date: Wed, 6 May 2020 14:36:17 -0400 Subject: [PATCH 09/11] better names for options --- test/functional/shared.js | 8 ++++---- test/functional/write_concern.test.js | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/test/functional/shared.js b/test/functional/shared.js index 26b91224f0..4e741dd8f6 100644 --- a/test/functional/shared.js +++ b/test/functional/shared.js @@ -204,8 +204,8 @@ class EventCollector { * * @param {string|Array} commands commands to filter for * @param {object} [options] options to pass on to configuration.newClient - * @param {object} [options.dbOptions] connection string options - * @param {object} [options.serverOptions] MongoClient options + * @param {object} [options.queryOptions] connection string options + * @param {object} [options.clientOptions] MongoClient options * @param {withMonitoredClientCallback} callback the test function */ function withMonitoredClient(commands, options, callback) { @@ -219,8 +219,8 @@ function withMonitoredClient(commands, options, callback) { return function(done) { const configuration = this.configuration; const client = configuration.newClient( - Object.assign({ monitorCommands: true }, options.dbOptions), - Object.assign({}, options.serverOptions) + Object.assign({ monitorCommands: true }, options.queryOptions), + Object.assign({}, options.clientOptions) ); const events = []; client.on('commandStarted', filterForCommands(commands, events)); diff --git a/test/functional/write_concern.test.js b/test/functional/write_concern.test.js index 16a21a38fc..d4b5c1afef 100644 --- a/test/functional/write_concern.test.js +++ b/test/functional/write_concern.test.js @@ -48,13 +48,13 @@ describe('Write Concern', function() { // baseline to confirm client option is working it( 'should set write concern with j: true client option', - withMonitoredClient('insert', { serverOptions: { j: true } }, writeConcernJournalOptionTest) + withMonitoredClient('insert', { clientOptions: { j: true } }, writeConcernJournalOptionTest) ); // ensure query option in connection string passes through it( 'should set write concern with journal=true connection string option', - withMonitoredClient('insert', { dbOptions: { journal: true } }, writeConcernJournalOptionTest) + withMonitoredClient('insert', { queryOptions: { journal: true } }, writeConcernJournalOptionTest) ); }); From e703f23dfc2809ffec12c1192e06f7607769042d Mon Sep 17 00:00:00 2001 From: emadum Date: Wed, 6 May 2020 17:05:28 -0400 Subject: [PATCH 10/11] fix lint error --- test/functional/write_concern.test.js | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/test/functional/write_concern.test.js b/test/functional/write_concern.test.js index d4b5c1afef..19b7523261 100644 --- a/test/functional/write_concern.test.js +++ b/test/functional/write_concern.test.js @@ -20,9 +20,9 @@ describe('Write Concern', function() { generateTopologyTests(testSuites, testContext); }); - // TODO - implement `read-write-concern/connection-string` spec tests + // TODO: once `read-write-concern/connection-string` spec tests are implemented these can likely be removed describe('test journal connection string option', function() { - function writeConcernJournalOptionTest(client, events, done) { + function journalOptionTest(client, events, done) { expect(client).to.have.nested.property('s.options'); const clientOptions = client.s.options; expect(clientOptions).to.containSubset({ j: true }); @@ -48,14 +48,13 @@ describe('Write Concern', function() { // baseline to confirm client option is working it( 'should set write concern with j: true client option', - withMonitoredClient('insert', { clientOptions: { j: true } }, writeConcernJournalOptionTest) + withMonitoredClient('insert', { clientOptions: { j: true } }, journalOptionTest) ); // ensure query option in connection string passes through it( 'should set write concern with journal=true connection string option', - withMonitoredClient('insert', { queryOptions: { journal: true } }, writeConcernJournalOptionTest) + withMonitoredClient('insert', { queryOptions: { journal: true } }, journalOptionTest) ); - }); }); From ebf6ca3f06ec77b3c6116a3672bc4a9e36e0673b Mon Sep 17 00:00:00 2001 From: emadum Date: Thu, 7 May 2020 18:03:35 -0400 Subject: [PATCH 11/11] cleanup --- lib/connection_string.js | 2 +- test/functional/shared.js | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/connection_string.js b/lib/connection_string.js index c55f56fd65..ceda32d6e5 100644 --- a/lib/connection_string.js +++ b/lib/connection_string.js @@ -427,7 +427,7 @@ function parseQueryString(query, options) { // special cases for known deprecated options if (result.wtimeout && result.wtimeoutms) { - result.wtimeout = undefined; + delete result.wtimeout; console.warn('Unsupported option `wtimeout` specified'); } diff --git a/test/functional/shared.js b/test/functional/shared.js index 4e741dd8f6..424f1d5c99 100644 --- a/test/functional/shared.js +++ b/test/functional/shared.js @@ -219,8 +219,8 @@ function withMonitoredClient(commands, options, callback) { return function(done) { const configuration = this.configuration; const client = configuration.newClient( - Object.assign({ monitorCommands: true }, options.queryOptions), - Object.assign({}, options.clientOptions) + Object.assign({}, options.queryOptions), + Object.assign({ monitorCommands: true }, options.clientOptions) ); const events = []; client.on('commandStarted', filterForCommands(commands, events));