feat: Add request context middleware for config and dependency injection in hooks #8480
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! |
Moumouls
changed the title
|
I will reformat the title to use the proper commit message syntax. |
parse-github-assistant
bot
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## alpha #8480 +/- ##
==========================================
- Coverage 93.04% 93.02% -0.02%
==========================================
Files 187 187
Lines 15147 15158 +11
Branches 174 176 +2
==========================================
+ Hits 14093 14101 +8
- Misses 1042 1045 +3
Partials 12 12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@Moumouls Any suggestion what we should do with this PR? Does this require more development? If so; I'd change this to draft, otherwise maybe we can get this merged? |
|
Hi @mtrezza we don't need more development here. This PR is to allow developers to mutate the config using a provided middleware. It's super useful, when you need to inject globally on each request (after the Parse middleware) some providers, for monitoring, feature flags, based on the current user/ parse config. Then the injected prop will be available in each hooks, GQL API |
|
And no breaking change, right? |
# Conflicts: # spec/CloudCode.spec.js
✅ 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. |
📝 WalkthroughWalkthroughAdds per-request dependency-injection middleware and propagates a request-scoped Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Express as Express App
participant PS as ParseServer
participant Router as FunctionsRouter
participant Cloud as Cloud Code Trigger
participant Dev as Developer Hook
rect rgba(200,235,255,0.12)
note over Express,PS: requestContextMiddleware applied (if configured)
Client->>Express: REST request (e.g., save)
Express->>PS: req/res
PS->>PS: applyRequestContextMiddleware(req,res,next)
note right of PS: middleware may set/modify req.config
end
PS->>Router: Route handling
Router->>Cloud: Invoke trigger/function with req (includes config)
Cloud->>Dev: beforeSave/afterSave/... invoked with request.config
Dev-->>Cloud: Continue/response
Cloud-->>Client: Result
sequenceDiagram
autonumber
actor Client
participant Express as Express App
participant GQL as ParseGraphQLServer
participant Apollo as GraphQL Engine
participant Cloud as Cloud Code Trigger
participant Dev as Developer Hook
rect rgba(200,235,255,0.12)
note over Express,GQL: requestContextMiddleware applied at graphQLPath (if configured)
Client->>Express: GraphQL operation
Express->>GQL: req/res
GQL->>GQL: applyRequestContextMiddleware(req,res,next)
note right of GQL: middleware may set/modify req.config
end
GQL->>Apollo: Execute operation
Apollo->>Cloud: Resolve (hooks/resolvers) with req (includes config)
Cloud->>Dev: Hook receives request.config
Dev-->>Cloud: Continue/response
Cloud-->>Client: Operation result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
spec/CloudCode.spec.js(10 hunks)spec/requestContextMiddleware.spec.js(1 hunks)src/Controllers/HooksController.js(1 hunks)src/GraphQL/ParseGraphQLServer.js(1 hunks)src/Options/Definitions.js(1 hunks)src/Options/docs.js(1 hunks)src/Options/index.js(1 hunks)src/ParseServer.ts(2 hunks)src/Routers/FunctionsRouter.js(2 hunks)src/cloud-code/Parse.Cloud.js(5 hunks)src/triggers.js(3 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/CloudCode.spec.js
🧬 Code graph analysis (4)
src/Options/index.js (1)
resources/buildConfigDefinitions.js (1)
res(374-379)
spec/requestContextMiddleware.spec.js (1)
spec/helper.js (1)
reconfigureServer(171-205)
spec/CloudCode.spec.js (5)
spec/helper.js (2)
Parse(4-4)reconfigureServer(171-205)src/Auth.js (3)
Parse(1-1)req(538-538)req(551-551)src/RestWrite.js (1)
query(679-692)src/RestQuery.js (2)
query(324-333)query(334-334)spec/FilesController.spec.js (1)
mockAdapter(10-20)
src/GraphQL/ParseGraphQLServer.js (1)
src/middlewares.js (3)
handleParseHeaders(72-316)handleParseSession(347-383)handleParseSession(347-383)
🪛 Biome (2.1.2)
src/Options/index.js
[error] 346-346: 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). (1)
- GitHub Check: codecov/project
🔇 Additional comments (11)
spec/requestContextMiddleware.spec.js (2)
1-21: LGTM!The dynamic import pattern for node-fetch (line 2) correctly handles ESM-only modules, and the REST API test properly verifies that the middleware-injected config is accessible in hooks.
22-55: LGTM!The GraphQL test properly verifies that the middleware works with GraphQL mutations and that the injected config is accessible in hooks.
src/cloud-code/Parse.Cloud.js (1)
673-673: LGTM!The documentation additions consistently expose the
configproperty across all relevant Cloud Code request interfaces (TriggerRequest, FileTriggerRequest, BeforeFindRequest, AfterFindRequest, FunctionRequest, JobRequest), enabling developers to access server configuration during hook execution.Also applies to: 688-688, 726-726, 740-740, 749-749, 756-756
spec/CloudCode.spec.js (8)
2018-2024: LGTM!The test properly verifies that
req.configis available in cloud functions and follows the recommended async/await pattern.
2048-2056: LGTM!The test correctly verifies that
req.configis accessible in beforeSave hooks.
2113-2121: LGTM!The test correctly verifies that
req.configis accessible in afterSave hooks.
2181-2190: LGTM!The test correctly verifies that
req.configis accessible in beforeDelete hooks.
2537-2548: LGTM!The test correctly verifies that
req.configis accessible in beforeFind hooks across different query operations (get, first, find).
3433-3433: LGTM!The assertion correctly verifies that
req.configis present in beforeLogin hooks, consistent with the other request properties being checked in this test.
3551-3551: LGTM!The assertion correctly verifies that
req.configis present in afterLogin hooks, consistent with the other request properties being checked in this test.
3754-3761: LGTM!The test correctly verifies that
req.configis accessible in Parse.File beforeSave hooks.
|
btw @mtrezza
No breaking change here, just simple new feature |
|
I still have a flaky, weird test issue |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
spec/requestContextMiddlewareRest.spec.js (1)
10-14: Add cleanup for the registered hook to prevent test interference.The
Parse.Cloud.beforeSavehook is registered globally without cleanup. This is the same issue as in the GraphQL test file and can cause test interference.Add an
afterEachorafterAllblock to clean up:+ afterEach(() => { + Parse.Cloud._removeHook('beforeSave', '_User'); + }); + it('should support dependency injection on rest api', async () => {Note: Verify the correct Parse Cloud Code cleanup API for your version.
🧹 Nitpick comments (1)
spec/requestContextMiddlewareRest.spec.js (1)
15-20: Consider adding explicit error handling for clarity.While
user.signUp()will throw an error on failure (which the test framework will catch), adding explicit error handling or response validation can improve test clarity and debugging.Consider this pattern:
const user = new Parse.User(); user.setUsername('test'); user.setPassword('test'); - await user.signUp(); + const signedUpUser = await user.signUp(); + expect(signedUpUser.id).toBeDefined(); expect(called).toBeTruthy();This makes the test's expectations more explicit and easier to debug if something goes wrong.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
spec/requestContextMiddlewareGraphQL.spec.js(1 hunks)spec/requestContextMiddlewareRest.spec.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
spec/requestContextMiddlewareRest.spec.js (1)
spec/helper.js (1)
reconfigureServer(171-205)
spec/requestContextMiddlewareGraphQL.spec.js (2)
spec/helper.js (1)
reconfigureServer(171-205)spec/ParseGraphQLServer.spec.js (1)
fetch(4-4)
⏰ 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). (13)
- GitHub Check: Node 18
- GitHub Check: Node 20
- GitHub Check: MongoDB 8, ReplicaSet
- GitHub Check: PostgreSQL 17, PostGIS 3.5
- GitHub Check: PostgreSQL 18, PostGIS 3.6
- GitHub Check: MongoDB 6, ReplicaSet
- GitHub Check: MongoDB 7, ReplicaSet
- GitHub Check: Redis Cache
- GitHub Check: PostgreSQL 15, PostGIS 3.5
- GitHub Check: PostgreSQL 15, PostGIS 3.3
- GitHub Check: PostgreSQL 16, PostGIS 3.5
- GitHub Check: PostgreSQL 15, PostGIS 3.4
- GitHub Check: Docker Build
🔇 Additional comments (2)
spec/requestContextMiddlewareGraphQL.spec.js (1)
1-7: LGTM!The test suite setup and middleware definition correctly implement the per-request config mutation pattern described in the PR objectives.
spec/requestContextMiddlewareRest.spec.js (1)
1-7: LGTM!The test suite setup and middleware definition correctly implement the per-request config mutation pattern for REST API validation.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
spec/requestContextMiddleware.spec.js (1)
20-39: Critical: Missingfetchimport will cause runtime error.The
fetchfunction is used but not imported, causing aReferenceErrorat runtime.Add the import at the top of the file:
+const fetch = (...args) => import('node-fetch').then(({ default: fetch }) => fetch(...args)); + describe('requestContextMiddleware', () => {Additionally, verify the GraphQL mutation succeeded before asserting the hook was called.
The test doesn't validate the GraphQL response. If the mutation fails, the hook won't be called, leading to unclear test failures.
Add response validation:
- await fetch('http://localhost:8378/graphql', { + const response = await fetch('http://localhost:8378/graphql', { method: 'POST', headers: { 'Content-Type': 'application/json', 'X-Parse-Application-Id': 'test', 'X-Parse-Master-Key': 'test', }, body: JSON.stringify({ query: ` mutation { createUser(input: { fields: { username: "test", password: "test" } }) { user { objectId } } } `, }), }); + const result = await response.json(); + expect(result.data.createUser.user.objectId).toBeDefined(); expect(called).toBeTruthy();
🧹 Nitpick comments (1)
spec/requestContextMiddleware.spec.js (1)
48-48: Minor: Initializecalledfor consistency.The variable is declared but not initialized, unlike the first test where it's set to
false. While this works fine in JavaScript, initializing tofalseimproves consistency.- let called; + let called = false;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
spec/requestContextMiddleware.spec.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
spec/requestContextMiddleware.spec.js (2)
spec/helper.js (1)
reconfigureServer(171-205)spec/ParseGraphQLServer.spec.js (1)
fetch(4-4)
⏰ 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 17, PostGIS 3.5
- GitHub Check: PostgreSQL 15, PostGIS 3.5
- GitHub Check: PostgreSQL 16, PostGIS 3.5
- GitHub Check: PostgreSQL 18, PostGIS 3.6
- GitHub Check: PostgreSQL 15, PostGIS 3.4
- GitHub Check: Node 20
- GitHub Check: PostgreSQL 15, PostGIS 3.3
- GitHub Check: MongoDB 7, ReplicaSet
- GitHub Check: Node 18
- GitHub Check: Redis Cache
- GitHub Check: MongoDB 6, ReplicaSet
- GitHub Check: MongoDB 8, ReplicaSet
- GitHub Check: Docker Build
- GitHub Check: Code Analysis (javascript)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
spec/requestContextMiddleware.spec.js (3)
20-38: Validate the GraphQL response to clarify test failures.The test doesn't check whether the GraphQL mutation succeeded. If the mutation fails, the hook won't be called, and the assertion on line 39 will fail with a misleading message about
calledbeing 1 instead of 2, rather than indicating that the GraphQL call itself failed.Parse and assert the response before checking the hook invocation count:
- await fetch('http://localhost:8378/graphql', { + const response = await fetch('http://localhost:8378/graphql', { method: 'POST', headers: { 'Content-Type': 'application/json', 'X-Parse-Application-Id': 'test', 'X-Parse-Master-Key': 'test', }, body: JSON.stringify({ query: ` mutation { createUser(input: { fields: { username: "test2", password: "test2" } }) { user { objectId } } } `, }), }); + const result = await response.json(); + expect(result.data?.createUser?.user?.objectId).toBeDefined(); expect(called).toBe(2);
1-41: Missingfetchimport will cause runtime error.Line 20 uses
fetchbut the function is not imported at the top of the file. This will cause aReferenceErrorwhen the test executes.Based on the pattern in
spec/ParseGraphQLServer.spec.js, add the import at the top of the file:+const fetch = (...args) => import('node-fetch').then(({ default: fetch }) => fetch(...args)); + describe('requestContextMiddleware', () => {
11-14: Clean up the registered hook to prevent test pollution.The
Parse.Cloud.beforeSavehook registered on line 11 is never removed after the test completes. This can pollute subsequent tests that create_Userobjects, causing them to fail unexpectedly or exhibit non-deterministic behavior.Add cleanup using a try/finally block or an afterEach:
Parse.Cloud.beforeSave('_User', request => { expect(request.config.aCustomController).toEqual('aCustomController'); called++; }); + try { - const user = new Parse.User(); + const user = new Parse.User(); user.setUsername('test'); user.setPassword('test'); await user.signUp(); await fetch('http://localhost:8378/graphql', { method: 'POST', headers: { 'Content-Type': 'application/json', 'X-Parse-Application-Id': 'test', 'X-Parse-Master-Key': 'test', }, body: JSON.stringify({ query: ` mutation { createUser(input: { fields: { username: "test2", password: "test2" } }) { user { objectId } } } `, }), }); expect(called).toBe(2); + } finally { + Parse.Cloud._removeHook('beforeSave', '_User'); + }
🧹 Nitpick comments (1)
spec/requestContextMiddleware.spec.js (1)
3-40: Consider splitting REST and GraphQL tests for clearer intent.The test validates dependency injection for both REST and GraphQL APIs in a single test case. While this works, splitting them into separate test cases would improve clarity and make failures easier to diagnose (e.g., if only one API path breaks).
Example structure:
it('should inject config via requestContextMiddleware for REST API', async () => { const requestContextMiddleware = (req, res, next) => { req.config.aCustomController = 'aCustomController'; next(); }; let called = false; await reconfigureServer({ requestContextMiddleware }); try { Parse.Cloud.beforeSave('_User', request => { expect(request.config.aCustomController).toEqual('aCustomController'); called = true; }); const user = new Parse.User(); user.setUsername('test'); user.setPassword('test'); await user.signUp(); expect(called).toBe(true); } finally { Parse.Cloud._removeHook('beforeSave', '_User'); } }); it('should inject config via requestContextMiddleware for GraphQL API', async () => { // Similar structure for GraphQL });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
spec/requestContextMiddleware.spec.js(1 hunks)src/GraphQL/ParseGraphQLServer.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/GraphQL/ParseGraphQLServer.js
🧰 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: 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/requestContextMiddleware.spec.js
🧬 Code graph analysis (1)
spec/requestContextMiddleware.spec.js (2)
spec/helper.js (1)
reconfigureServer(171-205)spec/ParseGraphQLServer.spec.js (1)
fetch(4-4)
⏰ 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). (13)
- GitHub Check: MongoDB 7, ReplicaSet
- GitHub Check: MongoDB 6, ReplicaSet
- GitHub Check: Node 18
- GitHub Check: Redis Cache
- GitHub Check: Node 20
- GitHub Check: MongoDB 8, ReplicaSet
- GitHub Check: PostgreSQL 15, PostGIS 3.5
- GitHub Check: PostgreSQL 17, PostGIS 3.5
- GitHub Check: PostgreSQL 16, PostGIS 3.5
- GitHub Check: PostgreSQL 18, PostGIS 3.6
- GitHub Check: PostgreSQL 15, PostGIS 3.4
- GitHub Check: Docker Build
- GitHub Check: PostgreSQL 15, PostGIS 3.3
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
spec/ParseGraphQLServer.spec.js(1 hunks)spec/rest.spec.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
spec/rest.spec.js (1)
spec/helper.js (2)
reconfigureServer(171-205)Parse(4-4)
spec/ParseGraphQLServer.spec.js (1)
spec/helper.js (3)
parseServer(158-158)reconfigureServer(171-205)Parse(4-4)
⏰ 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: MongoDB 8, ReplicaSet
- GitHub Check: Node 18
- GitHub Check: Node 20
- GitHub Check: MongoDB 6, ReplicaSet
- GitHub Check: MongoDB 7, ReplicaSet
- GitHub Check: Redis Cache
- GitHub Check: PostgreSQL 15, PostGIS 3.5
- GitHub Check: PostgreSQL 18, PostGIS 3.6
- GitHub Check: PostgreSQL 17, PostGIS 3.5
- GitHub Check: PostgreSQL 16, PostGIS 3.5
- GitHub Check: PostgreSQL 15, PostGIS 3.3
- GitHub Check: PostgreSQL 15, PostGIS 3.4
- GitHub Check: Docker Build
- GitHub Check: Code Analysis (javascript)
…ls/parse-server into moumouls/requestContextMiddleWare
Signed-off-by: Manuel <[email protected]>
|
Waiting for CI |
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)
spec/ParseGraphQLServer.spec.js(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
PR: parse-community/parse-server#9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: Tests in the parse-server repository should use promise-based approaches rather than callback patterns with `done()`. Use a pattern where a Promise is created that resolves when the event occurs, then await that promise.
Applied to files:
spec/ParseGraphQLServer.spec.js
📚 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
📚 Learning: 2025-05-04T20:41:05.147Z
Learnt from: mtrezza
PR: parse-community/parse-server#9445
File: spec/ParseLiveQuery.spec.js:1312-1338
Timestamp: 2025-05-04T20:41:05.147Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`.
Applied to files:
spec/ParseGraphQLServer.spec.js
🧬 Code graph analysis (1)
spec/ParseGraphQLServer.spec.js (1)
spec/helper.js (3)
parseServer(158-158)reconfigureServer(171-205)Parse(4-4)
⏰ 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). (13)
- GitHub Check: MongoDB 6, ReplicaSet
- GitHub Check: Redis Cache
- GitHub Check: Node 20
- GitHub Check: Node 18
- GitHub Check: MongoDB 8, ReplicaSet
- GitHub Check: MongoDB 7, ReplicaSet
- GitHub Check: PostgreSQL 15, PostGIS 3.3
- GitHub Check: PostgreSQL 17, PostGIS 3.5
- GitHub Check: PostgreSQL 15, PostGIS 3.5
- GitHub Check: PostgreSQL 16, PostGIS 3.5
- GitHub Check: PostgreSQL 18, PostGIS 3.6
- GitHub Check: PostgreSQL 15, PostGIS 3.4
- GitHub Check: Docker Build
|
@coderabbitai propose angular commit PR title |
|
Tip For best results, initiate chat on the files or code changes. @mtrezza Based on the changes in this PR, here are my Angular commit title suggestions: Recommended: Alternatives: The recommended title clearly communicates:
The current title "feat: RequestContextMiddleware and config in hooks" is also valid, but the recommended version better describes the purpose of the feature rather than just its components. |
mtrezza
changed the title
# [8.3.0-alpha.6](8.3.0-alpha.5...8.3.0-alpha.6) (2025-10-14) ### Features * Add request context middleware for config and dependency injection in hooks ([#8480](#8480)) ([64f104e](64f104e))
|
🎉 This change has been released in version 8.3.0-alpha.6 |
|
Yayyyyyyyy thanks @mtrezza ! |
3 participants
Pull Request
Issue
Closes: #8479 8479
Approach
Add a middleware option and add config to hooks
Tasks
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests