Skip to content

Commit 9cc1940

Browse files
os-zhuangclaude
andauthored
fix(driver-sql): introspection trio — error contract (#7332 extended), covering-PK membership, declared column order (#11203)
* fix(driver-sql): a failed PK/FK/unique introspection read throws instead of reporting absence Extends the #7332 ruling that introspectIndexes already carries to its three siblings, with the identical option shape and default: onFailure?: 'throw' | 'partial', defaulting to 'throw'. A bare catch {} returning [] converted a failed read into a positive assertion of absence — and primaryKeys is consumed as an addressing / upsert-conflict-target key, so downstream code acted on the wrong answer. introspectSchema's four reads all take the default; every in-tree caller already handles a throw. Fixes #11161 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RfyXxZ2WPjcjhuXpiQQc3y * fix(driver-sql): bound the PG primary-key join to indnkeyatts so INCLUDE'd columns are not key members A covering primary key (CREATE UNIQUE INDEX ... INCLUDE (payload) promoted via ADD CONSTRAINT ... PRIMARY KEY USING INDEX) carries its payload columns in pg_index.indkey; indnkeyatts counts the leading entries that are key members and was never consulted, so payload was reported as part of the key. Measured on PostgreSQL 16.13: indkey='2 1 3', indnkeyatts=2. The bound k.ord <= i.indnkeyatts fixes membership while preserving #11101's declared key ORDER. Fixes #11162 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RfyXxZ2WPjcjhuXpiQQc3y * fix(driver-sql): introspect columns in declared order from the catalog ordinal, not columnInfo() key order knex's columnInfo() is an object keyed by column name with no ORDER BY behind it; on MySQL 8.0.46 the row order is alphabetical, so the same table introspected through different dialects returned different columns arrays and federated-object drafts got their fields alphabetized. The order now comes from the catalog's ordinal on all three dialects (ORDINAL_POSITION / ordinal_position / PRAGMA table_info cid); columnInfo() remains the source of the per-column facts, which knex already normalises per dialect. Fixes #11163 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RfyXxZ2WPjcjhuXpiQQc3y --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 21756b3 commit 9cc1940

7 files changed

Lines changed: 688 additions & 12 deletions
Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
---
2+
"@objectstack/driver-sql": patch
3+
---
4+
5+
**Bug fix:** on Postgres, `introspectPrimaryKeys` no longer reports a covering primary key's `INCLUDE`'d columns as key members (#11162).
6+
7+
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)`.
8+
9+
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.
Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
---
2+
"@objectstack/driver-sql": patch
3+
---
4+
5+
**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).
6+
7+
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.
8+
9+
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.
Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
---
2+
"@objectstack/driver-sql": minor
3+
---
4+
5+
**BREAKING**: a failed primary-key / foreign-key / unique-constraint introspection read now throws instead of silently reporting absence (#11161).
6+
7+
`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".
8+
9+
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).
10+
11+
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.
12+
13+
<!-- 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 -->
Lines changed: 209 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,209 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* [#11163] `introspectColumns` must report a table's columns in DECLARED
5+
* order on **every** dialect — SQLite, Postgres and MySQL — for the same
6+
* table.
7+
*
8+
* The method built its array from knex's `columnInfo()`, an object KEYED BY
9+
* COLUMN NAME whose key-insertion order is the row order of knex's own
10+
* `information_schema.columns` query — which carries no `ORDER BY`. Measured
11+
* live: SQLite and PostgreSQL 16.13 happened to return declared order; MySQL
12+
* 8.0.46 returned **alphabetical** order, so the same table introspected
13+
* through different dialects returned different `columns` arrays, and a
14+
* federated object drafted from a MySQL remote (ADR-0015
15+
* `generateObjectDraft` / the persisted `external_catalog`) got its fields
16+
* alphabetized rather than in the order the remote declares them.
17+
*
18+
* The fix reads the order from the catalog's own ordinal
19+
* (`ORDINAL_POSITION` / `ordinal_position` / `PRAGMA table_info`'s `cid`) —
20+
* the ordinal is the fact; a plan's row order is not, on ANY dialect.
21+
*
22+
* ## ⭐ Why the fixtures' alphabetical order differs from their declared order
23+
*
24+
* Alphabetical order is exactly what the buggy path returned on MySQL, so a
25+
* fixture whose declared order IS alphabetical would make every assertion
26+
* below a tautology the buggy code also passes. {@link TWO_KEY_TABLE} reuses
27+
* #11101's permutation shape (`carrier_code, shipment_id, leg_seq` — its
28+
* alphabetical order swaps the last two), and {@link Z_FIRST_TABLE} differs
29+
* in the FIRST position too, so an arm that merely happened to agree on the
30+
* leading column cannot pass by accident. The `non-vacuous` leg pins both
31+
* constants against their own DDL text.
32+
*
33+
* ## How the three dialects are held to ONE answer
34+
*
35+
* Same construction as the #11101 key-order file: every cell runs the same
36+
* DDL and asserts the same constants, through `declareDialectCell` — live
37+
* cells are a named skip without `OS_TEST_POSTGRES_URL` /
38+
* `OS_TEST_MYSQL_URL`, and a red under `OS_EXPECT_LIVE_DIALECT_MATRIX=1`.
39+
*/
40+
41+
import { describe, it, expect, beforeEach, afterEach } from 'vitest';
42+
import { SqlDriver } from '../src/index.js';
43+
import { DIALECT_CELLS, declareDialectCell, type DialectCell } from './live-dialect-matrix.testkit.js';
44+
45+
const MATRIX = 'declared COLUMN order';
46+
47+
/** Tables this file owns. The schema they land in is per-file (#9350). */
48+
const TWO_KEY_TABLE = 'os11163_shipment_legs';
49+
const Z_FIRST_TABLE = 'os11163_zone_areas';
50+
51+
/**
52+
* #11101's fixture shape, reused deliberately: declared order
53+
* `carrier_code, shipment_id, leg_seq`, alphabetical order
54+
* `carrier_code, leg_seq, shipment_id` — a real permutation.
55+
*/
56+
const TWO_KEY_DDL = `create table ${TWO_KEY_TABLE} (
57+
carrier_code varchar(64) not null,
58+
shipment_id varchar(64) not null,
59+
leg_seq integer,
60+
primary key (shipment_id, carrier_code)
61+
)`;
62+
63+
const TWO_KEY_COLUMN_ORDER = ['carrier_code', 'shipment_id', 'leg_seq'];
64+
65+
/**
66+
* Alphabetical differs in the FIRST position: `zone_code` is declared first
67+
* and sorts last.
68+
*/
69+
const Z_FIRST_DDL = `create table ${Z_FIRST_TABLE} (
70+
zone_code varchar(64) not null,
71+
area_code varchar(64) not null,
72+
seq integer
73+
)`;
74+
75+
const Z_FIRST_COLUMN_ORDER = ['zone_code', 'area_code', 'seq'];
76+
77+
/** Exact ordered array, with the alphabetical degradation named. */
78+
function expectDeclaredColumnOrder(actual: string[], declared: string[], cell: DialectCell): void {
79+
expect(
80+
actual,
81+
`${cell.label}: introspected columns must be in DECLARED order — alphabetical is the ` +
82+
`#11163 defect (knex columnInfo() key order), and any other order is a plan's accident`,
83+
).toEqual(declared);
84+
}
85+
86+
function declareColumnOrderSuite(cell: DialectCell): void {
87+
describe(`introspectColumns declared order — ${cell.label} (#11163)`, () => {
88+
let driver: SqlDriver;
89+
90+
beforeEach(async () => {
91+
driver = new SqlDriver(cell.config());
92+
for (const t of [TWO_KEY_TABLE, Z_FIRST_TABLE]) {
93+
await driver.execute(`drop table if exists ${t}`).catch(() => {});
94+
}
95+
await driver.execute(TWO_KEY_DDL);
96+
await driver.execute(Z_FIRST_DDL);
97+
});
98+
99+
afterEach(async () => {
100+
for (const t of [TWO_KEY_TABLE, Z_FIRST_TABLE]) {
101+
await driver.execute(`drop table if exists ${t}`).catch(() => {});
102+
}
103+
await driver.disconnect();
104+
});
105+
106+
it('asserts the fixtures are non-vacuous: declared order differs from alphabetical order', async () => {
107+
for (const [ddl, declared] of [
108+
[TWO_KEY_DDL, TWO_KEY_COLUMN_ORDER],
109+
[Z_FIRST_DDL, Z_FIRST_COLUMN_ORDER],
110+
] as const) {
111+
// The constant really is the order the DDL declares — read off the
112+
// fixture's own text so it cannot quietly stop describing its table.
113+
const declaredAt = declared.map((c) => ddl.indexOf(`\n ${c} `));
114+
expect(declaredAt.every((at) => at > 0)).toBe(true);
115+
expect([...declaredAt].sort((x, y) => x - y)).toEqual(declaredAt);
116+
117+
// Alphabetical ≠ declared: the whole premise. Without this, every
118+
// assertion below is a tautology the buggy code also passed.
119+
expect([...declared].sort()).not.toEqual(declared);
120+
}
121+
// And the z-first fixture disagrees in the FIRST position specifically.
122+
expect([...Z_FIRST_COLUMN_ORDER].sort()[0]).not.toBe(Z_FIRST_COLUMN_ORDER[0]);
123+
});
124+
125+
it('reports columns in declared order, not alphabetical order', async () => {
126+
const schema = await driver.introspectSchema();
127+
128+
expectDeclaredColumnOrder(
129+
schema.tables[TWO_KEY_TABLE].columns.map((c) => c.name),
130+
TWO_KEY_COLUMN_ORDER,
131+
cell,
132+
);
133+
expectDeclaredColumnOrder(
134+
schema.tables[Z_FIRST_TABLE].columns.map((c) => c.name),
135+
Z_FIRST_COLUMN_ORDER,
136+
cell,
137+
);
138+
});
139+
140+
it('keeps every per-column fact paired with its column across the reorder', async () => {
141+
const schema = await driver.introspectSchema();
142+
const byName = Object.fromEntries(
143+
schema.tables[TWO_KEY_TABLE].columns.map((c) => [c.name, c]),
144+
);
145+
146+
// The facts still come from knex's columnInfo(); the reorder must not
147+
// detach them from their names. nullable is the one fact every dialect
148+
// spells the same way through knex's normalisation.
149+
expect(byName.carrier_code.nullable).toBe(false);
150+
expect(byName.shipment_id.nullable).toBe(false);
151+
expect(byName.leg_seq.nullable).toBe(true);
152+
// And the key flags derived downstream still land on the key columns.
153+
expect(byName.carrier_code.primaryKey).toBe(true);
154+
expect(byName.shipment_id.primaryKey).toBe(true);
155+
expect(byName.leg_seq.primaryKey).toBe(false);
156+
});
157+
});
158+
}
159+
160+
for (const cell of DIALECT_CELLS) {
161+
declareDialectCell(cell, MATRIX, declareColumnOrderSuite);
162+
}
163+
164+
/**
165+
* The catalog fact each rewritten arm rests on, pinned per live dialect: the
166+
* ordinal is the DECLARED position. (The alphabetical row order the unordered
167+
* query happened to return is deliberately NOT pinned — it is unspecified by
168+
* both engines; the measured pre-fix output is recorded in the PR body
169+
* instead, exactly as the #11101 key-order file does for its defect.)
170+
*/
171+
function declareCatalogPins(cell: DialectCell): void {
172+
if (cell.id === 'sqlite') return; // `cid` ordinality is pinned by the #10997 composite-key file's PRAGMA pin
173+
174+
describe(`introspectColumns catalog facts — ${cell.label} (#11163)`, () => {
175+
let driver: SqlDriver;
176+
177+
beforeEach(async () => {
178+
driver = new SqlDriver(cell.config());
179+
await driver.execute(`drop table if exists ${TWO_KEY_TABLE}`).catch(() => {});
180+
await driver.execute(TWO_KEY_DDL);
181+
});
182+
183+
afterEach(async () => {
184+
await driver.execute(`drop table if exists ${TWO_KEY_TABLE}`).catch(() => {});
185+
await driver.disconnect();
186+
});
187+
188+
it('the catalog ordinal is the declared column position', async () => {
189+
const sql =
190+
cell.id === 'pg'
191+
? `select column_name, ordinal_position from information_schema.columns
192+
where table_name = '${TWO_KEY_TABLE}'
193+
and table_catalog = current_database() and table_schema = current_schema()`
194+
: `select COLUMN_NAME as column_name, ORDINAL_POSITION as ordinal_position
195+
from information_schema.COLUMNS
196+
where TABLE_SCHEMA = DATABASE() and TABLE_NAME = '${TWO_KEY_TABLE}'`;
197+
const res: any = await driver.execute(sql);
198+
const rows: any[] = cell.id === 'pg' ? res.rows : res[0];
199+
const ordinalByName = Object.fromEntries(
200+
rows.map((r: any) => [r.column_name, Number(r.ordinal_position)]),
201+
);
202+
expect(ordinalByName).toEqual({ carrier_code: 1, shipment_id: 2, leg_seq: 3 });
203+
});
204+
});
205+
}
206+
207+
for (const cell of DIALECT_CELLS) {
208+
declareDialectCell(cell, `${MATRIX} catalog facts`, declareCatalogPins);
209+
}
Lines changed: 121 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,121 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* [#11162] A covering primary key's INCLUDE'd columns are NOT key members.
5+
*
6+
* Postgres reaches a covering primary key via
7+
* `CREATE UNIQUE INDEX … INCLUDE (payload)` promoted with
8+
* `ALTER TABLE … ADD CONSTRAINT … PRIMARY KEY USING INDEX`. For such an index
9+
* `pg_index.indkey` holds the key columns *and* the INCLUDE'd payload columns;
10+
* `indnkeyatts` is the count of the leading entries that are actually key
11+
* members. `introspectPrimaryKeys` read `indkey` whole and never consulted
12+
* `indnkeyatts`, so `payload` was reported as part of the key.
13+
*
14+
* A key with an extra member is a DIFFERENT key: an upsert conflict target
15+
* naming a non-key column does not match the constraint, and schema-drift
16+
* comparison against a correctly-declared `(k2, k1)` reports a phantom
17+
* `unexpected_key_member:payload`. Measured on a live PostgreSQL 16.13:
18+
* `indkey = '2 1 3'`, `indnkeyatts = 2`, and both the pre-#11101 and
19+
* post-#11101 queries returned `payload` (#11101 repaired ORDER, not
20+
* membership — the two arms agreed on the wrong membership).
21+
*
22+
* ## Why this file is PG-only
23+
*
24+
* MySQL has no covering-index concept for a PRIMARY KEY and SQLite has no
25+
* INCLUDE at all — the defect is not expressible there, so the cell list is
26+
* exactly `pg`, declared through `declareDialectCell` so an unprovisioned run
27+
* is a named skip (and a red under `OS_EXPECT_LIVE_DIALECT_MATRIX=1`), never
28+
* a silent pass.
29+
*
30+
* ## Why the assertion is the EXACT ORDERED array
31+
*
32+
* Two reasons. Membership alone would pass a fix that broke #11101's ordering
33+
* repair — the fixture's key `(k2, k1)` is deliberately declared out of column
34+
* sequence so ordering stays observable, and the exact array holds both
35+
* properties at once. And a set/length assertion could go green over the
36+
* method's failure modes; the exact array cannot.
37+
*/
38+
39+
import { describe, it, expect, beforeEach, afterEach } from 'vitest';
40+
import { SqlDriver } from '../src/index.js';
41+
import { PG_CELL, declareDialectCell, type DialectCell } from './live-dialect-matrix.testkit.js';
42+
43+
const MATRIX = 'covering primary-key MEMBERSHIP';
44+
45+
/** Table this file owns. The schema it lands in is per-file (#9350). */
46+
const TABLE = 'os11162_covered';
47+
48+
/**
49+
* Column order is `k1, k2, payload`; the KEY is `(k2, k1)` — out of column
50+
* sequence on purpose, so this fixture can see an ordering regression too.
51+
* `payload` is carried by the index but is NOT a key member.
52+
*/
53+
const DDL = [
54+
`create table ${TABLE} (k1 varchar(64) not null, k2 varchar(64) not null, payload varchar(64))`,
55+
`create unique index ${TABLE}_pk on ${TABLE} (k2, k1) include (payload)`,
56+
`alter table ${TABLE} add constraint ${TABLE}_pkey primary key using index ${TABLE}_pk`,
57+
];
58+
59+
/** The declared key: exactly the two key columns, in declared key order. */
60+
const KEY_ORDER = ['k2', 'k1'];
61+
62+
function declareCoveringKeySuite(cell: DialectCell): void {
63+
describe(`introspectPrimaryKeys covering-key membership — ${cell.label} (#11162)`, () => {
64+
let driver: SqlDriver;
65+
66+
beforeEach(async () => {
67+
driver = new SqlDriver(cell.config());
68+
await driver.execute(`drop table if exists ${TABLE}`).catch(() => {});
69+
for (const stmt of DDL) await driver.execute(stmt);
70+
});
71+
72+
afterEach(async () => {
73+
await driver.execute(`drop table if exists ${TABLE}`).catch(() => {});
74+
await driver.disconnect();
75+
});
76+
77+
it('pins the catalog facts the fix rests on: indkey carries payload, indnkeyatts bounds the key', async () => {
78+
const res: any = await driver.execute(
79+
`select i.indkey::text as indkey, i.indnatts, i.indnkeyatts
80+
from pg_index i
81+
where i.indrelid = '${TABLE}'::regclass and i.indisprimary`,
82+
);
83+
const row = res.rows[0];
84+
// k1 is attnum 1, k2 attnum 2, payload attnum 3: the key columns in KEY
85+
// order, then the INCLUDE'd column. If a server ever stopped reporting
86+
// this shape, the arm would be wrong for a reason no assertion on its
87+
// OUTPUT could localise.
88+
expect(row.indkey).toBe('2 1 3');
89+
expect(Number(row.indnatts)).toBe(3);
90+
expect(Number(row.indnkeyatts)).toBe(2);
91+
});
92+
93+
it('reports ONLY the key columns, in declared key order — INCLUDE columns are not members', async () => {
94+
const schema = await driver.introspectSchema();
95+
const introspected = schema.tables[TABLE].primaryKeys;
96+
97+
// Exact ordered array: membership (#11162) and order (#11101) at once.
98+
expect(
99+
introspected,
100+
`${cell.label}: a covering PK must report its key columns only — ` +
101+
`'payload' is an INCLUDE'd column, and reporting it makes this a DIFFERENT addressing key`,
102+
).toEqual(KEY_ORDER);
103+
104+
// The pre-fix answer, named: what both the pre- and post-#11101 queries
105+
// returned on a live 16.13 before this bound existed.
106+
expect(introspected).not.toEqual(['k2', 'k1', 'payload']);
107+
});
108+
109+
it('derives the per-column primaryKey flag from the bounded membership', async () => {
110+
const schema = await driver.introspectSchema();
111+
const flags = Object.fromEntries(
112+
schema.tables[TABLE].columns.map((c) => [c.name, c.primaryKey === true]),
113+
);
114+
// `introspectSchema` derives this FROM `primaryKeys`, so the phantom
115+
// member corrupted this signal too.
116+
expect(flags).toEqual({ k1: true, k2: true, payload: false });
117+
});
118+
});
119+
}
120+
121+
declareDialectCell(PG_CELL, MATRIX, declareCoveringKeySuite);

0 commit comments

Comments
 (0)