Properly handle return values in beforeSave (#5228)

* added failing test case to CloudCode.spec.js

a possible bug found where beforeSave does not apply changes to request
object if the beforeSave hook ends with 'true' returned

* moddified triggers to return null when beforeSave
also changed test cases to be more descriptive + added extra test case that returns promise in the beforeSave

* address original issue

* Revert "address original issue"

This reverts commit e01c57d1de5c4b2fe21e9ebd590211d21330cdda.

* fix promises and tests

* Add a test to verify that a failed beforeChange hook will
prevent updating the object.

spec/CloudCode.spec.js

@@ -162,6 +162,27 @@ describe('Cloud Code', () => {
     );
   });
 
+  it("test beforeSave changed object fail doesn't change object", async function() {
+    Parse.Cloud.beforeSave('BeforeSaveChanged', function(req) {
+      if (req.object.has('fail')) {
+        return Promise.reject(new Error('something went wrong'));
+      }
+
+      return Promise.resolve();
+    });
+
+    const obj = new Parse.Object('BeforeSaveChanged');
+    obj.set('foo', 'bar');
+    await obj.save();
+    obj.set('foo', 'baz').set('fail', true);
+    try {
+      await obj.save();
+    } catch (e) {
+      await obj.fetch();
+      expect(obj.get('foo')).toBe('bar');
+    }
+  });
+
   it('test beforeSave returns value on create and update', done => {
     Parse.Cloud.beforeSave('BeforeSaveChanged', function(req) {
       req.object.set('foo', 'baz');
@@ -179,6 +200,45 @@ describe('Cloud Code', () => {
     });
   });
 
+  it('test beforeSave applies changes when beforeSave returns true', done => {
+    Parse.Cloud.beforeSave('Insurance', function(req) {
+      req.object.set('rate', '$49.99/Month');
+      return true;
+    });
+
+    const insurance = new Parse.Object('Insurance');
+    insurance.set('rate', '$5.00/Month');
+    insurance.save().then(insurance => {
+      expect(insurance.get('rate')).toEqual('$49.99/Month');
+      done();
+    });
+  });
+
+  it('test beforeSave applies changes and resolves returned promise', done => {
+    Parse.Cloud.beforeSave('Insurance', function(req) {
+      req.object.set('rate', '$49.99/Month');
+      return new Parse.Query('Pet').get(req.object.get('pet').id).then(pet => {
+        pet.set('healthy', true);
+        return pet.save();
+      });
+    });
+
+    const pet = new Parse.Object('Pet');
+    pet.set('healthy', false);
+    pet.save().then(pet => {
+      const insurance = new Parse.Object('Insurance');
+      insurance.set('pet', pet);
+      insurance.set('rate', '$5.00/Month');
+      insurance.save().then(insurance => {
+        expect(insurance.get('rate')).toEqual('$49.99/Month');
+        new Parse.Query('Pet').get(insurance.get('pet').id).then(pet => {
+          expect(pet.get('healthy')).toEqual(true);
+          done();
+        });
+      });
+    });
+  });
+
   it('test afterSave ran and created an object', function(done) {
     Parse.Cloud.afterSave('AfterSaveTest', function(req) {
       const obj = new Parse.Object('AfterSaveProof');

spec/CloudCodeLogger.spec.js

@@ -101,7 +101,7 @@ describe('Cloud Code Logger', () => {
       expect(cloudTriggerMessage[0]).toBe('info');
       expect(cloudTriggerMessage[2].triggerType).toEqual('beforeSave');
       expect(cloudTriggerMessage[1]).toMatch(
-        /beforeSave triggered for MyObject for user [^ ]*\n {2}Input: {}\n {2}Result: {}/
+        /beforeSave triggered for MyObject for user [^ ]*\n {2}Input: {}\n {2}Result: {"object":{}}/
       );
       expect(cloudTriggerMessage[2].user).toBe(user.id);
       expect(errorMessage[0]).toBe('error');

src/triggers.js

@@ -254,6 +254,7 @@ export function getResponseObject(request, resolve, reject) {
       // Use the JSON response
       if (
         response &&
+        typeof response === 'object' &&
         !request.object.equals(response) &&
         request.triggerName === Types.beforeSave
       ) {
@@ -573,6 +574,20 @@ export function maybeRunTrigger(
             auth
           );
         }
+        // beforeSave is expected to return null (nothing)
+        if (triggerType === Types.beforeSave) {
+          if (promise && typeof promise.then === 'function') {
+            return promise.then(response => {
+              // response.object may come from express routing before hook
+              if (response && response.object) {
+                return response;
+              }
+              return null;
+            });
+          }
+          return null;
+        }
+
         return promise;
       })
       .then(success, error);