perf: Update to @apollo/server 5.0.0 #9888
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! ❌ Please fill out all fields with a placeholder |
✅ 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. |
📝 WalkthroughWalkthroughReplaces Apollo Express adapter with Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Express
participant ParseGraphQLServer
participant ApolloServer
participant IntrospectionPlugin
Client->>Express: POST /graphql (operation)
Express->>ParseGraphQLServer: forward request
ParseGraphQLServer->>ApolloServer: prepare/execute operation
ApolloServer->>IntrospectionPlugin: requestDidStart / onRequest
IntrospectionPlugin->>IntrospectionPlugin: quick string scan for "__schema"
alt contains "__schema"
IntrospectionPlugin-->>ApolloServer: throw introspection error (403)
else contains "__type"
IntrospectionPlugin->>ParseGraphQLServer: parse operation AST (hasTypeIntrospection)
alt AST indicates type introspection (including aliases/fragments)
IntrospectionPlugin-->>ApolloServer: throw introspection error (403)
else
IntrospectionPlugin-->>ApolloServer: allow execution
end
else
IntrospectionPlugin-->>ApolloServer: allow execution
end
ApolloServer-->>Client: result or error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
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 ignored due to path filters (1)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧠 Learnings (4)📚 Learning: 2025-09-21T15:43:32.265ZApplied to files:
📚 Learning: 2025-08-27T09:08:34.252ZApplied to files:
📚 Learning: 2025-04-30T19:31:35.344ZApplied to files:
📚 Learning: 2025-08-26T14:06:31.853ZApplied to files:
⏰ 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)
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. 🔧 ast-grep (0.39.7)spec/ParseGraphQLServer.spec.jsThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/GraphQL/ParseGraphQLServer.js (1)
16-52: Consider using Apollo Server v5's native introspection controls instead of string-based detection.Apollo Server v5 provides built-in introspection controls via the
introspectionboolean option, thenodeEnvoption to override NODE_ENV-derived defaults, andvalidationRulesincluding a disable-introspection validation rule that runs before execution. The verification found no existing false positive scenarios in the codebase (no user-defined fields or classes containing__schema), so the current string-based detection works correctly. However, leveraging Apollo's nativeintrospectionoption or validation rules would be more robust and maintainable than a custom plugin withincludes('__schema')detection.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
package.json(1 hunks)src/GraphQL/ParseGraphQLServer.js(2 hunks)
🔇 Additional comments (2)
src/GraphQL/ParseGraphQLServer.js (1)
4-4: LGTM: Correct import for Apollo Server v5.The migration from
@apollo/server/express4to@as-integrations/express5aligns with Apollo Server v5's architectural changes, where Express integration was moved to a separate package.package.json (1)
23-24: No breaking changes or security issues detected with Apollo Server 5.0.0 upgrade.Apollo Server 5.0.0 has no known security vulnerabilities, and all critical breaking changes are properly addressed:
- Node.js v20.0.0+ is required; package.json correctly constrains versions to compatible ranges (>=20.18.0, >=22.12.0)
- graphql >= v16.11.0 is required as a peer dependency; the codebase specifies 16.11.0
- Express middleware import has changed; this is addressed in the codebase migration
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## alpha #9888 +/- ##
==========================================
+ Coverage 93.06% 93.07% +0.01%
==========================================
Files 187 187
Lines 15187 15202 +15
Branches 177 177
==========================================
+ Hits 14134 14150 +16
+ Misses 1041 1040 -1
Partials 12 12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
mtrezza
changed the title
…/parse-server into moumouls/upgrade-appollo-server
|
@mtrezza i improved the system |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
spec/ParseGraphQLServer.spec.js (3)
733-771: Harden negative introspection tests with fragments and variables.Great coverage for direct and aliased __type; consider adding fragment- and variable-based cases to prevent bypass via AST shape changes.
Example additions:
+ it('should block __type introspection in fragment without master key', async () => { + try { + await apolloClient.query({ + query: gql` + query { + ...Frag + } + fragment Frag on Query { + __type(name: "User") { name } + } + `, + }); + fail('should have thrown an error'); + } catch (e) { + expect(e.message).toEqual('Response not successful: Received status code 403'); + expect(e.networkError.result.errors[0].message).toEqual('Introspection is not allowed'); + } + }); + + it('should block variable-based __type introspection without master key', async () => { + try { + await apolloClient.query({ + query: gql` + query TypeIntrospection($name: String!) { + __type(name: $name) { name } + } + `, + variables: { name: 'User' }, + }); + fail('should have thrown an error'); + } catch (e) { + expect(e.message).toEqual('Response not successful: Received status code 403'); + expect(e.networkError.result.errors[0].message).toEqual('Introspection is not allowed'); + } + });
773-792: Strengthen positive assertions for allowed __type results.When introspection is permitted, assert specific fields (e.g., name === 'User', kind === 'OBJECT') to catch regressions beyond mere success.
Example:
- expect(introspection.data).toBeDefined(); - expect(introspection.data.__type).toBeDefined(); + expect(introspection.data?.__type?.name).toBe('User'); + expect(introspection.data?.__type?.kind).toBe('OBJECT'); + expect(introspection.errors).toBeUndefined();Also applies to: 794-813, 815-834, 836-852
1678-1681: Avoid redundant cache reset before recreating the server.You reset caches then replace parseGraphQLServer with a new instance, which already starts clean. Drop the reset to shave test time.
- await parseGraphQLServer.setGraphQLConfig({}); - await resetGraphQLCache(); - await createGQLFromParseServer(parseServer, { graphQLPublicIntrospection: true }); + await parseGraphQLServer.setGraphQLConfig({}); + await createGQLFromParseServer(parseServer, { graphQLPublicIntrospection: true });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
spec/ParseGraphQLServer.spec.js(5 hunks)src/GraphQL/ParseGraphQLServer.js(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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: 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/ParseGraphQLServer.spec.js
🧬 Code graph analysis (1)
spec/ParseGraphQLServer.spec.js (1)
spec/helper.js (2)
parseServer(167-167)reconfigureServer(180-214)
⏰ 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: PostgreSQL 15, PostGIS 3.3
- GitHub Check: PostgreSQL 15, PostGIS 3.4
- GitHub Check: PostgreSQL 16, PostGIS 3.5
- GitHub Check: Node 18
- GitHub Check: PostgreSQL 17, PostGIS 3.5
- GitHub Check: PostgreSQL 18, PostGIS 3.6
- GitHub Check: PostgreSQL 15, PostGIS 3.5
- GitHub Check: Node 20
- GitHub Check: Redis Cache
- GitHub Check: MongoDB 8, ReplicaSet
- GitHub Check: MongoDB 6, ReplicaSet
- GitHub Check: MongoDB 7, ReplicaSet
- GitHub Check: Code Analysis (javascript)
- GitHub Check: Docker Build
🔇 Additional comments (8)
src/GraphQL/ParseGraphQLServer.js (4)
4-4: LGTM! Import changes align with Apollo Server v5 migration.The migration from
@apollo/server/express4to@as-integrations/express5and the addition ofparsefor AST-based introspection detection are necessary for the upgrade.Also applies to: 7-7
38-46: LGTM! Clean error handling extraction.The helper function provides consistent error messaging and proper HTTP 403 status for introspection denial.
64-78: LGTM! Well-optimized two-stage introspection check.The implementation balances security and performance effectively:
- Fast path for
__schema(simple string match)- Smart path for
__type(AST parsing only when string is present)- Comments clearly explain the rationale
This addresses the performance concerns and bypass risks discussed in previous comments. The AST-based approach correctly handles whitespace variations that would bypass simple string matching like
__type(.Note: Once the fragment bypass issue in
hasTypeIntrospectionis fixed, this logic will be fully secure.
138-138: LGTM! Introspection control architecture is sound.Based on the extensive discussion in previous review comments, setting
introspection: trueat the Apollo Server level is the correct approach for Apollo Server v5. TheIntrospectionControlPluginhandles the ternary access control logic (master key always allowed, public opt-in viagraphQLPublicIntrospection), which cannot be expressed through Apollo's native binaryintrospectionsetting.spec/ParseGraphQLServer.spec.js (4)
857-859: Enabling public introspection per-suite is appropriate.This localizes schema type checks that rely on __type without requiring master key. LGTM.
1019-1021: Relay suite setup looks good.Recreating the server with graphQLPublicIntrospection avoids auth headers in type lookups. LGTM.
1567-1568: Parse Class Types suite setup is consistent.Per-suite enablement keeps tests independent. LGTM.
7968-7969: Function mutations suite setup is sound.Recreating with public introspection avoids auth headers for enum/type checks. LGTM. Based on learnings.
mtrezza
added
the
state:breaking
Marked as breaking change; can be merged with Parse Server 9 which will drop Node 18 support. |
|
Note that there is apparently a bug regarding introspection, see #9900. Does this PR address that issue as well? |
…/parse-server into moumouls/upgrade-appollo-server
|
@mtrezza it's fixed here the introspection detection is more precise |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
spec/ParseGraphQLServer.spec.js (2)
733-852: Good coverage for __type introspection; consider adding aliased __schema too.The new tests comprehensively cover __type (plain and aliased) across public/master/maintenance/public-introspection. To fully mirror behavior, add a companion test that blocks aliased __schema without keys.
857-859: Reconfiguring server per-suite is fine; re-init client if subscriptions ever appear.You re-run createGQLFromParseServer with graphQLPublicIntrospection enabled while keeping the previously created apolloClient. That’s OK for HTTP-only tests; if a suite later uses wsLink/subscriptions after reconfigure, re-create the client to avoid stale sockets.
Also applies to: 1019-1021, 1566-1568, 1680-1681, 7967-7969
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
spec/ParseGraphQLServer.spec.js(7 hunks)src/GraphQL/ParseGraphQLServer.js(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/GraphQL/ParseGraphQLServer.js
🧰 Additional context used
🧠 Learnings (5)
📚 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/ParseGraphQLServer.spec.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/ParseGraphQLServer.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/ParseGraphQLServer.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/ParseGraphQLServer.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/ParseGraphQLServer.spec.js
🧬 Code graph analysis (1)
spec/ParseGraphQLServer.spec.js (3)
spec/helper.js (2)
parseServer(167-167)reconfigureServer(180-214)spec/ParseGraphQLSchema.spec.js (1)
parseServer(6-6)spec/ParseGraphQLController.spec.js (1)
parseServer(10-10)
🪛 GitHub Check: Lint
spec/ParseGraphQLServer.spec.js
[failure] 6830-6830:
Expected indentation of 16 spaces but found 14
[failure] 11445-11445:
Expected indentation of 16 spaces but found 14
[failure] 11444-11444:
Expected indentation of 14 spaces but found 12
[failure] 11443-11443:
Expected indentation of 14 spaces but found 12
[failure] 11442-11442:
Expected indentation of 14 spaces but found 12
[failure] 11441-11441:
Expected indentation of 16 spaces but found 14
[failure] 11440-11440:
Expected indentation of 16 spaces but found 14
[failure] 11439-11439:
Expected indentation of 14 spaces but found 12
[failure] 11438-11438:
Expected indentation of 12 spaces but found 10
[failure] 11437-11437:
Expected indentation of 12 spaces but found 10
⏰ 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). (8)
- GitHub Check: PostgreSQL 17, PostGIS 3.5
- GitHub Check: MongoDB 8, ReplicaSet
- GitHub Check: MongoDB 6, ReplicaSet
- GitHub Check: PostgreSQL 18, PostGIS 3.6
- GitHub Check: PostgreSQL 16, PostGIS 3.5
- GitHub Check: PostgreSQL 15, PostGIS 3.4
- GitHub Check: PostgreSQL 15, PostGIS 3.3
- GitHub Check: Docker Build
🔇 Additional comments (1)
spec/ParseGraphQLServer.spec.js (1)
11437-11455: Disregard this review comment; no issues found after verification.The code at lines 11437–11455 in
spec/ParseGraphQLServer.spec.jsis already correctly indented using consistent 2-space spacing throughout. The diff shown contains no actual changes—both the removed and added sides are identical. Additionally, no ESLint configuration exists in the project to have "flagged" any indentation issues.The
SomeClassTypedefinition with the name'SomeClass'is intentional test setup code within abeforeEachblock for testing schema merging behavior. The GraphQL schema system already includes collision detection (ParseGraphQLSchema.js, lines 212–217 and 240–245) that safely handles type name conflicts, so there is no regression risk from this pattern in the test context.Likely an incorrect or invalid review comment.
|
oops @mtrezza missed the last feedback of coderabbit |
|
Waiting coderabbit check, but we should be good to go here @mtrezza ! |
3 participants
Pull Request
Issue
Closes: FILL_THIS_OUT
Approach
Update to V5, something changed on how introspection is managed internally, or maybe we got a bug before, introspection is always activated for master key, and public introspection is stil an option to opt in
Tasks
Summary by CodeRabbit
Chores
Features
Tests