diff --git a/.changeset/pagination-filter-logic-driver-axis.md b/.changeset/pagination-filter-logic-driver-axis.md new file mode 100644 index 0000000000..d78e3f4827 --- /dev/null +++ b/.changeset/pagination-filter-logic-driver-axis.md @@ -0,0 +1,21 @@ +--- +--- + +Test-only: run the two remaining `@objectstack/spec/data` shared matrices +`driver-sql` consumes — `PAGINATION_CASES` / `PAGINATION_UNORDERED_CASES` and +`FILTER_LOGIC_CASES` — across the ADR-0053 D-A3 DRIVER axis (`driver {SQLite, +Postgres at minimum}`) instead of a hard-coded `better-sqlite3` client (#4714, +finishing what #4245 started for the temporal matrix). Both files now sweep once +per cell of `DIALECT_CELLS` — SQLite always, live Postgres and MySQL whenever +`OS_TEST_POSTGRES_URL` / `OS_TEST_MYSQL_URL` are provisioned — over the same +cases, asserting the same row-id sets cell for cell, with issue-prefixed table +names so parallel suites cannot collide on a live server. The paged-read +property test is the one that gains: on SQLite it passes with or without the +tie-breaker (a twelve-row table hands ties back in rowid order every time), +while on live Postgres removing the tie-breaker makes it serve one row twice and +another never — objectui#3106 verbatim. `declareUnprovisionedCell` moves into +`live-dialect-matrix.testkit.ts` so all three matrices share one non-vacuity +guard: a missing URL is a named skip, and a red under +`OS_EXPECT_LIVE_DIALECT_MATRIX=1`. No new CI job — the existing +`Temporal Conformance (live PG + MySQL)` workflow already runs this whole +package against both servers. Releases nothing. diff --git a/packages/plugins/driver-sql/src/live-dialect-matrix.testkit.ts b/packages/plugins/driver-sql/src/live-dialect-matrix.testkit.ts index 7eeb28c01e..6a35ea5067 100644 --- a/packages/plugins/driver-sql/src/live-dialect-matrix.testkit.ts +++ b/packages/plugins/driver-sql/src/live-dialect-matrix.testkit.ts @@ -42,7 +42,7 @@ * Test-only: not exported from `index.ts`. */ -import { expect } from 'vitest'; +import { describe, expect, it } from 'vitest'; import type { SqlDriver, SqlDriverConfig } from './sql-driver.js'; /** The dialects `driver-sql` speaks that the matrices are run across. */ @@ -134,6 +134,37 @@ export const DIALECT_CELLS: readonly DialectCell[] = [ /** The live cells only — the ones the server-timezone axis applies to. */ export const LIVE_DIALECT_CELLS = DIALECT_CELLS.filter((c) => c.live); +/** + * Declare a cell nobody provisioned: REPORTED, never omitted. + * + * A named skip locally (so `it was not run` is readable in the output), a + * failure under `OS_EXPECT_LIVE_DIALECT_MATRIX=1` — which is what stops the + * `Temporal Conformance (live PG + MySQL)` job from quietly degrading to + * SQLite-only coverage if its `env:` block is ever dropped. + * + * Lives here rather than in each consumer for the same reason `DIALECT_CELLS` + * does: a guard copy-pasted per suite is a guard that can weaken in one copy + * and nowhere else — and "the matrix silently found zero cells and reported + * OK" is the failure #4646 already paid for once. + * + * @param matrix which matrix this cell belongs to, e.g. `temporal conformance` + * — it names both the suite and the failure message. + */ +export function declareUnprovisionedCell(cell: DialectCell, matrix: string): void { + describe(`sql-driver — ${matrix} matrix (${cell.label})`, () => { + it.skipIf(!EXPECT_LIVE_DIALECTS)( + `is provisioned — set ${cell.env} to run this cell of the D-A3 driver axis`, + () => { + expect.fail( + `${cell.env} is unset while OS_EXPECT_LIVE_DIALECT_MATRIX=1: this runner declared it ` + + `provisions live Postgres and MySQL, so the ${cell.label} cell of the ${matrix} ` + + `matrix must not be skipped (ADR-0053 D-A3 "Postgres at minimum").`, + ); + }, + ); + }); +} + /** What a server reports about its own timezone. */ export interface ServerZone { /** The dialect's own spelling: `Asia/Shanghai`, `+08:00`, `SYSTEM`, … */ diff --git a/packages/plugins/driver-sql/src/sql-driver-or-filter.test.ts b/packages/plugins/driver-sql/src/sql-driver-or-filter.test.ts index 2cab2940ba..10af2ecb52 100644 --- a/packages/plugins/driver-sql/src/sql-driver-or-filter.test.ts +++ b/packages/plugins/driver-sql/src/sql-driver-or-filter.test.ts @@ -1,8 +1,7 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. /** - * Filter logical-combinator conformance for the SQL compiler, on a real engine - * (in-memory better-sqlite3). + * Filter logical-combinator conformance for the SQL compiler, on real engines. * * The shared cases come from `@objectstack/spec/data` so this backend, * `driver-memory`, `formula`'s `matchesFilterCondition` and `read-scope-sql` @@ -18,96 +17,156 @@ * The SQL-specific cases below the conformance sweep cover ground the shared * table deliberately leaves out: a real DATE-typed column, and columns whose * values are not the shared fixture's plain strings. + * + * # The DRIVER axis (#4714, ADR-0053 D-A3) + * + * D-A3 declares the matrix over `driver {SQLite, Postgres at minimum}`. This + * suite used to hard-code `client: 'better-sqlite3'` — its describe was even + * named `(SQLite)` — so what it proved was that ONE engine executes the + * compiled predicate as the table says, while `where`-clause grouping and + * three-valued logic are precisely where dialects are free to differ. A + * compiler bug that only Postgres or MySQL can see had nothing to fail. + * + * So the sweep runs once per cell of `DIALECT_CELLS` — SQLite always, live + * Postgres and MySQL when `OS_TEST_POSTGRES_URL` / `OS_TEST_MYSQL_URL` are + * provisioned — over the SAME `FILTER_LOGIC_CASES`, asserting the SAME row-id + * sets cell for cell. A cell nobody provisioned is a named skip, and a red under + * `OS_EXPECT_LIVE_DIALECT_MATRIX=1` (`declareUnprovisionedCell`): a matrix that + * silently found zero live cells must not report OK (#4646). No new CI job — the + * `Temporal Conformance (live PG + MySQL)` workflow already runs this whole + * package against both servers. + * + * The shared cases are consumed here, never edited: if a live cell goes red the + * finding is that dialect's compile, not the case. (Nothing here is temporal, so + * the D-B3 server-timezone axis does not apply — requiring a non-UTC server + * would only manufacture reds that say nothing about `$or`.) */ -import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; import { FILTER_LOGIC_CASES, FILTER_LOGIC_ROWS } from '@objectstack/spec/data'; import { SqlDriver } from '../src/index.js'; +import { + DIALECT_CELLS, + declareUnprovisionedCell, + type DialectCell, +} from './live-dialect-matrix.testkit.js'; + +/** + * Issue-prefixed table names: the live cells share one database with every + * other suite in this package (and with each other's runs), so the bare `t` / + * `task` this suite used while it was SQLite-only would be a collision waiting + * to be read as a conformance failure. + */ +const FILTER_TABLE = 'os4714_filter_logic'; +const DATE_WINDOW_TABLE = 'os4714_filter_logic_windows'; + +// ── The driver axis ───────────────────────────────────────────────────────── + +for (const cell of DIALECT_CELLS) { + if (!cell.available) { + declareUnprovisionedCell(cell, 'filter-logic conformance'); + continue; + } + declareFilterLogicSweep(cell); +} + +function declareFilterLogicSweep(cell: DialectCell): void { + describe(`SqlDriver filter logic conformance (${cell.label})`, () => { + let driver: SqlDriver; + let knexInstance: any; -describe('SqlDriver filter logic conformance (SQLite)', () => { - let driver: SqlDriver; - let knexInstance: any; + beforeAll(async () => { + driver = new SqlDriver(cell.config()); + knexInstance = (driver as any).knex; - beforeEach(async () => { - driver = new SqlDriver({ - client: 'better-sqlite3', - connection: { filename: ':memory:' }, - useNullAsDefault: true, + // Live cells reuse one database, so the sweep starts from a dropped table. + await knexInstance.schema.dropTableIfExists(FILTER_TABLE); + await knexInstance.schema.createTable(FILTER_TABLE, (t: any) => { + t.string('id').primary(); + t.string('a'); + t.string('b'); + t.string('c'); + t.string('owner'); + t.string('status'); + t.string('parent_object'); + t.string('parent_id'); + }); + await knexInstance(FILTER_TABLE).insert([...FILTER_LOGIC_ROWS]); }); - knexInstance = (driver as any).knex; - await knexInstance.schema.createTable('t', (t: any) => { - t.string('id').primary(); - t.string('a'); - t.string('b'); - t.string('c'); - t.string('owner'); - t.string('status'); - t.string('parent_object'); - t.string('parent_id'); + afterAll(async () => { + await knexInstance?.schema.dropTableIfExists(FILTER_TABLE).catch(() => {}); + await driver?.disconnect?.(); }); - await knexInstance('t').insert([...FILTER_LOGIC_ROWS]); - }); - afterEach(async () => { - await knexInstance.destroy(); - }); + describe('shared conformance cases', () => { + for (const c of FILTER_LOGIC_CASES) { + it(c.name, async () => { + const rows = await driver.find(FILTER_TABLE, { object: FILTER_TABLE, where: c.filter }); + const got = rows + .map((r: any) => String(r.id)) + .sort((x: string, y: string) => x.localeCompare(y)); + expect(got, c.note).toEqual(c.expected); + }); + } + }); - describe('shared conformance cases', () => { - for (const c of FILTER_LOGIC_CASES) { - it(c.name, async () => { - const rows = await driver.find('t', { object: 't', where: c.filter }); - const got = rows - .map((r: any) => String(r.id)) - .sort((x: string, y: string) => x.localeCompare(y)); - expect(got, c.note).toEqual(c.expected); + /** + * The abutting-window pattern the automation skill docs recommend and the CLI + * flow linter blesses (`lint-flow-patterns`): each tier is one field carrying + * two operators. "Windows tile the timeline so each record matches exactly one + * tier" only holds if those operators AND — under the old compile every tier + * degenerated to `d >= lo OR d < hi`, i.e. matched every row. + * + * The shared table pins this shape on plain strings; this pins it on a real + * date column, where value coercion also runs — and now on each dialect's own + * DATE type, which is where a bare `YYYY-MM-DD` comparand stops being one + * agreed thing (the D-B2 divergence, measured on PG @ Asia/Shanghai). + */ + describe('multi-operator date windows inside $or', () => { + beforeAll(async () => { + await knexInstance.schema.dropTableIfExists(DATE_WINDOW_TABLE); + await knexInstance.schema.createTable(DATE_WINDOW_TABLE, (t: any) => { + t.string('id').primary(); + t.date('end_date'); + }); + await knexInstance(DATE_WINDOW_TABLE).insert([ + { id: 'd07', end_date: '2026-08-07' }, + { id: 'd15', end_date: '2026-08-15' }, + { id: 'd30', end_date: '2026-08-30' }, + { id: 'd60', end_date: '2026-09-29' }, + ]); }); - } - }); - /** - * The abutting-window pattern the automation skill docs recommend and the CLI - * flow linter blesses (`lint-flow-patterns`): each tier is one field carrying - * two operators. "Windows tile the timeline so each record matches exactly one - * tier" only holds if those operators AND — under the old compile every tier - * degenerated to `d >= lo OR d < hi`, i.e. matched every row. - * - * The shared table pins this shape on plain strings; this pins it on a real - * date column, where value coercion also runs. - */ - describe('multi-operator date windows inside $or', () => { - beforeEach(async () => { - await knexInstance.schema.createTable('task', (t: any) => { - t.string('id').primary(); - t.date('end_date'); + afterAll(async () => { + await knexInstance?.schema.dropTableIfExists(DATE_WINDOW_TABLE).catch(() => {}); }); - await knexInstance('task').insert([ - { id: 'd07', end_date: '2026-08-07' }, - { id: 'd15', end_date: '2026-08-15' }, - { id: 'd30', end_date: '2026-08-30' }, - { id: 'd60', end_date: '2026-09-29' }, - ]); - }); - it('matches only the rows inside the abutting windows', async () => { - const rows = await driver.find('task', { - object: 'task', - where: { - $or: [ - { end_date: { $gte: '2026-08-07', $lt: '2026-08-08' } }, - { end_date: { $gte: '2026-08-30', $lt: '2026-08-31' } }, - ], - }, + it('matches only the rows inside the abutting windows', async () => { + const rows = await driver.find(DATE_WINDOW_TABLE, { + object: DATE_WINDOW_TABLE, + where: { + $or: [ + { end_date: { $gte: '2026-08-07', $lt: '2026-08-08' } }, + { end_date: { $gte: '2026-08-30', $lt: '2026-08-31' } }, + ], + }, + }); + expect(rows.map((r: any) => r.id).sort()).toEqual(['d07', 'd30']); }); - expect(rows.map((r: any) => r.id).sort()).toEqual(['d07', 'd30']); - }); - it('keeps a window AND-ed with a sibling key in the same branch', async () => { - const rows = await driver.find('task', { - object: 'task', - where: { $or: [{ id: 'nope' }, { end_date: { $gte: '2026-08-07', $lt: '2026-08-31' }, id: 'd15' }] }, + it('keeps a window AND-ed with a sibling key in the same branch', async () => { + const rows = await driver.find(DATE_WINDOW_TABLE, { + object: DATE_WINDOW_TABLE, + where: { + $or: [ + { id: 'nope' }, + { end_date: { $gte: '2026-08-07', $lt: '2026-08-31' }, id: 'd15' }, + ], + }, + }); + expect(rows.map((r: any) => r.id)).toEqual(['d15']); }); - expect(rows.map((r: any) => r.id)).toEqual(['d15']); }); }); -}); +} diff --git a/packages/plugins/driver-sql/src/sql-driver-pagination-conformance.test.ts b/packages/plugins/driver-sql/src/sql-driver-pagination-conformance.test.ts index 2b707978d5..e7ea4b9362 100644 --- a/packages/plugins/driver-sql/src/sql-driver-pagination-conformance.test.ts +++ b/packages/plugins/driver-sql/src/sql-driver-pagination-conformance.test.ts @@ -13,13 +13,11 @@ * a future driver has to satisfy however it chooses to. * * 2. **The clause** — assert the SQL that actually reached the database - * carries the ordering. The property test cannot be trusted on its own - * *here*: SQLite over a twelve-row table hands back both ties and wholly - * unordered rows in rowid order every time, so the partition check passes - * with or without the fix. It is a real check on MongoDB (where the - * reshuffle is documented and observable) and a regression lock on the - * memory driver; on SQL it would pass the day someone deletes the feature. - * The emitted-SQL assertion is the half that fails then. + * carries the ordering. On the embedded cell the property test cannot be + * trusted on its own: SQLite over a twelve-row table hands back both ties + * and wholly unordered rows in rowid order every time, so the partition + * check passes with or without the fix. The emitted-SQL assertion is the + * half that fails then. * * That asymmetry is the point rather than an apology for it: the property is * the contract, and the clause is how this backend happens to keep it. @@ -29,9 +27,44 @@ * distinction is load-bearing: a test that rebuilds the ORDER BY itself asserts * only that the helper works, and stays green on the day `find()` stops calling * it — which is precisely the day this file exists for. + * + * # The DRIVER axis (#4714, ADR-0053 D-A3) + * + * D-A3 declares the matrix over `driver {SQLite, Postgres at minimum}`. Both + * describes above used to hard-code `client: 'better-sqlite3'`, so the shared + * `PAGINATION_CASES` / `PAGINATION_UNORDERED_CASES` only ever ran on the one + * backend whose head note (above) says the property half proves nothing there. + * That is the asymmetry stated as a permanent excuse rather than as the local + * fact it is: on a real server the partition check is the half with teeth, + * because a plan that reshuffles ties across two statements is what objectui#3106 + * was actually reported as. + * + * So both halves now run once per cell of `DIALECT_CELLS` — SQLite always, live + * Postgres and MySQL when `OS_TEST_POSTGRES_URL` / `OS_TEST_MYSQL_URL` are + * provisioned — over the SAME cases, asserting the SAME row-id sets cell for + * cell. A cell nobody provisioned is a named skip, and a red under + * `OS_EXPECT_LIVE_DIALECT_MATRIX=1` (`declareUnprovisionedCell`): a matrix that + * silently found zero live cells must not report OK (#4646). No new CI job — the + * `Temporal Conformance (live PG + MySQL)` workflow already runs this whole + * package against both servers. + * + * Two things this file deliberately does NOT do: + * + * - **No server-timezone axis.** D-B3's three-way skew guard belongs to the + * temporal matrix, whose answers a zone can move; nothing here compares an + * instant, so requiring a non-UTC server would only manufacture reds that + * say nothing about pagination. + * - **No tie-breaking added to the fixture to keep a dialect green.** The + * shared rows repeat their sort keys on purpose; a live cell that goes red + * over them is the finding this axis exists to surface, not a case to + * soften. (`@objectstack/spec/data` is consumed here, never edited.) + * + * Honest limit of the live cells: twelve rows is one seq scan, so a server MAY + * hand ties back in a steady order anyway and pass without the tie-breaker. + * The clause half — now also per dialect — is what stays sharp in that case. */ -import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { describe, it, expect, beforeAll, afterAll, vi } from 'vitest'; import { PAGINATION_ALL_IDS, PAGINATION_CASES, @@ -40,6 +73,60 @@ import { } from '@objectstack/spec/data'; import type { QueryAST } from '@objectstack/spec/data'; import { SqlDriver } from '../src/index.js'; +import { + DIALECT_CELLS, + declareUnprovisionedCell, + type DialectCell, +} from './live-dialect-matrix.testkit.js'; + +/** + * One table per sweep, issue-prefixed: the live cells share a database with + * every other suite in this package (and with each other's runs), so a name + * collision would show up as a pagination failure in whichever suite lost the + * race. + */ +const PAGED_TABLE = 'os4714_pagination'; +const CLAUSE_TABLE = 'os4714_pagination_clause'; + +/** + * How each dialect spells a quoted identifier — knex's own quoting, which is + * what the emitted SQL carries. The clause assertions match on the quoted form + * rather than a bare word so `id` cannot be satisfied by an unrelated + * substring, exactly as they did when this file was SQLite-only. + */ +const IDENTIFIER_QUOTE: Record = { + sqlite: '`', + pg: '"', + mysql: '`', +}; + +/** `/order by .*"status" asc,.*"id" asc/i` for the cell's quoting. */ +function orderedBy(cell: DialectCell, ...keys: Array<[string, 'asc' | 'desc']>): RegExp { + const q = IDENTIFIER_QUOTE[cell.id]; + return new RegExp(`order by ${keys.map(([f, d]) => `.*${q}${f}${q} ${d}`).join(',')}`, 'i'); +} + +/** Live cells reuse one database, so every sweep starts from a dropped table. */ +async function freshTable( + driver: SqlDriver, + shape: { name: string; fields: Record }, +): Promise { + await driver.execute(`drop table if exists ${shape.name}`).catch(() => {}); + await driver.initObjects([shape as any]); +} + +async function dropTable(driver: SqlDriver | undefined, table: string): Promise { + await driver?.execute(`drop table if exists ${table}`).catch(() => {}); +} + +const ticketShape = (name: string) => ({ + name, + fields: { + status: { type: 'string' }, + rank: { type: 'integer' }, + name: { type: 'string' }, + }, +}); /** * Records the SQL of every statement this driver sends, so a test can assert @@ -54,257 +141,288 @@ class InspectableSqlDriver extends SqlDriver { }); } - /** The SQL of the single statement issued while running `run`. */ - async sqlOf(run: () => Promise): Promise { + /** + * The SQL of the single statement `run` issued **against `table`**. + * + * Scoped to the table rather than "the only statement of the run" because a + * live cell's connection is pooled and a dialect may introspect on first use; + * neither is the statement under test. Two statements against the same table + * still fail here, which is the assertion's actual job. + */ + async sqlOf(table: string, run: () => Promise): Promise { const from = this.statements.length; await run(); - const issued = this.statements.slice(from); - expect(issued, 'expected exactly one statement').toHaveLength(1); + const issued = this.statements.slice(from).filter((sql) => sql.includes(table)); + expect(issued, `expected exactly one statement against ${table}`).toHaveLength(1); return issued[0]!; } } -describe('SqlDriver — paged reads are a partition of the result set (objectui#3106)', () => { - let driver: SqlDriver; +// ── The driver axis ───────────────────────────────────────────────────────── - beforeEach(async () => { - driver = new SqlDriver({ - client: 'better-sqlite3', - connection: { filename: ':memory:' }, - useNullAsDefault: true, - }); +for (const cell of DIALECT_CELLS) { + if (!cell.available) { + declareUnprovisionedCell(cell, 'paged-read conformance'); + continue; + } + declarePartitionSweep(cell); + declareClauseSweep(cell); +} - 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 }); - } - }); +// ── Half 1: the property, over the shared cases ───────────────────────────── - afterEach(async () => { - await driver.disconnect(); - }); +function declarePartitionSweep(cell: DialectCell): void { + describe(`SqlDriver — paged reads are a partition of the result set (objectui#3106) (${cell.label})`, () => { + let driver: SqlDriver; - 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', - { object: 'ticket', orderBy: [...testCase.orderBy], limit: testCase.pageSize, offset }, - { bypassTenantAudit: true }, - ); - seen.push(...page.map((r) => String(r.id))); + beforeAll(async () => { + driver = new SqlDriver(cell.config()); + await freshTable(driver, ticketShape(PAGED_TABLE)); + + for (const row of PAGINATION_ROWS) { + await driver.create(PAGED_TABLE, { ...row }, { bypassTenantAudit: true }); } + }); - 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()); + afterAll(async () => { + await dropTable(driver, PAGED_TABLE); + await driver?.disconnect?.(); }); - it(`orders pages consistently with the requested sort — ${testCase.name}`, async () => { + const walk = async (query: Omit, pageSize: number) => { const paged: Array> = []; - for (let offset = 0; offset < PAGINATION_ROWS.length; offset += testCase.pageSize) { + for (let offset = 0; offset < PAGINATION_ROWS.length; offset += pageSize) { const page = await driver.find( - 'ticket', - { object: 'ticket', orderBy: [...testCase.orderBy], limit: testCase.pageSize, offset }, + PAGED_TABLE, + { object: PAGED_TABLE, ...query, limit: pageSize, offset }, { bypassTenantAudit: true }, ); paged.push(...page); } + return paged; + }; - // 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', - { object: 'ticket', orderBy: [...testCase.orderBy] }, - { bypassTenantAudit: true }, - ); - expect(paged.map((r) => r.id)).toEqual(whole.map((r) => r.id)); - }); - } - - for (const testCase of PAGINATION_UNORDERED_CASES) { - it(`visits every row exactly once with NO orderBy at all — ${testCase.name}`, async () => { - const seen: string[] = []; - for (let offset = 0; offset < PAGINATION_ROWS.length; offset += testCase.pageSize) { - const page = await driver.find( - 'ticket', - { object: 'ticket', limit: testCase.pageSize, offset }, - { bypassTenantAudit: true }, + for (const testCase of PAGINATION_CASES) { + it(`visits every row exactly once — ${testCase.name}`, async () => { + const seen = (await walk({ orderBy: [...testCase.orderBy] }, testCase.pageSize)).map((r) => + String(r.id), ); - 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()); - }); + expect(seen).toHaveLength(PAGINATION_ALL_IDS.length); + expect(new Set(seen).size).toBe(PAGINATION_ALL_IDS.length); + expect([...seen].sort()).toEqual([...PAGINATION_ALL_IDS].sort()); + }); - it(`walks an unsorted read in id order — ${testCase.name}`, async () => { - const paged: string[] = []; - for (let offset = 0; offset < PAGINATION_ROWS.length; offset += testCase.pageSize) { - const page = await driver.find( - 'ticket', - { object: 'ticket', limit: testCase.pageSize, offset }, + it(`orders pages consistently with the requested sort — ${testCase.name}`, async () => { + const paged = await walk({ orderBy: [...testCase.orderBy] }, testCase.pageSize); + + // 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( + PAGED_TABLE, + { object: PAGED_TABLE, orderBy: [...testCase.orderBy] }, { bypassTenantAudit: true }, ); - paged.push(...page.map((r) => String(r.id))); - } + expect(paged.map((r) => r.id)).toEqual(whole.map((r) => r.id)); + }); + } - // The fixture's ids are shuffled relative to insertion order, so id order - // is visibly an order this driver chose rather than the one the table - // would have handed back anyway. - expect(paged).toEqual([...PAGINATION_ALL_IDS].sort()); - }); - } + for (const testCase of PAGINATION_UNORDERED_CASES) { + it(`visits every row exactly once with NO orderBy at all — ${testCase.name}`, async () => { + const seen = (await walk({}, testCase.pageSize)).map((r) => String(r.id)); - it('leaves an UNPAGED unordered read alone — no sort is imposed on a caller who asked for none', async () => { - // The carve-out (objectstack#4363): with no `limit`/`offset` the caller - // receives the whole matching set, so no slice can be wrong, and an imposed - // ORDER BY would only change plan selection. The order below is SQLite's - // own answer, not one this driver asked for — which the shuffled fixture - // ids make visible. - const rows = await driver.find('ticket', { object: 'ticket' }, { bypassTenantAudit: true }); - expect(rows.map((r) => r.id)).toEqual(PAGINATION_ROWS.map((r) => 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()); + }); -describe('SqlDriver — the ORDER BY that reaches the database', () => { - let driver: InspectableSqlDriver; + it(`walks an unsorted read in id order — ${testCase.name}`, async () => { + const paged = (await walk({}, testCase.pageSize)).map((r) => String(r.id)); - beforeEach(async () => { - driver = new InspectableSqlDriver({ - client: 'better-sqlite3', - connection: { filename: ':memory:' }, - useNullAsDefault: true, + // The fixture's ids are shuffled relative to insertion order, so id order + // is visibly an order this driver chose rather than the one the table + // would have handed back anyway. + expect(paged).toEqual([...PAGINATION_ALL_IDS].sort()); + }); + } + + it('leaves an UNPAGED unordered read alone — no sort is imposed on a caller who asked for none', async () => { + // The carve-out (objectstack#4363): with no `limit`/`offset` the caller + // receives the whole matching set, so no slice can be wrong, and an + // imposed ORDER BY would only change plan selection. + const rows = await driver.find( + PAGED_TABLE, + { object: PAGED_TABLE }, + { bypassTenantAudit: true }, + ); + expect([...rows.map((r) => String(r.id))].sort()).toEqual([...PAGINATION_ALL_IDS].sort()); + + // WHICH order those rows arrive in is the dialect's own answer, and the + // one thing #4363 promises about this shape is that the driver adds + // nothing to it — so only the embedded cell, whose plan is fixed, pins the + // exact sequence (insertion order, visibly not the shuffled ids' order). + // MySQL hands back InnoDB primary-key order and Postgres its heap order; + // pinning either would assert the server's plan as if it were our + // contract. The contract half — that no ORDER BY was emitted at all — is + // asserted per dialect in the clause sweep below. + if (!cell.live) { + expect(rows.map((r) => r.id)).toEqual(PAGINATION_ROWS.map((r) => r.id)); + } }); - await driver.initObjects([ - { name: 'ticket', fields: { status: { type: 'string' }, rank: { type: 'integer' } } }, - ]); - driver.captureStatements(); }); +} - afterEach(async () => { - await driver.disconnect(); - }); +// ── Half 2: the ORDER BY that reaches the database ────────────────────────── + +function declareClauseSweep(cell: DialectCell): void { + describe(`SqlDriver — the ORDER BY that reaches the database (${cell.label})`, () => { + let driver: InspectableSqlDriver; + + beforeAll(async () => { + driver = new InspectableSqlDriver(cell.config()); + await freshTable(driver, { + name: CLAUSE_TABLE, + fields: { status: { type: 'string' }, rank: { type: 'integer' } }, + }); + // One warm-up read before the recorder is installed: a live dialect may + // introspect on first use, and that statement is not the one under test. + await driver.find(CLAUSE_TABLE, { object: CLAUSE_TABLE }, { bypassTenantAudit: true }); + driver.captureStatements(); + }); - const sqlOfFind = (query: Omit) => - driver.sqlOf(() => driver.find('ticket', { object: 'ticket', ...query }, { bypassTenantAudit: true })); + afterAll(async () => { + await dropTable(driver, CLAUSE_TABLE); + await driver?.disconnect?.(); + }); - it('appends `id` after a non-unique sort key', async () => { - const sql = await sqlOfFind({ orderBy: [{ field: 'status', order: 'asc' }] }); - expect(sql).toMatch(/order by .*`status` asc, .*`id` asc/i); - }); + const sqlOfFind = (query: Omit) => + driver.sqlOf(CLAUSE_TABLE, () => + driver.find(CLAUSE_TABLE, { object: CLAUSE_TABLE, ...query }, { bypassTenantAudit: true }), + ); - it('appends `id` in the LAST key\'s direction, so one index pass can serve it', async () => { - const sql = await sqlOfFind({ - orderBy: [ - { field: 'status', order: 'asc' }, - { field: 'rank', order: 'desc' }, - ], + it('appends `id` after a non-unique sort key', async () => { + const sql = await sqlOfFind({ orderBy: [{ field: 'status', order: 'asc' }] }); + expect(sql).toMatch(orderedBy(cell, ['status', 'asc'], ['id', 'asc'])); }); - expect(sql).toMatch(/order by .*`status` asc, .*`rank` desc, .*`id` desc/i); - }); - it('does not repeat `id` when the caller already sorted by it', async () => { - const sql = await sqlOfFind({ orderBy: [{ field: 'id', order: 'desc' }], limit: 5, offset: 5 }); - expect(sql.match(/`id`/g) ?? []).toHaveLength(1); - }); - - it('orders a paged read by `id` when the caller sent no orderBy (objectstack#4363)', async () => { - expect(await sqlOfFind({ limit: 5, offset: 5 })).toMatch(/order by .*`id` asc/i); - }); + it("appends `id` in the LAST key's direction, so one index pass can serve it", async () => { + const sql = await sqlOfFind({ + orderBy: [ + { field: 'status', order: 'asc' }, + { field: 'rank', order: 'desc' }, + ], + }); + expect(sql).toMatch(orderedBy(cell, ['status', 'asc'], ['rank', 'desc'], ['id', 'desc'])); + }); - it('counts `limit` alone as paged — page one must agree with pages two onward', async () => { - // A list view's first request is routinely `limit=50` with no offset at - // all. Ordering only the offset-carrying pages would cut page one from a - // different arrangement than the rest of the walk: the defect intact, - // wearing a fix. - expect(await sqlOfFind({ limit: 5 })).toMatch(/order by .*`id` asc/i); - expect(await sqlOfFind({ offset: 5 })).toMatch(/order by .*`id` asc/i); - expect(await sqlOfFind({ orderBy: [], limit: 5 })).toMatch(/order by .*`id` asc/i); - }); + it('does not repeat `id` when the caller already sorted by it', async () => { + const sql = await sqlOfFind({ orderBy: [{ field: 'id', order: 'desc' }], limit: 5, offset: 5 }); + const q = IDENTIFIER_QUOTE[cell.id]; + expect(sql.match(new RegExp(`${q}id${q}`, 'g')) ?? []).toHaveLength(1); + }); - it('emits no ORDER BY for an unpaged read with no orderBy', async () => { - expect(await sqlOfFind({})).not.toMatch(/order by/i); - expect(await sqlOfFind({ where: { status: 'open' } })).not.toMatch(/order by/i); - expect(await sqlOfFind({ orderBy: [] })).not.toMatch(/order by/i); - }); + it('orders a paged read by `id` when the caller sent no orderBy (objectstack#4363)', async () => { + expect(await sqlOfFind({ limit: 5, offset: 5 })).toMatch(orderedBy(cell, ['id', 'asc'])); + }); - it('leaves `findOne` unordered — its `limit: 1` is not page one of a walk', async () => { - // `findOne` promises *a* matching record, not a position in a sequence, so - // there is no partition to keep. Reading its `limit: 1` as a page would - // impose `ORDER BY id LIMIT 1` on the hottest read in the system, which is - // the shape that makes a planner abandon the predicate's own index: on a - // 2M-row Postgres table `WHERE owner_id = ? LIMIT 1` measured 0.08 ms - // unordered and 7.8 ms with `ORDER BY id`. `MongoDBDriver.findOne` has - // never sorted either — this keeps the two drivers saying the same thing. - const sql = await driver.sqlOf(() => - driver.findOne('ticket', { object: 'ticket', where: { status: 'open' } }, { - bypassTenantAudit: true, - }), - ); - expect(sql).not.toMatch(/order by/i); - expect(sql).toMatch(/limit/i); - }); + it('counts `limit` alone as paged — page one must agree with pages two onward', async () => { + // A list view's first request is routinely `limit=50` with no offset at + // all. Ordering only the offset-carrying pages would cut page one from a + // different arrangement than the rest of the walk: the defect intact, + // wearing a fix. + expect(await sqlOfFind({ limit: 5 })).toMatch(orderedBy(cell, ['id', 'asc'])); + expect(await sqlOfFind({ offset: 5 })).toMatch(orderedBy(cell, ['id', 'asc'])); + expect(await sqlOfFind({ orderBy: [], limit: 5 })).toMatch(orderedBy(cell, ['id', 'asc'])); + }); - it('still honors an orderBy the caller gave findOne, tie-breaker and all', async () => { - const sql = await driver.sqlOf(() => - driver.findOne('ticket', { object: 'ticket', orderBy: [{ field: 'status', order: 'desc' }] }, { - bypassTenantAudit: true, - }), - ); - expect(sql).toMatch(/order by .*`status` desc, .*`id` desc/i); - }); + it('emits no ORDER BY for an unpaged read with no orderBy', async () => { + expect(await sqlOfFind({})).not.toMatch(/order by/i); + expect(await sqlOfFind({ where: { status: 'open' } })).not.toMatch(/order by/i); + expect(await sqlOfFind({ orderBy: [] })).not.toMatch(/order by/i); + }); - it('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. It costs even - // more on the unsorted paged read: there is no requested sort to fall back - // to, so a wrong guess turns a reshuffle into a failed read. - expect(driver['paginationTieBreaker']('some_remote_table')).toBeNull(); - expect(driver['orderKeysFor']('some_remote_table', { object: 'some_remote_table', limit: 5, offset: 5 })).toEqual([]); - }); + it('leaves `findOne` unordered — its `limit: 1` is not page one of a walk', async () => { + // `findOne` promises *a* matching record, not a position in a sequence, so + // there is no partition to keep. Reading its `limit: 1` as a page would + // impose `ORDER BY id LIMIT 1` on the hottest read in the system, which is + // the shape that makes a planner abandon the predicate's own index: on a + // 2M-row Postgres table `WHERE owner_id = ? LIMIT 1` measured 0.08 ms + // unordered and 7.8 ms with `ORDER BY id`. `MongoDBDriver.findOne` has + // never sorted either — this keeps the two drivers saying the same thing. + const sql = await driver.sqlOf(CLAUSE_TABLE, () => + driver.findOne( + CLAUSE_TABLE, + { object: CLAUSE_TABLE, where: { status: 'open' } }, + { bypassTenantAudit: true }, + ), + ); + expect(sql).not.toMatch(/order by/i); + expect(sql).toMatch(/limit/i); + }); - it('says so, once, when it cannot keep the guarantee on a table it did not create', () => { - // Behavior is unchanged for these tables — the statement goes out exactly - // as before. What changes is that the gap is announced instead of being - // left for a user counting records to find, which is the same reason the - // rule exists at all. - const warn = vi.spyOn(driver['logger'], 'warn').mockImplementation(() => {}); - try { - driver['orderKeysFor']('some_remote_table', { object: 'some_remote_table', limit: 5, offset: 5 }); - driver['orderKeysFor']('some_remote_table', { object: 'some_remote_table', limit: 5, offset: 10 }); - expect(warn, 'once per object, not per query').toHaveBeenCalledTimes(1); - const message = warn.mock.calls[0]![0]; - expect(message, 'names the object').toContain('some_remote_table'); - expect(message, 'names the consequence').toMatch(/NOT deterministic/); - expect(message, 'names a remedy').toMatch(/orderBy/); - - // A managed table keeps the guarantee, so it must stay quiet. - driver['orderKeysFor']('ticket', { object: 'ticket', limit: 5, offset: 5 }); - // So must an unpaged read, and a sorted one: neither is the silent case. - driver['orderKeysFor']('some_remote_table', { object: 'some_remote_table' }); - driver['orderKeysFor']( - 'some_remote_table', - { object: 'some_remote_table', orderBy: [{ field: 'status', order: 'asc' }], limit: 5 }, + it('still honors an orderBy the caller gave findOne, tie-breaker and all', async () => { + const sql = await driver.sqlOf(CLAUSE_TABLE, () => + driver.findOne( + CLAUSE_TABLE, + { object: CLAUSE_TABLE, orderBy: [{ field: 'status', order: 'desc' }] }, + { bypassTenantAudit: true }, + ), ); - expect(warn).toHaveBeenCalledTimes(1); - } finally { - warn.mockRestore(); - } + expect(sql).toMatch(orderedBy(cell, ['status', 'desc'], ['id', 'desc'])); + }); + + 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. It costs even + // more on the unsorted paged read: there is no requested sort to fall back + // to, so a wrong guess turns a reshuffle into a failed read. + expect(driver['paginationTieBreaker']('some_remote_table')).toBeNull(); + expect( + driver['orderKeysFor']('some_remote_table', { + object: 'some_remote_table', + limit: 5, + offset: 5, + }), + ).toEqual([]); + }); + + it('says so, once, when it cannot keep the guarantee on a table it did not create', () => { + // Behavior is unchanged for these tables — the statement goes out exactly + // as before. What changes is that the gap is announced instead of being + // left for a user counting records to find, which is the same reason the + // rule exists at all. + // + // Its own object name, because "once" is counted per object for the LIFE + // of the driver and this sweep shares one driver across its tests (a live + // cell cannot afford a fresh connection per assertion). Reusing the name + // the test above already paged would measure that suite's leftovers. + const remote = 'warned_remote_table'; + const warn = vi.spyOn(driver['logger'], 'warn').mockImplementation(() => {}); + try { + driver['orderKeysFor'](remote, { object: remote, limit: 5, offset: 5 }); + driver['orderKeysFor'](remote, { object: remote, limit: 5, offset: 10 }); + expect(warn, 'once per object, not per query').toHaveBeenCalledTimes(1); + const message = warn.mock.calls[0]![0]; + expect(message, 'names the object').toContain(remote); + expect(message, 'names the consequence').toMatch(/NOT deterministic/); + expect(message, 'names a remedy').toMatch(/orderBy/); + + // A managed table keeps the guarantee, so it must stay quiet. + driver['orderKeysFor'](CLAUSE_TABLE, { object: CLAUSE_TABLE, limit: 5, offset: 5 }); + // So must an unpaged read, and a sorted one: neither is the silent case. + driver['orderKeysFor'](remote, { object: remote }); + driver['orderKeysFor'](remote, { + object: remote, + orderBy: [{ field: 'status', order: 'asc' }], + limit: 5, + }); + expect(warn).toHaveBeenCalledTimes(1); + } finally { + warn.mockRestore(); + } + }); }); -}); +} diff --git a/packages/plugins/driver-sql/src/sql-driver-temporal-conformance.test.ts b/packages/plugins/driver-sql/src/sql-driver-temporal-conformance.test.ts index 33dfef1fe2..8367916536 100644 --- a/packages/plugins/driver-sql/src/sql-driver-temporal-conformance.test.ts +++ b/packages/plugins/driver-sql/src/sql-driver-temporal-conformance.test.ts @@ -78,8 +78,8 @@ import { SqlDriver } from '../src/index.js'; import { LegacyStorageDriver } from './legacy-datetime-storage.testkit.js'; import { DIALECT_CELLS, - EXPECT_LIVE_DIALECTS, assertThreeWayZoneSkew, + declareUnprovisionedCell, readServerZone, type DialectCell, type ServerZone, @@ -133,7 +133,11 @@ async function dropTable(driver: SqlDriver | undefined, table: string): Promise< for (const cell of DIALECT_CELLS) { if (!cell.available) { - declareUnprovisionedCell(cell); + // Reported, never omitted — a named skip locally, a red under + // `OS_EXPECT_LIVE_DIALECT_MATRIX=1`. The guard is the testkit's (shared with + // the pagination and filter-logic matrices, #4714) so it cannot weaken in + // one consumer's copy alone. + declareUnprovisionedCell(cell, 'temporal conformance'); continue; } if (cell.live) declareServerTimezoneAxis(cell); @@ -143,27 +147,6 @@ for (const cell of DIALECT_CELLS) { declareLegacyTimeSweep(cell); } -/** - * A cell nobody provisioned is REPORTED, not omitted: a named skip locally, a - * failure under `OS_EXPECT_LIVE_DIALECT_MATRIX=1` — which is what stops the CI - * job from quietly degrading to SQLite-only coverage if its `env:` block is - * ever dropped. - */ -function declareUnprovisionedCell(cell: DialectCell): void { - describe(`sql-driver — temporal conformance matrix (${cell.label})`, () => { - it.skipIf(!EXPECT_LIVE_DIALECTS)( - `is provisioned — set ${cell.env} to run this cell of the D-A3 driver axis`, - () => { - expect.fail( - `${cell.env} is unset while OS_EXPECT_LIVE_DIALECT_MATRIX=1: this runner declared it ` + - `provisions live Postgres and MySQL, so the ${cell.label} cell of the temporal ` + - `conformance matrix must not be skipped (ADR-0053 D-A3 "Postgres at minimum").`, - ); - }, - ); - }); -} - /** * The server-timezone axis (D-B3), asserted rather than assumed: three clocks — * the server's, the process's, and UTC — must be pairwise different, or every