Skip to content

Commit 1f2a02b

Browse files
os-litantclaude
andauthored
fix(cli): generated SQL migrations give timestamp columns their time zone (#16070)
* fix(cli): generated SQL migrations give timestamp columns their time zone (#15521) `os generate migration --format sql` spelled its two audit-stamp columns, and every declared `datetime` field, as bare `TIMESTAMP`. On PostgreSQL that is `timestamp WITHOUT time zone`, while both other producers of the same columns yield `timestamptz`: driver-sql's `createAuditTimestampColumn` and this CLI's own TypeScript migration format both build them with knex's `table.timestamp`, and `createColumn`'s `datetime` arm states the zone-aware column as a decision — "Postgres deliberately keeps `table.timestamp` -> `timestamptz`". Driven rather than compiled: all three producers were run against a live PostgreSQL 16.13 and their columns read back out of `information_schema.columns`. Only the SQL format came back zone-naive. The consequence is a data defect, not a cosmetic type difference: a zone-naive column stores the wall clock of whatever session wrote the row, and `DEFAULT now()` is folded into that session's TimeZone on the way in. Two defaulted rows inserted six milliseconds apart, one under `TimeZone='UTC'` and one under `Asia/Tokyo`, were recorded nine hours apart in the generated table and 3 ms apart in the driver's own. The whole temporal class was enumerated in the same run and `datetime` is its only divergent member; `date` and `time` already agreed on all three producers, so neither moves. The nullability half of #15521 is deliberately untouched — the driver leaves both audit columns nullable, both generators say NOT NULL, nothing fails either way, and the driver's audit DDL is dialect-branched in a way a Postgres- flavoured generated migration does not reproduce. It stays recorded, with the `now()` / `CURRENT_TIMESTAMP` default spelling beside it, in generate-builtin-id-column.pin.test.ts. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01D47qPfEWVPmhguWgBZCi5N * docs(changeset): cite ADR-0053 D-B4 as the governing decision (#15521) Contract review corrected the AUTHORITY behind this change, not its answer. The changeset cited driver-sql's own comment; the decision it implements is ADR-0053 D-B4 (accepted), whose resolution states both sites in one sentence: `Field.datetime` maps to DATETIME(3) on MySQL while "Postgres deliberately keeps timestamptz", and "the builtin created_at/updated_at take the same type -- the registry declares them Field.datetime". So the declared-field row and the audit-stamp rows belong in one diff by decision rather than by this seat's judgement, and a reader of the release notes does not have to reconstruct that from a driver source comment. Changeset prose only. No code, no pin, no test and no FIELD_TYPE_SQL_MAP entry moves; the nullability and now()/CURRENT_TIMESTAMP rows stay untouched and open with the maintainer. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01D47qPfEWVPmhguWgBZCi5N --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent a4816a7 commit 1f2a02b

4 files changed

Lines changed: 198 additions & 30 deletions

File tree

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
---
2+
"@objectstack/cli": patch
3+
---
4+
5+
`os generate migration --format sql` gives every timestamp column the time zone the platform actually stores.
6+
7+
The SQL format spelled its two audit-stamp columns — and every declared `datetime` field — as bare `TIMESTAMP`. In PostgreSQL that is `timestamp WITHOUT time zone`, while both of the other producers of the same columns yield `timestamptz`. This implements **[ADR-0053](../docs/adr/0053-date-and-datetime-semantics.md) D-B4** (accepted), which governs both sites in one sentence: `Field.datetime` maps to `DATETIME(3)` on MySQL while "Postgres deliberately keeps `timestamptz`", and "the builtin `created_at`/`updated_at` take the same type — the registry declares them `Field.datetime`". So the declared-field row and the audit-stamp rows are one decision rather than two judgement calls, and `driver-sql`'s `createAuditTimestampColumn`, its `createColumn` `datetime` arm and this CLI's TypeScript migration format were already implementing it — the SQL format was the one producer that was not.
8+
9+
Driven, not compiled: all three producers were run against a live PostgreSQL 16.13 and their columns read back out of `information_schema.columns`. Only the SQL format came back zone-naive, and the consequence is a data defect rather than a cosmetic type difference. A zone-naive column stores the wall clock of whatever session wrote the row and keeps nothing to recover the offset from, and `DEFAULT now()` is folded into that session's `TimeZone` on the way in. Two defaulted rows inserted **six milliseconds apart**, one under `TimeZone='UTC'` and one under `Asia/Tokyo`, were recorded **nine hours apart** in the generated table and 3 ms apart in the driver's own:
10+
11+
```
12+
sqlgen (timestamp) a_utc 2026-09-05 22:31:28.309421
13+
sqlgen (timestamp) b_tokyo 2026-09-06 07:31:28.315458 <- +9h, same instant
14+
tsgen (timestamptz) a_utc 2026-09-05 22:31:28.31332+00
15+
tsgen (timestamptz) b_tokyo 2026-09-05 22:31:28.316401+00
16+
```
17+
18+
The whole temporal class was enumerated in that same run and `datetime` is its only divergent member: `date` is `DATE` and `time` is `TIME` on all three producers, so neither moves.
19+
20+
Two things this deliberately does not change. The audit columns' **nullability** stays as it is: the driver leaves both nullable and both generators say `NOT NULL`, nothing fails either way, and the driver's own audit DDL is dialect-branched in a way a Postgres-flavoured generated migration does not reproduce — so which side moves is a ruling, recorded in `generate-builtin-id-column.pin.test.ts` and still open. The `DEFAULT now()` spelling stays too: it is the same instant as the driver's `CURRENT_TIMESTAMP` (both are `transaction_timestamp()`) and only reads differently in the catalog.
21+
22+
Scope for an existing project: already-generated migration files are checked-in artifacts and are not rewritten, and no deployed column is altered — a table created from an older generated migration keeps `timestamp without time zone` until its owner migrates it. What changes is what the next generated migration says.

packages/cli/src/commands/generate-builtin-id-column.pin.test.ts

Lines changed: 78 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -44,11 +44,24 @@
4444
* That pin covers the FIELDS; this one covers the builtin column itself, which
4545
* is not a vocabulary entry and so had no rule anywhere.
4646
*
47-
* ## ⚠️ Recorded divergence, NOT a ruling: the audit-stamp columns (#15040)
47+
* ## The audit-stamp columns: one half ruled (#15521), one half still recorded
4848
*
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.
49+
* #15040 measured a THIRD disagreement in the same pass and recorded it here
50+
* without correcting it. #15521 split that record in two and ruled only the
51+
* half that was decidable:
52+
*
53+
* TYPE — RULED. The sql format spelled both columns bare `TIMESTAMP`
54+
* (`timestamp WITHOUT time zone`) while the driver and the typescript format
55+
* both build them with knex's `table.timestamp` = `timestamptz`. Driven on a
56+
* live PostgreSQL 16.13: two defaulted rows inserted 6 ms apart under
57+
* different session timezones landed NINE HOURS apart in the zone-naive
58+
* column and 3 ms apart in the aware one. One producer of three was wrong and
59+
* nothing had to be decided, so the sql format moved.
60+
*
61+
* NULLABILITY (and the `now()` / `CURRENT_TIMESTAMP` default spelling) —
62+
* STILL RECORDED, still not ruled: the driver leaves both columns nullable
63+
* and both generators say NOT NULL. Nothing fails either way, so which side
64+
* moves is a ruling #15521 holds open.
5265
*/
5366

5467
import fs from 'node:fs';
@@ -173,40 +186,79 @@ describe('the builtin id column both migration generators emit (#15040)', () =>
173186
}
174187
});
175188

