sql_jsc: add SQLDataCell tag constructors and replace hand-rolled struct literals

[alii]

What this does

Extracts the SQLDataCell tag-constructor work from #31664 (closed). Adds 12 named constructors (SQLDataCell::null(), int4(v), uint4(v), int8(v), float8(v), bool_(v), date(v), date_with_tz(v), string(s), json(s), raw(d), uint8(v)) and replaces ~135 hand-rolled SQLDataCell { tag: Tag::X, value: SQLDataCellValue { x: v }, free_value: 0, ..Default::default() } literals across the postgres and mysql row decoders (DataCell.rs, ResultSet.rs, DecodeBinaryValue.rs). Adds a shared to_js_object helper used by both Putter::to_js and Row::to_js. Deletes three near-identical private clone-utf8 helpers in favour of one. Net −294 lines of src.

Row-decode hot path; every constructor verified field-for-field identical (tag, active value member, free_value) to the literal it replaces. Default::default() is unchanged from main ({Null, {null:0}, 0, 0, 0}) so every ..Default::default() expands the same.

Also fixes a pre-existing bug the refactor surfaced in decode_binary: the is_null branch did continue; before assigning index / is_indexed_column, so a NULL on a digit-named column over the binary protocol landed at object index 0 instead of the column's numeric name (and hit ASSERT(cell.isIndexedColumn()) in debug). decode_binary now mirrors decode_text.

Commits:

