sql_jsc: extract shared query/connection ctor-arg parsing across postgres and mysql#32135
Conversation
…Message a payload() accessor
…nt status across drivers
|
Updated 2:40 PM PT - Jun 11th, 2026
✅ @alii, your commit 4507f95d664ad7f7d0b99a190a393f3a39858762 passed in 🧪 To try this PR locally: bunx bun-pr 32135That installs a local version of the PR into your bun-32135 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
💤 Files with no reviewable changes (3)
WalkthroughThis PR refactors the SQL module infrastructure by consolidating Postgres protocol handling, extracting shared Status types, introducing centralized constructor argument parsing for MySQL and Postgres drivers, and delegating column deduplication logic to a shared helper function. ChangesSQL Module Refactoring
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@src/sql_jsc/lib.rs`:
- Around line 23-24: The module declarations connection_ctor_args and
query_ctor_args break the existing PascalCase filename convention described in
the file comment; fix by making the module names match PascalCase filenames
(e.g., rename files to ConnectionCtorArgs.rs and QueryCtorArgs.rs) and update
the module declarations to use #[path = "ConnectionCtorArgs.rs"] pub mod
ConnectionCtorArgs; and #[path = "QueryCtorArgs.rs"] pub mod QueryCtorArgs; so
they are consistent with the other modules (or alternatively update the file
comment to explicitly document these snake_case exceptions if you intentionally
want to keep the current filenames).
In `@test/js/sql/postgres-multi-statement-fields.test.ts`:
- Around line 139-141: Update the test comment to clarify that the decoding
divergence occurs for declared lengths below 4 (not equal to 4): note that a
declared length of exactly 4 is treated as empty by both
ErrorResponse::decode_internal and NoticeResponse::decode_notice_internal, while
lengths < 4 diverge because ErrorResponse::decode_internal returns
Err(InvalidMessageLength) whereas NoticeResponse::decode_notice_internal uses
saturating_sub(4) and yields an empty notice; retain the test case ("empty
NoticeResponse", pkt("N", Buffer.alloc(0))) but change the wording to reflect
this precise distinction.
🪄 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: 23533595-b673-41c0-acd1-37632c57b28c
📒 Files selected for processing (20)
src/sql/lib.rssrc/sql/postgres/protocol/ErrorResponse.rssrc/sql/postgres/protocol/FieldMessage.rssrc/sql/postgres/protocol/NoticeResponse.rssrc/sql/shared/QueryStatus.rssrc/sql/shared/StatementStatus.rssrc/sql_jsc/lib.rssrc/sql_jsc/mysql/JSMySQLConnection.rssrc/sql_jsc/mysql/JSMySQLQuery.rssrc/sql_jsc/mysql/MySQLStatement.rssrc/sql_jsc/postgres.rssrc/sql_jsc/postgres/PostgresSQLConnection.rssrc/sql_jsc/postgres/PostgresSQLQuery.rssrc/sql_jsc/postgres/PostgresSQLStatement.rssrc/sql_jsc/postgres/protocol/error_response_jsc.rssrc/sql_jsc/postgres/protocol/notice_response_jsc.rssrc/sql_jsc/shared/SQLDataCell.rssrc/sql_jsc/shared/connection_ctor_args.rssrc/sql_jsc/shared/query_ctor_args.rstest/js/sql/postgres-multi-statement-fields.test.ts
💤 Files with no reviewable changes (2)
- src/sql_jsc/postgres/protocol/notice_response_jsc.rs
- src/sql_jsc/postgres.rs
- rename ctor_args files to PascalCase per the documented sql_jsc convention - drop StatementStatus::is_running() (zero callers; sites already compare == Parsing) - drop misleading "unlike ErrorResponse" clause from empty-notice test comment
|
@claude review |
There was a problem hiding this comment.
I didn't find any correctness issues — the extracted helpers look behaviorally equivalent to the per-driver code they replace — but this is a 20-file refactor touching SSL_CTX lifecycle and connection setup for both drivers, and CI is currently red on 4507f95, so I'd like a human to sign off.
Extended reasoning...
Overview
This PR deduplicates the createQuery / createConnection argument-parsing prologues shared between the Postgres and MySQL Rust drivers into src/sql_jsc/shared/{QueryCtorArgs,ConnectionCtorArgs}.rs, extracts the duplicated dedupe_columns loop into SQLDataCell.rs, promotes QueryStatus/StatementStatus to bun_sql::shared, collapses NoticeResponse into a type alias of ErrorResponse (with decode_notice_internal), and adds a FieldMessage::payload() accessor. Net −58 lines across 20 files; new mock-server test cases cover the rewired NoticeResponse path.
Security risks
ConnectionCtorArgs::parse and guard_tls centralize the TLS config parsing, per-VM SSL_CTX* cache lookup, and the scopeguard that calls SSL_CTX_free on early exit. I traced the new code against both removed prologues and it preserves the same ssl_mode decode (negative/≥5 → Disable, 0–4 indexed), the same as_usockets_for_client_verification() cache key, the same error message, and the same ScopeGuard::into_inner disarm point at both callers — so I don't see a leak, double-free, or behavior change. That said, this is unsafe FFI to BoringSSL on a security-sensitive path, so it merits a second pair of eyes.
Level of scrutiny
Medium-high. The PR description is unusually thorough about behavioral equivalence (verbatim error messages, validation order, sql_vm() ≡ bun_vm(), .unwrap_or_oom() semantics), and my spot-checks of QueryCtorArgs, dedupe_columns (reverse iteration via .iter_mut().rev() matching the old decrementing while-loop), and the SSLConfig::from_js Ok/Err handling all line up. But it's a cross-cutting refactor of connection/query construction for two production database drivers, not a config tweak.
Other factors
- robobun reports failures on Build #61944 for the latest commit (4507f95); I'd want CI green before merge regardless.
- alii has already been reviewing — both inline threads (
self.0onFieldMessage,is_runningonStatementStatus) are resolved, and 4507f95 droppedis_runningfrom the shared enum per that feedback. The CodeRabbit naming-convention nit was also addressed in 4507f95. - Given an active human reviewer is already engaged and the change touches TLS lifecycle + unsafe code, deferring rather than auto-approving.
## What this does Third (and last Rust) slice from #31994, on top of #32128 and #32135. Consolidates the per-driver `StackReader` cursor type into `src/sql/shared/StackReader.rs`. Net −29 lines across 6 files. `StackReader` is the cursor-over-borrowed-buffer that both drivers' `NewReader` types wrap on the per-row decode path. The MySQL and Postgres copies were near-identical; the only per-driver bits were the error variant returned on a short read and the `NewReader<C>` wrapper type. Those become two small traits (`ShortRead`, `WrapReader`) that each driver implements in ~10 lines; the per-driver `StackReader.rs` files are now those impls plus a re-export. Also drops two dead items from `mysql/protocol/NewReader.rs`: the `NewReaderOf<C>` type alias (= `NewReader<C>`, zero callers) and `Decode::decode_allocator` (body identical to `decode`, zero callers). ## Behavioral equivalence - MySQL: byte-for-byte semantic match. Old already used `&Cell<usize>`; `skip` negative-count branch via `saturating_sub` is identical to the old explicit clamp. - Postgres: cursor was `&mut usize`; new shared uses `&Cell<usize>` for both. `IntoCursor for &mut usize` is `Cell::from_mut` (a `repr(transparent)` pointer cast), so the existing `&mut consumed`/`&mut offset` call site at `PostgresSQLConnection.rs:982` is unchanged and reads back the same locals after the reader is consumed. - `ShortRead::SHORT_READ` monomorphizes to the literal `AnyMySQLError::ShortRead` / `AnyPostgresError::ShortRead` — identical `Err` values to before. Two strictly-safer deltas on the postgres side, both unreachable on real wire data but the correct direction: - `ensure_length`/`ensure_capacity` use `checked_add` where the old `buffer.len() >= offset + length` could overflow-wrap in release on adversarial declared lengths. - `skip(count > isize::MAX)` clamps to buffer end instead of overflow-wrapping. ## Performance No new dynamic dispatch or boxing — `ShortRead`/`WrapReader`/`IntoCursor` are all generic-bound. `Cell::get/set` on `usize` compiles to plain load/store. The per-row hot path (`DataRow` → `NewReader::int` → `wrapped.read(N)`) is slightly leaner: shared `read` advances the cursor with `offset.set(offset + count)` instead of routing through `skip()` with its redundant bounds re-check. The one added `isize::try_from().unwrap_or()` in the postgres `skip` bridge is not on the row path (`skip` is called only from `FieldDescription` once per column definition and `Authentication` once per connection). ## Verification - `cargo check -p bun_sql -p bun_sql_jsc` and `cargo clippy --no-deps`: clean. - `cargo fmt`: no diffs. - Zero callers of `NewReaderOf` / `decode_allocator` via grep. ## Left for a follow-up The `src/js/internal/sql/shared.ts` JS adapter consolidation from #31994 (`BasePooledConnection`, +1308/−2273) — the higher-risk pool/lifecycle piece.
…n in shared.ts (#32145) ## What this does Final slice from #31994 (closed as too big), on top of #32128, #32135, #32141. Consolidates the JS adapters' duplicated pool/connection/query plumbing from `src/js/internal/sql/{postgres,mysql,sqlite}.ts` into `src/js/internal/sql/shared.ts` as a `BasePooledConnection` class hierarchy. Net −995 lines across 4 files. Two commits: - **Cherry-pick from #31994** (1aa3dbb) — checks out `src/js/internal/sql/` from `origin/claude/split/sql` as-is. This intentionally clobbers two pool-lifecycle fixes that landed on `main` since #31994's merge-base: #32041 (throwing onconnect/onclose corrupting the pool) and #32097 (forced `close()` resolving mid-handshake). - **Re-integrate #32041 + #32097 into the new structure** (6e6fd4f) — re-hosts both fixes in `BasePooledConnection`: - #32041: try/finally around the user `onconnect`/`onclose` calls in `handleConnected` and `#finishClose` so a throwing callback never skips pool bookkeeping; the `createPooledConnectionHandle` catch always defers via `process.nextTick` (drops the per-driver `deferSyncCloseError` flag — both drivers now match). - #32097: `startConnection()` is `abstract Promise<void>` and both subclasses await + assign `this.connection` at creation; `#beginConnecting` awaits it and closes the handle if `onFinish` was set (pool force-closed) in the microtask window before it materialized. ## Behavioral equivalence The original `BasePooledConnection` extraction in #31994 carried a per-file behavioral-equivalence audit against its merge-base (placeholders, escaping, helper commands, error message text, pool/transaction semantics all preserved) and was green on full CI build 61383. This PR re-applies that diff and adds the two missing fixes; the load-bearing verification is below. ## Verification - `bun bd`: builds clean. - The three lifecycle test files (`sql-onconnect-onclose-throw.test.ts`, `sql-close-pending-connection.test.ts`, `sql-connect-error-reporting.test.ts`): 26 pass / 0 fail. - **New tests in this PR**: `sql-onconnect-onclose-throw.test.ts` gains a forced-close variant per driver (a throwing `onclose` while the pool is force-closed mid-handshake, against a fake `net` server). The two re-hosted fixes meet in `BasePooledConnection`'s close handler, and that interaction had no coverage. Note these pass against `origin/main`'s `src/js/internal/sql` too (verified): both underlying fixes already live on main per-driver, so versus main this PR is equivalence-preserving by design and no test can fail on one side only. The HEAD~1 stash test below is the proof that the re-integration commit is load-bearing. - **Load-bearing stash test**: built and ran the same three files at HEAD~1 (the cherry-pick before re-integration) — 6 fail (`mysql: forced close() resolves when called before the native handle is stored` timeout; `postgres: pool calls from onclose are safe when connecting fails synchronously` → `reentry threw: TypeError`; both drivers' `throwing onclose still rejects pending queries on connect refused` timeouts). With the re-integration: 26 pass. The diff is required for both fixes. - Full `test/js/sql/` against live `postgres_plain`/`mysql_plain`/`*_tls` containers: 1526 pass / 7 fail / 6 errors locally. Every failure reproduces on `origin/main` in the same environment — see Notes below. ## Notes on local-only test failures (none introduced by this PR) - `sql-mysql-query-string-leak.test.ts` fails locally on a debug+ASAN build for both `origin/main` (314.6 MiB) and this branch (317.2 MiB), within noise of each other; both exceed the 256 MiB ASAN threshold. A `heapStats()` probe of the same workload shows `MySQLQuery 2→1` after GC — wrappers are finalized correctly. The test passes on `main`'s CI (release build); the threshold is tuned for release RSS overhead, not local debug. - `sql.test.ts > query string memory leak test` (postgres): same category. - `sqlite-sql.test.ts > Query Normalization Fuzzing Tests > handles exotic but valid SQL patterns`: 5.5s timeout on debug builds — pre-existing per #31994's original verification notes. - `describeWithContainer` cold-start races: `docker compose up -d --wait` returns non-zero on an already-healthy container (`test/docker/index.ts:157`), causing beforeEach hook timeouts in `sql-mysql.test.ts` (TLS), `sql-mysql-bind-oob`, `sql-mysql.helpers`, `sql-mysql.auth`. Pre-existing harness flake; clears on warm rerun. The Rust-side changes from #32097 (`src/sql_jsc/`) were already on `main` before this PR — only the JS hooks needed re-integrating.
What this does
Second slice carved out of #31994 (closed as too big), stacked on #32128 (merged). This one is the
src/sql_jsc/Rust constructor-args dedup — no JS adapter changes. Net −58 lines across 20 files.Three commits:
src/sql/prerequisites (46d3fb8) —QueryStatusmoves frommysql/toshared/(re-exported under the oldmysql::query_statuspath so nothing breaks); newshared/StatementStatuswith the same 4 variants both drivers already had;FieldMessagegains apayload()accessor so the only caller no longer needs an external exhaustive match;NoticeResponsebecomes a type alias ofErrorResponsedecoded viadecode_notice_internal(same wire format, body identical to the oldNoticeResponse::decode_internal).src/sql_jsc/dedup (dc96ffd) — the headline change. Extracts the duplicatedcreateQuery(...)andcreateConnection(...)argument parsing/validation prologue fromJSMySQLQuery/PostgresSQLQueryandJSMySQLConnection/PostgresSQLConnectionintosrc/sql_jsc/shared/{query_ctor_args,connection_ctor_args}.rs. Also moves the duplicateddedupe_columnsfromMySQLStatement/PostgresSQLStatementintoshared/SQLDataCell.rs, switches both Statement types to the sharedStatementStatus, and drops the now-unusednotice_response_jsc.rs(replaced byFieldMessage::payload()).NoticeResponseand degenerate empty-notice cases to the existing async-message framing test intest/js/sql/postgres-multi-statement-fields.test.ts. This path had no prior coverage intest/js/sql.Behavioral equivalence
Line-by-line diff of every removed per-driver block against the new shared helper:
query_ctor_argserror messages verbatim ("query must be a string","values must be an array","simple query cannot have parameters","query is too long",throw_invalid_argument_type("query", "pendingValue", "Array")); validation order unchanged.connection_ctor_argsvalidation order unchanged;ssl_modedecode equivalent for all i32 (negative → Disable, 0–4 → indexed, ≥5 → Disable);tlserror message verbatim; same per-VMSSL_CTX*cache andas_usockets_for_client_verification()key; sameSSL_CTX_freesymbol.dedupe_columnsiteration order and reserve size identical..expect("OOM")→.unwrap_or_oom()routes throughbun_core::handle_oomper repo convention — same crash on OOM, no observable difference otherwise.bun_vm()vssql_vm(): the latter is#[inline] fn sql_vm(&self) { self.bun_vm() }.No user-reachable input produces a different output, error message, validation order, or SSL_CTX cache key.
Verification
cargo check -p bun_sql -p bun_sql_jscandcargo clippy --no-deps: clean, zero warnings.cargo fmt: no diffs.bun bd testmock-server SQL suite (postgres-binary-numeric,postgres-binary-array-bounds,postgres-multi-statement-fields,sql-mysql-auth-short-nonce,sql-connect-error-reporting): 41 pass / 0 fail (39 prior + 2 new NoticeResponse cases).mainhave touchedsrc/sql_jsc/since Deduplicate SQL driver internals across postgres/mysql/sqlite #31994's merge-base, so the per-driver files are a clean checkout with no clobbered work.Left for a follow-up
The
src/js/internal/sql/shared.tsJS adapter consolidation from #31994 (theBasePooledConnectionextraction, +1308/−2273) is intentionally not in this PR — that's the higher-risk piece touching pool/connection lifecycle and the part that conflicted with #32028.