feat: Parallel include pointers #9861
Conversation
…moumouls/concurrent-include # Conflicts: # package.json
|
I will reformat the title to use the proper commit message syntax. |
parse-github-assistant
bot
changed the title
|
🚀 Thanks for opening this pull request! |
📝 WalkthroughWalkthrough
Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RestQuery
participant ExecTreeBuilder as ExecutionTree
participant Store as DataFetch
Client->>RestQuery: query with multiple include paths
activate RestQuery
RestQuery->>ExecTreeBuilder: buildExecutionTree(this.include)
ExecTreeBuilder-->>RestQuery: execution tree (dependencies)
rect rgb(220,245,220)
note over RestQuery,Store: Parallel fetches for independent include branches
RestQuery->>Store: fetch pointers for branch A
RestQuery->>Store: fetch pointers for branch B
end
Store-->>RestQuery: branch results
rect rgb(245,230,245)
note over RestQuery,Store: After parent resolved -> parallel nested fetches
RestQuery->>Store: fetch nested includes (e.g., post.author, post.category)
end
Store-->>RestQuery: nested results
RestQuery->>RestQuery: recursively propagate/hydrate results into root
RestQuery->>RestQuery: this.include = []
RestQuery-->>Client: hydrated results
deactivate RestQuery
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches✅ Passed checks (5 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 |
✅ 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. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## alpha #9861 +/- ##
==========================================
+ Coverage 84.05% 93.08% +9.03%
==========================================
Files 187 187
Lines 15224 15234 +10
Branches 177 177
==========================================
+ Hits 12796 14181 +1385
+ Misses 2412 1041 -1371
+ Partials 16 12 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…moumouls/concurrent-include # Conflicts: # .gitignore
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
1 similar comment
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/RestQuery.js(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.917Z
Learning: When reviewing Parse Server PRs that add new features, always check whether the feature is documented in the README.md file, though for new Parse Server options this is optional rather than required.
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 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()`.
Learnt from: EmpiDev
Repo: parse-community/parse-server PR: 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 }.
📚 Learning: 2025-08-26T14:06:31.853Z
Learnt from: EmpiDev
Repo: parse-community/parse-server PR: 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/RestQuery.js
| const indexedResults = this.response.results.reduce((indexed, result, i) => { | ||
| indexed[result.objectId] = i; | ||
| return indexed; | ||
| }, {}); | ||
|
|
||
| // Build the execution tree | ||
| const executionTree = {} | ||
| this.include.forEach(path => { | ||
| let current = executionTree; | ||
| path.forEach((node) => { | ||
| if (!current[node]) { | ||
| current[node] = { | ||
| path, | ||
| children: {} | ||
| }; | ||
| } | ||
| current = current[node].children | ||
| }); | ||
| } else if (this.include.length > 0) { | ||
| this.include = this.include.slice(1); | ||
| return this.handleInclude(); | ||
| }); | ||
|
|
||
| const recursiveExecutionTree = async (treeNode) => { | ||
| const { path, children } = treeNode; | ||
| const pathResponse = includePath( | ||
| this.config, | ||
| this.auth, | ||
| this.response, | ||
| path, | ||
| this.context, | ||
| this.restOptions, | ||
| this, | ||
| ); | ||
| if (pathResponse.then) { | ||
| const newResponse = await pathResponse | ||
| newResponse.results.forEach(newObject => { | ||
| // We hydrate the root of each result with sub results | ||
| this.response.results[indexedResults[newObject.objectId]][path[0]] = newObject[path[0]]; | ||
| }) | ||
| } | ||
| return Promise.all(Object.values(children).map(recursiveExecutionTree)); | ||
| } | ||
|
|
||
| return pathResponse; | ||
| await Promise.all(Object.values(executionTree).map(recursiveExecutionTree)); |
There was a problem hiding this comment.
Hydration must not depend solely on objectId.
indexedResults collapses every row whose objectId is missing (e.g. aggregate pipelines that $project out _id while keeping pointer fields) into the single undefined bucket. In that case only the last element in this.response.results is updated and the remaining rows keep their raw pointers, whereas the previous sequential implementation handled these projections correctly. That’s a correctness regression.
Please rework the assignment so we still hydrate by index whenever we cannot resolve an objectId, e.g. by falling back to the loop index.
- newResponse.results.forEach(newObject => {
- this.response.results[indexedResults[newObject.objectId]][path[0]] = newObject[path[0]];
- })
+ newResponse.results.forEach((newObject, idx) => {
+ const targetIndex = indexedResults[newObject.objectId];
+ const target = targetIndex !== undefined ? this.response.results[targetIndex] : this.response.results[idx];
+ if (target) {
+ target[path[0]] = newObject[path[0]];
+ }
+ })🤖 Prompt for AI Agents
In src/RestQuery.js around lines 864 to 905, the hydration currently collapses
rows with missing objectId because indexedResults maps undefined to a single
bucket; update the logic so indexedResults only maps defined objectId values to
their original index, and when applying newResponse.results use the iteration
index as a fallback: in the forEach use the callback index (e.g. (newObject, i))
and compute targetIndex = (newObject.objectId !== undefined &&
indexedResults[newObject.objectId] !== undefined) ?
indexedResults[newObject.objectId] : i, then assign
this.response.results[targetIndex][path[0]] = newObject[path[0]]; ensure you
handle falsy but valid objectId values correctly by explicitly checking
undefined.
There was a problem hiding this comment.
objectId is always present in parse returned include, does the issue still exists ?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there are any safeguards preventing includes with aggregate/pipeline queries
rg -n -C3 "pipeline.*include|include.*pipeline" --type=js
# Search for how aggregate queries handle includes
rg -n -C3 "findOptions.pipeline" --type=js -g "src/**"
# Check if there's validation that prevents includes on aggregate queries
rg -n -C3 "aggregate.*include|pipeline.*include" --type=js -g "spec/**"
# Look for any code that explicitly checks for objectId before processing includes
rg -n -C3 "objectId.*include|include.*objectId" --type=js -g "src/RestQuery.js"
Length of output: 717
🏁 Script executed:
#!/bin/bash
# Check the full execute() method to see the order of operations
ast-grep --pattern $'_UnsafeRestQuery.prototype.execute = function ($$$) {
$$$
}'
# Look for any validation that prevents includes with pipeline/aggregate
rg -n -B5 -A10 "handleInclude" src/RestQuery.js | head -60
# Search for tests that combine includes with aggregate queries
rg -n "aggregate.*include|pipeline.*include" spec/ --type=js -C2
# Check if aggregate queries can have the include option at all
rg -n "pipeline.*restOptions|aggregate.*restOptions" --type=js -g "spec/**" | head -20
Length of output: 3354
🏁 Script executed:
#!/bin/bash
# Check how AlwaysSelectedKeys (which includes objectId) is used
rg -n "AlwaysSelectedKeys" src/RestQuery.js -A3 -B3
# Look for aggregate query tests and whether they can have includes
rg -n "aggregate|pipeline" spec/RestQuery.spec.js -C3 | head -100
# Check if there's any code that strips objectId from results
rg -n "delete.*objectId|objectId.*delete" --type=js -g "src/**"
# Look for $project in aggregate pipelines that might exclude _id
rg -n '\$project' spec/ --type=js -C2 | head -50
Length of output: 4131
🏁 Script executed:
#!/bin/bash
# Check if aggregate queries can have include in their options
rg -n "include" spec/ParseQuery.Aggregate.spec.js -C2 | head -50
# Look for the aggregate route handler to see what options it accepts
rg -n "handleFind.*pipeline|pipeline.*handleFind" --type=js -g "src/**"
# Check RestQuery constructor to see if it validates pipeline + include
ast-grep --pattern $'function _UnsafeRestQuery(
$$$
) {
$$$
}'
# Search for any code that prevents includes on pipeline queries
rg -n "pipeline.*include|include.*pipeline" src/ --type=js -C3
Length of output: 9784
@Moumouls, You're right that in normal query results, objectId is always present. However, I found an edge case where it can be missing:
The edge case: When a RestQuery has both:
- A
pipelineoption with$project: { _id: 0 }(explicitly excluding objectId) - An
includeoption
Looking at the code:
- The constructor accepts both
pipeline(line 215) andinclude(line 236) without validation preventing their combination handleInclude()does NOT skip aggregate queries (unlikerunAfterFindTrigger()which checks forpipeline/distinctat line 927)- Tests show aggregate queries can use
$project: { _id: 0 }to exclude objectId (e.g.,spec/ParseQuery.Aggregate.spec.js:362-363)
While this is admittedly a rare/unusual scenario (combining includes with aggregates that exclude _id), the original concern is technically valid.
Recommendation: Either add a guard to skip handleInclude() when pipeline or distinct is present (similar to runAfterFindTrigger), or implement the fallback indexing logic suggested in the original comment to handle this edge case safely.
What do you think? Should we add protection against this edge case or is it acceptable to document that includes aren't supported with aggregate queries?
🧠 Learnings used
Learnt from: EmpiDev
Repo: parse-community/parse-server PR: 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 }.
Learnt from: EmpiDev
Repo: parse-community/parse-server PR: 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.
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.917Z
Learning: When reviewing Parse Server PRs that add new features, always check whether the feature is documented in the README.md file, though for new Parse Server options this is optional rather than required.
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 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()`.
There was a problem hiding this comment.
@mtrezza from you knowledge is it normal to allow _id: 0 on projections ?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Moumouls
changed the title
|
So @mtrezza depending of DB co location with the server or even DB performance ( under load, slow queries), this PR introduce a important optimization in case of 10ms DB latency it's already a 23x time faster 🚀 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
spec/RestQuery.spec.js (2)
390-390: Consider a more descriptive test name.The term "battle test" is informal. A clearer name would better describe the test's purpose and expected behavior.
Consider renaming to something like:
- it('battle test parallel include with 100 nested includes', async () => { + it('should efficiently load 100 nested includes in parallel', async () => {
445-447: Remove debug timing statements.The
console.timeandconsole.timeEndcalls are debug statements that clutter test output. If performance validation is needed, capture the timing in a variable and assert against a threshold; otherwise, remove these statements.Apply this diff to remove the timing statements:
- console.time('query.find'); const results = await query.find(); - console.timeEnd('query.find');Alternatively, if you want to validate performance:
+ const startTime = Date.now(); const results = await query.find(); + const duration = Date.now() - startTime; + expect(duration).toBeLessThan(5000); // Adjust threshold as needed - console.time('query.find'); - console.timeEnd('query.find');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
spec/RestQuery.spec.js(1 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.917Z
Learning: When reviewing Parse Server PRs that add new features, always check whether the feature is documented in the README.md file, though for new Parse Server options this is optional rather than required.
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 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()`.
📚 Learning: 2025-04-30T19:31:35.344Z
Learnt from: RahulLanjewar93
Repo: parse-community/parse-server PR: 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/RestQuery.spec.js
📚 Learning: 2025-05-04T20:41:05.147Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 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/RestQuery.spec.js
📚 Learning: 2025-08-27T12:33:06.237Z
Learnt from: EmpiDev
Repo: parse-community/parse-server PR: 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/RestQuery.spec.js
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`. The preferred pattern is to create a Promise that resolves when an expected event occurs, then await that Promise.
Applied to files:
spec/RestQuery.spec.js
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 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/RestQuery.spec.js
📚 Learning: 2025-10-16T19:27:05.311Z
Learnt from: Moumouls
Repo: parse-community/parse-server PR: 9883
File: spec/CloudCodeLogger.spec.js:410-412
Timestamp: 2025-10-16T19:27:05.311Z
Learning: In spec/CloudCodeLogger.spec.js, the test "should log cloud function triggers using the silent log level" (around lines 383-420) is known to be flaky and requires the extra `await new Promise(resolve => setTimeout(resolve, 100))` timeout after awaiting `afterSavePromise` for reliability, even though it may appear redundant.
Applied to files:
spec/RestQuery.spec.js
📚 Learning: 2025-08-26T14:06:31.853Z
Learnt from: EmpiDev
Repo: parse-community/parse-server PR: 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/RestQuery.spec.js
🧬 Code graph analysis (1)
spec/RestQuery.spec.js (1)
spec/helper.js (2)
Parse(4-4)i(346-346)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: PostgreSQL 15, PostGIS 3.5
- GitHub Check: PostgreSQL 16, PostGIS 3.5
- GitHub Check: PostgreSQL 17, PostGIS 3.5
- GitHub Check: PostgreSQL 15, PostGIS 3.4
- GitHub Check: PostgreSQL 18, PostGIS 3.6
- GitHub Check: PostgreSQL 15, PostGIS 3.3
🔇 Additional comments (1)
spec/RestQuery.spec.js (1)
390-470: Excellent test coverage for parallel include functionality.This test comprehensively validates the parallel include feature by:
- Creating a realistic 3-level object hierarchy (1 root → 10 Level1 → 100 Level2)
- Testing 110 nested include paths simultaneously
- Verifying all nested objects are properly hydrated
- Using async/await patterns consistently (per Parse Server conventions)
The test directly addresses the PR objective to validate parallel include loading with deep nesting. The structure is clear and assertions are thorough.
Based on learnings
|
I've added a perf check in the CI. It runs only specific tests though, see docs. Do you want to add a benchmark to see the diff? Note that this is alpha, not sure whether it works reliably. I tested it with another PR and it showed no perf impact. However, it shows a perf regression issue for this PR here for creating objects. The other PR shows similar values for the baseline, so the baseline itself may be correct. It could be that the perf test algorithm has an issue (miscalculating an improvement for a regression). This PR only seems to change the |
It will not show much @mtrezza as you can see on my benchmark
|
3 participants
Pull Request
Issue
Related to : #6501
Closes: #7981
Approach
I try to avoid tricks and weird pattern as much as possible.
The approach is "Plan and execute", so we build a tree, and then we execute using parallel promises, and we hydrate the tree using some references
Each branch is recursively executed independently as fast as possible, and new parallelism is introduced at each child node.
Results
Benchmark procedure: Create a Parse.Object with 10 points on it, then each child have also 10 points, resulting in 100 includes
Before optimization:
After optimization
Tasks
Summary by CodeRabbit
Refactor
Performance