fix: Apply requestContextMiddleware on directAccess requests - #10603
fix: Apply requestContextMiddleware on directAccess requests#10603Moumouls wants to merge 8 commits into
Conversation
Nested SDK ops under directAccess bypass Express, so DI injected via requestContextMiddleware was missing on Cloud hooks. Re-apply the middleware after Config.get() in ParseServerRESTController.
|
🚀 Thanks for opening this pull request! We appreciate your effort in improving the project. Please let us know once your pull request is ready for review. Tip
Note Please respond to review comments from AI agents just like you would to comments from a human reviewer. Let the reviewer resolve their own comments, unless they have reviewed and accepted your commit, or agreed with your explanation for why the feedback was incorrect. Caution Pull requests must be written using an AI agent with human supervision. Pull requests written entirely by a human will likely be rejected, because of lower code quality, higher review effort and the higher risk of introducing bugs. Please note that AI review comments on this pull request alone do not satisfy this requirement. Our CI and AI review are safeguards, not development tools. If many issues are flagged, rethink your development approach. Invest more effort in planning and design rather than using review cycles to fix low-quality code. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthrough
ChangesDirect-access request context
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Severity of issue fixed: Medium Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant ParentSave
participant ParseServerRESTController
participant requestContextMiddleware
participant ChildBeforeSave
ParentSave->>ParseServerRESTController: route nested Child.save
ParseServerRESTController->>requestContextMiddleware: apply request context
requestContextMiddleware-->>ParseServerRESTController: set req.config.aCustomController
ParseServerRESTController->>ChildBeforeSave: execute trigger with request config
ChildBeforeSave-->>ParentSave: observe aCustomController
Merge Risk: ⚪ Minimal · up to The direct-access context propagation change is ready to merge; no concrete current-head risk remains. Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (4 passed)
Full details: Out of Scope Changes checkExplanation The PR includes behavior changes that are not required for issue Resolution Remove the unrelated context-error and batch transaction behavior changes, or move them to a separate pull request. Keep only the async changes required to apply Full details: Engage In Review FeedbackExplanation The PR contains one review thread that the author resolved without reviewer retraction or an implementing commit. Reviewer dblythy proposed replacing the adapter with Resolution Reopen the dblythy review thread. Explain the incompatibility with
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Warning Some tools did not complete. Review the errors below. 🔧 Biome (2.5.10)src/Options/index.jsFile contains syntax errors that prevent linting: Line 18: Expected a type but instead found '?'.; Line 18: Expected a property, or a signature but instead found ';'.; Line 21: Expected a statement but instead found '?'.; Line 24: Expected a statement but instead found '?'.; Line 27: Expected a statement but instead found '?'.; Line 30: Expected a statement but instead found '?'.; Line 32: Expected a statement but instead found '?'.; Line 34: Expected a statement but instead found '?'.; Line 35: Expected a statement but instead found '}'.; Line 37: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 38: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 39: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 40: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Li ... [truncated 16757 characters] ... found '?'.; Line 912: Expected a statement but instead found '?'.; Line 914: Expected a statement but instead found '?'.; Line 915: Expected a statement but instead found '}'.; Line 929: Expected a type but instead found '?'.; Line 929: Expected a property, or a signature but instead found ';'.; Line 930: Expected a statement but instead found '}'.; Line 936: Expected a type but instead found '?'.; Line 936: Expected a property, or a signature but instead found ';'.; Line 940: Expected a statement but instead found '?'.; Line 944: Expected a statement but instead found '?'.; Line 948: Expected a statement but instead found '?'.; Line 952: Expected a statement but instead found '?'.; Line 956: Expected a statement but instead found '?'.; Line 957: Expected a statement but instead found '}'. Comment |
This comment has been minimized.
This comment has been minimized.
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/ParseServerRESTController.js`:
- Around line 32-63: Update applyRequestContextMiddleware and its documentation
to define a minimal synthetic-request contract for requestContextMiddleware,
explicitly stating that Express-only request properties and methods such as
req.get(), req.header(), req.ip, and req.body are unavailable. Ensure the
middleware is expected to use only the supported fields on the synthetic req,
rather than adding partial Express compatibility.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 65797509-2cd9-4814-aaf3-ad36e5001eb4
📒 Files selected for processing (4)
spec/rest.spec.jssrc/Options/Definitions.jssrc/Options/docs.jssrc/ParseServerRESTController.js
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## alpha #10603 +/- ##
==========================================
- Coverage 93.82% 93.80% -0.02%
==========================================
Files 192 192
Lines 16863 16877 +14
Branches 252 252
==========================================
+ Hits 15821 15831 +10
- Misses 1020 1024 +4
Partials 22 22 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Clarify that directAccess uses a minimal synthetic request and that Express-only accessors are unavailable, addressing review feedback.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Options/index.js`:
- Around line 421-422: Update the requestContextMiddleware type declaration in
the Options definitions to allow both synchronous and Promise-based completion
by using a void-or-Promise<void> return type, while preserving the existing
request, response, and next parameter types.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 53f020c2-d9f8-4ca2-bc23-29f358fdebcb
📒 Files selected for processing (4)
src/Options/Definitions.jssrc/Options/docs.jssrc/Options/index.jssrc/ParseServerRESTController.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/ParseServerRESTController.js
Align with handleRequest style while keeping callback and Promise middleware settlement support.
Align the Flow type with runtime support for async DI middleware.
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/ParseServerRESTController.js`:
- Around line 55-80: Update the requestContextMiddleware contract in
applyRequestContextMiddleware to explicitly require settlement via next() or a
resolving/rejecting Promise. Document that the synthetic req.config.req.headers
are only DI support and res is a bare object, so middleware must not rely on
response termination or other implicit paths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c10f6058-2a17-4445-b362-cc31ea0c2ef6
📒 Files selected for processing (5)
spec/rest.spec.jssrc/Options/Definitions.jssrc/Options/docs.jssrc/Options/index.jssrc/ParseServerRESTController.js
Document that directAccess synthetic res is bare and middleware must settle via next() or a Promise to avoid hanging nested ops.
|
@mtrezza resolved all comments |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
3 participants
Pull Request
Issue
Closes: #10602
Related: #8480 (requestContextMiddleware), orthogonal to #10291 (context clone).
Approach
With
directAccess: true, nested SDK ops useParseServerRESTControllerandConfig.get()without going through Express, sorequestContextMiddlewarenever ran and DI onreq.configwas lost.After loading a fresh config via
Config.get(), re-applyrequestContextMiddlewareon a synthetic{ config, headers: {} }request (callback + Promise). Skip when a parent config is already passed (batch children).Tasks
Test plan
rest context→ nested directAccess DI (was red before fix)rest context→ existing HTTP DI still passesParseServerRESTControllersuite (22 specs)Summary by CodeRabbit