Skip to content

Commit bf937f7

Browse files
aliialii
authored
sql: filter out undefined values in INSERT helper instead of treating as NULL (#25830)
### What does this PR do? the `sql()` helper now filters out `undefined` values in INSERT statements instead of converting them to `NULL`. This allows columns with `DEFAULT` values to use their defaults when `undefined` is passed, rather than being overridden with `NULL`. **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? - Added regression test for #25829 (NOT NULL column with DEFAULT) - Added tests for bulk insert with mixed undefined patterns which is the data loss scenario
1 parent ce97887 commit bf937f7

File tree

6 files changed

+242
-83
lines changed

6 files changed

+242
-83
lines changed

‎src/js/internal/sql/mysql.ts‎

Lines changed: 27 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import type { MySQLErrorOptions } from "internal/sql/errors";
22
import type { Query } from "./query";
33
import type { ArrayType, DatabaseAdapter, SQLArrayParameter, SQLHelper, SQLResultArray, SSLMode } from "./shared";
4-
const { SQLHelper, SSLMode, SQLResultArray } = require("internal/sql/shared");
4+
const { SQLHelper, SSLMode, SQLResultArray, buildDefinedColumnsAndQuery } = require("internal/sql/shared");
55
const {
66
Query,
77
SQLQueryFlags,
@@ -455,11 +455,10 @@ class PooledMySQLConnection {
455455
}
456456
}
457457

458-
class MySQLAdapter implements DatabaseAdapter<
459-
PooledMySQLConnection,
460-
$ZigGeneratedClasses.MySQLConnection,
461-
$ZigGeneratedClasses.MySQLQuery
462-
> {
458+
class MySQLAdapter
459+
implements
460+
DatabaseAdapter<PooledMySQLConnection, $ZigGeneratedClasses.MySQLConnection, $ZigGeneratedClasses.MySQLQuery>
461+
{
463462
public readonly connectionInfo: Bun.SQL.__internal.DefinedPostgresOrMySQLOptions;
464463

465464
public readonly connections: PooledMySQLConnection[];
@@ -1020,29 +1019,32 @@ class MySQLAdapter implements DatabaseAdapter<
10201019
// insert into users ${sql(users)} or insert into users ${sql(user)}
10211020
//
10221021

1023-
query += "(";
1024-
for (let j = 0; j < columnCount; j++) {
1025-
query += this.escapeIdentifier(columns[j]);
1026-
if (j < lastColumnIndex) {
1027-
query += ", ";
1028-
}
1022+
// Build column list while determining which columns have at least one defined value
1023+
const { definedColumns, columnsSql } = buildDefinedColumnsAndQuery(
1024+
columns,
1025+
items,
1026+
this.escapeIdentifier.bind(this),
1027+
);
1028+
1029+
const definedColumnCount = definedColumns.length;
1030+
if (definedColumnCount === 0) {
1031+
throw new SyntaxError("Insert needs to have at least one column with a defined value");
10291032
}
1030-
query += ") VALUES";
1033+
const lastDefinedColumnIndex = definedColumnCount - 1;
1034+
1035+
query += columnsSql;
10311036
if ($isArray(items)) {
10321037
const itemsCount = items.length;
10331038
const lastItemIndex = itemsCount - 1;
10341039
for (let j = 0; j < itemsCount; j++) {
10351040
query += "(";
10361041
const item = items[j];
1037-
for (let k = 0; k < columnCount; k++) {
1038-
const column = columns[k];
1042+
for (let k = 0; k < definedColumnCount; k++) {
1043+
const column = definedColumns[k];
10391044
const columnValue = item[column];
1040-
query += `?${k < lastColumnIndex ? ", " : ""}`;
1041-
if (typeof columnValue === "undefined") {
1042-
binding_values.push(null);
1043-
} else {
1044-
binding_values.push(columnValue);
1045-
}
1045+
query += `?${k < lastDefinedColumnIndex ? ", " : ""}`;
1046+
// If this item has undefined for a column that other items defined, use null
1047+
binding_values.push(typeof columnValue === "undefined" ? null : columnValue);
10461048
}
10471049
if (j < lastItemIndex) {
10481050
query += "),";
@@ -1053,15 +1055,11 @@ class MySQLAdapter implements DatabaseAdapter<
10531055
} else {
10541056
query += "(";
10551057
const item = items;
1056-
for (let j = 0; j < columnCount; j++) {
1057-
const column = columns[j];
1058+
for (let j = 0; j < definedColumnCount; j++) {
1059+
const column = definedColumns[j];
10581060
const columnValue = item[column];
1059-
query += `?${j < lastColumnIndex ? ", " : ""}`;
1060-
if (typeof columnValue === "undefined") {
1061-
binding_values.push(null);
1062-
} else {
1063-
binding_values.push(columnValue);
1064-
}
1061+
query += `?${j < lastDefinedColumnIndex ? ", " : ""}`;
1062+
binding_values.push(columnValue);
10651063
}
10661064
query += ") "; // the user can add RETURNING * or RETURNING id
10671065
}

‎src/js/internal/sql/postgres.ts‎

Lines changed: 37 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,13 @@
11
import type { PostgresErrorOptions } from "internal/sql/errors";
22
import type { Query } from "./query";
33
import type { ArrayType, DatabaseAdapter, SQLArrayParameter, SQLHelper, SQLResultArray, SSLMode } from "./shared";
4-
const { SQLHelper, SSLMode, SQLResultArray, SQLArrayParameter } = require("internal/sql/shared");
4+
const {
5+
SQLHelper,
6+
SSLMode,
7+
SQLResultArray,
8+
SQLArrayParameter,
9+
buildDefinedColumnsAndQuery,
10+
} = require("internal/sql/shared");
511
const {
612
Query,
713
SQLQueryFlags,
@@ -672,11 +678,14 @@ class PooledPostgresConnection {
672678
}
673679
}
674680

675-
class PostgresAdapter implements DatabaseAdapter<
676-
PooledPostgresConnection,
677-
$ZigGeneratedClasses.PostgresSQLConnection,
678-
$ZigGeneratedClasses.PostgresSQLQuery
679-
> {
681+
class PostgresAdapter
682+
implements
683+
DatabaseAdapter<
684+
PooledPostgresConnection,
685+
$ZigGeneratedClasses.PostgresSQLConnection,
686+
$ZigGeneratedClasses.PostgresSQLQuery
687+
>
688+
{
680689
public readonly connectionInfo: Bun.SQL.__internal.DefinedPostgresOrMySQLOptions;
681690

682691
public readonly connections: PooledPostgresConnection[];
@@ -1249,29 +1258,32 @@ class PostgresAdapter implements DatabaseAdapter<
12491258
// insert into users ${sql(users)} or insert into users ${sql(user)}
12501259
//
12511260

1252-
query += "(";
1253-
for (let j = 0; j < columnCount; j++) {
1254-
query += this.escapeIdentifier(columns[j]);
1255-
if (j < lastColumnIndex) {
1256-
query += ", ";
1257-
}
1261+
// Build column list while determining which columns have at least one defined value
1262+
const { definedColumns, columnsSql } = buildDefinedColumnsAndQuery(
1263+
columns,
1264+
items,
1265+
this.escapeIdentifier.bind(this),
1266+
);
1267+
1268+
const definedColumnCount = definedColumns.length;
1269+
if (definedColumnCount === 0) {
1270+
throw new SyntaxError("Insert needs to have at least one column with a defined value");
12581271
}
1259-
query += ") VALUES";
1272+
const lastDefinedColumnIndex = definedColumnCount - 1;
1273+
1274+
query += columnsSql;
12601275
if ($isArray(items)) {
12611276
const itemsCount = items.length;
12621277
const lastItemIndex = itemsCount - 1;
12631278
for (let j = 0; j < itemsCount; j++) {
12641279
query += "(";
12651280
const item = items[j];
1266-
for (let k = 0; k < columnCount; k++) {
1267-
const column = columns[k];
1281+
for (let k = 0; k < definedColumnCount; k++) {
1282+
const column = definedColumns[k];
12681283
const columnValue = item[column];
1269-
query += `$${binding_idx++}${k < lastColumnIndex ? ", " : ""}`;
1270-
if (typeof columnValue === "undefined") {
1271-
binding_values.push(null);
1272-
} else {
1273-
binding_values.push(columnValue);
1274-
}
1284+
query += `$${binding_idx++}${k < lastDefinedColumnIndex ? ", " : ""}`;
1285+
// If this item has undefined for a column that other items defined, use null
1286+
binding_values.push(typeof columnValue === "undefined" ? null : columnValue);
12751287
}
12761288
if (j < lastItemIndex) {
12771289
query += "),";
@@ -1282,15 +1294,11 @@ class PostgresAdapter implements DatabaseAdapter<
12821294
} else {
12831295
query += "(";
12841296
const item = items;
1285-
for (let j = 0; j < columnCount; j++) {
1286-
const column = columns[j];
1297+
for (let j = 0; j < definedColumnCount; j++) {
1298+
const column = definedColumns[j];
12871299
const columnValue = item[column];
1288-
query += `$${binding_idx++}${j < lastColumnIndex ? ", " : ""}`;
1289-
if (typeof columnValue === "undefined") {
1290-
binding_values.push(null);
1291-
} else {
1292-
binding_values.push(columnValue);
1293-
}
1300+
query += `$${binding_idx++}${j < lastDefinedColumnIndex ? ", " : ""}`;
1301+
binding_values.push(columnValue);
12941302
}
12951303
query += ") "; // the user can add RETURNING * or RETURNING id
12961304
}

‎src/js/internal/sql/shared.ts‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -177,6 +177,47 @@ class SQLHelper<T> {
177177
}
178178
}
179179

180+
/**
181+
* Build the column list for INSERT statements while determining which columns have defined values.
182+
* This combines column name generation with defined column detection in a single pass.
183+
* Returns the defined columns array and the SQL fragment for the column names.
184+
*/
185+
function buildDefinedColumnsAndQuery<T>(
186+
columns: (keyof T)[],
187+
items: T | T[],
188+
escapeIdentifier: (name: string) => string,
189+
): { definedColumns: (keyof T)[]; columnsSql: string } {
190+
const definedColumns: (keyof T)[] = [];
191+
let columnsSql = "(";
192+
const columnCount = columns.length;
193+
194+
for (let k = 0; k < columnCount; k++) {
195+
const column = columns[k];
196+
197+
// Check if any item has this column defined
198+
let hasDefinedValue = false;
199+
if ($isArray(items)) {
200+
for (let j = 0; j < items.length; j++) {
201+
if (typeof items[j][column] !== "undefined") {
202+
hasDefinedValue = true;
203+
break;
204+
}
205+
}
206+
} else {
207+
hasDefinedValue = typeof items[column] !== "undefined";
208+
}
209+
210+
if (hasDefinedValue) {
211+
if (definedColumns.length > 0) columnsSql += ", ";
212+
columnsSql += escapeIdentifier(column as string);
213+
definedColumns.push(column);
214+
}
215+
}
216+
217+
columnsSql += ") VALUES";
218+
return { definedColumns, columnsSql };
219+
}
220+
180221
const SQLITE_MEMORY = ":memory:";
181222
const SQLITE_MEMORY_VARIANTS: string[] = [":memory:", "sqlite://:memory:", "sqlite:memory"];
182223

@@ -911,6 +952,7 @@ export default {
911952
assertIsOptionsOfAdapter,
912953
parseOptions,
913954
SQLHelper,
955+
buildDefinedColumnsAndQuery,
914956
normalizeSSLMode,
915957
SQLResultArray,
916958
SQLArrayParameter,

‎src/js/internal/sql/sqlite.ts‎

Lines changed: 23 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import type * as BunSQLiteModule from "bun:sqlite";
22
import type { BaseQueryHandle, Query, SQLQueryResultMode } from "./query";
33
import type { ArrayType, DatabaseAdapter, OnConnected, SQLArrayParameter, SQLHelper, SQLResultArray } from "./shared";
44

5-
const { SQLHelper, SQLResultArray } = require("internal/sql/shared");
5+
const { SQLHelper, SQLResultArray, buildDefinedColumnsAndQuery } = require("internal/sql/shared");
66
const {
77
Query,
88
SQLQueryResultMode,
@@ -433,30 +433,33 @@ class SQLiteAdapter implements DatabaseAdapter<BunSQLiteModule.Database, BunSQLi
433433
// insert into users ${sql(users)} or insert into users ${sql(user)}
434434
//
435435

436-
query += "(";
437-
for (let j = 0; j < columnCount; j++) {
438-
query += this.escapeIdentifier(columns[j]);
439-
if (j < lastColumnIndex) {
440-
query += ", ";
441-
}
436+
// Build column list while determining which columns have at least one defined value
437+
const { definedColumns, columnsSql } = buildDefinedColumnsAndQuery(
438+
columns,
439+
items,
440+
this.escapeIdentifier.bind(this),
441+
);
442+
443+
const definedColumnCount = definedColumns.length;
444+
if (definedColumnCount === 0) {
445+
throw new SyntaxError("Insert needs to have at least one column with a defined value");
442446
}
443-
query += ") VALUES";
447+
const lastDefinedColumnIndex = definedColumnCount - 1;
448+
449+
query += columnsSql;
444450
if ($isArray(items)) {
445451
const itemsCount = items.length;
446452
const lastItemIndex = itemsCount - 1;
447453
for (let j = 0; j < itemsCount; j++) {
448454
query += "(";
449455
const item = items[j];
450-
for (let k = 0; k < columnCount; k++) {
451-
const column = columns[k];
456+
for (let k = 0; k < definedColumnCount; k++) {
457+
const column = definedColumns[k];
452458
const columnValue = item[column];
453459
// SQLite uses ? for placeholders, not $1, $2, etc.
454-
query += `?${k < lastColumnIndex ? ", " : ""}`;
455-
if (typeof columnValue === "undefined") {
456-
binding_values.push(null);
457-
} else {
458-
binding_values.push(columnValue);
459-
}
460+
query += `?${k < lastDefinedColumnIndex ? ", " : ""}`;
461+
// If this item has undefined for a column that other items defined, use null
462+
binding_values.push(typeof columnValue === "undefined" ? null : columnValue);
460463
}
461464
if (j < lastItemIndex) {
462465
query += "),";
@@ -467,16 +470,12 @@ class SQLiteAdapter implements DatabaseAdapter<BunSQLiteModule.Database, BunSQLi
467470
} else {
468471
query += "(";
469472
const item = items;
470-
for (let j = 0; j < columnCount; j++) {
471-
const column = columns[j];
473+
for (let j = 0; j < definedColumnCount; j++) {
474+
const column = definedColumns[j];
472475
const columnValue = item[column];
473476
// SQLite uses ? for placeholders
474-
query += `?${j < lastColumnIndex ? ", " : ""}`;
475-
if (typeof columnValue === "undefined") {
476-
binding_values.push(null);
477-
} else {
478-
binding_values.push(columnValue);
479-
}
477+
query += `?${j < lastDefinedColumnIndex ? ", " : ""}`;
478+
binding_values.push(columnValue);
480479
}
481480
query += ") "; // the user can add RETURNING * or RETURNING id
482481
}

‎test/CLAUDE.md‎

Lines changed: 28 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -107,7 +107,7 @@ When callbacks must be used and it's just a single callback, use `Promise.withRe
107107
108108
```ts
109109
const ws = new WebSocket("ws://localhost:8080");
110-
const { promise, resolve, reject } = Promise.withResolvers();
110+
const { promise, resolve, reject } = Promise.withResolvers<void>(); // Can specify any type here for resolution value
111111
ws.onopen = resolve;
112112
ws.onclose = reject;
113113
await promise;
@@ -153,6 +153,33 @@ To create a repetitive string, use `Buffer.alloc(count, fill).toString()` instea
153153
- Unit tests for specific features are organized by module (e.g., `/test/js/bun/`, `/test/js/node/`)
154154
- Integration tests are in `/test/integration/`
155155
156+
### Nested/complex object equality
157+
158+
Prefer usage of `.toEqual` rather than many `.toBe` assertions for nested or complex objects.
159+
160+
<example>
161+
162+
BAD (try to avoid doing this):
163+
164+
```ts
165+
expect(result).toHaveLength(3);
166+
expect(result[0].optional).toBe(null);
167+
expect(result[1].optional).toBe("middle-value"); // CRITICAL: middle item's value must be preserved
168+
expect(result[2].optional).toBe(null);
169+
```
170+
171+
**GOOD (always prefer this):**
172+
173+
```ts
174+
expect(result).toEqual([
175+
{ optional: null },
176+
{ optional: "middle-value" }, // CRITICAL: middle item's value must be preserved
177+
{ optional: null },
178+
]);
179+
```
180+
181+
</example>
182+
156183
### Common Imports from `harness`
157184
158185
```ts

0 commit comments

Comments
 (0)