diff --git a/.changeset/limit-zero-presence-sql-doors.md b/.changeset/limit-zero-presence-sql-doors.md new file mode 100644 index 0000000000..ce1b687f01 --- /dev/null +++ b/.changeset/limit-zero-presence-sql-doors.md @@ -0,0 +1,62 @@ +--- +'@objectstack/driver-sql': patch +'@objectstack/driver-memory': patch +'@objectstack/driver-mongodb': patch +'@objectstack/driver-turso': patch +'@objectstack/spec': minor +--- + +drivers: `limit: 0` returns no records, on every driver and every read door + +`limit: 0` was ruled in #6485 to mean **return no records**. Three of the five shipped +drivers did not honour it, in three different ways — and the ones that disagreed +returned **more** data than was requested, which on an ADR-0021 RLS read scope is +over-reach rather than a loose filter. Reachable since #6578: the client now puts +`top=0` on the wire, so the answer depended on which driver a deployment configured. + +**`driver-memory` — the slice was dropped.** `find()` sliced with `if (query.limit)`, +truthiness, and `0` is falsy. Measured before the fix, three rows seeded: +`{ limit: 0 }` returned **3 of 3**, and `{ limit: 0, offset: 1 }` returned 2 — the +OFFSET applied and the LIMIT silently did not, which is why every paging suite stayed +green over it. Two more sites of the same shape in `memory-analytics.ts` (the `$limit` +pipeline stage and the SQL string builder) moved with it. Mingo honours `{ $limit: 0 }` +as zero records (measured), so presence is sufficient there. + +**`driver-mongodb` — the value was forwarded faithfully, to a client that means +something else by it.** `buildFindOptions` already tested presence, so `0` arrived +exactly as written — but the MongoDB Node driver DEFINES `limit: 0` as *no limit*, so +the answer was still the whole collection. Fixed with an explicit short-circuit that +returns the empty result **before the client is consulted** (`[]` from `find`, `null` +from `findOne`, which had the same hole). No round trip is made for a query whose +answer is already known, and no future change in the upstream driver's reading of `0` +can move this behaviour. Deliberately `=== 0`, not `<= 0`. + +**`driver-sql` — two doors disagreed with a third.** `findRows()`, the door `find()` +goes through, has always compiled `limit` on presence. Two others compiled it on +truthiness: + +- `findWithWindowFunctions()` — the live window-function read door (#4286). Returns + rows, so this was user-visible wrong data: `{ limit: 0 }` returned the whole table. +- `analyzeQuery()` / `explain()` — returns a plan. It compiled `select * from "orders"` + where `find()` sent `... order by "id" asc limit ?`, so it explained a statement + other than the one that would run. + +`offset` moved with `limit` at both doors for internal consistency only. That half is +**measured to change nothing**: knex elides a zero offset on better-sqlite3, Postgres +and MySQL alike. It is pinned as the no-op it is rather than reported as a fix. + +**`driver-turso` remote transport — an `OFFSET` with no `LIMIT` was a syntax error.** +Surfaced by the new conformance control that reads with a bare offset. SQLite's grammar +is `LIMIT expr [OFFSET expr]`, and this compiler emitted the two clauses independently, +so `find(obj, { offset: N })` with no `limit` produced `near "OFFSET": syntax error` — +for **every** `N`, and only on the remote transport (the local half goes through knex, +which synthesises the `LIMIT -1` no-limit sentinel). Remote now builds the same +statement knex does. + +Result sets only ever get **narrower**. A caller who wants every row should omit +`limit` rather than pass `0`. + +`@objectstack/spec` gains `PAGINATION_ZERO_LIMIT_CASES`, the shared conformance +case-set pinning this — with controls, so "return nothing, always" cannot pass it. All +**five** drivers answer it, with **no DEBT rows**: future drift goes red at +`check:driver-conformance` rather than being discovered in production. diff --git a/packages/drivers/driver-memory/src/memory-analytics.ts b/packages/drivers/driver-memory/src/memory-analytics.ts index 2281d2cdaa..de9e8b5840 100644 --- a/packages/drivers/driver-memory/src/memory-analytics.ts +++ b/packages/drivers/driver-memory/src/memory-analytics.ts @@ -428,10 +428,18 @@ export class MemoryAnalyticsService implements IAnalyticsService { } // Stage 6: $limit and $skip + // + // PRESENCE on the limit, not truthiness (#6577) — the same defect and the + // same reason as `memory-driver.ts`'s slice: `limit: 0` means "return no + // records" (#6485), `0` is falsy, so the stage was omitted entirely and an + // analytics read that asked for none came back with every row. Mingo + // honours `{ $limit: 0 }` as zero records (measured: 3 in, 0 out), so + // pushing the stage is sufficient here — no short-circuit needed, unlike + // the MongoDB driver, whose upstream client defines `0` as "no limit". if (query.offset) { pipeline.push({ $skip: query.offset }); } - if (query.limit) { + if (query.limit !== undefined) { pipeline.push({ $limit: query.limit }); } @@ -595,7 +603,10 @@ export class MemoryAnalyticsService implements IAnalyticsService { ); sql += ` ORDER BY ${orderClauses.join(', ')}`; } - if (query.limit) { + // PRESENCE, not truthiness (#6577) — the third site of the same shape in + // this package. `limit: 0` means "return no records" (#6485), and a dropped + // `LIMIT 0` widens the statement to the whole table. + if (query.limit !== undefined) { sql += ` LIMIT ${query.limit}`; } if (query.offset) { diff --git a/packages/drivers/driver-memory/src/memory-driver.ts b/packages/drivers/driver-memory/src/memory-driver.ts index 8f8419b79b..c175776ec7 100644 --- a/packages/drivers/driver-memory/src/memory-driver.ts +++ b/packages/drivers/driver-memory/src/memory-driver.ts @@ -312,7 +312,19 @@ export class InMemoryDriver implements IDataDriver { } // 4. Pagination (Limit) - if (query.limit) { + // + // PRESENCE, not truthiness (#6577). `limit: 0` means "return no records" + // (#6485), and `0` is falsy — so `if (query.limit)` dropped the slice and + // answered a request for NOTHING with the WHOLE table. Measured before this + // line changed, three rows seeded: `{ limit: 0 }` returned 3, and + // `{ limit: 0, offset: 1 }` returned 2 — the OFFSET applied and the LIMIT + // silently did not, which is why the shape survived every paging suite. + // + // `offset` above is deliberately left on truthiness: `slice(0)` IS the + // identity slice, so presence and truthiness cannot be told apart there — + // no behaviour to fix. The #5499 freeze exception granted here is the limit + // door only. + if (query.limit !== undefined) { results = results.slice(0, query.limit); } diff --git a/packages/drivers/driver-memory/src/memory-pagination-conformance.test.ts b/packages/drivers/driver-memory/src/memory-pagination-conformance.test.ts index a25b848d86..7db8c2f3da 100644 --- a/packages/drivers/driver-memory/src/memory-pagination-conformance.test.ts +++ b/packages/drivers/driver-memory/src/memory-pagination-conformance.test.ts @@ -35,6 +35,7 @@ import { PAGINATION_CASES, PAGINATION_ROWS, PAGINATION_UNORDERED_CASES, + PAGINATION_ZERO_LIMIT_CASES, } from '@objectstack/spec/data'; import { InMemoryDriver } from './memory-driver.js'; @@ -110,4 +111,26 @@ describe('InMemoryDriver — paged reads are a partition of the result set (obje expect(paged.map((r) => r.id)).toEqual(PAGINATION_ROWS.map((r) => r.id)); }); } + + /** + * `limit: 0` returns no records (#6485/#6577). + * + * This is the driver the card was filed about. `find()` sliced with + * `if (query.limit)` — truthiness — so `limit: 0` dropped the slice and the + * read that asked for nothing was answered with all twelve rows. Measured + * before the fix on a three-row table: `{ limit: 0 }` -> 3, and + * `{ limit: 0, offset: 1 }` -> 2, i.e. the OFFSET applied and the LIMIT did + * not — which is why every paging suite here stayed green over it. + * + * The #5499 investment freeze was lifted for this door specifically + * (maintainer ruling on #6577), and for nothing else in this package. + */ + describe('`limit: 0` returns no records', () => { + for (const testCase of PAGINATION_ZERO_LIMIT_CASES) { + it(testCase.name, async () => { + const rows = await driver.find('ticket', { ...testCase.query }); + expect(rows).toHaveLength(testCase.expectedRowCount); + }); + } + }); }); diff --git a/packages/drivers/driver-mongodb/src/mongodb-driver.ts b/packages/drivers/driver-mongodb/src/mongodb-driver.ts index 387b4bbac4..454891bcb7 100644 --- a/packages/drivers/driver-mongodb/src/mongodb-driver.ts +++ b/packages/drivers/driver-mongodb/src/mongodb-driver.ts @@ -233,7 +233,34 @@ export class MongoDBDriver implements IDataDriver { return findOptions; } + /** + * `limit: 0` means **return no records** (#6485) — and this is the one driver + * where forwarding the value faithfully is not enough to say so. + * + * {@link buildFindOptions} already tests PRESENCE (`!== undefined`), so `0` + * reaches the client exactly as written. The divergence is one layer lower: + * the MongoDB Node driver DEFINES `limit: 0` as *no limit*, so a correctly + * forwarded `0` came back as the entire collection — the same wrong answer + * `driver-memory` gave for the opposite reason (it dropped the value; this + * one delivers it to a reader that means something else by it). + * + * So the contract is answered HERE rather than delegated: the empty result is + * returned without consulting the client at all. Two consequences worth being + * explicit about, because both are the point rather than side effects — no + * round trip is made for a query whose answer is already known, and no future + * change in the upstream driver's reading of `0` can move this behaviour. + * + * Deliberately `=== 0` and not `<= 0`: a negative limit is not a shape the + * contract defines, and quietly folding it into "no records" would invent an + * answer here instead of letting the layer that owns validation give one. + */ + private returnsNoRecords(query: DriverQuery): boolean { + return query.limit === 0; + } + async find(object: string, query: DriverQuery, options?: DriverOptions): Promise[]> { + if (this.returnsNoRecords(query)) return []; + const collection = this.getCollection(object); const session = this.getSession(options); @@ -246,6 +273,14 @@ export class MongoDBDriver implements IDataDriver { } async findOne(object: string, query: DriverQuery, options?: DriverOptions): Promise | null> { + // Same guard as `find()`, because this door has the same hole: `findOne` + // hands `query.limit` to the client through the SAME `buildFindOptions`, so + // `limit: 0` was read as "no limit" and answered with the first document — + // a record, where the contract says none. `null` is this signature's empty + // result. Leaving it out would have recreated, inside one driver, exactly + // the "one query, two answers" divergence this issue is about. + if (this.returnsNoRecords(query)) return null; + const collection = this.getCollection(object); const session = this.getSession(options); diff --git a/packages/drivers/driver-mongodb/src/mongodb-pagination-conformance.test.ts b/packages/drivers/driver-mongodb/src/mongodb-pagination-conformance.test.ts index c1fd5555a7..8279828abc 100644 --- a/packages/drivers/driver-mongodb/src/mongodb-pagination-conformance.test.ts +++ b/packages/drivers/driver-mongodb/src/mongodb-pagination-conformance.test.ts @@ -31,6 +31,7 @@ import { PAGINATION_CASES, PAGINATION_ROWS, PAGINATION_UNORDERED_CASES, + PAGINATION_ZERO_LIMIT_CASES, } from '@objectstack/spec/data'; import { MongoDBDriver } from './mongodb-driver.js'; import { createTestMongod } from './test-mongod.js'; @@ -60,6 +61,21 @@ describe.skipIf(!sharedMongod)('driver-mongodb — paged reads are a partition o if (sharedMongod) await sharedMongod.stop(); }); + /** + * `limit: 0` returns no records (#6485/#6577) — the whole case-set, controls + * included, against a real mongod. The controls are what stop the + * short-circuit added for this contract from being an unconditional "return + * nothing": they read 2 and 12 rows back through the same door. + */ + describe('`limit: 0` returns no records', () => { + for (const testCase of PAGINATION_ZERO_LIMIT_CASES) { + it(testCase.name, async () => { + const rows = await driver.find('ticket', { ...testCase.query }); + expect(rows).toHaveLength(testCase.expectedRowCount); + }); + } + }); + for (const testCase of PAGINATION_CASES) { it(`visits every row exactly once — ${testCase.name}`, async () => { const seen: string[] = []; @@ -166,3 +182,46 @@ describe('MongoDBDriver — the sort spec sent to the server', () => { ).toEqual({ id: -1 }); }); }); + +/** + * The `limit: 0` guard, on a driver that is NEVER CONNECTED — #6485/#6577. + * + * Outside the `skipIf` for the same reason the sort-spec block above is, and + * for one more that is specific to this contract: the guard's whole claim is + * that the answer is produced WITHOUT consulting the MongoDB client. A test + * that needs a running mongod to prove that would be asserting the opposite of + * what it is about. Here the URI is unreachable by construction (port 1), so if + * the short-circuit were removed these cases would not fail on a row count — + * they would fail on a connection error, which is exactly the right alarm. + * + * Why this driver needed a guard at all when its `buildFindOptions` was already + * correct: it tests presence (`!== undefined`), so `0` was forwarded faithfully + * — into a client that DEFINES `limit: 0` as *no limit*. Forwarding was the + * problem, not the fix. + */ +describe('MongoDBDriver — `limit: 0` is answered before the client is consulted', () => { + const driver = new MongoDBDriver({ url: 'mongodb://127.0.0.1:1/unused', database: 'unused' }); + + it('find() returns zero records without dialing the server', async () => { + await expect(driver.find('ticket', { limit: 0 })).resolves.toHaveLength(0); + }); + + it('find() returns zero records with an offset too', async () => { + await expect(driver.find('ticket', { limit: 0, offset: 5 })).resolves.toHaveLength(0); + }); + + it('findOne() returns null — the empty result for its signature', async () => { + await expect(driver.findOne('ticket', { limit: 0 })).resolves.toBeNull(); + }); + + it('does NOT short-circuit a non-zero limit — that read still needs the server', async () => { + // The control that keeps the guard from becoming "return nothing, always". + // With no short-circuit the unreachable URI is what answers, so a rejection + // here IS the assertion: the call went to the client, as it must. + await expect(driver.find('ticket', { limit: 2 })).rejects.toThrow(); + }); + + it('does NOT short-circuit a read with no limit at all', async () => { + await expect(driver.find('ticket', {})).rejects.toThrow(); + }); +}); diff --git a/packages/drivers/driver-sql/src/sql-driver-limit-zero-presence.test.ts b/packages/drivers/driver-sql/src/sql-driver-limit-zero-presence.test.ts new file mode 100644 index 0000000000..8021c4c833 --- /dev/null +++ b/packages/drivers/driver-sql/src/sql-driver-limit-zero-presence.test.ts @@ -0,0 +1,221 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * `limit: 0` is a request for no records, on **every** door this driver opens — + * objectstack#6577. + * + * # The asymmetry this file closes + * + * `findRows()`, the door `find()` goes through, compiles `limit` on PRESENCE + * (`query.limit !== undefined`), which is what the contract asks for and what + * `limit: 0` — ruled in #6485 to mean "return no records" — depends on. Two + * other doors in the same file compiled it on TRUTHINESS (`if (query.limit)`), + * and `0` is falsy, so the clause was dropped and the read that asked for + * nothing was answered with everything: + * + * - `findWithWindowFunctions()` — the live window-function read door (#4286). + * It returns ROWS, so the divergence was user-visible data. + * - `analyzeQuery()` / `explain()` — returns a PLAN. Its statement is built + * from the same `DriverQuery` and is only worth reading if it is the + * statement `find()` would run; with the LIMIT silently dropped it + * explained a different one. + * + * Measured on `main` at `3172831` (post-#6706), three rows seeded, before any + * line moved: + * + * ``` + * find { limit: 0 } -> 0 rows select * from `orders` order by `id` asc limit ? + * window { limit: 0 } -> 3 rows (the whole table) + * analyze { limit: 0 } -> select * from `orders` <- no LIMIT at all + * ``` + * + * # Why each half asserts what it does + * + * The window-function half asserts ROWS: it is the half a user feels. + * + * The `analyzeQuery` half cannot assert rows — it returns a plan. So it asserts + * its statement against the statement `find()` **actually sent**, read off + * knex's `query` event during a real `find()` rather than recompiled here. That + * distinction is load-bearing: a test that rebuilds the expected LIMIT itself + * asserts only that knex works, and stays green on the day the two doors + * diverge again — which is precisely the day this file exists for. + * + * # The controls are not padding + * + * Every zero case is stated beside the non-zero read it must NOT become. A + * driver that answered `[]` to everything would satisfy the `limit: 0` rows and + * fail the user completely; the property is "`limit` is honoured as written", + * not "`limit: 0` is special-cased". + * + * # `offset` moved with `limit`, and the honest reason + * + * Both doors also spelled `if (query.offset)`, against `findRows`'s + * `query.offset !== undefined`. That flip is **measured to change nothing**: + * knex elides a zero offset on every dialect this driver speaks — compiled + * `.offset(0)` is `select * from "orders"` on better-sqlite3, pg AND mysql2 + * alike — so no statement and no row set moves. It is made for internal + * consistency, so the three doors read identically, and it is pinned below as + * the no-op it is rather than sold as a fix. If knex ever stops eliding it, the + * pin is what notices. + */ + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import knexLib from 'knex'; +import { SqlDriver } from './index.js'; + +const TABLE = 'os6577_zero_page'; + +/** + * Records the SQL of every statement this driver sends, so a test can assert + * what `find()` really emitted rather than what it ought to have. + */ +class InspectableSqlDriver extends SqlDriver { + readonly statements: Array<{ sql: string; bindings: readonly unknown[] }> = []; + + captureStatements(): void { + this.knex.on('query', (data: { sql?: string; bindings?: readonly unknown[] }) => { + if (typeof data?.sql === 'string') { + this.statements.push({ sql: data.sql, bindings: data.bindings ?? [] }); + } + }); + } + + /** The one statement issued against {@link TABLE} while `run` executed. */ + async statementFor(run: () => Promise): Promise<{ sql: string; bindings: readonly unknown[] }> { + const before = this.statements.length; + await run(); + const issued = this.statements.slice(before).filter((s) => s.sql.includes(TABLE)); + expect(issued).toHaveLength(1); + return issued[0]!; + } +} + +/** `limit ?` / `limit 5` / `offset ?` — the pagination tail, whatever knex spelled. */ +function paginationTail(sql: string): string { + const m = sql.match(/\blimit\b.*$|\boffset\b.*$/i); + return m ? m[0].toLowerCase() : ''; +} + +const READ = { bypassTenantAudit: true } as any; + +describe('driver-sql — `limit: 0` returns no records on every door (#6577)', () => { + let driver: InspectableSqlDriver; + + beforeAll(async () => { + driver = new InspectableSqlDriver({ + client: 'better-sqlite3', + connection: { filename: ':memory:' }, + useNullAsDefault: true, + } as any); + await driver.initObjects([ + { + name: TABLE, + fields: { customer: { type: 'string' }, amount: { type: 'number' }, status: { type: 'string' } }, + }, + ] as any); + for (const row of [ + { customer: 'Alice', amount: 1200, status: 'completed' }, + { customer: 'Bob', amount: 300, status: 'pending' }, + { customer: 'Alice', amount: 50, status: 'completed' }, + ]) { + await driver.create(TABLE, row as any, READ); + } + driver.captureStatements(); + }); + + afterAll(async () => { + await driver.disconnect(); + }); + + const WINDOW = { + windowFunctions: [ + { function: 'ROW_NUMBER', alias: 'rn', orderBy: [{ field: 'amount', order: 'desc' }] }, + ], + }; + + describe('findWithWindowFunctions — the row-returning door', () => { + it('returns NO rows for `limit: 0`, where it used to return the whole table', async () => { + const rows = await driver.findWithWindowFunctions(TABLE, { ...WINDOW, limit: 0 } as any, READ); + expect(rows).toHaveLength(0); + }); + + it('still returns two rows for `limit: 2` — the clause is honoured, not special-cased', async () => { + const rows = await driver.findWithWindowFunctions(TABLE, { ...WINDOW, limit: 2 } as any, READ); + expect(rows).toHaveLength(2); + }); + + it('still returns every row when no `limit` is given at all', async () => { + const rows = await driver.findWithWindowFunctions(TABLE, { ...WINDOW } as any, READ); + expect(rows).toHaveLength(3); + }); + + it('returns no rows for `limit: 0` even with an offset past the first page', async () => { + const rows = await driver.findWithWindowFunctions(TABLE, { ...WINDOW, limit: 0, offset: 2 } as any, READ); + expect(rows).toHaveLength(0); + }); + + it('agrees with `find()` on the same query — one driver, one answer', async () => { + const viaWindow = await driver.findWithWindowFunctions(TABLE, { ...WINDOW, limit: 0 } as any, READ); + const viaFind = await driver.find(TABLE, { limit: 0 }, READ); + expect(viaWindow).toHaveLength(viaFind.length); + }); + }); + + describe('analyzeQuery — the plan door explains the statement `find()` runs', () => { + it('carries the same LIMIT `find()` emitted for `limit: 0`', async () => { + const emitted = await driver.statementFor(() => driver.find(TABLE, { limit: 0 }, READ)); + const analyzed = await driver.analyzeQuery(TABLE, { limit: 0 } as any, READ); + + // `find()` adds its own ORDER BY tie-breaker, so the whole statements are + // not expected to match — the pagination tail is the part under test. + expect(paginationTail(emitted.sql)).toBe('limit ?'); + expect(paginationTail(analyzed.sql)).toBe('limit ?'); + expect(emitted.bindings).toContain(0); + expect(analyzed.bindings).toContain(0); + }); + + it('emitted a statement with NO limit at all before this fix — the regression sentinel', async () => { + const analyzed = await driver.analyzeQuery(TABLE, { limit: 0 } as any, READ); + expect(analyzed.sql.toLowerCase()).toContain('limit'); + }); + + it('still carries the LIMIT for a non-zero `limit`', async () => { + const analyzed = await driver.analyzeQuery(TABLE, { limit: 2 } as any, READ); + expect(paginationTail(analyzed.sql)).toBe('limit ?'); + expect(analyzed.bindings).toContain(2); + }); + + it('emits no LIMIT when the caller gave none — presence, not a constant', async () => { + const analyzed = await driver.analyzeQuery(TABLE, {} as any, READ); + expect(analyzed.sql.toLowerCase()).not.toContain('limit'); + }); + + it('returns a plan alongside the statement, unchanged by any of this', async () => { + const analyzed = await driver.analyzeQuery(TABLE, { limit: 0 } as any, READ); + expect(analyzed.plan).toBeDefined(); + expect(analyzed.error).toBeUndefined(); + }); + }); + + /** + * The measurement that keeps the `offset` half of this change honest. It is a + * claim about knex, not about this driver, which is why it is compiled rather + * than executed and why all three dialects are named. + */ + describe('a zero offset is elided by knex on every dialect — so the offset flip is a no-op', () => { + for (const client of ['better-sqlite3', 'pg', 'mysql2'] as const) { + it(`${client}: \`.offset(0)\` compiles to no OFFSET clause`, async () => { + const k = knexLib({ client, useNullAsDefault: true } as any); + try { + expect(k('orders').select('*').offset(0).toSQL().sql.toLowerCase()).not.toContain('offset'); + // ...while a zero LIMIT is emitted, which is why that half is a fix. + expect(k('orders').select('*').limit(0).toSQL().sql.toLowerCase()).toContain('limit'); + // ...and a non-zero offset still reaches the statement. + expect(k('orders').select('*').offset(3).toSQL().sql.toLowerCase()).toContain('offset'); + } finally { + await k.destroy(); + } + }); + } + }); +}); diff --git a/packages/drivers/driver-sql/src/sql-driver-pagination-conformance.test.ts b/packages/drivers/driver-sql/src/sql-driver-pagination-conformance.test.ts index d168ff4191..c0a19300f7 100644 --- a/packages/drivers/driver-sql/src/sql-driver-pagination-conformance.test.ts +++ b/packages/drivers/driver-sql/src/sql-driver-pagination-conformance.test.ts @@ -70,6 +70,7 @@ import { PAGINATION_CASES, PAGINATION_ROWS, PAGINATION_UNORDERED_CASES, + PAGINATION_ZERO_LIMIT_CASES, } from '@objectstack/spec/data'; import type { QueryAST } from '@objectstack/spec/data'; import { SqlDriver } from '../src/index.js'; @@ -270,6 +271,27 @@ function declarePartitionSweep(cell: DialectCell): void { expect(rows.map((r) => r.id)).toEqual(PAGINATION_ROWS.map((r) => r.id)); } }); + + /** + * `limit: 0` means "return no records" (#6485/#6577). `findRows()` has + * always compiled `limit` on presence, so this cell is green on arrival — + * which is the point of pinning it: two sibling doors in this same file + * compiled it on TRUTHINESS until #6577, and returned the whole table for a + * query that asked for none. Run per dialect because `LIMIT 0` is the one + * bound value a server is free to treat as a special case. + */ + describe('`limit: 0` returns no records', () => { + for (const testCase of PAGINATION_ZERO_LIMIT_CASES) { + it(testCase.name, async () => { + const rows = await driver.find( + PAGED_TABLE, + { ...testCase.query }, + { bypassTenantAudit: true }, + ); + expect(rows).toHaveLength(testCase.expectedRowCount); + }); + } + }); }); } diff --git a/packages/drivers/driver-sql/src/sql-driver.ts b/packages/drivers/driver-sql/src/sql-driver.ts index be8b1d91ce..fc3a44a838 100644 --- a/packages/drivers/driver-sql/src/sql-driver.ts +++ b/packages/drivers/driver-sql/src/sql-driver.ts @@ -4194,8 +4194,14 @@ export class SqlDriver implements IDataDriver { } } - if (query.limit) builder.limit(query.limit); - if (query.offset) builder.offset(query.offset); + // PRESENCE, not truthiness — the same test `findRows()` makes (#6577). + // `limit: 0` means "return no records" (#6485), and `0` is falsy, so + // `if (query.limit)` dropped the clause and answered a request for NOTHING + // with the WHOLE table. Measured on `main` before this line changed: three + // rows seeded, `{ limit: 0 }` returned 3 here and 0 through `find()` — one + // driver, two answers to one `QueryAST`. + if (query.limit !== undefined) builder.limit(query.limit); + if (query.offset !== undefined) builder.offset(query.offset); return await builder; } @@ -4234,8 +4240,15 @@ export class SqlDriver implements IDataDriver { } } - if (query.limit) builder.limit(query.limit); - if (query.offset) builder.offset(query.offset); + // PRESENCE, not truthiness — see `findWithWindowFunctions()` above (#6577). + // The stake is different here and no smaller: this door returns a PLAN, and + // a plan is only worth reading if it explains the statement `find()` would + // actually run. Measured on `main` before this line changed: `{ limit: 0 }` + // compiled to `select * from `orders`` while `find()` sent + // `select * from `orders` order by `id` asc limit ?` — an EXPLAIN for a + // different query, which is the one thing an EXPLAIN must never be. + if (query.limit !== undefined) builder.limit(query.limit); + if (query.offset !== undefined) builder.offset(query.offset); const sql = builder.toSQL(); const client = (this.config as any).client; diff --git a/packages/drivers/driver-sqlite-wasm/src/sqlite-wasm-pagination-conformance.test.ts b/packages/drivers/driver-sqlite-wasm/src/sqlite-wasm-pagination-conformance.test.ts index 2f16426328..e436f1a2dd 100644 --- a/packages/drivers/driver-sqlite-wasm/src/sqlite-wasm-pagination-conformance.test.ts +++ b/packages/drivers/driver-sqlite-wasm/src/sqlite-wasm-pagination-conformance.test.ts @@ -26,9 +26,13 @@ import { PAGINATION_CASES, PAGINATION_ROWS, PAGINATION_UNORDERED_CASES, + PAGINATION_ZERO_LIMIT_CASES, } from '@objectstack/spec/data'; import { SqliteWasmDriver } from './index.js'; +/** Shared read options — a named const so no call site casts its own (#4674). */ +const READ_OPTIONS = { bypassTenantAudit: true } as unknown as Parameters[2]; + describe('driver-sqlite-wasm — paged reads are a partition of the result set', () => { let driver: SqliteWasmDriver; @@ -105,4 +109,20 @@ describe('driver-sqlite-wasm — paged reads are a partition of the result set', const rows = await driver.find('ticket', {} as any, { bypassTenantAudit: true } as any); expect(rows.map((r: any) => r.id)).toEqual(PAGINATION_ROWS.map((r) => r.id)); }); + + /** + * `limit: 0` means "return no records" (#6485/#6577). Inherited from + * `SqlDriver.findRows()`, which has always compiled `limit` on presence — but + * "inherits, therefore fine" is the assumption this whole file exists to + * disprove, and the sql.js dialect binds the LIMIT placeholder itself. A + * dialect that mis-bound a zero would fail in no other suite in the repo. + */ + describe('`limit: 0` returns no records', () => { + for (const testCase of PAGINATION_ZERO_LIMIT_CASES) { + it(testCase.name, async () => { + const rows = await driver.find('ticket', { ...testCase.query }, READ_OPTIONS); + expect(rows).toHaveLength(testCase.expectedRowCount); + }); + } + }); }); diff --git a/packages/drivers/driver-turso/src/remote-transport.ts b/packages/drivers/driver-turso/src/remote-transport.ts index ccdd92b2e3..e15473971b 100644 --- a/packages/drivers/driver-turso/src/remote-transport.ts +++ b/packages/drivers/driver-turso/src/remote-transport.ts @@ -1482,9 +1482,27 @@ export class RemoteTransport { } // PAGINATION + // + // SQLite's grammar is `LIMIT expr [OFFSET expr]` — an OFFSET cannot stand + // alone. This compiler emitted the two clauses independently, so + // `find(obj, { offset: N })` with no `limit` assembled `... OFFSET ?` and + // the server answered `near "OFFSET": syntax error` — for EVERY N, not a + // boundary value. The local transport never had it: knex synthesises the + // no-limit sentinel, compiling `.offset(3)` to `limit ? offset ?` bound + // `[-1, 3]` (measured). So this is the same statement knex would have + // built, and the two transports now answer a bare offset the same way + // instead of one working and one throwing (#6577, surfaced by the + // `PAGINATION_ZERO_LIMIT_CASES` control that reads with an offset alone). + // + // `-1` is SQLite's documented "no limit", not a magic number: a negative + // LIMIT means unbounded, which is exactly what "the caller gave no limit" + // has to compile to once the clause is mandatory. if (query.limit !== undefined) { sql += ` LIMIT ?`; allArgs.push(query.limit); + } else if (query.offset !== undefined) { + sql += ` LIMIT ?`; + allArgs.push(-1); } if (query.offset !== undefined) { sql += ` OFFSET ?`; diff --git a/packages/drivers/driver-turso/src/turso-pagination-conformance.test.ts b/packages/drivers/driver-turso/src/turso-pagination-conformance.test.ts index 4eeafb98cb..c6c2c0285f 100644 --- a/packages/drivers/driver-turso/src/turso-pagination-conformance.test.ts +++ b/packages/drivers/driver-turso/src/turso-pagination-conformance.test.ts @@ -31,6 +31,7 @@ import { PAGINATION_CASES, PAGINATION_ROWS, PAGINATION_UNORDERED_CASES, + PAGINATION_ZERO_LIMIT_CASES, } from '@objectstack/spec/data'; import { TursoDriver } from './turso-driver.js'; @@ -128,4 +129,25 @@ describe('TursoDriver — paged reads are a partition of the result set (local m ); expect(rows.map((r) => String(r.id))).toEqual(PAGINATION_ROWS.map((r) => r.id)); }); + + /** + * `limit: 0` means "return no records" (#6485/#6577). The LOCAL transport + * inherits `SqlDriver.findRows()`, which compiles `limit` on presence; the + * REMOTE transport compiles its own LIMIT/OFFSET and is pinned by the twin of + * this block in `turso-remote-pagination-conformance.test.ts`. Both are + * asserted because this driver is dual-transport, and a contract only one + * half keeps is a contract this package does not keep. + */ + describe('`limit: 0` returns no records', () => { + for (const testCase of PAGINATION_ZERO_LIMIT_CASES) { + it(testCase.name, async () => { + const rows: Array> = await driver.find( + 'ticket', + { ...testCase.query }, + { bypassTenantAudit: true }, + ); + expect(rows).toHaveLength(testCase.expectedRowCount); + }); + } + }); }); diff --git a/packages/drivers/driver-turso/src/turso-remote-pagination-conformance.test.ts b/packages/drivers/driver-turso/src/turso-remote-pagination-conformance.test.ts index 6678ba1e39..bf1803a7c6 100644 --- a/packages/drivers/driver-turso/src/turso-remote-pagination-conformance.test.ts +++ b/packages/drivers/driver-turso/src/turso-remote-pagination-conformance.test.ts @@ -61,6 +61,7 @@ import { PAGINATION_CASES, PAGINATION_ROWS, PAGINATION_UNORDERED_CASES, + PAGINATION_ZERO_LIMIT_CASES, } from '@objectstack/spec/data'; import { TursoDriver } from './turso-driver.js'; import { makeLibsqlSqliteStub, type LibsqlSqliteStub } from './libsql-sqlite-stub.testkit.js'; @@ -196,4 +197,94 @@ describe('TursoDriver remote — paged reads are a partition of the result set', expect(seen).toEqual([...PAGINATION_ALL_IDS].sort()); expect(seen).not.toEqual(PAGINATION_ROWS.map((r) => r.id)); }); + + /** + * `limit: 0` means "return no records" (#6485/#6577). This transport does not + * go through knex at all — `remote-transport.ts` assembles its own + * `LIMIT`/`OFFSET` — so its agreement with the local half is a separate fact + * needing its own measurement, not an inheritance. It reads `!== undefined` + * today; this is what stops that from silently becoming `if (query.limit)`. + */ + describe('`limit: 0` returns no records', () => { + for (const testCase of PAGINATION_ZERO_LIMIT_CASES) { + it(testCase.name, async () => { + const rows: Array> = await driver.find('ticket', { ...testCase.query }); + expect(rows).toHaveLength(testCase.expectedRowCount); + }); + } + }); + + /** + * The SELECT this transport puts on the wire for `query`, read off a + * recording client rather than recompiled here — the same instrument + * `remote-pagination-tiebreaker.test.ts` uses, and for the same reason: a + * test that rebuilds the expected statement asserts only that the builder + * works, and stays green on the day `find()` stops calling it. + */ + async function statementFor(query: Record): Promise<{ sql: string; args: unknown[] }> { + const sent: Array<{ sql: string; args: unknown[] }> = []; + const recorder = makeLibsqlSqliteStub(); + const client = { + execute: async (stmt: { sql: string; args?: unknown[] }) => { + sent.push({ sql: stmt.sql, args: stmt.args ?? [] }); + return recorder.execute(stmt); + }, + batch: async (stmts: Array<{ sql: string; args?: unknown[] }>) => { + for (const s of stmts) sent.push({ sql: s.sql, args: s.args ?? [] }); + return Promise.all(stmts.map((s) => recorder.execute(s))); + }, + close: () => recorder.close(), + }; + const recording = new TursoDriver({ url: 'libsql://recorder.turso.io', client: client as never }); + await recording.connect(); + await recording.syncSchema(TICKET_OBJECT.name, TICKET_OBJECT); + sent.length = 0; // drop the DDL round-trip; only the read is under test + await recording.find('ticket', query); + await recording.disconnect(); + recorder.close(); + const reads = sent.filter((s) => /^\s*SELECT/i.test(s.sql)); + expect(reads).toHaveLength(1); + return reads[0]!; + } + + /** + * An OFFSET with no LIMIT — the defect the case-set's bare-offset control + * surfaced here, and a separate bug from the `limit: 0` one it was added for. + * + * SQLite's grammar is `LIMIT expr [OFFSET expr]`, so an OFFSET cannot stand + * alone. This compiler emitted the two clauses independently and the server + * answered `near "OFFSET": syntax error` — for every offset value, not a + * boundary case, and only on THIS transport: the local half goes through knex, + * which synthesises the `LIMIT -1` no-limit sentinel. + * + * Kept as its own block rather than left to the shared control because the + * control asserts a row COUNT, and a count assertion reports this as "expected + * 12, got a thrown error" — which reads as a pagination fault rather than as + * a statement that never parsed. + */ + describe('an offset with no limit still assembles a legal statement', () => { + for (const offset of [0, 1, 5]) { + it(`offset ${offset} alone does not throw a syntax error`, async () => { + const rows: Array> = await driver.find('ticket', { offset }); + expect(rows).toHaveLength(PAGINATION_ROWS.length - offset); + }); + } + + it('sends what knex builds for the local transport — `LIMIT ? OFFSET ?` bound `[-1, 3]`', async () => { + const sent = await statementFor({ offset: 3 }); + expect(sent.sql.toUpperCase()).toContain('LIMIT ? OFFSET ?'); + expect(sent.args).toEqual([-1, 3]); + }); + + it("still sends the caller's own limit when one was given — the sentinel is not a default", async () => { + const sent = await statementFor({ limit: 0, offset: 3 }); + expect(sent.args).toEqual([0, 3]); + }); + + it('adds no LIMIT at all when neither was given — the sentinel is not unconditional', async () => { + const sent = await statementFor({}); + expect(sent.sql.toUpperCase()).not.toContain('LIMIT'); + expect(sent.args).toEqual([]); + }); + }); }); diff --git a/packages/spec/api-surface/data.json b/packages/spec/api-surface/data.json index d1c0460774..95097d55e9 100644 --- a/packages/spec/api-surface/data.json +++ b/packages/spec/api-surface/data.json @@ -402,6 +402,7 @@ "PAGINATION_CASES (const)", "PAGINATION_ROWS (const)", "PAGINATION_UNORDERED_CASES (const)", + "PAGINATION_ZERO_LIMIT_CASES (const)", "PaginationConformanceCase (interface)", "PaginationConformanceRow (interface)", "PerOperationRequiredPermissions (type)", @@ -569,6 +570,7 @@ "ValidationRuleSchema (const)", "ValueForm (type)", "ValueShapeFieldDef (interface)", + "ZeroLimitConformanceCase (interface)", "canonicalAstOperator (function)", "canonicalizeSqlType (function)", "classifyFilterToken (function)", diff --git a/packages/spec/src/data/pagination-conformance.ts b/packages/spec/src/data/pagination-conformance.ts index 1e24e1c76f..5acd0de459 100644 --- a/packages/spec/src/data/pagination-conformance.ts +++ b/packages/spec/src/data/pagination-conformance.ts @@ -168,3 +168,78 @@ export const PAGINATION_UNORDERED_CASES: readonly UnorderedPaginationConformance /** Every id in {@link PAGINATION_ROWS}, for the "visited exactly once" check. */ export const PAGINATION_ALL_IDS: readonly string[] = PAGINATION_ROWS.map((r) => r.id); + +/** + * One read whose `limit` must be honoured **as written** — including when it is + * `0`. + * + * # The ruling + * + * `limit: 0` means **return no records** (objectstack#6485). It is not "no + * limit", and it is not a value a layer may drop on its way down. + * + * # Why this is a separate case-set from {@link PAGINATION_CASES} + * + * Everything above is about a *page* being a partition. This is about the page + * SIZE being read at all, and the two fail independently: a driver can walk + * twelve rows in a perfect partition and still answer `{ limit: 0 }` with the + * whole table, because the two live at different lines. Keeping them apart is + * also what keeps the coverage ledger honest — a driver that answers one and + * not the other is a half-covered cell, and a shared marker would let it import + * its way to green. + * + * # The defect this exists to catch + * + * `if (query.limit) { ... }` — truthiness where the contract asks for presence. + * `0` is falsy, so the clause is dropped and the read that asked for nothing is + * answered with everything: the widest possible answer to the narrowest + * possible request. It is invisible from any single backend, which is why it + * needs a shared standard — two shipped drivers answered the same `QueryAST` + * with opposite result sets, and the one that disagreed returned MORE data than + * was asked for rather than less (objectstack#6577). + * + * # Why the controls are not padding + * + * A driver that returns `[]` for every query passes every `limit: 0` row here + * and fails its users completely. So each zero case is stated beside the + * non-zero read it must NOT become. The property is "`limit` is honoured as + * written", not "`limit: 0` is special-cased". + * + * # Scope + * + * `find()` and whatever a driver builds on it. Nothing here says HOW a driver + * keeps the promise, and the answers genuinely differ — which is the reason + * this is a shared case-set rather than a lint rule. The SQL family needed a + * presence check; `driver-memory` needed the same on its slice; and + * `driver-mongodb` needed neither, because it already forwarded `0` faithfully + * — into a client that DEFINES `limit: 0` as *no limit*. There the contract has + * to be answered BEFORE the client is consulted, by a short-circuit that + * returns the empty result without a round trip. Same standard, three + * mechanisms; the cases test the property, not the clause. + */ +export interface ZeroLimitConformanceCase { + /** Case label, used as the test name. */ + name: string; + /** The query, exactly as a caller writes it. */ + query: { limit?: number; offset?: number }; + /** + * How many of the twelve {@link PAGINATION_ROWS} must come back. A count + * rather than an id set on purpose: `offset` with no `orderBy` leaves + * *which* rows unspecified, and that is + * {@link PAGINATION_UNORDERED_CASES}' question, not this one. + */ + expectedRowCount: number; +} + +/** + * The cases. Three ask for nothing and must get nothing; three are the controls + * that stop "return nothing, always" from passing. + */ +export const PAGINATION_ZERO_LIMIT_CASES: readonly ZeroLimitConformanceCase[] = [ + { name: 'limit 0 returns no records', query: { limit: 0 }, expectedRowCount: 0 }, + { name: 'limit 0 with an explicit offset 0 returns no records', query: { limit: 0, offset: 0 }, expectedRowCount: 0 }, + { name: 'limit 0 past the first page still returns no records', query: { limit: 0, offset: 5 }, expectedRowCount: 0 }, + { name: 'control — limit 2 returns two records', query: { limit: 2 }, expectedRowCount: 2 }, + { name: 'control — offset 0 alone does not truncate', query: { offset: 0 }, expectedRowCount: PAGINATION_ROWS.length }, + { name: 'control — no limit returns every record', query: {}, expectedRowCount: PAGINATION_ROWS.length }, +]; diff --git a/scripts/check-driver-conformance.mjs b/scripts/check-driver-conformance.mjs index 218b968ce9..22f884f5e4 100644 --- a/scripts/check-driver-conformance.mjs +++ b/scripts/check-driver-conformance.mjs @@ -151,6 +151,11 @@ const CASE_SETS = [ marker: 'PAGINATION_UNORDERED_CASES', what: 'an UNSORTED paged read is a partition too — #4363', }, + { + file: 'pagination-conformance.ts', + marker: 'PAGINATION_ZERO_LIMIT_CASES', + what: '`limit: 0` returns no records, on presence not truthiness — #6485/#6577', + }, { file: 'filter-text-conformance.ts', marker: 'FILTER_TEXT_CASES',