diff --git a/lib/dao.js b/lib/dao.js index 257177b3..52a82641 100644 --- a/lib/dao.js +++ b/lib/dao.js @@ -285,21 +285,39 @@ DataAccessObject.create = function (data, options, cb) { } obj.__persisted = true; - saveDone.call(obj, function () { - createDone.call(obj, function () { - if (err) { - return cb(err, obj); - } - var context = { - Model: Model, - instance: obj, - isNewInstance: true, - hookState: hookState, - options: options - }; - Model.notifyObserversOf('after save', context, function(err) { - cb(err, obj); - if(!err) Model.emit('changed', obj); + var context = { + Model: Model, + data: val, + isNewInstance: true, + hookState: hookState, + options: options + }; + Model.notifyObserversOf('loaded', context, function(err) { + + // By default, the instance passed to create callback is NOT updated + // with the changes made through persist/loaded hooks. To preserve + // backwards compatibility, we introduced a new setting updateOnLoad, + // which if set, will apply these changes to the model instance too. + if(Model.settings.updateOnLoad) { + obj.setAttributes(context.data); + } + saveDone.call(obj, function () { + createDone.call(obj, function () { + if (err) { + return cb(err, obj); + } + var context = { + Model: Model, + instance: obj, + isNewInstance: true, + hookState: hookState, + options: options + }; + + Model.notifyObserversOf('after save', context, function(err) { + cb(err, obj); + if(!err) Model.emit('changed', obj); + }); }); }); }); @@ -475,33 +493,42 @@ DataAccessObject.updateOrCreate = DataAccessObject.upsert = function upsert(data }); } function done(err, data, info) { - var obj; - if (data && !(data instanceof Model)) { - inst._initProperties(data, { persisted: true }); - obj = inst; - } else { - obj = data; - } - if (err) { - cb(err, obj); - if(!err) { - Model.emit('changed', inst); + var context = { + Model: Model, + data: data, + hookState: ctx.hookState, + options: options + }; + Model.notifyObserversOf('loaded', context, function(err) { + var obj; + if (data && !(data instanceof Model)) { + inst._initProperties(data, { persisted: true }); + obj = inst; + } else { + obj = data; } - } else { - var context = { - Model: Model, - instance: obj, - isNewInstance: info ? info.isNewInstance : undefined, - hookState: hookState, - options: options - }; - Model.notifyObserversOf('after save', context, function(err) { + if (err) { cb(err, obj); if(!err) { Model.emit('changed', inst); } - }); - } + } else { + var context = { + Model: Model, + instance: obj, + isNewInstance: info ? info.isNewInstance : undefined, + hookState: hookState, + options: options + }; + + Model.notifyObserversOf('after save', context, function(err) { + cb(err, obj); + if(!err) { + Model.emit('changed', inst); + } + }); + } + }); } }); } else { @@ -586,36 +613,46 @@ DataAccessObject.findOrCreate = function findOrCreate(query, data, options, cb) function _findOrCreate(query, data, currentInstance) { var modelName = self.modelName; function findOrCreateCallback(err, data, created) { - var obj, Model = self.lookupModel(data); + var context = { + Model: Model, + data: data, + isNewInstance: created, + hookState: hookState, + options: options + }; + Model.notifyObserversOf('loaded', context, function(err) { + var obj, Model = self.lookupModel(data); - if (data) { - obj = new Model(data, {fields: query.fields, applySetters: false, - persisted: true}); - } + if (data) { + obj = new Model(data, {fields: query.fields, applySetters: false, + persisted: true}); + } - if (created) { - var context = { - Model: Model, - instance: obj, - isNewInstance: true, - hookState: hookState, - options: options - }; - Model.notifyObserversOf('after save', context, function(err) { + if (created) { + var context = { + Model: Model, + instance: obj, + isNewInstance: true, + hookState: hookState, + options: options + }; + Model.notifyObserversOf('after save', context, function(err) { + if (cb.promise) { + cb(err, [obj, created]); + } else { + cb(err, obj, created); + } + if (!err) Model.emit('changed', obj); + }); + + } else { if (cb.promise) { cb(err, [obj, created]); } else { cb(err, obj, created); } - if (!err) Model.emit('changed', obj); - }); - } else { - if (cb.promise) { - cb(err, [obj, created]); - } else { - cb(err, obj, created); } - } + }); } data = removeUndefined(data); @@ -1329,38 +1366,50 @@ DataAccessObject.find = function find(query, options, cb) { var Model = self.lookupModel(d); var obj = new Model(d, {fields: query.fields, applySetters: false, persisted: true}); - if (query && query.include) { - if (query.collect) { - // The collect property indicates that the query is to return the - // standalone items for a related model, not as child of the parent object - // For example, article.tags - obj = obj.__cachedRelations[query.collect]; - if (obj === null) { - obj = undefined; - } - } else { - // This handles the case to return parent items including the related - // models. For example, Article.find({include: 'tags'}, ...); - // Try to normalize the include - var includes = Inclusion.normalizeInclude(query.include || []); - includes.forEach(function(inc) { - var relationName = inc; - if (utils.isPlainObject(inc)) { - relationName = Object.keys(inc)[0]; - } + context = { + Model: Model, + instance: obj, + isNewInstance: false, + hookState: hookState, + options: options + }; - // Promote the included model as a direct property - var included = obj.__cachedRelations[relationName]; - if (Array.isArray(included)) { - included = new List(included, null, obj); + if (query && query.include) { + Model.notifyObserversOf('loaded', context, function(err) { + if (query.collect) { + // The collect property indicates that the query is to return the + // standalone items for a related model, not as child of the parent object + // For example, article.tags + obj = obj.__cachedRelations[query.collect]; + if (obj === null) { + obj = undefined; } - if (included) obj.__data[relationName] = included; - }); - delete obj.__data.__cachedRelations; - } + } else { + // This handles the case to return parent items including the related + // models. For example, Article.find({include: 'tags'}, ...); + // Try to normalize the include + var includes = Inclusion.normalizeInclude(query.include || []); + includes.forEach(function(inc) { + var relationName = inc; + if (utils.isPlainObject(inc)) { + relationName = Object.keys(inc)[0]; + } + + // Promote the included model as a direct property + var included = obj.__cachedRelations[relationName]; + if (Array.isArray(included)) { + included = new List(included, null, obj); + } + if (included) obj.__data[relationName] = included; + }); + delete obj.__data.__cachedRelations; + } + }); } if (obj !== undefined) { - results.push(obj); + Model.notifyObserversOf('loaded', context, function(err) { + results.push(obj); + }); } } @@ -1803,22 +1852,33 @@ DataAccessObject.prototype.save = function (options, cb) { if (err) { return cb(err, inst); } - inst._initProperties(data, { persisted: true }); + var context = { Model: Model, - instance: inst, + data: data, isNewInstance: result && result.isNewInstance, hookState: hookState, options: options }; - Model.notifyObserversOf('after save', context, function(err) { - if (err) return cb(err, inst); - updateDone.call(inst, function () { - saveDone.call(inst, function () { - cb(err, inst); - if(!err) { - Model.emit('changed', inst); - } + Model.notifyObserversOf('loaded', context, function(err) { + inst._initProperties(data, { persisted: true }); + + var context = { + Model: Model, + instance: inst, + isNewInstance: result && result.isNewInstance, + hookState: hookState, + options: options + }; + Model.notifyObserversOf('after save', context, function(err) { + if (err) return cb(err, inst); + updateDone.call(inst, function () { + saveDone.call(inst, function () { + cb(err, inst); + if(!err) { + Model.emit('changed', inst); + } + }); }); }); }); @@ -1956,6 +2016,7 @@ DataAccessObject.updateAll = function (where, data, options, cb) { function updateCallback(err, info) { if (err) return cb (err); + var context = { Model: Model, where: where, @@ -2291,20 +2352,37 @@ DataAccessObject.prototype.updateAttributes = function updateAttributes(data, op } function updateAttributesCallback(err) { - if (!err) inst.__persisted = true; - done.call(inst, function () { - saveDone.call(inst, function () { - if (err) return cb(err, inst); - var context = { - Model: Model, - instance: inst, - isNewInstance: false, - hookState: hookState, - options: options - }; - Model.notifyObserversOf('after save', context, function(err) { - if(!err) Model.emit('changed', inst); - cb(err, inst); + var context = { + Model: Model, + data: data, + hookState: hookState, + options: options + }; + Model.notifyObserversOf('loaded', context, function(err) { + if (!err) inst.__persisted = true; + + // By default, the instance passed to updateAttributes callback is NOT updated + // with the changes made through persist/loaded hooks. To preserve + // backwards compatibility, we introduced a new setting updateOnLoad, + // which if set, will apply these changes to the model instance too. + if(Model.settings.updateOnLoad) { + inst.setAttributes(context.data); + } + done.call(inst, function () { + saveDone.call(inst, function () { + if (err) return cb(err, inst); + + var context = { + Model: Model, + instance: inst, + isNewInstance: false, + hookState: hookState, + options: options + }; + Model.notifyObserversOf('after save', context, function(err) { + if(!err) Model.emit('changed', inst); + cb(err, inst); + }); }); }); }); diff --git a/test/persistence-hooks.suite.js b/test/persistence-hooks.suite.js index ffc2fff9..93f7f53f 100644 --- a/test/persistence-hooks.suite.js +++ b/test/persistence-hooks.suite.js @@ -6,6 +6,7 @@ module.exports = function(dataSource, should) { var observedContexts, expectedError, observersCalled; var TestModel, existingInstance; var migrated = false, lastId; + var triggered; var undefinedValue = undefined; @@ -112,6 +113,24 @@ module.exports = function(dataSource, should) { }); describe('PersistedModel.create', function() { + it('triggers hooks in the correct order', function(done) { + monitorHookExecution(); + + TestModel.create( + { name: 'created' }, + function(err, record, created) { + if (err) return done(err); + + triggered.should.eql([ + 'before save', + 'persist', + 'loaded', + 'after save' + ]); + done(); + }); + }); + it('triggers `before save` hook', function(done) { TestModel.observe('before save', pushContextAndNext()); @@ -210,16 +229,19 @@ module.exports = function(dataSource, should) { ctx.data.extra = 'hook data'; })); + // By default, the instance passed to create callback is NOT updated + // with the changes made through persist/loaded hooks. To preserve + // backwards compatibility, we introduced a new setting updateOnLoad, + // which if set, will apply these changes to the model instance too. + TestModel.settings.updateOnLoad = true; TestModel.create( { id: 'new-id', name: 'a name' }, function(err, instance) { if (err) return done(err); - // the, instance returned by `create` context does not have the - // values updated from `persist` hook - instance.should.not.have.property('extra', 'hook data'); + instance.should.have.property('extra', 'hook data'); - // So, we must query the database here because on `create` + // Also query the database here to verify that, on `create` // updates from `persist` hook are reflected into database TestModel.findById('new-id', function(err, dbInstance) { if (err) return done(err); @@ -234,6 +256,48 @@ module.exports = function(dataSource, should) { }); }); + it('triggers `loaded` hook', function(done) { + TestModel.observe('loaded', pushContextAndNext()); + + // By default, the instance passed to create callback is NOT updated + // with the changes made through persist/loaded hooks. To preserve + // backwards compatibility, we introduced a new setting updateOnLoad, + // which if set, will apply these changes to the model instance too. + TestModel.settings.updateOnLoad = true; + TestModel.create( + { id: 'new-id', name: 'a name' }, + function(err, instance) { + if (err) return done(err); + + observedContexts.should.eql(aTestModelCtx({ + data: { id: 'new-id', name: 'a name' }, + isNewInstance: true + })); + + done(); + }); + }); + + it('applies updates from `loaded` hook', function(done) { + TestModel.observe('loaded', pushContextAndNext(function(ctx){ + ctx.data.extra = 'hook data'; + })); + + // By default, the instance passed to create callback is NOT updated + // with the changes made through persist/loaded hooks. To preserve + // backwards compatibility, we introduced a new setting updateOnLoad, + // which if set, will apply these changes to the model instance too. + TestModel.settings.updateOnLoad = true; + TestModel.create( + { id: 'new-id', name: 'a name' }, + function(err, instance) { + if (err) return done(err); + + instance.should.have.property('extra', 'hook data'); + done(); + }); + }); + it('triggers `after save` hook', function(done) { TestModel.observe('after save', pushContextAndNext()); @@ -405,12 +469,7 @@ module.exports = function(dataSource, should) { }); it('triggers hooks in the correct order when not found', function(done) { - var triggered = []; - TestModel._notify = TestModel.notifyObserversOf; - TestModel.notifyObserversOf = function(operation, context, callback) { - triggered.push(operation); - this._notify.apply(this, arguments); - }; + monitorHookExecution(); TestModel.findOrCreate( { where: { name: 'new-record' } }, @@ -421,12 +480,39 @@ module.exports = function(dataSource, should) { 'access', 'before save', 'persist', + 'loaded', 'after save' ]); done(); }); }); + it('triggers hooks in the correct order when found', function(done) { + monitorHookExecution(); + + TestModel.findOrCreate( + { where: { name: existingInstance.name } }, + { name: existingInstance.name }, + function(err, record, created) { + if (err) return done(err); + + if (dataSource.connector.findOrCreate) { + triggered.should.eql([ + 'access', + 'before save', + 'persist', + 'loaded' + ]); + } else { + triggered.should.eql([ + 'access', + 'loaded' + ]); + } + done(); + }); + }); + it('aborts when `access` hook fails', function(done) { TestModel.observe('access', nextWithError(expectedError)); @@ -596,6 +682,98 @@ module.exports = function(dataSource, should) { }); }); + if (dataSource.connector.findOrCreate) { + it('triggers `loaded` hook when found', function(done) { + TestModel.observe('loaded', pushContextAndNext()); + + TestModel.findOrCreate( + { where: { name: existingInstance.name } }, + { name: existingInstance.name }, + function(err, record, created) { + if (err) return done(err); + + record.id.should.eql(existingInstance.id); + + // After the call to `connector.findOrCreate`, since the record + // already exists, `data.id` matches `existingInstance.id` + // as against the behaviour noted for `persist` hook + observedContexts.should.eql(aTestModelCtx({ + data: { + id: existingInstance.id, + name: existingInstance.name + }, + isNewInstance: false + })); + + done(); + }); + }); + } + + it('triggers `loaded` hook when not found', function(done) { + TestModel.observe('loaded', pushContextAndNext()); + + TestModel.findOrCreate( + { where: { name: 'new-record' } }, + { name: 'new-record' }, + function(err, record, created) { + if (err) return done(err); + + observedContexts.should.eql(aTestModelCtx({ + data: { + id: record.id, + name: 'new-record' + }, + isNewInstance: true + })); + + done(); + }); + }); + + if (dataSource.connector.findOrCreate) { + it('applies updates from `loaded` hook when found', function(done) { + TestModel.observe('loaded', pushContextAndNext(function(ctx){ + ctx.data.extra = 'hook data'; + })); + + TestModel.findOrCreate( + { where: { name: existingInstance.name } }, + { name: existingInstance.name }, + function(err, instance) { + if (err) return done(err); + + instance.should.have.property('extra', 'hook data'); + + done(); + }); + }); + } + + it('applies updates from `loaded` hook when not found', function(done) { + TestModel.observe('loaded', pushContextAndNext(function(ctx){ + ctx.data.extra = 'hook data'; + })); + + // Unoptimized connector gives a call to `create. But, + // by default, the instance passed to create callback is NOT updated + // with the changes made through persist/loaded hooks. To preserve + // backwards compatibility, we introduced a new setting updateOnLoad, + // which if set, will apply these changes to the model instance too. + // Note - in case of findOrCreate, this setting is needed ONLY for + // unoptimized connector. + TestModel.settings.updateOnLoad = true; + TestModel.findOrCreate( + { where: { name: 'new-record' } }, + { name: 'new-record' }, + function(err, instance) { + if (err) return done(err); + + instance.should.have.property('extra', 'hook data'); + done(); + }); + }); + it('triggers `after save` hook when not found', function(done) { TestModel.observe('after save', pushContextAndNext()); @@ -658,6 +836,22 @@ module.exports = function(dataSource, should) { }); describe('PersistedModel.prototype.save', function() { + it('triggers hooks in the correct order', function(done) { + monitorHookExecution(); + + existingInstance.save( + function(err, record, created) { + if (err) return done(err); + triggered.should.eql([ + 'before save', + 'persist', + 'loaded', + 'after save' + ]); + done(); + }); + }); + it('triggers `before save` hook', function(done) { TestModel.observe('before save', pushContextAndNext()); @@ -745,6 +939,38 @@ module.exports = function(dataSource, should) { }); }); + it('triggers `loaded` hook', function(done) { + TestModel.observe('loaded', pushContextAndNext()); + + existingInstance.name = 'changed'; + existingInstance.save(function(err, instance) { + if (err) return done(err); + + observedContexts.should.eql(aTestModelCtx({ + data: { + id: existingInstance.id, + name: 'changed' + }, + isNewInstance: false, + options: { throws: false, validate: true } + })); + + done(); + }); + }); + + it('applies updates from `loaded` hook', function(done) { + TestModel.observe('loaded', pushContextAndNext(function(ctx){ + ctx.data.extra = 'hook data'; + })); + + existingInstance.save(function(err, instance) { + if (err) return done(err); + instance.should.have.property('extra', 'hook data'); + done(); + }); + }); + it('triggers `after save` hook on update', function(done) { TestModel.observe('after save', pushContextAndNext()); @@ -811,6 +1037,23 @@ module.exports = function(dataSource, should) { }); describe('PersistedModel.prototype.updateAttributes', function() { + it('triggers hooks in the correct order', function(done) { + monitorHookExecution(); + + existingInstance.updateAttributes( + { name: 'changed' }, + function(err, record, created) { + if (err) return done(err); + triggered.should.eql([ + 'before save', + 'persist', + 'loaded', + 'after save' + ]); + done(); + }); + }); + it('triggers `before save` hook', function(done) { TestModel.observe('before save', pushContextAndNext()); @@ -894,7 +1137,42 @@ module.exports = function(dataSource, should) { ctx.data.extra = 'hook data'; })); - existingInstance.save(function(err, instance) { + // By default, the instance passed to updateAttributes callback is NOT updated + // with the changes made through persist/loaded hooks. To preserve + // backwards compatibility, we introduced a new setting updateOnLoad, + // which if set, will apply these changes to the model instance too. + TestModel.settings.updateOnLoad = true; + existingInstance.updateAttributes({ name: 'changed' }, function(err, instance) { + if (err) return done(err); + instance.should.have.property('extra', 'hook data'); + done(); + }); + }); + + it('triggers `loaded` hook', function(done) { + TestModel.observe('loaded', pushContextAndNext()); + existingInstance.updateAttributes({ name: 'changed' }, function(err) { + if (err) return done(err); + + observedContexts.should.eql(aTestModelCtx({ + data: { name: 'changed' } + })); + + done(); + }); + }); + + it('applies updates from `loaded` hook updateAttributes', function(done) { + TestModel.observe('loaded', pushContextAndNext(function(ctx){ + ctx.data.extra = 'hook data'; + })); + + // By default, the instance passed to updateAttributes callback is NOT updated + // with the changes made through persist/loaded hooks. To preserve + // backwards compatibility, we introduced a new setting updateOnLoad, + // which if set, will apply these changes to the model instance too. + TestModel.settings.updateOnLoad = true; + existingInstance.updateAttributes({ name: 'changed' }, function(err, instance) { if (err) return done(err); instance.should.have.property('extra', 'hook data'); done(); @@ -944,6 +1222,53 @@ module.exports = function(dataSource, should) { }); describe('PersistedModel.updateOrCreate', function() { + it('triggers hooks in the correct order on create', function(done) { + monitorHookExecution(); + + TestModel.updateOrCreate( + { id: 'not-found', name: 'not found' }, + function(err, record, created) { + if (err) return done(err); + triggered.should.eql([ + 'access', + 'before save', + 'persist', + 'loaded', + 'after save' + ]); + done(); + }); + }); + + it('triggers hooks in the correct order on update', function(done) { + monitorHookExecution(); + + TestModel.updateOrCreate( + { id: existingInstance.id, name: 'new name' }, + function(err, record, created) { + if (err) return done(err); + if (dataSource.connector.updateOrCreate) { + triggered.should.eql([ + 'access', + 'before save', + 'persist', + 'loaded', + 'after save' + ]); + } else { + triggered.should.eql([ + 'access', + 'loaded', + 'before save', + 'persist', + 'loaded', + 'after save' + ]); + } + done(); + }); + }); + it('triggers `access` hook on create', function(done) { TestModel.observe('access', pushContextAndNext()); @@ -1225,6 +1550,71 @@ module.exports = function(dataSource, should) { }); }); + it('triggers `loaded` hook on create', function(done) { + TestModel.observe('loaded', pushContextAndNext()); + + TestModel.updateOrCreate( + { id: 'new-id', name: 'a name' }, + function(err, instance) { + if (err) return done(err); + + if (dataSource.connector.updateOrCreate) { + observedContexts.should.eql(aTestModelCtx({ + data: { id: 'new-id', name: 'a name' } + })); + } else { + observedContexts.should.eql(aTestModelCtx({ + data: { + id: 'new-id', + name: 'a name' + }, + isNewInstance: true + })); + } + done(); + }); + }); + + it('triggers `loaded` hook on update', function(done) { + TestModel.observe('loaded', pushContextAndNext()); + + TestModel.updateOrCreate( + { id: existingInstance.id, name: 'updated name' }, + function(err, instance) { + if (err) return done(err); + + if (dataSource.connector.updateOrCreate) { + observedContexts.should.eql(aTestModelCtx({ + data: { + id: existingInstance.id, + name: 'updated name' + } + })); + } else { + // For Unoptimized connector, the callback function `pushContextAndNext` + // is called twice. As a result, observedContexts + // returns an array and NOT a single instance. + observedContexts.should.eql([ + aTestModelCtx({ + instance: { + id: existingInstance.id, + name: 'first', + extra: null + }, + isNewInstance: false, + options: { notify: false } + }), + aTestModelCtx({ + data: { + id: existingInstance.id, + name: 'updated name' + } + }) + ]); + } + done(); + }); + }); it('triggers `after save` hook on update', function(done) { TestModel.observe('after save', pushContextAndNext()); @@ -1646,6 +2036,19 @@ module.exports = function(dataSource, should) { }); }); + it('does not trigger `loaded`', function(done) { + TestModel.observe('loaded', pushContextAndNext()); + + TestModel.updateAll( + { where: { id: existingInstance.id } }, + { name: 'changed' }, + function(err, instance) { + if (err) return done(err); + observedContexts.should.eql("hook not called"); + done(); + }); + }); + it('triggers `after save` hook', function(done) { TestModel.observe('after save', pushContextAndNext()); @@ -1759,6 +2162,15 @@ module.exports = function(dataSource, should) { function getLastGeneratedUid() { return '' + lastId; } + + function monitorHookExecution() { + triggered = []; + TestModel._notify = TestModel.notifyObserversOf; + TestModel.notifyObserversOf = function(operation, context, callback) { + triggered.push(operation); + this._notify.apply(this, arguments); + }; + } }); function deepCloneToObject(obj) {