Implement `notify` for find method

lib/dao.js

@@ -1633,15 +1633,25 @@ DataAccessObject.find = function find(query, options, cb) {
       // using all documents
       // TODO [fabien] use default scope here?
 
-      var context = {
+      if (options.notify === false) {
-        Model: self,
+        queryGeo(query);
-        query: query,
+      } else {
-        hookState: hookState,
+        withNotifyGeo();
-        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;
@@ -1662,7 +1672,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, []);
           }
@@ -1673,10 +1683,9 @@ DataAccessObject.find = function find(query, options, cb) {
         } else {
           connector.all(self.modelName, {}, geoCallback);
         }
-      });
+        // already handled
-
+        return cb.promise;
-      // already handled
+      }
-      return cb.promise;
     }
   }
 
@@ -1717,19 +1726,23 @@ DataAccessObject.find = function find(query, options, cb) {
           }
         }
         if (obj !== undefined) {
-          context = {
+          if (options.notify === false) {
-            Model: Model,
-            instance: obj,
-            isNewInstance: false,
-            hookState: hookState,
-            options: options,
-          };
-
-          Model.notifyObserversOf('loaded', context, function(err) {
-            if (err) return next(err);
-
             next(null, obj);
-          });
+          } else {
+            context = {
+              Model: Model,
+              instance: obj,
+              isNewInstance: false,
+              hookState: hookState,
+              options: options,
+            };
+
+            Model.notifyObserversOf('loaded', context, function(err) {
+              if (err) return next(err);
+
+              next(null, obj);
+            });
+          }
         } else {
           next();
         }

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());
 
@@ -123,10 +162,10 @@ module.exports = function(dataSource, should, connectorCapabilities) {
       it('triggers `access` hook for geo queries', function(done) {
         TestModel.observe('access', ctxRecorder.recordAndNext());
 
-        TestModel.find({ where: { geo: { near: '10,20' }}}, function(err, list) {
+        TestModel.find({ where: { geo: [{ near: '10,20' }] }}, function(err, list) {
           if (err) return done(err);
           ctxRecorder.records.should.eql(aCtxForModel(TestModel, {
-            query: { where: { geo: { near: '10,20' }}},
+            query: { where: { geo: [{ near: '10,20' }] }},
           }));
           done();
         });
@@ -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();
           });
       });
@@ -2015,26 +2043,14 @@ module.exports = function(dataSource, should, connectorCapabilities) {
                 },
               }));
             } 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, {
-                  instance: {
-                    id: existingInstance.id,
-                    name: 'first',
-                    extra: null,
-                  },
-                  isNewInstance: false,
-                  options: { notify: false },
-                }),
                 aCtxForModel(TestModel, {
                   data: {
                     id: existingInstance.id,
                     name: 'updated name',
                   },
-                }),
+                })
-              ]);
+              );
             }
             done();
           });
@@ -2118,33 +2134,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();
             });
         });
@@ -2451,37 +2447,7 @@ module.exports = function(dataSource, should, connectorCapabilities) {
               expected.isNewInstance =
                 connectorCapabilities.replaceOrCreateReportsNewInstance ?
                 false : undefined;
-
+              ctxRecorder.records.should.eql(aCtxForModel(TestModel, expected));
-              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,
-                  }),
-                ]);
-              }
               done();
             });
         });