fix: Unhandled exception when calling Parse.Cloud.run with option value null (#2622)
#2623
Conversation
|
🚀 Thanks for opening this pull request! |
📝 Walkthrough""" WalkthroughThe static method Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Cloud
participant ParseObject
Client->>Cloud: run(name, data, options)
alt options is undefined or {}
Cloud->>ParseObject: _getRequestOptions({})
ParseObject-->>Cloud: {}
Cloud-->>Client: result
else options is null or not plain object
Cloud->>ParseObject: _getRequestOptions(options)
ParseObject-->>Cloud: throw Error
Cloud-->>Client: Error thrown
end
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. 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. 🔧 ESLint
npm error Exit handler never called! 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/__tests__/ParseObject-test.js (1)
3546-3565: LGTM! Well-structured tests for the falsy options fix.The test suite effectively covers the main falsy values (
null,"",undefined) that could cause runtime errors when passed toParse.Cloud.run. The tests are clear, concise, and directly address the issue described in the PR objectives.Consider adding tests for other falsy values for completeness:
+ it('returns empty object when options is false', () => { + const requestOptions = ParseObject._getRequestOptions(false); + expect(requestOptions).toEqual({}); + }); + + it('returns empty object when options is 0', () => { + const requestOptions = ParseObject._getRequestOptions(0); + expect(requestOptions).toEqual({}); + });Also consider adding a positive test case to ensure non-falsy values are handled correctly:
+ it('returns the options object when options is truthy', () => { + const options = { sessionToken: 'test' }; + const requestOptions = ParseObject._getRequestOptions(options); + expect(requestOptions).toEqual(options); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/ParseObject.ts(1 hunks)src/__tests__/ParseObject-test.js(1 hunks)
🔇 Additional comments (1)
src/ParseObject.ts (1)
517-519: LGTM! Excellent defensive programming practice.This early return when
optionsis falsy effectively prevents runtime errors whennull,undefined, or other falsy values are explicitly passed to the method. While the default parameter= {}handles the case when the argument is not provided, it doesn't protect against explicit falsy values likenull. This change ensures the method gracefully handles all falsy inputs by returning an emptyrequestOptionsobject, which aligns perfectly with the PR objective to fix issues whennullis passed as an option.
There was a problem hiding this comment.
Thanks for this PR! The first question is, do we really want a call to silently succeed, even if the developer made a coding mistake?
- Passing a string while an object is expected is a type violation, ergo clearly a coding mistake to which we want to alert the developer, so throwing makes sense.
- Passing
nullmay or may not be considered a coding mistake, but at a quick glance it seems like one to me. The allowed type for theoptionsargument isundefined(no options set), or an object. Anything else is a developer error. A variable that holdsnullmeans that the developer deliberately assigned no value to it, without ensuring that it is assigned the value of an object before passing it to the method. Hence it's a type violation as well.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## alpha #2623 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 63 63
Lines 6193 6195 +2
Branches 1460 1473 +13
=========================================
+ Hits 6193 6195 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@mtrezza Thanks for the feedback. Update this MR as you see fit. My take on this was mostly a bug fix: If "no options" then return an empty bracket, i.e. handle We found this error in |
|
I believe when throwing like suggested it should return a proper error, instead of an internal server error. We'd have to see in the test. Would you want to incorporate the changes? |
No, better you do it. I am unable to make changes for the next week as I am traveling and have limited access to the source code. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/__tests__/Cloud-test.js (1)
359-385: Consider adding more descriptive error message verification.While the tests correctly verify that errors are thrown for invalid options, consider enhancing the second test to also verify the specific error message to ensure consistent error reporting.
it('run throws with invalid options', () => { const values = [null, []]; for (const value of values) { - expect(() => Cloud.run('myfunction', {}, value)).toThrow(); + expect(() => Cloud.run('myfunction', {}, value)).toThrow('Invalid options type'); } });This would provide more precise validation of the error handling behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/ParseObject.ts(1 hunks)src/__tests__/Cloud-test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/ParseObject.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/__tests__/Cloud-test.js (15)
src/__tests__/RESTController-test.js (1)
CoreManager(7-7)src/__tests__/Parse-test.js (1)
CoreManager(14-14)src/__tests__/ParseObject-test.js (1)
CoreManager(146-146)src/__tests__/ParseFile-test.js (1)
CoreManager(11-11)src/__tests__/Hooks-test.js (1)
CoreManager(10-10)src/__tests__/ParseACL-test.js (1)
CoreManager(16-16)src/__tests__/LocalDatastore-test.js (1)
CoreManager(97-97)src/__tests__/ParseConfig-test.js (1)
CoreManager(19-19)src/__tests__/ParseRelation-test.js (1)
CoreManager(66-66)src/__tests__/ParseOp-test.js (1)
CoreManager(40-40)src/__tests__/Push-test.js (1)
CoreManager(20-20)src/__tests__/SingleInstanceStateController-test.js (1)
CoreManager(27-27)src/__tests__/canBeSerialized-test.js (1)
CoreManager(13-13)src/__tests__/Storage-test.js (1)
CoreManager(6-6)src/__tests__/unsavedChildren-test.js (1)
CoreManager(25-25)
🔇 Additional comments (2)
src/__tests__/Cloud-test.js (2)
360-377: Well-implemented test for valid empty options handling.This test correctly verifies that
Cloud.runaccepts bothundefinedand{}as valid options and properly defaults them to an empty object when passed to the underlying controller.The test setup with proper mocking and the iteration through valid values is well-structured.
379-384: Appropriate test for invalid options validation.This test correctly verifies that
Cloud.runthrows errors when passed invalid option types (nulland arrays). This aligns with the type safety approach mentioned in the AI summary where_getRequestOptionswas updated to enforce plain object validation.The test properly covers the edge cases that could cause runtime issues if not handled correctly.
|
@mtrezza I'm now back at work and I have now updated the code as I hope you wanted. Please review. Thanks. |
|
LGTM! I forgot the default option |
mtrezza
changed the title
Parse.Cloud.run with option value null (#2622)
Signed-off-by: Manuel <[email protected]>
Signed-off-by: Manuel <[email protected]>
Pull Request
This Pull Request is a fix for when passing null as a Parse.Cloud.run option
Issue
Closes: #2622
Closes: parse-community/parse-server#9766
Approach
Tasks
Summary by CodeRabbit
Bug Fixes
Tests