Repository navigation
Conversation
WalkthroughThe PR fixes JSON/JSONB parameter serialization in ChangesJSON/JSONB Parameter Serialization for Unprepared Queries
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Review rate limit: 2/5 reviews remaining, refill in 29 minutes and 19 seconds. Comment |
|
Updated 7:07 PM PT - May 4th, 2026
❌ @robobun, your commit 65062bc has 2 failures in
🧪 To try this PR locally: bunx bun-pr 30222That installs a local version of the PR into your bun-30222 --bun |
|
Found 1 issue this PR may fix: #28819 Fixes double encoding |
|
Found 1 issue this PR may fix:
Generated with Claude Code (https://claude.ai/code) |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — narrow, well-tested fix that routes object/array params through JSON.stringify when the OID is unspecified.
Extended reasoning...
Overview
This PR fixes #30221 by changing 4 lines of logic (plus a detailed comment) in src/sql/postgres/PostgresRequest.zig:writeBind, and adds 4 regression tests in test/js/sql/sql-prepare-false.test.ts. When prepare: false sends Parse with OID 0, the Bind serializer previously hit @enumFromInt(0) → undefined tag → else branch → String.fromJS → "[object Object]". The fix re-derives the tag via types.Tag.fromJS for the parameter_field == 0 case and routes .json/.jsonb results through the existing jsonStringifyFast branch.
Verification
I confirmed:
Tag.fromJS(Tag.zig:321) returns.jsonfor plain objects and array-likes, so the new branch correctly captures both repro shapes..json/.jsonbare not inisBinaryFormatSupported(Tag.zig:170), so they serialize as text — consistent with the format code0already written for OID-0 slots in the first loop. No format/payload mismatch.Signature.generate(Signature.zig:78) appends0for.json/.jsonb/.text/.timestamptz/etc., and the fix only diverts the.json/.jsonbsubset; strings, Dates, and nulls retain their existing (working) paths. Null values hitTag.fromJS→.numeric→ fall through → handled by the existingisEmptyOrUndefinedOrNullcheck immediately after.Tag.fromJScan throw onNumberObject/BooleanObject/non-indexable objects, but those would already have thrown inSignature.generatebeforewriteBindis reached, so no new error surface.
Security risks
None. Parameters remain bound via the wire protocol (no string interpolation into SQL); this only swaps Object.prototype.toString for JSON.stringify when serializing the bound value's text representation.
Level of scrutiny
Low–medium. The change is surgical, gated behind parameter_field == 0, and only affects the previously-broken object/array case. Behavior for all other value types is byte-identical to before. Four new tests exercise object, nested object, array, and both ::json/::jsonb casts.
Other factors
No CODEOWNERS for src/sql/. No prior reviewer comments to address. Bug-hunting agents found no issues. The PR description includes manual before/after verification plus regression checks for scalars, Date, bytea, BigInt, and bool.
![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 the current code and only fix it if needed.
Inline comments:
In `@test/js/sql/sql-prepare-false.test.ts`:
- Around line 98-148: The data handler assumes each `chunk` contains whole
protocol frames causing flakiness; buffer incoming bytes per-socket and parse
from that buffer instead of treating `chunk` as a complete packet. Concretely,
introduce a per-socket accumulator (e.g., `let buf = Buffer.alloc(0)` in the
server callback), append each `chunk` to `buf`, then run the existing parsing
loop against `buf` but only advance/consume when enough bytes are available for
the next field (startup length or message code+length); if there aren’t enough
bytes for a full frame, break the loop and keep the remainder in `buf` for the
next `data` event. Keep and update the existing flags (`gotStartup`) and logic
that uses `extractFirstParam`, `captured`, `parseComplete`, `bindComplete`,
etc., but operate on `buf` (slicing/consuming) rather than on `chunk`.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4bdbfa07-3ff4-4913-8a26-72671c707064
📒 Files selected for processing (2)
src/sql/postgres/PostgresRequest.zigtest/js/sql/sql-prepare-false.test.ts
|
The remaining CI failure is The SQL test passes cleanly — most recent debian-13 x64 ASAN shard: |
With `prepare: false`, Parse is sent with parameter OID 0 to let Postgres infer the type from context. `writeBind`'s value loop keyed off the same OID, so objects and arrays fell into the generic `else` branch and got `Object.prototype.toString`'d — producing `[object Object]` or a comma-joined list, both of which fail any `::jsonb` or `::json` cast. When parameter_field is 0, re-derive the tag from the JS value (matching what `Signature.generate` already did with `Tag.fromJS`) and route `.json` / `.jsonb` values through the JSON branch so `jsonStringifyFast` serializes them. Other types keep their existing text behavior. Fixes #30221
The docker-gated round-trip tests skip when the gate environment has no docker daemon. Add a mock Postgres backend alongside them that speaks just enough of the extended-query protocol (Parse / Describe / Bind / Execute / Sync) to capture the raw bytes the client places in the Bind message for parameter #0. Four cases assert the exact text that regressed in #30221 — plain object, nested object, plain array, object under ::json — plus three regression guards for string / int / null that the re-derivation path must not touch.
69b907a to
a2a0612
Compare
The mock returns OID 0, but the comment wrongly implied this mirrored real Postgres behavior for `::jsonb` casts. A real server resolves `$1::jsonb` to 3802 during Parse analysis. The reason this mock's OID doesn't matter is that `prepare: false` batches Parse + Describe + Bind + Execute + Sync in one write, so the first Bind is built from `statement.signature.fields` before any ParameterDescription arrives.
- Accumulate chunks per-socket instead of assuming each 'data' event holds whole protocol frames. TCP doesn't preserve boundaries, and on slower / loaded runners the Startup or Parse+Describe+Bind batch can be split. - Tighten the integer regression guard: Signature.generate writes OID 23 (int4, not 0), so the new parameter_field==0 branch is not taken for integers. Assert the exact 4-byte big-endian binary payload with format=1 — pre-fix else behavior would have been ASCII "42" in format=0, so this now actually proves the JSON re-derivation didn't steal this path.
There was a problem hiding this comment.
LGTM — both earlier nits were addressed in 65062bc, and the re-derivation is correctly scoped to only redirect json-shaped values when the OID is 0.
Extended reasoning...
Overview
This PR fixes #30221: with prepare: false, plain JS objects/arrays bound as parameters were stringified via Object.prototype.toString (→ "[object Object]") instead of JSON, because writeBind saw parameter_field == 0 and fell through to the generic String.fromJS branch. The fix is ~5 lines of logic in src/sql_jsc/postgres/PostgresRequest.zig: when the OID is 0, re-classify the value with the existing types.Tag.fromJS helper (the same one Signature.generate already ran on this value) and route .json/.jsonb results to the existing jsonStringifyFast arm. All other values fall through to the unchanged @enumFromInt(parameter_field) path. The rest of the diff is test coverage: a mock-Postgres wire-protocol harness that captures the raw Bind bytes, plus Docker-backed round-trip tests.
Security risks
None. Parameter values in the extended-query Bind message are length-prefixed binary fields, not interpolated into SQL text, so there is no injection surface. The change only affects how a JS object is serialized into that field (JSON text vs [object Object]). No auth, crypto, or permission code is touched.
Level of scrutiny
Moderate — Postgres wire-protocol encoding is user-facing, but the change is surgical and strictly additive. I verified against Signature.zig:78-87 that parameter_field == 0 is written for every tag outside {bool, int4, int8, float8, int2, numeric, float4, bytea}, and against Tag.zig:170-185 that OID 0 yields formatCode() == 0 (text) in the first loop, so the .json branch's text output stays consistent with the format code already written. Tag.fromJS is pure and was already invoked on the same value during Signature.generate, so re-invoking it cannot introduce a new throw. Non-JSON OID-0 slots (strings, Dates, Int32Array) take the identical pre-fix path, which the PR description and the regression-guard tests both confirm.
Other factors
I reviewed this PR twice previously; both nits (misleading int4 test title/assertion, inaccurate ParameterDescription comment) were addressed in 65062bc, as was CodeRabbit's TCP-frame-buffering concern. All inline threads are resolved. No CODEOWNERS cover src/sql_jsc/ or test/js/sql/. The bug-hunting system found no issues on the latest revision, CI is green except for an unrelated Windows HTTP flake, and the new test file passes 7/7 on the ASAN shard.
|
Closing: this PR's implementation lives entirely in Zig source files that have since been removed from the tree as part of the Rust migration. The change can no longer merge cleanly and the files it edits no longer exist on If the underlying issue is still present, it will need a fresh fix against the Rust implementation. |
1 participant
Fixes #30221.
Repro
Cause
With
prepare: false, the Parse message is sent with parameter OID0to let Postgres infer the type from context (e.g. from the::jsonbcast in the query). That same OID then drives the value-encoding switch in
writeBind:@enumFromInt(0)is not a definedTag, so the switch falls throughto
else, which callsString.fromJS:String.fromJSon a plain JS object invokesObject.prototype.toString(
"[object Object]"); arrays getArray.prototype.toString(
"1,2,3"). Postgres rejects both for anyjson/jsonbcast.The
prepare: truepath worked becauseParameterDescriptionfillsstatement.parameterswith real OIDs (114/3802), which routeto the existing
.jsonb, .jsonbranch →jsonStringifyFast.Fix
When
parameter_field == 0, re-derive the tag from the JS value withtypes.Tag.fromJS(the same helperSignature.generatealready usesto classify the slot). If the value is JSON-shaped (
.json/.jsonb), dispatch to the JSON branch sojsonStringifyFastruns.Everything else keeps its existing path — Dates, typed arrays,
BigInts, Bools, etc. are unchanged.
Format codes don't need adjusting: for an unspecified slot the first
loop already writes
0(text), and.json/.jsonbare text-formattypes, so serializer and format-code agree.
Verification
New tests in
test/js/sql/sql-prepare-false.test.tscover:::jsonb::json::jsonb::jsonbManual repro against local Postgres:
USE_SYSTEM_BUN=1(22P02 invalid input syntax).Regression checks that still produce identical output pre/post-fix on
prepare: false:int/text/float8scalars,Datewithout cast,Uint8Array→::bytea,BigInt→::int8,true→::bool, andInt32Array→::int[](still surfaces the pre-existingmalformed array literal" 1,2,3"— tracked by #29551 / #29552).