Repository navigation
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (10)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. WalkthroughChangesThe MySQL binding now reports statement commands and accurate result counts. It scans multi-statement SQL while handling comments, quotes, escapes, and executable comments. It also exposes the MySQL result metadata
Suggested reviewers: Merge Risk: ⚪ Minimal · up to MySQL statements now report affected-row counts and uppercase commands through the public result API, with the changed query forms covered by passing SQL tests. No actionable merge risk remains. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 9:10 AM PT - Sep 8th, 2026
❌ @robobun, your commit 1945966 has 1 failures in 🧪 To try this PR locally: bunx bun-pr 41886That installs a local version of the PR into your bun-41886 --bun |
|
Status: ready for review. Reproduced on bun 1.4.3-canary against MariaDB 11.8 and PostgreSQL 16: CI (build 112954, head 1945966): the SQL suites pass on every lane. The remaining red is |
…s code, honor NO_BACKSLASH_ESCAPES; expose the keyword table to JS from one place
There was a problem hiding this comment.
LGTM — f463c92 addresses the last point: the cursor now caches backslash_escapes per statement and handle_result_set_ok reads the flag from the pre-result status_flags, so statement K-1's body is scanned with the mode it was actually lexed under.
Re-checked the earlier four (-- needs a following space/control/EOF via at(pos+2) <= b' ', /*!//*M! treated as code, NO_BACKSLASH_ESCAPES gating the \ branch, commands derived from COMMON_KEYWORDS at binding init) — all still in place. The scanner's at() bounds-guards every lookahead, command_to_js propagates JsResult and a throw routes to reject before result() runs, and the change is confined to result metadata so a lexer miss on an unmodelled shape (ANSI_QUOTES, extra CALL result sets — both noted in the PR) mislabels command rather than affecting execution.
Extended reasoning...
Overview
The PR makes MySQL query results report command (the statement's leading keyword) and a correct count (affected rows for OK-only replies, row count for result sets), matching the PostgreSQL/SQLite adapters. It adds a small generic lexer (KeywordCursor) over the query text, threads NO_BACKSLASH_ESCAPES through the result path, exposes a shared keyword table from Rust to JS, and drops the borrowed CommandTag::Select hack in JSMySQLQuery. Ten files touched, ~250 net lines, plus a comprehensive test in the existing sql-mysql.test.ts.
Security risks
None material. The lexer reads user SQL text but only to derive an informational string; every index goes through the bounds-checked at() helper and the range returned into keyword_to_js is produced by the same function that walked the slice, so it cannot exceed sql.len(). Heap allocation in keyword_to_js is bounded by the run of consecutive ASCII letters. No new data flows to the server; query execution and row decoding are untouched.
Level of scrutiny
Moderate. The lexer is the only non-mechanical piece and it has been through three review rounds — each finding (-- rule, /*! executable comments, NO_BACKSLASH_ESCAPES, single-source keyword table, escape-mode lag) is now addressed with a targeted fix I re-verified in the current diff. The blast radius of any residual lexer edge case is a wrong result.command string on a multi-statement .simple() query, which is informational; count/affectedRows/rows are unaffected.
Other factors
No CODEOWNERS entry covers src/sql/ or src/sql_jsc/. The new test exercises both prepared and text protocols, ; inside quotes/comments, -- without a trailing space, # comments, /*!40101 ... */, REPLACE, and TRUNCATE, and the PR states it fails on the previous release. Exception propagation follows the repo convention (JsResult returned, error routed to reject_with_js_value with ensure_still_alive on the produced value). Bug hunt exited on dry_streak with no findings.
Problem
INSERT,UPDATE,DELETEor DDL statement hadcount = 0andcommand = null. The PostgreSQL and SQLite adapters fill both (count= rows affected,command="INSERT"/"UPDATE"/...), soif (result.count === 0)meant something else on MySQL. Only the MySQL-specificaffectedRowscarried the number.JSMySQLQuery::resolve(src/sql_jsc/mysql/JSMySQLQuery.rs) always builtCommandTag::Select(result_count), whereresult_countis the number of rows read (0 for a reply with no result set), andonResolveMySQLQuery(src/js/internal/sql/mysql.ts) ignored the tag and never setresult.command.Fix
handle_result_set_okpassescount = affected_rowswhen the OK packet answers a statement with no result set (no column-count header was received,HEADER_RECEIVEDunset), and the row count when it terminates a result set.affectedRowsandlastInsertRowidare unchanged.commandis the leading keyword of the statement, upper-cased. The MySQL protocol sends no command tag, so it comes from the query text, as on the SQLite adapter: a small scanner (src/sql/mysql/StatementKeyword.rs, generic over Latin-1/UTF-16 so the query string is not transcoded) skips whitespace, comments and((MySQL rules:--needs a following space,/*! ... */is code), and for a multi-statement query steps to the next;-separated statement per result, without splitting on;inside quotes or comments (NO_BACKSLASH_ESCAPESis read from the OK packet's status flags). Sixteen common keywords reach JS as an index into a table the binding exposes, the rest as a string.Bun.SQL: no documented or typed way to read the affected-row count portably across adapters #40432 (the sameUPDATEgivescount: 2on SQLite/PostgreSQL andcount: 0on MySQL). Out of scope: whetherUPDATEcounts matched or changed rows (CLIENT_FOUND_ROWS, sql(mysql): add foundRows option to opt into CLIENT_FOUND_ROWS #30848). sql: report affectedRows on PostgreSQL and SQLite query results #40434 addsaffectedRowsto the other adapters plus docs and types for the metadata fields; its wording for the MySQLcount/commandcells needs to follow whichever of the two lands first.countchange (about 20 lines inMySQLConnection.rs,JSMySQLQuery.rs,mysql.ts) and thecommandscanner are separable. If only the first is wanted now, say so and I will split the scanner into a follow-up.test/js/sql/sql-mysql.test.ts"result count and command are set for statements without a result set" (prepared and text protocol, multi-statement with;in quotes and comments,REPLACE,TRUNCATE,CREATE); fails on bun 1.4.3-canary. Also ran the rest ofsql-mysql.test.ts,sql-mysql.transactions.test.tsandsql-mysql.helpers.test.tsagainst MariaDB 11.8.Background
affected_rowsandlast_insert_id. PostgreSQL instead ends every statement withCommandComplete "INSERT 0 3", which is where itscommandandcountcome from.Bun.SQLresolves every query to anSQLResultArraywhose non-enumerablecount,command,lastInsertRowidandaffectedRowsmirror postgres.js. The MySQL native side callsonResolveMySQLQueryonce per result; a multi-statement orCALLquery produces several results, accumulated untilSERVER_MORE_RESULTS_EXISTSclears.Notes
INSERT 3->count=0 command=null affectedRows=3;UPDATE 3->count=0 command=null affectedRows=3;DELETE 1->count=0 command=null affectedRows=1. After:count=3 command=INSERT,count=3 command=UPDATE,count=1 command=DELETE;SELECTkeepscount = rows.lengthand getscommand=SELECT.CALL p()yields one result per result set plus a final OK; all of them reportcommand: "CALL". When aCALLis followed by more statements in the same text, the results after the procedure's first one are attributed one statement too early. The protocol gives no way to tell a procedure's extra result sets from the next statement's, andcommandis informational, so this is accepted.command: null, as PostgreSQL does for an empty tag.--rule,/*!executable comments,NO_BACKSLASH_ESCAPES, one keyword table) are all addressed in cc3ef7f.INSERTtexts cost nothing extra.