From cd71a37bfac2834e5df3f6010f51a6f0a354fbdc Mon Sep 17 00:00:00 2001 From: Simon Ho Date: Tue, 11 Aug 2015 23:02:29 -0700 Subject: [PATCH] Relax id requirement for basic query operations --- lib/dao.js | 31 +++++-- lib/model-definition.js | 22 +++++ test/basic-querying.test.js | 166 ++++++++++++++++++++++++++++++++++ test/model-definition.test.js | 1 - 4 files changed, 213 insertions(+), 7 deletions(-) diff --git a/lib/dao.js b/lib/dao.js index 0743b059..baa77307 100644 --- a/lib/dao.js +++ b/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'); +} diff --git a/lib/model-definition.js b/lib/model-definition.js index 21094812..ed037131 100644 --- a/lib/model-definition.js +++ b/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; +}; diff --git a/test/basic-querying.test.js b/test/basic-querying.test.js index dabcccec..711d42f7 100644 --- a/test/basic-querying.test.js +++ b/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 = [ { diff --git a/test/model-definition.test.js b/test/model-definition.test.js index 2f480946..df3bbed3 100644 --- a/test/model-definition.test.js +++ b/test/model-definition.test.js @@ -401,4 +401,3 @@ describe('ModelDefinition class', function () { props.regular.type.should.equal(props.sugar.type); }); }); -