From 2120e53f7f64135dc5a97cf4425a0c41d4778493 Mon Sep 17 00:00:00 2001 From: Fabien Franzen Date: Fri, 20 Mar 2015 17:49:32 +0100 Subject: [PATCH] Indicate result of destroyById/protototype.destroy Return `info.count` to the callback to indicate whether the model instance was deleted or not. When Model's `settings.strictDelete` is true, return 404 error when the model instance was not found. --- lib/connectors/memory.js | 3 +- lib/dao.js | 39 +++++++++--- test/crud-with-options.test.js | 10 +-- test/manipulation.test.js | 110 ++++++++++++++++++++++++++++++++- 4 files changed, 140 insertions(+), 22 deletions(-) diff --git a/lib/connectors/memory.js b/lib/connectors/memory.js index 8cb986e3..506b478c 100644 --- a/lib/connectors/memory.js +++ b/lib/connectors/memory.js @@ -265,8 +265,9 @@ Memory.prototype.find = function find(model, id, options, callback) { }; Memory.prototype.destroy = function destroy(model, id, options, callback) { + var exists = this.collection(model)[id]; delete this.collection(model)[id]; - this.saveToFile(null, callback); + this.saveToFile({ count: exists ? 1 : 0 }, callback); }; Memory.prototype.fromDb = function (model, data) { diff --git a/lib/dao.js b/lib/dao.js index d69ae93e..2636a650 100644 --- a/lib/dao.js +++ b/lib/dao.js @@ -1697,11 +1697,18 @@ DataAccessObject.removeById = DataAccessObject.destroyById = DataAccessObject.de var Model = this; - this.remove(byIdQuery(this, id).where, options, function(err) { - if ('function' === typeof cb) { - cb(err); + this.remove(byIdQuery(this, id).where, options, function(err, info) { + if (err) return cb(err); + var deleted = info && info.count > 0; + if (Model.settings.strictDelete && !deleted) { + err = new Error('No instance with id ' + id + ' found for ' + Model.modelName); + err.code = 'NOT_FOUND'; + err.statusCode = 404; + return cb(err); } - if(!err) Model.emit('deleted', id); + + cb(null, info); + Model.emit('deleted', id); }); return cb.promise; }; @@ -2149,8 +2156,15 @@ DataAccessObject.prototype.remove = // A hook modified the query, it is no longer // a simple 'delete model with the given id'. // We must switch to full query-based delete. - Model.deleteAll(where, { notify: false }, function(err) { - if (err) return cb(err); + Model.deleteAll(where, { notify: false }, function(err, info) { + if (err) return cb(err, false); + var deleted = info && info.count > 0; + if (Model.settings.strictDelete && !deleted) { + err = new Error('No instance with id ' + id + ' found for ' + Model.modelName); + err.code = 'NOT_FOUND'; + err.statusCode = 404; + return cb(err, false); + } var context = { Model: Model, where: where, @@ -2159,7 +2173,7 @@ DataAccessObject.prototype.remove = options: options }; Model.notifyObserversOf('after delete', context, function(err) { - cb(err); + cb(err, info); if (!err) Model.emit('deleted', id); }); }); @@ -2167,8 +2181,13 @@ DataAccessObject.prototype.remove = } inst.trigger('destroy', function (destroyed) { - function destroyCallback(err) { - if (err) { + function destroyCallback(err, info) { + if (err) return cb(err); + var deleted = info && info.count > 0; + if (Model.settings.strictDelete && !deleted) { + err = new Error('No instance with id ' + id + ' found for ' + Model.modelName); + err.code = 'NOT_FOUND'; + err.statusCode = 404; return cb(err); } @@ -2181,7 +2200,7 @@ DataAccessObject.prototype.remove = options: options }; Model.notifyObserversOf('after delete', context, function(err) { - cb(err); + cb(err, info); if (!err) Model.emit('deleted', id); }); }); diff --git a/test/crud-with-options.test.js b/test/crud-with-options.test.js index a44fcd76..9a5798cb 100644 --- a/test/crud-with-options.test.js +++ b/test/crud-with-options.test.js @@ -413,12 +413,12 @@ describe('crud-with-options', function () { it('should allow save(options, cb)', function (done) { var options = { foo: 'bar' }; var opts; - + User.observe('after save', function(ctx, next) { opts = ctx.options; next(); }); - + var u = new User(); u.save(options, function(err) { should.not.exist(err); @@ -480,12 +480,6 @@ describe('crud-with-options', function () { }); - describe('deleteById', function() { - it('should allow deleteById(id)', function () { - User.deleteById(1); - }); - }); - describe('updateAll ', function () { beforeEach(seed); diff --git a/test/manipulation.test.js b/test/manipulation.test.js index 3877f44e..aa516a47 100644 --- a/test/manipulation.test.js +++ b/test/manipulation.test.js @@ -490,7 +490,7 @@ describe('manipulation', function () { }); }); }); - + it('should ignore unknown attributes when strict: true', function(done) { person.updateAttributes({foo:'bar'}, function(err, p) { @@ -503,7 +503,7 @@ describe('manipulation', function () { }); }); }); - + it('should throw error on unknown attributes when strict: throw', function(done) { Person.definition.settings.strict = 'throw'; Person.findById(person.id, function(err, p) { @@ -521,7 +521,7 @@ describe('manipulation', function () { }); }); }); - + it('should throw error on unknown attributes when strict: throw', function(done) { Person.definition.settings.strict = 'validate'; Person.findById(person.id, function(err, p) { @@ -866,6 +866,92 @@ describe('manipulation', function () { }); }); + describe('deleteById', function() { + beforeEach(givenSomePeople); + afterEach(function() { + Person.settings.strictDelete = false; + }); + + it('should allow deleteById(id) - success', function (done) { + Person.findOne(function (e, p) { + Person.deleteById(p.id, function(err, info) { + if (err) return done(err); + info.should.have.property('count', 1); + done(); + }); + }); + }); + + it('should allow deleteById(id) - fail', function (done) { + Person.settings.strictDelete = false; + Person.deleteById(9999, function(err, info) { + if (err) return done(err); + info.should.have.property('count', 0); + done(); + }); + }); + + it('should allow deleteById(id) - fail with error', function (done) { + Person.settings.strictDelete = true; + Person.deleteById(9999, function(err) { + should.exist(err); + err.message.should.equal('No instance with id 9999 found for Person'); + err.should.have.property('code', 'NOT_FOUND'); + err.should.have.property('statusCode', 404); + done(); + }); + }); + }); + + describe('prototype.delete', function() { + beforeEach(givenSomePeople); + afterEach(function() { + Person.settings.strictDelete = false; + }); + + it('should allow delete(id) - success', function (done) { + Person.findOne(function (e, p) { + p.delete(function(err, info) { + if (err) return done(err); + info.should.have.property('count', 1); + done(); + }); + }); + }); + + it('should allow delete(id) - fail', function (done) { + Person.settings.strictDelete = false; + Person.findOne(function (e, p) { + p.delete(function(err, info) { + if (err) return done(err); + info.should.have.property('count', 1); + p.delete(function(err, info) { + if (err) return done(err); + info.should.have.property('count', 0); + done(); + }); + }); + }); + }); + + it('should allow delete(id) - fail with error', function (done) { + Person.settings.strictDelete = true; + Person.findOne(function (e, u) { + u.delete(function(err, info) { + if (err) return done(err); + info.should.have.property('count', 1); + u.delete(function(err) { + should.exist(err); + err.message.should.equal('No instance with id ' + u.id + ' found for Person'); + err.should.have.property('code', 'NOT_FOUND'); + err.should.have.property('statusCode', 404); + done(); + }); + }); + }); + }); + }); + describe('initialize', function () { it('should initialize object properly', function () { var hw = 'Hello word', @@ -1162,3 +1248,21 @@ describe('manipulation', function () { }); }); }); + +function givenSomePeople(done) { + var beatles = [ + { name: 'John Lennon', gender: 'male' }, + { name: 'Paul McCartney', gender: 'male' }, + { name: 'George Harrison', gender: 'male' }, + { name: 'Ringo Starr', gender: 'male' }, + { name: 'Pete Best', gender: 'male' }, + { name: 'Stuart Sutcliffe', gender: 'male' } + ]; + + async.series([ + Person.destroyAll.bind(Person), + function(cb) { + async.each(beatles, Person.create.bind(Person), cb); + } + ], done); +}