From 8cec925aead174984b790c2567630b2fae77bfd5 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 00:20:36 +0000 Subject: [PATCH] fix(driver-turso): give the remote transport the same deterministic-paging rule local mode has (#5653) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `RemoteTransport.buildSelectSQL` mapped the caller's `orderBy` verbatim and appended no unique column before `LIMIT`/`OFFSET`, so a paged read over a non-unique sort key had no tie-breaker and an unsorted paged read had no ORDER BY at all. SQLite promises nothing about how equal keys are arranged across two statements, so walking pages could serve one row twice and never serve another — while every page is full and every row legitimate. The same driver's local face has answered this since #4363, which left ONE driver giving two different ordering guarantees for the same query, selected by URL alone (the ADR-0053 D-A1 seam). Reuse, not a second rule: `TursoDriver.find`/`findOne` now resolve the complete sort key list through the inherited `SqlDriver.orderKeysFor()` and hand the transport a query that already carries it, so the three-state table (incl. #4363's unpaged-unordered carve-out) has one implementation. Resolving it at this seam rather than inside `buildSelectSQL` is also what keeps `findOne` unsorted: the transport injects its own `limit: 1`, which by SQL-build time is indistinguishable from page one of a walk. `paginationTieBreaker` is overridden to answer the one remote-specific FACT the rule needs — whether this driver created the table — from the objects synced through `RemoteTransport` (its CREATE TABLE always writes `"id" TEXT PRIMARY KEY`), since remote DDL never reaches the `initObjects` that fills the base class's `managedObjectFields`. Tables this driver did not create keep prior behaviour exactly and warn once. #5590's two "records the measured mechanism" pins are flipped with it: they now assert the concrete sequences the mechanism produces, each against the storage-order arrangement it replaced. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01WyvqvKMG6asi9aXjKE6xtx --- .../turso-remote-pagination-tiebreaker.md | 32 +++ .../src/remote-pagination-tiebreaker.test.ts | 201 ++++++++++++++++++ .../driver-turso/src/remote-transport.ts | 18 ++ .../drivers/driver-turso/src/turso-driver.ts | 97 ++++++++- .../src/turso-pagination-conformance.test.ts | 9 +- ...urso-remote-pagination-conformance.test.ts | 99 +++++---- 6 files changed, 412 insertions(+), 44 deletions(-) create mode 100644 .changeset/turso-remote-pagination-tiebreaker.md create mode 100644 packages/drivers/driver-turso/src/remote-pagination-tiebreaker.test.ts 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)); }); });