diff --git a/.changeset/turso-remote-pagination-tiebreaker.md b/.changeset/turso-remote-pagination-tiebreaker.md new file mode 100644 index 0000000000..5a2da0c981 --- /dev/null +++ b/.changeset/turso-remote-pagination-tiebreaker.md @@ -0,0 +1,32 @@ +--- +'@objectstack/driver-turso': patch +--- + +fix(driver-turso): remote 分页读补齐确定性排序,与 local 面共用同一条规则 + +`TursoDriver` 在 remote 传输(`libsql://` / `https://` 等 URL)下的分页读不满足 +`IDataDriver.find` 的确定性分页 MUST:`RemoteTransport.buildSelectSQL` 把调用方的 +`orderBy` 原样拼进 SQL 后直接接 `LIMIT` / `OFFSET`,不追加任何唯一列,无序分页读 +更是完全不排序。SQLite 不承诺并列行在两条语句之间排布一致,所以表一大、计划一变, +`ORDER BY status LIMIT 50 OFFSET 50` 翻页时就会有记录出现两次、另一条永远不出现 —— +每一页都是满的、每一行都合法,从任何单个响应里都看不出来。 + +同一个驱动的 local 面早已按 #4363 办事,于是一个驱动的两条传输对同一个分页查询给出 +不同的排序保证,而传输模式只由 URL 决定。 + +修法是**复用**而不是复制:`TursoDriver.find` / `findOne` 现在通过继承来的 +`SqlDriver.orderKeysFor()` 解析出完整排序键再交给传输层,三态规则只有一份实现 —— + +| `orderBy` | 分页 | 结果 | +|---|---|---| +| 非空 | 任意 | 调用方的键 + `id` | +| 空 | 有 `limit`/`offset` | 单独 `id` | +| 空 | 都没有 | 不加 ORDER BY(#4363 carve-out,原样保留) | + +`findOne` 的语义一并保住:它的 `limit: 1` 由传输层自己注入,若在 `buildSelectSQL` +里判定就会被误读成「页大小为 1 的第一页」,从而给系统里最热的读加上 +`ORDER BY id LIMIT 1` —— 正是让计划器放弃谓词自身索引的形状。 + +唯一列的判定沿用 local 面同样保守的前提:只有本驱动自己建的表才追加 `id` +(`RemoteTransport` 建表时无条件写入 `"id" TEXT PRIMARY KEY`);不是自己建的表保持 +原样并告警一次,绝不凭空发明排序列。 diff --git a/packages/drivers/driver-turso/src/remote-pagination-tiebreaker.test.ts b/packages/drivers/driver-turso/src/remote-pagination-tiebreaker.test.ts new file mode 100644 index 0000000000..7d3f82c4a7 --- /dev/null +++ b/packages/drivers/driver-turso/src/remote-pagination-tiebreaker.test.ts @@ -0,0 +1,201 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * The ORDER BY the REMOTE transport actually sends (#5653) — the clause-level + * companion to `turso-remote-pagination-conformance.test.ts`. + * + * That suite asks the contract's question ("is a page walk a partition of the + * result set?") on rows, which is the only instrument that can catch a *lost* + * clause. This one asks the question the fix turns on, which rows cannot + * distinguish on a twelve-row in-memory table: **which clause went out**. Both + * are needed, and neither substitutes for the other — a plan that happens to + * return insertion order satisfies the row assertions while the statement + * carries no tie-breaker at all, which is exactly how the gap #5653 names + * survived under a green suite. + * + * What is pinned here is the three-state table `SqlDriver.orderKeysFor` + * implements, observed through the SQL the remote transport emits: + * + * | `orderBy` | paged | ORDER BY sent | + * |---|---|---| + * | non-empty | either | caller's keys, then `"id"` | + * | empty | `limit`/`offset` present | `"id"` alone | + * | empty | neither | **none** — objectstack#4363's carve-out | + * + * plus the two conditions that keep it honest: a `findOne` (whose `limit: 1` + * the transport injects *itself*) must NOT be read as a page, and a table this + * driver never created gets no invented `id` column. + */ + +import { describe, it, expect, vi } from 'vitest'; +import { TursoDriver } from './turso-driver.js'; + +const TICKET_OBJECT = { + name: 'ticket', + fields: { + status: { type: 'string' }, + rank: { type: 'integer' }, + }, +}; + +/** + * A remote driver over a client that records every statement and answers with + * no rows. Rows are irrelevant here — the statement is the measurement. + */ +async function makeRecordingRemoteDriver(options?: { sync?: boolean }) { + const statements: Array<{ sql: string; args: unknown[] }> = []; + const record = (stmt: any) => { + statements.push({ sql: stmt?.sql ?? String(stmt), args: stmt?.args ?? [] }); + return { rows: [], columns: [], rowsAffected: 0 }; + }; + const client = { + execute: vi.fn(async (stmt: any) => record(stmt)), + batch: vi.fn(async (stmts: any[]) => stmts.map(record)), + close: vi.fn(), + }; + const driver = new TursoDriver({ + url: 'libsql://tiebreaker.turso.io', + client: client as never, + }); + await driver.connect(); + expect(driver.transportMode).toBe('remote'); + if (options?.sync !== false) { + await driver.syncSchema(TICKET_OBJECT.name, TICKET_OBJECT); + } + statements.length = 0; // drop the DDL round-trip; only reads are under test + return { driver, statements }; +} + +/** The ORDER BY clause of the last statement sent, or `null` if it had none. */ +const lastOrderBy = (statements: Array<{ sql: string }>): string | null => { + const sql = statements[statements.length - 1]?.sql ?? ''; + const match = /ORDER BY (.*?)(?: LIMIT | OFFSET |$)/.exec(sql); + return match ? match[1] : null; +}; + +describe('TursoDriver remote — the ORDER BY a paged read goes out with (#5653)', () => { + it('appends the id tie-breaker after the caller sort keys on a paged read', async () => { + const { driver, statements } = await makeRecordingRemoteDriver(); + await driver.find('ticket', { + orderBy: [{ field: 'status', order: 'asc' }], + limit: 5, + offset: 5, + }); + expect(lastOrderBy(statements)).toBe('"status" ASC, "id" ASC'); + expect(statements[statements.length - 1].sql).toContain('LIMIT ? OFFSET ?'); + await driver.disconnect(); + }); + + it('orders a paged read with NO orderBy by id alone', async () => { + const { driver, statements } = await makeRecordingRemoteDriver(); + await driver.find('ticket', { limit: 5, offset: 5 }); + expect(lastOrderBy(statements)).toBe('"id" ASC'); + await driver.disconnect(); + }); + + it('leaves an UNPAGED unordered read with no ORDER BY at all (#4363 carve-out)', async () => { + // The carve-out is half the fix, not a leftover: an unpaged read hands back + // the whole matching set, so there is no partial view to be wrong about, + // and an imposed sort would only change plan selection for the majority of + // reads in the system. + const { driver, statements } = await makeRecordingRemoteDriver(); + await driver.find('ticket', {}); + expect(statements[statements.length - 1].sql).not.toContain('ORDER BY'); + expect(lastOrderBy(statements)).toBeNull(); + await driver.disconnect(); + }); + + it('still appends the tie-breaker to an UNPAGED sorted read', async () => { + // Row one of the table: a caller who named a non-unique key gets a total + // order whether or not this particular statement is sliced — which is what + // makes the paged walk and the whole-set read agree. + const { driver, statements } = await makeRecordingRemoteDriver(); + await driver.find('ticket', { orderBy: [{ field: 'status', order: 'asc' }] }); + expect(lastOrderBy(statements)).toBe('"status" ASC, "id" ASC'); + await driver.disconnect(); + }); + + it('counts `limit` alone as paged — page one is not exempt', async () => { + // A page one that disagrees with the ordering pages two onward use leaves + // the defect fully intact while looking like a fix. + const { driver, statements } = await makeRecordingRemoteDriver(); + await driver.find('ticket', { limit: 50 }); + expect(lastOrderBy(statements)).toBe('"id" ASC'); + await driver.disconnect(); + }); + + it('follows the last requested key direction, so a compound index still walks in one pass', async () => { + const { driver, statements } = await makeRecordingRemoteDriver(); + await driver.find('ticket', { + orderBy: [{ field: 'status', order: 'asc' }, { field: 'rank', order: 'desc' }], + limit: 5, + }); + expect(lastOrderBy(statements)).toBe('"status" ASC, "rank" DESC, "id" DESC'); + await driver.disconnect(); + }); + + it('does not repeat a key the caller already sorted by', async () => { + const { driver, statements } = await makeRecordingRemoteDriver(); + await driver.find('ticket', { orderBy: [{ field: 'id', order: 'desc' }], limit: 5 }); + expect(lastOrderBy(statements)).toBe('"id" DESC'); + await driver.disconnect(); + }); + + it('leaves findOne unsorted despite the `limit: 1` the transport injects itself', async () => { + // `RemoteTransport.findOne` spells an id lookup as `find(..., limit: 1)`. + // Read as a page that would earn `ORDER BY id LIMIT 1` — the shape that + // makes a planner drop the predicate's own index and walk the primary key + // (~100× on the measurement in `SqlDriver.findRows`). findOne promises *a* + // matching record, never a position in a sequence, so there is no partition + // to preserve and nothing to buy with that. + const { driver, statements } = await makeRecordingRemoteDriver(); + await driver.findOne('ticket', { where: { id: 't1' } }); + const sql = statements[statements.length - 1].sql; + expect(sql).toContain('LIMIT ?'); + expect(sql).not.toContain('ORDER BY'); + await driver.disconnect(); + }); + + it('still completes a findOne that asked for its OWN order — singleRowLookup withholds nothing there', async () => { + const { driver, statements } = await makeRecordingRemoteDriver(); + await driver.findOne('ticket', { + where: { status: 'open' }, + orderBy: [{ field: 'rank', order: 'asc' }], + }); + // Row one of the table is "non-empty `orderBy` | either | caller's keys + + // id", and `either` means it — `singleRowLookup` only governs the row below + // it, where the caller named no key at all and the alternative would be to + // invent a whole sort for a lookup that never promised a position. A caller + // who DID name a key asked for a defined order among its ties, so the + // tie-breaker completes it here exactly as it does locally + // (`SqlDriver.findRows` reaches `orderKeysFor` with the same flag and the + // same non-empty key list, and appends the same column). + expect(lastOrderBy(statements)).toBe('"rank" ASC, "id" ASC'); + await driver.disconnect(); + }); + + it('invents no ordering column for a table this driver did not create', async () => { + // The precondition the whole rule rests on: `id` is known to exist because + // `RemoteTransport.buildCreateTableSQL` wrote it. On a table that arrived + // some other way, `ORDER BY id` risks failing the entire statement, so the + // conservative answer — prior behaviour exactly — is the right one. + const { driver, statements } = await makeRecordingRemoteDriver({ sync: false }); + await driver.find('unmanaged_table', { limit: 5, offset: 5 }); + expect(statements[statements.length - 1].sql).not.toContain('ORDER BY'); + await driver.disconnect(); + }); + + it('says so once when a paged unsorted read cannot be made deterministic', async () => { + // A MUST that quietly does not hold is the invisible failure the rule was + // written against, so the unserviceable case is announced rather than left + // to a user counting records. Once per object, not once per query. + const { driver } = await makeRecordingRemoteDriver({ sync: false }); + const warn = vi.spyOn((driver as unknown as { logger: { warn: (m: string) => void } }).logger, 'warn'); + await driver.find('unmanaged_table', { limit: 5 }); + await driver.find('unmanaged_table', { limit: 5, offset: 5 }); + const matching = warn.mock.calls.filter((c) => /NOT deterministic/.test(String(c[0]))); + expect(matching).toHaveLength(1); + warn.mockRestore(); + await driver.disconnect(); + }); +}); diff --git a/packages/drivers/driver-turso/src/remote-transport.ts b/packages/drivers/driver-turso/src/remote-transport.ts index 00f54890a3..28f1f7f9ae 100644 --- a/packages/drivers/driver-turso/src/remote-transport.ts +++ b/packages/drivers/driver-turso/src/remote-transport.ts @@ -941,6 +941,24 @@ export class RemoteTransport { /** * Build a SELECT SQL statement from a QueryAST-like object. + * + * **The ORDER BY below is rendered, not decided.** `query.orderBy` is already + * the COMPLETE sort key list by the time it gets here — caller's keys plus + * whatever the deterministic-paging contract requires + * (`IDataDriver.find` / objectstack#4363) — because `TursoDriver` resolves it + * through the inherited `SqlDriver.orderKeysFor` in `toRemoteReadQuery` + * before handing the query over. Reusing that one method is what stops this + * driver's two transports from giving the same paged query different ordering + * guarantees on nothing but a URL (#5653, ADR-0053 D-A1). + * + * So: do NOT grow a tie-breaker rule of your own in here. Besides being the + * second copy the fix was about, this method cannot see the distinction the + * rule turns on — {@link findOne} spells an id lookup as + * `find(object, { ...query, limit: 1 })`, which by this point is + * indistinguishable from page one of a walk with page size 1, and the two + * want opposite clauses (see `toRemoteReadQuery`). An empty or absent + * `orderBy` is likewise an ANSWER — #4363's carve-out for an unpaged + * unordered read — and emitting no ORDER BY for it is correct. */ private buildSelectSQL(object: string, query: any): { sql: string; args: any[] } { const fields = query.fields && Array.isArray(query.fields) && query.fields.length > 0 diff --git a/packages/drivers/driver-turso/src/turso-driver.ts b/packages/drivers/driver-turso/src/turso-driver.ts index 56487dddeb..e178edba36 100644 --- a/packages/drivers/driver-turso/src/turso-driver.ts +++ b/packages/drivers/driver-turso/src/turso-driver.ts @@ -235,6 +235,20 @@ export class TursoDriver extends SqlDriver { */ private remoteTransport: RemoteTransport | null = null; + /** + * Objects whose physical table THIS driver created through the remote + * transport — the remote-mode answer to the one question + * {@link SqlDriver.paginationTieBreaker} asks. + * + * Local mode records that fact inside `SqlDriver.initObjects`, in + * `managedObjectFields`. Remote DDL never reaches that method (it goes out + * over `@libsql/client` — see {@link initObjects}), so the base map stays + * empty however many tables the transport has created, and the inherited + * rule reading an empty map would answer "not mine" for every object in + * remote mode. Same question, same answer, different place to look it up. + */ + private readonly remoteManagedObjects = new Set(); + constructor(config: TursoDriverConfig) { const mode = TursoDriver.detectMode(config); const knexConfig = TursoDriver.toKnexConfig(config, mode); @@ -483,12 +497,12 @@ export class TursoDriver extends SqlDriver { // =================================== override async find(object: string, query: any, options?: any): Promise { - if (this.isRemote) return this.formatRemoteRows(object, await this.remoteTransport!.find(object, this.toRemoteQuery(object, query))); + if (this.isRemote) return this.formatRemoteRows(object, await this.remoteTransport!.find(object, this.toRemoteReadQuery(object, query))); return super.find(object, query, options); } override async findOne(object: string, query: any, options?: any): Promise { - if (this.isRemote) return this.formatRemoteRow(object, await this.remoteTransport!.findOne(object, this.toRemoteQuery(object, query))); + if (this.isRemote) return this.formatRemoteRow(object, await this.remoteTransport!.findOne(object, this.toRemoteReadQuery(object, query, { singleRowLookup: true }))); return super.findOne(object, query, options); } @@ -709,6 +723,78 @@ export class TursoDriver extends SqlDriver { return { ...query, where: this.toRemoteFilter(object, query.where) }; } + /** + * A READ query as the remote transport should receive it: the caller's + * `where` compiled through {@link toRemoteQuery}, and the complete ORDER BY + * the deterministic-paging contract asks for (`IDataDriver.find`, + * objectstack#4363) already resolved into `orderBy`. + * + * The order comes from the inherited {@link SqlDriver.orderKeysFor} — the + * same method local mode calls, not a second copy of its three-state table — + * so the two transports of this ONE driver cannot answer the same paged + * query with different ordering guarantees when only the URL differs. That + * split is the seam ADR-0053 D-A1 exists to close, and it was open here: + * `RemoteTransport.buildSelectSQL` mapped the caller's `orderBy` verbatim and + * appended no unique column, so `ORDER BY status LIMIT 50 OFFSET 50` served + * its ties in whatever arrangement the plan chose — one row twice, another + * never, several screens apart (#5653). `RemoteTransport` keeps its job: + * assemble SQL for the query it is handed. + * + * Deriving it HERE rather than inside `buildSelectSQL` is also what keeps + * `findOne` correct. The transport spells an id lookup as + * `find(object, { ...query, limit: 1 })`, so by the time the SQL is built a + * `findOne` is indistinguishable from "page one of a walk with page size 1" + * — and the two want opposite things: `ORDER BY id LIMIT 1` is the shape + * that makes a planner abandon the predicate's own index and walk the + * primary key instead (~100× on the measurement recorded in + * `SqlDriver.findRows`), and `findOne` promises *a* matching record, never a + * position in a sequence. Up here the two callers are still distinguishable, + * and `singleRowLookup` is how they say so — exactly as they do locally. + * + * `orderKeysFor` returns `[]` for the third row of its table — an unpaged + * read with no `orderBy` (#4363's deliberate carve-out) — and an empty + * `orderBy` makes `buildSelectSQL` emit no ORDER BY clause at all, i.e. the + * same statement it emitted before this method existed. + */ + private toRemoteReadQuery( + object: string, + query: any, + opts?: { singleRowLookup?: boolean }, + ): any { + if (!query || typeof query !== 'object') return query; + const orderBy = this.orderKeysFor(object, query, opts).map((key) => ({ + field: key.field, + order: key.direction, + })); + return this.toRemoteQuery(object, { ...query, orderBy }); + } + + /** + * The unique column a paged read can be made deterministic with (#4363), + * answered for remote mode. + * + * The RULE is not restated here: {@link SqlDriver.orderKeysFor} still decides + * *when* a tie-breaker is appended and in which direction, and remote reads + * go through it (see {@link toRemoteReadQuery}). What is remote-specific is + * the single FACT that rule needs — did this driver create the table, and + * does it therefore carry an `id` primary key? + * `RemoteTransport.buildCreateTableSQL` opens every table it creates with + * `"id" TEXT PRIMARY KEY`, so for anything this driver synced the answer is + * yes; it is simply recorded in {@link remoteManagedObjects}, because the + * base class's `managedObjectFields` is filled by `SqlDriver.initObjects` + * and remote DDL never calls it. + * + * The base method's conservatism is kept deliberately for everything else: a + * table this driver did not create still gets `null` and no invented ORDER + * BY. An `id` column that is not there fails the whole statement, and + * guessing on a federated table (ADR-0015) would trade a reshuffle among + * ties for the loss of the caller's entire result. + */ + protected override paginationTieBreaker(object: string): string | null { + if (!this.isRemote) return super.paginationTieBreaker(object); + return this.remoteManagedObjects.has(object) ? 'id' : null; + } + /** Apply the inherited read-coercion to a single remote row (in place). */ private formatRemoteRow(object: string, row: T): T { if (row && typeof row === 'object') this.formatOutput(object, row as any); @@ -731,8 +817,15 @@ export class TursoDriver extends SqlDriver { * canonical logic, so the two can never drift. A managed object is its own * physical table, so the default `remoteName === name` mapping is a no-op for * the RemoteTransport SQL (which addresses tables by object name directly). + * + * It also records the object as one whose table this driver created, which is + * the whole input to {@link paginationTieBreaker} in remote mode. That goes + * FIRST and outside the `try`: both callers reach here only after the DDL has + * already succeeded, so the table exists with its `id` primary key whether or + * not the best-effort coercion registration below does. */ private registerRemoteFieldMetadata(obj: { name: string; fields?: Record }): void { + this.remoteManagedObjects.add(obj.name); try { this.registerExternalObject({ name: obj.name, fields: obj.fields, tenancy: (obj as any).tenancy }); } catch { diff --git a/packages/drivers/driver-turso/src/turso-pagination-conformance.test.ts b/packages/drivers/driver-turso/src/turso-pagination-conformance.test.ts index c9e749c206..c34b522b88 100644 --- a/packages/drivers/driver-turso/src/turso-pagination-conformance.test.ts +++ b/packages/drivers/driver-turso/src/turso-pagination-conformance.test.ts @@ -17,9 +17,12 @@ * * The REMOTE transport keeps its own file * (`turso-remote-pagination-conformance.test.ts`): it does not go through knex - * at all and assembles its own ORDER BY / LIMIT / OFFSET, which is a second - * implementation of this contract rather than a second engine under the same - * one. + * at all and assembles its own ORDER BY / LIMIT / OFFSET, so the same contract + * has to be re-measured against a second SQL assembler. Since #5653 it is only + * the assembler that is second — `TursoDriver.toRemoteReadQuery` resolves the + * remote sort keys through this very same `orderKeysFor`, so the rule has one + * implementation and the URL no longer decides which guarantee a paged read + * gets. */ import { describe, it, expect, beforeAll, afterAll } from 'vitest'; diff --git a/packages/drivers/driver-turso/src/turso-remote-pagination-conformance.test.ts b/packages/drivers/driver-turso/src/turso-remote-pagination-conformance.test.ts index dff2c6c9c9..6678ba1e39 100644 --- a/packages/drivers/driver-turso/src/turso-remote-pagination-conformance.test.ts +++ b/packages/drivers/driver-turso/src/turso-remote-pagination-conformance.test.ts @@ -6,37 +6,45 @@ * * The local twin of this suite passes by INHERITANCE: `TursoDriver extends * SqlDriver`, so `orderKeysFor()` appends the `id` tie-breaker to every paged - * read. Remote mode inherits nothing — `RemoteTransport.buildSelectSQL` - * assembles its own ORDER BY / LIMIT / OFFSET — which makes it a second - * implementation of this contract inside ONE driver, selected by URL alone. - * That is exactly the shape #4363 wrote these cases for. + * read. Remote mode does not inherit the SQL — `RemoteTransport.buildSelectSQL` + * assembles its own ORDER BY / LIMIT / OFFSET — so what has to be proved here + * is that it does not also carry a second implementation of the *rule*, chosen + * by nothing but the URL. That is exactly the shape #4363 wrote these cases + * for. * - * ## What this suite measured, stated plainly + * ## What this suite measured before #5653, and what it measures now * - * Both case-sets pass here, and they do NOT pass for the reason the local - * twin's do. `buildSelectSQL` maps the caller's `orderBy` entries verbatim and - * appends no unique column, so: + * When #5590 first wrote this file both case-sets passed, and they did NOT + * pass for the reason the local twin's did. `buildSelectSQL` mapped the + * caller's `orderBy` entries verbatim and appended no unique column, so: * - * - a sorted paged read goes out as `ORDER BY status LIMIT ? OFFSET ?`, and - * the ties come back in storage order rather than id order; - * - an unsorted paged read goes out with no ORDER BY at all. + * - a sorted paged read went out as `ORDER BY status LIMIT ? OFFSET ?`, and + * the ties came back in storage order rather than id order; + * - an unsorted paged read went out with no ORDER BY at all. * - * The property holds on this fixture because the stub is `better-sqlite3` over + * The property held on this fixture because the stub is `better-sqlite3` over * a twelve-row in-memory table: one plan, one arrangement, every time. On a * real endpoint the arrangement of equal keys across two statements is not * promised — the case-set's own module doc says so, and names the unsorted * read as the same defect at full strength rather than as an exemption. The * `driver-memory` carve-out ("storage order steady between reads") is about a - * JS array, not a SQL plan. + * JS array, not a SQL plan. So a green run then read as: **the transport + * satisfied the cases without implementing the mechanism the contract asks + * for** — and the two `records the measured mechanism` tests below pinned that + * gap rather than let it sit undocumented under a green suite. * - * So the honest reading of a green run here is: **the transport currently - * satisfies the cases without implementing the mechanism the contract asks - * for.** That gap is filed as #5653, and the two `records the measured - * mechanism` tests below pin it — they assert the tie arrangement IS storage - * order, so the day #5653 lands they go red and get updated with it, instead - * of the divergence sitting here undocumented under a green suite. Fixing the - * transport is deliberately not this file's job (#5590's boundary: write the - * suite, do not grade your own paper). + * #5653 closed it, and those two pins were flipped with it — they now assert + * the concrete sequences the mechanism produces (ties in id order; an unsorted + * page walk in id order), each against the storage-order arrangement it + * replaced, so restoring the old behaviour turns them red rather than leaving + * them passing for a vacuous reason. The fix is NOT a second rule inside + * `buildSelectSQL`: `TursoDriver.toRemoteReadQuery` resolves the whole ORDER BY + * through the same inherited `SqlDriver.orderKeysFor` local mode calls, and the + * transport renders what it is handed. The clause that comes out of that is + * pinned separately, statement by statement, in + * `remote-pagination-tiebreaker.test.ts` — rows on a twelve-row table cannot + * tell a real tie-breaker from a lucky plan, which is the whole reason this + * file needed pins in the first place. * * ## Why a SQLite-backed client stub * @@ -139,10 +147,18 @@ describe('TursoDriver remote — paged reads are a partition of the result set', expect([...seen].sort()).toEqual([...PAGINATION_ALL_IDS].sort()); }); - it(`page boundaries are invisible with NO orderBy — ${testCase.name}`, async () => { - const paged = await walk(testCase.pageSize); - const whole: Array> = await driver.find('ticket', {}); - expect(paged).toEqual(whole.map((r) => String(r.id))); + it(`walks an unsorted read in id order — ${testCase.name}`, async () => { + // The local twin's assertion, now that remote answers the same way. Its + // predecessor here compared the page walk against the UNPAGED unordered + // read and passed because neither was ordered; after #5653 the two + // legitimately differ — the walk is a partition ordered by `id`, while + // the unpaged read keeps #4363's carve-out and is handed back in whatever + // order the plan chose. Comparing them would now pin the carve-out's + // absence, so the honest replacement is the property the walk itself must + // have. The fixture's ids are shuffled relative to insertion order, so + // this distinguishes "the tie-breaking ORDER BY reached the endpoint" + // from "SQLite happened to hand back rowid order". + expect(await walk(testCase.pageSize)).toEqual([...PAGINATION_ALL_IDS].sort()); }); } @@ -152,27 +168,32 @@ describe('TursoDriver remote — paged reads are a partition of the result set', }); /** - * The two pins. See the module doc: the cases above pass, but not by the - * mechanism the contract names, and a green suite that leaves that unsaid is - * how "covered" quietly stops meaning anything. Both assert the CURRENT - * behaviour and are expected to go red — and be rewritten — the day #5653 - * gives this transport the tie-breaker local mode already has. + * The two pins, flipped by #5653. See the module doc: the cases above pass, + * and until #5653 they did not pass by the mechanism the contract names — + * a green suite that leaves that unsaid is how "covered" quietly stops + * meaning anything. Each now asserts the concrete sequence the mechanism + * produces AND the storage-order arrangement it replaced, so a regression + * that took the tie-breaker back out turns them red on the positive + * assertion instead of leaving them green on an empty one. */ - it('records the measured mechanism: a sorted paged read appends NO tie-breaker (#5653)', async () => { + it('a sorted paged read breaks its ties by id, not by storage order (#5653)', async () => { const seen = await walk(5, [{ field: 'status', order: 'asc' }]); - // What the caller's key alone produces: the `status` groups in order, and - // INSIDE each group the rows in storage (insertion) order. With the `id` - // tie-breaker local mode appends, the `done` group would instead read - // r02,r03,r09,r10 — id order. + // The `status` groups in order, and INSIDE each group the rows in id + // order — `done` reads r02,r03,r09,r10, where the caller's key alone left + // it r03,r09,r02,r10 (insertion order). + const groupedByStatusThenId = [...PAGINATION_ROWS] + .sort((x, y) => x.status.localeCompare(y.status) || x.id.localeCompare(y.id)) + .map((row) => row.id); + expect(seen).toEqual(groupedByStatusThenId); const groupedByStatusThenInsertion = PAGINATION_ROWS.map((row, index) => ({ row, index })) .sort((x, y) => x.row.status.localeCompare(y.row.status) || x.index - y.index) .map(({ row }) => row.id); - expect(seen).toEqual(groupedByStatusThenInsertion); + expect(seen).not.toEqual(groupedByStatusThenInsertion); }); - it('records the measured mechanism: an unsorted paged read is served in storage order, not id order (#5653)', async () => { + it('an unsorted paged read is served in id order, not storage order (#5653)', async () => { const seen = await walk(5); - expect(seen).toEqual(PAGINATION_ROWS.map((r) => r.id)); - expect(seen).not.toEqual([...PAGINATION_ALL_IDS].sort()); + expect(seen).toEqual([...PAGINATION_ALL_IDS].sort()); + expect(seen).not.toEqual(PAGINATION_ROWS.map((r) => r.id)); }); });