Skip to content
This repository was archived by the owner on May 17, 2021. It is now read-only.

fix: Explicitly pass read preference with db.command commands COMPASS-4539 #282

Merged
merged 4 commits into from
Dec 15, 2020
Merged

fix: Explicitly pass read preference with db.command commands COMPASS-4539 #282

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f550abb
Explicitly pass read preference with db.command commands
Anemy Dec 15, 2020
9114386
Remove duplicated read preference option creation
Anemy Dec 15, 2020
8c0dba9
Bump connection model and better peer deps
Anemy Dec 15, 2020
93f6bb7
Change connection model peer dep to *
Anemy Dec 15, 2020
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
42 changes: 25 additions & 17 deletions lib/instance-detail-helper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,13 @@ const {

const debug = require('debug')('mongodb-data-service:instance-detail-helper');

function getReadPreferenceOptions(db) {
Copy link
Contributor

@mcasimir mcasimir Dec 15, 2020

Choose a reason for hiding this comment

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

Isn't db.s.readPreference a private method in the driver?

Also .. I think here we can just hardcode ReadPreference.PRIMARY_PREFERRED, i see no harm, may be missing something

Copy link
Member Author

@Anemy Anemy Dec 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, wrote this quick using a bit of what we already had https://github.com/mongodb-js/data-service/blob/master/lib/instance-detail-helper.js#L500 , I'll see if there's a public method we can use. Looks like we don't pass our connection model through these instance detail helpers.

// `db.command` does not use the read preference set on the
// connection, so here we explicitly to specify it in the options.
const readPreference = db.s ? db.s.readPreference : ReadPreference.PRIMARY;
return { readPreference };
}

/**
* aggregates stats across all found databases
* @param {Object} results async.auto results
Expand Down Expand Up @@ -78,7 +85,7 @@ function getBuildInfo(results, done) {
};

const adminDb = db.databaseName === 'admin' ? db : db.admin();
adminDb.command(spec, {}, function(err, res) {
adminDb.command(spec, getReadPreferenceOptions(db), function(err, res) {
if (err) {
// buildInfo doesn't require any privileges to run, so if it fails,
// something really went wrong and we should return the error.
Expand All @@ -98,7 +105,7 @@ function getCmdLineOpts(results, done) {
};

const adminDb = db.databaseName === 'admin' ? db : db.admin();
adminDb.command(spec, {}, function(err, res) {
adminDb.command(spec, getReadPreferenceOptions(db), function(err, res) {
if (err) {
debug('getCmdLineOpts failed!', err);
return done(null, { errmsg: err.message });
Expand Down Expand Up @@ -213,7 +220,7 @@ function getHostInfo(results, done) {
};

const adminDb = db.databaseName === 'admin' ? db : db.admin();
adminDb.command(spec, {}, function(err, res) {
adminDb.command(spec, getReadPreferenceOptions(db), function(err, res) {
if (err) {
if (isNotAuthorized(err)) {
// if the error is that the user is not authorized, silently ignore it
Expand Down Expand Up @@ -253,12 +260,14 @@ function listDatabases(results, done) {
listDatabases: 1
};

const options = getReadPreferenceOptions(db);

const adminDb = db.databaseName === 'admin' ? db : db.admin();
adminDb.command(spec, {}, function(err, res) {
adminDb.command(spec, options, function(err, res) {
if (err) {
if (isNotAuthorized(err)) {
// eslint-disable-next-line no-shadow
adminDb.command({connectionStatus: 1, showPrivileges: 1}, {}, function(err, res) {
adminDb.command({connectionStatus: 1, showPrivileges: 1}, options, function(err, res) {
if (err) {
done(err);
return;
Expand Down Expand Up @@ -343,7 +352,7 @@ function getDatabase(client, db, name, done) {
dbStats: 1
};

const options = {};
const options = getReadPreferenceOptions(db);
debug('running dbStats for `%s`...', name);
client.db(name).command(spec, options, function(err, res) {
if (err) {
Expand Down Expand Up @@ -376,8 +385,15 @@ function getDatabases(results, done) {
function getUserInfo(results, done) {
const db = results.db;

const options = getReadPreferenceOptions(db);

// get the user privileges
db.command({ connectionStatus: 1, showPrivileges: true }, {}, function(
db.command({
connectionStatus: 1,
showPrivileges: true
},
options,
function(
err,
res
) {
Expand All @@ -394,7 +410,7 @@ function getUserInfo(results, done) {
}
const user = res.authInfo.authenticatedUsers[0];

db.command({ usersInfo: user, showPrivileges: true }, {}, function(
db.command({ usersInfo: user, showPrivileges: true }, options, function(
_err,
_res
) {
Expand Down Expand Up @@ -491,15 +507,7 @@ function getDatabaseCollections(db, done) {

const spec = {};

/**
* @note: Durran: For some reason the listCollections call does not take into
* account the read preference that was set on the db instance - it only looks
* in the passed options: https://github.com/mongodb/node-mongodb-native/blob/2.2/lib/db.js#L671
*/
const rp = db.s ? db.s.readPreference : ReadPreference.PRIMARY;
const options = { readPreference: rp };

db.listCollections(spec, options).toArray(function(err, res) {
db.listCollections(spec, getReadPreferenceOptions(db)).toArray(function(err, res) {
if (err) {
if (isNotAuthorized(err) || isMongosLocalException(err)) {
// if the error is that the user is not authorized, silently ignore it
Expand Down
6 changes: 5 additions & 1 deletion lib/native-client.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,11 @@ class NativeClient extends EventEmitter {
* @param {Function} callback - The callback.
*/
listDatabases(callback) {
this.database.admin().command({ listDatabases: 1 }, {}, (error, result) => {
this.database.admin().command({
listDatabases: 1
}, {
readPreference: this.model.readPreference
Copy link
Contributor

@mcasimir mcasimir Dec 15, 2020

Choose a reason for hiding this comment

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

this also depends on how is used, if is only used by compass for building the UI i'd go with primary preferred always, what would be the consequence of listing dbs from a secondary?

}, (error, result) => {
if (error) {
return callback(this._translateMessage(error));
}
Expand Down
7 changes: 3 additions & 4 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 3 additions & 3 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@
"check": "mongodb-js-precommit"
},
"peerDependencies": {
"mongodb": "^3.6.3",
"mongodb-connection-model": "^17.3.0"
"mongodb": "3.x",
"mongodb-connection-model": "*"
},
"dependencies": {
"async": "^3.2.0",
Expand All @@ -41,7 +41,7 @@
"mocha": "^8.2.1",
"mock-require": "^3.0.3",
"mongodb": "^3.6.3",
"mongodb-connection-model": "^17.3.0",
"mongodb-connection-model": "^18.0.0",
"mongodb-js-precommit": "^2.2.1",
"mongodb-runner": "^4.8.0",
"xvfb-maybe": "^0.2.1"
Expand Down
38 changes: 38 additions & 0 deletions test/instance-detail-helper-mocked.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
const assert = require('assert');
const { ReadPreference } = require('mongodb');
const {
getAllowedCollections,
getAllowedDatabases,
Expand Down Expand Up @@ -383,6 +384,43 @@ describe('instance-detail-helper-mocked', function() {
done();
});
});
it('should pass the read preference explicity when it is set', function(done) {
let passedOptions;
results.db = makeMockDB(function(times, command, options, callback) {
if (command.listDatabases) {
passedOptions = options;
return callback(null, { databases: [] });
}
return callback(null, {});
});
results.db.s = {
readPreference: ReadPreference.SECONDARY
};

listDatabases(results, function() {
assert.deepEqual(passedOptions, {
readPreference: 'secondary'
});
done();
});
});
it('defaults to passing the read preference as PRIMARY', function(done) {
let passedOptions;
results.db = makeMockDB(function(times, command, options, callback) {
if (command.listDatabases) {
passedOptions = options;
return callback(null, { databases: [] });
}
return callback(null, {});
});

listDatabases(results, function() {
assert.deepEqual(passedOptions, {
readPreference: ReadPreference.PRIMARY
});
done();
});
});
it('should pass on other errors from the listDatabases command', function(done) {
// instead of the real db handle, pass in the mocked one
results.db = makeMockDB(
Expand Down