fix(api): make ctx.json() with no arguments read the request body (depends on #2999) - #3010
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 55aca343ea
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| json: { | ||
| (): Promise<unknown>; | ||
| (data: unknown, init?: ResponseInit): Response; |
There was a problem hiding this comment.
Implement ctx.json() in isolated pages routes
With WORKER_ISOLATION_ENABLED=1 and WORKER_ISOLATION_API=1, executePagesRoute() takes the isolated branch (src/routing/api/route-executor.ts:450), where src/security/sandbox/worker-script.ts:457 still builds ctx.json as only a response helper. A pages API handler using the newly supported await ctx.json() in that environment still receives a Response object instead of the parsed body, so POST payloads continue to be dropped under worker isolation; mirror this arity check in the worker context or share the same context builder there.
Useful? React with 👍 / 👎.
6df7b93 to
b6d4ac1
Compare
55aca34 to
c5ea111
Compare
kwakayama
left a comment
There was a problem hiding this comment.
Score: 84/100
The tests show the intended arity split works: ctx.json() parses the request body, while ctx.json(data, init?) still builds a JSON response. This fixes a real footgun for handlers using the zero-arg form.
Not merge-ready yet:
- This is stacked on lower open PRs, including blocked/type-failing PRs.
- The PR status rollup only shows CLA, not full CI.
Architectural watch item: ctx.json now means request parsing or response creation depending only on arity. That may be pragmatic given existing usage, but it is a fragile public boundary and should be documented as an intentional overload.
b6d4ac1 to
4566899
Compare
c5ea111 to
d223eeb
Compare
4566899 to
7d862f1
Compare
d223eeb to
10b0811
Compare
10b0811 to
6490cad
Compare
7d862f1 to
0d6621f
Compare
kwakayama
left a comment
There was a problem hiding this comment.
Follow-up after fixes: approving. The public API docs now explain \ request parsing vs \ response creation, and examples were updated. I also scrubbed real-looking example names to synthetic test data. Score: 91/100. Next step: merge after base stack and checks are green.
|
Clean follow-up after the approval above: Score: 91/100. Fix added: public docs now explain ctx.json() request parsing vs ctx.json(data, init?) response creation. I also scrubbed real-looking example names to synthetic test data. Next step: merge after the base stack and refreshed checks are green. |
6490cad to
84b9305
Compare
0d6621f to
1c906fa
Compare
a829396 to
f4f212a
Compare
4a473a3 to
ee402cc
Compare
f4f212a to
4bc6a04
Compare
ee402cc to
edcb779
Compare
4bc6a04 to
7ea42f9
Compare
edcb779 to
d054352
Compare
7ea42f9 to
1beb02b
Compare
d054352 to
7f53c76
Compare
1beb02b to
153b882
Compare
7f53c76 to
94cb29b
Compare
7811905 to
e321644
Compare
94cb29b to
36d7973
Compare
e321644 to
bb3bcde
Compare
36d7973 to
17b4f24
Compare
bb3bcde to
a3ed383
Compare
17b4f24 to
7a3df0c
Compare
a3ed383 to
e0f731f
Compare
7a3df0c to
daf8bbb
Compare
e0f731f to
4eaf654
Compare
daf8bbb to
4df9e0e
Compare
4eaf654 to
3e8f040
Compare
4df9e0e to
57e5df1
Compare
3e8f040 to
d962947
Compare
57e5df1 to
9923c98
Compare
`ctx.json` was a response builder only, so `await ctx.json()` stringified
`undefined` into a Response. Handlers reading a posted payload got a Response
object back, which serialised out as `{}`, and every posted body was silently
dropped.
It is now overloaded on arity: `ctx.json()` parses and returns the request
body, `ctx.json(data, init?)` returns a JSON Response as before.
Worker isolation builds its own API context, so it got the same helper rather
than a second copy of the rule. Under WORKER_ISOLATION_API=1 the duplicate was
still response-only, which would have made a handler behave differently
depending on whether isolation was enabled.
Fixes bug 9 of the reproducer matrix.
Constraint: PR review required the public API overload to be documented, not just covered by tests. Confidence: high Scope-risk: narrow Tested: documentation-only change; existing ctx.json tests cover behavior Not-tested: docs site build
d962947 to
82d10a5
Compare
kwakayama
left a comment
There was a problem hiding this comment.
Score: 94/100
Critical review result: this fixes a real API-route bug. await ctx.json() previously returned a JSON Response built from undefined, so handlers that intended to parse POST JSON silently received a Response object and often serialized {} back to the client. The arity overload is narrow, keeps ctx.json(data, init?) response behavior intact, and covers the sandbox worker path by sharing the helper.
Validation reviewed:
deno test --allow-all src/routing/api/context-builder.test.tsdeno task lint:sanitizer-baselinedeno task lintdeno task typecheck
Recommended next step: merge after GitHub required checks complete green.
Summary
ctx.json()in pages API routes looked like a request-body parser but actually built a JSONResponsefromundefined. Handlers that wroteconst body = await ctx.json()received aResponseobject, and echoing it back serialized to{}, silently dropping POST payloads.This PR makes
ctx.jsonarity-based:ctx.json()parses the request body, whilectx.json(data, init?)still builds a JSONResponse. The worker-isolation API path now uses the same helper so isolated and in-process handlers behave the same.Verification
deno test --allow-all src/routing/api/context-builder.test.tsdeno task lint:sanitizer-baselinedeno task lintdeno task typecheckReview status
Score: 94/100. Recommended next step: merge after GitHub required checks finish green.