Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions .changeset/driver-sql-covering-pk-membership.md
Original file line numberDiff line numberDiff line change
@@ -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.
9 changes: 9 additions & 0 deletions .changeset/driver-sql-declared-column-order.md
Original file line numberDiff line numberDiff line change
@@ -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.
13 changes: 13 additions & 0 deletions .changeset/driver-sql-introspection-error-contract.md
Original file line numberDiff line numberDiff line change
@@ -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.

<!-- adr-0087: not-required (no-migration-prescription) runtime error-contract change on SqlDriver's protected introspection methods; no authorable metadata key changes shape, so `objectstack migrate meta` has nothing to rewrite -->
Original file line numberDiff line numberDiff line change
@@ -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);
}
Original file line numberDiff line numberDiff line change
@@ -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);
Loading
Loading