Merge pull request #689 from strongloop/relax-id-requirement

Relax id requirement for basic query operations

lib/dao.js

@@ -856,7 +856,9 @@ DataAccessObject.findById = function findById(id, filter, options, cb) {
   assert(typeof options === 'object', 'The options argument must be an object');
   assert(typeof cb === 'function', 'The cb argument must be a function');
 
-  if (id == null || id === '') {
+  if (!this.definition.hasIdDefined()) {
+    process.nextTick(cb(getIdDefinitionError()));
+  } else if (id == null || id === '') {
     process.nextTick(function() {
       cb(new Error('Model::findById requires the id argument'));
     });
@@ -904,13 +906,16 @@ DataAccessObject.findByIds = function(ids, query, options, cb) {
   assert(typeof options === 'object', 'The options argument must be an object');
   assert(typeof cb === 'function', 'The cb argument must be a function');
 
-  var pk = idName(this);
-  if (ids.length === 0) {
-    process.nextTick(function() { cb(null, []); });
-    return cb.promise;;
+  if (!this.definition.hasIdDefined()) {
+    process.nextTick(cb(getIdDefinitionError()));
+    return cb.promise;
+  } else if (ids.length === 0) {
+    process.nextTick(cb(null, []));
+    return cb.promise;
   }
 
   var filter = { where: {} };
+  var pk = idName(this);
   filter.where[pk] = { inq: [].concat(ids) };
   mergeQuery(filter, query || {});
   this.find(filter, options, function(err, results) {
@@ -1692,7 +1697,10 @@ DataAccessObject.removeById = DataAccessObject.destroyById = DataAccessObject.de
   assert(typeof options === 'object', 'The options argument must be an object');
   assert(typeof cb === 'function', 'The cb argument must be a function');
 
-  if (id == null || id === '') {
+  if (!this.definition.hasIdDefined()) {
+    process.nextTick(cb(getIdDefinitionError()));
+    return cb.promise;
+  } else if (id == null || id === '') {
     process.nextTick(function() {
       cb(new Error('Model::deleteById requires the id argument'));
     });
@@ -2533,3 +2541,14 @@ jutil.mixin(DataAccessObject, Relation);
  * Add 'transaction'
  */
 jutil.mixin(DataAccessObject, require('./transaction'));
+
+/**
+ * Return an error object stating the `id` property must be part of the model
+ * definition.
+ *
+ * @return {Object} The error stating the requirements
+ */
+function getIdDefinitionError() {
+  return new Error('`id` property must be defined as part of the model ' +
+    'definition');
+}

lib/model-definition.js

@@ -302,3 +302,25 @@ ModelDefinition.prototype.toJSON = function (forceRebuild) {
   this.json = json;
   return json;
 };
+
+/**
+ * Checks if the id property has been defined in the model definition. Returns
+ * true if the id is found in the model definition properties OR in at least one
+ * id object from the _id property.
+ *
+ * @returns {Boolean} True if id is defined, false otherwise
+ */
+ModelDefinition.prototype.hasIdDefined = function() {
+  var isDefined = false;
+
+  if (this.properties.id)
+    isDefined = true;
+
+  // check if id is defined by a relation
+  this.ids().forEach(function(item) {
+    if (item.hasOwnProperty('id'))
+      isDefined = true;
+  });
+
+  return isDefined;
+};

test/basic-querying.test.js

@@ -620,6 +620,172 @@ describe('basic-querying', function () {
   });
 });
 
+describe('queries', function() {
+  var Todo;
+
+  before(function setupDb(done) {
+    var db = getSchema();
+    Todo = db.define('Todo', {
+      id: false,
+      content: {type: 'string'}
+    }, {
+      idInjection: false
+    });
+    db.automigrate(done);
+  });
+  beforeEach(function resetFixtures(done) {
+    Todo.destroyAll(function() {
+      Todo.create([
+        {content: 'Buy eggs'},
+        {content: 'Buy milk'},
+        {content: 'Buy sausages'}
+      ], done);
+    });
+  });
+
+  context('that do not require an id', function() {
+    it('should work for create', function(done) {
+      Todo.create({content: 'Buy ham'}, function(err) {
+        should.not.exist(err);
+        done();
+      });
+    });
+
+    it('should work for updateOrCreate/upsert', function(done) {
+      var aliases = ['updateOrCreate', 'upsert'];
+      async.each(aliases, function(alias, cb) {
+        Todo[alias]({content: 'Buy ham'}, function(err) {
+          should.not.exist(err);
+          cb();
+        });
+      }, done);
+    });
+
+    it('should work for findOrCreate', function(done) {
+      Todo.findOrCreate({content: 'Buy ham'}, function(err) {
+        should.not.exist(err);
+        done();
+      });
+    });
+
+    it('should work for exists', function(done) {
+      Todo.exists({content: 'Buy ham'}, function(err) {
+        should.not.exist(err);
+        done();
+      });
+    });
+
+    it('should work for find', function(done) {
+      Todo.find(function(err) {
+        should.not.exist(err);
+        done();
+      });
+    });
+
+    it('should work for findOne', function(done) {
+      Todo.findOne(function(err) {
+        should.not.exist(err);
+        done();
+      });
+    });
+
+    it('should work for deleteAll/destroyAll/remove', function(done) {
+      // FIXME: We should add a DAO.delete static method alias for consistency
+      // (DAO.prototype.delete instance method already exists)
+      var aliases = ['deleteAll', 'destroyAll', 'remove'];
+      async.each(aliases, function(alias, cb) {
+        Todo[alias](function(err) {
+          should.not.exist(err);
+          cb();
+        });
+      }, done);
+    });
+
+    it('should work for update/updateAll', function(done) {
+      Todo.update({content: 'Buy ham'}, function(err) {
+        should.not.exist(err);
+        done();
+      });
+    });
+
+    it('should work for count', function(done) {
+      Todo.count({content: 'Buy eggs'}, function(err) {
+        should.not.exist(err);
+        done();
+      });
+    });
+
+    it('should work for save', function(done) {
+      var todo = new Todo();
+      todo.content = 'Buy ham';
+      todo.save(function(err) {
+        should.not.exist(err);
+        done();
+      });
+    });
+
+    it('should work for delete', function(done) {
+      Todo.findOne(function(err, todo) {
+        todo.delete(function(err) {
+          should.not.exist(err);
+          done();
+        });
+      });
+    });
+
+    it('should work for updateAttribute', function(done) {
+      Todo.findOne(function(err, todo) {
+        todo.updateAttribute('content', 'Buy ham', function(err) {
+          should.not.exist(err);
+          done();
+        });
+      });
+    });
+
+    it('should work for updateAttributes', function(done) {
+      Todo.findOne(function(err, todo) {
+        todo.updateAttributes({content: 'Buy ham'}, function(err) {
+          should.not.exist(err);
+          done();
+        });
+      });
+    });
+  });
+
+  context('that require an id', function() {
+    var expectedErrMsg = '`id` property must be defined as part of the model ' +
+        'definition';
+
+    it('should return an error for findById', function(done) {
+      Todo.findById(1, function(err) {
+        should.exist(err);
+        err.message.should.equal(expectedErrMsg);
+        done();
+      });
+    });
+
+    it('should return an error for findByIds', function(done) {
+      Todo.findByIds([1, 2], function(err) {
+        should.exist(err);
+        err.message.should.equal(expectedErrMsg);
+        done();
+      });
+    });
+
+    it('should return an error for deleteById/destroyById/removeById',
+        function(done) {
+      var aliases = ['deleteById', 'destroyById', 'removeById'];
+      async.each(aliases, function(alias, cb) {
+        Todo[alias](1, function(err) {
+          should.exist(err);
+          err.message.should.equal(expectedErrMsg);
+          cb();
+        });
+      }, done);
+    });
+  });
+});
+
 function seed(done) {
   var beatles = [
     {

test/model-definition.test.js

@@ -401,4 +401,3 @@ describe('ModelDefinition class', function () {
     props.regular.type.should.equal(props.sugar.type);
   });
 });
-