Skip to content

Commit 8644d1d

Browse files
os-litantclaude
andauthored
fix(cli): generated migrations give the table's own id the driver's column shape (#15518)
Both migration generators hardcoded the primary key as a UUID: generateMigrationSql ' "id" UUID PRIMARY KEY DEFAULT gen_random_uuid(),' generateMigrationTs " table.uuid('id').primary().defaultTo(db.fn.uuid());" driver-sql emits `table.string('id').primary()` for that column — knex's varchar(255), SqlDriver.DEFAULT_STRING_VARCHAR_CHARS. A platform id is a string, not a uuid, so on Postgres the generated table refused the platform's first insert with `22P02 invalid input syntax for type uuid`. The DEFAULT is the quieter half and the reason this is worth correcting rather than working around: the driver emits no database-side default, because its insert path always supplies the id itself. `gen_random_uuid()` therefore only ever fired for an out-of-band insert, handing that row a 36-character uuid the platform's generator would never mint — one table holding two incompatible id shapes, silently. Both generators now emit the driver's own answer. The correction also closes a contradiction inside generate.ts, whose prose already stated that a reference column takes the width of the target's id column *because* the driver emits `table.string('id').primary()`, a few hundred lines above the two lines that emitted uuid. generate-builtin-id-column.pin.test.ts reads the width from the driver's own DEFAULT_STRING_VARCHAR_CHARS rather than transcribing 255, so the generators cannot drift away from the driver again without a named failure. Its last case records — deliberately without correcting — a third disagreement measured in the same pass: the audit-stamp columns. driver-sql emits them nullable (`table.timestamp(name).defaultTo(knex.fn.now())`); both generators emit them NOT NULL. Which side moves is not this change's to decide. Claude-Session: https://claude.ai/code/session_01D47qPfEWVPmhguWgBZCi5N Co-authored-by: os-litant <noreply@anthropic.com>
1 parent 85a2459 commit 8644d1d

3 files changed

Lines changed: 253 additions & 2 deletions

File tree

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
---
2+
"@objectstack/cli": patch
3+
---
4+
5+
`os generate migration` gives a table's own `id` column the shape the platform actually creates.
6+
7+
Both migration generators hardcoded the primary key as a UUID — `"id" UUID PRIMARY KEY DEFAULT gen_random_uuid()` in the SQL format, `table.uuid('id').primary().defaultTo(db.fn.uuid())` in the TypeScript one (the default format). The platform's SQL driver emits `table.string('id').primary()`, which is knex's `varchar(255)`. A platform id is a string, not a uuid, so on Postgres the generated table refused the platform's very first insert with `22P02 invalid input syntax for type uuid`.
8+
9+
The quieter half is the `DEFAULT`, and it is why this was worth correcting rather than working around. The driver emits no database-side default at all — its insert path always supplies the id itself — so `gen_random_uuid()` never fired for a platform write, only for an out-of-band one, handing that row a 36-character uuid this platform's id generator would never mint. One table would then hold two incompatible id shapes, with nothing said.
10+
11+
Both generators now emit the driver's own answer: `"id" VARCHAR(255) PRIMARY KEY` and `table.string('id').primary()`. The correction also closes a contradiction inside the generator file, whose prose already stated that a reference column takes the width of the target's `id` column *because* the driver emits `table.string('id').primary()` — a few hundred lines above the two lines that emitted `uuid`.
12+
13+
`generate-builtin-id-column.pin.test.ts` reads the width from the driver's own `DEFAULT_STRING_VARCHAR_CHARS` rather than transcribing `255`, so the generators cannot drift away from the driver again without a named failure.
Lines changed: 217 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,217 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* THE #15040 PIN: the `id` column both migration generators emit is the column
5+
* `driver-sql` actually creates.
6+
*
7+
* ## The defect
8+
*
9+
* `generate.ts` hardcodes the table's own primary key in each of its two
10+
* migration generators, and both said `uuid`:
11+
*
12+
* ```
13+
* generateMigrationSql ' "id" UUID PRIMARY KEY DEFAULT gen_random_uuid(),'
14+
* generateMigrationTs " table.uuid('id').primary().defaultTo(db.fn.uuid());"
15+
* ```
16+
*
17+
* The platform emits `table.string('id').primary()` — knex's `varchar(255)`,
18+
* `SqlDriver.DEFAULT_STRING_VARCHAR_CHARS`. A platform id is not a uuid, so on
19+
* Postgres the generated table refuses the platform's first insert outright
20+
* with `22P02 invalid input syntax for type uuid`.
21+
*
22+
* ⭐ The `DEFAULT gen_random_uuid()` half is the one this pin is really for,
23+
* because it is the half that is NOT loud. The driver emits no database-side
24+
* default at all: its insert path always supplies the id itself (`create()`
25+
* takes `_id`, else a caller-supplied `id`, else mints one). So the default
26+
* never fires for a platform write and only ever fires for an out-of-band one —
27+
* handing that row a 36-character uuid this platform's generator would never
28+
* mint, leaving one table holding two incompatible id shapes, silently.
29+
*
30+
* ## Why the pin reads the driver instead of asserting `varchar(255)`
31+
*
32+
* The whole shape of this card is "the generator disagrees with the driver". A
33+
* pin that transcribed `VARCHAR(255)` would re-create that defect one layer up:
34+
* the day the driver's id column moves, the generator would be wrong again and
35+
* this file would still be green. So the width is READ from
36+
* `DEFAULT_STRING_VARCHAR_CHARS` where it is declared, and the typescript
37+
* generator's line is compared against the driver's own call, byte for byte.
38+
* Both extractions carry a non-vacuity control — a source reader that matched
39+
* nothing would pass while measuring nothing.
40+
*
41+
* This is the same authority `generate-field-type-vocabulary.pin.test.ts`
42+
* already reads for the REFERENCE_VALUE_TYPES width, and for the same reason: a
43+
* reference column holds the TARGET's id, so both questions have one answer.
44+
* That pin covers the FIELDS; this one covers the builtin column itself, which
45+
* is not a vocabulary entry and so had no rule anywhere.
46+
*
47+
* ## ⚠️ Recorded divergence, NOT a ruling: the audit-stamp columns (#15040)
48+
*
49+
* The last `it` below records — and deliberately does not correct — a THIRD
50+
* disagreement measured in the same pass. It is recorded so it cannot change
51+
* shape unnoticed, and so no reader mistakes this file for having decided it.
52+
*/
53+
54+
import fs from 'node:fs';
55+
import path from 'node:path';
56+
import { fileURLToPath } from 'node:url';
57+
58+
import { describe, expect, it } from 'vitest';
59+
60+
import { generateMigrationSql, generateMigrationTs } from './generate.js';
61+
62+
const HERE = path.dirname(fileURLToPath(import.meta.url));
63+
const GENERATE_TS = path.resolve(HERE, 'generate.ts');
64+
const GENERATE_SOURCE = fs.readFileSync(GENERATE_TS, 'utf8');
65+
66+
/** The authority, read where it lives — never transcribed here. */
67+
const DRIVER_SQL_SRC = path.resolve(HERE, '../../../drivers/driver-sql/src');
68+
const SQL_DRIVER_SOURCE = fs.readFileSync(path.join(DRIVER_SQL_SRC, 'sql-driver.ts'), 'utf8');
69+
70+
/** The one line the driver emits for a managed table's primary key. */
71+
const DRIVER_ID_COLUMN = "table.string('id').primary();";
72+
73+
/** `varchar(n)` for a bare `table.string(name)`, read off the driver's constant. */
74+
function driverDefaultVarcharChars(): number {
75+
const m = SQL_DRIVER_SOURCE.match(/DEFAULT_STRING_VARCHAR_CHARS = (\d+);/);
76+
if (!m) {
77+
throw new Error(
78+
'DEFAULT_STRING_VARCHAR_CHARS not found in sql-driver.ts. That constant is the width this ' +
79+
'pin reads instead of transcribing, so a rename must fail loudly here rather than leave ' +
80+
'the generators unmeasured.',
81+
);
82+
}
83+
return Number(m[1]);
84+
}
85+
86+
const CONFIG = {
87+
objects: {
88+
account: {
89+
name: 'account',
90+
fields: { title: { type: 'text' }, owner: { type: 'lookup' } },
91+
},
92+
},
93+
};
94+
95+
/** The `id` line `os generate migration --format sql` emits. */
96+
function emittedSqlIdLine(): string {
97+
const lines = generateMigrationSql(CONFIG as Record<string, unknown>).split('\n');
98+
const found = lines.filter((l) => /^\s*"id"\s/.test(l));
99+
expect(found, 'the sql generator emitted no id column at all — the reader is broken').toHaveLength(1);
100+
return found[0].trim();
101+
}
102+
103+
/** The `id` line `os generate migration` (typescript, the DEFAULT format) emits. */
104+
function emittedTsIdLine(): string {
105+
const lines = generateMigrationTs(CONFIG as Record<string, unknown>).split('\n');
106+
const found = lines.filter((l) => /table\.\w+\('id'\)/.test(l));
107+
expect(found, 'the typescript generator emitted no id column at all — the reader is broken').toHaveLength(1);
108+
return found[0].trim();
109+
}
110+
111+
describe('the builtin id column both migration generators emit (#15040)', () => {
112+
it('reads a real driver source that still owns the id column (control)', () => {
113+
// Non-vacuity for both extractions below.
114+
expect(SQL_DRIVER_SOURCE.length).toBeGreaterThan(10_000);
115+
expect(
116+
SQL_DRIVER_SOURCE,
117+
'driver-sql no longer emits `table.string(\'id\').primary()`. This pin corrects the ' +
118+
'generators TOWARD the driver, so if the driver moved, re-read #15040 before touching ' +
119+
'generate.ts — the authority is the driver, not this file.',
120+
).toContain(DRIVER_ID_COLUMN);
121+
expect(driverDefaultVarcharChars()).toBeGreaterThan(0);
122+
});
123+
124+
it('the typescript generator emits the driver\'s own line, byte for byte', () => {
125+
expect(emittedTsIdLine()).toBe(DRIVER_ID_COLUMN);
126+
});
127+
128+
it('the sql generator emits the driver\'s width, read from the driver', () => {
129+
expect(emittedSqlIdLine()).toBe(`"id" VARCHAR(${driverDefaultVarcharChars()}) PRIMARY KEY,`);
130+
});
131+
132+
it('neither generator gives the id a uuid type', () => {
133+
// The loud half: a platform id is not a uuid, and Postgres says so with
134+
// `22P02 invalid input syntax for type uuid` on the first insert.
135+
expect(emittedSqlIdLine()).not.toMatch(/\bUUID\b/i);
136+
expect(emittedTsIdLine()).not.toMatch(/table\.uuid\(/);
137+
// Anti-vacuity: the predicates really do fire on the shapes this replaced.
138+
expect('"id" UUID PRIMARY KEY DEFAULT gen_random_uuid(),').toMatch(/\bUUID\b/i);
139+
expect("table.uuid('id').primary().defaultTo(db.fn.uuid());").toMatch(/table\.uuid\(/);
140+
});
141+
142+
it('neither generator gives the id a database-side default', () => {
143+
// The quiet half, and the reason this is p2 rather than p3. The driver
144+
// emits no default because `create()` always supplies an id; a default
145+
// therefore only ever fires for an out-of-band insert, and mints an id
146+
// shape the platform never would.
147+
expect(emittedSqlIdLine()).not.toMatch(/\bDEFAULT\b/i);
148+
expect(emittedTsIdLine()).not.toMatch(/defaultTo\(/);
149+
// The driver's own id line carries no default either — read, not assumed.
150+
expect(DRIVER_ID_COLUMN).not.toMatch(/defaultTo\(/);
151+
// Anti-vacuity: both predicates fire on what was there before.
152+
expect('"id" UUID PRIMARY KEY DEFAULT gen_random_uuid(),').toMatch(/\bDEFAULT\b/i);
153+
expect("table.uuid('id').primary().defaultTo(db.fn.uuid());").toMatch(/defaultTo\(/);
154+
});
155+
156+
it('the id column is still emitted by BOTH generators, in each table', () => {
157+
// The correction must not be mistaken for a deletion: the primary key is
158+
// still there, and still first.
159+
const sql = generateMigrationSql(CONFIG as Record<string, unknown>);
160+
expect(sql).toContain('CREATE TABLE IF NOT EXISTS "account" (');
161+
expect(sql.indexOf('"id"')).toBeLessThan(sql.indexOf('"title"'));
162+
const ts = generateMigrationTs(CONFIG as Record<string, unknown>);
163+
expect(ts).toContain("await db.schema.createTable('account'");
164+
expect(ts.indexOf("table.string('id')")).toBeLessThan(ts.indexOf("table.string('title')"));
165+
// Each generator carries exactly ONE hardcoded id line — the shape that let
166+
// these two disagree with the driver in the first place, and the reason a
167+
// fix to one of them can silently leave the other behind. Counted over the
168+
// source so a third copy cannot arrive unmeasured; the literals are the
169+
// emitted lines themselves, so this cannot drift from what is asserted above.
170+
for (const line of [emittedSqlIdLine(), emittedTsIdLine()]) {
171+
expect(GENERATE_SOURCE.split(line), `generate.ts emits \`${line}\` from more than one place`)
172+
.toHaveLength(2);
173+
}
174+
});
175+
176+
// ── Recorded divergence, NOT coverage, and NOT a ruling ──────────────────
177+
//
178+
// Measured in the same pass as the id column, on the same two generators.
179+
// The audit-stamp columns disagree with the driver too, in a way the id
180+
// column did not, and the disagreement is NULLABILITY rather than type:
181+
//
182+
// driver-sql `table.timestamp(name).defaultTo(knex.fn.now())` (nullable)
183+
// sql gen `"created_at" TIMESTAMP NOT NULL DEFAULT now()`
184+
// ts gen `table.timestamps(true, true)` — knex 3.3.0 compiles this
185+
// to `.notNullable().defaultTo(CURRENT_TIMESTAMP)` on both
186+
// columns (`knex/lib/schema/tablebuilder.js`).
187+
//
188+
// Compiled offline against knex's pg dialect, the two shapes are:
189+
//
190+
// driver "created_at" timestamptz default CURRENT_TIMESTAMP
191+
// ts gen "created_at" timestamptz not null default CURRENT_TIMESTAMP
192+
//
193+
// Unlike the id column this is not obviously a wrong value: the driver stamps
194+
// both columns on every write, so NOT NULL is arguably the truer constraint —
195+
// and the driver's own DDL is dialect-branched (`datetime(3)` on MySQL, a
196+
// canonical ISO default on SQLite) in a way a Postgres-flavoured generated
197+
// migration does not try to reproduce. Which side moves is not this card's to
198+
// decide, so nothing here changes those lines. Asserted only so the
199+
// divergence cannot change shape unnoticed.
200+
it('#15040 record — the audit-stamp columns diverge from the driver, deliberately unresolved', () => {
201+
const sql = generateMigrationSql(CONFIG as Record<string, unknown>);
202+
expect(sql).toContain('"created_at" TIMESTAMP NOT NULL DEFAULT now()');
203+
expect(sql).toContain('"updated_at" TIMESTAMP NOT NULL DEFAULT now()');
204+
expect(generateMigrationTs(CONFIG as Record<string, unknown>)).toContain('table.timestamps(true, true);');
205+
// The driver side, read where it lives: one audit-column builder, and its
206+
// default arm carries no `.notNullable()`.
207+
expect(
208+
SQL_DRIVER_SOURCE,
209+
'driver-sql\'s audit-column DDL moved — re-read the #15040 record above before trusting it.',
210+
).toContain('table.timestamp(name).defaultTo(this.knex.fn.now());');
211+
const auditArm = SQL_DRIVER_SOURCE.slice(
212+
SQL_DRIVER_SOURCE.indexOf('protected createAuditTimestampColumn('),
213+
).slice(0, 600);
214+
expect(auditArm.length).toBeGreaterThan(100);
215+
expect(auditArm).not.toContain('notNullable()');
216+
});
217+
});

packages/cli/src/commands/generate.ts

Lines changed: 23 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1171,7 +1171,23 @@ export function generateMigrationSql(config: Record<string, unknown>): string {
11711171
const fields = (obj.fields ?? {}) as Record<string, Record<string, unknown>>;
11721172

11731173
lines.push(`CREATE TABLE IF NOT EXISTS "${tableName}" (`);
1174-
lines.push(' "id" UUID PRIMARY KEY DEFAULT gen_random_uuid(),');
1174+
// #15040 — the table's OWN id, corrected to what `driver-sql` emits for it:
1175+
// `table.string('id').primary()`, i.e. knex's `varchar(255)`
1176+
// (`SqlDriver.DEFAULT_STRING_VARCHAR_CHARS`). This is the same derivation
1177+
// `lookup` / `master_detail` / `user` / `tree` above already state — a
1178+
// reference column holds the TARGET's id — applied one column to the left,
1179+
// to the id itself. A platform id is not a uuid, so Postgres refused one in
1180+
// a `uuid` column with `22P02 invalid input syntax for type uuid` on the
1181+
// FIRST insert.
1182+
//
1183+
// The DEFAULT goes with the type, and it is the quieter half: the driver
1184+
// emits no database-side default because its own insert path always
1185+
// supplies the id (`create()` takes `_id`, else a caller-supplied `id`,
1186+
// else mints one). A `DEFAULT gen_random_uuid()` therefore never fires for
1187+
// a platform write and only fires for an out-of-band one — handing that row
1188+
// a 36-character uuid this platform's id generator would never mint, so the
1189+
// table would end up holding two incompatible id shapes with nothing said.
1190+
lines.push(' "id" VARCHAR(255) PRIMARY KEY,');
11751191

11761192
const fieldLines: string[] = [];
11771193
for (const [fieldName, fieldDef] of Object.entries(fields)) {
@@ -1228,7 +1244,12 @@ export function generateMigrationTs(config: Record<string, unknown>): string {
12281244
const fields = (obj.fields ?? {}) as Record<string, Record<string, unknown>>;
12291245

12301246
lines.push(` await db.schema.createTable('${tableName}', (table: any) => {`);
1231-
lines.push(" table.uuid('id').primary().defaultTo(db.fn.uuid());");
1247+
// #15040 — the driver's own line for this column, emitted verbatim:
1248+
// `table.string('id').primary()`. See `generateMigrationSql` above for the
1249+
// derivation and for why the `.defaultTo(db.fn.uuid())` half goes with it
1250+
// (on Postgres `knex.fn.uuid()` compiles to `(gen_random_uuid())`, so the
1251+
// two generators were emitting one and the same wrong default).
1252+
lines.push(" table.string('id').primary();");
12321253

12331254
for (const [fieldName, fieldDef] of Object.entries(fields)) {
12341255
const fType = String(fieldDef.type || 'text');

0 commit comments

Comments
 (0)