From 5cfbfe3a196efb7b9e7e8c0d00cb55c15806a965 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Miroslav=20Bajto=C5=A1?= <miroslav@strongloop.com>
Date: Fri, 30 Jan 2015 11:01:48 +0100
Subject: [PATCH] Fix regression in `.save()` from 1fd6eff1

The commit 1fd6eff1 (intent-based hooks) introduced a subtle regression
in `.save()` method where dynamic property setters were invoked twice.

This commit fixes the problem by moving pre-save data normalization
into `before save` callback.
---
 lib/dao.js                | 16 ++++-----
 test/manipulation.test.js | 69 +++++++++++++++++++++++++++++++++++++--
 2 files changed, 73 insertions(+), 12 deletions(-)

diff --git a/lib/dao.js b/lib/dao.js
index 4e868279..afb11518 100644
--- a/lib/dao.js
+++ b/lib/dao.js
@@ -1132,21 +1132,19 @@ DataAccessObject.prototype.save = function (options, callback) {
     options.throws = false;
   }
 
-  var inst = this;
-  var data = inst.toObject(true);
-  var modelName = Model.modelName;
-
-  Model.applyProperties(data, this);
-
   if (this.isNewRecord()) {
     return Model.create(this, callback);
-  } else {
-    inst.setAttributes(data);
   }
 
+  var inst = this;
+  var modelName = Model.modelName;
+
   Model.notifyObserversOf('before save', { Model: Model, instance: inst }, function(err) {
     if (err) return callback(err);
-    data = inst.toObject(true);
+
+    var data = inst.toObject(true);
+    Model.applyProperties(data, inst);
+    inst.setAttributes(data);
 
     // validate first
     if (!options.validate) {
diff --git a/test/manipulation.test.js b/test/manipulation.test.js
index 3f3dbed1..adc6a379 100644
--- a/test/manipulation.test.js
+++ b/test/manipulation.test.js
@@ -22,6 +22,22 @@ describe('manipulation', function () {
 
   });
 
+  // A simplified implementation of LoopBack's User model
+  // to reproduce problems related to properties with dynamic setters
+  // For the purpose of the tests, we use a counter instead of a hash fn.
+  var StubUser, stubPasswordCounter;
+  before(function setupStubUserModel(done) {
+    StubUser = db.createModel('StubUser', { password: String }, { forceId: true });
+    StubUser.setter.password = function(plain) {
+      this.$password = plain + '-' + (++stubPasswordCounter);
+    };
+    db.automigrate('StubUser', done);
+  });
+
+  beforeEach(function resetStubPasswordCounter() {
+    stubPasswordCounter = 0;
+  });
+
   describe('create', function () {
 
     before(function (done) {
@@ -40,7 +56,7 @@ describe('manipulation', function () {
         });
       });
     });
-    
+
     it('should instantiate an object', function (done) {
       var p = new Person({name: 'Anatoliy'});
       p.name.should.equal('Anatoliy');
@@ -62,7 +78,7 @@ describe('manipulation', function () {
       person.should.be.an.instanceOf(Person);
       should.not.exist(person.id);
     });
-    
+
     it('should not allow user-defined value for the id of object - create', function (done) {
       Person.create({id: 123456}, function (err, p) {
         err.should.be.instanceof(ValidationError);
@@ -74,7 +90,7 @@ describe('manipulation', function () {
         done();
       });
     });
-    
+
     it('should not allow user-defined value for the id of object - save', function (done) {
       var p = new Person({id: 123456});
       p.isNewRecord().should.be.true;
@@ -214,6 +230,23 @@ describe('manipulation', function () {
       });
     });
 
+    it('should preserve properties with dynamic setters', function(done) {
+      // This test reproduces a problem discovered by LoopBack unit-test
+      // "User.hasPassword() should match a password after it is changed"
+      StubUser.create({ password: 'foo' }, function(err, created) {
+        if (err) return done(err);
+        created.password = 'bar';
+        created.save(function(err, saved) {
+          if (err) return done(err);
+          saved.password.should.equal('bar-2');
+          StubUser.findById(created.id, function(err, found) {
+            if (err) return done(err);
+            found.password.should.equal('bar-2');
+            done();
+          });
+        });
+      });
+    });
   });
 
   describe('updateAttributes', function () {
@@ -238,6 +271,36 @@ describe('manipulation', function () {
     });
   });
 
+  describe('updateOrCreate', function() {
+    it('should preserve properties with dynamic setters on create', function(done) {
+      StubUser.updateOrCreate({ id: 'newid', password: 'foo' }, function(err, created) {
+        if (err) return done(err);
+        created.password.should.equal('foo-1');
+        StubUser.findById(created.id, function(err, found) {
+          if (err) return done(err);
+          found.password.should.equal('foo-1');
+          done();
+        });
+      });
+    });
+
+    it('should preserve properties with dynamic setters on update', function(done) {
+      StubUser.create({ password: 'foo' }, function(err, created) {
+        if (err) return done(err);
+        var data = { id: created.id, password: 'bar' };
+        StubUser.updateOrCreate(data, function(err, updated) {
+          if (err) return done(err);
+          updated.password.should.equal('bar-2');
+          StubUser.findById(created.id, function(err, found) {
+            if (err) return done(err);
+            found.password.should.equal('bar-2');
+            done();
+          });
+        });
+      });
+    });
+  });
+
   describe('destroy', function () {
 
     it('should destroy record', function (done) {