Fix `notify` bugs for `find`

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 = {
+      function withNotifyGeo() {
-        Model: self,
+        var context = {
-        query: query,
+          Model: self,
-        hookState: hookState,
+          query: query,
-        options: options,
+          hookState: hookState,
-      };
+          options: options,
-      self.notifyObserversOf('access', context, function(err, ctx) {
+        };
-        if (err) return cb(err);
+        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 = {
+        function buildResult(data) {
-          Model: Model,
+          d = data;
-          data: d,
-          isNewInstance: false,
-          hookState: hookState,
-          options: options,
-        };
-
-        Model.notifyObserversOf('loaded', context, function(err) {
-          if (err) return callback(err);
-          d = context.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);

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([
-              hookMonitor.names.should.eql([
+              'access',
-                'access',
+              'before save',
-                'before save',
+              'persist',
-                'persist',
+              'loaded',
-                'loaded',
+              'after save',
-                'after save',
+            ]);
-              ]);
-            } else {
-              hookMonitor.names.should.eql([
-                'access',
-                'loaded',
-                '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`
+              ctxRecorder.records.should.eql(
-              // 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 },
-                }),
                 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([
-                hookMonitor.names.should.eql([
+                'access',
-                  'access',
+                'before save',
-                  'before save',
+                'persist',
-                  'persist',
+                'loaded',
-                  'loaded',
+                'after save',
-                  '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',
-                ]);
-              };
               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));
-                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,
-                  }),
-                ]);
-              }
               done();
             });
         });