sql(mysql): add foundRows option to opt into CLIENT_FOUND_ROWS#30848
sql(mysql): add foundRows option to opt into CLIENT_FOUND_ROWS#30848robobun wants to merge 8 commits into
Conversation
|
Updated 4:30 AM PT - May 21st, 2026
❌ @autofix-ci[bot], your commit 5e65cda has some failures in 🧪 To try this PR locally: bunx bun-pr 30848That installs a local version of the PR into your bun-30848 --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:
WalkthroughThis PR adds a ChangesMySQL foundRows feature
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.
I didn't find any bugs, but this changes the default affectedRows semantics for all existing Bun MySQL users (now matched-rows instead of changed-rows) — that's a product decision worth a human sign-off, and there's also a CI build failure on x64-baseline.
Extended reasoning...
Overview
This PR adds a foundRows connection option to Bun.SQL's MySQL adapter that controls the CLIENT_FOUND_ROWS capability flag during handshake, threaded from the public TypeScript API (sql.d.ts, shared.ts, mysql.ts, private.d.ts) through the Rust FFI layer (JSMySQLConnection.rs) into the protocol handshake (MySQLConnection.rs). It ships with a self-contained mock-server test (sql-mysql-found-rows.test.ts). It also bundles two unrelated changes: a JSON.stringify wrap for the OVERLAY_CSS define in bake-codegen.ts (cold-build fix), and ~10 Rust files of pure cargo fmt whitespace from the autofix.ci commit.
Security risks
None identified. The new option is a boolean that toggles a single capability bit; URL query parsing is case-insensitive string comparison with no injection surface; the null-byte / protocol-injection guards on connection strings are unchanged.
Level of scrutiny
Medium-high. The mechanical plumbing is straightforward and the capability-bit change in handle_handshake is minimal and correctly placed before intersect(). However, the key judgment call is that foundRows defaults to true, which silently flips affectedRows semantics for every existing Bun MySQL user from changed-rows to matched-rows. The PR argues this matches mysql2/mariadb defaults (true), but it's still a behavioral break for anyone already on Bun's MySQL adapter — that's a product/compat decision a maintainer should explicitly endorse rather than have auto-approved.
Other factors
- robobun reports a build failure on
x64-baseline - build-rustfor the latest commit, which should be resolved or explained before merge. - The PR mixes three concerns (feature + unrelated build fix + autofix formatting); fine if that's the project norm, but worth a human glance.
- Test coverage for the new option is solid (6 cases including URL vs. options precedence and OK_Packet round-trip).
fe16787 to
69a7af4
Compare
6a6c689 to
345720f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/bun-types/sql.d.ts`:
- Around line 383-387: Update the JSDoc for the foundRows option (the comment
block around foundRows in packages/bun-types/sql.d.ts) to remove "MySQL only"
and explicitly state it applies to both MySQL and MariaDB protocols/adapters;
mention that when enabled the driver reports rows matched by the WHERE clause in
affectedRows and that this matches the default behavior of the mysql2 and
mariadb drivers so adapter: "mariadb" is supported.
🪄 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: ce0252cd-d4b6-4f23-a5b5-282ac2d665c5
📒 Files selected for processing (18)
packages/bun-types/sql.d.tssrc/codegen/bake-codegen.tssrc/crash_handler/lib.rssrc/errno/lib.rssrc/js/internal/sql/mysql.tssrc/js/internal/sql/shared.tssrc/js/private.d.tssrc/perf/tracy.rssrc/runtime/cli/Arguments.rssrc/runtime/cli/run_command.rssrc/runtime/cli/upgrade_command.rssrc/runtime/jsc_hooks.rssrc/runtime/webview/ChromeProcess.rssrc/spawn/process.rssrc/spawn_sys/spawn_process.rssrc/sql_jsc/mysql/JSMySQLConnection.rssrc/sql_jsc/mysql/MySQLConnection.rstest/js/sql/sql-mysql-found-rows.test.ts
There was a problem hiding this comment.
Thanks for addressing all the nits — the code looks good to me. I'm not auto-approving only because this flips the default affectedRows semantics for existing MySQL users (changed-rows → matched-rows), which is a product call worth a maintainer's explicit sign-off; the musl build failures in CI are also worth a glance before merge.
Extended reasoning...
Overview
This PR adds a foundRows connection option to Bun.SQL's MySQL/MariaDB adapter, threading it from parseOptions() (TS) → createMySQLConnection (TS binding) → JSMySQLConnection::create_instance (Rust FFI) → MySQLConnection.found_rows field → handle_handshake's CLIENT_FOUND_ROWS capability bit. It defaults to true, matching the mysql2/mariadb driver defaults. The PR also bundles an unrelated cold-build fix in bake-codegen.ts (wrap OVERLAY_CSS in JSON.stringify) and ~10 Rust files of purely cosmetic #[cfg(...)] attribute reformatting from autofix.ci. A new mock-MySQL-server test file (7 cases) and a docs section in runtime/sql.mdx round it out.
All three of my earlier inline nits (duplicate "max" in the type union, String() coercion for duplicate URL query keys, and the missing sql.mdx docs section) have been addressed and resolved.
Security risks
None identified. The option is a boolean that toggles a single well-defined MySQL capability flag during the handshake; no user-controlled data flows into protocol bytes beyond that. URL query parsing coerces through String() before .toLowerCase(), avoiding the array-method TypeError on duplicate keys.
Level of scrutiny
Moderate-to-high. The implementation is small, well-isolated, and well-tested, but two things push this above the auto-approve bar:
- Default behavior change. Setting
foundRows: trueby default flipsaffectedRowssemantics for every existing Bun MySQL user from changed-rows to matched-rows. Code that relied onaffectedRows === 0to mean "no column values changed" will silently behave differently. The PR description and docs are clear about this and the rationale (ecosystem alignment) is sound, but choosing to ship a default flip vs. an opt-in is a product decision a maintainer should explicitly sign off on. - Wire-protocol path. The Rust change in
MySQLConnection::handle_handshakerestructures capability negotiation slightly (compute → set bit → intersect). It's correct on inspection, but handshake code is load-bearing.
Other factors
- CI (Build #54959) shows build failures on three musl targets (x64-musl, x64-musl-baseline, aarch64-musl). These may be infrastructure-related and unrelated to this change, but should be confirmed green before merge.
- The bundled
bake-codegen.tsfix is independent of the MySQL change (acknowledged in the PR description). - Test coverage for the new option is thorough (default, explicit true/false, URL form, options-over-URL precedence, duplicate-key URL, and
affectedRowsround-trip via mock server).
6f70981 to
b8dcadb
Compare
mysql2 and mariadb both enable CLIENT_FOUND_ROWS by default, so an UPDATE that matches a row but does not change any column value returns affectedRows: 1. Bun.SQL's MySQL adapter had the flag defined in Capabilities but never set it and had no way to opt in, so the same query returned affectedRows: 0 — indistinguishable from an UPDATE that matched nothing. Code migrated from mysql2 / mariadb hit this. Add a foundRows connection option (default true, matching mysql2 and mariadb) that OR-s CLIENT_FOUND_ROWS into the client-side capability set built in handshake negotiation. Accepted as options.foundRows and as the URL query param ?foundRows=false; options object wins over URL. For non-MySQL adapters the option is parsed and ignored. Fixes #30843
Bun.build parses define: values as JSON. The other defines
(side, IS_ERROR_RUNTIME, IS_BUN_DEVELOPMENT) already go through
JSON.stringify / String, but OVERLAY_CSS was passing the raw minified
CSS as-is. That CSS starts with '*{...}'; bun versions that lack the
auto-quote fallback from #30679 reject it with 'Operators are not
allowed in JSON' and abort bake-codegen. The CI Dockerfile bootstraps
from bun 1.3.13, which predates the fix, so cold builds fail. Wrap
the value in JSON.stringify to match the other defines.
node-http-backpressure.test.ts allocates a 2GB buffer and was SIGKILL'd on one aarch64 shard. Unrelated to the MySQL foundRows diff; every other test-bun shard on every other platform passed.
Pre-existing typo that predates this PR — the Define<> key union listed "max" twice. TypeScript dedupes string-literal unions so it's purely cosmetic, but this PR already reformats the same line to fit "foundRows", so drop the duplicate on the same touch. Spotted by @claude on review.
…asing Bun's URLSearchParams.toJSON() returns an Array when a key appears more than once in the query string, so `?foundRows=true&foundRows=false` gave us an array and `.toLowerCase()` threw `TypeError`. Coerce via String() first to match the sslmode branch's handling — duplicate keys now fall through to the default instead of crashing the connection path. Covered by a new test case in sql-mysql-found-rows.test.ts. Spotted by @claude on review.
…ction JSDoc had "MySQL only" which could imply `adapter: "mariadb"` is unsupported. Both adapters route through MySQLAdapter → createMySQLConnection → the same MySQL wire protocol, so the option works for either. Also add a short block to docs/runtime/sql.mdx explaining the new default (matched-rows) and how to opt out via `foundRows: false` or `?foundRows=false` on the URL.
2655518 to
5e65cda
Compare
There was a problem hiding this comment.
All prior nits are addressed and I didn't find any bugs, but this changes the default affectedRows semantics for existing MySQL users and threads a new arg through the Rust handshake path — worth a human sign-off on the default flip.
Extended reasoning...
Overview
Adds a foundRows connection option (default true) for the MySQL/MariaDB adapter that toggles CLIENT_FOUND_ROWS in the HandshakeResponse41 capability flags. Threaded through parseOptions (shared.ts) → createMySQLConnection (mysql.ts, positional arg 16) → JSMySQLConnection::create_instance (Rust FFI) → MySQLConnection::init → handle_handshake. Also adds public types, docs in runtime/sql.mdx, a 300-line mock-server test, and an unrelated build fix in bake-codegen.ts.
Security risks
None identified. The option only flips a single capability bit during handshake; no auth, crypto, or input-validation surface changes. The URL query-string parser coerces via String() before .toLowerCase() (fixed after my earlier nit), so no new throw paths on adversarial URLs.
Level of scrutiny
Medium-high. While the implementation is clean, well-tested, and all prior review feedback (mine and CodeRabbit's) has been addressed, this is not a purely additive change — it flips the default affectedRows semantics for every existing Bun MySQL user from changed-rows to matched-rows. That's the right call for mysql2/mariadb compatibility, but it's a behavioral default change to a public API that a maintainer should explicitly approve. It also touches the Rust protocol handshake (MySQLConnection.rs:642-655) and adds a positional FFI argument (callframe.argument(16)), which sits adjacent to the recently-rebased allowPublicKeyRetrieval plumbing.
Other factors
- All four prior inline comments (3 from me, 1 from CodeRabbit) are resolved with follow-up commits.
- Bundles an unrelated
bake-codegen.tsbuild fix (JSON.stringifywrap forOVERLAY_CSS) — reasonable but worth a glance. - Test coverage is solid (mock server inspects the actual capability bit on the wire, plus precedence and duplicate-key cases).
Fixes #30843.
Repro
Cause
Capabilities::get_default_capabilitiesinsrc/sql/mysql/Capabilities.rsnever set
CLIENT_FOUND_ROWS, and there was no user-facing option to optin. The MySQL server's
OK_Packet.affected_rowstherefore carriedchanged-rows counts, not matched-rows counts.
mysql2enablesCLIENT_FOUND_ROWSby default (opt-out viaflags: ["-FOUND_ROWS"]) andmariadbdoes the same (opt-out viafoundRows: false). Code migrated from either driver silently lost theability to tell "matched but unchanged" from "didn't match" apart.
Fix
Add a
foundRowsconnection option (accepted on the options object andas a
?foundRows=...URL query param). Default istrue, matching themysql2 / mariadb defaults. When enabled, the client OR-s
CLIENT_FOUND_ROWSinto the capability set it requests during theHandshakeResponse41, so subsequent
affected_rowscounts come back withmatched-rows semantics.
The option is threaded through:
parseOptions→createMySQLConnectioncall →JSMySQLConnection::create_instance→MySQLConnection::init→MySQLConnection.found_rowsfield →handle_handshake'srequested.CLIENT_FOUND_ROWS.parseOptionsalso writesfoundRowsinto theDefinedOptionsstructure for Postgres/SQLite adapters — they ignore it, so the option
is MySQL-only without any adapter switch in the parser.
Verification
New test:
test/js/sql/sql-mysql-found-rows.test.tsuses a minimal mockMySQL server (no Docker required) to inspect the client's
HandshakeResponse41
capability_flagsbit 1 (CLIENT_FOUND_ROWS):foundRows)foundRows: truefoundRows: false?foundRows=falsefoundRows: true+ URL?foundRows=falseaffectedRows: 1reaches JSFail-before:
Pass-after:
Existing MySQL tests (
sql-mysql-auth-short-nonce,28004,adapter-env-var-precedence,adapter-override,sql-mysql-cached-error,sql-mysql-bind-oob,sql-mysql-bind-blob-borrow,sql-mysql-raw-length-prefix,sql-mysql-columns-realloc-oom,sql-mysql-clean-reentry) still pass.Extra commit: build fix
build: JSON-stringify OVERLAY_CSS define value in bake-codegen— wrapsthe raw minified CSS in
JSON.stringifyso the pre-auto-quote bootstrapbun (1.3.13, used in the CI Dockerfile) can parse
define:values thatstart with
*{...}. Independent of the MySQL change; needed for coldbuilds.
Rebase notes
Rebased onto
0b20408b65(Hardening: input validation and bounds tighteningacross 26 subsystems, #31129), which added
allowPublicKeyRetrievalto thesame MySQL connection plumbing I was touching for
foundRows. Resolved bykeeping both parameters side-by-side:
packages/bun-types/sql.d.ts— both JSDoc blocks retained, in orderallowPublicKeyRetrievalthenfoundRows.src/js/internal/sql/mysql.ts—createMySQLConnectionarg list is...!prepare, !!allowPublicKeyRetrieval, !!foundRows.src/sql_jsc/mysql/JSMySQLConnection.rs—callframe.argument(15)readsallow_public_key_retrieval,argument(16)readsfound_rows.src/sql_jsc/mysql/MySQLConnection.rs—initsignature ends withallow_public_key_retrieval, found_rows.All 7
sql-mysql-found-rows.test.tscases still pass after the rebase.