search

digitalbazaar / bedrock-mongodb

Bedrock mongodb module
Apache License 2.0
2 stars 3 forks source link

Driver3 #33

Closed aljones15 closed 4 years ago

aljones15 commented 4 years ago

As of June 3 2020 important top level app(s) are running just fine with this.

This PR is breaking for the following libraries:

There are multiple other Draft PRs awaiting this release:

any library that passes 3 parameters to mongo's find function (ex: find(query, options, fields)) will be broken after this release.

Ensure all of these deprecated methods are not being used:

Issues Addressed: https://github.com/digitalbazaar/bedrock-mongodb/issues/19 Update to Driver 3.x https://github.com/digitalbazaar/bedrock-mongodb/issues/35 useUnifiedTopology: true https://github.com/digitalbazaar/bedrock-mongodb/issues/36 runOnceAsync removed https://github.com/digitalbazaar/bedrock-mongodb/issues/34 remove idGen & migration

codecov-commenter commented 4 years ago

Codecov Report

Merging #33 into master will increase coverage by 16.44%. The diff coverage is 57.40%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #33       +/-   ##
===========================================
+ Coverage   35.76%   52.20%   +16.44%     
===========================================
  Files           5        4        -1     
  Lines         576      385      -191     
===========================================
- Hits          206      201        -5     
+ Misses        370      184      -186
Impacted Files Coverage Δ
lib/index.js 47.27% <56.60%> (+4.08%) :arrow_up:
lib/config.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 8fbfcb0...e3bda9d. Read the comment docs.

mattcollier commented 4 years ago

Deprecation warning when using this new stuff:

(node:25866) DeprecationWarning: current Server Discovery and Monitoring engine is deprecated, and will be removed in a future version. To use the new Server Discover and Monitoring engine, pass option { useUnifiedTopology: true } to the MongoClient constructor.
mattcollier commented 4 years ago

Need to replace use of runOnceAsync with runOnce.

Thanks to new node 14 feature --trace-warnings figured out where this one was coming from!

(node:26039) DeprecationWarning: runOnceAsync() is deprecated. Use runOnce() instead.
    at deprecatedCallbackified (util.js:210:5)
    at initDatabase (/home/matt/bedrock-dev/bedrock-rsvp/test/node_modules/bedrock-mongodb/lib/index.js:146:64)
    at runTask (/home/matt/bedrock-dev/bedrock-rsvp/test/node_modules/bedrock-mongodb/node_modules/async/dist/async.js:1634:13)
    at /home/matt/bedrock-dev/bedrock-rsvp/test/node_modules/bedrock-mongodb/node_modules/async/dist/async.js:1572:13
    at processQueue (/home/matt/bedrock-dev/bedrock-rsvp/test/node_modules/bedrock-mongodb/node_modules/async/dist/async.js:1582:13)
    at Object.auto (/home/matt/bedrock-dev/bedrock-rsvp/test/node_modules/bedrock-mongodb/node_modules/async/dist/async.js:1568:5)
    at init (/home/matt/bedrock-dev/bedrock-rsvp/test/node_modules/bedrock-mongodb/lib/index.js:145:9)
    at internal/util.js:297:30
    at new Promise (<anonymous>)
    at init (internal/util.js:296:12)
aljones15 commented 4 years ago

Need to replace use of runOnceAsync with runOnce.

Thanks to new node 14 feature --trace-warnings figured out where this one was coming from!

(node:26039) DeprecationWarning: runOnceAsync() is deprecated. Use runOnce() instead.
    at deprecatedCallbackified (util.js:210:5)
    at initDatabase (/home/matt/bedrock-dev/bedrock-rsvp/test/node_modules/bedrock-mongodb/lib/index.js:146:64)
    at runTask (/home/matt/bedrock-dev/bedrock-rsvp/test/node_modules/bedrock-mongodb/node_modules/async/dist/async.js:1634:13)
    at /home/matt/bedrock-dev/bedrock-rsvp/test/node_modules/bedrock-mongodb/node_modules/async/dist/async.js:1572:13
    at processQueue (/home/matt/bedrock-dev/bedrock-rsvp/test/node_modules/bedrock-mongodb/node_modules/async/dist/async.js:1582:13)
    at Object.auto (/home/matt/bedrock-dev/bedrock-rsvp/test/node_modules/bedrock-mongodb/node_modules/async/dist/async.js:1568:5)
    at init (/home/matt/bedrock-dev/bedrock-rsvp/test/node_modules/bedrock-mongodb/lib/index.js:145:9)
    at internal/util.js:297:30
    at new Promise (<anonymous>)
    at init (internal/util.js:296:12)

