Before find fix #9769
Before find fix #9769
Conversation
Signed-off-by: Manuel <5673677+mtrezza@users.noreply.github.com>
|
🚀 Thanks for opening this pull request! |
📝 WalkthroughWalkthroughThe changes introduce support for short-circuiting the database query in Parse Server by allowing Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant REST API
participant runFindTriggers
participant beforeFindTrigger
participant afterFindTrigger
participant Database
Client->>REST API: find/get request
REST API->>runFindTriggers: call with query params
runFindTriggers->>beforeFindTrigger: run beforeFind
alt beforeFind returns objects/array
runFindTriggers->>afterFindTrigger: run afterFind with objects
afterFindTrigger-->>runFindTriggers: modified objects
runFindTriggers-->>REST API: return objects as JSON
else beforeFind returns nothing
runFindTriggers->>Database: execute query
Database-->>runFindTriggers: query results
runFindTriggers->>afterFindTrigger: run afterFind with results
afterFindTrigger-->>runFindTriggers: modified results
runFindTriggers-->>REST API: return results as JSON
end
REST API-->>Client: response
Assessment against linked issues
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 warn config production Use 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 (
|
![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 (3)
src/triggers.js (1)
592-600: Consider stricter array validation for better type safety.The logic correctly identifies when a beforeFind trigger returns objects to short-circuit the query. However, the array validation using
some()could accept arrays with mixed types (Parse.Objects and plain objects), which might cause issues downstream.Consider this more strict validation to ensure all array elements are Parse.Objects:
- } else if ( - Array.isArray(result) && - (!result.length || result.some(obj => obj instanceof Parse.Object)) - ) { + } else if ( + Array.isArray(result) && + (result.length === 0 || result.every(obj => obj instanceof Parse.Object)) + ) {This ensures that either the array is empty (valid empty result) or all elements are Parse.Objects, preventing potential type inconsistencies.
spec/CloudCode.spec.js (2)
231-244: Consider using a more realistic object ID in the get query test.While this test correctly validates the
getquery scenario, using a hardcoded string'objId'as the object ID parameter may not represent realistic usage patterns. Consider using a properly formatted object ID or documenting why this specific value is used.- const newObj = await new Parse.Query('beforeFind').get('objId'); + const newObj = await new Parse.Query('beforeFind').get('validObjectIdFormat');Alternatively, you could generate a valid object ID format or add a comment explaining the test scenario.
203-257: Consider more descriptive class names for better test clarity.All four new tests use
'beforeFind'as the class name, which could be confusing since it's also the name of the trigger type being tested. Consider using more descriptive class names like'TestObject','BeforeFindTestClass', or specific names for each test scenario to improve readability and reduce confusion.Example for better clarity:
-Parse.Cloud.beforeFind('beforeFind', () => { +Parse.Cloud.beforeFind('BeforeFindTestClass', () => {
📜 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(2 hunks)
🔇 Additional comments (11)
src/triggers.js (2)
458-461: LGTM: Good optimization to prevent redundant object conversion.The instance check prevents unnecessary re-conversion of objects that are already Parse.Object instances, improving performance and avoiding potential issues with redundant processing.
604-604: LGTM: Objects property correctly enables result short-circuiting.The
objectsproperty allows the calling code to access the Parse.Objects returned by the beforeFind trigger, enabling the short-circuit behavior described in the PR objectives.spec/CloudCode.spec.js (3)
203-216: Good test coverage for beforeFind returning a single object.This test effectively validates that
beforeFindcan return a singleParse.Objectinstance that bypasses the database query. The test structure is solid and validates both the trigger behavior and that the returned object is a proper Parse object that can be saved.
217-230: Well-implemented test for beforeFind returning an array of objects.This test properly validates the array return functionality and maintains consistency with the previous test pattern. The verification that
afterFindreceives the correct objects is particularly good.
245-257: Excellent test for empty array return scenario.This test effectively validates the edge case where
beforeFindreturns an empty array, properly setting up a real object in the database to ensure the trigger actually bypasses the query. The expectation thatnewObjisundefinedcorrectly validates the behavior.src/rest.js (6)
26-35: LGTM: Well-structured function signature.The function signature is clear and comprehensive, properly accepting all necessary parameters including the new
isGetflag to distinguish between find and get operations.
36-47: LGTM: Proper trigger execution and parameter updates.The beforeFind trigger is executed correctly, and the function properly updates
restWhereandrestOptionsfrom the trigger result using fallback assignment.
63-74: LGTM: Fallback query execution is correct.The fallback to normal RestQuery execution when no objects are returned by the trigger is implemented correctly, maintaining backward compatibility.
77-80: LGTM: Clean refactoring of find function.The function is now properly focused on its core responsibilities: enforcing security and delegating to the shared helper. This reduces code duplication and improves maintainability.
85-94: LGTM: Consistent implementation of get function.The get function correctly calls
runFindTriggerswith the appropriate parameters, including theisGetflag and the objectId constraint. This maintains the expected behavior while leveraging the shared logic.
48-62:✅ Verification successful
Verify object type assumptions in trigger-returned objects.
The code assumes that objects returned by the beforeFind trigger have a
_toFullJSON()method. While this should be handled by the trigger normalization mentioned in the AI summary, it would be good to verify this assumption.
🏁 Script executed:
#!/bin/bash # Description: Verify that maybeRunQueryTrigger normalizes returned objects to Parse.Object instances # Expected: Find evidence that the trigger system ensures returned objects are Parse.Object instances # Search for maybeRunQueryTrigger implementation and object normalization ast-grep --pattern 'maybeRunQueryTrigger($$$) { $$$ }' # Search for object normalization or Parse.Object conversion in triggers rg -A 10 -B 5 "Parse\.Object\.(fromJSON|new)" src/triggers.jsLength of output: 1217
Verified: Trigger-returned objects are Parse.Object instances
I confirmed that in src/triggers.js the afterFind pipeline runs each returned item through:
if (object instanceof Parse.Object) { return object; } return Parse.Object.fromJSON(object);This guarantees that every object has the
_toFullJSON()method. No further action is needed.
|
4 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