Non trivial before save pointer clobber (#2406)

* test case to check beforeSave changes clobbers fetched pointer fields
Basically if beforeSave makes any changes to the object it is trying to save, the fetched pointer fields on the client gets clobbered to only pointer.

* propogate only changed fields to response.
Earlier we were returning all fields even if any changes happened in beforeSave. This causes the fetched pointer fields on the client to get clobbered to only pointers.
This fix returns only the changed fields thus avoiding pointer clobber.

* The goal of this comparision seems to be checking that the all returns the user correctly.
Also it is consistent with the hosted parse that user.username not returned from PUT request.

Author: sud80

spec/CloudCode.spec.js

@@ -684,6 +684,44 @@ describe('Cloud Code', () => {
     });
   });
 
+it('beforeSave should not affect fetched pointers', done => {
+    Parse.Cloud.beforeSave('BeforeSaveUnchanged', (req, res) => {
+      res.success();
+    });
+
+    Parse.Cloud.beforeSave('BeforeSaveChanged', function(req, res) {
+      req.object.set('foo', 'baz');
+      res.success();
+    });
+
+    var TestObject =  Parse.Object.extend("TestObject");
+    var BeforeSaveUnchangedObject = Parse.Object.extend("BeforeSaveUnchanged");
+    var BeforeSaveChangedObject = Parse.Object.extend("BeforeSaveChanged");
+
+    var aTestObject = new TestObject();
+    aTestObject.set("foo", "bar");
+    aTestObject.save()
+    .then(aTestObject => {
+      var aBeforeSaveUnchangedObject = new BeforeSaveUnchangedObject();
+      aBeforeSaveUnchangedObject.set("aTestObject", aTestObject);
+      expect(aBeforeSaveUnchangedObject.get("aTestObject").get("foo")).toEqual("bar");
+      return aBeforeSaveUnchangedObject.save();
+    })
+    .then(aBeforeSaveUnchangedObject => {
+      expect(aBeforeSaveUnchangedObject.get("aTestObject").get("foo")).toEqual("bar");
+
+      var aBeforeSaveChangedObject = new BeforeSaveChangedObject();
+      aBeforeSaveChangedObject.set("aTestObject", aTestObject);
+      expect(aBeforeSaveChangedObject.get("aTestObject").get("foo")).toEqual("bar");
+      return aBeforeSaveChangedObject.save();
+    })
+    .then(aBeforeSaveChangedObject => {
+      expect(aBeforeSaveChangedObject.get("aTestObject").get("foo")).toEqual("bar");
+      expect(aBeforeSaveChangedObject.get("foo")).toEqual("baz");
+      done();
+    });
+  });
+
   it_exclude_dbs(['postgres'])('should fully delete objects when using `unset` with beforeSave (regression test for #1840)', done => {
     var TestObject = Parse.Object.extend('TestObject');
     var NoBeforeSaveObject = Parse.Object.extend('NoBeforeSave');

spec/ParseUser.spec.js

@@ -2374,7 +2374,7 @@ describe('Parse.User testing', () => {
           },
           json: true
         }, (err, res, body) => {
-          expect(body.username).toEqual(user.username);
+          expect(body.username).toEqual('user');
           expect(body.objectId).toEqual(originalUserId);
           if (err) {
             reject(err);

src/RestWrite.js

@@ -165,9 +165,12 @@ RestWrite.prototype.runBeforeTrigger = function() {
     return triggers.maybeRunTrigger(triggers.Types.beforeSave, this.auth, updatedObject, originalObject, this.config);
   }).then((response) => {
     if (response && response.object) {
-      if (!_.isEqual(this.data, response.object)) {
-        this.storage.changedByTrigger = true;
-      }
+      this.storage.fieldsChangedByTrigger = _.reduce(response.object, (result, value, key) => {
+        if (!_.isEqual(this.data[key], value)) {
+          result.push(key);
+        }
+        return result;
+      }, []);
       this.data = response.object;
       // We should delete the objectId for an update write
       if (this.query && this.query.objectId) {
@@ -792,9 +795,7 @@ RestWrite.prototype.runDatabaseOperation = function() {
     return this.config.database.update(this.className, this.query, this.data, this.runOptions)
     .then(response => {
       response.updatedAt = this.updatedAt;
-      if (this.storage.changedByTrigger) {
-        this.updateResponseWithData(response, this.data);
-      }
+      this._updateResponseWithData(response, this.data);
       this.response = { response };
     });
   } else {
@@ -850,9 +851,7 @@ RestWrite.prototype.runDatabaseOperation = function() {
       if (this.responseShouldHaveUsername) {
         response.username = this.data.username;
       }
-      if (this.storage.changedByTrigger) {
-        this.updateResponseWithData(response, this.data);
-      }
+      this._updateResponseWithData(response, this.data);
       this.response = {
         status: 201,
         response,
@@ -940,9 +939,12 @@ RestWrite.prototype.cleanUserAuthData = function() {
   }
 };
 
-RestWrite.prototype.updateResponseWithData = function(response, data) {
+RestWrite.prototype._updateResponseWithData = function(response, data) {
+  if (_.isEmpty(this.storage.fieldsChangedByTrigger)) {
+    return response;
+  }
   let clientSupportsDelete = ClientSDK.supportsForwardDelete(this.clientSDK);
-  Object.keys(data).forEach(fieldName => {
+  this.storage.fieldsChangedByTrigger.forEach(fieldName => {
     let dataValue = data[fieldName];
     let responseValue = response[fieldName];