Skip to content

Commit 927ccbb

Browse files
os-zhuangclaude
andauthored
fix(driver-sql): introspectPrimaryKeys returns the PG and MySQL key in declared key order (#11101) (#11164)
`SqlDriver.introspectPrimaryKeys` ordered its result on exactly one of its three dialect arms. #10997 repaired SQLite (completeness and key ordering, by sorting on the `PRAGMA table_info` ordinal); the Postgres and MySQL arms returned the composite key in unspecified row order. - Postgres: `a.attnum = ANY(i.indkey)` is a MEMBERSHIP test. `i.indkey` is an `int2vector` holding the key's attnums in key order, but `ANY()` reads it as a set and discards the position, and the query carried no `ORDER BY`. It now joins the ordinality of `indkey` (`unnest(i.indkey) WITH ORDINALITY`) and orders by that ordinal. - MySQL: `KEY_COLUMN_USAGE.ORDINAL_POSITION` IS the key ordinal and was selected by neither the projection nor an order clause. It now carries `ORDER BY ORDINAL_POSITION`. Both arms were measured returning COLUMN order — the key reversed — on live servers before the fix: PostgreSQL 16.13 and MySQL 8.0.46, over a table declared `(carrier_code, shipment_id, leg_seq)` with `PRIMARY KEY (shipment_id, carrier_code)`. Notably InnoDB did NOT return ordinal order, contradicting the usual folklore. `primaryKeys` is consumed as an addressing / upsert-conflict-target key (federated-object codegen, the persisted `external_catalog` under ADR-0015, schema-drift comparison), so a key in the wrong order is a DIFFERENT key. All three dialects now agree on the same table. The method's silent `catch { return [] }` is deliberately NOT changed here — it is a separate error-contract decision with its own blast radius, filed as its own finding. It does dictate the test shape: every assertion is positive and ordered, because a query a server rejects degrades to "no primary key at all" and a does-not-throw or set-equality test would stay green over total key loss. Claude-Session: https://claude.ai/code/session_01RfyXxZ2WPjcjhuXpiQQc3y Co-authored-by: Claude <noreply@anthropic.com>
1 parent 46d34ab commit 927ccbb

3 files changed

Lines changed: 368 additions & 2 deletions

File tree

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
---
2+
"@objectstack/driver-sql": patch
3+
---
4+
5+
fix(driver-sql): `introspectPrimaryKeys` returns the Postgres and MySQL composite key in DECLARED KEY ORDER (#11101)
6+
7+
`SqlDriver.introspectPrimaryKeys` ordered its result on exactly one of its three
8+
dialect arms. #10997 repaired SQLite (completeness *and* key ordering, by sorting
9+
on the `PRAGMA table_info` ordinal); the Postgres and MySQL arms returned the key
10+
in unspecified row order.
11+
12+
- **Postgres**: `a.attnum = ANY(i.indkey)` is a *membership* test. `i.indkey` is
13+
an `int2vector` holding the key's attnums **in key order**, but `ANY()` reads
14+
the vector as a set and discards the position, and the query carried no
15+
`ORDER BY`. It now joins the **ordinality** of `indkey`
16+
(`unnest(i.indkey) WITH ORDINALITY`) and orders by that ordinal.
17+
- **MySQL**: `KEY_COLUMN_USAGE.ORDINAL_POSITION` *is* the key ordinal and was
18+
selected by neither the projection nor an order clause. It now carries
19+
`ORDER BY ORDINAL_POSITION`.
20+
21+
Both arms were measured returning **column order** — the key reversed — against
22+
live servers before the fix: PostgreSQL 16.13 and MySQL 8.0.46, on a table
23+
declared `(carrier_code, shipment_id, leg_seq)` with
24+
`PRIMARY KEY (shipment_id, carrier_code)`. The MySQL result is worth naming
25+
explicitly, because the received wisdom is the opposite: InnoDB did **not**
26+
return ordinal order for an out-of-sequence key.
27+
28+
Why the order is load-bearing rather than cosmetic: `primaryKeys` is consumed as
29+
an **addressing / upsert-conflict-target** key — federated-object codegen, the
30+
persisted `external_catalog` under ADR-0015, and schema-drift comparison against
31+
a declared key. For those consumers a key in the wrong order is a *different*
32+
key. Until now the same table introspected through different dialects could
33+
disagree, since SQLite reported declared key order and the other two did not; all
34+
three now agree.
35+
36+
Covered by `sql-driver-primary-key-order-dialects.test.ts`, which runs the same
37+
DDL on all three dialects and asserts the exact ordered array. Its live Postgres
38+
and MySQL cells execute in the `Temporal Conformance (live PG + MySQL)` CI job
39+
(a required check) and are reported as named skips elsewhere.
Lines changed: 301 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,301 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* [#11101] `introspectPrimaryKeys` must report a composite key in DECLARED KEY
5+
* ORDER on **every** dialect — SQLite, Postgres and MySQL — for the same table.
6+
*
7+
* #10997 repaired the SQLite arm (completeness *and* ordering, by sorting on the
8+
* `PRAGMA table_info` ordinal). The other two arms did not order at all:
9+
*
10+
* - **Postgres**: `a.attnum = ANY(i.indkey)` is a MEMBERSHIP test. `i.indkey`
11+
* holds the key's attnums in key order, but `ANY()` reads the vector as a set
12+
* and discards the position; with no `ORDER BY` the row order was whatever
13+
* the plan yielded.
14+
* - **MySQL**: `KEY_COLUMN_USAGE.ORDINAL_POSITION` *is* the key ordinal and was
15+
* selected by neither the projection nor an order clause.
16+
*
17+
* Both were measured returning **column order** on live servers before the fix
18+
* (PostgreSQL 16.13 and MySQL 8.0.46 — see the PR body), i.e. the key REVERSED
19+
* for the fixture below. `primaryKeys` is consumed as an addressing /
20+
* upsert-conflict-target key (federated-object codegen, the persisted
21+
* `external_catalog` under ADR-0015, schema-drift comparison), so a key in the
22+
* wrong order is a DIFFERENT key — and the same table introspected through
23+
* different dialects disagreed.
24+
*
25+
* ## ⛔ Why every assertion here is POSITIVE and ORDERED
26+
*
27+
* `introspectPrimaryKeys` wraps its whole body in `catch { }` and returns `[]`.
28+
* A query that is invalid on a live server therefore does **not** fail loudly —
29+
* it degrades to *no primary key at all*, with no diagnostic. So a test that
30+
* asserts "does not throw", or that checks membership / set equality, is
31+
* worthless here: it stays green over total key loss.
32+
*
33+
* Every leg asserts the **exact array**, and {@link expectDeclaredKeyOrder}
34+
* checks the length first so a degradation reads as "the silent catch ate the
35+
* query" rather than as a diff nobody can interpret. (The catch itself is out of
36+
* scope for this card and is filed separately — this file does not pin it.)
37+
*
38+
* ## ⛔ Why the fixture declares its key OUT OF COLUMN SEQUENCE
39+
*
40+
* Column order and key order coincide for most tables, and column order is
41+
* exactly what the unordered queries already returned — so a table whose key
42+
* follows its columns proves nothing. {@link KEY_ORDER} is a genuine permutation
43+
* of the key columns' positions, and `asserts the fixture is non-vacuous` fails
44+
* if a later edit ever flattens it back into column sequence.
45+
*
46+
* ## How the three dialects are held to ONE answer
47+
*
48+
* Every cell runs the **same DDL** and asserts against the **same**
49+
* {@link KEY_ORDER} constant, so agreement across dialects is by construction
50+
* rather than by a cross-suite comparison that vitest's file parallelism could
51+
* not make reliable. The live cells are declared through `declareDialectCell`:
52+
* REPORTED as a named skip without `OS_TEST_POSTGRES_URL` / `OS_TEST_MYSQL_URL`,
53+
* and a hard failure under `OS_EXPECT_LIVE_DIALECT_MATRIX=1` — which is what the
54+
* `Temporal Conformance (live PG + MySQL)` job sets, so these legs really do
55+
* execute against `postgres:16` and `mysql:8.0` on a required check.
56+
*/
57+
58+
import { describe, it, expect, beforeEach, afterEach } from 'vitest';
59+
import { SqlDriver } from '../src/index.js';
60+
import { DIALECT_CELLS, declareDialectCell, type DialectCell } from './live-dialect-matrix.testkit.js';
61+
62+
const MATRIX = 'composite primary-key ORDER';
63+
64+
/** Tables this file owns. The SCHEMA/database they land in is per-file (#9350). */
65+
const TWO_PART = 'os11101_shipment_legs';
66+
const THREE_PART = 'os11101_route_hops';
67+
68+
/**
69+
* Column order — deliberately NOT the key order.
70+
*
71+
* `varchar(64)` rather than `text` so one DDL string is legal on all three
72+
* dialects: MySQL cannot take a bare `TEXT` column into a primary key without a
73+
* prefix length, and the point of this file is that the three dialects answer
74+
* identically **for the same table**.
75+
*/
76+
const TWO_PART_DDL = `create table ${TWO_PART} (
77+
carrier_code varchar(64) not null,
78+
shipment_id varchar(64) not null,
79+
leg_seq integer,
80+
primary key (shipment_id, carrier_code)
81+
)`;
82+
83+
/**
84+
* Column order exactly as {@link TWO_PART_DDL} declares it.
85+
*
86+
* Held as a constant rather than read back from `introspectSchema().columns`,
87+
* because that list is NOT in declared column order on every dialect: measured
88+
* on MySQL 8.0.46 it comes back ALPHABETICAL (`carrier_code, leg_seq,
89+
* shipment_id`), since `introspectColumns` builds it from knex's
90+
* `columnInfo()` — an object keyed by column name. That is a separate finding
91+
* filed from this card; it is not this file's subject, and depending on it here
92+
* would make the fixture's own premise dialect-specific.
93+
*
94+
* The `asserts the fixture is non-vacuous` leg pins this constant against the
95+
* DDL text so the two cannot drift apart.
96+
*/
97+
const COLUMN_ORDER = ['carrier_code', 'shipment_id', 'leg_seq'];
98+
99+
/** Declared KEY order: a reversal of the two key columns' positions. */
100+
const KEY_ORDER = ['shipment_id', 'carrier_code'];
101+
102+
/** The same key as the buggy arms reported it — i.e. in COLUMN order. */
103+
const KEY_IN_COLUMN_ORDER = ['carrier_code', 'shipment_id'];
104+
105+
/**
106+
* A three-part key that is a genuine PERMUTATION, not merely a reversal.
107+
*
108+
* A two-column fixture cannot tell "sorted by key ordinal" apart from "sorted
109+
* backwards", and a fix that reversed the row order would satisfy the two-part
110+
* leg while still being wrong. `(b, c, a)` over columns `(c, a, b, d)` is fixed
111+
* by neither reversal nor sorting.
112+
*/
113+
const THREE_PART_DDL = `create table ${THREE_PART} (
114+
c varchar(64) not null,
115+
a varchar(64) not null,
116+
b varchar(64) not null,
117+
d integer,
118+
primary key (b, c, a)
119+
)`;
120+
121+
const THREE_PART_KEY_ORDER = ['b', 'c', 'a'];
122+
123+
/**
124+
* Assert the exact ordered key, with the `[]` degradation named.
125+
*
126+
* The length check is first on purpose: under the method's silent `catch` a
127+
* query that a server rejects yields `[]`, and "expected [] to equal
128+
* ['shipment_id', 'carrier_code']" does not tell the next reader that the SQL
129+
* never ran. This message does.
130+
*/
131+
function expectDeclaredKeyOrder(actual: string[], expected: string[], cell: DialectCell): void {
132+
expect(
133+
actual.length,
134+
`${cell.label}: introspectPrimaryKeys returned ${actual.length} column(s), expected ` +
135+
`${expected.length}. An EMPTY result usually means the dialect arm's query was rejected by ` +
136+
`the server and swallowed by the method's \`catch { }\` — read the query, not this fixture.`,
137+
).toBe(expected.length);
138+
139+
expect(actual, `${cell.label}: key must be in DECLARED order, not column order`).toEqual(expected);
140+
}
141+
142+
function declareKeyOrderSuite(cell: DialectCell): void {
143+
describe(`introspectPrimaryKeys key order — ${cell.label} (#11101)`, () => {
144+
let driver: SqlDriver;
145+
146+
beforeEach(async () => {
147+
driver = new SqlDriver(cell.config());
148+
for (const t of [TWO_PART, THREE_PART]) {
149+
await driver.execute(`drop table if exists ${t}`).catch(() => {});
150+
}
151+
await driver.execute(TWO_PART_DDL);
152+
await driver.execute(THREE_PART_DDL);
153+
});
154+
155+
afterEach(async () => {
156+
for (const t of [TWO_PART, THREE_PART]) {
157+
await driver.execute(`drop table if exists ${t}`).catch(() => {});
158+
}
159+
await driver.disconnect();
160+
});
161+
162+
it('asserts the fixture is non-vacuous: declared key order differs from column order', async () => {
163+
// 1. COLUMN_ORDER really is the order the DDL declares — an anti-drift
164+
// check on the constant, read off the fixture's own text so it cannot
165+
// quietly stop describing the table it names.
166+
const declaredAt = COLUMN_ORDER.map((c) => TWO_PART_DDL.indexOf(`\n ${c} `));
167+
expect(declaredAt.every((at) => at > 0)).toBe(true);
168+
expect([...declaredAt].sort((x, y) => x - y)).toEqual(declaredAt);
169+
170+
// 2. The key columns, taken in COLUMN order, are not the declared KEY
171+
// order. This is the whole premise of the fixture: column order is
172+
// precisely what the unordered queries already returned, so a key that
173+
// followed its columns would make every assertion below a tautology
174+
// the buggy arms also passed.
175+
expect(KEY_IN_COLUMN_ORDER).toEqual(COLUMN_ORDER.filter((c) => KEY_ORDER.includes(c)));
176+
expect(KEY_ORDER).not.toEqual(KEY_IN_COLUMN_ORDER);
177+
178+
// 3. The table really has those columns (as a SET — see COLUMN_ORDER's
179+
// note on why the introspected order is not comparable across
180+
// dialects).
181+
const schema = await driver.introspectSchema();
182+
const found = schema.tables[TWO_PART].columns.map((c) => c.name).sort();
183+
expect(found).toEqual([...COLUMN_ORDER].sort());
184+
});
185+
186+
it('reports a two-part composite key in declared key order, not column order', async () => {
187+
const schema = await driver.introspectSchema();
188+
const introspected = schema.tables[TWO_PART].primaryKeys;
189+
190+
expectDeclaredKeyOrder(introspected, KEY_ORDER, cell);
191+
192+
// The pre-fix answer, named: this is what both live servers returned
193+
// before the rewrite, and it is a DIFFERENT addressing key.
194+
expect(introspected).not.toEqual(KEY_IN_COLUMN_ORDER);
195+
});
196+
197+
it('reports a three-part key that is a permutation of column order', async () => {
198+
const schema = await driver.introspectSchema();
199+
const introspected = schema.tables[THREE_PART].primaryKeys;
200+
201+
expectDeclaredKeyOrder(introspected, THREE_PART_KEY_ORDER, cell);
202+
203+
// Neither the column order nor its reverse — so an arm that merely
204+
// reversed rows, or sorted them, cannot pass this.
205+
expect(introspected).not.toEqual(['c', 'a', 'b']);
206+
expect(introspected).not.toEqual(['b', 'a', 'c']);
207+
expect(introspected).not.toEqual([...THREE_PART_KEY_ORDER].sort());
208+
});
209+
210+
it('derives the per-column primaryKey flag for every key member', async () => {
211+
const schema = await driver.introspectSchema();
212+
const flags = Object.fromEntries(
213+
schema.tables[TWO_PART].columns.map((c) => [c.name, c.primaryKey === true]),
214+
);
215+
216+
// `introspectSchema` derives this FROM `primaryKeys`, so it is the second
217+
// signal an empty result would corrupt.
218+
expect(flags).toEqual({ carrier_code: true, shipment_id: true, leg_seq: false });
219+
});
220+
});
221+
}
222+
223+
for (const cell of DIALECT_CELLS) {
224+
declareDialectCell(cell, MATRIX, declareKeyOrderSuite);
225+
}
226+
227+
/**
228+
* The catalog facts each rewritten arm rests on, pinned per dialect.
229+
*
230+
* Same role as `#10997`'s "`pk` is a 1-based ordinal, not a boolean" pin: if a
231+
* server ever stopped reporting these, the arm above would be wrong for a
232+
* reason no assertion on its OUTPUT could localise.
233+
*
234+
* ⛔ Note what is deliberately NOT pinned: the row order the *unordered* query
235+
* returns. That order is unspecified by both engines — asserting the reversal
236+
* these servers happen to produce would be pinning a behaviour neither vendor
237+
* promises. The measured pre-fix output is recorded in the PR body instead.
238+
*/
239+
function declareCatalogPins(cell: DialectCell): void {
240+
if (cell.id === 'sqlite') return; // covered by sql-driver-composite-primary-key-introspection.test.ts
241+
242+
describe(`introspectPrimaryKeys catalog facts — ${cell.label} (#11101)`, () => {
243+
let driver: SqlDriver;
244+
245+
beforeEach(async () => {
246+
driver = new SqlDriver(cell.config());
247+
await driver.execute(`drop table if exists ${TWO_PART}`).catch(() => {});
248+
await driver.execute(TWO_PART_DDL);
249+
});
250+
251+
afterEach(async () => {
252+
await driver.execute(`drop table if exists ${TWO_PART}`).catch(() => {});
253+
await driver.disconnect();
254+
});
255+
256+
if (cell.id === 'pg') {
257+
it('pg_index.indkey holds attnums in KEY order, while pg_attribute is in COLUMN order', async () => {
258+
const indkey: any = await driver.execute(
259+
`select i.indkey::text as indkey from pg_index i
260+
where i.indrelid = '${TWO_PART}'::regclass and i.indisprimary`,
261+
);
262+
// carrier_code is attnum 1, shipment_id is attnum 2 — so "2 1" is the
263+
// key order, and it is the REVERSE of the attnum sequence. This is the
264+
// position `a.attnum = ANY(i.indkey)` discarded.
265+
expect(indkey.rows[0].indkey).toBe('2 1');
266+
267+
const atts: any = await driver.execute(
268+
`select attnum, attname from pg_attribute
269+
where attrelid = '${TWO_PART}'::regclass and attnum > 0 and not attisdropped
270+
order by attnum`,
271+
);
272+
expect(atts.rows.map((r: any) => r.attname)).toEqual([
273+
'carrier_code',
274+
'shipment_id',
275+
'leg_seq',
276+
]);
277+
});
278+
}
279+
280+
if (cell.id === 'mysql') {
281+
it('KEY_COLUMN_USAGE.ORDINAL_POSITION is the key ordinal', async () => {
282+
const res: any = await driver.execute(
283+
`select COLUMN_NAME as column_name, ORDINAL_POSITION as ordinal_position
284+
from information_schema.KEY_COLUMN_USAGE
285+
where TABLE_SCHEMA = DATABASE() and TABLE_NAME = '${TWO_PART}'
286+
and CONSTRAINT_NAME = 'PRIMARY'`,
287+
);
288+
const ordinalByName = Object.fromEntries(
289+
res[0].map((r: any) => [r.column_name, Number(r.ordinal_position)]),
290+
);
291+
// The ordinal is the KEY position, not the column position: shipment_id
292+
// is the table's SECOND column but the key's FIRST member.
293+
expect(ordinalByName).toEqual({ shipment_id: 1, carrier_code: 2 });
294+
});
295+
}
296+
});
297+
}
298+
299+
for (const cell of DIALECT_CELLS) {
300+
declareDialectCell(cell, `${MATRIX} catalog facts`, declareCatalogPins);
301+
}

