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

Conversation

emadum
Copy link
Contributor

@emadum emadum commented May 6, 2020

Description

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-2442

What changed?

  • replace journal with j in options during connect operation
  • updated test helpers
    • refactored withMonitoredClient so it can pass options on to newClient
    • added some jsdoc to helpers for better code completion when writing tests

Are there any files to ignore?

@emadum emadum marked this pull request as ready for review May 6, 2020 01:41
@emadum emadum requested review from mbroadst and reggi May 6, 2020 12:14
Copy link
Contributor

@reggi reggi left a comment

Choose a reason for hiding this comment

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

LGTM, I like the additions to the monitorClient

Copy link
Contributor

@reggi reggi left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@mbroadst mbroadst left a comment

Choose a reason for hiding this comment

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

LGTM with a small nit

@@ -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.

@@ -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.

@emadum emadum merged commit 41d291a into master May 7, 2020
@emadum emadum deleted the NODE-2422/journal-ignored-in-connection-string branch May 7, 2020 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants