Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: honor journal=true in connection string #2354

Merged
merged 12 commits into from May 7, 2020
2 changes: 1 addition & 1 deletion lib/connection_string.js
Expand Up @@ -427,7 +427,7 @@ function parseQueryString(query, options) {

// special cases for known deprecated options
if (result.wtimeout && result.wtimeoutms) {
delete result.wtimeout;
result.wtimeout = undefined;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is counter to what I've said previously, but in these non-critical paths I think there's actual value to outright removing a known bad value. In the future we might solve this by "plucking" all the values we care about into a new object.

console.warn('Unsupported option `wtimeout` specified');
}

Expand Down
6 changes: 6 additions & 0 deletions lib/operations/connect.js
Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to me it seems like j is the odd man out here, shouldn't we filter that out and depend on journal throughout the codebase? j is just the field on the command send to the server.

UPDATE: I now see that this is a pretty small change, while what I'm proposing would potentially touch much more of the codebase. Maybe we can defer my suggestion here to @reggi s work on MongoClientOptions

Copy link
Contributor Author

@emadum emadum May 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think this is the minimal change necessary to fix this issue; it's probably not worth diving deep on this now when we're already planning on overhauling this entire options system; definitely worth addressing when we do the overhaul.

Copy link
Member

@mbroadst mbroadst May 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw I think the original report is for 3.5, so we'll need a port there too

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_finalOptions.journal = undefined;
}

// resolve tls options if needed
resolveTLSOptions(_finalOptions);

Expand Down
34 changes: 32 additions & 2 deletions test/functional/shared.js
Expand Up @@ -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);

Expand Down Expand Up @@ -192,13 +199,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({ monitorCommands: true }, options.queryOptions),
Object.assign({}, options.clientOptions)
);
const events = [];
client.on('commandStarted', filterForCommands(commands, events));
client.connect((err, client) => {
Expand All @@ -211,6 +234,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)
);
});
});