packages/drivers/driver-sql/src/sql-driver.ts

Lines changed: 28 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12715,14 +12715,32 @@ export class SqlDriver implements IDataDriver {
1271512715

1271612716
try {
1271712717
if (this.isPostgres) {
12718+
// `i.indkey` is an `int2vector` holding the key's attnums IN KEY ORDER,
12719+
// but `a.attnum = ANY(i.indkey)` is a MEMBERSHIP test: it reads the
12720+
// vector as a set and discards the position. With no `ORDER BY`, the row
12721+
// order was whatever the plan yielded — measured on PostgreSQL 16.13,
12722+
// `pg_attribute` scan order, i.e. COLUMN order. For a table declared
12723+
// `(carrier_code, shipment_id, leg_seq)` with `PRIMARY KEY (shipment_id,
12724+
// carrier_code)` that is the key REVERSED.
12725+
//
12726+
// Joining the ORDINALITY of `indkey` keeps the position that the
12727+
// membership test threw away, and `ORDER BY k.ord` makes the result the
12728+
// declared key order — the same guarantee the SQLite arm below gets from
12729+
// sorting on the `PRAGMA table_info` ordinal. The two orders differ
12730+
// whenever a key is declared out of column sequence, and this list is
12731+
// used as an addressing / upsert-conflict-target key, where the order is
12732+
// load-bearing: a key in the wrong order is a DIFFERENT key.
1271812733
const result = await this.knex.raw(
1271912734
`
1272012735
SELECT a.attname as column_name
1272112736
FROM pg_index i
12722-
JOIN pg_attribute a ON a.attrelid = i.indrelid
12723-
AND a.attnum = ANY(i.indkey)
12737+
CROSS JOIN LATERAL unnest(i.indkey) WITH ORDINALITY AS k(attnum, ord)
12738+
JOIN pg_attribute a
12739+
ON a.attrelid = i.indrelid
12740+
AND a.attnum = k.attnum
1272412741
WHERE i.indrelid = ?::regclass
1272512742
AND i.indisprimary
12743+
ORDER BY k.ord
1272612744
`,
1272712745
[tableName],
1272812746
);
@@ -12731,13 +12749,21 @@ export class SqlDriver implements IDataDriver {
1273112749
primaryKeys.push(row.column_name);
1273212750
}
1273312751
} else if (this.isMysql) {
12752+
// `KEY_COLUMN_USAGE.ORDINAL_POSITION` IS the key ordinal, and it was
12753+
// selected by neither the projection nor an order clause. Without
12754+
// `ORDER BY` the row order is unspecified — and measured on MySQL
12755+
// 8.0.46 it is COLUMN order, not ordinal order, so an out-of-sequence
12756+
// key came back reversed. (The InnoDB folklore that it "tends to"
12757+
// return ordinal order does not hold on this shape.) Same reason as the
12758+
// Postgres arm above: the order is load-bearing.
1273412759
const result = await this.knex.raw(
1273512760
`
1273612761
SELECT COLUMN_NAME as column_name
1273712762
FROM information_schema.KEY_COLUMN_USAGE
1273812763
WHERE TABLE_SCHEMA = DATABASE()
1273912764
AND TABLE_NAME = ?
1274012765
AND CONSTRAINT_NAME = 'PRIMARY'
12766+
ORDER BY ORDINAL_POSITION
1274112767
`,
1274212768
[tableName],
1274312769
);

0 commit comments

Comments
 (0)