176-
// ── Recorded divergence, NOT coverage, and NOT a ruling ──────────────────
189+
// ── #15521, the TYPE half: ruled, and now asserted as agreement ────────
190+
//
191+
// Bare `TIMESTAMP` is `timestamp WITHOUT time zone`. Both knex producers of
192+
// these same two columns — `driver-sql`'s `createAuditTimestampColumn` and
193+
// `generateMigrationTs`'s `table.timestamps(true, true)` — yield
194+
// `timestamptz`. Read back out of `information_schema.columns` on a live
195+
// PostgreSQL 16.13, all three producers driven:
177196
//
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:
197+
// driver created_at timestamp with time zone null=YES default=CURRENT_TIMESTAMP
198+
// ts gen created_at timestamp with time zone null=NO default=CURRENT_TIMESTAMP
199+
// sql gen created_at timestamp without time zone null=NO default=now()
181200
//
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`).
201+
// Asserted as AGREEMENT with the driver rather than as the literal
202+
// `TIMESTAMPTZ` alone: the driver's builder is read where it lives, so the day
203+
// it stops emitting a knex `table.timestamp` this fails here instead of
204+
// leaving the generators quietly wrong again — the same discipline the id
205+
// column above already uses for its width.
206+
it('#15521 — the audit columns take the driver\'s zone-AWARE type in both generators', () => {
207+
const sql = generateMigrationSql(CONFIG as Record<string, unknown>);
208+
for (const col of ['created_at', 'updated_at']) {
209+
expect(sql).toContain(`"${col}" TIMESTAMPTZ NOT NULL DEFAULT now()`);
210+
// `\b` discriminates: `TIMESTAMPTZ` is not a match for `TIMESTAMP\b`.
211+
expect(
212+
sql,
213+
`the sql format spells ${col} zone-NAIVE again — a defaulted row then records the ` +
214+
'wall clock of whatever session wrote it, with nothing left to recover the offset from',
215+
).not.toMatch(new RegExp(`"${col}" TIMESTAMP\\b`));
216+
}
217+
// Anti-vacuity: the predicate really does fire on the shape this replaced.
218+
expect(' "created_at" TIMESTAMP NOT NULL DEFAULT now()').toMatch(/"created_at" TIMESTAMP\b/);
219+
// The authority, read where it lives — both knex paths, neither transcribed.
220+
expect(
221+
SQL_DRIVER_SOURCE,
222+
'driver-sql no longer builds the audit columns with knex\'s `table.timestamp`, which is ' +
223+
'what makes them `timestamptz` on Postgres. Re-read #15521 before trusting the ' +
224+
'generators\' TIMESTAMPTZ.',
225+
).toContain('table.timestamp(name).defaultTo(this.knex.fn.now());');
226+
expect(generateMigrationTs(CONFIG as Record<string, unknown>)).toContain('table.timestamps(true, true);');
227+
});
228+
229+
// ── #15521, the NULLABILITY half: recorded, NOT coverage, and NOT a ruling ──
187230
//
188-
// Compiled offline against knex's pg dialect, the two shapes are:
231+
// The driver leaves both columns nullable; both generators say NOT NULL:
189232
//
190-
// driver "created_at" timestamptz default CURRENT_TIMESTAMP
191-
// ts gen "created_at" timestamptz not null default CURRENT_TIMESTAMP
233+
// driver-sql `table.timestamp(name).defaultTo(knex.fn.now())` (nullable)
234+
// sql gen `"created_at" TIMESTAMPTZ NOT NULL DEFAULT now()`
235+
// ts gen `table.timestamps(true, true)` — knex 3.3.0 compiles this to
236+
// `.notNullable().defaultTo(CURRENT_TIMESTAMP)` on both columns
237+
// (`knex/lib/schema/tablebuilder.js`), which the live-Postgres
238+
// catalog read above confirms as `null=NO`.
192239
//
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
240+
// Unlike the type half this is not a wrong value: the driver stamps both
241+
// columns on every write, so NOT NULL is arguably the truer constraint, and
242+
// the driver's own audit DDL is dialect-branched (`datetime(3)` on MySQL, a
196243
// 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', () => {
244+
// migration does not try to reproduce — so "match the driver byte for byte"
245+
// is not even well-defined across dialects. Which side moves is #15521's to
246+
// decide; nothing here changes those lines.
247+
//
248+
// The DEFAULT spelling rides with it: this generator's `now()` and the
249+
// driver's `CURRENT_TIMESTAMP` are the same instant (both are
250+
// `transaction_timestamp()`), but Postgres keeps them textually apart in the
251+
// catalog, so they are two rows of the same schema-diff noise.
252+
it('#15521 record — the audit columns\' NULLABILITY diverges, deliberately unresolved', () => {
201253
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()');
254+
expect(sql).toContain('"created_at" TIMESTAMPTZ NOT NULL DEFAULT now()');
255+
expect(sql).toContain('"updated_at" TIMESTAMPTZ NOT NULL DEFAULT now()');
204256
expect(generateMigrationTs(CONFIG as Record<string, unknown>)).toContain('table.timestamps(true, true);');
205257
// The driver side, read where it lives: one audit-column builder, and its
206258
// default arm carries no `.notNullable()`.
207259
expect(
208260
SQL_DRIVER_SOURCE,
209-
'driver-sql\'s audit-column DDL moved — re-read the #15040 record above before trusting it.',
261+
'driver-sql\'s audit-column DDL moved — re-read the #15521 record above before trusting it.',
210262
).toContain('table.timestamp(name).defaultTo(this.knex.fn.now());');
211263
const auditArm = SQL_DRIVER_SOURCE.slice(
212264
SQL_DRIVER_SOURCE.indexOf('protected createAuditTimestampColumn('),

packages/cli/src/commands/generate-field-type-vocabulary.pin.test.ts

Lines changed: 47 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -543,7 +543,7 @@ describe('#14828 — the SQL answers are the platform’s, not this file’s inv
543543
it('a member typed as a STRING on the record never takes a NUMERIC column', () => {
544544
// ⚠️ Deliberately "not numeric" rather than "is a character column".
545545
// `date` / `datetime` / `time` are strings on the record and take DATE /
546-
// TIMESTAMP / TIME columns, which is correct — a rule demanding a varchar
546+
// TIMESTAMPTZ / TIME columns, which is correct — a rule demanding a varchar
547547
// would report those three as defects. What `autonumber` did is the
548548
// narrower thing: a rendered string in a column that only accepts numbers.
549549
const NUMERIC_COLUMN = /^(SERIAL|BIGSERIAL|SMALLSERIAL|INTEGER|INT|BIGINT|SMALLINT|DECIMAL|NUMERIC|REAL|FLOAT|DOUBLE)\b/i;
@@ -571,6 +571,52 @@ describe('#14828 — the SQL answers are the platform’s, not this file’s inv
571571
expect(tsColumn('autonumber')).toBe("table.string('f_autonumber')");
572572
});
573573

574+
// ── Rule 5: the TEMPORAL class takes the driver's own column types (#15521) ──
575+
//
576+
// The whole class, driven against a live PostgreSQL 16.13 — every producer
577+
// run for real and its columns read back out of `information_schema.columns`
578+
// — and exactly one member diverged:
579+
//
580+
// date DATE driver DATE ts DATE agree
581+
// datetime timestamp WITHOUT time zone driver timestamptz ts timestamptz DIVERGED
582+
// time time without time zone driver time ts time agree
583+
//
584+
// `datetime` was the only bare `TIMESTAMP` in this map, and bare `TIMESTAMP`
585+
// is `timestamp WITHOUT time zone`: the column stores the wall clock of
586+
// whatever session wrote the row and keeps nothing to recover the offset
587+
// from. `createColumn`'s own arm calls the zone-aware column a decision
588+
// rather than a default ("Postgres deliberately keeps `table.timestamp` →
589+
// `timestamptz`"), and this file's typescript generator already emitted
590+
// `table.timestamp` for it — so the SQL map was one producer of three
591+
// disagreeing with the other two, which is what #15521 corrected.
592+
//
593+
// Asserted against the driver's arm rather than against the literal, for the
594+
// reason Rule 3 already gives: a transcribed `TIMESTAMPTZ` would re-create the
595+
// defect one layer up the day the driver's arm moves.
596+
597+
it('the temporal members take the column type the driver builds for them', () => {
598+
// The driver's non-MySQL arms, read where they live. `table.timestamp` is
599+
// `timestamptz` on Postgres; `table.date` and `table.time` are their plain
600+
// selves, which is why only `datetime` had a time zone to lose.
601+
expect(createColumnArm('datetime')).toContain('table.timestamp(name)');
602+
expect(createColumnArm('date')).toContain('table.date(name)');
603+
expect(createColumnArm('time')).toContain('table.time(name)');
604+
605+
expect(sqlColumn('datetime')).toBe('TIMESTAMPTZ');
606+
expect(sqlColumn('date')).toBe('DATE');
607+
expect(sqlColumn('time')).toBe('TIME');
608+
609+
// The third producer agrees by construction — the same knex builders.
610+
expect(tsColumn('datetime')).toBe("table.timestamp('f_datetime')");
611+
expect(tsColumn('date')).toBe("table.date('f_date')");
612+
expect(tsColumn('time')).toBe("table.time('f_time')");
613+
614+
// Anti-vacuity: the reader really discriminates, and the pre-#15521
615+
// spelling really is a different answer rather than a formatting variant.
616+
expect(sqlColumn('datetime')).not.toBe('TIMESTAMP');
617+
expect(sqlColumn('this_is_not_a_field_type')).toBeNull();
618+
});
619+
574620
// ── Recorded divergence, NOT coverage: FILE_REFERENCE_TYPES (#15041) ─────
575621
//
576622
// These five are in driver-sql's `JSON_COLUMN_TYPES` (it spreads the class by

packages/cli/src/commands/generate.ts

Lines changed: 51 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1014,7 +1014,23 @@ const FIELD_TYPE_SQL_MAP: Record<string, string | null> = {
10141014
percent: 'DECIMAL(5,2)',
10151015
boolean: 'BOOLEAN',
10161016
date: 'DATE',
1017-
datetime: 'TIMESTAMP',
1017+
// #15521 — TIMESTAMPTZ, not TIMESTAMP, for the same reason and with the same
1018+
// measured consequence as the audit-stamp columns in `generateMigrationSql`
1019+
// below: bare `TIMESTAMP` is `timestamp WITHOUT time zone`, while the driver
1020+
// creates a declared `Field.datetime` as `table.timestamp(name)` = knex's
1021+
// `timestamptz`. `createColumn`'s `datetime` arm states that as a decision,
1022+
// not an accident — "Postgres deliberately keeps `table.timestamp` →
1023+
// `timestamptz`: asking for precision 3 there would REDUCE it from
1024+
// microseconds" — and this file's OWN typescript generator already emitted
1025+
// `table.timestamp` for it, so the SQL format was one producer of three
1026+
// disagreeing with the other two. Measured on live PostgreSQL 16.13, a
1027+
// `datetime` field: driver `timestamp with time zone`, ts format `timestamp
1028+
// with time zone`, this map `timestamp without time zone`.
1029+
//
1030+
// The whole temporal class was enumerated in that same run, and it is the only
1031+
// member that diverged: `date` is DATE and `time` is TIME on all three
1032+
// producers, so neither moves.
1033+
datetime: 'TIMESTAMPTZ',
10181034
time: 'TIME',
10191035
email: 'VARCHAR(255)',
10201036
phone: 'VARCHAR(50)',
@@ -1200,8 +1216,40 @@ export function generateMigrationSql(config: Record<string, unknown>): string {
12001216
fieldLines.push(` "${fieldName}" ${sqlType}${notNull}`);
12011217
}
12021218

