From 7a8803cb38c998436888a4da0875a75c30995e99 Mon Sep 17 00:00:00 2001
From: Amir Jafarian <amirali.jfr61@gmail.com>
Date: Tue, 31 May 2016 18:10:44 -0400
Subject: [PATCH] Give warning if PK is changed in hooks

* Give warning if PK is changed in `before save` and `loaded`
 operation hooks for replaceById
---
 lib/dao.js                | 15 ++++++++
 lib/model-builder.js      |  1 +
 test/manipulation.test.js | 74 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 90 insertions(+)

diff --git a/lib/dao.js b/lib/dao.js
index daaf2f14..ad2c0981 100644
--- a/lib/dao.js
+++ b/lib/dao.js
@@ -2671,6 +2671,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) {
@@ -2722,7 +2729,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 = {
diff --git a/lib/model-builder.js b/lib/model-builder.js
index 480a2542..47c59dd4 100644
--- a/lib/model-builder.js
+++ b/lib/model-builder.js
@@ -218,6 +218,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) {
diff --git a/test/manipulation.test.js b/test/manipulation.test.js
index ffefff34..0aece801 100644
--- a/test/manipulation.test.js
+++ b/test/manipulation.test.js
@@ -984,6 +984,73 @@ 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('should set cannotOverwritePKInLoadedHook 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('loaded');
+          p.replaceAttributes({ title: 'b' }, function(err, data) {
+            if (err) return done(err);
+            Post._warned.cannotOverwritePKInLoadedHook.should.equal(true);
+            data.id.should.equal(postInstance.id);
+            done();
+          });
+        });
+      });
+
       it('works without options(promise variant)', function(done) {
         Post.findById(postInstance.id)
       .then(function(p) {
@@ -1059,6 +1126,13 @@ describe('manipulation', function() {
           });
         });
       });
+
+      function changePostIdInHook(operationHook) {
+        Post.observe(operationHook, function(ctx, next) {
+          (ctx.data || ctx.instance).id = 99;
+          next();
+        });
+      }
     });
   }