From fab9190c57d03d6e38cdb576c7f37e165972a21c Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 9 Sep 2026 16:09:46 +0000 Subject: [PATCH 1/2] fix(cli): a generated migration carries the field-level unique index the driver creates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both migration formats emitted the table and none of the object's declared uniqueness. Measured on live PostgreSQL 16.13, one object through all three producers, pg_indexes per schema: driver probe_pkey, uniq_probe_keyed_unique sql gen probe_pkey ts gen probe_pkey Two rows with the same value in a `unique: true` field were refused by the platform's table and accepted by both generated ones, with nothing reporting it. The key set was already computed here for #16091's column widths; only the index it implies was missing. The sql format emits an inline `CONSTRAINT UNIQUE (...)` — what knex's `table.unique(columns, { indexName })` compiles to on PostgreSQL, so both pg_indexes and pg_constraint agree with the driver — and the ts format emits that knex call itself. Names come from a transcription of driver-sql's `buildIndexName` (#5726 forbids a static driver import from a CLI production module), pinned against the driver's own export. Two shapes stay unemitted and are now NAMED in the generated file rather than dropped: the ADR-0120 D3 organization-scoped composite, whose COALESCE key part knex's schema builder cannot express, and object-level `indexes[]`, which a second normalizer reads with different token semantics. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015QE8qk46e5CHJxyQEUjbf8 --- ...generate-declared-unique-index.pin.test.ts | 536 ++++++++++++++++++ .../generate-string-family-width.pin.test.ts | 7 +- packages/cli/src/commands/generate.ts | 224 +++++++- 3 files changed, 760 insertions(+), 7 deletions(-) create mode 100644 packages/cli/src/commands/generate-declared-unique-index.pin.test.ts diff --git a/packages/cli/src/commands/generate-declared-unique-index.pin.test.ts b/packages/cli/src/commands/generate-declared-unique-index.pin.test.ts new file mode 100644 index 0000000000..cee4962373 --- /dev/null +++ b/packages/cli/src/commands/generate-declared-unique-index.pin.test.ts @@ -0,0 +1,536 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * THE #16317 PIN: a FIELD-LEVEL `unique` declaration reaches the generated + * table as the same index `driver-sql` creates for the same object. + * + * ## The defect + * + * Both migration formats emitted a table and no constraint at all. Driven on + * live PostgreSQL 16.13 — one object, three schemas, one producer each, + * `pg_indexes` read back per schema: + * + * ``` + * { name: 'probe', fields: { keyed_unique: { type: 'text', unique: true, maxLength: 100 } } } + * + * driver probe_pkey, uniq_probe_keyed_unique + * sql gen probe_pkey + * ts gen probe_pkey + * ``` + * + * Two rows with the same `keyed_unique` value were REFUSED by the platform's + * table (`23505 ... violates unique constraint "uniq_probe_keyed_unique"`) and + * ACCEPTED by both generated ones, with nothing reporting it. A scaffold that + * creates the table for an object dropped a uniqueness guarantee the object + * declares. After the repair, on the same cluster: + * + * ``` + * driver probe_pkey, uniq_probe_keyed_unique + * sql gen probe_pkey, uniq_probe_keyed_unique + * ts gen probe_pkey, uniq_probe_keyed_unique + * ``` + * + * ...and the duplicate insert is refused by all three, each naming the same + * constraint. The COLUMN — the #16091 result this change must not spend — read + * `character varying(100)` on all three both before and after. + * + * ## What this pin does NOT claim, stated so nobody reads it as closed + * + * Two declaration shapes are deliberately unemitted, and this file asserts that + * they are unemitted *and named*, never that they are handled: + * + * 1. The ADR-0120 D3 ORGANIZATION-SCOPED form — `(COALESCE(, + * '__global__'), )`. Emitting the bare composite instead would be + * worse than emitting nothing: under SQL's NULL-distinct UNIQUE a bare + * `(organization_id, field)` constrains NOTHING on rows with no + * organization, which on a single-tenant stack is every row (#5030) — a + * constraint advertised and not delivered, which is the failure Prime + * Directive #10 names. + * 2. OBJECT-LEVEL `indexes[]`. `normalizeDeclaredIndex` is a second + * normalizer that reads the same `unique: true` token DIFFERENTLY (verbatim + * as global, a maintainer ruling), so it is a second transcription with a + * second pin, not a loop added to this one. + * + * ⭐ Measured for the record, because "can the TypeScript format express an + * expression key part through knex at all" was the open question that kept the + * scoped form off this change (knex 3.3.0, live PostgreSQL 16.13): + * + * ``` + * table.unique([knex.raw("COALESCE(...)"), 'f'], {indexName}) + * -> knex compiles ALTER TABLE ... ADD CONSTRAINT ... UNIQUE (COALESCE(...), "f") + * -> PostgreSQL: syntax error at or near "(" (a UNIQUE CONSTRAINT + * takes no expression key part; only a unique INDEX does) + * table.unique(["COALESCE(...)", 'f'], {indexName}) + * -> knex quotes it as an identifier + * -> PostgreSQL: column "COALESCE(""organization_id"", '__global__')" does not exist + * db.raw(`CREATE UNIQUE INDEX ... (COALESCE("organization_id", '__global__'), "f")`) + * -> ACCEPTED; materialised as + * CREATE UNIQUE INDEX k_c ON t USING btree (COALESCE(organization_id, '__global__'::character varying), f) + * ``` + * + * So the two formats are NOT unequally capable — the emitted `up(db)` receives a + * knex handle and `db.raw` is exactly the seam `SqlDriver.createNullSafeUniqueIndex` + * already uses for this — but knex's SCHEMA BUILDER cannot express it in either + * format, so the scoped form costs a raw statement rather than another + * `table.unique(...)` line. That is a fact for whoever takes the scoped half, + * recorded here rather than acted on. + * + * ## Why this pin reads the driver instead of asserting the names + * + * The same reason `generate-string-family-width.pin.test.ts` gives: the whole + * shape of this card is "the generator disagrees with the driver", so a pin that + * transcribed `uniq_probe_keyed_unique` would re-create the defect one layer up + * and stay green the day the driver's naming moves. Every name and every key-part + * ordering here is recomputed from `driver-sql`'s own exported + * `uniqueIndexesFromFields` / `buildIndexName` / `GLOBAL_TENANT`. + * + * ⭐ And the leaves are not the authority. Above the differential sits THE REAL + * CHAIN: all three producers driven into ONE in-memory better-sqlite3 database, + * one table each, with the indexes read back out of the database's own catalog + * and the duplicate row offered to each table. That is the live-PostgreSQL + * acceptance above, transplanted into a tier that runs everywhere — the + * differential underneath localises a failure to one builder and is explicitly + * NOT the authority where the two could disagree. + * + * ⚠️ One SQLite artifact, named so nobody reads it as a finding: SQLite ignores + * the identifier on a table-level `CONSTRAINT UNIQUE (...)` and + * materialises `sqlite_autoindex__N` instead. The sql format's DDL is a + * PostgreSQL claim by #15521 and its constraint NAME is asserted textually + * (against the driver's own computed name) plus on the live cluster above; what + * the SQLite chain carries for that format is the KEY PARTS and the ENFORCEMENT. + * The ts format's `table.unique(cols, { indexName })` is a named index on both + * engines and is compared by name. + * + * ⚠️ Parse-time defaults are outside this comparison ON PURPOSE. An `autonumber` + * field that omits `unique` is `unique: 'organization'` by contract, materialized + * in `FieldSchema`'s `.overwrite()` tail — so it reaches BOTH producers already + * present, and every case here feeds the generator and the driver the SAME + * object. A pin that fed one a parsed object and the other a raw one would be + * measuring the parser. + */ + +import { SqlDriver, GLOBAL_TENANT, buildIndexName, uniqueIndexesFromFields } from '@objectstack/driver-sql'; +import { afterAll, describe, expect, it } from 'vitest'; + +import { generateMigrationSql, generateMigrationTs } from './generate.js'; + +// ── The oracle ────────────────────────────────────────────────────────────── + +/** + * The driver's own `protected` judgments, reached by widening rather than + * re-derived — the technique `generate-string-family-width.pin.test.ts` + * established: `protected` is a compile-time visibility rule, so a subclass + * publishes the driver's OWN body without copying a character of it. + */ +class DriverOracle extends SqlDriver { + public readonly warnings: string[] = []; + + protected override logger = { + warn: (msg: string) => { this.warnings.push(msg); }, + error: (msg: string) => { this.warnings.push(msg); }, + }; + + /** `SqlDriver.computeTenantField`, unmodified. */ + public tenantFieldFor(object: { fields?: Record; tenancy?: unknown }): string | null { + return this.computeTenantField(object); + } + + /** The knex handle this driver opened — the database every producer writes into. */ + public get db(): any { + return this.knex; + } +} + +function newOracle(): DriverOracle { + return new DriverOracle({ + client: 'better-sqlite3', + connection: { filename: ':memory:' }, + useNullAsDefault: true, + } as any); +} + +/** One index as the catalog reports it: `{ name, unique, columns | expression }`. */ +interface PhysicalIndexRow { + name: string; + unique: boolean; + /** SQLite's own provenance: `pk` | `u` (a UNIQUE constraint) | `c` (CREATE INDEX). */ + origin: string; + /** The key parts as SQLite reports them; `null` for an expression key part. */ + columns: Array; +} + +/** + * Every index on a table, MINUS the primary key. + * + * The `id` PRIMARY KEY is a unique index on all three producers and always has + * been (`probe_pkey` in the card's own PostgreSQL table); leaving it in would + * make "all three agree" true for a reason that has nothing to do with this + * card. `origin` is SQLite's own answer, not a name heuristic. + */ +async function physicalIndexes(db: any, table: string): Promise { + const list = (await db.raw(`PRAGMA index_list("${table}")`)) as Array<{ + name: string; + unique: number; + origin: string; + }>; + const out: PhysicalIndexRow[] = []; + for (const row of list) { + if (row.origin === 'pk') continue; + const info = (await db.raw(`PRAGMA index_info("${row.name}")`)) as Array<{ name: string | null }>; + out.push({ name: row.name, unique: row.unique === 1, origin: row.origin, columns: info.map((c) => c.name) }); + } + return out.sort((a, b) => a.name.localeCompare(b.name)); +} + +/** Statements of the sql format, comment lines removed. */ +function sqlStatements(sql: string): string[] { + const stripped = sql.split('\n').filter((l) => !l.trim().startsWith('--')).join('\n'); + return stripped.split(';').map((s) => s.trim()).filter(Boolean); +} + +/** + * The ts format's module, loaded WITHOUT touching the filesystem. + * + * The emitted source is TypeScript only in its two annotations; stripping them + * and turning the two `export`s into locals makes it an ordinary function body. + * ⛔ Not a parse check — `generate-emission-parses.test.ts` owns that; this is + * how the third producer gets RUN. + */ +function loadEmittedTs(ts: string): { up: (db: any) => Promise; down: (db: any) => Promise } { + const js = ts + .replace(/: any/g, '') + .replace(/: Promise/g, '') + .replace(/export async function/g, 'async function'); + // eslint-disable-next-line no-new-func + return new Function(`${js}\nreturn { up, down };`)() as { up: (db: any) => Promise; down: (db: any) => Promise }; +} + +// ── The corpus ────────────────────────────────────────────────────────────── + +interface Probe { + label: string; + object: Record; + /** What the object's declarations mean for the emitters, asserted below. */ + expect: 'emitted' | 'scoped-not-emitted' | 'no-column' | 'none'; +} + +const ORG = { type: 'text', maxLength: 64 } as const; +const KEYED = { type: 'text', maxLength: 100 } as const; + +const CORPUS: Probe[] = [ + { + label: "the card's own object — plain `unique: true`, no organization column", + object: { name: 'p_plain', fields: { keyed_unique: { ...KEYED, unique: true } } }, + expect: 'emitted', + }, + { + label: "`unique: 'global'` beside an organization column — platform-wide, single-column", + object: { name: 'p_global', fields: { organization_id: ORG, f: { ...KEYED, unique: 'global' } } }, + expect: 'emitted', + }, + { + label: '`unique: true` beside an organization column — the ADR-0120 D3 scoped composite', + object: { name: 'p_scoped_true', fields: { organization_id: ORG, f: { ...KEYED, unique: true } } }, + expect: 'scoped-not-emitted', + }, + { + label: "explicit `unique: 'organization'` — the same scoped composite", + object: { name: 'p_scoped_word', fields: { organization_id: ORG, f: { ...KEYED, unique: 'organization' } } }, + expect: 'scoped-not-emitted', + }, + { + label: 'unique ON the organization column itself — "one row per tenant" stays single-column', + object: { name: 'p_on_tenant', fields: { organization_id: { ...ORG, unique: true } } }, + expect: 'emitted', + }, + { + label: '`tenancy: { enabled: false }` — the explicit opt-out beats column presence', + object: { + name: 'p_no_tenancy', + tenancy: { enabled: false }, + fields: { organization_id: ORG, f: { ...KEYED, unique: true } }, + }, + expect: 'emitted', + }, + { + label: 'a declared `tenancy.tenantField` naming a real field', + object: { + name: 'p_declared_tenant', + tenancy: { tenantField: 'org' }, + fields: { org: ORG, f: { ...KEYED, unique: true } }, + }, + expect: 'scoped-not-emitted', + }, + { + label: 'a name past the 60-character identifier budget — hash-suffixed', + object: { + name: 'p_' + 'n'.repeat(64), + fields: { keyed_unique_with_a_long_name: { ...KEYED, unique: true } }, + }, + expect: 'emitted', + }, + { + label: 'unique on a VIRTUAL field — the driver materialises no column, so neither may we', + object: { name: 'p_virtual', fields: { f: { type: 'formula', unique: true } } }, + expect: 'no-column', + }, + { + label: 'two unique fields at different scopes on one object', + object: { + name: 'p_two', + fields: { a: { ...KEYED, unique: true }, b: { type: 'email', maxLength: 80, unique: 'global' } }, + }, + expect: 'emitted', + }, + { + label: 'no unique declaration at all', + object: { name: 'p_none', fields: { f: KEYED } }, + expect: 'none', + }, + { + label: 'an explicit `unique: false` opt-out', + object: { name: 'p_false', fields: { f: { ...KEYED, unique: false } } }, + expect: 'none', + }, +]; + +const configFor = (object: Record) => ({ objects: { o: object } }) as Record; + +/** The index descriptors the DRIVER's own normalizer says an object asks for. */ +function driverIndexes(oracle: DriverOracle, object: Record) { + return uniqueIndexesFromFields( + String(object.name), + (object.fields ?? {}) as Record, + oracle.tenantFieldFor(object), + ); +} + +// ──────────────────────────────────────────────────────────────────────────── +// A. THE REAL CHAIN — three producers, one database, the catalog as the witness +// ──────────────────────────────────────────────────────────────────────────── + +describe('#16317 — the index each generator emits is the index the driver CREATES', () => { + const drivers: DriverOracle[] = []; + afterAll(async () => { for (const d of drivers) await d.db.destroy(); }); + + async function threeTables(object: Record) { + const base = String(object.name); + const driverDrv = newOracle(); + const sqlDrv = newOracle(); + const tsDrv = newOracle(); + drivers.push(driverDrv, sqlDrv, tsDrv); + + await driverDrv.initObjects([object as any]); + + const sql = generateMigrationSql(configFor(object)); + for (const stmt of sqlStatements(sql)) await sqlDrv.db.raw(stmt); + + const ts = generateMigrationTs(configFor(object)); + await loadEmittedTs(ts).up(tsDrv.db); + + return { + sqlText: sql, + tsText: ts, + driver: await physicalIndexes(driverDrv.db, base), + sqlgen: await physicalIndexes(sqlDrv.db, base), + tsgen: await physicalIndexes(tsDrv.db, base), + dbs: { driver: driverDrv.db, sqlgen: sqlDrv.db, tsgen: tsDrv.db }, + }; + } + + /** The key-part shape of every UNIQUE index on a table, name-free. */ + const uniqueShapes = (rows: PhysicalIndexRow[]) => + rows.filter((r) => r.unique).map((r) => r.columns.join(',')).sort(); + + for (const probe of CORPUS) { + it(`${probe.label}`, async () => { + const built = await threeTables(probe.object); + const wanted = driverIndexes(newOracle(), probe.object); + + // ⭐ THE AUTHORITY. Whatever the driver's own table carries, the ts + // format's table carries too — by NAME, because both go through knex's + // named-index spelling — except for the two declared exclusions. + const emittable = probe.expect === 'emitted'; + if (emittable) { + expect(uniqueShapes(built.tsgen)).toEqual(uniqueShapes(built.driver)); + expect(uniqueShapes(built.sqlgen)).toEqual(uniqueShapes(built.driver)); + const driverNames = built.driver.filter((r) => r.unique).map((r) => r.name).sort(); + const tsNames = built.tsgen.filter((r) => r.unique).map((r) => r.name).sort(); + expect(tsNames).toEqual(driverNames); + // The sql format's identifier: SQLite drops it (see the header), so it + // is read out of the DDL and compared with the driver's own name. + for (const want of wanted) expect(built.sqlText).toContain(`CONSTRAINT "${want.name}" UNIQUE (`); + } + + if (probe.expect === 'scoped-not-emitted') { + // The driver DOES build it — the expression key part is why we do not. + expect(wanted.length).toBeGreaterThan(0); + for (const want of wanted) { + expect(want.nullSafeColumns ?? []).not.toHaveLength(0); + // ⛔ Absence must be loud: named in the generated file, both formats. + expect(built.sqlText).toContain(`NOT EMITTED: UNIQUE index "${want.name}"`); + expect(built.tsText).toContain(`NOT EMITTED: UNIQUE index '${want.name}'`); + expect(built.sqlText).toContain(`COALESCE("${want.nullSafeColumns![0]}", '${GLOBAL_TENANT}')`); + // ...and NOT emitted as the bare composite, which would advertise a + // constraint the table does not carry (#5030). + expect(built.sqlText).not.toContain(`CONSTRAINT "${want.name}" UNIQUE (`); + expect(built.tsText).not.toContain(`indexName: '${want.name}'`); + } + expect(uniqueShapes(built.tsgen)).toHaveLength(0); + expect(uniqueShapes(built.sqlgen)).toHaveLength(0); + } + + if (probe.expect === 'no-column') { + // The driver asks for the index and then skips it — no column was + // materialized. The generator must reach the same end, and say so. + expect(wanted.length).toBeGreaterThan(0); + expect(uniqueShapes(built.driver)).toHaveLength(0); + expect(uniqueShapes(built.tsgen)).toHaveLength(0); + expect(uniqueShapes(built.sqlgen)).toHaveLength(0); + for (const want of wanted) { + expect(built.sqlText).toContain(`NOT EMITTED: UNIQUE index "${want.name}"`); + expect(built.tsText).toContain(`NOT EMITTED: UNIQUE index '${want.name}'`); + } + } + + if (probe.expect === 'none') { + expect(wanted).toHaveLength(0); + expect(uniqueShapes(built.driver)).toHaveLength(0); + expect(uniqueShapes(built.tsgen)).toHaveLength(0); + expect(uniqueShapes(built.sqlgen)).toHaveLength(0); + expect(built.sqlText).not.toContain('NOT EMITTED'); + expect(built.tsText).not.toContain('NOT EMITTED'); + } + }); + } + + /** + * ⭐ THE CARD'S OWN CONSEQUENCE, as a behaviour rather than as a catalog row: + * "two rows with the same `keyed_unique` value are refused by the platform's + * table and accepted by both generated ones". + */ + it("the duplicate row the platform refuses is refused by BOTH generated tables", async () => { + const object = { name: 'p_enforce', fields: { keyed_unique: { ...KEYED, unique: true } } }; + const built = await threeTables(object); + const verdicts: Record = {}; + for (const [who, db] of Object.entries(built.dbs)) { + await db.raw(`INSERT INTO "p_enforce" ("id", "keyed_unique") VALUES ('a', 'dup')`); + try { + await db.raw(`INSERT INTO "p_enforce" ("id", "keyed_unique") VALUES ('b', 'dup')`); + verdicts[who] = 'accepted'; + } catch { + verdicts[who] = 'refused'; + } + } + expect(verdicts).toEqual({ driver: 'refused', sqlgen: 'refused', tsgen: 'refused' }); + }); + + /** + * NON-VACUITY for the whole block. A corpus that built no unique index at all + * would satisfy every `toEqual` above by agreeing on emptiness. + */ + it('the corpus actually exercises every class it claims to', async () => { + const oracle = newOracle(); + drivers.push(oracle); + const counts = { emitted: 0, scoped: 0, noColumn: 0, none: 0, hashed: 0 }; + for (const probe of CORPUS) { + const wanted = driverIndexes(oracle, probe.object); + if (probe.expect === 'emitted') counts.emitted += wanted.length; + if (probe.expect === 'scoped-not-emitted') counts.scoped += wanted.length; + if (probe.expect === 'no-column') counts.noColumn += wanted.length; + if (probe.expect === 'none') counts.none += wanted.length; + for (const w of wanted) if (/_[0-9a-f]{8}$/.test(w.name)) counts.hashed += 1; + } + expect(counts.emitted).toBeGreaterThanOrEqual(5); + expect(counts.scoped).toBeGreaterThanOrEqual(3); + expect(counts.noColumn).toBe(1); + expect(counts.none).toBe(0); + expect(counts.hashed).toBe(1); + }); +}); + +// ──────────────────────────────────────────────────────────────────────────── +// B. THE LEAF DIFFERENTIAL — the transcriptions against the driver's exports +// ──────────────────────────────────────────────────────────────────────────── + +describe('#16317 — the transcriptions in generate.ts are the driver\'s own', () => { + const oracle = newOracle(); + afterAll(async () => { await oracle.db.destroy(); }); + + /** + * `buildIndexName` — swept rather than sampled, and over the truncation + * boundary in particular: a mirror that dropped the hash suffix agrees with + * the driver on every short name and diverges on every long one. + */ + it('every generated identifier is buildIndexName\'s own answer, hash suffix included', () => { + let hashed = 0; + for (const width of [1, 10, 40, 47, 48, 49, 50, 51, 60, 61, 90]) { + const table = 't'.repeat(width); + const object = { name: table, fields: { f: { ...KEYED, unique: true } } }; + const [want] = driverIndexes(oracle, object); + expect(generateMigrationSql(configFor(object))).toContain(`CONSTRAINT "${want.name}" UNIQUE ("f")`); + expect(generateMigrationTs(configFor(object))).toContain(`{ indexName: '${want.name}' }`); + expect(want.name).toBe(buildIndexName(table, ['f'], true)); + if (/_[0-9a-f]{8}$/.test(want.name)) hashed += 1; + } + // Non-vacuity: the sweep really did cross the budget. + expect(hashed).toBeGreaterThanOrEqual(3); + }); + + /** The `__global__` sentinel the unemitted-index note names. */ + it('the sentinel the NOT EMITTED note prints is the driver\'s GLOBAL_TENANT', () => { + const object = { name: 'g_probe', fields: { organization_id: ORG, f: { ...KEYED, unique: true } } }; + expect(generateMigrationSql(configFor(object))).toContain(`'${GLOBAL_TENANT}'`); + expect(generateMigrationTs(configFor(object))).toContain(`'${GLOBAL_TENANT}'`); + }); + + /** + * The scoping rule itself, over the whole corpus: NAME, KEY PARTS and their + * ORDER, recomputed from `uniqueIndexesFromFields`. + */ + it('name, key parts and their order match uniqueIndexesFromFields across the corpus', () => { + let checked = 0; + for (const probe of CORPUS) { + const sql = generateMigrationSql(configFor(probe.object)); + const ts = generateMigrationTs(configFor(probe.object)); + for (const want of driverIndexes(oracle, probe.object)) { + checked += 1; + const scoped = (want.nullSafeColumns ?? []).length > 0; + const emitted = probe.expect === 'emitted'; + const cols = want.columns.map((c) => `"${c}"`).join(', '); + if (emitted) { + expect(scoped).toBe(false); + expect(sql).toContain(`CONSTRAINT "${want.name}" UNIQUE (${cols})`); + expect(ts).toContain( + `table.unique([${want.columns.map((c) => `'${c}'`).join(', ')}], { indexName: '${want.name}' });`, + ); + } else { + expect(sql).toContain(`NOT EMITTED: UNIQUE index "${want.name}" on (${want.columns.join(', ')})`); + expect(ts).toContain(`NOT EMITTED: UNIQUE index '${want.name}' on (${want.columns.join(', ')})`); + } + } + } + expect(checked).toBeGreaterThanOrEqual(9); + }); + + /** + * ⛔ OBJECT-LEVEL `indexes[]` stays unemitted — asserted, so the day someone + * adds it they land here and read the header rather than discovering that a + * second normalizer with different token semantics was silently folded in. + */ + it('object-level indexes[] is still emitted by neither format', () => { + const object = { + name: 'p_declared_idx', + fields: { a: KEYED, b: KEYED }, + indexes: [{ fields: ['a', 'b'], unique: true }], + }; + const sql = generateMigrationSql(configFor(object)); + const ts = generateMigrationTs(configFor(object)); + expect(sql).not.toContain('UNIQUE ('); + expect(ts).not.toContain('table.unique('); + // ...and the column-width answer #16091 computes from the same declaration + // is untouched by that: `a` and `b` are key parts, so they are sized. + expect(sql).toContain('"a" VARCHAR(100)'); + expect(ts).toContain("table.string('a', 100)"); + }); +}); diff --git a/packages/cli/src/commands/generate-string-family-width.pin.test.ts b/packages/cli/src/commands/generate-string-family-width.pin.test.ts index 1bdb42850e..74e3db943b 100644 --- a/packages/cli/src/commands/generate-string-family-width.pin.test.ts +++ b/packages/cli/src/commands/generate-string-family-width.pin.test.ts @@ -103,7 +103,12 @@ * * ⚠️ "A generated migration emits no index, so no generated column is * ever keyed" is FALSE and was this pin's own first answer. It describes - * the generator's OUTPUT; the driver keys on the object's INPUT. Driven on + * the generator's OUTPUT; the driver keys on the object's INPUT. Its + * premise is now false as well — since #16317 the generators DO emit the + * field-level unique index (`generate-declared-unique-index.pin.test.ts`) + * — and the argument is unchanged by that: the declaration sets read here + * stay strictly wider than what that emitter emits, so deriving one from + * the other in either direction re-creates this defect. Driven on * live PostgreSQL 16.13, `{ type: 'text', unique: true, maxLength: 100 }` * built `varchar(100)` on the platform and TEXT in both generated tables, * and a 300-character write was REFUSED by the driver's table (`22001 diff --git a/packages/cli/src/commands/generate.ts b/packages/cli/src/commands/generate.ts index a9b5f34491..b5078da01b 100644 --- a/packages/cli/src/commands/generate.ts +++ b/packages/cli/src/commands/generate.ts @@ -2,6 +2,7 @@ import { Args, Command, Flags } from '@oclif/core'; import chalk from 'chalk'; +import { createHash } from 'node:crypto'; import fs from 'fs'; import path from 'path'; @@ -1547,12 +1548,20 @@ function tenantFieldOf(obj: Record): string | null { * * ⭐ This is what the text family branches on, and it is read off the OBJECT'S * DECLARATIONS — `field.unique` and `indexes[]` — never off anything either - * generator emits. A generated migration still emits no `CREATE INDEX`; that is - * a fact about this generator's OUTPUT and it is not the question. The driver - * asks what the object DECLARES, so a `Field.text({ unique: true, maxLength: - * 100 })` is `varchar(100)` on the platform and must be `varchar(100)` here. - * Reasoning from the emitted output instead ("no index is emitted, so nothing - * is ever keyed") is how this arm was first got wrong. + * generator emits. The driver asks what the object DECLARES, so a + * `Field.text({ unique: true, maxLength: 100 })` is `varchar(100)` on the + * platform and must be `varchar(100)` here. Reasoning from the emitted output + * instead ("no index is emitted, so nothing is ever keyed") is how this arm was + * first got wrong. + * + * ⚠️ That reasoning is now wrong in a SECOND way, and the sentence that used to + * stand here — "a generated migration still emits no `CREATE INDEX`" — is no + * longer true: {@link uniqueIndexesForObject} emits the field-level ones + * (#16317). It is still not the question. The declaration sets this function + * reads are strictly WIDER than what that emitter emits — object-level + * `indexes[]` and the organization-scoped expression form are declared here and + * emitted nowhere — so deriving one from the other in either direction + * re-creates the defect this warning was first written for. * * ⚠️ Deliberately NOT filtered by which columns this generator goes on to emit, * for the same reason the driver's is not filtered by `physicalColumns`: @@ -1613,6 +1622,156 @@ function indexKeyColumns(obj: Record): ReadonlySet { return out; } +/** + * `driver-sql`'s `buildIndexName`, for a generated migration. + * + * The names have to agree character for character or the two producers do not + * converge: `syncDeclaredIndexes` skips an index whose NAME it already finds on + * the table, so a generated table carrying the same constraint under a + * different identifier gets a SECOND, redundant index on the first boot — and + * `schema-drift.ts` then reports the generator's one as an orphan to drop. + * + * Transcribed rather than imported for the reason every mirror in this file is + * (#5726): these generators are SYNCHRONOUS and a CLI production module may + * only `await import()` a driver package. `generate-declared-unique-index.pin.test.ts` + * is what keeps the transcription honest — it recomputes every name from the + * driver's own exported `uniqueIndexesFromFields` and compares. + */ +/** + * `driver-sql`'s `GLOBAL_TENANT` — the sentinel the ADR-0120 D3 NULL-safe + * organization key part folds a NULL organization onto. + * + * Transcribed for the same #5726 reason as the rest of this block, and it + * reaches only a COMMENT in the generated file: this format emits no expression + * key part, so the sentinel is here to NAME the index that was not emitted, not + * to build one. The pin compares it against the driver's own export. + */ +const GLOBAL_TENANT_KEY = '__global__'; + +const INDEX_NAME_MAX = 60; +/** Chars kept from `_
` before the `_` suffix of a truncated name. */ +const INDEX_NAME_HEAD = INDEX_NAME_MAX - 9; + +function buildIndexName(table: string, columns: string[], unique: boolean): string { + const prefix = unique ? 'uniq' : 'idx'; + const base = `${prefix}_${table}_${columns.join('_')}`; + if (base.length <= INDEX_NAME_MAX) return base; + const hash = createHash('sha1').update(base).digest('hex').slice(0, 8); + return `${`${prefix}_${table}`.slice(0, INDEX_NAME_HEAD)}_${hash}`; +} + +/** One index a FIELD-LEVEL `unique` declaration asks for. */ +interface MirroredUniqueIndex { + /** {@link buildIndexName}'s answer — the identifier the driver would use. */ + name: string; + /** The key parts, in the driver's order (tenant column first when scoped). */ + columns: string[]; + /** + * The tenant column whose key part materializes as the ADR-0120 D3 NULL-safe + * expression `COALESCE(, '__global__')` rather than as a bare column, + * or `null` for a plain single-column unique. An expression key part is what + * neither format emits — see {@link uniqueIndexesForObject}. + */ + nullSafeColumn: string | null; +} + +/** + * `driver-sql`'s `uniqueIndexesFromFields` — the FIELD-LEVEL `unique` + * declarations of one object, as concrete index descriptors (#16317). + * + * ⭐ This is the half of the answer {@link indexKeyColumns} already computed and + * threw away. That function resolves the same declarations into a flat SET of + * key COLUMNS, because sizing a column is all it was asked for; the index those + * same declarations imply needs the columns GROUPED, ordered and named, which + * is what this returns. The two read the same three predicates — + * {@link isUniqueScopeDeclared}, {@link isOrganizationScopedUnique} and + * {@link tenantFieldOf} — so they cannot disagree about which fields are keyed. + * + * Scoping rule, transcribed from the driver (ADR-0120 D1/D3): + * - `unique: 'global'` → single-column `(field)`, platform-wide. + * - `unique: true` / `'organization'` on a tenant-scoped table → composite + * `(COALESCE(tenantField, '__global__'), field)`, tenant column FIRST. + * - `unique: true` / `'organization'` with no tenant column → `(field)`. + * - a unique declaration ON the tenant column itself stays single-column — + * `(organization_id, organization_id)` is not a constraint. + * + * ⛔ OBJECT-LEVEL `indexes[]` is deliberately absent here. `normalizeDeclaredIndex` + * is the driver's other normalizer and reads the same token differently — a + * declared `unique: true` is taken VERBATIM as global there, a maintainer ruling + * rather than an oversight — so it is a second transcription with a second pin, + * not a loop added to this one. A generated migration still emits nothing for + * `indexes[]`. + */ +function uniqueIndexesForObject(obj: Record): MirroredUniqueIndex[] { + const fields = (obj?.fields ?? {}) as Record; + const table = String(obj?.name || 'unknown'); + const tenantField = tenantFieldOf(obj); + const out: MirroredUniqueIndex[] = []; + for (const [name, field] of Object.entries(fields)) { + if (!isUniqueScopeDeclared(field?.unique)) continue; + const scoped = + isOrganizationScopedUnique(field.unique) && tenantField != null && tenantField !== name; + const columns = scoped ? [tenantField as string, name] : [name]; + out.push({ + name: buildIndexName(table, columns, true), + columns, + nullSafeColumn: scoped ? (tenantField as string) : null, + }); + } + return out; +} + +/** + * The subset of {@link uniqueIndexesForObject} a format may actually emit, and + * a line for every one it may not. + * + * Two exclusions, and BOTH are the driver's own behaviour rather than a + * convenience here: + * + * 1. A key part with no column. `syncDeclaredIndexes` skips a declared index + * whose columns are not in `physicalColumns` and warns; the generator's + * equivalent of "not materialized" is a field this file emits no column + * for — a VIRTUAL `formula` (#14828). Emitting the index anyway produces + * DDL that refuses to run at all. + * 2. An EXPRESSION key part. `COALESCE(, '__global__')` is what the + * driver builds through raw DDL precisely because knex's schema builder + * cannot express it, and it is NOT interchangeable with the bare composite: + * under SQL's NULL-distinct UNIQUE a bare `(organization_id, field)` + * enforces NOTHING on rows without an organization, which on a + * single-tenant stack is every row (#5030). So emitting the bare composite + * here would ADVERTISE a constraint the table does not carry — worse than + * emitting nothing, and the failure mode Prime Directive #10 names. + * + * ⛔ Neither exclusion is silent. A skipped index is named in the generated file + * itself, with what it would have keyed, because the operator reading that file + * is the only person who can act on it — "Absence must be loud". + */ +function partitionUniqueIndexes( + obj: Record, + emittedColumns: ReadonlySet, +): { emit: MirroredUniqueIndex[]; skipped: Array<{ index: MirroredUniqueIndex; why: string }> } { + const emit: MirroredUniqueIndex[] = []; + const skipped: Array<{ index: MirroredUniqueIndex; why: string }> = []; + for (const index of uniqueIndexesForObject(obj)) { + const missing = index.columns.filter((c) => !emittedColumns.has(c)); + if (missing.length > 0) { + skipped.push({ index, why: `no column is generated for ${missing.join(', ')}` }); + continue; + } + if (index.nullSafeColumn !== null) { + skipped.push({ + index, + why: + `its organization key part is COALESCE("${index.nullSafeColumn}", '${GLOBAL_TENANT_KEY}'), ` + + 'an expression key part this format does not emit; the platform creates it at boot', + }); + continue; + } + emit.push(index); + } + return { emit, skipped }; +} + /** * The column one field takes. * @@ -1758,6 +1917,10 @@ export function generateMigrationSql(config: Record): string { // #16091 — resolved once per object, off the object's own declarations. See // {@link indexKeyColumns}: the text family's width depends on it. const keyColumns = indexKeyColumns(obj); + // #16317 — which columns this table actually gets, so a declared unique + // index over a column no field materialises is skipped rather than emitted + // as DDL that cannot run. Filled by the loop below, read after it. + const emittedColumns = new Set(['id']); for (const [fieldName, fieldDef] of Object.entries(fields)) { const sqlType = fieldTypeToSql( String(fieldDef.type || 'text'), @@ -1774,6 +1937,7 @@ export function generateMigrationSql(config: Record): string { // for the driver's own recorded reason; ⛔ do not restate it here. const notNull = declaredNotNull(fieldDef) ? ' NOT NULL' : ''; fieldLines.push(` "${fieldName}" ${sqlType}${notNull}`); + emittedColumns.add(fieldName); } // #15521 — TIMESTAMPTZ, not TIMESTAMP. Bare `TIMESTAMP` is `timestamp @@ -1831,8 +1995,34 @@ export function generateMigrationSql(config: Record): string { // the driver moves it fails there instead of leaving these quietly wrong. fieldLines.push(' "created_at" TIMESTAMPTZ DEFAULT CURRENT_TIMESTAMP'); fieldLines.push(' "updated_at" TIMESTAMPTZ DEFAULT CURRENT_TIMESTAMP'); + + // #16317 — the object's FIELD-LEVEL `unique` declarations, as the table + // constraints `driver-sql` creates for the same object. Emitted as an + // inline `CONSTRAINT ... UNIQUE` rather than as a following `CREATE UNIQUE + // INDEX` for two reasons: it is what knex's `table.unique(columns, { + // indexName })` — the driver's own call — compiles to on PostgreSQL, so + // both catalogs agree (`pg_indexes` AND `pg_constraint`, not just the + // first); and it stays inside this statement's `IF NOT EXISTS`, which a + // separate `ALTER TABLE ... ADD CONSTRAINT` has no spelling for. + // + // Before this, two rows with the same value in a `unique: true` field were + // REFUSED by the platform's table and ACCEPTED by both generated ones, with + // nothing reporting it — measured on live PostgreSQL 16.13, `pg_indexes` + // for one object driven through all three producers: + // + // driver probe_pkey, uniq_probe_keyed_unique + // sql gen probe_pkey + // ts gen probe_pkey + const { emit, skipped } = partitionUniqueIndexes(obj, emittedColumns); + for (const index of emit) { + const columns = index.columns.map((c) => `"${c}"`).join(', '); + fieldLines.push(` CONSTRAINT "${index.name}" UNIQUE (${columns})`); + } lines.push(fieldLines.join(',\n')); lines.push(');'); + for (const { index, why } of skipped) { + lines.push(`-- NOT EMITTED: UNIQUE index "${index.name}" on (${index.columns.join(', ')}) — ${why}.`); + } lines.push(''); } @@ -1876,6 +2066,9 @@ export function generateMigrationTs(config: Record): string { // {@link indexKeyColumns}: the text family's width depends on it. const keyColumns = indexKeyColumns(obj); + // #16317 — which columns this table actually gets; see the sql format above. + const emittedColumns = new Set(['id']); + lines.push(` await db.schema.createTable('${tableName}', (table: any) => {`); // #15040 — the driver's own line for this column, emitted verbatim: // `table.string('id').primary()`. See `generateMigrationSql` above for the @@ -1901,6 +2094,7 @@ export function generateMigrationTs(config: Record): string { // and not the spec's `isMultiValueField` value predicate. if (fieldDef.multiple) { lines.push(` table.jsonb('${fieldName}')${required};`); + emittedColumns.add(fieldName); continue; } @@ -2070,6 +2264,7 @@ export function generateMigrationTs(config: Record): string { if (colMethod === null) continue; lines.push(` ${colMethod}${required};`); + emittedColumns.add(fieldName); } // #15521 — `driver-sql`'s own audit-column line, emitted verbatim modulo @@ -2084,6 +2279,23 @@ export function generateMigrationTs(config: Record): string { // nullability moves here; the SQL format above pays the default-text row. lines.push(" table.timestamp('created_at').defaultTo(db.fn.now());"); lines.push(" table.timestamp('updated_at').defaultTo(db.fn.now());"); + + // #16317 — the same FIELD-LEVEL `unique` declarations the sql format above + // emits, through the driver's OWN call: `syncDeclaredIndexes` builds a + // plain unique through `table.unique(columns, { indexName: name })`, and + // this is that line with `table` bound to the create-table builder instead + // of an alter-table one. The `indexName` is not decoration — it is what + // makes the driver recognise the constraint as already present on its first + // boot against a table this migration created, instead of adding a second + // one under its own name and then reporting this one as an orphan. + const { emit, skipped } = partitionUniqueIndexes(obj, emittedColumns); + for (const index of emit) { + const columns = index.columns.map((c) => `'${c}'`).join(', '); + lines.push(` table.unique([${columns}], { indexName: '${index.name}' });`); + } + for (const { index, why } of skipped) { + lines.push(` // NOT EMITTED: UNIQUE index '${index.name}' on (${index.columns.join(', ')}) — ${why}.`); + } lines.push(' });'); } From f2494e8e5b8b72f4e2f6f40365f48b5194a7d2c6 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 9 Sep 2026 16:15:17 +0000 Subject: [PATCH 2/2] chore(changeset): patch @objectstack/cli for the emitted unique index Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015QE8qk46e5CHJxyQEUjbf8 --- ...e-migration-emits-declared-unique-index.md | 64 +++++++++++++++++++ 1 file changed, 64 insertions(+) create mode 100644 .changeset/generate-migration-emits-declared-unique-index.md diff --git a/.changeset/generate-migration-emits-declared-unique-index.md b/.changeset/generate-migration-emits-declared-unique-index.md new file mode 100644 index 0000000000..07676f0bb3 --- /dev/null +++ b/.changeset/generate-migration-emits-declared-unique-index.md @@ -0,0 +1,64 @@ +--- +"@objectstack/cli": patch +--- + +fix(cli): `os generate migration` emits the field-level unique index the driver creates (#16317) + +## What was wrong + +Both migration formats emitted the table and none of the object's declared +uniqueness. Measured on live PostgreSQL 16.13 — one object driven through all +three producers into three schemas, `pg_indexes` read back per schema: + +```ts +{ name: 'probe', fields: { keyed_unique: { type: 'text', unique: true, maxLength: 100 } } } +``` + +| producer | before | after | +|:--|:--|:--| +| `driver-sql` via `initObjects` | `probe_pkey`, `uniq_probe_keyed_unique` | unchanged | +| `--format sql` | `probe_pkey` | `probe_pkey`, **`uniq_probe_keyed_unique`** | +| `--format ts` | `probe_pkey` | `probe_pkey`, **`uniq_probe_keyed_unique`** | + +Two rows with the same `keyed_unique` value were refused by the platform's table +(`23505 ... violates unique constraint "uniq_probe_keyed_unique"`) and accepted +by both generated ones, with nothing reporting it: a scaffold that creates the +table for an object silently dropped a uniqueness guarantee the object declares. +After the change the duplicate is refused by all three, each naming the same +constraint. + +The key set was not missing — it was already computed here to size the keyed +text family's columns; only the index it implies was never emitted. + +## What it does now + +- **`--format sql`** emits an inline `CONSTRAINT "" UNIQUE ()`. + That is what knex's `table.unique(columns, { indexName })` — the driver's own + call — compiles to on PostgreSQL, so a generated table and a platform-created + one agree in `pg_constraint` as well as in `pg_indexes`; and it stays inside + the statement's `IF NOT EXISTS`, which a following `ALTER TABLE ... ADD + CONSTRAINT` has no spelling for. +- **`--format ts`** emits that knex call itself, `indexName` included — which is + what makes the driver recognise the constraint as already present on its first + boot against a generated table, instead of adding a second one under its own + name and then reporting the generated one as an orphan to drop. +- Names come from a transcription of `driver-sql`'s `buildIndexName`, pinned + against the driver's own export (a CLI production module may not statically + value-import a driver package). + +## What it deliberately still does not emit — and now says so + +Both formats print a `NOT EMITTED:` line naming the index, its key parts and the +reason, instead of dropping it silently: + +- the **organization-scoped composite** (`unique: true` / `'organization'` on an + object with an organization column), whose key part is + `COALESCE(, '__global__')`. Emitting the bare composite + instead would be worse than emitting nothing: under SQL's NULL-distinct + `UNIQUE` it constrains no row that has no organization, which on a + single-tenant deployment is every row. +- an index over a column no field materialises (a virtual `formula` field) — + the same skip the driver performs, where the driver logs a warning. + +Object-level `indexes[]` remains unemitted by both formats; it is normalized by +a different driver-side rule and is not covered by this change.