also looking at this
fix(sqlite): a nested query's bindings were being thrown away
#1085
2 files
+156
-6
| @@ -263,16 +263,39 @@ function createSQLiteSQL(filename: string): SQL { | ||
| 263 | 263 | for (let i = 0; i < values.length; i++) { |
| 264 | 264 | const value = values[i] |
| 265 | 265 | |
| 266 | // Handle query objects (nested queries) FIRST. | |
| 267 | // | |
| 268 | // Order matters, and getting it wrong is stacksjs/bun-query-builder#1084. | |
| 269 | // The queries this wrapper hands back (see the object returned below) | |
| 270 | // carry `raw: () => sql` so callers can read their text — which means | |
| 271 | // they satisfy the raw-marker test underneath as well. With that branch | |
| 272 | // first, interpolating one query into another spliced its SQL TEXT and | |
| 273 | // silently threw away its `values`, so the placeholders arrived with no | |
| 274 | // bindings behind them: | |
| 275 | // | |
| 276 | // db.selectFrom('t').where('id', '>', 0).paginate(5) | |
| 277 | // -> SQLite query expected 3 values, received 2 | |
| 278 | // | |
| 279 | // and the COUNT half of the same call failed the other way: bun:sqlite | |
| 280 | // only arity-checks when some values are passed, so `WHERE id > ?` bound | |
| 281 | // the placeholder to NULL, matched nothing, and reported total 0 rather | |
| 282 | // than throwing. `paginate` nests twice, which is why it was the loudest | |
| 283 | // victim, but every method that composes a query into a template — the | |
| 284 | // pagination family, exists/first/value, the soft-delete filter — was | |
| 285 | // dropping bindings the same way. | |
| 286 | // | |
| 287 | // The discriminator is `values` being an ARRAY: raw markers carry | |
| 288 | // `__raw` and no `values`, and Bun's native query objects expose `values` | |
| 289 | // as a method, so neither is captured here. | |
| 290 | if (value && typeof value === 'object' && typeof value.sql === 'string' && Array.isArray(value.values)) { | |
| 291 | sql += value.sql + (strings[i + 1] || '') | |
| 292 | params.push(...value.values) | |
| 293 | } | |
| 266 | 294 | // Handle raw SQL markers (from sql('identifier') or sql.raw()) |
| 267 | if (value && typeof value === 'object' && (value.__raw || value.raw)) { | |
| 295 | else if (value && typeof value === 'object' && (value.__raw || value.raw)) { | |
| 268 | 296 | const rawValue = value.__raw ? value.value : (typeof value.raw === 'string' ? value.raw : value.raw()) |
| 269 | 297 | sql += rawValue + (strings[i + 1] || '') |
| 270 | 298 | } |
| 271 | // Handle query objects (nested queries) | |
| 272 | else if (value && typeof value === 'object' && 'sql' in value && 'values' in value) { | |
| 273 | sql += value.sql + (strings[i + 1] || '') | |
| 274 | params.push(...value.values) | |
| 275 | } | |
| 276 | 299 | // Handle arrays - expand to placeholders |
| 277 | 300 | else if (Array.isArray(value)) { |
| 278 | 301 | const placeholders = value.map(() => '?').join(', ') |
| @@ -0,0 +1,127 @@ | ||
| 1 | import { afterEach, beforeEach, describe, expect, it } from 'bun:test' | |
| 2 | import { buildDatabaseSchema, config, createQueryBuilder, setConfig } from '../src' | |
| 3 | import { resetConnection } from '../src/db' | |
| 4 | ||
| 5 | /** | |
| 6 | * Regression coverage for stacksjs/bun-query-builder#1084. | |
| 7 | * | |
| 8 | * `paginate()` threw `SQLite query expected 3 values, received 2` on any query | |
| 9 | * whose WHERE contributed a binding, and the arithmetic named the cause: the | |
| 10 | * shortfall was always exactly the number of parameters the WHERE needed, and | |
| 11 | * `whereNull` — the one where-form that adds SQL text but no bindings — was the | |
| 12 | * one that worked. | |
| 13 | * | |
| 14 | * The bug was NOT in `paginate`. It was in the bun:sqlite wrapper's | |
| 15 | * tagged-template interpolation (`processTaggedTemplate` in db.ts). The queries | |
| 16 | * that wrapper returns carry `raw: () => sql` so callers can read their text, | |
| 17 | * which also made them satisfy the raw-marker branch that was tested FIRST. So | |
| 18 | * interpolating one query into another spliced its SQL text and discarded its | |
| 19 | * `values`, leaving placeholders with nothing behind them. | |
| 20 | * | |
| 21 | * `paginate` nests twice — once for `COUNT(*) FROM (…)` and once for | |
| 22 | * `… LIMIT ? OFFSET ?` — which made it the loudest victim, but everything that | |
| 23 | * composes a query into a template was affected. The count half failed more | |
| 24 | * quietly than the page half: bun:sqlite only arity-checks when some values are | |
| 25 | * passed, so `WHERE id > ?` bound the placeholder to NULL, matched nothing, and | |
| 26 | * reported `total: 0` instead of throwing. That is why these tests assert | |
| 27 | * `meta.total` explicitly rather than only that `paginate()` does not throw. | |
| 28 | * | |
| 29 | * These must run against the library's own SQLite connection (config-driven, | |
| 30 | * via `resetConnection()`), NOT an injected `new SQL(...)`. An injected | |
| 31 | * connection routes around the wrapper entirely, so the bug is invisible — that | |
| 32 | * is precisely why the existing pagination tests never caught this. | |
| 33 | */ | |
| 34 | ||
| 35 | const schema = buildDatabaseSchema({ | |
| 36 | Row: { | |
| 37 | name: 'Row', | |
| 38 | table: 'p_rows', | |
| 39 | primaryKey: 'id', | |
| 40 | attributes: { id: { validation: { rule: {} } }, executed_at: { validation: { rule: {} } } }, | |
| 41 | }, | |
| 42 | } as any) | |
| 43 | ||
| 44 | let saved: { dialect: any, database: any } | |
| 45 | ||
| 46 | beforeEach(async () => { | |
| 47 | saved = { dialect: config.dialect, database: { ...config.database } } | |
| 48 | setConfig({ dialect: 'sqlite', database: { database: ':memory:' } }) | |
| 49 | resetConnection() | |
| 50 | ||
| 51 | const db = createQueryBuilder<typeof schema>({ schema }) | |
| 52 | await db.unsafe('CREATE TABLE p_rows (id INTEGER PRIMARY KEY, executed_at TEXT)') | |
| 53 | for (let i = 1; i <= 20; i++) | |
| 54 | await db.unsafe(`INSERT INTO p_rows (id, executed_at) VALUES (${i}, ${i > 13 ? `'done'` : 'NULL'})`) | |
| 55 | }) | |
| 56 | ||
| 57 | afterEach(() => { | |
| 58 | setConfig({ dialect: saved.dialect, database: saved.database } as any) | |
| 59 | resetConnection() | |
| 60 | }) | |
| 61 | ||
| 62 | const qb = () => createQueryBuilder<typeof schema>({ schema }) | |
| 63 | ||
| 64 | describe('paginate carries the WHERE clause bindings (#1084)', () => { | |
| 65 | it('pages a filtered query instead of throwing', async () => { | |
| 66 | // Was: SQLite query expected 3 values, received 2 | |
| 67 | const result: any = await qb().selectFrom('p_rows').selectAll().where('id', '>', 10).paginate(5) | |
| 68 | ||
| 69 | expect(result.data.map((r: any) => r.id)).toEqual([11, 12, 13, 14, 15]) | |
| 70 | }) | |
| 71 | ||
| 72 | it('counts only the filtered rows', async () => { | |
| 73 | // A separate assertion on purpose: the count query failed SILENTLY (bound | |
| 74 | // the placeholder to NULL and matched nothing), so it would have reported | |
| 75 | // total 0 even once the page query stopped throwing. | |
| 76 | const result: any = await qb().selectFrom('p_rows').selectAll().where('id', '>', 10).paginate(5) | |
| 77 | ||
| 78 | expect(result.meta.total).toBe(10) | |
| 79 | expect(result.meta.total).not.toBe(0) | |
| 80 | }) | |
| 81 | ||
| 82 | it('binds a whereIn list', async () => { | |
| 83 | // Was: expected 5 values, received 2 — the shortfall is the list length. | |
| 84 | const result: any = await qb().selectFrom('p_rows').selectAll().whereIn('id', [1, 2, 3]).paginate(5) | |
| 85 | ||
| 86 | expect(result.data.map((r: any) => r.id)).toEqual([1, 2, 3]) | |
| 87 | expect(result.meta.total).toBe(3) | |
| 88 | }) | |
| 89 | ||
| 90 | it('still works for a where-form that contributes no bindings', async () => { | |
| 91 | // whereNull was the case that always worked; it must not regress. | |
| 92 | const result: any = await qb().selectFrom('p_rows').selectAll().whereNull('executed_at').paginate(5) | |
| 93 | ||
| 94 | expect(result.meta.total).toBe(13) | |
| 95 | }) | |
| 96 | ||
| 97 | it('still works with no filter at all', async () => { | |
| 98 | const result: any = await qb().selectFrom('p_rows').selectAll().paginate(5) | |
| 99 | ||
| 100 | expect(result.data.length).toBe(5) | |
| 101 | expect(result.meta.total).toBe(20) | |
| 102 | }) | |
| 103 | }) | |
| 104 | ||
| 105 | describe('other query-composing methods see the WHERE (#1084)', () => { | |
| 106 | // Same root cause, quieter symptoms: these returned the un-filtered answer | |
| 107 | // rather than throwing, because their nested fragment lost its bindings. | |
| 108 | it('exists() respects the filter', async () => { | |
| 109 | expect(await qb().selectFrom('p_rows').selectAll().where('id', '>', 10).exists()).toBe(true) | |
| 110 | expect(await qb().selectFrom('p_rows').selectAll().where('id', '>', 999).exists()).toBe(false) | |
| 111 | }) | |
| 112 | ||
| 113 | it('first() returns a row matching the filter', async () => { | |
| 114 | const row: any = await qb().selectFrom('p_rows').selectAll().where('id', '>', 10).first() | |
| 115 | ||
| 116 | expect(row?.id).toBe(11) | |
| 117 | }) | |
| 118 | ||
| 119 | it('count() agrees with the number of rows get() returns', async () => { | |
| 120 | // The cheapest tripwire for this whole family: if a composing path loses | |
| 121 | // its bindings again, these two stop matching. | |
| 122 | const q = () => qb().selectFrom('p_rows').selectAll().where('id', '>', 10) | |
| 123 | const rows: any = await q().get() | |
| 124 | ||
| 125 | expect(Number(await q().count())).toBe(rows.length) | |
| 126 | }) | |
| 127 | }) | |