Syncing afterSave/afterDelete trigger calls (Issue #2489) #2499
Conversation
…uest execution flow (Issue 2489). After this change, afterSave and afterDelete triggers CAN return a promise, which needs to be resolved inside a trigger for REST request flow to continue. If trigger doesn't return a promise, request flow continues.
ghost
added
the
| //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) && triggerPromise && typeof triggerPromise.then === "function") |
There was a problem hiding this comment.
add { around multi line if please.
Current coverage is 91.92% (diff: 100%)@@ master #2499 diff @@
==========================================
Files 93 96 +3
Lines 10629 10892 +263
Methods 1310 1345 +35
Messages 0 0
Branches 1728 1759 +31
==========================================
+ Hits 9829 10013 +184
- Misses 800 879 +79
Partials 0 0
|
|
@cipiripper updated the pull request. |
|
@cipiripper updated the pull request. |
|
Seems that the tests are failing, can you have a look? |
|
Yes. I'll take look at it tomorrow.
|
|
thanks 👏 |
|
@cipiripper updated the pull request - view changes |
facebook-github-bot
added
GH Review: review-needed
and removed
GH Review: needs-revision
labels
|
@cipiripper updated the pull request. |
| return triggerPromise.then(resolve, resolve); | ||
| } | ||
| else { | ||
| return resolve(); |
There was a problem hiding this comment.
Not sure we should call resolve here? What's the rationale?
There was a problem hiding this comment.
@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.
* ParsePlatform/master: (100 commits) Only allow basic auth credentials with a known appId (parse-community#2574) vk.com provider registered (parse-community#2579) chore(package): update parse-server-push-adapter to version 1.1.0 (parse-community#2588) vk.com auth data manager implemented (parse-community#2578) Fix a typo (parse-community#2563) Makes sure routes don't overlap and yield a header set error (parse-community#2559) Postgres: $all, $and CLP and more (parse-community#2551) Changelog 2.2.18 (parse-community#2558) chore(package): update winston-daily-rotate-file to version 1.3.0 (parse-community#2547) chore(package): update parse-server-s3-adapter to version 1.0.5 (parse-community#2536) Adds bcrypt native binding for better login performance (parse-community#2549) chore(package): update mongodb to version 2.2.7 (parse-community#2554) Make parse-server cloud code logging closer parse.com legacy (parse-community#2550) chore(package): update pg-promise to version 5.3.1 (parse-community#2519) Postgres: Operations, Hooks, OAuth login, Files support (parse-community#2528) Syncing afterSave/afterDelete trigger calls (Issue parse-community#2489) (parse-community#2499) Updated README.md (parse-community#2538) Fix capitalization, typo, and grammar mistake (parse-community#2533) Update ISSUE_TEMPLATE.md fix typo (parse-community#2525) ...
Implemented syncing afterSave/afterDelete trigger calls with REST request execution flow (Issue 2489). After this change, afterSave and afterDelete triggers CAN return a promise, which needs to be resolved inside a trigger for REST request flow to continue. If trigger doesn't return a promise, request flow continues.