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: unmerged writeConcern options #2744

Closed
wants to merge 1 commit into from
Closed

Conversation

wolrajhti
Copy link

@wolrajhti wolrajhti commented Feb 18, 2021

Description

In 3.6.4 was added a depreciation warning about how writeConcern options should be formatted

console.warn(
`Top-level use of w, wtimeout, j, and fsync is deprecated. Use writeConcern instead.`
);

#2743 solves the problem of multiple emitions of depreciation warning, but digging into the code to find why this warning is triggered so many times, I found that sourceOptions.writeConcern is never merged into targetOptions.writeConcern in utils.mergeOptionsAndWriteConcern

// Write concern keys
var writeConcernKeys = ['w', 'j', 'wtimeout', 'fsync'];
// Merge the write concern options
var mergeOptionsAndWriteConcern = function(targetOptions, sourceOptions, keys, mergeWriteConcern) {
// Mix in any allowed options
for (var i = 0; i < keys.length; i++) {
if (!targetOptions[keys[i]] && sourceOptions[keys[i]] !== undefined) {
targetOptions[keys[i]] = sourceOptions[keys[i]];
}
}
// No merging of write concern
if (!mergeWriteConcern) return targetOptions;
// Found no write Concern options
var found = false;
for (i = 0; i < writeConcernKeys.length; i++) {
if (targetOptions[writeConcernKeys[i]]) {
found = true;
break;
}
}
if (!found) {
for (i = 0; i < writeConcernKeys.length; i++) {
if (sourceOptions[writeConcernKeys[i]]) {
targetOptions[writeConcernKeys[i]] = sourceOptions[writeConcernKeys[i]];
}
}
}
return targetOptions;
};

To fix it, my proposal is to add writeConcern to the collectionKeys in db.js. Also, utils.mergeOptionsAndWriteConcern is not part of the public API and used only 1 time in db.js, so I propose to remove the trailing, always true, argument : mergeWriteConcern

The original bug was reported here: https://jira.mongodb.org/browse/NODE-3114

@nodesocket
Copy link

@wolrajhti thanks so much for this work. Look forward to this fix merged and a new release.

wolrajhti referenced this pull request Mar 3, 2021
Updates all logging statements to use node's process.emitWarning API.
Allows for filtering, capturing and silencing of warnings.

NODE-2317, NODE-3114
@nbbeeken
Copy link
Contributor

nbbeeken commented Mar 3, 2021

Thanks again for the contribution!
Closing as #2743 contained the fix proposed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants