Skip to content

Syncing afterSave/afterDelete trigger calls (Issue #2489) #2499

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Aug 17, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
156 changes: 156 additions & 0 deletions spec/CloudCode.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,162 @@ describe('Cloud Code', () => {
}, 500);
});

it('test afterSave ran on created object and returned a promise', function(done) {
Parse.Cloud.afterSave('AfterSaveTest2', function(req) {
let obj = req.object;
if(!obj.existed())
{
let promise = new Parse.Promise();
setTimeout(function(){
obj.set('proof', obj.id);
obj.save().then(function(){
promise.resolve();
});
}, 1000);

return promise;
}
});

let obj = new Parse.Object('AfterSaveTest2');
obj.save().then(function(){
let query = new Parse.Query('AfterSaveTest2');
query.equalTo('proof', obj.id);
query.find().then(function(results) {
expect(results.length).toEqual(1);
let savedObject = results[0];
expect(savedObject.get('proof')).toEqual(obj.id);
done();
},
function(error) {
fail(error);
done();
});
});
});

it('test afterSave ignoring promise, object not found', function(done) {
Parse.Cloud.afterSave('AfterSaveTest2', function(req) {
let obj = req.object;
if(!obj.existed())
{
let promise = new Parse.Promise();
setTimeout(function(){
obj.set('proof', obj.id);
obj.save().then(function(){
promise.resolve();
});
}, 1000);

return promise;
}
});

let obj = new Parse.Object('AfterSaveTest2');
obj.save().then(function(){
done();
})

let query = new Parse.Query('AfterSaveTest2');
query.equalTo('proof', obj.id);
query.find().then(function(results) {
expect(results.length).toEqual(0);
},
function(error) {
fail(error);
});
});

it('test afterSave rejecting promise', function(done) {
Parse.Cloud.afterSave('AfterSaveTest2', function(req) {
let promise = new Parse.Promise();
setTimeout(function(){
promise.reject("THIS SHOULD BE IGNORED");
}, 1000);

return promise;
});

let obj = new Parse.Object('AfterSaveTest2');
obj.save().then(function(){
done();
}, function(error){
fail(error);
done();
})
});

it('test afterDelete returning promise, object is deleted when destroy resolves', function(done) {
Parse.Cloud.afterDelete('AfterDeleteTest2', function(req) {
let promise = new Parse.Promise();

setTimeout(function(){
let obj = new Parse.Object('AfterDeleteTestProof');
obj.set('proof', req.object.id);
obj.save().then(function(){
promise.resolve();
});

}, 1000);

return promise;
});

let errorHandler = function(error) {
fail(error);
done();
}

let obj = new Parse.Object('AfterDeleteTest2');
obj.save().then(function(){
obj.destroy().then(function(){
let query = new Parse.Query('AfterDeleteTestProof');
query.equalTo('proof', obj.id);
query.find().then(function(results) {
expect(results.length).toEqual(1);
let deletedObject = results[0];
expect(deletedObject.get('proof')).toEqual(obj.id);
done();
}, errorHandler);
}, errorHandler)
}, errorHandler);
});

it('test afterDelete ignoring promise, object is not yet deleted', function(done) {
Parse.Cloud.afterDelete('AfterDeleteTest2', function(req) {
let promise = new Parse.Promise();

setTimeout(function(){
let obj = new Parse.Object('AfterDeleteTestProof');
obj.set('proof', req.object.id);
obj.save().then(function(){
promise.resolve();
});

}, 1000);

return promise;
});

let errorHandler = function(error) {
fail(error);
done();
}

let obj = new Parse.Object('AfterDeleteTest2');
obj.save().then(function(){
obj.destroy().then(function(){
done();
})

let query = new Parse.Query('AfterDeleteTestProof');
query.equalTo('proof', obj.id);
query.find().then(function(results) {
expect(results.length).toEqual(0);
}, errorHandler);
}, errorHandler);
});

