diff --git a/.changeset/driver-sql-covering-pk-membership.md b/.changeset/driver-sql-covering-pk-membership.md new file mode 100644 index 0000000000..2199a529c0 --- /dev/null +++ b/.changeset/driver-sql-covering-pk-membership.md @@ -0,0 +1,9 @@ +--- +"@objectstack/driver-sql": patch +--- + +**Bug fix:** on Postgres, `introspectPrimaryKeys` no longer reports a covering primary key's `INCLUDE`'d columns as key members (#11162). + +For a primary key created as `CREATE UNIQUE INDEX … INCLUDE (payload)` and promoted with `ALTER TABLE … ADD CONSTRAINT … PRIMARY KEY USING INDEX`, `pg_index.indkey` holds the key columns *and* the payload columns; `indnkeyatts` counts the leading entries that are actually key members and was never consulted, so `payload` came back as part of the key. Measured on a live PostgreSQL 16.13: `indkey = '2 1 3'`, `indnkeyatts = 2`, and the introspected key was `k2, k1, payload` for a declared `(k2, k1)`. + +A key with an extra member is a different key: an upsert conflict target naming a non-key column does not match the constraint, and schema-drift comparison against a correctly-declared key reports a phantom `unexpected_key_member`. The join is now bounded with `k.ord <= i.indnkeyatts`, which preserves the declared key order established by #11101. `indnkeyatts` exists on PG 11+; no change for ordinary (non-covering) primary keys. diff --git a/.changeset/driver-sql-declared-column-order.md b/.changeset/driver-sql-declared-column-order.md new file mode 100644 index 0000000000..0440b997e8 --- /dev/null +++ b/.changeset/driver-sql-declared-column-order.md @@ -0,0 +1,9 @@ +--- +"@objectstack/driver-sql": patch +--- + +**Bug fix:** `introspectColumns` (and therefore `introspectSchema`) now reports a table's columns in declared order on every dialect, read from the catalog's own ordinal (#11163). + +The column array was built from knex's `columnInfo()`, an object keyed by column name whose key-insertion order is the row order of a catalog query with no `ORDER BY`. Measured live: SQLite and PostgreSQL 16.13 happened to return declared order, MySQL 8.0.46 returned **alphabetical** order — so the same table introspected through different dialects returned different `columns` arrays, and a federated object drafted from a MySQL remote (ADR-0015) got its fields alphabetized rather than in the order the remote declares them. + +The order now comes from the catalog ordinal on all three dialects — `information_schema.COLUMNS.ORDINAL_POSITION` (MySQL), `information_schema.columns.ordinal_position` (Postgres), `PRAGMA table_info`'s `cid` (SQLite) — while `columnInfo()` remains the source of the per-column facts (`type`, `nullable`, `defaultValue`, `maxLength`), which knex already normalises per dialect. diff --git a/.changeset/driver-sql-introspection-error-contract.md b/.changeset/driver-sql-introspection-error-contract.md new file mode 100644 index 0000000000..dae5fc3932 --- /dev/null +++ b/.changeset/driver-sql-introspection-error-contract.md @@ -0,0 +1,13 @@ +--- +"@objectstack/driver-sql": minor +--- + +**BREAKING**: a failed primary-key / foreign-key / unique-constraint introspection read now throws instead of silently reporting absence (#11161). + +`introspectPrimaryKeys`, `introspectForeignKeys` and `introspectUniqueConstraints` wrapped their whole dialect dispatch in a bare `catch {}` and returned `[]`, so a query a live server rejected degraded to "this table has no primary key / foreign keys / unique constraints" with no diagnostic. `primaryKeys` is consumed as an addressing / upsert-conflict-target key (federated-object codegen, the persisted `external_catalog` under ADR-0015, schema-drift comparison), so the silent empty answer was a wrong answer downstream code acted on, not "we don't know". + +This extends the #7332 ruling the sibling `introspectIndexes` already carries, with the identical option shape and default: `onFailure?: 'throw' | 'partial'`, defaulting to `'throw'`. A caller whose short read is self-correcting may ask for one by name with `{ onFailure: 'partial' }`. Consequently `introspectSchema` over a partially-readable database now fails loudly instead of emitting tables whose keys silently read as absent; its in-tree callers already handle a throw (the datasource health check reports `{ ok: false }`, the REST/CLI introspection seams surface the error). + +The un-hiding immediately proved its worth: the Postgres arm of `introspectUniqueConstraints` had been invalid SQL all along (`SELECT c.column_name` with no alias `c` in scope — `missing FROM-clause entry`), so live Postgres never reported a unique constraint through this method. That query is repaired in the same change (alias fixed, and the lookup scoped to `current_schemas(false)` the way `introspectSchema`'s own table listing already is), so `isUnique` is now populated on Postgres for the first time. + + diff --git a/packages/drivers/driver-sql/src/sql-driver-column-order-dialects.test.ts b/packages/drivers/driver-sql/src/sql-driver-column-order-dialects.test.ts new file mode 100644 index 0000000000..01c2b6f642 --- /dev/null +++ b/packages/drivers/driver-sql/src/sql-driver-column-order-dialects.test.ts @@ -0,0 +1,209 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#11163] `introspectColumns` must report a table's columns in DECLARED + * order on **every** dialect — SQLite, Postgres and MySQL — for the same + * table. + * + * The method built its array from knex's `columnInfo()`, an object KEYED BY + * COLUMN NAME whose key-insertion order is the row order of knex's own + * `information_schema.columns` query — which carries no `ORDER BY`. Measured + * live: SQLite and PostgreSQL 16.13 happened to return declared order; MySQL + * 8.0.46 returned **alphabetical** order, so the same table introspected + * through different dialects returned different `columns` arrays, and a + * federated object drafted from a MySQL remote (ADR-0015 + * `generateObjectDraft` / the persisted `external_catalog`) got its fields + * alphabetized rather than in the order the remote declares them. + * + * The fix reads the order from the catalog's own ordinal + * (`ORDINAL_POSITION` / `ordinal_position` / `PRAGMA table_info`'s `cid`) — + * the ordinal is the fact; a plan's row order is not, on ANY dialect. + * + * ## ⭐ Why the fixtures' alphabetical order differs from their declared order + * + * Alphabetical order is exactly what the buggy path returned on MySQL, so a + * fixture whose declared order IS alphabetical would make every assertion + * below a tautology the buggy code also passes. {@link TWO_KEY_TABLE} reuses + * #11101's permutation shape (`carrier_code, shipment_id, leg_seq` — its + * alphabetical order swaps the last two), and {@link Z_FIRST_TABLE} differs + * in the FIRST position too, so an arm that merely happened to agree on the + * leading column cannot pass by accident. The `non-vacuous` leg pins both + * constants against their own DDL text. + * + * ## How the three dialects are held to ONE answer + * + * Same construction as the #11101 key-order file: every cell runs the same + * DDL and asserts the same constants, through `declareDialectCell` — live + * cells are a named skip without `OS_TEST_POSTGRES_URL` / + * `OS_TEST_MYSQL_URL`, and a red under `OS_EXPECT_LIVE_DIALECT_MATRIX=1`. + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { SqlDriver } from '../src/index.js'; +import { DIALECT_CELLS, declareDialectCell, type DialectCell } from './live-dialect-matrix.testkit.js'; + +const MATRIX = 'declared COLUMN order'; + +/** Tables this file owns. The schema they land in is per-file (#9350). */ +const TWO_KEY_TABLE = 'os11163_shipment_legs'; +const Z_FIRST_TABLE = 'os11163_zone_areas'; + +/** + * #11101's fixture shape, reused deliberately: declared order + * `carrier_code, shipment_id, leg_seq`, alphabetical order + * `carrier_code, leg_seq, shipment_id` — a real permutation. + */ +const TWO_KEY_DDL = `create table ${TWO_KEY_TABLE} ( + carrier_code varchar(64) not null, + shipment_id varchar(64) not null, + leg_seq integer, + primary key (shipment_id, carrier_code) +)`; + +const TWO_KEY_COLUMN_ORDER = ['carrier_code', 'shipment_id', 'leg_seq']; + +/** + * Alphabetical differs in the FIRST position: `zone_code` is declared first + * and sorts last. + */ +const Z_FIRST_DDL = `create table ${Z_FIRST_TABLE} ( + zone_code varchar(64) not null, + area_code varchar(64) not null, + seq integer +)`; + +const Z_FIRST_COLUMN_ORDER = ['zone_code', 'area_code', 'seq']; + +/** Exact ordered array, with the alphabetical degradation named. */ +function expectDeclaredColumnOrder(actual: string[], declared: string[], cell: DialectCell): void { + expect( + actual, + `${cell.label}: introspected columns must be in DECLARED order — alphabetical is the ` + + `#11163 defect (knex columnInfo() key order), and any other order is a plan's accident`, + ).toEqual(declared); +} + +function declareColumnOrderSuite(cell: DialectCell): void { + describe(`introspectColumns declared order — ${cell.label} (#11163)`, () => { + let driver: SqlDriver; + + beforeEach(async () => { + driver = new SqlDriver(cell.config()); + for (const t of [TWO_KEY_TABLE, Z_FIRST_TABLE]) { + await driver.execute(`drop table if exists ${t}`).catch(() => {}); + } + await driver.execute(TWO_KEY_DDL); + await driver.execute(Z_FIRST_DDL); + }); + + afterEach(async () => { + for (const t of [TWO_KEY_TABLE, Z_FIRST_TABLE]) { + await driver.execute(`drop table if exists ${t}`).catch(() => {}); + } + await driver.disconnect(); + }); + + it('asserts the fixtures are non-vacuous: declared order differs from alphabetical order', async () => { + for (const [ddl, declared] of [ + [TWO_KEY_DDL, TWO_KEY_COLUMN_ORDER], + [Z_FIRST_DDL, Z_FIRST_COLUMN_ORDER], + ] as const) { + // The constant really is the order the DDL declares — read off the + // fixture's own text so it cannot quietly stop describing its table. + const declaredAt = declared.map((c) => ddl.indexOf(`\n ${c} `)); + expect(declaredAt.every((at) => at > 0)).toBe(true); + expect([...declaredAt].sort((x, y) => x - y)).toEqual(declaredAt); + + // Alphabetical ≠ declared: the whole premise. Without this, every + // assertion below is a tautology the buggy code also passed. + expect([...declared].sort()).not.toEqual(declared); + } + // And the z-first fixture disagrees in the FIRST position specifically. + expect([...Z_FIRST_COLUMN_ORDER].sort()[0]).not.toBe(Z_FIRST_COLUMN_ORDER[0]); + }); + + it('reports columns in declared order, not alphabetical order', async () => { + const schema = await driver.introspectSchema(); + + expectDeclaredColumnOrder( + schema.tables[TWO_KEY_TABLE].columns.map((c) => c.name), + TWO_KEY_COLUMN_ORDER, + cell, + ); + expectDeclaredColumnOrder( + schema.tables[Z_FIRST_TABLE].columns.map((c) => c.name), + Z_FIRST_COLUMN_ORDER, + cell, + ); + }); + + it('keeps every per-column fact paired with its column across the reorder', async () => { + const schema = await driver.introspectSchema(); + const byName = Object.fromEntries( + schema.tables[TWO_KEY_TABLE].columns.map((c) => [c.name, c]), + ); + + // The facts still come from knex's columnInfo(); the reorder must not + // detach them from their names. nullable is the one fact every dialect + // spells the same way through knex's normalisation. + expect(byName.carrier_code.nullable).toBe(false); + expect(byName.shipment_id.nullable).toBe(false); + expect(byName.leg_seq.nullable).toBe(true); + // And the key flags derived downstream still land on the key columns. + expect(byName.carrier_code.primaryKey).toBe(true); + expect(byName.shipment_id.primaryKey).toBe(true); + expect(byName.leg_seq.primaryKey).toBe(false); + }); + }); +} + +for (const cell of DIALECT_CELLS) { + declareDialectCell(cell, MATRIX, declareColumnOrderSuite); +} + +/** + * The catalog fact each rewritten arm rests on, pinned per live dialect: the + * ordinal is the DECLARED position. (The alphabetical row order the unordered + * query happened to return is deliberately NOT pinned — it is unspecified by + * both engines; the measured pre-fix output is recorded in the PR body + * instead, exactly as the #11101 key-order file does for its defect.) + */ +function declareCatalogPins(cell: DialectCell): void { + if (cell.id === 'sqlite') return; // `cid` ordinality is pinned by the #10997 composite-key file's PRAGMA pin + + describe(`introspectColumns catalog facts — ${cell.label} (#11163)`, () => { + let driver: SqlDriver; + + beforeEach(async () => { + driver = new SqlDriver(cell.config()); + await driver.execute(`drop table if exists ${TWO_KEY_TABLE}`).catch(() => {}); + await driver.execute(TWO_KEY_DDL); + }); + + afterEach(async () => { + await driver.execute(`drop table if exists ${TWO_KEY_TABLE}`).catch(() => {}); + await driver.disconnect(); + }); + + it('the catalog ordinal is the declared column position', async () => { + const sql = + cell.id === 'pg' + ? `select column_name, ordinal_position from information_schema.columns + where table_name = '${TWO_KEY_TABLE}' + and table_catalog = current_database() and table_schema = current_schema()` + : `select COLUMN_NAME as column_name, ORDINAL_POSITION as ordinal_position + from information_schema.COLUMNS + where TABLE_SCHEMA = DATABASE() and TABLE_NAME = '${TWO_KEY_TABLE}'`; + const res: any = await driver.execute(sql); + const rows: any[] = cell.id === 'pg' ? res.rows : res[0]; + const ordinalByName = Object.fromEntries( + rows.map((r: any) => [r.column_name, Number(r.ordinal_position)]), + ); + expect(ordinalByName).toEqual({ carrier_code: 1, shipment_id: 2, leg_seq: 3 }); + }); + }); +} + +for (const cell of DIALECT_CELLS) { + declareDialectCell(cell, `${MATRIX} catalog facts`, declareCatalogPins); +} diff --git a/packages/drivers/driver-sql/src/sql-driver-covering-primary-key-membership.test.ts b/packages/drivers/driver-sql/src/sql-driver-covering-primary-key-membership.test.ts new file mode 100644 index 0000000000..05cf07264a --- /dev/null +++ b/packages/drivers/driver-sql/src/sql-driver-covering-primary-key-membership.test.ts @@ -0,0 +1,121 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#11162] A covering primary key's INCLUDE'd columns are NOT key members. + * + * Postgres reaches a covering primary key via + * `CREATE UNIQUE INDEX … INCLUDE (payload)` promoted with + * `ALTER TABLE … ADD CONSTRAINT … PRIMARY KEY USING INDEX`. For such an index + * `pg_index.indkey` holds the key columns *and* the INCLUDE'd payload columns; + * `indnkeyatts` is the count of the leading entries that are actually key + * members. `introspectPrimaryKeys` read `indkey` whole and never consulted + * `indnkeyatts`, so `payload` was reported as part of the key. + * + * A key with an extra member is a DIFFERENT key: an upsert conflict target + * naming a non-key column does not match the constraint, and schema-drift + * comparison against a correctly-declared `(k2, k1)` reports a phantom + * `unexpected_key_member:payload`. Measured on a live PostgreSQL 16.13: + * `indkey = '2 1 3'`, `indnkeyatts = 2`, and both the pre-#11101 and + * post-#11101 queries returned `payload` (#11101 repaired ORDER, not + * membership — the two arms agreed on the wrong membership). + * + * ## Why this file is PG-only + * + * MySQL has no covering-index concept for a PRIMARY KEY and SQLite has no + * INCLUDE at all — the defect is not expressible there, so the cell list is + * exactly `pg`, declared through `declareDialectCell` so an unprovisioned run + * is a named skip (and a red under `OS_EXPECT_LIVE_DIALECT_MATRIX=1`), never + * a silent pass. + * + * ## Why the assertion is the EXACT ORDERED array + * + * Two reasons. Membership alone would pass a fix that broke #11101's ordering + * repair — the fixture's key `(k2, k1)` is deliberately declared out of column + * sequence so ordering stays observable, and the exact array holds both + * properties at once. And a set/length assertion could go green over the + * method's failure modes; the exact array cannot. + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { SqlDriver } from '../src/index.js'; +import { PG_CELL, declareDialectCell, type DialectCell } from './live-dialect-matrix.testkit.js'; + +const MATRIX = 'covering primary-key MEMBERSHIP'; + +/** Table this file owns. The schema it lands in is per-file (#9350). */ +const TABLE = 'os11162_covered'; + +/** + * Column order is `k1, k2, payload`; the KEY is `(k2, k1)` — out of column + * sequence on purpose, so this fixture can see an ordering regression too. + * `payload` is carried by the index but is NOT a key member. + */ +const DDL = [ + `create table ${TABLE} (k1 varchar(64) not null, k2 varchar(64) not null, payload varchar(64))`, + `create unique index ${TABLE}_pk on ${TABLE} (k2, k1) include (payload)`, + `alter table ${TABLE} add constraint ${TABLE}_pkey primary key using index ${TABLE}_pk`, +]; + +/** The declared key: exactly the two key columns, in declared key order. */ +const KEY_ORDER = ['k2', 'k1']; + +function declareCoveringKeySuite(cell: DialectCell): void { + describe(`introspectPrimaryKeys covering-key membership — ${cell.label} (#11162)`, () => { + let driver: SqlDriver; + + beforeEach(async () => { + driver = new SqlDriver(cell.config()); + await driver.execute(`drop table if exists ${TABLE}`).catch(() => {}); + for (const stmt of DDL) await driver.execute(stmt); + }); + + afterEach(async () => { + await driver.execute(`drop table if exists ${TABLE}`).catch(() => {}); + await driver.disconnect(); + }); + + it('pins the catalog facts the fix rests on: indkey carries payload, indnkeyatts bounds the key', async () => { + const res: any = await driver.execute( + `select i.indkey::text as indkey, i.indnatts, i.indnkeyatts + from pg_index i + where i.indrelid = '${TABLE}'::regclass and i.indisprimary`, + ); + const row = res.rows[0]; + // k1 is attnum 1, k2 attnum 2, payload attnum 3: the key columns in KEY + // order, then the INCLUDE'd column. If a server ever stopped reporting + // this shape, the arm would be wrong for a reason no assertion on its + // OUTPUT could localise. + expect(row.indkey).toBe('2 1 3'); + expect(Number(row.indnatts)).toBe(3); + expect(Number(row.indnkeyatts)).toBe(2); + }); + + it('reports ONLY the key columns, in declared key order — INCLUDE columns are not members', async () => { + const schema = await driver.introspectSchema(); + const introspected = schema.tables[TABLE].primaryKeys; + + // Exact ordered array: membership (#11162) and order (#11101) at once. + expect( + introspected, + `${cell.label}: a covering PK must report its key columns only — ` + + `'payload' is an INCLUDE'd column, and reporting it makes this a DIFFERENT addressing key`, + ).toEqual(KEY_ORDER); + + // The pre-fix answer, named: what both the pre- and post-#11101 queries + // returned on a live 16.13 before this bound existed. + expect(introspected).not.toEqual(['k2', 'k1', 'payload']); + }); + + it('derives the per-column primaryKey flag from the bounded membership', async () => { + const schema = await driver.introspectSchema(); + const flags = Object.fromEntries( + schema.tables[TABLE].columns.map((c) => [c.name, c.primaryKey === true]), + ); + // `introspectSchema` derives this FROM `primaryKeys`, so the phantom + // member corrupted this signal too. + expect(flags).toEqual({ k1: true, k2: true, payload: false }); + }); + }); +} + +declareDialectCell(PG_CELL, MATRIX, declareCoveringKeySuite); diff --git a/packages/drivers/driver-sql/src/sql-driver-introspection-error-contract.test.ts b/packages/drivers/driver-sql/src/sql-driver-introspection-error-contract.test.ts new file mode 100644 index 0000000000..20c15a6587 --- /dev/null +++ b/packages/drivers/driver-sql/src/sql-driver-introspection-error-contract.test.ts @@ -0,0 +1,149 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#11161] A failed introspection read must surface as a THROW, never as an + * empty collection. + * + * `introspectPrimaryKeys`, `introspectForeignKeys` and + * `introspectUniqueConstraints` used to wrap their whole dialect dispatch in a + * bare `catch {}` and return `[]` — so a query a live server rejects degraded + * to "this table has no primary key / foreign keys / unique constraints", with + * no diagnostic. `primaryKeys` is consumed as an addressing / + * upsert-conflict-target key (federated-object codegen, the persisted + * `external_catalog` under ADR-0015, schema-drift comparison), so the silent + * `[]` was a *wrong answer downstream code acts on*, not "we don't know". + * + * #7332 already ruled this exact question for the sibling method + * `introspectIndexes`: `onFailure?: 'throw' | 'partial'`, **defaulting to + * `'throw'`** — only a caller that can CORRECT a short read may ask for one by + * name. This file pins that the three siblings now carry the same contract. + * + * ## ⛔ Why every failure leg asserts a REJECTION, not "does not throw" + * + * The defect's exact shape was a resolved `[]` over a failed read — a test + * asserting "does not throw" is the defect's own green. Each leg therefore + * asserts the promise REJECTS and that the rejection carries the underlying + * error (the knex pool refusal, or Postgres' `undefined_table` with its + * SQLSTATE), plus the positive half: `{ onFailure: 'partial' }` still resolves, + * because the opt-in — not the default — is where a short read is legal. + * + * ## How the failure is manufactured + * + * - **Every cell** (SQLite embedded + live PG/MySQL): destroy the connection + * pool, then introspect. The read cannot happen; under the old catch each + * method resolved `[]` anyway. + * - **Postgres additionally**: introspect a non-existent relation. + * `?::regclass` raises `undefined_table` (SQLSTATE 42P01) — the exact + * measured evidence from #11101's development, where an invalid spelling of + * the rewritten `pg_index` query produced no error, no log and an empty key. + * (The FK/unique arms are parameterized `information_schema` reads, so a + * missing table is legitimately zero rows there — only the pool sabotage can + * exercise their catch on a live server.) + */ + +import { describe, it, expect, afterEach } from 'vitest'; +import { SqlDriver } from '../src/index.js'; +import { DIALECT_CELLS, declareDialectCell, type DialectCell } from './live-dialect-matrix.testkit.js'; + +const MATRIX = 'introspection error contract'; + +/** A table created per cell so the healthy-read legs have something real. */ +const TABLE = 'os11161_err_contract'; + +/** knex's tarn pool refusal after `destroy()` — dialect-independent. */ +const POOL_REFUSAL = /Unable to acquire a connection/; + +/** The three siblings, exposed for direct pinning (they are `protected`). */ +class IntrospectionProbeDriver extends SqlDriver { + primaryKeys(table: string, opts?: { onFailure?: 'throw' | 'partial' }) { + return this.introspectPrimaryKeys(table, opts); + } + foreignKeys(table: string, opts?: { onFailure?: 'throw' | 'partial' }) { + return this.introspectForeignKeys(table, opts); + } + uniqueConstraints(table: string, opts?: { onFailure?: 'throw' | 'partial' }) { + return this.introspectUniqueConstraints(table, opts); + } +} + +function declareErrorContractSuite(cell: DialectCell): void { + describe(`introspection error contract — ${cell.label} (#11161)`, () => { + let driver: IntrospectionProbeDriver; + + afterEach(async () => { + // The sabotage legs already destroyed the pool; a second destroy is a + // no-op, so this is safe either way. + await driver.disconnect().catch(() => {}); + }); + + it('a read the server cannot answer REJECTS by default — for all three siblings', async () => { + driver = new IntrospectionProbeDriver(cell.config()); + await driver.execute(`drop table if exists ${TABLE}`).catch(() => {}); + await driver.execute( + `create table ${TABLE} (id varchar(64) not null, label varchar(64), primary key (id))`, + ); + // Sabotage: destroy the pool so the next read fails before any row + // arrives. Under the old `catch {}` every method below resolved `[]`. + await driver.disconnect(); + + await expect(driver.primaryKeys(TABLE)).rejects.toThrow(POOL_REFUSAL); + await expect(driver.foreignKeys(TABLE)).rejects.toThrow(POOL_REFUSAL); + await expect(driver.uniqueConstraints(TABLE)).rejects.toThrow(POOL_REFUSAL); + }); + + it("`onFailure: 'partial'` is the opt-in: a failed read resolves to what was read before it", async () => { + driver = new IntrospectionProbeDriver(cell.config()); + await driver.disconnect(); + + // Nothing was read before the failure, so the partial answer is empty — + // but it RESOLVES, by the caller's explicit request (#7332's shape). + await expect(driver.primaryKeys(TABLE, { onFailure: 'partial' })).resolves.toEqual([]); + await expect(driver.foreignKeys(TABLE, { onFailure: 'partial' })).resolves.toEqual([]); + await expect(driver.uniqueConstraints(TABLE, { onFailure: 'partial' })).resolves.toEqual([]); + }); + + it("`onFailure: 'partial'` on a HEALTHY read returns the full answer — partial is not 'empty'", async () => { + driver = new IntrospectionProbeDriver(cell.config()); + await driver.execute(`drop table if exists ${TABLE}`).catch(() => {}); + await driver.execute( + `create table ${TABLE} (id varchar(64) not null, label varchar(64), primary key (id))`, + ); + + await expect(driver.primaryKeys(TABLE, { onFailure: 'partial' })).resolves.toEqual(['id']); + + await driver.execute(`drop table ${TABLE}`); + }); + + if (cell.id === 'pg') { + it('a server-side query rejection surfaces with its SQLSTATE — the measured undefined_table case', async () => { + driver = new IntrospectionProbeDriver(cell.config()); + + // `?::regclass` raises `undefined_table` for a relation that does not + // exist (measured on PostgreSQL 16.13 — the evidence #11161 was filed + // with). The old catch converted it to a confident empty key. + const outcome = await driver.primaryKeys('os11161_no_such_relation').then( + (value) => ({ resolved: true as const, value }), + (error: unknown) => ({ resolved: false as const, error }), + ); + expect( + outcome.resolved, + 'introspectPrimaryKeys resolved ' + + (outcome.resolved ? JSON.stringify(outcome.value) : '') + + ' over a rejected query — the #11161 defect shape: a failed read reported as a ' + + 'table without a primary key.', + ).toBe(false); + if (!outcome.resolved) { + // The underlying Postgres error, undecorated: SQLSTATE on `code`, + // the relation named in the message. This is what #7332's 'throw' + // default exists to deliver to the caller. + expect((outcome.error as { code?: string }).code).toBe('42P01'); + expect(String((outcome.error as Error).message)).toMatch(/os11161_no_such_relation/); + } + }); + } + }); +} + +for (const cell of DIALECT_CELLS) { + declareDialectCell(cell, MATRIX, declareErrorContractSuite); +} diff --git a/packages/drivers/driver-sql/src/sql-driver.ts b/packages/drivers/driver-sql/src/sql-driver.ts index e0e03aa885..ecc75e8432 100644 --- a/packages/drivers/driver-sql/src/sql-driver.ts +++ b/packages/drivers/driver-sql/src/sql-driver.ts @@ -10141,6 +10141,13 @@ export class SqlDriver implements IDataDriver { } for (const tableName of tableNames) { + // All four reads take the #7332 default (`onFailure: 'throw'`): this + // caller cannot correct a short read, and `introspectColumns` has never + // swallowed — so a partially-readable database fails the whole-schema + // introspection loudly instead of emitting tables whose keys silently + // read as absent (#11161). Every in-tree caller of `introspectSchema` + // already handles a throw (the datasource health check reports + // `{ ok: false }`, the REST/CLI seams surface the error). const columns = await this.introspectColumns(tableName); const foreignKeys = await this.introspectForeignKeys(tableName); const primaryKeys = await this.introspectPrimaryKeys(tableName); @@ -12843,11 +12850,80 @@ export class SqlDriver implements IDataDriver { // ── Introspection internals ───────────────────────────────────────────────── + /** + * The table's column names in DECLARED order, read from the catalog's own + * ordinal (#11163). + * + * knex's `columnInfo()` is an object KEYED BY COLUMN NAME, built from a + * catalog query that carries no `ORDER BY` — so its key-insertion order is + * whatever row order the server's plan yielded, which is unspecified on + * every dialect. Measured: SQLite and PostgreSQL 16.13 happened to return + * declared order; MySQL 8.0.46 returned ALPHABETICAL order, so a federated + * object drafted from a MySQL remote got its fields alphabetized. The + * ordinal is the fact and the row order is not, so every arm orders by the + * catalog's ordinal rather than trusting a plan. + * + * Each arm reads the same catalog `columnInfo()` populates, with the same + * scoping knex itself applies there (PG: `current_schema()`; MySQL: + * `DATABASE()`; SQLite: `PRAGMA table_info`, whose `cid` is the ordinal). + */ + protected async introspectColumnOrder(tableName: string): Promise { + if (this.isPostgres) { + const result = await this.knex.raw( + `SELECT column_name + FROM information_schema.columns + WHERE table_name = ? + AND table_catalog = current_database() + AND table_schema = current_schema() + ORDER BY ordinal_position`, + [tableName], + ); + return result.rows.map((row: any) => row.column_name); + } + if (this.isMysql) { + const result = await this.knex.raw( + `SELECT COLUMN_NAME as column_name + FROM information_schema.COLUMNS + WHERE TABLE_SCHEMA = DATABASE() + AND TABLE_NAME = ? + ORDER BY ORDINAL_POSITION`, + [tableName], + ); + return result[0].map((row: any) => row.column_name); + } + if (this.isSqlite) { + const safeTableName = tableName.replace(/[^a-zA-Z0-9_]/g, ''); + const result = await this.knex.raw(`PRAGMA table_info(${safeTableName})`); + return (result as { name: string; cid: number }[]) + .slice() + .sort((a, b) => a.cid - b.cid) + .map((row) => row.name); + } + return []; + } + protected async introspectColumns(tableName: string): Promise { - const columnInfo = await this.knex(tableName).columnInfo(); + const columnInfo = (await this.knex(tableName).columnInfo()) as Record; const columns: IntrospectedColumn[] = []; - for (const [colName, info] of Object.entries(columnInfo)) { + // #11163: emit in DECLARED order, not the `columnInfo()` object's + // key-insertion order (alphabetical on MySQL — see + // {@link introspectColumnOrder}). `columnInfo()` stays the source of the + // per-column FACTS because each dialect spells type / nullable / + // defaultValue / maxLength differently and knex already normalises them; + // only the ORDER comes from the ordinal read. A name one read has and the + // other lacks (the catalog moved between the two statements) is appended + // in `columnInfo()` order rather than dropped, so the merge can reorder + // but never lose a column. + const has = (name: string) => Object.prototype.hasOwnProperty.call(columnInfo, name); + const orderedNames = (await this.introspectColumnOrder(tableName)).filter(has); + const seen = new Set(orderedNames); + for (const name of Object.keys(columnInfo)) { + if (!seen.has(name)) orderedNames.push(name); + } + + for (const colName of orderedNames) { + const info = columnInfo[colName]; let type = 'string'; let maxLength: number | undefined; @@ -12877,7 +12953,26 @@ export class SqlDriver implements IDataDriver { return columns; } - protected async introspectForeignKeys(tableName: string): Promise { + /** + * ⚠️ A failed read is an ERROR, not a table without foreign keys (#7332 — + * the ruling {@link introspectIndexes} carries, extended to this sibling by + * #11161). This used to wrap the whole dialect dispatch in a bare `catch {}` + * and return `[]`, so a query a live server rejects degraded to "this table + * has no foreign keys" with no diagnostic — a positive assertion of absence + * that downstream consumers act on. See {@link introspectIndexes} for the + * full rationale; the option shape and default are deliberately identical. + */ + protected async introspectForeignKeys( + tableName: string, + opts: { + /** + * What a failed read means to THIS caller (#7332). `'throw'` (the + * default) surfaces it; `'partial'` returns whatever was read before the + * failure — correct only where a short read is self-correcting. + */ + onFailure?: 'throw' | 'partial'; + } = {}, + ): Promise { const foreignKeys: IntrospectedForeignKey[] = []; try { @@ -12956,14 +13051,38 @@ export class SqlDriver implements IDataDriver { }); } } - } catch { - // silently ignore introspection errors + } catch (e) { + // Only a caller that can CORRECT a short read may ask for one (#7332). + if (opts.onFailure !== 'partial') throw e; } return foreignKeys; } - protected async introspectPrimaryKeys(tableName: string): Promise { + /** + * ⚠️ A failed read is an ERROR, not a table without a primary key (#7332 — + * the ruling {@link introspectIndexes} carries, extended to this sibling by + * #11161). This used to wrap the whole dialect dispatch in a bare `catch {}` + * and return `[]` — and `[]` does not read downstream as "the read failed": + * it reads as *this table has no primary key*, a legal and meaningful answer + * that federated-object codegen, the persisted `external_catalog` + * (ADR-0015) and schema-drift comparison all act on. Measured on a live + * PostgreSQL 16.13: a query against a non-existent relation raises + * `undefined_table`, which the old catch converted to a confident empty key. + * See {@link introspectIndexes} for the full rationale; the option shape and + * default are deliberately identical. + */ + protected async introspectPrimaryKeys( + tableName: string, + opts: { + /** + * What a failed read means to THIS caller (#7332). `'throw'` (the + * default) surfaces it; `'partial'` returns whatever was read before the + * failure — correct only where a short read is self-correcting. + */ + onFailure?: 'throw' | 'partial'; + } = {}, + ): Promise { const primaryKeys: string[] = []; try { @@ -12983,6 +13102,18 @@ export class SqlDriver implements IDataDriver { // whenever a key is declared out of column sequence, and this list is // used as an addressing / upsert-conflict-target key, where the order is // load-bearing: a key in the wrong order is a DIFFERENT key. + // + // `k.ord <= i.indnkeyatts` bounds the join to the KEY columns (#11162). + // For a covering primary key — `CREATE UNIQUE INDEX … INCLUDE (payload)` + // promoted via `ADD CONSTRAINT … PRIMARY KEY USING INDEX` — `indkey` + // holds the key columns AND the INCLUDE'd payload columns, and + // `indnkeyatts` is the count of the leading entries that are actually + // key members. Measured on PostgreSQL 16.13: `indkey = '2 1 3'`, + // `indnkeyatts = 2`, and without the bound `payload` was reported as a + // key member — a key with an extra member is a DIFFERENT key, for the + // same addressing reasons as above. `indnkeyatts` exists on PG 11+; + // this driver already requires 9.4+ syntax (`WITH ORDINALITY`) and CI + // runs 16. const result = await this.knex.raw( ` SELECT a.attname as column_name @@ -12993,6 +13124,7 @@ export class SqlDriver implements IDataDriver { AND a.attnum = k.attnum WHERE i.indrelid = ?::regclass AND i.indisprimary + AND k.ord <= i.indnkeyatts ORDER BY k.ord `, [tableName], @@ -13056,27 +13188,60 @@ export class SqlDriver implements IDataDriver { primaryKeys.push(row.name); } } - } catch { - // silently ignore + } catch (e) { + // Only a caller that can CORRECT a short read may ask for one (#7332). + if (opts.onFailure !== 'partial') throw e; } return primaryKeys; } - protected async introspectUniqueConstraints(tableName: string): Promise { + /** + * ⚠️ A failed read is an ERROR, not a table without unique constraints + * (#7332 — the ruling {@link introspectIndexes} carries, extended to this + * sibling by #11161). This used to wrap the whole dialect dispatch in a bare + * `catch {}` and return `[]`, silently converting a failed read into a + * positive assertion of absence. See {@link introspectIndexes} for the full + * rationale; the option shape and default are deliberately identical. + */ + protected async introspectUniqueConstraints( + tableName: string, + opts: { + /** + * What a failed read means to THIS caller (#7332). `'throw'` (the + * default) surfaces it; `'partial'` returns whatever was read before the + * failure — correct only where a short read is self-correcting. + */ + onFailure?: 'throw' | 'partial'; + } = {}, + ): Promise { const uniqueColumns: string[] = []; try { if (this.isPostgres) { + // ⚠️ This query was INVALID until #11161: it selected `c.column_name` + // while the only aliases in scope were `tc` and `ccu`, so every + // execution raised `missing FROM-clause entry for table "c"` — and the + // bare `catch {}` this method carried until #11161 converted that into + // `[]` on every call. Live Postgres therefore NEVER reported a unique + // constraint through this method; the defect surfaced (as nine loud + // test failures) the moment the catch stopped swallowing, which is the + // #7332 contract doing exactly its job. The schema pin follows + // `introspectSchema`'s own table listing (`current_schemas(false)`, + // #9350's pattern): `constraint_column_usage` spans every schema the + // user can read, so without it the newly-working query would report a + // same-named table's constraints from a schema the session never + // reaches. const result = await this.knex.raw( ` - SELECT c.column_name + SELECT ccu.column_name FROM information_schema.table_constraints tc JOIN information_schema.constraint_column_usage AS ccu ON tc.constraint_schema = ccu.constraint_schema AND tc.constraint_name = ccu.constraint_name WHERE tc.constraint_type = 'UNIQUE' AND tc.table_name = ? + AND tc.table_schema = ANY (current_schemas(false)) `, [tableName], ); @@ -13122,8 +13287,9 @@ export class SqlDriver implements IDataDriver { } } } - } catch { - // silently ignore + } catch (e) { + // Only a caller that can CORRECT a short read may ask for one (#7332). + if (opts.onFailure !== 'partial') throw e; } return uniqueColumns;