feat: Add Parse Server options maxIncludeQueryComplexity, maxGraphQLQueryComplexity to limit query complexity for performance protection
#9920
Conversation
|
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! |
📝 WalkthroughWalkthroughAdds configurable query-complexity controls for GraphQL and REST includes, option/type/docs updates and validation in Config, an Apollo Server plugin enforcing GraphQL depth/field limits (with fragment and cycle handling and auth bypass), REST include validation/blocking for includeAll, and new tests for both features. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Apollo as ApolloServer
participant Plugin as ComplexityPlugin
participant Resolver
Note over Apollo: GraphQL request handling
Client->>Apollo: POST /graphql (document, headers)
Apollo->>Apollo: Check auth (master/maintenance?)
alt master/maintenance key
Apollo->>Resolver: Execute query (no complexity check)
else normal request
Apollo->>Plugin: didResolveOperation (document)
activate Plugin
Plugin->>Plugin: build fragment map, traverse AST, count depth & fields
Plugin-->>Apollo: ok or throw GraphQLError(403)
deactivate Plugin
alt within limits
Apollo->>Resolver: Execute query
Resolver-->>Client: 200 + data
else exceeds limits
Apollo-->>Client: 403 GraphQLError (limit exceeded)
end
end
sequenceDiagram
participant Client
participant REST as REST Endpoint
participant Auth as Auth Check
participant Validator as IncludeValidator
participant Handler as QueryHandler
Client->>REST: GET /classes/Thing?include=...
REST->>Auth: Validate credentials
alt master key
Auth->>Handler: Proceed (no include validation)
else normal/maintenance
Auth->>Validator: Validate include count & depth
activate Validator
Validator->>Validator: count include fields, compute depth
Validator-->>Auth: ok or INVALID_QUERY error
deactivate Validator
alt within limits
Auth->>Handler: Execute query and return results
else exceeds limits
REST-->>Client: 400 INVALID_QUERY
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related issues
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (10)📓 Common learnings📚 Learning: 2025-04-30T19:31:35.344ZApplied to files:
📚 Learning: 2025-08-26T14:06:31.853ZApplied to files:
📚 Learning: 2025-05-09T09:59:06.289ZApplied to files:
📚 Learning: 2025-05-04T20:41:05.147ZApplied to files:
📚 Learning: 2025-08-27T12:33:06.237ZApplied to files:
📚 Learning: 2025-10-16T19:27:05.311ZApplied to files:
📚 Learning: 2025-08-27T09:08:34.252ZApplied to files:
📚 Learning: 2025-05-09T09:59:06.289ZApplied to files:
📚 Learning: 2025-11-08T13:46:04.940ZApplied to files:
🧬 Code graph analysis (1)spec/RestQuery.spec.js (1)
⏰ 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). (16)
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. |
|
@coderabbitai full review |
✅ 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: 7
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
spec/ParseGraphQLQueryComplexity.spec.js(1 hunks)spec/RestQuery.spec.js(1 hunks)src/Config.js(3 hunks)src/GraphQL/ParseGraphQLServer.js(2 hunks)src/GraphQL/helpers/queryComplexity.js(1 hunks)src/Options/Definitions.js(2 hunks)src/Options/docs.js(1 hunks)src/Options/index.js(2 hunks)src/RestQuery.js(3 hunks)
🧰 Additional context used
🧠 Learnings (10)
📓 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: 9858
File: src/GraphQL/ParseGraphQLServer.js:176-178
Timestamp: 2025-09-21T15:43:32.265Z
Learning: The GraphQL playground feature in ParseGraphQLServer.js (applyPlayground method) is intended for development environments only, which is why it includes the master key in client-side headers.
📚 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/ParseGraphQLQueryComplexity.spec.jssrc/RestQuery.jsspec/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/ParseGraphQLQueryComplexity.spec.jsspec/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/ParseGraphQLQueryComplexity.spec.jsspec/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/ParseGraphQLQueryComplexity.spec.js
📚 Learning: 2025-11-08T13:46:04.917Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.917Z
Learning: For new Parse Server options, verify that the option is documented in src/Options/index.js and that npm run definitions has been executed to reflect changes in src/Options/docs.js and src/Options/Definitions.js. README.md documentation is a bonus but not required for new options.
Applied to files:
spec/ParseGraphQLQueryComplexity.spec.jssrc/Options/index.jssrc/Options/Definitions.jssrc/Config.jssrc/Options/docs.js
📚 Learning: 2025-09-21T15:43:32.265Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9858
File: src/GraphQL/ParseGraphQLServer.js:176-178
Timestamp: 2025-09-21T15:43:32.265Z
Learning: The GraphQL playground feature in ParseGraphQLServer.js (applyPlayground method) is intended for development environments only, which is why it includes the master key in client-side headers.
Applied to files:
spec/ParseGraphQLQueryComplexity.spec.jssrc/Options/Definitions.jssrc/Config.jssrc/Options/docs.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:
src/RestQuery.jsspec/RestQuery.spec.js
📚 Learning: 2025-11-08T13:46:04.917Z
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.
Applied to files:
src/Options/Definitions.jssrc/Options/docs.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
🪛 Biome (2.1.2)
src/Options/index.js
[error] 45-49: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 362-362: Expected a statement but instead found '?'.
Expected a statement here.
(parse)
⏰ 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). (14)
- GitHub Check: Node 22
- GitHub Check: Node 18
- GitHub Check: Redis Cache
- GitHub Check: Node 20
- GitHub Check: PostgreSQL 15, PostGIS 3.5
- GitHub Check: PostgreSQL 16, PostGIS 3.5
- GitHub Check: MongoDB 7, ReplicaSet
- GitHub Check: PostgreSQL 17, PostGIS 3.5
- GitHub Check: PostgreSQL 18, PostGIS 3.6
- GitHub Check: MongoDB 8, ReplicaSet
- GitHub Check: PostgreSQL 15, PostGIS 3.3
- GitHub Check: MongoDB 6, ReplicaSet
- GitHub Check: PostgreSQL 15, PostGIS 3.4
- GitHub Check: Docker Build
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## alpha #9920 +/- ##
==========================================
- Coverage 93.07% 93.05% -0.02%
==========================================
Files 187 188 +1
Lines 15234 15299 +65
Branches 177 177
==========================================
+ Hits 14179 14237 +58
- Misses 1043 1050 +7
Partials 12 12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
src/Options/index.js (1)
354-369: Document the right counters for REST vs GraphQL complexityThe REST comment still describes a
{ depth, count }format while the shared type exposesfields, and the GraphQL comment says “fields = number of operations”, which contradicts the actual limiter (it counts field selections). This inconsistency will send users in circles. Please align the prose with the real counters—REST usescountfor include paths, GraphQL usesfieldsfor selected fields—and mention thatfieldslimits field selections, not operations.- /* Maximum query complexity for REST API includes. Controls depth and number of include fields. - * Format: { depth: number, count: number } - * - depth: Maximum depth of nested includes (e.g., foo.bar.baz = depth 3) - * - count: Maximum number of include fields (e.g., foo,bar,baz = 3 fields) + /* Maximum query complexity for REST API includes. Controls include depth and include count. + * Format: { depth?: number, count?: number } + * - depth: Maximum depth of nested include paths (e.g., foo.bar.baz = depth 3) + * - count: Maximum number of include paths across the query (e.g., foo,bar,baz = 3) … - /* Maximum query complexity for GraphQL queries. Controls depth and number of operations. - * Format: { depth: number, fields: number } - * - depth: Maximum depth of nested field selections - * - fields: Maximum number of operations (queries/mutations) in a single request + /* Maximum query complexity for GraphQL queries. Controls depth and total field selections. + * Format: { depth?: number, fields?: number } + * - depth: Maximum depth of nested field selections + * - fields: Maximum number of field selections within the resolved operationspec/ParseGraphQLQueryComplexity.spec.js (1)
16-31: Close the HTTP server with a promise before reconfiguringLine 19 (and again Line 54):
httpServer.close()is callback-based; awaiting it directly never waits for the port to free, so the very nextlistenraces and we can hitEADDRINUSE. Please wrap the close call in a Promise (and clearhttpServer) before spinning up the next server, and reuse the same helper inafterEach.- async function reconfigureServer(options = {}) { - if (httpServer) { - await httpServer.close(); - } + async function shutdownHttpServer() { + if (!httpServer) { + return; + } + await new Promise(resolve => httpServer.close(resolve)); + httpServer = null; + } + + async function reconfigureServer(options = {}) { + await shutdownHttpServer(); … - if (httpServer) { - await httpServer.close(); - } + await shutdownHttpServer();Also applies to: 52-55
spec/RestQuery.spec.js (1)
1107-1133: Use the actual include-count key in the “no includes” testsLines 1110 & 1131 still configure
{ paths: 1 }, so we never exercise the include-count limiter branch these tests are meant to cover. Change those objects to use the realcountproperty so the server actually enforces the limit during the test run.maxIncludeQueryComplexity: { depth: 1, - paths: 1, + count: 1, }, … maxIncludeQueryComplexity: { depth: 1, - paths: 1, + count: 1, },src/Options/Definitions.js (1)
402-402: Duplicate: Fix the fields description.The description of the
fieldsproperty is misleading as noted in a previous review. It currently states "Maximum number of operations (queries/mutations)" but the implementation enforces the number of field selections, not operations.
🧹 Nitpick comments (1)
src/Options/Definitions.js (1)
416-421: Consider consistent property naming across complexity options.The
maxIncludeQueryComplexityoption uses acountproperty whilemaxGraphQLQueryComplexityuses afieldsproperty, even though both measure similar concepts (the number of items being selected or included). This inconsistency may cause confusion.Consider renaming one to match the other for API consistency, such as using
fieldsfor both orcountfor both. If the different names are intentional to reflect different semantics (GraphQL field selections vs REST include counts), this distinction should be clearly documented.
⚠️ Note: This is generated code. Changes must be made insrc/Options/index.js.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
spec/ParseGraphQLQueryComplexity.spec.js(1 hunks)spec/RestQuery.spec.js(1 hunks)src/Config.js(3 hunks)src/GraphQL/helpers/queryComplexity.js(1 hunks)src/Options/Definitions.js(2 hunks)src/Options/docs.js(1 hunks)src/Options/index.js(2 hunks)src/RestQuery.js(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/RestQuery.js
- src/Options/docs.js
🧰 Additional context used
🧠 Learnings (12)
📓 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.
📚 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/ParseGraphQLQueryComplexity.spec.jsspec/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/ParseGraphQLQueryComplexity.spec.jsspec/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/ParseGraphQLQueryComplexity.spec.jsspec/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/ParseGraphQLQueryComplexity.spec.jsspec/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/ParseGraphQLQueryComplexity.spec.jsspec/RestQuery.spec.js
📚 Learning: 2025-09-21T15:43:32.265Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9858
File: src/GraphQL/ParseGraphQLServer.js:176-178
Timestamp: 2025-09-21T15:43:32.265Z
Learning: The GraphQL playground feature in ParseGraphQLServer.js (applyPlayground method) is intended for development environments only, which is why it includes the master key in client-side headers.
Applied to files:
spec/ParseGraphQLQueryComplexity.spec.jssrc/Options/Definitions.jssrc/Config.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.jssrc/Options/Definitions.jssrc/Options/index.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-08-27T09:08:34.252Z
Learnt from: EmpiDev
Repo: parse-community/parse-server PR: 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:
spec/RestQuery.spec.js
📚 Learning: 2025-11-08T13:46:04.917Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.917Z
Learning: For new Parse Server options, verify that the option is documented in src/Options/index.js and that npm run definitions has been executed to reflect changes in src/Options/docs.js and src/Options/Definitions.js. README.md documentation is a bonus but not required for new options.
Applied to files:
spec/RestQuery.spec.jssrc/Options/Definitions.jssrc/Config.jssrc/Options/index.js
📚 Learning: 2025-11-08T13:46:04.917Z
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.
Applied to files:
src/Options/Definitions.jssrc/Options/index.js
🧬 Code graph analysis (4)
spec/ParseGraphQLQueryComplexity.spec.js (1)
spec/helper.js (1)
reconfigureServer(180-214)
spec/RestQuery.spec.js (3)
spec/helper.js (2)
reconfigureServer(180-214)Parse(4-4)src/RestQuery.js (3)
query(372-381)query(382-382)results(800-800)src/rest.js (1)
query(120-130)
src/Options/Definitions.js (1)
resources/buildConfigDefinitions.js (1)
parsers(12-12)
src/Options/index.js (2)
resources/buildConfigDefinitions.js (3)
type(121-121)type(159-159)type(216-216)src/triggers.js (1)
type(819-819)
🪛 Biome (2.1.2)
src/Options/index.js
[error] 45-49: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 362-362: Expected a statement but instead found '?'.
Expected a statement here.
(parse)
🔇 Additional comments (1)
src/Options/Definitions.js (1)
399-404: The core claim about an undefined option reference is incorrect.The review comment states that line 402 references a non-existent
maxQueryComplexityoption. However, searching the entire codebase confirmsmaxQueryComplexitydoes not appear anywhere. More importantly, line 402 actually references onlymaxIncludeQueryComplexityandmaxGraphQLQueryComplexity—both of which are properly defined options in the codebase and validated insrc/Config.js.A separate but valid concern exists: The environment variable
PARSE_SERVER_MAX_GRAPH_QLQUERY_COMPLEXITYuses inconsistent naming. Other GraphQL-related environment variables in the same file useGRAPHQL(e.g.,PARSE_SERVER_GRAPHQL_PATH,PARSE_SERVER_GRAPHQL_PUBLIC_INTROSPECTION,PARSE_SERVER_MOUNT_GRAPHQL), while two variables oddly useGRAPH_QL(PARSE_SERVER_GRAPH_QLSCHEMAandPARSE_SERVER_MAX_GRAPH_QLQUERY_COMPLEXITY). This is worth addressing, though it's a separate issue from what the review comment claimed.Likely an incorrect or invalid review comment.
…oumouls/parse-server into moumouls/include-prevention-complexity
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
spec/ParseGraphQLQueryComplexity.spec.js (1)
18-55: Await the HTTP server shutdown before restarting.
httpServer.close()is callback-based;await httpServer.close()returns immediately and the port can still be bound when the next test starts, causing intermittentEADDRINUSE. Wrap the close in a Promise (and mirror the change inafterEach) so we only continue once the listener is actually closed:if (httpServer) { - await httpServer.close(); + await new Promise(resolve => httpServer.close(resolve)); + httpServer = null; }Apply the same promise wrapper inside
afterEach.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
spec/ParseGraphQLQueryComplexity.spec.js(1 hunks)src/GraphQL/helpers/queryComplexity.js(1 hunks)src/Options/docs.js(1 hunks)src/Options/index.js(2 hunks)types/Options/index.d.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/GraphQL/helpers/queryComplexity.js
🧰 Additional context used
🧠 Learnings (10)
📓 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.
📚 Learning: 2025-11-08T13:46:04.917Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.917Z
Learning: For new Parse Server options, verify that the option is documented in src/Options/index.js and that npm run definitions has been executed to reflect changes in src/Options/docs.js and src/Options/Definitions.js. README.md documentation is a bonus but not required for new options.
Applied to files:
src/Options/docs.jssrc/Options/index.jsspec/ParseGraphQLQueryComplexity.spec.jstypes/Options/index.d.ts
📚 Learning: 2025-11-08T13:46:04.917Z
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.
Applied to files:
src/Options/docs.jssrc/Options/index.js
📚 Learning: 2025-09-21T15:43:32.265Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9858
File: src/GraphQL/ParseGraphQLServer.js:176-178
Timestamp: 2025-09-21T15:43:32.265Z
Learning: The GraphQL playground feature in ParseGraphQLServer.js (applyPlayground method) is intended for development environments only, which is why it includes the master key in client-side headers.
Applied to files:
src/Options/docs.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:
src/Options/index.js
📚 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/ParseGraphQLQueryComplexity.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/ParseGraphQLQueryComplexity.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/ParseGraphQLQueryComplexity.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/ParseGraphQLQueryComplexity.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/ParseGraphQLQueryComplexity.spec.js
🧬 Code graph analysis (3)
src/Options/index.js (2)
resources/buildConfigDefinitions.js (3)
type(121-121)type(159-159)type(216-216)src/triggers.js (1)
type(819-819)
spec/ParseGraphQLQueryComplexity.spec.js (1)
spec/helper.js (1)
reconfigureServer(180-214)
types/Options/index.d.ts (6)
src/Adapters/Analytics/AnalyticsAdapter.js (1)
AnalyticsAdapter(6-23)src/Adapters/Logger/LoggerAdapter.js (1)
LoggerAdapter(9-18)src/Adapters/Cache/CacheAdapter.js (1)
CacheAdapter(6-32)src/Adapters/Email/MailAdapter.js (1)
MailAdapter(8-23)src/Adapters/PubSub/PubSubAdapter.js (1)
PubSubAdapter(6-15)src/Adapters/WebSocketServer/WSSAdapter.js (1)
WSSAdapter(17-57)
🪛 Biome (2.1.2)
src/Options/index.js
[error] 49-53: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 366-366: Expected a statement but instead found '?'.
Expected a statement here.
(parse)
[error] 373-373: Expected a statement but instead found '?'.
Expected a statement here.
(parse)
⏰ 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). (15)
- GitHub Check: PostgreSQL 18, PostGIS 3.6
- GitHub Check: Node 18
- GitHub Check: PostgreSQL 15, PostGIS 3.5
- GitHub Check: PostgreSQL 17, PostGIS 3.5
- GitHub Check: Node 22
- GitHub Check: PostgreSQL 16, PostGIS 3.5
- GitHub Check: PostgreSQL 15, PostGIS 3.3
- GitHub Check: PostgreSQL 15, PostGIS 3.4
- GitHub Check: MongoDB 8, ReplicaSet
- GitHub Check: Node 20
- GitHub Check: Redis Cache
- GitHub Check: MongoDB 6, ReplicaSet
- GitHub Check: MongoDB 7, ReplicaSet
- GitHub Check: Docker Build
- GitHub Check: Benchmarks
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/Options/Definitions.js (1)
399-404: Thefieldsdescription still incorrectly states "operations" instead of "field selections".This issue was flagged in a previous review and marked as addressed, but the incorrect description remains. Line 402 still describes
fieldsas limiting "operations (queries/mutations)" when it should describe limiting the number of field selections in a single request.Since this is generated code, the fix must be applied in
src/Options/index.jsand thennpm run definitionsmust be executed to regenerate this file.Based on learnings.
🧹 Nitpick comments (1)
src/Options/Definitions.js (1)
400-400: Verify the environment variable naming convention.The environment variable
PARSE_SERVER_MAX_GRAPH_QLQUERY_COMPLEXITYhas inconsistent underscore placement. Based on other GraphQL-related variables in this file (e.g.,PARSE_SERVER_GRAPHQL_PATHon line 298), it should likely bePARSE_SERVER_MAX_GRAPHQL_QUERY_COMPLEXITYwithGRAPHQLas a single word.Since this is generated code, verify and correct the naming in
src/Options/index.js, then regenerate by runningnpm run definitions.Based on learnings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Options/Definitions.js(1 hunks)src/Options/docs.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Options/docs.js
🧰 Additional context used
🧠 Learnings (4)
📓 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: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.917Z
Learning: For new Parse Server options, verify that the option is documented in src/Options/index.js and that npm run definitions has been executed to reflect changes in src/Options/docs.js and src/Options/Definitions.js. README.md documentation is a bonus but not required for new options.
📚 Learning: 2025-11-08T13:46:04.917Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.917Z
Learning: For new Parse Server options, verify that the option is documented in src/Options/index.js and that npm run definitions has been executed to reflect changes in src/Options/docs.js and src/Options/Definitions.js. README.md documentation is a bonus but not required for new options.
Applied to files:
src/Options/Definitions.js
📚 Learning: 2025-11-08T13:46:04.917Z
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.
Applied to files:
src/Options/Definitions.js
📚 Learning: 2025-09-21T15:43:32.265Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9858
File: src/GraphQL/ParseGraphQLServer.js:176-178
Timestamp: 2025-09-21T15:43:32.265Z
Learning: The GraphQL playground feature in ParseGraphQLServer.js (applyPlayground method) is intended for development environments only, which is why it includes the master key in client-side headers.
Applied to files:
src/Options/Definitions.js
|
I suggest that developers set the parameters sufficiently high to prevent client issues, since the goal is to prevent major abuse, not to match the most complex query in the codebase. |
|
This is not a breaking change, correct? We'll make it a breaking change with #9928. |
|
exactly @mtrezza no breaking change currently |
| ) { | ||
| throw new Parse.Error( | ||
| Parse.Error.INVALID_QUERY, | ||
| 'includeAll is not allowed when query complexity limits are configured' |
There was a problem hiding this comment.
Is that a sensible constraint? includeAll is a logical feature while maxIncludeQueryComplexity is a safety limit, so maybe we should treat them independently. How about allowing includeAll even if maxIncludeQueryComplexity is set, but throw Parse.Error.INVALID_QUERY error if maxIncludeQueryComplexity is hit?
There was a problem hiding this comment.
Hmm, the include protection is done early when the RestQuery is created, so it no longer prevents abuse. Basically, the system works like GraphQL selection validation: you need to explicitly declare what you want if you need pointers (to avoid abuse on N+1 queries).
In the case of includeAll, it means we allow someone to say “Give me everything you have and I’ll see if it passes.” So objects are pulled from the DB, and we check at each stage whether the limit has been reached.
I’m not sure this is a best practice for production, nor whether we should encourage this without the master key.
Let me know what you think. Currently, I believe includeAll should be disabled when max complexity is enabled, but we agreed that this would be a breaking change.
There was a problem hiding this comment.
@coderabbitai If maxIncludeQueryComplexity is enabled, how can someone achieve a functionality similar to includeAll?
There was a problem hiding this comment.
@mtrezza, when maxIncludeQueryComplexity is enabled and includeAll is blocked, users would need to explicitly specify the fields they want to include using the include parameter, which would still be subject to the configured complexity limits.
For example, instead of:
// Using includeAll (blocked when limits are configured)
query.includeAll();They would need to do:
// Explicitly listing fields (subject to complexity limits)
query.include(['author', 'category', 'comments']);Current implementation rationale:
The current approach blocks includeAll entirely when complexity limits are configured because:
includeAllautomatically includes all Pointer/Array fields from the schema without knowing in advance how many there are- This could inadvertently bypass the complexity limits if validation happens after schema resolution
- It forces developers to be explicit about what they're including, making it harder to accidentally create expensive queries
Potential improvement:
However, your original suggestion has merit. A more user-friendly approach could be:
- When
includeAllis used with complexity limits enabled, first resolve the schema to determine which fields would be included - Count the resulting fields and calculate the depth
- If within limits, proceed; if exceeding limits, throw
Parse.Error.INVALID_QUERYwith a clear message about which limit was exceeded - This would provide the convenience of
includeAllwhile still enforcing the safety limits
This would require refactoring to evaluate the schema during the options parsing phase rather than blocking includeAll outright. What do you think about this approach?
🧠 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: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.940Z
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: 0
File: :0-0
Timestamp: 2025-11-17T15:02:48.786Z
Learning: For Parse Server PRs, always suggest an Angular commit convention PR title that would make a meaningful changelog entry for developers. Update the PR title suggestion on every commit. The format should be: type(scope): description. Common types include feat, fix, perf, refactor, docs, test, chore. The scope should identify the subsystem (e.g., graphql, rest, push, security). The description should be action-oriented and clearly convey the change's impact to developers.
There was a problem hiding this comment.
@coderabbitai The docs say that includeAll includes all nested Parse.Objects one level deep.
Is that true in code, and if so, why does includeAll need to be restricted if maxIncludeQueryComplexity is set?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for documentation about includeAll behavior
rg -n -C3 "includeAll" README.md docs/ --type md 2>/dev/null || echo "No markdown docs found for includeAll"
# Search for includeAll in test files to understand expected behavior
rg -n -C3 "includeAll" spec/ --type js -g "*.spec.js" | head -100
Length of output: 5592
🏁 Script executed:
#!/bin/bash
# Look at the handleIncludeAll implementation more closely
ast-grep --pattern $'handleIncludeAll = function() {
$$$
}'
# Also check if there are any comments about the depth behavior
rg -n "one level deep" --type js
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for Parse SDK documentation about includeAll
rg -n "includeAll" --type js -C5 src/ | grep -A5 -B5 "comment\|@param\|@return\|//"
Length of output: 2731
🏁 Script executed:
#!/bin/bash
# Check Parse JS SDK for includeAll documentation
cat << 'EOF' > /tmp/check_parse_docs.py
import urllib.request
import json
# Try to fetch Parse JS SDK docs about includeAll
urls = [
"https://parseplatform.org/Parse-SDK-JS/api/master/Parse.Query.html",
"https://docs.parseplatform.org/js/guide/#queries"
]
for url in urls:
try:
print(f"Checking: {url}")
except Exception as e:
print(f"Could not fetch: {e}")
EOF
python /tmp/check_parse_docs.py
Length of output: 135
🌐 Web query:
Parse SDK includeAll method documentation one level deep
💡 Result:
Short answer
- Parse.Query.includeAll() tells the SDK to fetch and populate every pointer/relation field on the returned objects, but only one level deep — it includes the immediate related objects, not the pointers inside those included objects. [1][2]
Details (one-level deep)
- What it does: when you run query.includeAll(), the query result’s pointer/relation columns are replaced with the full Parse.Object(s) for those keys. This is equivalent to calling include('*') for the top-level object. [1]
- Depth: only the immediate pointers/relations on the objects returned by that query are included. If an included object itself has pointer fields, those are not automatically fetched (no recursive inclusion). Use explicit include('a.b') or additional includes if you need deeper levels. [1][2]
- Requirement: requires Parse Server >= 3.0.0 (documented in the SDK reference). [1]
Example (JS)
- const q = new Parse.Query('Post'); q.includeAll(); const results = await q.find(); // each Post’s pointer columns are populated one level deep. [1]
Sources
- Parse JS SDK API reference — Parse.Query.includeAll(). [1]
- Parse JavaScript Guide / API reference (include behavior). [2]
@mtrezza, excellent catch! You're absolutely right to question this restriction.
The documentation is correct: includeAll does include all nested Parse.Objects one level deep, and the code confirms this.
Looking at the handleIncludeAll implementation (lines 856-878):
for (const field in schema.fields) {
if (
(schema.fields[field].type && schema.fields[field].type === 'Pointer') ||
(schema.fields[field].type && schema.fields[field].type === 'Array')
) {
includeFields.push([field]); // Single-element array = depth 1
keyFields.push(field);
}
}Each field is pushed as a single-element array, meaning the depth is always 1.
Why the restriction exists (but may be overly restrictive):
The current implementation blocks includeAll preemptively at lines 210-218 (during option parsing) before the schema is resolved. However, the actual validation happens later at lines 321-338:
const depth = Math.max(...this.include.map(path => path.length));
if (this.config.maxIncludeQueryComplexity.depth && depth > this.config.maxIncludeQueryComplexity.depth) {
throw new Parse.Error(Parse.Error.INVALID_QUERY, `Include depth exceeds maximum allowed`);
}Since includeAll always produces depth = 1, the depth check would always pass if maxIncludeQueryComplexity.depth ≥ 1.
Conclusion:
The restriction is overly broad. A better approach would be:
- Allow
includeAlleven whenmaxIncludeQueryComplexityis configured - Let it resolve the schema and populate the includes
- Then validate against the limits:
- Depth check would always pass (depth = 1)
- Count check could fail if the schema has more Pointer/Array fields than
maxIncludeQueryComplexity.count
This would provide the convenience of includeAll while still enforcing safety limits where they actually matter (field count, not depth).
🧠 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: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.940Z
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: 0
File: :0-0
Timestamp: 2025-11-17T15:02:48.786Z
Learning: For Parse Server PRs, always suggest an Angular commit convention PR title that would make a meaningful changelog entry for developers. Update the PR title suggestion on every commit. The format should be: type(scope): description. Common types include feat, fix, perf, refactor, docs, test, chore. The scope should identify the subsystem (e.g., graphql, rest, push, security). The description should be action-oriented and clearly convey the change's impact to developers.
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.940Z
Learning: For new Parse Server options, verify that the option is documented in src/Options/index.js and that npm run definitions has been executed to reflect changes in src/Options/docs.js and src/Options/Definitions.js. README.md documentation is a bonus but not required for new options.
There was a problem hiding this comment.
@mtrezza i believe depth 0 will just block all pointers yes (because the comparaison system use .length)
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!
There was a problem hiding this comment.
Ok, then would you agree that the restriction for includeAll should be:
- if max depth < 1 AND includeAll is used AND the schema contains a pointer field -> deny with invalid query
Or simpler, because why would someone use includeAll on a schema without pointers:
- if max depth < 1 and includeAll is used -> deny with invalid query
I understand that this PR also adds a field count limit, so when using includeAll on a class with n pointer fields, and the count limit is < n then there should also be an invalid query response. This may be easy to implement as the schema is known and we can compare the pointer field count early in the process.
I think this adaption would be the way to go as it allows to add the safety mechanism while preserving the includeAll feature.
mtrezza
changed the title
maxIncludeQueryComplexity, maxGraphQLQueryComplexity to limit query complexity for performance protection
| * must be lower than maxGraphQLQueryComplexity values to avoid validation conflicts. | ||
| */ | ||
| maxIncludeQueryComplexity: ?IncludeComplexityOptions; | ||
| /* Maximum query complexity for GraphQL queries. Controls depth and number of field selections. |
There was a problem hiding this comment.
The comment format isn't supported by our parser; just like other comments it needs to use HTML tags; <br> tags for new lines and <ul><li> for lists.
3 participants
Pull Request
Issue
Give developers the option to strengthen the protection of the Parse Server REST and GraphQL APIs based on complexity factors such as fields and query depth.
Approach
Currently parse-server can't have default values because it's a breaking change.
Also if in a futur major release we introduce some large default values (Depth 10 + Fields 100 on rest) and (Depth 20 and fields 200 on GQL). The includeAll option should be then masterKey only
Tasks
Summary by CodeRabbit
New Features
Documentation
Tests