Addressed the runOnceAsync issue here: https://github.com/digitalbazaar/bedrock-mongodb/pull/33/commits/24bb26da128b8be62f3178bf9f322e29bb4b0823

the code in that commit does not address removing db.authenticate yet.

aljones15 commented 4 years ago

Deprecation warning when using this new stuff:

(node:25866) DeprecationWarning: current Server Discovery and Monitoring engine is deprecated, and will be removed in a future version. To use the new Server Discover and Monitoring engine, pass option { useUnifiedTopology: true } to the MongoClient constructor.

(node:16700) DeprecationWarning: The option autoReconnect is incompatible with the unified topology, please read more by visiting http://bit.ly/2D8WfT6

If I can get around to this I will, but the deprecation warning is not fatal on the other hand { useUnifiedTopology: true } is if autoReconnect: true is still set.

Docs on the newer topology method here: http://mongodb.github.io/node-mongodb-native/3.3/reference/unified-topology/

dlongley commented 4 years ago

This link has more information (more than the 3.3 reference) on the unified topology changes: https://mongodb.github.io/node-mongodb-native/3.6/reference/unified-topology/

dlongley commented 4 years ago

So regarding autoReconnect, it looks like previous behavior would have used these settings:

   * - **autoReconnect**
     - Server
     - boolean
     - true
     - Enables autoReconnecting when using a standalone server in the legacy topology setup
   * - **reconnectTries**
     - Server
     - integer
     - 30
     - When ``autoReconnect`` is enabled, the server attempt to reconnect #times.
   * - **reconnectInterval**
     - Server
     - integer
     - 1000
     - When ``autoReconnect`` is enabled, the server will wait # milliseconds between retries.

https://github.com/mongodb/node-mongodb-native/blob/71eee740721b6bfc8e0f7656e1d2dc418eeea7da/docs/guide/connection-settings.txt#L226-L240

So, with autoReconnect: true, the code would effectively implement a loop whereby an attempt to connect to the server would be performed every second for up to 30 seconds. With useUnifiedTopology, instead, the docs say a loop like this:

function executeOperation(topology, operation, callback) {
  const readPreference = resolveReadPreference(operation);
  topology.selectServer(readPreference, (err, server) => {
    if (err) {
      // This error is most likely a "Server selection timed out after Xms"
      return callback(err);
    }

    // checks a connection out of the server to execute the operation, then checks it back in
    server.withConnection(conn => operation.execute(conn, callback));
  })
}

Will be executed for up to serverSelectionTimeoutMS which defaults to 30 seconds. It's unclear how often an attempt will be made to get a connection (not clear if this is modulated by 1 second intervals or something reasonably equivalent to the other method), but it doesn't really matter to us.

Given all this, I think {useUnifiedTopology: true, serverSelectionTimeoutMS: 30000, autoReconnect: false} is roughly equivalent to {autoReconnect: true}. So let's turn off autoReconnect and, ideally, just remove the option from our defaults moving forward.

davidlehn commented 4 years ago

If this package, or our packages in general, are using newer database features, please update the config.js required server version:

// server version requirement with server-style string
config.mongodb.requirements.serverVersion = '>=3.0';
aljones15 commented 4 years ago

Approving ... left a few nits. My main concern is over whether the authentication test is working since there was a typo in the test config. If we can confirm that's ok, I think we're good here.

your commit for the typo did break the auth tests: (node:3668) UnhandledPromiseRejectionWarning: DatabaseError: Could not initialize database.

which is strange as I added a logger and saw the tests hit the login and pass with: config.mongodb.forceAuthentication = true; set to true. It is probably the case that the authMechanism is the problem.

NOTE: this PR has been approved, but still contains a call on Cursor.prototype.nextObject which has been deprecated. I will handle that tomorrow. DO NOT MERGE or RELEASE until nextObject has been removed.

dlongley commented 4 years ago

I'm happy for this to move ahead when others are as well.