• Cherry-pick from origin/ali/sql-jsc-cleanup (sql: deduplicate and remove dead code across the MySQL/Postgres drivers #31664)
• Reconcile with main: re-adds dedupe_columns (sql_jsc: extract shared query/connection ctor-arg parsing across postgres and mysql #32135) and the IntoOptionalData trait + raw() signature the cherry-pick clobbered; strips stale TODO(port)/PORT NOTE comments
• Route mysql Row::to_js through the shared to_js_object; drop redundant .as_deref()
• Migrate mysql parse_value_and_set_cell / decode_text / decode_binary to the named constructors; delete duplicate clone_wtf_string_or_null helper
• Fix decode_binary NULL cell index/is_indexed_column; drop redundant count param from to_js_object
• Migrate decode_binary_value to the named constructors; delete third clone_utf8_wtf_impl helper

Verification

• cargo check -p bun_sql_jsc and cargo clippy --no-deps: clean
• sql-mysql-binary-null-indexed.test.ts (new): fails on main ({0: null, 2: 42}), passes with fix ({2: 42, 5: null, 7: null})
• 30/30 mock-server decode tests pass: postgres-binary-numeric, postgres-binary-array-bounds, sql-mysql-mediumint, sql-mysql-raw-length-prefix, postgres-multi-statement-fields
• sql-mysql-datetime-roundtrip mock-server TZ variants pass (text-protocol DATE/DATETIME path)

Noted (pre-existing, not introduced)

• Array::allocated_slice() (SQLDataCell.rs:123) has no callers; the diff added a CAUTION comment about its uninit-spare-capacity hazard rather than deleting it
• 4 pre-existing TODO comments in the touched files preserved verbatim

[robobun]

^{Updated 8:47 AM PT - Jun 16th, 2026}

❌ @robobun, your commit 03acefd has 1 failures in Build #62757 (All Failures):

• test/js/web/fetch/fetch-leak.test.ts - code 1 on 🍎 14 aarch64

---

🧪   To try this PR locally:

bunx bun-pr 32367

That installs a local version of the PR into your bun-32367 executable, so you can run:

bun-32367 --bun

[coderabbitai]

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

Introduces typed constructor methods (null, int4, uint4, int8, uint8, float8, bool_, date, date_with_tz, string, json) on SQLDataCell, along with a to_js_object wrapper and internal clone_utf8_or_null helper. Postgres array parsing and scalar decoding are refactored to use these constructors throughout, eliminating manual tag/Value struct literals. MySQL ResultSet and binary protocol decoder integrate the new to_js_object method and refactor value decoding and cell initialization to use constructors. MySQL integer validation is refactored into generic int_range/validate_int/validate_bigint helpers. A regression test verifies correct NULL placement in digit-named columns under the binary protocol.

Changes

SQLDataCell constructor API and SQL value decoding refactor

Layer / File(s)	Summary
SQLDataCell constructor helpers and to_js_object src/sql_jsc/shared/SQLDataCell.rs	Adds #[inline] constructors for all scalar types (null, int4, uint4, int8, uint8, float8, bool_, date, date_with_tz) and owned string-like cells (string, json; both set free_value = 1). Introduces internal clone_utf8_or_null for UTF-8 cloning. Adds public to_js_object that extracts cached column names from CachedStructure and delegates to construct_object_from_data_cell. Updates raw() empty path to return SQLDataCell::null().
Postgres parse_array refactored to use constructors src/sql_jsc/postgres/DataCell.rs	All element pushes in parse_array replace manual Tag/Value struct literals with SQLDataCell::date, ::json, ::string, ::null, ::bool_, ::float8, ::int8, ::int4, and ::uint4 helpers. Array::default() replaces explicit {ptr, len, cap} construction for empty arrays. Special-case handling for "NULL", "NaN", "Infinity", "true", and "false" strings simplified to helper-based construction.
Postgres from_bytes and Putter wiring refactored src/sql_jsc/postgres/DataCell.rs	All from_bytes arms (int2, int4, int8, float8, float4, numeric, jsonb/json, bool, date/timestamp/timestamptz, time/timetz, fallback) return SQLDataCell constructors instead of manual struct literals. Putter::to_js calls SQLDataCell::to_js_object with self.list and field count directly. Putter::put_impl uses SQLDataCell::raw(as_deref()) and SQLDataCell::null().
MySQL ResultSet Row and value decoding integration src/sql_jsc/mysql/protocol/ResultSet.rs	Row::to_js computes cell count and calls SQLDataCell::to_js_object directly, passing self.values and cached_structure. parse_value_and_set_cell refactored to use SQLDataCell constructors for all type conversions. decode_text and decode_binary cell initialization and null-handling simplified to use SQLDataCell::null(). Imports updated to remove ExternColumnIdentifier and local clone_wtf_string_or_null helper removed.
MySQL binary protocol value decoder refactored src/sql_jsc/mysql/protocol/DecodeBinaryValue.rs	decode_binary_value uses SQLDataCell constructor helpers (uint4, int4, int8, uint8, float8, string, date, json, bool_, raw) instead of manual CellTag/CellValue construction. 64-bit integer handling range-checks to 32-bit or emits 64-bit when bigint is set, otherwise encodes out-of-range values as decimal strings. TIME/DATE/TIMESTAMP and enum/set/string/blob arms use new constructors. Removes private clone_utf8_wtf_impl helper.
MySQL integer validation helpers and from_js refactor src/sql_jsc/mysql/MySQLValue.rs	Adds private int_range, validate_int, and validate_bigint generics that derive min/max bounds from the target Rust integer type and map JS range/type errors to MySQL errors. Value::from_js match arms for MYSQL_TYPE_SHORT, MYSQL_TYPE_LONG, and MYSQL_TYPE_LONGLONG call these helpers, removing duplicated inline range specifications and hardcoded min/max values.
Regression test for MySQL binary NULL column mapping test/js/sql/sql-mysql-binary-null-indexed.test.ts	Adds end-to-end integration test with a real MySQL/MariaDB server (Docker container or local unix socket/TCP provisioning). Verifies that NULL values in digit-named columns are placed at correct indices under both binary-protocol prepared queries and simple-protocol text paths, catching the bug where NULL could be mis-assigned to index 0.

Possibly related PRs

• oven-sh/bun#31513: Modifies MySQL decoding logic for NEWDECIMAL and string-vs-raw output by refactoring parse_value_and_set_cell in src/sql_jsc/mysql/protocol/ResultSet.rs and decode_binary_value in src/sql_jsc/mysql/protocol/DecodeBinaryValue.rs.
• oven-sh/bun#31437: Adds MYSQL_TYPE_YEAR handling to MySQL decoding in src/sql_jsc/mysql/protocol/DecodeBinaryValue.rs and mirrors it in ResultSet.rs.
• oven-sh/bun#31007: Addresses MySQL binary-protocol INT24/MEDIUMINT handling in src/sql_jsc/mysql/protocol/DecodeBinaryValue.rs.

Suggested reviewers

• cirospaciari

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely summarizes the main change: adding named constructors for SQLDataCell and replacing hand-rolled struct literals across row decoders.
Description check	✅ Passed	The description comprehensively covers what the PR does, how verification was performed, and documents a pre-existing bug fix, following the template structure.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[alii]

@robobun adopt

[robobun]

Adopted. All review findings addressed; HEAD is 03acefd.

6785a4e fixes a pre-existing bug the refactor surfaced: decode_binary skipped index/is_indexed_column for NULL cells, so a NULL on a digit-named column over the binary protocol landed at slot 0. test/js/sql/sql-mysql-binary-null-indexed.test.ts runs against a real MySQL server via describeWithContainer and fails on main ({0: null, 2: 42}), passes with the fix ({2: 42, 5: null, 7: null}).

50739b9 completes the constructor migration in DecodeBinaryValue.rs. No hand-rolled SQLDataCell {} literals left in src/sql_jsc/mysql/. Net −294 lines of src.

03acefd drops the custom mysqld_safe harness; both MySQL tests now use describeWithContainer directly, matching sql-mysql.helpers.test.ts et al.

cargo check / clippy clean; all postgres + mysql decode tests pass.

[claude]

All prior feedback addressed (including the decode_binary NULL-index fix + regression test) and nothing new from this pass — deferring only because this is a ~600-line change across the per-row decode hot path for both drivers plus a behavioral fix, which is more than I'll auto-approve.

Extended reasoning...

Overview

This PR adds 12 #[inline] named constructors on SQLDataCell and replaces ~110 hand-rolled SQLDataCell { tag: ..., value: ..., ..Default::default() } literals across src/sql_jsc/postgres/DataCell.rs and src/sql_jsc/mysql/protocol/ResultSet.rs. It also adds a shared to_js_object wrapper used by both Putter::to_js and Row::to_js, refactors the MYSQL_TYPE_{SHORT,LONG,LONGLONG} arms of Value::from_js into generic validate_int/validate_bigint helpers driven by bun_core::Integer, and — going beyond pure refactoring — fixes a pre-existing bug where decode_binary's is_null branch continue;d past the index/is_indexed_column assignments, with a new mock-server regression test (test/js/sql/sql-mysql-binary-null-indexed.test.ts). Net −327 lines.

Security risks

None identified. No auth, crypto, permissions, or untrusted-input parsing surface is changed. The refactor preserves free_value semantics (every replaced Tag::String/Tag::Json literal already set free_value: 1, matching string()/json()), and to_js_object now derives the cell count from the slice length, eliminating a (ptr, separate-len) footgun at the FFI boundary rather than introducing one.

Level of scrutiny

Medium-high. The bulk of the diff is mechanical and individually verifiable, but it sits in the per-row decode hot path that every Postgres/MySQL result flows through, the cells cross the Rust↔C++ FFI boundary into JSC__constructObjectFromDataCell, and one hunk (decode_binary) is a real control-flow / behavioral change rather than a literal-for-constructor swap. That combination puts it past my auto-approve threshold even though I found nothing wrong with it.

Other factors

I've iterated on this PR three times; every comment (the leftover .as_deref(), the unmigrated mysql Row::to_js, the duplicate clone_wtf_string_or_null, the redundant count param, and the pre-existing decode_binary NULL-cell index bug) has been addressed and resolved, the last with a dedicated test. The current bug-hunting pass came back clean. CodeRabbit also had no actionable comments. No CODEOWNERS entry covers src/sql_jsc/. The MySQLValue.rs helper refactor relies on T::MIN_I128/T::MAX_I128/T::ZERO reproducing the previously hard-coded bounds (including min: 0 for u64), which holds for bun_core::Integer, but is one more thing a human glance would confirm.

[claude]

All five of my earlier review points are addressed and I found nothing new on 50739b9 — the constructor migrations look field-for-field equivalent and the decode_binary NULL-index fix is covered by the new test — but given the size (~600 lines across the row-decode hot path, FFI-adjacent #[repr(C)] struct) plus the one real behavioral change, this is worth a quick human pass before merge.

Extended reasoning...

Overview

This PR adds 12 named SQLDataCell constructors (null, int4, uint4, int8, uint8, float8, bool_, date, date_with_tz, string, json, plus a shared to_js_object wrapper) and replaces ~110 hand-rolled SQLDataCell { tag, value, free_value, ..Default::default() } literals across src/sql_jsc/postgres/DataCell.rs, src/sql_jsc/mysql/protocol/ResultSet.rs, and src/sql_jsc/mysql/protocol/DecodeBinaryValue.rs. It also extracts validate_int/validate_bigint generics in MySQLValue.rs, and fixes a pre-existing bug in decode_binary where the is_null branch continue;d before assigning index/is_indexed_column, with a new mock-server regression test.

I reviewed this across five iterations; every point I raised (.as_deref() leftover, to_js_object second caller, duplicate clone_wtf_string_or_null, redundant count param, unmigrated DecodeBinaryValue.rs) was addressed in 53e4076..50739b9, and the bug-hunting system found nothing on the final commit.

Security risks

None identified. The changes are internal to SQL row decoding; no auth, crypto, permissions, or untrusted-input parsing surface is altered. The #[repr(C)] layout of SQLDataCell is unchanged (constructors use ..Default::default() exactly as the literals did), so the FFI boundary with SQLClient.cpp is preserved.

Level of scrutiny

Moderate-to-high. ~95% of the diff is purely mechanical (literal → constructor) and I spot-checked every constructor against the literals it replaces — including the few sites where the old code skipped the is_empty() check (numeric, time/timetz, "00:00:00", b"\x08") and confirmed the new null-pointer-on-empty path is either unreachable or renders identically in C++. But this is the per-row decode hot path for both Postgres and MySQL, the touched struct crosses FFI to C++, and the decode_binary restructuring is a real control-flow change (not a rename). That combination puts it outside the "obvious mechanical change" bucket I'd auto-approve.

Other factors

• All inline review threads are resolved; no outstanding human reviewer comments.
• PR description reports cargo check/clippy clean and 37 mock-server decode tests passing; the new sql-mysql-binary-null-indexed.test.ts exercises both protocols.
• The MySQLValue.rs int_range<T> refactor relies on bun_core::Integer::{MIN_I128, MAX_I128, ZERO} matching the previously hard-coded bounds; that's straightforward but worth a glance from someone familiar with that trait.
• coderabbit suggested cirospaciari as reviewer, which seems right given the area.

[claude]

All prior review feedback addressed (07d66dc extracts the shared ensureLocalMySQL harness — thanks). Nothing further from me; given this rewrites ~135 sites on the row-decode hot path across both SQL backends and folds in the decode_binary NULL-index behavioral fix, worth a human pass before merge.

Extended reasoning...

Overview

This PR adds 12 named SQLDataCell constructors and replaces ~135 hand-rolled #[repr(C)] struct literals across the Postgres and MySQL row decoders (postgres/DataCell.rs, mysql/protocol/ResultSet.rs, mysql/protocol/DecodeBinaryValue.rs), adds a shared to_js_object wrapper, consolidates three duplicate clone-utf8 helpers into one, refactors MySQLValue.rs integer validation into generic validate_int/validate_bigint helpers backed by bun_core::Integer::{MIN_I128, MAX_I128, ZERO}, fixes a pre-existing decode_binary bug where NULL cells skipped index/is_indexed_column assignment, adds a real-MySQL regression test for that fix, and extracts a shared mysql-local-harness.ts for the two non-docker MySQL tests. Net −294 lines of src across 8 files.

Security risks

None identified. The change is internal to SQL result decoding; no new untrusted-input parsing surface, no auth/crypto/permissions, no new FFI signatures. The #[repr(C)] SQLDataCell layout and Default impl are unchanged, so the C++ ABI contract with JSC__constructObjectFromDataCell (SQLClient.cpp) is preserved.

Level of scrutiny

High. Every Postgres and MySQL result row flows through these decoders, the cells cross FFI to C++ for object construction, and the PR bundles a behavioral fix (decode_binary NULL placement) alongside the refactor. I spot-checked the constructors against the literals they replace (tag, active union member, free_value) and verified the bun_core::Integer trait constants (<$t>::MIN as i128 / <$t>::MAX as i128 / 0) match the hardcoded IntegerRange bounds they replace in MySQLValue.rs — all semantically identical. But the surface area is large enough that a maintainer who owns this path should sign off.

Other factors

Six rounds of bot review feedback have all been applied (the last, extracting ensureLocalMySQL, landed in 07d66dc — HEAD). The bug-hunting system found nothing on the current state. The behavioral fix has a regression test against real MySQL/MariaDB that fails on main and passes here. No CODEOWNERS entry covers src/sql_jsc/. alii has been engaged on the PR (adopted it, requested the real-server test) but hasn't yet reviewed the final state.