Repository navigation
Conversation
|
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:
WalkthroughThis PR changes PostgreSQL Bind encoding so JavaScript arrays are sent as PostgreSQL text array literals, with array format forced to text, recursive array rendering, special handling for JSON, dates, buffers, and objects, plus wire helpers and tests for Bind-frame inspection. ChangesArray Bind Serialization
Related issues: Suggested labels: bun:sql, postgres, bug-fix Suggested reviewers: SQL/Postgres protocol reviewer 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
Updated 10:07 PM PT - Aug 23rd, 2026
❌ @robobun, your commit e8df861 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 33579That installs a local version of the PR into your bun-33579 --bun |
|
Confirmed: the repro in #29551 ( |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Addressed both in 5053bd3:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/sql_jsc/postgres/PostgresRequest.rs (1)
160-163: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTrim these comment blocks to the 3-line limit.
Both added comment blocks exceed the repository’s comment-length rule; keep only the durable invariant here and move extra detail elsewhere if needed. As per coding guidelines, “Keep code comments to 3 lines max.”
Proposed concise comments
- // Serialize JS arrays as postgres text array literals (`{1,2,3}`), - // matching what postgres.js sends. The format code for these parameters - // was declared as 0 (text) in the loop above. json/jsonb arrays are - // excluded: they must stay JSON text (`[1,2,3]`), handled below. + // Serialize JS arrays as PostgreSQL text array literals. The format code + // was declared as text above; scalar json/jsonb arrays stay JSON text + // (`[1,2,3]`) and are handled below.-/// Serialize a JS array into a PostgreSQL text array literal (e.g. `{1,2,3}`, -/// `{"a","b"}`, `{1,NULL,3}`). Nested arrays become nested braces. Scalar -/// elements are double-quoted with `"` and `\` escaped, which postgres accepts -/// for every element type; `null`/`undefined` become an unquoted `NULL`. -/// Element text mirrors the JS-side `serializeArray` -/// (`src/js/internal/sql/postgres.ts`): `json[]`/`jsonb[]` elements and plain -/// objects -> JSON, `Date` -> ISO string, `Buffer` in `bytea[]` -> `\x<hex>`, -/// everything else -> `toString`. `depth` guards against stack overflow from -/// pathologically nested input; postgres itself rejects more than 6 dimensions. +/// Serialize a JS array into a PostgreSQL text array literal. +/// Mirrors JS-side `serializeArray` for NULL, JSON, Date, bytea, and nesting. +/// `depth` guards against stack overflow from pathologically nested input.Also applies to: 274-282
🤖 Prompt for 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. In `@src/sql_jsc/postgres/PostgresRequest.rs` around lines 160 - 163, The added comment blocks in PostgresRequest are too long and need to be trimmed to the repository’s 3-line limit. Shorten the comments near the array-serialization logic in PostgresRequest::... by keeping only the durable invariant and removing the extra implementation details, and apply the same cleanup to the other affected comment block referenced in the review so both stay within the comment-length guideline.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@src/sql_jsc/postgres/PostgresRequest.rs`:
- Around line 160-163: The added comment blocks in PostgresRequest are too long
and need to be trimmed to the repository’s 3-line limit. Shorten the comments
near the array-serialization logic in PostgresRequest::... by keeping only the
durable invariant and removing the extra implementation details, and apply the
same cleanup to the other affected comment block referenced in the review so
both stay within the comment-length guideline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 72258858-da5b-4d7d-afb4-fec5574ed73f
📒 Files selected for processing (1)
src/sql_jsc/postgres/PostgresRequest.rs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/sql_jsc/postgres/PostgresRequest.rs (1)
272-282: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueFix the stale depth-limit comment
MAX_ARRAY_DEPTHalready returnsInvalidQueryBinding; just update the “pg max is 6” note to match the actual 64-level safety cap.🤖 Prompt for 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. In `@src/sql_jsc/postgres/PostgresRequest.rs` around lines 272 - 282, The depth-limit comment in write_array_literal is stale: it still says “pg max is 6” even though MAX_ARRAY_DEPTH is 64 and overflow already maps to InvalidQueryBinding. Update the doc comment above write_array_literal to describe the actual 64-level safety cap, keeping the rest of the serialization behavior unchanged.
🤖 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.
Outside diff comments:
In `@src/sql_jsc/postgres/PostgresRequest.rs`:
- Around line 272-282: The depth-limit comment in write_array_literal is stale:
it still says “pg max is 6” even though MAX_ARRAY_DEPTH is 64 and overflow
already maps to InvalidQueryBinding. Update the doc comment above
write_array_literal to describe the actual 64-level safety cap, keeping the rest
of the serialization behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2732f518-41cb-4453-9fb4-e17c267ff33b
📒 Files selected for processing (1)
src/sql_jsc/postgres/PostgresRequest.rs
|
The diff is ready; CI is red on lanes unrelated to this change. All four CI runs on this branch (builds 69528, 69570, 69590, 69648) hit the same infra failure on so those jobs never downloaded a binary and never ran any tests. The remaining red is explicitly labeled The new test A maintainer can merge when ready or retry the darwin lane. |
|
Issue #39878 reports the same bug: a JS array bound through |
66334a1 to
2fb65ad
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
2fb65ad to
b569f68
Compare
|
Rebased onto main and squashed to one commit (b569f68). Conflicts were small:
Also pruned the inline comments flagged by the comment checker down to four one-liners.
|
|
Status after the rebase (b569f68, build 102364): the only red lane is
Addendum: `test/js/sql/postgres-tls-ctx-leak.test.ts` also showed up as yellow (failed in the parallel batch, passed alone). It runs only `select 1` with no parameters and counts `PostgresSQLConnection` objects after GC, so it does not reach the Bind encoding this PR changes. |
There was a problem hiding this comment.
I reviewed the rebased/squashed commit and found no new issues — all four earlier findings (OwnedString leak, hex helper reuse, jsonb[] primitive elements, Uint8Array in bytea[]) are correctly carried forward, and the comment-cop flags are resolved. Because this adds a ~90-line native serializer that iterates arbitrary JS values on the Postgres Bind path, a human look is still worthwhile.
What was reviewed:
- Format-code loop vs. value loop consistency:
int4_array/float4_arrayare the only array tags withis_binary_format_supported() == true, and both now hitforce_textviavalue.is_array(); every other array tag already returnsformat_code() == 0. - All three
json_stringify_fastsites wrap the out-param inOwnedStringso the +1 WTF ref is released on every exit. is_array_buffer_likeguardsis_cell()beforejs_type(); the bytea branch runs before the JSON-object branch.- Rebase conflict resolution in
wire-frames.ts— main'spgParseComplete/pgBindComplete/pgParameterDescription/pgReadFrontendMessagesare reused; onlypgNoData/pgDecodeBindare new.
Extended reasoning...
Overview
This PR adds text-format PostgreSQL array-literal encoding to write_bind in src/sql_jsc/postgres/PostgresRequest.rs. A JS array bound as a query parameter now emits {"1","2","3"} with format code 0, matching postgres.js/node-postgres. Previously int4[]/float4[] were declared binary but carried scalar/ASCII garbage, and every other array OID emitted a bare CSV. The change adds write_array_literal (~70 lines) and write_quoted_element, removes the dead int4_array match arm, forces format 0 for array values in the format-code loop, and fixes a pre-existing OwnedString leak in the json|jsonb arm. A new 13-case mock-backend test asserts exact Bind-frame bytes, and wire-frames.ts gains pgNoData + pgDecodeBind.
Security risks
None identified. Array-literal escaping (write_quoted_element) backslash-escapes " and \ per PostgreSQL array_in rules; the output goes into a Bind parameter value, not an SQL string, so this is not an injection surface. The MAX_ARRAY_DEPTH = 64 guard bounds recursion on pathological nested input and returns InvalidQueryBinding rather than overflowing the stack.
Level of scrutiny
Medium-high. This is native Rust that iterates arbitrary user JS values on a hot database path — every element goes through get_index, and per branch through json_stringify_fast, as_array_buffer, or BunString::from_js, all of which can throw or allocate WTF-refcounted strings. The repo's review guidance calls out exactly this class (exception propagation after every JS-entering call, +1 WTF ref ownership, GC re-entrancy during coercions). The PR has already been through four review iterations on this pass addressing an OwnedString leak, a Uint8Array-in-bytea[] silent-corruption regression, jsonb[] primitive-element parity with serializeArray, and a hex-helper duplication — all resolved and verified present in the squashed diff. The rebase also resolved non-trivial conflicts in wire-frames.ts (deduplicating helpers main added independently).
Other factors
- The mock-backend test approach is deterministic and asserts exact wire bytes; the 13 cases cover the input-space checklist reasonably (empty, nested, null elements, escaping, delimiter override for
box[],Date,Buffer+Uint8Arrayforbytea[],jsonbscalar carve-out,jsonb[]with primitives and objects). is_binary_format_supported()lists onlyint4_arrayandfloat4_arrayamong array tags; the added|| value.is_array()in theforce_textcheck covers exactly those, and every other array tag already yieldsformat_code() == 0, so the two passes over the parameter list stay consistent.- All eight comment-cop flags posted after the rebase are resolved; the surviving four inline comments in the new code are one-liners.
- Not approving because a ~90-line native serializer that walks user JS values on the Bind path is outside the "simple/mechanical" bar for auto-approval, and the rebase touched shared test infrastructure.
A raw JS array bound as a query parameter had no encoder in write_bind. int4_array wrote a 4-byte scalar under a binary format declaration, float4_array stringified to ASCII under a binary declaration, and every other array OID got JS toString output with no braces. PostgreSQL rejected all of them. Serialize arrays as text array literals and declare format 0. Element handling mirrors the JS-side serializeArray: json[]/jsonb[] elements and plain objects go through JSON.stringify, Date becomes an ISO string, any ArrayBufferView in bytea[] becomes \x hex, box[] uses the ; delimiter, null becomes NULL, and nesting produces nested braces. The superseded int4_array match arm is removed. Fixes #29551
b569f68 to
e8df861
Compare
|
Rebased onto main again (e8df861eb8). One conflict, in 13/13 tests pass on the rebased build, and the repro queries round-trip against PostgreSQL 17. |
|
Status after the second rebase (e8df861, build 104587): the only failed job is again That lane never downloads a binary and never runs a test, so it carries no signal about this change. Every other red item in 104587 is labeled
|
There was a problem hiding this comment.
I re-reviewed after the e8df861 rebase and found no bugs. Given this is ~90 lines of new native serialization touching JSC value iteration and wire encoding, a human sign-off is still worthwhile.
Checked in this pass: the three json_stringify_fast sites and the BunString::from_js fallback under #40238's owning String — Drop now releases the WTF ref and the to_utf8_without_ref() borrow is copied before str drops; the || value.is_array() addition to force_text covers exactly the two binary-supporting array tags (int4_array/float4_array) so the declared format code and the text-literal payload agree; branch ordering keeps Uint8Array in bytea[] on the hex path ahead of the plain-object JSON path; the depth-64 guard bounds cyclic/nested input.
Extended reasoning...
Overview
The PR adds a native array-literal encoder to the PostgreSQL Bind writer (write_array_literal + write_quoted_element in src/sql_jsc/postgres/PostgresRequest.rs, ~90 new lines), removes the dead/wrong int4_array match arm, and forces format code 0 for JS-array parameters. It ships a 13-case wire-byte test against a mock backend and adds pgNoData/pgDecodeBind helpers to test/js/sql/wire-frames.ts.
Prior review history
I ran four inline-review rounds on earlier revisions (WTF ref leak in json_stringify_fast, hand-rolled hex table, jsonb[] string-primitive divergence, Uint8Array-in-bytea[] silently JSON-stringified). All were addressed and are resolved in the thread. The branch was rebased twice since; the most recent rebase (e8df861, three days ago) adapted the three json_stringify_fast call sites to #40238, which made bun_core::String own its WTF ref and gave it Drop. I verified the current code against src/bun_core/string/mod.rs:1146 and src/jsc/JSValue.rs:1586: the returned String is a named local, the to_utf8_without_ref() slice borrows from it, and the bytes are copied into out before the local drops — no leak, no UAF. The pre-existing json | jsonb arm was updated the same way.
Security risks
None identified. The recursion depth guard (64) bounds adversarial nesting; every fallible JS call (get_length, get_index, json_stringify_fast, from_js) propagates via ?; element bytes are escaped for " and \\ per the PostgreSQL array-literal grammar. No untrusted length is used to size an allocation.
Level of scrutiny
Moderate-to-high. This is new hand-written serialization in a database driver hot path (bulk inserts via unnest(), = any() IN-lists), iterating arbitrary JS values from native code. It has already surfaced two real bugs during review (a per-element ref leak and a silent-corruption path for Uint8Array in bytea[]), and was rebased three days ago onto a String-ownership refactor. That is enough moving parts that a maintainer should sign off rather than land on automated approval alone.
Other factors
Tests are deterministic wire-byte assertions against a mock backend; they verified fail-before/pass-after on both ASAN-debug and release. End-to-end PostgreSQL 17 verification is stated but manual. All prior review threads are resolved and the comment-cop findings were addressed in the squash.
This branch has not been deployed
1 participant
What
A raw JavaScript array passed as a query parameter (not via
sql.array) was encoded as wire garbage, so every array parameter failed against a real PostgreSQL server:This is how you write
IN-lists (where id = any($1)), batch lookups, andunnest($1)bulk inserts, so all of those were broken.postgres.js/node-postgressend{1,2,3}/{"a","b"}and the same queries work.Cause
The Bind writer (
write_bindinsrc/sql_jsc/postgres/PostgresRequest.rs) had no array value encoder:Tag::int4_arrayfell into a scalarvalue.coerce::<i32>()arm and wrote a 4-byte scalar, while still declaring format 1 (binary).Tag::float4_arrayfell into the genericToStringarm ([1.5,2.5]->"1.5,2.5") but was declared binary, so the server read ASCII as a binary array header.text[],int8[], ...) also hit theToStringarm, producing"1,2"/"a,b"with no{}braces, which is not a PostgreSQL array literal.Fix
Serialize any JS array parameter as a PostgreSQL text array literal and declare it format 0 (text), matching what
postgres.jsdoes:[1, 2, 3]->{"1","2","3"}["a", "b"]->{"a","b"}{{"1","2"},{"3","4"}}null/undefinedelements -> unquotedNULL"and\in element text are escapedjson/jsonbarray parameters are excluded and keep their existing JSON-text serialization ([1,2,3]), and a recursion-depth guard bounds pathologically nested input.Verification
test/js/sql/postgres-bind-array-params.test.tsdrives the prepared-query flow against a mock backend and asserts the exact bytes of the Bind message (format code + parameter value), so it is deterministic and needs no real PostgreSQL. The array-OID cases fail on the unfixed build (format code 1, scalar/ToStringpayload) and pass with the fix; thejsonbcase guards against regressing JSON serialization.Also verified end to end against PostgreSQL 17: all five queries above now return correct results.
Fixes #29551
[review] gate passed · iteration 6 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 6
evidence per changed file