Merge pull request #968 from strongloop/warning_for_changing_PK_in_hooks_2.x

Give warning if PK is changed in hooks

lib/dao.js

@@ -2666,6 +2666,13 @@ DataAccessObject.replaceById = function(id, data, options, cb) {
   Model.notifyObserversOf('before save', context, function(err, ctx) {
     if (err) return cb(err);
 
+    if (ctx.instance[pkName] !== id && !Model._warned.cannotOverwritePKInBeforeSaveHook) {
+      Model._warned.cannotOverwritePKInBeforeSaveHook = true;
+      console.warn('WARNING: id property cannot be changed from ' +
+        id + ' to ' + inst[pkName] + ' for model:' + Model.modelName +
+        ' in \'before save\' operation hook');
+    }
+
     data = inst.toObject(false);
 
     if (strict) {
@@ -2717,7 +2724,15 @@ DataAccessObject.replaceById = function(id, data, options, cb) {
           Model.notifyObserversOf('loaded', ctx, function(err) {
             if (err) return cb(err);
 
+            if (ctx.data[pkName] !== id && !Model._warned.cannotOverwritePKInLoadedHook) {
+              Model._warned.cannotOverwritePKInLoadedHook = true;
+              console.warn('WARNING: id property cannot be changed from ' +
+                id + ' to ' + ctx.data[pkName] + ' for model:' + Model.modelName +
+                ' in \'loaded\' operation hook');
+            }
+
             inst.__persisted = true;
+            ctx.data[pkName] = id;
             inst.setAttributes(ctx.data);
 
             var context = {

lib/model-builder.js

@@ -231,6 +231,7 @@ ModelBuilder.prototype.define = function defineClass(className, properties, sett
   hiddenProperty(ModelClass, 'http', { path: pathName });
   hiddenProperty(ModelClass, 'base', ModelBaseClass);
   hiddenProperty(ModelClass, '_observers', {});
+  hiddenProperty(ModelClass, '_warned', {});
 
   // inherit ModelBaseClass static methods
   for (var i in ModelBaseClass) {

test/manipulation.test.js

@@ -980,6 +980,58 @@ describe('manipulation', function() {
         });
       });
 
+      it('should ignore PK if it is set for `instance`' +
+      'in `before save` operation hook', function(done) {
+        Post.findById(postInstance.id, function(err, p) {
+          if (err) return done(err);
+          changePostIdInHook('before save');
+          p.replaceAttributes({ title: 'b' }, function(err, data) {
+            data.id.should.eql(postInstance.id);
+            if (err) return done(err);
+            Post.find(function(err, p) {
+              if (err) return done(err);
+              p[0].id.should.eql(postInstance.id);
+              done();
+            });
+          });
+        });
+      });
+
+      it('should set cannotOverwritePKInBeforeSaveHook flag, if `instance` in' +
+      '`before save` operation hook is set, so we report a warning just once',
+      function(done) {
+        Post.findById(postInstance.id, function(err, p) {
+          if (err) return done(err);
+          changePostIdInHook('before save');
+          p.replaceAttributes({ title: 'b' }, function(err, data) {
+            if (err) return done(err);
+            Post._warned.cannotOverwritePKInBeforeSaveHook.should.equal(true);
+            data.id.should.equal(postInstance.id);
+            done();
+          });
+        });
+      });
+
+      it('should ignore PK if it is set for `data`' +
+      'in `loaded` operation hook', function(done) {
+        Post.findById(postInstance.id, function(err, p) {
+          if (err) return done(err);
+          changePostIdInHook('loaded');
+          p.replaceAttributes({ title: 'b' }, function(err, data) {
+            data.id.should.eql(postInstance.id);
+            if (err) return done(err);
+            // clear observers to make sure `loaded`
+            // hook does not affect `find()` method
+            Post.clearObservers('loaded');
+            Post.find(function(err, p) {
+              if (err) return done(err);
+              p[0].id.should.eql(postInstance.id);
+              done();
+            });
+          });
+        });
+      });
+
       it('works without options(promise variant)', function(done) {
         Post.findById(postInstance.id)
       .then(function(p) {
@@ -1055,6 +1107,13 @@ describe('manipulation', function() {
           });
         });
       });
+
+      function changePostIdInHook(operationHook) {
+        Post.observe(operationHook, function(ctx, next) {
+          (ctx.data || ctx.instance).id = 99;
+          next();
+        });
+      }
     });
   }