fix: replace original request body after middleware execution (#77662)#7
fix: replace original request body after middleware execution (#77662)#7MitchLewis930 wants to merge 1 commit intopr_037_beforefrom
Conversation
…#77662) In vercel#77553 we fixed reading the request body in middleware using the `nodejs` runtime. However, this caused issues with subsequent reads like in server actions. In sandbox, [we run](https://github.com/vercel/next.js/blob/1e62ce2c61048ddc0297f1a4f268894541975521/packages/next/src/server/web/sandbox/sandbox.ts#L146-L148) `.finalize()` after middleware is executed so we should do the same here. Fixes vercel#77646 --------- Co-authored-by: JJ Kasper <jj@jjsweb.site>
📝 WalkthroughWalkthroughChanges introduce node-based middleware support for tests by adding request body handling to the server middleware path, creating conditional test drivers for node middleware variants, and modifying middleware configuration to support nodejs runtime execution. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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: 2
🤖 Fix all issues with AI agents
In
`@test/e2e/app-dir/actions/app-action-size-limit-invalid-node-middleware.test.ts`:
- Around line 1-3: This file sets process.env.TEST_NODE_MIDDLEWARE at module
scope which can leak into other tests; capture the current value before
overwriting (e.g., const prev = process.env.TEST_NODE_MIDDLEWARE), set
process.env.TEST_NODE_MIDDLEWARE = 'true' as it currently does, and register an
afterAll cleanup that restores the original value
(process.env.TEST_NODE_MIDDLEWARE = prev or delete it if prev is undefined) so
the environment is not mutated for other tests; reference the module-level
assignment of process.env.TEST_NODE_MIDDLEWARE and add an afterAll hook to
restore the previous value.
In `@test/e2e/app-dir/actions/middleware-node.js`:
- Around line 13-18: Update the JSDoc for the exported config so the annotated
type matches the included runtime property: either remove the JSDoc annotation
entirely from the exported symbol config to let inference handle the shape, or
replace import('next/server').MiddlewareConfig with a more specific inline type
that includes runtime: 'nodejs' (and optional matcher if needed) so the runtime
property is allowed; ensure the modification targets the exported const named
config and the JSDoc comment referencing MiddlewareConfig.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
packages/next/src/server/next-server.tstest/e2e/app-dir/actions/app-action-form-state-node-middleware.test.tstest/e2e/app-dir/actions/app-action-form-state.test.tstest/e2e/app-dir/actions/app-action-node-middleware.test.tstest/e2e/app-dir/actions/app-action-size-limit-invalid-node-middleware.test.tstest/e2e/app-dir/actions/app-action-size-limit-invalid.test.tstest/e2e/app-dir/actions/app-action.test.tstest/e2e/app-dir/actions/middleware-node.jstest/e2e/app-dir/actions/middleware.jstest/e2e/app-dir/actions/next.config.js
💤 Files with no reviewable changes (1)
- test/e2e/app-dir/actions/middleware.js
🧰 Additional context used
🧬 Code graph analysis (4)
test/e2e/app-dir/actions/app-action-form-state.test.ts (2)
test/lib/e2e-utils/index.ts (1)
FileRef(122-128)turbopack/crates/turbo-tasks-fs/src/lib.rs (1)
join(1233-1244)
test/e2e/app-dir/actions/app-action-size-limit-invalid.test.ts (2)
test/lib/e2e-utils/index.ts (1)
FileRef(122-128)turbopack/crates/turbo-tasks-fs/src/lib.rs (1)
join(1233-1244)
test/e2e/app-dir/actions/app-action.test.ts (2)
test/lib/e2e-utils/index.ts (1)
FileRef(122-128)turbopack/crates/turbo-tasks-fs/src/lib.rs (1)
join(1233-1244)
packages/next/src/server/next-server.ts (2)
test/e2e/middleware-general/app/middleware-node.js (2)
params(18-30)result(20-20)test/e2e/middleware-general/app/middleware.js (2)
params(27-39)result(29-29)
🔇 Additional comments (9)
test/e2e/app-dir/actions/next.config.js (1)
7-9: LGTM!The experimental
nodeMiddlewareflag is correctly enabled to support the node-based middleware testing introduced in this PR.test/e2e/app-dir/actions/middleware-node.js (1)
4-11: LGTM!The middleware implementation is correct:
- Uses async function as required
- Properly rewrites requests matching the path pattern
- Falls through with
NextResponse.next()for other requeststest/e2e/app-dir/actions/app-action.test.ts (1)
2-2: LGTM!The conditional middleware override is well-implemented:
FileRefis correctly used to reference the node middleware file- The
overrideFilespattern cleanly enables test reuse for both edge and node middleware scenarios- Path construction using
join(__dirname, 'middleware-node.js')is correctAlso applies to: 21-27
test/e2e/app-dir/actions/app-action-node-middleware.test.ts (1)
1-3: LGTM!Clean test driver pattern that enables running the full action test suite under the node middleware configuration by setting the environment variable before requiring the base test.
test/e2e/app-dir/actions/app-action-form-state.test.ts (1)
2-4: LGTM!The middleware override pattern is consistently applied, matching the implementation in
app-action.test.ts.Also applies to: 7-13
test/e2e/app-dir/actions/app-action-form-state-node-middleware.test.ts (1)
1-3: LGTM!Consistent test driver pattern for the form state tests under node middleware configuration.
test/e2e/app-dir/actions/app-action-size-limit-invalid.test.ts (2)
1-1: LGTM!The middleware override setup is consistent with the other test files in this PR.
Also applies to: 6-6, 12-18
49-57: Good catch: patched configs includenodeMiddleware: true.The patched
next.config.jscontents correctly includenodeMiddleware: truealongside the invalidserverActions.bodySizeLimitvalues. This ensures the node middleware feature remains enabled when the config is temporarily overwritten during these test cases.Also applies to: 64-76, 83-95
packages/next/src/server/next-server.ts (1)
1689-1709: Good safeguard for node middleware body handling.Cloning the body for the adapter and finalizing in
finallyshould reliably restore the original stream for downstream reads, even on errors.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| process.env.TEST_NODE_MIDDLEWARE = 'true' | ||
|
|
||
| require('./app-action-size-limit-invalid.test') |
There was a problem hiding this comment.
Restore TEST_NODE_MIDDLEWARE after the suite to avoid env leakage.
Since this sets a global env var at module scope, it can bleed into subsequent test files in the same worker. Consider saving the previous value and restoring it in afterAll.
🔧 Suggested patch
-process.env.TEST_NODE_MIDDLEWARE = 'true'
-
-require('./app-action-size-limit-invalid.test')
+const previousValue = process.env.TEST_NODE_MIDDLEWARE
+process.env.TEST_NODE_MIDDLEWARE = 'true'
+
+require('./app-action-size-limit-invalid.test')
+
+afterAll(() => {
+ if (previousValue === undefined) {
+ delete process.env.TEST_NODE_MIDDLEWARE
+ } else {
+ process.env.TEST_NODE_MIDDLEWARE = previousValue
+ }
+})🤖 Prompt for AI Agents
In
`@test/e2e/app-dir/actions/app-action-size-limit-invalid-node-middleware.test.ts`
around lines 1 - 3, This file sets process.env.TEST_NODE_MIDDLEWARE at module
scope which can leak into other tests; capture the current value before
overwriting (e.g., const prev = process.env.TEST_NODE_MIDDLEWARE), set
process.env.TEST_NODE_MIDDLEWARE = 'true' as it currently does, and register an
afterAll cleanup that restores the original value
(process.env.TEST_NODE_MIDDLEWARE = prev or delete it if prev is undefined) so
the environment is not mutated for other tests; reference the module-level
assignment of process.env.TEST_NODE_MIDDLEWARE and add an afterAll hook to
restore the previous value.
| /** | ||
| * @type {import('next/server').MiddlewareConfig} | ||
| */ | ||
| export const config = { | ||
| runtime: 'nodejs', | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's understand the repository structure and find Next.js type definitions
git ls-files | grep -E "(middleware|server)" | head -20Repository: Signal65/next.js-CodeRabbit
Length of output: 1103
🏁 Script executed:
# Search for MiddlewareConfig type definition in the codebase
rg -n "type MiddlewareConfig|interface MiddlewareConfig" --type=ts -A 10Repository: Signal65/next.js-CodeRabbit
Length of output: 4557
🏁 Script executed:
# Also search for middleware config exports to understand the type structure
rg -n "export.*MiddlewareConfig" --type=tsRepository: Signal65/next.js-CodeRabbit
Length of output: 1073
🏁 Script executed:
# Check if runtime property is used in middleware configs anywhere
rg -n "runtime.*nodejs" --type=ts -C 3Repository: Signal65/next.js-CodeRabbit
Length of output: 25948
🏁 Script executed:
# Let's check the MiddlewareConfigInputSchema to see all supported properties
sed -n '95,150p' packages/next/src/build/segment-config/middleware/middleware-config.tsRepository: Signal65/next.js-CodeRabbit
Length of output: 1417
🏁 Script executed:
# Let's check the actual file under review to see what it contains
cat -n test/e2e/app-dir/actions/middleware-node.jsRepository: Signal65/next.js-CodeRabbit
Length of output: 648
🏁 Script executed:
# Also check if there's a separate type definition for runtime in middleware context
rg -n "runtime.*nodejs" packages/next/src/build/segment-config/middleware/ -A 2 -B 2Repository: Signal65/next.js-CodeRabbit
Length of output: 53
🏁 Script executed:
# Search for where runtime is parsed/validated in middleware config
rg -n "runtime" packages/next/src/build/segment-config/middleware/middleware-config.ts -A 2 -B 2Repository: Signal65/next.js-CodeRabbit
Length of output: 53
🏁 Script executed:
# Check the types exported from next/server to understand what types are available
cat packages/next/src/server/web/types.tsRepository: Signal65/next.js-CodeRabbit
Length of output: 1931
🏁 Script executed:
# Let's look at what types are available in next/server.d.ts
cat packages/next/server.d.ts | head -30Repository: Signal65/next.js-CodeRabbit
Length of output: 1286
🏁 Script executed:
# Search for all type definitions that include runtime in middleware context
rg -n "type.*Middleware.*Config|interface.*Middleware.*Config" packages/next/src --type=ts -A 5Repository: Signal65/next.js-CodeRabbit
Length of output: 8896
Update JSDoc type annotation to include the runtime property or use a more specific type.
The MiddlewareConfig type exported from next/server does not include the runtime property, which is set to 'nodejs' in this config. Consider either:
- Removing the JSDoc type annotation and relying on type inference
- Using a more specific type that includes all properties:
{ runtime: 'nodejs'; matcher?: ... } - Or filing a feature request to add
runtimeto the officialMiddlewareConfigtype definition
Relevant code snippet
/**
* `@type` {import('next/server').MiddlewareConfig}
*/
export const config = {
runtime: 'nodejs',
}🤖 Prompt for AI Agents
In `@test/e2e/app-dir/actions/middleware-node.js` around lines 13 - 18, Update the
JSDoc for the exported config so the annotated type matches the included runtime
property: either remove the JSDoc annotation entirely from the exported symbol
config to let inference handle the shape, or replace
import('next/server').MiddlewareConfig with a more specific inline type that
includes runtime: 'nodejs' (and optional matcher if needed) so the runtime
property is allowed; ensure the modification targets the exported const named
config and the JSDoc comment referencing MiddlewareConfig.
2 participants
PR_037
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.