From 8a43d5d63321e4b00e06c850978d8e624f3e6814 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 24 Aug 2026 13:14:20 +0000 Subject: [PATCH] fix(driver-sql): order the MySQL introspectForeignKeys read by the key ordinal (#11379) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `information_schema.KEY_COLUMN_USAGE` was read with no `ORDER BY`, so the row order of a composite foreign key's columns was whatever the plan yielded. `IntrospectedForeignKey` is a flat per-column record with no ordinal field, so a composite key is expressed as ordered sibling rows and the order is load-bearing. Measured on MySQL 8.0.46: this predicate returned key order unpinned, but the sibling `introspectPrimaryKeys` predicate over the same view, in the same session, returned an out-of-sequence primary key in column order. The view does not preserve the ordinal for free — which order you get is decided by the WHERE clause. The pin is on the emitted SQL rather than on the row order, because a row-order assertion passes with or without the clause on this predicate. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01VK8rFDtg8eREaxBGX99Csn --- ...ql-mysql-fk-introspection-ordinal-order.md | 31 +++ ...-introspect-fk-mysql-ordinal-order.test.ts | 193 ++++++++++++++++++ packages/drivers/driver-sql/src/sql-driver.ts | 28 +++ 3 files changed, 252 insertions(+) create mode 100644 .changeset/driver-sql-mysql-fk-introspection-ordinal-order.md create mode 100644 packages/drivers/driver-sql/src/sql-driver-11379-introspect-fk-mysql-ordinal-order.test.ts diff --git a/.changeset/driver-sql-mysql-fk-introspection-ordinal-order.md b/.changeset/driver-sql-mysql-fk-introspection-ordinal-order.md new file mode 100644 index 0000000000..82aae4191c --- /dev/null +++ b/.changeset/driver-sql-mysql-fk-introspection-ordinal-order.md @@ -0,0 +1,31 @@ +--- +"@objectstack/driver-sql": patch +--- + +fix(driver-sql): order the MySQL `introspectForeignKeys` read by the key ordinal (#11379) + +`SqlDriver.introspectForeignKeys`' MySQL arm read `information_schema.KEY_COLUMN_USAGE` +with no `ORDER BY`. `ORDINAL_POSITION` is the key ordinal and was selected by neither the +projection nor an order clause, so the row order of a composite foreign key's columns was +whatever the query plan happened to yield. + +That order is load-bearing. `IntrospectedForeignKey` is a flat per-column record with no +ordinal field, so a composite key is expressed as **ordered sibling rows** — `(x, y) +references p (a, b)` is `x -> p.a` then `y -> p.b`, and there is nothing for a consumer to +recover the position from if the rows arrive permuted. The Postgres arm pins this with +`ORDER BY … k.ord`; the MySQL arm was leaving it to the optimizer. + +This is a determinism fix rather than the repair of a wrong answer, and the measurement is +what distinguishes the two. On MySQL 8.0.46, a foreign key declared out of column sequence +— `foreign key (second_col, first_col) references ooo_parent (pa, pb)` — came back in key +order through this predicate with no `ORDER BY` at all. But on the same server, in the +same session, over the same view, the sibling `introspectPrimaryKeys` predicate +(`CONSTRAINT_NAME = 'PRIMARY'`) returned an out-of-sequence primary key in **column** +order — `carrier_code` at ordinal 2 ahead of `shipment_id` at ordinal 1. `KEY_COLUMN_USAGE` +therefore does not preserve the ordinal for free on this server: which of the two orders +you get is decided by the `WHERE` clause, and nothing declared that. The foreign-key +predicate was on the lucky side of a choice nobody made. + +Consumers that read composite foreign keys through `introspectSchema` — federated-object +codegen, the persisted `external_catalog` (ADR-0015), and schema-drift comparison — now get +the declared key order from MySQL by construction rather than by plan choice. diff --git a/packages/drivers/driver-sql/src/sql-driver-11379-introspect-fk-mysql-ordinal-order.test.ts b/packages/drivers/driver-sql/src/sql-driver-11379-introspect-fk-mysql-ordinal-order.test.ts new file mode 100644 index 0000000000..f8cda1bbf2 --- /dev/null +++ b/packages/drivers/driver-sql/src/sql-driver-11379-introspect-fk-mysql-ordinal-order.test.ts @@ -0,0 +1,193 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#11379] `introspectForeignKeys`' MySQL arm must ORDER BY the key ordinal. + * + * ## Why this pin is structural, and why that is the honest shape + * + * The card was filed as an OBSERVATION, and it says so up front: it **did not + * reproduce**. Re-measured here on live MySQL 8.0.46 before this pin was + * written, with the reporter's own fixture — a key declared out of column + * sequence, `foreign key (second_col, first_col) references ooo_parent + * (pa, pb)`, so that "key order" and "column order" are different answers — + * the arm's query WITHOUT `ORDER BY` returned: + * + * second_col -> pa (ORDINAL_POSITION 1) + * first_col -> pb (ORDINAL_POSITION 2) + * + * which is key order: the correct answer, unpinned. So a behavioural pin — + * "the columns come back in ordinal order" — is **vacuous** on this predicate. + * It passes today, it passes with the fix, and it passes with the fix reverted. + * A green that cannot go red is not evidence, so this file does not write one, + * and does not dress one up as a guard. + * + * ## What was measured that makes the fix more than cosmetic + * + * On the SAME server, in the SAME session, against the SAME view, the sibling + * `introspectPrimaryKeys` predicate — `CONSTRAINT_NAME = 'PRIMARY'` instead of + * `REFERENCED_TABLE_NAME IS NOT NULL` — read an out-of-sequence primary key + * `PRIMARY KEY (shipment_id, carrier_code)` back as: + * + * carrier_code (ORDINAL_POSITION 2) + * shipment_id (ORDINAL_POSITION 1) + * + * i.e. COLUMN order, the wrong answer — reproducing #11101's measurement + * exactly. `KEY_COLUMN_USAGE` therefore does NOT preserve the ordinal for free + * on this server: which of the two orders comes back is decided by the WHERE + * clause, and nothing declares that. The foreign-key predicate is currently on + * the lucky side of a choice nobody made. That is what the `ORDER BY` removes, + * and it is why "it did not reproduce" is not a reason to leave it out. + * + * ⛔ Deliberately NOT attempted here: proving that some plan shape on some + * supported MySQL version returns the foreign-key predicate out of ordinal + * order. That needs a fixture large enough to change the plan, and the card + * rules it out as beyond what an observation should spend. + * + * ## So the pin is on the emitted SQL, and it can go red + * + * Removing `ORDER BY ORDINAL_POSITION` from the arm turns the first test in + * this file red — verified by doing it, not by assuming it. That is the whole + * claim this file makes, and it is stated no more strongly than that. + * + * ⚠️ It is a pin on **this method's** emitted statement, captured at the knex + * seam — never a grep of the source file for the literal. `sql-driver.ts` + * contains `ORDER BY ORDINAL_POSITION` three times (`introspectColumnOrder`, + * this method, and `introspectPrimaryKeys`), so a file-level match would report + * this arm as fixed while it was still unordered — which is exactly how a live + * defect gets closed as already-absorbed. + * + * The second test pins the other half of the same contract, which lives in TS + * rather than in SQL: the arm must EMIT the rows in the order the server + * returned them. A sort, a `Map` keyed by column name, or a regrouping pass + * inserted into that loop would silently undo the `ORDER BY` above, and unlike + * the row order itself, that one is fully determined here and really can fail. + */ + +import { describe, it, expect, afterEach } from 'vitest'; +import { SqlDriver } from '../src/index.js'; + +/** One row of `KEY_COLUMN_USAGE` as the MySQL arm's projection aliases it. */ +interface FkRow { + column_name: string; + referenced_table: string; + referenced_column: string; + constraint_name: string; +} + +/** + * A driver that DECLARES MySQL and answers from a canned result set. + * + * `isMysql` is derived from `config.client` and from nothing else, and the + * constructor already keeps `this.config` as the DECLARED target while the knex + * instance points somewhere else (#6743 — that split is the documented + * behaviour of this class, not a hole this test opens). So re-declaring the + * client after construction drives the REAL dispatch through the REAL getter, + * while the transport stays an in-memory SQLite handle that is never asked to + * execute anything. No MySQL server, so this pin runs in every CI job rather + * than only in the provisioned live-matrix one. + */ +class MysqlFkEmissionProbe extends SqlDriver { + /** Every statement the arm handed to knex, in order. */ + readonly emitted: { sql: string; bindings: unknown }[] = []; + + constructor(private readonly rows: FkRow[]) { + super({ + client: 'better-sqlite3', + connection: { filename: ':memory:' }, + useNullAsDefault: true, + }); + + (this.config as { client?: string }).client = 'mysql2'; + + const knex = this.knex as unknown as Record; + // knex defines `raw` as non-writable (but configurable), so a plain + // assignment throws — the swap has to go through `defineProperty`. + Object.defineProperty(knex, 'raw', { + configurable: true, + value: (sql: unknown, bindings: unknown) => { + this.emitted.push({ sql: String(sql), bindings }); + // mysql2 hands knex back `[rows, fields]`; the arm reads `result[0]`. + return [this.rows, []]; + }, + }); + } + + foreignKeys(table: string) { + return this.introspectForeignKeys(table); + } + + /** The one statement this method emitted. Fails loudly if it was not one. */ + soleStatement(): string { + expect( + this.emitted.length, + 'introspectForeignKeys emitted no statement, or more than one — the ' + + 'capture below would be measuring nothing. Did the dialect dispatch ' + + 'stop reaching the MySQL arm?', + ).toBe(1); + return this.emitted[0]!.sql; + } +} + +/** + * The reporter's fixture, as rows: `(second_col, first_col)` referencing + * `(pa, pb)` — a key declared out of column sequence, so key order and column + * order are different answers and an accidental sort is visible. + */ +const OUT_OF_SEQUENCE_ROWS: FkRow[] = [ + { + column_name: 'second_col', + referenced_table: 'ooo_parent', + referenced_column: 'pa', + constraint_name: 'fk_ooo', + }, + { + column_name: 'first_col', + referenced_table: 'ooo_parent', + referenced_column: 'pb', + constraint_name: 'fk_ooo', + }, +]; + +describe('introspectForeignKeys (MySQL) orders a composite key by the ordinal (#11379)', () => { + let probe: MysqlFkEmissionProbe | undefined; + + afterEach(async () => { + await (probe as unknown as { knex?: { destroy(): Promise } } | undefined)?.knex?.destroy(); + probe = undefined; + }); + + it('emits ORDER BY ORDINAL_POSITION on the KEY_COLUMN_USAGE read', async () => { + probe = new MysqlFkEmissionProbe(OUT_OF_SEQUENCE_ROWS); + await probe.foreignKeys('ooo_child'); + + const sql = probe.soleStatement(); + + // Control first: the captured statement really is the foreign-key read of + // this method, not some other statement that happened past the seam. Without + // this, the assertion below could go green on the wrong query — the + // file-level-grep failure mode, one layer in. + expect(sql).toMatch(/information_schema\.KEY_COLUMN_USAGE/i); + expect(sql).toMatch(/REFERENCED_TABLE_NAME IS NOT NULL/i); + expect(sql).not.toMatch(/CONSTRAINT_NAME\s*=\s*'PRIMARY'/i); + + // The pin: the ordinal clause is in THIS statement, and it comes after the + // predicate that identifies it, so it cannot be satisfied by a clause that + // belongs to a different read. + expect(sql).toMatch(/REFERENCED_TABLE_NAME IS NOT NULL[\s\S]*ORDER BY\s+ORDINAL_POSITION/i); + }); + + it('emits the rows in the order the server returned them', async () => { + probe = new MysqlFkEmissionProbe(OUT_OF_SEQUENCE_ROWS); + const keys = await probe.foreignKeys('ooo_child'); + + // `IntrospectedForeignKey` is a flat per-column record with no ordinal + // field, so ORDERED SIBLING ROWS is the only way a composite key is + // expressed (#11324). Re-sorting or regrouping in the arm would undo the + // `ORDER BY` above without touching the SQL. + expect(keys.map((k) => `${k.columnName} -> ${k.referencedTable}.${k.referencedColumn}`)).toEqual([ + 'second_col -> ooo_parent.pa', + 'first_col -> ooo_parent.pb', + ]); + expect(keys.every((k) => k.constraintName === 'fk_ooo')).toBe(true); + }); +}); diff --git a/packages/drivers/driver-sql/src/sql-driver.ts b/packages/drivers/driver-sql/src/sql-driver.ts index c799779952..46471a6291 100644 --- a/packages/drivers/driver-sql/src/sql-driver.ts +++ b/packages/drivers/driver-sql/src/sql-driver.ts @@ -13883,6 +13883,33 @@ export class SqlDriver implements IDataDriver { }); } } else if (this.isMysql) { + // `KEY_COLUMN_USAGE.ORDINAL_POSITION` IS the key ordinal, and it is + // selected by neither the projection nor an order clause — so without + // `ORDER BY` the row order of a composite key's columns is whatever the + // plan yields. The order is load-bearing for the same reason it is on + // the Postgres arm above: #11324 made a composite foreign key ORDERED + // SIBLING ROWS in this flat per-column record — `(x, y) references + // p (a, b)` is `x -> p.a` then `y -> p.b` — and `IntrospectedForeignKey` + // carries no ordinal field for a consumer to recover the position from. + // + // ⚠️ This clause is NOT a repair of a wrong answer, and the measurement + // that says so is the reason to keep it. On MySQL 8.0.46, a key declared + // out of column sequence — `foreign key (second_col, first_col) + // references ooo_parent (pa, pb)` — came back in KEY order through THIS + // predicate with no `ORDER BY` at all: the right answer, unpinned. But + // on the same server, in the same session, the sibling + // `introspectPrimaryKeys` predicate over the SAME view returned COLUMN + // order for an out-of-sequence primary key — `carrier_code` (ordinal 2) + // ahead of `shipment_id` (ordinal 1) — reproducing #11101 exactly. So + // this view does not preserve the ordinal for free on this server: + // WHICH of the two orders you get is decided by the WHERE clause, and + // nothing declares that. (Same conclusion as the primary-key arm: the + // InnoDB folklore that the view "tends to" return ordinal order does not + // hold on an out-of-sequence key.) What the clause removes is a + // dependence on a plan choice nobody chose — see the pin in + // `sql-driver-11379-introspect-fk-mysql-ordinal-order.test.ts`, which is + // deliberately a pin on the emitted SQL rather than on the row order, + // because a row-order assertion passes here with or without this line. const result = await this.knex.raw( ` SELECT @@ -13894,6 +13921,7 @@ export class SqlDriver implements IDataDriver { WHERE TABLE_SCHEMA = DATABASE() AND TABLE_NAME = ? AND REFERENCED_TABLE_NAME IS NOT NULL + ORDER BY ORDINAL_POSITION `, [tableName], );