Conversation
write_bind sends a parameter in text format when the server-reported type has no binary encoder (date, text, domains, OID 0 with prepare: false). A JS Date on that path went through BunString::from_js, that is Date.prototype.toString(), which Postgres rejects for a date parameter with 22007 and stores verbatim for a text one. Send toISOString() output instead, as postgres.js and pg do. timestamp and timestamptz parameters are bound in binary and are unchanged.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. WalkthroughChangesThe Postgres text-format binder now serializes valid JavaScript Postgres Date binding
Suggested reviewers: Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to PostgreSQL Date parameters now use stable ISO text serialization, fixing locale-dependent date parsing while preserving invalid-date rejection and existing timestamp behavior. The covered binding paths are ready to merge. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status Reproduced on Bun 1.4.3 against PostgreSQL 17 with the default import { SQL } from "bun";
const sql = new SQL({ url: "postgres://bun_sql_test@127.0.0.1:5432/bun_sql_test", max: 1 });
await sql`create temp table w3 (d date)`;
await sql`insert into w3 (d) values (${new Date("2024-05-06T07:08:09.123Z")})`;
// PostgresError 22007: invalid input syntax for type date: "Mon May 06 2024 07:08:09 GMT+0000 (Coordinated Universal Time)"
The diff is green. The red lanes in builds #112805 and #112842 are unrelated to this change: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — focused fix, well-covered.
Checked the lifetime of the borrowed slice in the text arm: iso_buf, str, and utf8 are all hoisted above the if, so both branches' backing storage outlives the writer.write(slice) call (borrow-checker enforced, no unsafe). to_iso_string returning None for NaN time falls through to the existing BunString::from_js path, so no partial Bind is written mid-error. The new test file follows the per-topic postgres-*.test.ts convention already established in test/js/sql/, uses the container harness, and pins the exact 22007 message plus a timestamptz/timestamp regression guard for the untouched binary path.
Extended reasoning...
Overview
The PR touches two files: a ~15-line change to the text-format catch-all arm of write_bind in src/sql_jsc/postgres/PostgresRequest.rs, and a new 93-line test file. The Rust change detects a JS Date via value.is_date() and writes toISOString() output into a 64-byte stack buffer instead of falling through to BunString::from_js (which yields Date.prototype.toString() — rejected by Postgres for date/domain columns with SQLSTATE 22007). Non-Date values and invalid Dates keep the existing path unchanged.
Security risks
None. No untrusted input parsing is added; the change only alters which string representation of a client-side Date is written to the outgoing Bind message. No unsafe blocks, no allocation, no FFI surface change — to_iso_string is an existing helper that writes into a caller-owned stack buffer and returns a lifetime-tied slice.
Level of scrutiny
Low-to-moderate. The diff is small, mechanical, and in safe Rust where the borrow checker enforces the deferred-init lifetime pattern (iso_buf/str/utf8 declared before the branching if let, so the resulting &[u8] cannot dangle). The one user-visible behavior change — a Date bound to ::text now yields the ISO string rather than the locale toString() form — is intentional, matches postgres.js and pg, and is pinned by a test. The PR description explicitly reasons through invalid dates (no mid-write early return, avoiding the partial-buffer hazard until #34732 lands), domains, prepare: false, and the untouched binary timestamp/timestamptz path.
Other factors
Test coverage is strong for the size of the change: prepared and cached-statement runs, all three insert parameter paths (positional, sql({..}), unsafe), a non-UTC session time zone round-trip, ::text binding, invalid-Date rejection with exact errno and message, a regression guard on the binary timestamp path, and prepare: false. The new file sits alongside ~40 sibling single-topic postgres-*.test.ts files, so placement matches local convention rather than violating the "add to existing file" default. No CODEOWNERS entry covers src/sql_jsc/ or test/js/sql/. Exit reason was dry_streak with no findings and no outstanding third-party reviews on the timeline.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Updated 6:45 AM PT - Sep 8th, 2026
❌ @robobun, your commit 9899625 has 4 failures in
🧪 To try this PR locally: bunx bun-pr 41955That installs a local version of the PR into your bun-41955 --bun |
There was a problem hiding this comment.
LGTM — focused fix with the variant matrix covered.
Checked the iso_buf/str/utf8 deferred-init borrows in the text-format arm — all outlive slice through writer.write. to_iso_string returning None on NaN correctly falls through to toString(), which the invalid-Date test pins to a server-side 22007. Tests use the container harness (hermetic), exercise both prepared-statement passes plus prepare: false, and confirm the timestamptz/timestamp binary paths are untouched. New test file follows the existing test/js/sql/postgres-*.test.ts split.
Extended reasoning...
Overview
The PR changes the fallback text-format arm of write_bind in src/sql_jsc/postgres/PostgresRequest.rs (lines 218-238) so that a JS Date bound to a Postgres parameter type without a binary encoder (notably date, but also text, domains, and everything under prepare: false) is serialized as ISO-8601 via the existing JSValue::to_iso_string helper instead of Date.prototype.toString(). The change is ~15 lines of Rust: a 64-byte stack buffer, an if let-chain that tries to_iso_string for valid Dates, and the original BunString::from_js path for everything else (including new Date(NaN), since to_iso_string returns None for a NaN time value). A new 93-line test file exercises seven scenarios against the containerized Postgres harness.
Security risks
None identified. This is output serialization of a parameter value inside the wire-protocol Bind message; the value is still length-prefixed via writer.length()/write_excluding_self() exactly as before, so there is no change to framing or any injection surface. The ISO string is produced by JSC's own date formatter into a fixed stack buffer — no user-controlled length drives an allocation. No auth, crypto, or permission code is touched.
Level of scrutiny
Low-to-moderate. The Rust change is small, uses an existing in-tree helper (to_iso_string), and preserves the original code path verbatim for the non-Date case. The deferred-initialization pattern (let str; let utf8;) keeps the BunString and its to_utf8() borrow alive for the whole block, satisfying the src/CLAUDE.md rule that a to_utf8() result borrows the String. The iso_buf stack array likewise outlives the slice borrow through the single writer.write(slice) call. I checked JSValue::to_iso_string at src/jsc/JSValue.rs:1044 — it returns None for non-Date or NaN, so the fallback ordering is sound and is_date() before it is a cheap guard rather than a correctness requirement.
Other factors
Test coverage is strong for a fix of this size: prepared-statement first and cached passes, all three insert parameter paths (literal, object helper, sql.unsafe), UTC calendar-date semantics under a +14 session time zone with a round-trip, ::text yielding the ISO string, invalid Date pinned to the server's 22007 with the exact message, timestamptz/timestamp binary paths asserted unchanged, and prepare: false. Tests use describeWithContainer (hermetic, no public network) and await using for cleanup. The new file follows the established test/js/sql/postgres-*.test.ts convention (dozens of sibling files already exist), so it does not violate the "add to existing file" default. No CODEOWNERS entry covers src/sql_jsc/ or test/js/sql/. The PR's embedded CI evidence shows 5/7 tests failing on the base release build and all 7 passing on the PR's ASAN and release builds. No outstanding human CHANGES_REQUESTED reviews; the resolved inline threads were from bots and the author. Exit reason was dry_streak.
…ind bytes with mock-server tests FieldDescription::type_tag() truncated a result column's OID to 16 bits for the Bind result format code, while DataCell mapped an OID above 65535 to text. A user-defined type whose OID aliases a binary-decoded builtin modulo 65536 was requested in binary and read as text. Tag::from_oid() is now the one mapping, for parameters, result format codes and DataCell. wire-frames.ts gains pgNoData() and pgDecodeBind(). The new mock-server tests in postgres-bind-parameter-format.test.ts check the exact format codes and value bytes of the Bind message without a database. The container tests from #41912 and the Date test file from #41955 are carried over.
|
Closing in favor of #41976. With its text fallback a Date reaches the text arm for |
1 participant
Problem
prepare: true, a JSDatebound to adateparameter fails withPostgresError 22007 invalid input syntax for type date: "Mon May 06 2024 07:08:09 GMT+0000 (Coordinated Universal Time)".write_bind(src/sql_jsc/postgres/PostgresRequest.rs:218). Bun declares OID 0 for aDate, the server answersdate(1082), which has no binary encoder, and the arm serializes withDate.prototype.toString().Fix
Dateas itstoISOString()output. Other values (and an invalidDate) keeptoString().date,timestamp,timestamptzandtextinput. postgres.js and pg send the same form.textparameters, domains, and allprepare: falseparameters, so aDatebound totextnow stores the ISO string. This re-lands sql: serialize Date parameters as ISO 8601 in Postgres text format #29013 and supersedes theDatebranch of postgres: encode Date and object parameters correctly with prepare: false #39452.test/js/sql/postgres-date-param-text.test.ts(5 of 7 fail on 1.4.3), plus the other Postgres date tests andsql.test.ts. Self-reviewed: 6 concerns raised, all packaging and merge order, addressed in Notes.Background
prepare: trueBun binds with the described types: binary for those inTag::is_binary_format_supported, text for the rest.datecolumn decodes to aDateat UTC midnight. Ondateinput the server keeps the calendar date as written, so the UTC ISO string round-trips in any time zone.Fixes #29010. Refs #39450.
Notes
Date: it is sent as"Invalid Date"and the server answers 22007 (pinned by a test). sql: serialize Date parameters as ISO 8601 in Postgres text format #29013 ended up returningInvalidQueryBindingfrom the middle ofwrite_bindfor it. On main that leaves a partial Bind message in the write buffer and breaks the connection on the next query (sql(postgres): discard a partial Bind when a parameter fails to encode #34732 adds the rollback). Until that lands, letting the server answer 22007 is the safe choice, and it is what postgres: encode Date and object parameters correctly with prepare: false #39452 does too. postgres.js throws client-side (toISOString()of an invalid Date throws).prepare: false, Bun.SQL with prepare:false serializes object JSONB parameters as "[object Object]" #30221,SQL:prepare: falsechanges parameter encoding —Dateand plain objects stop binding #39450) sends ISO for aDateonly when the bind-time OID is 0. This arm covers OID 0 too, so after this lands postgres: encode Date and object parameters correctly with prepare: false #39452 can drop itsTag::timestamptzbranch and keep the JSON half. Its Date tests pass with either. sql(postgres): declare text format for Bind parameters that have no binary encoder #41912 (Bind format codes,ParamEncoding) renames this arm toParamEncoding::Text, a one-line textual conflict. Theis_date()branch goes inside that arm.timeandtimetz: the Postgrestimeparsers reject the ISOTseparator, andtimeis declared binary in Bind without a binary encoder (the value loop writes text bytes, the server answers22008 time out of range, sql(postgres): declare text format for Bind parameters that have no binary encoder #41912 fixes that mismatch). ADatebound to those keeps failing, now with the ISO text in thetimetzmessage. Bind an'HH:MM:SS'string for those types.interval: noDatetext form is valid input. It fails before and after.±YYYYYY-MM-DDform, which Postgres rejects (time zone displacement out of range).toString()output was rejected too.create domain d as timestamptzthenselect $1::dwith aDatefailed before this change (a domain OID is not a binary tag) and works now.dateandtimestampand does not depend on the server or client time zone.sql.test.tsrun (docker gate bypassed, local Postgres 17): 15 failures on 1.4.3 and the same 15 plus 4 debug-build timeouts with this branch (should not timeout in long resultshas a hard 10 s budget, the other three pass with a longer timeout). None bind aDate.sql-prepare-false.test.ts,sql-postgres-datetime-roundtrip.test.ts,postgres-infinity-date.test.ts,postgres-timestamptz-text.test.ts,postgres-datestyle.test.ts(38 pass).[human-review] gate passed · iteration 2 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 2
evidence per changed file