ReviewOS
Reload

also looking at this

stacks/bun-query-builder

fix(browser)!: refuse orWhere rather than sending it as an AND

#1131
Merged glennmichael123 wants to merge fix/1096-browser-orwhere into main
2 files +136 -34

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/src/browser.tsmodified+24-34
@@ -401,23 +401,19 @@ export class BrowserQueryBuilder<T = any> {
401401 */
402402 orWhere(column: string, value: unknown): this
403403 orWhere(column: string, operator: WhereOperator, value: unknown): this
404 orWhere(column: string, operatorOrValue: unknown, value?: unknown): this {
405 if (value === undefined) {
406 this.state.wheres.push({
407 column,
408 operator: '=',
409 value: operatorOrValue,
410 boolean: 'or',
411 })
412 }
413 else {
414 this.state.wheres.push({
415 column,
416 operator: operatorOrValue as WhereOperator,
417 value,
418 boolean: 'or',
419 })
420 }
404 orWhere(_column: string, _operatorOrValue: unknown, _value?: unknown): this {
405 // Refused rather than recorded. `boolean: 'or'` was pushed onto the where
406 // list and buildQueryParams never read it, so the term went out as a plain
407 // filter and any receiving API read the whole set as a conjunction:
408 //
409 // .where('role', 'admin').orWhere('role', 'moderator')
410 // -> role=admin&role=moderator (AND, not OR)
411 //
412 // That is a superset of the intended rows, returned without complaint. A
413 // wire spelling for OR would have to be agreed with whatever serves these
414 // requests, and inventing one here would ship a format nothing reads —
415 // the same silent failure, one layer further away. See #1096.
416 throw new TypeError('[browser] orWhere() is not supported by this client. The query-param format it builds has no spelling for a disjunction, so the term would be sent as a plain filter and the server would read it as AND \u2014 silently returning a superset of the rows you asked for. Express the alternatives as a single filter (whereIn), or issue one request per branch and merge the results.')
421417 return this
422418 }
423419
@@ -536,8 +532,8 @@ export class BrowserQueryBuilder<T = any> {
536532 private buildQueryParams(): URLSearchParams {
537533 const params = new URLSearchParams()
538534
539 // As in BrowserModelQueryBuilder: `boolean` is not read, so an `orWhere`
540 // serialises as an AND. Left alone deliberately — see #1096.
535 // Every recorded where is a conjunct: orWhere() now throws rather than
536 // recording a disjunction this format cannot express. See #1096.
541537 // Add where clauses as query params
542538 for (const where of this.state.wheres) {
543539 if (where.operator === '=') {
@@ -887,17 +883,13 @@ else {
887883 }
888884
889885 orWhere<K extends BrowserColumnName<TDef>>(
890 column: K,
891 operatorOrValue: WhereOperator | (K extends keyof BrowserModelAttributes<TDef> ? BrowserModelAttributes<TDef>[K] : unknown),
892 value?: K extends keyof BrowserModelAttributes<TDef> ? BrowserModelAttributes<TDef>[K] : unknown
886 _column: K,
887 _operatorOrValue: WhereOperator | (K extends keyof BrowserModelAttributes<TDef> ? BrowserModelAttributes<TDef>[K] : unknown),
888 _value?: K extends keyof BrowserModelAttributes<TDef> ? BrowserModelAttributes<TDef>[K] : unknown
893889 ): BrowserModelQueryBuilder<TDef, TSelected> {
894 if (value === undefined) {
895 this._wheres.push({ column: column as string, operator: '=', value: operatorOrValue, boolean: 'or' })
896 }
897else {
898 this._wheres.push({ column: column as string, operator: operatorOrValue as WhereOperator, value, boolean: 'or' })
899 }
900 return this
890 // See BrowserQueryBuilder.orWhere: the recorded `boolean` was never read,
891 // so a disjunction was transmitted as a conjunction. #1096.
892 throw new TypeError('[browser] orWhere() is not supported by this client. The query-param format it builds has no spelling for a disjunction, so the term would be sent as a plain filter and the server would read it as AND \u2014 silently returning a superset of the rows you asked for. Express the alternatives as a single filter (whereIn), or issue one request per branch and merge the results.')
901893 }
902894
903895 whereIn<K extends BrowserColumnName<TDef>>(
@@ -985,11 +977,9 @@ else {
985977 private buildQueryParams(): URLSearchParams {
986978 const params = new URLSearchParams()
987979
988 // `where.boolean` is not read here, so an `orWhere` serialises exactly like
989 // a `where` and the server reads it as AND. Representing OR needs a wire
990 // format that this file has no precedent for, so it is left alone rather
991 // than invented — see #1096. BrowserQueryBuilder.buildQueryParams has the
992 // same gap.
980 // Every recorded where is a conjunct, which is what this format can say.
981 // orWhere() throws rather than recording a disjunction that would be
982 // transmitted — and read by the server — as one more AND term. See #1096.
993983 for (const where of this._wheres) {
994984 if (where.operator === '=') {
995985 params.append(where.column, String(where.value))
packages/bun-query-builder/test/browser.or-where-refused.test.tsadded+112-0
@@ -0,0 +1,112 @@
1/**
2 * `orWhere` refuses rather than transmitting a disjunction as a conjunction.
3 * stacksjs/bun-query-builder#1096 — the half left open by #1108.
4 *
5 * Both browser builders recorded `boolean: 'or'` on the where list, and
6 * `buildQueryParams` never read it. Every recorded where was appended as one
7 * more parameter, which any receiving API reads as AND:
8 *
9 * .where('role', 'admin').orWhere('role', 'moderator')
10 * -> role=admin&role=moderator
11 *
12 * So the query returned a superset of the intended rows, with no error and a
13 * request that looks correct in a network panel.
14 *
15 * Throwing is the honest option here. The alternative — inventing a wire
16 * spelling like `filter[col][or][]` — would have to be agreed with whatever
17 * serves these requests; shipping one unilaterally produces the same silent
18 * failure, one layer further away, when the server ignores what it does not
19 * recognise.
20 *
21 * This IS a behaviour break for anyone calling orWhere today. What they have
22 * today is already wrong, just quietly, so the break replaces bad rows with a
23 * message naming the constraint and the two ways around it.
24 */
25
26import { afterEach, beforeEach, describe, expect, it } from 'bun:test'
27import { BrowserQueryBuilder, configureBrowser, createBrowserModel } from '../src/browser'
28
29const realFetch = globalThis.fetch
30let captured: string[] = []
31
32beforeEach(() => {
33 captured = []
34 globalThis.fetch = (async (input: any) => {
35 captured.push(typeof input === 'string' ? input : String(input?.url ?? input))
36 return new Response(JSON.stringify({ data: [] }), {
37 status: 200,
38 headers: { 'Content-Type': 'application/json' },
39 })
40 }) as any
41 configureBrowser({ baseUrl: 'http://x.test/api' } as any)
42})
43
44afterEach(() => {
45 globalThis.fetch = realFetch
46})
47
48const User = createBrowserModel({
49 name: 'User',
50 table: 'users',
51 traits: { useApi: { uri: 'users' } },
52 attributes: { role: { fillable: true }, status: { fillable: true } },
53} as any)
54
55describe('BrowserModelQueryBuilder.orWhere is refused (#1096)', () => {
56 it('throws instead of recording a term that would be sent as AND', () => {
57 expect(() => (User as any).query().where('role', 'admin').orWhere('role', 'moderator'))
58 .toThrow(/orWhere\(\) is not supported by this client/)
59 })
60
61 it('the message says why, not just no', () => {
62 let message = ''
63 try {
64 (User as any).query().orWhere('role', 'moderator')
65 }
66 catch (error: any) {
67 message = error.message
68 }
69
70 // Naming the mechanism is the point: the caller has to know the term would
71 // have been read as AND, or the refusal looks arbitrary.
72 expect(message).toContain('no spelling for a disjunction')
73 expect(message).toContain('AND')
74 // And it has to leave them somewhere to go.
75 expect(message).toContain('whereIn')
76 })
77
78 it('it is a TypeError, matching the other refusals in this codebase', () => {
79 expect(() => (User as any).query().orWhere('role', 'moderator')).toThrow(TypeError)
80 })
81
82 it('nothing is sent — the throw happens at the call, not at the request', async () => {
83 expect(() => (User as any).query().where('role', 'admin').orWhere('role', 'x')).toThrow()
84 expect(captured).toEqual([])
85 })
86})
87
88describe('BrowserQueryBuilder.orWhere is refused too (#1096)', () => {
89 it('both builders agree', () => {
90 const qb = new BrowserQueryBuilder('users' as any)
91 expect(() => (qb as any).where('role', 'admin').orWhere('role', 'moderator'))
92 .toThrow(/orWhere\(\) is not supported by this client/)
93 })
94})
95
96describe('the conjunctive paths are untouched (#1096)', () => {
97 it('chained where() still serialises as before', async () => {
98 await (User as any).query().where('role', 'admin').where('status', 'active').get()
99
100 expect(captured).toHaveLength(1)
101 expect(captured[0]).toContain('role=admin')
102 expect(captured[0]).toContain('status=active')
103 })
104
105 it('whereIn — the suggested replacement — still works', async () => {
106 await (User as any).query().whereIn('role', ['admin', 'moderator']).get()
107
108 expect(captured).toHaveLength(1)
109 // The alternatives-as-one-filter form the error message points at.
110 expect(decodeURIComponent(captured[0])).toContain('role[]=admin,moderator')
111 })
112})