Skip to content

NODE-2579/3.6/global-promise-prereq #2340

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.

Sign up for GitHub

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

Closed
wants to merge 6 commits into from
Closed

NODE-2579/3.6/global-promise-prereq #2340

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
158c141
adds promise-provider
May 1, 2020
32eb671
tests use promise-provider
May 1, 2020
9960092
uses promise provider & no-native
May 1, 2020
d3e8428
implements eslint-plugin-promise
May 1, 2020
a6bc216
rm Admin promiseLibrary property
May 1, 2020
75278d0
use class, symbol, validator, fix test
May 4, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions .eslintrc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,16 @@
"ecmaVersion": 2017
},
"plugins": [
"prettier"
"prettier",
"promise"
],
"rules": {
"prettier/prettier": ["error", {
"singleQuote": true,
"tabWidth": 2,
"printWidth": 100
}],

"promise/no-native": "error",
"no-console": 0,
"eqeqeq": ["error", "always", {"null": "ignore"}],
"strict": ["error", "global"]
Expand Down
6 changes: 3 additions & 3 deletions lib/admin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
'use strict';

const PromiseProvider = require('./promise_provider');
const applyWriteConcern = require('./utils').applyWriteConcern;

const AddUserOperation = require('./operations/add_user');
const ExecuteDbAdminCommandOperation = require('./operations/execute_db_admin_command');
const RemoveUserOperation = require('./operations/remove_user');
Expand Down Expand Up @@ -42,14 +42,14 @@ const executeOperation = require('./operations/execute_operation');
* @class
* @return {Admin} a collection instance.
*/
function Admin(db, topology, promiseLibrary) {
Copy link
Contributor Author

@reggi reggi May 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Had the idea of removing this promiseLibrary option, and rather just pull it out of db.

function Admin(db, topology) {
if (!(this instanceof Admin)) return new Admin(db, topology);

// Internal state
this.s = {
db: db,
topology: topology,
promiseLibrary: promiseLibrary
promiseLibrary: PromiseProvider.get(db, topology)
Copy link
Member

Choose a reason for hiding this comment

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

I thought the idea was to remove every instance of promiseLibrary in favor of getting it from the PromiseProvider?

Copy link
Contributor Author

@reggi reggi May 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

It is my understanding that each time a method takes-in promiseLibrary it set's it for it's class-scope and it's-children. And I've tried to illustrate this in this example. I thought we were all in-agreement that 3.6 can't change this functionality.

// here mongoClient sets `bluebird`
const client = await MongoClient("mongodb://127.0.0.1", {
    promiseLibrary: bluebird,
    useUnifiedTopology: true
});

const db = client.db("test");

// here in collection I'm setting the promiseLibrary to `q`
const collectionPromise = db.collection("test", { promiseLibrary: q }).find().toArray();

I would assume here the driver would provide any promises coming from client to be bluebird and promises coming from collectionPromise to be q.

This is the "parody" i believed we we were trying to support in 3.6

If we use a "global" setter / getter, and don't store in-parent-class and pass options, we loose this tracking of what class is using what.

My plan was to remove the stores in master and only .set() in MongoClient (and mognodb.Promise = bluebird module-getter)

This is related to the private-slack thread here.

};
}

Expand Down
3 changes: 3 additions & 0 deletions lib/async/async_iterator.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
'use strict';

const PromiseProvider = require('../promise_provider');

// async function* asyncIterator() {
// while (true) {
// const value = await this.next();
Expand All @@ -14,6 +16,7 @@

// TODO: change this to the async generator function above
function asyncIterator() {
const Promise = PromiseProvider.get();
const cursor = this;

return {
Expand Down
6 changes: 4 additions & 2 deletions lib/bulk/common.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
'use strict';

const PromiseProvider = require('../promise_provider');
const Long = require('../core').BSON.Long;
const MongoError = require('../core').MongoError;
const ObjectID = require('../core').BSON.ObjectID;
Expand Down Expand Up @@ -775,7 +776,7 @@ class BulkOperationBase {
const writeConcern = finalOptions.writeConcern;

// Get the promiseLibrary
const promiseLibrary = options.promiseLibrary || Promise;
const promiseLibrary = PromiseProvider.get(options, collection, topology);

// Final results
const bulkResult = {
Expand Down Expand Up @@ -1022,12 +1023,13 @@ class BulkOperationBase {
* @param {*} callback
*/
_handleEarlyError(err, callback) {
const Promise = PromiseProvider.get(this);
if (typeof callback === 'function') {
callback(err, null);
return;
}

return this.s.promiseLibrary.reject(err);
return Promise.reject(err);
}

/**
Expand Down
25 changes: 14 additions & 11 deletions lib/change_stream.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
'use strict';

const PromiseProvider = require('./promise_provider');
Copy link
Contributor

Choose a reason for hiding this comment

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

Just wanted to point out that this class has been refactored to use maybePromise so most of these changes will cause merge conflicts once #2333 makes its way into 3.6. It might be easier to just revert this file and add a /* es-lint-disable promise/no-native */ at the top, along with a note to update once that PR is merged in - not a big deal either way.

const EventEmitter = require('events');
const isResumableError = require('./error').isResumableError;
const MongoError = require('./core').MongoError;
Expand Down Expand Up @@ -85,7 +86,7 @@ class ChangeStream extends EventEmitter {
);
}

this.promiseLibrary = parent.s.promiseLibrary;
this.promiseLibrary = PromiseProvider.get(options, parent);
if (!this.options.readPreference && parent.s.readPreference) {
this.options.readPreference = parent.s.readPreference;
}
Expand Down Expand Up @@ -138,10 +139,11 @@ class ChangeStream extends EventEmitter {
* @return {Promise} returns Promise if no callback passed
*/
next(callback) {
var self = this;
const self = this;
const Promise = PromiseProvider.get(self);
if (this.isClosed()) {
if (callback) return callback(new Error('Change Stream is not open.'), null);
return self.promiseLibrary.reject(new Error('Change Stream is not open.'));
return Promise.reject(new Error('Change Stream is not open.'));
}

return this.cursor
Expand Down Expand Up @@ -169,9 +171,11 @@ class ChangeStream extends EventEmitter {
* @return {Promise} returns Promise if no callback passed
*/
close(callback) {
const Promise = PromiseProvider.get(this);

if (!this.cursor) {
if (callback) return callback();
return this.promiseLibrary.resolve();
return Promise.resolve();
}

// Tidy up the existing cursor
Expand All @@ -186,7 +190,7 @@ class ChangeStream extends EventEmitter {
});
}

const PromiseCtor = this.promiseLibrary || Promise;
const PromiseCtor = PromiseProvider.get(this);
return new PromiseCtor((resolve, reject) => {
cursor.close(err => {
['data', 'close', 'end', 'error'].forEach(event => cursor.removeAllListeners(event));
Expand Down Expand Up @@ -472,6 +476,7 @@ function waitForTopologyConnected(topology, options, callback) {
// Handle new change events. This method brings together the routes from the callback, event emitter, and promise ways of using ChangeStream.
function processNewChange(args) {
const changeStream = args.changeStream;
const Promise = PromiseProvider.get(changeStream);
const error = args.error;
const change = args.change;
const callback = args.callback;
Expand All @@ -486,9 +491,7 @@ function processNewChange(args) {
}

const error = new MongoError('ChangeStream is closed');
return typeof callback === 'function'
? callback(error, null)
: changeStream.promiseLibrary.reject(error);
return typeof callback === 'function' ? callback(error, null) : Promise.reject(error);
}

const topology = changeStream.topology;
Expand Down Expand Up @@ -546,7 +549,7 @@ function processNewChange(args) {

if (eventEmitter) return changeStream.emit('error', error);
if (typeof callback === 'function') return callback(error, null);
return changeStream.promiseLibrary.reject(error);
return Promise.reject(error);
}

changeStream.attemptingResume = false;
Expand All @@ -558,7 +561,7 @@ function processNewChange(args) {

if (eventEmitter) return changeStream.emit('error', noResumeTokenError);
if (typeof callback === 'function') return callback(noResumeTokenError, null);
return changeStream.promiseLibrary.reject(noResumeTokenError);
return Promise.reject(noResumeTokenError);
}

// cache the resume token
Expand All @@ -571,7 +574,7 @@ function processNewChange(args) {
// Return the change
if (eventEmitter) return changeStream.emit('change', change);
if (typeof callback === 'function') return callback(error, change);
return changeStream.promiseLibrary.resolve(change);
return Promise.resolve(change);
}

/**
Expand Down
20 changes: 12 additions & 8 deletions lib/collection.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
'use strict';

const PromiseProvider = require('./promise_provider');
const deprecate = require('util').deprecate;
const deprecateOptions = require('./utils').deprecateOptions;
const checkCollectionName = require('./utils').checkCollectionName;
Expand Down Expand Up @@ -128,7 +129,7 @@ function Collection(db, topology, dbName, name, pkFactory, options) {
const namespace = new MongoDBNamespace(dbName, name);

// Get the promiseLibrary
const promiseLibrary = options.promiseLibrary || Promise;
const promiseLibrary = PromiseProvider.get(options, db, topology);
Copy link
Contributor Author

@reggi reggi May 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Here's a case that I'm taking about. It's a little more comprehensive than it previously was. I read this as, check the options, then check db, then check topology.


// Set custom primary key factory if provided
pkFactory = pkFactory == null ? ObjectID : pkFactory;
Expand Down Expand Up @@ -443,7 +444,7 @@ Collection.prototype.find = deprecateOptions(
newOptions.db = this.s.db;

// Add the promise library
newOptions.promiseLibrary = this.s.promiseLibrary;
newOptions.promiseLibrary = PromiseProvider.get(options, this);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is within collection.find it's not clear to me which methods you can pass in promiseLibrary and which you can't. Should this be PromiseProvider.get(this); or PromiseProvider.get(options, this);? Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

If I'm understanding things correctly, in order to keep the behavior unchanged here for collection.find, it should be PromiseProvider.get(this); - the original code always sets newOptions.promiseLibrary = this.s.promiseLibrary regardless of the user passing in options.promiseLibrary.

Any methods in Collection which don't call something like options.promiseLibrary = this.s.promiseLibrary; before executing the operation would need to honor the promiseLibrary passed in by the user on that method - if there are any such methods.


// Set raw if available at collection level
if (newOptions.raw == null && typeof this.s.raw === 'boolean') newOptions.raw = this.s.raw;
Expand Down Expand Up @@ -748,13 +749,14 @@ Collection.prototype.insert = deprecate(function(docs, options, callback) {
* @return {Promise} returns Promise if no callback passed
*/
Collection.prototype.updateOne = function(filter, update, options, callback) {
const Promise = PromiseProvider.get(this);
if (typeof options === 'function') (callback = options), (options = {});
options = options || {};

const err = checkForAtomicOperators(update);
if (err) {
if (typeof callback === 'function') return callback(err);
return this.s.promiseLibrary.reject(err);
return Promise.reject(err);
Copy link
Member

Choose a reason for hiding this comment

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

these are frustrating.. they should really be deferred to the execute method of UpdateOneOperation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should that change be in this PR?

Copy link
Member

Choose a reason for hiding this comment

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

no, just calling it out

}

options = Object.assign({}, options);
Expand Down Expand Up @@ -827,13 +829,14 @@ Collection.prototype.replaceOne = function(filter, doc, options, callback) {
* @return {Promise<Collection~updateWriteOpResult>} returns Promise if no callback passed
*/
Collection.prototype.updateMany = function(filter, update, options, callback) {
const Promise = PromiseProvider.get(this);
if (typeof options === 'function') (callback = options), (options = {});
options = options || {};

const err = checkForAtomicOperators(update);
if (err) {
if (typeof callback === 'function') return callback(err);
return this.s.promiseLibrary.reject(err);
return Promise.reject(err);
}

options = Object.assign({}, options);
Expand Down Expand Up @@ -1754,6 +1757,7 @@ Collection.prototype.findOneAndReplace = function(filter, replacement, options,
* @return {Promise<Collection~findAndModifyWriteOpResultObject>} returns Promise if no callback passed
*/
Collection.prototype.findOneAndUpdate = function(filter, update, options, callback) {
const Promise = PromiseProvider.get(this);
if (typeof options === 'function') (callback = options), (options = {});
options = options || {};

Expand All @@ -1766,7 +1770,7 @@ Collection.prototype.findOneAndUpdate = function(filter, update, options, callba
const err = checkForAtomicOperators(update);
if (err) {
if (typeof callback === 'function') return callback(err);
return this.s.promiseLibrary.reject(err);
return Promise.reject(err);
}

const findOneAndUpdateOperation = new FindOneAndUpdateOperation(this, filter, update, options);
Expand Down Expand Up @@ -1990,7 +1994,7 @@ Collection.prototype.parallelCollectionScan = deprecate(function(options, callba
options.readPreference = resolveReadPreference(this, options);

// Add a promiseLibrary
options.promiseLibrary = this.s.promiseLibrary;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little confused about what setting options.promiseLibrary before running executeLegacyOperation does. As you can see here, executeLegacyOperation ignores the options and grabs the promiseLibrary from the topology. If I'm not missing something, we can just delete anything that sets options.promiseLibrary before a call to executeLegacyOperation.

options.promiseLibrary = PromiseProvider.get(this);

if (options.session) {
options.session = undefined;
Expand Down Expand Up @@ -2171,7 +2175,7 @@ Collection.prototype.initializeUnorderedBulkOp = function(options) {
options.ignoreUndefined = this.s.options.ignoreUndefined;
}

options.promiseLibrary = this.s.promiseLibrary;
options.promiseLibrary = PromiseProvider.get(this);
return unordered(this.s.topology, this, options);
};

Expand All @@ -2194,7 +2198,7 @@ Collection.prototype.initializeOrderedBulkOp = function(options) {
if (options.ignoreUndefined == null) {
options.ignoreUndefined = this.s.options.ignoreUndefined;
}
options.promiseLibrary = this.s.promiseLibrary;
options.promiseLibrary = PromiseProvider.get(this);
return ordered(this.s.topology, this, options);
};

Expand Down
4 changes: 3 additions & 1 deletion lib/core/sdam/topology.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
'use strict';

const PromiseProvider = require('../../promise_provider');
const Denque = require('denque');
const EventEmitter = require('events');
const ServerDescription = require('./server_description').ServerDescription;
Expand Down Expand Up @@ -193,7 +195,7 @@ class Topology extends EventEmitter {
// Active client sessions
sessions: new Set(),
// Promise library
promiseLibrary: options.promiseLibrary || Promise,
promiseLibrary: PromiseProvider.get(options),
Copy link
Member

Choose a reason for hiding this comment

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

A user can't create a Topology, so the promiseLibrary provided to this class is only whatever is set on the top level MongoClient. I think we can remove storage here, and just depend on the PromiseProvider.get

credentials: options.credentials,
clusterTime: null,

Expand Down
4 changes: 4 additions & 0 deletions lib/core/sessions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
'use strict';

const PromiseProvider = require('../promise_provider');
const retrieveBSON = require('./connection/utils').retrieveBSON;
const EventEmitter = require('events');
const BSON = retrieveBSON();
Expand Down Expand Up @@ -216,6 +217,7 @@ class ClientSession extends EventEmitter {
* @return {Promise} A promise is returned if no callback is provided
*/
commitTransaction(callback) {
const Promise = PromiseProvider.get(this);
if (typeof callback === 'function') {
endTransaction(this, 'commitTransaction', callback);
return;
Expand All @@ -235,6 +237,7 @@ class ClientSession extends EventEmitter {
* @return {Promise} A promise is returned if no callback is provided
*/
abortTransaction(callback) {
const Promise = PromiseProvider.get(this);
if (typeof callback === 'function') {
endTransaction(this, 'abortTransaction', callback);
return;
Expand Down Expand Up @@ -342,6 +345,7 @@ function userExplicitlyEndedTransaction(session) {
}

function attemptTransaction(session, startTime, fn, options) {
const Promise = PromiseProvider.get(session);
session.startTransaction(options);

let promise;
Expand Down
13 changes: 7 additions & 6 deletions lib/cursor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
'use strict';

const PromiseProvider = require('./promise_provider');
const Transform = require('stream').Transform;
const PassThrough = require('stream').PassThrough;
const deprecate = require('util').deprecate;
Expand Down Expand Up @@ -111,7 +112,7 @@ class Cursor extends CoreCursor {
const currentNumberOfRetries = numberOfRetries;

// Get the promiseLibrary
const promiseLibrary = options.promiseLibrary || Promise;
const promiseLibrary = PromiseProvider.get(options, topology);

// Internal cursor state
this.s = {
Expand Down Expand Up @@ -727,6 +728,7 @@ class Cursor extends CoreCursor {
* @return {Promise} if no callback supplied
*/
forEach(iterator, callback) {
const Promise = PromiseProvider.get(this);
// Rewind cursor state
this.rewind();

Expand All @@ -751,7 +753,7 @@ class Cursor extends CoreCursor {
}
});
} else {
return new this.s.promiseLibrary((fulfill, reject) => {
return new Promise((fulfill, reject) => {
each(this, (err, doc) => {
if (err) {
reject(err);
Expand Down Expand Up @@ -910,6 +912,7 @@ class Cursor extends CoreCursor {
* @return {Promise} returns Promise if no callback passed
*/
close(options, callback) {
const Promise = PromiseProvider.get(this);
if (typeof options === 'function') (callback = options), (options = {});
options = Object.assign({}, { skipKillCursors: false }, options);

Expand All @@ -929,17 +932,15 @@ class Cursor extends CoreCursor {
}

// Return a Promise
return new this.s.promiseLibrary(resolve => {
resolve();
});
return Promise.resolve();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was an opinionated change, no reason to call new here just to resolve.

};

if (this.cursorState.session) {
if (typeof callback === 'function') {
return this._endSession(() => completeClose());
}

return new this.s.promiseLibrary(resolve => {
return new Promise(resolve => {
this._endSession(() => completeClose().then(resolve));
});
}
Expand Down
Loading