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

Mongo package linting #8175

Merged
merged 20 commits into from
Jan 18, 2017
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/mongo/allow_tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ if (Meteor.isServer) {
check(nonce, String);
check(idGeneration, String);
var cursors = [];
var needToConfigure = undefined;
var needToConfigure;

// helper for defining a collection. we are careful to create just one
// Mongo.Collection even if the sub body is rerun, by caching them.
Expand Down
28 changes: 10 additions & 18 deletions packages/mongo/collection.js
Original file line number Diff line number Diff line change
Expand Up @@ -63,18 +63,14 @@ Mongo.Collection = function (name, options) {
switch (options.idGeneration) {
case 'MONGO':
self._makeNewID = function () {
var src = name
? DDP.randomStream('/collection/' + name)
: Random.insecure;
var src = name ? DDP.randomStream('/collection/' + name) : Random.insecure;
return new Mongo.ObjectID(src.hexString(24));
};
break;
case 'STRING':
default:
self._makeNewID = function () {
var src = name
? DDP.randomStream('/collection/' + name)
: Random.insecure;
var src = name ? DDP.randomStream('/collection/' + name) : Random.insecure;
return src.id();
};
break;
Expand Down Expand Up @@ -247,8 +243,7 @@ Mongo.Collection = function (name, options) {
}

// autopublish
if (Package.autopublish && !options._preventAutopublish && self._connection
&& self._connection.publish) {
if (Package.autopublish && !options._preventAutopublish && self._connection && self._connection.publish) {
self._connection.publish(null, function () {
return self.find();
}, {is_auto: true});
Expand Down Expand Up @@ -423,7 +418,7 @@ function convertRegexpToMongoSelector(regexp) {
selector.$options = regexOptions;

return selector;
};
}

// 'insert' immediately returns the inserted document's new _id.
// The others return values immediately if you are in a stub, an in-memory
Expand Down Expand Up @@ -474,8 +469,7 @@ Mongo.Collection.prototype.insert = function insert(doc, callback) {
doc = _.extend({}, doc);

if ('_id' in doc) {
if (!doc._id || !(typeof doc._id === 'string'
|| doc._id instanceof Mongo.ObjectID)) {
if (!doc._id || !(typeof doc._id === 'string' || doc._id instanceof Mongo.ObjectID)) {
throw new Error("Meteor requires document _id fields to be non-empty strings or ObjectIDs");
}
} else {
Expand Down Expand Up @@ -511,8 +505,7 @@ Mongo.Collection.prototype.insert = function insert(doc, callback) {
return result;
};

const wrappedCallback = wrapCallback(
callback, chooseReturnValueFromCollectionResult);
const wrappedCallback = wrapCallback(callback, chooseReturnValueFromCollectionResult);

if (this._isRemoteCollection()) {
const result = this._callMutatorMethod("insert", [doc], wrappedCallback);
Expand All @@ -534,7 +527,7 @@ Mongo.Collection.prototype.insert = function insert(doc, callback) {
}
throw e;
}
}
};

/**
* @summary Modify one or more documents in the collection. Returns the number of matched documents.
Expand All @@ -560,8 +553,7 @@ Mongo.Collection.prototype.update = function update(selector, modifier, ...optio
if (options && options.upsert) {
// set `insertedId` if absent. `insertedId` is a Meteor extension.
if (options.insertedId) {
if (!(typeof options.insertedId === 'string'
|| options.insertedId instanceof Mongo.ObjectID))
if (!(typeof options.insertedId === 'string' || options.insertedId instanceof Mongo.ObjectID))
throw new Error("insertedId must be string or ObjectID");
} else if (! selector._id) {
options.insertedId = this._makeNewID();
Expand Down Expand Up @@ -595,7 +587,7 @@ Mongo.Collection.prototype.update = function update(selector, modifier, ...optio
}
throw e;
}
}
};

/**
* @summary Remove documents from the collection
Expand Down Expand Up @@ -636,7 +628,7 @@ Mongo.Collection.prototype.remove = function remove(selector, callback) {
Mongo.Collection.prototype._isRemoteCollection = function _isRemoteCollection() {
// XXX see #MeteorServerNull
return this._connection && this._connection !== Meteor.server;
}
};

// Convert the callback to not return a result if there is an error
function wrapCallback(callback, convertResult) {
Expand Down
10 changes: 8 additions & 2 deletions packages/mongo/collection_tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ Tinytest.add('collection - call find with sort function',

var sorter = function (a, b) {
return a.a - b.a;
}
};

var getSorted = function (collection) {
return collection.find({}, {sort: sorter}).map(function (doc) { return doc.a; });
Expand All @@ -89,7 +89,13 @@ Tinytest.add('collection - call native find with sort function',
if (Meteor.isServer) {
test.throws(
function () {
nativeCollection.find({}, {sort: function(){}}).map(function (doc) { return doc.a; })
nativeCollection
.find({}, {
sort: function () {},
Copy link
Contributor

Choose a reason for hiding this comment

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

Though I'm not going to request changes, in the future you should know you can write methods like this using method syntax:

nativeCollection
  .find({}, {
    sort() {},
  })

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @benjamn ,
Thank you for merge.
I wish to use ES7 syntax, but most of file seems to be ES5. Am I wrong?

})
.map(function (doc) {
return doc.a;
});
},
/Illegal sort clause/
);
Expand Down
2 changes: 1 addition & 1 deletion packages/mongo/connection_options.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,4 @@
Mongo.setConnectionOptions = function setConnectionOptions (options) {
check(options, Object);
Mongo._connectionOptions = options;
}
};
63 changes: 32 additions & 31 deletions packages/mongo/mongo_driver.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@

var path = Npm.require('path');
var MongoDB = NpmModuleMongodb;
var Fiber = Npm.require('fibers');
var Future = Npm.require(path.join('fibers', 'future'));

MongoInternals = {};
Expand Down Expand Up @@ -63,8 +62,7 @@ var replaceMongoAtomWithMeteor = function (document) {
if (document instanceof MongoDB.ObjectID) {
return new Mongo.ObjectID(document.toHexString());
}
if (document["EJSON$type"] && document["EJSON$value"]
&& _.size(document) === 2) {
if (document["EJSON$type"] && document["EJSON$value"] && _.size(document) === 2) {
return EJSON.fromJSONValue(replaceNames(unmakeMongoLegal, document));
}
if (document instanceof MongoDB.Timestamp) {
Expand Down Expand Up @@ -275,12 +273,12 @@ MongoConnection.prototype._createCappedCollection = function (
// after the observer notifiers have added themselves to the write
// fence), you should call 'committed()' on the object returned.
MongoConnection.prototype._maybeBeginWrite = function () {
var self = this;
var fence = DDPServer._CurrentWriteFence.get();
if (fence)
if (fence) {
return fence.beginWrite();
else
} else {
return {committed: function () {}};
}
};

// Internal interface: adds a callback which is called when the Mongo primary
Expand Down Expand Up @@ -324,10 +322,11 @@ var writeCallback = function (write, refresh, callback) {
}
}
write.committed();
if (callback)
if (callback) {
callback(err, result);
else if (err)
} else if (err) {
throw err;
}
};
};

Expand Down Expand Up @@ -368,16 +367,15 @@ MongoConnection.prototype._insert = function (collection_name, document,
var collection = self.rawCollection(collection_name);
collection.insert(replaceTypes(document, replaceMeteorAtomWithMongo),
{safe: true}, callback);
} catch (e) {
} catch (err) {
write.committed();
throw e;
throw err;
}
};

// Cause queries that may be affected by the selector to poll in this write
// fence.
MongoConnection.prototype._refresh = function (collectionName, selector) {
var self = this;
var refreshKey = {collection: collectionName};
// If we know which documents we're removing, don't poll queries that are
// specific to other documents. (Note that multiple notifications here should
Expand All @@ -400,10 +398,11 @@ MongoConnection.prototype._remove = function (collection_name, selector,
if (collection_name === "___meteor_failure_test_collection") {
var e = new Error("Failure test");
e.expected = true;
if (callback)
if (callback) {
return callback(e);
else
} else {
throw e;
}
}

var write = self._maybeBeginWrite();
Expand All @@ -419,9 +418,9 @@ MongoConnection.prototype._remove = function (collection_name, selector,
};
collection.remove(replaceTypes(selector, replaceMeteorAtomWithMongo),
{safe: true}, wrappedCallback);
} catch (e) {
} catch (err) {
write.committed();
throw e;
throw err;
}
};

Expand Down Expand Up @@ -475,10 +474,11 @@ MongoConnection.prototype._update = function (collection_name, selector, mod,
if (collection_name === "___meteor_failure_test_collection") {
var e = new Error("Failure test");
e.expected = true;
if (callback)
if (callback) {
return callback(e);
else
} else {
throw e;
}
}

// explicit safety check. null and undefined can crash the mongo
Expand All @@ -494,7 +494,6 @@ MongoConnection.prototype._update = function (collection_name, selector, mod,
throw new Error(
"Only plain objects may be used as replacement" +
" documents in MongoDB");
return;
}

if (!options) options = {};
Expand Down Expand Up @@ -522,11 +521,11 @@ MongoConnection.prototype._update = function (collection_name, selector, mod,
var knownId = selector._id || mod._id;

if (options._forbidReplace && ! isModify) {
var e = new Error("Invalid modifier. Replacements are forbidden.");
var err = new Error("Invalid modifier. Replacements are forbidden.");
if (callback) {
return callback(e);
return callback(err);
} else {
throw e;
throw err;
}
}

Expand All @@ -549,14 +548,15 @@ MongoConnection.prototype._update = function (collection_name, selector, mod,
// This callback does not need to be bindEnvironment'ed because
// simulateUpsertWithInsertedId() wraps it and then passes it through
// bindEnvironmentForWrite.
function (err, result) {
function (error, result) {
// If we got here via a upsert() call, then options._returnObject will
// be set and we should return the whole object. Otherwise, we should
// just return the number of affected docs to match the mongo API.
if (result && ! options._returnObject)
callback(err, result.numberAffected);
else
callback(err, result);
if (result && ! options._returnObject) {
callback(error, result.numberAffected);
} else {
callback(error, result);
}
}
);
} else {
Expand Down Expand Up @@ -936,7 +936,7 @@ Cursor.prototype._publishCursor = function (sub) {
Cursor.prototype._getCollectionName = function () {
var self = this;
return self._cursorDescription.collectionName;
}
};

Cursor.prototype.observe = function (callbacks) {
var self = this;
Expand Down Expand Up @@ -1092,7 +1092,7 @@ _.extend(SynchronousCursor.prototype, {
return self.map(_.identity);
},

count: function (applySkipLimit: false) {
count: function (applySkipLimit = false) {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

var self = this;
return self._synchronousCount(applySkipLimit).wait();
},
Expand Down Expand Up @@ -1120,18 +1120,19 @@ MongoConnection.prototype.tail = function (cursorDescription, docCallback) {
var cursor = self._createSynchronousCursor(cursorDescription);

var stopped = false;
var lastTS = undefined;
var lastTS;
var loop = function () {
var doc = null;
while (true) {
if (stopped)
return;
try {
var doc = cursor._nextObject();
doc = cursor._nextObject();
} catch (err) {
// There's no good way to figure out if this was actually an error
// from Mongo. Ah well. But either way, we need to retry the cursor
// (unless the failure was because the observe got stopped).
doc = null;
// `doc` is already `null` here;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this change is safe or just a "linting" change – at least not without further consideration or explanation.

This is in a loop. While doc is null the first time around, if cursor._nextObject throws an error, it would be left as the previous doc that was processed on this cursor (loop). A tailable cursor remains open until additional documents arrive.

Do you disagree?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@abernix you're right. My bad.

}
// Since cursor._nextObject can yield, we need to check again to see if
// we've been stopped before calling the callback.
Expand Down