-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
fix: honor journal=true in connection string #2354
Conversation
There was a problem hiding this 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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this 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
lib/connection_string.js
Outdated
@@ -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; |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…gnored-in-connection-string
Description
Ensure the
journal
option returned byparseQueryString
is convertedinto
j
during theconnect
operation. This fixes the issue of thewrite concern not being set on commands where
journal
is only specifiedin the connection string.
NODE-2442
What changed?
journal
withj
in options duringconnect
operationwithMonitoredClient
so it can pass options on tonewClient
Are there any files to ignore?