sql: deduplicate and remove dead code across the MySQL/Postgres drivers#31664
Closed
alii wants to merge 11 commits into
Closed
sql: deduplicate and remove dead code across the MySQL/Postgres drivers#31664alii wants to merge 11 commits into
alii wants to merge 11 commits into
Conversation
Collaborator
This comment was marked as off-topic.
This comment was marked as off-topic.
Member
Author
|
@robobun fix review comments |
Collaborator
|
All review rounds addressed — every thread replied & resolved, full equivalence audit pinned in the PR description, and the two deliberate Postgres error-shape deltas are locked in by tests that fail on CI state: the only recurring red is macOS lane infrastructure — Buildkite agents repeatedly show Expired (never picking jobs up), and the one x64 run that executed failed only on pre-existing docker-gated files ( @alii has since merged Summary of what landed across the rounds
|
Contributor
WalkthroughThis PR centralizes SQL connection and query argument parsing into shared modules ( ChangesSQL JSC Refactor: Shared Parsing and Protocol Unification
Possibly related PRs
Suggested reviewers
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
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/mysql/JSMySQLConnection.rs (1)
481-526:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winReject negative timeout values instead of panicking.
connection_args::parse()preserves these as signedi32s, so a JS caller can pass-1and hit theexpect("int cast")on Lines 524-526. That turns invalid user input into a runtime panic in core code; validate and throw before disarmingargs.tls.As per coding guidelines, `src/**/*.rs`: Do not use `.unwrap()` on fallible paths that user input or the OS can hit at runtime — return the error instead. `.unwrap()` is only for invariants you can prove.Suggested fix
let Some(args) = connection_args::parse::<SSLMode>(vm, global_object, callframe)? else { return Ok(JSValue::ZERO); }; + let idle_timeout_interval_ms = u32::try_from(args.idle_timeout).map_err(|_| { + global_object.throw_invalid_arguments(format_args!("idleTimeout must be >= 0")) + })?; + let connection_timeout_ms = u32::try_from(args.connection_timeout).map_err(|_| { + global_object.throw_invalid_arguments(format_args!("connectionTimeout must be >= 0")) + })?; + let max_lifetime_interval_ms = u32::try_from(args.max_lifetime).map_err(|_| { + global_object.throw_invalid_arguments(format_args!("maxLifetime must be >= 0")) + })?; ... - idle_timeout_interval_ms: u32::try_from(args.idle_timeout).expect("int cast"), - connection_timeout_ms: u32::try_from(args.connection_timeout).expect("int cast"), - max_lifetime_interval_ms: u32::try_from(args.max_lifetime).expect("int cast"), + idle_timeout_interval_ms, + connection_timeout_ms, + max_lifetime_interval_ms,🤖 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/mysql/JSMySQLConnection.rs` around lines 481 - 526, The code currently converts signed timeout fields (args.idle_timeout, args.connection_timeout, args.max_lifetime) to u32 with expect(...) which panics on negative input; instead validate these three fields are >= 0 and fit into u32 before calling scopeguard::ScopeGuard::into_inner(args.tls) (i.e. before disarming TLS ownership), and if any are invalid return a JS exception/Err (propagate the parse/validation error) rather than panicking; replace the u32::try_from(...).expect(...) calls in the JSMySQLConnection construction with safe checked conversions that produce an early return with a descriptive error when a negative value is provided.
🤖 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/mysql/JSMySQLQuery.rs`:
- Line 100: The shared parser query_args::parse is making pending_value
effectively required by unwrapping to JSValue::UNDEFINED and then rejecting
non-array-like values; change the parser so it preserves whether the third
argument was actually provided (e.g., return Option<JSValue> or a flag) and only
perform the "must be array-like" validation when the caller actually passed a
third argument, leaving omitted pendingValue as undefined for callers like
JSMySQLQuery construction that rely on the default behavior; update any call
sites (e.g., the call in JSMySQLQuery.rs that does let args =
query_args::parse(global_this, callframe)?) to handle the new optional/flag
return so omitted pendingValue continues to behave as before.
In `@src/sql_jsc/postgres/DataCell.rs`:
- Around line 779-787: The T::int8 branch currently ignores the binary flag and
always treats the payload as text or raw bytes; update the T::int8 match to
first check if binary is true and, if so, decode the 8-byte wire format into an
i64 (use an explicit signed 8-byte conversion from the bytes buffer) and return
SQLDataCell::int8(parsed_i64); otherwise keep the existing text-path behavior
that uses bigint/text handling and bun_core::fmt::parse_decimal::<i64>(bytes) or
SQLDataCell::string(bytes). Ensure you reference and modify the T::int8 branch
where bigint, binary, SQLDataCell::int8, and bun_core::fmt::parse_decimal are
used.
In `@src/sql_jsc/postgres/PostgresSQLQuery.rs`:
- Around line 774-777: In do_run(), the error path that handles
writer.write(&protocol::SYNC) returns without releasing the speculative query
ref taken earlier; ensure you call release_query_ref() before
remove_statements_entry()/drop(signature)/return Err(...) so the extra
this.ref_() from Line 524 is released and prepared queries aren’t pinned—i.e.,
add a release_query_ref() call in the Err(err) branch that currently calls
remove_statements_entry() and drop(signature) before returning the
throw_write_error.
In `@src/sql_jsc/shared/CachedStructure.rs`:
- Around line 32-35: The code calls
seen_fields.get_or_put(name.slice()).expect("OOM") which can panic on
allocation; replace this with the repo's OOM trap by using the provided helper
(e.g., call .unwrap_or_oom() on the Result or invoke bun_core::handle_oom) so
the allocation failure triggers the controlled crash path instead of unwinding
through the SQL/JSC boundary; update the expression around
seen_fields.get_or_put and ensure the resulting value uses the
unwrap_or_oom/handle_oom variant instead of expect("OOM").
In `@src/sql_jsc/shared/connection_args.rs`:
- Around line 97-101: Move the existing SAFETY comment so it immediately
precedes the unsafe block that calls ssl_ptr.as_mut() to satisfy Clippy;
specifically, relocate the comment about the native handle of a connected TLS
socket being `SSL*` and living for the duration of the handshake callback to
directly above the unsafe { ssl_ptr.as_mut() } expression (referencing ssl_ptr
and the unsafe block that yields ssl) so the safety rationale is attached to
that unsafe operation.
In `@src/sql_jsc/shared/query_args.rs`:
- Around line 44-62: The code currently treats a missing pendingValue (default
JSValue::UNDEFINED) as an error because it unconditionally checks
pending_value.js_type().is_array_like(); update the validation in the shared
createInstance path to allow undefined by only rejecting when pending_value is
not undefined and also not array-like (i.e., change the condition to skip the
array-like check if pending_value is JSValue::UNDEFINED). Reference:
pending_value, JSValue::UNDEFINED, createInstance.
---
Outside diff comments:
In `@src/sql_jsc/mysql/JSMySQLConnection.rs`:
- Around line 481-526: The code currently converts signed timeout fields
(args.idle_timeout, args.connection_timeout, args.max_lifetime) to u32 with
expect(...) which panics on negative input; instead validate these three fields
are >= 0 and fit into u32 before calling
scopeguard::ScopeGuard::into_inner(args.tls) (i.e. before disarming TLS
ownership), and if any are invalid return a JS exception/Err (propagate the
parse/validation error) rather than panicking; replace the
u32::try_from(...).expect(...) calls in the JSMySQLConnection construction with
safe checked conversions that produce an early return with a descriptive error
when a negative value is provided.
🪄 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: b55b9eef-489c-41b0-b8a8-3d71e625f14a
📒 Files selected for processing (33)
src/sql_jsc/jsc.rssrc/sql_jsc/lib.rssrc/sql_jsc/mysql/JSMySQLConnection.rssrc/sql_jsc/mysql/JSMySQLQuery.rssrc/sql_jsc/mysql/MySQLConnection.rssrc/sql_jsc/mysql/MySQLContext.rssrc/sql_jsc/mysql/MySQLQuery.rssrc/sql_jsc/mysql/MySQLStatement.rssrc/sql_jsc/mysql/MySQLValue.rssrc/sql_jsc/mysql/protocol/DecodeBinaryValue.rssrc/sql_jsc/mysql/protocol/ResultSet.rssrc/sql_jsc/mysql/protocol/Signature.rssrc/sql_jsc/mysql/protocol/any_mysql_error_jsc.rssrc/sql_jsc/postgres.rssrc/sql_jsc/postgres/DataCell.rssrc/sql_jsc/postgres/PostgresRequest.rssrc/sql_jsc/postgres/PostgresSQLConnection.rssrc/sql_jsc/postgres/PostgresSQLContext.rssrc/sql_jsc/postgres/PostgresSQLQuery.rssrc/sql_jsc/postgres/PostgresSQLStatement.rssrc/sql_jsc/postgres/Signature.rssrc/sql_jsc/postgres/protocol/error_response_jsc.rssrc/sql_jsc/postgres/types/PostgresString.rssrc/sql_jsc/postgres/types/bool.rssrc/sql_jsc/postgres/types/bytea.rssrc/sql_jsc/postgres/types/date.rssrc/sql_jsc/postgres/types/json.rssrc/sql_jsc/postgres/types/tag_jsc.rssrc/sql_jsc/shared/CachedStructure.rssrc/sql_jsc/shared/QueryBindingIterator.rssrc/sql_jsc/shared/SQLDataCell.rssrc/sql_jsc/shared/connection_args.rssrc/sql_jsc/shared/query_args.rs
💤 Files with no reviewable changes (10)
- src/sql_jsc/postgres/types/bool.rs
- src/sql_jsc/postgres/types/json.rs
- src/sql_jsc/postgres/types/PostgresString.rs
- src/sql_jsc/postgres.rs
- src/sql_jsc/postgres/types/bytea.rs
- src/sql_jsc/postgres/types/date.rs
- src/sql_jsc/shared/QueryBindingIterator.rs
- src/sql_jsc/postgres/Signature.rs
- src/sql_jsc/postgres/types/tag_jsc.rs
- src/sql_jsc/mysql/protocol/Signature.rs
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
I didn't find any bugs, but this is a large refactor (33 files, ~3.8k lines) that touches security-sensitive paths — TLS server verification, socket adoption, SSL_CTX lifetime, and null-byte injection checks — so it warrants a human look, and per the description it should wait on the CI docker lanes before merge.
Extended reasoning...
Overview
This PR is a no-behavior-change cleanup pass over the bun_sql_jsc crate, net -1,303 lines across 33 files. It deduplicates SQL driver code between MySQL and Postgres: shared SQLDataCell tag constructors replacing ~94 hand-rolled struct literals; shared createInstance argument parsing (shared/connection_args.rs, shared/query_args.rs); shared TLS handshake verification and socket-adoption (adopt_socket_tls, verify_tls_server); extracted error-cleanup blocks in PostgresSQLConnection::advance and PostgresSQLQuery::do_run; consolidated duplicate-field checks; plus dead-code removal (unused Signature::hash, postgres/types/{bool,bytea,json,PostgresString}.rs, Decimal, MaybeBytes/IntoOptionalData traits, etc.).
Security risks
Several refactors touch security-relevant code: verify_tls_server in shared/connection_args.rs consolidates the VerifyCa/VerifyFull cert and hostname-identity check that both drivers run after the TLS handshake; adopt_socket_tls in jsc.rs consolidates unsafe raw us_socket_t* adoption into the TLS group with SNI; the shared connection_args::parse owns the null-byte injection check on username/password/database/path and the SSL_CTX errdefer guard. These all look behavior-preserving on inspection (the fail-closed paths for missing servername / null SSL handle are retained, the null-byte check covers the same four fields), but they are exactly the kind of consolidation where a subtle ordering or short-circuit change could weaken verification, so a second pair of eyes is appropriate.
Level of scrutiny
This deserves careful human review. Beyond the TLS surface, the structural refactors carry subtleties: report_write_failure takes a raw *mut PostgresSQLStatement to avoid a protected &mut across JS re-entry; the Utf8Arg struct relies on field declaration order for drop correctness (the ZigStringSlice may borrow the OwnedString); PostgresRequest::parse_and_bind_and_execute was reshaped to call write_query (Parse+Describe) instead of separate Parse/Describe writes; and the Postgres on_handshake logic was flattened into a single boolean expression. The bulk of the diff (the SQLDataCell::int4/uint4/string/etc. constructor swaps) is mechanical and low-risk.
Other factors
The bug-hunting system found no issues. No CODEOWNERS cover src/sql_jsc/. The PR description explicitly notes that the real-server SQL tests only run in CI's docker lanes and asks not to merge before they pass — local tests skip when no DB is reachable, so CI green on those lanes is the meaningful signal here. Given the size, the unsafe/FFI-adjacent restructuring, and the security-sensitive TLS consolidation, I'm deferring rather than approving.
Net -1573 lines across the bun_sql_jsc crate with no behavior change. Deduplication: - Add SQLDataCell tag constructors and replace ~94 hand-rolled struct literals across the Postgres DataCell and MySQL ResultSet / DecodeBinaryValue decoders - Share createInstance argument parsing between the Postgres and MySQL connection and query classes (new shared/connection_args.rs and shared/query_args.rs) - Extract repeated error-cleanup blocks in PostgresSQLConnection::advance and PostgresSQLQuery::do_run - Collapse JSMySQLQuery's 8 copy-pasted cached-property accessors - Share the TLS handshake-verify and socket-adoption paths between drivers - Dedupe field tables in error_response_jsc::to_js and duplicate-field checks in Statement Dead code removal: - Unused Signature::hash in both drivers - Unused to_js helpers in postgres/types (bool, bytea, json, PostgresString) - Unused MySQLValue Decimal parsing and to_unix_timestamp - Unused QueryBindingIterator::reset and IntoJSHostFn marker impls Simplification: - Inline single-use traits (MaybeBytes, IntoOptionalData, DateToJs, Reader/Writer forwarding) - Replace MySQLQuery's hand-rolled Flags bitfield with a plain struct
…mark_duplicate_columns - clippy (undocumented_unsafe_blocks) requires the SAFETY comment to sit directly on the statement containing the unsafe block; the intervening ssl_ptr binding broke that association in connection_args::verify_tls_server. - The dedup-map allocation in mark_duplicate_columns now routes OOM through bun_core's controlled out_of_memory() handler instead of a plain panic.
The Sync-failure branch in do_run() was the only write-error path that returned without pairing the speculative this.ref_(), pinning the query object if the flush write failed. The omission is inherited from the Zig original; every sibling branch releases the ref, so this one now does too.
…ecode Covers the consolidated argument parsers and statement decode path without needing docker or a live server: - connection_args: null bytes in username/password/database and a non-boolean/non-object tls reject before any socket is created, for both the postgres and mysql adapters - query_args: a simple query with parameters is refused at handle creation - mark_duplicate_columns + the SQLDataCell decode paths, via a mock MySQL server returning two identically-named columns: object rows keep the last occurrence (text and binary protocol), .values() keeps every cell
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
test/js/sql/sql.test.ts (1)
12667-12667:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
port: 0in test base options.
port: 1violates the test rule requiringport: 0. Since these cases assert pre-connect validation,0is sufficient and keeps the suite consistent.As per coding guidelines: “Always use
port: 0in tests - do not hardcode ports or use custom random port functions”.🤖 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 `@test/js/sql/sql.test.ts` at line 12667, The test is using a hardcoded port value of 1 in the test's base options which violates the rule to always use port: 0; locate where the test constructs its base options (the object passed to the client/server setup in this test case—look for "port: 1" near the test that calls commandComplete("SELECT 1")) and change that port value to 0 so the test uses an ephemeral port and remains consistent with the test-suite guideline.src/sql_jsc/postgres/PostgresSQLConnection.rs (1)
425-465:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStart the protocol only after TLS verification succeeds.
adopt_socket_tls()kicks off the TLS handshake, but Line 464 immediately callsself.start(). InSSLMode::VerifyCa/SSLMode::VerifyFull, that can queue the Postgres startup packet beforeon_handshake()finishes the cert/hostname check, so we may send application bytes to a peer we reject moments later.Suggested fix
pub fn setup_tls(&self) { debug!("setupTLS"); // `vm_mut()` is `'static`, so `tls_group` borrows the VM singleton — // not `*self` — and stays live across the field reads below. let tls_group: &mut bun_uws::SocketGroup = self.vm_mut().postgres_socket_group::<true>(); @@ if !adopted { self.fail( b"Failed to upgrade to TLS", AnyPostgresError::TLSUpgradeFailed, ); return; } - self.start(); } @@ let ok = success == 1 && (!verify || connection_args::verify_tls_server( self.ssl_mode == SSLMode::VerifyFull, &self.tls_config, self.socket.get().get_native_handle(), ssl_error.error_no, )); if !ok { let Ok(v) = verify_error_to_js(&ssl_error, self.global()) else { return; }; self.fail_with_js_value(v); + return; } + + self.start(); }Also applies to: 813-833
🤖 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/PostgresSQLConnection.rs` around lines 425 - 465, The TLS handshake currently starts via jsc::adopt_socket_tls in setup_tls but setup_tls calls self.start() immediately, which can send the startup packet before certificate/hostname verification completes; change the flow so that self.start() is invoked only after the TLS verification callback (on_handshake / the adopt_socket_tls completion handler) confirms success — i.e., remove the immediate self.start() from setup_tls and instead call self.start() from the handshake-success path inside the adopt_socket_tls/on_handshake callback (and on error call self.fail with AnyPostgresError::TLSUpgradeFailed); apply the same change to the other setup_tls occurrence referenced (lines ~813-833) so protocol start is gated on successful verification.src/sql_jsc/postgres/DataCell.rs (1)
830-865:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a binary
DATEdecode path.
T::dateuses a 4-byte binary payload, but this branch only treats 8-byte values as binary. In binary results,DATEtherefore falls through to the text parser and feeds raw bytes intoparse_date, which corrupts prepared-query decoding.🤖 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/DataCell.rs` around lines 830 - 865, The binary decode branch incorrectly only checks for 8-byte payloads, causing 4-byte T::date binary values to be treated as text; update the tag handling in the match inside DataCell.rs so that when binary is true and tag == T::date with bytes.len() == 4 you call crate::postgres::types::date::from_binary(bytes) and return SQLDataCell::date(...) (similar to the T::timestamp/T::timestamptz cases that call from_binary), ensuring T::date is decoded via the 4-byte binary path instead of falling through to parse_date.
♻️ Duplicate comments (1)
src/sql_jsc/shared/query_args.rs (1)
44-62:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAllow omitted
pendingValueto stayundefined.Line 44 defaults a missing third argument to
JSValue::UNDEFINED, and Line 60 immediately rejects it becauseundefinedis not array-like. That turns an omitted optional argument into a construction-time throw on every caller of this shared parser.Suggested fix
- if !pending_value.js_type().is_array_like() { + if !pending_value.is_undefined() && !pending_value.js_type().is_array_like() { return Err(global_this.throw_invalid_argument_type("query", "pendingValue", "Array")); }🤖 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/shared/query_args.rs` around lines 44 - 62, The code currently treats a missing/omitted pending_value as JSValue::UNDEFINED then immediately rejects it because pending_value.js_type().is_array_like() fails; update the validation in the shared parser (symbols: pending_value, pending_value.js_type(), global_this.throw_invalid_argument_type) to allow undefined by only performing the "array-like" check when pending_value is not undefined (e.g., change the condition to check pending_value.is_undefined() first and only call js_type().is_array_like() when it's not undefined), so omitted pendingValue stays undefined instead of throwing.
🤖 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/postgres/PostgresSQLQuery.rs`:
- Around line 526-545: The OOM/error early returns after calling this.ref_() are
leaking the speculative ref and sometimes leaving this.statement or cache
entries pinned; replace the direct returns at the get_or_put(...) allocation
error and the requests.write_item(this_ptr) failure sites so they first call the
shared cleanup closure release_query_ref (which invokes this.release_statement()
and unsafe { Self::deref(this_ptr) }) and then route the error through the
existing throw_write_error closure (postgres_error_to_js via throw_write_error),
instead of returning immediately; ensure both failure paths use
release_query_ref() followed by returning throw_write_error(...) to reuse the
same speculative-ref cleanup and avoid leaks.
In `@src/sql_jsc/shared/connection_args.rs`:
- Around line 229-240: The null-byte check loop currently iterates over
(&username, "username"), (&password, "password"), (&database, "database"),
(&path, "path") but omits the Postgres startup-string field `options`; update
the iteration to include (&options, "options") so `options` is rejected on
embedded NULs as well, keeping the same logic that calls
global_object.throw_invalid_arguments when strings::index_of_char finds a null
byte.
In `@test/js/sql/sql-mysql.test.ts`:
- Around line 1385-1432: The shared no-server MySQL test suite (the describe
block titled "shared createInstance validation (no server)" and its contained
tests using SQL/using/sql`...`) should be moved out of sql-mysql.test.ts into a
new file named like sql-mysql-nodocker.test.ts (or sql-mysql.shared.test.ts) so
non-Docker/mock cases are separated from Docker-gated MySQL container tests;
locate the describe("shared createInstance validation (no server)") block and
its tests (including the three test.concurrent.each for null-byte fields, the
tls type test, and the simple query parameters test using SQL and
unsafe().simple()) and copy them to the new test file, remove them from the
original file, and ensure any necessary imports/exports (e.g., SQL, using
helper) are present at the top of the new file so the tests run in the
non-Docker suite.
---
Outside diff comments:
In `@src/sql_jsc/postgres/DataCell.rs`:
- Around line 830-865: The binary decode branch incorrectly only checks for
8-byte payloads, causing 4-byte T::date binary values to be treated as text;
update the tag handling in the match inside DataCell.rs so that when binary is
true and tag == T::date with bytes.len() == 4 you call
crate::postgres::types::date::from_binary(bytes) and return
SQLDataCell::date(...) (similar to the T::timestamp/T::timestamptz cases that
call from_binary), ensuring T::date is decoded via the 4-byte binary path
instead of falling through to parse_date.
In `@src/sql_jsc/postgres/PostgresSQLConnection.rs`:
- Around line 425-465: The TLS handshake currently starts via
jsc::adopt_socket_tls in setup_tls but setup_tls calls self.start() immediately,
which can send the startup packet before certificate/hostname verification
completes; change the flow so that self.start() is invoked only after the TLS
verification callback (on_handshake / the adopt_socket_tls completion handler)
confirms success — i.e., remove the immediate self.start() from setup_tls and
instead call self.start() from the handshake-success path inside the
adopt_socket_tls/on_handshake callback (and on error call self.fail with
AnyPostgresError::TLSUpgradeFailed); apply the same change to the other
setup_tls occurrence referenced (lines ~813-833) so protocol start is gated on
successful verification.
In `@test/js/sql/sql.test.ts`:
- Line 12667: The test is using a hardcoded port value of 1 in the test's base
options which violates the rule to always use port: 0; locate where the test
constructs its base options (the object passed to the client/server setup in
this test case—look for "port: 1" near the test that calls
commandComplete("SELECT 1")) and change that port value to 0 so the test uses an
ephemeral port and remains consistent with the test-suite guideline.
---
Duplicate comments:
In `@src/sql_jsc/shared/query_args.rs`:
- Around line 44-62: The code currently treats a missing/omitted pending_value
as JSValue::UNDEFINED then immediately rejects it because
pending_value.js_type().is_array_like() fails; update the validation in the
shared parser (symbols: pending_value, pending_value.js_type(),
global_this.throw_invalid_argument_type) to allow undefined by only performing
the "array-like" check when pending_value is not undefined (e.g., change the
condition to check pending_value.is_undefined() first and only call
js_type().is_array_like() when it's not undefined), so omitted pendingValue
stays undefined instead of throwing.
🪄 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: 4058b6ff-55ae-43cf-af22-9b7e9c2ed5f4
📒 Files selected for processing (35)
src/sql_jsc/jsc.rssrc/sql_jsc/lib.rssrc/sql_jsc/mysql/JSMySQLConnection.rssrc/sql_jsc/mysql/JSMySQLQuery.rssrc/sql_jsc/mysql/MySQLConnection.rssrc/sql_jsc/mysql/MySQLContext.rssrc/sql_jsc/mysql/MySQLQuery.rssrc/sql_jsc/mysql/MySQLStatement.rssrc/sql_jsc/mysql/MySQLValue.rssrc/sql_jsc/mysql/protocol/DecodeBinaryValue.rssrc/sql_jsc/mysql/protocol/ResultSet.rssrc/sql_jsc/mysql/protocol/Signature.rssrc/sql_jsc/mysql/protocol/any_mysql_error_jsc.rssrc/sql_jsc/postgres.rssrc/sql_jsc/postgres/DataCell.rssrc/sql_jsc/postgres/PostgresRequest.rssrc/sql_jsc/postgres/PostgresSQLConnection.rssrc/sql_jsc/postgres/PostgresSQLContext.rssrc/sql_jsc/postgres/PostgresSQLQuery.rssrc/sql_jsc/postgres/PostgresSQLStatement.rssrc/sql_jsc/postgres/Signature.rssrc/sql_jsc/postgres/protocol/error_response_jsc.rssrc/sql_jsc/postgres/types/PostgresString.rssrc/sql_jsc/postgres/types/bool.rssrc/sql_jsc/postgres/types/bytea.rssrc/sql_jsc/postgres/types/date.rssrc/sql_jsc/postgres/types/json.rssrc/sql_jsc/postgres/types/tag_jsc.rssrc/sql_jsc/shared/CachedStructure.rssrc/sql_jsc/shared/QueryBindingIterator.rssrc/sql_jsc/shared/SQLDataCell.rssrc/sql_jsc/shared/connection_args.rssrc/sql_jsc/shared/query_args.rstest/js/sql/sql-mysql.test.tstest/js/sql/sql.test.ts
💤 Files with no reviewable changes (9)
- src/sql_jsc/shared/QueryBindingIterator.rs
- src/sql_jsc/postgres/types/bytea.rs
- src/sql_jsc/postgres/types/PostgresString.rs
- src/sql_jsc/postgres.rs
- src/sql_jsc/postgres/Signature.rs
- src/sql_jsc/postgres/types/tag_jsc.rs
- src/sql_jsc/postgres/types/json.rs
- src/sql_jsc/mysql/protocol/Signature.rs
- src/sql_jsc/postgres/types/bool.rs
Member
Author
|
@robobun make sure there is absolutely no observable difference and this is a refactor only |
Collaborator
|
Re-audited the full diff file-by-file against
If you want strict bug-for-bug compat with old-Postgres plain |
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/sql_jsc/postgres/PostgresSQLQuery.rs (1)
837-843:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlso unwind the named statement cache entry on enqueue OOM.
By this point the named-prepared-statement path may already have stored
stmtintoconnection.statements(Lines 797-820).release_query_ref()only drops the query-held ref, so this branch can leave an orphanedPending/Parsingstatement cached even though the request never enteredconnection.requests. A later query can then pick up that stale cached statement.Hoist the same statement-map cleanup used by the earlier write-failure branches into this OOM path before returning.
🤖 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/PostgresSQLQuery.rs` around lines 837 - 843, The OOM branch that returns after failing to enqueue the request must also remove the prepared-statement entry from connection.statements to avoid leaving a stale Pending/Parsing cache; update the block handling connection.requests.with_mut(...).is_err() so that, before calling release_query_ref() and returning Err(...), it performs the same statement-map cleanup used in the earlier write-failure branches (remove the stmt entry from connection.statements if it was inserted), referencing the same stmt identifier and connection.statements map to ensure the cached statement is unwound on enqueue failure.
🤖 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.
Duplicate comments:
In `@src/sql_jsc/postgres/PostgresSQLQuery.rs`:
- Around line 837-843: The OOM branch that returns after failing to enqueue the
request must also remove the prepared-statement entry from connection.statements
to avoid leaving a stale Pending/Parsing cache; update the block handling
connection.requests.with_mut(...).is_err() so that, before calling
release_query_ref() and returning Err(...), it performs the same statement-map
cleanup used in the earlier write-failure branches (remove the stmt entry from
connection.statements if it was inserted), referencing the same stmt identifier
and connection.statements map to ensure the cached statement is unwound on
enqueue failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fad7ccd6-3b5b-4b8c-ab68-ee722389b2ce
📒 Files selected for processing (6)
src/sql_jsc/postgres/PostgresRequest.rssrc/sql_jsc/postgres/PostgresSQLConnection.rssrc/sql_jsc/postgres/PostgresSQLQuery.rstest/js/sql/sql-mysql-createinstance-validation.test.tstest/js/sql/sql-mysql-duplicate-columns.test.tstest/js/sql/sql.test.ts
Member
Author
|
@robobun conflicts |
# Conflicts: # src/sql_jsc/mysql/MySQLStatement.rs # src/sql_jsc/postgres/DataCell.rs # src/sql_jsc/postgres/PostgresSQLConnection.rs
Collaborator
|
Conflicts resolved — merged |
This was referenced Jun 6, 2026
Member
Author
|
Closing because conflicts |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A cleanup pass over the
bun_sql_jsccrate: net -1,012 lines (1,563 added, 2,575 removed) across 33 source files.Exact list of behavior deltas (everything else is equivalence-audited refactor)
Re-audited file-by-file against
mainafter the question below about observable differences. The complete list of places where behavior differs, all on error paths:Not JS-observable (allocator-failure paths only; same errors thrown, just no leaked refs):
PostgresSQLQuery::do_runnow releases the speculative query ref on all three OOM-reachable error exits — the Sync write failure (456f648), and thestatements.get_or_put/ finalrequests.write_itemfailures (flagged by coderabbit as the remaining leaks of the same class). The omissions were inherited from the Zig original, which cleans up the simple-querywriteItemfailure but not these. Same error is thrown at the same point; the query/statement just no longer leak.mark_duplicate_columnsusesunwrap_or_oom()where the old inline code used.expect("OOM")— crash style on allocation failure only.JS-observable, Postgres only — the shared
connection_args::parseresolves two explicitTODO(port)deviations by adopting the behavior MySQL already had, which is also exactly what the Zig reference does:code: "BORINGSSL", message from the BoringSSL error queue) instead of a plainError("Failed to create SSL context"). Old code carriedTODO(port): Zig err.toJS(...); Zig doesglobalObject.throwValue(err.toJS(globalObject))(PostgresSQLConnection.zig:646), which is what MySQL's parser already did onmain.username/password/database/pathnow throwsERR_INVALID_ARG_TYPE(aTypeError) instead of a plainError. Message text is unchanged. Zig usesthrowInvalidArguments(PostgresSQLConnection.zig:704); MySQL onmainalready threw this.If bug-for-bug compatibility with the old Postgres plain-
Errors is preferred over Zig/MySQL parity for 3–4, say so and I'll parameterize those two throws — but the deleted code itself documented them as port deviations.What this does
Deduplication
SQLDataCelland replace ~94 hand-rolled struct literals across the PostgresDataCelland MySQLResultSet/DecodeBinaryValuedecoderscreateInstanceargument parsing between the Postgres and MySQL connection and query classes (newshared/connection_args.rs,shared/query_args.rs)PostgresSQLConnection::advanceandPostgresSQLQuery::do_runJSMySQLQuery's 8 copy-pasted cached-property accessorserror_response_jsc::to_jsand the duplicate-field checks inStatementDead code removal
Signature::hashin both driversto_jshelpers inpostgres/types(bool.rs,bytea.rs,json.rs,PostgresString.rsdeleted)MySQLValueDecimal parsing andto_unix_timestampQueryBindingIterator::resetandIntoJSHostFnmarker implsSimplification
MaybeBytes,IntoOptionalData,DateToJs, Reader/Writer forwarding)MySQLQuery's hand-rolledFlagsbitfield with a plain structHow this was reviewed
Each refactor was audited against the pre-refactor code for behavioral equivalence and performance (no added allocations, no removed early exits, no complexity changes on row-decode hot paths). A second full-diff audit after the rebase onto #31663 verified per area: every
SQLDataCellconstructor's tag/free_value/value against all ~94 replaced literals; every flag read/write in theFlags→struct swap; theadvance()cleanup extraction (discard/defer offsets, exception-first ordering); the TLS verify truth table including the null-servername fail-closed case; theerror_response_jscfield table (all field-code → property pairs, last-occurrence-wins, property order unchanged);mark_duplicate_columnssemantics (last occurrence wins, byte-wise names, no column-count cap change); thevalidate_int/validate_bigintranges per type; and the argument-decode order/messages inconnection_args/query_argsfor both drivers. The deltas found are items 1–4 above; nothing else differs.How we know it works
main: the Postgres null-byte rejection'sERR_INVALID_ARG_TYPETypeErrorand the SSL_CTX-failureERR_BORINGSSLcode (sql.test.ts, no server needed).connection_argsnull-byte/tls validation andquery_argssimple-query check for both adapters (sql-mysql-createinstance-validation.test.ts, the Postgres twin insql.test.ts— the errors throw before any socket exists), and duplicate-column decode through a mock MySQL server (sql-mysql-duplicate-columns.test.ts: object rows keep the last occurrence,.values()keeps every cell, text + binary protocol).bun bd testpasses onsql-mysql-datetime-roundtrip,sql-postgres-datetime-roundtrip,sql-mysql-mediumint,sql-mysql-raw-length-prefix,sql-mysql-bind-oob,sql-mysql.test.ts, and the no-docker parts ofsql.test.ts— including the sql: share the wall-clock datetime text parser between MySQL and Postgres #31663 shared datetime parser now feeding the refactored decode paths.Notes for review
The biggest hunks by far are mechanical: the
SQLDataCellconstructor swap accounts for most of the changed lines inDataCell.rs,ResultSet.rs, andDecodeBinaryValue.rs. The structural changes worth a closer look areshared/connection_args.rs,shared/query_args.rs, and the error-cleanup extraction inPostgresSQLConnection::advance.