it('test beforeSave happens on update', function(done) {
Parse.Cloud.beforeSave('BeforeSaveChanged', function(req, res) {
req.object.set('foo', 'baz');
Expand Down
16 changes: 8 additions & 8 deletions src/RestWrite.js
Original file line number Diff line number Diff line change
Expand Up @@ -296,10 +296,10 @@ RestWrite.prototype.handleAuthData = function(authData) {
// Login with auth data
delete results[0].password;
let userResult = results[0];

// need to set the objectId first otherwise location has trailing undefined
this.data.objectId = userResult.objectId;

// Determine if authData was updated
let mutatedAuthData = {};
Object.keys(authData).forEach((provider) => {
Expand All @@ -309,7 +309,7 @@ RestWrite.prototype.handleAuthData = function(authData) {
mutatedAuthData[provider] = providerData;
}
});

this.response = {
response: userResult,
location: this.location()
Expand All @@ -328,7 +328,7 @@ RestWrite.prototype.handleAuthData = function(authData) {
return this.config.database.update(this.className, {objectId: this.data.objectId}, {authData: mutatedAuthData}, {});
}
return;

} else if (this.query && this.query.objectId) {
// Trying to update auth data but users
// are different
Expand Down Expand Up @@ -476,7 +476,7 @@ RestWrite.prototype.handleFollowup = function() {
return this.config.database.destroy('_Session', sessionQuery)
.then(this.handleFollowup.bind(this));
}

if (this.storage && this.storage['generateNewSession']) {
delete this.storage['generateNewSession'];
return this.createSessionToken()
Expand Down Expand Up @@ -847,7 +847,7 @@ RestWrite.prototype.runDatabaseOperation = function() {
.then(response => {
response.objectId = this.data.objectId;
response.createdAt = this.data.createdAt;

if (this.responseShouldHaveUsername) {
response.username = this.data.username;
}
Expand Down Expand Up @@ -895,7 +895,7 @@ RestWrite.prototype.runAfterTrigger = function() {
this.config.liveQueryController.onAfterSave(updatedObject.className, updatedObject, originalObject);

// Run afterSave trigger
triggers.maybeRunTrigger(triggers.Types.afterSave, this.auth, updatedObject, originalObject, this.config);
return triggers.maybeRunTrigger(triggers.Types.afterSave, this.auth, updatedObject, originalObject, this.config);
};

// A helper to figure out what location this operation happens at.
Expand Down Expand Up @@ -949,7 +949,7 @@ RestWrite.prototype._updateResponseWithData = function(response, data) {
let responseValue = response[fieldName];

response[fieldName] = responseValue || dataValue;

// Strips operations from responses
if (response[fieldName] && response[fieldName].__op) {
delete response[fieldName];
Expand Down
3 changes: 1 addition & 2 deletions src/rest.js
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,7 @@ function del(config, auth, className, objectId, clientSDK) {
objectId: objectId
}, options);
}).then(() => {
triggers.maybeRunTrigger(triggers.Types.afterDelete, auth, inflatedObject, null, config);
return;
return triggers.maybeRunTrigger(triggers.Types.afterDelete, auth, inflatedObject, null, config);
});
}

Expand Down
21 changes: 17 additions & 4 deletions src/triggers.js
Original file line number Diff line number Diff line change
Expand Up @@ -158,15 +158,15 @@ function logTrigger(triggerType, className, input) {
return;
}
logger.info(`${triggerType} triggered for ${className}\nInput: ${JSON.stringify(input)}`, {
className,
className,
triggerType,
input
});
}

function logTriggerSuccess(triggerType, className, input, result) {
logger.info(`${triggerType} triggered for ${className}\nInput: ${JSON.stringify(input)}\nResult: ${JSON.stringify(result)}`, {
className,
className,
triggerType,
input,
result
Expand All @@ -175,7 +175,7 @@ function logTriggerSuccess(triggerType, className, input, result) {

function logTriggerError(triggerType, className, input, error) {
logger.error(`${triggerType} failed for ${className}\nInput: ${JSON.stringify(input)}\Error: ${JSON.stringify(error)}`, {
className,
className,
triggerType,
input,
error
Expand Down Expand Up @@ -209,7 +209,20 @@ export function maybeRunTrigger(triggerType, auth, parseObject, originalParseObj
Parse.masterKey = config.masterKey;
// For the afterSuccess / afterDelete
logTrigger(triggerType, parseObject.className, parseObject.toJSON());
trigger(request, response);

//AfterSave and afterDelete triggers can return a promise, which if they do, needs to be resolved before this promise is resolved,
//so trigger execution is synced with RestWrite.execute() call.
//If triggers do not return a promise, they can run async code parallel to the RestWrite.execute() call.
var triggerPromise = trigger(request, response);
if(triggerType === Types.afterSave || triggerType === Types.afterDelete)
{
if(triggerPromise && typeof triggerPromise.then === "function") {
return triggerPromise.then(resolve, resolve);
}
else {
return resolve();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure we should call resolve here? What's the rationale?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm. Checking.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@flovilmart Okay, so resolve() resolves the promise maybeRunTrigger returns, I did it the same as in line 197. So if "after" trigger doesn't return a promise, and caller function expects a promise to be resolved it will hang. So we need to resolve it in all code paths: inside the "after" trigger itself, or here in this line if after trigger did not return a promise. For "before" triggers, promise will be resolved using response objects from inside of the trigger. I checked. If I remove that line, a lot of tests fail, because request flow hangs waiting for this promise to be resolved.

}
}
});
};

Expand Down