Properly handle return values in beforeSave

[blastoy]

Closes: #5227

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

[flovilmart]

Awesome! Thanks! Tell me, would you be tempted to attempt a fix?

[blastoy]

I leveled up and I just got a quest!

Sure, I’ll give it a try 😊

[flovilmart]

Thanks @blastoy!

[blastoy]

OK, I've narrowed down the problem to this particular area:

parse-server/src/triggers.js

Lines 255 to 266 in a4b592a

	if (
	response &&
	!request.object.equals(response) &&
	request.triggerName === Types.beforeSave
	) {
	return resolve(response);
	}
	response = {};
	if (request.triggerName === Types.beforeSave) {
	response['object'] = request.object._getSaveJSON();
	}
	return resolve(response);

In the case where I add return true at the end of the beforeSave hook, the first if statement will resolve the response, which ends up being just a raw Boolean true.

In the case where I don't add return true at the end of the beforeSave hook, the first if statement is skipped and instead the second if statement executes, later on resolving an object that looks something like { object: {...} }.

What is the intent of the first if statement?

You can see some of the effects of what happens when I write return true at the end of the beforeSave hook below:

That object passed to logTriggerSuccessBeforeHook is just a raw Boolean true

parse-server/src/triggers.js

Lines 527 to 544 in a4b592a

	var { success, error } = getResponseObject(
	request,
	object => {
	logTriggerSuccessBeforeHook(
	triggerType,
	parseObject.className,
	parseObject.toJSON(),
	object,
	auth
	);
	if (
	triggerType === Types.beforeSave ||
	triggerType === Types.afterSave
	) {
	Object.assign(context, request.context);
	}
	resolve(object);
	},

That response in the then is just a raw Boolean true, response.object being undefined. No fieldsChangedByTrigger will be found so its like no changes were done to the object

parse-server/src/RestWrite.js

Lines 233 to 251 in a4b592a

	.then(response => {
	if (response && response.object) {
	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) {
	delete this.data.objectId;
	}
	}
	});

[codecov]

Codecov Report

Merging #5228 into master will decrease coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master   #5228      +/-   ##
=========================================
- Coverage   93.93%   93.9%   -0.04%     
=========================================
  Files         123     123              
  Lines        8975    9012      +37     
=========================================
+ Hits         8431    8463      +32     
- Misses        544     549       +5

Impacted Files	Coverage Δ
src/triggers.js	94.49% <100%> (+0.18%)	⬆️
src/Adapters/Storage/Mongo/MongoStorageAdapter.js	91.48% <0%> (-0.73%)	⬇️
src/ParseServer.js	96.26% <0%> (-0.48%)	⬇️
...dapters/Storage/Postgres/PostgresStorageAdapter.js	96.99% <0%> (-0.16%)	⬇️
src/RestQuery.js	96% <0%> (-0.11%)	⬇️
src/Options/Definitions.js	100% <0%> (ø)	⬆️
src/rest.js	98.85% <0%> (ø)	⬆️
...rc/Adapters/Storage/Mongo/MongoSchemaCollection.js	95.29% <0%> (ø)	⬆️
src/Controllers/SchemaController.js	96.39% <0%> (+0.07%)	⬆️
src/Controllers/DatabaseController.js	95.08% <0%> (+0.15%)	⬆️
... and 1 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2d7b992...16e947b. Read the comment docs.

[blastoy]

My solution to the bug was to add some code right after the beforeSave trigger finishes that ignores whatever the hook returned. Figured if beforeSave works when the hook returns nothing, then making sure returns are ignored would work. Let me know what you guys think!

[dplewis]

You have some failing tests. Please check Travis.

[dplewis]

@blastoy Ping?

[blastoy]

Hey @dplewis, sorry was away for the holidays. The failing test cases are failing for me regardless of whether I run the tests on a completely clean version of the master branch. I’m not too sure what is causing this. That’s where I left off before the break.

_{Sent with GitHawk}

[dplewis]

@blastoy Sorry for the late reply. Without looking into it, since you did change the functionality of the beforeSave, some of the beforeSave tests are failing. Update those tests to match your changes.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[dplewis]

@acinader I fixed the PR to address the original issue. It was discovered that returning in beforeSave was unnecessary and could lead to the hook not working. I can update the docs to address this.

[acinader]

this doesn't make sense to me.

1. you shouldn't have to use async/await
2. you should be able to return a promise and if it fails the save is aborted.

[dplewis]

you should be able to return a promise and if it fails the save is aborted.

This works fine, the issue is if the promise succeeds.

This works

Parse.Cloud.beforeSave('Insurance', function(req) {
  req.object.set('field', 'foo');
  return req.object;
});

This works

Parse.Cloud.beforeSave('Insurance', function(req) {
  req.object.set('field', 'foo');
  return new Promise(resolve => {
    resolve(req.object);
  });
});

This fails

Parse.Cloud.beforeSave('Insurance', function(req) {
  req.object.set('field', 'foo');
  return new Promise(resolve => {
    resolve('almost anything but req.object');
  });
});

Anything returned in the promise will be passed along as a response which prevents req.object from being passed.

[acinader]

the failing case is legal though. and in the case where you're calling a function that may reject or may resolve, then what?

I suspect that @blastoy had it right with "That response in the then is just a raw Boolean true, response.object being undefined. No fieldsChangedByTrigger will be found so its [sic] like no changes were done to the object"

If I've got it right, there are two options to solve:

1. document that the return object must be a promise that resolves to the object to save (which would be a breaking change from 2.x)

2. fix the logic to not fail if trigger resolves with any value which I think is consistent with 2.x and would be desireable.

[dplewis]

fix the logic to not fail if trigger resolves with any value which I think is consistent with 2.x and would be desireable.

@acinader @blastoy I went this route let me know if more tests are needed

[acinader]

Looks good to me.

I added a test to confirm that if you reject it won't change values. I couldn't find a test that did that and I looked!

If it all looks good to you, merge away.

[dplewis]

Good catch on that test!

@blastoy Thanks for the PR!