From 13aaabefb73bbb96db0ce3077acfb8eb9c4c78aa Mon Sep 17 00:00:00 2001 From: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Date: Fri, 31 Jul 2026 17:07:16 +0800 Subject: [PATCH] fix(data): paging a sorted read is a partition of the result set, not five queries that share a WHERE clause (objectui#3106) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ORDER BY status LIMIT 50 OFFSET 50` names a sort key that does not identify a row, and no backend promises that rows with equal keys keep the same relative arrangement between two queries. MongoDB documents this outright: `sort` + `skip`/`limit` on a non-unique key may return the same document more than once. Page 2 then repeats a row page 1 already showed and skips one nobody ever sees — with every page full, every row real, and the two halves of the symptom several screens apart. SqlDriver and MongoDBDriver append a unique tie-breaker to any non-empty `orderBy`, in the last requested key's direction: determinism holds either way, but a same-direction suffix is the one an index can still walk in a single pass. SqlDriver applies it only to objects it created itself (`initObjects` records those in `managedObjectFields`). A federated table (ADR-0015) may carry no `id` column, and guessing there would be worse than doing nothing — the resulting unknown-column error is answered by #3821's recovery ladder retrying with NO ORDER BY at all, trading a reshuffle among ties for the loss of the caller's whole sort. driver-memory needed no change: `Array#sort` is stable and the backing table's order does not move between reads. It gets a suite anyway, because that guarantee is implicit and is exactly what a refactor that looks like a speed-up (a hand-rolled sort, or sorting the array in place) would silently remove. The obligation is normative on `IDataDriver.find` and the cases are shared (`PAGINATION_CASES` in `@objectstack/spec/data`), so a future driver is held to it by a gate rather than by remembering. A paged read with NO `orderBy` is deliberately out of scope and filed as #4363. Co-Authored-By: Claude Fable 5 --- .changeset/paged-read-determinism.md | 45 +++++ .../src/memory-pagination-conformance.test.ts | 70 ++++++++ .../driver-mongodb/src/mongodb-driver.ts | 58 ++++-- .../mongodb-pagination-conformance.test.ts | 107 +++++++++++ .../sql-driver-pagination-conformance.test.ts | 167 ++++++++++++++++++ packages/plugins/driver-sql/src/sql-driver.ts | 45 ++++- packages/spec/api-surface.json | 5 + packages/spec/src/contracts/data-driver.ts | 22 +++ packages/spec/src/data/index.ts | 5 + .../spec/src/data/pagination-conformance.ts | 124 +++++++++++++ 10 files changed, 629 insertions(+), 19 deletions(-) create mode 100644 .changeset/paged-read-determinism.md create mode 100644 packages/plugins/driver-memory/src/memory-pagination-conformance.test.ts create mode 100644 packages/plugins/driver-mongodb/src/mongodb-pagination-conformance.test.ts create mode 100644 packages/plugins/driver-sql/src/sql-driver-pagination-conformance.test.ts create mode 100644 packages/spec/src/data/pagination-conformance.ts diff --git a/.changeset/paged-read-determinism.md b/.changeset/paged-read-determinism.md new file mode 100644 index 0000000000..2752ff1bd4 --- /dev/null +++ b/.changeset/paged-read-determinism.md @@ -0,0 +1,45 @@ +--- +"@objectstack/spec": patch +"@objectstack/driver-sql": patch +"@objectstack/driver-mongodb": patch +--- + +fix(data): paging a sorted read is a partition of the result set, not five queries that share a WHERE clause (objectui#3106) + +`ORDER BY status LIMIT 50 OFFSET 50` names a sort key that does not identify a +row, and no backend promises that rows with equal keys keep the same relative +arrangement between two queries. MongoDB documents this outright — `sort` + +`skip`/`limit` on a non-unique key "may return the same document more than +once". So page 2 could repeat a row page 1 already showed and skip one nobody +ever saw: + +``` +page 1: ORDER BY status LIMIT 5 OFFSET 0 -> [r05 r07 r11 r04 …] +page 2: ORDER BY status LIMIT 5 OFFSET 5 -> [r04 …] r04 again; one row never served +``` + +Every page is full, every row is real and belongs, and the duplicate sits +several screens from the omission — which is why this is found by a user +counting records, never by reading a response. + +`SqlDriver` and `MongoDBDriver` now append a unique tie-breaker to any non-empty +`orderBy`, in the last requested key's direction (determinism holds either way, +but a same-direction suffix is the one an index can still walk in one pass). +`driver-memory` already conformed — `Array#sort` is stable over a table whose +order does not move — and now has a suite saying so, because that property is +implicit and easy to lose in a refactor that looks like a speed-up. + +`SqlDriver` adds it only for objects it created itself (`initObjects` records +those). A federated table (ADR-0015) may have no `id` column, and guessing there +would be worse than doing nothing: the unknown-column error is answered by +#3821's ladder retrying with **no ORDER BY at all**, trading a reshuffle among +ties for the loss of the caller's whole sort. + +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. diff --git a/packages/plugins/driver-memory/src/memory-pagination-conformance.test.ts b/packages/plugins/driver-memory/src/memory-pagination-conformance.test.ts new file mode 100644 index 0000000000..3a8dcd912a --- /dev/null +++ b/packages/plugins/driver-memory/src/memory-pagination-conformance.test.ts @@ -0,0 +1,70 @@ +// 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`. + * + * 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. + * + * 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. + */ + +import { describe, it, expect, beforeEach } from 'vitest'; +import { PAGINATION_ALL_IDS, PAGINATION_CASES, PAGINATION_ROWS } from '@objectstack/spec/data'; +import { InMemoryDriver } from './memory-driver.js'; + +describe('InMemoryDriver — paged reads are a partition of the result set (objectui#3106)', () => { + let driver: InMemoryDriver; + + beforeEach(async () => { + driver = new InMemoryDriver({ persistence: false }); + await driver.connect(); + for (const row of PAGINATION_ROWS) { + await driver.create('ticket', { ...row }); + } + }); + + for (const testCase of PAGINATION_CASES) { + it(`visits every row exactly once — ${testCase.name}`, async () => { + const seen: string[] = []; + for (let offset = 0; offset < PAGINATION_ROWS.length; offset += testCase.pageSize) { + const page = await driver.find('ticket', { + orderBy: [...testCase.orderBy], + 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 — ${testCase.name}`, async () => { + const paged: any[] = []; + for (let offset = 0; offset < PAGINATION_ROWS.length; offset += testCase.pageSize) { + const page = await driver.find('ticket', { + orderBy: [...testCase.orderBy], + limit: testCase.pageSize, + offset, + } as any); + paged.push(...page); + } + + const whole = await driver.find('ticket', { orderBy: [...testCase.orderBy] } as any); + expect(paged.map((r) => r.id)).toEqual(whole.map((r: any) => r.id)); + }); + } +}); diff --git a/packages/plugins/driver-mongodb/src/mongodb-driver.ts b/packages/plugins/driver-mongodb/src/mongodb-driver.ts index 7921572658..0a8dba45af 100644 --- a/packages/plugins/driver-mongodb/src/mongodb-driver.ts +++ b/packages/plugins/driver-mongodb/src/mongodb-driver.ts @@ -238,15 +238,8 @@ export class MongoDBDriver implements IDataDriver { } // Sorting - if (query.orderBy && Array.isArray(query.orderBy)) { - const sort: Document = {}; - for (const item of query.orderBy) { - if (item.field) { - sort[this.mapFieldName(item.field)] = item.order === 'desc' ? -1 : 1; - } - } - findOptions.sort = sort; - } + const sort = this.buildSortSpec(query.orderBy); + if (sort) findOptions.sort = sort; // Pagination if (query.offset !== undefined) findOptions.skip = query.offset; @@ -284,15 +277,8 @@ export class MongoDBDriver implements IDataDriver { projection: { _id: 0 }, }; - if (query.orderBy && Array.isArray(query.orderBy)) { - const sort: Document = {}; - for (const item of query.orderBy) { - if (item.field) { - sort[this.mapFieldName(item.field)] = item.order === 'desc' ? -1 : 1; - } - } - findOptions.sort = sort; - } + const sort = this.buildSortSpec(query.orderBy); + if (sort) findOptions.sort = sort; if (query.offset !== undefined) findOptions.skip = query.offset; if (query.limit !== undefined) findOptions.limit = query.limit; @@ -611,6 +597,42 @@ export class MongoDBDriver implements IDataDriver { return field; } + /** + * The `sort` spec for a `find`, with a unique tie-breaker appended so that + * paging is a partition of the result set rather than a series of unrelated + * queries (objectui#3106, contract on `IDataDriver.find`). + * + * MongoDB is explicit that this is not free: `sort` on a non-unique key + * combined with `skip`/`limit` "may return the same document more than once" + * because equal keys have no defined relative order and nothing holds that + * order steady between two executions. Page 2 repeats a row from page 1 and + * silently drops another — with every page full, every row real, and the two + * halves of the symptom too far apart for anyone to notice. + * + * `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. + * + * Returns `undefined` when nothing was requested — an unordered read stays + * unordered (see the contract's explicit carve-out). + */ + private buildSortSpec(orderBy: QueryAST['orderBy']): Document | undefined { + if (!orderBy || !Array.isArray(orderBy)) return 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 (Object.keys(sort).length === 0) return undefined; + if (sort.id === undefined) sort.id = lastDirection; + return sort; + } + // ── Temporal storage form (#4047) ───────────────────────────────────────── /** diff --git a/packages/plugins/driver-mongodb/src/mongodb-pagination-conformance.test.ts b/packages/plugins/driver-mongodb/src/mongodb-pagination-conformance.test.ts new file mode 100644 index 0000000000..8827ecd293 --- /dev/null +++ b/packages/plugins/driver-mongodb/src/mongodb-pagination-conformance.test.ts @@ -0,0 +1,107 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Deterministic paged reads for the MongoDB driver (objectui#3106) — the + * contract on `IDataDriver.find`, checked against the shared cases in + * `@objectstack/spec/data` against a real MongoDB via `mongodb-memory-server`. + * + * This is the backend where the defect is not theoretical. MongoDB documents + * that `sort` on a non-unique key combined with `skip`/`limit` may return the + * same document more than once — equal keys have no defined relative order, and + * nothing holds one execution's arrangement steady for the next. So the driver + * appends `id` to every requested sort, and these cases walk the page + * boundaries that would otherwise be where a row is served twice while another + * is never served at all. + * + * 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. + */ + +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 { MongoDBDriver } from './mongodb-driver.js'; + +let sharedMongod: MongoMemoryServer | undefined; +try { + sharedMongod = await MongoMemoryServer.create({ instance: { launchTimeout: 60_000 } }); +} catch (err) { + console.warn( + '[driver-mongodb] Skipping pagination conformance — mongodb-memory-server could not start: ' + + `${(err as Error)?.message ?? String(err)}`, + ); +} + +describe.skipIf(!sharedMongod)('driver-mongodb — paged reads are a partition of the result set', () => { + const mongod = sharedMongod as MongoMemoryServer; + let driver: MongoDBDriver; + + beforeAll(async () => { + driver = new MongoDBDriver({ url: mongod.getUri(), database: 'pagination_conformance' }); + await driver.connect(); + for (const row of PAGINATION_ROWS) { + await driver.create('ticket', { ...row }); + } + }, 90_000); + + afterAll(async () => { + if (driver) await driver.disconnect(); + if (sharedMongod) await sharedMongod.stop(); + }); + + for (const testCase of PAGINATION_CASES) { + it(`visits every row exactly once — ${testCase.name}`, async () => { + const seen: string[] = []; + for (let offset = 0; offset < PAGINATION_ROWS.length; offset += testCase.pageSize) { + const page = await driver.find('ticket', { + orderBy: [...testCase.orderBy], + 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()); + }); + + it(`page boundaries are invisible — ${testCase.name}`, async () => { + const paged: any[] = []; + for (let offset = 0; offset < PAGINATION_ROWS.length; offset += testCase.pageSize) { + const page = await driver.find('ticket', { + orderBy: [...testCase.orderBy], + limit: testCase.pageSize, + offset, + } as any); + paged.push(...page); + } + + const whole = await driver.find('ticket', { orderBy: [...testCase.orderBy] } as any); + expect(paged.map((r) => r.id)).toEqual((whole as any[]).map((r) => r.id)); + }); + } + + 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(); + }); + + it('appends `id` in the LAST key\'s direction', () => { + expect(driver['buildSortSpec']([{ field: 'status', order: 'asc' }])).toEqual({ + status: 1, + id: 1, + }); + expect( + driver['buildSortSpec']([ + { 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 }); + }); +}); 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 new file mode 100644 index 0000000000..4f8d3aab06 --- /dev/null +++ b/packages/plugins/driver-sql/src/sql-driver-pagination-conformance.test.ts @@ -0,0 +1,167 @@ +// 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 + * `driver-mongodb` answer one standard. + * + * Two halves, because either alone would be reassuring and wrong: + * + * 1. **The property** — walk each case page by page and assert the pages + * 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. + * + * 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. + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { PAGINATION_ALL_IDS, PAGINATION_CASES, PAGINATION_ROWS } from '@objectstack/spec/data'; +import { SqlDriver } from '../src/index.js'; + +/** Reach the compiled statement without executing it. */ +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; + } +} + +describe('SqlDriver — paged reads are a partition of the result set (objectui#3106)', () => { + let driver: SqlDriver; + + beforeEach(async () => { + driver = new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: ':memory:' }, + useNullAsDefault: true, + }); + + await driver.initObjects([ + { + name: 'ticket', + fields: { + status: { type: 'string' }, + rank: { type: 'integer' }, + name: { type: 'string' }, + }, + }, + ]); + + for (const row of PAGINATION_ROWS) { + await driver.create('ticket', { ...row }, { bypassTenantAudit: true }); + } + }); + + afterEach(async () => { + await driver.disconnect(); + }); + + for (const testCase of PAGINATION_CASES) { + it(`visits every row exactly once — ${testCase.name}`, async () => { + const seen: string[] = []; + for (let offset = 0; offset < PAGINATION_ROWS.length; offset += testCase.pageSize) { + const page = await driver.find( + 'ticket', + { orderBy: [...testCase.orderBy], 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(`orders pages consistently with the requested sort — ${testCase.name}`, async () => { + const paged: Array> = []; + for (let offset = 0; offset < PAGINATION_ROWS.length; offset += testCase.pageSize) { + const page = await driver.find( + 'ticket', + { orderBy: [...testCase.orderBy], limit: testCase.pageSize, offset } as any, + { bypassTenantAudit: true } as any, + ); + paged.push(...page); + } + + // Concatenating the pages must reproduce the same order an unpaged read + // of the whole set gives — that the page boundaries are invisible is the + // user-facing half of the guarantee. + const whole = await driver.find( + 'ticket', + { orderBy: [...testCase.orderBy] } as any, + { bypassTenantAudit: true } as any, + ); + expect(paged.map((r) => r.id)).toEqual(whole.map((r) => r.id)); + }); + } + + it('leaves an unordered read alone — no sort is imposed on a caller who asked for none', async () => { + 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', () => { + let driver: InspectableSqlDriver; + + beforeEach(async () => { + driver = new InspectableSqlDriver({ + client: 'better-sqlite3', + connection: { filename: ':memory:' }, + useNullAsDefault: true, + }); + await driver.initObjects([ + { name: 'ticket', fields: { status: { type: 'string' }, rank: { type: 'integer' } } }, + ]); + }); + + afterEach(async () => { + await driver.disconnect(); + }); + + it('appends `id` after a non-unique sort key', () => { + const sql = driver.orderByClauseFor('ticket', [{ 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' }, + ]); + 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' }]); + expect(sql.match(/`id`/g) ?? []).toHaveLength(1); + }); + + 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. + expect(driver['paginationTieBreaker']('some_remote_table')).toBeNull(); + }); +}); diff --git a/packages/plugins/driver-sql/src/sql-driver.ts b/packages/plugins/driver-sql/src/sql-driver.ts index f2215ff716..a0c13c9287 100644 --- a/packages/plugins/driver-sql/src/sql-driver.ts +++ b/packages/plugins/driver-sql/src/sql-driver.ts @@ -1333,11 +1333,22 @@ 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) { - b.orderBy(this.remoteColumn(object, item.field, this.mapSortField(item.field)), item.order || 'asc'); + 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, + ); + } } // PAGINATION @@ -5440,6 +5451,38 @@ export class SqlDriver implements IDataDriver { return field; } + /** + * The unique column appended to a non-empty ORDER 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`). + * + * `ORDER BY status LIMIT 50 OFFSET 50` names a key that does not identify a + * row, and SQL promises nothing about how equal keys are arranged — nor that + * two executions arrange them the same way. Page 2 can then repeat a row + * page 1 already showed and skip one nobody ever sees. Every page looks + * 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 + * 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 + * than doing nothing: an `id` column that isn't there raises an unknown-column + * error, and the #3821 recovery ladder answers that by retrying with **no + * ORDER BY at all** — trading a reshuffle among ties for the loss of the + * caller's whole sort. + * + * Returns the LOGICAL field name; the call site maps it to a physical column + * like any other sort key. + */ + protected paginationTieBreaker(object: string): string | null { + const tableName = StorageNameMapping.resolveTableName({ name: object } as any); + const managed = + this.managedObjectFields.has(tableName) || this.managedObjectFields.has(object); + return managed ? 'id' : null; + } + /** * 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 aff1d07f2e..fd05e11ca2 100644 --- a/packages/spec/api-surface.json +++ b/packages/spec/api-surface.json @@ -464,6 +464,11 @@ "ObjectTitleCompleteness (interface)", "OperationCheckOptions (interface)", "OperatorKey (type)", + "PAGINATION_ALL_IDS (const)", + "PAGINATION_CASES (const)", + "PAGINATION_ROWS (const)", + "PaginationConformanceCase (interface)", + "PaginationConformanceRow (interface)", "PerOperationRequiredPermissions (type)", "PerOperationRequiredPermissionsSchema (const)", "PoolConfig (type)", diff --git a/packages/spec/src/contracts/data-driver.ts b/packages/spec/src/contracts/data-driver.ts index fd7a6771fe..6a1022b48b 100644 --- a/packages/spec/src/contracts/data-driver.ts +++ b/packages/spec/src/contracts/data-driver.ts @@ -64,6 +64,28 @@ 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. + * + * 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. + * + * 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). */ find(object: string, query: QueryAST, options?: DriverOptions): Promise[]>; diff --git a/packages/spec/src/data/index.ts b/packages/spec/src/data/index.ts index 3a86910f82..43c79badb8 100644 --- a/packages/spec/src/data/index.ts +++ b/packages/spec/src/data/index.ts @@ -7,6 +7,11 @@ export * from './filter.zod'; // against, so they cannot drift apart again (#3774). 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). +export * from './pagination-conformance'; export * from './date-macros.zod'; export * from './calendar-day'; // Session-scoped filter placeholders ({current_user_id} / {current_org_id}) — diff --git a/packages/spec/src/data/pagination-conformance.ts b/packages/spec/src/data/pagination-conformance.ts new file mode 100644 index 0000000000..e47d9c7a1a --- /dev/null +++ b/packages/spec/src/data/pagination-conformance.ts @@ -0,0 +1,124 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Canonical conformance cases for **deterministic paged reads** — the single + * standard every driver's `find()` is held to when `orderBy` and `limit`/ + * `offset` arrive together. + * + * # 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 + * the *same* order on page 2 as it was on page 1. + * + * # Why it is not free + * + * `ORDER BY status LIMIT 50 OFFSET 50` names a sort key that does not identify + * a row. Neither SQL nor MongoDB promises anything about how rows with equal + * keys are arranged relative to each other, and neither promises the *same* + * arrangement across two separate queries — the plan is free to differ, and on + * MongoDB the documented behavior is that `sort` + `skip`/`limit` on a + * non-unique key may return one document twice and another not at all. Each + * page is individually correct; the sequence of pages is not a partition of the + * collection. + * + * 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. + * + * # 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. + * + * # 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. + * + * @see IDataDriver.find in `contracts/data-driver.ts` for the normative wording + */ + +/** + * A row of the shared fixture. `status` and `rank` both repeat heavily — four + * rows per `status`, three per `rank` — so the tie population is large enough + * that an unstable arrangement cannot hide inside a single page. + * + * `id` is deliberately NOT in insertion order: a backend that returns ties in + * physical/insertion order is then visibly not returning them in id order, so a + * driver test that wants to assert the tie-breaker's *direction* has something + * to distinguish it from "did nothing". + */ +export interface PaginationConformanceRow { + id: string; + status: string; + rank: number; + name: string; +} + +/** + * Twelve rows, seeded in the order listed. Sort keys repeat; `id` is unique and + * shuffled relative to insertion order. + */ +export const PAGINATION_ROWS: readonly PaginationConformanceRow[] = [ + { id: 'r07', status: 'open', rank: 2, name: 'Alpha' }, + { id: 'r03', status: 'done', rank: 1, name: 'Bravo' }, + { id: 'r11', status: 'open', rank: 3, name: 'Charlie' }, + { id: 'r01', status: 'hold', rank: 2, name: 'Delta' }, + { id: 'r09', status: 'done', rank: 3, name: 'Echo' }, + { id: 'r05', status: 'open', rank: 1, name: 'Foxtrot' }, + { id: 'r12', status: 'hold', rank: 1, name: 'Golf' }, + { id: 'r02', status: 'done', rank: 2, name: 'Hotel' }, + { id: 'r08', status: 'hold', rank: 3, name: 'India' }, + { id: 'r04', status: 'open', rank: 2, name: 'Juliett' }, + { id: 'r10', status: 'done', rank: 1, name: 'Kilo' }, + { id: 'r06', status: 'hold', rank: 3, name: 'Lima' }, +]; + +/** One paged read to walk end to end. */ +export interface PaginationConformanceCase { + /** Case label, used as the test name. */ + name: string; + /** The `orderBy` the caller asked for — never unique on its own. */ + orderBy: ReadonlyArray<{ field: string; order: 'asc' | 'desc' }>; + /** Page size; every case's total (12) is deliberately not a multiple of it. */ + pageSize: number; +} + +/** + * The cases. Every one sorts by a key that repeats, so every one is a paged + * read whose page boundaries fall *inside* a group of equal keys — which is the + * only place the defect can appear. + * + * The multi-key case matters on its own: two sort keys narrow the ties but do + * not eliminate them (`status` × `rank` still leaves pairs), and a driver that + * appends a tie-breaker only for single-key sorts passes everything else here. + */ +export const PAGINATION_CASES: readonly PaginationConformanceCase[] = [ + { name: 'single ascending key with 4-row ties', orderBy: [{ field: 'status', order: 'asc' }], pageSize: 5 }, + { name: 'single descending key with 4-row ties', orderBy: [{ field: 'status', order: 'desc' }], pageSize: 5 }, + { name: 'numeric key with 4-row ties', orderBy: [{ field: 'rank', order: 'asc' }], pageSize: 4 }, + { name: 'page size 1 walks every boundary', orderBy: [{ field: 'status', order: 'asc' }], pageSize: 1 }, + { + name: 'multi-key sort with residual ties', + orderBy: [{ field: 'status', order: 'asc' }, { field: 'rank', order: 'desc' }], + pageSize: 5, + }, +]; + +/** 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);