Make falsey and {_id: <falsey>} selectors match nothing.

In Livedata and Minimongo, make falsey selectors match no documents,
instead of all documents.  Same for {_id: undefined}.  This is a
departure from most MongoDB drivers, but offers a safety belt around
selectors that are rarely useful and easy to accidentally create
programmatically.

For remove(), also protect against accidentally destroying an entire
collection when passing no args.  To empty a collection, pass the
wildcard selector explicitly: foo.remove({});

For find(), keep the standard mongo behavior of returning all documents
when no selector is passed in by explicitly checking arguments.length.

This change also makes typical read cases cleaner, allowing:

 x = foo.findOne(Session.get('foo_id'));

instead of

 x = Session.get('foo_id') && foo.findOne(Session.get('foo_id'));

packages/livedata/livedata_client.js

@@ -100,12 +100,17 @@ if (typeof Meteor === "undefined") Meteor = {};
       _name: name,
       _collection: new Collection(),
 
-      find: function (selector, options) {
-        return this._collection.find(selector, options);
+      find: function (/* selector, options */) {
+        // Collection.find() (return all docs) behaves differently
+        // from Collection.find(undefined) (return 0 docs).  so be
+        // careful about preserving the length of arguments when
+        // descending into minimongo.
+        return this._collection.find.apply(this._collection, Array.prototype.slice.call(arguments));
       },
 
-      findOne: function (selector) {
-        return this._collection.findOne(selector);
+      findOne: function (/* selector, options */) {
+        // as above
+        return this._collection.findOne.apply(this._collection, Array.prototype.slice.call(arguments));
       },
 
       insert: function (obj) {
@@ -125,9 +130,6 @@ if (typeof Meteor === "undefined") Meteor = {};
       },
 
       update: function (selector, mutator, options) {
-        if (typeof(selector) === "string")
-          selector = {_id: selector};
-
         if (this._name)
           Meteor._stream.emit('handle', {
             collection: this._name, type: 'update',
@@ -136,9 +138,6 @@ if (typeof Meteor === "undefined") Meteor = {};
       },
 
       remove: function (selector) {
-        if (typeof(selector) === "string")
-          selector = {_id: selector};
-
         if (this._name)
           Meteor._stream.emit('handle', {
             collection: this._name, type: 'remove', selector: selector});

packages/livedata/livedata_server.js

@@ -256,11 +256,16 @@ if (typeof Meteor === "undefined") Meteor = {};
       // kill it.
 
       find: function (selector, options) {
-        var cursor = new Meteor._mongo_driver.Cursor(this._name, selector, options);
-        return cursor;
+        if (arguments.length === 0)
+          selector = {};
+
+        return new Meteor._mongo_driver.Cursor(this._name, selector, options);
       },
 
       findOne: function (selector, options) {
+        if (arguments.length === 0)
+          selector = {};
+
         options = options || {};
         options.limit = 1;
         return this.find(selector, options).fetch()[0];
@@ -310,11 +315,4 @@ if (typeof Meteor === "undefined") Meteor = {};
 
     return ret;
   };
-
-  Meteor.Collection.Query = function (collection, selector, options) {
-    if (!options) options = {};
-
-    this.cursor = Meteor._mongo_driver.find(this._name, selector, options);
-  };
-
 })();

packages/livedata/mongo_driver.js

@@ -46,6 +46,21 @@
     }
   }
 
+  // protect against dangerous selectors.  falsey and {_id: falsey}
+  // are both likely programmer error, and not what you want,
+  // particularly for destructive operations.
+  function rewrite_selector (selector) {
+    // shorthand -- scalars match _id
+    if ((typeof selector === 'string') || (typeof selector === 'number'))
+      selector = {_id: selector};
+
+    if (!selector || (('_id' in selector) && !selector._id))
+      // can't match anything
+      return {_id: Meteor.uuid()};
+    else
+      return selector;
+  }
+
   //////////// Public API //////////
 
   function Cursor (collection_name, selector, options) {
@@ -53,15 +68,7 @@
     withCollection(collection_name, function(err, collection) {
       // XXX err handling
 
-      var single_result = false;
-      // if single id is passed
-      // XXX deal with both string and objectid
-      if (typeof selector === 'string') {
-        selector = {_id: selector};
-        single_result = true;
-      } else if (!selector) {
-        selector = {};
-      }
+      selector = rewrite_selector(selector);
 
       var cursor = collection.find(selector);
       // XXX is there a way to do this as for x in ['sort', 'limit', 'skip']?
@@ -150,10 +157,8 @@
     // needed, really.
 
     // XXX does not allow options. matches the client.
-    // XXX consider allowing string -> {_id: id} shortcut.
 
-    if (typeof(selector) === "string")
-      selector = {_id: selector};
+    selector = rewrite_selector(selector);
 
     withCollection(collection_name, function(err, collection) {
       // XXX err handling
@@ -172,8 +177,7 @@
     // I couldn't convince myself it was safe not to. Not sure if it is
     // needed, really.
 
-    if (typeof(selector) === "string")
-      selector = {_id: selector};
+    selector = rewrite_selector(selector);
 
     if (!options) options = {};
     // Default to multi. This is the oppposite of mongo. We'll see how it goes.

packages/liveui/liveui.js

@@ -169,7 +169,7 @@ Meteor.ui.renderList = function (query, options) {
   // protect against old invocations passing in a Collection or a
   // LiveResultsSet.
   // XXX remove in a few releases
-  if (!(query instanceof Collection.Query))
+  if (!(query instanceof Collection.Cursor))
     throw new Error("insert_before: at least one entry must exist");
 
   // create the top-level document fragment/range that will be

packages/minimongo/minimongo.js

@@ -1,14 +1,11 @@
 // XXX indexes
 // XXX type checking on selectors (graceful error if malformed)
-// XXX merge ad-hoc live query object and Query
-
-// XXX keep observe()s running after they're killed, because usually
-// the dependency tracker is going to spin them up again right away.
+// XXX merge ad-hoc live query object and Cursor
 
 // Collection: a set of documents that supports queries and modifiers.
 
-// Query: a specification for a particular subset of documents, w/
-// a defined order, limit, and offset.  creating a Query with Collection.find(),
+// Cursor: a specification for a particular subset of documents, w/
+// a defined order, limit, and offset.  creating a Cursor with Collection.find(),
 
 // LiveResultsSet: the return value of a live query.
 
@@ -40,19 +37,25 @@ Collection = function () {
 // XXX add one more sort form: "key"
 // XXX tests
 Collection.prototype.find = function (selector, options) {
-  return new Collection.Query(this, selector, options);
+  // default syntax for everything is to omit the selector argument.
+  // but if selector is explicitly passed in as false or undefined, we
+  // want a selector that matches nothing.
+  if (arguments.length === 0)
+    selector = {};
+
+  return new Collection.Cursor(this, selector, options);
 };
 
 // don't call this ctor directly.  use Collection.find().
-Collection.Query = function (collection, selector, options) {
+Collection.Cursor = function (collection, selector, options) {
   if (!options) options = {};
 
   this.collection = collection;
 
-  if (typeof selector === "string") {
+  if ((typeof selector === "string") || (typeof selector === "number")) {
     // stash for fast path
     this.selector_id = selector;
-    this.selector_f = Collection._compileSelector({_id: selector});
+    this.selector_f = Collection._compileSelector(selector);
   } else {
     this.selector_f = Collection._compileSelector(selector);
     this.sort_f = options.sort ? Collection._compileSort(options.sort) : null;
@@ -68,19 +71,22 @@ Collection.Query = function (collection, selector, options) {
     this.reactive = (options.reactive === undefined) ? true : options.reactive;
 };
 
-Collection.Query.prototype.rewind = function () {
+Collection.Cursor.prototype.rewind = function () {
   var self = this;
   self.db_objects = null;
   self.cursor_pos = 0;
 };
 
 Collection.prototype.findOne = function (selector, options) {
+  if (arguments.length === 0)
+    selector = {};
+
   options = options || {};
   options.limit = 1;
   return this.find(selector, options).fetch()[0];
 };
 
-Collection.Query.prototype.forEach = function (callback) {
+Collection.Cursor.prototype.forEach = function (callback) {
   var self = this;
   var doc;
 
@@ -97,7 +103,7 @@ Collection.Query.prototype.forEach = function (callback) {
     callback(Collection._deepcopy(self.db_objects[self.cursor_pos++]));
 };
 
-Collection.Query.prototype.map = function (callback) {
+Collection.Cursor.prototype.map = function (callback) {
   var self = this;
   var res = [];
   self.forEach(function (doc) {
@@ -106,7 +112,7 @@ Collection.Query.prototype.map = function (callback) {
   return res;
 };
 
-Collection.Query.prototype.fetch = function () {
+Collection.Cursor.prototype.fetch = function () {
   var self = this;
   var res = [];
   self.forEach(function (doc) {
@@ -115,7 +121,7 @@ Collection.Query.prototype.fetch = function () {
   return res;
 };
 
-Collection.Query.prototype.count = function () {
+Collection.Cursor.prototype.count = function () {
   var self = this;
 
   if (self.reactive)
@@ -130,7 +136,31 @@ Collection.Query.prototype.count = function () {
 // the handle that comes back from observe.
 Collection.LiveResultsSet = function () {};
 
-Collection.Query.prototype.observe = function (options) {
+// options to contain:
+//  * callbacks:
+//    - added (object, before_index)
+//    - changed (new_object, at_index)
+//    - moved (object, old_index, new_index) - can only fire with changed()
+//    - removed (id, at_index)
+//  * sort: sort descriptor
+//
+// attributes available on returned query handle:
+//  * stop(): end updates
+//  * indexOf(id): return current index of object in result set, or -1
+//  * collection: the collection this query is querying
+//
+// iff x is a returned query handle, (x instanceof
+// Collection.LiveResultsSet) is true
+//
+// initial results delivered through added callback
+// XXX maybe callbacks should take a list of objects, to expose transactions?
+// XXX maybe support field limiting (to limit what you're notified on)
+// XXX maybe support limit/skip
+// XXX it'd be helpful if removed got the object that just left the
+// query, not just its id
+// XXX document that initial results will definitely be delivered before we return [do, add to asana]
+
+Collection.Cursor.prototype.observe = function (options) {
   var self = this;
 
   if (self.skip || self.limit)
@@ -138,7 +168,7 @@ Collection.Query.prototype.observe = function (options) {
 
   var qid = self.collection.next_qid++;
 
-  // XXX merge this object w/ "this" Query.  they're the same.
+  // XXX merge this object w/ "this" Cursor.  they're the same.
   var query = self.collection.queries[qid] = {
     selector_f: self.selector_f, // not fast pathed
     sort_f: self.sort_f,
@@ -167,11 +197,11 @@ Collection.Query.prototype.observe = function (options) {
 // constructs sorted array of matching objects, but doesn't copy them.
 // respects sort, skip, and limit properties of the query.
 // if sort_f is falsey, no sort -- you get the natural order
-Collection.Query.prototype._getRawObjects = function () {
+Collection.Cursor.prototype._getRawObjects = function () {
   var self = this;
 
   // fast path for single ID value
-  if (self.selector_id)
+  if (self.selector_id && (self.selector_id in self.collection.docs))
     return [self.collection.docs[self.selector_id]];
 
   // slow path for arbitrary selector, sort, skip, limit
@@ -190,7 +220,7 @@ Collection.Query.prototype._getRawObjects = function () {
   return results.slice(idx_start, idx_end);
 };
 
-Collection.Query.prototype._markAsReactive = function (options) {
+Collection.Cursor.prototype._markAsReactive = function (options) {
   var self = this;
 
   var context = Meteor.deps.Context.current;
@@ -233,36 +263,11 @@ Collection.prototype.insert = function (doc) {
   }
 };
 
-// options to contain:
-//  * callbacks:
-//    - added (object, before_index)
-//    - changed (new_object, at_index)
-//    - moved (object, old_index, new_index) - can only fire with changed()
-//    - removed (id, at_index)
-//  * sort: sort descriptor
-//
-// attributes available on returned query handle:
-//  * stop(): end updates
-//  * indexOf(id): return current index of object in result set, or -1
-//  * collection: the collection this query is querying
-//
-// iff x is a returned query handle, (x instanceof
-// Collection.LiveResultsSet) is true
-//
-// initial results delivered through added callback
-// XXX maybe callbacks should take a list of objects, to expose transactions?
-// XXX maybe support field limiting (to limit what you're notified on)
-// XXX maybe support limit/skip
-// XXX it'd be helpful if removed got the object that just left the
-// query, not just its id
-// XXX document that initial results will definitely be delivered before we return [do, add to asana]
-
 Collection.prototype.remove = function (selector) {
   var self = this;
   var remove = [];
   var query_remove = [];
-  if (typeof(selector) === "string")
-    selector = {_id: selector};
+
   var selector_f = Collection._compileSelector(selector);
   for (var id in self.docs) {
     var doc = self.docs[id];
@@ -288,13 +293,10 @@ Collection.prototype.remove = function (selector) {
 // XXX atomicity: if multi is true, and one modification fails, do
 // we rollback the whole operation, or what?
 Collection.prototype.update = function (selector, mod, options) {
-  if (typeof(selector) === "string")
-    selector = {_id: selector};
-
   if (!options) options = {};
   // Default to multi. This is the oppposite of mongo. We'll see how it goes.
   if (typeof(options.multi) === "undefined")
-    options.multi = true
+    options.multi = true;
 
   var self = this;
   var any = false;

packages/minimongo/minimongo_tests.js

@@ -65,7 +65,17 @@ test("minimongo - basics", function () {
   assert.length(c.find({type: "kitten"}).fetch(), 1);
   assert.length(c.find({type: "cryptographer"}).fetch(), 3);
 
-  c.remove({});
+  c.remove(null);
+  c.remove(false);
+  c.remove(undefined);
+  assert.equal(c.find().count(), 4);
+
+  c.remove({_id: null});
+  c.remove({_id: false});
+  c.remove({_id: undefined});
+  assert.equal(c.find().count(), 4);
+
+  c.remove();
   assert.equal(0, c.find().count());
 
   c.insert({_id: 1, name: "strawberry", tags: ["fruit", "red", "squishy"]});
@@ -79,6 +89,19 @@ test("minimongo - basics", function () {
   assert.length(c.find({tags: "fruit"}).fetch(), 2);
   assert.length(c.find({tags: "red"}).fetch(), 3);
 
+  assert.equal(c.findOne(1).name, "strawberry");
+  assert.equal(c.findOne(2).name, "apple");
+  assert.equal(c.findOne(3).name, "rose");
+  assert.equal(c.findOne(4), undefined);
+  assert.equal(c.findOne("abc"), undefined);
+  assert.equal(c.findOne(undefined), undefined);
+
+  assert.equal(c.find(1).count(), 1);
+  assert.equal(c.find(4).count(), 0);
+  assert.equal(c.find("abc").count(), 0);
+  assert.equal(c.find(undefined).count(), 0);
+  assert.equal(c.find().count(), 3);
+
   var ev = "";
   var makecb = function (tag) {
     return {
@@ -196,6 +219,21 @@ test("minimongo - selector_compiler", function () {
   match({}, {});
   match({}, {a: 12});
 
+  // scalars
+  match(1, {_id: 1, a: 'foo'});
+  nomatch(1, {_id: 2, a: 'foo'});
+  match('a', {_id: 'a', a: 'foo'});
+  nomatch('a', {_id: 'b', a: 'foo'});
+
+  // safety
+  nomatch(undefined, {});
+  nomatch(undefined, {_id: 'foo'});
+  nomatch(false, {_id: 'foo'});
+  nomatch(null, {_id: 'foo'});
+  nomatch({_id: undefined}, {_id: 'foo'});
+  nomatch({_id: false}, {_id: 'foo'});
+  nomatch({_id: null}, {_id: 'foo'});
+
   // matching one or more keys
   nomatch({a: 12}, {});
   match({a: 12}, {a: 12});

packages/minimongo/selector.js

@@ -259,7 +259,17 @@ Collection._compileSelector = function (selector) {
   var literals = [];
   // you can pass a literal function instead of a selector
   if (selector instanceof Function)
-    return function (doc) {return selector.call(doc);}
+    return function (doc) {return selector.call(doc);};
+
+  // shorthand -- scalars match _id
+  if ((typeof(selector) === "string") || (typeof(selector) === "number"))
+    selector = {_id: selector};
+
+  // protect against dangerous selectors.  falsey and {_id: falsey}
+  // are both likely programmer error, and not what you want,
+  // particularly for destructive operations.
+  if (!selector || (('_id' in selector) && !selector._id))
+    return function (doc) {return false;};
 
   // eval() does not return a value in IE8, nor does the spec say it
   // should. Assign to a local to get the value, instead.

packages/templating/deftemplate.js

@@ -25,7 +25,7 @@ Meteor._hook_handlebars_each = function () {
 
   var orig = Handlebars._default_helpers.each;
   Handlebars._default_helpers.each = function (context, options) {
-    if (!(context instanceof Collection.Query))
+    if (!(context instanceof Collection.Cursor))
       return orig(context, options);
 
     var id = Meteor._pending_partials_idx_nonce++;