also looking at this
fix(browser): stop serialising an exclusion as an inclusion
#1108
2 files
+115
-1
| @@ -525,11 +525,15 @@ export class BrowserQueryBuilder<T = any> { | ||
| 525 | 525 | private buildQueryParams(): URLSearchParams { |
| 526 | 526 | const params = new URLSearchParams() |
| 527 | 527 | |
| 528 | // As in BrowserModelQueryBuilder: `boolean` is not read, so an `orWhere` | |
| 529 | // serialises as an AND. Left alone deliberately — see #1096. | |
| 528 | 530 | // Add where clauses as query params |
| 529 | 531 | for (const where of this.state.wheres) { |
| 530 | 532 | if (where.operator === '=') { |
| 531 | 533 | params.append(where.column, String(where.value)) |
| 532 | 534 | } |
| 535 | // `in` only — `not in` falls through to the filter branch, so an | |
| 536 | // exclusion stays distinguishable from an inclusion (#1096). | |
| 533 | 537 | else if (where.operator === 'in' && Array.isArray(where.value)) { |
| 534 | 538 | params.append(`${where.column}[]`, where.value.join(',')) |
| 535 | 539 | } |
| @@ -970,11 +974,22 @@ else { | ||
| 970 | 974 | private buildQueryParams(): URLSearchParams { |
| 971 | 975 | const params = new URLSearchParams() |
| 972 | 976 | |
| 977 | // `where.boolean` is not read here, so an `orWhere` serialises exactly like | |
| 978 | // a `where` and the server reads it as AND. Representing OR needs a wire | |
| 979 | // format that this file has no precedent for, so it is left alone rather | |
| 980 | // than invented — see #1096. BrowserQueryBuilder.buildQueryParams has the | |
| 981 | // same gap. | |
| 973 | 982 | for (const where of this._wheres) { |
| 974 | 983 | if (where.operator === '=') { |
| 975 | 984 | params.append(where.column, String(where.value)) |
| 976 | 985 | } |
| 977 | else if ((where.operator === 'in' || where.operator === 'not in') && Array.isArray(where.value)) { | |
| 986 | // `in` only. Including `not in` here made an exclusion serialise | |
| 987 | // byte-identically to an inclusion — `whereNotIn('role', ['banned'])` | |
| 988 | // and `whereIn('role', ['banned'])` both sent `role[]=banned`, so the | |
| 989 | // server returned precisely the rows the caller meant to exclude. | |
| 990 | // Falling through to the filter branch below is what the sibling | |
| 991 | // BrowserQueryBuilder already does. See #1096. | |
| 992 | else if (where.operator === 'in' && Array.isArray(where.value)) { | |
| 978 | 993 | params.append(`${where.column}[]`, where.value.join(',')) |
| 979 | 994 | } |
| 980 | 995 | else if (where.operator === 'is' && where.value === null) { |
| @@ -0,0 +1,99 @@ | ||
| 1 | /** | |
| 2 | * `whereNotIn` must not serialise identically to `whereIn`. | |
| 3 | * stacksjs/bun-query-builder#1096. | |
| 4 | * | |
| 5 | * `BrowserModelQueryBuilder.buildQueryParams` matched `in` and `not in` in the | |
| 6 | * same branch, so both emitted `column[]=a,b`. An exclusion went out on the | |
| 7 | * wire byte-identical to an inclusion, and the server answered with exactly the | |
| 8 | * rows the caller meant to remove. | |
| 9 | * | |
| 10 | * The idiom is overwhelmingly "exclude banned / deleted / private", so the | |
| 11 | * failure mode is showing the records you meant to hide — with no error, and | |
| 12 | * with a request that looks correct in a network panel. | |
| 13 | * | |
| 14 | * The sibling `BrowserQueryBuilder` already matched `in` alone and let `not in` | |
| 15 | * fall through to the `filter[column][operator]` branch, so this asserts the | |
| 16 | * two builders agree as well as that the exclusion survives. | |
| 17 | * | |
| 18 | * These read the outgoing URL rather than a mock server's interpretation of it: | |
| 19 | * the defect was entirely in what went on the wire. | |
| 20 | */ | |
| 21 | ||
| 22 | import { afterEach, beforeEach, describe, expect, it } from 'bun:test' | |
| 23 | import { BrowserQueryBuilder, configureBrowser, createBrowserModel } from '../src/browser' | |
| 24 | ||
| 25 | const realFetch = globalThis.fetch | |
| 26 | let captured: string[] = [] | |
| 27 | ||
| 28 | beforeEach(() => { | |
| 29 | captured = [] | |
| 30 | globalThis.fetch = (async (input: any) => { | |
| 31 | captured.push(typeof input === 'string' ? input : String(input?.url ?? input)) | |
| 32 | return new Response(JSON.stringify({ data: [] }), { | |
| 33 | status: 200, | |
| 34 | headers: { 'Content-Type': 'application/json' }, | |
| 35 | }) | |
| 36 | }) as any | |
| 37 | configureBrowser({ baseUrl: 'http://x.test/api' } as any) | |
| 38 | }) | |
| 39 | ||
| 40 | afterEach(() => { | |
| 41 | globalThis.fetch = realFetch | |
| 42 | }) | |
| 43 | ||
| 44 | /** The query string of the single request the call produced. */ | |
| 45 | function sentQuery(): URLSearchParams { | |
| 46 | expect(captured).toHaveLength(1) | |
| 47 | return new URL(captured[0]).searchParams | |
| 48 | } | |
| 49 | ||
| 50 | const Widget = createBrowserModel({ | |
| 51 | name: 'Nwidget', | |
| 52 | table: 'nwidgets', | |
| 53 | primaryKey: 'id', | |
| 54 | attributes: { | |
| 55 | role: { type: 'string', fillable: true }, | |
| 56 | name: { type: 'string', fillable: true }, | |
| 57 | }, | |
| 58 | } as any) | |
| 59 | ||
| 60 | describe('BrowserModelQueryBuilder not-in serialisation (#1096)', () => { | |
| 61 | it('sends an exclusion differently from an inclusion', async () => { | |
| 62 | await (Widget as any).query().whereIn('role', ['banned']).get() | |
| 63 | const inclusion = sentQuery() | |
| 64 | ||
| 65 | captured = [] | |
| 66 | await (Widget as any).query().whereNotIn('role', ['banned']).get() | |
| 67 | const exclusion = sentQuery() | |
| 68 | ||
| 69 | // The whole bug: these two were identical. | |
| 70 | expect(exclusion.toString()).not.toBe(inclusion.toString()) | |
| 71 | }) | |
| 72 | ||
| 73 | it('keeps the inclusion form unchanged', async () => { | |
| 74 | await (Widget as any).query().whereIn('role', ['a', 'b']).get() | |
| 75 | expect(sentQuery().get('role[]')).toBe('a,b') | |
| 76 | }) | |
| 77 | ||
| 78 | it('sends an exclusion under a filter key naming the operator', async () => { | |
| 79 | await (Widget as any).query().whereNotIn('role', ['banned']).get() | |
| 80 | const q = sentQuery() | |
| 81 | ||
| 82 | // Must not go out as the inclusion form. | |
| 83 | expect(q.get('role[]')).toBeNull() | |
| 84 | expect(q.get('filter[role][not in]')).toBe('banned') | |
| 85 | }) | |
| 86 | ||
| 87 | it('agrees with the sibling BrowserQueryBuilder', async () => { | |
| 88 | await (Widget as any).query().whereNotIn('role', ['banned']).get() | |
| 89 | const model = sentQuery() | |
| 90 | ||
| 91 | captured = [] | |
| 92 | await new BrowserQueryBuilder('nwidgets').whereNotIn('role', ['banned']).get() | |
| 93 | const plain = sentQuery() | |
| 94 | ||
| 95 | // The two builders serialise the same query the same way. They did not. | |
| 96 | expect(model.get('filter[role][not in]')).toBe(plain.get('filter[role][not in]')) | |
| 97 | expect(model.get('role[]')).toBe(plain.get('role[]')) | |
| 98 | }) | |
| 99 | }) | |