sql: filter out undefined values in INSERT helper instead of treating as NULL #25830
Conversation
|
Updated 11:51 AM PT - Jan 5th, 2026
❌ @alii, your commit dc27bc1 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 25830That installs a local version of the PR into your bun-25830 --bun |
WalkthroughFilters out undefined values in INSERT and UPDATE SQL generation for MySQL, Postgres, and SQLite. Adds a new helper, buildDefinedColumnsAndQuery, to compute defined columns across one-or-many items and uses it to produce column lists, placeholders, and bindings; enforces at least one defined column for INSERT. Changes
Possibly related PRs
Suggested reviewers
Pre-merge checks✅ Passed checks (4 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (4)
🧰 Additional context used📓 Path-based instructions (2)src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js}📄 CodeRabbit inference engine (src/js/CLAUDE.md)
Files:
src/js/{builtins,node,bun,thirdparty,internal}/**/*.ts📄 CodeRabbit inference engine (src/js/CLAUDE.md)
Files:
🧠 Learnings (4)📓 Common learnings📚 Learning: 2025-09-26T01:36:48.705ZApplied to files:
📚 Learning: 2025-09-25T22:07:13.851ZApplied to files:
📚 Learning: 2025-11-24T18:37:11.466ZApplied to files:
🧬 Code graph analysis (1)src/js/internal/sql/shared.ts (2)
🪛 Biome (2.1.2)src/js/internal/sql/mysql.ts[error] 4-4: Shouldn't redeclare 'SQLHelper'. Consider to delete it or rename it. 'SQLHelper' is defined here: (lint/suspicious/noRedeclare) [error] 4-4: Shouldn't redeclare 'SSLMode'. Consider to delete it or rename it. 'SSLMode' is defined here: (lint/suspicious/noRedeclare) [error] 4-4: Shouldn't redeclare 'SQLResultArray'. Consider to delete it or rename it. 'SQLResultArray' is defined here: (lint/suspicious/noRedeclare) src/js/internal/sql/postgres.ts[error] 5-5: Shouldn't redeclare 'SQLHelper'. Consider to delete it or rename it. 'SQLHelper' is defined here: (lint/suspicious/noRedeclare) [error] 6-6: Shouldn't redeclare 'SSLMode'. Consider to delete it or rename it. 'SSLMode' is defined here: (lint/suspicious/noRedeclare) [error] 7-7: Shouldn't redeclare 'SQLResultArray'. Consider to delete it or rename it. 'SQLResultArray' is defined here: (lint/suspicious/noRedeclare) [error] 8-8: Shouldn't redeclare 'SQLArrayParameter'. Consider to delete it or rename it. 'SQLArrayParameter' is defined here: (lint/suspicious/noRedeclare) src/js/internal/sql/sqlite.ts[error] 5-5: Shouldn't redeclare 'SQLHelper'. Consider to delete it or rename it. 'SQLHelper' is defined here: (lint/suspicious/noRedeclare) [error] 5-5: Shouldn't redeclare 'SQLResultArray'. Consider to delete it or rename it. 'SQLResultArray' is defined here: (lint/suspicious/noRedeclare) 🔇 Additional comments (9)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Fix all issues with AI Agents 🤖
In @test/CLAUDE.md:
- Around line 156-181: The "Nested/complex object equality" guidance appears
duplicated in the document; find all headings or blocks titled "Nested/complex
object equality" (and their example code blocks) and remove extra copies so only
a single canonical section remains, preserving the preferred .toEqual example
and its inline CRITICAL comment; ensure imports/links or surrounding references
still point to the remaining section and run a quick spell/format check to keep
the document consistent.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/js/internal/sql/mysql.ts (1)
1108-1143: Consider usinggetDefinedColumnsin UPDATE path for consistency.The UPDATE logic correctly filters undefined values inline (lines 1128-1131) and validates that at least one value exists (lines 1140-1142). However, for consistency with the INSERT path and to avoid the trailing comma cleanup logic (lines 1136-1139), consider refactoring to use
getDefinedColumnsto pre-compute which columns have defined values.This is optional as the current implementation is functionally correct.
🔎 Potential refactor using getDefinedColumns
// UPDATE users SET ${sql({ name: "John", age: 31 })} WHERE id = 1 let item; if ($isArray(items)) { if (items.length > 1) { throw new SyntaxError("Cannot use array of objects for UPDATE"); } item = items[0]; } else { item = items; } + // Filter out undefined columns + const definedColumns = getDefinedColumns(columns, item); + const definedColumnCount = definedColumns.length; + if (definedColumnCount === 0) { + throw new SyntaxError("Update needs to have at least one column with a defined value"); + } + const lastDefinedColumnIndex = definedColumnCount - 1; + // no need to include if is updateSet or upsert const isUpsert = query.trimEnd().endsWith("ON DUPLICATE KEY UPDATE"); if (command === SQLCommand.update && !isUpsert) { query += " SET "; } - let hasValues = false; - for (let i = 0; i < columnCount; i++) { - const column = columns[i]; + for (let i = 0; i < definedColumnCount; i++) { + const column = definedColumns[i]; const columnValue = item[column]; - if (typeof columnValue === "undefined") { - // skip undefined values, this is the expected behavior in JS - continue; - } - hasValues = true; - query += `${this.escapeIdentifier(column)} = ?${i < lastColumnIndex ? ", " : ""}`; + query += `${this.escapeIdentifier(column)} = ?${i < lastDefinedColumnIndex ? ", " : ""}`; binding_values.push(columnValue); } - if (query.endsWith(", ")) { - // we got an undefined value at the end, lets remove the last comma - query = query.substring(0, query.length - 2); - } - if (!hasValues) { - throw new SyntaxError("Update needs to have at least one column"); - } query += " "; // the user can add where clause after this
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
src/js/internal/sql/mysql.tssrc/js/internal/sql/postgres.tssrc/js/internal/sql/shared.tssrc/js/internal/sql/sqlite.tstest/CLAUDE.mdtest/js/sql/sqlite-sql.test.ts
🧰 Additional context used
📓 Path-based instructions (5)
src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js}
📄 CodeRabbit inference engine (src/js/CLAUDE.md)
src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js}: Use.$call()and.$apply()instead of.call()and.apply()to prevent user tampering with function invocation
Use string literalrequire()statements only; dynamic requires are not permitted
Export modules usingexport default { ... }syntax; modules are NOT ES modules
Use JSC intrinsics (prefixed with$) such as$Array.from(),$isCallable(), and$newArrayWithSize()for performance-critical operations
Use private globals and methods with$prefix (e.g.,$Array,map.$set()) instead of public JavaScript globals
Use$debug()for debug logging and$assert()for assertions; both are stripped in release builds
Validate function arguments using validators frominternal/validatorsand throw$ERR_*error codes for invalid arguments
Useprocess.platformandprocess.archfor platform detection; these values are inlined and dead-code eliminated at build time
Files:
src/js/internal/sql/shared.tssrc/js/internal/sql/sqlite.tssrc/js/internal/sql/mysql.tssrc/js/internal/sql/postgres.ts
src/js/{builtins,node,bun,thirdparty,internal}/**/*.ts
📄 CodeRabbit inference engine (src/js/CLAUDE.md)
Builtin functions must include
thisparameter typing in TypeScript to enable direct method binding in C++
Files:
src/js/internal/sql/shared.tssrc/js/internal/sql/sqlite.tssrc/js/internal/sql/mysql.tssrc/js/internal/sql/postgres.ts
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Usebun:testwith files that end in*.test.{ts,js,jsx,tsx,mjs,cjs}
Do not write flaky tests. Never wait for time to pass in tests; always wait for the condition to be met instead of using an arbitrary amount of time
Never use hardcoded port numbers in tests. Always useport: 0to get a random port
Prefer concurrent tests over sequential tests usingtest.concurrentordescribe.concurrentwhen multiple tests spawn processes or write files, unless it's very difficult to make them concurrent
When spawning Bun processes in tests, usebunExeandbunEnvfromharnessto ensure the same build of Bun is used and debug logging is silenced
Use-eflag for single-file tests when spawning Bun processes
UsetempDir()from harness to create temporary directories with files for multi-file tests instead of creating files manually
Prefer async/await over callbacks in tests
When callbacks must be used and it's just a single callback, usePromise.withResolversto create a promise that can be resolved or rejected from a callback
Do not set a timeout on tests. Bun already has timeouts
UseBuffer.alloc(count, fill).toString()instead of'A'.repeat(count)to create repetitive strings in tests, as ''.repeat is very slow in debug JavaScriptCore builds
Usedescribeblocks for grouping related tests
Always useawait usingorusingto ensure proper resource cleanup in tests for APIs like Bun.listen, Bun.connect, Bun.spawn, Bun.serve, etc
Always check exit codes and test error scenarios in error tests
Usedescribe.each()for parameterized tests
UsetoMatchSnapshot()for snapshot testing
UsebeforeAll(),afterEach(),beforeEach()for setup/teardown in tests
Track resources (servers, clients) in arrays for cleanup inafterEach()
Files:
test/js/sql/sqlite-sql.test.ts
**/*.test.ts?(x)
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.ts?(x): Never usebun testdirectly - always usebun bd testto run tests with debug build changes
For single-file tests, prefer-eflag overtempDir
For multi-file tests, prefertempDirandBun.spawnover single-file tests
UsenormalizeBunSnapshotto normalize snapshot output of tests
Never write tests that check for 'panic', 'uncaught exception', or similar strings in test output
UsetempDirfromharnessto create temporary directories - do not usetmpdirSyncorfs.mkdtempSync
When spawning processes in tests, expect stdout before expecting exit code for more useful error messages on test failure
Do not write flaky tests - do not usesetTimeoutin tests; instead await the condition to be met
Verify tests fail withUSE_SYSTEM_BUN=1 bun test <file>and pass withbun bd test <file>- tests are invalid if they pass with USE_SYSTEM_BUN=1
Test files must end with.test.tsor.test.tsx
Avoid shell commands likefindorgrepin tests - use Bun's Glob and built-in tools instead
Files:
test/js/sql/sqlite-sql.test.ts
test/**/*.test.ts?(x)
📄 CodeRabbit inference engine (CLAUDE.md)
Always use
port: 0in tests - do not hardcode ports or use custom random port number functions
Files:
test/js/sql/sqlite-sql.test.ts
🧠 Learnings (19)
📓 Common learnings
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 22946
File: src/js/internal/sql/postgres.ts:1326-1332
Timestamp: 2025-09-26T01:36:48.705Z
Learning: In PostgreSQL single-object INSERT path (src/js/internal/sql/postgres.ts), when handling array values in columnValue, the user prefers to rely on PostgreSQL's native type conversion rather than explicitly checking for typed arrays with isTypedArray(). The current $isArray(columnValue) check with serializeArray(columnValue, "JSON") is sufficient, and native PostgreSQL handling is preferred over explicit typed array detection in this specific INSERT context.
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 22946
File: test/js/sql/sql.test.ts:195-202
Timestamp: 2025-09-25T22:07:13.851Z
Learning: PR oven-sh/bun#22946: JSON/JSONB result parsing updates (e.g., returning parsed arrays instead of legacy strings) are out of scope for this PR; tests keep current expectations with a TODO. Handle parsing fixes in a separate PR.
📚 Learning: 2025-09-26T01:36:48.705Z
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 22946
File: src/js/internal/sql/postgres.ts:1326-1332
Timestamp: 2025-09-26T01:36:48.705Z
Learning: In PostgreSQL single-object INSERT path (src/js/internal/sql/postgres.ts), when handling array values in columnValue, the user prefers to rely on PostgreSQL's native type conversion rather than explicitly checking for typed arrays with isTypedArray(). The current $isArray(columnValue) check with serializeArray(columnValue, "JSON") is sufficient, and native PostgreSQL handling is preferred over explicit typed array detection in this specific INSERT context.
Applied to files:
src/js/internal/sql/shared.tstest/js/sql/sqlite-sql.test.tssrc/js/internal/sql/sqlite.tssrc/js/internal/sql/mysql.tssrc/js/internal/sql/postgres.ts
📚 Learning: 2025-11-24T18:37:30.259Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : When callbacks must be used and it's just a single callback, use `Promise.withResolvers` to create a promise that can be resolved or rejected from a callback
Applied to files:
test/CLAUDE.md
📚 Learning: 2025-09-05T19:48:44.481Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 21728
File: src/valkey/valkey.zig:0-0
Timestamp: 2025-09-05T19:48:44.481Z
Learning: In ValkeyCommand.Promise, when you need to resolve with already-converted JSValues (not RESPValues), use promise_ptr.promise.resolve() to bypass the RESP-to-JS conversion wrapper. The ValkeyCommand.Promise.resolve() method is for converting RESPValues, while direct .promise access is for pre-converted JSValues.
Applied to files:
test/CLAUDE.md
📚 Learning: 2025-09-03T01:30:58.001Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 21728
File: test/js/valkey/valkey.test.ts:264-271
Timestamp: 2025-09-03T01:30:58.001Z
Learning: For test/js/valkey/valkey.test.ts PUB/SUB tests, avoid arbitrary sleeps and async-forEach. Instead, resolve a Promise from the subscriber callback when the expected number of messages is observed and await it with a bounded timeout (e.g., withTimeout + Promise.withResolvers) to account for Redis server→subscriber propagation.
Applied to files:
test/CLAUDE.md
📚 Learning: 2025-11-24T18:37:30.259Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Always use `await using` or `using` to ensure proper resource cleanup in tests for APIs like Bun.listen, Bun.connect, Bun.spawn, Bun.serve, etc
Applied to files:
test/CLAUDE.md
📚 Learning: 2025-09-05T19:48:44.481Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 21728
File: src/valkey/valkey.zig:0-0
Timestamp: 2025-09-05T19:48:44.481Z
Learning: In ValkeyCommand.zig, the Promise type is a wrapper that encapsulates valkey-specific behavior. The actual underlying promise is accessed via the `.promise` field, so the correct pattern is `promise_ptr.promise.resolve(...)` not `promise_ptr.resolve(...)`.
Applied to files:
test/CLAUDE.md
📚 Learning: 2025-11-24T18:37:30.259Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Prefer async/await over callbacks in tests
Applied to files:
test/CLAUDE.md
📚 Learning: 2025-11-24T18:37:30.259Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Unit tests for specific features are organized by module (e.g., `/test/js/bun/`, `/test/js/node/`)
Applied to files:
test/CLAUDE.md
📚 Learning: 2025-11-24T18:37:30.259Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `toMatchSnapshot()` for snapshot testing
Applied to files:
test/CLAUDE.md
📚 Learning: 2025-11-24T18:37:30.259Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `beforeAll()`, `afterEach()`, `beforeEach()` for setup/teardown in tests
Applied to files:
test/CLAUDE.md
📚 Learning: 2025-11-24T18:37:30.259Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `describe` blocks for grouping related tests
Applied to files:
test/CLAUDE.md
📚 Learning: 2025-11-24T18:37:30.259Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : When spawning Bun processes in tests, use `bunExe` and `bunEnv` from `harness` to ensure the same build of Bun is used and debug logging is silenced
Applied to files:
test/CLAUDE.md
📚 Learning: 2025-10-19T02:44:46.354Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: packages/bun-otel/context-propagation.test.ts:1-1
Timestamp: 2025-10-19T02:44:46.354Z
Learning: In the Bun repository, standalone packages under packages/ (e.g., bun-vscode, bun-inspector-protocol, bun-plugin-yaml, bun-plugin-svelte, bun-debug-adapter-protocol, bun-otel) co-locate their tests with package source code using *.test.ts files. This follows standard npm/monorepo patterns. The test/ directory hierarchy (test/js/bun/, test/cli/, test/js/node/) is reserved for testing Bun's core runtime APIs and built-in functionality, not standalone packages.
Applied to files:
test/CLAUDE.md
📚 Learning: 2025-11-24T18:37:30.259Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `tempDir()` from harness to create temporary directories with files for multi-file tests instead of creating files manually
Applied to files:
test/CLAUDE.md
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : For multi-file tests, prefer `tempDir` and `Bun.spawn` over single-file tests
Applied to files:
test/CLAUDE.md
📚 Learning: 2025-11-24T18:37:30.259Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Always check exit codes and test error scenarios in error tests
Applied to files:
test/CLAUDE.md
📚 Learning: 2025-09-30T02:56:30.615Z
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 22700
File: test/js/sql/sql.test.ts:11673-11710
Timestamp: 2025-09-30T02:56:30.615Z
Learning: Repository oven-sh/bun tests run in a Docker image where the pgcrypto extension is pre-installed, so gen_random_uuid() is available without explicitly running CREATE EXTENSION in tests (e.g., in test/js/sql/sql.test.ts “upsert helper”).
Applied to files:
test/js/sql/sqlite-sql.test.ts
📚 Learning: 2025-09-25T22:07:13.851Z
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 22946
File: test/js/sql/sql.test.ts:195-202
Timestamp: 2025-09-25T22:07:13.851Z
Learning: PR oven-sh/bun#22946: JSON/JSONB result parsing updates (e.g., returning parsed arrays instead of legacy strings) are out of scope for this PR; tests keep current expectations with a TODO. Handle parsing fixes in a separate PR.
Applied to files:
src/js/internal/sql/postgres.ts
🧬 Code graph analysis (2)
src/js/internal/sql/mysql.ts (1)
src/js/internal/sql/shared.ts (1)
DatabaseAdapter(908-938)
src/js/internal/sql/postgres.ts (1)
src/js/internal/sql/shared.ts (1)
DatabaseAdapter(908-938)
🪛 Biome (2.1.2)
src/js/internal/sql/sqlite.ts
[error] 5-5: Shouldn't redeclare 'SQLHelper'. Consider to delete it or rename it.
'SQLHelper' is defined here:
(lint/suspicious/noRedeclare)
[error] 5-5: Shouldn't redeclare 'SQLResultArray'. Consider to delete it or rename it.
'SQLResultArray' is defined here:
(lint/suspicious/noRedeclare)
src/js/internal/sql/mysql.ts
[error] 4-4: Shouldn't redeclare 'SQLHelper'. Consider to delete it or rename it.
'SQLHelper' is defined here:
(lint/suspicious/noRedeclare)
[error] 4-4: Shouldn't redeclare 'SSLMode'. Consider to delete it or rename it.
'SSLMode' is defined here:
(lint/suspicious/noRedeclare)
[error] 4-4: Shouldn't redeclare 'SQLResultArray'. Consider to delete it or rename it.
'SQLResultArray' is defined here:
(lint/suspicious/noRedeclare)
src/js/internal/sql/postgres.ts
[error] 4-4: Shouldn't redeclare 'SQLHelper'. Consider to delete it or rename it.
'SQLHelper' is defined here:
(lint/suspicious/noRedeclare)
[error] 4-4: Shouldn't redeclare 'SSLMode'. Consider to delete it or rename it.
'SSLMode' is defined here:
(lint/suspicious/noRedeclare)
[error] 4-4: Shouldn't redeclare 'SQLResultArray'. Consider to delete it or rename it.
'SQLResultArray' is defined here:
(lint/suspicious/noRedeclare)
[error] 4-4: Shouldn't redeclare 'SQLArrayParameter'. Consider to delete it or rename it.
'SQLArrayParameter' is defined here:
(lint/suspicious/noRedeclare)
🔇 Additional comments (9)
test/CLAUDE.md (1)
110-114: Promise.withResolvers generic example is clear and correctExplicit
<void>here nicely signals that the resolution type is configurable; no issues with the example.src/js/internal/sql/shared.ts (2)
180-210: getDefinedColumns correctly computes the union of defined columnsThe helper does exactly what the INSERT helpers need: it walks the provided
columnsand includes each one if any item has a non-undefinedvalue for it, while treatingnulland other falsy values as defined. The T vs T[] split via$isArray(items)keeps the single-object path cheap and avoids extra allocations. This aligns with the new tests and the intended “omit only truly undefined columns” behavior.
940-952: Adding getDefinedColumns to the shared export is consistentExporting
getDefinedColumnsalongsideSQLHelperand friends is the right place for this shared insert/update utility; consumers (Postgres/MySQL/SQLite adapters) can now depend on a single implementation.src/js/internal/sql/postgres.ts (2)
4-4: Runtime import updated to expose getDefinedColumnsPulling
getDefinedColumnsfrominternal/sql/sharedalongsideSQLHelper/SQLResultArrayfits the existing pattern of type-only imports plus runtime requires; no behavioral concerns here.
1255-1263: INSERT helper now omits undefined-only columns and fixes bulk data lossThe refactored INSERT branch looks correct:
definedColumns = getDefinedColumns(columns, items)computes the union of columns with at least one non-undefinedvalue, so columns that areundefinedeverywhere are completely omitted from the INSERT, allowing DEFAULT/NOT NULL behavior to work as intended.- When
definedColumnCount === 0, throwingSyntaxError("Insert needs to have at least one column with a defined value")is appropriate and matches the new tests.- For multi-row inserts, using
definedColumnsfor both the column list and placeholder generation, and pushingnullwhen a particular row hasundefinedfor a column that exists in other rows, preserves values that only appear in later rows and eliminates the earlier “first-row-only” data-loss bug.- For single-row inserts,
definedColumnsguarantees thatcolumnValueis neverundefined, so bindings cannot accidentally reintroduce the oldNULL-for-undefined behavior.Overall, the placeholder numbering (
$${binding_idx++}) and binding order stay consistent across rows, so the generated SQL and bindings line up correctly.Also applies to: 1278-1284, 1294-1299
src/js/internal/sql/sqlite.ts (2)
5-5: SQLite shared import extended with getDefinedColumnsReusing
getDefinedColumnsfrominternal/sql/sharedfor the SQLite adapter keeps INSERT semantics aligned with the other adapters; the import itself is consistent with the existing pattern.
436-444: SQLite INSERT helper correctly filters undefined columns and preserves bulk valuesThe new INSERT branch achieves the intended semantics:
- It derives
definedColumnsfrom all items, so columns that areundefinedeverywhere are fully omitted from the generated(col1, col2, ...)list, letting SQLite defaults (or implicit NULL) apply.- When no column has a defined value, it throws
SyntaxError("Insert needs to have at least one column with a defined value"), which is what the helper-validation tests now assert.- For array inputs, it uses
definedColumnsboth for the column list and for generating?placeholders, and it pushesnullfor any per-rowundefinedwhere the column is present for other rows, preventing the earlier “column inferred from first row only” data loss.- For single-object inserts, the helper now never binds
undefinedvalues at all; those columns are simply dropped from the SQL, aligning with the NOT NULL + DEFAULT regression test.This matches the new sqlite tests’ expectations and keeps placeholder/binding alignment correct.
Also applies to: 447-482
src/js/internal/sql/mysql.ts (2)
1022-1028: Undefined filtering logic looks solid.The use of
getDefinedColumnscorrectly computes the union of columns with defined values across all items, and the error handling ensures at least one column is present. This properly addresses the PR objective of omitting undefined values from INSERT statements.
1031-1068: INSERT column generation correctly handles both single and multi-item cases.The logic properly:
- Builds the column list from
definedColumnswith escaped identifiers- For multi-item inserts (line 1050), correctly injects
nullfor columns that are defined in some items but undefined in others, ensuring all value tuples have the same column structure- For single-item inserts (line 1065), safely binds values directly since
definedColumnsonly includes columns with defined values in that item
| const column = columns[k]; | ||
| for (let k = 0; k < definedColumnCount; k++) { | ||
| const column = definedColumns[k]; | ||
| const columnValue = item[column]; |
There was a problem hiding this comment.
we can just check the columnValue here instead of looping twice using getDefinedColumns + having the right index of the actually valid values instead of k
Address review feedback by moving the inline defined column detection into a shared `buildDefinedColumnsAndQuery` function in shared.ts. This combines column name generation with defined column detection in a single pass, reducing code duplication across all SQL adapters.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/js/internal/sql/sqlite.ts (1)
537-551: Dead code in UPDATE path.The check at lines 546-550 is unreachable because line 540-542 already continues when
columnValueis undefined. The else branch will never execute.🔎 Proposed fix to remove dead code
for (let i = 0; i < columnCount; i++) { const column = columns[i]; const columnValue = item[column]; if (typeof columnValue === "undefined") { // skip undefined values, this is the expected behavior in JS continue; } // SQLite uses ? for placeholders query += `${this.escapeIdentifier(column)} = ?${i < lastColumnIndex ? ", " : ""}`; - if (typeof columnValue === "undefined") { - binding_values.push(null); - } else { - binding_values.push(columnValue); - } + binding_values.push(columnValue); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
src/js/internal/sql/mysql.tssrc/js/internal/sql/postgres.tssrc/js/internal/sql/shared.tssrc/js/internal/sql/sqlite.ts
🧰 Additional context used
📓 Path-based instructions (2)
src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js}
📄 CodeRabbit inference engine (src/js/CLAUDE.md)
src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js}: Use.$call()and.$apply()instead of.call()and.apply()to prevent user tampering with function invocation
Use string literalrequire()statements only; dynamic requires are not permitted
Export modules usingexport default { ... }syntax; modules are NOT ES modules
Use JSC intrinsics (prefixed with$) such as$Array.from(),$isCallable(), and$newArrayWithSize()for performance-critical operations
Use private globals and methods with$prefix (e.g.,$Array,map.$set()) instead of public JavaScript globals
Use$debug()for debug logging and$assert()for assertions; both are stripped in release builds
Validate function arguments using validators frominternal/validatorsand throw$ERR_*error codes for invalid arguments
Useprocess.platformandprocess.archfor platform detection; these values are inlined and dead-code eliminated at build time
Files:
src/js/internal/sql/shared.tssrc/js/internal/sql/mysql.tssrc/js/internal/sql/postgres.tssrc/js/internal/sql/sqlite.ts
src/js/{builtins,node,bun,thirdparty,internal}/**/*.ts
📄 CodeRabbit inference engine (src/js/CLAUDE.md)
Builtin functions must include
thisparameter typing in TypeScript to enable direct method binding in C++
Files:
src/js/internal/sql/shared.tssrc/js/internal/sql/mysql.tssrc/js/internal/sql/postgres.tssrc/js/internal/sql/sqlite.ts
🧠 Learnings (4)
📓 Common learnings
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 22946
File: test/js/sql/sql.test.ts:195-202
Timestamp: 2025-09-25T22:07:13.851Z
Learning: PR oven-sh/bun#22946: JSON/JSONB result parsing updates (e.g., returning parsed arrays instead of legacy strings) are out of scope for this PR; tests keep current expectations with a TODO. Handle parsing fixes in a separate PR.
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 22946
File: src/js/internal/sql/postgres.ts:1326-1332
Timestamp: 2025-09-26T01:36:48.705Z
Learning: In PostgreSQL single-object INSERT path (src/js/internal/sql/postgres.ts), when handling array values in columnValue, the user prefers to rely on PostgreSQL's native type conversion rather than explicitly checking for typed arrays with isTypedArray(). The current $isArray(columnValue) check with serializeArray(columnValue, "JSON") is sufficient, and native PostgreSQL handling is preferred over explicit typed array detection in this specific INSERT context.
📚 Learning: 2025-09-26T01:36:48.705Z
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 22946
File: src/js/internal/sql/postgres.ts:1326-1332
Timestamp: 2025-09-26T01:36:48.705Z
Learning: In PostgreSQL single-object INSERT path (src/js/internal/sql/postgres.ts), when handling array values in columnValue, the user prefers to rely on PostgreSQL's native type conversion rather than explicitly checking for typed arrays with isTypedArray(). The current $isArray(columnValue) check with serializeArray(columnValue, "JSON") is sufficient, and native PostgreSQL handling is preferred over explicit typed array detection in this specific INSERT context.
Applied to files:
src/js/internal/sql/mysql.tssrc/js/internal/sql/postgres.tssrc/js/internal/sql/sqlite.ts
📚 Learning: 2025-09-25T22:07:13.851Z
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 22946
File: test/js/sql/sql.test.ts:195-202
Timestamp: 2025-09-25T22:07:13.851Z
Learning: PR oven-sh/bun#22946: JSON/JSONB result parsing updates (e.g., returning parsed arrays instead of legacy strings) are out of scope for this PR; tests keep current expectations with a TODO. Handle parsing fixes in a separate PR.
Applied to files:
src/js/internal/sql/postgres.ts
📚 Learning: 2025-11-24T18:37:11.466Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:11.466Z
Learning: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js} : Validate function arguments using validators from `internal/validators` and throw `$ERR_*` error codes for invalid arguments
Applied to files:
src/js/internal/sql/postgres.ts
🧬 Code graph analysis (1)
src/js/internal/sql/shared.ts (2)
src/js/internal/sql/mysql.ts (1)
escapeIdentifier(480-482)src/js/internal/sql/postgres.ts (1)
escapeIdentifier(708-710)
🪛 Biome (2.1.2)
src/js/internal/sql/mysql.ts
[error] 4-4: Shouldn't redeclare 'SQLHelper'. Consider to delete it or rename it.
'SQLHelper' is defined here:
(lint/suspicious/noRedeclare)
[error] 4-4: Shouldn't redeclare 'SSLMode'. Consider to delete it or rename it.
'SSLMode' is defined here:
(lint/suspicious/noRedeclare)
[error] 4-4: Shouldn't redeclare 'SQLResultArray'. Consider to delete it or rename it.
'SQLResultArray' is defined here:
(lint/suspicious/noRedeclare)
src/js/internal/sql/postgres.ts
[error] 5-5: Shouldn't redeclare 'SQLHelper'. Consider to delete it or rename it.
'SQLHelper' is defined here:
(lint/suspicious/noRedeclare)
[error] 6-6: Shouldn't redeclare 'SSLMode'. Consider to delete it or rename it.
'SSLMode' is defined here:
(lint/suspicious/noRedeclare)
[error] 7-7: Shouldn't redeclare 'SQLResultArray'. Consider to delete it or rename it.
'SQLResultArray' is defined here:
(lint/suspicious/noRedeclare)
[error] 8-8: Shouldn't redeclare 'SQLArrayParameter'. Consider to delete it or rename it.
'SQLArrayParameter' is defined here:
(lint/suspicious/noRedeclare)
src/js/internal/sql/sqlite.ts
[error] 5-5: Shouldn't redeclare 'SQLHelper'. Consider to delete it or rename it.
'SQLHelper' is defined here:
(lint/suspicious/noRedeclare)
[error] 5-5: Shouldn't redeclare 'SQLResultArray'. Consider to delete it or rename it.
'SQLResultArray' is defined here:
(lint/suspicious/noRedeclare)
🔇 Additional comments (9)
src/js/internal/sql/shared.ts (2)
180-219: Well-implemented helper for determining defined columns.The implementation correctly:
- Scans all items to determine which columns have at least one defined value
- Builds the column list SQL fragment with proper escaping
- Handles both single-item and array cases
One minor observation: the function signature uses
(keyof T)[]for columns but casts tostringwhen callingescapeIdentifier. This works correctly since column names are always strings at runtime.
955-955: Export added correctly.The new helper is properly exported for use by the database adapters.
src/js/internal/sql/postgres.ts (2)
4-10: Import pattern is correct for Bun builtins.The static analysis warnings about redeclaration are false positives. This pattern of importing types separately and using
require()for runtime values is the standard approach in Bun's builtin modules, as documented in the coding guidelines.
1261-1304: INSERT path correctly updated to filter undefined values.The implementation properly:
- Uses
buildDefinedColumnsAndQueryto determine which columns have defined values across all items- Throws a clear error when no columns have defined values
- For multi-item inserts (line 1286): correctly converts undefined to null for columns that other items define
- For single-item inserts (line 1301): correctly pushes the value directly since
definedColumnsonly contains columns with defined valuessrc/js/internal/sql/sqlite.ts (2)
5-5: Import pattern is correct for Bun builtins.The static analysis warnings are false positives, as explained for the postgres adapter.
436-481: INSERT path correctly mirrors PostgreSQL implementation.The SQLite adapter's INSERT logic is consistent with the PostgreSQL adapter:
- Uses
buildDefinedColumnsAndQueryto consolidate column detection- Correctly handles multi-item inserts with undefined → null conversion
- Single-item path correctly pushes defined values directly
This addresses the past review comment about consolidating the logic into a shared helper.
src/js/internal/sql/mysql.ts (3)
4-4: Import pattern is correct for Bun builtins.The static analysis warnings are false positives, consistent with the other adapters.
1022-1065: INSERT path correctly mirrors other adapters.The MySQL adapter's INSERT logic is consistent with PostgreSQL and SQLite:
- Uses
buildDefinedColumnsAndQueryto consolidate column detection- Correctly handles multi-item inserts with undefined → null conversion
- Single-item path correctly pushes defined values directly
1105-1141: UPDATE path correctly handles undefined values.The UPDATE logic properly:
- Skips undefined values (line 1125-1128)
- Tracks whether any values were added with
hasValues- Throws a clear error if no columns have values
- Handles the trailing comma cleanup
What does this PR do?
the
sql()helper now filters outundefinedvalues in INSERT statements instead of converting them toNULL. This allows columns withDEFAULTvalues to use their defaults whenundefinedis passed, rather than being overridden withNULL.Before:
sql({ foo: undefined, id: "123" })in INSERT would generate(foo, id) VALUES (NULL, "123"), causing NOT NULL constraint violations even when the column has a DEFAULT.After:
sql({ foo: undefined, id: "123" })in INSERT generates(id) VALUES ("123"), omitting the undefined column entirely and letting the database use the DEFAULT value.Also fixes a data loss bug in bulk inserts where columns were determined only from the first item - now all items are checked, so values in later items aren't silently dropped.
Fixes #25829
How did you verify your code works?
INSERTqueries #25829 (NOT NULL column with DEFAULT)