From e9afb46edacf88f1736a6ff1a3e84397596f50d7 Mon Sep 17 00:00:00 2001 From: Amir Jafarian Date: Sat, 9 Apr 2016 10:59:09 -0400 Subject: [PATCH] Fix `notify` bugs for `find` --- lib/dao.js | 66 ++++++++++----- test/persistence-hooks.suite.js | 146 +++++++++++++------------------- 2 files changed, 100 insertions(+), 112 deletions(-) diff --git a/lib/dao.js b/lib/dao.js index 26502452..252ed4d2 100644 --- a/lib/dao.js +++ b/lib/dao.js @@ -1631,16 +1631,26 @@ DataAccessObject.find = function find(query, options, cb) { // do in memory query // using all documents // TODO [fabien] use default scope here? + if (options.notify === false) { + queryGeo(query); + } else { + withNotifyGeo(); + } - var context = { - Model: self, - query: query, - hookState: hookState, - options: options, - }; - self.notifyObserversOf('access', context, function(err, ctx) { - if (err) return cb(err); + function withNotifyGeo() { + var context = { + Model: self, + query: query, + hookState: hookState, + options: options, + }; + self.notifyObserversOf('access', context, function(err, ctx) { + if (err) return cb(err); + queryGeo(ctx.query); + }); + } + function queryGeo(query) { function geoCallback(err, data) { var memory = new Memory(); var modelName = self.modelName; @@ -1661,7 +1671,7 @@ DataAccessObject.find = function find(query, options, cb) { }); // FIXME: apply "includes" and other transforms - see allCb below - memory.all(modelName, ctx.query, options, cb); + memory.all(modelName, query, options, cb); } else { cb(null, []); } @@ -1672,8 +1682,7 @@ DataAccessObject.find = function find(query, options, cb) { } else { connector.all(self.modelName, {}, geoCallback); } - }); - + } // already handled return cb.promise; } @@ -1685,18 +1694,14 @@ DataAccessObject.find = function find(query, options, cb) { async.each(data, function(item, callback) { var d = item;//data[i]; var Model = self.lookupModel(d); + if (options.notify === false) { + buildResult(d); + } else { + withNotify(); + } - context = { - Model: Model, - data: d, - isNewInstance: false, - hookState: hookState, - options: options, - }; - - Model.notifyObserversOf('loaded', context, function(err) { - if (err) return callback(err); - d = context.data; + function buildResult(data) { + d = data; var ctorOpts = { fields: query.fields, @@ -1742,7 +1747,22 @@ DataAccessObject.find = function find(query, options, cb) { } else { callback(); } - }); + } + + function withNotify() { + context = { + Model: Model, + data: d, + isNewInstance: false, + hookState: hookState, + options: options, + }; + + Model.notifyObserversOf('loaded', context, function(err) { + if (err) return callback(err); + buildResult(context.data); + }); + } }, function(err) { if (err) return cb(err); diff --git a/test/persistence-hooks.suite.js b/test/persistence-hooks.suite.js index f5b3e0d7..aa12cc73 100644 --- a/test/persistence-hooks.suite.js +++ b/test/persistence-hooks.suite.js @@ -86,6 +86,45 @@ module.exports = function(dataSource, should, connectorCapabilities) { }); }); + it('should not trigger hooks, if notify is false', function(done) { + monitorHookExecution(); + TestModel.find( + { where: { id: '1' }}, + { notify: false }, + function(err, list) { + if (err) return done(err); + hookMonitor.names.should.be.empty(); + done(); + }); + }); + + it('should not trigger hooks for geo queries, if notify is false', + function(done) { + monitorHookExecution(); + + TestModel.find( + { where: { geo: { near: '10,20' }}}, + { notify: false }, + function(err, list) { + if (err) return done(err); + hookMonitor.names.should.be.empty(); + done(); + }); + }); + + it('should apply updates from `access` hook', function(done) { + TestModel.observe('access', function(ctx, next) { + ctx.query = { where: { name: 'second' }}; + next(); + }); + + TestModel.find({ name: 'first' }, function(err, list) { + if (err) return done(err); + list.map(get('name')).should.eql(['second']); + done(); + }); + }); + it('triggers `access` hook', function(done) { TestModel.observe('access', ctxRecorder.recordAndNext()); @@ -1664,24 +1703,13 @@ module.exports = function(dataSource, should, connectorCapabilities) { { id: existingInstance.id, name: 'new name' }, function(err, record, created) { if (err) return done(err); - if (dataSource.connector.updateOrCreate) { - hookMonitor.names.should.eql([ - 'access', - 'before save', - 'persist', - 'loaded', - 'after save', - ]); - } else { - hookMonitor.names.should.eql([ - 'access', - 'loaded', - 'before save', - 'persist', - 'loaded', - 'after save', - ]); - } + hookMonitor.names.should.eql([ + 'access', + 'before save', + 'persist', + 'loaded', + 'after save', + ]); done(); }); }); @@ -2017,25 +2045,14 @@ module.exports = function(dataSource, should, connectorCapabilities) { isNewInstance: false, })); } else { - // For Unoptimized connector, the callback function `contextRecorder.recordAndNext` - // is called twice. As a result, contextRecorder.records - // returns an array and NOT a single instance. - ctxRecorder.records.should.eql([ - aCtxForModel(TestModel, { - data: { - id: existingInstance.id, - name: 'first', - }, - isNewInstance: false, - options: { notify: false }, - }), + ctxRecorder.records.should.eql( aCtxForModel(TestModel, { data: { id: existingInstance.id, name: 'updated name', }, - }), - ]); + }) + ); } done(); }); @@ -2119,33 +2136,13 @@ module.exports = function(dataSource, should, connectorCapabilities) { { id: existingInstance.id, name: 'new name' }, function(err, record, created) { if (err) return done(err); - if (dataSource.connector.replaceOrCreate) { - hookMonitor.names.should.eql([ - 'access', - 'before save', - 'persist', - 'loaded', - 'after save', - ]); - } else { - // TODO: Please see loopback-datasource-juggler/issues#836 - // - // loaded hook is triggered twice in non-atomic version: - // 1) It gets triggered once by "find()" in this chain: - // "replaceORCreate()->findOne()->find()", - // which is a bug; Please see this ticket: - // loopback-datasource-juggler/issues#836. - // 2) It, also, gets triggered in "replaceAttributes()" - // in this chain replaceORCreate()->replaceAttributes() - hookMonitor.names.should.eql([ - 'access', - 'loaded', - 'before save', - 'persist', - 'loaded', - 'after save', - ]); - }; + hookMonitor.names.should.eql([ + 'access', + 'before save', + 'persist', + 'loaded', + 'after save', + ]); done(); }); }); @@ -2455,36 +2452,7 @@ module.exports = function(dataSource, should, connectorCapabilities) { connectorCapabilities.replaceOrCreateReportsNewInstance ? false : undefined; - if (dataSource.connector.replaceOrCreate) { - ctxRecorder.records.should.eql(aCtxForModel(TestModel, expected)); - } else { - // TODO: Please see loopback-datasource-juggler/issues#836 - // - // loaded hook is triggered twice in non-atomic version: - // 1) It gets triggered once by "find()" in this chain: - // "replaceORCreate()->findOne()->find()", - // which is a bug; Please see this ticket: - // loopback-datasource-juggler/issues#836. - // 2) It, also, gets triggered in "replaceAttributes()" - // in this chain replaceORCreate()->replaceAttributes() - ctxRecorder.records.should.eql([ - aCtxForModel(TestModel, { - data: { - id: existingInstance.id, - name: 'first', - }, - isNewInstance: false, - options: { notify: false }, - }), - aCtxForModel(TestModel, { - data: { - id: existingInstance.id, - name: 'replaced name', - }, - isNewInstance: false, - }), - ]); - } + ctxRecorder.records.should.eql(aCtxForModel(TestModel, expected)); done(); }); });