fix: A request with a malformed pointer and a beforeSave trigger hangs until timeout#10548
fix: A request with a malformed pointer and a beforeSave trigger hangs until timeout#10548dblythy wants to merge 1 commit into
Conversation
|
🚀 Thanks for opening this pull request! We appreciate your effort in improving the project. Please let us know once your pull request is ready for review. Tip
Note Please respond to review comments from AI agents just like you would to comments from a human reviewer. Let the reviewer resolve their own comments, unless they have reviewed and accepted your commit, or agreed with your explanation for why the feedback was incorrect. Caution Pull requests must be written using an AI agent with human supervision. Pull requests written entirely by a human will likely be rejected, because of lower code quality, higher review effort and the higher risk of introducing bugs. Please note that AI review comments on this pull request alone do not satisfy this requirement. Our CI and AI review are safeguards, not development tools. If many issues are flagged, rethink your development approach. Invest more effort in planning and design rather than using review cycles to fix low-quality code. |
📝 WalkthroughWalkthroughCloud Code trigger handling now uses awaited execution and normalized error propagation, including serialization failures. A regression test verifies malformed Pointer input returns HTTP 400 with ChangesTrigger error handling
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 inconclusive)
✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Biome (2.5.3)src/triggers.jsFile contains syntax errors that prevent linting: Line 230: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 230: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 230: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 230: return types can only be used in TypeScript files Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## alpha #10548 +/- ##
==========================================
- Coverage 92.66% 92.66% -0.01%
==========================================
Files 193 193
Lines 16981 16986 +5
Branches 248 248
==========================================
+ Hits 15736 15740 +4
- Misses 1224 1225 +1
Partials 21 21 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/triggers.js (1)
965-971: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winExtract the defensive
toJSON()fallback into a helper and use it for the log calls too.You already learned the lesson in the catch block (Lines 1020-1027): a malformed object can't be serialised. But
parseObject.toJSON()on Lines 968 and 999 is still bare, and the argument is evaluated before thelogLevel === 'silent'early-return inside the log functions — so the throw happens regardless of log configuration. Concretely, anafterSavetrigger that completes successfully on an object carrying an unsaved/empty-objectIdpointer will now be turned into aSCRIPT_FAILEDrejection purely because the logging argument blew up. That's pre-existing behaviour, granted, but this PR is precisely about not letting serialisation accidents dictate request outcomes, so leaving two-thirds of the pattern unguarded is a half-finished fix.♻️ Suggested helper + call sites
Add near the other trigger helpers:
function safeToJSON(parseObject) { try { return parseObject.toJSON(); } catch { // A malformed object (e.g. a pointer with an empty objectId) cannot be // serialised; log without it rather than fail the request (`#7490`). return { className: parseObject.className }; } }logTriggerAfterHook( triggerType, parseObject.className, - parseObject.toJSON(), + safeToJSON(parseObject), auth, config.logLevels.triggerAfter );logTriggerSuccessBeforeHook( triggerType, parseObject.className, - parseObject.toJSON(), + safeToJSON(parseObject), response,- let objectForLog; - try { - objectForLog = parseObject.toJSON(); - } catch { - // A malformed object (e.g. a pointer with an empty objectId) cannot be - // serialised; log without it rather than mask the original error (`#7490`). - objectForLog = { className: parseObject.className }; - } logTriggerErrorBeforeHook( triggerType, parseObject.className, - objectForLog, + safeToJSON(parseObject),Also applies to: 996-1005, 1020-1027
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/triggers.js` around lines 965 - 971, Extract the defensive serialization logic into a safeToJSON helper near the other trigger helpers, returning parseObject.toJSON() when possible and a className-only fallback when it throws. Replace the bare parseObject.toJSON() arguments in both logTriggerAfterHook and the corresponding log call around the after-trigger flow, and reuse the helper in the existing catch-block logging path.spec/CloudCode.spec.js (1)
156-175: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the
beforeSavehook actually ran, or this test can silently stop covering#7490.The regression is specifically "malformed pointer with a trigger installed". As written, the spec would pass just as happily if some future validation change rejected the pointer with
SCRIPT_FAILEDbeforemaybeRunTriggerwas ever reached — the coverage would quietly evaporate while the suite stayed green. A spy pins the test to the code path it claims to guard.I'd also note the test only proves the request settles with a 400; the hang it guards against is caught solely by Jasmine's timeout. That's acceptable, but the spy is what makes the assertion meaningful.
💚 Suggested tightening
- Parse.Cloud.beforeSave('Stuff', () => {}); + const hook = { beforeSave() {} }; + spyOn(hook, 'beforeSave').and.callThrough(); + Parse.Cloud.beforeSave('Stuff', hook.beforeSave); @@ expect(response.status).toBe(400); expect(response.data.code).toBe(Parse.Error.SCRIPT_FAILED); + expect(hook.beforeSave).toHaveBeenCalled();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@spec/CloudCode.spec.js` around lines 156 - 175, Update the beforeSave regression test to spy on or otherwise record execution of the installed Parse.Cloud.beforeSave hook, then assert that it ran after the request completes. Keep the existing malformed-pointer request and SCRIPT_FAILED response assertions, ensuring the test specifically covers the trigger execution path rather than only pre-trigger validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@spec/CloudCode.spec.js`:
- Around line 156-175: Update the beforeSave regression test to spy on or
otherwise record execution of the installed Parse.Cloud.beforeSave hook, then
assert that it ran after the request completes. Keep the existing
malformed-pointer request and SCRIPT_FAILED response assertions, ensuring the
test specifically covers the trigger execution path rather than only pre-trigger
validation.
In `@src/triggers.js`:
- Around line 965-971: Extract the defensive serialization logic into a
safeToJSON helper near the other trigger helpers, returning parseObject.toJSON()
when possible and a className-only fallback when it throws. Replace the bare
parseObject.toJSON() arguments in both logTriggerAfterHook and the corresponding
log call around the after-trigger flow, and reuse the helper in the existing
catch-block logging path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 241c130b-d1a9-44a7-a437-cf49c9995686
📒 Files selected for processing (2)
spec/CloudCode.spec.jssrc/triggers.js
1 participant
Closes #7490
A request with a malformed pointer (empty objectId) and a beforeSave trigger on the class would hang until timeout. On Node it actually crashes the process -
maybeRunTriggerwrapped its logic in anew Promiseexecutor whose inner chain was never connected toreject, so when serialising the object threw (Cannot create a pointer to an unsaved ParseObject) it became an unhandled rejection.Refactored
maybeRunTriggerto async/await so any throw in the trigger response path rejects cleanly (400 SCRIPT_FAILED) instead of hanging, and applied the same guard to the afterFind path.Summary by CodeRabbit