diff --git a/.changeset/paged-read-determinism.md b/.changeset/paged-read-determinism.md index 2752ff1bd4..3c2528f4f9 100644 --- a/.changeset/paged-read-determinism.md +++ b/.changeset/paged-read-determinism.md @@ -39,7 +39,7 @@ The obligation is now normative on `IDataDriver.find`, with shared cases in `@objectstack/spec/data` (`PAGINATION_CASES`) that all three drivers run — so a future driver is held to it by a gate rather than by remembering. -Deliberately not covered: a paged read with **no** `orderBy`. That is -non-deterministic on every backend by definition and imposing an order on -callers who asked for none changes plan selection far more broadly; filed as -#4363. +Not covered by this change: a paged read with **no** `orderBy`. Same defect, +wider blast radius, so it was carved out to #4363 rather than folded in — and +closed there, in the same release. The contract, the shared cases and both +drivers now cover a paged read whatever its `orderBy`, including none at all. diff --git a/.changeset/unordered-paged-read-determinism.md b/.changeset/unordered-paged-read-determinism.md new file mode 100644 index 0000000000..441bc1bcff --- /dev/null +++ b/.changeset/unordered-paged-read-determinism.md @@ -0,0 +1,63 @@ +--- +"@objectstack/spec": patch +"@objectstack/driver-sql": patch +"@objectstack/driver-mongodb": patch +--- + +fix(data): a paged read with no `orderBy` is a partition too — the shape every list view actually sends (#4363) + +objectui#3106's server half closed the **sorted** paged read: a non-empty +`orderBy` now carries a unique tie-breaker, so `ORDER BY status LIMIT 50 OFFSET +50` can no longer serve one row twice while never serving another. It stopped +there deliberately. This closes the half it left, which is the more common one. + +A list view whose metadata configures no `sort`, on which nobody has clicked a +column header, sends no `$orderby` at all. `SqlDriver` and `MongoDBDriver` then +emitted a bare `LIMIT`/`OFFSET` — and neither backend promises anything about +the order that slices: + +- **SQL** leaves the row order of an unordered read to the plan. Small tables + hand back insertion order in practice, which is exactly why this survives + testing; a parallel scan, an index scan, or a `VACUUM` need not. +- **MongoDB** returns natural order, which describes where a document currently + sits in its extent — and moves when the document does. + +Every row ties with every other on an empty sort key, so this is the same defect +at full strength rather than a different one: page 2 repeats a row page 1 showed +and drops one nobody sees, with every page full and every row real. + +Both drivers now order a paged read by their unique key column when the caller +supplied no sort keys — the same `id` the tie-breaker was already appending, now +standing alone. `driver-memory` again needed no change: it slices its backing +array, and two reads with no write between them see the identical sequence. The +contract asks for a partition, not for id order. + +**Unpaged reads are untouched, deliberately.** The rule keys off `limit`/ +`offset`, not off `orderBy` being absent. A read with neither hands back the +whole matching set, so no caller can be shown a partial view of it, and sorting +every read in the system would change plan selection to buy nothing. `limit` +alone does count as paged: page one of a walk is routinely `limit=50` with no +offset, and ordering only the later pages would leave the defect fully intact. + +`SqlDriver` keeps the existing restriction to objects it created itself +(`initObjects` records them). It matters more here than for the sorted case: on +a federated table (ADR-0015) there is no requested sort for #3821's ladder to +fall back to, so a wrong guess about `id` would turn a reshuffle into a failed +read. Those tables now get a warning — once per object, behavior unchanged — +because the contract states determinism as a MUST, and a MUST that quietly does +not hold is the same invisible failure the rule was written against. + +`findOne` is deliberately outside all of this, and the contract now says so. +Engines reach a driver with `limit: 1`, which is shaped exactly like page one of +a walk, but it promises *a* matching record rather than a position in a +sequence — nothing for a second call to be inconsistent with. Reading it as a +page would put `ORDER BY id LIMIT 1` on the hottest read in the system, which is +the classic shape for a planner to abandon the predicate's own index: measured +on Postgres 16 over 2M rows, `WHERE owner_id = ? LIMIT 1` went 0.08 ms → 7.8 ms +and swapped the `owner_id` index for the primary key. `MongoDBDriver.findOne` +has never sorted, so this also puts the two drivers back in step. + +The obligation is normative on `IDataDriver.find` and the cases are shared — +`PAGINATION_UNORDERED_CASES` alongside `PAGINATION_CASES` in +`@objectstack/spec/data` — so a future driver is held to both halves by a gate +rather than by remembering. diff --git a/content/docs/data-modeling/queries.mdx b/content/docs/data-modeling/queries.mdx index 8e44f1bb15..bee266d2ef 100644 --- a/content/docs/data-modeling/queries.mdx +++ b/content/docs/data-modeling/queries.mdx @@ -239,6 +239,21 @@ reaching `engine.find()` directly are unaffected. } ``` + +**Walking the pages visits every row exactly once**, whatever you sort by — or +whether you sort at all. The SQL and MongoDB drivers get there by ordering on a +unique column of their own, on top of whatever `orderBy` you gave, because a +sort key like `status` does not identify a row and no backend promises equal +keys keep the same arrangement between two queries. Without that, page 2 repeats +a row page 1 already showed and skips one nobody ever sees — every page full, +every row real, the two halves of the symptom several screens apart +(objectui#3106, #4363). + +The guarantee attaches to `limit`/`offset`, so it costs nothing on the reads +that do not paginate: a query with neither is returned in whatever order the +backend chooses, exactly as before. + + ### Keyset Pagination — a `where` predicate on the sort key `query.cursor` was **removed in `@objectstack/spec` 18** (#4286): nothing on the server diff --git a/content/docs/protocol/objectql/query-syntax.mdx b/content/docs/protocol/objectql/query-syntax.mdx index b5f3a2863a..64d8d9ba91 100644 --- a/content/docs/protocol/objectql/query-syntax.mdx +++ b/content/docs/protocol/objectql/query-syntax.mdx @@ -859,6 +859,15 @@ const page2 = await engine.find('customer', { **Drawback:** Slow for large offsets (database still scans all skipped rows). +**Determinism:** the walk above visits every `customer` exactly once even though +it names no `orderBy`. A driver orders any paged read by a unique column of its +own — appended to your sort keys, or standing alone when you gave none — because +an unordered `LIMIT`/`OFFSET` slices an arrangement nothing holds steady: SQL +leaves row order to the plan, and MongoDB's natural order moves when a document +does. Page 2 would otherwise repeat a row from page 1 and drop another, with +every page full and every row real (objectui#3106, #4363). A query with no +`limit`/`offset` is untouched — nothing is being sliced, so no order is imposed. + ### Keyset Pagination diff --git a/packages/plugins/driver-memory/src/memory-pagination-conformance.test.ts b/packages/plugins/driver-memory/src/memory-pagination-conformance.test.ts index 3a8dcd912a..79f85b92f3 100644 --- a/packages/plugins/driver-memory/src/memory-pagination-conformance.test.ts +++ b/packages/plugins/driver-memory/src/memory-pagination-conformance.test.ts @@ -1,27 +1,41 @@ // Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. /** - * Deterministic paged reads for the in-memory driver (objectui#3106) — the - * contract on `IDataDriver.find`, checked against the shared cases in - * `@objectstack/spec/data`. + * Deterministic paged reads for the in-memory driver (objectui#3106, + * objectstack#4363) — the contract on `IDataDriver.find`, checked against the + * shared cases in `@objectstack/spec/data`. * - * This driver needed **no change** to satisfy it, and that is worth a suite - * rather than a shrug. It sorts with `Array#sort`, which ES2019 onward - * guarantees is stable, over a table array whose order does not move between - * two reads — so equal keys keep the same relative arrangement on page 2 that - * they had on page 1, which is precisely what the contract asks for. + * This driver needed **no change** to satisfy either half, and that is worth a + * suite rather than a shrug. + * + * - **Sorted pages.** It sorts with `Array#sort`, which ES2019 onward + * guarantees is stable, over a table array whose order does not move between + * two reads — so equal keys keep the same relative arrangement on page 2 that + * they had on page 1. + * - **Unsorted pages.** With no `orderBy` it slices that same array directly, + * and the array is the storage: two reads with no write between them see the + * identical sequence. There is no plan to change its mind and no natural + * order to move, so the walk partitions the set without a sort being imposed. + * The contract asks for determinism, not for id order, and this driver + * supplies it from a different direction than SQL and MongoDB do. * * The guarantee is therefore load-bearing but implicit: it rests on `sort` - * being stable and on `applySort` copying rather than reordering the table in - * place. Both are easy to lose in a refactor that looks like a speed-up — a - * hand-rolled quicksort, or sorting the backing array directly — and neither - * loss would fail any other test in this package. That is what this file is - * for: it holds the property against the day the implementation changes, which - * is the only day it could break. + * being stable, on `applySort` copying rather than reordering the table in + * place, and on `find` slicing a copy rather than the live table. All are easy + * to lose in a refactor that looks like a speed-up — a hand-rolled quicksort, + * sorting the backing array directly, a "reuse the array" allocation saving — + * and no other test in this package would fail. That is what this file is for: + * it holds the property against the day the implementation changes, which is + * the only day it could break. */ import { describe, it, expect, beforeEach } from 'vitest'; -import { PAGINATION_ALL_IDS, PAGINATION_CASES, PAGINATION_ROWS } from '@objectstack/spec/data'; +import { + PAGINATION_ALL_IDS, + PAGINATION_CASES, + PAGINATION_ROWS, + PAGINATION_UNORDERED_CASES, +} from '@objectstack/spec/data'; import { InMemoryDriver } from './memory-driver.js'; describe('InMemoryDriver — paged reads are a partition of the result set (objectui#3106)', () => { @@ -67,4 +81,33 @@ describe('InMemoryDriver — paged reads are a partition of the result set (obje expect(paged.map((r) => r.id)).toEqual(whole.map((r: any) => r.id)); }); } + + for (const testCase of PAGINATION_UNORDERED_CASES) { + it(`visits every row exactly once with NO orderBy at all — ${testCase.name}`, async () => { + const seen: string[] = []; + for (let offset = 0; offset < PAGINATION_ROWS.length; offset += testCase.pageSize) { + const page = await driver.find('ticket', { limit: testCase.pageSize, offset } as any); + seen.push(...page.map((r: any) => String(r.id))); + } + + expect(seen).toHaveLength(PAGINATION_ALL_IDS.length); + expect(new Set(seen).size).toBe(PAGINATION_ALL_IDS.length); + expect([...seen].sort()).toEqual([...PAGINATION_ALL_IDS].sort()); + }); + + it(`page boundaries are invisible with NO orderBy — ${testCase.name}`, async () => { + const paged: any[] = []; + for (let offset = 0; offset < PAGINATION_ROWS.length; offset += testCase.pageSize) { + const page = await driver.find('ticket', { limit: testCase.pageSize, offset } as any); + paged.push(...page); + } + + // Concatenating the pages reproduces the unpaged read exactly. Note this + // is insertion order, NOT id order: the contract asks for a partition, + // and this driver's storage already provides one, so nothing is imposed. + const whole = await driver.find('ticket', {} as any); + expect(paged.map((r) => r.id)).toEqual(whole.map((r: any) => r.id)); + expect(paged.map((r) => r.id)).toEqual(PAGINATION_ROWS.map((r) => r.id)); + }); + } }); diff --git a/packages/plugins/driver-mongodb/src/mongodb-driver.ts b/packages/plugins/driver-mongodb/src/mongodb-driver.ts index 0a8dba45af..f1d768025b 100644 --- a/packages/plugins/driver-mongodb/src/mongodb-driver.ts +++ b/packages/plugins/driver-mongodb/src/mongodb-driver.ts @@ -238,7 +238,7 @@ export class MongoDBDriver implements IDataDriver { } // Sorting - const sort = this.buildSortSpec(query.orderBy); + const sort = this.buildSortSpec(query); if (sort) findOptions.sort = sort; // Pagination @@ -277,7 +277,7 @@ export class MongoDBDriver implements IDataDriver { projection: { _id: 0 }, }; - const sort = this.buildSortSpec(query.orderBy); + const sort = this.buildSortSpec(query); if (sort) findOptions.sort = sort; if (query.offset !== undefined) findOptions.skip = query.offset; @@ -609,27 +609,46 @@ export class MongoDBDriver implements IDataDriver { * silently drops another — with every page full, every row real, and the two * halves of the symptom too far apart for anyone to notice. * + * A paged read with **no** `sort` is the same defect at full strength + * (objectstack#4363), which is why this reads the whole query rather than + * just its `orderBy`. Unsorted documents come back in natural order, and + * natural order describes where a document currently sits in its extent — it + * moves when the document does, so page 2 of a walk can be cut from a layout + * page 1 no longer describes. The empty sort key is simply the case where + * every document ties with every other, so the same `id` suffix that was + * separating one `status` group ends up carrying the entire order. + * * `id` is always present (`create()` fills it when the caller omits one), so - * unlike the SQL driver there is no table this cannot apply to. It is - * appended in the LAST requested key's direction: determinism holds either - * way, but a same-direction suffix is the one a compound index can still walk - * in a single pass. + * unlike the SQL driver there is no collection this cannot apply to, and + * `syncCollectionSchema` gives every collection it provisions a unique + * `idx_id_unique` — so a sort that ends in `id` is index-served rather than + * a blocking in-memory sort against the 100 MB cap. It is appended in the + * LAST requested key's direction (`1` when there is none): determinism holds + * either way, but a same-direction suffix is the one a compound index can + * still walk in a single pass. * - * Returns `undefined` when nothing was requested — an unordered read stays - * unordered (see the contract's explicit carve-out). + * Returns `undefined` for a read that is neither sorted nor paged — nothing + * is being sliced there, so a caller who asked for no order keeps none (the + * contract's explicit carve-out). */ - private buildSortSpec(orderBy: QueryAST['orderBy']): Document | undefined { - if (!orderBy || !Array.isArray(orderBy)) return undefined; + private buildSortSpec(query: QueryAST): Document | undefined { const sort: Document = {}; let lastDirection: 1 | -1 = 1; - for (const item of orderBy) { - if (item.field) { - lastDirection = item.order === 'desc' ? -1 : 1; - sort[this.mapFieldName(item.field)] = lastDirection; + if (Array.isArray(query.orderBy)) { + for (const item of query.orderBy) { + if (item.field) { + lastDirection = item.order === 'desc' ? -1 : 1; + sort[this.mapFieldName(item.field)] = lastDirection; + } } } - if (Object.keys(sort).length === 0) return undefined; - if (sort.id === undefined) sort.id = lastDirection; + + const requested = Object.keys(sort).length > 0; + const paged = query.limit !== undefined || query.offset !== undefined; + if (!requested && !paged) return undefined; + + const idKey = this.mapFieldName('id'); + if (sort[idKey] === undefined) sort[idKey] = lastDirection; return sort; } diff --git a/packages/plugins/driver-mongodb/src/mongodb-pagination-conformance.test.ts b/packages/plugins/driver-mongodb/src/mongodb-pagination-conformance.test.ts index 8827ecd293..eb35523fbd 100644 --- a/packages/plugins/driver-mongodb/src/mongodb-pagination-conformance.test.ts +++ b/packages/plugins/driver-mongodb/src/mongodb-pagination-conformance.test.ts @@ -13,14 +13,25 @@ * boundaries that would otherwise be where a row is served twice while another * is never served at all. * + * A paged read with no `sort` at all (objectstack#4363) is covered by the same + * fixture's `PAGINATION_UNORDERED_CASES`. It is the harder half to observe here + * — an untouched twelve-document collection comes back in natural order every + * time, which happens to be stable — so the sort-spec assertions carry that + * half: they are what fails if the `id` order stops being emitted on a day the + * fixture would have come back in a usable order anyway. + * * The sort-spec assertions at the end are deliberately about the spec object - * rather than the rows: they are what fails if the tie-breaker is removed on a - * day the fixture happens to come back in a stable order anyway. + * rather than the rows, for the same reason. */ import { describe, it, expect, beforeAll, afterAll } from 'vitest'; import { MongoMemoryServer } from 'mongodb-memory-server'; -import { PAGINATION_ALL_IDS, PAGINATION_CASES, PAGINATION_ROWS } from '@objectstack/spec/data'; +import { + PAGINATION_ALL_IDS, + PAGINATION_CASES, + PAGINATION_ROWS, + PAGINATION_UNORDERED_CASES, +} from '@objectstack/spec/data'; import { MongoDBDriver } from './mongodb-driver.js'; let sharedMongod: MongoMemoryServer | undefined; @@ -83,25 +94,76 @@ describe.skipIf(!sharedMongod)('driver-mongodb — paged reads are a partition o }); } - it('leaves an unordered read alone — no sort is imposed on a caller who asked for none', () => { - expect(driver['buildSortSpec'](undefined)).toBeUndefined(); - expect(driver['buildSortSpec']([])).toBeUndefined(); + for (const testCase of PAGINATION_UNORDERED_CASES) { + it(`visits every row exactly once with NO sort at all — ${testCase.name}`, async () => { + const seen: string[] = []; + for (let offset = 0; offset < PAGINATION_ROWS.length; offset += testCase.pageSize) { + const page = await driver.find('ticket', { limit: testCase.pageSize, offset } as any); + seen.push(...page.map((r) => String(r.id))); + } + + expect(seen).toHaveLength(PAGINATION_ALL_IDS.length); + expect(new Set(seen).size).toBe(PAGINATION_ALL_IDS.length); + expect([...seen].sort()).toEqual([...PAGINATION_ALL_IDS].sort()); + }); + } +}); + +/** + * The sort spec itself — the half that fails when the feature is deleted. + * + * Deliberately **outside** the `skipIf`, on a driver that is constructed but + * never connected: `buildSortSpec` is pure, and the constructor opens no socket. + * Gating it on `mongodb-memory-server` would be a false dependency with a real + * cost, because that server is exactly what a restricted network cannot fetch — + * and it is precisely there that the whole file would otherwise go quiet while + * still reporting green. A driver's most load-bearing assertion should not be + * the first thing an offline run drops. + */ +describe('MongoDBDriver — the sort spec sent to the server', () => { + // Never connected: no URI is dialed, so this needs no server binary. + const driver = new MongoDBDriver({ url: 'mongodb://127.0.0.1:1/unused', database: 'unused' }); + + it('leaves an UNPAGED unordered read alone — no sort is imposed on a caller who asked for none', () => { + // The carve-out, and the reason this rule asks about pagination rather than + // sorting every read: with no slice there is no partial view to be wrong + // about, so the plan stays whatever it was. + expect(driver['buildSortSpec']({})).toBeUndefined(); + expect(driver['buildSortSpec']({ orderBy: [] })).toBeUndefined(); + expect(driver['buildSortSpec']({ where: { status: 'open' } })).toBeUndefined(); + }); + + it('orders a paged read by `id` when the caller sent no sort (objectstack#4363)', () => { + expect(driver['buildSortSpec']({ limit: 5, offset: 5 })).toEqual({ id: 1 }); + // `limit` alone is page one of a walk — ordering only the pages that carry + // an `offset` would leave page one cut from a different arrangement, which + // is the whole defect. + expect(driver['buildSortSpec']({ limit: 5 })).toEqual({ id: 1 }); + expect(driver['buildSortSpec']({ offset: 5 })).toEqual({ id: 1 }); + expect(driver['buildSortSpec']({ orderBy: [], limit: 5 })).toEqual({ id: 1 }); }); it('appends `id` in the LAST key\'s direction', () => { - expect(driver['buildSortSpec']([{ field: 'status', order: 'asc' }])).toEqual({ + expect(driver['buildSortSpec']({ orderBy: [{ field: 'status', order: 'asc' }] })).toEqual({ status: 1, id: 1, }); expect( - driver['buildSortSpec']([ - { field: 'status', order: 'asc' }, - { field: 'rank', order: 'desc' }, - ]), + driver['buildSortSpec']({ + orderBy: [ + { field: 'status', order: 'asc' }, + { field: 'rank', order: 'desc' }, + ], + }), ).toEqual({ status: 1, rank: -1, id: -1 }); }); it('does not override `id` when the caller already sorted by it', () => { - expect(driver['buildSortSpec']([{ field: 'id', order: 'desc' }])).toEqual({ id: -1 }); + expect(driver['buildSortSpec']({ orderBy: [{ field: 'id', order: 'desc' }] })).toEqual({ + id: -1, + }); + expect( + driver['buildSortSpec']({ orderBy: [{ field: 'id', order: 'desc' }], limit: 5, offset: 5 }), + ).toEqual({ id: -1 }); }); }); diff --git a/packages/plugins/driver-sql/src/sql-driver-pagination-conformance.test.ts b/packages/plugins/driver-sql/src/sql-driver-pagination-conformance.test.ts index 4f8d3aab06..e37c987d9a 100644 --- a/packages/plugins/driver-sql/src/sql-driver-pagination-conformance.test.ts +++ b/packages/plugins/driver-sql/src/sql-driver-pagination-conformance.test.ts @@ -1,9 +1,9 @@ // Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. /** - * Deterministic paged reads for the SQL driver (objectui#3106) — the contract - * on `IDataDriver.find`, checked against the shared cases in - * `@objectstack/spec/data` so this driver, `driver-memory` and + * Deterministic paged reads for the SQL driver (objectui#3106, + * objectstack#4363) — the contract on `IDataDriver.find`, checked against the + * shared cases in `@objectstack/spec/data` so this driver, `driver-memory` and * `driver-mongodb` answer one standard. * * Two halves, because either alone would be reassuring and wrong: @@ -12,36 +12,54 @@ * partition the collection. This is what a user experiences, and it is what * a future driver has to satisfy however it chooses to. * - * 2. **The clause** — assert the compiled SQL actually carries the tie-breaker. - * The property test cannot be trusted on its own *here*: SQLite over a - * twelve-row table will hand back ties in rowid order every time, so the - * partition check passes with or without the fix. It is a real check on - * MongoDB (where the reshuffle is documented and observable) and a - * regression lock on the memory driver; on SQL it would pass the day - * someone deletes the feature. The compiled-SQL assertion is the half that - * fails then. + * 2. **The clause** — assert the SQL that actually reached the database + * carries the ordering. The property test cannot be trusted on its own + * *here*: SQLite over a twelve-row table hands back both ties and wholly + * unordered rows in rowid order every time, so the partition check passes + * with or without the fix. It is a real check on MongoDB (where the + * reshuffle is documented and observable) and a regression lock on the + * memory driver; on SQL it would pass the day someone deletes the feature. + * The emitted-SQL assertion is the half that fails then. * * That asymmetry is the point rather than an apology for it: the property is * the contract, and the clause is how this backend happens to keep it. + * + * The second half reads the statement off knex's `query` event during a real + * `find()` rather than recompiling one the way the driver would have. That + * distinction is load-bearing: a test that rebuilds the ORDER BY itself asserts + * only that the helper works, and stays green on the day `find()` stops calling + * it — which is precisely the day this file exists for. */ -import { describe, it, expect, beforeEach, afterEach } from 'vitest'; -import { PAGINATION_ALL_IDS, PAGINATION_CASES, PAGINATION_ROWS } from '@objectstack/spec/data'; +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { + PAGINATION_ALL_IDS, + PAGINATION_CASES, + PAGINATION_ROWS, + PAGINATION_UNORDERED_CASES, +} from '@objectstack/spec/data'; import { SqlDriver } from '../src/index.js'; -/** Reach the compiled statement without executing it. */ +/** + * 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 { - orderByClauseFor(object: string, orderBy: Array<{ field: string; order: 'asc' | 'desc' }>): string { - const builder = this.getBuilder(object); - for (const item of orderBy) { - builder.orderBy(this.remoteColumn(object, item.field, this.mapSortField(item.field)), item.order); - } - const tie = this.paginationTieBreaker(object); - if (tie && !orderBy.some((o) => o.field === tie)) { - const last = orderBy[orderBy.length - 1]?.order ?? 'asc'; - builder.orderBy(this.remoteColumn(object, tie, this.mapSortField(tie)), last); - } - return builder.toSQL().sql; + readonly statements: string[] = []; + + captureStatements(): void { + this.knex.on('query', (data: { sql?: string }) => { + if (typeof data?.sql === 'string') this.statements.push(data.sql); + }); + } + + /** The SQL of the single statement issued while running `run`. */ + async sqlOf(run: () => Promise): Promise { + const from = this.statements.length; + await run(); + const issued = this.statements.slice(from); + expect(issued, 'expected exactly one statement').toHaveLength(1); + return issued[0]!; } } @@ -115,13 +133,53 @@ describe('SqlDriver — paged reads are a partition of the result set (objectui# }); } - it('leaves an unordered read alone — no sort is imposed on a caller who asked for none', async () => { + for (const testCase of PAGINATION_UNORDERED_CASES) { + it(`visits every row exactly once with NO orderBy at all — ${testCase.name}`, async () => { + const seen: string[] = []; + for (let offset = 0; offset < PAGINATION_ROWS.length; offset += testCase.pageSize) { + const page = await driver.find( + 'ticket', + { limit: testCase.pageSize, offset } as any, + { bypassTenantAudit: true } as any, + ); + seen.push(...page.map((r) => String(r.id))); + } + + expect(seen).toHaveLength(PAGINATION_ALL_IDS.length); + expect(new Set(seen).size).toBe(PAGINATION_ALL_IDS.length); + expect([...seen].sort()).toEqual([...PAGINATION_ALL_IDS].sort()); + }); + + it(`walks an unsorted read in id order — ${testCase.name}`, async () => { + const paged: string[] = []; + for (let offset = 0; offset < PAGINATION_ROWS.length; offset += testCase.pageSize) { + const page = await driver.find( + 'ticket', + { limit: testCase.pageSize, offset } as any, + { bypassTenantAudit: true } as any, + ); + paged.push(...page.map((r) => String(r.id))); + } + + // The fixture's ids are shuffled relative to insertion order, so id order + // is visibly an order this driver chose rather than the one the table + // would have handed back anyway. + expect(paged).toEqual([...PAGINATION_ALL_IDS].sort()); + }); + } + + it('leaves an UNPAGED unordered read alone — no sort is imposed on a caller who asked for none', async () => { + // The carve-out (objectstack#4363): with no `limit`/`offset` the caller + // receives the whole matching set, so no slice can be wrong, and an imposed + // ORDER BY would only change plan selection. The order below is SQLite's + // own answer, not one this driver asked for — which the shuffled fixture + // ids make visible. const rows = await driver.find('ticket', {} as any, { bypassTenantAudit: true } as any); expect(rows.map((r) => r.id)).toEqual(PAGINATION_ROWS.map((r) => r.id)); }); }); -describe('SqlDriver — the ORDER BY actually carries the tie-breaker', () => { +describe('SqlDriver — the ORDER BY that reaches the database', () => { let driver: InspectableSqlDriver; beforeEach(async () => { @@ -133,35 +191,119 @@ describe('SqlDriver — the ORDER BY actually carries the tie-breaker', () => { await driver.initObjects([ { name: 'ticket', fields: { status: { type: 'string' }, rank: { type: 'integer' } } }, ]); + driver.captureStatements(); }); afterEach(async () => { await driver.disconnect(); }); - it('appends `id` after a non-unique sort key', () => { - const sql = driver.orderByClauseFor('ticket', [{ field: 'status', order: 'asc' }]); + const sqlOfFind = (query: Record) => + driver.sqlOf(() => driver.find('ticket', query as any, { bypassTenantAudit: true } as any)); + + it('appends `id` after a non-unique sort key', async () => { + const sql = await sqlOfFind({ orderBy: [{ field: 'status', order: 'asc' }] }); expect(sql).toMatch(/order by .*`status` asc, .*`id` asc/i); }); - it('appends `id` in the LAST key\'s direction, so one index pass can serve it', () => { - const sql = driver.orderByClauseFor('ticket', [ - { field: 'status', order: 'asc' }, - { field: 'rank', order: 'desc' }, - ]); + it('appends `id` in the LAST key\'s direction, so one index pass can serve it', async () => { + const sql = await sqlOfFind({ + orderBy: [ + { field: 'status', order: 'asc' }, + { field: 'rank', order: 'desc' }, + ], + }); expect(sql).toMatch(/order by .*`status` asc, .*`rank` desc, .*`id` desc/i); }); - it('does not repeat `id` when the caller already sorted by it', () => { - const sql = driver.orderByClauseFor('ticket', [{ field: 'id', order: 'desc' }]); + it('does not repeat `id` when the caller already sorted by it', async () => { + const sql = await sqlOfFind({ orderBy: [{ field: 'id', order: 'desc' }], limit: 5, offset: 5 }); expect(sql.match(/`id`/g) ?? []).toHaveLength(1); }); + it('orders a paged read by `id` when the caller sent no orderBy (objectstack#4363)', async () => { + expect(await sqlOfFind({ limit: 5, offset: 5 })).toMatch(/order by .*`id` asc/i); + }); + + it('counts `limit` alone as paged — page one must agree with pages two onward', async () => { + // A list view's first request is routinely `limit=50` with no offset at + // all. Ordering only the offset-carrying pages would cut page one from a + // different arrangement than the rest of the walk: the defect intact, + // wearing a fix. + expect(await sqlOfFind({ limit: 5 })).toMatch(/order by .*`id` asc/i); + expect(await sqlOfFind({ offset: 5 })).toMatch(/order by .*`id` asc/i); + expect(await sqlOfFind({ orderBy: [], limit: 5 })).toMatch(/order by .*`id` asc/i); + }); + + it('emits no ORDER BY for an unpaged read with no orderBy', async () => { + expect(await sqlOfFind({})).not.toMatch(/order by/i); + expect(await sqlOfFind({ where: { status: 'open' } })).not.toMatch(/order by/i); + expect(await sqlOfFind({ orderBy: [] })).not.toMatch(/order by/i); + }); + + it('leaves `findOne` unordered — its `limit: 1` is not page one of a walk', async () => { + // `findOne` promises *a* matching record, not a position in a sequence, so + // there is no partition to keep. Reading its `limit: 1` as a page would + // impose `ORDER BY id LIMIT 1` on the hottest read in the system, which is + // the shape that makes a planner abandon the predicate's own index: on a + // 2M-row Postgres table `WHERE owner_id = ? LIMIT 1` measured 0.08 ms + // unordered and 7.8 ms with `ORDER BY id`. `MongoDBDriver.findOne` has + // never sorted either — this keeps the two drivers saying the same thing. + const sql = await driver.sqlOf(() => + driver.findOne('ticket', { where: { status: 'open' } } as any, { + bypassTenantAudit: true, + } as any), + ); + expect(sql).not.toMatch(/order by/i); + expect(sql).toMatch(/limit/i); + }); + + it('still honors an orderBy the caller gave findOne, tie-breaker and all', async () => { + const sql = await driver.sqlOf(() => + driver.findOne('ticket', { orderBy: [{ field: 'status', order: 'desc' }] } as any, { + bypassTenantAudit: true, + } as any), + ); + expect(sql).toMatch(/order by .*`status` desc, .*`id` desc/i); + }); + it('adds nothing for an object this driver did not create', () => { // A federated table (ADR-0015) may have no `id` column at all. Sorting by a // column that isn't there raises an unknown-column error, which the #3821 // ladder answers by retrying with NO ORDER BY — so a guess here would cost - // the caller their whole sort to fix a reshuffle among ties. + // the caller their whole sort to fix a reshuffle among ties. It costs even + // more on the unsorted paged read: there is no requested sort to fall back + // to, so a wrong guess turns a reshuffle into a failed read. expect(driver['paginationTieBreaker']('some_remote_table')).toBeNull(); + expect(driver['orderKeysFor']('some_remote_table', { limit: 5, offset: 5 } as any)).toEqual([]); + }); + + it('says so, once, when it cannot keep the guarantee on a table it did not create', () => { + // Behavior is unchanged for these tables — the statement goes out exactly + // as before. What changes is that the gap is announced instead of being + // left for a user counting records to find, which is the same reason the + // rule exists at all. + const warn = vi.spyOn(driver['logger'], 'warn').mockImplementation(() => {}); + try { + driver['orderKeysFor']('some_remote_table', { limit: 5, offset: 5 } as any); + driver['orderKeysFor']('some_remote_table', { limit: 5, offset: 10 } as any); + expect(warn, 'once per object, not per query').toHaveBeenCalledTimes(1); + const message = warn.mock.calls[0]![0]; + expect(message, 'names the object').toContain('some_remote_table'); + expect(message, 'names the consequence').toMatch(/NOT deterministic/); + expect(message, 'names a remedy').toMatch(/orderBy/); + + // A managed table keeps the guarantee, so it must stay quiet. + driver['orderKeysFor']('ticket', { limit: 5, offset: 5 } as any); + // So must an unpaged read, and a sorted one: neither is the silent case. + driver['orderKeysFor']('some_remote_table', {} as any); + driver['orderKeysFor']( + 'some_remote_table', + { orderBy: [{ field: 'status', order: 'asc' }], limit: 5 } as any, + ); + expect(warn).toHaveBeenCalledTimes(1); + } finally { + warn.mockRestore(); + } }); }); diff --git a/packages/plugins/driver-sql/src/sql-driver.ts b/packages/plugins/driver-sql/src/sql-driver.ts index a0c13c9287..5cfca205af 100644 --- a/packages/plugins/driver-sql/src/sql-driver.ts +++ b/packages/plugins/driver-sql/src/sql-driver.ts @@ -800,6 +800,13 @@ export class SqlDriver implements IDataDriver { /** De-dup set for boot-time drift warnings (keyed by {@link driftKey}). */ protected driftWarned = new Set(); + /** + * Objects already reported as unable to keep the deterministic-paging + * contract (objectstack#4363) — see {@link orderKeysFor}. One line per + * object, not per query. + */ + protected nondeterministicPagingWarned = new Set(); + /** Deferred-DDL mode (#3917) — see {@link setDeferredDdl}. */ protected deferredDdl = false; @@ -1319,6 +1326,46 @@ export class SqlDriver implements IDataDriver { // =================================== async find(object: string, query: QueryAST, options?: DriverOptions): Promise { + return this.findRows(object, query, options); + } + + /** + * The body of {@link find}, shared with {@link findOne}. + * + * `singleRowLookup` marks the caller as `findOne` rather than a page, and the + * only thing it changes is that no ordering is imposed on an unsorted read + * (objectstack#4363). `findOne` reaches here as `limit: 1`, which is + * indistinguishable from "page one of a walk with page size 1" — and the two + * want opposite things: + * + * - The paged read needs a total order, or its pages are not a partition. + * - `findOne` needs the plan its predicate earned. `ORDER BY id LIMIT 1` is + * the shape that makes a planner abandon the predicate's own index and walk + * the primary key applying a filter instead, because at `LIMIT 1` that + * looks like it will stop early. Measured on Postgres 16 over 2M rows, + * `WHERE owner_id = ? LIMIT 1` went 0.08 ms → 7.8 ms (~100×) and swapped + * `ticket_owner_idx` for `ticket_pkey`; at `LIMIT 5` and up the planner + * keeps the right index and pays only a top-N sort (~2 ms). Nothing is + * bought for that: `findOne` promises *a* matching record, never a position + * in a sequence, so there is no partition to preserve. + * + * That also puts this driver back in step with `MongoDBDriver.findOne`, which + * issues `collection.findOne` and has never sorted. The obligation the + * contract states is on `find`, and this keeps it there. + */ + private async findRows( + object: string, + query: QueryAST, + options?: DriverOptions, + singleRowLookup = false, + ): Promise { + // The whole ORDER BY, decided once: the caller's keys plus whatever this + // driver has to add to make a paged read deterministic. Derived out here + // rather than inside `buildBase` so the recovery ladder below can ask + // whether the statement HAS a sort to drop, instead of asking whether the + // caller supplied one — since #4363 those are different questions. + const orderKeys = this.orderKeysFor(object, query, { singleRowLookup }); + // Build everything EXCEPT the SELECT list, so the unknown-column retry // below can rebuild without re-deriving where/order/pagination. const buildBase = (opts?: { withOrderBy?: boolean }) => { @@ -1332,22 +1379,9 @@ export class SqlDriver implements IDataDriver { } // ORDER BY - if (withOrderBy && query.orderBy && Array.isArray(query.orderBy)) { - const sorted: string[] = []; - let lastDirection: 'asc' | 'desc' = 'asc'; - for (const item of query.orderBy) { - if (item.field) { - lastDirection = item.order === 'desc' ? 'desc' : 'asc'; - b.orderBy(this.remoteColumn(object, item.field, this.mapSortField(item.field)), lastDirection); - sorted.push(item.field); - } - } - const tieBreaker = sorted.length > 0 ? this.paginationTieBreaker(object) : null; - if (tieBreaker && !sorted.includes(tieBreaker)) { - b.orderBy( - this.remoteColumn(object, tieBreaker, this.mapSortField(tieBreaker)), - lastDirection, - ); + if (withOrderBy) { + for (const key of orderKeys) { + b.orderBy(this.remoteColumn(object, key.field, this.mapSortField(key.field)), key.direction); } } @@ -1398,7 +1432,7 @@ export class SqlDriver implements IDataDriver { // to lose), then the sort, then give up. const retries: Array<() => any> = []; if (query.fields) retries.push(() => buildBase().select('*')); - if (query.orderBy && Array.isArray(query.orderBy) && query.orderBy.length > 0) { + if (orderKeys.length > 0) { retries.push(() => buildBase({ withOrderBy: false }).select('*')); } results = []; @@ -1442,7 +1476,7 @@ export class SqlDriver implements IDataDriver { } if (query && typeof query === 'object') { - const results = await this.find(object, { ...query, limit: 1 }, options); + const results = await this.findRows(object, { ...query, limit: 1 }, options, true); return results[0] || null; } @@ -5452,7 +5486,7 @@ export class SqlDriver implements IDataDriver { } /** - * The unique column appended to a non-empty ORDER BY so that paging is a + * The unique column this driver orders a paged read by, so that paging is a * partition of the result set rather than five independent queries that * happen to share a WHERE clause (objectui#3106, contract on * `IDataDriver.find`). @@ -5464,7 +5498,14 @@ export class SqlDriver implements IDataDriver { * perfect on its own, which is why this is found by a user counting records * and not by reading a response. * - * Returns `null` — no tie-breaker, prior behavior exactly — unless this + * "Tie-breaker" stays the right word when the caller sent **no** `orderBy` at + * all (objectstack#4363): on an empty sort key every row ties with every + * other, so the same column that was breaking ties within a `status` group + * ends up carrying the entire order. That is why one method answers both call + * sites — the question is the same one, and a driver that could answer it for + * a sorted page but not an unsorted one would be fixing the rarer half. + * + * Returns `null` — no ordering column, prior behavior exactly — unless this * driver **created** the table and therefore knows it carries the `id` * primary key (`initObjects` populates {@link managedObjectFields} for * exactly those). Guessing on a federated table (ADR-0015) would be worse @@ -5483,6 +5524,87 @@ export class SqlDriver implements IDataDriver { return managed ? 'id' : null; } + /** + * The complete ORDER BY for a `find()`: the caller's sort keys, followed by + * {@link paginationTieBreaker} when the read needs one to be deterministic. + * Logical field names — the call site maps each to a physical column. + * + * Three shapes, and the third is objectstack#4363: + * + * | `orderBy` | paged | result | + * |---|---|---| + * | non-empty | either | caller's keys + `id` | + * | empty | `limit`/`offset` present | `id` alone | + * | empty | neither present | **nothing** — the statement gets no ORDER BY | + * + * The last row is the deliberate carve-out, and it is why this asks about + * pagination at all rather than sorting every read. An unpaged read hands + * back the whole matching set: the caller sees every row whatever order they + * arrive in, so there is no partial view to be wrong about, and an imposed + * ORDER BY would only change plan selection for the majority of reads in the + * system. The moment `limit`/`offset` appear the caller is being shown a + * *slice*, and which rows fall in it stops being a matter of presentation. + * + * `limit` alone counts as paged, without waiting for an `offset`. Page one of + * a walk is routinely `limit=50` with no offset at all, and a page one that + * disagrees with the ordering pages two onward use is exactly the defect — + * ordering only the later pages would leave the bug fully intact while + * looking like a fix. `singleRowLookup` is how {@link findOne}'s own + * `limit: 1` stays out of that reading; see {@link findRows}. + * + * The tie-breaker goes on in the LAST requested key's direction (`asc` when + * there is none): determinism holds either way, but a same-direction suffix + * is the one a compound index can still walk in a single pass. + * + * When the read is paged, unsorted, and {@link paginationTieBreaker} has no + * column to offer, this warns once for that object. The behavior is unchanged + * — the statement goes out exactly as it did before — but the contract states + * determinism as a MUST, and a MUST that quietly does not hold is the same + * invisible failure the rule exists to remove. + */ + protected orderKeysFor( + object: string, + query: QueryAST, + opts?: { singleRowLookup?: boolean }, + ): Array<{ field: string; direction: 'asc' | 'desc' }> { + const keys: Array<{ field: string; direction: 'asc' | 'desc' }> = []; + if (Array.isArray(query.orderBy)) { + for (const item of query.orderBy) { + if (item.field) { + keys.push({ field: item.field, direction: item.order === 'desc' ? 'desc' : 'asc' }); + } + } + } + + const paged = + !opts?.singleRowLookup && (query.limit !== undefined || query.offset !== undefined); + if (keys.length === 0 && !paged) return keys; + + const tieBreaker = this.paginationTieBreaker(object); + if (tieBreaker && !keys.some((k) => k.field === tieBreaker)) { + keys.push({ field: tieBreaker, direction: keys[keys.length - 1]?.direction ?? 'asc' }); + } else if (!tieBreaker && keys.length === 0) { + // Paged, unsorted, and no column this driver can trust to order by: the + // read goes out exactly as before, and its pages are not a partition. + // Say so once per object rather than leave it to be discovered by a user + // counting records — a guarantee the contract states as MUST, quietly + // unavailable, is the same invisible failure it was written against. + // (Sorted reads on such a table keep the caller's own ORDER BY, so only + // the ties within it reshuffle; this case has nothing at all.) + if (!this.nondeterministicPagingWarned.has(object)) { + this.nondeterministicPagingWarned.add(object); + this.logger.warn( + `Paged read of '${object}' is NOT deterministic: this driver did not create the table, ` + + 'so it cannot name a unique column to order by, and the query asked for no sort of ' + + 'its own. Walking the pages may serve one row twice and never serve another ' + + '(objectstack#4363). Give the query an `orderBy` on a unique column, or declare the ' + + 'object so this driver manages its table.', + ); + } + } + return keys; + } + /** * Physical column for a logical field on an external object that declares an * `external.columnMap` (ADR-0015). Returns `fallback` (the caller's existing diff --git a/packages/spec/api-surface.json b/packages/spec/api-surface.json index fd05e11ca2..6035ae72ba 100644 --- a/packages/spec/api-surface.json +++ b/packages/spec/api-surface.json @@ -467,6 +467,7 @@ "PAGINATION_ALL_IDS (const)", "PAGINATION_CASES (const)", "PAGINATION_ROWS (const)", + "PAGINATION_UNORDERED_CASES (const)", "PaginationConformanceCase (interface)", "PaginationConformanceRow (interface)", "PerOperationRequiredPermissions (type)", @@ -589,6 +590,7 @@ "UniqueScope (type)", "UniqueScopeSchema (const)", "UnknownAuthoringKeyFinding (interface)", + "UnorderedPaginationConformanceCase (interface)", "VALID_AST_OPERATORS (const)", "ValidationRule (type)", "ValidationRuleSchema (const)", diff --git a/packages/spec/src/contracts/data-driver.ts b/packages/spec/src/contracts/data-driver.ts index 6a1022b48b..5ed7a6f9c8 100644 --- a/packages/spec/src/contracts/data-driver.ts +++ b/packages/spec/src/contracts/data-driver.ts @@ -65,27 +65,42 @@ export interface IDataDriver { * Find multiple records matching the structured query. * MUST return `id` as string. MUST NOT return implementation details like `_id`. * - * **Paged reads MUST be deterministic.** When `orderBy` is non-empty and - * `limit`/`offset` are used to walk the result set, the driver MUST produce a - * total order — reading page after page visits every matching row exactly - * once. A sort key the caller supplied usually does not identify a row - * (`orderBy: status`), and no backend promises that rows with equal keys keep - * the same relative arrangement between two queries: on MongoDB, `sort` + - * `skip`/`limit` on a non-unique key may return one document twice and never - * return another. Drivers close this by appending a unique tie-breaker column - * of their own to the ORDER BY — which column, and whether the table has one - * at all, is the driver's knowledge, which is why the obligation sits here - * and not in the query engine. + * **Paged reads MUST be deterministic.** Whenever `limit`/`offset` are used + * to walk the result set, the driver MUST produce a total order — reading + * page after page visits every matching row exactly once. The obligation + * covers a paged read whatever its `orderBy`, including none at all, and it + * holds across the whole walk rather than within one response. + * + * Two shapes break it, and they are the same defect at different strengths: + * + * - **A sort key that does not identify a row** (`orderBy: status`). No + * backend promises rows with equal keys keep the same relative arrangement + * between two queries — on MongoDB, `sort` + `skip`/`limit` on a non-unique + * key may return one document twice and never return another. + * - **No `orderBy` at all**, which is the degenerate case of the first: every + * row ties with every other, so the *entire* page boundary is arbitrary. + * SQL leaves row order to the plan, and MongoDB's natural order moves when + * a document does. + * + * Drivers close both by ordering on a unique column of their own — appended + * to a requested `orderBy`, or standing alone when there is none. Which + * column, and whether the table has one at all, is the driver's knowledge, + * which is why the obligation sits here and not in the query engine. A driver + * that cannot name one is not free to reshuffle: it must return rows in an + * order its own storage holds steady between reads (`driver-memory`), or the + * guarantee is simply unavailable on that table and the driver says so. * * The failure this rules out is invisible in any single response: every page * is full, every row is real and belongs, and the duplicate sits several - * screens away from the omission. Checked by the shared - * `PAGINATION_CASES` fixture (`data/pagination-conformance.ts`) — a driver - * ships with those cases running against it. + * screens away from the omission. Checked by the shared `PAGINATION_CASES` + * and `PAGINATION_UNORDERED_CASES` fixtures + * (`data/pagination-conformance.ts`) — a driver ships with those cases + * running against it. * - * Deliberately NOT covered: a paged read with no `orderBy` at all. That is - * non-deterministic on every backend by definition; imposing an order on - * callers who asked for none is a separate decision (objectstack#4363). + * Deliberately NOT covered: an **unpaged** read with no `orderBy`. Nothing is + * being sliced, so no caller can be shown a partial view of the set, and + * imposing an order there would change plan selection for the majority of + * reads to buy nothing (objectstack#4363). */ find(object: string, query: QueryAST, options?: DriverOptions): Promise[]>; @@ -99,6 +114,17 @@ export interface IDataDriver { /** * Find a single record by query. * MUST return `id` as string. MUST NOT return implementation details like `_id`. + * + * The paged-read guarantee on {@link find} does **not** extend here, and the + * distinction is deliberate rather than an oversight. This method promises + * *a* matching record, never a position in a sequence, so there is no + * partition for a second call to be inconsistent with. Engines reach a driver + * with `limit: 1`, which is shaped exactly like page one of a walk — and a + * driver that read it that way would impose an ORDER BY on the single hottest + * read in the system, for which `ORDER BY LIMIT 1` is the classic shape + * that makes a planner abandon the predicate's own index. A driver that wants + * a deterministic single-row read should be handed an `orderBy`, which is a + * thing the caller can express (objectstack#4363). */ findOne(object: string, query: QueryAST, options?: DriverOptions): Promise | null>; diff --git a/packages/spec/src/data/index.ts b/packages/spec/src/data/index.ts index 43c79badb8..08c39bc149 100644 --- a/packages/spec/src/data/index.ts +++ b/packages/spec/src/data/index.ts @@ -8,9 +8,9 @@ export * from './filter.zod'; export * from './filter-logic-conformance'; export * from './temporal-conformance'; // Canonical conformance cases for deterministic paged reads — the standard -// every driver's `find()` is held to when `orderBy` and `limit`/`offset` arrive -// together, so a sort key that does not identify a row cannot let pages overlap -// or skip (#3106). +// every driver's `find()` is held to whenever `limit`/`offset` slice the result +// set, so neither a sort key that fails to identify a row (#3106) nor the +// absence of one altogether (#4363) can let pages overlap or skip. export * from './pagination-conformance'; export * from './date-macros.zod'; export * from './calendar-day'; diff --git a/packages/spec/src/data/pagination-conformance.ts b/packages/spec/src/data/pagination-conformance.ts index e47d9c7a1a..5b1b464a86 100644 --- a/packages/spec/src/data/pagination-conformance.ts +++ b/packages/spec/src/data/pagination-conformance.ts @@ -2,14 +2,14 @@ /** * Canonical conformance cases for **deterministic paged reads** — the single - * standard every driver's `find()` is held to when `orderBy` and `limit`/ - * `offset` arrive together. + * standard every driver's `find()` is held to whenever `limit`/`offset` slice + * the result set, with or without an `orderBy`. * * # The property * - * Reading a collection page by page under one `orderBy` must visit every - * matching row **exactly once**. Nothing here is about *which* order the rows - * come back in — that is the caller's `orderBy`. It is about the order being + * Reading a collection page by page must visit every matching row **exactly + * once**. Nothing here is about *which* order the rows come back in — where the + * caller asked for one, that is their `orderBy`. It is about the order being * the *same* order on page 2 as it was on page 1. * * # Why it is not free @@ -23,32 +23,44 @@ * page is individually correct; the sequence of pages is not a partition of the * collection. * + * Dropping the `ORDER BY` does not escape that — it is the same defect at full + * strength, because now *every* row ties with every other. SQL leaves the row + * order of an unordered read to the plan (insertion order in practice on a + * small table, and not once it goes parallel, switches to an index scan, or is + * `VACUUM`ed), and MongoDB's natural order moves whenever a document does. This + * matters more than the sorted case rather than less: a list view with no `sort` + * configured, which nobody has clicked a column header on, is the single most + * common paged read there is. + * * What that costs the user is worth being precise about, because the failure is * invisible from any single response: every page is full, every row is real, * every row belongs. A record simply never appears, and a different one appears * twice — several screens apart, where nobody is comparing. * - * The fix is one clause: append a column that *is* unique to the ORDER BY, so - * ties can no longer reorder. Drivers do this themselves (they are the ones who - * know their key column and whether the table has one), which is exactly why - * the standard has to live somewhere they can all be checked against. + * The fix is one clause: order by a column that *is* unique — appended to the + * caller's sort keys, or standing alone when they gave none. Drivers do this + * themselves (they are the ones who know their key column and whether the table + * has one), which is exactly why the standard has to live somewhere they can + * all be checked against. * * # What belongs here * * Only the shape of the read: rows whose sort keys repeat heavily, and the * `orderBy` + page size to walk them with. Storage form, id generation and the - * identity of the tie-breaking column are per-driver by design and asserted in - * each driver's own suite. + * identity of the ordering column are per-driver by design and asserted in each + * driver's own suite — as is *how* a driver keeps the promise. A backend whose + * own read order is already steady between reads (`driver-memory`) satisfies + * these cases without emitting a sort at all; the cases test the property, not + * the clause. * * # Scope * * The guarantee is on `find()` (and whatever a driver builds on it, e.g. a - * `findStream` that delegates). It says nothing about a query with NO `orderBy` - * at all — paging an unordered read is non-deterministic on every backend by - * definition, and forcing an order onto callers who asked for none is a much - * larger change to plan selection than this one. That gap is tracked separately - * (objectstack#4363); an unordered paged read is not covered by these cases and - * a driver is not failing them by reshuffling one. + * `findStream` that delegates), and only where the read is **paged**. It says + * nothing about an unpaged read with no `orderBy`: nothing is being sliced, so + * no caller can be shown a partial view of the set, and imposing an order there + * would change plan selection across the majority of reads to buy nothing. + * Drivers are expected to leave that shape exactly as it was (objectstack#4363). * * @see IDataDriver.find in `contracts/data-driver.ts` for the normative wording */ @@ -120,5 +132,39 @@ export const PAGINATION_CASES: readonly PaginationConformanceCase[] = [ }, ]; +/** + * One paged read with **no `orderBy` at all** to walk end to end. + * + * Deliberately a separate type from {@link PaginationConformanceCase} rather + * than the same one with an empty `orderBy`: the shape under test is a query + * that carries no sort *key* — `{ limit, offset }` and nothing else — and + * `orderBy: []` is a different value a driver may well treat differently. + * Keeping them apart means a suite cannot accidentally cover this case by + * spreading an empty array into the sorted one. + */ +export interface UnorderedPaginationConformanceCase { + /** Case label, used as the test name. */ + name: string; + /** Page size to walk the twelve rows with. */ + pageSize: number; +} + +/** + * The unordered cases. Same twelve rows, same walk, no `orderBy` — the shape a + * list view issues when its metadata configures no `sort` and the user has not + * clicked a column header, which is the majority of list reads. + * + * Two of the page sizes do not divide 12, so the walk ends on a short page and + * its last `offset` lands past the final full page — the boundary an + * off-by-one reads wrong. The third is 1, which puts a boundary between every + * adjacent pair of rows, so nowhere is left for an unstable arrangement to hide + * inside a page. + */ +export const PAGINATION_UNORDERED_CASES: readonly UnorderedPaginationConformanceCase[] = [ + { name: 'page size 5 over 12 rows', pageSize: 5 }, + { name: 'page size 7 leaves a short last page', pageSize: 7 }, + { name: 'page size 1 walks every boundary', pageSize: 1 }, +]; + /** 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);