From ec1b13011b6ca6f0383efdf61c7764beeab0720f Mon Sep 17 00:00:00 2001 From: Guido Ruiz Date: Thu, 6 Dec 2018 13:31:06 -0500 Subject: [PATCH 1/6] 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 --- spec/CloudCode.spec.js | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/spec/CloudCode.spec.js b/spec/CloudCode.spec.js index e02c4cc3ca..1a967f0ac7 100644 --- a/spec/CloudCode.spec.js +++ b/spec/CloudCode.spec.js @@ -179,6 +179,24 @@ describe('Cloud Code', () => { }); }); + it('test beforeSave returns value on create and update when beforeSave returns true', done => { + Parse.Cloud.beforeSave('BeforeSaveChanged', function(req) { + req.object.set('foo', 'baz'); + return true; + }); + + const obj = new Parse.Object('BeforeSaveChanged'); + obj.set('foo', 'bing'); + obj.save().then(() => { + expect(obj.get('foo')).toEqual('baz'); + obj.set('foo', 'bar'); + return obj.save().then(() => { + expect(obj.get('foo')).toEqual('baz'); + done(); + }); + }); + }); + it('test afterSave ran and created an object', function(done) { Parse.Cloud.afterSave('AfterSaveTest', function(req) { const obj = new Parse.Object('AfterSaveProof'); From a776de494b10454ac9cc813cd93da027ffe196a0 Mon Sep 17 00:00:00 2001 From: Guido Ruiz Date: Fri, 14 Dec 2018 13:51:44 -0500 Subject: [PATCH 2/6] 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 --- spec/CloudCode.spec.js | 43 +++++++++++++++++++++++++++++++----------- src/triggers.js | 10 ++++++++++ 2 files changed, 42 insertions(+), 11 deletions(-) diff --git a/spec/CloudCode.spec.js b/spec/CloudCode.spec.js index 1a967f0ac7..aaa18f1425 100644 --- a/spec/CloudCode.spec.js +++ b/spec/CloudCode.spec.js @@ -179,20 +179,41 @@ describe('Cloud Code', () => { }); }); - it('test beforeSave returns value on create and update when beforeSave returns true', done => { - Parse.Cloud.beforeSave('BeforeSaveChanged', function(req) { - req.object.set('foo', 'baz'); + 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 obj = new Parse.Object('BeforeSaveChanged'); - obj.set('foo', 'bing'); - obj.save().then(() => { - expect(obj.get('foo')).toEqual('baz'); - obj.set('foo', 'bar'); - return obj.save().then(() => { - expect(obj.get('foo')).toEqual('baz'); - done(); + 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(); + }); }); }); }); diff --git a/src/triggers.js b/src/triggers.js index ead83cac5c..ec363dc355 100644 --- a/src/triggers.js +++ b/src/triggers.js @@ -573,6 +573,16 @@ export function maybeRunTrigger( auth ); } + + // beforeSave is expected to return null (nothing) + if (triggerType === Types.beforeSave) { + // if the beforeSave returned a promise, return null after the promise is resolved + if (promise && typeof promise.then === 'function') { + return promise.then(() => null); + } + return null; + } + return promise; }) .then(success, error); From e01c57d1de5c4b2fe21e9ebd590211d21330cdda Mon Sep 17 00:00:00 2001 From: Diamond Lewis Date: Wed, 13 Mar 2019 12:10:10 -0500 Subject: [PATCH 3/6] address original issue --- 3.0.0.md | 3 ++- CHANGELOG.md | 1 + spec/CloudCode.spec.js | 25 ------------------------- src/triggers.js | 11 +---------- 4 files changed, 4 insertions(+), 36 deletions(-) diff --git a/3.0.0.md b/3.0.0.md index 7059707ac5..65297920a3 100644 --- a/3.0.0.md +++ b/3.0.0.md @@ -59,11 +59,12 @@ Parse.Cloud.beforeSave('Post', function(request, response) { As we can see the current way, with backbone style callbacks is quite tough to read and maintain. It's also not really trivial to handle errors, as you need to pass the response.error to each error handlers. -Now it can be modernized with promises: +Do not return in beforeSave hooks as this may have prevent the hook from working. ```js // after (with promises) Parse.Cloud.beforeSave('MyClassName', (request) => { + // Don't return here use await / async return Parse.Cloud.httpRequest({ url: request.object.get('fileURL') }).then((contents) => { diff --git a/CHANGELOG.md b/CHANGELOG.md index 702b3103c1..52c8e1f78e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,6 +44,7 @@ For more informations, visit the v3.0.0 [migration guide](https://github.com/par #### Breaking changes: * Cloud Code handlers have a new interface based on promises. +* Returning in before hooks are deprecated. * response.success / response.error are removed in Cloud Code * Cloud Code runs with Parse-SDK 2.0 * The aggregate now require aggregates to be passed in the form: `{"pipeline": [...]}` diff --git a/spec/CloudCode.spec.js b/spec/CloudCode.spec.js index aaa18f1425..99307cc87a 100644 --- a/spec/CloudCode.spec.js +++ b/spec/CloudCode.spec.js @@ -193,31 +193,6 @@ describe('Cloud Code', () => { }); }); - 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'); diff --git a/src/triggers.js b/src/triggers.js index ec363dc355..1ee76cf8f1 100644 --- a/src/triggers.js +++ b/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,16 +574,6 @@ export function maybeRunTrigger( auth ); } - - // beforeSave is expected to return null (nothing) - if (triggerType === Types.beforeSave) { - // if the beforeSave returned a promise, return null after the promise is resolved - if (promise && typeof promise.then === 'function') { - return promise.then(() => null); - } - return null; - } - return promise; }) .then(success, error); From aa9c57e0cee4f222a3fdf8f45fc7a7e3c546c5f5 Mon Sep 17 00:00:00 2001 From: Diamond Lewis Date: Thu, 14 Mar 2019 11:58:38 -0500 Subject: [PATCH 4/6] Revert "address original issue" This reverts commit e01c57d1de5c4b2fe21e9ebd590211d21330cdda. --- 3.0.0.md | 3 +-- CHANGELOG.md | 1 - spec/CloudCode.spec.js | 25 +++++++++++++++++++++++++ src/triggers.js | 11 ++++++++++- 4 files changed, 36 insertions(+), 4 deletions(-) diff --git a/3.0.0.md b/3.0.0.md index 65297920a3..7059707ac5 100644 --- a/3.0.0.md +++ b/3.0.0.md @@ -59,12 +59,11 @@ Parse.Cloud.beforeSave('Post', function(request, response) { As we can see the current way, with backbone style callbacks is quite tough to read and maintain. It's also not really trivial to handle errors, as you need to pass the response.error to each error handlers. -Do not return in beforeSave hooks as this may have prevent the hook from working. +Now it can be modernized with promises: ```js // after (with promises) Parse.Cloud.beforeSave('MyClassName', (request) => { - // Don't return here use await / async return Parse.Cloud.httpRequest({ url: request.object.get('fileURL') }).then((contents) => { diff --git a/CHANGELOG.md b/CHANGELOG.md index 52c8e1f78e..702b3103c1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,7 +44,6 @@ For more informations, visit the v3.0.0 [migration guide](https://github.com/par #### Breaking changes: * Cloud Code handlers have a new interface based on promises. -* Returning in before hooks are deprecated. * response.success / response.error are removed in Cloud Code * Cloud Code runs with Parse-SDK 2.0 * The aggregate now require aggregates to be passed in the form: `{"pipeline": [...]}` diff --git a/spec/CloudCode.spec.js b/spec/CloudCode.spec.js index 99307cc87a..aaa18f1425 100644 --- a/spec/CloudCode.spec.js +++ b/spec/CloudCode.spec.js @@ -193,6 +193,31 @@ describe('Cloud Code', () => { }); }); + 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'); diff --git a/src/triggers.js b/src/triggers.js index 1ee76cf8f1..ec363dc355 100644 --- a/src/triggers.js +++ b/src/triggers.js @@ -254,7 +254,6 @@ export function getResponseObject(request, resolve, reject) { // Use the JSON response if ( response && - typeof response === 'object' && !request.object.equals(response) && request.triggerName === Types.beforeSave ) { @@ -574,6 +573,16 @@ export function maybeRunTrigger( auth ); } + + // beforeSave is expected to return null (nothing) + if (triggerType === Types.beforeSave) { + // if the beforeSave returned a promise, return null after the promise is resolved + if (promise && typeof promise.then === 'function') { + return promise.then(() => null); + } + return null; + } + return promise; }) .then(success, error); From 57249f098a26348f87b036be849157be5daebc0b Mon Sep 17 00:00:00 2001 From: Diamond Lewis Date: Thu, 14 Mar 2019 12:14:35 -0500 Subject: [PATCH 5/6] fix promises and tests --- spec/CloudCodeLogger.spec.js | 2 +- src/triggers.js | 11 ++++++++--- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/spec/CloudCodeLogger.spec.js b/spec/CloudCodeLogger.spec.js index e53b8975f8..6d38ce2ea6 100644 --- a/spec/CloudCodeLogger.spec.js +++ b/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'); diff --git a/src/triggers.js b/src/triggers.js index ec363dc355..b8558e21dd 100644 --- a/src/triggers.js +++ b/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,12 +574,16 @@ export function maybeRunTrigger( auth ); } - // beforeSave is expected to return null (nothing) if (triggerType === Types.beforeSave) { - // if the beforeSave returned a promise, return null after the promise is resolved if (promise && typeof promise.then === 'function') { - return promise.then(() => null); + return promise.then(response => { + // response.object may come from express routing before hook + if (response && response.object) { + return response; + } + return null; + }); } return null; } From 16e947bedb32b8113d21897c3c0ddb79ba8e3017 Mon Sep 17 00:00:00 2001 From: Arthur Cinader <700572+acinader@users.noreply.github.com> Date: Thu, 14 Mar 2019 10:51:22 -0700 Subject: [PATCH 6/6] Add a test to verify that a failed beforeChange hook will prevent updating the object. --- spec/CloudCode.spec.js | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/spec/CloudCode.spec.js b/spec/CloudCode.spec.js index aaa18f1425..0b88ead0fd 100644 --- a/spec/CloudCode.spec.js +++ b/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');