sql(mysql): accept any spelling of ON DUPLICATE KEY UPDATE in the update helper - #32040
sql(mysql): accept any spelling of ON DUPLICATE KEY UPDATE in the update helper#32040robobun wants to merge 2 commits into
Conversation
|
Updated 8:37 PM PT - Jun 12th, 2026
✅ @robobun, your commit 124d4cfbb51001ae713f5eb73061cd0bd2905ea0 passed in 🧪 To try this PR locally: bunx bun-pr 32040That installs a local version of the PR into your bun-32040 --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 (3)
WalkthroughReplace case-sensitive UPSERT suffix detection with an anchored, case-insensitive regex and add protocol-level and functional regression tests that validate normalized prepared SQL and upsert behavior across varied keyword casings and whitespace. ChangesMySQL UPSERT case-insensitive detection
Suggested reviewers:
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
CI summary for maintainers: the sql changes in this PR have been green on every lane across all three builds. Each red lane had a different unrelated cause:
sql-mysql.helpers.test.ts occasionally hits a docker service startup race (container name already in use) and has passed on retry every time. The one allowed retrigger is used; not pushing more. The diff is ready for review. |
…update helper SQL keywords are case-insensitive, but the upsert suffix check in normalizeQuery was not, so a lowercase "on duplicate key update" got an extra SET appended and produced invalid SQL. Fixes #32035
c0efca0 to
4655783
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@src/js/internal/sql/mysql.ts`:
- Around line 282-287: The isUpsertUpdate method currently checks the literal
trailing substring which fails when there are multiple or non-space whitespace
characters; replace the exact-slice check with a case-insensitive regular
expression that matches the UPSERT suffix allowing flexible whitespace (e.g.
/\bON\s+DUPLICATE\s+KEY\s+UPDATE\s*$/i) against query.trimEnd(), so any
combination of spaces, tabs or newlines is recognized and prevents
normalizeQuery from appending a stray SET; update the function to use this regex
and keep the existing trimming behavior.
🪄 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: f2ab586f-2f8d-46f7-94ec-528517d8af83
📒 Files selected for processing (3)
src/js/internal/sql/mysql.tstest/js/sql/sql-mysql-upsert-keyword-case.test.tstest/js/sql/sql-mysql.helpers.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — one-line case-insensitivity fix with solid regression tests; the inline nit is pre-existing and non-blocking.
Extended reasoning...
Overview
The production change is a single method body in src/js/internal/sql/mysql.ts: isUpsertUpdate() now compares the uppercased 23-char tail of the trimmed query against "ON DUPLICATE KEY UPDATE" instead of using a case-sensitive endsWith(). This is a strict superset of the old matcher — uppercase queries match exactly as before, and lowercase/mixed-case spellings now also match. The rest of the diff is test coverage: a new Docker-free mock-server test that asserts the exact normalized SQL for three casings, plus a lowercase/mixed-case variant added next to the existing uppercase upsert test in sql-mysql.helpers.test.ts.
Security risks
None. The string being inspected is the static template-literal text accumulated by normalizeQuery in shared.ts, not user-supplied data, and the only effect of the predicate is whether the helper appends a literal " SET ". No new injection surface, no auth/crypto/permissions involvement.
Level of scrutiny
Low. This is a targeted bugfix for a reported issue (#32035) with an obvious root cause (case-sensitive comparison of case-insensitive SQL keywords). The fix is the minimal correct change, mirrors how detectCommand() already lowercases tokens, and cannot regress the previously-working uppercase path. Edge cases (query shorter than 23 chars after trim) degrade safely since .slice(-23) returns the whole string and the equality fails.
Other factors
- The one inline finding is explicitly flagged as a pre-existing, non-blocking nit (single-space requirement between keywords) that the old
endsWith()shared; it suggests a follow-up regex but doesn't affect correctness of this PR. - No CODEOWNERS match the touched paths.
- CI: robobun reports the sql tests green on every lane across three builds; red lanes are documented unrelated flakes (angular CLI Node version, EPIPE race, lsquic ASAN leak, duckdb segfault, darwin agent expiry).
- The base/sqlite
isUpsertUpdateoverrides returnfalseand are untouched, consistent with the PR description that MySQL is the only adapter using this suffix form.
SQL separates keywords with arbitrary whitespace, so match the clause with a case-insensitive regex instead of an exact single-space string.
robobun
changed the title
There was a problem hiding this comment.
LGTM — the whitespace handling from the earlier review is in, and the remaining tail-slice note is a non-blocking micro-optimization.
Extended reasoning...
Overview
This PR fixes #32035 by replacing a case-sensitive endsWith("ON DUPLICATE KEY UPDATE") check in MySQLAdapter.isUpsertUpdate with a case-insensitive, whitespace-tolerant regex (/\bON\s+DUPLICATE\s+KEY\s+UPDATE\s*$/i), hoisted to a module-level const. The rest of the diff is tests: a new mock-MySQL-server test that captures COM_STMT_PREPARE text and asserts the exact generated SQL across four spellings (no Docker required), plus three new live-DB cases in sql-mysql.helpers.test.ts.
Security risks
None. The change only affects whether the query builder appends a literal " SET " token between user-written keywords and the column-assignment list it already generates. Values remain parameterized as ? placeholders; identifiers are still escaped via escapeIdentifier. The regex is anchored, non-global (no lastIndex state), and runs on a string the builder already constructed — no user-controlled regex input.
Level of scrutiny
Low. The production change is effectively one line, in a leaf method whose only caller is normalizeQuery in shared.ts. The fix strictly widens the set of inputs that take the correct (no-SET) path; any input that matched the old endsWith also matches the new regex, so no regression surface. The mock-server test pins the exact generated SQL byte-for-byte, which is stronger than the prior coverage.
Other factors
- My earlier inline suggestion (accept flexible whitespace, not just case) and the parallel CodeRabbit comment were both addressed in 124d4cf, with test coverage for the newline/tab/multi-space cases added to both test files.
- The one new inline nit (regex now scans the full query instead of
.slice(-64)) is explicitly non-blocking and self-describes the cost as µs-scale, dwarfed by the DB round-trip and by the O(N) string concatenation that built the query. It is fine to address by either restoring the slice or just updating the stale sentence in the PR description; neither blocks merge. - CI failures across the three builds are documented by robobun as unrelated flakes (angular/cli Node version bump, EPIPE race, lsquic ASAN leak, duckdb segfault, darwin init snapshot/timeout); the sql tests themselves were green on every lane.
…raw TypeErrors and engine errors (#32156) Fixes #32155 ### Repro ```ts const sql = new SQL("sqlite://:memory:"); await sql`SELECT * FROM t WHERE id IN ${sql([null], "id")}`; // TypeError: null is not an object (evaluating 'strings') await sql`update t set ${sql({ name: undefined })} where id = 1`; // SQLiteError: near "where": syntax error await sql`update t SET ${sql({ name: undefined })} where id = 1`; // SyntaxError: Update needs to have at least one column ``` Both reproduce on 1.4.0 and main, on all three adapters for the first case. ### Cause 1. The keyed WHERE IN branch of the shared `normalizeQuery` (`src/js/internal/sql/shared.ts`) only guards items against `undefined` before reading `value[columns[0]]`, so a `null` item throws a raw TypeError. The same property-access-on-null pattern exists in the INSERT item scan (`buildDefinedColumnsAndQuery`) and in the keyed UPDATE branch, so `sql([{...}, null])` for INSERT and `sql(null, "col")` / `sql([undefined], "col")` for UPDATE hit the same TypeError. 2. The sqlite adapter's `throwIfUpdateEmpty` hook detects an empty update helper with `query.endsWith("SET ")`, a case-sensitive check against the user's original spelling. Command detection is case-insensitive, so lowercase `set` takes the helper path but misses the check and the malformed SQL reaches the engine. postgres and mysql use the `BaseSQLAdapter` default, which gates on the `hasValues` flag instead. ### Fix All in `src/js/internal/sql/shared.ts` plus the sqlite hook (after #32145 consolidated `normalizeQuery`, these are single sites covering all three adapters): - Keyed WHERE IN: a `null` item now throws `SyntaxError: Cannot use null as an item in WHERE IN helper with a column`. `undefined` items keep binding NULL (existing behavior), as do `null` items in the non-keyed form `sql([null])` and `null` values under the key (`sql([{ id: null }], "id")`). - INSERT (`buildDefinedColumnsAndQuery`): `null`/`undefined` items throw `SyntaxError: Cannot use null or undefined as an item in INSERT helper`. Previously both were raw TypeErrors. - Keyed UPDATE: a `null`/`undefined` item throws `SyntaxError: Cannot use null or undefined as an item in UPDATE helper`. - sqlite empty update: `SQLiteAdapter.throwIfUpdateEmpty` now checks the `hasValues` flag like the `BaseSQLAdapter` default (sqlite implements the adapter interface standalone, so the override stays but with the correct body), making the empty-helper error spelling-independent. One deliberate sqlite behavior change beyond the lowercase fix, raised in review: a literal assignment followed by an all-undefined helper, `UPDATE t SET updated_at = CURRENT_TIMESTAMP, ${sql({ name: undefined })}`, previously executed on sqlite (the trailing comma was stripped and the suffix check missed). The old behavior was order-dependent (the helper-first form already threw), and postgres/mysql throw for both orders, so sqlite now throws for both as well. A helper with defined values mixed with literal assignments keeps working; both are pinned by tests. The mysql `ON DUPLICATE KEY UPDATE` suffix check (`isUpsertUpdate`) has a similar case-sensitivity problem, tracked separately in #32035 (fix in #32040), so it is not touched here. ### Tests `test/js/sql/sql-helpers-validation.test.ts` (new): the validation contract is identical across the three adapters, so it is tested as one `describe.each` matrix (null item in keyed WHERE IN, null/undefined items in INSERT and keyed UPDATE, empty update helper for lowercase `set`, uppercase `SET`, the helper-emitted SET form, and the empty helper alongside a literal assignment in both orders), plus sqlite-only tests for the behaviors that must keep working (lowercase `set` with defined values updates, a helper with defined values alongside a literal assignment updates, undefined items / null column values / non-keyed null still bind NULL). Normalization runs when a query is first awaited, before any connection is attempted, so the postgres and mysql rows run without a server; sqlite uses `:memory:`. On the unfixed build, 11 of the 18 tests fail (raw `TypeError: null is not an object` instead of the validation errors, and the empty-helper updates reaching the engine); the other 7 are behavior-preservation and cross-adapter parity guards. All 18 pass with the fix. ### Rebase note Originally the guards were duplicated across `sqlite.ts`/`postgres.ts`/`mysql.ts`; after #32145 landed (consolidating `normalizeQuery` into `shared.ts`), the branch was rebased and the same fix re-applied at the now-shared sites, shrinking the diff to `shared.ts` + the sqlite `throwIfUpdateEmpty` hook.
1 participant
Fixes #32035
Problem
The MySQL adapter's update helper decides whether to append
SETby checking the query suffix with a case-sensitive, exact single-space string match:SQL keywords are case-insensitive and separated by arbitrary whitespace, and the command detection already tolerates both, so spellings like lowercase or line-wrapped clauses reach the update-helper path but fail the upsert check and get a spurious
SET:Fix
Match the clause with a case-insensitive regex that allows flexible whitespace in
MySQLAdapter.isUpsertUpdate(src/js/internal/sql/mysql.ts):The base adapter and sqlite override return false (sqlite and postgres upserts are
ON CONFLICT ... DO UPDATE SETwhere the user writesSET), so mysql was the only affected site.Note: the fix originally patched the inline suffix check in mysql.ts's normalizeQuery; after #32145 consolidated normalizeQuery into shared.ts, it now lives in the
isUpsertUpdatemethod that refactor introduced.Tests
test/js/sql/sql-mysql-upsert-keyword-case.test.ts(new): mock MySQL server captures theCOM_STMT_PREPAREtext and asserts the exact generated SQL for uppercase, lowercase, mixed-case, and mixed-whitespace (newline/tab/multi-space) spellings. Runs without Docker. Fails on the unfixed build with the spuriousSETvisible in the diff.test/js/sql/sql-mysql.helpers.test.ts: lowercase, mixed-case, and line-wrapped upsert variants alongside the existing uppercase one, against the real MySQL service.Verified with main's src and these tests the mock test fails (all non-exact spellings get
SET), and with the fix it passes. The full helpers file (15 tests including the new variants) passes end to end against a local MariaDB 11.8.