1203-
fieldLines.push(' "created_at" TIMESTAMP NOT NULL DEFAULT now()');
1204-
fieldLines.push(' "updated_at" TIMESTAMP NOT NULL DEFAULT now()');
1219+
// #15521 — TIMESTAMPTZ, not TIMESTAMP. Bare `TIMESTAMP` is `timestamp
1220+
// WITHOUT time zone`; both knex producers of these same two columns —
1221+
// `driver-sql`'s `createAuditTimestampColumn` and `generateMigrationTs`
1222+
// below — yield `timestamptz`. Driven rather than compiled: all three were
1223+
// run against a live PostgreSQL 16.13 and the columns read back out of
1224+
// `information_schema.columns`, where this literal was the only one that
1225+
// came back zone-naive.
1226+
//
1227+
// driver created_at timestamp with time zone null=YES default=CURRENT_TIMESTAMP
1228+
// ts gen created_at timestamp with time zone null=NO default=CURRENT_TIMESTAMP
1229+
// sql gen created_at timestamp without time zone null=NO default=now() <- this line
1230+
//
1231+
// Not a cosmetic type nit. A zone-naive column stores the wall clock of
1232+
// whatever session wrote the row and keeps nothing to recover the offset
1233+
// from, and `DEFAULT now()` is folded into that session's `TimeZone` on the
1234+
// way in. Two defaulted rows inserted SIX MILLISECONDS apart, one under
1235+
// `TimeZone='UTC'` and one under `Asia/Tokyo`, were recorded NINE HOURS
1236+
// apart in the generated table and 3 ms apart in the driver's own:
1237+
//
1238+
// sqlgen (timestamp) a_utc 2026-09-05 22:31:28.309421
1239+
// sqlgen (timestamp) b_tokyo 2026-09-06 07:31:28.315458 <- +9h, same instant
1240+
// tsgen (timestamptz) a_utc 2026-09-05 22:31:28.31332+00
1241+
// tsgen (timestamptz) b_tokyo 2026-09-05 22:31:28.316401+00
1242+
//
1243+
// ⚠️ The NOT NULL half of #15521 is deliberately NOT touched here. The
1244+
// driver leaves both columns nullable and both generators say NOT NULL;
1245+
// which side moves is a ruling that card holds open, and it is a different
1246+
// shape of question — nothing fails either way. `DEFAULT now()` is likewise
1247+
// left alone: it is the same instant as the driver's `CURRENT_TIMESTAMP`
1248+
// (both are `transaction_timestamp()`), it only reads differently in the
1249+
// catalog. `generate-builtin-id-column.pin.test.ts` records both of those,
1250+
// still unresolved, beside this half now resolved.
1251+
fieldLines.push(' "created_at" TIMESTAMPTZ NOT NULL DEFAULT now()');
1252+
fieldLines.push(' "updated_at" TIMESTAMPTZ NOT NULL DEFAULT now()');
12051253
lines.push(fieldLines.join(',\n'));
12061254
lines.push(');');
12071255
lines.push('');

0 commit comments

Comments
 (0)