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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update MongoDB driver to 3.0.5 #9790

Merged
merged 4 commits into from Apr 6, 2018

Conversation

@klaussner
Copy link
Member

@klaussner klaussner commented Apr 1, 2018

This PR updates the MongoDB driver from 2.2.34 to 3.0.5 and addresses the breaking changes described in CHANGES_3.0.0.md (node-mongodb-native) and meteor/meteor-feature-requests#268 (comment).

Summary of the changes:

  • Create the Db instance manually because MongoClient passes a client instance instead of a database to the callback function. I'm using the undocumented client.s.options.dbName property to get the database name because I couldn't find better way to access it without parsing the URL in the MongoConnection constructor.

  • Use Cursor.prototype.next instead of Cursor.prototype.nextObject (deprecated in version 2 and removed in version 3).

  • Use client instead of db to close the connection in MongoConnection.prototype.close.

  • Use the projection option instead of the fields parameter in MongoConnection.prototype._createSynchronousCursor.

I'll bump the versions and add an entry to History.md when the PR is ready to be approved. 馃檪 馃

Fixes meteor/meteor-feature-requests#268.

klaussner added 3 commits Apr 1, 2018
@klaussner klaussner force-pushed the klaussner:mongo-driver-update branch to 9594631 Apr 2, 2018
@klaussner
Copy link
Member Author

@klaussner klaussner commented Apr 2, 2018

Update:

Meteor Tool

The Meteor Tool used new Db(...).open() (removed in the new driver version) to connect to the database, so I changed it to use the new MongoClient construct instead.

Database Name

A fix for the database name problem will be included in the next version of node-mongodb-native (3.0.6). Calling client.db() (without the client.s.options.dbName argument) will create a Db object for the database from the connection URL, so we don't have to rely on an undocumented property.

MongoDB Errors

There are two locations in the Meteor codebase (accounts-base & allow-deny) where an error's name property is checked to find out if the error was thrown by MongoDB. In previous versions of the driver, most (or all?) MongoDB errors had name === "MongoError" but the new version sometimes throws errors with name === "BulkWriteError", for example, if a duplicate key is detected when calling insert. I'm not sure why the driver throws a BulkWriteError in this case because insert is not a bulk operation. 馃

I'm comparing name with both error names for now (5286d14) but maybe we can find a more robust way to identify MongoDB errors without checking the name property.

new Server('127.0.0.1', options.port, {
poolSize: 1,
socketOptions: {
connectTimeoutMS: 60000
}
}),
{ safe: true }

This comment has been minimized.

@klaussner

klaussner Apr 2, 2018
Author Member

The safe option of the Db constructor was deprecated a long time ago (in driver version 1) and is no longer implemented in versions 2 and 3. 馃懙

This comment has been minimized.

@abernix

abernix Apr 4, 2018
Member

I know it was removed from the documentation in the Node driver long ago, but I'm concerned that this still isn't a no-op. Unfortunately, I can't find the exact scenario where I looked into this, but I do remember digging into this in the past (with a similar revelation around the seeming deprecation of the safe option) long ago and being concerned by the fact that there seemed to be some support for it still in the connection string URI (for example).

The safe option once related to the write-concern, but I'm not completely confident in removing this if it's possible it's still being passed through to Mongo itself in some way. Can we confirm that it's not affecting the replica set configuration in any way (for example, does Mongo directly copy these to the connection string somewhere?) Do we need to more explicitly set a write-concern if we remove this? Should that be left as an implementation exercise to the developer?

This comment has been minimized.

@abernix

abernix Apr 6, 2018
Member

Ok, it appears that there's no way that safe is heading upstream to the core Mongo driver. I've concluded this based on this line in the Mongo Node driver Db instantiation which uses a filterOptions with a so-called "legalOptions" whitelist which is defined here and has no sign of safe.

This comment has been minimized.

@klaussner

klaussner Apr 7, 2018
Author Member

That's good to know! I totally overlooked the filterOptions call. 馃憖

I'm not sure if I'm interpreting the docs and the driver's commit history correctly, but it looks like the safe option was replaced with w = 1, which is now the default write concern, so all operations should be "safe" automatically.

@benjamn benjamn added this to the Release 1.6.2 milestone Apr 4, 2018
@benjamn
Copy link
Member

@benjamn benjamn commented Apr 6, 2018

@klaussner Can you add a bullet point about this change to History.md?

Done: e1614c0

@benjamn benjamn changed the base branch from devel to release-1.6.2 Apr 6, 2018
@benjamn benjamn merged commit c0bce71 into meteor:release-1.6.2 Apr 6, 2018
18 checks passed
18 checks passed
CLA Author has signed the Meteor CLA.
Details
ci/circleci: Clean Up Your tests passed on CircleCI!
Details
ci/circleci: Docs Your tests passed on CircleCI!
Details
ci/circleci: Get Ready Your tests passed on CircleCI!
Details
ci/circleci: Isolated Tests Your tests passed on CircleCI!
Details
ci/circleci: Test Group 0 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 1 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 10 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 2 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 3 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 4 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 5 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 6 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 7 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 8 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 9 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@klaussner
Copy link
Member Author

@klaussner klaussner commented Apr 7, 2018

Thanks! Should I bump the version numbers of the changed packages (mongo, accounts-base, and allow-deny)?

@hwillson hwillson mentioned this pull request Apr 11, 2018
benjamn added a commit that referenced this pull request Apr 19, 2018
Fixes #9827.

The client.s.databaseName property appears to have been introduced a long
time ago (in 2014), so it seems reliable:
mongodb/node-mongodb-native@14fd60b

The use of client.s.options.dbName was only recently introduced in #9790,
since the MongoDB.connect callback now receives a MongoClient object
rather than a Db object. Not sure if client.s.options.dbName was ever
reliable, but at least we know when the problem started.
@abernix
Copy link
Member

@abernix abernix commented May 22, 2018

@klaussner Did you have an opportunity to look at how connectivity worked with pre-3.6 Mongo deployments? Particularly curious if we should be concerned by the comment raised in #9632 (comment).

@klaussner
Copy link
Member Author

@klaussner klaussner commented May 22, 2018

It should work with all Mongo deployments because the latest driver supports all versions from 2.6 to 3.6. I made a few quick tests with two older versions (2.6 and 3.4) and didn't notice any problems. 馃

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

Successfully merging this pull request may close these issues.

None yet

3 participants
You can鈥檛 perform that action at this time.