Merge pull request #796 from strongloop/fix/various

Various fixes in operation hooks

lib/dao.js

@@ -266,7 +266,7 @@ DataAccessObject.create = function (data, options, cb) {
     if (options.validate === undefined && Model.settings.automaticValidation === false) {
       return create();
     }
-    
+
     // validation required
     obj.isValid(function (valid) {
       if (valid) {
@@ -521,6 +521,7 @@ DataAccessObject.updateOrCreate = DataAccessObject.upsert = function upsert(data
           var context = {
             Model: Model,
             data: data,
+            isNewInstance: info && info.isNewInstance,
             hookState: ctx.hookState,
             options: options
           };
@@ -1455,56 +1456,64 @@ DataAccessObject.find = function find(query, options, cb) {
       async.each(data, function(item, callback) {
         var d = item;//data[i];
         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,
+          data: d,
+          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 (included) obj.__data[relationName] = included;
-            });
-            delete obj.__data.__cachedRelations;
-          }
-        }
-        if (obj !== undefined) {
-          context = {
-            Model: Model,
-            instance: obj,
-            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,
+            applySetters: false,
+            persisted: true
           };
+          var obj = new Model(d, ctorOpts);
 
-          Model.notifyObserversOf('loaded', context, function(err) {
-            if (err) return callback(err);
+          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];
+                }
 
+                // 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);
             callback();
-          });
-        } else {
-          callback();
-        }
+          } else {
+            callback();
+          }
+        });
       },
       function(err) {
         if (err) return cb(err);
@@ -2548,6 +2557,7 @@ DataAccessObject.prototype.updateAttributes = function updateAttributes(data, op
             where: byIdQuery(Model, getIdValue(Model, inst)).where,
             data: context.data,
             currentInstance: inst,
+            isNewInstance: false,
             hookState: hookState,
             options: options
           };

support/describe-operation-hooks.js

@@ -11,7 +11,7 @@ var Memory = require('../lib/connectors/memory').Memory;
 
 var HOOK_NAMES = [
   'access',
-  'before save', 'persist', 'after save',
+  'before save', 'persist', 'loaded', 'after save',
   'before delete', 'after delete'
 ];
 

test/persistence-hooks.suite.js

@@ -129,7 +129,7 @@ module.exports = function(dataSource, should) {
 
       it('applies updates from `loaded` hook', function(done) {
         TestModel.observe('loaded', pushContextAndNext(function(ctx) {
-          ctx.instance.extra = 'hook data';
+          ctx.data.extra = 'hook data';
         }));
 
         TestModel.find(
@@ -138,7 +138,7 @@ module.exports = function(dataSource, should) {
             if (err) return done(err);
 
             observedContexts.should.eql(aTestModelCtx({
-              instance: {
+              data: {
                 id: "1",
                 name: "first",
                 extra: "hook data"
@@ -147,6 +147,8 @@ module.exports = function(dataSource, should) {
               hookState: { test: true },
               options: {}
             }));
+
+            list[0].should.have.property('extra', 'hook data');
             done();
           });
       })
@@ -1210,7 +1212,8 @@ module.exports = function(dataSource, should) {
               id: existingInstance.id,
               name: 'changed',
               extra: null
-            }
+            },
+            isNewInstance: false
           }));
 
           done();
@@ -1685,7 +1688,7 @@ module.exports = function(dataSource, should) {
           function(err, instance) {
             if (err) return done(err);
 
-            observedContexts.should.eql(aTestModelCtx({
+            var expectedContext = aTestModelCtx({
               where: { id: existingInstance.id },
               data: {
                 id: existingInstance.id,
@@ -1696,7 +1699,15 @@ module.exports = function(dataSource, should) {
                 name: 'updated name',
                 extra: undefined
               }
-            }));
+            });
+
+            if (!dataSource.connector.updateOrCreate) {
+              // When the connector does not provide updateOrCreate,
+              // DAO falls back to updateAttributes which sets this flag
+              expectedContext.isNewInstance = false;
+            }
+
+            observedContexts.should.eql(expectedContext);
             done();
           });
       });
@@ -1711,7 +1722,8 @@ module.exports = function(dataSource, should) {
 
             if (dataSource.connector.updateOrCreate) {
               observedContexts.should.eql(aTestModelCtx({
-                data: { id: 'new-id', name: 'a name' }
+                data: { id: 'new-id', name: 'a name' },
+                isNewInstance: true,
               }));
             } else {
               observedContexts.should.eql(aTestModelCtx({
@@ -1739,7 +1751,8 @@ module.exports = function(dataSource, should) {
                 data: {
                   id: existingInstance.id,
                   name: 'updated name'
-                }
+                },
+                isNewInstance: false
               }));
             } else {
               // For Unoptimized connector, the callback function `pushContextAndNext`
@@ -1747,10 +1760,9 @@ module.exports = function(dataSource, should) {
               // returns an array and NOT a single instance.
               observedContexts.should.eql([
                 aTestModelCtx({
-                  instance: {
+                  data: {
                     id: existingInstance.id,
                     name: 'first',
-                    extra: null
                   },
                   isNewInstance: false,
                   options: { notify: false }