feat: Allow short circuit of beforeFind #9770
Conversation
Signed-off-by: Manuel <5673677+mtrezza@users.noreply.github.com>
Signed-off-by: Manuel <5673677+mtrezza@users.noreply.github.com>
|
🚀 Thanks for opening this pull request! |
📝 WalkthroughWalkthroughCentralizes beforeFind/afterFind handling so beforeFind can short‑circuit find/get by returning objects/arrays; propagates isGet through trigger requests and RestQuery; refilters returned objects with a secondary query for ACL enforcement; updates trigger utilities and RestQuery to support isGet; adds tests for beforeFind/afterFind behaviors and maybeRunAfterFindTrigger edge cases. 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 (runBeforeFind/runAfterFind disabled)
REST->>Triggers: afterFind(request { isGet, ... }, results)
Triggers-->>REST: finalObjects
REST-->>Client: finalObjects
else
Triggers-->>REST: (possibly modified) query/options
REST->>DB: execute query (find/get)
DB-->>REST: results
REST->>Triggers: afterFind(request { isGet, ... }, results)
Triggers-->>REST: finalResults
REST-->>Client: finalResults
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Assessment against linked issues
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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.✅ security/snyk check is complete. No issues have been found. (View Details) |
|
Started CI... |
|
Hi @mtrezza, just wondering if there's anything else I should do to trigger a re-run of the tests? |
|
@Moumouls would you kindly review the changes made after your feedback and close the feedback that you see as addressed? |
| describe('beforeFind without DB operations', () => { | ||
| let findSpy; | ||
|
|
||
| beforeEach(() => { | ||
| const config = Config.get('test'); | ||
| const databaseAdapter = config.database.adapter; | ||
| findSpy = spyOn(databaseAdapter, 'find').and.callThrough(); | ||
| }); | ||
|
|
||
| it('beforeFind can return object without DB operation', async () => { | ||
| Parse.Cloud.beforeFind('TestObject', () => { | ||
| return new Parse.Object('TestObject', { foo: 'bar' }); | ||
| }); | ||
| Parse.Cloud.afterFind('TestObject', req => { | ||
| expect(req.objects).toBeDefined(); | ||
| expect(req.objects[0].get('foo')).toBe('bar'); | ||
| }); | ||
|
|
||
| const newObj = await new Parse.Query('TestObject').first(); | ||
| expect(newObj.className).toBe('TestObject'); | ||
| expect(newObj.toJSON()).toEqual({ foo: 'bar' }); | ||
| expect(findSpy).not.toHaveBeenCalled(); | ||
| await newObj.save(); |
There was a problem hiding this comment.
suggestion: i think we need some additional use cases about security on objects returned in a before find, what is the behavior ?
If the code return object with ACL to user B for user A does parse return the full object, does parse filter again on ACL ?
to be honest i'm not 100% confident on the implementation code, because of some usage of instance of, feature detection, mutation of objects, and fragmented changes, if we can add some comments in the code it could helps for future maintenance/debug
There was a problem hiding this comment.
Answer to your questions:
- If
beforeFindreturns aQuery→ the normal flow (RestQuery) continues with ACL/CLP applied. - If
beforeFindreturns objects (Parse.Objector an array) → the DB is short-circuited:afterFindis executed, but ACL/CLP/protectedFields are not re-applied.
If I understand your message correctly, the actions I should take are:
-
Add explicit comments in
runFindTriggers,RestQuery, andtriggersto document the flow and potential risks. -
A proposed security fix for the “short-circuit” case:
- Re-apply ACL/CLP systematically when
beforeFindreturns objects (except ifuseMasterKeyis used).
- Re-apply ACL/CLP systematically when
-
Add security test cases:
beforeFindreturns objects not accessible to the current user → they must not be visible.protectedFieldsare correctly hidden after re-filtering.
There was a problem hiding this comment.
Hi @EmpiDev , yes it could be a step forward, i think enforcing ACL protection even when DB is short circuited is important to prevent potential security risks and unwanted data leak.
There was a problem hiding this comment.
Hi @Moumouls, I think I’ve made all the necessary changes. Could you check ?
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
spec/CloudCode.spec.js (8)
205-213: Good isolation of DB interaction through adapter spy; consider minimizing cross-test trigger leakage.Spying on
databaseAdapter.findinbeforeEachis the right way to assert no DB access. One potential source of flakiness is reusing a generic class name likeTestObjectacross many specs in this file; triggers persist until overwritten. Using a unique class name perdescribe(or callingreconfigureServer()in anafterEach) would further isolate tests and avoid accidental interference from other suites that also useTestObject.
214-228: Short-circuit: single object path is well covered; add an explicit afterFind invocation assertion.The test validates object materialization and absence of DB calls. To fully prove the trigger pipeline, assert
afterFindran exactly once using a spy.Apply this minimal augmentation:
Parse.Cloud.afterFind('TestObject', req => { - expect(req.objects).toBeDefined(); - expect(req.objects[0].get('foo')).toBe('bar'); + expect(req.objects).toBeDefined(); + expect(req.objects[0].get('foo')).toBe('bar'); }); +const afterSpy = jasmine.createSpy('afterFindSpy'); +Parse.Cloud.afterFind('TestObject', afterSpy); const newObj = await new Parse.Query('TestObject').first(); ... +expect(afterSpy).toHaveBeenCalledTimes(1);
246-264: Short-circuit: get() path validated; consider asserting 0 DB calls end-to-end with an afterFind spy.The creation/reset of the spy before
get()is solid. As a small enhancement, assertafterFindwas called once to confirm the pipeline ran in the short-circuited GET path.
266-282: Short-circuit: empty array behavior verified; optionally add a find() assertion.Great:
first()returnsundefinedand no DB call occurs. You could also assert thatfind()returns[]under the same short-circuit to round out behavior.
319-346: Security regression coverage: protectedFields masking verified; add master-key counterpart.Good validation that protected fields are masked after re-filtering. Please add the master-key counterpart to confirm protected fields are visible with
{ useMasterKey: true }.Apply:
const res = await new Parse.Query('SecretMask').first({ sessionToken: user.getSessionToken() }); ... expect(Object.prototype.hasOwnProperty.call(json, 'secretField')).toBeFalse(); + +// With master key, protectedFields should not be masked +const resMk = await new Parse.Query('SecretMask').first({ useMasterKey: true }); +expect(resMk.get('secretField')).toBe('hiddenValue');
356-382: No-trigger path conversion: avoid manually setting object ids for unsaved Parse.Objects.Setting
parseObj1.id = 'obj1'andparseObj2.id = 'obj2'is unnecessary and can be misleading; the test only asserts attributes, not objectId. Removing these lines simplifies the test and avoids implying that settingidon unsaved instances is a supported pattern.Apply:
const parseObj1 = new Parse.Object(className); parseObj1.set('name', 'test1'); -parseObj1.id = 'obj1'; const parseObj2 = new Parse.Object(className); parseObj2.set('name', 'test2'); -parseObj2.id = 'obj2';
421-444: Plain object query with where: strengthen assertions to verify query semantics.You correctly assert
req.queryis aParse.Query. Add checks to ensure thewhereand options (e.g.,limit) survive conversion and are applied.Apply:
expect(receivedQuery).toBeInstanceOf(Parse.Query); -expect(result).toBeDefined(); +const qJSON = receivedQuery.toJSON(); +expect(qJSON.where).toEqual({ name: 'test' }); +expect(qJSON.limit).toBe(10); +expect(result).toBeDefined();
471-509: Invalid query parameter fallbacks: good defaulting behavior.Defaulting to a class-scoped
Parse.Querywhen the query parameter is invalid or null is sensible; assertions are crisp. Consider also asserting thattoJSON()yields an emptywhereto fully specify the default shape.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
spec/CloudCode.spec.js(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
PR: parse-community/parse-server#9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: Tests in the parse-server repository should use promise-based approaches rather than callback patterns with `done()`. Use a pattern where a Promise is created that resolves when the event occurs, then await that promise.
Applied to files:
spec/CloudCode.spec.js
📚 Learning: 2025-05-04T20:41:05.147Z
Learnt from: mtrezza
PR: parse-community/parse-server#9445
File: spec/ParseLiveQuery.spec.js:1312-1338
Timestamp: 2025-05-04T20:41:05.147Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`.
Applied to files:
spec/CloudCode.spec.js
🧬 Code graph analysis (1)
spec/CloudCode.spec.js (2)
spec/helper.js (3)
databaseAdapter(56-56)Parse(4-4)reconfigureServer(171-205)src/triggers.js (3)
result(1047-1047)result(1085-1085)maybeRunAfterFindTrigger(445-535)
🔇 Additional comments (3)
spec/CloudCode.spec.js (3)
230-244: Short-circuit: array path looks good.Coverage and assertions are appropriate: objects are returned, afterFind gets the objects, and DB is not called.
348-348: Targeted import is fine.Importing
maybeRunAfterFindTriggerdirectly is appropriate for unit-level behavior coverage outside the HTTP surface.
384-419: Null/undefined/empty inputs: good defensive coverage.Behavior matches expectations of returning an empty array.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/rest.js (2)
36-46: Good:isGetis now propagated intobeforeFindtrigger contextPassing
isGettotriggers.maybeRunQueryTriggergives Cloud Code full awareness of whether the operation is a find or a get. This addresses earlier feedback about clarity and avoids boolean-argument ambiguity by using an options object. Nicely done.
114-125: Good: role security is enforced prior to delegating to unified trigger flowCalling
enforceRoleSecurity('find', …)beforerunFindTriggersis clear and addresses prior confusion about where role checks happen.
🧹 Nitpick comments (1)
src/rest.js (1)
56-76: Potential performance issue with very large result sets returned bybeforeFindA very large
$inarray onobjectIdmay hit database parameter/packet limits or degrade performance. Consider batching the refilter into chunks (e.g., 1–5k IDs per batch) and merging results before ordering/pagination, or enforce a reasonable ceiling on the number of IDs accepted frombeforeFind.Would you like me to propose a chunking implementation that preserves order and pagination across batches?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/rest.js(1 hunks)src/triggers.js(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/triggers.js
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-26T14:06:31.809Z
Learnt from: EmpiDev
PR: parse-community/parse-server#9770
File: spec/CloudCode.spec.js:446-469
Timestamp: 2025-08-26T14:06:31.809Z
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
🧬 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 (3)
src/rest.js (3)
82-91: Good: correct construction of theParse.QueryforafterFindUsing
withJSON({ where: restWhere, ...restOptions })correctly spreads options without nesting them underwhere. This matches the expected pattern and avoids misinterpreting options likelimit/skip.
98-108: Good: centralized query execution withrunBeforeFind: falseDelegating to
RestQuerywithrunBeforeFind: falseprevents double-invocation ofbeforeFind. DefaultingrunAfterFindto true here is appropriate becauseafterFindfor the non-short-circuit path should occur insideRestQuery.
130-140: Confirmed:getthroughrunFindTriggerspreserves the array contract and response format
- The internal
maybeRunAfterFindTriggeralways wraps a singleParse.Objectin an array (viaobjectsInput.map(...)when there’s no trigger), so afterFind hooks always receiveobjectsas an array.- For the
getpath,runFindTriggersreturns{ results: afterFindProcessedObjects }unconditionally (and omitscountunlessrestOptions.countis true), matching the existing{ results: […] }shape.No changes are needed here—the hooks API and REST response remain backward-compatible.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/rest.js (2)
60-86: Preserve custom ranking and apply skip/limit after ACL sanitization (don’t pass order/skip/limit to refilter).Reapplying client
order/skip/limitinside the refilter query can scramble the custom ranking frombeforeFindand paginate on DB order. Instead: droporder/skip/limitfor the refilter query, then reorder sanitized results to match the originalids, and finally applyskip/limit.- if (ids.length > 0) { - const refilterWhere = isGet ? { objectId: ids[0] } : { objectId: { $in: ids } }; - - // Re-query with proper security: no triggers to avoid infinite loops - const refilterQuery = await RestQuery({ - method: isGet ? RestQuery.Method.get : RestQuery.Method.find, - config, - auth, - className, - restWhere: refilterWhere, - restOptions, - clientSDK, - context, - runBeforeFind: false, - runAfterFind: false, - }); - - const refiltered = await refilterQuery.execute(); - objectsForAfterFind = (refiltered && refiltered.results) || []; - } + if (ids.length > 0) { + const refilterWhere = isGet ? { objectId: ids[0] } : { objectId: { $in: ids } }; + // Exclude order/skip/limit for refilter; they will be reapplied after preserving ranking + const { order, skip, limit, ...refilterOptions } = restOptions || {}; + const refilterQuery = await RestQuery({ + method: isGet ? RestQuery.Method.get : RestQuery.Method.find, + config, + auth, + className, + restWhere: refilterWhere, + restOptions: refilterOptions, + clientSDK, + context, + runBeforeFind: false, + runAfterFind: false, + }); + const refiltered = await refilterQuery.execute(); + const sanitized = (refiltered && refiltered.results) || []; + const byId = new Map(sanitized.map(o => [(o.objectId || o.id), o])); + // Preserve the original ranking coming from beforeFind + objectsForAfterFind = ids.map(id => byId.get(id)).filter(Boolean); + // Re-apply pagination as requested by the client + const s = typeof skip === 'number' ? skip : 0; + if (typeof limit === 'number') { + objectsForAfterFind = objectsForAfterFind.slice(s, s + limit); + } else if (s) { + objectsForAfterFind = objectsForAfterFind.slice(s); + } + }
88-103: Honorcountin short-circuit responses and ensure array-shaped results.When
beforeFindreturns objects, the response currently dropscount. Also, be explicit thatresultsis an array.- // Run afterFind trigger on security-filtered objects + // Run afterFind trigger on security-filtered objects const afterFindProcessedObjects = await triggers.maybeRunAfterFindTrigger( triggers.Types.afterFind, auth, className, objectsForAfterFind, config, new Parse.Query(className).withJSON({ where: restWhere, ...restOptions }), - context, + // Expose a context flag so afterFind can adapt if needed + { ...(context || {}), beforeFindReturnObjects: true }, isGet ); - 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 (3)
src/rest.js (1)
58-59: Normalize to array for clarity and defensive correctness.
result.objectsis currently an array, but normalizing here avoids subtle bugs and improves readability.- let objectsForAfterFind = objectsFromBeforeFind; + let objectsForAfterFind = Array.isArray(objectsFromBeforeFind) + ? objectsFromBeforeFind + : [objectsFromBeforeFind];src/triggers.js (2)
490-510: Guard against non-arrayobjectsInputand avoid double JSON.stringify in logs.
objectsInput.map(...)will throw if a non-array sneaks in. Also,logTriggerSuccessBeforeHookinternally stringifies, so passing a stringified payload results in double encoding.- logTriggerSuccessBeforeHook( - triggerType, - classNameQuery, - 'AfterFind Input (Pre-Transform)', - JSON.stringify( - objectsInput.map(o => (o instanceof Parse.Object ? o.id + ':' + o.className : o)) - ), - auth, - config.logLevels.triggerBeforeSuccess - ); + const inputArr = Array.isArray(objectsInput) ? objectsInput : []; + logTriggerSuccessBeforeHook( + triggerType, + classNameQuery, + 'AfterFind Input (Pre-Transform)', + inputArr.map(o => (o instanceof Parse.Object ? `${o.id}:${o.className}` : o)), + auth, + config.logLevels.triggerBeforeSuccess + ); @@ - request.objects = objectsInput.map(currentObject => { + request.objects = inputArr.map(currentObject => {
456-466: Fallback when no afterFind trigger exists is mostly fine; minor robustness note.The branch returns
objectsInputas-is (JSON) when inputs are not Parse.Objects. Given upstream ensures arrays in the new short-circuit flow, this is fine. Consider normalizing to[]when falsy to be extra safe.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/rest.js(1 hunks)src/triggers.js(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-26T14:06:31.809Z
Learnt from: EmpiDev
PR: parse-community/parse-server#9770
File: spec/CloudCode.spec.js:446-469
Timestamp: 2025-08-26T14:06:31.809Z
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/triggers.jssrc/rest.js
🧬 Code graph analysis (1)
src/triggers.js (2)
src/RestWrite.js (3)
className(1742-1742)request(818-824)request(953-959)src/rest.js (2)
query(106-116)result(39-48)
🔇 Additional comments (6)
src/rest.js (2)
121-149: Role security checks added for find/get look good.Good defensive gate before delegating to the unified trigger flow.
105-118: No duplicateafterFindexecutionI’ve confirmed that in both the normal and short-circuit paths in
src/rest.js,RestQueryis invoked with bothrunAfterFind: false, runBeforeFind: false,(around lines 78–82 and 273–277), so the built-in
afterFindtrigger is disabled duringexecute(). The only timeafterFindruns is via the manualmaybeRunAfterFindTriggerin the short-circuit branch. Therefore, there is no risk ofafterFindfiring twice.src/triggers.js (4)
185-190: Preserving className in toJSON is correct and avoids losing type info.This fixes downstream consumers that rely on
classNamein JSON.
263-278: Adding isGet to request is a useful signal for triggers.Good addition; aligns with the PR’s goal to unify get/find flows.
467-477: Correct handling of plain query objects lackingwhere.Using
{ where: {}, ...query }matches expected Parse.Query JSON semantics and avoids nestingwhereincorrectly.
644-652: Array type check tightened toevery()is correct.Prevents mixed arrays from entering the short-circuit path.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/triggers.js (1)
642-650: Array validation tightened (every instead of some).Prevents mixed-type arrays from short-circuiting beforeFind, aligning with expectations. Nice.
🧹 Nitpick comments (3)
src/triggers.js (3)
488-497: Logging: avoid double JSON stringification and pass structured data.
logTriggerSuccessBeforeHookstringifies internally; pre-stringifying produces quoted JSON in logs. Also, passing the items array (not a string) yields cleaner logs.Apply:
- logTriggerSuccessBeforeHook( - triggerType, - classNameQuery, - 'AfterFind Input (Pre-Transform)', - JSON.stringify( - objectsInput.map(o => (o instanceof Parse.Object ? o.id + ':' + o.className : o)) - ), - auth, - config.logLevels.triggerBeforeSuccess - ); + const itemsForLog = (Array.isArray(objectsInput) ? objectsInput : []).map(o => + o instanceof Parse.Object ? `${o.id}:${o.className}` : o + ); + logTriggerSuccessBeforeHook( + triggerType, + classNameQuery, + 'AfterFind Input (Pre-Transform)', + itemsForLog, + auth, + config.logLevels.triggerBeforeSuccess + );
499-508: Guard against non-arrayobjectsInput.
objectsInput.map(...)will throw if undefined/null. Cheap defensive fix.Apply:
- request.objects = objectsInput.map(currentObject => { + const inputArray = Array.isArray(objectsInput) ? objectsInput : []; + request.objects = inputArray.map(currentObject => {
527-533: Logging: avoid double stringify on results.
logTriggerAfterHookstringifies internally; pass the value directly.Apply:
- logTriggerAfterHook( - triggerType, - classNameQuery, - JSON.stringify(resultsAsJSON), - auth, - config.logLevels.triggerAfter - ); + logTriggerAfterHook( + triggerType, + classNameQuery, + resultsAsJSON, + auth, + config.logLevels.triggerAfter + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
spec/CloudCode.spec.js(1 hunks)src/triggers.js(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- spec/CloudCode.spec.js
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-26T14:06:31.809Z
Learnt from: EmpiDev
PR: parse-community/parse-server#9770
File: spec/CloudCode.spec.js:446-469
Timestamp: 2025-08-26T14:06:31.809Z
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/triggers.js
🧬 Code graph analysis (1)
src/triggers.js (2)
src/RestQuery.js (3)
className(967-967)isGet(53-53)result(55-66)src/rest.js (2)
query(106-116)result(39-48)
🔇 Additional comments (3)
src/triggers.js (3)
185-190: Good change: preserve className in JSON when not overridden.This prevents losing the original class and avoids surprises in downstream serialization.
263-278: Add isGet to request object.Looks correct; conditional assignment avoids altering existing behavior for non-get flows.
459-463: No-trigger fast path is sensible.Returning pre-JSONed objects or [] preserves existing behavior with minimal overhead.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/rest.js(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-26T14:06:31.809Z
Learnt from: EmpiDev
PR: parse-community/parse-server#9770
File: spec/CloudCode.spec.js:446-469
Timestamp: 2025-08-26T14:06:31.809Z
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
🧬 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 (3)
src/rest.js (3)
26-47: Good centralization and isGet propagationConsolidating find/get trigger flow into runFindTriggers and passing isGet through before/after triggers reads clean and reduces duplication.
69-71: Good: id-less objects dropped for non-master/non-maintenancePrevents ACL/CLP/protectedFields bypass when beforeFind returns objects without objectId.
127-140: Role security enforcement and delegation look correctCalling enforceRoleSecurity for find/get and delegating to runFindTriggers with explicit isGet flags is clear and consistent.
Also applies to: 144-154
…ndTriggers" This reverts commit 0d68f26.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/rest.js (3)
95-95: withJSON should receive{ where: {} }whenrestWhereis undefinedThis aligns with team learnings and avoids test-only edge cases. Included in the larger diff above.
62-66: Block id‑less objects for non‑master/non‑maintenanceWithout an
objectId, refiltering is skipped and unsanitized objects can be returned. The unified diff above drops id‑less objects to maintain ACL/CLP/protectedFields guarantees.
58-102: Fix security leak, preserve ranking/pagination, normalize arrays, and honorcountin short‑circuit path
- Non‑master requests can leak unsanitized fields when
beforeFindreturns id‑less objects.afterFindmay receive a non‑array; response may return a non‑array.- Refiltering with
order/skip/limitbreaks external ranking and double‑applies pagination.countis ignored whenbeforeFindshort‑circuits.- For get, multiple objects can slip through.
- Also default
whereto{}for withJSON (per retrieved learnings).Apply this unified diff:
- let objectsForAfterFind = objectsFromBeforeFind; + // Normalize to array for consistent trigger semantics + const inputArray = Array.isArray(objectsFromBeforeFind) + ? objectsFromBeforeFind + : [objectsFromBeforeFind]; + let objectsForAfterFind = inputArray; @@ - if (!auth?.isMaster && !auth?.isMaintenance) { - const ids = (Array.isArray(objectsFromBeforeFind) ? objectsFromBeforeFind : [objectsFromBeforeFind]) - .map(o => (o && (o.id || o.objectId)) || null) - .filter(Boolean); - - if (ids.length > 0) { + if (!auth?.isMaster && !auth?.isMaintenance) { + const ids = inputArray + .map(o => (o && (o.id || o.objectId)) || null) + .filter(Boolean); + // No valid ids => do not return unsanitized data + if (ids.length === 0) { + objectsForAfterFind = []; + } else { const refilterWhere = isGet ? { objectId: ids[0] } : { objectId: { $in: ids } }; - // Re-query with proper security: no triggers to avoid infinite loops - const refilterQuery = await RestQuery({ + // Re-query with proper security: drop order/skip/limit to preserve external ranking. + // Reapply pagination after sanitization. + const { order, skip, limit, ...refilterOptions } = restOptions || {}; + const refilterQuery = await RestQuery({ method: isGet ? RestQuery.Method.get : RestQuery.Method.find, config, auth, className, restWhere: refilterWhere, - restOptions, + 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])); + // Preserve original ranking from beforeFind + objectsForAfterFind = ids.map(id => byId.get(id)).filter(Boolean); } } + // Enforce get cardinality + if (isGet && objectsForAfterFind.length > 1) { + objectsForAfterFind = objectsForAfterFind.slice(0, 1); + } + // Apply pagination after sanitization + const totalBeforePagination = objectsForAfterFind.length; + const s = restOptions?.skip || 0; + const l = typeof restOptions?.limit === 'number' ? restOptions.limit : objectsForAfterFind.length; + if (s || typeof restOptions?.limit === 'number') { + objectsForAfterFind = objectsForAfterFind.slice(s, s + l); + } + // Run afterFind trigger on security-filtered objects const afterFindProcessedObjects = await triggers.maybeRunAfterFindTrigger( triggers.Types.afterFind, auth, className, objectsForAfterFind, config, - new Parse.Query(className).withJSON({ where: restWhere, ...restOptions }), + // Ensure empty where when undefined (see retrieved learnings) + new Parse.Query(className).withJSON({ where: restWhere || {}, ...restOptions }), context, isGet ); - return { - results: afterFindProcessedObjects, - }; + // Ensure array response and honor `count` when requested + const resultsArray = Array.isArray(afterFindProcessedObjects) + ? afterFindProcessedObjects + : [afterFindProcessedObjects]; + const response = { results: resultsArray }; + if (restOptions?.count) { + response.count = totalBeforePagination; + } + return response;
🧹 Nitpick comments (1)
src/rest.js (1)
36-36: DefaultisGettofalsePrevents accidental undefined behavior if
optionsis omitted.- const { isGet } = options; + const { isGet = false } = options;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 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 (3)
src/RestQuery.js (6)
className(967-967)result(55-66)triggers(6-6)isGet(53-53)query(328-337)query(338-338)src/Auth.js (11)
restWhere(261-267)restWhere(367-367)restOptions(154-157)RestQuery(158-158)RestQuery(215-215)RestQuery(268-268)RestQuery(368-368)query(96-104)query(105-105)query(159-167)query(269-276)src/Routers/CloudCodeRouter.js (1)
triggers(4-4)
🔇 Additional comments (2)
src/rest.js (2)
121-133: Role security enforcement for find is in the right placeCalling
enforceRoleSecurity('find', ...)before delegating centralizes the check and mirrors existing patterns.
138-148: Single-object response semantics verified
Upstream routers (e.g. SessionsRouter, ClassesRouter), GraphQL helpers, and existing tests consistently unwrap and assert a single-item array viaresponse.results[0]andresults.length === 1, so enforcingisGet: truealigns with current expectations.
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