feat: Allow returning objects in Parse.Cloud.beforeFind without invoking database query
#9770
Conversation
Signed-off-by: Manuel <5673677+mtrezza@users.noreply.github.com>
Signed-off-by: Manuel <5673677+mtrezza@users.noreply.github.com>
…-server into beforeFind-fix
|
🚀 Thanks for opening this pull request! |
📝 WalkthroughWalkthroughCentralizes beforeFind/afterFind handling; allows beforeFind to short‑circuit find/get by returning objects/arrays; propagates isGet through trigger requests and RestQuery; refilters returned objects to enforce ACLs before afterFind; updates trigger utilities and adds tests for these behaviors. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant REST as REST Handler
participant Triggers
participant DB
Client->>REST: find/get request
REST->>Triggers: beforeFind(request { isGet, query, auth, ... })
alt beforeFind returns objects/array
Triggers-->>REST: objects/array
REST->>REST: refilter by IDs (no triggers) to enforce ACLs
REST->>Triggers: afterFind(request { isGet, query, auth, ... }, results)
Triggers-->>REST: processedResults
REST-->>Client: processedResults
else
Triggers-->>REST: (maybe modified) query/options
REST->>DB: execute query (find/get)
DB-->>REST: results
REST->>Triggers: afterFind(request { isGet, query, auth, ... }, results)
Triggers-->>REST: finalResults
REST-->>Client: finalResults
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Assessment against linked issues
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
src/rest.js (1)
53-54: Replace French comments with English for consistency.The codebase should maintain consistent language usage throughout.
- // Déclencher le trigger afterFind si des objets sont retournés + // Trigger afterFind if objects are returned- // Conserver la distinction entre get et find + // Maintain the distinction between get and findAlso applies to: 69-69
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
spec/CloudCode.spec.js(1 hunks)src/rest.js(2 hunks)src/triggers.js(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
spec/CloudCode.spec.js (1)
spec/CloudCode.Validator.spec.js (16)
Parse(2-2)obj(657-657)obj(674-674)obj(691-691)obj(713-713)obj(782-782)obj(806-806)obj(835-835)obj(851-851)obj(983-983)obj(1148-1148)obj(1169-1169)obj(1181-1181)obj(1206-1206)obj(1229-1229)obj(1247-1247)
🔇 Additional comments (7)
spec/CloudCode.spec.js (3)
204-216: LGTM: Well-structured test for beforeFind short-circuit functionality.This test correctly validates that
beforeFindcan return a single object directly, bypassing the database query. The test properly:
- Sets up both
beforeFindandafterFindtriggers- Validates the object properties and class name
- Confirms the returned object can be saved
218-230: LGTM: Good test for array return from beforeFind.This test correctly validates that
beforeFindcan return an array of objects. The logic is sound and follows the same pattern as the previous test.
246-257: LGTM: Proper test for empty array return.This test correctly validates that returning an empty array from
beforeFindresults in no objects being found, even when objects exist in the database.src/triggers.js (1)
475-477: LGTM! Good optimization to avoid unnecessary conversions.This change efficiently prevents redundant JSON-to-Parse.Object conversions when the object is already a Parse.Object instance, improving performance for the short-circuit functionality.
src/rest.js (3)
26-82: Excellent refactoring! The centralized trigger handling improves maintainability.The
runFindTriggersfunction effectively centralizes the beforeFind/afterFind trigger logic and correctly implements the short-circuit functionality. The logic properly:
- Runs the beforeFind trigger first
- Checks for returned objects to enable short-circuiting
- Runs afterFind when objects are present
- Falls back to normal query execution when no objects are returned
The distinction between
getandfindoperations is preserved through theisGetparameter.
85-97: Clean refactoring maintains existing functionality.Both
findandgetfunctions are properly refactored to use the newrunFindTriggershelper while preserving their original behavior through appropriate parameter passing.Also applies to: 100-112
65-65:✅ Verification successful
Verify _toFullJSON() compatibility with Parse.Object instances.
Ensure that calling
_toFullJSON()on Parse.Object instances returned from triggers works correctly and produces the expected serialization format.
🏁 Script executed:
#!/bin/bash # Description: Search for _toFullJSON usage and Parse.Object implementations to verify compatibility # Search for _toFullJSON method usage across the codebase rg -A 3 -B 3 "_toFullJSON" # Search for Parse.Object related implementations ast-grep --pattern 'Parse.Object.fromJSON($_)'Length of output: 5795
🏁 Script executed:
#!/bin/bash # Print the first 200 lines of src/rest.js to inspect the find handler sed -n '1,200p' src/rest.jsLength of output: 5761
🏁 Script executed:
#!/bin/bash sed -n '450,550p' src/triggers.js echo "----" sed -n '900,1050p' src/triggers.jsLength of output: 7099
_toFullJSON() Compatibility Confirmed
I’ve verified that any objects returned by triggers are always instances of Parse.Object—triggers wrap plain JSON via Parse.Object.fromJSON—and that parse.Object provides a _toFullJSON() method. Existing tests (e.g. ParseCloudCodePublisher.spec.js) already cover this serialization. No changes required.
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
Started CI... |
|
Hi @mtrezza, just wondering if there's anything else I should do to trigger a re-run of the tests? |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/rest.js (3)
66-69: Id-less objects short-circuit path: clarify/guard protectedFieldsComment says ACL doesn’t apply to unsaved objects, but returning them non-master also bypasses protectedFields sanitization. Either explicitly drop id-less objects for non-master or document that cloud code must sanitize them.
If you choose to guard, minimally:
- // Objects without IDs are(normally) unsaved objects - // For unsaved objects, the ACL security does not apply, so no need to redo the query. - // For saved objects, we need to re-query to ensure proper ACL/CLP enforcement + // Objects without IDs are (normally) unsaved. ACL checks don’t apply, + // but protectedFields may; returning them non-master may leak fields. + // Consider sanitizing here or dropping id-less objects for non-master.
98-101: Fix crash: spreading undefined and missingwhereinwithJSON
...restOptionsthrows whenrestOptionsis undefined; alsowheremay be undefined. Default both.Apply:
- new Parse.Query(className).withJSON({ where: restWhere, ...restOptions }), + new Parse.Query(className).withJSON({ where: restWhere || {}, ...(restOptions || {}) }),
58-106: afterFind must always receive and return arrays; enforce isGet single-object; honor countSingle-object short-circuit can pass a non-array to afterFind and return a non-array. Also, isGet can leak multiple objects and
countis ignored.Apply:
- let objectsForAfterFind = objectsFromBeforeFind; + // Normalize to array for consistent semantics + const inputArray = Array.isArray(objectsFromBeforeFind) + ? objectsFromBeforeFind + : [objectsFromBeforeFind]; + let objectsForAfterFind = inputArray; @@ - const ids = (Array.isArray(objectsFromBeforeFind) ? objectsFromBeforeFind : [objectsFromBeforeFind]) - .map(o => (o && (o.id || o.objectId)) || null) - .filter(Boolean); + const ids = inputArray + .map(o => (o && (o.id || o.objectId)) || null) + .filter(Boolean); @@ - const refiltered = await refilterQuery.execute(); - objectsForAfterFind = (refiltered && refiltered.results) || []; + const refiltered = await refilterQuery.execute(); + objectsForAfterFind = (refiltered && refiltered.results) || []; } } + // isGet must not return more than one object + if (isGet && objectsForAfterFind.length > 1) { + objectsForAfterFind = objectsForAfterFind.slice(0, 1); + } @@ - return { - results: afterFindProcessedObjects, - }; + const resultsArray = Array.isArray(afterFindProcessedObjects) + ? afterFindProcessedObjects + : [afterFindProcessedObjects]; + const response = { results: resultsArray }; + if (restOptions && restOptions.count) { + response.count = resultsArray.length; + } + return response;
🧹 Nitpick comments (1)
src/rest.js (1)
72-84: Preserve external ranking; don’t pass order/skip/limit to refilter; re-apply pagination after sanitizationRe-applying
restOptionsto the refilter query can change ordering/pagination relative to objects returned bybeforeFind(e.g., ES ranking).Apply:
- const refilterQuery = await RestQuery({ + const { order, skip, limit, ...refilterOptions } = (restOptions || {}); + const refilterQuery = await RestQuery({ method: isGet ? RestQuery.Method.get : RestQuery.Method.find, config, auth, className, restWhere: refilterWhere, - restOptions, + // Drop order/skip/limit during refilter; apply after sanitization + restOptions: refilterOptions, clientSDK, context, runBeforeFind: false, runAfterFind: false, }); @@ - const refiltered = await refilterQuery.execute(); - objectsForAfterFind = (refiltered && refiltered.results) || []; + const refiltered = await refilterQuery.execute(); + const sanitized = (refiltered && refiltered.results) || []; + const byId = new Map(sanitized.map(o => [(o.objectId || o.id), o])); + objectsForAfterFind = ids.map(id => byId.get(id)).filter(Boolean); + if (typeof skip === 'number' || typeof limit === 'number') { + const s = skip || 0; + const l = typeof limit === 'number' ? limit : objectsForAfterFind.length; + objectsForAfterFind = objectsForAfterFind.slice(s, s + l); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/rest.js(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: EmpiDev
PR: parse-community/parse-server#9770
File: src/triggers.js:467-477
Timestamp: 2025-08-27T12:33:06.237Z
Learning: In the Parse Server codebase, maybeRunAfterFindTrigger is called in production with Parse.Query objects constructed via withJSON(), so the plain object query handling bug only affects tests, not production code paths.
📚 Learning: 2025-08-26T14:06:31.853Z
Learnt from: EmpiDev
PR: parse-community/parse-server#9770
File: spec/CloudCode.spec.js:446-469
Timestamp: 2025-08-26T14:06:31.853Z
Learning: In the Parse Server codebase, when handling query objects in maybeRunAfterFindTrigger, objects without a where property that contain options like limit/skip should be treated as query JSON with an empty where clause using the spread pattern { where: {}, ...query }, not nested as { where: query }.
Applied to files:
src/rest.js
📚 Learning: 2025-08-27T12:33:06.237Z
Learnt from: EmpiDev
PR: parse-community/parse-server#9770
File: src/triggers.js:467-477
Timestamp: 2025-08-27T12:33:06.237Z
Learning: In the Parse Server codebase, maybeRunAfterFindTrigger is called in production with Parse.Query objects constructed via withJSON(), so the plain object query handling bug only affects tests, not production code paths.
Applied to files:
src/rest.js
🧬 Code graph analysis (1)
src/rest.js (1)
src/RestQuery.js (6)
className(967-967)result(55-66)triggers(6-6)isGet(53-53)query(328-337)query(338-338)
🔇 Additional comments (2)
src/rest.js (2)
124-136: find wrapper centralization looks goodRole enforcement first, then delegated to
runFindTriggers({ isGet: false }). Clean.
141-151: get wrapper centralization looks goodRole enforcement and
isGet: truepropagation are correct.
|
Started CI |
|
everything seems okay here @mtrezza :) |
|
@mtrezza i don't see some unresolved comments, or i can't resolve them because file are outdated or i approved the PR already |
|
@mtrezza I have resolved the lint issues. |
There was a problem hiding this comment.
@Moumouls I have found these open comments:
If you cannot resolve them because the button doesn't work, could you just confirm that they're resolved, so we can merge?
|
thanks @mtrezza , i reviewed, there is still one open comment: #9770 (comment) |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/triggers.js (1)
467-477: Optional: Improve query handling for plain objects withoutwhere.When a plain object without a
whereproperty (e.g.,{ limit: 5, skip: 0 }) is passed, the current code creates an emptyParse.Querywithout callingwithJSON, which silently drops query options likelimitandskip.Based on learnings, this issue only affects test scenarios since production code always passes
Parse.Queryinstances constructed viawithJSON(). However, for completeness and to avoid confusion when writing tests, consider applying the spread pattern:} else if (typeof query === 'object' && query !== null) { const parseQueryInstance = new Parse.Query(classNameQuery); - if (query.where) { - parseQueryInstance.withJSON(query); - } + const json = query.where ? query : { where: {}, ...query }; + parseQueryInstance.withJSON(json); request.query = parseQueryInstance;This ensures that top-level options are preserved when
whereis absent.Based on learnings.
spec/CloudCode.spec.js (1)
474-502: Test name doesn't match implementation.The test is named "should handle plain object query without where clause" but actually passes a
Parse.Queryinstance created withwithJSON({ limit: 5, skip: 1 }):const pq = new Parse.Query(className).withJSON({ limit: 5, skip: 1 });This tests the
query instanceof Parse.Querybranch (line 467 in triggers.js), not the plain object branch. Consider either:
- Renaming the test to "should handle Parse.Query created with withJSON without explicit where"
- Changing the test to pass a plain object:
{ limit: 5, skip: 1 }(though this would expose the issue mentioned in the optional refactor for triggers.js)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
spec/CloudCode.spec.js(1 hunks)src/rest.js(1 hunks)src/triggers.js(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/rest.js
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-08-27T12:33:06.237Z
Learnt from: EmpiDev
PR: parse-community/parse-server#9770
File: src/triggers.js:467-477
Timestamp: 2025-08-27T12:33:06.237Z
Learning: In the Parse Server codebase, maybeRunAfterFindTrigger is called in production with Parse.Query objects constructed via withJSON(), so the plain object query handling bug only affects tests, not production code paths.
Applied to files:
spec/CloudCode.spec.jssrc/triggers.js
📚 Learning: 2025-08-26T14:06:31.853Z
Learnt from: EmpiDev
PR: parse-community/parse-server#9770
File: spec/CloudCode.spec.js:446-469
Timestamp: 2025-08-26T14:06:31.853Z
Learning: In the Parse Server codebase, when handling query objects in maybeRunAfterFindTrigger, objects without a where property that contain options like limit/skip should be treated as query JSON with an empty where clause using the spread pattern { where: {}, ...query }, not nested as { where: query }.
Applied to files:
spec/CloudCode.spec.jssrc/triggers.js
📚 Learning: 2025-04-30T19:31:35.344Z
Learnt from: RahulLanjewar93
PR: parse-community/parse-server#9744
File: spec/ParseLiveQuery.spec.js:0-0
Timestamp: 2025-04-30T19:31:35.344Z
Learning: In the Parse Server codebase, the functions in QueryTools.js are typically tested through end-to-end behavior tests rather than direct unit tests, even though the functions are exported from the module.
Applied to files:
spec/CloudCode.spec.js
📚 Learning: 2025-08-27T09:08:34.252Z
Learnt from: EmpiDev
PR: parse-community/parse-server#9770
File: src/triggers.js:446-454
Timestamp: 2025-08-27T09:08:34.252Z
Learning: When analyzing function signature changes in Parse Server codebase, verify that call sites are actually incorrect before flagging them. Passing tests are a strong indicator that function calls are already properly aligned with new signatures.
Applied to files:
src/triggers.js
🧬 Code graph analysis (2)
spec/CloudCode.spec.js (3)
src/Config.js (1)
Config(35-768)spec/helper.js (3)
databaseAdapter(56-56)Parse(4-4)reconfigureServer(171-205)src/triggers.js (3)
result(1048-1048)result(1086-1086)maybeRunAfterFindTrigger(445-536)
src/triggers.js (2)
src/RestQuery.js (3)
className(967-967)isGet(53-53)result(55-66)src/rest.js (2)
query(109-119)result(39-48)
🔇 Additional comments (9)
src/triggers.js (5)
185-190: LGTM!The change correctly preserves the original object's className when no override is provided, maintaining backward compatibility while fixing cases where className might be lost during JSON conversion.
263-277: LGTM!The addition of the
isGetparameter is straightforward and correctly propagates the flag into the request object when defined.
456-463: LGTM!The no-trigger case correctly handles the conversion of Parse.Objects to JSON and returns an empty array for null/undefined inputs.
500-525: LGTM!The object conversion logic correctly:
- Preserves existing Parse.Object instances
- Converts plain objects to Parse.Object, maintaining their original className
- Executes the trigger with proper validator and skipWithMasterKey handling
642-654: LGTM!The result normalization correctly:
- Wraps single Parse.Object in an array
- Validates that all array elements are Parse.Objects using
every()(correctly updated from past feedback)- Returns the objects array as part of the response
spec/CloudCode.spec.js (4)
205-282: LGTM!The test suite comprehensively verifies the beforeFind short-circuit behavior:
- Properly uses spies to confirm no database operations occur
- Tests single object, array of objects, get query, and empty array scenarios
- Verifies afterFind receives the correct objects
- Confirms that returned unsaved objects can be saved afterward
284-375: LGTM! Critical security coverage.This test suite ensures that ACL filtering is properly applied to objects returned from beforeFind triggers:
- Verifies unauthorized users cannot access ACL-protected objects even when beforeFind returns them
- Confirms
get()throwsOBJECT_NOT_FOUNDfor unauthorized users- Tests master key bypass behavior
- Validates that
protectedFieldsmasking is applied after ACL re-filteringThis security coverage is essential for preventing data leaks when using the beforeFind short-circuit feature.
378-447: LGTM!The tests properly verify maybeRunAfterFindTrigger behavior for:
- Converting Parse.Object instances to JSON when no trigger is defined
- Handling null/undefined/empty array inputs gracefully
504-543: LGTM!The tests correctly verify that maybeRunAfterFindTrigger creates a default Parse.Query for invalid query parameters (strings and null).
|
Waiting for CI... |
mtrezza
changed the title
Parse.Cloud.beforeFind without invoking query
mtrezza
changed the title
Parse.Cloud.beforeFind without invoking queryParse.Cloud.beforeFind without invoking database query
# [8.3.0-alpha.5](8.3.0-alpha.4...8.3.0-alpha.5) (2025-10-14) ### Features * Allow returning objects in `Parse.Cloud.beforeFind` without invoking database query ([#9770](#9770)) ([0b47407](0b47407))
|
🎉 This change has been released in version 8.3.0-alpha.5 |
6 participants
Pull Request
Issue
Closes: #8693
Approach
This PR continues and supersedes #8694 by @dblythy.
Adds ability to return objects (or an empty array) from a beforeFind trigger.
Tasks
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Refactor