Repository navigation
Conversation
… in sql.array The sql(rows) batch-insert helper derived its column list from Object.keys(rows[0]) only. A key that first appeared on a later row was never emitted as a column, so its value was silently discarded and NULL was stored for every row. The downstream buildDefinedColumnsAndQuery already scans every row and null-fills missing values, so the column list just needs to be the union of keys across all rows. Separately, arrayValueSerializer had no branch for null: typeof null is 'object', so it fell through to JSON.stringify and produced the quoted string '"null"'. A TEXT[] column stored the four-character string and an INTEGER[] column rejected it with 22P02. Emit the unquoted null token (SQL NULL in Postgres array-literal syntax), matching the existing undefined handling.
|
Warning Review limit reached
Next review available in: 2 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
Comment |
|
Status: diff is green; all review threads resolved. Ready for review. Verified with: Fail-before (src/ reverted to main): Follow-ups since the first push:
CI: no SQL test failed on any lane across builds #77719, #77758, #77766. Remaining failures are unrelated to this diff:
|
|
Updated 7:41 AM PT - Jul 22nd, 2026
❌ @robobun, your commit b210150 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 35111That installs a local version of the PR into your bun-35111 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
The suggested link to #28980 is a false positive. That issue is about the prepared-statement cache key including per-parameter null type suffixes (causing unbounded |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs. Both fixes look correct and the tests are solid, but the key-union change is a user-facing API semantics decision (heterogeneous rows now succeed with unioned columns rather than silently using row-0's keys) that's worth a maintainer sign-off.
What was reviewed:
arrayValueSerializernull branch — verified unquotednullis the correct Postgres array-literal token; MySQL'sarray()throws so no sibling site to patch.SQLHelperunion path — confirmed single-object ([obj]), positional-array rows, and explicit-column calls still hit the oldObject.keys(value[0])path;buildDefinedColumnsAndQueryalready null-fills missing keys so downstream is unchanged.- Noted a minor edge:
WHERE id IN ${sql([{id:1},{id:2,other:x}])}(no explicit column) previously used onlyid, now unions to two columns and throws "Cannot use WHERE IN helper with multiple columns". Unlikely in practice but a behavior change.
Extended reasoning...
Overview
Two independent fixes in Bun.SQL helpers:
src/js/internal/sql/postgres.ts:arrayValueSerializergains an earlyvalue === nullbranch returning the unquoted token"null", matching the existingundefinedhandling. Without it,typeof null === "object"fell through toJSON.stringifyand produced the quoted string"null".src/js/internal/sql/shared.ts:SQLHelper's constructor now unionsObject.keysacross all rows when the caller passes a multi-row array of objects with no explicit column list, instead of deriving columns fromrows[0]only.- Tests added in both the no-server helper-validation file (sqlite in-memory +
serializedValuesinspection) and the docker-guarded live-Postgres suite.
Security risks
None. Column names still flow through escapeIdentifier; array elements are still escaped via the existing arrayEscape. The new branch only affects the literal null value.
Level of scrutiny
Medium. These are small, targeted fixes to real data-loss bugs, but they change user-visible semantics of a public API (sql(rows) batch insert). The union-keys approach is one valid design; another would be to throw on heterogeneous rows (forcing the caller to pass explicit columns). A maintainer should confirm union-and-null-fill is the intended contract.
Other factors
- Checked call sites:
SQLHelperis constructed at three places insrc/js/bun/sql.ts, all passing the user's array + rest-arg column names; the single-object path wraps in[obj]solength > 1is false and it takes the unchanged branch. - The union loop skips
null/non-object items rather than throwing;buildDefinedColumnsAndQuerystill throws "Cannot use null or undefined as an item in INSERT helper" downstream, so the error surface is preserved. - MySQL adapter's
array()throws "MySQL doesn't support arrays", so the null-serialization fix has no sibling to patch there. - One subtle knock-on: for
WHERE ... IN ${sql(objs)}with no explicit column and heterogeneous objects, the derived column count can now exceed 1, which trips the existing "Cannot use WHERE IN helper with multiple columns" guard. Previously it silently used row-0's single key. This is edge-case usage (WHERE IN with objects normally takes an explicit column), and turning silent key-drop into an error is arguably safer, but it's a behavior change worth being aware of. - Test coverage is good: fail-before was verified, explicit-column path has a regression guard, and the live-Postgres round-trip asserts both TEXT[] and INTEGER[] null handling.
Move the union-of-keys logic out of the SQLHelper constructor and into the INSERT branch of normalizeQuery, gated on a new autoColumns flag. WHERE IN over heterogeneous objects with no explicit column now keeps its existing first-row-keys behaviour instead of tripping the 'multiple columns' guard. Adds a regression test for that case.
|
Good catch on the WHERE IN edge case. Verified: on main, 198e234 scopes the key-union to the INSERT path via a new |
There was a problem hiding this comment.
Beyond the inline nit, I also checked: MySQL/SQLite have no sibling arrayValueSerializer needing the same null fix (MySQL's array() throws, SQLite has none); positional-array batch inserts (sql([[1,"a"],[2,"b"]])) are correctly excluded from key-widening by the !$isArray(items[0]) guard; and unionRowKeys' use of global Object.keys/Set matches the existing constructor code in the same file.
Extended reasoning...
The two fixes are correct and well-tested, and the WHERE IN regression from the first iteration was already scoped out with a covering test. Deferring rather than approving because this changes user-facing INSERT column-list derivation semantics in Bun.SQL — clearly the right change (the old behavior silently dropped data), but an API-surface behavior change worth a maintainer glance.
Extract the SQLHelper constructor's key check into validateHelperKey and run it on every key unionRowKeys adds from later rows, so a key like "70000" is rejected regardless of which row it first appears on.
There was a problem hiding this comment.
LGTM — the validateHelperKey extraction addresses my prior note, and both fixes (key-union for INSERT, unquoted null in sql.array) are correct and well-covered. The remaining inline nit (empty first row still hits the pre-widening columnCount === 0 guard) is not a regression and fails loudly, so it's non-blocking.
Extended reasoning...
Overview
Two independent silent-data-loss fixes in Bun.SQL helpers: (1) src/js/internal/sql/shared.ts gains unionRowKeys + an autoColumns flag on SQLHelper so batch INSERT with an auto-derived column list unions keys across all rows instead of dropping keys absent from row 0; (2) src/js/internal/sql/postgres.ts adds a value === null branch to arrayValueSerializer so sql.array([..., null, ...]) emits SQL NULL instead of the quoted string "null". The constructor's key-validation loop was extracted into validateHelperKey and applied to widened keys per my prior review. Tests cover both fixes without a server (sqlite in-memory + serializedValues inspection) plus docker-guarded Postgres round-trips.
Security risks
None identified. Widened column names come from Object.keys() (own-enumerable strings only), pass through validateHelperKey, and are quoted by escapeIdentifier, so there is no injection vector. The null branch in arrayValueSerializer emits a fixed literal token. No auth, TLS, or credential paths touched.
Level of scrutiny
Medium — user-facing SQL query generation, but the changes are narrowly scoped and strictly correctness-improving. The key-union is gated by autoColumns && (items) && items.length > 1 && !(items[0]), so explicit-column, single-row, positional-array, WHERE IN, and UPDATE paths are all untouched (WHERE IN preservation has a dedicated regression test). The validateHelperKey refactor is a straight extraction of the prior inline logic with identical semantics.
Other factors
My previous review's nit (validation not applied to widened keys) was addressed in 3e0954d and the thread resolved. The new inline nit — [{}, {id:1}] still throws Cannot INSERT with no columns because the columnCount === 0 guard runs before widening — is not a regression (it threw identically before this PR) and fails loudly with a clear message, so it doesn't block. Fail-before/pass-after evidence is in the PR body; the no-server tests in sql-helpers-validation.test.ts run on all three adapters and don't depend on the docker guard.
The shared columnCount === 0 guard fired before unionRowKeys, so
sql([{}, {id:1}]) still threw 'Cannot INSERT with no columns'. Skip the
guard for INSERT (it already has its own definedColumnCount === 0 check
after widening) so an empty first row is covered by the same widening as
any other missing-key row. Adds test coverage for the empty-first-row
and all-empty boundary cases.
There was a problem hiding this comment.
Both earlier inline comments are addressed and I didn't find anything further, but this changes the user-facing semantics of sql(rows) for heterogeneous row shapes (union-of-keys instead of first-row-only) and relaxes the columnCount === 0 guard for INSERT — worth a maintainer sign-off on the API choice.
What was reviewed:
validateHelperKeyextraction is byte-equivalent to the old inline loop; widened keys now go through it.- Widening is scoped to
autoColumns && multi-row && !array-row, so explicit columns / single-row / positional-array / WHERE IN / UPDATE paths are unchanged. - INSERT-with-zero-columns now falls through to the post-widening
definedColumnCount === 0throw (message changes for the single-{}-row case; still fails loudly). arrayValueSerializernull branch precedes the$isArraycheck so nested-array recursion still handles inner nulls;undefinedpath unchanged.
Extended reasoning...
Overview
Two fixes in src/js/internal/sql/: (1) SQLHelper now records whether its column list was auto-derived (autoColumns), and the INSERT branch of normalizeQuery widens that list to the union of own keys across all rows via a new unionRowKeys helper, so keys first appearing on a later row are no longer silently dropped. The constructor's key-validation loop is extracted to validateHelperKey and applied to widened keys as well. (2) arrayValueSerializer in postgres.ts gains a value === null branch so sql.array emits the unquoted null token instead of the JSON-stringified "null". Tests in sql-helpers-validation.test.ts (no-server sqlite/serialization) and sql.test.ts (docker-gated live Postgres) cover both.
Security risks
None identified. Column names come from Object.keys() (own-enumerable strings only, no prototype walk) and are routed through escapeIdentifier; validateHelperKey gates the widened set the same as the seed set. The sql.array change only narrows what reaches JSON.stringify.
Level of scrutiny
Medium-high: this is shared query-normalization code on the hot path for all three SQL adapters, and it changes user-visible behavior of a public helper. The mechanics are small and well-tested, but the design choice — silently unioning keys vs. requiring explicit columns for heterogeneous batches (or matching whatever postgres.js does) — is an API decision a maintainer should confirm.
Other factors
The PR went through three follow-up commits addressing review feedback (scoping widening to INSERT only, validating widened keys, and letting an empty first row fall through to widening). Test coverage includes the negative contracts (explicit columns still ignore extras, WHERE IN unchanged, all-rows-empty still throws). One incidental behavior change: INSERT ... ${sql([{}])} now throws "Insert needs to have at least one column with a defined value" instead of "Cannot INSERT with no columns" — a message change only. I did not find any correctness issues in the current revision.
This branch has not been deployed
2 participants
What
Two silent data-loss bugs in
Bun.SQLhelpers.Batch insert drops keys not present on the first row
SQLHelperderived the column list fromObject.keys(rows[0])only, so a key that first appears on a later row was never emitted and its value was silently discarded.buildDefinedColumnsAndQueryalready scans every row and null-fills missing values, so the fix is to union the keys across all rows when no explicit column list is given. Explicit columns (sql(rows, "a", "b")) and single-row / positional-array inputs are unchanged.sql.array([..., null, ...])stores the string"null"arrayValueSerializerhad no branch fornull;typeof null === "object"sent it throughJSON.stringifyand produced the quoted string"null". It now emits the unquotednulltoken (SQL NULL in Postgres array-literal syntax), matching the existingundefinedhandling.Tests
test/js/sql/sql-helpers-validation.test.tsgains no-server coverage for both fixes (sqlite in-memory for the insert helper,serializedValuesinspection forsql.array).test/js/sql/sql.test.tsgains live-Postgres round-trip tests under the docker guard.[review] gate passed · iteration 2 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 4 passed · 0 rejected · iteration 2
evidence per changed file