fix(client): orWhere now groups, so an earlier where keeps applying (#1083)

Review threads live on the whole diff, not on one commit, so none are shown here - a thread's line means something in the branch's final form, and painting it into an intermediate step would put it on code it is not about.

packages/bun-query-builder/test/client.dynamic-where.test.tsmodified+4-2

Changes to packages/bun-query-builder/test/client.dynamic-where.test.ts

		⋯52 linesShow the 52 lines above this hunk@@ -53,8 +53,10 @@ describe('dynamic whereX/orWhereX/andWhereX methods', () => {
53	53	// created_at = ?` — and it did so through a second code path (it also
54	54	// assigned `built = sql`${ensureBuilt()} OR ...``, which was the query
55	55	// actually executed). See #1083.
56		- const s = String(q.toSQL())
57		- expect(s).toContain('WHERE email = $1 AND (name = $2 OR role = $3) AND created_at = $4')
	56	+ // Placeholders normalised: the dialect comes from process-wide config, and
	57	+ // this assertion is about grouping, not about `?` vs `$n`.
	58	+ const s = String(q.toSQL()).replace(/\$\d+/g, '?')
	59	+ expect(s).toContain('WHERE email = ? AND (name = ? OR role = ?) AND created_at = ?')
58	60	})
59	61
60	62	it('treats array values as IN and scalars as =', () => {

packages/bun-query-builder/test/client.or-where-precedence.test.tsmodified+74-32

Changes to packages/bun-query-builder/test/client.or-where-precedence.test.ts

		⋯17 linesShow the 17 lines above this hunk@@ -18,8 +18,8 @@
18	18	* before. Those are asserted with `toBe`, not `toContain`.
19	19	*/
20	20
21		-import { describe, expect, it } from 'bun:test'
22		-import { buildDatabaseSchema, createQueryBuilder } from '../src'
	21	+import { afterEach, beforeEach, describe, expect, it } from 'bun:test'
	22	+import { buildDatabaseSchema, config, createQueryBuilder } from '../src'
23	23
24	24	const schema = buildDatabaseSchema({
25	25	Migration: {
		⋯9 linesShow the 9 lines above this hunk@@ -35,7 +35,18 @@ const schema = buildDatabaseSchema({
35	35	} as any)
36	36
37	37	const q = () => createQueryBuilder<typeof schema>({ schema }).selectFrom('migrations').selectAll() as any
38		-const sqlOf = (build: (b: any) => any): string => String(build(q()).toSQL())
	38	+
	39	+/**
	40	+ * Placeholders normalised to `?`.
	41	+ *
	42	+ * The dialect comes from process-wide config, so the same assertion sees `$1`
	43	+ * under Postgres and `?` under SQLite depending on what else has run. This
	44	+ * suite is about GROUPING, which is dialect-independent — normalising keeps it
	45	+ * from failing for a reason it does not test. Placeholder NUMBERING is asserted
	46	+ * separately, where it is the actual subject.
	47	+ */
	48	+const norm = (s: string): string => s.replace(/\$\d+/g, '?')
	49	+const sqlOf = (build: (b: any) => any): string => norm(String(build(q()).toSQL()))
39	50
40	51	describe('orWhere groups with the term before it (#1083)', () => {
41	52	it('brackets a trailing OR run — the reported chain', () => {
		⋯2 linesShow the 2 lines above this hunk@@ -44,9 +55,9 @@ describe('orWhere groups with the term before it (#1083)', () => {
44	55	.where('migration', 'like', '%create%')
45	56	.orWhere('migration', 'like', '%table%'))
46	57
47		- // Was: `WHERE id <= $1 AND migration like $2 OR migration like $3`,
	58	+ // Was: `WHERE id <= ? AND migration like ? OR migration like ?` (flat),
48	59	// which meant `(id <= 10 AND create) OR table` and returned 118 of 118.
49		- expect(sql).toContain('WHERE id <= $1 AND (migration like $2 OR migration like $3)')
	60	+ expect(sql).toContain('WHERE id <= ? AND (migration like ? OR migration like ?)')
50	61	})
51	62
52	63	it('brackets a leading OR run against the AND that follows', () => {
		⋯5 linesShow the 5 lines above this hunk@@ -58,7 +69,7 @@ describe('orWhere groups with the term before it (#1083)', () => {
58	69	// The mis-grouping ran in both directions: this used to parse as
59	70	// `id <= 10 OR (table AND batch = 2)`, so the trailing filter bound only
60	71	// to the OR's right arm.
61		- expect(sql).toContain('WHERE (id <= $1 OR migration like $2) AND batch = $3')
	72	+ expect(sql).toContain('WHERE (id <= ? OR migration like ?) AND batch = ?')
62	73	})
63	74
64	75	it('leaves a two-term OR chain byte-identical', () => {
		⋯1 lineShow the 1 line above this hunk@@ -66,7 +77,7 @@ describe('orWhere groups with the term before it (#1083)', () => {
66	77	// adding them anyway would churn the emitted SQL of every existing
67	78	// two-term query for no correctness gain.
68	79	expect(sqlOf(b => b.where('id', '<=', 10).orWhere('batch', '=', 2)))
69		- .toBe('SELECT * FROM migrations WHERE id <= $1 OR batch = $2')
	80	+ .toBe('SELECT * FROM migrations WHERE id <= ? OR batch = ?')
70	81	})
71	82
72	83	it('leaves an all-OR chain unbracketed', () => {
		⋯2 linesShow the 2 lines above this hunk@@ -75,13 +86,13 @@ describe('orWhere groups with the term before it (#1083)', () => {
75	86	.orWhere('migration', 'like', '%create%')
76	87	.orWhere('migration', 'like', '%table%'))
77	88
78		- expect(sql).toBe('SELECT * FROM migrations WHERE id <= $1 OR migration like $2 OR migration like $3')
	89	+ expect(sql).toBe('SELECT * FROM migrations WHERE id <= ? OR migration like ? OR migration like ?')
79	90	expect(sql).not.toContain('(')
80	91	})
81	92
82	93	it('leaves an all-AND chain byte-identical', () => {
83	94	expect(sqlOf(b => b.where('id', '<=', 10).where('batch', '=', 1).where('migration', 'like', '%x%')))
84		- .toBe('SELECT * FROM migrations WHERE id <= $1 AND batch = $2 AND migration like $3')
	95	+ .toBe('SELECT * FROM migrations WHERE id <= ? AND batch = ? AND migration like ?')
85	96	})
86	97
87	98	it('brackets every OR run in a longer chain', () => {
		⋯5 linesShow the 5 lines above this hunk@@ -93,7 +104,7 @@ describe('orWhere groups with the term before it (#1083)', () => {
93	104	.orWhere('batch', '=', 2))
94	105
95	106	// Was `a AND b OR c AND d OR e`, which degenerates to the whole table.
96		- expect(sql).toContain('WHERE id <= $1 AND (migration like $2 OR migration like $3) AND (batch = $4 OR batch = $5)')
	107	+ expect(sql).toContain('WHERE id <= ? AND (migration like ? OR migration like ?) AND (batch = ? OR batch = ?)')
97	108	})
98	109	})
99	110
		⋯25 linesShow the 25 lines above this hunk@@ -125,9 +136,9 @@ describe('the WHERE lands in the right place (#1083)', () => {
125	136	it('a where added after an orderBy still precedes the ORDER BY', () => {
126	137	const sql = sqlOf(b => b.where('id', '<=', 10).orderBy('id').orWhere('batch', '=', 2))
127	138
128		- // Was `… WHERE id <= $1 ORDER BY id ASC OR batch = $2` — a syntax error.
	139	+ // Was `… WHERE id <= ? ORDER BY id ASC OR batch = ?` — a syntax error.
129	140	expect(sql.indexOf('WHERE')).toBeLessThan(sql.indexOf('ORDER BY'))
130		- expect(sql).toBe('SELECT * FROM migrations WHERE id <= $1 OR batch = $2 ORDER BY id ASC')
	141	+ expect(sql).toBe('SELECT * FROM migrations WHERE id <= ? OR batch = ? ORDER BY id ASC')
131	142	})
132	143
133	144	it('a where added after a limit still precedes the LIMIT', () => {
		⋯15 linesShow the 15 lines above this hunk@@ -149,19 +160,15 @@ describe('whereGroup is the escape hatch for the old reading (#1083)', () => {
149	160	.whereGroup((g: any) => g.where('id', '<=', 10).where('migration', 'like', '%create%'))
150	161	.orWhere('migration', 'like', '%table%'))
151	162
152		- expect(sql).toContain('WHERE (id <= $1 AND migration like $2) OR migration like $3')
	163	+ expect(sql).toContain('WHERE (id <= ? AND migration like ?) OR migration like ?')
153	164	})
154	165
155		- it('numbers the group placeholders after the terms before it', () => {
	166	+ it('keeps the group params in emitted order', () => {
156	167	const built = q()
157	168	.where('batch', '=', 1)
158	169	.whereGroup((g: any) => g.where('id', '<=', 10).orWhere('id', '>=', 100))
159	170
160		- const sql = String(built.toSQL())
161		- expect(sql).toContain('WHERE batch = $1 AND (id <= $2 OR id >= $3)')
162		- // The sub-builder numbers from $1; without the offset both the outer and
163		- // inner first placeholder would be $1 and the values would bind shifted.
164		- expect((sql.match(/\$1/g) ?? []).length).toBe(1)
	171	+ expect(norm(String(built.toSQL()))).toContain('WHERE batch = ? AND (id <= ? OR id >= ?)')
165	172	expect(built.__rawState().params).toEqual([1, 10, 100])
166	173	})
167	174
		⋯2 linesShow the 2 lines above this hunk@@ -170,7 +177,7 @@ describe('whereGroup is the escape hatch for the old reading (#1083)', () => {
170	177	.where('batch', '=', 1)
171	178	.orWhereGroup((g: any) => g.where('id', '<=', 10).where('migration', 'like', '%x%')))
172	179
173		- expect(sql).toContain('WHERE batch = $1 OR (id <= $2 AND migration like $3)')
	180	+ expect(sql).toContain('WHERE batch = ? OR (id <= ? AND migration like ?)')
174	181	})
175	182
176	183	it('throws rather than widening when the callback adds nothing', () => {
		⋯7 linesShow the 7 lines above this hunk@@ -184,20 +191,55 @@ describe('object and raw forms are one term each (#1083)', () => {
184	191	it('brackets a multi-key object so a later orWhere binds to all of it', () => {
185	192	const sql = sqlOf(b => b.where({ batch: 1, id: 2 }).orWhere('migration', 'like', '%x%'))
186	193
187		- // Un-bracketed this was `batch = $1 AND id = $2 OR migration like $3`,
	194	+ // Un-bracketed this was `batch = ? AND id = ? OR migration like ?`,
188	195	// which drops `batch` — the object's own keys came apart.
189		- expect(sql).toContain('WHERE (batch = $1 AND id = $2) OR migration like $3')
	196	+ expect(sql).toContain('WHERE (batch = ? AND id = ?) OR migration like ?')
190	197	})
191	198
192	199	it('leaves a single-key object unbracketed', () => {
193	200	expect(sqlOf(b => b.where({ batch: 1 })))
194		- .toBe('SELECT * FROM migrations WHERE batch = $1')
	201	+ .toBe('SELECT * FROM migrations WHERE batch = ?')
195	202	})
196	203
197	204	it('treats orWhere({a, b}) as OR (a AND b)', () => {
198	205	const sql = sqlOf(b => b.where('id', '<=', 10).orWhere({ batch: 1, migration: 'x' }))
199	206
200		- expect(sql).toContain('OR (batch = $2 AND migration = $3)')
	207	+ expect(sql).toContain('OR (batch = ? AND migration = ?)')
	208	+ })
	209	+})
	210	+
	211	+describe('whereGroup placeholder numbering, per dialect (#1083)', () => {
	212	+ // The dialect is process-wide config, so it is pinned here rather than
	213	+ // assumed — this is the one assertion in the file that is ABOUT `?` vs `$n`.
	214	+ // Same save/restore shape as client.union-params.test.ts, which pins the
	215	+ // matching invariant for set-operator renumbering.
	216	+ let dialect: string
	217	+ beforeEach(() => { dialect = config.dialect })
	218	+ afterEach(() => { config.dialect = dialect as any })
	219	+
	220	+ it('Postgres: continues the outer numbering into the group', () => {
	221	+ config.dialect = 'postgres' as any
	222	+ const built = q()
	223	+ .where('batch', '=', 1)
	224	+ .whereGroup((g: any) => g.where('id', '<=', 10).orWhere('id', '>=', 100))
	225	+ const sql = String(built.toSQL())
	226	+
	227	+ // The sub-builder numbers its own placeholders from $1. Without the offset
	228	+ // the outer `batch` and the group's first term would BOTH be $1, and every
	229	+ // value after the group would bind shifted by one.
	230	+ expect(sql).toContain('WHERE batch = $1 AND (id <= $2 OR id >= $3)')
	231	+ expect((sql.match(/\$1\b/g) ?? []).length).toBe(1)
	232	+ expect(built.__rawState().params).toEqual([1, 10, 100])
	233	+ })
	234	+
	235	+ it('SQLite: positional `?` needs no renumbering', () => {
	236	+ config.dialect = 'sqlite' as any
	237	+ const built = q()
	238	+ .where('batch', '=', 1)
	239	+ .whereGroup((g: any) => g.where('id', '<=', 10).orWhere('id', '>=', 100))
	240	+
	241	+ expect(String(built.toSQL())).toContain('WHERE batch = ? AND (id <= ? OR id >= ?)')
	242	+ expect(built.__rawState().params).toEqual([1, 10, 100])
201	243	})
202	244	})
203	245
		⋯2 linesShow the 2 lines above this hunk@@ -206,13 +248,13 @@ describe('the sub-select builder groups the same way (#1083)', () => {
206	248	.selectFromSub(createQueryBuilder<typeof schema>({ schema }).selectFrom('migrations').selectAll() as any, 'm') as any
207	249
208	250	it('brackets a trailing OR run', () => {
209		- const sql = String(sub().where('id', '<=', 10).where('batch', '=', 1).orWhere('batch', '=', 2).toSQL())
	251	+ const sql = norm(String(sub().where('id', '<=', 10).where('batch', '=', 1).orWhere('batch', '=', 2).toSQL()))
210	252
211		- expect(sql).toContain('AND (batch = $2 OR batch = $3)')
	253	+ expect(sql).toContain('AND (batch = ? OR batch = ?)')
212	254	})
213	255
214	256	it('places a where added after an orderBy before the ORDER BY', () => {
215		- const sql = String(sub().orderBy('id').where('id', '<=', 10).toSQL())
	257	+ const sql = norm(String(sub().orderBy('id').where('id', '<=', 10).toSQL()))
216	258
217	259	// The old builder appended the WHERE to the end of the text, so ordering
218	260	// first made the query unparseable.
		⋯1 lineShow the 1 line above this hunk@@ -220,18 +262,18 @@ describe('the sub-select builder groups the same way (#1083)', () => {
220	262	})
221	263
222	264	it('emits WHERE for a leading orWhere', () => {
223		- const sql = String(sub().orWhere('id', '<=', 10).toSQL())
	265	+ const sql = norm(String(sub().orWhere('id', '<=', 10).toSQL()))
224	266
225		- expect(sql).toContain('WHERE id <= $1')
	267	+ expect(sql).toContain('WHERE id <= ?')
226	268	expect(sql).not.toMatch(/AS m\s+OR\b/)
227	269	})
228	270
229	271	it('supports whereGroup for the old reading', () => {
230		- const sql = String(sub()
	272	+ const sql = norm(String(sub()
231	273	.whereGroup((g: any) => g.where('id', '<=', 10).where('batch', '=', 1))
232	274	.orWhere('batch', '=', 2)
233		- .toSQL())
	275	+ .toSQL()))
234	276
235		- expect(sql).toContain('WHERE (id <= $1 AND batch = $2) OR batch = $3')
	277	+ expect(sql).toContain('WHERE (id <= ? AND batch = ?) OR batch = ?')
236	278	})
237	279	})

packages/bun-query-builder/test/types-and-inference.test.tsmodified+2-1

Changes to packages/bun-query-builder/test/types-and-inference.test.ts

		⋯893 linesShow the 893 lines above this hunk@@ -894,7 +894,8 @@ describe('Query Builder: fluent chaining', () => {
894	894	// 'AND'. This is the test that should have caught #1083: it emitted
895	895	// `id = ? OR email = ? AND is_active = ?` — `(id) OR (email AND active)` —
896	896	// and passed, because every keyword it looked for was present.
897		- expect(s).toContain('WHERE (id = $1 OR email = $2) AND is_active = $3')
	897	+ // Placeholders normalised — `?` vs `$n` depends on process-wide config.
	898	+ expect(s.replace(/\$\d+/g, '?')).toContain('WHERE (id = ? OR email = ?) AND is_active = ?')
898	899	})
899	900	})
900	901