Repository navigation
Conversation
|
Updated 3:22 PM PT - May 5th, 2026
❌ @autofix-ci[bot], your commit 58ab372 has 4 failures in
🧪 To try this PR locally: bunx bun-pr 28821That installs a local version of the PR into your bun-28821 --bun |
|
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:
WalkthroughDetect JS values that are already strings for MySQL/Postgres JSON types and bind them as raw UTF‑8 text (via Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/sql/mysql/MySQLTypes.zig`:
- Around line 488-496: The current branch in MySQLTypes.zig treats any JS string
as pre-serialized JSON which breaks JSON binds; change the logic so that
value.isString() only results in raw passthrough when an explicit pre-serialized
marker is present (e.g., a special property or a wrapper predicate like
isPreSerializedJSON(value)); otherwise, for JSON-typed binds call
jsonStringifyFast() (or the existing serialization path) to produce a quoted
JSON string. Update the branch around the Value{ .string = ... } return and add
a helper predicate (e.g., isPreSerializedJSON) to detect the explicit marker so
existing plain JS strings remain quoted as before.
In `@src/sql/postgres/PostgresRequest.zig`:
- Around line 106-129: The current branch in PostgresRequest.zig treats any JS
string as already-serialized JSON (value.isString()) which breaks plain-string
binds; change the condition so only values explicitly marked as pre-serialized
are sent verbatim: introduce and check for a sentinel (e.g., a special property
or Symbol like "__bun_pre_serialized_json" or
Symbol.for("bun.preSerializedJSON")) on the JS value instead of
value.isString(), and keep the existing String.fromJS + writer.write(slice) path
for sentinel-marked values while using value.jsonStringifyFast(globalObject,
&str) for ordinary JS strings; update the logic around String.fromJS,
jsonStringifyFast, and writer.write slices accordingly so only sentinel-marked
inputs bypass jsonStringifyFast.
In `@test/regression/issue/28819.test.ts`:
- Around line 12-73: The jsonb test in runJsonBindingTests() misses
string/null/boolean cases and there's no MySQL coverage; update the "strings
bound to ::jsonb are not double-encoded" test (table test_jsonb_28819) to also
INSERT the JSON.stringify("bare string"), JSON.stringify(null), and
JSON.stringify(true) values (same as the json test), and add a MySQL variant of
the matrix that runs the same set of INSERTs/SELECTs against a MySQL JSON column
(replicate the same expectations but without Postgres casting syntax, e.g. use
VALUES (?) or JSON literals appropriate for MySQL) so both the jsonb fast-path
and MySQL code paths are exercised; ensure changes are made inside
runJsonBindingTests and related test names so test discovery still picks them
up.
🪄 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: 6d7bf083-432e-489a-a858-343055a6724f
📥 Commits
Reviewing files that changed from the base of the PR and between 2f6f266 and d538d119011cad15bc195175b734b40c28d99a4a.
📒 Files selected for processing (3)
src/sql/mysql/MySQLTypes.zigsrc/sql/postgres/PostgresRequest.zigtest/regression/issue/28819.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR correctly fixes the double-encoding issue, but there's a behavioral regression worth human review: plain JS strings (e.g., 'hello') passed to json/jsonb columns were previously auto-quoted by jsonStringifyFast (producing valid JSON "hello"), but with this change they are sent verbatim, causing Postgres to return "invalid input syntax for type json". The test only covers strings already produced by JSON.stringify, so this regression goes undetected.
Extended reasoning...
Overview
The PR modifies and to add a fast-path for JSON parameters: when the bound value is already a JS string, it is sent verbatim to the server instead of being passed through . This fixes the reported double-encoding bug (#28819) for pre-stringified JSON. A regression test in is also added.
Security Risks
No security concerns — this is purely a data-encoding change in query parameter binding. No auth, crypto, or permission code is touched.
Level of Scrutiny
Medium-high. The change touches the core SQL parameter binding path for both Postgres and MySQL, which is a critical code path for correctness. The fix is mechanically simple and clearly addresses the described double-encoding issue, but it introduces a behavioral change that could silently break existing code.
Other Factors
The bug hunter identified a real behavioral regression: plain JS strings that are not valid JSON (e.g., 'hello', 'some-label') were previously auto-quoted via jsonStringifyFast (becoming '"hello"', a valid JSON string), but after this change they are sent verbatim, causing Postgres to reject them with "invalid input syntax for type json". The test suite only exercises values produced by JSON.stringify (which already carry their own quotes) and does not cover this case. This is a breaking change for any caller that relies on the old auto-quoting behavior, and it deserves a documented decision or a safeguard in the implementation. A human reviewer should weigh in on the intended contract for string parameters to json/jsonb columns.
| .jsonb, .json => { | ||
| var str = bun.String.empty; | ||
| defer str.deref(); | ||
| // Use jsonStringifyFast for SIMD-optimized serialization | ||
| try value.jsonStringifyFast(globalObject, &str); | ||
| const slice = str.toUTF8WithoutRef(bun.default_allocator); | ||
| defer slice.deinit(); | ||
| const l = try writer.length(); | ||
| try writer.write(slice.slice()); | ||
| try l.writeExcludingSelf(); | ||
| if (value.isString()) { | ||
| // The user pre-serialized their JSON value (e.g. via | ||
| // JSON.stringify). Send it verbatim; calling | ||
| // jsonStringifyFast here would wrap it in an extra layer | ||
| // of quoting and Postgres would store it as a JSON string. | ||
| const str = try String.fromJS(value, globalObject); | ||
| if (str.tag == .Dead) return error.OutOfMemory; | ||
| defer str.deref(); | ||
| const slice = str.toUTF8WithoutRef(bun.default_allocator); | ||
| defer slice.deinit(); | ||
| const l = try writer.length(); | ||
| try writer.write(slice.slice()); | ||
| try l.writeExcludingSelf(); | ||
| } else { | ||
| var str = bun.String.empty; |
There was a problem hiding this comment.
🔴 Plain JS strings that are not valid JSON (e.g., 'hello', 'some-label') sent to json/jsonb (Postgres) and MYSQL_TYPE_JSON (MySQL) columns now reach the server as bare unquoted text, causing Postgres to return "invalid input syntax for type json". Before this PR, all strings were auto-quoted through jsonStringifyFast, so 'hello' became '"hello"' (valid JSON); after the PR, the value.isString() fast-path sends the bytes verbatim. The test suite only exercises strings produced by JSON.stringify (which already carry their own quotes) and does not cover plain strings, so the regression is undetected.
Extended reasoning...
What the bug is and how it manifests
The PR introduces a fast-path for the .json/.jsonb branch (PostgresRequest.zig lines 105–120) and the MYSQL_TYPE_JSON branch (MySQLTypes.zig lines 485–495): when value.isString() is true, the string is forwarded verbatim to the wire without going through jsonStringifyFast. The intent is to avoid double-encoding strings that were already serialized via JSON.stringify. That intent is sound, but it creates a behavioral regression for any plain JS string that is not itself valid JSON.
The specific code path that triggers it
When a user writes:
const label = 'some-label';
await sql`INSERT INTO log (meta) VALUES (${label}::json)`;value.isString() is true, so the new code path is taken. The string some-label is sent verbatim. Postgres parses that as a JSON value and rejects it with ERROR: invalid input syntax for type json: "some-label". The same logic applies to MySQL's MYSQL_TYPE_JSON branch.
Why existing code doesn't prevent it
There is no validity check: the code does not distinguish between a string that is already valid JSON (e.g., '"hello"' produced by JSON.stringify('hello')) and a plain JS string that is not valid JSON (e.g., 'hello'). Any truthy value.isString() result takes the verbatim path.
What the impact would be
Any codebase that currently passes plain (non-JSON-stringified) strings to json/jsonb columns will start receiving Postgres server errors after this change. This is a silent behavioral regression: the old behavior was to auto-quote, making 'hello' into '"hello"'; the new behavior silently changes that to an error.
Addressing the refutation
The refutation argues this is intentional: "strings bound to json/jsonb parameters are treated as pre-serialized wire-format JSON." That design intent is documented in the PR description and is a reasonable approach. However, "intentional design choice" and "breaking change with no migration path" are not mutually exclusive. The old behavior — auto-quoting plain strings — was well-established and relied upon by callers. Changing it without documentation or a deprecation warning, and without a test that demonstrates the old behavior now errors, constitutes a real behavioral regression regardless of intent. The refutation does not dispute that plain strings now cause Postgres errors; it only argues the new behavior is correct. That is insufficient to claim the regression is acceptable.
How to fix it
The safest approach is to keep the verbatim path only for strings that are already valid JSON (i.e., start with {, [, a digit, ", t, f, or n and parse successfully), and fall through to jsonStringifyFast for all others. A lighter-weight alternative is to document this as a breaking change prominently in the PR and release notes, and update the test to assert that a plain non-JSON string now errors rather than silently masking the regression.
Step-by-step proof
- User has code:
await sqlINSERT INTO events (payload) VALUES (${'click'}::json)`` - Before this PR:
value.isString()→jsonStringifyFast('click')→ wire bytes"click"→ Postgres stores JSON string"click". Works. - After this PR:
value.isString()istrue→ verbatim fast-path → wire bytesclick(no quotes) → Postgres:ERROR: invalid input syntax for type json: "click". Crashes. - The test at line 38 of the new test file tests
JSON.stringify('bare string')='"bare string"'(already has quotes) — that is NOT the same as the bare string'bare string'and does not catch this regression.
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/regression/issue/28819.test.ts`:
- Around line 83-95: The nested try/finally in checkLocal that creates new SQL({
url, ... }) and then awaits sql`SELECT 1` can be simplified to use the
disposable pattern: replace the manual try/finally+sql.end() with an await using
(or equivalent disposable helper) to acquire the SQL instance and ensure
sql.end() is called automatically; update the function checkLocal to obtain the
SQL instance, run the tagged query (sql`SELECT 1`) inside the using block, and
remove the explicit finally that calls SQL.end(), leaving the outer catch to
return false on failure.
🪄 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: 79b078e7-8b6b-4632-9b93-0af040b7deb5
📥 Commits
Reviewing files that changed from the base of the PR and between d538d119011cad15bc195175b734b40c28d99a4a and 73f80734bfd7d138f6ccd4345e8594b8aef219c5.
📒 Files selected for processing (1)
test/regression/issue/28819.test.ts
e322693 to
5163435
Compare
f6ba967 to
8d97e98
Compare
| if (value.isString()) { | ||
| // The user pre-serialized their JSON value (e.g. via | ||
| // JSON.stringify). Send it verbatim; calling | ||
| // jsonStringifyFast here would wrap it in an extra layer | ||
| // of quoting. | ||
| const str = try bun.String.fromJS(value, globalObject); | ||
| defer str.deref(); | ||
| return Value{ .string = str.toUTF8(bun.default_allocator) }; | ||
| } |
There was a problem hiding this comment.
🟡 This value.isString() branch inside .MYSQL_TYPE_JSON appears to be unreachable: the only call site (MySQLQuery.zig:28) passes param.type from statement.signature.fields, which is populated by FieldType.fromJS — and that returns .MYSQL_TYPE_STRING (never .MYSQL_TYPE_JSON) for any JS string. So MySQL likely never had bug #28819 in the first place (a pre-stringified string already went verbatim through the else => fallback as MYSQL_TYPE_STRING). Harmless, but worth either dropping this hunk or confirming there's a path where server-reported param types feed into Value.fromJS — and the PR description's MySQL claim / lack of MySQL test coverage should probably be adjusted accordingly.
Extended reasoning...
What the issue is
The new if (value.isString()) branch added inside the .MYSQL_TYPE_JSON case of Value.fromJS (lines 488–496) is dead code. The field_type argument passed to Value.fromJS is derived exclusively from the same JS value being converted, via FieldType.fromJS, which classifies JS strings as .MYSQL_TYPE_STRING — never .MYSQL_TYPE_JSON. Therefore, whenever execution reaches the .MYSQL_TYPE_JSON arm, the bound value was necessarily a JS object (the only input for which FieldType.fromJS returns .MYSQL_TYPE_JSON), and value.isString() is always false.
The specific code path
The only MySQL call site of Value.fromJS is MySQLQuery.zig:28, which passes execute.param_types[i].type. execute.param_types is assigned to statement.signature.fields at MySQLQuery.zig:53 — it is not assigned to the server-reported statement.params from COM_STMT_PREPARE (those are stored at MySQLConnection.zig:889 but only used for length assertions at MySQLQuery.zig:46-47). signature.fields is populated solely by Signature.generate → FieldType.fromJS (Signature.zig:54). Looking at FieldType.fromJS in this file: if (tag.isStringLike()) return .MYSQL_TYPE_STRING; runs before the .MYSQL_TYPE_JSON return, which only fires for tag.isObject().
Why cached-statement reuse doesn't help
One might wonder whether a string parameter could be re-bound under a previously-JSON-typed cached statement. It cannot: the signature name encodes @tagName(tag) per parameter (Signature.zig:60), and the cache hash includes both the name and the field bytes. A string parameter and an object parameter therefore produce different cached statements, so cross-type reuse is impossible.
Step-by-step proof
- User writes
await sql\INSERT INTO t (j) VALUES (${JSON.stringify({a:1})})`` against a MySQL JSON column. Signature.generatecallsFieldType.fromJS(globalObject, '{"a":1}', &unsigned).value.isCell()→ true;tag.isStringLike()→ true → returns.MYSQL_TYPE_STRING. The signature field isMYSQL_TYPE_STRING.bindAndExecutesetsexecute.param_types = statement.signature.fields(allMYSQL_TYPE_STRING).Value.fromJS('{"a":1}', globalObject, .MYSQL_TYPE_STRING, false)falls through to theelse =>arm (line ~505), which already doesstr.toUTF8and sends the bytes verbatim — nojsonStringifyFast, no double-encoding.- The
.MYSQL_TYPE_JSONarm (and the newisString()branch inside it) is never entered for this input.
Impact
None at runtime — this is harmless defensive code. The practical concerns are: (a) the PR description states MySQL was affected by #28819, but the trace above shows pre-stringified JSON was already sent verbatim on MySQL via the else => fallback, so the original bug was Postgres-only; (b) no MySQL test was added, so nothing demonstrates this branch is reachable; (c) the inline comment ("The user pre-serialized their JSON value…") is misleading about reachability.
How to address
Either drop the MySQL hunk (and update the PR description to say the fix is Postgres-only), or — if there is a future plan to feed server-reported statement.params types into Value.fromJS — keep it as forward-looking defensive code but adjust the comment to note it's currently unreachable. A MySQL regression test would settle whether #28819 ever reproduced there.
8d97e98 to
fe9a53b
Compare
|
The failing CI checks are pre-existing flaky tests / infrastructure unrelated to this PR:
The PR touches only The fix itself is verified: the previous run showed the exact expected diff (object/array/number → wrongly stored as JSON string without the fix), and tests pass with the fix applied. Happy to retry after any main-branch deflaking. |
When a JS string is bound to a ::json or ::jsonb parameter, Bun was running jsonStringifyFast on it, wrapping the user's already-serialized JSON in an extra layer of quoting. Postgres then stored it as a JSON string rather than the intended object/array/number. Strings bound to json/jsonb columns are now sent verbatim. Non-string values (objects, arrays, numbers) are still JSON.stringified. Closes #28819
Add string/null/boolean cases to the jsonb test that were already covered for json, per review feedback.
Remove top-level await + local-postgres fallback. Matches the pattern used by 21311.test.ts and other postgres regression tests.
When isDockerEnabled() is false (farm gate / local dev without docker), run the regression against a localhost postgres at postgres@localhost:5432. This lets the gate verify the fix without needing Docker, while CI still uses the docker postgres_plain container.
e63a748 to
434816d
Compare
|
Rebased onto latest main. |
| // it's not always running when `bun test` is invoked directly). | ||
| async function tryStartLocalPostgres(): Promise<void> { | ||
| try { | ||
| const { existsSync, readdirSync } = await import("node:fs"); |
There was a problem hiding this comment.
🟡 Per test/CLAUDE.md, dynamic import() should only be used when the test is specifically exercising dynamic-import behavior; this is just test-infrastructure plumbing. Hoist existsSync / readdirSync to a top-level import { existsSync, readdirSync } from "node:fs"; instead.
Extended reasoning...
What the issue is
test/CLAUDE.md (the "Importing modules in tests" section, ~lines 218–240) states:
Only use dynamic import or require when the test is specifically testing something related to dynamic import or require. Otherwise, always use module-scope import statements.
It even gives a near-identical example flagged as BAD:
// BAD
const { readFile } = await import("node:fs");tryStartLocalPostgres() in this new test file does:
const { existsSync, readdirSync } = await import("node:fs");This is purely test-infrastructure code (probing /usr/lib/postgresql to opportunistically start a local server) — it has nothing to do with testing dynamic import or require semantics, so it falls squarely under the convention's "BAD" pattern.
Why nothing prevents it
node:fs is a builtin, so the dynamic import always resolves and works at runtime — there is no functional bug here. It is strictly a project-convention violation that the linter / type-checker won't catch.
Impact
None at runtime. The cost is consistency: module-scope imports surface the dependency at the top of the file, are eagerly resolved with the rest of the test's imports, and match how every other node:fs use in the test suite is written.
How to fix
Add to the existing top-level import block:
import { existsSync, readdirSync } from "node:fs";and drop the await import("node:fs") line from tryStartLocalPostgres(). The function is already async for the Bun.spawn(...).exited await, so nothing else needs to change.
Step-by-step walkthrough
- Test file loads; module-scope imports (
bun,bun:test,harness) resolve. - Top-level await runs
canConnect(...), which fails →tryStartLocalPostgres()is called. - Inside the helper,
await import("node:fs")resolves the builtin and destructuresexistsSync/readdirSync. - Those functions are used exactly once each to look for
/usr/lib/postgresql/<version>. - The dynamic import contributed nothing over a static one — no conditional loading, no lazy-evaluation benefit, no test of import semantics — so per
test/CLAUDE.mdit should be a static module-scope import.
|
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 #28819
Repro
Cause
In
PostgresRequest.writeBind(andMySQLTypes.toValue), the.json/.jsonb(and MySQLMYSQL_TYPE_JSON) branch always calledvalue.jsonStringifyFast()on the bound value. When the user had already stringified their JSON, this wrapped the string in an extra layer of JSON quoting, so Postgres/MySQL stored'{"hello":"world"}'as the JSON string value"{\"hello\":\"world\"}".Fix
When the bound value is already a JS string, send it verbatim to the server — the user already produced the wire-format JSON. Non-string values (objects, arrays, numbers) still run through
jsonStringifyFastas before.Verification
With the fix, the repro now produces:
Regression test:
test/regression/issue/28819.test.tscovers all six JSON types (object, array, number, string, null, boolean) for bothjsonandjsonbcolumns, plus re-verifies that raw JS objects/arrays are still serialized correctly.