Skip to content

fix: honor journal=true in connection string #2354

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

Merged
merged 12 commits into from
May 7, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions lib/operations/connect.js
Original file line number Diff line number Diff line change
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;
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
@@ -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)
);
});
});