Skip to content

Commit

Permalink
fix: honor journal=true in connection string
Browse files Browse the repository at this point in the history
Ensure the journal option returned byparseQueryString is converted
into j during the connect operation. This fixes the issue of the
write concern not being set on commands where journal is only
specified in the connection string.

NODE-2422
  • Loading branch information
emadum committed May 7, 2020
1 parent d3bd81f commit 41d291a
Show file tree
Hide file tree
Showing 3 changed files with 79 additions and 2 deletions.
6 changes: 6 additions & 0 deletions lib/operations/connect.js
Expand Up @@ -213,6 +213,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);

Expand Down
34 changes: 32 additions & 2 deletions test/functional/shared.js
Expand Up @@ -86,6 +86,13 @@ function withTempDb(name, options, client, operation, errorHandler) {
);
}

/**
* 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);

Expand Down Expand Up @@ -203,13 +210,29 @@ class EventCollector {
}
}

function withMonitoredClient(commands, callback) {
/**
* 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.queryOptions] connection string options
* @param {object} [options.clientOptions] MongoClient options
* @param {withMonitoredClientCallback} callback the test function
*/
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({}, options.queryOptions),
Object.assign({ monitorCommands: true }, options.clientOptions)
);
const events = [];
client.on('commandStarted', filterForCommands(commands, events));
client.connect((err, client) => {
Expand All @@ -222,6 +245,13 @@ function withMonitoredClient(commands, 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,
Expand Down
41 changes: 41 additions & 0 deletions 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() {
Expand All @@ -16,4 +19,42 @@ describe('Write Concern', function() {

generateTopologyTests(testSuites, testContext);
});

// TODO: once `read-write-concern/connection-string` spec tests are implemented these can likely be removed
describe('test journal connection string option', function() {
function journalOptionTest(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();
});
}

// baseline to confirm client option is working
it(
'should set write concern with j: true client option',
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 } }, journalOptionTest)
);
});
});

0 comments on commit 41d291a

Please sign in to comment.